diff mbox series

[v6,1/2] dt-bindings: Add binding for Renesas 8T49N241

Message ID 20210913170436.243-2-alexander.helms.jy@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Renesas 8T49N241 device driver | expand

Commit Message

Alex Helms Sept. 13, 2021, 5:04 p.m. UTC 
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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/renesas,8t49n241.yaml      | 190 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 2 files changed, 196 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml

Comments

Geert Uytterhoeven Oct. 13, 2021, 6:02 p.m. UTC | #1 
Hi Alex,

On Mon, Sep 13, 2021 at 7:05 PM Alex Helms
<alexander.helms.jy@renesas.com> 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 <alexander.helms.jy@renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

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

> +  reg:
> +    description: I2C device address
> +    enum: [ 0x7c, 0x6c, 0x7d, 0x6d, 0x7e, 0x6e, 0x7f, 0x6f ]

I think this is too strict: according to the datasheet, the full
device address can be customized when ordering.

> +examples:

> +    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 = <&xtal>;
> +            clock-names = "xtal";
> +
> +            renesas,settings=[

Missing spaces around equal sign.

> +                09 50 00 60 67 C5 6C FF 03 00 30 00 00 01 00 00

[...]

> +            ];

With the above fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

BTW, do you plan to add interrupt and/or GPIO support later?

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Oct. 14, 2021, 12:16 p.m. UTC | #2 
Hi Alex,

On Wed, Oct 13, 2021 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Sep 13, 2021 at 7:05 PM Alex Helms
> <alexander.helms.jy@renesas.com> 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 <alexander.helms.jy@renesas.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml

> BTW, do you plan to add interrupt and/or GPIO support later?

To clarify, and I really meant to add:

  interrupts:
    maxItems: 1

to the bindings now, and GPIO-related properties and subnodes later.

Gr{oetje,eeting}s,

                        Geert
Alex Helms Oct. 19, 2021, 9:52 p.m. UTC | #3 
On 10/14/2021 5:16 AM, Geert Uytterhoeven wrote:
> Hi Alex,
> 
> On Wed, Oct 13, 2021 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, Sep 13, 2021 at 7:05 PM Alex Helms
>> <alexander.helms.jy@renesas.com> 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 <alexander.helms.jy@renesas.com>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>
>> Thanks for your patch!
>>
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> 
>> BTW, do you plan to add interrupt and/or GPIO support later?
> 
> To clarify, and I really meant to add:
> 
>   interrupts:
>     maxItems: 1
> 
> to the bindings now, and GPIO-related properties and subnodes later.

Any additional features such as interrupts and GPIO properties would only be added if there is customer demand for such features. Since there is no interrupt support, does the "interrupts" item still need to be added to the yaml?

-Alex

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Geert Uytterhoeven Oct. 20, 2021, 8:20 a.m. UTC | #4 
Hi Alex,

On Tue, Oct 19, 2021 at 11:53 PM Alex Helms
<alexander.helms.jy@renesas.com> wrote:
> On 10/14/2021 5:16 AM, Geert Uytterhoeven wrote:
> > On Wed, Oct 13, 2021 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Mon, Sep 13, 2021 at 7:05 PM Alex Helms
> >> <alexander.helms.jy@renesas.com> 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 <alexander.helms.jy@renesas.com>
> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>
> >> Thanks for your patch!
> >>
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> >
> >> BTW, do you plan to add interrupt and/or GPIO support later?
> >
> > To clarify, and I really meant to add:
> >
> >   interrupts:
> >     maxItems: 1
> >
> > to the bindings now, and GPIO-related properties and subnodes later.
>
> Any additional features such as interrupts and GPIO properties would only be added if there is customer demand for such features. Since there is no interrupt support, does the "interrupts" item still need to be added to the yaml?

DT describes hardware, not software policy (or limitations of the driver).

Arguably that applies to both interrupts and GPIOs, but the latter is
more complex to describe, while the former is a simple "interrupts"
property.  It's not uncommon for board components to have their
interrupt line wired to an SoC, even if the driver doesn't use it.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Alex Helms Oct. 20, 2021, 4:56 p.m. UTC | #5 
Hi Geert,

On 10/20/2021 1:20 AM, Geert Uytterhoeven wrote:
> Hi Alex,
> 
> On Tue, Oct 19, 2021 at 11:53 PM Alex Helms
> <alexander.helms.jy@renesas.com> wrote:
>> On 10/14/2021 5:16 AM, Geert Uytterhoeven wrote:
>>> On Wed, Oct 13, 2021 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Mon, Sep 13, 2021 at 7:05 PM Alex Helms
>>>> <alexander.helms.jy@renesas.com> 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 <alexander.helms.jy@renesas.com>
>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>
>>>> Thanks for your patch!
>>>>
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
>>>
>>>> BTW, do you plan to add interrupt and/or GPIO support later?
>>>
>>> To clarify, and I really meant to add:
>>>
>>>   interrupts:
>>>     maxItems: 1
>>>
>>> to the bindings now, and GPIO-related properties and subnodes later.
>>
>> Any additional features such as interrupts and GPIO properties would only be added if there is customer demand for such features. Since there is no interrupt support, does the "interrupts" item still need to be added to the yaml?
> 
> DT describes hardware, not software policy (or limitations of the driver).
> 
> Arguably that applies to both interrupts and GPIOs, but the latter is
> more complex to describe, while the former is a simple "interrupts"
> property.  It's not uncommon for board components to have their
> interrupt line wired to an SoC, even if the driver doesn't use it.

I understand what you are describing but I don't understand how it is relevant for this device. The device is a clock generator on the i2c bus. It has a few GPIOs that can be configured as outputs for specific events like loss of lock or loss of signal, but I don't understand why that matters. 8t49n241 is similar to the existing dt and driver silabs,si5351 which does not define any interrupts. In fact it seems their dt describes both hardware and software policy because it uses custom dt keywords for changing driver behavior such as "silabs,pll-source" or "silabs,drive-strength". So looking at other examples of similar drivers, I don't understand why the 8t49n241 driver needs to define an interrupt.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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
> 

-Alex
Geert Uytterhoeven Oct. 20, 2021, 5:57 p.m. UTC | #6 
Hi Alex,

On Wed, Oct 20, 2021 at 6:57 PM Alex Helms
<alexander.helms.jy@renesas.com> wrote:
> On 10/20/2021 1:20 AM, Geert Uytterhoeven wrote:
> > On Tue, Oct 19, 2021 at 11:53 PM Alex Helms
> > <alexander.helms.jy@renesas.com> wrote:
> >> On 10/14/2021 5:16 AM, Geert Uytterhoeven wrote:
> >>> On Wed, Oct 13, 2021 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>> On Mon, Sep 13, 2021 at 7:05 PM Alex Helms
> >>>> <alexander.helms.jy@renesas.com> 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 <alexander.helms.jy@renesas.com>
> >>>>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>>>
> >>>> Thanks for your patch!
> >>>>
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
> >>>
> >>>> BTW, do you plan to add interrupt and/or GPIO support later?
> >>>
> >>> To clarify, and I really meant to add:
> >>>
> >>>   interrupts:
> >>>     maxItems: 1
> >>>
> >>> to the bindings now, and GPIO-related properties and subnodes later.
> >>
> >> Any additional features such as interrupts and GPIO properties would only be added if there is customer demand for such features. Since there is no interrupt support, does the "interrupts" item still need to be added to the yaml?
> >
> > DT describes hardware, not software policy (or limitations of the driver).
> >
> > Arguably that applies to both interrupts and GPIOs, but the latter is
> > more complex to describe, while the former is a simple "interrupts"
> > property.  It's not uncommon for board components to have their
> > interrupt line wired to an SoC, even if the driver doesn't use it.
>
> I understand what you are describing but I don't understand how it is
> relevant for this device. The device is a clock generator on the i2c
> bus. It has a few GPIOs that can be configured as outputs for specific
> events like loss of lock or loss of signal, but I don't understand why

According to the datasheet, the GPIOs can not only be used to indicate
clock generator status, but also as real GPIOs, e.g. to control LEDs
from software.

> that matters. 8t49n241 is similar to the existing dt and driver
> silabs,si5351 which does not define any interrupts. In fact it seems
> their dt describes both hardware and software policy because it uses
> custom dt keywords for changing driver behavior such as
> "silabs,pll-source" or "silabs,drive-strength". So looking at other
> examples of similar drivers, I don't understand why the 8t49n241
> driver needs to define an interrupt.

OK. So just ignore interrupts and GPIO for now.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
new file mode 100644
index 000000000..586dd6f5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
@@ -0,0 +1,190 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,8t49n241.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+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:
+      - renesas,8t49n241
+
+  reg:
+    description: I2C device address
+    enum: [ 0x7c, 0x6c, 0x7d, 0x6d, 0x7e, 0x6e, 0x7f, 0x6f ]
+
+  '#clock-cells':
+    const: 1
+
+  clock-names:
+    minItems: 1
+    maxItems: 3
+    items:
+      enum: [ xtal, clk0, clk1 ]
+
+  clocks:
+    minItems: 1
+    maxItems: 3
+
+  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: 791
+    maxItems: 791
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    /* 25MHz reference clock */
+    clk0: 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 = <&clk0>;
+            clock-names = "clk0";
+        };
+    };
+
+    /* Consumer referencing the 8T49N241 Q1 */
+    consumer {
+        /* ... */
+        clocks = <&renesas8t49n241_1 1>;
+        /* ... */
+    };
+  - |
+    /* 40MHz crystal */
+    xtal: 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 = <&xtal>;
+            clock-names = "xtal";
+
+            renesas,settings=[
+                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
+            ];
+        };
+    };
+
+    /* Consumer referencing the 8T49N241 Q1 */
+    consumer {
+        /* ... */
+        clocks = <&renesas8t49n241_2 1>;
+        /* ... */
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index eeb4c70b3..3b470d369 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15934,6 +15934,12 @@  L:	linux-remoteproc@vger.kernel.org
 S:	Maintained
 F:	drivers/net/wwan/rpmsg_wwan_ctrl.c
 
+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