Re: [PATCH 01/13] dt-bindings: clock: Add Amlogic A9 standardized model clock control units
From: Chuan Liu
Date: Wed Apr 08 2026 - 10:39:28 EST
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@xxxxxxxxxxx>
Add dt-binding documentation for standardized model clock control units
in A9 SoC family.
Signed-off-by: Chuan Liu <chuan.liu@xxxxxxxxxxx>
---
.../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@xxxxxxxxxxx>
+
+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