diff mbox series

[1/3] dt-bindings: pinctrl: Add bindings for mscc,ocelot-sgpio

Message ID 20200513141134.25819-2-lars.povlsen@microchip.com (mailing list archive)
State Not Applicable
Headers show
Series pinctrl: Adding support for Microchip serial GPIO controller | expand

Commit Message

Lars Povlsen May 13, 2020, 2:11 p.m. UTC 
This adds DT bindings for the Microsemi SGPIO controller, bindings
mscc,ocelot-sgpio and mscc,luton-sgpio.

Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 .../bindings/pinctrl/mscc,ocelot-sgpio.yaml   | 66 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 include/dt-bindings/gpio/mchp-sgpio.h         | 21 ++++++
 3 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/mscc,ocelot-sgpio.yaml
 create mode 100644 include/dt-bindings/gpio/mchp-sgpio.h

--
2.26.2

Comments

Linus Walleij May 18, 2020, 7:40 a.m. UTC | #1 
On Wed, May 13, 2020 at 4:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> This adds DT bindings for the Microsemi SGPIO controller, bindings
> mscc,ocelot-sgpio and mscc,luton-sgpio.
>
> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>

> +  microchip,sgpio-ports:
> +    description: This is a 32-bit bitmask, configuring whether a
> +      particular port in the controller is enabled or not. This allows
> +      unused ports to be removed from the bitstream and reduce latency.
> +    $ref: "/schemas/types.yaml#/definitions/uint32"

I don't know about this.

You are saying this pin controller can have up to 32 GPIO "ports"
(also known as banks).

Why can't you just represent each such port as a separate GPIO
node:

pinctrl@nnn {
    gpio@0 {
        ....
    };
    gpio@1 {
        ....
    };
    ....
    gpio@31 {
        ....
    };
};

Then if some of them are unused just set it to status = "disabled";

This also makes your Linux driver simpler because each GPIO port
just becomes a set of 32bit registers and you can use
select GPIO_GENERIC and bgpio_init() and save a whole
slew of standard stock code.

Yours,
Linus Walleij
Lars Povlsen May 18, 2020, 8:49 p.m. UTC | #2 
Linus Walleij writes:

> On Wed, May 13, 2020 at 4:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> This adds DT bindings for the Microsemi SGPIO controller, bindings
>> mscc,ocelot-sgpio and mscc,luton-sgpio.
>>
>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>
>> +  microchip,sgpio-ports:
>> +    description: This is a 32-bit bitmask, configuring whether a
>> +      particular port in the controller is enabled or not. This allows
>> +      unused ports to be removed from the bitstream and reduce latency.
>> +    $ref: "/schemas/types.yaml#/definitions/uint32"
>
> I don't know about this.
>
> You are saying this pin controller can have up to 32 GPIO "ports"
> (also known as banks).
>
> Why can't you just represent each such port as a separate GPIO
> node:
>
> pinctrl@nnn {
>     gpio@0 {
>         ....
>     };
>     gpio@1 {
>         ....
>     };
>     ....
>     gpio@31 {
>         ....
>     };
> };
>
> Then if some of them are unused just set it to status = "disabled";
>
> This also makes your Linux driver simpler because each GPIO port
> just becomes a set of 32bit registers and you can use
> select GPIO_GENERIC and bgpio_init() and save a whole
> slew of standard stock code.
>

Linus, thank you for your input.

The controller handles an array of 32*n signals, where n >= 1 && n <=
4.

The problem with the above approach is that the ports are disabled
*port*-wise - so they remove all (upto) 4 bits. That would be across the
banks.

You could of course have the "implied" semantics that a disabled port at
any bit position disabled all (bit positions for the same port).

But I don't know if this would be easier to understand, DT-wise.

What do you think...?

> Yours,
> Linus Walleij
Linus Walleij May 25, 2020, 8:50 a.m. UTC | #3 
On Mon, May 18, 2020 at 10:50 PM Lars Povlsen
<lars.povlsen@microchip.com> wrote:
> Linus Walleij writes:
>
> > On Wed, May 13, 2020 at 4:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
> >
> >> This adds DT bindings for the Microsemi SGPIO controller, bindings
> >> mscc,ocelot-sgpio and mscc,luton-sgpio.
> >>
> >> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> >
> >> +  microchip,sgpio-ports:
> >> +    description: This is a 32-bit bitmask, configuring whether a
> >> +      particular port in the controller is enabled or not. This allows
> >> +      unused ports to be removed from the bitstream and reduce latency.
> >> +    $ref: "/schemas/types.yaml#/definitions/uint32"
> >
> > I don't know about this.
> >
> > You are saying this pin controller can have up to 32 GPIO "ports"
> > (also known as banks).
> >
> > Why can't you just represent each such port as a separate GPIO
> > node:
> >
> > pinctrl@nnn {
> >     gpio@0 {
> >         ....
> >     };
> >     gpio@1 {
> >         ....
> >     };
> >     ....
> >     gpio@31 {
> >         ....
> >     };
> > };
> >
> > Then if some of them are unused just set it to status = "disabled";
> >
> > This also makes your Linux driver simpler because each GPIO port
> > just becomes a set of 32bit registers and you can use
> > select GPIO_GENERIC and bgpio_init() and save a whole
> > slew of standard stock code.
> >
>
> Linus, thank you for your input.
>
> The controller handles an array of 32*n signals, where n >= 1 && n <=
> 4.
>
> The problem with the above approach is that the ports are disabled
> *port*-wise - so they remove all (upto) 4 bits. That would be across the
> banks.
>
> You could of course have the "implied" semantics that a disabled port at
> any bit position disabled all (bit positions for the same port).

I don't understand this, you would have to elaborate...

In any case microchip,sgpio-ports is probably not the right thing,
use ngpios which is documented and just divide by 32 to get the
number of ports I think? But that is just in case they get
enabled strictly in sequence, otherwise you'd need a custom
property.

Yours,
Linus Walleij
Lars Povlsen May 25, 2020, 2:38 p.m. UTC | #4 
Linus Walleij writes:

> On Mon, May 18, 2020 at 10:50 PM Lars Povlsen
> <lars.povlsen@microchip.com> wrote:
>> Linus Walleij writes:
>>
>> > On Wed, May 13, 2020 at 4:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>> >
>> >> This adds DT bindings for the Microsemi SGPIO controller, bindings
>> >> mscc,ocelot-sgpio and mscc,luton-sgpio.
>> >>
>> >> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> >> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>> >
>> >> +  microchip,sgpio-ports:
>> >> +    description: This is a 32-bit bitmask, configuring whether a
>> >> +      particular port in the controller is enabled or not. This allows
>> >> +      unused ports to be removed from the bitstream and reduce latency.
>> >> +    $ref: "/schemas/types.yaml#/definitions/uint32"
>> >
>> > I don't know about this.
>> >
>> > You are saying this pin controller can have up to 32 GPIO "ports"
>> > (also known as banks).
>> >
>> > Why can't you just represent each such port as a separate GPIO
>> > node:
>> >
>> > pinctrl@nnn {
>> >     gpio@0 {
>> >         ....
>> >     };
>> >     gpio@1 {
>> >         ....
>> >     };
>> >     ....
>> >     gpio@31 {
>> >         ....
>> >     };
>> > };
>> >
>> > Then if some of them are unused just set it to status = "disabled";
>> >
>> > This also makes your Linux driver simpler because each GPIO port
>> > just becomes a set of 32bit registers and you can use
>> > select GPIO_GENERIC and bgpio_init() and save a whole
>> > slew of standard stock code.
>> >
>>
>> Linus, thank you for your input.
>>
>> The controller handles an array of 32*n signals, where n >= 1 && n <=
>> 4.
>>
>> The problem with the above approach is that the ports are disabled
>> *port*-wise - so they remove all (upto) 4 bits. That would be across the
>> banks.
>>
>> You could of course have the "implied" semantics that a disabled port at
>> any bit position disabled all (bit positions for the same port).
>
> I don't understand this, you would have to elaborate...
>
> In any case microchip,sgpio-ports is probably not the right thing,
> use ngpios which is documented and just divide by 32 to get the
> number of ports I think? But that is just in case they get
> enabled strictly in sequence, otherwise you'd need a custom
> property.
>

Hi Linus,

Yes, the problem is they're not in sequence. F.ex. you could have ports
0,1 enabled, skip 2,3,4 and have 5,6,7 enabled.

In the data stream you would then have:

p0.0 p0.1 p0.2 p0.3
p1.0 p1.1 p1.2 p1.3
p5.0 p5.1 p5.2 p5.3
p6.0 p6.1 p6.2 p6.3
p7.0 p7.1 p7.2 p7.3

I will mull about this and try to come up with something better and more
understandable.

Luckily, this is not gating for integrating sparx5, so its possible
we'll just skip the SGPIO driver for now.

I'll provide an update as soon as possible.

---Lars

> Yours,
> Linus Walleij
Linus Walleij May 26, 2020, 9:20 a.m. UTC | #5 
On Mon, May 25, 2020 at 4:38 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> Yes, the problem is they're not in sequence. F.ex. you could have ports
> 0,1 enabled, skip 2,3,4 and have 5,6,7 enabled.

Just use disabled nodes.

That would look like this in my idea of a device tree:

pinctrl@nnn {
    gpio0: gpio@0 {
        compatible = "foo";
        status = "ok";
        ....
    };
    gpio1: gpio@1 {
        compatible = "foo";
        status = "ok";
        ....
    };
    gpio2: gpio@2 {
        compatible = "foo";
        status = "disabled";
        ....
    };
    gpio3: gpio@3 {
        compatible = "foo";
        status = "disabled";
        ....
    };
    gpio4: gpio@4 {
        compatible = "foo";
        status = "disabled";
        ....
    };
    gpio5: gpio@5 {
        compatible = "foo";
        status = "ok";
        ....
    };
    gpio6: gpio@6 {
        compatible = "foo";
        status = "ok";
        ....
    };
    gpio7: gpio@7 {
        compatible = "foo";
        status = "ok";
        ....
    };
};

It is common to use the status to enable/disable nodes like this.

In the Linux kernel is is possible to iterate over these subnodes and
check which ones are enabled and disabled while keeping the
index by using something like:

i = 0;
struct device_node *np, *child;
for_each_child_of_node(np, child) {
    if (of_device_is_available(child)) {
        pr_info("populating device %d\n", i);
    }
    i++;
}

Certainly you can use i in the above loop to populate your registers
etc from an indexed array.

This way the consumers can pick their GPIO from the right port
and everything just using e.g.
my-gpios = <&gpio6 4 GPIO_OUT_LOW>;

Yours,
Linus Walleij
Lars Povlsen May 27, 2020, 8:05 a.m. UTC | #6 
Linus Walleij writes:

> On Mon, May 25, 2020 at 4:38 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> Yes, the problem is they're not in sequence. F.ex. you could have ports
>> 0,1 enabled, skip 2,3,4 and have 5,6,7 enabled.
>
> Just use disabled nodes.
>
> That would look like this in my idea of a device tree:
>
> pinctrl@nnn {
>     gpio0: gpio@0 {
>         compatible = "foo";
>         status = "ok";
>         ....
>     };
>     gpio1: gpio@1 {
>         compatible = "foo";
>         status = "ok";
>         ....
>     };
>     gpio2: gpio@2 {
>         compatible = "foo";
>         status = "disabled";
>         ....
>     };
>     gpio3: gpio@3 {
>         compatible = "foo";
>         status = "disabled";
>         ....
>     };
>     gpio4: gpio@4 {
>         compatible = "foo";
>         status = "disabled";
>         ....
>     };
>     gpio5: gpio@5 {
>         compatible = "foo";
>         status = "ok";
>         ....
>     };
>     gpio6: gpio@6 {
>         compatible = "foo";
>         status = "ok";
>         ....
>     };
>     gpio7: gpio@7 {
>         compatible = "foo";
>         status = "ok";
>         ....
>     };
> };
>
> It is common to use the status to enable/disable nodes like this.
>
> In the Linux kernel is is possible to iterate over these subnodes and
> check which ones are enabled and disabled while keeping the
> index by using something like:
>
> i = 0;
> struct device_node *np, *child;
> for_each_child_of_node(np, child) {
>     if (of_device_is_available(child)) {
>         pr_info("populating device %d\n", i);
>     }
>     i++;
> }
>
> Certainly you can use i in the above loop to populate your registers
> etc from an indexed array.
>
> This way the consumers can pick their GPIO from the right port
> and everything just using e.g.
> my-gpios = <&gpio6 4 GPIO_OUT_LOW>;
>

Linux, thank you for your input, it is much appreciated. I will use the
pattern in the driver in the next revision.

The only issue is that the gpios on the same "port" have restrictions on
their status - they can only be enabled "all" or "none" for gpios that
map to the same port. F.ex. gpio0, gpio32, gpio64 and gpio96 must all be
enabled or disabled because at the hardware level you control the
_port_. But as I noted earlier, that could just be the driver enforcing
this.

Thanks again.

---Lars

> Yours,
> Linus Walleij
Linus Walleij May 27, 2020, 1:45 p.m. UTC | #7 
On Wed, May 27, 2020 at 10:05 AM Lars Povlsen
<lars.povlsen@microchip.com> wrote:

> The only issue is that the gpios on the same "port" have restrictions on
> their status - they can only be enabled "all" or "none" for gpios that
> map to the same port. F.ex. gpio0, gpio32, gpio64 and gpio96 must all be
> enabled or disabled because at the hardware level you control the
> _port_.

This is fairly common. For example that an entire port/block share
a clock.

> But as I noted earlier, that could just be the driver enforcing
> this.

Yeps.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/mscc,ocelot-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/mscc,ocelot-sgpio.yaml
new file mode 100644
index 0000000000000..a332a0f4582fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/mscc,ocelot-sgpio.yaml
@@ -0,0 +1,66 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/mscc,ocelot-sgpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsemi 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:
+      - mscc,ocelot-sgpio
+      - mscc,luton-sgpio
+
+  clocks:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    description: Specifying the pin number and flags, as defined in
+      include/dt-bindings/gpio/gpio.h
+    const: 2
+
+  gpio-ranges:
+    maxItems: 1
+
+  microchip,sgpio-ports:
+    description: This is a 32-bit bitmask, configuring whether a
+      particular port in the controller is enabled or not. This allows
+      unused ports to be removed from the bitstream and reduce latency.
+    $ref: "/schemas/types.yaml#/definitions/uint32"
+
+required:
+  - compatible
+  - clocks
+  - gpio-controller
+  - '#gpio-cells'
+  - gpio-ranges
+
+examples:
+  - |
+    sgpio0: gpio@61101036c {
+        compatible = "mscc,ocelot-sgpio";
+        clocks = <&sys_clk>;
+        pinctrl-0 = <&sgpio0_pins>;
+        pinctrl-names = "default";
+        reg = <0x1101036c 0x100>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&gpio 0 64 64>;
+        microchip,sgpio-ports = <0x00ffffff>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index de64fd4548697..cdb63ca04670d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11233,6 +11233,7 @@  S:	Supported
 F:	Documentation/devicetree/bindings/mips/mscc.txt
 F:	Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
 F:	Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml
+F:	Documentation/devicetree/bindings/pinctrl/mscc,ocelot-sgpio.yaml
 F:	arch/mips/boot/dts/mscc/
 F:	arch/mips/configs/generic/board-ocelot.config
 F:	arch/mips/generic/board-ocelot.c
diff --git a/include/dt-bindings/gpio/mchp-sgpio.h b/include/dt-bindings/gpio/mchp-sgpio.h
new file mode 100644
index 0000000000000..0736158563f0a
--- /dev/null
+++ b/include/dt-bindings/gpio/mchp-sgpio.h
@@ -0,0 +1,21 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * This header provides constants for binding mscc,*-sgpio
+ *
+ * The first cell in the SGPIO specifier is the GPIO ID. The macros below
+ * provide machros for this.
+ *
+ * The second cell contains standard flag values specified in gpio.h.
+ */
+
+#ifndef _DT_BINDINGS_GPIO_MSCC_SGPIO_H
+#define _DT_BINDINGS_GPIO_MSCC_SGPIO_H
+
+#include <dt-bindings/gpio/gpio.h>
+
+#define MSCC_SGPIOS_PER_BANK	32
+#define MSCC_SGPIO_BANK_DEPTH	4
+
+#define MSCC_SGPIO(port, bit) ((bit * MSCC_SGPIOS_PER_BANK) + port)
+
+#endif