[PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[Alan Tull]

From: Alan Tull <atull@altera.com>

Hi Linus,

If you don't have any further comments, can you take this patch?

Alan

Jamie Iles (1):
  gpio: add a driver for the Synopsys DesignWare APB GPIO block

 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   59 +++
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-dwapb.c                          |  415 ++++++++++++++++++++
 4 files changed, 484 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
 create mode 100644 drivers/gpio/gpio-dwapb.c

-- 
1.7.9.5

[PATCH v9] gpio: add a driver for the Synopsys DesignWare APB GPIO block

[Alan Tull]

From: Jamie Iles <jamie@jamieiles.com>

The Synopsys DesignWare block is used in some ARM devices (picoxcell)
and can be configured to provide multiple banks of GPIO pins.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Signed-off-by: Alan Tull <atull@altera.com>
Reviewed-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

v9:	- cleanup in dt bindings doc
	- use of_get_child_count()
v8:	- remove socfpga.dtsi changes
	- minor cleanup in devicetree documentation
v7:	- use irq_generic_chip
	- support one irq per gpio line or one irq for many
	- s/bank/port/ and other cleanup
v6:	- (atull) squash the set of patches
	- use linear irq domain
	- build fixes. Original driver was reviewed on v3.2.
	- Fix setting irq edge type for 'rising' and 'both'.
	- Support as a loadable module.
	- Use bgpio_chip's spinlock during register access.
	- Clean up register names to match spec
	- s/bank/port/ because register names use the word 'port'
	- s/nr-gpio/nr-gpios/
	- don't get/put the of_node
	- remove signoffs/acked-by's because of changes
	- other cleanup
v5:	- handle sparse bank population correctly
v3:	- depend on rather than select IRQ_DOMAIN
	- split IRQ support into a separate patch
v2:	- use Rob Herring's irqdomain in generic irq chip patches
	- use reg property to indicate bank index
	- support irqs on both edges based on LinusW's u300 driver
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   59 +++
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-dwapb.c                          |  415 ++++++++++++++++++++
 4 files changed, 484 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
 create mode 100644 drivers/gpio/gpio-dwapb.c

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
new file mode 100644
index 0000000..2d5eca6
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -0,0 +1,59 @@
+* Synopsys DesignWare APB GPIO controller
+
+Required properties:
+- compatible : Should contain "snps,dw-apb-gpio"
+- reg : Address and length of the register set for the device
+
+The GPIO controller has a configurable number of ports, each of which are
+represented as child nodes with the following properties:
+
+Required properties:
+- compatible : "snps,dw-apb-gpio-port"
+- gpio-controller : Marks the device node as a gpio controller.
+- #gpio-cells : Should be two.  The first cell is the pin number and
+  the second cell is used to specify optional parameters (currently
+  unused).
+- reg : The integer port index of the port, a single cell.
+- #address-cells : should be 1.
+- #size-cells : should be 0.
+
+Optional properties:
+- interrupt-controller : The first port may be configured to be an interrupt
+controller.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+interrupt.  Shall be set to 2.  The first cell defines the interrupt number,
+the second encodes the triger flags encoded as described in
+Documentation/devicetree/bindings/interrupts.txt
+- interrupt-parent : The parent interrupt controller.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+generate the interrupts.
+- snps,nr-gpios : The number of pins in the port, a single cell.
+
+Example:
+
+gpio: gpio@20000 {
+	compatible = "snps,dw-apb-gpio";
+	reg = <0x20000 0x1000>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	porta: gpio-controller@0 {
+		compatible = "snps,dw-apb-gpio-port";
+		gpio-controller;
+		#gpio-cells = <2>;
+		snps,nr-gpio = <8>;
+		reg = <0>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupt-parent = <&vic1>;
+		interrupts = <0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>;
+	};
+
+	portb: gpio-controller@1 {
+		compatible = "snps,dw-apb-gpio-port";
+		gpio-controller;
+		#gpio-cells = <2>;
+		snps,nr-gpio = <8>;
+		reg = <1>;
+	};
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0f04444..17760dd 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -121,6 +121,15 @@ config GPIO_GENERIC_PLATFORM
 	help
 	  Say yes here to support basic platform_device memory-mapped GPIO controllers.
 
+config GPIO_DWAPB
+	tristate "Synopsys DesignWare APB GPIO driver"
+	select GPIO_GENERIC
+	select GENERIC_IRQ_CHIP
+	depends on OF_GPIO && IRQ_DOMAIN
+	help
+	  Say Y or M here to build support for the Synopsys DesignWare APB
+	  GPIO block.
+
 config GPIO_IT8761E
 	tristate "IT8761E GPIO support"
 	depends on X86  # unconditional access to IO space.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 7971e36..b4f852e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
+obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_F7188X)	+= gpio-f7188x.o
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
new file mode 100644
index 0000000..7dff515
--- /dev/null
+++ b/drivers/gpio/gpio-dwapb.c
@@ -0,0 +1,415 @@
+/*
+ * Copyright (c) 2011 Jamie Iles
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * All enquiries to support@picochip.com
+ */
+#include <linux/basic_mmio_gpio.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define GPIO_SWPORTA_DR		0x00
+#define GPIO_SWPORTA_DDR	0x04
+#define GPIO_SWPORTB_DR		0x0c
+#define GPIO_SWPORTB_DDR	0x10
+#define GPIO_SWPORTC_DR		0x18
+#define GPIO_SWPORTC_DDR	0x1c
+#define GPIO_SWPORTD_DR		0x24
+#define GPIO_SWPORTD_DDR	0x28
+#define GPIO_INTEN		0x30
+#define GPIO_INTMASK		0x34
+#define GPIO_INTTYPE_LEVEL	0x38
+#define GPIO_INT_POLARITY	0x3c
+#define GPIO_INTSTATUS		0x40
+#define GPIO_PORTA_EOI		0x4c
+#define GPIO_EXT_PORTA		0x50
+#define GPIO_EXT_PORTB		0x54
+#define GPIO_EXT_PORTC		0x58
+#define GPIO_EXT_PORTD		0x5c
+
+#define DWAPB_MAX_PORTS		4
+#define GPIO_EXT_PORT_SIZE	(GPIO_EXT_PORTB - GPIO_EXT_PORTA)
+#define GPIO_SWPORT_DR_SIZE	(GPIO_SWPORTB_DR - GPIO_SWPORTA_DR)
+#define GPIO_SWPORT_DDR_SIZE	(GPIO_SWPORTB_DDR - GPIO_SWPORTA_DDR)
+
+struct dwapb_gpio;
+
+struct dwapb_gpio_port {
+	struct bgpio_chip	bgc;
+	bool			is_registered;
+	struct dwapb_gpio	*gpio;
+};
+
+struct dwapb_gpio {
+	struct	device		*dev;
+	void __iomem		*regs;
+	struct dwapb_gpio_port	*ports;
+	unsigned int		nr_ports;
+	struct irq_domain	*domain;
+};
+
+static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	struct dwapb_gpio_port *port = container_of(bgc, struct
+						    dwapb_gpio_port, bgc);
+	struct dwapb_gpio *gpio = port->gpio;
+
+	return irq_create_mapping(gpio->domain, offset);
+}
+
+static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
+{
+	u32 v = readl(gpio->regs + GPIO_INT_POLARITY);
+
+	if (gpio_get_value(gpio->ports[0].bgc.gc.base + offs))
+		v &= ~BIT(offs);
+	else
+		v |= BIT(offs);
+
+	writel(v, gpio->regs + GPIO_INT_POLARITY);
+}
+
+static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+{
+	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS);
+
+	while (irq_status) {
+		int hwirq = fls(irq_status) - 1;
+		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
+
+		generic_handle_irq(gpio_irq);
+		irq_status &= ~BIT(hwirq);
+
+		if ((irq_get_trigger_type(gpio_irq) & IRQ_TYPE_SENSE_MASK)
+			== IRQ_TYPE_EDGE_BOTH)
+			dwapb_toggle_trigger(gpio, hwirq);
+	}
+
+	if (chip->irq_eoi)
+		chip->irq_eoi(irq_desc_get_irq_data(desc));
+}
+
+static void dwapb_irq_enable(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = gc->private;
+	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	val = readl(gpio->regs + GPIO_INTEN);
+	val |= BIT(d->hwirq);
+	writel(val, gpio->regs + GPIO_INTEN);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static void dwapb_irq_disable(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = gc->private;
+	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	val = readl(gpio->regs + GPIO_INTEN);
+	val &= ~BIT(d->hwirq);
+	writel(val, gpio->regs + GPIO_INTEN);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int dwapb_irq_set_type(struct irq_data *d, u32 type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = gc->private;
+	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
+	int bit = d->hwirq;
+	unsigned long level, polarity, flags;
+
+	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
+		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		return -EINVAL;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	level = readl(gpio->regs + GPIO_INTTYPE_LEVEL);
+	polarity = readl(gpio->regs + GPIO_INT_POLARITY);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_BOTH:
+		level |= BIT(bit);
+		dwapb_toggle_trigger(gpio, bit);
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		level |= BIT(bit);
+		polarity |= BIT(bit);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		level |= BIT(bit);
+		polarity &= ~BIT(bit);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		level &= ~BIT(bit);
+		polarity |= BIT(bit);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		level &= ~BIT(bit);
+		polarity &= ~BIT(bit);
+		break;
+	}
+
+	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
+	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
+	return 0;
+}
+
+static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
+				 struct dwapb_gpio_port *port)
+{
+	struct gpio_chip *gc = &port->bgc.gc;
+	struct device_node *node =  gc->of_node;
+	struct irq_chip_generic	*irq_gc;
+	unsigned int hwirq, ngpio = gc->ngpio;
+	struct irq_chip_type *ct;
+	int reg, err, irq;
+
+	if (of_get_property(node, "interrupts", &reg) == NULL)
+		return;
+
+	gpio->domain = irq_domain_add_linear(node, ngpio, &irq_generic_chip_ops,
+					     gpio);
+	if (!gpio->domain)
+		return;
+
+	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
+					     "gpio-dwapb", handle_level_irq,
+					     IRQ_NOREQUEST, 0,
+					     IRQ_GC_INIT_NESTED_LOCK);
+	if (err) {
+		dev_info(gpio->dev, "irq_alloc_domain_generic_chips failed\n");
+		irq_domain_remove(gpio->domain);
+		gpio->domain = NULL;
+		return;
+	}
+
+	for (hwirq = 0 ; hwirq < ngpio ; hwirq++) {
+		irq_gc = irq_get_domain_generic_chip(gpio->domain, hwirq);
+		if (!irq_gc) {
+			irq_domain_remove(gpio->domain);
+			gpio->domain = NULL;
+			return;
+		}
+
+		irq_gc->reg_base = gpio->regs;
+		irq_gc->private = gpio;
+
+		ct = irq_gc->chip_types;
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask = irq_gc_mask_set_bit;
+		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+		ct->chip.irq_set_type = dwapb_irq_set_type;
+		ct->chip.irq_enable = dwapb_irq_enable;
+		ct->chip.irq_disable = dwapb_irq_disable;
+		ct->regs.ack = GPIO_PORTA_EOI;
+		ct->regs.mask = GPIO_INTMASK;
+
+		irq_setup_generic_chip(irq_gc, IRQ_MSK(port->bgc.gc.ngpio),
+				IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
+
+		irq = irq_of_parse_and_map(node, hwirq);
+		if (!irq && (hwirq == 0)) {
+			dev_warn(gpio->dev, "no irq for bank %s\n",
+				port->bgc.gc.of_node->full_name);
+			return;
+		} else if (!irq)
+			break;
+
+		irq_set_chained_handler(irq, dwapb_irq_handler);
+		irq_set_handler_data(irq, gpio);
+	}
+	port->bgc.gc.to_irq = dwapb_gpio_to_irq;
+}
+
+static void dwapb_irq_teardown(struct dwapb_gpio *gpio)
+{
+	struct dwapb_gpio_port *port = &gpio->ports[0];
+	struct gpio_chip *gc = &port->bgc.gc;
+	unsigned int ngpio = gc->ngpio;
+	irq_hw_number_t hwirq;
+
+	if (!gpio->domain)
+		return;
+
+	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
+		irq_dispose_mapping(irq_find_mapping(gpio->domain, hwirq));
+
+	irq_domain_remove(gpio->domain);
+	gpio->domain = NULL;
+}
+
+static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
+			       struct device_node *port_np,
+			       unsigned int offs)
+{
+	struct dwapb_gpio_port *port;
+	u32 port_idx, ngpio;
+	void __iomem *dat, *set, *dirout;
+	int err;
+
+	if (of_property_read_u32(port_np, "reg", &port_idx) ||
+		port_idx >= DWAPB_MAX_PORTS) {
+		dev_err(gpio->dev, "missing/invalid port index for %s\n",
+			port_np->full_name);
+		return -EINVAL;
+	}
+
+	port = &gpio->ports[offs];
+	port->gpio = gpio;
+
+	if (of_property_read_u32(port_np, "snps,nr-gpios", &ngpio)) {
+		dev_info(gpio->dev, "failed to get number of gpios for %s\n",
+			 port_np->full_name);
+		ngpio = 32;
+	}
+
+	dat = gpio->regs + GPIO_EXT_PORTA + (port_idx * GPIO_EXT_PORT_SIZE);
+	set = gpio->regs + GPIO_SWPORTA_DR + (port_idx * GPIO_SWPORT_DR_SIZE);
+	dirout = gpio->regs + GPIO_SWPORTA_DDR +
+		(port_idx * GPIO_SWPORT_DDR_SIZE);
+
+	err = bgpio_init(&port->bgc, gpio->dev, 4, dat, set, NULL, dirout,
+			 NULL, false);
+	if (err) {
+		dev_err(gpio->dev, "failed to init gpio chip for %s\n",
+			port_np->full_name);
+		return err;
+	}
+
+	port->bgc.gc.ngpio = ngpio;
+	port->bgc.gc.of_node = port_np;
+
+	/*
+	 * Only port A can provide interrupts in all configurations of the IP.
+	 */
+	if (port_idx == 0 &&
+	    of_property_read_bool(port_np, "interrupt-controller"))
+		dwapb_configure_irqs(gpio, port);
+
+	err = gpiochip_add(&port->bgc.gc);
+	if (err)
+		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
+			port_np->full_name);
+	else
+		port->is_registered = true;
+
+	return err;
+}
+
+static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
+{
+	unsigned int m;
+
+	for (m = 0; m < gpio->nr_ports; ++m)
+		if (gpio->ports[m].is_registered)
+			WARN_ON(gpiochip_remove(&gpio->ports[m].bgc.gc));
+}
+
+static int dwapb_gpio_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct dwapb_gpio *gpio;
+	struct device_node *np;
+	int err;
+	unsigned int offs = 0;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+	gpio->dev = &pdev->dev;
+
+	gpio->nr_ports = of_get_child_count(pdev->dev.of_node);
+	if (!gpio->nr_ports) {
+		err = -EINVAL;
+		goto out_err;
+	}
+	gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports *
+				   sizeof(*gpio->ports), GFP_KERNEL);
+	if (!gpio->ports) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	gpio->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio->regs)) {
+		err = PTR_ERR(gpio->regs);
+		goto out_err;
+	}
+
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		err = dwapb_gpio_add_port(gpio, np, offs++);
+		if (err)
+			goto out_unregister;
+	}
+	platform_set_drvdata(pdev, gpio);
+
+	return 0;
+
+out_unregister:
+	dwapb_gpio_unregister(gpio);
+	dwapb_irq_teardown(gpio);
+
+out_err:
+	return err;
+}
+
+static int dwapb_gpio_remove(struct platform_device *pdev)
+{
+	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
+
+	dwapb_gpio_unregister(gpio);
+	dwapb_irq_teardown(gpio);
+
+	return 0;
+}
+
+static const struct of_device_id dwapb_of_match[] = {
+	{ .compatible = "snps,dw-apb-gpio" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dwapb_of_match);
+
+static struct platform_driver dwapb_gpio_driver = {
+	.driver		= {
+		.name	= "gpio-dwapb",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(dwapb_of_match),
+	},
+	.probe		= dwapb_gpio_probe,
+	.remove		= dwapb_gpio_remove,
+};
+
+module_platform_driver(dwapb_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
-- 
1.7.9.5

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[delicious quinoa]

On Fri, Dec 6, 2013 at 3:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> From: Alan Tull <atull@altera.com>
>
> Hi Linus,
>
> If you don't have any further comments, can you take this patch?
>
> Alan

Basically, this is a driver that Jamie wrote and was upstreaming a few
years ago.  For some reason it never quite made it into the kernel.  I
picked it up, updated the interrupt support, posted it to the list and
went through some reviews with it.

This driver has been reviewed, I hope it can make it into the kernel.

Alan

>
> Jamie Iles (1):
>   gpio: add a driver for the Synopsys DesignWare APB GPIO block
>
>  .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   59 +++
>  drivers/gpio/Kconfig                               |    9 +
>  drivers/gpio/Makefile                              |    1 +
>  drivers/gpio/gpio-dwapb.c                          |  415 ++++++++++++++++++++
>  4 files changed, 484 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
>  create mode 100644 drivers/gpio/gpio-dwapb.c
>
> --
> 1.7.9.5
>
>

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[Steffen Trumtrar]

Hi Alan!

On Wed, Dec 11, 2013 at 02:15:25PM -0600, delicious quinoa wrote:
> On Fri, Dec 6, 2013 at 3:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
> > From: Alan Tull <atull@altera.com>
> >
> > Hi Linus,
> >
> > If you don't have any further comments, can you take this patch?
> >
> > Alan
> 
> Basically, this is a driver that Jamie wrote and was upstreaming a few
> years ago.  For some reason it never quite made it into the kernel.  I
> picked it up, updated the interrupt support, posted it to the list and
> went through some reviews with it.
> 
> This driver has been reviewed, I hope it can make it into the kernel.
> 
> Alan
> 

Sorry for chiming in late. I tested your driver on the Sockit and the GPIOs
work fine (basically just tested the 4 HPS LEDS that are on this board).

With the interrupt support I had a little trouble though:
The Sockit has an ADXL345 on the i2c bus with a gpio as interrupt line.

First: When you use "interrupt-parent", you have to specify the gpio node,
rather than the gpio-bank that has the interrupt-controller property. Is
that expected? I'm not really sure if that is normal behavior...

Second: The interrupt is registered as "GIC 37", which is a real interrupt on
the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
range). The interrupt from the gpiochip itself isn't registered at all ?!

Third: The interrupt didn't work. But that might also be my failure. I guess/hope
you tested interrupts successfully?!

(Fourth: small typo in the documentation example: "snps,nr-gpio" instead of "snps,nr-gpios")

Regards,
Steffen


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[delicious quinoa]

On Thu, Dec 12, 2013 at 3:08 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Sorry for chiming in late. I tested your driver on the Sockit and the GPIOs
> work fine (basically just tested the 4 HPS LEDS that are on this board).
>
> With the interrupt support I had a little trouble though:
> The Sockit has an ADXL345 on the i2c bus with a gpio as interrupt line.
>
> First: When you use "interrupt-parent", you have to specify the gpio node,
> rather than the gpio-bank that has the interrupt-controller property. Is
> that expected? I'm not really sure if that is normal behavior...

I think it can work several ways.  In my device tree, the gic is one
level up in the device tree above the gpio.  So I don't have to
specify interrupt-controller in the gpio node or in the bank.

>
> Second: The interrupt is registered as "GIC 37", which is a real interrupt on
> the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
> range). The interrupt from the gpiochip itself isn't registered at all ?!

The bindings documentation is an example.  You will need to adjust the
device tree bindings values to suit your board.  For example, on my
socfpga cyclone5 board, I needed:

        gpio0: gpio@ff708000 {
                compatible = "snps,dw-apb-gpio";
                reg = <0xff708000 0x1000>;
                #address-cells = <1>;
                #size-cells = <0>;

                port0a: gpio-controller@0 {
                        compatible = "snps,dw-apb-gpio-port";
                        gpio-controller;
                        #gpio-cells = <2>;
                        snps,nr-gpios = <29>;
                        reg = <0>;
                        interrupt-controller;
                        interrupts = <0 164 4>;
                        #interrupt-cells = <2>;
                        clocks = <&per_base_clk>;
                        };
                };

(so on for gpio1 and gpio2)

>
> Third: The interrupt didn't work. But that might also be my failure. I guess/hope
> you tested interrupts successfully?!

Yes, tested and working for me, including the interrupts.

>
> (Fourth: small typo in the documentation example: "snps,nr-gpio" instead of "snps,nr-gpios")

Yes, should be the plural form.

Thanks for testing!

Alan

>
> Regards,
> Steffen
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[delicious quinoa]

On Thu, Dec 12, 2013 at 3:08 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:

> Second: The interrupt is registered as "GIC 37", which is a real interrupt on
> the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
> range). The interrupt from the gpiochip itself isn't registered at all ?!

Hi Stephen,

Did you export the gpio lines and set the edge in sysfs?  Because the
interrupts aren't allocated otherwise.

For instance:

root@socfpga_cyclone5:~# echo 195 > /sys/class/gpio/export
root@socfpga_cyclone5:~# echo rising > /sys/class/gpio/gpio195/edge

Now I can see a pretty nicely named interrupt in /proc/interrupts:

256:          0          0  gpio-dwapb  24  gpiolib

Alan Tull
Altera Corp

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[Steffen Trumtrar]

Hi!

On Thu, Jan 30, 2014 at 01:40:04PM -0600, delicious quinoa wrote:
> On Thu, Dec 12, 2013 at 3:08 AM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> 
> > Second: The interrupt is registered as "GIC 37", which is a real interrupt on
> > the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
> > range). The interrupt from the gpiochip itself isn't registered at all ?!
> 
> Hi Stephen,
> 
> Did you export the gpio lines and set the edge in sysfs?  Because the
> interrupts aren't allocated otherwise.
> 
> For instance:
> 
> root@socfpga_cyclone5:~# echo 195 > /sys/class/gpio/export
> root@socfpga_cyclone5:~# echo rising > /sys/class/gpio/gpio195/edge
> 
> Now I can see a pretty nicely named interrupt in /proc/interrupts:
> 
> 256:          0          0  gpio-dwapb  24  gpiolib
> 

I didn't try that and I think this behaviour is pretty uncommon.
This should be fixed in the driver. I never wrote a gpiochip-driver,
so I don't know what is missing, but maybe just some functioncall ?!
All other drivers I came across have that entry from probing without
any fiddling.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[delicious quinoa]

On Thu, Jan 30, 2014 at 2:50 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Hi!
>
> On Thu, Jan 30, 2014 at 01:40:04PM -0600, delicious quinoa wrote:
>> On Thu, Dec 12, 2013 at 3:08 AM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>>
>> > Second: The interrupt is registered as "GIC 37", which is a real interrupt on
>> > the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
>> > range). The interrupt from the gpiochip itself isn't registered at all ?!
>>
>> Hi Stephen,
>>
>> Did you export the gpio lines and set the edge in sysfs?  Because the
>> interrupts aren't allocated otherwise.
>>
>> For instance:
>>
>> root@socfpga_cyclone5:~# echo 195 > /sys/class/gpio/export
>> root@socfpga_cyclone5:~# echo rising > /sys/class/gpio/gpio195/edge
>>
>> Now I can see a pretty nicely named interrupt in /proc/interrupts:
>>
>> 256:          0          0  gpio-dwapb  24  gpiolib
>>
>
> I didn't try that and I think this behaviour is pretty uncommon.
> This should be fixed in the driver. I never wrote a gpiochip-driver,
> so I don't know what is missing, but maybe just some functioncall ?!
> All other drivers I came across have that entry from probing without
> any fiddling.

Hi Steffen,

Do you mean 'all other gpio drivers' or 'all other non-gpio drivers'?

This is the behavior that is implemented in the community gpio
framework drivers/gpio/gpiolib.c, not anything special implemented in
this dw gpio driver.

It's documented in Documentation/gpio/sysfs.txt and
Documentation/ABI/testing/sysfs-gpio.

You get userspace control of a gpio by 'export'ing it in sysfs.  And
then by default, the interrupt edge is set to 'none' (no irq) until
you set the edge in sysfs.

Alan

>
> Thanks,
> Steffen
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[Steffen Trumtrar]

On Thu, Jan 30, 2014 at 03:15:11PM -0600, delicious quinoa wrote:
> On Thu, Jan 30, 2014 at 2:50 PM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > Hi!
> >
> > On Thu, Jan 30, 2014 at 01:40:04PM -0600, delicious quinoa wrote:
> >> On Thu, Dec 12, 2013 at 3:08 AM, Steffen Trumtrar
> >> <s.trumtrar@pengutronix.de> wrote:
> >>
> >> > Second: The interrupt is registered as "GIC 37", which is a real interrupt on
> >> > the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
> >> > range). The interrupt from the gpiochip itself isn't registered at all ?!
> >>
> >> Hi Stephen,
> >>
> >> Did you export the gpio lines and set the edge in sysfs?  Because the
> >> interrupts aren't allocated otherwise.
> >>
> >> For instance:
> >>
> >> root@socfpga_cyclone5:~# echo 195 > /sys/class/gpio/export
> >> root@socfpga_cyclone5:~# echo rising > /sys/class/gpio/gpio195/edge
> >>
> >> Now I can see a pretty nicely named interrupt in /proc/interrupts:
> >>
> >> 256:          0          0  gpio-dwapb  24  gpiolib
> >>
> >
> > I didn't try that and I think this behaviour is pretty uncommon.
> > This should be fixed in the driver. I never wrote a gpiochip-driver,
> > so I don't know what is missing, but maybe just some functioncall ?!
> > All other drivers I came across have that entry from probing without
> > any fiddling.
> 
> Hi Steffen,
> 
> Do you mean 'all other gpio drivers' or 'all other non-gpio drivers'?

I meant gpio drivers (at least the ones I have used).

> 
> This is the behavior that is implemented in the community gpio
> framework drivers/gpio/gpiolib.c, not anything special implemented in
> this dw gpio driver.
> 
> It's documented in Documentation/gpio/sysfs.txt and
> Documentation/ABI/testing/sysfs-gpio.
> 
> You get userspace control of a gpio by 'export'ing it in sysfs.  And
> then by default, the interrupt edge is set to 'none' (no irq) until
> you set the edge in sysfs.
> 

Hm, okay...for GPIOs I'm with you. But when I specify a gpio as
interrupt for a device, I have to first export it manually before I can
use the device? Sounds weird.

Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v9] gpio: add a driver for Synopsys DesignWare APB GPIO

[delicious quinoa]

On Thu, Jan 30, 2014 at 3:32 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> On Thu, Jan 30, 2014 at 03:15:11PM -0600, delicious quinoa wrote:
>> On Thu, Jan 30, 2014 at 2:50 PM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>> > Hi!
>> >
>> > On Thu, Jan 30, 2014 at 01:40:04PM -0600, delicious quinoa wrote:
>> >> On Thu, Dec 12, 2013 at 3:08 AM, Steffen Trumtrar
>> >> <s.trumtrar@pengutronix.de> wrote:
>> >>
>> >> > Second: The interrupt is registered as "GIC 37", which is a real interrupt on
>> >> > the Socfpga. I would expect it to be marked as "GPIO 2xx" (or something in that
>> >> > range). The interrupt from the gpiochip itself isn't registered at all ?!
>> >>
>> >> Hi Stephen,
>> >>
>> >> Did you export the gpio lines and set the edge in sysfs?  Because the
>> >> interrupts aren't allocated otherwise.
>> >>
>> >> For instance:
>> >>
>> >> root@socfpga_cyclone5:~# echo 195 > /sys/class/gpio/export
>> >> root@socfpga_cyclone5:~# echo rising > /sys/class/gpio/gpio195/edge
>> >>
>> >> Now I can see a pretty nicely named interrupt in /proc/interrupts:
>> >>
>> >> 256:          0          0  gpio-dwapb  24  gpiolib
>> >>
>> >
>> > I didn't try that and I think this behaviour is pretty uncommon.
>> > This should be fixed in the driver. I never wrote a gpiochip-driver,
>> > so I don't know what is missing, but maybe just some functioncall ?!
>> > All other drivers I came across have that entry from probing without
>> > any fiddling.
>>
>> Hi Steffen,
>>
>> Do you mean 'all other gpio drivers' or 'all other non-gpio drivers'?
>
> I meant gpio drivers (at least the ones I have used).
>
>>
>> This is the behavior that is implemented in the community gpio
>> framework drivers/gpio/gpiolib.c, not anything special implemented in
>> this dw gpio driver.
>>
>> It's documented in Documentation/gpio/sysfs.txt and
>> Documentation/ABI/testing/sysfs-gpio.
>>
>> You get userspace control of a gpio by 'export'ing it in sysfs.  And
>> then by default, the interrupt edge is set to 'none' (no irq) until
>> you set the edge in sysfs.
>>
>
> Hm, okay...for GPIOs I'm with you. But when I specify a gpio as
> interrupt for a device, I have to first export it manually before I can
> use the device? Sounds weird.
>
> Steffen

Seems normal usage both in userspace and in kernel drivers that need a gpio.

Looking at other kernel drivers that use a gpio, I can see examples
that do gpio_request() to get the gpio that they care about.  Then
gpio_to_irq() and request_irq().  If you grep for 'gpio_to_irq' then
look to see how they got the gpio, you'll see it pretty quickly.

Alan