[PATCH 0/3] GPIO support for BRCMSTB

[PATCH 0/3] GPIO support for BRCMSTB

[Gregory Fong]

This patchset adds support for the GPIO controller (UPG GIO) used on Broadcom's
various BRCMSTB SoCs (BCM7XXX and others).  It uses the "basic-mmio-gpio"
interface to try to reduce duplication of the base logic.

There is only one IRQ for each GIO IP block (i.e. several register banks share
an IRQ).  After briefly looking into the generic IRQ chip implementation, it
seemed like in this case that using it would result in the driver being more
complex than necessary because AFAICT it expects a 1:1 mapping of
irq_chip_generic to gpio_chip.  It seemed like less of a pain to have a single
irq_chip since we have a single IRQ for all register banks (multiple
gpio_chips).  I might be missing something, maybe using a shared IRQ across
multiple irq_chips is easier than I think?  Suggestions welcome.

For all existing hardware, this block hooked up to the BCM7120 L2 IRQ
controller and so will require CONFIG_BCM7120_L2_IRQ=y.

Gregory Fong (3):
  dt-bindings: add brcmstb-gpio GPIO binding
  gpio: Add GPIO support for Broadcom STB SoCs
  gpio: brcmstb: Add interrupt support

 .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt |   65 +++
 MAINTAINERS                                        |    7 +
 arch/arm/mach-bcm/Kconfig                          |    1 +
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-brcmstb.c                        |  492 ++++++++++++++++++++
 6 files changed, 575 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
 create mode 100644 drivers/gpio/gpio-brcmstb.c

-- 
1.7.9.5

[PATCH 1/3] dt-bindings: add brcmstb-gpio GPIO binding

[Gregory Fong]

Add binding for Broadcom STB "UPG GIO" GPIO controller.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
 .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt |   65 ++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
new file mode 100644
index 0000000..435f1bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
@@ -0,0 +1,65 @@
+Broadcom STB "UPG GIO" GPIO controller
+
+The controller's registers are organized as sets of eight 32-bit
+registers with each set controlling a bank of up to 32 pins.  A single
+interrupt is shared for all of the banks handled by the controller.
+
+Required properties:
+
+- compatible:
+    Must be "brcm,brcmstb-gpio"
+
+- reg:
+    Define the base and range of the I/O address space containing
+    the brcmstb GPIO controller registers
+
+- #gpio-cells:
+    Should be <2>.  The first cell is the pin number (within the controller's
+    pin space), and the second is used for the following:
+    bit[0]: polarity (0 for active-high, 1 for active-low)
+
+- gpio-controller:
+    Specifies that the node is a GPIO controller.
+
+- brcm,gpio-bank-widths:
+    Number of GPIO lines for each bank.  Number of elements must
+    correspond to number of banks suggested by the 'reg' property.
+
+Optional properties:
+
+- interrupts:
+    The interrupt shared by all GPIO lines for this controller.
+
+- interrupt-parent:
+    phandle of the parent interrupt controller
+
+- #interrupt-cells:
+    Should be <2>.  The first cell is the GPIO number, the second should specify
+    flags.  The following subset of flags is supported:
+    - bits[3:0] trigger type and level flags
+        1 = low-to-high edge triggered
+        2 = high-to-low edge triggered
+        4 = active high level-sensitive
+        8 = active low level-sensitive
+      Valid combinations are 1, 2, 3, 4, 8.
+    See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+- interrupt-controller:
+    Marks the device node as an interrupt controller
+
+- interrupt-names:
+    The name of the IRQ resource used by this controller
+
+Example:
+	upg_gio: gpio@f040a700 {
+		#gpio-cells = <0x2>;
+		#interrupt-cells = <0x2>;
+		compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
+		gpio-controller;
+		interrupt-controller;
+		reg = <0xf040a700 0x80>;
+		interrupt-parent = <0xf>;
+		interrupts = <0x6>;
+		interrupt-names = "upg_gio";
+		brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>;
+	};
-- 
1.7.9.5

Re: [PATCH 1/3] dt-bindings: add brcmstb-gpio GPIO binding

[Linus Walleij]

On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Add binding for Broadcom STB "UPG GIO" GPIO controller.
>
> Signed-off-by: Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Looks all right so patch applied.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] gpio: Add GPIO support for Broadcom STB SoCs

[Gregory Fong]

This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs
(BCM7XXX and some others).  Uses basic_mmio_gpio to instantiate a
gpio_chip for each bank.  The driver assumes that it handles the base
set of GPIOs on the system and that it can start its numbering sequence
from 0, so any GPIO expanders used with it must dynamically assign GPIO
numbers after this driver has finished registering its GPIOs.

Does not implement the interrupt-controller portion yet, will be done in a
future commit.

List-usage-fixed-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 MAINTAINERS                 |    7 ++
 arch/arm/mach-bcm/Kconfig   |    1 +
 drivers/gpio/Kconfig        |    8 ++
 drivers/gpio/Makefile       |    1 +
 drivers/gpio/gpio-brcmstb.c |  243 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 260 insertions(+)
 create mode 100644 drivers/gpio/gpio-brcmstb.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b399b34..781806a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2235,6 +2235,13 @@ N:	bcm9583*
 N:	bcm583*
 N:	bcm113*
 
+BROADCOM BRCMSTB GPIO DRIVER
+M:	Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+L:	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
+S:	Supported
+F:	drivers/gpio/gpio-brcmstb.c
+F:	Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
+
 BROADCOM KONA GPIO DRIVER
 M:	Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
 L:	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 8b11f44..0ac9e4b3 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -144,6 +144,7 @@ config ARCH_BRCMSTB
 	select BRCMSTB_GISB_ARB
 	select BRCMSTB_L2_IRQ
 	select BCM7120_L2_IRQ
+	select ARCH_WANT_OPTIONAL_GPIOLIB
 	help
 	  Say Y if you intend to run the kernel on a Broadcom ARM-based STB
 	  chipset.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index caefe80..5f79b7f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -126,6 +126,14 @@ config GPIO_BCM_KONA
 	help
 	  Turn on GPIO support for Broadcom "Kona" chips.
 
+config GPIO_BRCMSTB
+	tristate "BRCMSTB GPIO support"
+	default y if ARCH_BRCMSTB
+	depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
+	select GPIO_GENERIC
+	help
+	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
+
 config GPIO_CLPS711X
 	tristate "CLPS711X GPIO support"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index f71bb97..9bfaaa8 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_GPIO_ALTERA)  	+= gpio-altera.o
 obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
+obj-$(CONFIG_GPIO_BRCMSTB)	+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
new file mode 100644
index 0000000..c8f9014
--- /dev/null
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -0,0 +1,243 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/basic_mmio_gpio.h>
+
+#define GIO_BANK_SIZE           0x20
+#define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
+#define GIO_DATA(bank)          (((bank) * GIO_BANK_SIZE) + 0x04)
+#define GIO_IODIR(bank)         (((bank) * GIO_BANK_SIZE) + 0x08)
+#define GIO_EC(bank)            (((bank) * GIO_BANK_SIZE) + 0x0c)
+#define GIO_EI(bank)            (((bank) * GIO_BANK_SIZE) + 0x10)
+#define GIO_MASK(bank)          (((bank) * GIO_BANK_SIZE) + 0x14)
+#define GIO_LEVEL(bank)         (((bank) * GIO_BANK_SIZE) + 0x18)
+#define GIO_STAT(bank)          (((bank) * GIO_BANK_SIZE) + 0x1c)
+
+struct brcmstb_gpio_bank {
+	struct list_head node;
+	int id;
+	struct bgpio_chip bgc;
+	u32 imask;  /* irq mask shadow register */
+	struct brcmstb_gpio_priv *parent_priv;  /* used in interrupt handler */
+};
+
+struct brcmstb_gpio_priv {
+	struct list_head bank_list;
+	void __iomem *reg_base;
+	int num_banks;
+	struct platform_device *pdev;
+	int gpio_base;
+};
+
+#define GPIO_PER_BANK           32
+#define GPIO_BANK(gpio)         ((gpio) >> 5)
+/* assumes GPIO_PER_BANK is a multiple of 2 */
+#define GPIO_BIT(gpio)          ((gpio) & (GPIO_PER_BANK - 1))
+
+static inline struct brcmstb_gpio_priv *
+brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	struct brcmstb_gpio_bank *bank =
+		container_of(bgc, struct brcmstb_gpio_bank, bgc);
+
+	return bank->parent_priv;
+}
+
+/* Make sure that the number of banks matches up between properties */
+static int brcmstb_gpio_sanity_check_banks(struct device *dev,
+		struct device_node *np, struct resource *res)
+{
+	int res_num_banks = resource_size(res) / GIO_BANK_SIZE;
+	int num_banks = of_property_count_u32_elems(np, "brcm,gpio-bank-widths");
+
+	if (res_num_banks != num_banks) {
+		dev_err(dev, "Mismatch in banks: res had %d, bank-widths had %d\n",
+				res_num_banks, num_banks);
+		return -EINVAL;
+	} else {
+		return 0;
+	}
+}
+
+static int brcmstb_gpio_remove(struct platform_device *pdev)
+{
+	struct brcmstb_gpio_priv *priv = platform_get_drvdata(pdev);
+	struct list_head *pos;
+	struct brcmstb_gpio_bank *bank;
+	int ret = 0;
+
+	list_for_each(pos, &priv->bank_list) {
+		bank = list_entry(pos, struct brcmstb_gpio_bank, node);
+		ret = bgpio_remove(&bank->bgc);
+		if (ret)
+			dev_err(&pdev->dev, "gpiochip_remove fail in cleanup");
+	}
+	return ret;
+}
+
+static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
+		const struct of_phandle_args *gpiospec, u32 *flags)
+{
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+	int offset;
+
+	if (gc->of_gpio_n_cells != 2) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+		return -EINVAL;
+
+	offset = gpiospec->args[0] - (gc->base - priv->gpio_base);
+	if (offset >= gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[1];
+
+	return offset;
+}
+
+static int brcmstb_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void __iomem *reg_base;
+	struct brcmstb_gpio_priv *priv;
+	struct resource *res;
+	struct property *prop;
+	const __be32 *p;
+	u32 bank_width;
+	int err;
+	static int gpio_base;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg_base))
+		return PTR_ERR(reg_base);
+
+	priv->gpio_base = gpio_base;
+	priv->reg_base = reg_base;
+	priv->pdev = pdev;
+
+	INIT_LIST_HEAD(&priv->bank_list);
+	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
+		return -EINVAL;
+
+	of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p,
+			bank_width) {
+		struct brcmstb_gpio_bank *bank;
+		struct bgpio_chip *bgc;
+		struct gpio_chip *gc;
+
+		bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
+		if (!bank) {
+			err = -ENOMEM;
+			goto fail;
+		}
+
+		bank->parent_priv = priv;
+		bank->id = priv->num_banks;
+
+		/*
+		 * Regs are 4 bytes wide, have data reg, no set/clear regs,
+		 * and direction bits have 0 = output and 1 = input
+		 */
+		bgc = &bank->bgc;
+		err = bgpio_init(bgc, dev, 4,
+				reg_base + GIO_DATA(bank->id),
+				NULL, NULL, NULL,
+				reg_base + GIO_IODIR(bank->id), 0);
+		if (err) {
+			dev_err(dev, "bgpio_init() failed\n");
+			goto fail;
+		}
+
+		gc = &bgc->gc;
+		gc->of_node = np;
+		gc->owner = THIS_MODULE;
+		gc->label = np->full_name;
+		gc->base = gpio_base;
+		gc->of_gpio_n_cells = 2;
+		gc->of_xlate = brcmstb_gpio_of_xlate;
+
+		if (bank_width <= 0 || bank_width > GPIO_PER_BANK) {
+			gc->ngpio = GPIO_PER_BANK;
+			dev_warn(dev, "Invalid bank width %d, assume %d\n",
+					bank_width, gc->ngpio);
+		} else {
+			gc->ngpio = bank_width;
+		}
+
+		bank->imask =
+			bgc->read_reg(reg_base + GIO_MASK(bank->id));
+
+		err = gpiochip_add(gc);
+		if (err) {
+			dev_err(dev, "Could not add gpiochip for bank %d\n",
+					bank->id);
+			goto fail;
+		}
+		gpio_base += gc->ngpio;
+		dev_dbg(dev, "bank=%d, base=%d, ngpio=%d\n", bank->id,
+			gc->base, gc->ngpio);
+
+		/* Everything looks good, so add bank to list */
+		list_add(&bank->node, &priv->bank_list);
+
+		priv->num_banks++;
+	}
+
+	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
+			priv->num_banks, priv->gpio_base, gpio_base - 1);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+fail:
+	(void) brcmstb_gpio_remove(pdev);
+	return err;
+}
+
+static struct of_device_id brcmstb_gpio_of_match[] = {
+	{ .compatible = "brcm,brcmstb-gpio" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, brcmstb_gpio_of_match);
+
+static struct platform_driver brcmstb_gpio_driver = {
+	.driver = {
+		.name = "brcmstb-gpio",
+		.of_match_table = brcmstb_gpio_of_match,
+	},
+	.probe = brcmstb_gpio_probe,
+	.remove = brcmstb_gpio_remove,
+};
+module_platform_driver(brcmstb_gpio_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Driver for BRCMSTB UPG GPIO");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] gpio: Add GPIO support for Broadcom STB SoCs

[Paul Bolle]

Just a nit: a license mismatch.

On Wed, 2015-05-06 at 01:37 -0700, Gregory Fong wrote:
> --- /dev/null
> +++ b/drivers/gpio/gpio-brcmstb.c

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

This states the license is GPL v2.

> +MODULE_LICENSE("GPL");

And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the ident used in the MODULE_LICENSE() macro needs to change.

Thanks,


Paul Bolle

Re: [PATCH 2/3] gpio: Add GPIO support for Broadcom STB SoCs

[Gregory Fong]

On Thu, May 07, 2015 at 10:18:49AM +0200, Paul Bolle wrote:
> Just a nit: a license mismatch.
> 
> On Wed, 2015-05-06 at 01:37 -0700, Gregory Fong wrote:
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-brcmstb.c
> 
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> This states the license is GPL v2.
> 
> > +MODULE_LICENSE("GPL");
> 
> And, according to include/linux/module.h, this states the license is GPL
> v2 or later. So I think either the comment at the top of this file or
> the ident used in the MODULE_LICENSE() macro needs to change.

Will fix that, thanks.

Gregory

Re: [PATCH 2/3] gpio: Add GPIO support for Broadcom STB SoCs

[Linus Walleij]

On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs
> (BCM7XXX and some others).  Uses basic_mmio_gpio to instantiate a
> gpio_chip for each bank.  The driver assumes that it handles the base
> set of GPIOs on the system and that it can start its numbering sequence
> from 0, so any GPIO expanders used with it must dynamically assign GPIO
> numbers after this driver has finished registering its GPIOs.
>
> Does not implement the interrupt-controller portion yet, will be done in a
> future commit.
>
> List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>

(...)
>  arch/arm/mach-bcm/Kconfig   |    1 +
(...)
> --- a/arch/arm/mach-bcm/Kconfig
> +++ b/arch/arm/mach-bcm/Kconfig
> @@ -144,6 +144,7 @@ config ARCH_BRCMSTB
>         select BRCMSTB_GISB_ARB
>         select BRCMSTB_L2_IRQ
>         select BCM7120_L2_IRQ
> +       select ARCH_WANT_OPTIONAL_GPIOLIB

Please remove this from this patch. I don't want to patch around
in the platforms from the GPIO tree. Take this oneliner through
ARM SoC.

> +config GPIO_BRCMSTB
> +       tristate "BRCMSTB GPIO support"
> +       default y if ARCH_BRCMSTB
> +       depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
> +       select GPIO_GENERIC

Nice.

> +++ b/drivers/gpio/gpio-brcmstb.c
(...)
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>

#include <linux/gpio/driver.h>

should be sufficient.

> +struct brcmstb_gpio_bank {
> +       struct list_head node;
> +       int id;
> +       struct bgpio_chip bgc;
> +       u32 imask;  /* irq mask shadow register */

Why? Is it a write-only register that can't be read back?

> +       struct brcmstb_gpio_priv *parent_priv;  /* used in interrupt handler */

...and this patch does not enable IRQs...

> +struct brcmstb_gpio_priv {
> +       struct list_head bank_list;
> +       void __iomem *reg_base;
> +       int num_banks;
> +       struct platform_device *pdev;
> +       int gpio_base;

Usually we don't like it when you hardcode gpio_base, and this
field should anyway be present inside the bgpio_chip.gc.base
isn't it?

> +#define GPIO_PER_BANK           32
> +#define GPIO_BANK(gpio)         ((gpio) >> 5)
> +/* assumes GPIO_PER_BANK is a multiple of 2 */
> +#define GPIO_BIT(gpio)          ((gpio) & (GPIO_PER_BANK - 1))

But this macro and GPIO_PER_BANK does not respect the DT binding
stating the number of used lines.

You need to call these MAX_GPIO_PER_BANK or something.

> +static int brcmstb_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       void __iomem *reg_base;
> +       struct brcmstb_gpio_priv *priv;
> +       struct resource *res;
> +       struct property *prop;
> +       const __be32 *p;
> +       u32 bank_width;
> +       int err;
> +       static int gpio_base;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       reg_base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(reg_base))
> +               return PTR_ERR(reg_base);
> +
> +       priv->gpio_base = gpio_base;
> +       priv->reg_base = reg_base;
> +       priv->pdev = pdev;
> +
> +       INIT_LIST_HEAD(&priv->bank_list);
> +       if (brcmstb_gpio_sanity_check_banks(dev, np, res))
> +               return -EINVAL;
> +
> +       of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p,
> +                       bank_width) {
> +               struct brcmstb_gpio_bank *bank;
> +               struct bgpio_chip *bgc;
> +               struct gpio_chip *gc;
> +
> +               bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
> +               if (!bank) {
> +                       err = -ENOMEM;
> +                       goto fail;
> +               }
> +
> +               bank->parent_priv = priv;
> +               bank->id = priv->num_banks;
> +
> +               /*
> +                * Regs are 4 bytes wide, have data reg, no set/clear regs,
> +                * and direction bits have 0 = output and 1 = input
> +                */
> +               bgc = &bank->bgc;
> +               err = bgpio_init(bgc, dev, 4,
> +                               reg_base + GIO_DATA(bank->id),
> +                               NULL, NULL, NULL,
> +                               reg_base + GIO_IODIR(bank->id), 0);
> +               if (err) {
> +                       dev_err(dev, "bgpio_init() failed\n");
> +                       goto fail;
> +               }
> +
> +               gc = &bgc->gc;
> +               gc->of_node = np;
> +               gc->owner = THIS_MODULE;
> +               gc->label = np->full_name;
> +               gc->base = gpio_base;

I strongly suggest that you try using -1 as base here instead
for dynamic assignment of GPIO numbers.

> +               gc->of_gpio_n_cells = 2;
> +               gc->of_xlate = brcmstb_gpio_of_xlate;
> +
> +               if (bank_width <= 0 || bank_width > GPIO_PER_BANK) {
> +                       gc->ngpio = GPIO_PER_BANK;
> +                       dev_warn(dev, "Invalid bank width %d, assume %d\n",
> +                                       bank_width, gc->ngpio);

I wonder if this should not simply return an error.
If that number is wrong the DTS is completely screwed up.

> +               } else {
> +                       gc->ngpio = bank_width;
> +               }
> +
> +               bank->imask =
> +                       bgc->read_reg(reg_base + GIO_MASK(bank->id));

And this mask also mask the unused pins as GIO_MASK()
does not respect bank_width.

> +               err = gpiochip_add(gc);
> +               if (err) {
> +                       dev_err(dev, "Could not add gpiochip for bank %d\n",
> +                                       bank->id);
> +                       goto fail;
> +               }
> +               gpio_base += gc->ngpio;

This complicates things. Use -1 as base for dynamic assignment
of GPIO numbers.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: Add GPIO support for Broadcom STB SoCs

[Gregory Fong]

Hi Linus,

On Tue, May 12, 2015 at 3:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
>
>> This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs
>> (BCM7XXX and some others).  Uses basic_mmio_gpio to instantiate a
>> gpio_chip for each bank.  The driver assumes that it handles the base
>> set of GPIOs on the system and that it can start its numbering sequence
>> from 0, so any GPIO expanders used with it must dynamically assign GPIO
>> numbers after this driver has finished registering its GPIOs.
>>
>> Does not implement the interrupt-controller portion yet, will be done in a
>> future commit.
>>
>> List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com>
>> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
>
> (...)
>>  arch/arm/mach-bcm/Kconfig   |    1 +
> (...)
>> --- a/arch/arm/mach-bcm/Kconfig
>> +++ b/arch/arm/mach-bcm/Kconfig
>> @@ -144,6 +144,7 @@ config ARCH_BRCMSTB
>>         select BRCMSTB_GISB_ARB
>>         select BRCMSTB_L2_IRQ
>>         select BCM7120_L2_IRQ
>> +       select ARCH_WANT_OPTIONAL_GPIOLIB
>
> Please remove this from this patch. I don't want to patch around
> in the platforms from the GPIO tree. Take this oneliner through
> ARM SoC.

Will move to a separate patch.

>
>> +config GPIO_BRCMSTB
>> +       tristate "BRCMSTB GPIO support"
>> +       default y if ARCH_BRCMSTB
>> +       depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
>> +       select GPIO_GENERIC
>
> Nice.
>
>> +++ b/drivers/gpio/gpio-brcmstb.c
> (...)
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/gpio.h>
>
> #include <linux/gpio/driver.h>
>
> should be sufficient.

OK.

>
>> +struct brcmstb_gpio_bank {
>> +       struct list_head node;
>> +       int id;
>> +       struct bgpio_chip bgc;
>> +       u32 imask;  /* irq mask shadow register */
>
> Why? Is it a write-only register that can't be read back?

No, that wasn't necessary.  Will change to just read the register.

>
>> +       struct brcmstb_gpio_priv *parent_priv;  /* used in interrupt handler */
>
> ...and this patch does not enable IRQs...

OK, I will move it to the patch that does.

>
>> +struct brcmstb_gpio_priv {
>> +       struct list_head bank_list;
>> +       void __iomem *reg_base;
>> +       int num_banks;
>> +       struct platform_device *pdev;
>> +       int gpio_base;
>
> Usually we don't like it when you hardcode gpio_base, and this
> field should anyway be present inside the bgpio_chip.gc.base
> isn't it?

This was needed to deal with having a single irq_chip shared across
all of the gpio_chips in a GIO block.  You mentioned that this might
not be the Right Way to do this in your reply on the cover page so
I'll try to explain the reasoning better there.

FWIW: yes, this is inside the first bank's bgpio_chip, and it would be
possible to extract that info.  However, since it is used in
- brcmstb_gpio_to_irq
- brcmstb_gpio_hwirq_to_offset
- brcmstb_gpio_irq_bank_handler
- brcmstb_gpio_of_xlate

It seemed like it would be easier to follow if this were just stored
this in the priv struct, even if it is duplication of information.

>
>> +#define GPIO_PER_BANK           32
>> +#define GPIO_BANK(gpio)         ((gpio) >> 5)
>> +/* assumes GPIO_PER_BANK is a multiple of 2 */
>> +#define GPIO_BIT(gpio)          ((gpio) & (GPIO_PER_BANK - 1))
>
> But this macro and GPIO_PER_BANK does not respect the DT binding
> stating the number of used lines.
>
> You need to call these MAX_GPIO_PER_BANK or something.

Will change the name to MAX_GPIO_PER_BANK.

>
>> +static int brcmstb_gpio_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       void __iomem *reg_base;
>> +       struct brcmstb_gpio_priv *priv;
>> +       struct resource *res;
>> +       struct property *prop;
>> +       const __be32 *p;
>> +       u32 bank_width;
>> +       int err;
>> +       static int gpio_base;
>> +
>> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       reg_base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(reg_base))
>> +               return PTR_ERR(reg_base);
>> +
>> +       priv->gpio_base = gpio_base;
>> +       priv->reg_base = reg_base;
>> +       priv->pdev = pdev;
>> +
>> +       INIT_LIST_HEAD(&priv->bank_list);
>> +       if (brcmstb_gpio_sanity_check_banks(dev, np, res))
>> +               return -EINVAL;
>> +
>> +       of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p,
>> +                       bank_width) {
>> +               struct brcmstb_gpio_bank *bank;
>> +               struct bgpio_chip *bgc;
>> +               struct gpio_chip *gc;
>> +
>> +               bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
>> +               if (!bank) {
>> +                       err = -ENOMEM;
>> +                       goto fail;
>> +               }
>> +
>> +               bank->parent_priv = priv;
>> +               bank->id = priv->num_banks;
>> +
>> +               /*
>> +                * Regs are 4 bytes wide, have data reg, no set/clear regs,
>> +                * and direction bits have 0 = output and 1 = input
>> +                */
>> +               bgc = &bank->bgc;
>> +               err = bgpio_init(bgc, dev, 4,
>> +                               reg_base + GIO_DATA(bank->id),
>> +                               NULL, NULL, NULL,
>> +                               reg_base + GIO_IODIR(bank->id), 0);
>> +               if (err) {
>> +                       dev_err(dev, "bgpio_init() failed\n");
>> +                       goto fail;
>> +               }
>> +
>> +               gc = &bgc->gc;
>> +               gc->of_node = np;
>> +               gc->owner = THIS_MODULE;
>> +               gc->label = np->full_name;
>> +               gc->base = gpio_base;
>
> I strongly suggest that you try using -1 as base here instead
> for dynamic assignment of GPIO numbers.

That is what I did originally.  However, this results in a very
unpleasant numbering scheme, at least as currently implemented.

When -1 is base, as you know, numbering goes descending from 255
(IIRC).  Right now I'm using the of_property_for_each_u32 loop over
bank widths to go through the banks.  To keep the example
straightforward, let's pretend our GIO block only has two banks.
Here's how they're arranged:

bank 0: starts at 0xf040a700, contains GPIOs 0-31
bank 1: starts at 0xf040a720, contains GPIOs 32-63

Right now, with -1 as base, calling gpiochip_add() inside of that loop
will results in them getting this numbering:

bank 0: linux GPIOs 224-255
bank 1: linux GPIOs 192-223

which has the obvious downside that the ordering doesn't at all match
the hardware, making it completely unintuitive for anyone using the
(admittedly quite awful) sysfs interface.

Looking at this now, I think I could just add another loop afterward
to do the gpiochip_add()'s in reverse order, resulting in the
numbering ascending with banks as expected.  Does this seem sensible?

>
>> +               gc->of_gpio_n_cells = 2;
>> +               gc->of_xlate = brcmstb_gpio_of_xlate;
>> +
>> +               if (bank_width <= 0 || bank_width > GPIO_PER_BANK) {
>> +                       gc->ngpio = GPIO_PER_BANK;
>> +                       dev_warn(dev, "Invalid bank width %d, assume %d\n",
>> +                                       bank_width, gc->ngpio);
>
> I wonder if this should not simply return an error.
> If that number is wrong the DTS is completely screwed up.

You're right, probably better to have that be an error.  Will change that.

>
>> +               } else {
>> +                       gc->ngpio = bank_width;
>> +               }
>> +
>> +               bank->imask =
>> +                       bgc->read_reg(reg_base + GIO_MASK(bank->id));
>
> And this mask also mask the unused pins as GIO_MASK()
> does not respect bank_width.

I'll be getting rid of imask anyway as you suggested.  But since you
mentioned it, I just realized that the register specification says
that reads of the reserved bits can return an unknown value but writes
of those bits should be 0.  While in practice it doesn't seem to cause
any problems, I'll change that anyway just to be safe.

>
>> +               err = gpiochip_add(gc);
>> +               if (err) {
>> +                       dev_err(dev, "Could not add gpiochip for bank %d\n",
>> +                                       bank->id);
>> +                       goto fail;
>> +               }
>> +               gpio_base += gc->ngpio;
>
> This complicates things. Use -1 as base for dynamic assignment
> of GPIO numbers.

(this was covered above)

Thanks for the review,
Gregory

Re: [PATCH 2/3] gpio: Add GPIO support for Broadcom STB SoCs

[Linus Walleij]

On Tue, May 12, 2015 at 8:46 PM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
> On Tue, May 12, 2015 at 3:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> Usually we don't like it when you hardcode gpio_base, and this
>> field should anyway be present inside the bgpio_chip.gc.base
>> isn't it?
>
> This was needed to deal with having a single irq_chip shared across
> all of the gpio_chips in a GIO block.  You mentioned that this might
> not be the Right Way to do this in your reply on the cover page so
> I'll try to explain the reasoning better there.
>
> FWIW: yes, this is inside the first bank's bgpio_chip, and it would be
> possible to extract that info.  However, since it is used in
> - brcmstb_gpio_to_irq
> - brcmstb_gpio_hwirq_to_offset
> - brcmstb_gpio_irq_bank_handler
> - brcmstb_gpio_of_xlate
>
> It seemed like it would be easier to follow if this were just stored
> this in the priv struct, even if it is duplication of information.

OK I see. Even if you go for the approach I suggest in the cover
letter it is indeed necessary to keep track of the base I
can see, since we span multiple gpio_chips So it's fine with a
local variable for this.

>>> +               gc->base = gpio_base;
>>
>> I strongly suggest that you try using -1 as base here instead
>> for dynamic assignment of GPIO numbers.
>
> That is what I did originally.  However, this results in a very
> unpleasant numbering scheme, at least as currently implemented.
>
> When -1 is base, as you know, numbering goes descending from 255
> (IIRC).  Right now I'm using the of_property_for_each_u32 loop over
> bank widths to go through the banks.  To keep the example
> straightforward, let's pretend our GIO block only has two banks.
> Here's how they're arranged:
>
> bank 0: starts at 0xf040a700, contains GPIOs 0-31
> bank 1: starts at 0xf040a720, contains GPIOs 32-63
>
> Right now, with -1 as base, calling gpiochip_add() inside of that loop
> will results in them getting this numbering:
>
> bank 0: linux GPIOs 224-255
> bank 1: linux GPIOs 192-223

Yeah I kind of think it's a feature because we don't want people
to rely on the static GPIO numbering ;)

Buy OK yeah I see the point. Let's keep the base static for
now.

> Looking at this now, I think I could just add another loop afterward
> to do the gpiochip_add()'s in reverse order, resulting in the
> numbering ascending with banks as expected.  Does this seem sensible?

No, that relies on the internal semantic structure of the
gpiolib. It's better to use .base as a hint then.

>> And this mask also mask the unused pins as GIO_MASK()
>> does not respect bank_width.
>
> I'll be getting rid of imask anyway as you suggested.

Awesome.

Yours,
Linus Walleij

[PATCH 3/3] gpio: brcmstb: Add interrupt support

[Gregory Fong]

Create an irq_chip for each GIO block.  Uses chained IRQ handling since
known uses of this block have a BCM7120 L2 interrupt controller as a
parent.  Supports interrupts for all GPIOs.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
 drivers/gpio/Kconfig        |    1 +
 drivers/gpio/gpio-brcmstb.c |  249 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 250 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5f79b7f..2f9b913 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -131,6 +131,7 @@ config GPIO_BRCMSTB
 	default y if ARCH_BRCMSTB
 	depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
 	select GPIO_GENERIC
+	select IRQ_DOMAIN
 	help
 	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index c8f9014..6195838 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -17,6 +17,8 @@
 #include <linux/of_irq.h>
 #include <linux/module.h>
 #include <linux/basic_mmio_gpio.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
 
 #define GIO_BANK_SIZE           0x20
 #define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
@@ -40,7 +42,11 @@ struct brcmstb_gpio_priv {
 	struct list_head bank_list;
 	void __iomem *reg_base;
 	int num_banks;
+	int num_gpios;
 	struct platform_device *pdev;
+	struct irq_chip irq_chip;
+	struct irq_domain *irq_domain;
+	int parent_irq;
 	int gpio_base;
 };
 
@@ -59,6 +65,226 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
 	return bank->parent_priv;
 }
 
+static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
+		unsigned int offset, bool enable)
+{
+	struct bgpio_chip *bgc = &bank->bgc;
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	unsigned long mask = bgc->pin2mask(bgc, offset);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	if (enable)
+		bank->imask |= mask;
+	else
+		bank->imask &= ~mask;
+	bgc->write_reg(priv->reg_base + GIO_MASK(bank->id), bank->imask);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
+{
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+	/* gc_offset is relative to this gpio_chip; want real offset */
+	int offset = gc_offset + (gc->base - priv->gpio_base);
+
+	if (offset >= priv->num_gpios)
+		return -ENXIO;
+	return irq_create_mapping(priv->irq_domain, offset);
+}
+
+/* -------------------- IRQ chip functions -------------------- */
+
+static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
+		struct brcmstb_gpio_bank *bank)
+{
+	return hwirq - (bank->bgc.gc.base - bank->parent_priv->gpio_base);
+}
+
+static void brcmstb_gpio_irq_mask(struct irq_data *d)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	int offset = brcmstb_gpio_hwirq_to_offset(d->hwirq, bank);
+
+	brcmstb_gpio_set_imask(bank, offset, false);
+}
+
+static void brcmstb_gpio_irq_unmask(struct irq_data *d)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	int offset = brcmstb_gpio_hwirq_to_offset(d->hwirq, bank);
+
+	brcmstb_gpio_set_imask(bank, offset, true);
+}
+
+static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
+	u32 edge_insensitive, iedge_insensitive;
+	u32 edge_config, iedge_config;
+	u32 level, ilevel;
+	unsigned long flags;
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_LOW:
+		level = 0;
+		edge_config = 0;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		level = mask;
+		edge_config = 0;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		level = 0;
+		edge_config = 0;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		level = 0;
+		edge_config = mask;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		level = 0;
+		edge_config = 0;  /* don't care, but want known value */
+		edge_insensitive = mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&bank->bgc.lock, flags);
+
+	iedge_config = bank->bgc.read_reg(priv->reg_base +
+			GIO_EC(bank->id)) & ~mask;
+	iedge_insensitive = bank->bgc.read_reg(priv->reg_base +
+			GIO_EI(bank->id)) & ~mask;
+	ilevel = bank->bgc.read_reg(priv->reg_base +
+			GIO_LEVEL(bank->id)) & ~mask;
+
+	bank->bgc.write_reg(priv->reg_base + GIO_EC(bank->id),
+			iedge_config | edge_config);
+	bank->bgc.write_reg(priv->reg_base + GIO_EI(bank->id),
+			iedge_insensitive | edge_insensitive);
+	bank->bgc.write_reg(priv->reg_base + GIO_LEVEL(bank->id),
+			ilevel | level);
+
+	spin_unlock_irqrestore(&bank->bgc.lock, flags);
+	return 0;
+}
+
+static void brcmstb_gpio_irq_bank_handler(int irq,
+		struct brcmstb_gpio_bank *bank)
+{
+	void __iomem *reg_base = bank->parent_priv->reg_base;
+	unsigned long status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->bgc.lock, flags);
+	while ((status = bank->bgc.read_reg(reg_base + GIO_STAT(bank->id)) &
+			 bank->imask)) {
+		int bit;
+		for_each_set_bit(bit, &status, 32) {
+			int hwirq = bank->bgc.gc.base -
+				bank->parent_priv->gpio_base + bit;
+			int child_irq =
+				irq_find_mapping(bank->parent_priv->irq_domain,
+						 hwirq);
+			u32 stat = bank->bgc.read_reg(reg_base +
+						      GIO_STAT(bank->id));
+			bank->bgc.write_reg(reg_base + GIO_STAT(bank->id),
+					    stat | BIT(bit));
+			generic_handle_irq(child_irq);
+		}
+	}
+	spin_unlock_irqrestore(&bank->bgc.lock, flags);
+}
+
+/* Each UPG GIO block has one IRQ for all banks */
+static void brcmstb_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct brcmstb_gpio_priv *priv = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct list_head *pos;
+
+	chained_irq_enter(chip, desc);
+	list_for_each(pos, &priv->bank_list) {
+		struct brcmstb_gpio_bank *bank =
+			list_entry(pos, struct brcmstb_gpio_bank, node);
+		brcmstb_gpio_irq_bank_handler(irq, bank);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+static struct brcmstb_gpio_bank *brcmstb_gpio_hwirq_to_bank(
+		struct brcmstb_gpio_priv *priv, irq_hw_number_t hwirq)
+{
+	struct list_head *pos;
+	int i = 0;
+
+	/* banks are in descending order */
+	list_for_each_prev(pos, &priv->bank_list) {
+		struct brcmstb_gpio_bank *bank =
+			list_entry(pos, struct brcmstb_gpio_bank, node);
+		i += bank->bgc.gc.ngpio;
+		if (hwirq < i)
+			return bank;
+	}
+	return NULL;
+}
+
+/*
+ * This lock class tells lockdep that GPIO irqs are in a different
+ * category than their parents, so it won't report false recursion.
+ */
+static struct lock_class_key brcmstb_gpio_irq_lock_class;
+
+
+static int brcmstb_gpio_irq_map(struct irq_domain *d, unsigned int irq,
+		irq_hw_number_t hwirq)
+{
+	struct brcmstb_gpio_priv *priv = d->host_data;
+	struct brcmstb_gpio_bank *bank =
+		brcmstb_gpio_hwirq_to_bank(priv, hwirq);
+	struct platform_device *pdev = priv->pdev;
+	int ret;
+
+	if (!bank)
+		return -EINVAL;
+
+	dev_dbg(&pdev->dev, "Mapping irq %d for gpio line %d (bank %d)\n",
+		irq, (int)hwirq, bank->id);
+	ret = irq_set_chip_data(irq, bank);
+	if (ret < 0)
+		return ret;
+	irq_set_lockdep_class(irq, &brcmstb_gpio_irq_lock_class);
+	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
+#ifdef CONFIG_ARM
+	set_irq_flags(irq, IRQF_VALID);
+#else
+	irq_set_noprobe(irq);
+#endif
+	return 0;
+}
+
+static void brcmstb_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
+{
+	irq_set_chip_and_handler(irq, NULL, NULL);
+	irq_set_chip_data(irq, NULL);
+}
+
+static struct irq_domain_ops brcmstb_gpio_irq_domain_ops = {
+	.map = brcmstb_gpio_irq_map,
+	.unmap = brcmstb_gpio_irq_unmap,
+	.xlate = irq_domain_xlate_twocell,
+};
+
 /* Make sure that the number of banks matches up between properties */
 static int brcmstb_gpio_sanity_check_banks(struct device *dev,
 		struct device_node *np, struct resource *res)
@@ -140,6 +366,11 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	priv->gpio_base = gpio_base;
 	priv->reg_base = reg_base;
 	priv->pdev = pdev;
+	priv->parent_irq = platform_get_irq(pdev, 0);
+	if (priv->parent_irq < 0) {
+		dev_err(dev, "Couldn't get IRQ");
+		return -ENOENT;
+	}
 
 	INIT_LIST_HEAD(&priv->bank_list);
 	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
@@ -181,6 +412,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		gc->base = gpio_base;
 		gc->of_gpio_n_cells = 2;
 		gc->of_xlate = brcmstb_gpio_of_xlate;
+		gc->to_irq = brcmstb_gpio_to_irq;
 
 		if (bank_width <= 0 || bank_width > GPIO_PER_BANK) {
 			gc->ngpio = GPIO_PER_BANK;
@@ -209,6 +441,23 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		priv->num_banks++;
 	}
 
+	priv->num_gpios = gpio_base - priv->gpio_base;
+	priv->irq_chip.name = dev_name(dev);
+	priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
+	priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
+	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
+	priv->irq_domain =
+		irq_domain_add_linear(np, priv->num_gpios,
+				      &brcmstb_gpio_irq_domain_ops,
+				      priv);
+	if (!priv->irq_domain) {
+		dev_err(dev, "Couldn't allocate IRQ domain\n");
+		err = -ENXIO;
+		goto fail;
+	}
+	irq_set_chained_handler(priv->parent_irq, brcmstb_gpio_irq_handler);
+	irq_set_handler_data(priv->parent_irq, priv);
+
 	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
 			priv->num_banks, priv->gpio_base, gpio_base - 1);
 
-- 
1.7.9.5

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Linus Walleij]

On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> There is only one IRQ for each GIO IP block (i.e. several register banks share
> an IRQ).  After briefly looking into the generic IRQ chip implementation, it
> seemed like in this case that using it would result in the driver being more
> complex than necessary because AFAICT it expects a 1:1 mapping of
> irq_chip_generic to gpio_chip.  It seemed like less of a pain to have a single
> irq_chip since we have a single IRQ for all register banks (multiple
> gpio_chips).  I might be missing something, maybe using a shared IRQ across
> multiple irq_chips is easier than I think?  Suggestions welcome.

What is needed is a 1:1 mapping between GPIO offsets and IRQ
offsets.

If you just number your GPIOs 0...n and your IRQs 0...n
it should work just fine with one irqchip for all banks.

What screws things up is likely that the hardware supports
32 lines per bank and not all are used.

I suggest you enable 32 line and 32 IRQs per bank,
so that hwirq maps nicely 1:1 on the GPIO offsets,
then just use the width thing to NACK operations on
GPIO lines you are not using. This way you can also
decode and warn on spurious IRQs on the unused lines.

Yours,
Linus Walleij

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Gregory Fong]

On Tue, May 12, 2015 at 3:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
>
>> There is only one IRQ for each GIO IP block (i.e. several register banks share
>> an IRQ).  After briefly looking into the generic IRQ chip implementation, it
>> seemed like in this case that using it would result in the driver being more
>> complex than necessary because AFAICT it expects a 1:1 mapping of
>> irq_chip_generic to gpio_chip.  It seemed like less of a pain to have a single
>> irq_chip since we have a single IRQ for all register banks (multiple
>> gpio_chips).  I might be missing something, maybe using a shared IRQ across
>> multiple irq_chips is easier than I think?  Suggestions welcome.
>
> What is needed is a 1:1 mapping between GPIO offsets and IRQ
> offsets.
>
> If you just number your GPIOs 0...n and your IRQs 0...n
> it should work just fine with one irqchip for all banks.
>
> What screws things up is likely that the hardware supports
> 32 lines per bank and not all are used.
>
> I suggest you enable 32 line and 32 IRQs per bank,
> so that hwirq maps nicely 1:1 on the GPIO offsets,
> then just use the width thing to NACK operations on
> GPIO lines you are not using. This way you can also
> decode and warn on spurious IRQs on the unused lines.

For having 32 lines per bank, the big problem here is the upper limit
of 256 GPIOs.  We would hit that limit on SoCs that already exist with
the SoC GPIOs alone, let alone any GPIO extenders.  I'm really not
sure what the right way would be to deal with that.

Anyway, I don't think I understand IRQ domains and irq_chip_generic
very well.  One possibility _might_ be to use multiple irq_chips.  But
from what I do understand, if there's only a single parent IRQ used by
all of the GPIO banks, in order to use an irq_chip per bank, this
would have to stop using the chained irq logic because the parent IRQ
is shared across banks, and that implementation seems unnecessarily
confusing.  Any other ideas?

Best regards,
Gregory

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Linus Walleij]

On Tue, May 12, 2015 at 9:38 PM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, May 12, 2015 at 3:59 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> There is only one IRQ for each GIO IP block (i.e. several register banks share
>>> an IRQ).  After briefly looking into the generic IRQ chip implementation, it
>>> seemed like in this case that using it would result in the driver being more
>>> complex than necessary because AFAICT it expects a 1:1 mapping of
>>> irq_chip_generic to gpio_chip.  It seemed like less of a pain to have a single
>>> irq_chip since we have a single IRQ for all register banks (multiple
>>> gpio_chips).  I might be missing something, maybe using a shared IRQ across
>>> multiple irq_chips is easier than I think?  Suggestions welcome.
>>
>> What is needed is a 1:1 mapping between GPIO offsets and IRQ
>> offsets.
>>
>> If you just number your GPIOs 0...n and your IRQs 0...n
>> it should work just fine with one irqchip for all banks.
>>
>> What screws things up is likely that the hardware supports
>> 32 lines per bank and not all are used.
>>
>> I suggest you enable 32 line and 32 IRQs per bank,
>> so that hwirq maps nicely 1:1 on the GPIO offsets,
>> then just use the width thing to NACK operations on
>> GPIO lines you are not using. This way you can also
>> decode and warn on spurious IRQs on the unused lines.
>
> For having 32 lines per bank, the big problem here is the upper limit
> of 256 GPIOs.

Which arch is this?
Usually this limit comes from
arch/*/include/asm/gpio.h

For ARM that was bumped to 512 a while back. It is also possible
to define a custom value for your system by defining
ARCH_NR_GPIOS

> Anyway, I don't think I understand IRQ domains and irq_chip_generic
> very well.  One possibility _might_ be to use multiple irq_chips.

That is probably not possible if there is just one IRQ for all
banks.

The task of the irqdomain is a 1-to-1 translation from one
hardware numberspace to the Linux IRQ number space.

In your case the hardware IRQ (hwirq) numberspace
should be:

bank0: 0..31
bank1: 32..63
....
bankn: 32*n..32*n+31

I think the gpiolib irqchip code can translate that properly
as it is just a simple 0...x mapping, the irq handler need
some magic to loop over all banks from 0..n though.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Gregory Fong]

Hi Linus,

On Wed, May 13, 2015 at 1:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, May 12, 2015 at 9:38 PM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
>> On Tue, May 12, 2015 at 3:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
>>>
>>>> There is only one IRQ for each GIO IP block (i.e. several register banks share
>>>> an IRQ).  After briefly looking into the generic IRQ chip implementation, it
>>>> seemed like in this case that using it would result in the driver being more
>>>> complex than necessary because AFAICT it expects a 1:1 mapping of
>>>> irq_chip_generic to gpio_chip.  It seemed like less of a pain to have a single
>>>> irq_chip since we have a single IRQ for all register banks (multiple
>>>> gpio_chips).  I might be missing something, maybe using a shared IRQ across
>>>> multiple irq_chips is easier than I think?  Suggestions welcome.
>>>
>>> What is needed is a 1:1 mapping between GPIO offsets and IRQ
>>> offsets.
>>>
>>> If you just number your GPIOs 0...n and your IRQs 0...n
>>> it should work just fine with one irqchip for all banks.
>>>
>>> What screws things up is likely that the hardware supports
>>> 32 lines per bank and not all are used.
>>>
>>> I suggest you enable 32 line and 32 IRQs per bank,
>>> so that hwirq maps nicely 1:1 on the GPIO offsets,
>>> then just use the width thing to NACK operations on
>>> GPIO lines you are not using. This way you can also
>>> decode and warn on spurious IRQs on the unused lines.
>>
>> For having 32 lines per bank, the big problem here is the upper limit
>> of 256 GPIOs.
>
> Which arch is this?
> Usually this limit comes from
> arch/*/include/asm/gpio.h
>
> For ARM that was bumped to 512 a while back. It is also possible
> to define a custom value for your system by defining
> ARCH_NR_GPIOS
>
>> Anyway, I don't think I understand IRQ domains and irq_chip_generic
>> very well.  One possibility _might_ be to use multiple irq_chips.
>
> That is probably not possible if there is just one IRQ for all
> banks.
>
> The task of the irqdomain is a 1-to-1 translation from one
> hardware numberspace to the Linux IRQ number space.
>
> In your case the hardware IRQ (hwirq) numberspace
> should be:
>
> bank0: 0..31
> bank1: 32..63
> ....
> bankn: 32*n..32*n+31
>
> I think the gpiolib irqchip code can translate that properly
> as it is just a simple 0...x mapping, the irq handler need
> some magic to loop over all banks from 0..n though.

I've now actually attempted to use the gpiolib irqchip code.  This
driver can't directly use gpiochip_irqchip_add() because of the
multiple gpiochip : one irqchip map.  At first, I thought it might be
possible to simply add a new argument (or break things into a helper
function) to allow setting the associated IRQ domain, but then I can't
use the generic map and unmap functions which expect the irq_domain
host_data member to be struct gpiochip *, which makes no sense in this
case.  That puts me right back to implementing a special version of
the map and unmap function.

Since there doesn't appear to be any benefit to using the gpiolib
irqchip code for this case, I'm going to stick with my implementation
from patch 3 of this patchset.  I've also added to it to allow for
using the GPIOs as a wakeup source, and will submit that as well with
V2.

Thanks,
Gregory

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Linus Walleij]

On Wed, May 27, 2015 at 5:26 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I've now actually attempted to use the gpiolib irqchip code.  This
> driver can't directly use gpiochip_irqchip_add() because of the
> multiple gpiochip : one irqchip map.  At first, I thought it might be
> possible to simply add a new argument (or break things into a helper
> function) to allow setting the associated IRQ domain, but then I can't
> use the generic map and unmap functions which expect the irq_domain
> host_data member to be struct gpiochip *, which makes no sense in this
> case.  That puts me right back to implementing a special version of
> the map and unmap function.

I see.

I wonder if it is better to use one device per bank, and then set
IRQF_SHARED when you issue request_irq(), and have the
IRQ handler return IRQ_NONE if the IRQ comes from this
bank, or IRQ_HANDLED if it has handled an IRQ from its own
space.

This way multiple banks can share a single interrupt line.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 11:05:21AM +0200, Linus Walleij wrote:
> ... and have the
> IRQ handler return IRQ_NONE if the IRQ comes from this
> bank, or IRQ_HANDLED if it has handled an IRQ from its own
> space.

I'm not sure that's worded correctly or not, but... fwiw, all IRQ handlers
_should_ be returning IRQ_NONE if the IRQ does not belong to them whether
or not they're shared handlers.

That is so the kernel can detect stuck IRQs and report that fact, rather
than just locking up silently.  ARM CPUs make no progress when the IRQ to
them is permanently asserted, so a stuck IRQ can be rather problematical.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Linus Walleij]

On Tue, Jun 2, 2015 at 6:28 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 02, 2015 at 11:05:21AM +0200, Linus Walleij wrote:
>> ... and have the
>> IRQ handler return IRQ_NONE if the IRQ comes from this
>> bank, or IRQ_HANDLED if it has handled an IRQ from its own
>> space.
>
> I'm not sure that's worded correctly or not, but... fwiw, all IRQ handlers
> _should_ be returning IRQ_NONE if the IRQ does not belong to them whether
> or not they're shared handlers.

Yup that's right. Sorry for being unclear.

Yours,
Linus Walleij

Re: [PATCH 0/3] GPIO support for BRCMSTB

[Gregory Fong]

On Tue, Jun 2, 2015 at 2:05 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, May 27, 2015 at 5:26 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> I've now actually attempted to use the gpiolib irqchip code.  This
>> driver can't directly use gpiochip_irqchip_add() because of the
>> multiple gpiochip : one irqchip map.  At first, I thought it might be
>> possible to simply add a new argument (or break things into a helper
>> function) to allow setting the associated IRQ domain, but then I can't
>> use the generic map and unmap functions which expect the irq_domain
>> host_data member to be struct gpiochip *, which makes no sense in this
>> case.  That puts me right back to implementing a special version of
>> the map and unmap function.
>
> I see.
>

I think I was wrong, it isn't actually necessary to have a single
irq_chip as long as all of the IRQs use the same handler.  I have a
preliminary implementation doing that instead that seems to work fine.
Once I've had a chance to verify that it works in all cases, will post
that as v3.

Cheers,
Gregory
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html