[PATCH v3 0/4] GPIO support for BRCMSTB

[PATCH v3 0/4] GPIO support for BRCMSTB

[Gregory Fong]

Adds interrupt support for the GPIO controller (UPG GIO) used on Broadcom's
various BRCMSTB SoCs (BCM7XXX and others).  For all existing hardware, this
block hooks up to the BCM7120 L2 IRQ controller and so will require
CONFIG_BCM7120_L2_IRQ=y.

New in v3:
- fix a null pointer dereference noticed by Tim Ross
- use the gpiolib irqchip helpers as recommended by Linus Walleij
- update device tree bindings as suggested by Brian Norris:
  https://lkml.org/lkml/2015/5/29/802
- add S5 (cold boot) support

The following are not included in this patchset:
- Initial device tree bindings (merged from v1 to GPIO tree)
- Initial GPIO support w/o interrupts (merged from v2 to GPIO tree)
- ARM Kconfig changes (merged from v2 to arm-soc tree)

Previous revisions:
v1: https://lkml.org/lkml/2015/5/6/199
v2: https://lkml.org/lkml/2015/5/28/853

Gregory Fong (4):
  gpio: brcmstb: fix null ptr dereference in driver remove
  dt-bindings: brcmstb-gpio: document properties for wakeup
  gpio: brcmstb: Add interrupt and wakeup source support
  gpio: brcmstb: support wakeup from S5 cold boot

 .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt |  35 ++-
 drivers/gpio/Kconfig                               |   1 +
 drivers/gpio/gpio-brcmstb.c                        | 321 ++++++++++++++++++++-
 3 files changed, 341 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH v3 1/4] gpio: brcmstb: fix null ptr dereference in driver remove

[Gregory Fong]

If a failure occurs during probe, brcmstb_gpio_remove() is called. In
remove, we call platform_get_drvdata(), but at the time of failure in
the probe the driver data hadn't yet been set which leads to a NULL
ptr dereference in the remove's list_for_each.  Call
platform_set_drvdata() and set up list head right after allocating the
priv struct to both avoid the null pointer dereference that could
occur today.  To guard against potential future changes, check for
null pointer in remove.

Reported-by: Tim Ross <tross@broadcom.com>
Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
New in v3.

 drivers/gpio/gpio-brcmstb.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 7a3cb1f..4630a81 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -87,6 +87,15 @@ static int brcmstb_gpio_remove(struct platform_device *pdev)
 	struct brcmstb_gpio_bank *bank;
 	int ret = 0;
 
+	if (!priv) {
+		dev_err(&pdev->dev, "called %s without drvdata!\n", __func__);
+		return -EFAULT;
+	}
+
+	/*
+	 * You can lose return values below, but we report all errors, and it's
+	 * more important to actually perform all of the steps.
+	 */
 	list_for_each(pos, &priv->bank_list) {
 		bank = list_entry(pos, struct brcmstb_gpio_bank, node);
 		ret = bgpio_remove(&bank->bgc);
@@ -143,6 +152,8 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	platform_set_drvdata(pdev, priv);
+	INIT_LIST_HEAD(&priv->bank_list);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	reg_base = devm_ioremap_resource(dev, res);
@@ -153,7 +164,6 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	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;
 
@@ -221,8 +231,6 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	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:
-- 
1.9.1

Re: [PATCH v3 1/4] gpio: brcmstb: fix null ptr dereference in driver remove

[Linus Walleij]

On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> If a failure occurs during probe, brcmstb_gpio_remove() is called. In
> remove, we call platform_get_drvdata(), but at the time of failure in
> the probe the driver data hadn't yet been set which leads to a NULL
> ptr dereference in the remove's list_for_each.  Call
> platform_set_drvdata() and set up list head right after allocating the
> priv struct to both avoid the null pointer dereference that could
> occur today.  To guard against potential future changes, check for
> null pointer in remove.
>
> Reported-by: Tim Ross <tross@broadcom.com>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> ---
> New in v3.

Patch applied.

Yours,
Linus Walleij

[PATCH v3 2/4] dt-bindings: brcmstb-gpio: document properties for wakeup

[Gregory Fong]

Some brcmstb GPIO controllers can be used to wake from suspend, so use
the de facto standard property 'wakeup-source' to mark the nodes of
controllers with that capability.

Also document interrupts-extended, which will be used for wakeup
handling because the interrupt parent for the wake IRQ is different
from the regular IRQ.

While we're at it, a few more fixes: We don't actually use the
"interrupt-names" property, so remove it from the listed optional
properties and from the examples.  And since we're modifying the
examples, also follow Brian's suggestions to:
- change #gpio-cells, #interrupt-cells, and brcm,gpio-bank-widths from
  hex to dec
- use phandles

Reviewed-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
v3: Update per Brian's suggestions described in above message.

 .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 35 +++++++++++++++++-----
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
index 435f1bc..b405b44 100644
--- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
@@ -33,6 +33,13 @@ Optional properties:
 - interrupt-parent:
     phandle of the parent interrupt controller
 
+- interrupts-extended:
+    Alternate form of specifying interrupts and parents that allows for
+    multiple parents.  This takes precedence over 'interrupts' and
+    'interrupt-parent'.  Wakeup-capable GPIO controllers often route their
+    wakeup interrupt lines through a different interrupt controller than the
+    primary interrupt line, making this property necessary.
+
 - #interrupt-cells:
     Should be <2>.  The first cell is the GPIO number, the second should specify
     flags.  The following subset of flags is supported:
@@ -47,19 +54,33 @@ Optional properties:
 - interrupt-controller:
     Marks the device node as an interrupt controller
 
-- interrupt-names:
-    The name of the IRQ resource used by this controller
+- wakeup-source:
+    GPIOs for this controller can be used as a wakeup source
 
 Example:
 	upg_gio: gpio@f040a700 {
-		#gpio-cells = <0x2>;
-		#interrupt-cells = <0x2>;
+		#gpio-cells = <2>;
+		#interrupt-cells = <2>;
 		compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
 		gpio-controller;
 		interrupt-controller;
 		reg = <0xf040a700 0x80>;
-		interrupt-parent = <0xf>;
+		interrupt-parent = <&irq0_intc>;
+		interrupts = <0x6>;
+		brcm,gpio-bank-widths = <32 32 32 24>;
+	};
+
+	upg_gio_aon: gpio@f04172c0 {
+		#gpio-cells = <2>;
+		#interrupt-cells = <2>;
+		compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
+		gpio-controller;
+		interrupt-controller;
+		reg = <0xf04172c0 0x40>;
+		interrupt-parent = <&irq0_aon_intc>;
 		interrupts = <0x6>;
-		interrupt-names = "upg_gio";
-		brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>;
+		interrupts-extended = <&irq0_aon_intc 0x6>,
+			<&aon_pm_l2_intc 0x5>;
+		wakeup-source;
+		brcm,gpio-bank-widths = <18 4>;
 	};
-- 
1.9.1

Re: [PATCH v3 2/4] dt-bindings: brcmstb-gpio: document properties for wakeup

[Linus Walleij]

On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Some brcmstb GPIO controllers can be used to wake from suspend, so use
> the de facto standard property 'wakeup-source' to mark the nodes of
> controllers with that capability.
>
> Also document interrupts-extended, which will be used for wakeup
> handling because the interrupt parent for the wake IRQ is different
> from the regular IRQ.
>
> While we're at it, a few more fixes: We don't actually use the
> "interrupt-names" property, so remove it from the listed optional
> properties and from the examples.  And since we're modifying the
> examples, also follow Brian's suggestions to:
> - change #gpio-cells, #interrupt-cells, and brcm,gpio-bank-widths from
>   hex to dec
> - use phandles
>
> Reviewed-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> v3: Update per Brian's suggestions described in above message.

I'm very uncertain regarding these interrupts-extended etc.

Can someone who understands that ACK this?

E.g. Grant Likely, Björn Helgaas, Rob Herring.

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 v3 2/4] dt-bindings: brcmstb-gpio: document properties for wakeup

[Florian Fainelli]

On 13/07/15 05:29, Linus Walleij wrote:
> On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Some brcmstb GPIO controllers can be used to wake from suspend, so use
>> the de facto standard property 'wakeup-source' to mark the nodes of
>> controllers with that capability.
>>
>> Also document interrupts-extended, which will be used for wakeup
>> handling because the interrupt parent for the wake IRQ is different
>> from the regular IRQ.
>>
>> While we're at it, a few more fixes: We don't actually use the
>> "interrupt-names" property, so remove it from the listed optional
>> properties and from the examples.  And since we're modifying the
>> examples, also follow Brian's suggestions to:
>> - change #gpio-cells, #interrupt-cells, and brcm,gpio-bank-widths from
>>   hex to dec
>> - use phandles
>>
>> Reviewed-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> v3: Update per Brian's suggestions described in above message.
> 
> I'm very uncertain regarding these interrupts-extended etc.

Gregory's additions to the binding are correct and comply with how the
'interrupts-extended' property is meant to be used. FYI, this is
required because most wake-up capable HW on brcmstb chips have N
interrupts feeding to a GIC, but their wake-up interrupt(s) feed to an
always-on interrupt controller. With the 'interrupts' +
'interrupt-parent' property, you can "cross" multiple interrupt domains.

The precedence and rules for interpreting the "legacy" interrupts
property vs. interrupts-extended is documented in the tree.

> 
> Can someone who understands that ACK this?

Acked-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> 
> E.g. Grant Likely, Björn Helgaas, Rob Herring.
> 
> Yours,
> Linus Walleij
> 


-- 
Florian
--
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 v3 3/4] gpio: brcmstb: Add interrupt and wakeup source support

[Gregory Fong]

Uses the gpiolib irqchip helpers.  For this to work, the irq setup
function is called once per bank instead of once per device.  Note
that all known uses of this block have a BCM7120 L2 interrupt
controller as a parent.  Supports interrupts for all GPIOs.

In the IRQ handler, we check for raised IRQs for invalid GPIOs and
warn (ratelimited) if they're encountered.

Also, several drivers (e.g. gpio-keys) allow for GPIOs to be
configured as wakeup sources, and this GPIO controller supports that
through a separate interrupt path.

The de-facto standard DT property "wakeup-source" is checked, since
that indicates whether the GPIO controller hardware can wake.  Uses
the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
any of its own wakeup source configuration.

Aside regarding gpiolib irqchip helpers: It wasn't obvious (to me)
that you can have multiple chained irqchips and associated IRQ domains
for a single parent IRQ, and as long as the xlate function is written
correctly, a GPIO IRQ request end up checking the correct domain and
will get associated with the correct IRQ.  What helps make this clear
is to read
  drivers/gpio/gpiolib-of.c:
   - of_gpiochip_find_and_xlate()
   - of_get_named_gpiod_flags()
  drivers/gpio/gpiolib.c:
   - gpiochip_find()

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
v3:
- combine commits to add interrupt support and allow GPIOs to be wakeup sources
- change to use the gpiolib irqchip helpers, reducing unnecessary code
  duplication.

 drivers/gpio/Kconfig        |   1 +
 drivers/gpio/gpio-brcmstb.c | 263 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 258 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5f79b7f..f723c7e 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 GPIOLIB_IRQCHIP
 	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 4630a81..141509b 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -17,6 +17,9 @@
 #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>
+#include <linux/interrupt.h>
 
 #define GIO_BANK_SIZE           0x20
 #define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
@@ -34,14 +37,17 @@ struct brcmstb_gpio_bank {
 	struct bgpio_chip bgc;
 	struct brcmstb_gpio_priv *parent_priv;
 	u32 width;
+	struct irq_chip irq_chip;
 };
 
 struct brcmstb_gpio_priv {
 	struct list_head bank_list;
 	void __iomem *reg_base;
-	int num_banks;
 	struct platform_device *pdev;
+	int parent_irq;
 	int gpio_base;
+	bool can_wake;
+	int parent_wake_irq;
 };
 
 #define MAX_GPIO_PER_BANK           32
@@ -63,6 +69,184 @@ 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;
+	u32 mask = bgc->pin2mask(bgc, offset);
+	u32 imask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	imask = bgc->read_reg(priv->reg_base + GIO_MASK(bank->id));
+	if (enable)
+		imask |= mask;
+	else
+		imask &= ~mask;
+	bgc->write_reg(priv->reg_base + GIO_MASK(bank->id), imask);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+/* -------------------- IRQ chip functions -------------------- */
+
+static void brcmstb_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_bank *bank = brcmstb_gpio_gc_to_bank(gc);
+
+	brcmstb_gpio_set_imask(bank, d->hwirq, false);
+}
+
+static void brcmstb_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_bank *bank = brcmstb_gpio_gc_to_bank(gc);
+
+	brcmstb_gpio_set_imask(bank, d->hwirq, true);
+}
+
+static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_bank *bank = brcmstb_gpio_gc_to_bank(gc);
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	u32 mask = BIT(d->hwirq);
+	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 int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+	int ret = 0;
+
+	/*
+	 * Only enable wake IRQ once for however many hwirqs can wake
+	 * since they all use the same wake IRQ.  Mask will be set
+	 * up appropriately thanks to IRQCHIP_MASK_ON_SUSPEND flag.
+	 */
+	if (enable)
+		ret = enable_irq_wake(priv->parent_wake_irq);
+	else
+		ret = disable_irq_wake(priv->parent_wake_irq);
+	if (ret)
+		dev_err(&priv->pdev->dev, "failed to %s wake-up interrupt\n",
+				enable ? "enable" : "disable");
+	return ret;
+}
+
+static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
+{
+	struct brcmstb_gpio_priv *priv = data;
+
+	if (!priv || irq != priv->parent_wake_irq)
+		return IRQ_NONE;
+	pm_wakeup_event(&priv->pdev->dev, 0);
+	return IRQ_HANDLED;
+}
+
+static void brcmstb_gpio_irq_bank_handler(int irq,
+		struct brcmstb_gpio_bank *bank)
+{
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	struct irq_domain *irq_domain = bank->bgc.gc.irqdomain;
+	void __iomem *reg_base = 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->bgc.read_reg(reg_base + GIO_MASK(bank->id)))) {
+		int bit;
+
+		for_each_set_bit(bit, &status, 32) {
+			u32 stat = bank->bgc.read_reg(reg_base +
+						      GIO_STAT(bank->id));
+			if (bit >= bank->width)
+				dev_warn(&priv->pdev->dev,
+					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
+					 bank->id, bit);
+			bank->bgc.write_reg(reg_base + GIO_STAT(bank->id),
+					    stat | BIT(bit));
+			generic_handle_irq(irq_find_mapping(irq_domain, bit));
+		}
+	}
+	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 gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct list_head *pos;
+
+	/* Interrupts weren't properly cleared during probe */
+	BUG_ON(!priv || !chip);
+
+	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);
+}
+
 /* 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)
@@ -100,7 +284,7 @@ static int brcmstb_gpio_remove(struct platform_device *pdev)
 		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");
+			dev_err(&pdev->dev, "gpiochip_remove fail in cleanup\n");
 	}
 	return ret;
 }
@@ -121,7 +305,7 @@ static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
 		return -EINVAL;
 
 	offset = gpiospec->args[0] - (gc->base - priv->gpio_base);
-	if (offset >= gc->ngpio)
+	if (offset >= gc->ngpio || offset < 0)
 		return -EINVAL;
 
 	if (unlikely(offset >= bank->width)) {
@@ -136,6 +320,55 @@ static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
 	return offset;
 }
 
+/* Before calling, must have bank->parent_irq set and gpiochip registered */
+static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
+		struct brcmstb_gpio_bank *bank)
+{
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	bank->irq_chip.name = dev_name(dev);
+	bank->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
+	bank->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
+	bank->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
+
+	/* Ensures that all non-wakeup IRQs are disabled at suspend */
+	bank->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND;
+
+	if (IS_ENABLED(CONFIG_PM_SLEEP) && !priv->can_wake &&
+			of_property_read_bool(np, "wakeup-source")) {
+		priv->parent_wake_irq = platform_get_irq(pdev, 1);
+		if (priv->parent_wake_irq < 0) {
+			dev_warn(dev,
+				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
+		} else {
+			int err = devm_request_irq(dev, priv->parent_wake_irq,
+					brcmstb_gpio_wake_irq_handler, 0,
+					"brcmstb-gpio-wake", priv);
+
+			if (err < 0) {
+				dev_err(dev, "Couldn't request wake IRQ");
+				return err;
+			}
+
+			device_set_wakeup_capable(dev, true);
+			device_wakeup_enable(dev);
+			priv->can_wake = true;
+		}
+	}
+
+	if (priv->can_wake)
+		bank->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
+
+	gpiochip_irqchip_add(&bank->bgc.gc, &bank->irq_chip, 0,
+			handle_simple_irq, IRQ_TYPE_NONE);
+	gpiochip_set_chained_irqchip(&bank->bgc.gc, &bank->irq_chip,
+			priv->parent_irq, brcmstb_gpio_irq_handler);
+
+	return 0;
+}
+
 static int brcmstb_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -146,6 +379,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	struct property *prop;
 	const __be32 *p;
 	u32 bank_width;
+	int num_banks = 0;
 	int err;
 	static int gpio_base;
 
@@ -164,6 +398,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	priv->reg_base = reg_base;
 	priv->pdev = pdev;
 
+	if (of_property_read_bool(np, "interrupt-controller")) {
+		priv->parent_irq = platform_get_irq(pdev, 0);
+		if (priv->parent_irq < 0) {
+			dev_err(dev, "Couldn't get IRQ");
+			return -ENOENT;
+		}
+	} else {
+		priv->parent_irq = -ENOENT;
+	}
+
 	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
 		return -EINVAL;
 
@@ -180,7 +424,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		}
 
 		bank->parent_priv = priv;
-		bank->id = priv->num_banks;
+		bank->id = num_banks;
 		if (bank_width <= 0 || bank_width > MAX_GPIO_PER_BANK) {
 			dev_err(dev, "Invalid bank width %d\n", bank_width);
 			goto fail;
@@ -219,17 +463,24 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 			goto fail;
 		}
 		gpio_base += gc->ngpio;
+
+		if (priv->parent_irq >= 0) {
+			err = brcmstb_gpio_irq_setup(pdev, bank);
+			if (err)
+				goto fail;
+		}
+
 		dev_dbg(dev, "bank=%d, base=%d, ngpio=%d, width=%d\n", bank->id,
 			gc->base, gc->ngpio, bank->width);
 
 		/* Everything looks good, so add bank to list */
 		list_add(&bank->node, &priv->bank_list);
 
-		priv->num_banks++;
+		num_banks++;
 	}
 
 	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
-			priv->num_banks, priv->gpio_base, gpio_base - 1);
+			num_banks, priv->gpio_base, gpio_base - 1);
 
 	return 0;
 
-- 
1.9.1

Re: [PATCH v3 3/4] gpio: brcmstb: Add interrupt and wakeup source support

[Linus Walleij]

On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> Uses the gpiolib irqchip helpers.  For this to work, the irq setup
> function is called once per bank instead of once per device.  Note
> that all known uses of this block have a BCM7120 L2 interrupt
> controller as a parent.  Supports interrupts for all GPIOs.
>
> In the IRQ handler, we check for raised IRQs for invalid GPIOs and
> warn (ratelimited) if they're encountered.
>
> Also, several drivers (e.g. gpio-keys) allow for GPIOs to be
> configured as wakeup sources, and this GPIO controller supports that
> through a separate interrupt path.
>
> The de-facto standard DT property "wakeup-source" is checked, since
> that indicates whether the GPIO controller hardware can wake.  Uses
> the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
> any of its own wakeup source configuration.
>
> Aside regarding gpiolib irqchip helpers: It wasn't obvious (to me)
> that you can have multiple chained irqchips and associated IRQ domains
> for a single parent IRQ, and as long as the xlate function is written
> correctly, a GPIO IRQ request end up checking the correct domain and
> will get associated with the correct IRQ.  What helps make this clear
> is to read
>   drivers/gpio/gpiolib-of.c:
>    - of_gpiochip_find_and_xlate()
>    - of_get_named_gpiod_flags()
>   drivers/gpio/gpiolib.c:
>    - gpiochip_find()

Sorry for the unclarities, this is a bit hairy. Suggestions as to
how we can make it easier and/or bring code for this into gpiolib
are welcome. Right now I have a hard time seeing any way to
make this more generic and helpful :/

Overall this patch looks real nice. Some nitpicks below.

> @@ -164,6 +398,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>         priv->reg_base = reg_base;
>         priv->pdev = pdev;
>
> +       if (of_property_read_bool(np, "interrupt-controller")) {
> +               priv->parent_irq = platform_get_irq(pdev, 0);
> +               if (priv->parent_irq < 0) {

This should be <= 0 since 0 is NO_IRQ

> +                       dev_err(dev, "Couldn't get IRQ");
> +                       return -ENOENT;
> +               }
> +       } else {
> +               priv->parent_irq = -ENOENT;
> +       }

Why should this code only execute if the node is marked
"interrupt-controller"? It makes it seem like IRQs cannot arrive
to it unless it is intended to serve other consumers.

Maybe in practice this is true, but...

> +               if (priv->parent_irq >= 0) {
> +                       err = brcmstb_gpio_irq_setup(pdev, bank);
> +                       if (err)
> +                               goto fail;
> +               }

Again 0 is NO_IRQ so this should be > 0 not >= 0.

Yours,
Linus Walleij

Re: [PATCH v3 3/4] gpio: brcmstb: Add interrupt and wakeup source support

[Gregory Fong]

On Mon, Jul 13, 2015 at 5:58 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Uses the gpiolib irqchip helpers.  For this to work, the irq setup
>> function is called once per bank instead of once per device.  Note
>> that all known uses of this block have a BCM7120 L2 interrupt
>> controller as a parent.  Supports interrupts for all GPIOs.
>>
>> In the IRQ handler, we check for raised IRQs for invalid GPIOs and
>> warn (ratelimited) if they're encountered.
>>
>> Also, several drivers (e.g. gpio-keys) allow for GPIOs to be
>> configured as wakeup sources, and this GPIO controller supports that
>> through a separate interrupt path.
>>
>> The de-facto standard DT property "wakeup-source" is checked, since
>> that indicates whether the GPIO controller hardware can wake.  Uses
>> the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
>> any of its own wakeup source configuration.
>>
>> Aside regarding gpiolib irqchip helpers: It wasn't obvious (to me)
>> that you can have multiple chained irqchips and associated IRQ domains
>> for a single parent IRQ, and as long as the xlate function is written
>> correctly, a GPIO IRQ request end up checking the correct domain and
>> will get associated with the correct IRQ.  What helps make this clear
>> is to read
>>   drivers/gpio/gpiolib-of.c:
>>    - of_gpiochip_find_and_xlate()
>>    - of_get_named_gpiod_flags()
>>   drivers/gpio/gpiolib.c:
>>    - gpiochip_find()
>
> Sorry for the unclarities, this is a bit hairy. Suggestions as to
> how we can make it easier and/or bring code for this into gpiolib
> are welcome. Right now I have a hard time seeing any way to
> make this more generic and helpful :/

I'll see about putting together an update to the documentation
discussing more about the case where you have one IRQ shared by
multiple GPIO banks.

>
> Overall this patch looks real nice. Some nitpicks below.
>
>> @@ -164,6 +398,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>>         priv->reg_base = reg_base;
>>         priv->pdev = pdev;
>>
>> +       if (of_property_read_bool(np, "interrupt-controller")) {
>> +               priv->parent_irq = platform_get_irq(pdev, 0);
>> +               if (priv->parent_irq < 0) {
>
> This should be <= 0 since 0 is NO_IRQ

Will fix.

>
>> +                       dev_err(dev, "Couldn't get IRQ");
>> +                       return -ENOENT;
>> +               }
>> +       } else {
>> +               priv->parent_irq = -ENOENT;
>> +       }
>
> Why should this code only execute if the node is marked
> "interrupt-controller"? It makes it seem like IRQs cannot arrive
> to it unless it is intended to serve other consumers.
>
> Maybe in practice this is true, but...

If the node does not contain the "interrupt-controller" property, the
hardware does not support GPIO interrupts, and I designed the driver
specifically to allow that to work.
If the node does contain that property, then being unable to complete
IRQ setup is a fatal error, because something is badly misconfigured.

>
>> +               if (priv->parent_irq >= 0) {
>> +                       err = brcmstb_gpio_irq_setup(pdev, bank);
>> +                       if (err)
>> +                               goto fail;
>> +               }
>
> Again 0 is NO_IRQ so this should be > 0 not >= 0.

OK, will change.

Thanks,
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

[PATCH v3 4/4] gpio: brcmstb: support wakeup from S5 cold boot

[Gregory Fong]

For wake from S5, we need to:
- register a reboot handler
- set wakeup capability before requesting IRQ so wakeup count is
  incremented
- mask all GPIO IRQs and clear any pending interrupts during driver
  probe to since no driver will yet be registered to handle any IRQs
  carried over from boot at that time, and it's possible that the
  booted kernel does not request the same IRQ anyway.

This means that /sys/.../power/wakeup_count is valid at boot time, and
we can properly account for S5 wakeup stats. e.g.:

  ### After waking from S5 from a GPIO key
  # cat /sys/bus/platform/drivers/brcmstb-gpio/f04172c0.gpio/power/wakeup
  enabled
  # cat /sys/bus/platform/drivers/brcmstb-gpio/f04172c0.gpio/power/wakeup_count
  1

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
New in v3.

 drivers/gpio/gpio-brcmstb.c | 56 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 141509b..dedb35c 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -20,6 +20,7 @@
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/interrupt.h>
+#include <linux/reboot.h>
 
 #define GIO_BANK_SIZE           0x20
 #define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
@@ -48,6 +49,7 @@ struct brcmstb_gpio_priv {
 	int gpio_base;
 	bool can_wake;
 	int parent_wake_irq;
+	struct notifier_block reboot_notifier;
 };
 
 #define MAX_GPIO_PER_BANK           32
@@ -167,10 +169,9 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
+static int __brcmstb_gpio_irq_set_wake(struct brcmstb_gpio_priv *priv,
+		unsigned int enable)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
 	int ret = 0;
 
 	/*
@@ -188,6 +189,14 @@ static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
 	return ret;
 }
 
+static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+
+	return __brcmstb_gpio_irq_set_wake(priv, enable);
+}
+
 static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
 {
 	struct brcmstb_gpio_priv *priv = data;
@@ -247,6 +256,19 @@ static void brcmstb_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int brcmstb_gpio_reboot(struct notifier_block *nb,
+		unsigned long action, void *data)
+{
+	struct brcmstb_gpio_priv *priv =
+		container_of(nb, struct brcmstb_gpio_priv, reboot_notifier);
+
+	/* Enable GPIO for S5 cold boot */
+	if (action == SYS_POWER_OFF)
+		__brcmstb_gpio_irq_set_wake(priv, 1);
+
+	return NOTIFY_DONE;
+}
+
 /* 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)
@@ -286,6 +308,12 @@ static int brcmstb_gpio_remove(struct platform_device *pdev)
 		if (ret)
 			dev_err(&pdev->dev, "gpiochip_remove fail in cleanup\n");
 	}
+	if (priv->reboot_notifier.notifier_call) {
+		ret = unregister_reboot_notifier(&priv->reboot_notifier);
+		if (ret)
+			dev_err(&pdev->dev,
+				"failed to unregister reboot notifier\n");
+	}
 	return ret;
 }
 
@@ -343,7 +371,16 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 			dev_warn(dev,
 				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
 		} else {
-			int err = devm_request_irq(dev, priv->parent_wake_irq,
+			int err;
+
+			/*
+			 * Set wakeup capability before requesting wakeup
+			 * interrupt, so we can process boot-time "wakeups"
+			 * (e.g., from S5 cold boot)
+			 */
+			device_set_wakeup_capable(dev, true);
+			device_wakeup_enable(dev);
+			err = devm_request_irq(dev, priv->parent_wake_irq,
 					brcmstb_gpio_wake_irq_handler, 0,
 					"brcmstb-gpio-wake", priv);
 
@@ -352,8 +389,9 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 				return err;
 			}
 
-			device_set_wakeup_capable(dev, true);
-			device_wakeup_enable(dev);
+			priv->reboot_notifier.notifier_call =
+				brcmstb_gpio_reboot;
+			register_reboot_notifier(&priv->reboot_notifier);
 			priv->can_wake = true;
 		}
 	}
@@ -456,6 +494,12 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		/* not all ngpio lines are valid, will use bank width later */
 		gc->ngpio = MAX_GPIO_PER_BANK;
 
+		/*
+		 * Mask all interrupts by default, since wakeup interrupts may
+		 * be retained from S5 cold boot
+		 */
+		bank->bgc.write_reg(reg_base + GIO_MASK(bank->id), 0);
+
 		err = gpiochip_add(gc);
 		if (err) {
 			dev_err(dev, "Could not add gpiochip for bank %d\n",
-- 
1.9.1

Re: [PATCH v3 4/4] gpio: brcmstb: support wakeup from S5 cold boot

[Linus Walleij]

On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> For wake from S5, we need to:
> - register a reboot handler
> - set wakeup capability before requesting IRQ so wakeup count is
>   incremented
> - mask all GPIO IRQs and clear any pending interrupts during driver
>   probe to since no driver will yet be registered to handle any IRQs
>   carried over from boot at that time, and it's possible that the
>   booted kernel does not request the same IRQ anyway.
>
> This means that /sys/.../power/wakeup_count is valid at boot time, and
> we can properly account for S5 wakeup stats. e.g.:
>
>   ### After waking from S5 from a GPIO key
>   # cat /sys/bus/platform/drivers/brcmstb-gpio/f04172c0.gpio/power/wakeup
>   enabled
>   # cat /sys/bus/platform/drivers/brcmstb-gpio/f04172c0.gpio/power/wakeup_count
>   1
>
> Signed-off-by: Gregory Fong <gregory.0xf0-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

(...)
> -static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
> +static int __brcmstb_gpio_irq_set_wake(struct brcmstb_gpio_priv *priv,
> +               unsigned int enable)
>  {
> -       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -       struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
>         int ret = 0;

I don't usually like to refactor code with __foo wrapper functions with
underscores or double underscores in front of them.

Is it possible to give this a more unique name?

> +               /*
> +                * Mask all interrupts by default, since wakeup interrupts may
> +                * be retained from S5 cold boot
> +                */
> +               bank->bgc.write_reg(reg_base + GIO_MASK(bank->id), 0);

Aha I see, that's some clever code, nice.

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 v3 4/4] gpio: brcmstb: support wakeup from S5 cold boot

[Gregory Fong]

On Mon, Jul 13, 2015 at 6:03 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 18, 2015 at 3:00 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
>
>> For wake from S5, we need to:
>> - register a reboot handler
>> - set wakeup capability before requesting IRQ so wakeup count is
>>   incremented
>> - mask all GPIO IRQs and clear any pending interrupts during driver
>>   probe to since no driver will yet be registered to handle any IRQs
>>   carried over from boot at that time, and it's possible that the
>>   booted kernel does not request the same IRQ anyway.
>>
>> This means that /sys/.../power/wakeup_count is valid at boot time, and
>> we can properly account for S5 wakeup stats. e.g.:
>>
>>   ### After waking from S5 from a GPIO key
>>   # cat /sys/bus/platform/drivers/brcmstb-gpio/f04172c0.gpio/power/wakeup
>>   enabled
>>   # cat /sys/bus/platform/drivers/brcmstb-gpio/f04172c0.gpio/power/wakeup_count
>>   1
>>
>> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
>
> (...)
>> -static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
>> +static int __brcmstb_gpio_irq_set_wake(struct brcmstb_gpio_priv *priv,
>> +               unsigned int enable)
>>  {
>> -       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> -       struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
>>         int ret = 0;
>
> I don't usually like to refactor code with __foo wrapper functions with
> underscores or double underscores in front of them.
>
> Is it possible to give this a more unique name?

Sure, just have to come up with a good one :-).

>
>> +               /*
>> +                * Mask all interrupts by default, since wakeup interrupts may
>> +                * be retained from S5 cold boot
>> +                */
>> +               bank->bgc.write_reg(reg_base + GIO_MASK(bank->id), 0);
>
> Aha I see, that's some clever code, nice.
>
> Yours,
> Linus Walleij