linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexander Helms <alexander.helms.jy@renesas.com>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"geert+renesas@glider.be" <geert+renesas@glider.be>,
	David Cater <david.cater.jc@renesas.com>,
	"michal.simek@xilinx.com" <michal.simek@xilinx.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: Add binding for Renesas 8T49N241
Date: Fri, 2 Jul 2021 14:51:02 -0700	[thread overview]
Message-ID: <20210702215102.GA20007@renesas.com> (raw)
In-Reply-To: <OS0PR01MB592240DACD7D18609EA70AE0861F9@OS0PR01MB5922.jpnprd01.prod.outlook.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

-- 

  reply	other threads:[~2021-07-02 21:51 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-01 19:41 [PATCH 0/2] Renesas 8T49N241 device driver Alex Helms
2021-07-01 19:41 ` [PATCH 1/2] dt-bindings: Add binding for Renesas 8T49N241 Alex Helms
2021-07-01 19:41 ` [PATCH 2/2] clk: Add ccf driver " Alex Helms
2021-07-01 22:21   ` kernel test robot
2021-07-01 23:22     ` [PATCH v2 0/2] Renesas 8T49N241 device driver Alex Helms
2021-07-01 23:22       ` [PATCH v2 1/2] dt-bindings: Add binding for Renesas 8T49N241 Alex Helms
2021-07-02  6:52         ` Biju Das
2021-07-02 21:51           ` Alexander Helms [this message]
2021-07-03 10:15             ` Biju Das
2021-07-02 13:28         ` Rob Herring
2021-07-01 23:22       ` [PATCH v2 2/2] clk: Add ccf driver " Alex Helms
2021-07-02  0:57       ` [PATCH v2 0/2] Renesas 8T49N241 device driver Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210702215102.GA20007@renesas.com \
    --to=alexander.helms.jy@renesas.com \
    --cc=biju.das.jz@bp.renesas.com \
    --cc=david.cater.jc@renesas.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=mturquette@baylibre.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).