[PATCH 0/2] gpio/omap: updates for v3.10

[PATCH 0/2] gpio/omap: updates for v3.10

[Jon Hunter]

Summary of updates:
- Convert OMAP GPIO driver to use linear mapping for IRQ domains
- Avoid crashes seen if a gpio bank is not enabled when requesting an IRQ.

Testing includes:
- Boot testing on OMAP5912 OSK, OMAP2420 H4, OMAP3430 SDP, OMAP4430 SDP
  and AM335x EVM.
- Verified that GPIO interrupts are working on OMAP5912 OSK, OMAP2420 H4,
  OMAP3430 SDP and OMAP4430 SDP by making sure networking was working
  correctly as these boards use a GPIO with the ethernet chips. Also
  checked /proc/interrupts to ensure GPIO interrupt counts are
  incrementing as expected.

Jon Hunter (2):
  gpio/omap: convert gpio irq domain to linear mapping
  gpio/omap: warn if bank is not enabled on setting irq type

 drivers/gpio/gpio-omap.c |   75 +++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

-- 
1.7.10.4

[PATCH 0/2] gpio/omap: updates for v3.10

[Jon Hunter]

Summary of updates:
- Convert OMAP GPIO driver to use linear mapping for IRQ domains
- Avoid crashes seen if a gpio bank is not enabled when requesting an IRQ.

Testing includes:
- Boot testing on OMAP5912 OSK, OMAP2420 H4, OMAP3430 SDP, OMAP4430 SDP
  and AM335x EVM.
- Verified that GPIO interrupts are working on OMAP5912 OSK, OMAP2420 H4,
  OMAP3430 SDP and OMAP4430 SDP by making sure networking was working
  correctly as these boards use a GPIO with the ethernet chips. Also
  checked /proc/interrupts to ensure GPIO interrupt counts are
  incrementing as expected.

Jon Hunter (2):
  gpio/omap: convert gpio irq domain to linear mapping
  gpio/omap: warn if bank is not enabled on setting irq type

 drivers/gpio/gpio-omap.c |   75 +++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Jon Hunter]

Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
domain. This is not necessary because we do not need to assign a
specific interrupt number to the GPIO IRQ domain. Therefore, convert
the OMAP GPIO driver to use a linear mapping instead.

Please note that this also allows to simplify the logic in the OMAP
gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
virtual irq number from the GPIO bank and bank index.

Reported-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 drivers/gpio/gpio-omap.c |   72 ++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..c3598d1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -53,7 +53,6 @@ struct gpio_bank {
 	struct list_head node;
 	void __iomem *base;
 	u16 irq;
-	int irq_base;
 	struct irq_domain *domain;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
@@ -89,7 +88,14 @@ struct gpio_bank {
 
 static int irq_to_gpio(struct gpio_bank *bank, unsigned int gpio_irq)
 {
-	return gpio_irq - bank->irq_base + bank->chip.base;
+	return bank->chip.base + gpio_irq;
+}
+
+static int omap_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
+
+	return irq_find_mapping(bank->domain, offset);
 }
 
 static void _set_gpio_direction(struct gpio_bank *bank, int gpio, int is_input)
@@ -427,7 +433,7 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 #endif
 
 	if (!gpio)
-		gpio = irq_to_gpio(bank, d->irq);
+		gpio = irq_to_gpio(bank, d->hwirq);
 
 	if (type & ~IRQ_TYPE_SENSE_MASK)
 		return -EINVAL;
@@ -580,7 +586,7 @@ static void _reset_gpio(struct gpio_bank *bank, int gpio)
 static int gpio_wake_enable(struct irq_data *d, unsigned int enable)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
 	return _set_gpio_wakeup(bank, gpio, enable);
 }
@@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 {
 	void __iomem *isr_reg = NULL;
 	u32 isr;
-	unsigned int gpio_irq, gpio_index;
+	unsigned int i;
 	struct gpio_bank *bank;
 	int unmasked = 0;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 		if (!isr)
 			break;
 
-		gpio_irq = bank->irq_base;
-		for (; isr != 0; isr >>= 1, gpio_irq++) {
-			int gpio = irq_to_gpio(bank, gpio_irq);
-
+		for (i = 0; isr != 0; isr >>= 1, i++) {
 			if (!(isr & 1))
 				continue;
 
-			gpio_index = GPIO_INDEX(bank, gpio);
-
 			/*
 			 * Some chips can't respond to both rising and falling
 			 * at the same time.  If this irq was requested with
@@ -737,10 +738,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 			 * to respond to the IRQ for the opposite direction.
 			 * This will be indicated in the bank toggle_mask.
 			 */
-			if (bank->toggle_mask & (1 << gpio_index))
-				_toggle_gpio_edge_triggering(bank, gpio_index);
+			if (bank->toggle_mask & (1 << i))
+				_toggle_gpio_edge_triggering(bank, i);
 
-			generic_handle_irq(gpio_irq);
+			generic_handle_irq(irq_find_mapping(bank->domain, i));
 		}
 	}
 	/* if bank has any level sensitive GPIO pin interrupt
@@ -756,7 +757,7 @@ exit:
 static void gpio_irq_shutdown(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -767,7 +768,7 @@ static void gpio_irq_shutdown(struct irq_data *d)
 static void gpio_ack_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
 	_clear_gpio_irqstatus(bank, gpio);
 }
@@ -775,7 +776,7 @@ static void gpio_ack_irq(struct irq_data *d)
 static void gpio_mask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -787,7 +788,7 @@ static void gpio_mask_irq(struct irq_data *d)
 static void gpio_unmask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned int irq_mask = GPIO_BIT(bank, gpio);
 	u32 trigger = irqd_get_trigger_type(d);
 	unsigned long flags;
@@ -953,14 +954,6 @@ static void gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
 
-static int gpio_2irq(struct gpio_chip *chip, unsigned offset)
-{
-	struct gpio_bank *bank;
-
-	bank = container_of(chip, struct gpio_bank, chip);
-	return bank->irq_base + offset;
-}
-
 /*---------------------------------------------------------------------*/
 
 static void __init omap_gpio_show_rev(struct gpio_bank *bank)
@@ -1057,7 +1050,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 	bank->chip.direction_output = gpio_output;
 	bank->chip.set_debounce = gpio_debounce;
 	bank->chip.set = gpio_set;
-	bank->chip.to_irq = gpio_2irq;
+	bank->chip.to_irq = omap_gpio_to_irq;
 	if (bank->is_mpuio) {
 		bank->chip.label = "mpuio";
 		if (bank->regs->wkup_en)
@@ -1072,15 +1065,16 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 
 	gpiochip_add(&bank->chip);
 
-	for (j = bank->irq_base; j < bank->irq_base + bank->width; j++) {
-		irq_set_lockdep_class(j, &gpio_lock_class);
-		irq_set_chip_data(j, bank);
+	for (j = 0; j < bank->width; j++) {
+		int irq = irq_create_mapping(bank->domain, j);
+		irq_set_lockdep_class(irq, &gpio_lock_class);
+		irq_set_chip_data(irq, bank);
 		if (bank->is_mpuio) {
-			omap_mpuio_alloc_gc(bank, j, bank->width);
+			omap_mpuio_alloc_gc(bank, irq, bank->width);
 		} else {
-			irq_set_chip(j, &gpio_irq_chip);
-			irq_set_handler(j, handle_simple_irq);
-			set_irq_flags(j, IRQF_VALID);
+			irq_set_chip_and_handler(irq, &gpio_irq_chip,
+						 handle_simple_irq);
+			set_irq_flags(irq, IRQF_VALID);
 		}
 	}
 	irq_set_chained_handler(bank->irq, gpio_irq_handler);
@@ -1130,14 +1124,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	bank->chip.of_node = of_node_get(node);
 #endif
 
-	bank->irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
-	if (bank->irq_base < 0) {
-		dev_err(dev, "Couldn't allocate IRQ numbers\n");
+	bank->domain = irq_domain_add_linear(node, bank->width,
+					     &irq_domain_simple_ops, NULL);
+	if (!bank->domain)
 		return -ENODEV;
-	}
-
-	bank->domain = irq_domain_add_legacy(node, bank->width, bank->irq_base,
-					     0, &irq_domain_simple_ops, NULL);
 
 	if (bank->regs->set_dataout && bank->regs->clr_dataout)
 		bank->set_dataout = _set_gpio_dataout_reg;
-- 
1.7.10.4

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Jon Hunter]

Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
domain. This is not necessary because we do not need to assign a
specific interrupt number to the GPIO IRQ domain. Therefore, convert
the OMAP GPIO driver to use a linear mapping instead.

Please note that this also allows to simplify the logic in the OMAP
gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
virtual irq number from the GPIO bank and bank index.

Reported-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 drivers/gpio/gpio-omap.c |   72 ++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..c3598d1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -53,7 +53,6 @@ struct gpio_bank {
 	struct list_head node;
 	void __iomem *base;
 	u16 irq;
-	int irq_base;
 	struct irq_domain *domain;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
@@ -89,7 +88,14 @@ struct gpio_bank {
 
 static int irq_to_gpio(struct gpio_bank *bank, unsigned int gpio_irq)
 {
-	return gpio_irq - bank->irq_base + bank->chip.base;
+	return bank->chip.base + gpio_irq;
+}
+
+static int omap_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
+
+	return irq_find_mapping(bank->domain, offset);
 }
 
 static void _set_gpio_direction(struct gpio_bank *bank, int gpio, int is_input)
@@ -427,7 +433,7 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 #endif
 
 	if (!gpio)
-		gpio = irq_to_gpio(bank, d->irq);
+		gpio = irq_to_gpio(bank, d->hwirq);
 
 	if (type & ~IRQ_TYPE_SENSE_MASK)
 		return -EINVAL;
@@ -580,7 +586,7 @@ static void _reset_gpio(struct gpio_bank *bank, int gpio)
 static int gpio_wake_enable(struct irq_data *d, unsigned int enable)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
 	return _set_gpio_wakeup(bank, gpio, enable);
 }
@@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 {
 	void __iomem *isr_reg = NULL;
 	u32 isr;
-	unsigned int gpio_irq, gpio_index;
+	unsigned int i;
 	struct gpio_bank *bank;
 	int unmasked = 0;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 		if (!isr)
 			break;
 
-		gpio_irq = bank->irq_base;
-		for (; isr != 0; isr >>= 1, gpio_irq++) {
-			int gpio = irq_to_gpio(bank, gpio_irq);
-
+		for (i = 0; isr != 0; isr >>= 1, i++) {
 			if (!(isr & 1))
 				continue;
 
-			gpio_index = GPIO_INDEX(bank, gpio);
-
 			/*
 			 * Some chips can't respond to both rising and falling
 			 * at the same time.  If this irq was requested with
@@ -737,10 +738,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 			 * to respond to the IRQ for the opposite direction.
 			 * This will be indicated in the bank toggle_mask.
 			 */
-			if (bank->toggle_mask & (1 << gpio_index))
-				_toggle_gpio_edge_triggering(bank, gpio_index);
+			if (bank->toggle_mask & (1 << i))
+				_toggle_gpio_edge_triggering(bank, i);
 
-			generic_handle_irq(gpio_irq);
+			generic_handle_irq(irq_find_mapping(bank->domain, i));
 		}
 	}
 	/* if bank has any level sensitive GPIO pin interrupt
@@ -756,7 +757,7 @@ exit:
 static void gpio_irq_shutdown(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -767,7 +768,7 @@ static void gpio_irq_shutdown(struct irq_data *d)
 static void gpio_ack_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
 	_clear_gpio_irqstatus(bank, gpio);
 }
@@ -775,7 +776,7 @@ static void gpio_ack_irq(struct irq_data *d)
 static void gpio_mask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -787,7 +788,7 @@ static void gpio_mask_irq(struct irq_data *d)
 static void gpio_unmask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
-	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned int irq_mask = GPIO_BIT(bank, gpio);
 	u32 trigger = irqd_get_trigger_type(d);
 	unsigned long flags;
@@ -953,14 +954,6 @@ static void gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
 
-static int gpio_2irq(struct gpio_chip *chip, unsigned offset)
-{
-	struct gpio_bank *bank;
-
-	bank = container_of(chip, struct gpio_bank, chip);
-	return bank->irq_base + offset;
-}
-
 /*---------------------------------------------------------------------*/
 
 static void __init omap_gpio_show_rev(struct gpio_bank *bank)
@@ -1057,7 +1050,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 	bank->chip.direction_output = gpio_output;
 	bank->chip.set_debounce = gpio_debounce;
 	bank->chip.set = gpio_set;
-	bank->chip.to_irq = gpio_2irq;
+	bank->chip.to_irq = omap_gpio_to_irq;
 	if (bank->is_mpuio) {
 		bank->chip.label = "mpuio";
 		if (bank->regs->wkup_en)
@@ -1072,15 +1065,16 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 
 	gpiochip_add(&bank->chip);
 
-	for (j = bank->irq_base; j < bank->irq_base + bank->width; j++) {
-		irq_set_lockdep_class(j, &gpio_lock_class);
-		irq_set_chip_data(j, bank);
+	for (j = 0; j < bank->width; j++) {
+		int irq = irq_create_mapping(bank->domain, j);
+		irq_set_lockdep_class(irq, &gpio_lock_class);
+		irq_set_chip_data(irq, bank);
 		if (bank->is_mpuio) {
-			omap_mpuio_alloc_gc(bank, j, bank->width);
+			omap_mpuio_alloc_gc(bank, irq, bank->width);
 		} else {
-			irq_set_chip(j, &gpio_irq_chip);
-			irq_set_handler(j, handle_simple_irq);
-			set_irq_flags(j, IRQF_VALID);
+			irq_set_chip_and_handler(irq, &gpio_irq_chip,
+						 handle_simple_irq);
+			set_irq_flags(irq, IRQF_VALID);
 		}
 	}
 	irq_set_chained_handler(bank->irq, gpio_irq_handler);
@@ -1130,14 +1124,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	bank->chip.of_node = of_node_get(node);
 #endif
 
-	bank->irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
-	if (bank->irq_base < 0) {
-		dev_err(dev, "Couldn't allocate IRQ numbers\n");
+	bank->domain = irq_domain_add_linear(node, bank->width,
+					     &irq_domain_simple_ops, NULL);
+	if (!bank->domain)
 		return -ENODEV;
-	}
-
-	bank->domain = irq_domain_add_legacy(node, bank->width, bank->irq_base,
-					     0, &irq_domain_simple_ops, NULL);
 
 	if (bank->regs->set_dataout && bank->regs->clr_dataout)
 		bank->set_dataout = _set_gpio_dataout_reg;
-- 
1.7.10.4

[PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Jon Hunter]

For OMAP devices, if a gpio is being used as an interrupt source but has
not been requested by calling gpio_request(), a call to request_irq()
may cause the kernel hang because the gpio bank may be disabled and
hence the register access will fail. To prevent such hangs, test for
this case and warn if this is detected.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 drivers/gpio/gpio-omap.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c3598d1..0d30c7a 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -427,6 +427,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 	int retval;
 	unsigned long flags;
 
+	if (WARN_ON(!bank->mod_usage))
+		return -EINVAL;
+
 #ifdef CONFIG_ARCH_OMAP1
 	if (d->irq > IH_MPUIO_BASE)
 		gpio = OMAP_MPUIO(d->irq - IH_MPUIO_BASE);
-- 
1.7.10.4

[PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Jon Hunter]

For OMAP devices, if a gpio is being used as an interrupt source but has
not been requested by calling gpio_request(), a call to request_irq()
may cause the kernel hang because the gpio bank may be disabled and
hence the register access will fail. To prevent such hangs, test for
this case and warn if this is detected.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 drivers/gpio/gpio-omap.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c3598d1..0d30c7a 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -427,6 +427,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 	int retval;
 	unsigned long flags;
 
+	if (WARN_ON(!bank->mod_usage))
+		return -EINVAL;
+
 #ifdef CONFIG_ARCH_OMAP1
 	if (d->irq > IH_MPUIO_BASE)
 		gpio = OMAP_MPUIO(d->irq - IH_MPUIO_BASE);
-- 
1.7.10.4

Re: [PATCH 0/2] gpio/omap: updates for v3.10

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Summary of updates:
> - Convert OMAP GPIO driver to use linear mapping for IRQ domains
> - Avoid crashes seen if a gpio bank is not enabled when requesting an IRQ.

Acked-by: Kevin Hilman <khilman@linaro.org>

[PATCH 0/2] gpio/omap: updates for v3.10

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Summary of updates:
> - Convert OMAP GPIO driver to use linear mapping for IRQ domains
> - Avoid crashes seen if a gpio bank is not enabled when requesting an IRQ.

Acked-by: Kevin Hilman <khilman@linaro.org>

Re: [PATCH 0/2] gpio/omap: updates for v3.10

[Javier Martinez Canillas]

On Fri, Mar 1, 2013 at 6:22 PM, Jon Hunter <jon-hunter@ti.com> wrote:
> Summary of updates:
> - Convert OMAP GPIO driver to use linear mapping for IRQ domains
> - Avoid crashes seen if a gpio bank is not enabled when requesting an IRQ.
>
> Testing includes:
> - Boot testing on OMAP5912 OSK, OMAP2420 H4, OMAP3430 SDP, OMAP4430 SDP
>   and AM335x EVM.
> - Verified that GPIO interrupts are working on OMAP5912 OSK, OMAP2420 H4,
>   OMAP3430 SDP and OMAP4430 SDP by making sure networking was working
>   correctly as these boards use a GPIO with the ethernet chips. Also
>   checked /proc/interrupts to ensure GPIO interrupt counts are
>   incrementing as expected.
>
> Jon Hunter (2):
>   gpio/omap: convert gpio irq domain to linear mapping
>   gpio/omap: warn if bank is not enabled on setting irq type
>
>  drivers/gpio/gpio-omap.c |   75 +++++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 41 deletions(-)
>
> --

I've tested on a TI OMAP3 DM3735 board (IGEPv2) and the ethernet chip
(which use a GPIO line as a IRQ) is working correctly too.

So, for both patches:

Tested-by: Javier Martinez Canillas <javier@dowhile0.org>

[PATCH 0/2] gpio/omap: updates for v3.10

[Javier Martinez Canillas]

On Fri, Mar 1, 2013 at 6:22 PM, Jon Hunter <jon-hunter@ti.com> wrote:
> Summary of updates:
> - Convert OMAP GPIO driver to use linear mapping for IRQ domains
> - Avoid crashes seen if a gpio bank is not enabled when requesting an IRQ.
>
> Testing includes:
> - Boot testing on OMAP5912 OSK, OMAP2420 H4, OMAP3430 SDP, OMAP4430 SDP
>   and AM335x EVM.
> - Verified that GPIO interrupts are working on OMAP5912 OSK, OMAP2420 H4,
>   OMAP3430 SDP and OMAP4430 SDP by making sure networking was working
>   correctly as these boards use a GPIO with the ethernet chips. Also
>   checked /proc/interrupts to ensure GPIO interrupt counts are
>   incrementing as expected.
>
> Jon Hunter (2):
>   gpio/omap: convert gpio irq domain to linear mapping
>   gpio/omap: warn if bank is not enabled on setting irq type
>
>  drivers/gpio/gpio-omap.c |   75 +++++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 41 deletions(-)
>
> --

I've tested on a TI OMAP3 DM3735 board (IGEPv2) and the ethernet chip
(which use a GPIO line as a IRQ) is working correctly too.

So, for both patches:

Tested-by: Javier Martinez Canillas <javier@dowhile0.org>

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Santosh Shilimkar]

On Friday 01 March 2013 10:52 PM, Jon Hunter wrote:
> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> domain. This is not necessary because we do not need to assign a
> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> the OMAP GPIO driver to use a linear mapping instead.
> 
> Please note that this also allows to simplify the logic in the OMAP
> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> virtual irq number from the GPIO bank and bank index.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
Cool.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Santosh Shilimkar]

On Friday 01 March 2013 10:52 PM, Jon Hunter wrote:
> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> domain. This is not necessary because we do not need to assign a
> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> the OMAP GPIO driver to use a linear mapping instead.
> 
> Please note that this also allows to simplify the logic in the OMAP
> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> virtual irq number from the GPIO bank and bank index.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
Cool.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Santosh Shilimkar]

On Friday 01 March 2013 10:52 PM, Jon Hunter wrote:
> For OMAP devices, if a gpio is being used as an interrupt source but has
> not been requested by calling gpio_request(), a call to request_irq()
> may cause the kernel hang because the gpio bank may be disabled and
> hence the register access will fail. To prevent such hangs, test for
> this case and warn if this is detected.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Santosh Shilimkar]

On Friday 01 March 2013 10:52 PM, Jon Hunter wrote:
> For OMAP devices, if a gpio is being used as an interrupt source but has
> not been requested by calling gpio_request(), a call to request_irq()
> may cause the kernel hang because the gpio bank may be disabled and
> hence the register access will fail. To prevent such hangs, test for
> this case and warn if this is detected.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> domain. This is not necessary because we do not need to assign a
> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> the OMAP GPIO driver to use a linear mapping instead.
> 
> Please note that this also allows to simplify the logic in the OMAP
> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> virtual irq number from the GPIO bank and bank index.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>

Reviewed-by: Felipe Balbi <balbi@ti.com>

Just one suggestion below for a later patch.

> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  {
>  	void __iomem *isr_reg = NULL;
>  	u32 isr;
> -	unsigned int gpio_irq, gpio_index;
> +	unsigned int i;
>  	struct gpio_bank *bank;
>  	int unmasked = 0;
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  		if (!isr)
>  			break;
>  
> -		gpio_irq = bank->irq_base;
> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
> -			int gpio = irq_to_gpio(bank, gpio_irq);
> -
> +		for (i = 0; isr != 0; isr >>= 1, i++) {
>  			if (!(isr & 1))
>  				continue;

this will iterate over all 32 GPIOs, a better way to handle this would
be to have something like:

while (isr) {
	unsigned long bit = __ffs(isr);

	/* clear this bit */
	isr &= ~bit;

	generic_handle_irq(irq_find_mapping(bank->domain, bit);
}

this way you will only iterate the amount of bits enabled in the isr
register.

ps: completely untested ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Felipe Balbi]

On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> domain. This is not necessary because we do not need to assign a
> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> the OMAP GPIO driver to use a linear mapping instead.
> 
> Please note that this also allows to simplify the logic in the OMAP
> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> virtual irq number from the GPIO bank and bank index.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>

Reviewed-by: Felipe Balbi <balbi@ti.com>

Just one suggestion below for a later patch.

> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  {
>  	void __iomem *isr_reg = NULL;
>  	u32 isr;
> -	unsigned int gpio_irq, gpio_index;
> +	unsigned int i;
>  	struct gpio_bank *bank;
>  	int unmasked = 0;
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  		if (!isr)
>  			break;
>  
> -		gpio_irq = bank->irq_base;
> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
> -			int gpio = irq_to_gpio(bank, gpio_irq);
> -
> +		for (i = 0; isr != 0; isr >>= 1, i++) {
>  			if (!(isr & 1))
>  				continue;

this will iterate over all 32 GPIOs, a better way to handle this would
be to have something like:

while (isr) {
	unsigned long bit = __ffs(isr);

	/* clear this bit */
	isr &= ~bit;

	generic_handle_irq(irq_find_mapping(bank->domain, bit);
}

this way you will only iterate the amount of bits enabled in the isr
register.

ps: completely untested ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130302/0330d140/attachment.sig>

Re: [PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Fri, Mar 01, 2013 at 11:22:48AM -0600, Jon Hunter wrote:
> For OMAP devices, if a gpio is being used as an interrupt source but has
> not been requested by calling gpio_request(), a call to request_irq()
> may cause the kernel hang because the gpio bank may be disabled and
> hence the register access will fail. To prevent such hangs, test for
> this case and warn if this is detected.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index c3598d1..0d30c7a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -427,6 +427,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
>  	int retval;
>  	unsigned long flags;
>  
> +	if (WARN_ON(!bank->mod_usage))

It would be better to use WARN(!bank->mod_usage, "error message\n") IMO.

no strong feelings however:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Felipe Balbi]

On Fri, Mar 01, 2013 at 11:22:48AM -0600, Jon Hunter wrote:
> For OMAP devices, if a gpio is being used as an interrupt source but has
> not been requested by calling gpio_request(), a call to request_irq()
> may cause the kernel hang because the gpio bank may be disabled and
> hence the register access will fail. To prevent such hangs, test for
> this case and warn if this is detected.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index c3598d1..0d30c7a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -427,6 +427,9 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
>  	int retval;
>  	unsigned long flags;
>  
> +	if (WARN_ON(!bank->mod_usage))

It would be better to use WARN(!bank->mod_usage, "error message\n") IMO.

no strong feelings however:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130302/236f9c77/attachment.sig>

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Grant Likely]

On Fri, 1 Mar 2013 11:22:47 -0600, Jon Hunter <jon-hunter@ti.com> wrote:
> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> domain. This is not necessary because we do not need to assign a
> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> the OMAP GPIO driver to use a linear mapping instead.
> 
> Please note that this also allows to simplify the logic in the OMAP
> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> virtual irq number from the GPIO bank and bank index.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>

Applied, thanks.

g.

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Grant Likely]

On Fri, 1 Mar 2013 11:22:47 -0600, Jon Hunter <jon-hunter@ti.com> wrote:
> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> domain. This is not necessary because we do not need to assign a
> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> the OMAP GPIO driver to use a linear mapping instead.
> 
> Please note that this also allows to simplify the logic in the OMAP
> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> virtual irq number from the GPIO bank and bank index.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>

Applied, thanks.

g.

Re: [PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Grant Likely]

On Fri, 1 Mar 2013 11:22:48 -0600, Jon Hunter <jon-hunter@ti.com> wrote:
> For OMAP devices, if a gpio is being used as an interrupt source but has
> not been requested by calling gpio_request(), a call to request_irq()
> may cause the kernel hang because the gpio bank may be disabled and
> hence the register access will fail. To prevent such hangs, test for
> this case and warn if this is detected.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>

Applied, thanks.

g.

[PATCH 2/2] gpio/omap: warn if bank is not enabled on setting irq type

[Grant Likely]

On Fri, 1 Mar 2013 11:22:48 -0600, Jon Hunter <jon-hunter@ti.com> wrote:
> For OMAP devices, if a gpio is being used as an interrupt source but has
> not been requested by calling gpio_request(), a call to request_irq()
> may cause the kernel hang because the gpio bank may be disabled and
> hence the register access will fail. To prevent such hangs, test for
> this case and warn if this is detected.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>

Applied, thanks.

g.

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Jon Hunter]

On 03/02/2013 05:48 AM, Felipe Balbi wrote:
> On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
>> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
>> domain. This is not necessary because we do not need to assign a
>> specific interrupt number to the GPIO IRQ domain. Therefore, convert
>> the OMAP GPIO driver to use a linear mapping instead.
>>
>> Please note that this also allows to simplify the logic in the OMAP
>> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
>> virtual irq number from the GPIO bank and bank index.
>>
>> Reported-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> 
> Reviewed-by: Felipe Balbi <balbi@ti.com>
> 
> Just one suggestion below for a later patch.
> 
>> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>  {
>>  	void __iomem *isr_reg = NULL;
>>  	u32 isr;
>> -	unsigned int gpio_irq, gpio_index;
>> +	unsigned int i;
>>  	struct gpio_bank *bank;
>>  	int unmasked = 0;
>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>  		if (!isr)
>>  			break;
>>  
>> -		gpio_irq = bank->irq_base;
>> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
>> -			int gpio = irq_to_gpio(bank, gpio_irq);
>> -
>> +		for (i = 0; isr != 0; isr >>= 1, i++) {
>>  			if (!(isr & 1))
>>  				continue;
> 
> this will iterate over all 32 GPIOs, a better way to handle this would
> be to have something like:

Worse case, if only bit 31 was set then I agree this is not that
efficient. Or even if one bit is set. However, the loop itself will
iterate while isr != 0 so not always over each bit. No different to the
existing code.

> while (isr) {
> 	unsigned long bit = __ffs(isr);
>
> 	/* clear this bit */
> 	isr &= ~bit;
> 
> 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
> }
> 
> this way you will only iterate the amount of bits enabled in the isr
> register.

Definitely cleaner but I am wondering which approach would be more
efficient from an instruction standpoint. This could definitely be much
more efficient if there is only a couple bits set.

> ps: completely untested ;-)

No problem. Thanks for the inputs.

Cheers
Jon

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Jon Hunter]

On 03/02/2013 05:48 AM, Felipe Balbi wrote:
> On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
>> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
>> domain. This is not necessary because we do not need to assign a
>> specific interrupt number to the GPIO IRQ domain. Therefore, convert
>> the OMAP GPIO driver to use a linear mapping instead.
>>
>> Please note that this also allows to simplify the logic in the OMAP
>> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
>> virtual irq number from the GPIO bank and bank index.
>>
>> Reported-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> 
> Reviewed-by: Felipe Balbi <balbi@ti.com>
> 
> Just one suggestion below for a later patch.
> 
>> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>  {
>>  	void __iomem *isr_reg = NULL;
>>  	u32 isr;
>> -	unsigned int gpio_irq, gpio_index;
>> +	unsigned int i;
>>  	struct gpio_bank *bank;
>>  	int unmasked = 0;
>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>  		if (!isr)
>>  			break;
>>  
>> -		gpio_irq = bank->irq_base;
>> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
>> -			int gpio = irq_to_gpio(bank, gpio_irq);
>> -
>> +		for (i = 0; isr != 0; isr >>= 1, i++) {
>>  			if (!(isr & 1))
>>  				continue;
> 
> this will iterate over all 32 GPIOs, a better way to handle this would
> be to have something like:

Worse case, if only bit 31 was set then I agree this is not that
efficient. Or even if one bit is set. However, the loop itself will
iterate while isr != 0 so not always over each bit. No different to the
existing code.

> while (isr) {
> 	unsigned long bit = __ffs(isr);
>
> 	/* clear this bit */
> 	isr &= ~bit;
> 
> 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
> }
> 
> this way you will only iterate the amount of bits enabled in the isr
> register.

Definitely cleaner but I am wondering which approach would be more
efficient from an instruction standpoint. This could definitely be much
more efficient if there is only a couple bits set.

> ps: completely untested ;-)

No problem. Thanks for the inputs.

Cheers
Jon

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]

On Mon, Mar 04, 2013 at 11:06:12AM -0600, Jon Hunter wrote:
> 
> On 03/02/2013 05:48 AM, Felipe Balbi wrote:
> > On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
> >> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> >> domain. This is not necessary because we do not need to assign a
> >> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> >> the OMAP GPIO driver to use a linear mapping instead.
> >>
> >> Please note that this also allows to simplify the logic in the OMAP
> >> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> >> virtual irq number from the GPIO bank and bank index.
> >>
> >> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> > 
> > Reviewed-by: Felipe Balbi <balbi@ti.com>
> > 
> > Just one suggestion below for a later patch.
> > 
> >> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> >>  {
> >>  	void __iomem *isr_reg = NULL;
> >>  	u32 isr;
> >> -	unsigned int gpio_irq, gpio_index;
> >> +	unsigned int i;
> >>  	struct gpio_bank *bank;
> >>  	int unmasked = 0;
> >>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> >> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> >>  		if (!isr)
> >>  			break;
> >>  
> >> -		gpio_irq = bank->irq_base;
> >> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
> >> -			int gpio = irq_to_gpio(bank, gpio_irq);
> >> -
> >> +		for (i = 0; isr != 0; isr >>= 1, i++) {
> >>  			if (!(isr & 1))
> >>  				continue;
> > 
> > this will iterate over all 32 GPIOs, a better way to handle this would
> > be to have something like:
> 
> Worse case, if only bit 31 was set then I agree this is not that
> efficient. Or even if one bit is set. However, the loop itself will
> iterate while isr != 0 so not always over each bit. No different to the
> existing code.
> 
> > while (isr) {
> > 	unsigned long bit = __ffs(isr);
> >
> > 	/* clear this bit */
> > 	isr &= ~bit;
> > 
> > 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
> > }
> > 
> > this way you will only iterate the amount of bits enabled in the isr
> > register.
> 
> Definitely cleaner but I am wondering which approach would be more
> efficient from an instruction standpoint. This could definitely be much
> more efficient if there is only a couple bits set.

__ffs() is done with CLZ instruction, so it's pretty fast.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Felipe Balbi]

On Mon, Mar 04, 2013 at 11:06:12AM -0600, Jon Hunter wrote:
> 
> On 03/02/2013 05:48 AM, Felipe Balbi wrote:
> > On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
> >> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
> >> domain. This is not necessary because we do not need to assign a
> >> specific interrupt number to the GPIO IRQ domain. Therefore, convert
> >> the OMAP GPIO driver to use a linear mapping instead.
> >>
> >> Please note that this also allows to simplify the logic in the OMAP
> >> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
> >> virtual irq number from the GPIO bank and bank index.
> >>
> >> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> > 
> > Reviewed-by: Felipe Balbi <balbi@ti.com>
> > 
> > Just one suggestion below for a later patch.
> > 
> >> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> >>  {
> >>  	void __iomem *isr_reg = NULL;
> >>  	u32 isr;
> >> -	unsigned int gpio_irq, gpio_index;
> >> +	unsigned int i;
> >>  	struct gpio_bank *bank;
> >>  	int unmasked = 0;
> >>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> >> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> >>  		if (!isr)
> >>  			break;
> >>  
> >> -		gpio_irq = bank->irq_base;
> >> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
> >> -			int gpio = irq_to_gpio(bank, gpio_irq);
> >> -
> >> +		for (i = 0; isr != 0; isr >>= 1, i++) {
> >>  			if (!(isr & 1))
> >>  				continue;
> > 
> > this will iterate over all 32 GPIOs, a better way to handle this would
> > be to have something like:
> 
> Worse case, if only bit 31 was set then I agree this is not that
> efficient. Or even if one bit is set. However, the loop itself will
> iterate while isr != 0 so not always over each bit. No different to the
> existing code.
> 
> > while (isr) {
> > 	unsigned long bit = __ffs(isr);
> >
> > 	/* clear this bit */
> > 	isr &= ~bit;
> > 
> > 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
> > }
> > 
> > this way you will only iterate the amount of bits enabled in the isr
> > register.
> 
> Definitely cleaner but I am wondering which approach would be more
> efficient from an instruction standpoint. This could definitely be much
> more efficient if there is only a couple bits set.

__ffs() is done with CLZ instruction, so it's pretty fast.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130304/4ca672d8/attachment.sig>

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Jon Hunter]

On 03/04/2013 11:11 AM, Felipe Balbi wrote:
> On Mon, Mar 04, 2013 at 11:06:12AM -0600, Jon Hunter wrote:
>>
>> On 03/02/2013 05:48 AM, Felipe Balbi wrote:
>>> On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
>>>> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
>>>> domain. This is not necessary because we do not need to assign a
>>>> specific interrupt number to the GPIO IRQ domain. Therefore, convert
>>>> the OMAP GPIO driver to use a linear mapping instead.
>>>>
>>>> Please note that this also allows to simplify the logic in the OMAP
>>>> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
>>>> virtual irq number from the GPIO bank and bank index.
>>>>
>>>> Reported-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>>
>>> Reviewed-by: Felipe Balbi <balbi@ti.com>
>>>
>>> Just one suggestion below for a later patch.
>>>
>>>> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>>>  {
>>>>  	void __iomem *isr_reg = NULL;
>>>>  	u32 isr;
>>>> -	unsigned int gpio_irq, gpio_index;
>>>> +	unsigned int i;
>>>>  	struct gpio_bank *bank;
>>>>  	int unmasked = 0;
>>>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>>>  		if (!isr)
>>>>  			break;
>>>>  
>>>> -		gpio_irq = bank->irq_base;
>>>> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
>>>> -			int gpio = irq_to_gpio(bank, gpio_irq);
>>>> -
>>>> +		for (i = 0; isr != 0; isr >>= 1, i++) {
>>>>  			if (!(isr & 1))
>>>>  				continue;
>>>
>>> this will iterate over all 32 GPIOs, a better way to handle this would
>>> be to have something like:
>>
>> Worse case, if only bit 31 was set then I agree this is not that
>> efficient. Or even if one bit is set. However, the loop itself will
>> iterate while isr != 0 so not always over each bit. No different to the
>> existing code.
>>
>>> while (isr) {
>>> 	unsigned long bit = __ffs(isr);
>>>
>>> 	/* clear this bit */
>>> 	isr &= ~bit;
>>>
>>> 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
>>> }
>>>
>>> this way you will only iterate the amount of bits enabled in the isr
>>> register.
>>
>> Definitely cleaner but I am wondering which approach would be more
>> efficient from an instruction standpoint. This could definitely be much
>> more efficient if there is only a couple bits set.
> 
> __ffs() is done with CLZ instruction, so it's pretty fast.

Ok, yes I see that now for ARMv5 onwards. Grant has pushed the patch,
but may be we can update this as an optimisation separately.

Thanks
Jon

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Jon Hunter]

On 03/04/2013 11:11 AM, Felipe Balbi wrote:
> On Mon, Mar 04, 2013 at 11:06:12AM -0600, Jon Hunter wrote:
>>
>> On 03/02/2013 05:48 AM, Felipe Balbi wrote:
>>> On Fri, Mar 01, 2013 at 11:22:47AM -0600, Jon Hunter wrote:
>>>> Currently the OMAP GPIO driver uses a legacy mapping for the GPIO IRQ
>>>> domain. This is not necessary because we do not need to assign a
>>>> specific interrupt number to the GPIO IRQ domain. Therefore, convert
>>>> the OMAP GPIO driver to use a linear mapping instead.
>>>>
>>>> Please note that this also allows to simplify the logic in the OMAP
>>>> gpio_irq_handler() routine, by using irq_find_mapping() to obtain the
>>>> virtual irq number from the GPIO bank and bank index.
>>>>
>>>> Reported-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>>
>>> Reviewed-by: Felipe Balbi <balbi@ti.com>
>>>
>>> Just one suggestion below for a later patch.
>>>
>>>> @@ -680,7 +686,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>>>  {
>>>>  	void __iomem *isr_reg = NULL;
>>>>  	u32 isr;
>>>> -	unsigned int gpio_irq, gpio_index;
>>>> +	unsigned int i;
>>>>  	struct gpio_bank *bank;
>>>>  	int unmasked = 0;
>>>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> @@ -721,15 +727,10 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>>>  		if (!isr)
>>>>  			break;
>>>>  
>>>> -		gpio_irq = bank->irq_base;
>>>> -		for (; isr != 0; isr >>= 1, gpio_irq++) {
>>>> -			int gpio = irq_to_gpio(bank, gpio_irq);
>>>> -
>>>> +		for (i = 0; isr != 0; isr >>= 1, i++) {
>>>>  			if (!(isr & 1))
>>>>  				continue;
>>>
>>> this will iterate over all 32 GPIOs, a better way to handle this would
>>> be to have something like:
>>
>> Worse case, if only bit 31 was set then I agree this is not that
>> efficient. Or even if one bit is set. However, the loop itself will
>> iterate while isr != 0 so not always over each bit. No different to the
>> existing code.
>>
>>> while (isr) {
>>> 	unsigned long bit = __ffs(isr);
>>>
>>> 	/* clear this bit */
>>> 	isr &= ~bit;
>>>
>>> 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
>>> }
>>>
>>> this way you will only iterate the amount of bits enabled in the isr
>>> register.
>>
>> Definitely cleaner but I am wondering which approach would be more
>> efficient from an instruction standpoint. This could definitely be much
>> more efficient if there is only a couple bits set.
> 
> __ffs() is done with CLZ instruction, so it's pretty fast.

Ok, yes I see that now for ARMv5 onwards. Grant has pushed the patch,
but may be we can update this as an optimisation separately.

Thanks
Jon

Re: [PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

Hi,

On Mon, Mar 04, 2013 at 11:58:23AM -0600, Jon Hunter wrote:
> >>> while (isr) {
> >>> 	unsigned long bit = __ffs(isr);
> >>>
> >>> 	/* clear this bit */
> >>> 	isr &= ~bit;
> >>>
> >>> 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
> >>> }
> >>>
> >>> this way you will only iterate the amount of bits enabled in the isr
> >>> register.
> >>
> >> Definitely cleaner but I am wondering which approach would be more
> >> efficient from an instruction standpoint. This could definitely be much
> >> more efficient if there is only a couple bits set.
> > 
> > __ffs() is done with CLZ instruction, so it's pretty fast.
> 
> Ok, yes I see that now for ARMv5 onwards. Grant has pushed the patch,
> but may be we can update this as an optimisation separately.

sure, that was my suggestion from the beginning :-) It's clearly subject
to a separate patch.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] gpio/omap: convert gpio irq domain to linear mapping

[Felipe Balbi]

Hi,

On Mon, Mar 04, 2013 at 11:58:23AM -0600, Jon Hunter wrote:
> >>> while (isr) {
> >>> 	unsigned long bit = __ffs(isr);
> >>>
> >>> 	/* clear this bit */
> >>> 	isr &= ~bit;
> >>>
> >>> 	generic_handle_irq(irq_find_mapping(bank->domain, bit);
> >>> }
> >>>
> >>> this way you will only iterate the amount of bits enabled in the isr
> >>> register.
> >>
> >> Definitely cleaner but I am wondering which approach would be more
> >> efficient from an instruction standpoint. This could definitely be much
> >> more efficient if there is only a couple bits set.
> > 
> > __ffs() is done with CLZ instruction, so it's pretty fast.
> 
> Ok, yes I see that now for ARMv5 onwards. Grant has pushed the patch,
> but may be we can update this as an optimisation separately.

sure, that was my suggestion from the beginning :-) It's clearly subject
to a separate patch.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130304/57368795/attachment-0001.sig>