[RFC,1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings

Commit Message

Biju Das Feb. 20, 2023, 1:13 p.m. UTC

Document Renesas versa3 clock generator(5P35023) bindings.

The 5P35023 is a VersaClock programmable clock generator and
is designed for low-power, consumer, and high-performance PCI
Express applications. The 5P35023 device is a three PLL
architecture design, and each PLL is individually programmable
and allowing for up to 6 unique frequency outputs.

Signed-off-by: Biju Das <[e-mail]>
---
 .../bindings/clock/renesas,versaclock3.yaml   | 135 ++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml

Comments

Krzysztof Kozlowski Feb. 22, 2023, 9:34 a.m. UTC | #1

On 20/02/2023 14:13, Biju Das wrote:
> Document Renesas versa3 clock generator(5P35023) bindings.
> 
> The 5P35023 is a VersaClock programmable clock generator and
> is designed for low-power, consumer, and high-performance PCI
> Express applications. The 5P35023 device is a three PLL
> architecture design, and each PLL is individually programmable
> and allowing for up to 6 unique frequency outputs.
> 
> Signed-off-by: Biju Das <[e-mail]>
> ---
>  .../bindings/clock/renesas,versaclock3.yaml   | 135 ++++++++++++++++++
>  1 file changed, 135 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> new file mode 100644
> index 000000000000..f45b8da73ec3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml

Filename usually is based on the compatible. Why these two are so different?


> @@ -0,0 +1,135 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,versaclock3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas VersaClock 3 programmable I2C clock generators
> +
> +description: |
> +  The 5P35023 is a VersaClock programmable clock generator and
> +  is designed for low-power, consumer, and high-performance PCI
> +  express applications. The 5P35023 device is a three PLL
> +  architecture design, and each PLL is individually programmable
> +  and allowing for up to 6 unique frequency outputs.
> +
> +  An internal OTP memory allows the user to store the configuration
> +  in the device. After power up, the user can change the device register
> +  settings through the I2C interface when I2C mode is selected.
> +
> +  The driver can read a full register map from the DT, and will use that
> +  register map to initialize the attached part (via I2C) when the system
> +  boots. Any configuration not supported by the common clock framework
> +  must be done via the full register map, including optimized settings.
> +
> +  Link to datasheet: https://www.renesas.com/us/en/products/clocks-timing/
> +                     clock-generation/programmable-clocks/
> +                     5p35023-versaclock-3s-programmable-clock-generator

I think link should not be wrapped. Start in next line and just make it
long.

While touching this, please keep the same order of entries as in
example-schema, so maintainers go after title.

> +
> +maintainers:
> +  - Biju Das <[e-mail]>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,5p35023
> +
> +  reg:
> +    description: I2C device address

Drop description, it's obvious.

> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  clock-names:
> +    oneOf:
> +      - items:
> +          - const: x1
> +      - items:
> +          - const: clkin

This should be specific, not one or another. Why do you have two
entirely different clock inputs?

(and if this stays, then just items: - enum: [])


> +
> +  clocks:
> +    maxItems: 1
> +
> +  renesas,settings:
> +    description: Optional, complete register map of the device.
> +      Optimized settings for the device must be provided in full
> +      and are written during initialization.
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    minItems: 37

maxItems instead... but I am not sure that we allow register settings in
DT in general.

> +
> +  assigned-clocks:
> +    minItems: 6

Drop.

> +
> +  assigned-clock-rates:
> +    minItems: 6

Drop.

> +
> +  renesas,clock-divider-read-only:
> +    description: Flag for setting divider in read only mode.

Flag? Then type: boolean.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5

This is broken...

> +
> +  renesas,clock-flags:
> +    description: Flags used in common clock frame work for configuring
> +      clk outputs. See include/linux/clk-provider.h

These are not bindings, so why do you non-bindings constants as
bindings? They can change anytime. Choose one:
1. Drop entire property,
2. Make it a proper binding property, so an ABI and explain why this is
DT specific. None of clock providers have to do it, so you need here
good explanation.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 6

maxItems instead

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* 24MHz crystal */
> +    x1_x2: xtal {
> +      compatible = "fixed-clock";
> +      #clock-cells = <0>;
> +      clock-frequency = <24000000>;
> +    };

Drop this part, obvious.

> +
> +    i2c@0 {
> +        reg = <0x0 0x100>;

Just i2c { and drop reg

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        versa3: clock-generator@68 {
> +            compatible = "renesas,5p35023";
> +            reg = <0x68>;
> +            #clock-cells = <1>;
> +
> +            clocks = <&x1_x2>;
> +            clock-names = "x1";
> +
> +            renesas,settings = [
> +                80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
> +                00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
> +                80 b0 45 c4 95
> +            ];
> +
> +            assigned-clocks = <&versa3 0>,
> +                              <&versa3 1>,
> +                              <&versa3 2>,
> +                              <&versa3 3>,
> +                              <&versa3 4>,
> +                              <&versa3 5>;
> +            assigned-clock-rates = <12288000>, <25000000>,
> +                                   <12000000>, <11289600>,
> +                                   <11289600>, <24000000>;
> +            renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
> +            renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
> +                                  <2176>, <2048>;
> +        };
> +    };
> +
> +    /* Consumer referencing the versa 3 */
> +    consumer {
> +        /* ... */
> +        clocks = <&versa3 3>;
> +        /* ... */

Drop consumer. Do you see it anywhere else?

Best regards,
Krzysztof

Biju Das March 8, 2023, 2:39 p.m. UTC | #2

Hi Krzysztof Kozlowski,

Thanks for the feedback.

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Wednesday, February 22, 2023 9:34 AM
> To: Biju Das <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
> [e-mail]; [e-mail]; [e-mail];
> Fabrizio Castro <[e-mail]>
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 20/02/2023 14:13, Biju Das wrote:
> > Document Renesas versa3 clock generator(5P35023) bindings.
> >
> > The 5P35023 is a VersaClock programmable clock generator and is
> > designed for low-power, consumer, and high-performance PCI Express
> > applications. The 5P35023 device is a three PLL architecture design,
> > and each PLL is individually programmable and allowing for up to 6
> > unique frequency outputs.
> >
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> >  .../bindings/clock/renesas,versaclock3.yaml   | 135 ++++++++++++++++++
> >  1 file changed, 135 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> > b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> > new file mode 100644
> > index 000000000000..f45b8da73ec3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> 
> Filename usually is based on the compatible. Why these two are so different?

Versa3 clock generator has the following variants.

	5P35023, 5L35021, 5L35023 and 5P35021

RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
And added compatible for specific one.

> 
> 
> > +title: Renesas VersaClock 3 programmable I2C clock generators
> > +
> > +description: |
> > +  The 5P35023 is a VersaClock programmable clock generator and
> > +  is designed for low-power, consumer, and high-performance PCI
> > +  express applications. The 5P35023 device is a three PLL
> > +  architecture design, and each PLL is individually programmable
> > +  and allowing for up to 6 unique frequency outputs.
> > +
> > +  An internal OTP memory allows the user to store the configuration
> > + in the device. After power up, the user can change the device
> > + register  settings through the I2C interface when I2C mode is selected.
> > +
> > +  The driver can read a full register map from the DT, and will use
> > + that  register map to initialize the attached part (via I2C) when
> > + the system  boots. Any configuration not supported by the common
> > + clock framework  must be done via the full register map, including
> optimized settings.
> > +
> > +  Link to datasheet: https://www.renesas.com/us/en/products/clocks-
> timing/
> > +                     clock-generation/programmable-clocks/
> > +
> > + 5p35023-versaclock-3s-programmable-clock-generator
> 
> I think link should not be wrapped. Start in next line and just make it
> long.

OK.

> 
> While touching this, please keep the same order of entries as in example-
> schema, so maintainers go after title.

Agreed.

> 
> > +
> > +maintainers:
> > +  - Biju Das <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,5p35023
> > +
> > +  reg:
> > +    description: I2C device address
> 
> Drop description, it's obvious.
OK.

> 
> > +    maxItems: 1
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  clock-names:
> > +    oneOf:
> > +      - items:
> > +          - const: x1
> > +      - items:
> > +          - const: clkin
> 
> This should be specific, not one or another. Why do you have two entirely
> different clock inputs?

Reference input can be Crystal oscillator interface input(x1) or differential
clock input pin(clkin)

> 
> (and if this stays, then just items: - enum: [])


OK, I will use enum.
> 
> 
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  renesas,settings:
> > +    description: Optional, complete register map of the device.
> > +      Optimized settings for the device must be provided in full
> > +      and are written during initialization.
> > +    $ref: /schemas/types.yaml#/definitions/uint8-array
> > +    minItems: 37
> 
> maxItems instead... but I am not sure that we allow register settings in DT
> in general.

Agreed. I guess it is allowed [1]
[1] https://lore.kernel.org/linux-renesas-soc/833d[e-mail]/

> 
> > +
> > +  assigned-clocks:
> > +    minItems: 6
> 
> Drop.

OK.

> 
> > +
> > +  assigned-clock-rates:
> > +    minItems: 6
> 
> Drop.

OK.

> 
> > +
> > +  renesas,clock-divider-read-only:
> > +    description: Flag for setting divider in read only mode.
> 
> Flag? Then type: boolean.

Agreed.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 5
> 
> This is broken...
OK you mean maxItems. Based on Boolean type I will update this
> 
> > +
> > +  renesas,clock-flags:
> > +    description: Flags used in common clock frame work for configuring
> > +      clk outputs. See include/linux/clk-provider.h
> 
> These are not bindings, so why do you non-bindings constants as bindings?
> They can change anytime. Choose one:
> 1. Drop entire property,
> 2. Make it a proper binding property, so an ABI and explain why this is DT
> specific. None of clock providers have to do it, so you need here good
> explanation.

I will choose 2 and will explain as user should be allowed to
configure the output clock from clk generator, so that it has flexibility
for
1) changing the rates (propagate rate change up one level)
2) fixed always 
3) don't gate the ouput clk at all.

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 6
> 
> maxItems instead

OK.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    /* 24MHz crystal */
> > +    x1_x2: xtal {
> > +      compatible = "fixed-clock";
> > +      #clock-cells = <0>;
> > +      clock-frequency = <24000000>;
> > +    };
> 
> Drop this part, obvious.
OK.
> 
> > +
> > +    i2c@0 {
> > +        reg = <0x0 0x100>;
> 
> Just i2c { and drop reg

Agreed.
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        versa3: clock-generator@68 {
> > +            compatible = "renesas,5p35023";
> > +            reg = <0x68>;
> > +            #clock-cells = <1>;
> > +
> > +            clocks = <&x1_x2>;
> > +            clock-names = "x1";
> > +
> > +            renesas,settings = [
> > +                80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
> > +                00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
> > +                80 b0 45 c4 95
> > +            ];
> > +
> > +            assigned-clocks = <&versa3 0>,
> > +                              <&versa3 1>,
> > +                              <&versa3 2>,
> > +                              <&versa3 3>,
> > +                              <&versa3 4>,
> > +                              <&versa3 5>;
> > +            assigned-clock-rates = <12288000>, <25000000>,
> > +                                   <12000000>, <11289600>,
> > +                                   <11289600>, <24000000>;
> > +            renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
> > +            renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
> > +                                  <2176>, <2048>;
> > +        };
> > +    };
> > +
> > +    /* Consumer referencing the versa 3 */
> > +    consumer {
> > +        /* ... */
> > +        clocks = <&versa3 3>;
> > +        /* ... */
> 
> Drop consumer. Do you see it anywhere else?

No, will drop.

Cheers,
Biju

Geert Uytterhoeven March 8, 2023, 2:50 p.m. UTC | #3

Hi Biju,

On Wed, Mar 8, 2023 at 3:39 PM Biju Das <[e-mail]> wrote:
> > From: Krzysztof Kozlowski <[e-mail]>
> > On 20/02/2023 14:13, Biju Das wrote:
> > > Document Renesas versa3 clock generator(5P35023) bindings.
> > >
> > > The 5P35023 is a VersaClock programmable clock generator and is
> > > designed for low-power, consumer, and high-performance PCI Express
> > > applications. The 5P35023 device is a three PLL architecture design,
> > > and each PLL is individually programmable and allowing for up to 6
> > > unique frequency outputs.
> > >
> > > Signed-off-by: Biju Das <[e-mail]>

> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml

> > > +  clock-names:
> > > +    oneOf:
> > > +      - items:
> > > +          - const: x1
> > > +      - items:
> > > +          - const: clkin
> >
> > This should be specific, not one or another. Why do you have two entirely
> > different clock inputs?
>
> Reference input can be Crystal oscillator interface input(x1) or differential
> clock input pin(clkin)

I believe that's purely a hardware feature, which does not need any
software configuration?
I.e. logically, there's just a single clock input, i.e. no need for clock-names.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Biju Das March 8, 2023, 2:57 p.m. UTC | #4

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> Hi Biju,
> 
> On Wed, Mar 8, 2023 at 3:39 PM Biju Das <[e-mail]> wrote:
> > > From: Krzysztof Kozlowski <[e-mail]> On
> > > 20/02/2023 14:13, Biju Das wrote:
> > > > Document Renesas versa3 clock generator(5P35023) bindings.
> > > >
> > > > The 5P35023 is a VersaClock programmable clock generator and is
> > > > designed for low-power, consumer, and high-performance PCI Express
> > > > applications. The 5P35023 device is a three PLL architecture
> > > > design, and each PLL is individually programmable and allowing for
> > > > up to 6 unique frequency outputs.
> > > >
> > > > Signed-off-by: Biju Das <[e-mail]>
> 
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.
> > > > +++ yaml
> 
> > > > +  clock-names:
> > > > +    oneOf:
> > > > +      - items:
> > > > +          - const: x1
> > > > +      - items:
> > > > +          - const: clkin
> > >
> > > This should be specific, not one or another. Why do you have two
> > > entirely different clock inputs?
> >
> > Reference input can be Crystal oscillator interface input(x1) or
> > differential clock input pin(clkin)
> 
> I believe that's purely a hardware feature, which does not need any software
> configuration?
> I.e. logically, there's just a single clock input, i.e. no need for clock-
> names.

OK. Agreed. Will remove clock-names.

Cheers,
Biju

Krzysztof Kozlowski March 8, 2023, 6:47 p.m. UTC | #5

On 08/03/2023 15:39, Biju Das wrote:

>>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
>>
>> Filename usually is based on the compatible. Why these two are so different?
> 
> Versa3 clock generator has the following variants.
> 
> 	5P35023, 5L35021, 5L35023 and 5P35021
> 
> RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
> And added compatible for specific one.

And what about other devices? Since you do not add them, just keep
compatible as filename.

>>
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  renesas,settings:
>>> +    description: Optional, complete register map of the device.
>>> +      Optimized settings for the device must be provided in full
>>> +      and are written during initialization.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    minItems: 37
>>
>> maxItems instead... but I am not sure that we allow register settings in DT
>> in general.
> 
> Agreed. I guess it is allowed [1]
> [1] https://lore.kernel.org/linux-renesas-soc/833d[e-mail]/

I don't see Rob's review on this, so what do you prove exactly?

> 
>>
>>> +
>>> +  assigned-clocks:
>>> +    minItems: 6
>>
>> Drop.
> 
> OK.
> 
>>
>>> +
>>> +  assigned-clock-rates:
>>> +    minItems: 6
>>
>> Drop.
> 
> OK.
> 
>>
>>> +
>>> +  renesas,clock-divider-read-only:
>>> +    description: Flag for setting divider in read only mode.
>>
>> Flag? Then type: boolean.
> 
> Agreed.
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    minItems: 5
>>
>> This is broken...
> OK you mean maxItems. Based on Boolean type I will update this

I mean, it does not match the description. Maybe I just don't understand
here something, but flag is boolean. Anyway, minItems means you can have
million of items, so was it intended?

>>
>>> +
>>> +  renesas,clock-flags:
>>> +    description: Flags used in common clock frame work for configuring
>>> +      clk outputs. See include/linux/clk-provider.h
>>
>> These are not bindings, so why do you non-bindings constants as bindings?
>> They can change anytime. Choose one:
>> 1. Drop entire property,
>> 2. Make it a proper binding property, so an ABI and explain why this is DT
>> specific. None of clock providers have to do it, so you need here good
>> explanation.
> 
> I will choose 2 and will explain as user should be allowed to
> configure the output clock from clk generator, so that it has flexibility
> for
> 1) changing the rates (propagate rate change up one level)
> 2) fixed always 
> 3) don't gate the ouput clk at all.

User's choice is task for user-space, so not a good explanation for DT.

Best regards,
Krzysztof

Biju Das March 8, 2023, 6:55 p.m. UTC | #6

Hi Krzysztof Kozlowski,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 08/03/2023 15:39, Biju Das wrote:
> 
> >>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.ya
> >>> +++ ml
> >>
> >> Filename usually is based on the compatible. Why these two are so
> different?
> >
> > Versa3 clock generator has the following variants.
> >
> > 	5P35023, 5L35021, 5L35023 and 5P35021
> >
> > RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
> > And added compatible for specific one.
> 
> And what about other devices? Since you do not add them, just keep
> compatible as filename.

OK.

> 
> >>
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +
> >>> +  renesas,settings:
> >>> +    description: Optional, complete register map of the device.
> >>> +      Optimized settings for the device must be provided in full
> >>> +      and are written during initialization.
> >>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> >>> +    minItems: 37
> >>
> >> maxItems instead... but I am not sure that we allow register settings
> >> in DT in general.
> >
> > Agreed. I guess it is allowed [1]
> > [1]
> 
> I don't see Rob's review on this, so what do you prove exactly?

Subject: [PATCH v9 1/2] dt-bindings: Add binding for Renesas 8T49N241
Date: Fri,  4 Feb 2022 10:57:38 +0100	[thread overview]
Message-ID: <833d[e-mail]> (raw)
In-Reply-To: <[e-mail]>

From: Alex Helms <[e-mail]>

Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
The 8T49N241 accepts up to two differential or single-ended input clocks
and a fundamental-mode crystal input. The internal PLL can lock to either
of the input reference clocks or to the crystal to behave as a frequency
synthesizer.

Signed-off-by: Alex Helms <[e-mail]>
Reviewed-by: Rob Herring <[e-mail]>
Reviewed-by: Geert Uytterhoeven <[e-mail]>
Signed-off-by: Michal Simek <[e-mail]>
---
> 
> >
> >>
> >>> +
> >>> +  assigned-clocks:
> >>> +    minItems: 6
> >>
> >> Drop.
> >
> > OK.
> >
> >>
> >>> +
> >>> +  assigned-clock-rates:
> >>> +    minItems: 6
> >>
> >> Drop.
> >
> > OK.
> >
> >>
> >>> +
> >>> +  renesas,clock-divider-read-only:
> >>> +    description: Flag for setting divider in read only mode.
> >>
> >> Flag? Then type: boolean.
> >
> > Agreed.
> >>
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> +    minItems: 5
> >>
> >> This is broken...
> > OK you mean maxItems. Based on Boolean type I will update this
> 
> I mean, it does not match the description. Maybe I just don't understand
> here something, but flag is boolean. Anyway, minItems means you can have
> million of items, so was it intended?

It is a mistake.

> 
> >>
> >>> +
> >>> +  renesas,clock-flags:
> >>> +    description: Flags used in common clock frame work for configuring
> >>> +      clk outputs. See include/linux/clk-provider.h
> >>
> >> These are not bindings, so why do you non-bindings constants as bindings?
> >> They can change anytime. Choose one:
> >> 1. Drop entire property,
> >> 2. Make it a proper binding property, so an ABI and explain why this
> >> is DT specific. None of clock providers have to do it, so you need
> >> here good explanation.
> >
> > I will choose 2 and will explain as user should be allowed to
> > configure the output clock from clk generator, so that it has
> > flexibility for
> > 1) changing the rates (propagate rate change up one level)
> > 2) fixed always
> > 3) don't gate the ouput clk at all.
> 
> User's choice is task for user-space, so not a good explanation for DT.

I don't think clock generator HW has any business with user space.

It is clk generator HW specific. Clk generator is vital component which
provides clocks to the system. We are providing some hardware feature which
is exposed as dt properties.

Like clock output is fixed rate clock or dynamic rate clock/

Cheers,
Biju

Krzysztof Kozlowski March 8, 2023, 7:17 p.m. UTC | #7

On 08/03/2023 19:55, Biju Das wrote:
> 
> Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
> The 8T49N241 accepts up to two differential or single-ended input clocks
> and a fundamental-mode crystal input. The internal PLL can lock to either
> of the input reference clocks or to the crystal to behave as a frequency
> synthesizer.
> 
> Signed-off-by: Alex Helms <[e-mail]>
> Reviewed-by: Rob Herring <[e-mail]>

Ah, indeed, fine then.

>>
>>>>
>>>>> +
>>>>> +  renesas,clock-flags:
>>>>> +    description: Flags used in common clock frame work for configuring
>>>>> +      clk outputs. See include/linux/clk-provider.h
>>>>
>>>> These are not bindings, so why do you non-bindings constants as bindings?
>>>> They can change anytime. Choose one:
>>>> 1. Drop entire property,
>>>> 2. Make it a proper binding property, so an ABI and explain why this
>>>> is DT specific. None of clock providers have to do it, so you need
>>>> here good explanation.
>>>
>>> I will choose 2 and will explain as user should be allowed to
>>> configure the output clock from clk generator, so that it has
>>> flexibility for
>>> 1) changing the rates (propagate rate change up one level)
>>> 2) fixed always
>>> 3) don't gate the ouput clk at all.
>>
>> User's choice is task for user-space, so not a good explanation for DT.
> 
> I don't think clock generator HW has any business with user space.
> 
> It is clk generator HW specific. Clk generator is vital component which
> provides clocks to the system. 

Every clock controller is vital...

> We are providing some hardware feature which
> is exposed as dt properties.
> 
> Like clock output is fixed rate clock or dynamic rate clock/

OK, I wait then for proper description which will explain and justify this.


Best regards,
Krzysztof

Biju Das March 9, 2023, 7:57 a.m. UTC | #8

Hi Krzysztof Kozlowski,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 08/03/2023 19:55, Biju Das wrote:
> >
> > Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
> > The 8T49N241 accepts up to two differential or single-ended input
> > clocks and a fundamental-mode crystal input. The internal PLL can lock
> > to either of the input reference clocks or to the crystal to behave as
> > a frequency synthesizer.
> >
> > Signed-off-by: Alex Helms <[e-mail]>
> > Reviewed-by: Rob Herring <[e-mail]>
> 
> Ah, indeed, fine then.
> 
> >>
> >>>>
> >>>>> +
> >>>>> +  renesas,clock-flags:
> >>>>> +    description: Flags used in common clock frame work for
> configuring
> >>>>> +      clk outputs. See include/linux/clk-provider.h
> >>>>
> >>>> These are not bindings, so why do you non-bindings constants as
> bindings?
> >>>> They can change anytime. Choose one:
> >>>> 1. Drop entire property,
> >>>> 2. Make it a proper binding property, so an ABI and explain why
> >>>> this is DT specific. None of clock providers have to do it, so you
> >>>> need here good explanation.
> >>>
> >>> I will choose 2 and will explain as user should be allowed to
> >>> configure the output clock from clk generator, so that it has
> >>> flexibility for
> >>> 1) changing the rates (propagate rate change up one level)
> >>> 2) fixed always
> >>> 3) don't gate the ouput clk at all.
> >>
> >> User's choice is task for user-space, so not a good explanation for DT.
> >
> > I don't think clock generator HW has any business with user space.
> >
> > It is clk generator HW specific. Clk generator is vital component
> > which provides clocks to the system.
> 
> Every clock controller is vital...
> 
> > We are providing some hardware feature which is exposed as dt
> > properties.
> >
> > Like clock output is fixed rate clock or dynamic rate clock/
> 
> OK, I wait then for proper description which will explain and justify this.

Here it is, Please let me know is it ok?

renesas,output-clock-fixed-rate-mode:
    type: boolean
    description:
      In output clock fixed rate mode, the output clock frequency is always
      fixed and the hardware will use the values from the OTP or full register
	map initialized during boot.
      If not given, the output clock rate is not fixed.
    maxItems: 6

Cheers,
Biju

Krzysztof Kozlowski March 9, 2023, 9:13 a.m. UTC | #9

On 09/03/2023 08:57, Biju Das wrote:
>>> It is clk generator HW specific. Clk generator is vital component
>>> which provides clocks to the system.
>>
>> Every clock controller is vital...
>>
>>> We are providing some hardware feature which is exposed as dt
>>> properties.
>>>
>>> Like clock output is fixed rate clock or dynamic rate clock/
>>
>> OK, I wait then for proper description which will explain and justify this.
> 
> Here it is, Please let me know is it ok?
> 
> renesas,output-clock-fixed-rate-mode:
>     type: boolean
>     description:
>       In output clock fixed rate mode, the output clock frequency is always
>       fixed and the hardware will use the values from the OTP or full register
> 	map initialized during boot.
>       If not given, the output clock rate is not fixed.
>     maxItems: 6

boolean is scalar, not array, so no maxItems. If the frequency is taken
from OTP or register map, why they cannot also provide information the
clock is fixed?

> 
> Cheers,
> Biju

Best regards,
Krzysztof

Biju Das March 9, 2023, 9:18 a.m. UTC | #10

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Thursday, March 9, 2023 9:14 AM
> To: Biju Das <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
> [e-mail]; [e-mail]; [e-mail];
> Fabrizio Castro <[e-mail]>
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 09/03/2023 08:57, Biju Das wrote:
> >>> It is clk generator HW specific. Clk generator is vital component
> >>> which provides clocks to the system.
> >>
> >> Every clock controller is vital...
> >>
> >>> We are providing some hardware feature which is exposed as dt
> >>> properties.
> >>>
> >>> Like clock output is fixed rate clock or dynamic rate clock/
> >>
> >> OK, I wait then for proper description which will explain and justify
> this.
> >
> > Here it is, Please let me know is it ok?
> >
> > renesas,output-clock-fixed-rate-mode:
> >     type: boolean
> >     description:
> >       In output clock fixed rate mode, the output clock frequency is
> always
> >       fixed and the hardware will use the values from the OTP or full
> register
> > 	map initialized during boot.
> >       If not given, the output clock rate is not fixed.
> >     maxItems: 6
> 
> boolean is scalar, not array, so no maxItems. If the frequency is taken from
> OTP or register map, why they cannot also provide information the clock is
> fixed?

OK, I will make an array property instead. From HW perspective each clock output from the
Clock generator is controllable ie, fixed rate or dynamic rate.

If all the output clocks are fixed rate one, then frequency is taken from OTP or
register map. But if any one clock output generates dynamic rate, then it uses
dynamic settings.

Cheers,
Biju

Krzysztof Kozlowski March 9, 2023, 9:44 a.m. UTC | #11

On 09/03/2023 10:18, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Thursday, March 9, 2023 9:14 AM
>> To: Biju Das <[e-mail]>; Michael Turquette
>> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob Herring
>> <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>
>> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
>> [e-mail]; [e-mail]; [e-mail];
>> Fabrizio Castro <[e-mail]>
>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
>> generator bindings
>>
>> On 09/03/2023 08:57, Biju Das wrote:
>>>>> It is clk generator HW specific. Clk generator is vital component
>>>>> which provides clocks to the system.
>>>>
>>>> Every clock controller is vital...
>>>>
>>>>> We are providing some hardware feature which is exposed as dt
>>>>> properties.
>>>>>
>>>>> Like clock output is fixed rate clock or dynamic rate clock/
>>>>
>>>> OK, I wait then for proper description which will explain and justify
>> this.
>>>
>>> Here it is, Please let me know is it ok?
>>>
>>> renesas,output-clock-fixed-rate-mode:
>>>     type: boolean
>>>     description:
>>>       In output clock fixed rate mode, the output clock frequency is
>> always
>>>       fixed and the hardware will use the values from the OTP or full
>> register
>>> 	map initialized during boot.
>>>       If not given, the output clock rate is not fixed.
>>>     maxItems: 6
>>
>> boolean is scalar, not array, so no maxItems. If the frequency is taken from
>> OTP or register map, why they cannot also provide information the clock is
>> fixed?
> 
> OK, I will make an array property instead. From HW perspective each clock output from the
> Clock generator is controllable ie, fixed rate or dynamic rate.
> 
> If all the output clocks are fixed rate one, then frequency is taken from OTP or
> register map. But if any one clock output generates dynamic rate, then it uses
> dynamic settings.

Second try, same question, let me know if it is not clear:

"why they cannot also provide information the clock is fixed?"

Best regards,
Krzysztof

Biju Das March 9, 2023, 9:53 a.m. UTC | #12

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Thursday, March 9, 2023 9:44 AM
> To: Biju Das <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
> [e-mail]; [e-mail]; [e-mail];
> Fabrizio Castro <[e-mail]>
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 09/03/2023 10:18, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Thursday, March 9, 2023 9:14 AM
> >> To: Biju Das <[e-mail]>; Michael Turquette
> >> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> >> Herring <[e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>
> >> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
> >> [e-mail]; [e-mail];
> >> [e-mail]; Fabrizio Castro
> >> <[e-mail]>
> >> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
> >> clock generator bindings
> >>
> >> On 09/03/2023 08:57, Biju Das wrote:
> >>>>> It is clk generator HW specific. Clk generator is vital component
> >>>>> which provides clocks to the system.
> >>>>
> >>>> Every clock controller is vital...
> >>>>
> >>>>> We are providing some hardware feature which is exposed as dt
> >>>>> properties.
> >>>>>
> >>>>> Like clock output is fixed rate clock or dynamic rate clock/
> >>>>
> >>>> OK, I wait then for proper description which will explain and
> >>>> justify
> >> this.
> >>>
> >>> Here it is, Please let me know is it ok?
> >>>
> >>> renesas,output-clock-fixed-rate-mode:
> >>>     type: boolean
> >>>     description:
> >>>       In output clock fixed rate mode, the output clock frequency is
> >> always
> >>>       fixed and the hardware will use the values from the OTP or
> >>> full
> >> register
> >>> 	map initialized during boot.
> >>>       If not given, the output clock rate is not fixed.
> >>>     maxItems: 6
> >>
> >> boolean is scalar, not array, so no maxItems. If the frequency is
> >> taken from OTP or register map, why they cannot also provide
> >> information the clock is fixed?
> >
> > OK, I will make an array property instead. From HW perspective each
> > clock output from the Clock generator is controllable ie, fixed rate or
> dynamic rate.
> >
> > If all the output clocks are fixed rate one, then frequency is taken
> > from OTP or register map. But if any one clock output generates
> > dynamic rate, then it uses dynamic settings.
> 
> Second try, same question, let me know if it is not clear:
> 
> "why they cannot also provide information the clock is fixed?"

This information we are providing through dt.

It is a complex clock generator which provides 6 HW clock outputs.
The 6 HW clock outputs can be individually controllable to generate
Either fixed frequency or dynamic frequency.

 Output clk1 "diff2",
 Output clk2 "diff1",
 Output clk3 "se3",
 Output clk4 "se2",
 Output clk5 "se1",
 Output clk6 "ref"

I want to make "Output clk4" from clock generator as dynamic frequency one
And make other clock frequency from clock generator as fixed one.

How do you describe this in dt? Please share your thoughts.

Cheers,
Biju

Geert Uytterhoeven March 9, 2023, 9:58 a.m. UTC | #13

Hi Biju,

On Thu, Mar 9, 2023 at 10:44 AM Krzysztof Kozlowski
<[e-mail]> wrote:
> On 09/03/2023 10:18, Biju Das wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> On 09/03/2023 08:57, Biju Das wrote:
> >>>>> It is clk generator HW specific. Clk generator is vital component
> >>>>> which provides clocks to the system.
> >>>>
> >>>> Every clock controller is vital...
> >>>>
> >>>>> We are providing some hardware feature which is exposed as dt
> >>>>> properties.
> >>>>>
> >>>>> Like clock output is fixed rate clock or dynamic rate clock/
> >>>>
> >>>> OK, I wait then for proper description which will explain and justify
> >> this.
> >>>
> >>> Here it is, Please let me know is it ok?
> >>>
> >>> renesas,output-clock-fixed-rate-mode:
> >>>     type: boolean
> >>>     description:
> >>>       In output clock fixed rate mode, the output clock frequency is
> >> always
> >>>       fixed and the hardware will use the values from the OTP or full
> >> register
> >>>     map initialized during boot.
> >>>       If not given, the output clock rate is not fixed.
> >>>     maxItems: 6
> >>
> >> boolean is scalar, not array, so no maxItems. If the frequency is taken from
> >> OTP or register map, why they cannot also provide information the clock is
> >> fixed?
> >
> > OK, I will make an array property instead. From HW perspective each clock output from the
> > Clock generator is controllable ie, fixed rate or dynamic rate.
> >
> > If all the output clocks are fixed rate one, then frequency is taken from OTP or
> > register map. But if any one clock output generates dynamic rate, then it uses
> > dynamic settings.
>
> Second try, same question, let me know if it is not clear:
>
> "why they cannot also provide information the clock is fixed?"

What is the actual use case?
My understanding is:
  1. If the OTP is programmed, the clock generator will be configured
     from the OTP on power-on,
  2. The clock generator can be (re)configured from software.
     a. If the OTP is programmed, this is not needed,
     b. For critical clocks, you may want to prevent this.

Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
and purely software? Or are there OTP bits to enforce this?

Perhaps you need a per-output "do-not-change-frequency" flag,
probably with a generic name, in the spirit of "regulator-always-on"
for regulators?

Now, if all the output clocks are fixed rate, you might want to describe
this in DTS using a set of fixed{,-factor-}-clocks?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Krzysztof Kozlowski March 9, 2023, 10:15 a.m. UTC | #14

On 09/03/2023 10:53, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Thursday, March 9, 2023 9:44 AM
>> To: Biju Das <[e-mail]>; Michael Turquette
>> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob Herring
>> <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>
>> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
>> [e-mail]; [e-mail]; [e-mail];
>> Fabrizio Castro <[e-mail]>
>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
>> generator bindings
>>
>> On 09/03/2023 10:18, Biju Das wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: Thursday, March 9, 2023 9:14 AM
>>>> To: Biju Das <[e-mail]>; Michael Turquette
>>>> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
>>>> Herring <[e-mail]>; Krzysztof Kozlowski
>>>> <[e-mail]>
>>>> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Fabrizio Castro
>>>> <[e-mail]>
>>>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
>>>> clock generator bindings
>>>>
>>>> On 09/03/2023 08:57, Biju Das wrote:
>>>>>>> It is clk generator HW specific. Clk generator is vital component
>>>>>>> which provides clocks to the system.
>>>>>>
>>>>>> Every clock controller is vital...
>>>>>>
>>>>>>> We are providing some hardware feature which is exposed as dt
>>>>>>> properties.
>>>>>>>
>>>>>>> Like clock output is fixed rate clock or dynamic rate clock/
>>>>>>
>>>>>> OK, I wait then for proper description which will explain and
>>>>>> justify
>>>> this.
>>>>>
>>>>> Here it is, Please let me know is it ok?
>>>>>
>>>>> renesas,output-clock-fixed-rate-mode:
>>>>>     type: boolean
>>>>>     description:
>>>>>       In output clock fixed rate mode, the output clock frequency is
>>>> always
>>>>>       fixed and the hardware will use the values from the OTP or
>>>>> full
>>>> register
>>>>> 	map initialized during boot.
>>>>>       If not given, the output clock rate is not fixed.
>>>>>     maxItems: 6
>>>>
>>>> boolean is scalar, not array, so no maxItems. If the frequency is
>>>> taken from OTP or register map, why they cannot also provide
>>>> information the clock is fixed?
>>>
>>> OK, I will make an array property instead. From HW perspective each
>>> clock output from the Clock generator is controllable ie, fixed rate or
>> dynamic rate.
>>>
>>> If all the output clocks are fixed rate one, then frequency is taken
>>> from OTP or register map. But if any one clock output generates
>>> dynamic rate, then it uses dynamic settings.
>>
>> Second try, same question, let me know if it is not clear:
>>
>> "why they cannot also provide information the clock is fixed?"
> 
> This information we are providing through dt.

No, you are not. We just discuss it. If we do not agree, you are not
going to provide information through DT.

> 
> It is a complex clock generator which provides 6 HW clock outputs.
> The 6 HW clock outputs can be individually controllable to generate
> Either fixed frequency or dynamic frequency.

Ah, indeed. 6 clock outputs prohibits configuring this from OTP. If only
there were 5 outputs then it would be possible...

> 
>  Output clk1 "diff2",
>  Output clk2 "diff1",
>  Output clk3 "se3",
>  Output clk4 "se2",
>  Output clk5 "se1",
>  Output clk6 "ref"
> 
> I want to make "Output clk4" from clock generator as dynamic frequency one
> And make other clock frequency from clock generator as fixed one.
> 
> How do you describe this in dt? Please share your thoughts.

Read from OTP or registers.

Best regards,
Krzysztof

Krzysztof Kozlowski March 9, 2023, 10:17 a.m. UTC | #15

On 09/03/2023 10:58, Geert Uytterhoeven wrote:
>>>>> Here it is, Please let me know is it ok?
>>>>>
>>>>> renesas,output-clock-fixed-rate-mode:
>>>>>     type: boolean
>>>>>     description:
>>>>>       In output clock fixed rate mode, the output clock frequency is
>>>> always
>>>>>       fixed and the hardware will use the values from the OTP or full
>>>> register
>>>>>     map initialized during boot.
>>>>>       If not given, the output clock rate is not fixed.
>>>>>     maxItems: 6
>>>>
>>>> boolean is scalar, not array, so no maxItems. If the frequency is taken from
>>>> OTP or register map, why they cannot also provide information the clock is
>>>> fixed?
>>>
>>> OK, I will make an array property instead. From HW perspective each clock output from the
>>> Clock generator is controllable ie, fixed rate or dynamic rate.
>>>
>>> If all the output clocks are fixed rate one, then frequency is taken from OTP or
>>> register map. But if any one clock output generates dynamic rate, then it uses
>>> dynamic settings.
>>
>> Second try, same question, let me know if it is not clear:
>>
>> "why they cannot also provide information the clock is fixed?"
> 
> What is the actual use case?
> My understanding is:
>   1. If the OTP is programmed, the clock generator will be configured
>      from the OTP on power-on,
>   2. The clock generator can be (re)configured from software.
>      a. If the OTP is programmed, this is not needed,
>      b. For critical clocks, you may want to prevent this.
> 
> Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
> and purely software? Or are there OTP bits to enforce this?
> 
> Perhaps you need a per-output "do-not-change-frequency" flag,
> probably with a generic name, in the spirit of "regulator-always-on"
> for regulators?
> 
> Now, if all the output clocks are fixed rate, you might want to describe
> this in DTS using a set of fixed{,-factor-}-clocks?
> 

I would also argue that fixed frequency is actually also dynamic
frequency, just with a limit to one frequency...

Best regards,
Krzysztof

Geert Uytterhoeven March 9, 2023, 10:25 a.m. UTC | #16

Hi Krzysztof,

On Thu, Mar 9, 2023 at 11:17 AM Krzysztof Kozlowski
<[e-mail]> wrote:
> On 09/03/2023 10:58, Geert Uytterhoeven wrote:
> >>>>> Here it is, Please let me know is it ok?
> >>>>>
> >>>>> renesas,output-clock-fixed-rate-mode:
> >>>>>     type: boolean
> >>>>>     description:
> >>>>>       In output clock fixed rate mode, the output clock frequency is
> >>>> always
> >>>>>       fixed and the hardware will use the values from the OTP or full
> >>>> register
> >>>>>     map initialized during boot.
> >>>>>       If not given, the output clock rate is not fixed.
> >>>>>     maxItems: 6
> >>>>
> >>>> boolean is scalar, not array, so no maxItems. If the frequency is taken from
> >>>> OTP or register map, why they cannot also provide information the clock is
> >>>> fixed?
> >>>
> >>> OK, I will make an array property instead. From HW perspective each clock output from the
> >>> Clock generator is controllable ie, fixed rate or dynamic rate.
> >>>
> >>> If all the output clocks are fixed rate one, then frequency is taken from OTP or
> >>> register map. But if any one clock output generates dynamic rate, then it uses
> >>> dynamic settings.
> >>
> >> Second try, same question, let me know if it is not clear:
> >>
> >> "why they cannot also provide information the clock is fixed?"
> >
> > What is the actual use case?
> > My understanding is:
> >   1. If the OTP is programmed, the clock generator will be configured
> >      from the OTP on power-on,
> >   2. The clock generator can be (re)configured from software.
> >      a. If the OTP is programmed, this is not needed,
> >      b. For critical clocks, you may want to prevent this.
> >
> > Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
> > and purely software? Or are there OTP bits to enforce this?
> >
> > Perhaps you need a per-output "do-not-change-frequency" flag,
> > probably with a generic name, in the spirit of "regulator-always-on"
> > for regulators?

Actually we already have "assigned-clock{,-rate}s" properties for that.

> > Now, if all the output clocks are fixed rate, you might want to describe
> > this in DTS using a set of fixed{,-factor-}-clocks?
>
> I would also argue that fixed frequency is actually also dynamic
> frequency, just with a limit to one frequency...

Indeed.

Gr{oetje,eeting}s,

                        Geert

Biju Das March 9, 2023, 10:30 a.m. UTC | #17

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> Hi Biju,
> 
> On Thu, Mar 9, 2023 at 10:44 AM Krzysztof Kozlowski
> <[e-mail]> wrote:
> > On 09/03/2023 10:18, Biju Das wrote:
> > >> -----Original Message-----
> > >> From: Krzysztof Kozlowski <[e-mail]> On
> > >> 09/03/2023 08:57, Biju Das wrote:
> > >>>>> It is clk generator HW specific. Clk generator is vital
> > >>>>> component which provides clocks to the system.
> > >>>>
> > >>>> Every clock controller is vital...
> > >>>>
> > >>>>> We are providing some hardware feature which is exposed as dt
> > >>>>> properties.
> > >>>>>
> > >>>>> Like clock output is fixed rate clock or dynamic rate clock/
> > >>>>
> > >>>> OK, I wait then for proper description which will explain and
> > >>>> justify
> > >> this.
> > >>>
> > >>> Here it is, Please let me know is it ok?
> > >>>
> > >>> renesas,output-clock-fixed-rate-mode:
> > >>>     type: boolean
> > >>>     description:
> > >>>       In output clock fixed rate mode, the output clock frequency
> > >>> is
> > >> always
> > >>>       fixed and the hardware will use the values from the OTP or
> > >>> full
> > >> register
> > >>>     map initialized during boot.
> > >>>       If not given, the output clock rate is not fixed.
> > >>>     maxItems: 6
> > >>
> > >> boolean is scalar, not array, so no maxItems. If the frequency is
> > >> taken from OTP or register map, why they cannot also provide
> > >> information the clock is fixed?
> > >
> > > OK, I will make an array property instead. From HW perspective each
> > > clock output from the Clock generator is controllable ie, fixed rate or
> dynamic rate.
> > >
> > > If all the output clocks are fixed rate one, then frequency is taken
> > > from OTP or register map. But if any one clock output generates
> > > dynamic rate, then it uses dynamic settings.
> >
> > Second try, same question, let me know if it is not clear:
> >
> > "why they cannot also provide information the clock is fixed?"
> 
> What is the actual use case?
> My understanding is:
>   1. If the OTP is programmed, the clock generator will be configured
>      from the OTP on power-on,

Correct.

>   2. The clock generator can be (re)configured from software.
>      a. If the OTP is programmed, this is not needed,


Yes, But we miss some HW functionality.

Eg:
On RZ/G2L SMARC EVK, By default audio mclk is connected to
11.2896 MHz clk (se2 output from clock generator)  which is multiple of 44.1KHz.
and this clock is a non-critical clock.

48Khz playback/record is not possible with Audio codec, if we just use the
value from OTP.

But by changing parent of "se2 clock", it is possible to achieve
48 KHz playback.

>      b. For critical clocks, you may want to prevent this.

For caseb, Critical clocks we won't change its registers.
The reconfiguration is only for non-critical clocks.

> Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy, and
> purely software? Or are there OTP bits to enforce this?

Nothing OTP bits related. 

> 
> Perhaps you need a per-output "do-not-change-frequency" flag, probably with
> a generic name, in the spirit of "regulator-always-on"
> for regulators?

Yes "do-not-change-frequency" flag for per-output is sensible one.

> Now, if all the output clocks are fixed rate, you might want to describe
> this in DTS using a set of fixed{,-factor-}-clocks?

On Ideal case, all the output clocks are fixed rate and use the value from OTP.

But cases like, non critical clocks we should be able to
change frequency of that particular clock output.

In audio playback case, it is just 1 bit for changing the parent,
After the initial reconfiguration.

Cheers,
Biju

Biju Das March 9, 2023, 11:18 a.m. UTC | #18

Hi Krzysztof Kozlowski,

> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> On 09/03/2023 10:53, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Thursday, March 9, 2023 9:44 AM
> >> To: Biju Das <[e-mail]>; Michael Turquette
> >> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> >> Herring <[e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>
> >> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
> >> [e-mail]; [e-mail];
> >> [e-mail]; Fabrizio Castro
> >> <[e-mail]>
> >> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
> >> clock generator bindings
> >>
> >> On 09/03/2023 10:18, Biju Das wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: Thursday, March 9, 2023 9:14 AM
> >>>> To: Biju Das <[e-mail]>; Michael Turquette
> >>>> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> >>>> Herring <[e-mail]>; Krzysztof Kozlowski
> >>>> <[e-mail]>
> >>>> Cc: Geert Uytterhoeven <[e-mail]>; linux-renesas-
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; Fabrizio Castro
> >>>> <[e-mail]>
> >>>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
> >>>> clock generator bindings
> >>>>
> >>>> On 09/03/2023 08:57, Biju Das wrote:
> >>>>>>> It is clk generator HW specific. Clk generator is vital
> >>>>>>> component which provides clocks to the system.
> >>>>>>
> >>>>>> Every clock controller is vital...
> >>>>>>
> >>>>>>> We are providing some hardware feature which is exposed as dt
> >>>>>>> properties.
> >>>>>>>
> >>>>>>> Like clock output is fixed rate clock or dynamic rate clock/
> >>>>>>
> >>>>>> OK, I wait then for proper description which will explain and
> >>>>>> justify
> >>>> this.
> >>>>>
> >>>>> Here it is, Please let me know is it ok?
> >>>>>
> >>>>> renesas,output-clock-fixed-rate-mode:
> >>>>>     type: boolean
> >>>>>     description:
> >>>>>       In output clock fixed rate mode, the output clock frequency
> >>>>> is
> >>>> always
> >>>>>       fixed and the hardware will use the values from the OTP or
> >>>>> full
> >>>> register
> >>>>> 	map initialized during boot.
> >>>>>       If not given, the output clock rate is not fixed.
> >>>>>     maxItems: 6
> >>>>
> >>>> boolean is scalar, not array, so no maxItems. If the frequency is
> >>>> taken from OTP or register map, why they cannot also provide
> >>>> information the clock is fixed?
> >>>
> >>> OK, I will make an array property instead. From HW perspective each
> >>> clock output from the Clock generator is controllable ie, fixed rate
> >>> or
> >> dynamic rate.
> >>>
> >>> If all the output clocks are fixed rate one, then frequency is taken
> >>> from OTP or register map. But if any one clock output generates
> >>> dynamic rate, then it uses dynamic settings.
> >>
> >> Second try, same question, let me know if it is not clear:
> >>
> >> "why they cannot also provide information the clock is fixed?"
> >
> > This information we are providing through dt.
> 
> No, you are not. We just discuss it. If we do not agree, you are not going
> to provide information through DT.
> 
> >
> > It is a complex clock generator which provides 6 HW clock outputs.
> > The 6 HW clock outputs can be individually controllable to generate
> > Either fixed frequency or dynamic frequency.
> 
> Ah, indeed. 6 clock outputs prohibits configuring this from OTP. If only
> there were 5 outputs then it would be possible...
> 
> >
> >  Output clk1 "diff2",
> >  Output clk2 "diff1",
> >  Output clk3 "se3",
> >  Output clk4 "se2",
> >  Output clk5 "se1",
> >  Output clk6 "ref"
> >
> > I want to make "Output clk4" from clock generator as dynamic frequency
> > one And make other clock frequency from clock generator as fixed one.
> >
> > How do you describe this in dt? Please share your thoughts.
> 
> Read from OTP or registers.

Looks no other option. So will drop this property and match the one from OTP with full regmap.

Cheers,
Biju

Biju Das March 9, 2023, 3:15 p.m. UTC | #19

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
> 
> Hi Krzysztof,
> 
> On Thu, Mar 9, 2023 at 11:17 AM Krzysztof Kozlowski
> <[e-mail]> wrote:
> > On 09/03/2023 10:58, Geert Uytterhoeven wrote:
> > >>>>> Here it is, Please let me know is it ok?
> > >>>>>
> > >>>>> renesas,output-clock-fixed-rate-mode:
> > >>>>>     type: boolean
> > >>>>>     description:
> > >>>>>       In output clock fixed rate mode, the output clock
> > >>>>> frequency is
> > >>>> always
> > >>>>>       fixed and the hardware will use the values from the OTP or
> > >>>>> full
> > >>>> register
> > >>>>>     map initialized during boot.
> > >>>>>       If not given, the output clock rate is not fixed.
> > >>>>>     maxItems: 6
> > >>>>
> > >>>> boolean is scalar, not array, so no maxItems. If the frequency is
> > >>>> taken from OTP or register map, why they cannot also provide
> > >>>> information the clock is fixed?
> > >>>
> > >>> OK, I will make an array property instead. From HW perspective
> > >>> each clock output from the Clock generator is controllable ie, fixed
> rate or dynamic rate.
> > >>>
> > >>> If all the output clocks are fixed rate one, then frequency is
> > >>> taken from OTP or register map. But if any one clock output
> > >>> generates dynamic rate, then it uses dynamic settings.
> > >>
> > >> Second try, same question, let me know if it is not clear:
> > >>
> > >> "why they cannot also provide information the clock is fixed?"
> > >
> > > What is the actual use case?
> > > My understanding is:
> > >   1. If the OTP is programmed, the clock generator will be configured
> > >      from the OTP on power-on,
> > >   2. The clock generator can be (re)configured from software.
> > >      a. If the OTP is programmed, this is not needed,
> > >      b. For critical clocks, you may want to prevent this.
> > >
> > > Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
> > > and purely software? Or are there OTP bits to enforce this?
> > >
> > > Perhaps you need a per-output "do-not-change-frequency" flag,
> > > probably with a generic name, in the spirit of "regulator-always-on"
> > > for regulators?
> 
> Actually we already have "assigned-clock{,-rate}s" properties for that.

Actually it is a good pointer. I can make use of this property.

Cheers,
Biju

Patch

diff --git a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
new file mode 100644
index 000000000000..f45b8da73ec3
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
@@ -0,0 +1,135 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,versaclock3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas VersaClock 3 programmable I2C clock generators
+
+description: |
+  The 5P35023 is a VersaClock programmable clock generator and
+  is designed for low-power, consumer, and high-performance PCI
+  express applications. The 5P35023 device is a three PLL
+  architecture design, and each PLL is individually programmable
+  and allowing for up to 6 unique frequency outputs.
+
+  An internal OTP memory allows the user to store the configuration
+  in the device. After power up, the user can change the device register
+  settings through the I2C interface when I2C mode is selected.
+
+  The driver can read a full register map from the DT, and will use that
+  register map to initialize the attached part (via I2C) when the system
+  boots. Any configuration not supported by the common clock framework
+  must be done via the full register map, including optimized settings.
+
+  Link to datasheet: https://www.renesas.com/us/en/products/clocks-timing/
+                     clock-generation/programmable-clocks/
+                     5p35023-versaclock-3s-programmable-clock-generator
+
+maintainers:
+  - Biju Das <[e-mail]>
+
+properties:
+  compatible:
+    enum:
+      - renesas,5p35023
+
+  reg:
+    description: I2C device address
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+  clock-names:
+    oneOf:
+      - items:
+          - const: x1
+      - items:
+          - const: clkin
+
+  clocks:
+    maxItems: 1
+
+  renesas,settings:
+    description: Optional, complete register map of the device.
+      Optimized settings for the device must be provided in full
+      and are written during initialization.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    minItems: 37
+
+  assigned-clocks:
+    minItems: 6
+
+  assigned-clock-rates:
+    minItems: 6
+
+  renesas,clock-divider-read-only:
+    description: Flag for setting divider in read only mode.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 5
+
+  renesas,clock-flags:
+    description: Flags used in common clock frame work for configuring
+      clk outputs. See include/linux/clk-provider.h
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 6
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    /* 24MHz crystal */
+    x1_x2: xtal {
+      compatible = "fixed-clock";
+      #clock-cells = <0>;
+      clock-frequency = <24000000>;
+    };
+
+    i2c@0 {
+        reg = <0x0 0x100>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        versa3: clock-generator@68 {
+            compatible = "renesas,5p35023";
+            reg = <0x68>;
+            #clock-cells = <1>;
+
+            clocks = <&x1_x2>;
+            clock-names = "x1";
+
+            renesas,settings = [
+                80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
+                00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
+                80 b0 45 c4 95
+            ];
+
+            assigned-clocks = <&versa3 0>,
+                              <&versa3 1>,
+                              <&versa3 2>,
+                              <&versa3 3>,
+                              <&versa3 4>,
+                              <&versa3 5>;
+            assigned-clock-rates = <12288000>, <25000000>,
+                                   <12000000>, <11289600>,
+                                   <11289600>, <24000000>;
+            renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
+            renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
+                                  <2176>, <2048>;
+        };
+    };
+
+    /* Consumer referencing the versa 3 */
+    consumer {
+        /* ... */
+        clocks = <&versa3 3>;
+        /* ... */
+    };