[PATCH] gpio: support for Synopsys DesignWare APB GPIO

[PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Jamie Iles]

This patch adds support for the Synopsys DesignWare APB GPIO controller
that can be found in some ARM systems.  The controller supports up to
4x32 bit ports and port A is capable of raising interrupts.

Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/gpio/Kconfig                  |    8 +
 drivers/gpio/Makefile                 |    1 +
 drivers/gpio/dw_gpio.c                |  573 +++++++++++++++++++++++++++++++++
 include/linux/platform_data/dw_gpio.h |   24 ++
 4 files changed, 606 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/dw_gpio.c
 create mode 100644 include/linux/platform_data/dw_gpio.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d3b2953..135d996 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -131,6 +131,14 @@ config GPIO_VX855
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+config GPIO_DW
+	bool "Synopsys DesignWare APB GPIO controller"
+	depends on GPIOLIB
+	help
+	  Say M here to build support for the Synopsys DesignWare APB
+	  GPIO controller found in some ARM systems.  This GPIO controller
+	  supports upto 4 ports of GPIO and GPIO interrupts on port A.
+
 comment "I2C GPIO expanders:"
 
 config GPIO_MAX7300
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becef59..377d8b9 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -43,3 +43,4 @@ obj-$(CONFIG_GPIO_SX150X)	+= sx150x.o
 obj-$(CONFIG_GPIO_VX855)	+= vx855_gpio.o
 obj-$(CONFIG_GPIO_ML_IOH)	+= ml_ioh_gpio.o
 obj-$(CONFIG_AB8500_GPIO)       += ab8500-gpio.o
+obj-$(CONFIG_GPIO_DW)		+= dw_gpio.o
diff --git a/drivers/gpio/dw_gpio.c b/drivers/gpio/dw_gpio.c
new file mode 100644
index 0000000..1a474df
--- /dev/null
+++ b/drivers/gpio/dw_gpio.c
@@ -0,0 +1,573 @@
+/*
+ * Driver for the Synopsys DesignWare GPIO Controller.
+ *
+ * Copyright (C) 2011 Picochip Ltd, 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.
+ */
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/dw_gpio.h>
+
+struct dw_gpio_chip {
+	struct gpio_chip	chip;
+	void __iomem		*iobase;
+	int			porta_end;
+	int			portb_end;
+	int			portc_end;
+	int			portd_end;
+	int			irq_base;
+	int			nr_irq;
+	spinlock_t		lock;
+};
+
+static inline struct dw_gpio_chip *to_dw_gpio_chip(struct gpio_chip *chip)
+{
+	return container_of(chip, struct dw_gpio_chip, chip);
+}
+
+#define GPIO_SW_PORT_A_DR_REG_OFFSET		0x00
+#define GPIO_SW_PORT_A_DDR_REG_OFFSET		0x04
+#define GPIO_SW_PORT_A_CTL_REG_OFFSET		0x08
+#define GPIO_SW_PORT_B_DR_REG_OFFSET		0x0C
+#define GPIO_SW_PORT_B_DDR_REG_OFFSET		0x10
+#define GPIO_SW_PORT_B_CTL_REG_OFFSET		0x14
+#define GPIO_SW_PORT_C_DR_REG_OFFSET		0x18
+#define GPIO_SW_PORT_C_DDR_REG_OFFSET		0x1C
+#define GPIO_SW_PORT_C_CTL_REG_OFFSET		0x20
+#define GPIO_SW_PORT_D_DR_REG_OFFSET		0x24
+#define GPIO_SW_PORT_D_DDR_REG_OFFSET		0x28
+#define GPIO_SW_PORT_D_CTL_REG_OFFSET		0x2C
+
+#define GPIO_INT_EN_REG_OFFSET			0x30
+#define GPIO_INT_MASK_REG_OFFSET		0x34
+#define GPIO_INT_TYPE_LEVEL_REG_OFFSET		0x38
+#define GPIO_INT_POLARITY_REG_OFFSET		0x3c
+#define GPIO_INT_STATUS_REG_OFFSET		0x40
+#define GPIO_PORT_A_EOI_REG_OFFSET		0x4c
+
+#define GPIO_SW_PORT_A_EXT_REG_OFFSET		0x50
+#define GPIO_SW_PORT_B_EXT_REG_OFFSET		0x54
+#define GPIO_SW_PORT_C_EXT_REG_OFFSET		0x58
+#define GPIO_SW_PORT_D_EXT_REG_OFFSET		0x5C
+
+#define GPIO_CFG1_REG_OFFSET		0x74
+#define GPIO_NR_PORTS_SHIFT		2
+#define GPIO_NR_PORTS_MASK		(0x3 << GPIO_NR_PORTS_SHIFT)
+
+#define GPIO_CFG2_REG_OFFSET		0x70
+#define GPIO_PORTA_WIDTH_SHIFT		0
+#define GPIO_PORTA_WIDTH_MASK		(0x1F << GPIO_PORTA_WIDTH_SHIFT)
+#define GPIO_PORTB_WIDTH_SHIFT		5
+#define GPIO_PORTB_WIDTH_MASK		(0x1F << GPIO_PORTB_WIDTH_SHIFT)
+#define GPIO_PORTC_WIDTH_SHIFT		10
+#define GPIO_PORTC_WIDTH_MASK		(0x1F << GPIO_PORTC_WIDTH_SHIFT)
+#define GPIO_PORTD_WIDTH_SHIFT		15
+#define GPIO_PORTD_WIDTH_MASK		(0x1F << GPIO_PORTD_WIDTH_SHIFT)
+
+/*
+ * Get the port mapping for the controller.  There are 4 possible ports that
+ * we can use but some platforms may reserve ports for hardware purposes that
+ * cannot be used as GPIO so we allow these to be masked off.
+ */
+static void dw_gpio_configure_ports(struct dw_gpio_chip *dw,
+				    unsigned long port_mask)
+{
+	unsigned long cfg1, cfg2;
+	int nr_ports;
+
+	cfg1 = readl(dw->iobase + GPIO_CFG1_REG_OFFSET);
+	cfg2 = readl(dw->iobase + GPIO_CFG2_REG_OFFSET);
+
+	nr_ports = (cfg1 & GPIO_NR_PORTS_MASK) >> GPIO_NR_PORTS_SHIFT;
+
+	if (port_mask & DW_GPIO_PORTA_MASK) {
+		dw->porta_end = ((cfg2 & GPIO_PORTA_WIDTH_MASK) >>
+				 GPIO_PORTA_WIDTH_SHIFT) + 1;
+		if (nr_ports == 1)
+			return;
+	} else
+		dw->porta_end = 0;
+
+	if (port_mask & DW_GPIO_PORTB_MASK) {
+		dw->portb_end = ((cfg2 & GPIO_PORTB_WIDTH_MASK) >>
+				 GPIO_PORTB_WIDTH_SHIFT) + 1 + dw->porta_end;
+		if (nr_ports == 2)
+			return;
+	} else
+		dw->portb_end = dw->porta_end;
+
+	if (port_mask & DW_GPIO_PORTC_MASK) {
+		dw->portc_end = ((cfg2 & GPIO_PORTC_WIDTH_MASK) >>
+				 GPIO_PORTC_WIDTH_SHIFT) + 1 + dw->portb_end;
+		if (nr_ports == 3)
+			return;
+	} else
+		dw->portc_end = dw->portb_end;
+
+	if (port_mask & DW_GPIO_PORTD_MASK)
+		dw->portd_end = ((cfg2 & GPIO_PORTD_WIDTH_MASK) >>
+				 GPIO_PORTD_WIDTH_SHIFT) + 1 + dw->portc_end;
+	else
+		dw->portd_end = dw->portb_end;
+}
+
+#define __DWGPIO_REG(__chip, __gpio_nr, __reg)				\
+	({								\
+		void __iomem *ret = NULL;				\
+		if ((__gpio_nr) <= (__chip)->porta_end)			\
+			ret = ((__chip)->iobase +			\
+				GPIO_SW_PORT_A_##__reg##_REG_OFFSET);	\
+		else if ((__gpio_nr) <= (__chip)->portb_end)		\
+			ret = ((__chip)->iobase +			\
+				GPIO_SW_PORT_B_##__reg##_REG_OFFSET);	\
+		else if ((__gpio_nr) <= (__chip)->portc_end)		\
+			ret = ((__chip)->iobase +			\
+				GPIO_SW_PORT_C_##__reg##_REG_OFFSET);	\
+		else							\
+			ret = ((__chip)->iobase +			\
+				GPIO_SW_PORT_D_##__reg##_REG_OFFSET);	\
+		ret;							\
+	})
+
+#define DWGPIO_DR(__chip, __gpio_nr)	__DWGPIO_REG((__chip), \
+						     (__gpio_nr), DR)
+#define DWGPIO_DDR(__chip, __gpio_nr)	__DWGPIO_REG((__chip), \
+						     (__gpio_nr), DDR)
+#define DWGPIO_CTL(__chip, __gpio_nr)	__DWGPIO_REG((__chip), \
+						     (__gpio_nr), CTL)
+#define DWGPIO_EXT(__chip, __gpio_nr)	__DWGPIO_REG((__chip), \
+						     (__gpio_nr), EXT)
+#define INT_EN_REG(__chip)		((__chip)->iobase + \
+					 GPIO_INT_EN_REG_OFFSET)
+#define INT_MASK_REG(__chip)		((__chip)->iobase + \
+					 GPIO_INT_MASK_REG_OFFSET)
+#define INT_TYPE_REG(__chip)		((__chip)->iobase + \
+					 GPIO_INT_TYPE_LEVEL_REG_OFFSET)
+#define INT_POLARITY_REG(__chip)	((__chip)->iobase + \
+					 GPIO_INT_POLARITY_REG_OFFSET)
+#define INT_STATUS_REG(__chip)		((__chip)->iobase + \
+					 GPIO_INT_STATUS_REG_OFFSET)
+#define EOI_REG(__chip)			((__chip)->iobase + \
+					 GPIO_PORT_A_EOI_REG_OFFSET)
+
+static inline unsigned dw_gpio_bit_offs(struct dw_gpio_chip *dw,
+					unsigned offset)
+{
+	/*
+	 * The gpios are controlled via three sets of registers. The register
+	 * addressing is already taken care of by the __DWGPIO_REG macro,
+	 * this takes care of the bit offsets within each register.
+	 */
+	if (offset <= dw->porta_end)
+		return offset;
+	else if (offset <= dw->portb_end)
+		return offset - dw->porta_end;
+	else if (offset <= dw->portc_end)
+		return offset - dw->portb_end;
+	else
+		return offset - dw->portc_end;
+}
+
+static int dwgpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
+	void __iomem *ddr = DWGPIO_DDR(dw, offset);
+	void __iomem *cr = DWGPIO_CTL(dw, offset);
+	unsigned long flags, val, bit_offset = dw_gpio_bit_offs(dw, offset);
+
+	spin_lock_irqsave(&dw->lock, flags);
+	/* Mark the pin as an output. */
+	val = readl(ddr);
+	val &= ~(1 << bit_offset);
+	writel(val, ddr);
+
+	/* Set the pin as software controlled. */
+	val = readl(cr);
+	val &= ~(1 << bit_offset);
+	writel(val, cr);
+	spin_unlock_irqrestore(&dw->lock, flags);
+
+	return 0;
+}
+
+static int dwgpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
+	void __iomem *ext = DWGPIO_EXT(dw, offset);
+	unsigned long bit_offset = dw_gpio_bit_offs(dw, offset);
+
+	return !!(readl(ext) & (1 << bit_offset));
+}
+
+static void dwgpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
+	void __iomem *dr = DWGPIO_DR(dw, offset);
+	unsigned long val, flags, bit_offset = dw_gpio_bit_offs(dw, offset);
+
+	spin_lock_irqsave(&dw->lock, flags);
+	val = readl(dr);
+	val &= ~(1 << bit_offset);
+	val |= (!!value << bit_offset);
+	writel(val, dr);
+	spin_unlock_irqrestore(&dw->lock, flags);
+}
+
+static int dwgpio_direction_output(struct gpio_chip *chip, unsigned offset,
+				    int value)
+{
+	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
+	void __iomem *ddr = DWGPIO_DDR(dw, offset);
+	void __iomem *cr = DWGPIO_CTL(dw, offset);
+	unsigned long flags, val, bit_offset = dw_gpio_bit_offs(dw, offset);
+
+	/* Set the value first so we don't glitch. */
+	dwgpio_set(chip, offset, value);
+
+	spin_lock_irqsave(&dw->lock, flags);
+	/* Mark the pin as an output. */
+	val = readl(ddr);
+	val |= (1 << bit_offset);
+	writel(val, ddr);
+
+	/* Set the pin as software controlled. */
+	val = readl(cr);
+	val &= ~(1 << bit_offset);
+	writel(val, cr);
+	spin_unlock_irqrestore(&dw->lock, flags);
+
+	return 0;
+}
+
+static int dwgpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
+
+	return dw->nr_irq && offset < dw->nr_irq ? dw->irq_base + offset :
+		-EINVAL;
+}
+
+static void dwgpio_irq_enable(struct irq_data *d)
+{
+	int gpio = irq_to_gpio(d->irq);
+	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
+	void *port_inten = INT_EN_REG(dw);
+	unsigned long val, flags;
+
+	spin_lock_irqsave(&dw->lock, flags);
+	val = readl(port_inten);
+	val |= (1 << gpio);
+	writel(val, port_inten);
+	spin_unlock_irqrestore(&dw->lock, flags);
+}
+
+static void dwgpio_irq_disable(struct irq_data *d)
+{
+	int gpio = irq_to_gpio(d->irq);
+	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
+	void __iomem *port_inten = INT_EN_REG(dw);
+	unsigned long val, flags;
+
+	spin_lock_irqsave(&dw->lock, flags);
+	val = readl(port_inten);
+	val &= ~(1 << gpio);
+	writel(val, port_inten);
+	spin_unlock_irqrestore(&dw->lock, flags);
+}
+
+static void dwgpio_irq_mask(struct irq_data *d)
+{
+	int gpio = irq_to_gpio(d->irq);
+	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
+	void __iomem *port_mask = INT_MASK_REG(dw);
+	unsigned long val, flags;
+
+	spin_lock_irqsave(&dw->lock, flags);
+	val = readl(port_mask);
+	val |= (1 << gpio);
+	writel(val, port_mask);
+	spin_unlock_irqrestore(&dw->lock, flags);
+}
+
+static void dwgpio_irq_ack(struct irq_data *d)
+{
+	int gpio = irq_to_gpio(d->irq);
+	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
+	void __iomem *port_intmask = INT_MASK_REG(dw);
+	void __iomem *port_eoi = EOI_REG(dw);
+	void __iomem *port_inttype_level = INT_TYPE_REG(dw);
+	unsigned long val, flags;
+
+	spin_lock_irqsave(&dw->lock, flags);
+	val = readl(port_inttype_level);
+	if (val & (1 << gpio)) {
+		/* Edge-sensitive */
+		val = readl(port_eoi);
+		val |= (1 << gpio);
+		writel(val, port_eoi);
+	} else {
+		/* Level-sensitive */
+		val = readl(port_intmask);
+		val |= (1 << gpio);
+		writel(val, port_intmask);
+	}
+	spin_unlock_irqrestore(&dw->lock, flags);
+}
+
+static void dwgpio_irq_unmask(struct irq_data *d)
+{
+	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
+	int gpio = irq_to_gpio(d->irq);
+	void __iomem *port_intmask = INT_MASK_REG(dw);
+	unsigned long val, flags;
+
+	spin_lock_irqsave(&dw->lock, flags);
+	val = readl(port_intmask);
+	val &= ~(1 << gpio);
+	writel(val, port_intmask);
+	spin_unlock_irqrestore(&dw->lock, flags);
+}
+
+static int dwgpio_irq_set_type(struct irq_data *d,
+			       unsigned int trigger)
+{
+	int gpio = irq_to_gpio(d->irq);
+	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
+	void __iomem *port_inttype_level = INT_TYPE_REG(dw);
+	void __iomem *port_int_polarity = INT_POLARITY_REG(dw);
+	unsigned long level, polarity, flags;
+
+	if (trigger & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
+			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		return -EINVAL;
+
+	spin_lock_irqsave(&dw->lock, flags);
+	level = readl(port_inttype_level);
+	polarity = readl(port_int_polarity);
+
+	if (trigger & IRQ_TYPE_EDGE_RISING) {
+		level	    |= (1 << gpio);
+		polarity    |= (1 << gpio);
+	} else if (trigger & IRQ_TYPE_EDGE_FALLING) {
+		level	    |= (1 << gpio);
+		polarity    &= ~(1 << gpio);
+	} else if (trigger & IRQ_TYPE_LEVEL_HIGH) {
+		level	    &= ~(1 << gpio);
+		polarity    |= (1 << gpio);
+	} else if (trigger & IRQ_TYPE_LEVEL_LOW) {
+		level	    &= ~(1 << gpio);
+		polarity    &= ~(1 << gpio);
+	}
+
+	writel(level, port_inttype_level);
+	writel(polarity, port_int_polarity);
+	spin_unlock_irqrestore(&dw->lock, flags);
+
+	return 0;
+}
+
+static struct irq_chip dwgpio_irqchip = {
+	.name		= "dwgpio",
+	.irq_ack	= dwgpio_irq_ack,
+	.irq_mask	= dwgpio_irq_mask,
+	.irq_unmask	= dwgpio_irq_unmask,
+	.irq_enable	= dwgpio_irq_enable,
+	.irq_disable	= dwgpio_irq_disable,
+	.irq_set_type	= dwgpio_irq_set_type,
+};
+
+static void dw_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	struct dw_gpio_chip *dw = irq_get_handler_data(irq);
+	int i;
+
+	/*
+	 * Mask and ack the interrupt in the parent interrupt controller
+	 * before handling it.
+	 */
+	desc->irq_data.chip->irq_mask(&desc->irq_data);
+	desc->irq_data.chip->irq_ack(&desc->irq_data);
+	for (;;) {
+		unsigned long status = readl(INT_STATUS_REG(dw));
+
+		if (!status)
+			break;
+		writel(status, EOI_REG(dw));
+
+		for_each_set_bit(i, &status, 8)
+			generic_handle_irq(dw->irq_base + i);
+	}
+	desc->irq_data.chip->irq_unmask(&desc->irq_data);
+}
+
+/*
+ * We want to enable/disable interrupts for the GPIO pins through the GPIO
+ * block itself.  The current configuration assumes a 1-to-1 mapping of GPIO
+ * interrupt sources to IRQ numbers.  We use a chained handler as the GPIO
+ * IRQ's may pass through a separate VIC on some systems so need to be
+ * enabled/disabled there too.
+ *
+ * The chained handler simply converts from the virtual IRQ handler to the
+ * real interrupt source and calls the GPIO IRQ handler.
+ */
+static void dwgpio_irq_init(struct dw_gpio_chip *dw,
+			    struct resource *demux_irqs,
+			    struct resource *irqs)
+{
+	int i;
+
+	if (!demux_irqs && !irqs)
+		return;
+
+	if (!demux_irqs || !irqs ||
+	    resource_size(demux_irqs) != resource_size(irqs)) {
+		dev_warn(dw->chip.dev, "unsupported IRQ demuxing configuration, continuing without GPIO IRQ support\n");
+		return;
+	}
+
+	dw->irq_base = irqs->start;
+
+	writel(0, INT_EN_REG(dw));
+	writel(~0, EOI_REG(dw));
+	for (i = irqs->start; i <= irqs->end; ++i) {
+		irq_set_chip_and_handler_name(i, &dwgpio_irqchip,
+					      handle_simple_irq, "demux");
+		irq_set_chip_data(i, dw);
+		irq_set_status_flags(i, IRQ_TYPE_EDGE_BOTH |
+					IRQ_TYPE_LEVEL_HIGH |
+					IRQ_TYPE_LEVEL_LOW);
+		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+	}
+
+	for (i = demux_irqs->start; i <= demux_irqs->end; ++i) {
+		irq_set_handler_data(i, dw);
+		irq_set_chained_handler(i, dw_gpio_irq_handler);
+		set_irq_flags(i, IRQF_VALID);
+	}
+}
+
+static int __devinit dwgpio_probe(struct platform_device *pdev)
+{
+	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct resource *demux_irqs, *irqs;
+	struct dw_gpio_chip *dw = devm_kzalloc(&pdev->dev, sizeof(*dw),
+					       GFP_KERNEL);
+	int err;
+	struct dw_gpio_pdata *pdata = pdev->dev.platform_data;
+
+	if (!dw)
+		return -ENOMEM;
+
+	if (!mem || !pdata)
+		return -ENODEV;
+
+	if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+				     "dwgpio"))
+		return -EBUSY;
+
+	dw->iobase = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
+	if (!dw->iobase)
+		return -ENOMEM;
+
+	dw_gpio_configure_ports(dw, pdata->port_mask);
+
+	dw->chip.direction_input	= dwgpio_direction_input;
+	dw->chip.direction_output	= dwgpio_direction_output;
+	dw->chip.get			= dwgpio_get;
+	dw->chip.set			= dwgpio_set;
+	dw->chip.to_irq			= dwgpio_to_irq;
+	dw->chip.owner			= THIS_MODULE;
+	dw->chip.base			= pdata->gpio_base;
+	dw->chip.ngpio			= pdata->ngpio;
+	dw->chip.dev			= &pdev->dev;
+	spin_lock_init(&dw->lock);
+
+	err = gpiochip_add(&dw->chip);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register dwgpio chip\n");
+		return err;
+	}
+
+	demux_irqs = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	irqs = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+	dwgpio_irq_init(dw, demux_irqs, irqs);
+	platform_set_drvdata(pdev, dw);
+
+	return 0;
+}
+
+static void dwgpio_irq_stop(struct dw_gpio_chip *dw,
+			    struct resource *demux_irqs,
+			    struct resource *irqs)
+{
+	int i;
+
+	if (!demux_irqs || !irqs ||
+	    resource_size(demux_irqs) != resource_size(irqs))
+		return;
+
+	for (i = irqs->start; i < irqs->end; ++i) {
+		irq_set_chip(i, NULL);
+		irq_set_chip_data(i, NULL);
+	}
+
+	for (i = demux_irqs->start; i < demux_irqs->end; ++i) {
+		irq_set_chip(i, NULL);
+		irq_set_chip_data(i, NULL);
+	}
+}
+
+static int __devexit dwgpio_remove(struct platform_device *pdev)
+{
+	struct resource *irqs, *demux_irqs;
+	struct dw_gpio_chip *dw = platform_get_drvdata(pdev);
+	int err;
+
+	demux_irqs = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	irqs = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+	dwgpio_irq_stop(dw, demux_irqs, irqs);
+
+	err = gpiochip_remove(&dw->chip);
+	if (err) {
+		dev_err(&pdev->dev, "failed to remove gpio_chip\n");
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, NULL);
+
+out:
+	return err;
+}
+
+static struct platform_driver dwgpio_driver = {
+	.probe		= dwgpio_probe,
+	.remove		= __devexit_p(dwgpio_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "dwgpio",
+	},
+};
+
+static int __init dwgpio_init(void)
+{
+	return platform_driver_register(&dwgpio_driver);
+}
+module_init(dwgpio_init);
+
+static void __exit dwgpio_exit(void)
+{
+	platform_driver_unregister(&dwgpio_driver);
+}
+module_exit(dwgpio_exit);
+
+MODULE_AUTHOR("Jamie Iles");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Synopsys DesignWare GPIO driver");
diff --git a/include/linux/platform_data/dw_gpio.h b/include/linux/platform_data/dw_gpio.h
new file mode 100644
index 0000000..1ec64c5
--- /dev/null
+++ b/include/linux/platform_data/dw_gpio.h
@@ -0,0 +1,24 @@
+/*
+ * Platform data for the Synopsys DesignWare GPIO Controller.
+ *
+ * Copyright (C) 2011 Picochip Ltd, 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.
+ */
+#ifndef __DW_GPIO_PDATA_H__
+#define __DW_GPIO_PDATA_H__
+
+#define DW_GPIO_PORTA_MASK	(1 << 0)
+#define DW_GPIO_PORTB_MASK	(1 << 1)
+#define DW_GPIO_PORTC_MASK	(1 << 2)
+#define DW_GPIO_PORTD_MASK	(1 << 3)
+
+struct dw_gpio_pdata {
+	int		gpio_base;	/* First GPIO in the chip. */
+	int		ngpio;		/* Number of GPIO pins. */
+	unsigned long	port_mask;	/* Mask of ports to use for GPIO. */
+};
+
+#endif /* __DW_GPIO_PDATA_H__ */
-- 
1.7.4

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Randy Dunlap]

On Fri,  1 Apr 2011 14:47:18 +0100 Jamie Iles wrote:

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d3b2953..135d996 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -131,6 +131,14 @@ config GPIO_VX855
>  	  additional drivers must be enabled in order to use the
>  	  functionality of the device.
>  
> +config GPIO_DW
> +	bool "Synopsys DesignWare APB GPIO controller"
> +	depends on GPIOLIB
> +	help
> +	  Say M here to build support for the Synopsys DesignWare APB
> +	  GPIO controller found in some ARM systems.  This GPIO controller
> +	  supports upto 4 ports of GPIO and GPIO interrupts on port A.

	           up to

> +
>  comment "I2C GPIO expanders:"
>  
>  config GPIO_MAX7300


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Thomas Gleixner]

B1;2401;0cOn Fri, 1 Apr 2011, Jamie Iles wrote:

> This patch adds support for the Synopsys DesignWare APB GPIO controller
> that can be found in some ARM systems.  The controller supports up to

Not only ARM.

> 4x32 bit ports and port A is capable of raising interrupts.

So I assume that there are already a bunch of ARM SoC gpio
implementations in the arch/arm/* space which implement the very same
thing. Did you check that? And if yes, then it would be very helpful
to actually convert those platforms so there is a reason why we merge
another instance.

> +#define GPIO_SW_PORT_A_DR_REG_OFFSET		0x00
> +#define GPIO_SW_PORT_A_DDR_REG_OFFSET		0x04
> +#define GPIO_SW_PORT_A_CTL_REG_OFFSET		0x08
> +#define GPIO_SW_PORT_B_DR_REG_OFFSET		0x0C

I think this whole register offset thing needs to be runtime
configurable. We all know that SoC designers decide to move registers
around for pointless reasons or just change the stride.

> +#define GPIO_INT_EN_REG_OFFSET			0x30
> +#define GPIO_INT_MASK_REG_OFFSET		0x34

I bet there are implementations of the very same thing which have irq
support for more than one port. So that wants to be generalized as
well.

> +/*
> + * Get the port mapping for the controller.  There are 4 possible ports that
> + * we can use but some platforms may reserve ports for hardware purposes that
> + * cannot be used as GPIO so we allow these to be masked off.
> + */
> +static void dw_gpio_configure_ports(struct dw_gpio_chip *dw,
> +				    unsigned long port_mask)
> +{
> +	unsigned long cfg1, cfg2;
> +	int nr_ports;
> +
> +	cfg1 = readl(dw->iobase + GPIO_CFG1_REG_OFFSET);
> +	cfg2 = readl(dw->iobase + GPIO_CFG2_REG_OFFSET);
> +
> +	nr_ports = (cfg1 & GPIO_NR_PORTS_MASK) >> GPIO_NR_PORTS_SHIFT;
> +
> +	if (port_mask & DW_GPIO_PORTA_MASK) {
> +		dw->porta_end = ((cfg2 & GPIO_PORTA_WIDTH_MASK) >>
> +				 GPIO_PORTA_WIDTH_SHIFT) + 1;
> +		if (nr_ports == 1)
> +			return;

These nr_ports checks are pointless. This is a setup function and not
a hotpath.

> +	} else
> +		dw->porta_end = 0;
> +
> +	if (port_mask & DW_GPIO_PORTB_MASK) {
> +		dw->portb_end = ((cfg2 & GPIO_PORTB_WIDTH_MASK) >>
> +				 GPIO_PORTB_WIDTH_SHIFT) + 1 + dw->porta_end;
> +		if (nr_ports == 2)
> +			return;
> +	} else
> +		dw->portb_end = dw->porta_end;
> +
> +	if (port_mask & DW_GPIO_PORTC_MASK) {
> +		dw->portc_end = ((cfg2 & GPIO_PORTC_WIDTH_MASK) >>
> +				 GPIO_PORTC_WIDTH_SHIFT) + 1 + dw->portb_end;
> +		if (nr_ports == 3)
> +			return;
> +	} else
> +		dw->portc_end = dw->portb_end;
> +
> +	if (port_mask & DW_GPIO_PORTD_MASK)
> +		dw->portd_end = ((cfg2 & GPIO_PORTD_WIDTH_MASK) >>
> +				 GPIO_PORTD_WIDTH_SHIFT) + 1 + dw->portc_end;
> +	else
> +		dw->portd_end = dw->portb_end;
> +}

So you try to create a linear GPIO space which skips reserved
pins. What's the point of that? It adds useless overhead in the
hotpath for no value. It is confusing as there is no relation between
the HW pin and the gpio number anymore.

Why can't we just have a linear space where the reserved gpios are
simply not accessible?

> +#define __DWGPIO_REG(__chip, __gpio_nr, __reg)				\
> +	({								\
> +		void __iomem *ret = NULL;				\
> +		if ((__gpio_nr) <= (__chip)->porta_end)			\
> +			ret = ((__chip)->iobase +			\
> +				GPIO_SW_PORT_A_##__reg##_REG_OFFSET);	\
> +		else if ((__gpio_nr) <= (__chip)->portb_end)		\
> +			ret = ((__chip)->iobase +			\
> +				GPIO_SW_PORT_B_##__reg##_REG_OFFSET);	\
> +		else if ((__gpio_nr) <= (__chip)->portc_end)		\
> +			ret = ((__chip)->iobase +			\
> +				GPIO_SW_PORT_C_##__reg##_REG_OFFSET);	\
> +		else							\
> +			ret = ((__chip)->iobase +			\
> +				GPIO_SW_PORT_D_##__reg##_REG_OFFSET);	\
> +		ret;							\
> +	})
> +

> +static inline unsigned dw_gpio_bit_offs(struct dw_gpio_chip *dw,
> +					unsigned offset)
> +{
> +	/*
> +	 * The gpios are controlled via three sets of registers. The register
> +	 * addressing is already taken care of by the __DWGPIO_REG macro,

Shudder. A linear mapping would avoid all that ugliness.

> +	 * this takes care of the bit offsets within each register.
> +	 */
> +	if (offset <= dw->porta_end)
> +		return offset;
> +	else if (offset <= dw->portb_end)
> +		return offset - dw->porta_end;
> +	else if (offset <= dw->portc_end)
> +		return offset - dw->portb_end;
> +	else
> +		return offset - dw->portc_end;
> +}
> +
> +static int dwgpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
> +	void __iomem *ddr = DWGPIO_DDR(dw, offset);
> +	void __iomem *cr = DWGPIO_CTL(dw, offset);
> +	unsigned long flags, val, bit_offset = dw_gpio_bit_offs(dw, offset);
> +
> +	spin_lock_irqsave(&dw->lock, flags);
> +	/* Mark the pin as an output. */
> +	val = readl(ddr);

Why is this value not cached ?

> +	val &= ~(1 << bit_offset);
> +	writel(val, ddr);
> +
> +	/* Set the pin as software controlled. */
> +	val = readl(cr);

Ditto

> +	val &= ~(1 << bit_offset);
> +	writel(val, cr);
> +	spin_unlock_irqrestore(&dw->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int dwgpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
> +	void __iomem *ext = DWGPIO_EXT(dw, offset);
> +	unsigned long bit_offset = dw_gpio_bit_offs(dw, offset);
> +
> +	return !!(readl(ext) & (1 << bit_offset));
> +}
> +
> +static void dwgpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
> +	void __iomem *dr = DWGPIO_DR(dw, offset);
> +	unsigned long val, flags, bit_offset = dw_gpio_bit_offs(dw, offset);
> +
> +	spin_lock_irqsave(&dw->lock, flags);
> +	val = readl(dr);

Even more so. More instances ignored.

> +static void dwgpio_irq_enable(struct irq_data *d)
> +{
> +	int gpio = irq_to_gpio(d->irq);
> +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> +	void *port_inten = INT_EN_REG(dw);
> +	unsigned long val, flags;
> +
> +	spin_lock_irqsave(&dw->lock, flags);

This is nested into irq_desc->lock and wants to be a raw_spinlock
therefor.

> +	val = readl(port_inten);
> +	val |= (1 << gpio);
> +	writel(val, port_inten);
> +	spin_unlock_irqrestore(&dw->lock, flags);
> +}
> +
> +static void dwgpio_irq_disable(struct irq_data *d)
> +{
> +	int gpio = irq_to_gpio(d->irq);
> +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> +	void __iomem *port_inten = INT_EN_REG(dw);

All this information can be precomputed at setup time and supplied as
irq_chip_data to the irq in a form wich does not require to compute it
over and over.

> +static void dwgpio_irq_ack(struct irq_data *d)
> +{
> +	int gpio = irq_to_gpio(d->irq);
> +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> +	void __iomem *port_intmask = INT_MASK_REG(dw);
> +	void __iomem *port_eoi = EOI_REG(dw);
> +	void __iomem *port_inttype_level = INT_TYPE_REG(dw);
> +	unsigned long val, flags;
> +
> +	spin_lock_irqsave(&dw->lock, flags);
> +	val = readl(port_inttype_level);
> +	if (val & (1 << gpio)) {
> +		/* Edge-sensitive */
> +		val = readl(port_eoi);
> +		val |= (1 << gpio);
> +		writel(val, port_eoi);
> +	} else {
> +		/* Level-sensitive */
> +		val = readl(port_intmask);
> +		val |= (1 << gpio);
> +		writel(val, port_intmask);
> +	}

These conditionals are completely wrong. We use different irq_chip
implementations to distinguish different flows.

> +	spin_unlock_irqrestore(&dw->lock, flags);
> +}

> +static int dwgpio_irq_set_type(struct irq_data *d,
> +			       unsigned int trigger)
> +{
> +	int gpio = irq_to_gpio(d->irq);
> +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> +	void __iomem *port_inttype_level = INT_TYPE_REG(dw);
> +	void __iomem *port_int_polarity = INT_POLARITY_REG(dw);
> +	unsigned long level, polarity, flags;
> +
> +	if (trigger & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> +			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&dw->lock, flags);
> +	level = readl(port_inttype_level);
> +	polarity = readl(port_int_polarity);
> +
> +	if (trigger & IRQ_TYPE_EDGE_RISING) {
> +		level	    |= (1 << gpio);
> +		polarity    |= (1 << gpio);
> +	} else if (trigger & IRQ_TYPE_EDGE_FALLING) {
> +		level	    |= (1 << gpio);
> +		polarity    &= ~(1 << gpio);
> +	} else if (trigger & IRQ_TYPE_LEVEL_HIGH) {
> +		level	    &= ~(1 << gpio);
> +		polarity    |= (1 << gpio);
> +	} else if (trigger & IRQ_TYPE_LEVEL_LOW) {
> +		level	    &= ~(1 << gpio);
> +		polarity    &= ~(1 << gpio);
> +	}
> +
> +	writel(level, port_inttype_level);
> +	writel(polarity, port_int_polarity);

So this should set the proper irq chip for the edge/level flow, but
yes that's pointless. See below.

> +	spin_unlock_irqrestore(&dw->lock, flags);
> +
> +	return 0;
> +}
> +
> +static struct irq_chip dwgpio_irqchip = {
> +	.name		= "dwgpio",
> +	.irq_ack	= dwgpio_irq_ack,
> +	.irq_mask	= dwgpio_irq_mask,
> +	.irq_unmask	= dwgpio_irq_unmask,
> +	.irq_enable	= dwgpio_irq_enable,
> +	.irq_disable	= dwgpio_irq_disable,
> +	.irq_set_type	= dwgpio_irq_set_type,
> +};
> +
> +static void dw_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
> +{
> +	struct dw_gpio_chip *dw = irq_get_handler_data(irq);
> +	int i;
> +
> +	/*
> +	 * Mask and ack the interrupt in the parent interrupt controller
> +	 * before handling it.
> +	 */
> +	desc->irq_data.chip->irq_mask(&desc->irq_data);
> +	desc->irq_data.chip->irq_ack(&desc->irq_data);

Please get chip and irq_data with the proper accessors.

> +	for (;;) {
> +		unsigned long status = readl(INT_STATUS_REG(dw));
> +
> +		if (!status)
> +			break;
> +		writel(status, EOI_REG(dw));
> +
> +		for_each_set_bit(i, &status, 8)
> +			generic_handle_irq(dw->irq_base + i);
> +	}
> +	desc->irq_data.chip->irq_unmask(&desc->irq_data);
> +}
> +
> +/*
> + * We want to enable/disable interrupts for the GPIO pins through the GPIO
> + * block itself.  The current configuration assumes a 1-to-1 mapping of GPIO
> + * interrupt sources to IRQ numbers.  We use a chained handler as the GPIO
> + * IRQ's may pass through a separate VIC on some systems so need to be
> + * enabled/disabled there too.
> + *
> + * The chained handler simply converts from the virtual IRQ handler to the
> + * real interrupt source and calls the GPIO IRQ handler.
> + */
> +static void dwgpio_irq_init(struct dw_gpio_chip *dw,
> +			    struct resource *demux_irqs,
> +			    struct resource *irqs)
> +{
> +	int i;
> +
> +	if (!demux_irqs && !irqs)
> +		return;
> +
> +	if (!demux_irqs || !irqs ||
> +	    resource_size(demux_irqs) != resource_size(irqs)) {
> +		dev_warn(dw->chip.dev, "unsupported IRQ demuxing configuration, continuing without GPIO IRQ support\n");
> +		return;
> +	}
> +
> +	dw->irq_base = irqs->start;
> +
> +	writel(0, INT_EN_REG(dw));
> +	writel(~0, EOI_REG(dw));
> +	for (i = irqs->start; i <= irqs->end; ++i) {
> +		irq_set_chip_and_handler_name(i, &dwgpio_irqchip,
> +					      handle_simple_irq, "demux");

Why do you have irq_ack/mask/unmask functions when you use
handle_simple_irq? Just because the random driver you copied from has
them as well? They are never called.

> +		irq_set_chip_data(i, dw);
> +		irq_set_status_flags(i, IRQ_TYPE_EDGE_BOTH |
> +					IRQ_TYPE_LEVEL_HIGH |
> +					IRQ_TYPE_LEVEL_LOW);

What's that for ?

> +		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);

Why are those irqs probable ?

> +	}
> +
> +	for (i = demux_irqs->start; i <= demux_irqs->end; ++i) {
> +		irq_set_handler_data(i, dw);
> +		irq_set_chained_handler(i, dw_gpio_irq_handler);
> +		set_irq_flags(i, IRQF_VALID);
> +	}
> +}

> +static void dwgpio_irq_stop(struct dw_gpio_chip *dw,
> +			    struct resource *demux_irqs,
> +			    struct resource *irqs)
> +{
> +	int i;
> +
> +	if (!demux_irqs || !irqs ||
> +	    resource_size(demux_irqs) != resource_size(irqs))
> +		return;
> +
> +	for (i = irqs->start; i < irqs->end; ++i) {

Are those interrupts disabled? And what removes the handler ?

> +		irq_set_chip(i, NULL);
> +		irq_set_chip_data(i, NULL);
> +	}
> +
> +	for (i = demux_irqs->start; i < demux_irqs->end; ++i) {

Ditto

> +		irq_set_chip(i, NULL);
> +		irq_set_chip_data(i, NULL);
> +	}
> +}

Sigh. This is another incarnation of a bog standard GPIO driver, which
has the ever repeating pattern of configuration, input read, output
write and interrupt functions. It's about time to stop that nonsense.

GPIO implementations are not that different and we really want to
abstract out the few implementations and be done with it.

Thanks,

	tglx

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Jamie Iles]

Hi Thomas,

It doesn't sound like it's worth resubmitting this driver and it would 
be better to focus on a generic GPIO driver - I can certainly see the 
attraction in that.  I have a few comments inline particularly wrt 
IRQ handling stuff so I can hopefully get a better grasp on it.

Many thanks for taking the time to review though, it's much appreciated.

Jamie

On Sun, Apr 03, 2011 at 12:10:04AM +0200, Thomas Gleixner wrote:
> B1;2401;0cOn Fri, 1 Apr 2011, Jamie Iles wrote:
> 
> > This patch adds support for the Synopsys DesignWare APB GPIO controller
> > that can be found in some ARM systems.  The controller supports up to
> 
> Not only ARM.

OK, fair enough.

> 
> > 4x32 bit ports and port A is capable of raising interrupts.
> 
> So I assume that there are already a bunch of ARM SoC gpio
> implementations in the arch/arm/* space which implement the very same
> thing. Did you check that? And if yes, then it would be very helpful
> to actually convert those platforms so there is a reason why we merge
> another instance.

I did check the ones currently in arch/arm/ and I couldn't see any that 
were clearly Synposys DesignWare GPIO blocks.  That's not to say there 
aren't any, but given the spec for the IP version I have I couldn't 
identify any.  I work on a couple of ARM chips that use this block that 
we were hoping to mainline support for in the future.

> 
> > +#define GPIO_SW_PORT_A_DR_REG_OFFSET		0x00
> > +#define GPIO_SW_PORT_A_DDR_REG_OFFSET		0x04
> > +#define GPIO_SW_PORT_A_CTL_REG_OFFSET		0x08
> > +#define GPIO_SW_PORT_B_DR_REG_OFFSET		0x0C
> 
> I think this whole register offset thing needs to be runtime
> configurable. We all know that SoC designers decide to move registers
> around for pointless reasons or just change the stride.

OK, for a generic GPIO driver I don't disagree but this comment isn't 
specific to this driver right?

> > +#define GPIO_INT_EN_REG_OFFSET			0x30
> > +#define GPIO_INT_MASK_REG_OFFSET		0x34
> 
> I bet there are implementations of the very same thing which have irq
> support for more than one port. So that wants to be generalized as
> well.

Perhaps if there are newer versions of the IP then Synopsys could have 
added support for interrupts on other ports but from the spec I have 
only port A can be configured for IRQs.

> 
> > +/*
> > + * Get the port mapping for the controller.  There are 4 possible ports that
> > + * we can use but some platforms may reserve ports for hardware purposes that
> > + * cannot be used as GPIO so we allow these to be masked off.
> > + */
> > +static void dw_gpio_configure_ports(struct dw_gpio_chip *dw,
> > +				    unsigned long port_mask)
> > +{
> > +	unsigned long cfg1, cfg2;
> > +	int nr_ports;
> > +
> > +	cfg1 = readl(dw->iobase + GPIO_CFG1_REG_OFFSET);
> > +	cfg2 = readl(dw->iobase + GPIO_CFG2_REG_OFFSET);
> > +
> > +	nr_ports = (cfg1 & GPIO_NR_PORTS_MASK) >> GPIO_NR_PORTS_SHIFT;
> > +
> > +	if (port_mask & DW_GPIO_PORTA_MASK) {
> > +		dw->porta_end = ((cfg2 & GPIO_PORTA_WIDTH_MASK) >>
> > +				 GPIO_PORTA_WIDTH_SHIFT) + 1;
> > +		if (nr_ports == 1)
> > +			return;
> 
> These nr_ports checks are pointless. This is a setup function and not
> a hotpath.

I put those there because it wasn't clear from the Synopsys 
documentation if the port widths for ports that weren't implemented were 
set to zero and don't have enough variety of implementations to test..

> 
> > +	} else
> > +		dw->porta_end = 0;
> > +
> > +	if (port_mask & DW_GPIO_PORTB_MASK) {
> > +		dw->portb_end = ((cfg2 & GPIO_PORTB_WIDTH_MASK) >>
> > +				 GPIO_PORTB_WIDTH_SHIFT) + 1 + dw->porta_end;
> > +		if (nr_ports == 2)
> > +			return;
> > +	} else
> > +		dw->portb_end = dw->porta_end;
> > +
> > +	if (port_mask & DW_GPIO_PORTC_MASK) {
> > +		dw->portc_end = ((cfg2 & GPIO_PORTC_WIDTH_MASK) >>
> > +				 GPIO_PORTC_WIDTH_SHIFT) + 1 + dw->portb_end;
> > +		if (nr_ports == 3)
> > +			return;
> > +	} else
> > +		dw->portc_end = dw->portb_end;
> > +
> > +	if (port_mask & DW_GPIO_PORTD_MASK)
> > +		dw->portd_end = ((cfg2 & GPIO_PORTD_WIDTH_MASK) >>
> > +				 GPIO_PORTD_WIDTH_SHIFT) + 1 + dw->portc_end;
> > +	else
> > +		dw->portd_end = dw->portb_end;
> > +}
> 
> So you try to create a linear GPIO space which skips reserved
> pins. What's the point of that? It adds useless overhead in the
> hotpath for no value. It is confusing as there is no relation between
> the HW pin and the gpio number anymore.
> 
> Why can't we just have a linear space where the reserved gpios are
> simply not accessible?

So the reason here is that the IP may be configured with multiple ports 
of different sizes.  A real world example we have is port widths of 
8-16-1-32 and the only actual reserved GPIO is the single pin on port C.  
So we could say that the chip supports 128 GPIOs with loads of reserved 
GPIOs but that didn't seem too intuitive to me.  I'm a bit conflicted on 
this one.

> 
> > +#define __DWGPIO_REG(__chip, __gpio_nr, __reg)				\
> > +	({								\
> > +		void __iomem *ret = NULL;				\
> > +		if ((__gpio_nr) <= (__chip)->porta_end)			\
> > +			ret = ((__chip)->iobase +			\
> > +				GPIO_SW_PORT_A_##__reg##_REG_OFFSET);	\
> > +		else if ((__gpio_nr) <= (__chip)->portb_end)		\
> > +			ret = ((__chip)->iobase +			\
> > +				GPIO_SW_PORT_B_##__reg##_REG_OFFSET);	\
> > +		else if ((__gpio_nr) <= (__chip)->portc_end)		\
> > +			ret = ((__chip)->iobase +			\
> > +				GPIO_SW_PORT_C_##__reg##_REG_OFFSET);	\
> > +		else							\
> > +			ret = ((__chip)->iobase +			\
> > +				GPIO_SW_PORT_D_##__reg##_REG_OFFSET);	\
> > +		ret;							\
> > +	})
> > +
> 
> > +static inline unsigned dw_gpio_bit_offs(struct dw_gpio_chip *dw,
> > +					unsigned offset)
> > +{
> > +	/*
> > +	 * The gpios are controlled via three sets of registers. The register
> > +	 * addressing is already taken care of by the __DWGPIO_REG macro,
> 
> Shudder. A linear mapping would avoid all that ugliness.

Yes, that would certainly simplify all of this.

> 
> > +	 * this takes care of the bit offsets within each register.
> > +	 */
> > +	if (offset <= dw->porta_end)
> > +		return offset;
> > +	else if (offset <= dw->portb_end)
> > +		return offset - dw->porta_end;
> > +	else if (offset <= dw->portc_end)
> > +		return offset - dw->portb_end;
> > +	else
> > +		return offset - dw->portc_end;
> > +}
> > +
> > +static int dwgpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > +{
> > +	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
> > +	void __iomem *ddr = DWGPIO_DDR(dw, offset);
> > +	void __iomem *cr = DWGPIO_CTL(dw, offset);
> > +	unsigned long flags, val, bit_offset = dw_gpio_bit_offs(dw, offset);
> > +
> > +	spin_lock_irqsave(&dw->lock, flags);
> > +	/* Mark the pin as an output. */
> > +	val = readl(ddr);
> 
> Why is this value not cached ?

Fair point!

> 
> > +	val &= ~(1 << bit_offset);
> > +	writel(val, ddr);
> > +
> > +	/* Set the pin as software controlled. */
> > +	val = readl(cr);
> 
> Ditto
> 
> > +	val &= ~(1 << bit_offset);
> > +	writel(val, cr);
> > +	spin_unlock_irqrestore(&dw->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwgpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > +	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
> > +	void __iomem *ext = DWGPIO_EXT(dw, offset);
> > +	unsigned long bit_offset = dw_gpio_bit_offs(dw, offset);
> > +
> > +	return !!(readl(ext) & (1 << bit_offset));
> > +}
> > +
> > +static void dwgpio_set(struct gpio_chip *chip, unsigned offset, int value)
> > +{
> > +	struct dw_gpio_chip *dw = to_dw_gpio_chip(chip);
> > +	void __iomem *dr = DWGPIO_DR(dw, offset);
> > +	unsigned long val, flags, bit_offset = dw_gpio_bit_offs(dw, offset);
> > +
> > +	spin_lock_irqsave(&dw->lock, flags);
> > +	val = readl(dr);
> 
> Even more so. More instances ignored.
> 
> > +static void dwgpio_irq_enable(struct irq_data *d)
> > +{
> > +	int gpio = irq_to_gpio(d->irq);
> > +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> > +	void *port_inten = INT_EN_REG(dw);
> > +	unsigned long val, flags;
> > +
> > +	spin_lock_irqsave(&dw->lock, flags);
> 
> This is nested into irq_desc->lock and wants to be a raw_spinlock
> therefor.

OK, thanks.

> 
> > +	val = readl(port_inten);
> > +	val |= (1 << gpio);
> > +	writel(val, port_inten);
> > +	spin_unlock_irqrestore(&dw->lock, flags);
> > +}
> > +
> > +static void dwgpio_irq_disable(struct irq_data *d)
> > +{
> > +	int gpio = irq_to_gpio(d->irq);
> > +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> > +	void __iomem *port_inten = INT_EN_REG(dw);
> 
> All this information can be precomputed at setup time and supplied as
> irq_chip_data to the irq in a form wich does not require to compute it
> over and over.

OK, that makes sense.

> 
> > +static void dwgpio_irq_ack(struct irq_data *d)
> > +{
> > +	int gpio = irq_to_gpio(d->irq);
> > +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> > +	void __iomem *port_intmask = INT_MASK_REG(dw);
> > +	void __iomem *port_eoi = EOI_REG(dw);
> > +	void __iomem *port_inttype_level = INT_TYPE_REG(dw);
> > +	unsigned long val, flags;
> > +
> > +	spin_lock_irqsave(&dw->lock, flags);
> > +	val = readl(port_inttype_level);
> > +	if (val & (1 << gpio)) {
> > +		/* Edge-sensitive */
> > +		val = readl(port_eoi);
> > +		val |= (1 << gpio);
> > +		writel(val, port_eoi);
> > +	} else {
> > +		/* Level-sensitive */
> > +		val = readl(port_intmask);
> > +		val |= (1 << gpio);
> > +		writel(val, port_intmask);
> > +	}
> 
> These conditionals are completely wrong. We use different irq_chip
> implementations to distinguish different flows.

I'm not sure I understand this, are you saying that there would be two 
'struct irq_chip's - one for level and the other for edge or are you 
referring to the flows internal to the generic IRQ layer?  If the 
former, then is it safe to assign another chip to an IRQ in the 
.irq_set_type method of another chip or am I barking up the wrong tree 
here?

I can see that I shouldn't be doing the mask for the level-sensitive IRQ 
here though.

>
> > +	spin_unlock_irqrestore(&dw->lock, flags);
> > +}
> 
> > +static int dwgpio_irq_set_type(struct irq_data *d,
> > +			       unsigned int trigger)
> > +{
> > +	int gpio = irq_to_gpio(d->irq);
> > +	struct dw_gpio_chip *dw = irq_data_get_irq_chip_data(d);
> > +	void __iomem *port_inttype_level = INT_TYPE_REG(dw);
> > +	void __iomem *port_int_polarity = INT_POLARITY_REG(dw);
> > +	unsigned long level, polarity, flags;
> > +
> > +	if (trigger & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> > +			IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> > +		return -EINVAL;
> > +
> > +	spin_lock_irqsave(&dw->lock, flags);
> > +	level = readl(port_inttype_level);
> > +	polarity = readl(port_int_polarity);
> > +
> > +	if (trigger & IRQ_TYPE_EDGE_RISING) {
> > +		level	    |= (1 << gpio);
> > +		polarity    |= (1 << gpio);
> > +	} else if (trigger & IRQ_TYPE_EDGE_FALLING) {
> > +		level	    |= (1 << gpio);
> > +		polarity    &= ~(1 << gpio);
> > +	} else if (trigger & IRQ_TYPE_LEVEL_HIGH) {
> > +		level	    &= ~(1 << gpio);
> > +		polarity    |= (1 << gpio);
> > +	} else if (trigger & IRQ_TYPE_LEVEL_LOW) {
> > +		level	    &= ~(1 << gpio);
> > +		polarity    &= ~(1 << gpio);
> > +	}
> > +
> > +	writel(level, port_inttype_level);
> > +	writel(polarity, port_int_polarity);
> 
> So this should set the proper irq chip for the edge/level flow, but
> yes that's pointless. See below.

OK, got that.

> 
> > +	spin_unlock_irqrestore(&dw->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct irq_chip dwgpio_irqchip = {
> > +	.name		= "dwgpio",
> > +	.irq_ack	= dwgpio_irq_ack,
> > +	.irq_mask	= dwgpio_irq_mask,
> > +	.irq_unmask	= dwgpio_irq_unmask,
> > +	.irq_enable	= dwgpio_irq_enable,
> > +	.irq_disable	= dwgpio_irq_disable,
> > +	.irq_set_type	= dwgpio_irq_set_type,
> > +};
> > +
> > +static void dw_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
> > +{
> > +	struct dw_gpio_chip *dw = irq_get_handler_data(irq);
> > +	int i;
> > +
> > +	/*
> > +	 * Mask and ack the interrupt in the parent interrupt controller
> > +	 * before handling it.
> > +	 */
> > +	desc->irq_data.chip->irq_mask(&desc->irq_data);
> > +	desc->irq_data.chip->irq_ack(&desc->irq_data);
> 
> Please get chip and irq_data with the proper accessors.
> 
> > +	for (;;) {
> > +		unsigned long status = readl(INT_STATUS_REG(dw));
> > +
> > +		if (!status)
> > +			break;
> > +		writel(status, EOI_REG(dw));
> > +
> > +		for_each_set_bit(i, &status, 8)
> > +			generic_handle_irq(dw->irq_base + i);
> > +	}
> > +	desc->irq_data.chip->irq_unmask(&desc->irq_data);
> > +}
> > +
> > +/*
> > + * We want to enable/disable interrupts for the GPIO pins through the GPIO
> > + * block itself.  The current configuration assumes a 1-to-1 mapping of GPIO
> > + * interrupt sources to IRQ numbers.  We use a chained handler as the GPIO
> > + * IRQ's may pass through a separate VIC on some systems so need to be
> > + * enabled/disabled there too.
> > + *
> > + * The chained handler simply converts from the virtual IRQ handler to the
> > + * real interrupt source and calls the GPIO IRQ handler.
> > + */
> > +static void dwgpio_irq_init(struct dw_gpio_chip *dw,
> > +			    struct resource *demux_irqs,
> > +			    struct resource *irqs)
> > +{
> > +	int i;
> > +
> > +	if (!demux_irqs && !irqs)
> > +		return;
> > +
> > +	if (!demux_irqs || !irqs ||
> > +	    resource_size(demux_irqs) != resource_size(irqs)) {
> > +		dev_warn(dw->chip.dev, "unsupported IRQ demuxing configuration, continuing without GPIO IRQ support\n");
> > +		return;
> > +	}
> > +
> > +	dw->irq_base = irqs->start;
> > +
> > +	writel(0, INT_EN_REG(dw));
> > +	writel(~0, EOI_REG(dw));
> > +	for (i = irqs->start; i <= irqs->end; ++i) {
> > +		irq_set_chip_and_handler_name(i, &dwgpio_irqchip,
> > +					      handle_simple_irq, "demux");
> 
> Why do you have irq_ack/mask/unmask functions when you use
> handle_simple_irq? Just because the random driver you copied from has
> them as well? They are never called.

So would it be correct to start of with handle_level_irq() then to 
change the handler in the irq_set_type method with 
__irq_set_handler_locked().

> 
> > +		irq_set_chip_data(i, dw);
> > +		irq_set_status_flags(i, IRQ_TYPE_EDGE_BOTH |
> > +					IRQ_TYPE_LEVEL_HIGH |
> > +					IRQ_TYPE_LEVEL_LOW);
> 
> What's that for ?

I thought that meant that the IRQ was capable of being driven by these 
methods but I guess that's not the case.  Should this be saying what the 
interrupt type currently is?  I guess this can probably be removed 
completely here.

> 
> > +		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
> 
> Why are those irqs probable ?

I'm sorry to say that these came from existing drivers and these 
shouldn't be probeable.

> > +	}
> > +
> > +	for (i = demux_irqs->start; i <= demux_irqs->end; ++i) {
> > +		irq_set_handler_data(i, dw);
> > +		irq_set_chained_handler(i, dw_gpio_irq_handler);
> > +		set_irq_flags(i, IRQF_VALID);
> > +	}
> > +}
> 
> > +static void dwgpio_irq_stop(struct dw_gpio_chip *dw,
> > +			    struct resource *demux_irqs,
> > +			    struct resource *irqs)
> > +{
> > +	int i;
> > +
> > +	if (!demux_irqs || !irqs ||
> > +	    resource_size(demux_irqs) != resource_size(irqs))
> > +		return;
> > +
> > +	for (i = irqs->start; i < irqs->end; ++i) {
> 
> Are those interrupts disabled? And what removes the handler ?

So what would be the preferred way of handling this?  I can see that 
checking that all of the IRQs are disabled first then return -EBUSY from 
the .remove() method of the driver if not would be one way, but perhaps 
there's a better alternative.

> > +		irq_set_chip(i, NULL);
> > +		irq_set_chip_data(i, NULL);
> > +	}
> > +
> > +	for (i = demux_irqs->start; i < demux_irqs->end; ++i) {
> 
> Ditto
> 
> > +		irq_set_chip(i, NULL);
> > +		irq_set_chip_data(i, NULL);
> > +	}
> > +}
> 
> Sigh. This is another incarnation of a bog standard GPIO driver, which
> has the ever repeating pattern of configuration, input read, output
> write and interrupt functions. It's about time to stop that nonsense.
> 
> GPIO implementations are not that different and we really want to
> abstract out the few implementations and be done with it.

Fair comments.  I don't feel I have the expertise to undertake this 
generic abstraction myself but I'd be more than happy to help out if 
someone else did.

Thanks again for the review,

Jamie

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Grant Likely]

Hi Jamie,

On Sat, Apr 2, 2011 at 8:59 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> On Sun, Apr 03, 2011 at 12:10:04AM +0200, Thomas Gleixner wrote:
>> > +
>> > +   if (port_mask & DW_GPIO_PORTD_MASK)
>> > +           dw->portd_end = ((cfg2 & GPIO_PORTD_WIDTH_MASK) >>
>> > +                            GPIO_PORTD_WIDTH_SHIFT) + 1 + dw->portc_end;
>> > +   else
>> > +           dw->portd_end = dw->portb_end;
>> > +}
>>
>> So you try to create a linear GPIO space which skips reserved
>> pins. What's the point of that? It adds useless overhead in the
>> hotpath for no value. It is confusing as there is no relation between
>> the HW pin and the gpio number anymore.
>>
>> Why can't we just have a linear space where the reserved gpios are
>> simply not accessible?
>
> So the reason here is that the IP may be configured with multiple ports
> of different sizes.  A real world example we have is port widths of
> 8-16-1-32 and the only actual reserved GPIO is the single pin on port C.
> So we could say that the chip supports 128 GPIOs with loads of reserved
> GPIOs but that didn't seem too intuitive to me.  I'm a bit conflicted on
> this one.

The solution here I believe is to simply treat each bank as a separate
device.  For a generic driver, I see little advantage in linking all
the banks together unless there are tight interdependencies between
the banks, which I suspect is unlikely.

>> > +   writel(0, INT_EN_REG(dw));
>> > +   writel(~0, EOI_REG(dw));
>> > +   for (i = irqs->start; i <= irqs->end; ++i) {
>> > +           irq_set_chip_and_handler_name(i, &dwgpio_irqchip,
>> > +                                         handle_simple_irq, "demux");
>>
>> Why do you have irq_ack/mask/unmask functions when you use
>> handle_simple_irq? Just because the random driver you copied from has
>> them as well? They are never called.
>
> So would it be correct to start of with handle_level_irq() then to
> change the handler in the irq_set_type method with
> __irq_set_handler_locked().

Thomas, I think the IRQ apis are a source of confusion for a lot of
people.  I find I tend to get lost in the details.  Is there a good
document covering the current state of irq handling that folks like
James can be pointed towards?  I don't see much in the Documentation
directory covering things like how to use irqchips.  Best seems to be
Documentation/arm/Interrupts which may be out of date.

>> > +           irq_set_chip(i, NULL);
>> > +           irq_set_chip_data(i, NULL);
>> > +   }
>> > +}
>>
>> Sigh. This is another incarnation of a bog standard GPIO driver, which
>> has the ever repeating pattern of configuration, input read, output
>> write and interrupt functions. It's about time to stop that nonsense.
>>
>> GPIO implementations are not that different and we really want to
>> abstract out the few implementations and be done with it.
>
> Fair comments.  I don't feel I have the expertise to undertake this
> generic abstraction myself but I'd be more than happy to help out if
> someone else did.

Actually, there is a starting point you can work with.  Look at
drivers/gpio/basic_mmio_gpio.c, and see if it can be molded to suit
your purposes.  (cc'ing Anton as he wrote it).  Start with the basic
GPIO access, and then look at grafting in the IRQ functionality as a
second step.

There don't appear to be any actual users of that driver in mainline
at the moment, so feel free to propose changes if need be.  I'm not
hugely thrilled with the current method that the driver uses to define
the register locations (using named resources).  My instinct would be
to use a single register resource region with offsets for each
register type defined from the base of it, but Anton can probably fill
us in on the reason that approach was used.

g.

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

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3954 bytes --]

On Sat, 2 Apr 2011, Grant Likely wrote:
> Hi Jamie,
> 
> On Sat, Apr 2, 2011 at 8:59 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> > On Sun, Apr 03, 2011 at 12:10:04AM +0200, Thomas Gleixner wrote:
> >> > +
> >> > +   if (port_mask & DW_GPIO_PORTD_MASK)
> >> > +           dw->portd_end = ((cfg2 & GPIO_PORTD_WIDTH_MASK) >>
> >> > +                            GPIO_PORTD_WIDTH_SHIFT) + 1 + dw->portc_end;
> >> > +   else
> >> > +           dw->portd_end = dw->portb_end;
> >> > +}
> >>
> >> So you try to create a linear GPIO space which skips reserved
> >> pins. What's the point of that? It adds useless overhead in the
> >> hotpath for no value. It is confusing as there is no relation between
> >> the HW pin and the gpio number anymore.
> >>
> >> Why can't we just have a linear space where the reserved gpios are
> >> simply not accessible?
> >
> > So the reason here is that the IP may be configured with multiple ports
> > of different sizes.  A real world example we have is port widths of
> > 8-16-1-32 and the only actual reserved GPIO is the single pin on port C.
> > So we could say that the chip supports 128 GPIOs with loads of reserved
> > GPIOs but that didn't seem too intuitive to me.  I'm a bit conflicted on
> > this one.
> 
> The solution here I believe is to simply treat each bank as a separate
> device.  For a generic driver, I see little advantage in linking all
> the banks together unless there are tight interdependencies between
> the banks, which I suspect is unlikely.

Right. There are some chips which have shared base control registers
for the ports (some call them banks) but that can be dealt with as
well.
 
> >> > +   writel(0, INT_EN_REG(dw));
> >> > +   writel(~0, EOI_REG(dw));
> >> > +   for (i = irqs->start; i <= irqs->end; ++i) {
> >> > +           irq_set_chip_and_handler_name(i, &dwgpio_irqchip,
> >> > +                                         handle_simple_irq, "demux");
> >>
> >> Why do you have irq_ack/mask/unmask functions when you use
> >> handle_simple_irq? Just because the random driver you copied from has
> >> them as well? They are never called.
> >
> > So would it be correct to start of with handle_level_irq() then to
> > change the handler in the irq_set_type method with
> > __irq_set_handler_locked().

Yes.
 
> Thomas, I think the IRQ apis are a source of confusion for a lot of
> people.  I find I tend to get lost in the details.  Is there a good
> document covering the current state of irq handling that folks like
> James can be pointed towards?  I don't see much in the Documentation
> directory covering things like how to use irqchips.  Best seems to be
> Documentation/arm/Interrupts which may be out of date.

Yes I know, that documentation sucks. I need to find a few cycles to
update it.

For the question at hand, yes we can change the chip and the handler
in the irq_set_type() callback of the current irq_chip. That avoids
conditionals in the chip functions.

> Actually, there is a starting point you can work with.  Look at
> drivers/gpio/basic_mmio_gpio.c, and see if it can be molded to suit
> your purposes.  (cc'ing Anton as he wrote it).  Start with the basic
> GPIO access, and then look at grafting in the IRQ functionality as a
> second step.
> 
> There don't appear to be any actual users of that driver in mainline
> at the moment, so feel free to propose changes if need be.  I'm not
> hugely thrilled with the current method that the driver uses to define
> the register locations (using named resources).  My instinct would be
> to use a single register resource region with offsets for each
> register type defined from the base of it, but Anton can probably fill
> us in on the reason that approach was used.

Right, that avoids multiple ioremaps for the same physical address
space. There are more odds in that driver. It should setup proper
function pointers for accessing the registers instead of runtime
evaluation of everything.

Thanks,

	tglx

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Anton Vorontsov]

On Sun, Apr 03, 2011 at 11:30:41AM +0200, Thomas Gleixner wrote:
[...]
> > Actually, there is a starting point you can work with.  Look at
> > drivers/gpio/basic_mmio_gpio.c, and see if it can be molded to suit
> > your purposes.

It's surely doable for most memory-mapped GPIO controllers, the
logic for toggling the GPIO values/direction and reading input
state is pretty much the same across all the controllers.

> > There don't appear to be any actual users of that driver in mainline
> > at the moment, so feel free to propose changes if need be.

Yeah, I haven't had time to post the users (there are two: a custom
FPGA GPIO controller, and a GPIO controller in the Cavium ECONA
ARM machine).

I'll happily review/test any changes to the driver though.

> > I'm not
> > hugely thrilled with the current method that the driver uses to define
> > the register locations (using named resources).  My instinct would be
> > to use a single register resource region with offsets for each
> > register type defined from the base of it, but Anton can probably fill
> > us in on the reason that approach was used.

Well, I did it that way because you don't have to pass the offsets via
platform data (you don't need platform data most of the time, i.e. if
you use dynamic bases).

And with device tree, you wouldn't need any constructors:

gpio-controller@f00 {
	reg = <0xf00 0x4      // would map to "dat" resource
	       0xf04 0x4      // would maps to "set" resource
	       0xf08 0x4>;    // would maps to "clr" resource
};

That is, I dreamed about something like this, the way to (re)map
DT resources to Linux resources:

static const struct of_resource_map resmap[] = {
	/* type   DTnum  Lnum  Name  */
	{  MEM,   0,     1,    "mmio space1" },
	{  MEM,   1,     0,    "mmio space2" },
	{  IRQ,   0,     1,    "tx irq" },
	{  IRQ,   1,     0,    "rx irq" },
	{},
};

static struct platform_driver drv = {
	.driver.of_match_table = ...;
	.driver.of_resource_map_table = &resmap;
};

But still, I have no problem with either approaches, be it mutiple
resources or a resource + offsets.

> Right, that avoids multiple ioremaps for the same physical address
> space.

I guess this could be solved by a tiny logic inside the driver (i.e.
if the registers lie within a single page, map them with a single
ioremap call, otherwise fallback to multiple ioremaps). Not sure if
that worth a trouble though.

> There are more odds in that driver. It should setup proper
> function pointers for accessing the registers instead of runtime
> evaluation of everything.

Good idea, thanks.

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Jamie Iles]

Hi Anton,

On Sun, Apr 03, 2011 at 04:03:44PM +0400, Anton Vorontsov wrote:
> > > I'm not
> > > hugely thrilled with the current method that the driver uses to define
> > > the register locations (using named resources).  My instinct would be
> > > to use a single register resource region with offsets for each
> > > register type defined from the base of it, but Anton can probably fill
> > > us in on the reason that approach was used.
> 
> Well, I did it that way because you don't have to pass the offsets via
> platform data (you don't need platform data most of the time, i.e. if
> you use dynamic bases).

Well I'm happy to give it a go for some more complex chips with multiple 
banks but I'm not sure how to accomplish this without platform data.  My 
first idea would be to have something like:

struct mmio_gpio_bank {
	unsigned int		ngpio;
	unsigned long		set_offs;
	unsigned long		clr_offs;
	unsigned long		dout_offs;
	unsigned long		din_offs;
	unsigned long		dir_offs;
};

struct mmio_gpio_pdata {
	size_t			bus_width_bits;
	int			gpio_base;
	unsigned int		nr_banks;
	struct mmio_gpio_bank	banks[];
};

and have one iomem resource for the whole controller.  This allows us to 
cope with the controllers where each bank has a different number of GPIO 
pins but I'm not sure how device tree friendly it is.  If there's a 
better way then please let me know and I'll give it a go, though
at first it does need to be able to work without device tree support.

Looking at some of the different IRQ demuxing schemes they seem to vary 
quite a bit so I'm not sure how to handle that in a relatively generic 
way but perhaps that can come later.

Jamie

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Grant Likely]

On Sun, Apr 3, 2011 at 8:07 AM, Jamie Iles <jamie@jamieiles.com> wrote:
> Hi Anton,
>
> On Sun, Apr 03, 2011 at 04:03:44PM +0400, Anton Vorontsov wrote:
>> > > I'm not
>> > > hugely thrilled with the current method that the driver uses to define
>> > > the register locations (using named resources).  My instinct would be
>> > > to use a single register resource region with offsets for each
>> > > register type defined from the base of it, but Anton can probably fill
>> > > us in on the reason that approach was used.
>>
>> Well, I did it that way because you don't have to pass the offsets via
>> platform data (you don't need platform data most of the time, i.e. if
>> you use dynamic bases).
>
> Well I'm happy to give it a go for some more complex chips with multiple
> banks but I'm not sure how to accomplish this without platform data.

I'm rarely accused of being a fan of platform data; however, for
platform_devices the pattern is well established.  Until an viable
alternative is implemented, I don't think you need to avoid it.

> My first idea would be to have something like:
>
> struct mmio_gpio_bank {
>        unsigned int            ngpio;
>        unsigned long           set_offs;
>        unsigned long           clr_offs;
>        unsigned long           dout_offs;
>        unsigned long           din_offs;
>        unsigned long           dir_offs;
> };
>
> struct mmio_gpio_pdata {
>        size_t                  bus_width_bits;
>        int                     gpio_base;
>        unsigned int            nr_banks;
>        struct mmio_gpio_bank   banks[];
> };

As discussed earlier in the thread, you probably don't need to support
multiple banks with this driver.  Instead, create a separate device
instance for each bank.

> and have one iomem resource for the whole controller.  This allows us to
> cope with the controllers where each bank has a different number of GPIO
> pins but I'm not sure how device tree friendly it is.

Device tree is just a data structure.  About the only thing that
cannot be passed by a device tree node is callback function pointers.
Everything else can be described.  I see no worries here.

>  If there's a
> better way then please let me know and I'll give it a go, though
> at first it does need to be able to work without device tree support.
>
> Looking at some of the different IRQ demuxing schemes they seem to vary
> quite a bit so I'm not sure how to handle that in a relatively generic
> way but perhaps that can come later.

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

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Jamie Iles]

Hi Grant,

On Sun, Apr 03, 2011 at 08:47:25AM -0600, Grant Likely wrote:
> On Sun, Apr 3, 2011 at 8:07 AM, Jamie Iles <jamie@jamieiles.com> wrote:
> > My first idea would be to have something like:
> >
> > struct mmio_gpio_bank {
> >        unsigned int            ngpio;
> >        unsigned long           set_offs;
> >        unsigned long           clr_offs;
> >        unsigned long           dout_offs;
> >        unsigned long           din_offs;
> >        unsigned long           dir_offs;
> > };
> >
> > struct mmio_gpio_pdata {
> >        size_t                  bus_width_bits;
> >        int                     gpio_base;
> >        unsigned int            nr_banks;
> >        struct mmio_gpio_bank   banks[];
> > };
> 
> As discussed earlier in the thread, you probably don't need to support
> multiple banks with this driver.  Instead, create a separate device
> instance for each bank.

The reason I proposed this was for controllers where the registers 
aren't grouped together for each bank.  For example, the Synopsys block 
has:

	0x00-0x08 bank A control registers
	0x0c-0x14 bank B control registers
	...
	0x50	  bank A input register
	0x54	  bank B input register.

So when you mentioned before using a single register resource with 
offsets I understood it to be something like what I've proposed 
otherwise multiple banks would have overlapping resources (or the 
resource would just be used to indicate the start address rather than 
start + end).

Also, it's not clear here but this would create one gpio_chip per bank.

Jamie

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Anton Vorontsov]

On Sun, Apr 03, 2011 at 04:22:44PM +0100, Jamie Iles wrote:
[...]
> > As discussed earlier in the thread, you probably don't need to support
> > multiple banks with this driver.  Instead, create a separate device
> > instance for each bank.
> 
> The reason I proposed this was for controllers where the registers 
> aren't grouped together for each bank.  For example, the Synopsys block 
> has:
> 
> 	0x00-0x08 bank A control registers
> 	0x0c-0x14 bank B control registers
> 	...
> 	0x50	  bank A input register
> 	0x54	  bank B input register.

Which, I guess, is another point for "multiple resources" approach.
Grant?

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Grant Likely]

On Sun, Apr 03, 2011 at 04:22:44PM +0100, Jamie Iles wrote:
> Hi Grant,
> 
> On Sun, Apr 03, 2011 at 08:47:25AM -0600, Grant Likely wrote:
> > On Sun, Apr 3, 2011 at 8:07 AM, Jamie Iles <jamie@jamieiles.com> wrote:
> > > My first idea would be to have something like:
> > >
> > > struct mmio_gpio_bank {
> > >        unsigned int            ngpio;
> > >        unsigned long           set_offs;
> > >        unsigned long           clr_offs;
> > >        unsigned long           dout_offs;
> > >        unsigned long           din_offs;
> > >        unsigned long           dir_offs;
> > > };
> > >
> > > struct mmio_gpio_pdata {
> > >        size_t                  bus_width_bits;
> > >        int                     gpio_base;
> > >        unsigned int            nr_banks;
> > >        struct mmio_gpio_bank   banks[];
> > > };
> > 
> > As discussed earlier in the thread, you probably don't need to support
> > multiple banks with this driver.  Instead, create a separate device
> > instance for each bank.
> 
> The reason I proposed this was for controllers where the registers 
> aren't grouped together for each bank.  For example, the Synopsys block 
> has:
> 
> 	0x00-0x08 bank A control registers
> 	0x0c-0x14 bank B control registers
> 	...
> 	0x50	  bank A input register
> 	0x54	  bank B input register.
> 
> So when you mentioned before using a single register resource with 
> offsets I understood it to be something like what I've proposed 
> otherwise multiple banks would have overlapping resources (or the 
> resource would just be used to indicate the start address rather than 
> start + end).

That may be a problem for request_mem_region() which would indeed
prevent the driver from specifying the full register range with
offsets inside it.  I suspect that the platform_bus_type will inhibit
two platform_devices with overlapping regions from getting registered.
Fair enough, that is a pretty strong argument for Anton's model.  I
don't think it would be a good idea to try and work around it by
making the resource only include the start address.

It does mean some gymnastics for a device tree binding to figure out
which registers are present, but the appropriate behaviour could be
selected by a set of compatible values for each kind of register
interface.

> 
> Also, it's not clear here but this would create one gpio_chip per bank.

And that's bad why?  :-)

g.

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Jamie Iles]

On Mon, Apr 04, 2011 at 08:48:38PM -0600, Grant Likely wrote:
> On Sun, Apr 03, 2011 at 04:22:44PM +0100, Jamie Iles wrote:
> > The reason I proposed this was for controllers where the registers 
> > aren't grouped together for each bank.  For example, the Synopsys 
> > block has:
> > 
> > 	0x00-0x08 bank A control registers
> > 	0x0c-0x14 bank B control registers
> > 	...
> > 	0x50	  bank A input register
> > 	0x54	  bank B input register.
> > 
> > So when you mentioned before using a single register resource with 
> > offsets I understood it to be something like what I've proposed 
> > otherwise multiple banks would have overlapping resources (or the 
> > resource would just be used to indicate the start address rather than 
> > start + end).
> 
> That may be a problem for request_mem_region() which would indeed
> prevent the driver from specifying the full register range with
> offsets inside it.  I suspect that the platform_bus_type will inhibit
> two platform_devices with overlapping regions from getting registered.
> Fair enough, that is a pretty strong argument for Anton's model.  I
> don't think it would be a good idea to try and work around it by
> making the resource only include the start address.
> 
> It does mean some gymnastics for a device tree binding to figure out
> which registers are present, but the appropriate behaviour could be
> selected by a set of compatible values for each kind of register
> interface.

OK, I'll start having a look at this then.  I have a few other things on 
at the moment so it may take me a little while to get onto it but I'll 
try and get some patches posted fairly quickly.

Anton, I'm happy to do some of the changes that Thomas suggested such as 
removing some of the runtime calculations too unless you're planning on 
doing these.

> > 
> > Also, it's not clear here but this would create one gpio_chip per bank.
> 
> And that's bad why?  :-)

Not a bad thing, I just realized that my initial description was far 
from complete!

Jamie

Re: [PATCH] gpio: support for Synopsys DesignWare APB GPIO

[Anton Vorontsov]

On Tue, Apr 05, 2011 at 09:18:18AM +0100, Jamie Iles wrote:
[...]
> OK, I'll start having a look at this then.  I have a few other things on 
> at the moment so it may take me a little while to get onto it but I'll 
> try and get some patches posted fairly quickly.

Great!

> Anton, I'm happy to do some of the changes that Thomas suggested such as 
> removing some of the runtime calculations

Sure thing, feel free to make the changes.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com