| Message ID | 20200326213251.54457-1-aford173@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC] clk: vc5: Add bindings for output configurations | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 4 errors, 1 warnings, 46 lines checked" |
Hi Adam, CC Marek On Thu, Mar 26, 2020 at 10:33 PM Adam Ford <aford173@gmail.com> wrote: > The Versaclock can be purchased in a non-programmed configuration. > If that is the case, the driver needs to configure the chip to > output the correct signal type, voltage and slew. > > This RFC is proposing an additional binding to allow non-programmed > chips to be configured beyond their default configuration. > > Signed-off-by: Adam Ford <aford173@gmail.com> > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > index 05a245c9df08..4bc46ed9ba4a 100644 > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > @@ -30,6 +30,25 @@ Required properties: > - 5p49v5933 and > - 5p49v5935: (optional) property not present or "clkin". > > +For all output ports, an option child node can be used to specify: > + > +- mode: can be one of > + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic > + - CMOS > + - HCSL > + - LVDS: Low voltage differential signal > + > +- voltage-level: can be one of the following microvolts > + - 1800000 > + - 2500000 > + - 3300000 > +- slew: Percent of normal, can be one of > + - P80 > + - P85 > + - P90 > + - P100 Why the P prefixes? Can't you just use integer values? After the conversion to json-schema, these values can be validated, too. > + > + > ==Mapping between clock specifier and physical pins== > > When referencing the provided clock in the DT using phandle and > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: > > ==Example== > > +#include <dt-bindings/versaclock.h> Does not exist? > + > /* 25MHz reference crystal */ > ref25: ref25m { > compatible = "fixed-clock"; > @@ -80,6 +101,13 @@ i2c-master-node { > /* Connect XIN input to 25MHz reference */ > clocks = <&ref25m>; > clock-names = "xin"; > + > + ports@1 { > + reg = <1>; > + mode = <CMOS>; > + pwr_sel = <1800000>; > + slew = <P80>; > + }; > }; > }; Gr{oetje,eeting}s, Geert
On Fri, Mar 27, 2020 at 4:41 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Adam, > > CC Marek > > On Thu, Mar 26, 2020 at 10:33 PM Adam Ford <aford173@gmail.com> wrote: > > The Versaclock can be purchased in a non-programmed configuration. > > If that is the case, the driver needs to configure the chip to > > output the correct signal type, voltage and slew. > > > > This RFC is proposing an additional binding to allow non-programmed > > chips to be configured beyond their default configuration. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > index 05a245c9df08..4bc46ed9ba4a 100644 > > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > @@ -30,6 +30,25 @@ Required properties: > > - 5p49v5933 and > > - 5p49v5935: (optional) property not present or "clkin". > > > > +For all output ports, an option child node can be used to specify: > > + > > +- mode: can be one of > > + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic > > + - CMOS > > + - HCSL > > + - LVDS: Low voltage differential signal > > + > > +- voltage-level: can be one of the following microvolts > > + - 1800000 > > + - 2500000 > > + - 3300000 > > +- slew: Percent of normal, can be one of > > + - P80 > > + - P85 > > + - P90 > > + - P100 > > Why the P prefixes? Can't you just use integer values? > After the conversion to json-schema, these values can be validated, too. > > > + > > + > > ==Mapping between clock specifier and physical pins== > > > > When referencing the provided clock in the DT using phandle and > > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: > > > > ==Example== > > > > +#include <dt-bindings/versaclock.h> > > Does not exist? Not yet. Before actually coding anything, I wanted to get feedback on how the bindings should look. In this file would be definitions of terms like P80, CMOS, and the other items that are defined for mode and slew. > > > + > > /* 25MHz reference crystal */ > > ref25: ref25m { > > compatible = "fixed-clock"; > > @@ -80,6 +101,13 @@ i2c-master-node { > > /* Connect XIN input to 25MHz reference */ > > clocks = <&ref25m>; > > clock-names = "xin"; > > + > > + ports@1 { > > + reg = <1>; > > + mode = <CMOS>; > > + pwr_sel = <1800000>; > > + slew = <P80>; > > + }; > > }; > > }; > > 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
On Fri, Mar 27, 2020 at 5:05 AM Adam Ford <aford173@gmail.com> wrote: > > On Fri, Mar 27, 2020 at 4:41 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Adam, > > > > CC Marek > > > > On Thu, Mar 26, 2020 at 10:33 PM Adam Ford <aford173@gmail.com> wrote: > > > The Versaclock can be purchased in a non-programmed configuration. > > > If that is the case, the driver needs to configure the chip to > > > output the correct signal type, voltage and slew. > > > > > > This RFC is proposing an additional binding to allow non-programmed > > > chips to be configured beyond their default configuration. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > > index 05a245c9df08..4bc46ed9ba4a 100644 > > > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > > @@ -30,6 +30,25 @@ Required properties: > > > - 5p49v5933 and > > > - 5p49v5935: (optional) property not present or "clkin". > > > > > > +For all output ports, an option child node can be used to specify: > > > + > > > +- mode: can be one of > > > + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic > > > + - CMOS > > > + - HCSL > > > + - LVDS: Low voltage differential signal > > > + > > > +- voltage-level: can be one of the following microvolts > > > + - 1800000 > > > + - 2500000 > > > + - 3300000 > > > +- slew: Percent of normal, can be one of > > > + - P80 > > > + - P85 > > > + - P90 > > > + - P100 > > > > Why the P prefixes? Can't you just use integer values? > > After the conversion to json-schema, these values can be validated, too. That makes sense. We can just use numbers. > > > > > + > > > + > > > ==Mapping between clock specifier and physical pins== > > > > > > When referencing the provided clock in the DT using phandle and > > > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: > > > > > > ==Example== > > > > > > +#include <dt-bindings/versaclock.h> > > > > Does not exist? > > Not yet. Before actually coding anything, I wanted to get feedback on > how the bindings should look. In this file would be definitions of > terms like P80, CMOS, and the other items that are defined for mode > and slew. The intent was to create this file and define a sensible translation between the arbitrary the numbers 0 to 7 and the acronyms for "output type". Would it be better to just use strings for output type (and not create this header file)? I think I've seen something like that in a TI driver. I hesitate to put a bunch of string compares in a driver. Is there another way? Could we use properties and only allow one? > > > > > > + > > > /* 25MHz reference crystal */ > > > ref25: ref25m { > > > compatible = "fixed-clock"; > > > @@ -80,6 +101,13 @@ i2c-master-node { > > > /* Connect XIN input to 25MHz reference */ > > > clocks = <&ref25m>; > > > clock-names = "xin"; > > > + > > > + ports@1 { > > > + reg = <1>; > > > + mode = <CMOS>; > > > + pwr_sel = <1800000>; > > > + slew = <P80>; > > > + }; > > > }; > > > }; > > > > 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
On Thu, Mar 26, 2020 at 04:32:51PM -0500, Adam Ford wrote: > The Versaclock can be purchased in a non-programmed configuration. > If that is the case, the driver needs to configure the chip to > output the correct signal type, voltage and slew. > > This RFC is proposing an additional binding to allow non-programmed > chips to be configured beyond their default configuration. > > Signed-off-by: Adam Ford <aford173@gmail.com> > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > index 05a245c9df08..4bc46ed9ba4a 100644 > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > @@ -30,6 +30,25 @@ Required properties: > - 5p49v5933 and > - 5p49v5935: (optional) property not present or "clkin". > > +For all output ports, an option child node can be used to specify: > + > +- mode: can be one of > + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic > + - CMOS > + - HCSL > + - LVDS: Low voltage differential signal > + > +- voltage-level: can be one of the following microvolts > + - 1800000 > + - 2500000 > + - 3300000 > +- slew: Percent of normal, can be one of > + - P80 > + - P85 > + - P90 > + - P100 > + > + > ==Mapping between clock specifier and physical pins== > > When referencing the provided clock in the DT using phandle and > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: > > ==Example== > > +#include <dt-bindings/versaclock.h> > + > /* 25MHz reference crystal */ > ref25: ref25m { > compatible = "fixed-clock"; > @@ -80,6 +101,13 @@ i2c-master-node { > /* Connect XIN input to 25MHz reference */ > clocks = <&ref25m>; > clock-names = "xin"; > + > + ports@1 { 'ports' is already taken as a node name. > + reg = <1>; What do the reg value signify? > + mode = <CMOS>; > + pwr_sel = <1800000>; Not documented. Don't use '-' in property names. > + slew = <P80>; > + }; > }; > }; > > -- > 2.25.1 >
On Sat, Apr 4, 2020 at 8:28 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Mar 26, 2020 at 04:32:51PM -0500, Adam Ford wrote: > > The Versaclock can be purchased in a non-programmed configuration. > > If that is the case, the driver needs to configure the chip to > > output the correct signal type, voltage and slew. > > > > This RFC is proposing an additional binding to allow non-programmed > > chips to be configured beyond their default configuration. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > index 05a245c9df08..4bc46ed9ba4a 100644 > > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > @@ -30,6 +30,25 @@ Required properties: > > - 5p49v5933 and > > - 5p49v5935: (optional) property not present or "clkin". > > > > +For all output ports, an option child node can be used to specify: > > + > > +- mode: can be one of > > + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic > > + - CMOS > > + - HCSL > > + - LVDS: Low voltage differential signal > > + > > +- voltage-level: can be one of the following microvolts > > + - 1800000 > > + - 2500000 > > + - 3300000 > > +- slew: Percent of normal, can be one of > > + - P80 > > + - P85 > > + - P90 > > + - P100 > > + > > + > > ==Mapping between clock specifier and physical pins== > > > > When referencing the provided clock in the DT using phandle and > > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: > > > > ==Example== > > > > +#include <dt-bindings/versaclock.h> > > + > > /* 25MHz reference crystal */ > > ref25: ref25m { > > compatible = "fixed-clock"; > > @@ -80,6 +101,13 @@ i2c-master-node { > > /* Connect XIN input to 25MHz reference */ > > clocks = <&ref25m>; > > clock-names = "xin"; > > + > > + ports@1 { > > 'ports' is already taken as a node name. Rob, The clock chip can drive multiple clocks and each output is independent of the rest. The idea is that port@1 would represent output 1, port@2 would represent output 2, etc. Is there a name you'd think we should use to represent each output? Different variations of this chip can have different number of outputs. > > > + reg = <1>; > > What do the reg value signify? I am fine if we drop we drop it. I was under the assumption that reg =<1> had to correspond to the port@1 and that it was required since other devices with port sub-nodes use the reg entry. > > > + mode = <CMOS>; > > + pwr_sel = <1800000>; > > Not documented. Don't use '-' in property names. Do you have a preference to what name or convention you want us to use? > Thanks for the review. adam > > + slew = <P80>; > > + }; > > }; > > }; > > > > -- > > 2.25.1 > >
On Sat, Apr 4, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> wrote: > > On Sat, Apr 4, 2020 at 8:28 PM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Mar 26, 2020 at 04:32:51PM -0500, Adam Ford wrote: > > > The Versaclock can be purchased in a non-programmed configuration. > > > If that is the case, the driver needs to configure the chip to > > > output the correct signal type, voltage and slew. > > > > > > This RFC is proposing an additional binding to allow non-programmed > > > chips to be configured beyond their default configuration. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > > index 05a245c9df08..4bc46ed9ba4a 100644 > > > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt > > > @@ -30,6 +30,25 @@ Required properties: > > > - 5p49v5933 and > > > - 5p49v5935: (optional) property not present or "clkin". > > > > > > +For all output ports, an option child node can be used to specify: > > > + > > > +- mode: can be one of > > > + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic > > > + - CMOS > > > + - HCSL > > > + - LVDS: Low voltage differential signal > > > + > > > +- voltage-level: can be one of the following microvolts > > > + - 1800000 > > > + - 2500000 > > > + - 3300000 > > > +- slew: Percent of normal, can be one of > > > + - P80 > > > + - P85 > > > + - P90 > > > + - P100 > > > + > > > + > > > ==Mapping between clock specifier and physical pins== > > > > > > When referencing the provided clock in the DT using phandle and > > > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: > > > > > > ==Example== > > > > > > +#include <dt-bindings/versaclock.h> > > > + > > > /* 25MHz reference crystal */ > > > ref25: ref25m { > > > compatible = "fixed-clock"; > > > @@ -80,6 +101,13 @@ i2c-master-node { > > > /* Connect XIN input to 25MHz reference */ > > > clocks = <&ref25m>; > > > clock-names = "xin"; > > > + > > > + ports@1 { > > > > 'ports' is already taken as a node name. > Rob, > > The clock chip can drive multiple clocks and each output is > independent of the rest. The idea is that port@1 would represent > output 1, port@2 would represent output 2, etc. > Is there a name you'd think we should use to represent each output? clock-output@...? > Different variations of this chip can have different number of > outputs. > > > > > > + reg = <1>; > > > > What do the reg value signify? > > I am fine if we drop we drop it. I was under the assumption that reg > =<1> had to correspond to the port@1 and that it was required since > other devices with port sub-nodes use the reg entry. I wasn't suggesting dropping it. Just what 0, 1, 2, etc. corresponds to as you explained above. Just put that into the 'reg' description. > > > + mode = <CMOS>; > > > + pwr_sel = <1800000>; > > > > Not documented. Don't use '-' in property names. > > Do you have a preference to what name or convention you want us to use? Errr, that was supposed to say '_'. Using hyphens is fine. Also, needs a vendor prefix and if that's in microvolts needs a unit suffix. Rob
diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt index 05a245c9df08..4bc46ed9ba4a 100644 --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt @@ -30,6 +30,25 @@ Required properties: - 5p49v5933 and - 5p49v5935: (optional) property not present or "clkin". +For all output ports, an option child node can be used to specify: + +- mode: can be one of + - LVPECL: Low-voltage positive/psuedo emitter-coupled logic + - CMOS + - HCSL + - LVDS: Low voltage differential signal + +- voltage-level: can be one of the following microvolts + - 1800000 + - 2500000 + - 3300000 +- slew: Percent of normal, can be one of + - P80 + - P85 + - P90 + - P100 + + ==Mapping between clock specifier and physical pins== When referencing the provided clock in the DT using phandle and @@ -62,6 +81,8 @@ clock specifier, the following mapping applies: ==Example== +#include <dt-bindings/versaclock.h> + /* 25MHz reference crystal */ ref25: ref25m { compatible = "fixed-clock"; @@ -80,6 +101,13 @@ i2c-master-node { /* Connect XIN input to 25MHz reference */ clocks = <&ref25m>; clock-names = "xin"; + + ports@1 { + reg = <1>; + mode = <CMOS>; + pwr_sel = <1800000>; + slew = <P80>; + }; }; };
The Versaclock can be purchased in a non-programmed configuration. If that is the case, the driver needs to configure the chip to output the correct signal type, voltage and slew. This RFC is proposing an additional binding to allow non-programmed chips to be configured beyond their default configuration. Signed-off-by: Adam Ford <aford173@gmail.com>