Re: [PATCH 1/5] dt-bindings: clock: sun55i-a523-ccu: Add A523 CPU CCU clock controller

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 3 Sep 2025 10:08:33 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

Hi,

> On Wed, Sep 03, 2025 at 01:09:06AM +0100, 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      | 25 +++++++++++++++++++
> >  .../dt-bindings/clock/sun55i-a523-cpu-ccu.h   | 13 ++++++++++
> >  2 files changed, 38 insertions(+)
> >  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 1dbd92febc471..367d26800fd0d 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
> >  
> > @@ -64,6 +65,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 based on compatible.
> 
> 
> > @@ -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
> > +#define CLK_CPU_DSU		8
> > +#define CLK_CPU_B		9  
> 
> I don't see the header being used by the driver and odd numbers (they
> should start from 0 or 1) suggest these are not bindings.

This header is included by the private header (at the end of patch 4/5).
The private header is then included by the driver.
Happy to change that, but that's the pattern used in all the other drivers.

Those numbers represent the publicly exposed clocks, the other clocks are
internal. Having gaps in those numbers is somewhat common in sunxi-ng
(check sun50i-h616-ccu.h). This large gap at the beginning here is mostly
due to the somewhat extreme design of this CCU, which requires quite some
helper clocks to get to the actual ones.
We could sort the identifiers to have the public clocks first, but
that would only be the case until we discover a missed clock (which seems
to happen from times to times). And again, that's the pattern used in the
sibling drivers, so I'd rather stay consistent here.

Cheers,
Andre

> Otherwise please explain in commit msg what exactly are you binding
> here.
> 
> Best regards,
> Krzysztof
>