Re: [PATCH 03/18] dt-bindings: clock: add Zhihe A210 clock controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: clock: zhihe,a210-clk: missing subsystem controller input clocks
- [Medium] dt-bindings: clock: zhihe,a210-clk: disjoint register blocks merged in one node
- [Medium] dt-bindings: clock: zhihe,a210-clk: non-clock macros and global IDs in header
--

commit f418ceaa00f70a29b9cfb989d2292f9ccba767ae
Author: Zhiguo Zhu <zhiguo.zhu@linux.alibaba.com>

dt-bindings: clock: add Zhihe A210 clock controllers

This patch adds the devicetree binding schema and clock ID header for the
Zhihe A210 SoC clock controllers.

> diff --git a/Documentation/devicetree/bindings/clock/zhihe,a210-clk.yaml b/Documentation/devicetree/bindings/clock/zhihe,a210-clk.yaml
> new file mode 100644
> index 0000000000000..a3546d2f5977c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/zhihe,a210-clk.yaml
> @@ -0,0 +1,231 @@

[ ... ]

> +  clocks:
> +    description: Input reference oscillators.
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 3
> +    items:
> +      enum:
> +        - osc-32k
> +        - osc-24m
> +        - rc-24m

[Severity: Medium]
In the A210 SoC clock hierarchy, zhihe,a210-clk (managing pll-wrap and
top-crg) and zhihe,a210-aon-clk generate the root PLLs, dividers, muxes, and
subsystem clock gates (such as top_cfg_aclk, uart_sclk, video_pll_foutvco,
gmac_pll_foutpostdiv, dpu0_pll_foutpostdiv, aon_osc_clk_logic, aon_rtc_clk,
npu_aclk, npu_cclk, vp_aclk, and tee_clk), which then feed the 8 subsystem
clock controllers.

In Documentation/devicetree/bindings/clock/zhihe,a210-clk.yaml, the
top-level properties:clocks and properties:clock-names definitions only allow
up to 3 external reference oscillators (osc-32k, osc-24m, rc-24m) with
maxItems: 3, and in allOf only the zhihe,a210-clk branch requires clocks and
clock-names.

Any devicetree node using one of the 8 subsystem clock controller compatible
strings (zhihe,a210-peri-clk, zhihe,a210-gpu-clk, zhihe,a210-pcie-clk,
zhihe,a210-usb-clk, zhihe,a210-vi-clk, zhihe,a210-vp-clk, zhihe,a210-vo-clk,
or zhihe,a210-npu-clk) that specifies its parent clocks from zhihe,a210-clk
or zhihe,a210-aon-clk will fail dtbs_check schema validation, while omitting
clocks leaves the hardware clock tree topology undocumented in devicetree and
forces the driver to rely on hardcoded global clock names.

How can the 8 subsystem clock controller nodes declare their upstream input
clocks in devicetree if clocks and clock-names only permit the 3 external
reference oscillators, and should each subsystem compatible in allOf specify
and require its input clocks?

[ ... ]

> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: zhihe,a210-clk
> +    then:
> +      properties:
> +        reg:
> +          minItems: 9
> +        reg-names:
> +          items:
> +            - const: pll-wrap
> +            - const: top-crg
> +            - const: cpu-ss-clk-sysreg
> +            - const: cpu-ss-cpu-pll
> +            - const: ddr0-sysreg
> +            - const: ddr1-sysreg
> +            - const: slc-dual-sysreg
> +            - const: top-crg-t
> +            - const: cpu-ss-ccu
> +        clocks:
> +          minItems: 3
> +        clock-names:
> +          items:
> +            - const: osc-32k
> +            - const: osc-24m
> +            - const: rc-24m
> +      required:
> +        - clocks
> +        - clock-names
> +
> +  - if:
> +      properties:
> +        compatible:
> +          const: zhihe,a210-peri-clk
> +    then:
> +      properties:
> +        reg:
> +          minItems: 5
> +          maxItems: 5
> +        reg-names:
> +          items:
> +            - const: peri0-sysreg
> +            - const: peri1-sysreg
> +            - const: peri2-sysreg
> +            - const: peri3-sysreg
> +            - const: tee-crg

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/clock/zhihe,a210-clk.h>
> +
> +    clock-controller@260000 {
> +        compatible = "zhihe,a210-clk";
> +        reg = <0x00260000 0x1000>,
> +              <0x00250000 0x1000>,
> +              <0x10141600 0x100>,
> +              <0x10141400 0x100>,
> +              <0x04810000 0x1000>,
> +              <0x05810000 0x1000>,
> +              <0x04900000 0x1000>,
> +              <0x20250000 0x1000>,
> +              <0x10140000 0xe00>;
> +        reg-names = "pll-wrap", "top-crg", "cpu-ss-clk-sysreg", "cpu-ss-cpu-pll",
> +                    "ddr0-sysreg", "ddr1-sysreg", "slc-dual-sysreg", "top-crg-t",
> +                    "cpu-ss-ccu";
> +        #clock-cells = <1>;
> +        clocks = <&osc_32k>, <&osc_24m>, <&rc_24m>;
> +        clock-names = "osc-32k", "osc-24m", "rc-24m";
> +    };

[Severity: Medium]
In Documentation/devicetree/bindings/clock/zhihe,a210-clk.yaml, the
allOf:zhihe,a210-clk schema and the clock-controller@260000 example node
aggregate 9 disjoint hardware register blocks across unrelated SoC
subsystems into a single devicetree node: pll-wrap (0x00260000), top-crg
(0x00250000), cpu-ss-clk-sysreg (0x10141600), cpu-ss-cpu-pll (0x10141400),
ddr0-sysreg (0x04810000), ddr1-sysreg (0x05810000), slc-dual-sysreg
(0x04900000), top-crg-t (0x20250000), and cpu-ss-ccu (0x10140000).

Similarly, allOf:zhihe,a210-peri-clk aggregates 5 separate hardware blocks
(peri0-sysreg, peri1-sysreg, peri2-sysreg, peri3-sysreg, and tee-crg) at
disjoint address ranges into a single devicetree node, even though those 5
subsystems are independent hardware blocks (and are modeled as 5 separate
nodes zhihe,a210-peri0-reset through zhihe,a210-peri3-reset and
zhihe,a210-tee-reset in the reset binding). Merging these blocks into single
virtual nodes also claims entire 0x1000 *-sysreg ranges that contain
non-clock control registers.

Should these separate hardware register blocks be modeled as individual
devicetree nodes matching the physical SoC layout instead of grouping
disjoint register ranges into zhihe,a210-clk and zhihe,a210-peri-clk?

> diff --git a/include/dt-bindings/clock/zhihe,a210-clk.h b/include/dt-bindings/clock/zhihe,a210-clk.h
> new file mode 100644
> index 0000000000000..aa59d91a9ad05
> --- /dev/null
> +++ b/include/dt-bindings/clock/zhihe,a210-clk.h
> @@ -0,0 +1,649 @@

[ ... ]

> +#define VO_PLL2_DIV_EN                                     347
> +#define VO_PLL2_DIV_NUM                                    348
> +#define VO_PLL1_DIV_EN                                     349
> +#define VO_PLL1_DIV_NUM                                    350
> +#define VO_PLL0_DIV_EN                                     351
> +#define VO_PLL0_DIV_NUM                                    352

[ ... ]

> +#define CLK_DUMMY                                          505
> +#define OSC_32K                                            506
> +#define OSC_24M                                            507
> +#define RC_24M                                             508

[ ... ]

> +#define CLK_END 590
> +
> +#endif

[Severity: Medium]
In include/dt-bindings/clock/zhihe,a210-clk.h, all 9 clock controller
compatibles share a single interleaved global 0..589 ID space rather than
per-controller 0-based clock IDs, which requires every controller instance in
drivers/clk/zhihe/clk-a210-ap.c to allocate a 590-entry clk_hw_onecell_data
array.

In the VI_SYSREG, VO_SYSREG, and VP_SYSREG sections, hardware divider enable
bits and divider ratio bitfields of the same composite divider clock are
exposed as separate clock IDs (for example, VI_PRE_ISP_CORE_CLK_DIV_EN and
VI_PRE_ISP_CORE_CLK_DIV_NUM, VO_PLL0_DIV_EN and VO_PLL0_DIV_NUM, and
VP_DECOMP_DIV_EN and VP_DECOMP_DIV_NUM). In drivers/clk/zhihe/clk-a210-ap.c,
only the *_DIV_NUM IDs are registered as composite dividers (using the
corresponding *_DIV_EN bit as the gate bit), leaving all 16 *_DIV_EN macros
as unused register-bitfield definitions in the devicetree ABI header.

Additionally, CLK_DUMMY is a software driver implementation artifact,
OSC_32K, OSC_24M, and RC_24M are external input reference clocks passed via
the clocks property rather than outputs of the clock controller, and CLK_END
is an internal array bound whose value changes whenever new clock IDs are
added, breaking devicetree binding ABI invariance.

Could CLK_END, CLK_DUMMY, OSC_32K, OSC_24M, RC_24M, and the unused *_DIV_EN
bitfield macros be dropped from include/dt-bindings/clock/zhihe,a210-clk.h,
and should each clock controller define its own 0-based clock IDs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-a210-init-v1-0-5e438f972d24@iscas.ac.cn?part=3