Re: [PATCH v8 12/12] ARM: dts: zte: Declare zx297520v3 CRM device nodes

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The lspcrm device defines a hardware reset dependency on matrixcrm (ZX297520V3_LSP_RESET), but the CRM drivers never request or deassert it, which can lead to a hardware hang when accessing the registers.
- [Low] Nodes under the `soc` bus are not sorted by their unit address.
- [Low] Non-generic node name `syscon-reboot`.
--

--- Patch [12]: [PATCH v8 12/12] ARM: dts: zte: Declare zx297520v3 CRM device nodes ---
commit ae42492b8ef6fce4731646f6907ba63dc94e9ffa
Author: Stefan Dösinger <[email protected]>

ARM: dts: zte: Declare zx297520v3 CRM device nodes

This commit wires up the zx297520v3 CRM device nodes, clock inputs,
and resets, getting rid of the placeholder uartclk node.

> diff --git a/arch/arm/boot/dts/zte/zx297520v3.dtsi b/arch/arm/boot/dts/zte/zx297520v3.dtsi
> --- a/arch/arm/boot/dts/zte/zx297520v3.dtsi
> +++ b/arch/arm/boot/dts/zte/zx297520v3.dtsi

[ ... ]

> @@ -70,13 +75,74 @@ gic: interrupt-controller@f2000000 {
>  			      <0xf2040000 0x20000>;
>  		};
>  
> +		topcrm: clock-controller@13b000 {
> +			compatible = "zte,zx297520v3-topcrm", "syscon";
> +			reg = <0x0013b000 0x400>;

[Severity: Low]
Are these new nodes inserted out of order relative to their unit addresses?

The topcrm node at 13b000 is placed before matrixcrm at 1306000, and lspcrm
at 1400000 is placed before uart0 at 131000. It is customary to sort nodes
under a bus by their unit address to maintain readability.

[ ... ]

> +			#clock-cells = <1>;
> +			#phy-cells = <1>;
> +			#reset-cells = <1>;
> +
> +			syscon-reboot {
> +				compatible = "syscon-reboot";
> +				offset = <0x0>;
> +				value = <0x1>;
> +				mask = <0x1>;
> +			};

[Severity: Low]
Should this node use a generic name like reboot instead of syscon-reboot?

The Devicetree specification usually recommends node names to reflect the
function of the device rather than its programming model or driver name.

> +		};
> +
> +		matrixcrm: clock-controller@1306000 {
> +			compatible = "zte,zx297520v3-matrixcrm";
> +			reg = <0x01306000 0x400>;

[ ... ]

> +		lspcrm: clock-controller@1400000 {
> +			compatible = "zte,zx297520v3-lspcrm";
> +			reg = <0x01400000 0x100>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;

[ ... ]

> +			clock-names = "mpll-d5", "mpll-d4", "mpll-d6", "mpll-d8", "mpll-d12",
> +				      "osc26m", "osc32k", "pclk", "tdm-wclk", "dpll-d4";
> +			resets = <&matrixcrm ZX297520V3_LSP_RESET>;
> +		};

[Severity: High]
Does this hardware reset dependency require explicit handling in the driver?

The DTS correctly defines the dependency, but looking at zx_clk_common_probe()
in drivers/clk/zte/clk-zx.c, the regmap is obtained using:

drivers/clk/zte/clk-zx.c:zx_clk_common_probe() {
    ...
    map = device_node_to_regmap(of_node);
    if (IS_ERR(map))
        return PTR_ERR(map);
    ...
}

Because the lspcrm node lacks the syscon compatible string, this bypasses the
generic syscon reset handling. As neither the MFD driver nor child drivers
manually deassert this reset, won't subsequent register accesses hit a reset
block and potentially hang the hardware?

> +
>  		uart0: serial@131000 {

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