Re: [PATCH v2 1/3] dt-bindings: clock: rockchip: Add RK3572 CRU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: reset: rockchip: skipped reset ID 421 in rk3572-cru.h
- [Low] dt-bindings: clock: rockchip: CLK_NR_CLKS and ID gap in rk3572-cru.h
- [Low] dt-bindings: clock: rockchip: schema and example issues in rk3572-cru.yaml
--

commit 23f6ead5b7b62aaf8bea62aacf6220f09c8b789f
Author: Elaine Zhang <zhangqing@rock-chips.com>

dt-bindings: clock: rockchip: Add RK3572 CRU

This patch adds the Devicetree binding schema and the clock and reset ID
header files for the Rockchip RK3572 Clock and Reset Unit (CRU).

>     dt-bindings: clock: rockchip: Add RK3572 CRU
>     
>     The RK3572 CRU uses the PMU0 GRF for the AUPLL reference mux.
>     Describe the controller and define its clock and reset IDs.
>     
>     Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>     
>     - Documented the xin24m input.
>     - Made the PMU0 GRF phandle required.
>     - Updated the example.
>     
>     Signed-off-by: Cunhao Lu <1579567540@qq.com>
>
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3572-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3572-cru.yaml
> new file mode 100644
> index 0000000000000..89e360ccb8846
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3572-cru.yaml
> @@ -0,0 +1,57 @@

[ ... ]

> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xin24m
> +
> +  rockchip,grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: PMU0 GRF register block.
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#clock-cells"
> +  - "#reset-cells"
> +  - rockchip,grf
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clock-controller@26090000 {
> +      compatible = "rockchip,rk3572-cru";
> +      reg = <0x26090000 0x60000>;
> +      #clock-cells = <1>;
> +      #reset-cells = <1>;
> +      rockchip,grf = <&pmu0_grf>;
> +    };

[Severity: Low]
In Documentation/devicetree/bindings/clock/rockchip,rk3572-cru.yaml, the
example node body for clock-controller@26090000 uses 2-space indentation
instead of the 4-space indentation expected by dt-check-style in strict mode.

Also, since clock-names only has a single entry, should it use a direct
const: xin24m rather than a single-element items list, and should clocks and
clock-names be listed under required and included in the example (matching
the changelog notes left between the two Signed-off-by lines in the commit
message)?

> diff --git a/include/dt-bindings/clock/rockchip,rk3572-cru.h b/include/dt-bindings/clock/rockchip,rk3572-cru.h
> new file mode 100644
> index 0000000000000..9c586d9e9ae91
> --- /dev/null
> +++ b/include/dt-bindings/clock/rockchip,rk3572-cru.h
> @@ -0,0 +1,560 @@

[ ... ]

> +#define ACLK_CRYPTO_NS				525
> +#define CLK_PKA_CRYPTO_NS			526
> +#define PCLK_DDR_MON				527
> +
> +/* Secure clocks */
> +#define ACLK_CCI				600
> +#define SCLK_DDR				601

[ ... ]

> +#define PCLK_WDT_S				613
> +#define TCLK_WDT_S				614
> +#define HCLK_TRNG_S				615
> +
> +#define CLK_NR_CLKS				(HCLK_TRNG_S + 1)
> +#endif

[Severity: Low]
Should CLK_NR_CLKS be dropped from
include/dt-bindings/clock/rockchip,rk3572-cru.h, and should the secure clock
IDs starting at ACLK_CCI be contiguous with PCLK_DDR_MON (527)?

Exposing CLK_NR_CLKS in the Devicetree binding header couples the DT ABI to
the total number of clocks if new clocks are added later. Rockchip clock
drivers calculate the maximum clock ID dynamically via
rockchip_clk_find_max_clk_id(), and CLK_NR_CLKS was previously removed from
other Rockchip binding headers in commits 2dc66a5ab2c6 and fb234516c5a0.

In addition, jumping from PCLK_DDR_MON (527) to ACLK_CCI (600) leaves an
unused gap of 72 indices (528..599) in the clk_hw pointer table allocated by
rockchip_clk_init().

> diff --git a/include/dt-bindings/reset/rockchip,rk3572-cru.h b/include/dt-bindings/reset/rockchip,rk3572-cru.h
> new file mode 100644
> index 0000000000000..a789475326818
> --- /dev/null
> +++ b/include/dt-bindings/reset/rockchip,rk3572-cru.h
> @@ -0,0 +1,713 @@

[ ... ]

> +/* CRU-->SOFTRST_CON87 */
> +#define SRST_PRESETN_CENTER_BIU                  417
> +#define SRST_RESETN_TEST_JUDGE_24M_CENTER        418
> +#define SRST_RESETN_TEST_JUDGE_CENTER            419
> +#define SRST_RESETN_LINKSYM_HDMITXPHY0           420
> +
> +/* PHPPHY_CRU-->SOFTRST_CON00 */
> +#define SRST_PRESETN_PHPPHY_CRU                  422
> +#define SRST_PRESETN_PCIE2_COMBOPHY0             423

[Severity: Medium]
Is reset ID 421 intentionally skipped between SRST_RESETN_LINKSYM_HDMITXPHY0
(420) and SRST_PRESETN_PHPPHY_CRU (422) in
include/dt-bindings/reset/rockchip,rk3572-cru.h?

All other reset IDs in this header are sequential 0-based integers. Leaving a
gap at 421 results in an uninitialized zero entry at index 421 in the driver's
lookup table, which rockchip_softrst_assert() and rockchip_softrst_deassert()
would silently treat as bank 0, bit 0 (SOFTRST_CON00 bit 0) if reset ID 421 is
ever referenced in Devicetree.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-add-board-ok3572-c-clk-v2-0-90b98836be6e@qq.com?part=1