| Message ID | 20200903133528.8595-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, thanks for your patch! On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > This adds DT bindings for the Microsemi/Microchip SGPIO controller, What I do not understand is why this GPIO controller is placed in the bindings of the pin controllers? Do you plan to add pin control properties to the bindings in the future? > +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. This doesn't sound like it will ever be pin control? > + gpio-controller: true > + > + '#gpio-cells': > + description: GPIO consumers must specify four arguments, first the > + port number, then the bit number, then a input/output flag and > + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). > + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants > + PIN_INPUT and PIN_OUTPUT. > + const: 4 I do not follow this new third input/output flag at all. - If it is a property of the hardware, it is something the driver should handle by determining which hardware it is from the compatible string. - If it is a configuration it should be turned into something that is generic and useful for *all* GPIO controllers. If it is pin config it should use the pinconf bindings rather than shortcuts like this, but I think it is something the driver can do as an effect of the pin being requested as input or output in the operating system, depending on who the consumer is. Linux for example has GPIOD_OUT_LOW, GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS... - Is it not just a hog? We have bindings for those. > + 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. Arguments must be between 0 and 31. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + minItems: 2 > + maxItems: 64 And you are *absolutely sure* that you can't just figure this out from the compatible string? Or add a few compatible strings for the existing variants? > + 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 I understand why you need this binding now, OK. > +/* mchp-sgpio specific pin type defines */ > +#undef PIN_OUTPUT > +#undef PIN_INPUT > +#define PIN_OUTPUT 0 > +#define PIN_INPUT 1 I'm not a fan of this. It seems like something that should be set in response to the gpiochip callbacks .direction_input and .direction_output callbacks. Yours, Linus Walleij
Linus Walleij writes: > Hi Lars, > > thanks for your patch! You're welcome - thank you for you taking time to review it! > > On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> This adds DT bindings for the Microsemi/Microchip SGPIO controller, > > What I do not understand is why this GPIO controller is placed in the > bindings of the pin controllers? Do you plan to add pin control > properties to the bindings in the future? I have made provisions for some of the generic pinconf parameters, and since the controller also has support for some alternate modes like (syncronized) blink at various rates, I thought I better add it as pinctrl straight away. > >> +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. > > This doesn't sound like it will ever be pin control? above. > >> + gpio-controller: true >> + >> + '#gpio-cells': >> + description: GPIO consumers must specify four arguments, first the >> + port number, then the bit number, then a input/output flag and >> + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). >> + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants >> + PIN_INPUT and PIN_OUTPUT. >> + const: 4 > > I do not follow this new third input/output flag at all. > Its actually a sort of bank address, since the individual "pins" are unidirectional. The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl binding header files... I can drop the define and use, but as it will be used to address individual pins, I think it adds to readability. Like this (excerpts from a DT with a switchdev driver using SFP's and LED's on sgpio): /{ leds { compatible = "gpio-leds"; led@0 { label = "eth60:yellow"; gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; default-state = "off"; }; ... }; }; &axi { sfp_eth60: sfp-eth60 { compatible = "sff,sfp"; i2c-bus = <&i2c152>; tx-disable-gpios = <&sgpio2 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; rate-select0-gpios = <&sgpio2 28 1 PIN_OUTPUT GPIO_ACTIVE_HIGH>; los-gpios = <&sgpio2 28 0 PIN_INPUT GPIO_ACTIVE_HIGH>; mod-def0-gpios = <&sgpio2 28 1 PIN_INPUT GPIO_ACTIVE_LOW>; tx-fault-gpios = <&sgpio2 28 2 PIN_INPUT GPIO_ACTIVE_HIGH>; }; ... }; > - If it is a property of the hardware, it is something the driver should > handle by determining which hardware it is from the compatible > string. > > - If it is a configuration it should be turned into something that is generic > and useful for *all* GPIO controllers. If it is pin config it should use > the pinconf bindings rather than shortcuts like this, but I think it is > something the driver can do as an effect of the pin being requested > as input or output in the operating system, depending on who the > consumer is. Linux for example has GPIOD_OUT_LOW, > GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS... > > - Is it not just a hog? We have bindings for those. I hope the above shed some light on this. > >> + 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. Arguments must be between 0 and 31. >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + minItems: 2 >> + maxItems: 64 > > And you are *absolutely sure* that you can't just figure this out > from the compatible string? Or add a few compatible strings for > the existing variants? > Yes, this really needs to be configured for each board individually - and cant be probed. It defines how the bitstream to/from the shift registers is constructed/demuxed. >> + 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 > > I understand why you need this binding now, OK. > >> +/* mchp-sgpio specific pin type defines */ >> +#undef PIN_OUTPUT >> +#undef PIN_INPUT >> +#define PIN_OUTPUT 0 >> +#define PIN_INPUT 1 > > I'm not a fan of this. It seems like something that should be set in > response to the gpiochip callbacks .direction_input and > .direction_output callbacks. > As I tried to explain above, its a part of the pin address - aka bank selector - whether your are accessing the input or the output side. And since the directions have totally different - and concurrent - use, they need to be individually addressed, not "configured". In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the OUT is the sfp tx-disable control. > Yours, > Linus Walleij ---Lars
Hi Lars, thanks for working on this! On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > > What I do not understand is why this GPIO controller is placed in the > > bindings of the pin controllers? Do you plan to add pin control > > properties to the bindings in the future? > > I have made provisions for some of the generic pinconf parameters, and > since the controller also has support for some alternate modes like > (syncronized) blink at various rates, I thought I better add it as > pinctrl straight away. OK fair enough let's keep the bindings here. BTW the latter function sounds like some kind of PWM? > >> + gpio-controller: true > >> + > >> + '#gpio-cells': > >> + description: GPIO consumers must specify four arguments, first the > >> + port number, then the bit number, then a input/output flag and > >> + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). > >> + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants > >> + PIN_INPUT and PIN_OUTPUT. > >> + const: 4 > > > > I do not follow this new third input/output flag at all. > > Its actually a sort of bank address, since the individual "pins" are > unidirectional. I'm a bit confused here... The standard advice for any "banked" GPIOs is to represent each "bank" as a separate node (with a corresponding gpio_chip in the Linux kernel). Then you can just use the standard bindings to pick a line from one of these nodes. > The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl > binding header files... I can drop the define and use, but as it will be > used to address individual pins, I think it adds to readability. Hmmm. What makes these names expecially confusing is the Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines: input-enable input-disable output-enable output-high output-low In the Linux kernel further there is: include/linux/pinctrl/pinconf-generic.h that defines: PIN_CONFIG_INPUT_ENABLE PIN_CONFIG_OUTPUT_ENABLE PIN_CONFIG_OUTPUT Since you are using the pin control framework this gets really hard to hash out. I don't really understand why it is needed. > Like this (excerpts from a DT with a switchdev driver using SFP's and > LED's on sgpio): > > /{ > leds { > compatible = "gpio-leds"; > led@0 { > label = "eth60:yellow"; > gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; > default-state = "off"; > }; > ... > }; > }; If what you intend to achieve is to make the GPIO come up in output mode, you can either just have the driver do that as needed by the consumer. If you absolutely have to do it in the device tree, then implement pin control (pin config) and have it something like this: leds { compatible = "gpio-leds"; pinctrl-names = "default"; pinctrl-0 = <&my_led_pinctrl>; led@0 { label = "eth60:yellow"; gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>; default-state = "off"; }; ... my_led_pinctrl: pinctrl-led { pins = "gpio95"; // Just an example way of referring to the pin bias-disable; output-enable; }; }; > >> + 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. Arguments must be between 0 and 31. > >> + $ref: /schemas/types.yaml#/definitions/uint32-array > >> + minItems: 2 > >> + maxItems: 64 > > > > And you are *absolutely sure* that you can't just figure this out > > from the compatible string? Or add a few compatible strings for > > the existing variants? > > Yes, this really needs to be configured for each board individually - > and cant be probed. It defines how the bitstream to/from the shift > registers is constructed/demuxed. And you have considered the option of simply letting the driver check which board we are then? The property at the very top of the device tree. if (of_machine_is_compatible("my_board")) { .... } else if (of_machine_is_compatible("my_other_board")) { .... } So that you simply use the board compatible string to determine this? > >> +/* mchp-sgpio specific pin type defines */ > >> +#undef PIN_OUTPUT > >> +#undef PIN_INPUT > >> +#define PIN_OUTPUT 0 > >> +#define PIN_INPUT 1 > > > > I'm not a fan of this. It seems like something that should be set in > > response to the gpiochip callbacks .direction_input and > > .direction_output callbacks. > > > > As I tried to explain above, its a part of the pin address - aka bank > selector - whether your are accessing the input or the output side. And > since the directions have totally different - and concurrent - use, they > need to be individually addressed, not "configured". > > In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the > OUT is the sfp tx-disable control. I suspect the proper way to do it is to create one node for the input side and one node for the output side and also create two different gpio chips in the kernel. my-device { compatible = "my-device"; gpioin: input-gpio { .... }; gpioout: output-gpio { .... }; }; Note: I didn't think over the naming in this example. You will need code in your driver to parse the subnodes and populate two gpio_chips. Yours, Linus Walleij
Linus Walleij writes: > Hi Lars, > > thanks for working on this! > > On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> > What I do not understand is why this GPIO controller is placed in the >> > bindings of the pin controllers? Do you plan to add pin control >> > properties to the bindings in the future? >> >> I have made provisions for some of the generic pinconf parameters, and >> since the controller also has support for some alternate modes like >> (syncronized) blink at various rates, I thought I better add it as >> pinctrl straight away. > > OK fair enough let's keep the bindings here. > > BTW the latter function sounds like some kind of PWM? Yes, it has PWM functionality as well. > >> >> + gpio-controller: true >> >> + >> >> + '#gpio-cells': >> >> + description: GPIO consumers must specify four arguments, first the >> >> + port number, then the bit number, then a input/output flag and >> >> + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). >> >> + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants >> >> + PIN_INPUT and PIN_OUTPUT. >> >> + const: 4 >> > >> > I do not follow this new third input/output flag at all. >> >> Its actually a sort of bank address, since the individual "pins" are >> unidirectional. > > I'm a bit confused here... > The standard advice for any "banked" GPIOs is to represent > each "bank" as a separate node (with a corresponding gpio_chip > in the Linux kernel). Then you can just use the standard > bindings to pick a line from one of these nodes. Yes, that seems to be a good model. > >> The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl >> binding header files... I can drop the define and use, but as it will be >> used to address individual pins, I think it adds to readability. > > Hmmm. What makes these names expecially confusing is the > Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines: > input-enable > input-disable > output-enable > output-high > output-low > > In the Linux kernel further there is: > include/linux/pinctrl/pinconf-generic.h that defines: > PIN_CONFIG_INPUT_ENABLE > PIN_CONFIG_OUTPUT_ENABLE > PIN_CONFIG_OUTPUT > > Since you are using the pin control framework this gets really > hard to hash out. > Yes, as the pins are fixed-function, the "input-enable", "input-disable" and "output-enable" are not really useful. > I don't really understand why it is needed. > >> Like this (excerpts from a DT with a switchdev driver using SFP's and >> LED's on sgpio): >> >> /{ >> leds { >> compatible = "gpio-leds"; >> led@0 { >> label = "eth60:yellow"; >> gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; >> default-state = "off"; >> }; >> ... >> }; >> }; > > If what you intend to achieve is to make the GPIO come up in output mode, > you can either just have the driver do that as needed by the consumer. > If you absolutely have to do it in the device tree, then implement > pin control (pin config) and have it something like this: > > leds { > compatible = "gpio-leds"; > pinctrl-names = "default"; > pinctrl-0 = <&my_led_pinctrl>; > led@0 { > label = "eth60:yellow"; > gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>; > default-state = "off"; > }; > ... > > my_led_pinctrl: pinctrl-led { > pins = "gpio95"; // Just an example way of referring to the pin > bias-disable; > output-enable; > }; > }; No, the PIN_OUTPUT is purely for adressing. But as you suggested, I'll split the into separate nodes. That will eliminate the "PIN_OUTPUT" and the bindings header. > >> >> + 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. Arguments must be between 0 and 31. >> >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> >> + minItems: 2 >> >> + maxItems: 64 >> > >> > And you are *absolutely sure* that you can't just figure this out >> > from the compatible string? Or add a few compatible strings for >> > the existing variants? >> >> Yes, this really needs to be configured for each board individually - >> and cant be probed. It defines how the bitstream to/from the shift >> registers is constructed/demuxed. > > And you have considered the option of simply letting the driver > check which board we are then? The property at the very > top of the device tree. > > if (of_machine_is_compatible("my_board")) { > .... > } else if (of_machine_is_compatible("my_other_board")) { > .... > } No, board-specific code is undesireable, as our customers should be able to design own boards without driver changes. > > So that you simply use the board compatible string to determine > this? > >> >> +/* mchp-sgpio specific pin type defines */ >> >> +#undef PIN_OUTPUT >> >> +#undef PIN_INPUT >> >> +#define PIN_OUTPUT 0 >> >> +#define PIN_INPUT 1 >> > >> > I'm not a fan of this. It seems like something that should be set in >> > response to the gpiochip callbacks .direction_input and >> > .direction_output callbacks. >> > >> >> As I tried to explain above, its a part of the pin address - aka bank >> selector - whether your are accessing the input or the output side. And >> since the directions have totally different - and concurrent - use, they >> need to be individually addressed, not "configured". >> >> In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the >> OUT is the sfp tx-disable control. > > I suspect the proper way to do it is to create one node for > the input side and one node for the output side and also create > two different gpio chips in the kernel. > > my-device { > compatible = "my-device"; > gpioin: input-gpio { > .... > }; > gpioout: output-gpio { > .... > }; > }; > > Note: I didn't think over the naming in this example. > > You will need code in your driver to parse the subnodes and > populate two gpio_chips. Yes, I will modify the driver to use separate nodes for each direction. Thank you for your comments, it is highly appreciated. ---Lars > > Yours, > Linus Walleij
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..25a50ba9e893 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml @@ -0,0 +1,88 @@ +# 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" + + compatible: + enum: + - microchip,sparx5-sgpio + - mscc,ocelot-sgpio + - mscc,luton-sgpio + + clocks: + maxItems: 1 + + gpio-controller: true + + '#gpio-cells': + description: GPIO consumers must specify four arguments, first the + port number, then the bit number, then a input/output flag and + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants + PIN_INPUT and PIN_OUTPUT. + const: 4 + + gpio-ranges: + 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. Arguments must be between 0 and 31. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 64 + + 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 + +required: + - compatible + - clocks + - gpio-controller + - '#gpio-cells' + - gpio-ranges + - microchip,sgpio-port-ranges + +examples: + - | + sgpio2: gpio@1101036c { + compatible = "microchip,sparx5-sgpio"; + pinctrl-0 = <&sgpio0_pins>; + pinctrl-names = "default"; + reg = <0x1101036c 0x100>; + clocks = <&sys_clk>; + gpio-controller; + gpio-ranges = <&sgpio0 0 0 192>; + #gpio-cells = <4>; + microchip,sgpio-port-ranges = <0 0 16 18 28 31>; + microchip,sgpio-frequency = <25000000>; + }; diff --git a/include/dt-bindings/gpio/mchp-sgpio.h b/include/dt-bindings/gpio/mchp-sgpio.h new file mode 100644 index 000000000000..61b8aa75d870 --- /dev/null +++ b/include/dt-bindings/gpio/mchp-sgpio.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * This header provides constants for binding the Microchip SGPIO + * driver. + * + */ + +#ifndef _DT_BINDINGS_GPIO_MCHP_SGPIO_H +#define _DT_BINDINGS_GPIO_MCHP_SGPIO_H + +#include <dt-bindings/gpio/gpio.h> + +/* mchp-sgpio specific pin type defines */ +#undef PIN_OUTPUT +#undef PIN_INPUT +#define PIN_OUTPUT 0 +#define PIN_INPUT 1 + +#endif
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 | 88 +++++++++++++++++++ include/dt-bindings/gpio/mchp-sgpio.h | 19 ++++ 2 files changed, 107 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml create mode 100644 include/dt-bindings/gpio/mchp-sgpio.h -- 2.27.0