[PATCH] mips: gpio: mscc_sgpio: Add DT bindings documentation

[PATCH] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Lars Povlsen]

Hello device tree!

I am submitting this device tree driver binding documentation for review
comments.

The driver is currently implemented on u-boot, but will also eventually
be upstreamed to Linux.

By using a serial interface, the MSCC SIO controller significantly
extends the number of available GPIOs with a minimum number of
additional pins on the device. The primary purpose of the SIO controller
is to connect control signals from SFP modules and to act as an LED
controller.

This add device tree binding documentation for the MSCC serial GPIO
driver.

Signed-off-by: Lars Povlsen <lars.povlsen@microsemi.com>
---
 doc/device-tree-bindings/gpio/mscc_sgpio.txt | 39 ++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 doc/device-tree-bindings/gpio/mscc_sgpio.txt

diff --git a/doc/device-tree-bindings/gpio/mscc_sgpio.txt b/doc/device-tree-bindings/gpio/mscc_sgpio.txt
new file mode 100644
index 0000000000..24e6aa345e
--- /dev/null
+++ b/doc/device-tree-bindings/gpio/mscc_sgpio.txt
@@ -0,0 +1,39 @@
+MSCC Serial GPIO driver
+
+Required properties:
+- compatible : "mscc,luton-sgpio" or "mscc,ocelot-sgpio"
+- clock: Reference clock used to generate clock divider setting. See
+  mscc,sgpio-frequency property.
+- reg : Physical base address and length of the controller's registers.
+- #gpio-cells : Should be two. The first cell is the pin number and the
+  second cell is used to specify optional parameters:
+  - bit 0 specifies polarity (0 for normal, 1 for inverted)
+- gpio-controller : Marks the device node as a GPIO controller.
+- gpio-ranges: The 4th cell will typically be 32*<sgpio-bitcount>
+
+Optional properties:
+- mscc,sgpio-frequency: The frequency at which the serial bitstream is
+  generated and sampled. Default: 12500000 (Hz).
+- mscc,sgpio-bitcount: The number of bits enabled per
+  port. (1-4). Default: 2.
+- mscc,sgpio-ports: A bitmask (32 bits) of which ports are enabled in
+  the serialized gpio stream. Default: 0xFFFFFFFF.
+
+Typically the pinctrl-0 and pinctrl-names properties will also be
+present to enable the use of the SIO CLK, LD, DI and DO for some
+regular GPIO pins.
+
+Example:
+
+sgpio: gpio@10700f8 {
+       compatible = "mscc,ocelot-sgpio";
+       pinctrl-0 = <&sgpio_pins>;
+       pinctrl-names = "default";
+       reg = <0x10700f8 0x100>;
+       gpio-controller;
+       #gpio-cells = <2>;
+       gpio-ranges = <&sgpio 0 0 64>; // 32*<sgpio-bitcount>
+       mscc,sgpio-frequency = <12500>;
+       mscc,sgpio-bitcount = <2>;
+       mscc,sgpio-ports = <0x000FFFFF>;
+};
-- 
2.19.1

Re: [PATCH] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Linus Walleij]

Hi Lars,

thanks for your patch!

On Mon, Nov 26, 2018 at 1:22 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> +MSCC Serial GPIO driver

Expand the acronym please, if IIUC this is
"Microsemi Corporation (MSCC) Serial GPIO driver"

> +- mscc,sgpio-frequency: The frequency at which the serial bitstream is
> +  generated and sampled. Default: 12500000 (Hz).

Uhm what is a serial bitstream in this context, I am lost.
Please elaborate.

> +- mscc,sgpio-bitcount: The number of bits enabled per
> +  port. (1-4). Default: 2.

Can't you just use the standard "ngpios" property for this?
See
Documentation/devicetree/bindings/gpio/gpio.txt

Why is this a global setting for all ports and not per-port?

> +- mscc,sgpio-ports: A bitmask (32 bits) of which ports are enabled in
> +  the serialized gpio stream. Default: 0xFFFFFFFF.

I don't understand this either. Are there up to 32 ports?
(Also referred to as "banks") with 1-4 GPIOs each?

Yours,
Linus Walleij

Re: [PATCH] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Rob Herring]

On Fri, Dec 7, 2018 at 3:43 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Lars,
>
> thanks for your patch!
>
> On Mon, Nov 26, 2018 at 1:22 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
> > +MSCC Serial GPIO driver
>
> Expand the acronym please, if IIUC this is
> "Microsemi Corporation (MSCC) Serial GPIO driver"
>
> > +- mscc,sgpio-frequency: The frequency at which the serial bitstream is
> > +  generated and sampled. Default: 12500000 (Hz).
>
> Uhm what is a serial bitstream in this context, I am lost.
> Please elaborate.

Is this SGPIO as in this[1]? If so, then I don't think using the GPIO
binding is appropriate here. SGPIO is pretty fixed use. There is a
bitbanged SGPIO implementation in the Calxeda SATA driver BTW.

Rob

[1] https://en.wikipedia.org/wiki/SGPIO

RE: [PATCH] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Lars.Povlsen]

Hi Rob!

Sorry about the delay in response, the mail got lost in the sheer volume
of the devicetree list.

I appreciate your response, see below.

---Lars

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, December 7, 2018 14:54
> To: Linus Walleij <linus.walleij@linaro.org>; Lars Povlsen - M31675
> <Lars.Povlsen@microchip.com>
> Cc: devicetree@vger.kernel.org; Gregory CLEMENT
> <gregory.clement@bootlin.com>
> Subject: Re: [PATCH] mips: gpio: mscc_sgpio: Add DT bindings
> documentation
> 
> On Fri, Dec 7, 2018 at 3:43 AM Linus Walleij <linus.walleij@linaro.org>
> wrote:
> >
> > Hi Lars,
> >
> > thanks for your patch!
> >
> > On Mon, Nov 26, 2018 at 1:22 PM Lars Povlsen
> <lars.povlsen@microchip.com> wrote:
> >
> > > +MSCC Serial GPIO driver
> >
> > Expand the acronym please, if IIUC this is
> > "Microsemi Corporation (MSCC) Serial GPIO driver"
> >
> > > +- mscc,sgpio-frequency: The frequency at which the serial bitstream
> is
> > > +  generated and sampled. Default: 12500000 (Hz).
> >
> > Uhm what is a serial bitstream in this context, I am lost.
> > Please elaborate.
> 
> Is this SGPIO as in this[1]? If so, then I don't think using the GPIO
> binding is appropriate here. SGPIO is pretty fixed use. There is a
> bitbanged SGPIO implementation in the Calxeda SATA driver BTW.
> 

I was not aware of this [1] interface for HDD attachment. There is some similarity
in the electrical interface, but this device is specifically for extending GPIO
capacity on a SOC which is pin-limited. So I insist that the GPIO binding is highly
appropriate here.

> Rob
> 
> [1] https://en.wikipedia.org/wiki/SGPIO

[PATCH v2] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Lars Povlsen]

    mips: mscc_sgpio: Add DT bindings documentation
    
    This add device tree binding documentation for the MSCC serial GPIO
    driver.
    
v2 changes:
 - Added more device background info
 - Dropped "mscc,sgpio-bitcount" in favour of "ngpios" and "gpio-ranges".
 - Elaborated "mscc,sgpio-ports" explanation


    Signed-off-by: Lars Povlsen <lars.povlsen@microsemi.com>

diff --git a/doc/device-tree-bindings/gpio/mscc_sgpio.txt b/doc/device-tree-bindings/gpio/mscc_sgpio.txt
new file mode 100644
index 0000000000..2db2d277a5
--- /dev/null
+++ b/doc/device-tree-bindings/gpio/mscc_sgpio.txt
@@ -0,0 +1,45 @@
+Microsemi Corporation (MSCC) Serial GPIO driver
+
+The MSCC serial GPIO extends the number or GPIO's on the system by
+means of 4 dedicated pins: one input, one output, one clock and one
+strobe pin. By attaching a number of (external) shift registers, the
+effective GPIO count can be extended by up to 128 GPIO's per
+controller.
+
+Required properties:
+- compatible : "mscc,luton-sgpio" or "mscc,ocelot-sgpio"
+- clock: Reference clock used to generate clock divider setting. See
+  mscc,sgpio-frequency property.
+- reg : Physical base address and length of the controller's registers.
+- #gpio-cells : Should be two. The first cell is the pin number and the
+  second cell is used to specify optional parameters:
+  - bit 0 specifies polarity (0 for normal, 1 for inverted)
+- gpio-controller : Marks the device node as a GPIO controller.
+- gpio-ranges: Standard gpio range(s): phandle, gpio base, pinctrl base
+  and count.
+
+Optional properties:
+- ngpios: Set the number of GPIO's actually used.
+- mscc,sgpio-frequency: The frequency at which the serial bitstream is
+  generated and sampled. Default: 12500000 (Hz).
+- mscc,sgpio-ports: A bitmask (32 bits) of which ports are enabled in
+  the serialized gpio stream. One 'port' will transport from 1 to 4
+  gpio bits. Default: 0xFFFFFFFF.
+
+Typically the pinctrl-0 and pinctrl-names properties will also be
+present to enable the use of the SIO CLK, LD, DI and DO for some
+regular GPIO pins.
+
+Example:
+
+sgpio: gpio@10700f8 {
+       compatible = "mscc,ocelot-sgpio";
+       pinctrl-0 = <&sgpio_pins>;
+       pinctrl-names = "default";
+       reg = <0x10700f8 0x100>;
+       gpio-controller;
+       #gpio-cells = <2>;
+       gpio-ranges = <&sgpio 0 0 64>;
+       mscc,sgpio-frequency = <12500>;
+       mscc,sgpio-ports = <0x000FFFFF>;
+};

Re: [PATCH v2] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Linus Walleij]

On Thu, Dec 27, 2018 at 2:12 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

>     mips: mscc_sgpio: Add DT bindings documentation
>
>     This add device tree binding documentation for the MSCC serial GPIO
>     driver.
>
> v2 changes:
>  - Added more device background info
>  - Dropped "mscc,sgpio-bitcount" in favour of "ngpios" and "gpio-ranges".
>  - Elaborated "mscc,sgpio-ports" explanation
>
>
>     Signed-off-by: Lars Povlsen <lars.povlsen@microsemi.com>

Overall looks good to me!
Acked-by: Linus Walleij <linus.walleij@linaro.org>

> +Optional properties:
> +- ngpios: Set the number of GPIO's actually used.

Just say see: gpio.txt

Yours,
Linus Walleij

RE: [PATCH v2] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Lars.Povlsen]

Hi Linus!

Thank you for your input, I will refer to gpio.txt
in the next version.

Cheers,

---Lars

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Sunday, December 30, 2018 21:00
> To: Lars Povlsen - M31675 <Lars.Povlsen@microchip.com>
> Cc: Device Tree <devicetree@vger.kernel.org>; Gregory Clement
> <gregory.clement@bootlin.com>; Rob Herring <robh+dt@kernel.org>
> Subject: Re: [PATCH v2] mips: gpio: mscc_sgpio: Add DT bindings
> documentation
> 
> On Thu, Dec 27, 2018 at 2:12 PM Lars Povlsen
> <lars.povlsen@microchip.com> wrote:
> 
> >     mips: mscc_sgpio: Add DT bindings documentation
> >
> >     This add device tree binding documentation for the MSCC serial
> GPIO
> >     driver.
> >
> > v2 changes:
> >  - Added more device background info
> >  - Dropped "mscc,sgpio-bitcount" in favour of "ngpios" and "gpio-
> ranges".
> >  - Elaborated "mscc,sgpio-ports" explanation
> >
> >
> >     Signed-off-by: Lars Povlsen <lars.povlsen@microsemi.com>
> 
> Overall looks good to me!
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> > +Optional properties:
> > +- ngpios: Set the number of GPIO's actually used.
> 
> Just say see: gpio.txt
> 
> Yours,
> Linus Walleij

Re: [PATCH] mips: gpio: mscc_sgpio: Add DT bindings documentation

[Lars.Povlsen]

Hi Linus!

Thank you very much for your feedback - and sorry about the delay in 
getting back to you.

I will send you a v2 updated version addressing you comments. Thank you!

See below for comments

---Lars

> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Fri, 7 Dec 2018 10:43:38 +0100
> Message-ID: <CACRpkdZ47tELuJR3DG-
> d8tsvz4ez_j6TxCYiiGT3L01CJ1LSTA@mail.gmail.com>
> Subject: Re: [PATCH] mips: gpio: mscc_sgpio: Add DT bindings
> documentation
> To: lars.povlsen@microchip.com
> Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
> <devicetree@vger.kernel.org>,
> 	Gregory Clement <gregory.clement@bootlin.com>
> 
> Hi Lars,
> 
> thanks for your patch!
> 
> On Mon, Nov 26, 2018 at 1:22 PM Lars Povlsen
> <lars.povlsen@microchip.com> wrote:
> 
> > +MSCC Serial GPIO driver
> 
> Expand the acronym please, if IIUC this is
> "Microsemi Corporation (MSCC) Serial GPIO driver"

Will do.

> 
> > +- mscc,sgpio-frequency: The frequency at which the serial bitstream
> is
> > +  generated and sampled. Default: 12500000 (Hz).
> 
> Uhm what is a serial bitstream in this context, I am lost.
> Please elaborate.

I added some background details to the description.

> 
> > +- mscc,sgpio-bitcount: The number of bits enabled per
> > +  port. (1-4). Default: 2.
> 
> Can't you just use the standard "ngpios" property for this?
> See
> Documentation/devicetree/bindings/gpio/gpio.txt
> 

I dropped this property in favor of deriving from gpio-ranges and
(optionally) ngpios as proposed.

> Why is this a global setting for all ports and not per-port?
> 
> > +- mscc,sgpio-ports: A bitmask (32 bits) of which ports are enabled in
> > +  the serialized gpio stream. Default: 0xFFFFFFFF.
> 
> I don't understand this either. Are there up to 32 ports?
> (Also referred to as "banks") with 1-4 GPIOs each?

Yes each port represent 1-4 GPIOs. Changed to:

- mscc,sgpio-ports: A bitmask (32 bits) of which ports are enabled in
  the serialized gpio stream. One 'port' will transport from 1 to 4
  gpio bits. Default: 0xFFFFFFFF.


Again, thank you very much for your comments - they are highly appreciated.

---Lars

> 
> Yours,
> Linus Walleij
> -------------------- End of forwarded message --------------------
> 
> --
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com