Re: [PATCH v3 2/4] dt-bindings: clock: sun55i-a523-ccu: Add A523 CPU CCU clock controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/10/2026 00:50, Andre Przywara wrote:
> On Tue, 29 Sep 2026 10:42:29 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> Hi Krzysztof,
> 
> thanks for the review!
> 
>> On Sat, Sep 26, 2026 at 12:51:10AM +0200, Andre Przywara wrote:
>>> There are four clock controllers in the A523 SoC, but only three are
>>> described in the DT binding so far.
>>>
>>> Add a description for the CPU CCU, which provides separate clocks for
>>> the two CPU clusters and the DSU interconnect.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  .../clock/allwinner,sun55i-a523-ccu.yaml      | 38 ++++++++++++++++++-
>>>  .../dt-bindings/clock/sun55i-a523-cpu-ccu.h   | 13 +++++++
>>>  2 files changed, 50 insertions(+), 1 deletion(-)
>>>  create mode 100644 include/dt-bindings/clock/sun55i-a523-cpu-ccu.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/allwinner,sun55i-a523-ccu.yaml b/Documentation/devicetree/bindings/clock/allwinner,sun55i-a523-ccu.yaml
>>> index 58be701a720ea..8d5481fb63ae8 100644
>>> --- a/Documentation/devicetree/bindings/clock/allwinner,sun55i-a523-ccu.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/allwinner,sun55i-a523-ccu.yaml
>>> @@ -19,6 +19,7 @@ properties:
>>>    compatible:
>>>      enum:
>>>        - allwinner,sun55i-a523-ccu
>>> +      - allwinner,sun55i-a523-cpu-ccu
>>>        - allwinner,sun55i-a523-mcu-ccu
>>>        - allwinner,sun55i-a523-r-ccu
>>>  
>>> @@ -35,12 +36,23 @@ properties:
>>>  
>>>  required:
>>>    - "#clock-cells"
>>> -  - "#reset-cells"
>>>    - compatible
>>>    - reg
>>>    - clocks
>>>    - clock-names
>>>  
>>> +if:
>>
>> This goes to the allOf block.
> 
> OK.
> 
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        const: allwinner,sun55i-a523-cpu-ccu
>>> +then:
>>> +  properties:
>>> +    "#reset-cells": false
>>> +else:
>>> +  required:
>>> +    - "#reset-cells"
>>> +
>>>  allOf:
>>>    - if:
>>>        properties:
>>> @@ -64,6 +76,30 @@ allOf:
>>>              - const: iosc
>>>              - const: losc-fanout
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          enum:
>>> +            - allwinner,sun55i-a523-cpu-ccu
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: High Frequency Oscillator (usually at 24MHz)
>>> +            - description: Low Frequency Oscillator (usually at 32kHz)
>>> +            - description: Internal Oscillator
>>> +            - description: Peripherals PLL 0 (1200 MHz output)
>>> +            - description: Peripherals PLL 0 (600 MHz output)
>>> +
>>> +        clock-names:
>>> +          items:
>>> +            - const: hosc
>>> +            - const: losc
>>> +            - const: iosc
>>> +            - const: pll-periph0-2x
>>> +            - const: pll-periph0-600m
>>> +
>>>    - if:
>>>        properties:
>>>          compatible:
>>> diff --git a/include/dt-bindings/clock/sun55i-a523-cpu-ccu.h b/include/dt-bindings/clock/sun55i-a523-cpu-ccu.h
>>> new file mode 100644
>>> index 0000000000000..042f2310f64de
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/sun55i-a523-cpu-ccu.h
>>
>> Filename should match compatible.
> 
> Is that a new requirement? We never did this for the Allwinner clock or
> reset bindings, all headers are named like sun<x>i-<soc>-<name>-ccu.h.

I say EXACTLY the same thing for ~4 years, so yes. New requirement since
4 years. Allwinner for sure received same feedback already.

> And I think especially since we have sun55i-a523-mcu-ccu.h and
> sun55i-a523-r-ccu.h, I think we should stay consistent, at least for
> the existing SoCs.
> 
>>> @@ -0,0 +1,13 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>>> +/*
>>> + * Copyright 2025 Arm Ltd.
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_CLK_SUN55I_A523_CPU_CCU_H_
>>> +#define _DT_BINDINGS_CLK_SUN55I_A523_CPU_CCU_H_
>>> +
>>> +#define CLK_CPU_L		7
>>
>> Indices start from 0 or 1
> 
> This is also something very common for Allwinner clocks: the CCU has
> clocks that are not exposed to the DT, because they are internal
> clocks we need to model the clock tree. We number them logically,

They are not ABI thus do not create impression they should be and you
prepare some placeholder for them.

These are not IDs of your clocks in the hardware. This is only the ABI.

Again, same feedback I repeat for 4 years.

Best regards,
Krzysztof