PowerPC: implement support for GPIO LIB API

arch/powerpc/kernel/gpio.c new file +14 -0

+int of_mm_gpiochip_add(struct device_node *np,
+		       const struct of_gpio_chip *of_gc)
+{
+	int ret = -ENOMEM;
+	struct of_mm_gpio_chip *mm_gc;
+
+	mm_gc = kzalloc(sizeof(*mm_gc), GFP_KERNEL);
+	if (!mm_gc)
+		goto err0;
+
+	mm_gc->of_gc = *of_gc;
+
+	mm_gc->of_gc.gc.label = kstrdup(np->full_name, GFP_KERNEL);
+	if (!mm_gc->of_gc.gc.label)

arch/powerpc/sysdev/qe_lib/qe_io.c +7 -0

+static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct port_regs *regs = mm_gc->regs;
+	u32 pin_mask;
+	u32 tmp_val;
+

Hi Anton,

Quoted 7 lines

arch/powerpc/sysdev/qe_lib/qe_io.c +7 -0

+static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct port_regs *regs = mm_gc->regs;
+	u32 pin_mask;
+	u32 tmp_val;
+

arch/powerpc/sysdev/qe_lib/qe_io.c int par_io_of_config(struct device_node *np) · line 249

Quoted 10 lines

arch/powerpc/sysdev/qe_lib/qe_io.c +10 -0

+	/* calculate pin location */
+	pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
+
+	tmp_val = in_be32(&regs->cpdata);
+
+	if (val == 0)
+		out_be32(&regs->cpdata, ~pin_mask & tmp_val);
+	else
+		out_be32(&regs->cpdata, pin_mask | tmp_val);
+}

I see a possible problem with this (and in the corresponding call in CPM1, as well):

if there is a pin configured as open drain, you might accidently switch this pin to 0 while switching a different pin, if an external device is pulling the pin to 0.

Quoted 22 lines · Jochen Friedrich

Hi Anton,

Quoted 7 lines

arch/powerpc/sysdev/qe_lib/qe_io.c +7 -0

+static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+   struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+   struct port_regs *regs = mm_gc->regs;
+   u32 pin_mask;
+   u32 tmp_val;
+

arch/powerpc/sysdev/qe_lib/qe_io.c int par_io_of_config(struct device_node *np) · line 249

Quoted 10 lines

arch/powerpc/sysdev/qe_lib/qe_io.c +10 -0

+   /* calculate pin location */
+   pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
+
+   tmp_val = in_be32(&regs->cpdata);
+
+   if (val == 0)
+           out_be32(&regs->cpdata, ~pin_mask & tmp_val);
+   else
+           out_be32(&regs->cpdata, pin_mask | tmp_val);
+}

I see a possible problem with this (and in the corresponding call in CPM1, as well):

Quoted 2 lines

if there is a pin configured as open drain, you might accidently switch this pin to 0 while switching a different pin, if an external device is pulling the pin to 0.

Unfortunately I can't think out any workaround for this, except implementing generic gpio_bank_{,un}lock(gpio_pin_on_the_bank), and start using it in the drivers that might care about this issue. Though, looking into i2c-gpio.c I don't clearly see were we can insert these locks, there should be "start/end transaction" handlers or something, but it seems that it's in the bitbanging code, not in the i2c-gpio driver..

Actually, I see this as a hardware limitation. For example, on ARMs PXA2xx, there are separate, per bank, read/set/clear GPIO registers, not all-in-one data register.

--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 34 lines · Jochen Friedrich

Quoted 22 lines

Hi Anton,

Quoted 7 lines

arch/powerpc/sysdev/qe_lib/qe_io.c +7 -0

+static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+   struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+   struct port_regs *regs = mm_gc->regs;
+   u32 pin_mask;
+   u32 tmp_val;
+

arch/powerpc/sysdev/qe_lib/qe_io.c int par_io_of_config(struct device_node *np) · line 249

Quoted 10 lines

arch/powerpc/sysdev/qe_lib/qe_io.c +10 -0

+   /* calculate pin location */
+   pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
+
+   tmp_val = in_be32(&regs->cpdata);
+
+   if (val == 0)
+           out_be32(&regs->cpdata, ~pin_mask & tmp_val);
+   else
+           out_be32(&regs->cpdata, pin_mask | tmp_val);
+}

I see a possible problem with this (and in the corresponding call in CPM1, as well):

Quoted 2 lines

if there is a pin configured as open drain, you might accidently switch this pin to 0 while switching a different pin, if an external device is pulling the pin to 0.

Unfortunately I can't think out any workaround for this, except implementing generic gpio_bank_{,un}lock(gpio_pin_on_the_bank), and start using it in the drivers that might care about this issue. Though, looking into i2c-gpio.c I don't clearly see were we can insert these locks, there should be "start/end transaction" handlers or something, but it seems that it's in the bitbanging code, not in the i2c-gpio driver..

Quoted 3 lines

Actually, I see this as a hardware limitation. For example, on ARMs PXA2xx, there are separate, per bank, read/set/clear GPIO registers, not all-in-one data register.

I've run into this exact issue on other GPIO hardware too. It's not uncommon behaviour in GPIO hardware.

The solution is to not depend on the hardware to remember what the output pin values should be. Add a shadow register in the driver private data. Set the pin state for each output pin in the shadow register and then write that value to the hardware. That way input state doesn't interfere with the output values.

Also, you do still need spinlocks around the manipulation of the shared registers; otherwise you'll have very hard to debug race conditions. Probably one spin lock per bank.

Hi all,

Thanks for the previous review and ideas. Here is the RFC for the GPIO LIB support.

I'm using it like this:

        qe_pio_a: gpio-controller@1400 {
                #gpio-cells = <1>;
                compatible = "fsl,qe-pario-bank";

                reg = <0x1400 0x18>;
                gpio-controller;
        };

        qe_pio_e: gpio-controller@1460 {
                #gpio-cells = <1>;
                compatible = "fsl,qe-pario-bank";
                reg = <0x1460 0x18>;
                gpio-controller;
        };

        nand-flash@1,0 {
                compatible = "stmicro,NAND512W3A2BN6E", "fsl,upm-nand";
                reg = <1 0 1>;
                width = <1>;
                upm = "A";
                upm-addr-offset = <16>;
                upm-cmd-offset = <8>;
                gpios = <&qe_pio_e 18 &qe_pio_a 9>;
                wait-pattern;
                wait-write;
        };

As you can see I've split single "par_io" node into banks, thus the benefits:
- we can handle banks of different width (Jochen Friedrich asked for this, IIRC. Or I've misunderstood ;-).
- we can handle banks with different paddings (no more #ifdef PPC85xx).

Also, it's possible to specify different gpio-controllers in the gpios = <> property. gpio-specifier is controller specific.

The remaining question is about gpio.c placement. prom_parse.c doesn't feel as the appropriate place anymore. I think of:

- drivers/gpio/of_gpio.c (driver/gpio/ is in -mm tree);
- drivers/of/gpio.c
- keep it in arch/powerpc/kernel/ (though, there is nothing much PowerPC specific).

So, to try these patches you'll need apply these from the -mm tree:
generic-gpio-gpio_chip-support.patch generic-gpio-gpio_chip-support-fix.patch generic-gpio-gpio_chip-support-gpiolib-locking-simplified.patch

Or just issue this in your working tree:

git pull git://git.infradead.org/users/cbou/powerpc-gpio.git gpiolib

powerpc-gpio.git's gpiolib branch is galak/powerpc.git master branch + -mm patches + these patches.

Support for CPM2 is pending.

Thanks!

--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2