Re: [PATCH 02/10] dt-bindings: clock: Add Amlogic A9 PLL clock controller

[Jian Hu <jian.hu@amlogic.com>]

Hi Krzysztof,

Thanks for your review.

On 5/15/2026 4:09 PM, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon, May 11, 2026 at 08:47:24PM +0800, Jian Hu wrote:
>> Add the PLL clock controller dt-bindings for the Amlogic A9 SoC family.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>>   .../bindings/clock/amlogic,a9-pll-clkc.yaml        | 110 +++++++++++++++++++++
>>   include/dt-bindings/clock/amlogic,a9-pll-clkc.h    |  55 +++++++++++
>>   2 files changed, 165 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a9-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a9-pll-clkc.yaml
>> new file mode 100644
>> index 000000000000..4ee6013ba1a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a9-pll-clkc.yaml
>> @@ -0,0 +1,110 @@
>> +# 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-pll-clkc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic A9 Series PLL Clock Controller
>> +
>> +maintainers:
>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>> +  - Jerome Brunet <jbrunet@baylibre.com>
>> +  - Jian Hu <jian.hu@amlogic.com>
>> +  - Xianwei Zhao <xianwei.zhao@amlogic.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - amlogic,a9-gp0-pll
>> +      - amlogic,a9-hifi0-pll
>> +      - amlogic,a9-hifi1-pll
>> +      - amlogic,a9-mclk0-pll
>> +      - amlogic,a9-mclk1-pll
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: pll input oscillator gate
>> +      - description: fixed input clock source for mclk_sel_0
>> +      - description: u3p2pll input clock source for mclk_sel_0 (optional)
> Second clock is also optional. Drop "(optional)" comment, just
> confusing.


GP0 has only one parent clock, while MCLK has three.

The second and third parent entries of GP0 are vacant,

so they need to be marked optional.

I will add the optional property for the second clock in the next revision.

>> +    minItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: in0
>> +      - const: in1
>> +      - const: in2
> Pretty pointless names, drop property.


Ok, I will drop them.

    clock-names:
-    items:
-      - const: in0
-      - const: in1
-      - const: in2
      minItems: 1

>> +    minItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - '#clock-cells'
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - amlogic,a9-mclk0-pll
>> +              - amlogic,a9-mclk1-pll
>> +
>> +    then:
>> +      properties:
>> +        clocks:
>> +          maxItems: 3
> No, minItems instead. maxItems is already 3, so what is the point of
> redefining it?


Ok, I will use minItems instead.

>> +
>> +        clock-names:
>> +          maxItems: 3
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - amlogic,a9-gp0-pll
>> +              - amlogic,a9-hifi0-pll
>> +              - amlogic,a9-hifi1-pll
>> +
>> +    then:
>> +      properties:
>> +        clocks:
>> +          maxItems: 1
>> +
>> +        clock-names:
>> +          maxItems: 1
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    apb4 {
> soc


Ok, I will rename it to soc.

>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        clock-controller@8200 {
>> +            compatible = "amlogic,a9-gp0-pll";
>> +            reg = <0x0 0x8200 0x0 0x20>;
>> +            #clock-cells = <1>;
>> +            clocks = <&scmi_clk 0>;
>> +            clock-names = "in0";
>> +        };
>> +
>> +        clock-controller@8330 {
>> +            compatible = "amlogic,a9-mclk0-pll";
>> +            reg = <0x0 0x8330 0x0 0x14>;
>> +            #clock-cells = <1>;
>> +            clocks = <&scmi_clk 4>,
>> +                     <&scmi_clk 8>;
>> +            clock-names = "in0", "in1";
> One example is enough, you have exactly the same properties.


Ok, I will drop the second clock node.

>
> Best regards,
> Krzysztof
>
Best regards,

Jian


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic