Hi Alexander, > Subject: Re: [PATCH v2 1/2] dt-bindings: Add binding for Renesas 8T49N241 > > 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@xxxxxxxxxxx> > > > Sent: 02 July 2021 00:23 > > > To: linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > > > clk@xxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx > > > Cc: robh+dt@xxxxxxxxxx; sboyd@xxxxxxxxxx; mturquette@xxxxxxxxxxxx; > > > geert+renesas@xxxxxxxxx; Alexander Helms > > > geert+<alexander.helms.jy@xxxxxxxxxxx>; > > > David Cater <david.cater.jc@xxxxxxxxxxx>; michal.simek@xxxxxxxxxx > > > 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@xxxxxxxxxxx> > > > --- > > > .../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%2Fde > > > +vice > > > +tree.org%2Fschemas%2Fclock%2Frenesas%2C8t49n24x.yaml%23&data=04 > > > +%7C0 > > > +1%7Cbiju.das.jz%40bp.renesas.com%7Ce51e24d06083403a3d0e08d93ce73933 > > > +%7C5 > > > +3d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637607786063741484%7CUnkn > > > +own% > > > +7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL > > > +CJXV > > > +CI6Mn0%3D%7C1000&sdata=BIKHeHpyG3vloEEO02HzM6TtWqQtDlSGRMifOQVp > > > +rPI% > > > +3D&reserved=0 > > > +$schema: > > > +https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde > > > +vice > > > +tree.org%2Fmeta-schemas%2Fcore.yaml%23&data=04%7C01%7Cbiju.das. > > > +jz%4 > > > +0bp.renesas.com%7Ce51e24d06083403a3d0e08d93ce73933%7C53d82571da1947 > > > +e49c > > > +b4625a166a4a2a%7C0%7C0%7C637607786063751443%7CUnknown%7CTWFpbGZsb3d > > > +8eyJ > > > +WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1 > > > +000& > > > +amp;sdata=3XhlKhlwT3hr0KVjynaiMlqMC94tq3sEYgNH6LMf9Kc%3D&reserv > > > +ed=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@xxxxxxxxxxx> > > > + - David Cater <david.cater.jc@xxxxxxxxxxx> > > > + > > > +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. May be it is ok for this device. I am also working on a similar solution with versa3 clock device. Most of the values are pre-programmed. But to achieve a specific use case, I need a different configuration. The external configuration software produced a full regmap. But comparing with original pre-programmed values, It is hardly 7 register changes. Also I need to define clock flags property in DT. So user can decide whether the output clock from clock device can 1) change rate 2) read only 3) just change the parent but don't change any other configuration. Regards, Biju > > > > 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@xxxxxxxxxxx> > > > +M: David Cater <david.cater.jc@xxxxxxxxxxx> > > > +S: Odd Fixes > > > +F: Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml > > > + > > > RENESAS CLOCK DRIVERS > > > M: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > > L: linux-renesas-soc@xxxxxxxxxxxxxxx > > > -- > > > 2.30.2 > > > > Regards, > > Alex > > --
