Re: [PATCH v1 1/1] arm64: dts: imx8qm-ss-dma: add LPUART4

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect devicetree clock indices map both 'ipg' and 'baud' clocks to the baud clock, leaving 'ipg' unenabled.
- [Low] The `dmas` property hardcodes the magic number `1` instead of using the `FSL_EDMA_RX` macro.
--

--- Patch [1]: [PATCH v1 1/1] arm64: dts: imx8qm-ss-dma: add LPUART4 ---
commit 7ee265f098cc61c07a1f39c116e402d04752161d
Author: Stefano Radaelli <[email protected]>

arm64: dts: imx8qm-ss-dma: add LPUART4

Add the missing LPUART4 node to the i.MX8QM DMA subsystem description.

> diff --git a/arch/arm64/boot/dts/freescale/imx8qm-ss-dma.dtsi b/arch/arm64/boot/dts/freescale/imx8qm-ss-dma.dtsi
> index 974e193f8dcb9..25c5e44231639 100644
> --- a/arch/arm64/boot/dts/freescale/imx8qm-ss-dma.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8qm-ss-dma.dtsi
> @@ -8,6 +8,22 @@
>  /delete-node/ &adma_pwm_lpcg;
>  
>  &dma_subsys {
> +	lpuart4: serial@5a0a0000 {
> +		compatible = "fsl,imx8qm-lpuart", "fsl,imx8qxp-lpuart";
> +		reg = <0x5a0a0000 0x1000>;
> +		interrupts = <GIC_SPI 349 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		clocks = <&uart4_lpcg 1>, <&uart4_lpcg 0>;

[Severity: High]
Does this clock configuration leave the IPG clock unenabled and map both clocks
to the baud clock?

The i.MX8QXP LPCG clock driver uses integer division to resolve the clock
index (idx = clkspec->args[0] / 4). Both 1 / 4 and 0 / 4 truncate to index 0,
which corresponds to the baud clock.

Because the IPG clock (which should be at index 16, or IMX_LPCG_CLK_4) is
never referenced, accessing the unclocked UART peripheral can cause a
synchronous external abort or bus hang.

Should this use IMX_LPCG_CLK_4 and IMX_LPCG_CLK_0 instead of the literal 1
and 0?

> +		clock-names = "ipg", "baud";
> +		assigned-clocks = <&clk IMX_SC_R_UART_4 IMX_SC_PM_CLK_PER>;
> +		assigned-clock-rates = <80000000>;
> +		power-domains = <&pd IMX_SC_R_UART_4>;
> +		dma-names = "rx","tx";
> +		dmas = <&edma2 20 0 1>,

[Severity: Low]
This isn't a bug, but does this hardcode the magic number 1 instead of using
the standard FSL_EDMA_RX macro?

> +		       <&edma2 21 0 0>;
> +		status = "disabled";
> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1