Re: [PATCH 01/13] dt-bindings: clock: Add Amlogic A9 standardized model clock control units

Chuan Liu
2026-04-08T14:36:53Z

Hi Krzysztof,
Thanks for review.

On 2/9/2026 9:14 PM, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
>
> On 09/02/2026 06:48, Chuan Liu via B4 Relay wrote:
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> Add dt-binding documentation for standardized model clock control units
>> in A9 SoC family.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>> .../bindings/clock/amlogic,a9-model-ccu.yaml | 435 +++++++++++++++++++++
>> 1 file changed, 435 insertions(+)
>
> Brief review, you still have to read basic guidelines to not repeat the
> basic mistakes.
>
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a9-model-ccu.yaml b/Documentation/devicetree/bindings/clock/amlogic,a9-model-ccu.yaml
>> new file mode 100644
>> index 000000000000..56c5cbe1b246
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a9-model-ccu.yaml
>> @@ -0,0 +1,435 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2026 Amlogic, Inc. All rights reserved
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/amlogic,a9-model-ccu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic A9 Family Standardized Model Clock Control Unit
>> +
>> +maintainers:
>> + - Chuan Liu <chuan.liu@amlogic.com>
>> +
>> +description:
>> + The clock tree within the A9 is composed of numerous instances of these
>> + standardized model CCU (Clock Control Units).
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - items:
>
> Drop
>
>> + - const: amlogic,a9-composite-ccu
>> + description: Supports clock source selection, frequency division, and
>> + clock gating.
>
> This is just one big enum. You can add comments if you insist...

Thanks for pointing out. I'll refer to this approach for future comments
if necessary:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/samsung,exynos5433-clock.yaml?h=v7.0-rc7#n26

>
>
>> + - items:
>> + - const: amlogic,a9-composite-ccu-mult
>> + description: Some modules have multiple input clocks and contain
>> + multiple composite-ccus internally.
>> + - items:
>> + - const: amlogic,a9-noglitch-ccu
>> + description: Provides the same functionality as composite-ccu but
>> + includes glitch suppression during frequency transitions.
>> + - items:
>> + - const: amlogic,a9-noglitch-ccu-mult
>> + description: Some modules have multiple input clocks and contain
>> + multiple noglitch-ccus internally.
>> + - items:
>> + - const: amlogic,a9-sysbus-ccu
>> + description: Consists of multiple gating arrays, commonly used for
>> + Amlogic's sys_clk and axi_clk.
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + minItems: 1
>> + maxItems: 16
>> +
>> + clock-names:
>> + minItems: 1
>> + maxItems: 16
>> +
>> + clock-output-names:
>> + minItems: 1
>> + # The sysbus-ccu of A9 supports up to 128 gates
>> + maxItems: 128
>> +
>> + '#clock-cells':
>> + description:
>> + The clock controller of a module may contain one or more CCU(s). When a
>> + clock controller has multiple CCUs, an index is required to specify a
>> + particular CCU within the clock controller.
>
> Drop
>
>> + oneOf:
>> + - const: 0
>> + description: Single clock output, no specifier needed
>> + - const: 1
>> + description: Multiple clocks, index selects specific output
>
> Drop all descriptions. That's enum. Do not explain usu how DT works.
>
>> +
>> + amlogic,clock-max-frequency:
>
> Drop property. So many wrong things here... First, start from basic
> guidelines like talks or docs in kernel and understand the suffixes.
>
> Second not a DT property.
>

Thanks for pointing out. Martin also helped me explain the reasons in
detail. Thanks again.

>> + description: |
>> + Each clock's maximum output frequency is constrained during hardware
>> + design to ensure proper timing requirements for the clock network. If the
>> + clock frequency configured exceeds this design limit, it can lead to
>> + abnormal behavior in modules relying on that clock and may even cause
>> + cross-talk that affects other modules.
>> +
>> + In the driver, this property is parsed, and interface functions from the
>
> Why would we care about driver?
>
> This is binding, we talk about hardware.
>
>
>> + CCF are called to enforce the clock's maximum frequency, preventing
>> + potential issues caused by excessive clock frequency configurations.
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> +
>> + amlogic,reg-layout:
>
> No, drop. Compatible defines it.
>
>> + description:
>> + These standardized model CCUs require register configuration for their
>> + clock functions. This property node describes the register layout
>> + parameters for each model's CCU.
>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +
>> +allOf:

[...]

>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: amlogic,a9-noglitch-ccu-mult
>> + then:
>> + properties:
>> + clocks:
>> + minItems: 1
>
> Why?

noglitch-ccu processes the input clock and outputs it, so it needs to
have at least one input clock.

>
>> + items:
>> + - description: input clock source 0
>> + - description: input clock source 1 (optional)
>
> Drop optional. Don't repeat constraints in free form text.
>
>> + - description: input clock source 2 (optional)
>> + - description: input clock source 3 (optional)
>> + - description: input clock source 4 (optional)
>> + - description: input clock source 5 (optional)
>> + - description: input clock source 6 (optional)
>> + - description: input clock source 7 (optional)
>> + clock-names:
>> + minItems: 1
>> + items:
>> + - const: clkin0
>> + - const: clkin1
>> + - const: clkin2
>> + - const: clkin3
>> + - const: clkin4
>> + - const: clkin5
>> + - const: clkin6
>> + - const: clkin7
>> + amlogic,reg-layout:
>> + description: |
>> + composite-ccu contains one register layout parameters:
>> + * register offset
>> + required:
>> + - amlogic,reg-layout
>> + - clock-names
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: amlogic,a9-sysbus-ccu
>> + then:
>> + properties:
>> + clocks:
>> + maxItems: 1
>> + description: input clock of sysbus-ccu
>
> list the items with description instead.
>
>> + amlogic,reg-layout:
>> + description: |
>> + composite-ccu contains two register layout parameters:
>> + * register offset
>> + * bit offset
>> + required:
>> + - amlogic,reg-layout
>
> This is huge and amount of ifs is clearly suggesting you combined way
> too much into one file.
>

I'll try to split large blocks of if statements like this into separate
.yaml files. Thanks for your suggestion.

>
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-output-names
>> + - "#clock-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + clk_dummy: clock-dummy {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <0>;
>> + clock-output-names = "dummy";
>> + status = "disabled";
>> + };
>
> Not relevant, drop entire node.
>
>> +
>> + apb {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + clock-controller@350 {
>> + compatible = "amlogic,a9-composite-ccu";
>> + reg = <0x0 0x350 0x0 0x4>;
>> + #clock-cells = <0>;
>> + amlogic,clock-max-frequency = <50000000>;
>> + amlogic,reg-layout = <0x0 0 7>;
>> + clock-output-names = "sar_adc";
>> + clocks = <&xtal_24m>,
>> + <&scmi_clk 17>;
>> + clock-names = "clkin0", "clkin1";
>> + };
>> +
>> + clock-controller@290 {
>> + compatible = "amlogic,a9-composite-ccu-mult";
>> + reg = <0x0 0x290 0x0 0x8>;
>> + #clock-cells = <1>;
>> + amlogic,clock-max-frequency = <250000000>,
>> + <250000000>,
>> + <1200000000>;
>> + amlogic,reg-layout = <0x0 0 7>,
>> + <0x0 16 7>,
>> + <0x4 0 7>;
>> + clock-output-names = "sd_emmc_a",
>> + "sd_emmc_b",
>> + "sd_emmc_c";
>> + clocks = <&xtal_24m>,
>> + <&scmi_clk 6>,
>> + <&scmi_clk 10>;
>> + clock-names = "clkin0",
>> + "clkin1",
>> + "clkin2";
>> + };
>
> One or two are enough. Drop the rest.
>

I have no objections to the comments here or the earlier ones I haven't
replied to. To avoid taking up more of your time, I didn't reply one by
one here. Thanks again.

>>
>
>
> Best regards,
> Krzysztof

--
Best regards,
Chuan