[PATCH 0/7] gpio: brcmstb: improved interrupt and wake support

[PATCH 0/7] gpio: brcmstb: improved interrupt and wake support

[Doug Berger]

This patch set collects a number of improvements to the GPIO driver
used by Broadcom Set-Top-Box devices.

Primarily they are aimed at correcting problems with the interrupt
controller implementation, but they also extend the functionality for
waking on GPIO interrupts.

Doug Berger (7):
  gpio: brcmstb: allow all instances to be wakeup sources
  gpio: brcmstb: release the bgpio lock during irq handlers
  gpio: brcmstb: switch to handle_level_irq flow
  gpio: brcmstb: correct the configuration of level interrupts
  gpio: brcmstb: enable masking of interrupts when changing type
  gpio: brcmstb: consolidate interrupt domains
  gpio: brcmstb: implement suspend/resume/shutdown

 drivers/gpio/Kconfig        |   2 +-
 drivers/gpio/gpio-brcmstb.c | 419 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 321 insertions(+), 100 deletions(-)

-- 
2.14.1

[PATCH 1/7] gpio: brcmstb: allow all instances to be wakeup sources

[Doug Berger]

This commit allows a wakeup parent interrupt to be shared between
instances.

It also removes the redundant can_wake member of the private data
structure by using whether the parent_wake_irq has been defined to
indicate that the GPIO device can wake.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 27e92e57adae..7f39b160a4d5 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Broadcom Corporation
+ * Copyright (C) 2015-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -46,7 +46,6 @@ struct brcmstb_gpio_priv {
 	struct platform_device *pdev;
 	int parent_irq;
 	int gpio_base;
-	bool can_wake;
 	int parent_wake_irq;
 	struct notifier_block reboot_notifier;
 };
@@ -349,10 +348,11 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	/* 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 &&
+	if (IS_ENABLED(CONFIG_PM_SLEEP) && !priv->parent_wake_irq &&
 			of_property_read_bool(np, "wakeup-source")) {
 		priv->parent_wake_irq = platform_get_irq(pdev, 1);
 		if (priv->parent_wake_irq < 0) {
+			priv->parent_wake_irq = 0;
 			dev_warn(dev,
 				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
 		} else {
@@ -364,8 +364,9 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 			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);
+					       brcmstb_gpio_wake_irq_handler,
+					       IRQF_SHARED,
+					       "brcmstb-gpio-wake", priv);
 
 			if (err < 0) {
 				dev_err(dev, "Couldn't request wake IRQ");
@@ -375,11 +376,10 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 			priv->reboot_notifier.notifier_call =
 				brcmstb_gpio_reboot;
 			register_reboot_notifier(&priv->reboot_notifier);
-			priv->can_wake = true;
 		}
 	}
 
-	if (priv->can_wake)
+	if (priv->parent_wake_irq)
 		bank->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
 
 	err = gpiochip_irqchip_add(&bank->gc, &bank->irq_chip, 0,
-- 
2.14.1

Re: [PATCH 1/7] gpio: brcmstb: allow all instances to be wakeup sources

[Gregory Fong]

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> This commit allows a wakeup parent interrupt to be shared between
> instances.
>
> It also removes the redundant can_wake member of the private data
> structure by using whether the parent_wake_irq has been defined to
> indicate that the GPIO device can wake.
>
> Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

Re: [PATCH 1/7] gpio: brcmstb: allow all instances to be wakeup sources

[Linus Walleij]

On Sat, Sep 30, 2017 at 5:40 AM, Doug Berger <opendmb@gmail.com> wrote:

> This commit allows a wakeup parent interrupt to be shared between
> instances.
>
> It also removes the redundant can_wake member of the private data
> structure by using whether the parent_wake_irq has been defined to
> indicate that the GPIO device can wake.
>
> Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Patch applied with Gregory's and Florian's ACKs.

Yours,
Linus Walleij

[PATCH 2/7] gpio: brcmstb: release the bgpio lock during irq handlers

[Doug Berger]

The basic memory-mapped GPIO controller lock must be released
before calling the registered GPIO interrupt handlers to allow
the interrupt handlers to access the hardware.  Otherwise, the
hardware accesses will deadlock when they attempt to grab the
lock.

Since the lock is only needed to protect the calculation of
unmasked pending interrupts create a dedicated function to
perform this and hide the complexity.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 7f39b160a4d5..8945861876f9 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -62,6 +62,21 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
 	return bank->parent_priv;
 }
 
+static unsigned long
+brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
+{
+	void __iomem *reg_base = bank->parent_priv->reg_base;
+	unsigned long status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
+	status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
+		 bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
+	spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
+
+	return status;
+}
+
 static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
 		unsigned int offset, bool enable)
 {
@@ -204,11 +219,8 @@ static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 	struct irq_domain *irq_domain = bank->gc.irqdomain;
 	void __iomem *reg_base = priv->reg_base;
 	unsigned long status;
-	unsigned long flags;
 
-	spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
-	while ((status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
-			 bank->gc.read_reg(reg_base + GIO_MASK(bank->id)))) {
+	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
 		int bit;
 
 		for_each_set_bit(bit, &status, 32) {
@@ -223,7 +235,6 @@ static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 			generic_handle_irq(irq_find_mapping(irq_domain, bit));
 		}
 	}
-	spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
 }
 
 /* Each UPG GIO block has one IRQ for all banks */
-- 
2.14.1

Re: [PATCH 2/7] gpio: brcmstb: release the bgpio lock during irq handlers

[Gregory Fong]

Hi Doug,

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> The basic memory-mapped GPIO controller lock must be released
> before calling the registered GPIO interrupt handlers to allow
> the interrupt handlers to access the hardware.  Otherwise, the
> hardware accesses will deadlock when they attempt to grab the
> lock.

I was having some trouble understanding exactly what the problem was
here, but I think I see it now.  Since this locks the entire bank,
where some GPIOs might be set as inputs and some as inputs (and
interrupt sources), then an interrupt on a GPIO that is supposed to
set another GPIO in the bank would result in deadlock.  Is that
correct?  If so, please update the commit message to make that clear,
and nice fix.  If not that, it would be nice to know what scenario can
cause a problem.

Thanks,
Gregory

Re: [PATCH 2/7] gpio: brcmstb: release the bgpio lock during irq handlers

[Doug Berger]

On 10/03/2017 06:55 PM, Gregory Fong wrote:
> Hi Doug,
> 
> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>> The basic memory-mapped GPIO controller lock must be released
>> before calling the registered GPIO interrupt handlers to allow
>> the interrupt handlers to access the hardware.  Otherwise, the
>> hardware accesses will deadlock when they attempt to grab the
>> lock.
> 
> I was having some trouble understanding exactly what the problem was
> here, but I think I see it now.  Since this locks the entire bank,
> where some GPIOs might be set as inputs and some as inputs (and
> interrupt sources), then an interrupt on a GPIO that is supposed to
> set another GPIO in the bank would result in deadlock.  Is that
> correct?  If so, please update the commit message to make that clear,
> and nice fix.  If not that, it would be nice to know what scenario can
> cause a problem.

That is an example, but there are really many possibilities.

Basically, if a registered interrupt handler wants to access its GPIO
you are likely to run into trouble.  Another example might be an
interrupt that is configured to trigger on either edge transition and
the handler wants to know whether the input is currently high or low.

I can submit a V2 with a change in the description if you would like,
but I'm not sure what the clearest example would be.

> 
> Thanks,
> Gregory
> 

Thanks for the feedback,
    Doug

Re: [PATCH 2/7] gpio: brcmstb: release the bgpio lock during irq handlers

[Gregory Fong]

On Tue, Oct 3, 2017 at 7:09 PM, Doug Berger <opendmb@gmail.com> wrote:
> On 10/03/2017 06:55 PM, Gregory Fong wrote:
>> Hi Doug,
>>
>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>>> The basic memory-mapped GPIO controller lock must be released
>>> before calling the registered GPIO interrupt handlers to allow
>>> the interrupt handlers to access the hardware.  Otherwise, the
>>> hardware accesses will deadlock when they attempt to grab the
>>> lock.
>>
>> I was having some trouble understanding exactly what the problem was
>> here, but I think I see it now.  Since this locks the entire bank,
>> where some GPIOs might be set as inputs and some as inputs (and
>> interrupt sources), then an interrupt on a GPIO that is supposed to
>> set another GPIO in the bank would result in deadlock.  Is that
>> correct?  If so, please update the commit message to make that clear,
>> and nice fix.  If not that, it would be nice to know what scenario can
>> cause a problem.
>
> That is an example, but there are really many possibilities.
>
> Basically, if a registered interrupt handler wants to access its GPIO
> you are likely to run into trouble.  Another example might be an
> interrupt that is configured to trigger on either edge transition and
> the handler wants to know whether the input is currently high or low.
>
> I can submit a V2 with a change in the description if you would like,
> but I'm not sure what the clearest example would be.

If you could just mention both of these possible cases, that would be
great! With that change,

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

[PATCH 3/7] gpio: brcmstb: switch to handle_level_irq flow

[Doug Berger]

Reading and writing the gpio bank status register each time a pending
interrupt bit is serviced could cause new pending bits to be cleared
without servicing the associated interrupts.

By using the handle_level_irq flow instead of the handle_simple_irq
flow we get proper handling of interrupt masking as well as acking
of interrupts.  The irq_ack method is added to support this.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 8945861876f9..c186141927bb 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -114,6 +114,16 @@ static void brcmstb_gpio_irq_unmask(struct irq_data *d)
 	brcmstb_gpio_set_imask(bank, d->hwirq, true);
 }
 
+static void brcmstb_gpio_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	u32 mask = BIT(d->hwirq);
+
+	gc->write_reg(priv->reg_base + GIO_STAT(bank->id), mask);
+}
+
 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);
@@ -217,21 +227,16 @@ static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 {
 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
 	struct irq_domain *irq_domain = bank->gc.irqdomain;
-	void __iomem *reg_base = priv->reg_base;
 	unsigned long status;
 
 	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
 		int bit;
 
 		for_each_set_bit(bit, &status, 32) {
-			u32 stat = bank->gc.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->gc.write_reg(reg_base + GIO_STAT(bank->id),
-					    stat | BIT(bit));
 			generic_handle_irq(irq_find_mapping(irq_domain, bit));
 		}
 	}
@@ -354,6 +359,7 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	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_ack = brcmstb_gpio_irq_ack;
 	bank->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
 
 	/* Ensures that all non-wakeup IRQs are disabled at suspend */
@@ -394,7 +400,7 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 		bank->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
 
 	err = gpiochip_irqchip_add(&bank->gc, &bank->irq_chip, 0,
-				   handle_simple_irq, IRQ_TYPE_NONE);
+				   handle_level_irq, IRQ_TYPE_NONE);
 	if (err)
 		return err;
 	gpiochip_set_chained_irqchip(&bank->gc, &bank->irq_chip,
-- 
2.14.1

Re: [PATCH 3/7] gpio: brcmstb: switch to handle_level_irq flow

[Gregory Fong]

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> Reading and writing the gpio bank status register each time a pending
> interrupt bit is serviced could cause new pending bits to be cleared
> without servicing the associated interrupts.
>
> By using the handle_level_irq flow instead of the handle_simple_irq
> flow we get proper handling of interrupt masking as well as acking
> of interrupts.  The irq_ack method is added to support this.
>
> Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

[PATCH 4/7] gpio: brcmstb: correct the configuration of level interrupts

[Doug Berger]

This commit corrects a bug when configuring the GPIO hardware for
IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH interrupt types. The
hardware is now correctly configured to support those types.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index c186141927bb..0418cb266586 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -137,13 +137,13 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	switch (type) {
 	case IRQ_TYPE_LEVEL_LOW:
-		level = 0;
+		level = mask;
 		edge_config = 0;
 		edge_insensitive = 0;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
 		level = mask;
-		edge_config = 0;
+		edge_config = mask;
 		edge_insensitive = 0;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-- 
2.14.1

Re: [PATCH 4/7] gpio: brcmstb: correct the configuration of level interrupts

[Gregory Fong]

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> This commit corrects a bug when configuring the GPIO hardware for
> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH interrupt types. The
> hardware is now correctly configured to support those types.
>
> Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

[PATCH 5/7] gpio: brcmstb: enable masking of interrupts when changing type

[Doug Berger]

Mask the GPIO interrupt while its type is being changed, just in case
it can prevent a spurious interrupt.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 0418cb266586..e2fff559c1ca 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -363,7 +363,9 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	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;
+	/* and that interrupts are masked when changing their type  */
+	bank->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND |
+			       IRQCHIP_SET_TYPE_MASKED;
 
 	if (IS_ENABLED(CONFIG_PM_SLEEP) && !priv->parent_wake_irq &&
 			of_property_read_bool(np, "wakeup-source")) {
-- 
2.14.1

Re: [PATCH 5/7] gpio: brcmstb: enable masking of interrupts when changing type

[Gregory Fong]

Hi Doug,

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> Mask the GPIO interrupt while its type is being changed, just in case
> it can prevent a spurious interrupt.

"Just in case"?  I don't have access to hardware documentation for
this anymore, but I'd expect to some stronger claim that the hardware
actually requires masking before changing the trigger type.  If you
can quote documentation for this or explain an actual problem seen,
that would be good.

>
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>  drivers/gpio/gpio-brcmstb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index 0418cb266586..e2fff559c1ca 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> @@ -363,7 +363,9 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
>         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;
> +       /* and that interrupts are masked when changing their type  */

This doesn't follow the kernel multi-line comment style, please adjust.

> +       bank->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND |
> +                              IRQCHIP_SET_TYPE_MASKED;
>
>         if (IS_ENABLED(CONFIG_PM_SLEEP) && !priv->parent_wake_irq &&
>                         of_property_read_bool(np, "wakeup-source")) {
> --
> 2.14.1
>

Thanks,
Gregory

Re: [PATCH 5/7] gpio: brcmstb: enable masking of interrupts when changing type

[Doug Berger]

On 10/03/2017 07:10 PM, Gregory Fong wrote:
> Hi Doug,
> 
> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>> Mask the GPIO interrupt while its type is being changed, just in case
>> it can prevent a spurious interrupt.
> 
> "Just in case"?  I don't have access to hardware documentation for
> this anymore, but I'd expect to some stronger claim that the hardware
> actually requires masking before changing the trigger type.  If you
> can quote documentation for this or explain an actual problem seen,
> that would be good.

Well spotted ;).  This was a protectionist change added at the request
of a user of the driver.  I believe that it is superfluous and I suppose
that belief leaked through in the language of my comment.

In actuality, the GPIO APIs don't make provision for clearing GPIO
interrupts before enabling them so this masking is really only a
deferral of a spurious interrupt if one is triggered by changes in the
hardware programming.

I can strike this from the upstream submission and save a couple lines
of source code (after IRQCHIP_MASK_ON_SUSPEND goes away) if you prefer.

> 
>>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>> ---
>>  drivers/gpio/gpio-brcmstb.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index 0418cb266586..e2fff559c1ca 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> @@ -363,7 +363,9 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
>>         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;
>> +       /* and that interrupts are masked when changing their type  */
> 
> This doesn't follow the kernel multi-line comment style, please adjust.
> 
>> +       bank->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND |
>> +                              IRQCHIP_SET_TYPE_MASKED;
>>
>>         if (IS_ENABLED(CONFIG_PM_SLEEP) && !priv->parent_wake_irq &&
>>                         of_property_read_bool(np, "wakeup-source")) {
>> --
>> 2.14.1
>>
> 
> Thanks,
> Gregory
> 

Thanks again,
    Doug

Re: [PATCH 5/7] gpio: brcmstb: enable masking of interrupts when changing type

[Gregory Fong]

On Tue, Oct 3, 2017 at 7:22 PM, Doug Berger <opendmb@gmail.com> wrote:
> On 10/03/2017 07:10 PM, Gregory Fong wrote:
>> Hi Doug,
>>
>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>>> Mask the GPIO interrupt while its type is being changed, just in case
>>> it can prevent a spurious interrupt.
>>
>> "Just in case"?  I don't have access to hardware documentation for
>> this anymore, but I'd expect to some stronger claim that the hardware
>> actually requires masking before changing the trigger type.  If you
>> can quote documentation for this or explain an actual problem seen,
>> that would be good.
>
> Well spotted ;).  This was a protectionist change added at the request
> of a user of the driver.  I believe that it is superfluous and I suppose
> that belief leaked through in the language of my comment.
>
> In actuality, the GPIO APIs don't make provision for clearing GPIO
> interrupts before enabling them so this masking is really only a
> deferral of a spurious interrupt if one is triggered by changes in the
> hardware programming.

Indeed.  If the hardware is behaving in unexpected ways that would be
a whole different kind of problem.

>
> I can strike this from the upstream submission and save a couple lines
> of source code (after IRQCHIP_MASK_ON_SUSPEND goes away) if you prefer.
>

Yes, please remove.

Thanks,
Gregory

[PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

The GPIOLIB IRQ chip helpers were very appealing, but badly broke
the 1:1 mapping between a GPIO controller's device_node and its
interrupt domain.

This commit consolidates the per bank irq domains to a version
where we have one larger interrupt domain per GPIO controller
instance spanning multiple GPIO banks.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/Kconfig        |   2 +-
 drivers/gpio/gpio-brcmstb.c | 188 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 145 insertions(+), 45 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 796b11c489ae..28dd05aaa408 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -139,7 +139,7 @@ config GPIO_BRCMSTB
 	default y if (ARCH_BRCMSTB || BMIPS_GENERIC)
 	depends on OF_GPIO && (ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST)
 	select GPIO_GENERIC
-	select GPIOLIB_IRQCHIP
+	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 e2fff559c1ca..752a46ce3589 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -37,20 +37,22 @@ struct brcmstb_gpio_bank {
 	struct gpio_chip gc;
 	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;
 	struct platform_device *pdev;
+	struct irq_domain *irq_domain;
+	struct irq_chip irq_chip;
 	int parent_irq;
 	int gpio_base;
+	int num_gpios;
 	int parent_wake_irq;
 	struct notifier_block reboot_notifier;
 };
 
-#define MAX_GPIO_PER_BANK           32
+#define MAX_GPIO_PER_BANK       32
 #define GPIO_BANK(gpio)         ((gpio) >> 5)
 /* assumes MAX_GPIO_PER_BANK is a multiple of 2 */
 #define GPIO_BIT(gpio)          ((gpio) & (MAX_GPIO_PER_BANK - 1))
@@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
 	return status;
 }
 
+static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
+					struct brcmstb_gpio_bank *bank)
+{
+	return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
+}
+
 static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
 		unsigned int offset, bool enable)
 {
 	struct gpio_chip *gc = &bank->gc;
 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
-	u32 mask = gc->pin2mask(gc, offset);
+	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));
 	u32 imask;
 	unsigned long flags;
 
@@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
 	spin_unlock_irqrestore(&gc->bgpio_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 void brcmstb_gpio_irq_mask(struct irq_data *d)
@@ -119,7 +138,7 @@ static void brcmstb_gpio_irq_ack(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
-	u32 mask = BIT(d->hwirq);
+	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
 
 	gc->write_reg(priv->reg_base + GIO_STAT(bank->id), mask);
 }
@@ -129,7 +148,7 @@ 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 = gpiochip_get_data(gc);
 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
-	u32 mask = BIT(d->hwirq);
+	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
 	u32 edge_insensitive, iedge_insensitive;
 	u32 edge_config, iedge_config;
 	u32 level, ilevel;
@@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
 static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 {
 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
-	struct irq_domain *irq_domain = bank->gc.irqdomain;
+	struct irq_domain *domain = priv->irq_domain;
+	int hwbase = bank->gc.base - priv->gpio_base;
 	unsigned long status;
+	unsigned int irq;
 
 	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
-		int bit;
-
-		for_each_set_bit(bit, &status, 32) {
-			if (bit >= bank->width)
+		for_each_set_bit(irq, &status, 32) {
+			if (irq >= bank->width)
 				dev_warn(&priv->pdev->dev,
 					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
-					 bank->id, bit);
-			generic_handle_irq(irq_find_mapping(irq_domain, bit));
+					 bank->id, irq);
+			irq = irq_linear_revmap(domain, irq + hwbase);
+			generic_handle_irq(irq);
 		}
 	}
 }
@@ -245,8 +265,7 @@ static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 /* Each UPG GIO block has one IRQ for all banks */
 static void brcmstb_gpio_irq_handler(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 brcmstb_gpio_priv *priv = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	struct brcmstb_gpio_bank *bank;
 
@@ -272,6 +291,63 @@ static int brcmstb_gpio_reboot(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static struct brcmstb_gpio_bank *brcmstb_gpio_hwirq_to_bank(
+		struct brcmstb_gpio_priv *priv, irq_hw_number_t hwirq)
+{
+	struct brcmstb_gpio_bank *bank;
+	int i = 0;
+
+	/* banks are in descending order */
+	list_for_each_entry_reverse(bank, &priv->bank_list, node) {
+		i += bank->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->gc);
+	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_level_irq);
+	irq_set_noprobe(irq);
+	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 const 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)
@@ -293,13 +369,25 @@ static int brcmstb_gpio_remove(struct platform_device *pdev)
 {
 	struct brcmstb_gpio_priv *priv = platform_get_drvdata(pdev);
 	struct brcmstb_gpio_bank *bank;
-	int ret = 0;
+	int offset, ret = 0, virq;
 
 	if (!priv) {
 		dev_err(&pdev->dev, "called %s without drvdata!\n", __func__);
 		return -EFAULT;
 	}
 
+	if (priv->parent_irq > 0)
+		irq_set_chained_handler_and_data(priv->parent_irq, NULL, NULL);
+
+	/* Remove all IRQ mappings and delete the domain */
+	if (priv->irq_domain) {
+		for (offset = 0; offset < priv->num_gpios; offset++) {
+			virq = irq_find_mapping(priv->irq_domain, offset);
+			irq_dispose_mapping(virq);
+		}
+		irq_domain_remove(priv->irq_domain);
+	}
+
 	/*
 	 * You can lose return values below, but we report all errors, and it's
 	 * more important to actually perform all of the steps.
@@ -347,28 +435,24 @@ static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
 	return offset;
 }
 
-/* Before calling, must have bank->parent_irq set and gpiochip registered */
+/* priv->parent_irq and priv->num_gpios must be set before calling */
 static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
-		struct brcmstb_gpio_bank *bank)
+		struct brcmstb_gpio_priv *priv)
 {
-	struct brcmstb_gpio_priv *priv = bank->parent_priv;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	int err;
 
-	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_ack = brcmstb_gpio_irq_ack;
-	bank->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
-
-	/* Ensures that all non-wakeup IRQs are disabled at suspend */
-	/* and that interrupts are masked when changing their type  */
-	bank->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND |
-			       IRQCHIP_SET_TYPE_MASKED;
+	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");
+		return -ENXIO;
+	}
 
-	if (IS_ENABLED(CONFIG_PM_SLEEP) && !priv->parent_wake_irq &&
-			of_property_read_bool(np, "wakeup-source")) {
+	if (of_property_read_bool(np, "wakeup-source")) {
 		priv->parent_wake_irq = platform_get_irq(pdev, 1);
 		if (priv->parent_wake_irq < 0) {
 			priv->parent_wake_irq = 0;
@@ -389,7 +473,7 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 
 			if (err < 0) {
 				dev_err(dev, "Couldn't request wake IRQ");
-				return err;
+				goto out_free_domain;
 			}
 
 			priv->reboot_notifier.notifier_call =
@@ -398,17 +482,30 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 		}
 	}
 
+	priv->irq_chip.name = dev_name(dev);
+	priv->irq_chip.irq_disable = brcmstb_gpio_irq_mask;
+	priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
+	priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
+	priv->irq_chip.irq_ack = brcmstb_gpio_irq_ack;
+	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
+
+	/* Ensures that all non-wakeup IRQs are disabled at suspend */
+	/* and that interrupts are masked when changing their type  */
+	priv->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND |
+			       IRQCHIP_SET_TYPE_MASKED;
+
 	if (priv->parent_wake_irq)
-		bank->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
+		priv->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
 
-	err = gpiochip_irqchip_add(&bank->gc, &bank->irq_chip, 0,
-				   handle_level_irq, IRQ_TYPE_NONE);
-	if (err)
-		return err;
-	gpiochip_set_chained_irqchip(&bank->gc, &bank->irq_chip,
-			priv->parent_irq, brcmstb_gpio_irq_handler);
+	irq_set_chained_handler_and_data(priv->parent_irq,
+					 brcmstb_gpio_irq_handler, priv);
 
 	return 0;
+
+out_free_domain:
+	irq_domain_remove(priv->irq_domain);
+
+	return err;
 }
 
 static int brcmstb_gpio_probe(struct platform_device *pdev)
@@ -513,6 +610,8 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		gc->of_xlate = brcmstb_gpio_of_xlate;
 		/* not all ngpio lines are valid, will use bank width later */
 		gc->ngpio = MAX_GPIO_PER_BANK;
+		if (priv->parent_irq > 0)
+			gc->to_irq = brcmstb_gpio_to_irq;
 
 		/*
 		 * Mask all interrupts by default, since wakeup interrupts may
@@ -528,12 +627,6 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		}
 		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);
 
@@ -543,6 +636,13 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		num_banks++;
 	}
 
+	priv->num_gpios = gpio_base - priv->gpio_base;
+	if (priv->parent_irq > 0) {
+		err = brcmstb_gpio_irq_setup(pdev, priv);
+		if (err)
+			goto fail;
+	}
+
 	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
 			num_banks, priv->gpio_base, gpio_base - 1);
 
-- 
2.14.1

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Gregory Fong]

Hi Doug,

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
> the 1:1 mapping between a GPIO controller's device_node and its
> interrupt domain.

Out of curiosity, what sort of problems have you seen from this?

>
> This commit consolidates the per bank irq domains to a version
> where we have one larger interrupt domain per GPIO controller
> instance spanning multiple GPIO banks.

This works (and is reminiscent to my initially submitted
implementation at [1]), but I think it might make sense to keep as-is
(using the gpiolib irqchip helpers), and instead allocate an irqchip
fwnode per bank and use to_of_node() to set it as the of_node for the
gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
might go away...

Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
says "get rid of this and use gpiochip->parent->of_node everywhere"?
It seems like it would still be beneficial to be able to override the
associated node for a gpiochip, since that's what's used for the
irqdomain, but if that's going away, obviously we don't want to start
using that now.

Thanks,
Gregory

[1] https://patchwork.kernel.org/patch/6347811/

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/03/2017 08:03 PM, Gregory Fong wrote:
> Hi Doug,
> 
> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>> the 1:1 mapping between a GPIO controller's device_node and its
>> interrupt domain.
> 
> Out of curiosity, what sort of problems have you seen from this?
> 

As you know, the BRCMSTB devices conceptually distinguish between an
always-on GPIO device and a regular GPIO device that each can have many
more than 32 General Purpose I/Os. The driver supports these by dividing
the GPIO across a number of banks each of which is implemented as a
separate gpiochip as an implementation convenience. The main issue is
that each gpiochip that uses the GPIOLIB IRQ chip helpers creates its
own irq domain even though they are associated with the same device and
device-tree node.

When another device-tree node references a GPIO device as its interrupt
parent, the irq_create_of_mapping() function looks for the irq domain of
the GPIO device and since all bank irq domains reference the same GPIO
device node it always resolves to the irq domain of the first bank
regardless of which bank the number of the GPIO should resolve. This
domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
can't be mapped by the device-tree.

>>
>> This commit consolidates the per bank irq domains to a version
>> where we have one larger interrupt domain per GPIO controller
>> instance spanning multiple GPIO banks.
> 
> This works (and is reminiscent to my initially submitted
> implementation at [1]), but I think it might make sense to keep as-is
> (using the gpiolib irqchip helpers), and instead allocate an irqchip
> fwnode per bank and use to_of_node() to set it as the of_node for the
> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
> might go away...
> 
> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
> says "get rid of this and use gpiochip->parent->of_node everywhere"?
> It seems like it would still be beneficial to be able to override the
> associated node for a gpiochip, since that's what's used for the
> irqdomain, but if that's going away, obviously we don't want to start
> using that now.
> 

Yes, this is effectively a reversion to an earlier implementation. I
produced an implementation based on the generic irqchip libraries, but
that was stripped from this submission when I discovered that no support
exists within the generic irqchip libraries for removal of domain
generic chips and we wanted to preserve the module support of this driver.

It is conceivable that the current GPIO device-tree nodes could be
broken down into separate devices per bank, but it is believed that this
would only confuse things for users of the device as the concept
diverges from the concept expressed in device documentation.

> Thanks,
> Gregory
> 
> [1] https://patchwork.kernel.org/patch/6347811/
> 

Thanks for the review,
    Doug

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/04/2017 02:24 PM, Doug Berger wrote:
> On 10/03/2017 08:03 PM, Gregory Fong wrote:
>> Hi Doug,
>>
>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>>> the 1:1 mapping between a GPIO controller's device_node and its
>>> interrupt domain.
>>
>> Out of curiosity, what sort of problems have you seen from this?
>>
> 
> As you know, the BRCMSTB devices conceptually distinguish between an
> always-on GPIO device and a regular GPIO device that each can have many
> more than 32 General Purpose I/Os. The driver supports these by dividing
> the GPIO across a number of banks each of which is implemented as a
> separate gpiochip as an implementation convenience. The main issue is
> that each gpiochip that uses the GPIOLIB IRQ chip helpers creates its
> own irq domain even though they are associated with the same device and
> device-tree node.
> 
> When another device-tree node references a GPIO device as its interrupt
> parent, the irq_create_of_mapping() function looks for the irq domain of
> the GPIO device and since all bank irq domains reference the same GPIO
> device node it always resolves to the irq domain of the first bank
> regardless of which bank the number of the GPIO should resolve. This
> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
> can't be mapped by the device-tree.
> 
>>>
>>> This commit consolidates the per bank irq domains to a version
>>> where we have one larger interrupt domain per GPIO controller
>>> instance spanning multiple GPIO banks.
>>
>> This works (and is reminiscent to my initially submitted
>> implementation at [1]), but I think it might make sense to keep as-is
>> (using the gpiolib irqchip helpers), and instead allocate an irqchip
>> fwnode per bank and use to_of_node() to set it as the of_node for the
>> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
>> might go away...
>>
>> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
>> says "get rid of this and use gpiochip->parent->of_node everywhere"?
>> It seems like it would still be beneficial to be able to override the
>> associated node for a gpiochip, since that's what's used for the
>> irqdomain, but if that's going away, obviously we don't want to start
>> using that now.
>>
> 
> Yes, this is effectively a reversion to an earlier implementation. I
> produced an implementation based on the generic irqchip libraries, but
> that was stripped from this submission when I discovered that no support
> exists within the generic irqchip libraries for removal of domain
> generic chips and we wanted to preserve the module support of this driver.
> 
> It is conceivable that the current GPIO device-tree nodes could be
> broken down into separate devices per bank, but it is believed that this
> would only confuse things for users of the device as the concept
> diverges from the concept expressed in device documentation.
> 
>> Thanks,
>> Gregory
>>
>> [1] https://patchwork.kernel.org/patch/6347811/
>>
> 
> Thanks for the review,
>     Doug
> 

Gregory,

Do you have any additional feedback on patches 6 and 7 before I submit a
version 2?

Thanks,
    Doug

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Gregory Fong]

Hi Doug,

On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:
> On 10/03/2017 08:03 PM, Gregory Fong wrote:
> > On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
> >> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
> >> the 1:1 mapping between a GPIO controller's device_node and its
> >> interrupt domain.
> > 
> > Out of curiosity, what sort of problems have you seen from this?
> > 
> 
> [snip]
> 
> When another device-tree node references a GPIO device as its interrupt
> parent, the irq_create_of_mapping() function looks for the irq domain of
> the GPIO device and since all bank irq domains reference the same GPIO
> device node it always resolves to the irq domain of the first bank
> regardless of which bank the number of the GPIO should resolve. This
> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
> can't be mapped by the device-tree.

Thanks for clarifying.  This would be great information to include in
the commit message.

> 
> >>
> >> This commit consolidates the per bank irq domains to a version
> >> where we have one larger interrupt domain per GPIO controller
> >> instance spanning multiple GPIO banks.
> > 
> > This works (and is reminiscent to my initially submitted
> > implementation at [1]), but I think it might make sense to keep as-is
> > (using the gpiolib irqchip helpers), and instead allocate an irqchip
> > fwnode per bank and use to_of_node() to set it as the of_node for the
> > gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
> > might go away...
> > 
> > Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
> > says "get rid of this and use gpiochip->parent->of_node everywhere"?
> > It seems like it would still be beneficial to be able to override the
> > associated node for a gpiochip, since that's what's used for the
> > irqdomain, but if that's going away, obviously we don't want to start
> > using that now.
> > 
> 
> Yes, this is effectively a reversion to an earlier implementation. I
> produced an implementation based on the generic irqchip libraries, but
> that was stripped from this submission when I discovered that no support
> exists within the generic irqchip libraries for removal of domain
> generic chips and we wanted to preserve the module support of this driver.

Considering this is heavily based on my initial implementation (several
functions are exactly the same), it'd be nice to have some small
attribution in the commit message. :-)

> 
> It is conceivable that the current GPIO device-tree nodes could be
> broken down into separate devices per bank, but it is believed that this
> would only confuse things for users of the device as the concept
> diverges from the concept expressed in device documentation.

OK, that sounds like a worse alternative.  And since these are all
actually using the same parent IRQ, it does make sense to keep them all
in the same IRQ domain.


On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:
> [snip]
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index e2fff559c1ca..752a46ce3589 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> [snip]
> @@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>  	return status;
>  }
>  
> +static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
> +					struct brcmstb_gpio_bank *bank)
> +{
> +	return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
> +}
> +
>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>  		unsigned int offset, bool enable)
>  {
>  	struct gpio_chip *gc = &bank->gc;
>  	struct brcmstb_gpio_priv *priv = bank->parent_priv;
> -	u32 mask = gc->pin2mask(gc, offset);
> +	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));

Consider renaming "offset" to "hwirq".

>  	u32 imask;
>  	unsigned long flags;
>  
> @@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>  	spin_unlock_irqrestore(&gc->bgpio_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);

Consider renaming "gc_offset" to "offset" and "offset" to "hwirq" to
keep things consistent.

> +
> +	if (offset >= priv->num_gpios)
> +		return -ENXIO;
> +	return irq_create_mapping(priv->irq_domain, offset);
> +}
> +
>  
> [snip]
>
> @@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
>  static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
>  {
>  	struct brcmstb_gpio_priv *priv = bank->parent_priv;
> -	struct irq_domain *irq_domain = bank->gc.irqdomain;
> +	struct irq_domain *domain = priv->irq_domain;
> +	int hwbase = bank->gc.base - priv->gpio_base;
>  	unsigned long status;
> +	unsigned int irq;
>  
>  	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
> -		int bit;
> -
> -		for_each_set_bit(bit, &status, 32) {
> -			if (bit >= bank->width)
> +		for_each_set_bit(irq, &status, 32) {
> +			if (irq >= bank->width)
>  				dev_warn(&priv->pdev->dev,
>  					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
> -					 bank->id, bit);
> -			generic_handle_irq(irq_find_mapping(irq_domain, bit));
> +					 bank->id, irq);
> +			irq = irq_linear_revmap(domain, irq + hwbase);
> +			generic_handle_irq(irq);

I'm not a fan of the change to use "irq" to mean several different
things in this function.  How about this instead?

static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
{
	struct brcmstb_gpio_priv *priv = bank->parent_priv;
	struct irq_domain *domain = priv->irq_domain;
	int hwbase = bank->gc.base - priv->gpio_base;
	unsigned long status;

	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
		int offset;

		for_each_set_bit(offset, &status, 32) {
			if (offset >= bank->width)
 				dev_warn(&priv->pdev->dev,
 					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
					 bank->id, offset);
			generic_handle_irq(irq_linear_revmap(domain, offset + hwbase));


>  		}
>  	}
>  }
> [snip] 
> 

The rest looks good.

Thanks,
Gregory

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/19/2017 12:57 AM, Gregory Fong wrote:
> Hi Doug,
> 
> On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:
>> On 10/03/2017 08:03 PM, Gregory Fong wrote:
>>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger <opendmb@gmail.com> wrote:
>>>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>>>> the 1:1 mapping between a GPIO controller's device_node and its
>>>> interrupt domain.
>>>
>>> Out of curiosity, what sort of problems have you seen from this?
>>>
>>
>> [snip]
>>
>> When another device-tree node references a GPIO device as its interrupt
>> parent, the irq_create_of_mapping() function looks for the irq domain of
>> the GPIO device and since all bank irq domains reference the same GPIO
>> device node it always resolves to the irq domain of the first bank
>> regardless of which bank the number of the GPIO should resolve. This
>> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
>> can't be mapped by the device-tree.
> 
> Thanks for clarifying.  This would be great information to include in
> the commit message.
>

Will do.

>>
>>>>
>>>> This commit consolidates the per bank irq domains to a version
>>>> where we have one larger interrupt domain per GPIO controller
>>>> instance spanning multiple GPIO banks.
>>>
>>> This works (and is reminiscent to my initially submitted
>>> implementation at [1]), but I think it might make sense to keep as-is
>>> (using the gpiolib irqchip helpers), and instead allocate an irqchip
>>> fwnode per bank and use to_of_node() to set it as the of_node for the
>>> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
>>> might go away...
>>>
>>> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
>>> says "get rid of this and use gpiochip->parent->of_node everywhere"?
>>> It seems like it would still be beneficial to be able to override the
>>> associated node for a gpiochip, since that's what's used for the
>>> irqdomain, but if that's going away, obviously we don't want to start
>>> using that now.
>>>
>>
>> Yes, this is effectively a reversion to an earlier implementation. I
>> produced an implementation based on the generic irqchip libraries, but
>> that was stripped from this submission when I discovered that no support
>> exists within the generic irqchip libraries for removal of domain
>> generic chips and we wanted to preserve the module support of this driver.
> 
> Considering this is heavily based on my initial implementation (several
> functions are exactly the same), it'd be nice to have some small
> attribution in the commit message. :-)
> 

Yes, I suppose I should have said "a reversion to your earlier
implementation" above :).  I'd be happy to to add your Signed-of-by if
you would like, but you'll have to let me know which email address to
use (the one in the original downstream submission or this one).

>>
>> It is conceivable that the current GPIO device-tree nodes could be
>> broken down into separate devices per bank, but it is believed that this
>> would only confuse things for users of the device as the concept
>> diverges from the concept expressed in device documentation.
> 
> OK, that sounds like a worse alternative.  And since these are all
> actually using the same parent IRQ, it does make sense to keep them all
> in the same IRQ domain.
> 
> 
> On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:
>> [snip]
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index e2fff559c1ca..752a46ce3589 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> [snip]
>> @@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>>  	return status;
>>  }
>>  
>> +static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
>> +					struct brcmstb_gpio_bank *bank)
>> +{
>> +	return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
>> +}
>> +
>>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>>  		unsigned int offset, bool enable)
>>  {
>>  	struct gpio_chip *gc = &bank->gc;
>>  	struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> -	u32 mask = gc->pin2mask(gc, offset);
>> +	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));
> 
> Consider renaming "offset" to "hwirq".
> 

I could do that, but I was just using the existing argument so now you
are editing yourself ;).  I'll think about it.

>>  	u32 imask;
>>  	unsigned long flags;
>>  
>> @@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>>  	spin_unlock_irqrestore(&gc->bgpio_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);
> 
> Consider renaming "gc_offset" to "offset" and "offset" to "hwirq" to
> keep things consistent.
> 

Sounds better.  I'll consider that too.

>> +
>> +	if (offset >= priv->num_gpios)
>> +		return -ENXIO;
>> +	return irq_create_mapping(priv->irq_domain, offset);
>> +}
>> +
>>  
>> [snip]
>>
>> @@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
>>  static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
>>  {
>>  	struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> -	struct irq_domain *irq_domain = bank->gc.irqdomain;
>> +	struct irq_domain *domain = priv->irq_domain;
>> +	int hwbase = bank->gc.base - priv->gpio_base;
>>  	unsigned long status;
>> +	unsigned int irq;
>>  
>>  	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
>> -		int bit;
>> -
>> -		for_each_set_bit(bit, &status, 32) {
>> -			if (bit >= bank->width)
>> +		for_each_set_bit(irq, &status, 32) {
>> +			if (irq >= bank->width)
>>  				dev_warn(&priv->pdev->dev,
>>  					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
>> -					 bank->id, bit);
>> -			generic_handle_irq(irq_find_mapping(irq_domain, bit));
>> +					 bank->id, irq);
>> +			irq = irq_linear_revmap(domain, irq + hwbase);
>> +			generic_handle_irq(irq);
> 
> I'm not a fan of the change to use "irq" to mean several different
> things in this function.  How about this instead?
> 
> static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
> {
> 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
> 	struct irq_domain *domain = priv->irq_domain;
> 	int hwbase = bank->gc.base - priv->gpio_base;
> 	unsigned long status;
> 
> 	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
> 		int offset;
> 
> 		for_each_set_bit(offset, &status, 32) {
> 			if (offset >= bank->width)
>  				dev_warn(&priv->pdev->dev,
>  					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
> 					 bank->id, offset);
> 			generic_handle_irq(irq_linear_revmap(domain, offset + hwbase));
> 
> 

That is more or less where I started, but the "ugliness" began creeping
in with the need to wrap that last line at 80 characters. It seemed
silly to declare two separate ints for transitory purposes just to make
the naming clearer, but I'll revisit it too.

>>  		}
>>  	}
>>  }
>> [snip] 
>>
> 
> The rest looks good.
> 
> Thanks,
> Gregory
> 

Thanks for taking the time and the thoughtful feedback,
    Doug

[PATCH 7/7] gpio: brcmstb: implement suspend/resume/shutdown

[Doug Berger]

This commit corrects problems with the previous wake implementation
by implementing suspend and resume power management operations and
the driver shutdown operation.

Wake masks are used to keep track of which GPIO should wake the
device.  On suspend the GPIO state is saved and the possible wakeup
sources are explicitly unmasked in the hardware. Non-wakeup sources
are explicitly masked so IRQCHIP_MASK_ON_SUSPEND is no longer
necessary.  The saved state of the GPIO is restored upon resume.
It is important not to write to the GPIO status register since this
has the effect of clearing bits.  The status register is explicitly
removed from the register save and restore to ensure this.

The shutdown operation allows the hardware to be put into the same
quiesced state as the suspend operation and removes the need for
the reboot notifier.

Unfortunately, there appears to be some confusion about whether
a pending disabled wake interrupt should wake the system. If a wake
capable interrupt is disabled using the default "lazy disable"
behavior and it is triggered before the suspend_device_irq call
the interrupt hardware will be acknowledged by mask_ack_irq and the
IRQS_PENDING flag is added to its state. However, the IRQS_PENDING
flag of wake interrupts is not checked to prevent the transition to
suspend and the hardware has been acked which prevents its wakeup.
If the lazy disabled interrupt is triggered after the call to
suspend_device_irqs then the wakeup logic will abort the suspend.
The irq_disable method is defined by this GPIO driver to prevent
lazy disable so that the pending hardware state remains asserted
allowing the hardware to wake and providing a consistent behavior.

In addition, the IRQ_DISABLE_UNLAZY flag is set for the non-wake
parent interrupt as a convenience to prevent the need to add code
to the brcmstb_gpio_irq_handler to support "lazy disable" of the
non-wake parent interrupt when it is disabled during suspend and
resume. Chained interrupt parents are not normally disabled, but
these GPIO devices have different parent interrupts for wake and
non-wake handling. It is convenient to mask the non-wake parent
when suspending to preserve the hardware state for proper wakeup
accounting when the driver is resumed.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 200 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 151 insertions(+), 49 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 752a46ce3589..c964ed71a68d 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -19,17 +19,29 @@
 #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)
-#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)
+
+enum gio_reg_index {
+	GIO_REG_ODEN = 0,
+	GIO_REG_DATA,
+	GIO_REG_IODIR,
+	GIO_REG_EC,
+	GIO_REG_EI,
+	GIO_REG_MASK,
+	GIO_REG_LEVEL,
+	GIO_REG_STAT,
+	GIO_REG_COUNT
+};
+
+#define GIO_BANK_SIZE           (GIO_REG_COUNT * sizeof(u32))
+#define GIO_BANK_OFF(bank, off)	(((bank) * GIO_BANK_SIZE) + (off * sizeof(u32)))
+#define GIO_ODEN(bank)          GIO_BANK_OFF(bank, GIO_REG_ODEN)
+#define GIO_DATA(bank)          GIO_BANK_OFF(bank, GIO_REG_DATA)
+#define GIO_IODIR(bank)         GIO_BANK_OFF(bank, GIO_REG_IODIR)
+#define GIO_EC(bank)            GIO_BANK_OFF(bank, GIO_REG_EC)
+#define GIO_EI(bank)            GIO_BANK_OFF(bank, GIO_REG_EI)
+#define GIO_MASK(bank)          GIO_BANK_OFF(bank, GIO_REG_MASK)
+#define GIO_LEVEL(bank)         GIO_BANK_OFF(bank, GIO_REG_LEVEL)
+#define GIO_STAT(bank)          GIO_BANK_OFF(bank, GIO_REG_STAT)
 
 struct brcmstb_gpio_bank {
 	struct list_head node;
@@ -37,6 +49,8 @@ struct brcmstb_gpio_bank {
 	struct gpio_chip gc;
 	struct brcmstb_gpio_priv *parent_priv;
 	u32 width;
+	u32 wake_active;
+	u32 regs[GIO_REG_STAT]; /* Don't save and restore GIO_REG_STAT */
 };
 
 struct brcmstb_gpio_priv {
@@ -49,7 +63,6 @@ struct brcmstb_gpio_priv {
 	int gpio_base;
 	int num_gpios;
 	int parent_wake_irq;
-	struct notifier_block reboot_notifier;
 };
 
 #define MAX_GPIO_PER_BANK       32
@@ -65,15 +78,22 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
 }
 
 static unsigned long
-brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
+__brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
 {
 	void __iomem *reg_base = bank->parent_priv->reg_base;
+
+	return bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
+	       bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
+}
+
+static unsigned long
+brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
+{
 	unsigned long status;
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
-	status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
-		 bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
+	status = __brcmstb_gpio_get_active_irqs(bank);
 	spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
 
 	return status;
@@ -209,11 +229,6 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
 {
 	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
@@ -227,7 +242,17 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
 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);
+	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
+
+	/* Do not do anything specific for now, suspend/resume callbacks will
+	 * configure the interrupt mask appropriately
+	 */
+	if (enable)
+		bank->wake_active |= mask;
+	else
+		bank->wake_active &= ~mask;
 
 	return brcmstb_gpio_priv_set_wake(priv, enable);
 }
@@ -238,7 +263,8 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
 
 	if (!priv || irq != priv->parent_wake_irq)
 		return IRQ_NONE;
-	pm_wakeup_event(&priv->pdev->dev, 0);
+
+	/* Nothing to do */
 	return IRQ_HANDLED;
 }
 
@@ -278,19 +304,6 @@ static void brcmstb_gpio_irq_handler(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_priv_set_wake(priv, 1);
-
-	return NOTIFY_DONE;
-}
-
 static struct brcmstb_gpio_bank *brcmstb_gpio_hwirq_to_bank(
 		struct brcmstb_gpio_priv *priv, irq_hw_number_t hwirq)
 {
@@ -395,12 +408,6 @@ static int brcmstb_gpio_remove(struct platform_device *pdev)
 	list_for_each_entry(bank, &priv->bank_list, node)
 		gpiochip_remove(&bank->gc);
 
-	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;
 }
 
@@ -460,9 +467,8 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
 		} else {
 			/*
-			 * Set wakeup capability before requesting wakeup
-			 * interrupt, so we can process boot-time "wakeups"
-			 * (e.g., from S5 cold boot)
+			 * Set wakeup capability so we can process boot-time
+			 * "wakeups" (e.g., from S5 cold boot)
 			 */
 			device_set_wakeup_capable(dev, true);
 			device_wakeup_enable(dev);
@@ -475,10 +481,6 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 				dev_err(dev, "Couldn't request wake IRQ");
 				goto out_free_domain;
 			}
-
-			priv->reboot_notifier.notifier_call =
-				brcmstb_gpio_reboot;
-			register_reboot_notifier(&priv->reboot_notifier);
 		}
 	}
 
@@ -499,6 +501,7 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 
 	irq_set_chained_handler_and_data(priv->parent_irq,
 					 brcmstb_gpio_irq_handler, priv);
+	irq_set_status_flags(priv->parent_irq, IRQ_DISABLE_UNLAZY);
 
 	return 0;
 
@@ -508,6 +511,99 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	return err;
 }
 
+static void brcmstb_gpio_bank_save(struct brcmstb_gpio_priv *priv,
+				   struct brcmstb_gpio_bank *bank)
+{
+	struct gpio_chip *gc = &bank->gc;
+	unsigned int i;
+
+	for (i = 0; i < GIO_REG_STAT; i++)
+		bank->regs[i] = gc->read_reg(priv->reg_base +
+					     GIO_BANK_OFF(bank->id, i));
+}
+
+static void brcmstb_gpio_quiesce(struct device *dev, bool save)
+{
+	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
+	struct brcmstb_gpio_bank *bank;
+	struct gpio_chip *gc;
+	u32 imask;
+
+	/* disable non-wake interrupt */
+	if (priv->parent_irq >= 0)
+		disable_irq(priv->parent_irq);
+
+	list_for_each_entry(bank, &priv->bank_list, node) {
+		gc = &bank->gc;
+
+		if (save)
+			brcmstb_gpio_bank_save(priv, bank);
+
+		/* Unmask GPIOs which have been flagged as wake-up sources */
+		if (priv->parent_wake_irq)
+			imask = bank->wake_active;
+		else
+			imask = 0;
+		gc->write_reg(priv->reg_base + GIO_MASK(bank->id),
+			       imask);
+	}
+}
+
+static void brcmstb_gpio_shutdown(struct platform_device *pdev)
+{
+	/* Enable GPIO for S5 cold boot */
+	brcmstb_gpio_quiesce(&pdev->dev, 0);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static void brcmstb_gpio_bank_restore(struct brcmstb_gpio_priv *priv,
+				      struct brcmstb_gpio_bank *bank)
+{
+	struct gpio_chip *gc = &bank->gc;
+	unsigned int i;
+
+	for (i = 0; i < GIO_REG_STAT; i++)
+		gc->write_reg(priv->reg_base + GIO_BANK_OFF(bank->id, i),
+			      bank->regs[i]);
+}
+
+static int brcmstb_gpio_suspend(struct device *dev)
+{
+	brcmstb_gpio_quiesce(dev, 1);
+	return 0;
+}
+
+static int brcmstb_gpio_resume(struct device *dev)
+{
+	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
+	struct brcmstb_gpio_bank *bank;
+	u32 wake_mask = 0;
+
+	list_for_each_entry(bank, &priv->bank_list, node) {
+		wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
+		brcmstb_gpio_bank_restore(priv, bank);
+	}
+
+	if (priv->parent_wake_irq && wake_mask)
+		pm_wakeup_event(dev, 0);
+
+	/* enable non-wake interrupt */
+	if (priv->parent_irq >= 0)
+		enable_irq(priv->parent_irq);
+
+	return 0;
+}
+
+#else
+#define brcmstb_gpio_suspend	NULL
+#define brcmstb_gpio_resume	NULL
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
+	.suspend_noirq	= brcmstb_gpio_suspend,
+	.resume_noirq = brcmstb_gpio_resume,
+};
+
 static int brcmstb_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -517,7 +613,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct property *prop;
 	const __be32 *p;
-	u32 bank_width;
+	u32 bank_width, wake_mask = 0;
 	int num_banks = 0;
 	int err;
 	static int gpio_base;
@@ -617,6 +713,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		 * Mask all interrupts by default, since wakeup interrupts may
 		 * be retained from S5 cold boot
 		 */
+		wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
 		gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
 
 		err = gpiochip_add_data(gc, bank);
@@ -646,6 +743,9 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
 			num_banks, priv->gpio_base, gpio_base - 1);
 
+	if (priv->parent_wake_irq && wake_mask)
+		pm_wakeup_event(dev, 0);
+
 	return 0;
 
 fail:
@@ -664,9 +764,11 @@ static struct platform_driver brcmstb_gpio_driver = {
 	.driver = {
 		.name = "brcmstb-gpio",
 		.of_match_table = brcmstb_gpio_of_match,
+		.pm = &brcmstb_gpio_pm_ops,
 	},
 	.probe = brcmstb_gpio_probe,
 	.remove = brcmstb_gpio_remove,
+	.shutdown = brcmstb_gpio_shutdown,
 };
 module_platform_driver(brcmstb_gpio_driver);
 
-- 
2.14.1

Re: [PATCH 7/7] gpio: brcmstb: implement suspend/resume/shutdown

[Gregory Fong]

Hi Doug,

Nice description of the problem with dealing with a pending disabled
wake interrupt and the solution.  A few remarks:

On Fri, Sep 29, 2017 at 08:40:57PM -0700, Doug Berger wrote:
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index 752a46ce3589..c964ed71a68d 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> @@ -19,17 +19,29 @@
>  #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)
> -#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)
> +
> +enum gio_reg_index {
> +	GIO_REG_ODEN = 0,
> +	GIO_REG_DATA,
> +	GIO_REG_IODIR,
> +	GIO_REG_EC,
> +	GIO_REG_EI,
> +	GIO_REG_MASK,
> +	GIO_REG_LEVEL,
> +	GIO_REG_STAT,
> +	GIO_REG_COUNT

Please change the name or add a comment to make it clear that this is
not for an actual register.

> +};
> +
> [snip]
> @@ -37,6 +49,8 @@ struct brcmstb_gpio_bank {
>  	struct gpio_chip gc;
>  	struct brcmstb_gpio_priv *parent_priv;
>  	u32 width;
> +	u32 wake_active;
> +	u32 regs[GIO_REG_STAT]; /* Don't save and restore GIO_REG_STAT */

Consider naming to make it clearer that this is for save/restore, not
some kind of shadow reg implementation or anything like that.

>  };
>  
>  struct brcmstb_gpio_priv {
> @@ -49,7 +63,6 @@ struct brcmstb_gpio_priv {
>  	int gpio_base;
>  	int num_gpios;
>  	int parent_wake_irq;
> -	struct notifier_block reboot_notifier;
>  };
>  
>  #define MAX_GPIO_PER_BANK       32
> @@ -65,15 +78,22 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
>  }
>  
>  static unsigned long
> -brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> +__brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>  {
>  	void __iomem *reg_base = bank->parent_priv->reg_base;
> +
> +	return bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
> +	       bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
> +}
> +
> +static unsigned long
> +brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> +{
>  	unsigned long status;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
> -	status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
> -		 bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
> +	status = __brcmstb_gpio_get_active_irqs(bank);
>  	spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
>  
>  	return status;
> @@ -209,11 +229,6 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
>  {
>  	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
> @@ -227,7 +242,17 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
>  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);
> +	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
> +	struct brcmstb_gpio_priv *priv = bank->parent_priv;
> +	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
> +
> +	/* Do not do anything specific for now, suspend/resume callbacks will
> +	 * configure the interrupt mask appropriately
> +	 */

Please fix comment format (this is the network subsystem style).

> 
> [snip] 
> @@ -508,6 +511,99 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
> [...]
> +static void brcmstb_gpio_shutdown(struct platform_device *pdev)
> +{
> +	/* Enable GPIO for S5 cold boot */
> +	brcmstb_gpio_quiesce(&pdev->dev, 0);

nit: use false instead of 0 (it's a bool).

> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> [snip]
> +static int brcmstb_gpio_resume(struct device *dev)
> +{
> +	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
> +	struct brcmstb_gpio_bank *bank;
> +	u32 wake_mask = 0;

This isn't really being used as a mask, contrary to appearances.  It's
just tracking whether any active IRQs were seen.  Please change to use a
bool instead and adjust the name accordingly.

> +
> +	list_for_each_entry(bank, &priv->bank_list, node) {
> +		wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
> +		brcmstb_gpio_bank_restore(priv, bank);
> +	}
> +
> +	if (priv->parent_wake_irq && wake_mask)
> +		pm_wakeup_event(dev, 0);
> +
> +	/* enable non-wake interrupt */
> +	if (priv->parent_irq >= 0)
> +		enable_irq(priv->parent_irq);
> +
> +	return 0;
> +}
> +
> +#else
> +#define brcmstb_gpio_suspend	NULL
> +#define brcmstb_gpio_resume	NULL
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> +	.suspend_noirq	= brcmstb_gpio_suspend,
> +	.resume_noirq = brcmstb_gpio_resume,
> +};
> +
>  static int brcmstb_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -517,7 +613,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct property *prop;
>  	const __be32 *p;
> -	u32 bank_width;
> +	u32 bank_width, wake_mask = 0;

Same comment on wake_mask as for brcmstb_gpio_resume() above.

>  	int num_banks = 0;
>  	int err;
>  	static int gpio_base;
> @@ -617,6 +713,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>  		 * Mask all interrupts by default, since wakeup interrupts may
>  		 * be retained from S5 cold boot
>  		 */
> +		wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
>  		gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
>  
>  		err = gpiochip_add_data(gc, bank);
> @@ -646,6 +743,9 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>  	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
>  			num_banks, priv->gpio_base, gpio_base - 1);
>  
> +	if (priv->parent_wake_irq && wake_mask)
> +		pm_wakeup_event(dev, 0);

Why is this called in probe?

Thanks,
Gregory

Re: [PATCH 7/7] gpio: brcmstb: implement suspend/resume/shutdown

[Doug Berger]

On 10/19/2017 02:03 AM, Gregory Fong wrote:
> Hi Doug,
> 
> Nice description of the problem with dealing with a pending disabled
> wake interrupt and the solution.  A few remarks:
> 
> On Fri, Sep 29, 2017 at 08:40:57PM -0700, Doug Berger wrote:
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index 752a46ce3589..c964ed71a68d 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> @@ -19,17 +19,29 @@
>>  #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)
>> -#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)
>> +
>> +enum gio_reg_index {
>> +	GIO_REG_ODEN = 0,
>> +	GIO_REG_DATA,
>> +	GIO_REG_IODIR,
>> +	GIO_REG_EC,
>> +	GIO_REG_EI,
>> +	GIO_REG_MASK,
>> +	GIO_REG_LEVEL,
>> +	GIO_REG_STAT,
>> +	GIO_REG_COUNT
> 
> Please change the name or add a comment to make it clear that this is
> not for an actual register.
> 

Will do.

>> +};
>> +
>> [snip]
>> @@ -37,6 +49,8 @@ struct brcmstb_gpio_bank {
>>  	struct gpio_chip gc;
>>  	struct brcmstb_gpio_priv *parent_priv;
>>  	u32 width;
>> +	u32 wake_active;
>> +	u32 regs[GIO_REG_STAT]; /* Don't save and restore GIO_REG_STAT */
> 
> Consider naming to make it clearer that this is for save/restore, not
> some kind of shadow reg implementation or anything like that.
> 

Will do.

>>  };
>>  
>>  struct brcmstb_gpio_priv {
>> @@ -49,7 +63,6 @@ struct brcmstb_gpio_priv {
>>  	int gpio_base;
>>  	int num_gpios;
>>  	int parent_wake_irq;
>> -	struct notifier_block reboot_notifier;
>>  };
>>  
>>  #define MAX_GPIO_PER_BANK       32
>> @@ -65,15 +78,22 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
>>  }
>>  
>>  static unsigned long
>> -brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>> +__brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>>  {
>>  	void __iomem *reg_base = bank->parent_priv->reg_base;
>> +
>> +	return bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
>> +	       bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
>> +}
>> +
>> +static unsigned long
>> +brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
>> +{
>>  	unsigned long status;
>>  	unsigned long flags;
>>  
>>  	spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
>> -	status = bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
>> -		 bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
>> +	status = __brcmstb_gpio_get_active_irqs(bank);
>>  	spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
>>  
>>  	return status;
>> @@ -209,11 +229,6 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
>>  {
>>  	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
>> @@ -227,7 +242,17 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
>>  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);
>> +	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
>> +	struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> +	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
>> +
>> +	/* Do not do anything specific for now, suspend/resume callbacks will
>> +	 * configure the interrupt mask appropriately
>> +	 */
> 
> Please fix comment format (this is the network subsystem style).
> 

Thanks, force of habit ;).

>>
>> [snip] 
>> @@ -508,6 +511,99 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
>> [...]
>> +static void brcmstb_gpio_shutdown(struct platform_device *pdev)
>> +{
>> +	/* Enable GPIO for S5 cold boot */
>> +	brcmstb_gpio_quiesce(&pdev->dev, 0);
> 
> nit: use false instead of 0 (it's a bool).
> 

Will do.

>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> [snip]
>> +static int brcmstb_gpio_resume(struct device *dev)
>> +{
>> +	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
>> +	struct brcmstb_gpio_bank *bank;
>> +	u32 wake_mask = 0;
> 
> This isn't really being used as a mask, contrary to appearances.  It's
> just tracking whether any active IRQs were seen.  Please change to use a
> bool instead and adjust the name accordingly.
> 

I see your point, but I believe it is cleaner to use this to consolidate
the bit masks returned by each __brcmstb_gpio_get_active_irqs() call.
This allows a single test rather than a test per bank.

>> +
>> +	list_for_each_entry(bank, &priv->bank_list, node) {
>> +		wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
>> +		brcmstb_gpio_bank_restore(priv, bank);
>> +	}
>> +
>> +	if (priv->parent_wake_irq && wake_mask)
>> +		pm_wakeup_event(dev, 0);
>> +
>> +	/* enable non-wake interrupt */
>> +	if (priv->parent_irq >= 0)
>> +		enable_irq(priv->parent_irq);
>> +
>> +	return 0;
>> +}
>> +
>> +#else
>> +#define brcmstb_gpio_suspend	NULL
>> +#define brcmstb_gpio_resume	NULL
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
>> +	.suspend_noirq	= brcmstb_gpio_suspend,
>> +	.resume_noirq = brcmstb_gpio_resume,
>> +};
>> +
>>  static int brcmstb_gpio_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> @@ -517,7 +613,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>>  	struct resource *res;
>>  	struct property *prop;
>>  	const __be32 *p;
>> -	u32 bank_width;
>> +	u32 bank_width, wake_mask = 0;
> 
> Same comment on wake_mask as for brcmstb_gpio_resume() above.
> 

Same response.

>>  	int num_banks = 0;
>>  	int err;
>>  	static int gpio_base;
>> @@ -617,6 +713,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>>  		 * Mask all interrupts by default, since wakeup interrupts may
>>  		 * be retained from S5 cold boot
>>  		 */
>> +		wake_mask |= __brcmstb_gpio_get_active_irqs(bank);
>>  		gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
>>  
>>  		err = gpiochip_add_data(gc, bank);
>> @@ -646,6 +743,9 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>>  	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
>>  			num_banks, priv->gpio_base, gpio_base - 1);
>>  
>> +	if (priv->parent_wake_irq && wake_mask)
>> +		pm_wakeup_event(dev, 0);
> 
> Why is this called in probe?
> 

This allows proper wakeup accounting when waking from S5.

> Thanks,
> Gregory
> 

Thanks again for the review.  I will try to get a v2 out next week.
    Doug

Re: [PATCH 7/7] gpio: brcmstb: implement suspend/resume/shutdown

[Gregory Fong]

On Thu, Oct 19, 2017 at 11:39 AM, Doug Berger <opendmb@gmail.com> wrote:
>>> +static int brcmstb_gpio_resume(struct device *dev)
>>> +{
>>> +    struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
>>> +    struct brcmstb_gpio_bank *bank;
>>> +    u32 wake_mask = 0;
>>
>> This isn't really being used as a mask, contrary to appearances.  It's
>> just tracking whether any active IRQs were seen.  Please change to use a
>> bool instead and adjust the name accordingly.
>>
>
> I see your point, but I believe it is cleaner to use this to consolidate
> the bit masks returned by each __brcmstb_gpio_get_active_irqs() call.
> This allows a single test rather than a test per bank.

What about something like this?

bool need_wakeup_event = false;

list_for_each_entry(bank, &priv->bank_list, node) {
        need_wakeup_event |= !!__brcmstb_gpio_get_active_irqs(bank);
        brcmstb_gpio_bank_restore(priv, bank);
}

if (priv->parent_wake_irq && need_wakeup_event)
        pm_wakeup_event(dev, 0);

Re: [PATCH 7/7] gpio: brcmstb: implement suspend/resume/shutdown

[Doug Berger]

On 10/20/2017 05:54 PM, Gregory Fong wrote:
> On Thu, Oct 19, 2017 at 11:39 AM, Doug Berger <opendmb@gmail.com> wrote:
>>>> +static int brcmstb_gpio_resume(struct device *dev)
>>>> +{
>>>> +    struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
>>>> +    struct brcmstb_gpio_bank *bank;
>>>> +    u32 wake_mask = 0;
>>>
>>> This isn't really being used as a mask, contrary to appearances.  It's
>>> just tracking whether any active IRQs were seen.  Please change to use a
>>> bool instead and adjust the name accordingly.
>>>
>>
>> I see your point, but I believe it is cleaner to use this to consolidate
>> the bit masks returned by each __brcmstb_gpio_get_active_irqs() call.
>> This allows a single test rather than a test per bank.
> 
> What about something like this?
> 
> bool need_wakeup_event = false;
> 
> list_for_each_entry(bank, &priv->bank_list, node) {
>         need_wakeup_event |= !!__brcmstb_gpio_get_active_irqs(bank);
>         brcmstb_gpio_bank_restore(priv, bank);
> }
> 
> if (priv->parent_wake_irq && need_wakeup_event)
>         pm_wakeup_event(dev, 0);
> 

It's less efficient, but it is not performance sensitive so if you feel
this is more understandable I'll make the change.

Thanks,
    Doug

Re: [PATCH 0/7] gpio: brcmstb: improved interrupt and wake support

[Florian Fainelli]

On 09/29/2017 08:40 PM, Doug Berger wrote:
> This patch set collects a number of improvements to the GPIO driver
> used by Broadcom Set-Top-Box devices.
> 
> Primarily they are aimed at correcting problems with the interrupt
> controller implementation, but they also extend the functionality for
> waking on GPIO interrupts.

This all looks good to me, thanks a lot for sending those patches out!

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> 
> Doug Berger (7):
>   gpio: brcmstb: allow all instances to be wakeup sources
>   gpio: brcmstb: release the bgpio lock during irq handlers
>   gpio: brcmstb: switch to handle_level_irq flow
>   gpio: brcmstb: correct the configuration of level interrupts
>   gpio: brcmstb: enable masking of interrupts when changing type
>   gpio: brcmstb: consolidate interrupt domains
>   gpio: brcmstb: implement suspend/resume/shutdown
> 
>  drivers/gpio/Kconfig        |   2 +-
>  drivers/gpio/gpio-brcmstb.c | 419 +++++++++++++++++++++++++++++++++-----------
>  2 files changed, 321 insertions(+), 100 deletions(-)
> 

-- 
Florian

Re: [PATCH 0/7] gpio: brcmstb: improved interrupt and wake support

[Linus Walleij]

On Sat, Sep 30, 2017 at 5:40 AM, Doug Berger <opendmb@gmail.com> wrote:

> This patch set collects a number of improvements to the GPIO driver
> used by Broadcom Set-Top-Box devices.
>
> Primarily they are aimed at correcting problems with the interrupt
> controller implementation, but they also extend the functionality for
> waking on GPIO interrupts.

I saw there was some mention of a v2 patch set inline, so I
backed out the applied patch.

Please send the v2 series and collect the ACKs from Gregory
and Florian so I can apply it all!

Yours,
Linus Walleij