Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1742484 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] gpio: brcmstb: improved interrupt and wake support Doug Berger <opendmb@gmail.com> - 2017-09-30 05:50 +0200
    [PATCH 4/7] gpio: brcmstb: correct the configuration of level interrupts Doug Berger <opendmb@gmail.com> - 2017-09-30 05:50 +0200
    [PATCH 5/7] gpio: brcmstb: enable masking of interrupts when changing type Doug Berger <opendmb@gmail.com> - 2017-09-30 05:50 +0200
    [PATCH 1/7] gpio: brcmstb: allow all instances to be wakeup sources Doug Berger <opendmb@gmail.com> - 2017-09-30 05:50 +0200
    [PATCH 3/7] gpio: brcmstb: switch to handle_level_irq flow Doug Berger <opendmb@gmail.com> - 2017-09-30 05:50 +0200
    [PATCH 2/7] gpio: brcmstb: release the bgpio lock during irq handlers Doug Berger <opendmb@gmail.com> - 2017-09-30 05:50 +0200
    Re: [PATCH 0/7] gpio: brcmstb: improved interrupt and wake support Florian Fainelli <f.fainelli@gmail.com> - 2017-09-30 07:40 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1742510

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web