Re: [PATCH v2 1/2] dt-bindings: Add binding for Renesas 8T49N241

[Alexander Helms <alexander.helms.jy@renesas.com>]

Hi Biju,

Please see my comments below. Also, I'm new to the LKML process and learning, please forgive any errors.

The 07/01/2021 23:52, Biju Das wrote:
> Hi Alex,
> 
> Thanks for the patch.
> 
> > -----Original Message-----
> > From: Alex Helms <alexander.helms.jy@renesas.com>
> > Sent: 02 July 2021 00:23
> > To: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > clk@vger.kernel.org; linux-renesas-soc@vger.kernel.org
> > Cc: robh+dt@kernel.org; sboyd@kernel.org; mturquette@baylibre.com;
> > geert+renesas@glider.be; Alexander Helms <alexander.helms.jy@renesas.com>;
> > David Cater <david.cater.jc@renesas.com>; michal.simek@xilinx.com
> > Subject: [PATCH v2 1/2] dt-bindings: Add binding for Renesas 8T49N241
> > 
> > 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 <alexander.helms.jy@renesas.com>
> > ---
> >  .../bindings/clock/renesas,8t49n241.yaml      | 183 ++++++++++++++++++
> >  MAINTAINERS                                   |   6 +
> >  2 files changed, 189 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> > b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> > new file mode 100644
> > index 000000000..d817ec46d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> > @@ -0,0 +1,183 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
> > +tree.org%2Fschemas%2Fclock%2Frenesas%2C8t49n24x.yaml%23&amp;data=04%7C0
> > +1%7Cbiju.das.jz%40bp.renesas.com%7Ce51e24d06083403a3d0e08d93ce73933%7C5
> > +3d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637607786063741484%7CUnknown%
> > +7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> > +CI6Mn0%3D%7C1000&amp;sdata=BIKHeHpyG3vloEEO02HzM6TtWqQtDlSGRMifOQVprPI%
> > +3D&amp;reserved=0
> > +$schema:
> > +https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
> > +tree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cbiju.das.jz%4
> > +0bp.renesas.com%7Ce51e24d06083403a3d0e08d93ce73933%7C53d82571da1947e49c
> > +b4625a166a4a2a%7C0%7C0%7C637607786063751443%7CUnknown%7CTWFpbGZsb3d8eyJ
> > +WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> > +amp;sdata=3XhlKhlwT3hr0KVjynaiMlqMC94tq3sEYgNH6LMf9Kc%3D&amp;reserved=0
> > +
> > +title: Binding for Renesas 8T49N241 Universal Frequency Translator
> > +
> > +description: |
> > +  The 8T49N241 has one fractional-feedback PLL that can be used as a
> > +  jitter attenuator and frequency translator. It is equipped with one
> > +  integer and three fractional output dividers, allowing the generation
> > +  of up to four different output frequencies, ranging from 8kHz to 1GHz.
> > +  These frequencies are completely independent of each other, the input
> > +  reference frequencies and the crystal reference frequency. The device
> > +  places virtually no constraints on input to output frequency
> > +conversion,
> > +  supporting all FEC rates, including the new revision of ITU-T
> > +  Recommendation G.709 (2009), most with 0ppm conversion error.
> > +  The outputs may select among LVPECL, LVDS, HCSL or LVCMOS output
> > levels.
> > +
> > +  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.
> > +
> > +  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 just to the crystal
> > + to behave as a  frequency synthesizer. The PLL can use the second
> > + input for redundant  backup of the primary input reference, but in
> > + this case, both input clock  references must be related in frequency.
> > +
> > +  All outputs are currently assumed to be LVDS, unless overridden in
> > + the  full register map in the DT.
> > +
> > +maintainers:
> > +  - Alex Helms <alexander.helms.jy@renesas.com>
> > +  - David Cater <david.cater.jc@renesas.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - 8t49n241
> > +
> > +  reg:
> > +    description: I2C device address
> > +    enum: [ 0x7c, 0x6c, 0x7d, 0x6d, 0x7e, 0x6e, 0x7f, 0x6f ]
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  clock-names:
> > +    description: Name of the input clock
> > +    minItems: 1
> > +    maxItems: 3
> > +    items:
> > +      enum: [ input-xtal, input-clk0, input-clk1 ]
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    /* 25MHz reference clock */
> > +    input_clk0: input_clk0 {
> > +      compatible = "fixed-clock";
> > +      #clock-cells = <0>;
> > +      clock-frequency = <25000000>;
> > +    };
> > +
> > +    i2c@0 {
> > +        reg = <0x0 0x100>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        renesas8t49n241_1: clock-generator@6c {
> > +            compatible = "renesas,8t49n241";
> > +            reg = <0x6c>;
> > +            #clock-cells = <1>;
> > +
> > +            clocks = <&input_clk0>;
> > +            clock-names = "input-clk0";
> > +        };
> > +    };
> > +
> > +    /* Consumer referencing the 8T49N241 Q1 */
> > +    consumer {
> > +        /* ... */
> > +        clocks = <&renesas8t49n241_1 1>;
> > +        /* ... */
> > +    };
> > +  - |
> > +    /* 40MHz crystal */
> > +    input_xtal: input_xtal {
> > +      compatible = "fixed-clock";
> > +      #clock-cells = <0>;
> > +      clock-frequency = <40000000>;
> > +    };
> > +
> > +    i2c@0 {
> > +        reg = <0x0 0x100>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        renesas8t49n241_2: clock-generator@6c {
> > +            compatible = "renesas,8t49n241";
> > +            reg = <0x6c>;
> > +            #clock-cells = <1>;
> > +
> > +            clocks = <&input_xtal>;
> > +            clock-names = "input-xtal";
> > +
> > +            settings=[
> 
> optional or required property?
> 

My mistake, `settings` is optional and should be in the schema.
I'll fix this in the next patch version.

> > +                09 50 00 60 67 C5 6C FF 03 00 30 00 00 01 00 00
> > +                01 07 00 00 07 00 00 77 6D 06 00 00 00 00 00 FF
> > +                FF FF FF 00 3F 00 2A 00 16 33 33 00 01 00 00 D0
> > +                00 00 00 00 00 00 00 00 00 04 00 00 00 02 00 00
> > +                00 00 00 00 00 00 00 17 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 D7 0A 2B 20 00 00 00 0B
> > +                00 00 00 00 00 00 00 00 00 00 27 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                C3 00 08 01 00 00 00 00 00 00 00 00 00 30 00 00
> > +                00 0A 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +                00 00 00 00 85 00 00 9C 01 D4 02 71 07 00 00 00
> > +                00 83 00 10 02 08 8C
> > +            ];
> > +        };
> > +    };
> > +
> 
> One question, 
> Full regmap or just overriding few registers which is better, assuming the device has default values programmed in OTP.
> Again that needs {offset, value}???

`settings` must be the full regmap. The next patch version will have that clarified in the schema description.
The intent here is the external configuration software produces optimized settings and the results are the full regmap.

> 
> Regards,
> Biju
> 
>  
> > +    /* Consumer referencing the 8T49N241 Q1 */
> > +    consumer {
> > +        /* ... */
> > +        clocks = <&renesas8t49n241_2 1>;
> > +        /* ... */
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0cce91cd5..882d79ead 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15575,6 +15575,12 @@ F:	include/linux/rpmsg/
> >  F:	include/uapi/linux/rpmsg.h
> >  F:	samples/rpmsg/
> > 
> > +RENESAS 8T49N24X DRIVER
> > +M:	Alex Helms <alexander.helms.jy@renesas.com>
> > +M:	David Cater <david.cater.jc@renesas.com>
> > +S:	Odd Fixes
> > +F:	Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> > +
> >  RENESAS CLOCK DRIVERS
> >  M:	Geert Uytterhoeven <geert+renesas@glider.be>
> >  L:	linux-renesas-soc@vger.kernel.org
> > --
> > 2.30.2
> 

Regards,

Alex

--