Re: [PATCH v2 01/14] dt-bindings: clock: Add RaspberryPi RP1 clock bindings

[Andrea della Porta <andrea.porta@xxxxxxxx>]

Hi Krzysztof,

On 08:31 Tue 08 Oct     , Krzysztof Kozlowski wrote:
> On Mon, Oct 07, 2024 at 02:39:44PM +0200, Andrea della Porta wrote:
> > Add device tree bindings for the clock generator found in RP1 multi
> > function device, and relative entries in MAINTAINERS file.
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@xxxxxxxx>
> > ---
> >  .../clock/raspberrypi,rp1-clocks.yaml         | 62 +++++++++++++++++++
> >  MAINTAINERS                                   |  6 ++
> >  .../clock/raspberrypi,rp1-clocks.h            | 61 ++++++++++++++++++
> >  3 files changed, 129 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> >  create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> > new file mode 100644
> > index 000000000000..5e2e98051bf3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RaspberryPi RP1 clock generator
> > +
> > +maintainers:
> > +  - Andrea della Porta <andrea.porta@xxxxxxxx>
> > +
> > +description: |
> > +  The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO,
> > +  VIDEO), and each PLL output can be programmed though dividers to generate
> > +  the clocks to drive the sub-peripherals embedded inside the chipset.
> > +
> > +  Link to datasheet:
> > +  https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: raspberrypi,rp1-clocks
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#clock-cells':
> > +    description:
> > +      The index in the assigned-clocks is mapped to the output clock as per
> 
> How assigned-clocks is related to this? Drop.

This node provides clock for several peripherals, and for minimum functionality
at least 3 clocks have to be setup through assigned-clocks (and
assigned-clock-rates). That should be done in this same node (the provider of the
clocks) because those clocks are shared among peripherals or clock generators,
so cannot be described from consumers or we could incur in multiple declaration of
the same clock. I dropped the assigned-clocks and assigned-clock-rates from the
example section because Conor commented (see the first patchset version) that
according to him those properties were not relevant there, maybe I failed to produce
a careful explanation about why they are important. What should be the right course
of action, then? Just drop that description or leave it as it is (maybe augmenting
it with what I've explained here) and add again the dropped properties in the
example? I would be inclined to vote for the latter, but I'm not sure... 

> 
> > +      definitions in dt-bindings/clock/raspberrypi,rp1-clocks.h.
> 
> Use full paths, so they can be validated. This applies to all your
> patches.

Ack.

> 
> > +    const: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: rp1-xosc
> 
> Drop clock-names, redundant. Or just "xosc". Hyphens are not recommended
> character and rp1 is redundant.

Ack.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/raspberrypi,rp1-clocks.h>
> > +
> > +    rp1 {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        rp1_clocks: clocks@c040018000 {
> 
> Drop unused label.

Ack.

> 
> > +            compatible = "raspberrypi,rp1-clocks";
> > +            reg = <0xc0 0x40018000 0x0 0x10038>;
> > +            #clock-cells = <1>;
> > +            clocks = <&clk_rp1_xosc>;
> > +            clock-names =  "rp1-xosc";
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c27f3190737f..75a66e3e34c9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19380,6 +19380,12 @@ F:	Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> >  F:	drivers/media/platform/raspberrypi/pisp_be/
> >  F:	include/uapi/linux/media/raspberrypi/
> >  
> > +RASPBERRY PI RP1 PCI DRIVER
> > +M:	Andrea della Porta <andrea.porta@xxxxxxxx>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> > +F:	include/dt-bindings/clock/rp1.h
> > +
> >  RC-CORE / LIRC FRAMEWORK
> >  M:	Sean Young <sean@xxxxxxxx>
> >  L:	linux-media@xxxxxxxxxxxxxxx
> > diff --git a/include/dt-bindings/clock/raspberrypi,rp1-clocks.h b/include/dt-bindings/clock/raspberrypi,rp1-clocks.h
> > new file mode 100644
> > index 000000000000..b7c1eaa74eae
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/raspberrypi,rp1-clocks.h
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> 
> Any reason for different license?

Not really, I'll revert it back to the usual (GPL-2.0-only OR BSD-2-Clause), as
the other schemas.

Many thanks,

Andrea

> 
> > +/*
> > + * Copyright (C) 2021 Raspberry Pi Ltd.
> > + */
> 
> Best regards,
> Krzysztof
>