| Message ID | 20201008130515.2385825-2-lars.povlsen@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: Adding support for Microchip/Microsemi serial GPIO controller | expand |
Hi Lars! This is overall looking fine. Except for the 3 cell business. I just can't wrap my head around why that is needed. On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > + '#gpio-cells': > + const: 3 So at the very least needs a description making it crystal clear why each cell is needed, and used for since the standard bindings are not used. + sgpio_in2: gpio@0 { + reg = <0>; + compatible = "microchip,sparx5-sgpio-bank"; + gpio-controller; + #gpio-cells = <3>; + ngpios = <96>; + }; So here reg = 0 and the out port has reg 1. Isn't that what you also put in the second cell of the GPIO phandle? Then why? The driver can very well just parse its own reg property and fill that in. When you obtain a phandle like that: gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; Isn't that 0 just duplicating the "reg"? Just parse reg when you set up your driver state and put it as variable in the state container for your driver state for this particular gpio_chip. No need to get it from the phandle. Yours, Linus Walleij
Linus Walleij writes: > Hi Lars! > > This is overall looking fine. Except for the 3 cell business. I just can't > wrap my head around why that is needed. > > On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> + '#gpio-cells': >> + const: 3 > > So at the very least needs a description making it crystal clear why each > cell is needed, and used for since the standard bindings are not used. > > + sgpio_in2: gpio@0 { > + reg = <0>; > + compatible = "microchip,sparx5-sgpio-bank"; > + gpio-controller; > + #gpio-cells = <3>; > + ngpios = <96>; > + }; > > So here reg = 0 and the out port has reg 1. Isn't that what you also put > in the second cell of the GPIO phandle? Then why? The driver > can very well just parse its own reg property and fill that in. Linus, NO! The second cell is the second dimension - NOT the direction. As I wrote previously, the direction is now inherent from the handle, ie. the "reg" value of the handle. The hardware describe a "port" and a "bit index" addressing, where the second cell in gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; is the "bit index" - not the "reg" from the phandle. In the example above, note ngpios = <96>; As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the (input) GPIO cells will be: p0b0, p0b1, p0b2 ... p31b0, p31b1, p31b2 being identical to <&sgpio_inX 0 0 GPIO_OUT_LOW> <&sgpio_inX 0 1 GPIO_OUT_LOW> <&sgpio_inX 0 2 GPIO_OUT_LOW> ... <&sgpio_inX 31 0 GPIO_OUT_LOW> <&sgpio_inX 31 1 GPIO_OUT_LOW> <&sgpio_inX 31 2 GPIO_OUT_LOW> ('X' being the SGPIO controller instance). So no, there *really* is a need for a 3-cell GPIO specifier (or whatever its called). Hope this is clearer now... ---Lars > > When you obtain a phandle like that: > > gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; > > Isn't that 0 just duplicating the "reg"? Just parse reg when you set up > your driver state and put it as variable in the state container for your > driver state for this particular gpio_chip. No need to get it from > the phandle. > > Yours, > Linus Walleij
On Thu, Oct 08, 2020 at 03:05:13PM +0200, Lars Povlsen wrote: > This adds DT bindings for the Microsemi/Microchip SGPIO controller, > bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and > mscc,luton-sgpio. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > --- > .../pinctrl/microchip,sparx5-sgpio.yaml | 140 ++++++++++++++++++ > 1 file changed, 140 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml > new file mode 100644 > index 000000000000..fc41495800ed > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml > @@ -0,0 +1,140 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/microchip,sparx5-sgpio.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microsemi/Microchip Serial GPIO controller > + > +maintainers: > + - Lars Povlsen <lars.povlsen@microchip.com> > + > +description: | > + By using a serial interface, the SIO controller significantly extend > + the number of available GPIOs with a minimum number of additional > + pins on the device. The primary purpose of the SIO controllers is to > + connect control signals from SFP modules and to act as an LED > + controller. > + > +properties: > + $nodename: > + pattern: "^gpio@[0-9a-f]+$" > + > + compatible: > + enum: > + - microchip,sparx5-sgpio > + - mscc,ocelot-sgpio > + - mscc,luton-sgpio > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + microchip,sgpio-port-ranges: > + description: This is a sequence of tuples, defining intervals of > + enabled ports in the serial input stream. The enabled ports must > + match the hardware configuration in order for signals to be > + properly written/read to/from the controller holding > + registers. Being tuples, then number of arguments must be > + even. The tuples mast be ordered (low, high) and are > + inclusive. > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > + items: > + items: > + - description: | > + "low" indicates start bit number of range > + minimum: 0 > + maximum: 31 > + - description: | > + "high" indicates end bit number of range > + minimum: 0 > + maximum: 31 > + minItems: 1 > + maxItems: 32 > + > + microchip,sgpio-frequency: > + description: The sgpio controller frequency (Hz). This dictates > + the serial bitstream speed, which again affects the latency in > + getting control signals back and forth between external shift > + registers. The speed must be no larger than half the system > + clock, and larger than zero. > + $ref: /schemas/types.yaml#/definitions/uint32 Perhaps use 'bus-frequency' here. If not, needs unit suffix. Either way, you can drop the type in those cases. With that, Reviewed-by: Rob Herring <robh@kernel.org> > + minimum: 1 > + default: 12500000 > + > +patternProperties: > + "^gpio@[0-1]$": > + type: object > + properties: > + compatible: > + const: microchip,sparx5-sgpio-bank > + > + reg: > + description: | > + The GPIO bank number. "0" is designates the input pin bank, > + "1" the output bank. > + maxItems: 1 > + > + gpio-controller: true > + > + '#gpio-cells': > + const: 3 > + > + ngpios: > + minimum: 1 > + maximum: 128 > + > + required: > + - compatible > + - reg > + - gpio-controller > + - '#gpio-cells' > + - ngpios > + > + additionalProperties: false > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + - clocks > + - microchip,sgpio-port-ranges > + - "#address-cells" > + - "#size-cells" > + > +examples: > + - | > + sgpio2: gpio@1101059c { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "microchip,sparx5-sgpio"; > + clocks = <&sys_clk>; > + pinctrl-0 = <&sgpio2_pins>; > + pinctrl-names = "default"; > + reg = <0x1101059c 0x100>; > + microchip,sgpio-port-ranges = <0 0>, <16 18>, <28 31>; > + microchip,sgpio-frequency = <25000000>; > + sgpio_in2: gpio@0 { > + reg = <0>; > + compatible = "microchip,sparx5-sgpio-bank"; > + gpio-controller; > + #gpio-cells = <3>; > + ngpios = <96>; > + }; > + sgpio_out2: gpio@1 { > + compatible = "microchip,sparx5-sgpio-bank"; > + reg = <1>; > + gpio-controller; > + #gpio-cells = <3>; > + ngpios = <96>; > + }; > + }; > -- > 2.25.1
On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > > So here reg = 0 and the out port has reg 1. Isn't that what you also put > > in the second cell of the GPIO phandle? Then why? The driver > > can very well just parse its own reg property and fill that in. > > NO! The second cell is the second dimension - NOT the direction. As I > wrote previously, the direction is now inherent from the handle, ie. the > "reg" value of the handle. OK I get it ... I think :) > The hardware describe a "port" and a "bit index" addressing, where the > second cell in > > gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; > > is the "bit index" - not the "reg" from the phandle. As long as the bindings specify exactly what is meant by bit index and the tupe (port, bit_index) is what uniquely addresses a certain GPIO line then it is fine I suppose. > In the example above, note > > ngpios = <96>; > > As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the > (input) GPIO cells will be: > > p0b0, p0b1, p0b2 > ... > p31b0, p31b1, p31b2 > > being identical to > > <&sgpio_inX 0 0 GPIO_OUT_LOW> > <&sgpio_inX 0 1 GPIO_OUT_LOW> > <&sgpio_inX 0 2 GPIO_OUT_LOW> > ... > <&sgpio_inX 31 0 GPIO_OUT_LOW> > <&sgpio_inX 31 1 GPIO_OUT_LOW> > <&sgpio_inX 31 2 GPIO_OUT_LOW> > > ('X' being the SGPIO controller instance). So 32 possible ports with 3 possible bit indexes on each? This constraint should go into the bindings as well so it becomes impossible to put in illegal port numbers or bit indices. (Use the YAML min/max constraints, I suppose?) > So no, there *really* is a need for a 3-cell GPIO specifier (or whatever > its called). If that is the natural way to address the hardware lines and what is used in the documentation then it's fine, it's just so unorthodox that I have to push back on it a bit you know. Yours, Linus Walleij
Linus Walleij writes: > On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> > So here reg = 0 and the out port has reg 1. Isn't that what you also put >> > in the second cell of the GPIO phandle? Then why? The driver >> > can very well just parse its own reg property and fill that in. >> >> NO! The second cell is the second dimension - NOT the direction. As I >> wrote previously, the direction is now inherent from the handle, ie. the >> "reg" value of the handle. > > OK I get it ... I think :) Great! > >> The hardware describe a "port" and a "bit index" addressing, where the >> second cell in >> >> gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>; >> >> is the "bit index" - not the "reg" from the phandle. > > As long as the bindings specify exactly what is meant by bit index > and the tupe (port, bit_index) is what uniquely addresses a certain > GPIO line then it is fine I suppose. > Yes, that is confirmed. >> In the example above, note >> >> ngpios = <96>; >> >> As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the >> (input) GPIO cells will be: >> >> p0b0, p0b1, p0b2 >> ... >> p31b0, p31b1, p31b2 >> >> being identical to >> >> <&sgpio_inX 0 0 GPIO_OUT_LOW> >> <&sgpio_inX 0 1 GPIO_OUT_LOW> >> <&sgpio_inX 0 2 GPIO_OUT_LOW> >> ... >> <&sgpio_inX 31 0 GPIO_OUT_LOW> >> <&sgpio_inX 31 1 GPIO_OUT_LOW> >> <&sgpio_inX 31 2 GPIO_OUT_LOW> >> >> ('X' being the SGPIO controller instance). > > So 32 possible ports with 3 possible bit indexes on each? > This constraint should go into the bindings as well so it becomes > impossible to put in illegal port numbers or bit indices. > > (Use the YAML min/max constraints, I suppose?) > Yes, I will to see if constraints in the GPIO args is possible. >> So no, there *really* is a need for a 3-cell GPIO specifier (or whatever >> its called). > > If that is the natural way to address the hardware lines > and what is used in the documentation then it's fine, it's just so > unorthodox that I have to push back on it a bit you know. > Yes, this piece of hw is certainly not a stock GPIO controller, so that was kinda expected. But I think we ended up with an abstraction that fits as good as possible. I will send a new (last?) revision that includes the suggestions from Rob tomorrow. Thank you for your time and comments (also Rob!) ---Lars
diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml new file mode 100644 index 000000000000..fc41495800ed --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml @@ -0,0 +1,140 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/microchip,sparx5-sgpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microsemi/Microchip Serial GPIO controller + +maintainers: + - Lars Povlsen <lars.povlsen@microchip.com> + +description: | + By using a serial interface, the SIO controller significantly extend + the number of available GPIOs with a minimum number of additional + pins on the device. The primary purpose of the SIO controllers is to + connect control signals from SFP modules and to act as an LED + controller. + +properties: + $nodename: + pattern: "^gpio@[0-9a-f]+$" + + compatible: + enum: + - microchip,sparx5-sgpio + - mscc,ocelot-sgpio + - mscc,luton-sgpio + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + microchip,sgpio-port-ranges: + description: This is a sequence of tuples, defining intervals of + enabled ports in the serial input stream. The enabled ports must + match the hardware configuration in order for signals to be + properly written/read to/from the controller holding + registers. Being tuples, then number of arguments must be + even. The tuples mast be ordered (low, high) and are + inclusive. + $ref: /schemas/types.yaml#/definitions/uint32-matrix + items: + items: + - description: | + "low" indicates start bit number of range + minimum: 0 + maximum: 31 + - description: | + "high" indicates end bit number of range + minimum: 0 + maximum: 31 + minItems: 1 + maxItems: 32 + + microchip,sgpio-frequency: + description: The sgpio controller frequency (Hz). This dictates + the serial bitstream speed, which again affects the latency in + getting control signals back and forth between external shift + registers. The speed must be no larger than half the system + clock, and larger than zero. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + default: 12500000 + +patternProperties: + "^gpio@[0-1]$": + type: object + properties: + compatible: + const: microchip,sparx5-sgpio-bank + + reg: + description: | + The GPIO bank number. "0" is designates the input pin bank, + "1" the output bank. + maxItems: 1 + + gpio-controller: true + + '#gpio-cells': + const: 3 + + ngpios: + minimum: 1 + maximum: 128 + + required: + - compatible + - reg + - gpio-controller + - '#gpio-cells' + - ngpios + + additionalProperties: false + +additionalProperties: false + +required: + - compatible + - reg + - clocks + - microchip,sgpio-port-ranges + - "#address-cells" + - "#size-cells" + +examples: + - | + sgpio2: gpio@1101059c { + #address-cells = <1>; + #size-cells = <0>; + compatible = "microchip,sparx5-sgpio"; + clocks = <&sys_clk>; + pinctrl-0 = <&sgpio2_pins>; + pinctrl-names = "default"; + reg = <0x1101059c 0x100>; + microchip,sgpio-port-ranges = <0 0>, <16 18>, <28 31>; + microchip,sgpio-frequency = <25000000>; + sgpio_in2: gpio@0 { + reg = <0>; + compatible = "microchip,sparx5-sgpio-bank"; + gpio-controller; + #gpio-cells = <3>; + ngpios = <96>; + }; + sgpio_out2: gpio@1 { + compatible = "microchip,sparx5-sgpio-bank"; + reg = <1>; + gpio-controller; + #gpio-cells = <3>; + ngpios = <96>; + }; + };
This adds DT bindings for the Microsemi/Microchip SGPIO controller, bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and mscc,luton-sgpio. Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> --- .../pinctrl/microchip,sparx5-sgpio.yaml | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml -- 2.25.1