[PATCH RFC 0/3] PowerPC: implement support for GPIO LIB API

[PATCH RFC 0/3] PowerPC: implement support for GPIO LIB API

[Anton Vorontsov]

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: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

[PATCH 1/3] [POWERPC] Implement support for the GPIO LIB API

[Anton Vorontsov]

This patch implements support for the GPIO LIB API. Two calls
unimplemented though: irq_to_gpio and gpio_to_irq.

Various OF helpers provided to ease life and reduce code duplication.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |   58 ++++++++
 arch/powerpc/Kconfig                         |    5 +
 arch/powerpc/kernel/Makefile                 |    1 +
 arch/powerpc/kernel/gpio.c                   |  186 ++++++++++++++++++++++++++
 include/asm-powerpc/gpio.h                   |  126 +++++++++++++++++
 5 files changed, 376 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/kernel/gpio.c
 create mode 100644 include/asm-powerpc/gpio.h

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index ee0209a..dd2613c 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -60,6 +60,10 @@ Table of Contents
     3) OpenPIC Interrupt Controllers
     4) ISA Interrupt Controllers
 
+  VIII - Specifying GPIO information for devices
+    1) gpios property
+    2) gpio-controller nodes
+
   Appendix A - Sample SOC node for MPC8540
 
 
@@ -2618,6 +2622,60 @@ encodings listed below:
 	2 =  high to low edge sensitive type enabled
 	3 =  low to high edge sensitive type enabled
 
+VIII - Specifying GPIO information for devices
+==============================================
+
+1) gpios property
+-----------------
+
+Nodes that makes use of GPIOs should define them using `gpios' property,
+format of which is: <&gpio-controller1-phandle gpio1-specifier
+		     &gpio-controller2-phandle gpio2-specifier
+		     ...>;
+
+Note that gpio-specifier length is controller dependent.
+
+gpio-specifier may encode: bank, pin position inside the bank,
+whether pin is open-drain and whether pin is active-low.
+
+Example of the node using GPIOs:
+
+	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>;
+		wait-pattern;
+		wait-write;
+	};
+
+In that example gpio-specifier is "18" and encodes GPIO pin number only,
+as accepted by the "qe_pio_e" gpio-controller.
+
+2) gpio-controller nodes
+------------------------
+
+Every GPIO controller node must have #gpio-cells property defined,
+this information will be used to translate gpio-specifiers.
+
+Example of two SOC GPIO banks defined as gpio-controller nodes:
+
+	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;
+	};
 
 Appendix A - Sample SOC node for MPC8540
 ========================================
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 66a3d8c..b38c84a 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -73,6 +73,11 @@ config GENERIC_FIND_NEXT_BIT
 	bool
 	default y
 
+config GENERIC_GPIO
+	bool
+	help
+	  Generic GPIO API support
+
 config ARCH_NO_VIRT_TO_BUS
 	def_bool PPC64
 
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 9374bc9..943045c 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_BOOTX_TEXT)	+= btext.o
 obj-$(CONFIG_SMP)		+= smp.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
 obj-$(CONFIG_PPC_UDBG_16550)	+= legacy_serial.o udbg_16550.o
+obj-$(CONFIG_GPIO_LIB)		+= gpio.o
 
 pci64-$(CONFIG_PPC64)		+= pci_dn.o isa-bridge.o
 obj-$(CONFIG_PCI)		+= pci_$(CONFIG_WORD_SIZE).o $(pci64-y) \
diff --git a/arch/powerpc/kernel/gpio.c b/arch/powerpc/kernel/gpio.c
new file mode 100644
index 0000000..f41d1e0
--- /dev/null
+++ b/arch/powerpc/kernel/gpio.c
@@ -0,0 +1,186 @@
+/*
+ * OF helpers for the GPIO API
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ * Copyright (c) 2007  Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <asm/prom.h>
+#include <asm/gpio.h>
+
+int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
+			 const void *gpio_spec)
+{
+	const u32 *gpio = gpio_spec;
+
+	if (*gpio > of_gc->gc.ngpio)
+		return -EINVAL;
+
+	return *gpio;
+}
+EXPORT_SYMBOL(of_gpio_simple_xlate);
+
+int of_get_gpio(struct device_node *np, int index)
+{
+	int ret = -EINVAL;
+	struct device_node *gc;
+	struct of_gpio_chip *of_gc = NULL;
+	int size;
+	const u32 *gpios;
+	u32 nr_cells;
+	int i;
+	const void *gpio_spec;
+	const u32 *gpio_cells;
+	int gpio_index = 0;
+
+	gpios = of_get_property(np, "gpios", &size);
+	if (!gpios) {
+		ret = -ENOENT;
+		goto err0;
+	}
+	nr_cells = size / sizeof(u32);
+
+	for (i = 0; i < nr_cells;) {
+		const phandle *gpio_phandle;
+
+		gpio_phandle = gpios + i;
+		gpio_spec = gpio_phandle + 1;
+
+		gc = of_find_node_by_phandle(*gpio_phandle);
+		if (!gc) {
+			pr_debug("%s: could not find phandle for gpios\n",
+				 np->full_name);
+			goto err0;
+		}
+
+		of_gc = gc->data;
+		if (!of_gc) {
+			pr_debug("%s: gpio controller %s isn't registered\n",
+				 np->full_name, gc->full_name);
+			goto err1;
+		}
+
+		gpio_cells = of_get_property(gc, "#gpio-cells", &size);
+		if (!gpio_cells || size != sizeof(*gpio_cells) ||
+				*gpio_cells != of_gc->gpio_cells) {
+			pr_debug("%s: wrong #gpio-cells for %s\n",
+				 np->full_name, gc->full_name);
+			goto err1;
+		}
+
+		/* Next phandle is at phandle cells + #gpio-cells */
+		i += sizeof(*gpio_phandle) / sizeof(u32) + *gpio_cells;
+		if (i >= nr_cells + 1) {
+			pr_debug("%s: insufficient gpio-spec length\n",
+				 np->full_name);
+			goto err1;
+		}
+
+		if (gpio_index == index)
+			break;
+
+		of_gc = NULL;
+		of_node_put(gc);
+		gpio_index++;
+	}
+
+	if (!of_gc) {
+		ret = -ENOENT;
+		goto err0;
+	}
+
+	ret = of_gc->xlate(of_gc, np, gpio_spec);
+	if (ret < 0)
+		goto err1;
+
+	ret += of_gc->gc.base;
+err1:
+	of_node_put(gc);
+err0:
+	pr_debug("%s exited with status %d\n", __func__, ret);
+	return ret;
+}
+EXPORT_SYMBOL(of_get_gpio);
+
+static int of_get_gpiochip_base(struct device_node *np)
+{
+	struct device_node *gc = NULL;
+	int gpiochip_base = 0;
+
+	while ((gc = of_find_all_nodes(gc))) {
+		if (!of_get_property(gc, "gpio-controller", NULL))
+			continue;
+
+		if (gc != np) {
+			gpiochip_base += ARCH_GPIOS_PER_CHIP;
+			continue;
+		}
+
+		of_node_put(gc);
+
+		if (gpiochip_base >= ARCH_OF_GPIOS_END)
+			return -ENOSPC;
+
+		return gpiochip_base;
+	}
+
+	return -ENOENT;
+}
+
+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)
+		goto err1;
+
+	ret = of_get_gpiochip_base(np);
+	if (ret < 0)
+		goto err2;
+
+	mm_gc->regs = of_iomap(np, 0);
+	if (!mm_gc->regs) {
+		ret = -ENOMEM;
+		goto err1;
+	}
+
+	mm_gc->of_gc.gc.base = ret;
+
+	np->data = &mm_gc->of_gc;
+
+	ret = gpiochip_add(&mm_gc->of_gc.gc);
+	if (ret)
+		goto err2;
+
+	of_node_get(np);
+
+	pr_debug("%s: registered as generic GPIO chip, base is %d\n",
+		 np->full_name, mm_gc->of_gc.gc.base);
+	return 0;
+err2:
+	np->data = NULL;
+	kfree(mm_gc->of_gc.gc.label);
+err1:
+	kfree(mm_gc);
+err0:
+	pr_err("%s: GPIO chip registration failed with status %d\n",
+	       np->full_name, ret);
+	return ret;
+}
+EXPORT_SYMBOL(of_mm_gpiochip_add);
diff --git a/include/asm-powerpc/gpio.h b/include/asm-powerpc/gpio.h
new file mode 100644
index 0000000..1cc2e90
--- /dev/null
+++ b/include/asm-powerpc/gpio.h
@@ -0,0 +1,126 @@
+/*
+ * Generic GPIO API implementation for PowerPC.
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ * Copyright (c) 2007  Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __ASM_POWERPC_GPIO_H
+#define __ASM_POWERPC_GPIO_H
+
+#include <asm-generic/gpio.h>
+
+#ifdef CONFIG_GPIO_LIB
+
+#define ARCH_OF_GPIOS_BASE	0
+#define ARCH_OF_GPIOS_END	(ARCH_GPIOS_PER_CHIP * 7)
+#define ARCH_NON_OF_GPIOS_BASE	ARCH_OF_GPIOS_END
+#define ARCH_NON_OF_GPIOS_END	ARCH_NR_GPIOS
+
+#if ARCH_NON_OF_GPIOS_BASE >= ARCH_NON_OF_GPIOS_END
+#error "Default ARCH_NR_GPIOS isn't sufficient, define yours."
+#endif
+
+/*
+ * We don't (yet) implement inlined/rapid versions for on-chip gpios.
+ * Just call gpiolib.
+ */
+static inline int gpio_get_value(unsigned int gpio)
+{
+	return __gpio_get_value(gpio);
+}
+
+static inline void gpio_set_value(unsigned int gpio, int value)
+{
+	__gpio_set_value(gpio, value);
+}
+
+/*
+ * Not implemented, yet.
+ */
+static inline int gpio_to_irq(unsigned int gpio)
+{
+	return -ENOSYS;
+}
+
+static inline int irq_to_gpio(unsigned int irq)
+{
+	return -ENOSYS;
+}
+
+/*
+ * Generic OF GPIO chip
+ */
+struct of_gpio_chip {
+	struct gpio_chip gc;
+	int gpio_cells;
+	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
+		     const void *gpio_spec);
+};
+
+#define to_of_gpio_chip(x) container_of(x, struct of_gpio_chip, gc)
+
+/*
+ * OF GPIO chip for memory mapped banks
+ */
+struct of_mm_gpio_chip {
+	struct of_gpio_chip of_gc;
+	void __iomem *regs;
+};
+
+#define to_of_mm_gpio_chip(x) container_of(to_of_gpio_chip(x), \
+					   struct of_mm_gpio_chip, of_gc)
+
+/**
+ * of_gpio_simple_xlate - Translate GPIO number given in the device tree
+ * @of_gc:	pointer to the OF GPIO chip
+ * @np:		device node to get GPIO from
+ * @gpio_spec:	pointer to the gpio-spec field in the `gpios' property
+ *
+ * This is gpio-controller specific function, returns translated GPIO
+ * number.
+ */
+extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
+				struct device_node *np,
+				const void *gpio_spec);
+
+/**
+ * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
+ * @np:		device node to get GPIO from
+ * @index:	index of the GPIO
+ *
+ * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
+ * value on the error condition.
+ */
+extern int of_get_gpio(struct device_node *np, int index);
+
+/**
+ * of_mm_gpiochip_add - Add memory mapped GPIO chip (bank)
+ * @np:		device node of the GPIO chip
+ * @of_gc:	pointer to the of_gpio_chip template
+ *
+ * To use this function you should fill of_gc template with:
+ *
+ * 1) In the gpio_chip structure:
+ *    a) all callbacks
+ *    b) ngpios (GPIOs per bank)
+ *
+ * 2) In the of_gpio_chip structure:
+ *    a) xlate callback
+ *    b) gpio_cells
+ *
+ * If succeeded, this function will map bank's memory and will
+ * do all necessary work for you. Then you'll able to use .regs
+ * to manage GPIOs from the callbacks.
+ */
+extern int of_mm_gpiochip_add(struct device_node *np,
+			      const struct of_gpio_chip *of_gc);
+
+#endif /* CONFIG_GPIO_LIB */
+
+#endif /* __ASM_POWERPC_GPIO_H */
-- 
1.5.2.2

Re: [PATCH 1/3] [POWERPC] Implement support for the GPIO LIB API

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Tue, 8 Jan 2008 21:45:18 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +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)
> +		goto err1;
> +
> +	ret = of_get_gpiochip_base(np);
> +	if (ret < 0)
> +		goto err2;
> +
> +	mm_gc->regs = of_iomap(np, 0);
> +	if (!mm_gc->regs) {
> +		ret = -ENOMEM;
> +		goto err1;

Should this be err2?

> +	}
> +
> +	mm_gc->of_gc.gc.base = ret;
> +
> +	np->data = &mm_gc->of_gc;
> +
> +	ret = gpiochip_add(&mm_gc->of_gc.gc);
> +	if (ret)
> +		goto err2;
> +
> +	of_node_get(np);

Why do you this of_node_get(np) when you do not keep a reference to np in
this routine?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 2/3] [POWERPC] QE: split par_io_config_pin()

[Anton Vorontsov]

This patch splits par_io_config_pin so we can use it with GPIO LIB API.

Also add a comment regarding #ifdef CONFIG_PPC_85xx being legacy.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/sysdev/qe_lib/qe_io.c |   60 +++++++++++++++++++++++------------
 1 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/sysdev/qe_lib/qe_io.c b/arch/powerpc/sysdev/qe_lib/qe_io.c
index e53ea4d..aef893b 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_io.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_io.c
@@ -37,6 +37,10 @@ struct port_regs {
 	__be32	cppar1;		/* Pin assignment register */
 	__be32	cppar2;		/* Pin assignment register */
 #ifdef CONFIG_PPC_85xx
+	/*
+	 * This is needed for legacy support only, should go away,
+	 * because we started using per-bank gpio chips.
+	 */
 	u8	pad[8];
 #endif
 };
@@ -63,28 +67,29 @@ int par_io_init(struct device_node *np)
 	return 0;
 }
 
-int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
-		      int assignment, int has_irq)
+static void __par_io_config_pin(struct port_regs __iomem *par_io,
+				u8 pin, int dir, int open_drain,
+				int assignment, int has_irq)
 {
-	u32 pin_mask1bit, pin_mask2bits, new_mask2bits, tmp_val;
-
-	if (!par_io)
-		return -1;
+	u32 pin_mask1bit;
+	u32 pin_mask2bits;
+	u32 new_mask2bits;
+	u32 tmp_val;
 
 	/* calculate pin location for single and 2 bits information */
 	pin_mask1bit = (u32) (1 << (NUM_OF_PINS - (pin + 1)));
 
 	/* Set open drain, if required */
-	tmp_val = in_be32(&par_io[port].cpodr);
+	tmp_val = in_be32(&par_io->cpodr);
 	if (open_drain)
-		out_be32(&par_io[port].cpodr, pin_mask1bit | tmp_val);
+		out_be32(&par_io->cpodr, pin_mask1bit | tmp_val);
 	else
-		out_be32(&par_io[port].cpodr, ~pin_mask1bit & tmp_val);
+		out_be32(&par_io->cpodr, ~pin_mask1bit & tmp_val);
 
 	/* define direction */
 	tmp_val = (pin > (NUM_OF_PINS / 2) - 1) ?
-		in_be32(&par_io[port].cpdir2) :
-		in_be32(&par_io[port].cpdir1);
+		in_be32(&par_io->cpdir2) :
+		in_be32(&par_io->cpdir1);
 
 	/* get all bits mask for 2 bit per port */
 	pin_mask2bits = (u32) (0x3 << (NUM_OF_PINS -
@@ -96,36 +101,49 @@ int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
 
 	/* clear and set 2 bits mask */
 	if (pin > (NUM_OF_PINS / 2) - 1) {
-		out_be32(&par_io[port].cpdir2,
+		out_be32(&par_io->cpdir2,
 			 ~pin_mask2bits & tmp_val);
 		tmp_val &= ~pin_mask2bits;
-		out_be32(&par_io[port].cpdir2, new_mask2bits | tmp_val);
+		out_be32(&par_io->cpdir2, new_mask2bits | tmp_val);
 	} else {
-		out_be32(&par_io[port].cpdir1,
+		out_be32(&par_io->cpdir1,
 			 ~pin_mask2bits & tmp_val);
 		tmp_val &= ~pin_mask2bits;
-		out_be32(&par_io[port].cpdir1, new_mask2bits | tmp_val);
+		out_be32(&par_io->cpdir1, new_mask2bits | tmp_val);
 	}
 	/* define pin assignment */
 	tmp_val = (pin > (NUM_OF_PINS / 2) - 1) ?
-		in_be32(&par_io[port].cppar2) :
-		in_be32(&par_io[port].cppar1);
+		in_be32(&par_io->cppar2) :
+		in_be32(&par_io->cppar1);
 
 	new_mask2bits = (u32) (assignment << (NUM_OF_PINS -
 			(pin % (NUM_OF_PINS / 2) + 1) * 2));
 	/* clear and set 2 bits mask */
 	if (pin > (NUM_OF_PINS / 2) - 1) {
-		out_be32(&par_io[port].cppar2,
+		out_be32(&par_io->cppar2,
 			 ~pin_mask2bits & tmp_val);
 		tmp_val &= ~pin_mask2bits;
-		out_be32(&par_io[port].cppar2, new_mask2bits | tmp_val);
+		out_be32(&par_io->cppar2, new_mask2bits | tmp_val);
 	} else {
-		out_be32(&par_io[port].cppar1,
+		out_be32(&par_io->cppar1,
 			 ~pin_mask2bits & tmp_val);
 		tmp_val &= ~pin_mask2bits;
-		out_be32(&par_io[port].cppar1, new_mask2bits | tmp_val);
+		out_be32(&par_io->cppar1, new_mask2bits | tmp_val);
 	}
+}
+
+/*
+ * This is "legacy" function that takes port number as an argument
+ * instead of pointer to the appropriate bank.
+ */
+int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
+		      int assignment, int has_irq)
+{
+	if (!par_io || port >= num_par_io_ports)
+		return -EINVAL;
 
+	__par_io_config_pin(&par_io[port], pin, dir, open_drain, assignment,
+			    has_irq);
 	return 0;
 }
 EXPORT_SYMBOL(par_io_config_pin);
-- 
1.5.2.2

[PATCH 3/3] [POWERPC] QE: implement GPIO LIB API

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |   32 +++++++----
 arch/powerpc/platforms/Kconfig               |    2 +
 arch/powerpc/sysdev/qe_lib/qe_io.c           |   73 ++++++++++++++++++++++++++
 include/asm-powerpc/qe.h                     |    1 +
 4 files changed, 96 insertions(+), 12 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index dd2613c..e279152 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1696,24 +1696,32 @@ platforms are moved over to use the flattened-device-tree model.
    information.
 
    Required properties:
-   - device_type : should be "par_io".
+   - #gpio-cells : should be "1".
+   - compatible : should be "fsl,qe-pario-bank"
    - reg : offset to the register set and its length.
-   - num-ports : number of Parallel I/O ports
+   - gpio-controller : node to identify gpio controllers.
 
-   Example:
-	par_io@1400 {
-		reg = <1400 100>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-		device_type = "par_io";
-		num-ports = <7>;
-		ucc_pin@01 {
-			......
-		};
+   For example, two QE Par I/O banks:
+	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;
+	};
 
    vi) Pin configuration nodes
 
+   NOTE: pin configuration nodes are obsolete. Usually, their existance
+         is an evidence of the firmware shortcomings. Such fixups are
+         better handled by the Linux board file, not the device tree.
+
    Required properties:
    - linux,phandle : phandle of this node; likely referenced by a QE
      device.
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index ea22cad..8dff946 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -265,6 +265,8 @@ config TAU_AVERAGE
 config QUICC_ENGINE
 	bool
 	select PPC_LIB_RHEAP
+	select GENERIC_GPIO
+	select GPIO_LIB
 	help
 	  The QUICC Engine (QE) is a new generation of communications
 	  coprocessors on Freescale embedded CPUs (akin to CPM in older chips).
diff --git a/arch/powerpc/sysdev/qe_lib/qe_io.c b/arch/powerpc/sysdev/qe_lib/qe_io.c
index aef893b..2a1ff45 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_io.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_io.c
@@ -23,6 +23,7 @@
 
 #include <asm/io.h>
 #include <asm/prom.h>
+#include <asm/gpio.h>
 #include <sysdev/fsl_soc.h>
 
 #undef DEBUG
@@ -213,6 +214,78 @@ int par_io_of_config(struct device_node *np)
 }
 EXPORT_SYMBOL(par_io_of_config);
 
+/*
+ * GPIO LIB API implementation
+ */
+
+static int qe_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct port_regs *regs = mm_gc->regs;
+	u32 pin_mask;
+
+	/* calculate pin location */
+	pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
+
+	return !!(in_be32(&regs->cpdata) & pin_mask);
+}
+
+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;
+
+	/* 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);
+}
+
+static int qe_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+	__par_io_config_pin(mm_gc->regs, gpio, 2, 0, 0, 0);
+
+	return 0;
+}
+
+static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+	__par_io_config_pin(mm_gc->regs, gpio, 1, 0, 0, 0);
+	qe_gpio_set(gc, gpio, val);
+
+	return 0;
+}
+
+static struct of_gpio_chip qe_gc = {
+	.gpio_cells = 1,
+	.xlate = of_gpio_simple_xlate,
+
+	.gc = {
+		.ngpio = NUM_OF_PINS,
+		.direction_input = qe_gpio_dir_in,
+		.direction_output = qe_gpio_dir_out,
+		.get = qe_gpio_get,
+		.set = qe_gpio_set,
+	},
+};
+
+int qe_gpiochip_add(struct device_node *np)
+{
+	return of_mm_gpiochip_add(np, &qe_gc);
+}
+EXPORT_SYMBOL(qe_gpiochip_add);
+
 #ifdef DEBUG
 static void dump_par_io(void)
 {
diff --git a/include/asm-powerpc/qe.h b/include/asm-powerpc/qe.h
index a24b7b1..2efda82 100644
--- a/include/asm-powerpc/qe.h
+++ b/include/asm-powerpc/qe.h
@@ -77,6 +77,7 @@ enum qe_clock {
 /* Export QE common operations */
 extern void qe_reset(void);
 extern int par_io_init(struct device_node *np);
+extern int qe_gpiochip_add(struct device_node *np);
 extern int par_io_of_config(struct device_node *np);
 extern int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
 			     int assignment, int has_irq);
-- 
1.5.2.2

Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API

[Jochen Friedrich]

Hi Anton,

> +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;
> +
> +	/* 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.

i2c-gpio.c and w1-gpio.c (in -mm) are examples of drivers which use open drain pins
and which would fail if another pin on the same port would be set during a transfer.

Thanks,
Jochen

Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API

[Anton Vorontsov]

On Sun, Jan 27, 2008 at 02:42:12PM +0100, Jochen Friedrich wrote:
> Hi Anton,
> 
> > +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;
> > +
> > +	/* 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.

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: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API

[Grant Likely]

On 1/27/08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Sun, Jan 27, 2008 at 02:42:12PM +0100, Jochen Friedrich wrote:
> > Hi Anton,
> >
> > > +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;
> > > +
> > > +   /* 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.
>
> 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.

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.

Here's what I did for a Xilinx GPIO block driver (but ignore the fact
that I'm not using driver private data so only one GPIO block can be
supported; that will be fixed before I post this driver)

+void gpio_set_value(unsigned gpio, int value)
+{
+       unsigned long flags;
+
+       if (!gpio_regs)
+               return;
+
+       spin_lock_irqsave(&gpio_spinlock, flags);
+       if (value)
+               gpio_output_state |= 1<<gpio;
+       else
+               gpio_output_state &= ~(1<<gpio);
+       out_be32(gpio_regs, gpio_output_state);
+       spin_unlock_irqrestore(&gpio_spinlock, flags);
+}
+EXPORT_SYMBOL_GPL(gpio_set_value);

Cheers,
g.

>
> --
> Anton Vorontsov
> email: cbou@mail.ru
> backup email: ya-cbou@yandex.ru
> irc://irc.freenode.net/bd2
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API

[Anton Vorontsov]

On Sun, Jan 27, 2008 at 01:59:51PM -0700, Grant Likely wrote:
a> On 1/27/08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Sun, Jan 27, 2008 at 02:42:12PM +0100, Jochen Friedrich wrote:
> > > Hi Anton,
> > >
> > > > +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;
> > > > +
> > > > +   /* 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.
> >
> > 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.
> 
> 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.

Great idea, much thanks. Would be easy to implement also.

> 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.

With GPIO LIB we already have per bank spinlock, so it isn't
a problem.


Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH RFC 0/3] PowerPC: implement support for GPIO LIB API

[Grant Likely]

On 1/8/08, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> Hi all,
>
> Thanks for the previous review and ideas. Here is the RFC for the
> GPIO LIB support.

I like this layout for gpios.  It's concise and compact.  It will work
well for my gpio driver also.

>
> I'm using it like this:
>
>         qe_pio_a: gpio-controller@1400 {
>                 #gpio-cells = <1>;
>                 compatible = "fsl,qe-pario-bank";

Here's my only comment: Compatible should have "fsl,<chip>-pario-bank"
first.  Be specific for the first value of the compatible property.
You can add generic names after the specific one if you like, but it's
hard to come up with a generic name when you don't know what a
manufacture is going to do with it's marketing names in the future.

>                 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: cbou@mail.ru
> backup email: ya-cbou@yandex.ru
> irc://irc.freenode.net/bd2
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.