Hi Krzysztof Kozlowski, Thanks for the feedback. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Wednesday, February 22, 2023 9:34 AM > To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Michael Turquette > <mturquette@xxxxxxxxxxxx>; Stephen Boyd <sboyd@xxxxxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@xxxxxxxxxx> > Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; linux-renesas- > soc@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx> > 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 <biju.das.jz@xxxxxxxxxxxxxx> > > --- > > .../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 <biju.das.jz@xxxxxxxxxxxxxx> > > + > > +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/833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xxxxxxxxxx/ > > > + > > + 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
