arm/tegra: Remove use of TEGRA_GPIO_TO_IRQ

Enhance the driver to dynamically allocate the base IRQ number, and create an IRQ domain for itself. The use of an IRQ domain ensures that any device tree node interrupts properties are correctly parsed.

Fix the DT binding documentation to describe interrupt-related properties, and the contents of "child" node interrupts property.

Update tegra20.dtsi to specify the required interrupt-related properties.

Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer gives correct results since the IRQ numbers for GPIOs are dynamically allocated.

Signed-off-by Stephen Warren <[email protected]>

---
 .../devicetree/bindings/gpio/gpio_nvidia.txt       |   10 ++++++
 arch/arm/boot/dts/tegra20.dtsi                     |    2 +
 arch/arm/mach-tegra/include/mach/gpio-tegra.h      |    2 -
 drivers/gpio/gpio-tegra.c                          |   32 +++++++++++++++----
 4 files changed, 37 insertions(+), 9 deletions(-)

Documentation/devicetree/bindings/gpio/gpio_nvidia.txt +10 -0

index eb4b530..20b991d 100644
@@ -6,3 +6,13 @@ Required properties:
   second cell is used to specify optional parameters:
   - bit 0 specifies polarity (0 for normal, 1 for inverted)
 - gpio-controller : Marks the device node as a GPIO controller.
+- #interrupt-cells : Should be 2.
+  The first cell is the GPIO number.
+  The second cell is used to specify flags:
+    bits[3:0] trigger type and level flags:
+      1 = low-to-high edge triggered.
+      2 = high-to-low edge triggered.
+      4 = active high level-sensitive.
+      8 = active low level-sensitive.
+      Valid combinations are 1, 2, 3, 4, 8.
+- interrupt-controller : Marks the device node as an interrupt controller.

arch/arm/boot/dts/tegra20.dtsi +2 -0

index db6f562..e25f4a6 100644
@@ -79,6 +79,8 @@
 			       0 55 0x04
 			       0 87 0x04
 			       0 89 0x04 >;
+		interrupt-controller;
+		#interrupt-cells = <2>;
 		#gpio-cells = <2>;
 		gpio-controller;
 	};

arch/arm/mach-tegra/include/mach/gpio-tegra.h +0 -0

index 87d37fd..6140820 100644

arch/arm/mach-tegra/include/mach/gpio-tegra.h +0 -2

-#define TEGRA_GPIO_TO_IRQ(gpio) (INT_GPIO_BASE + (gpio))
-
 struct tegra_gpio_table {
 	int	gpio;	/* GPIO number */
 	bool	enable;	/* Enable for GPIO at init? */

drivers/gpio/gpio-tegra.c +8 -6

index 61044c8..9fa5783 100644
@@ -25,6 +25,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
+#include <linux/irqdomain.h>
 
 #include <asm/mach/irq.h>
 

@@ -74,7 +75,8 @@ struct tegra_gpio_bank {
 #endif
 };
 
-
+static struct irq_domain irq_domain;
+static struct irq_domain_ops irq_domain_ops;
 static void __iomem *regs;
 static struct tegra_gpio_bank tegra_gpio_banks[7];
 

@@ -139,7 +141,7 @@ static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
 
 static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	return TEGRA_GPIO_TO_IRQ(offset);
+	return irq_domain_to_irq(&irq_domain, offset);
 }
 
 static struct gpio_chip tegra_gpio_chip = {

@@ -155,28 +157,28 @@ static struct gpio_chip tegra_gpio_chip = {
 
 static void tegra_gpio_irq_ack(struct irq_data *d)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 
 	tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
 }
 
 static void tegra_gpio_irq_mask(struct irq_data *d)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 
 	tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 0);
 }
 
 static void tegra_gpio_irq_unmask(struct irq_data *d)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 
 	tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 1);
 }
 
 static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	int port = GPIO_PORT(gpio);
 	int lvl_type;

drivers/gpio/gpio-tegra.c +3 -1

+	irq_domain_add(&irq_domain);
+
 	for (i = 0; i < ARRAY_SIZE(tegra_gpio_banks); i++) {
 		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 		if (!res) {

@@ -388,7 +406,7 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 	gpiochip_add(&tegra_gpio_chip);
 
 	for (gpio = 0; gpio < TEGRA_NR_GPIOS; gpio++) {
-		int irq = TEGRA_GPIO_TO_IRQ(gpio);
+		int irq = irq_domain_to_irq(&irq_domain, gpio);
 		/* No validity check; all Tegra GPIOs are valid IRQs */
 
 		bank = &tegra_gpio_banks[GPIO_BANK(gpio)];

Quoted 14 lines · Stephen Warren

Enhance the driver to dynamically allocate the base IRQ number, and create an IRQ domain for itself. The use of an IRQ domain ensures that any device tree node interrupts properties are correctly parsed.

Fix the DT binding documentation to describe interrupt-related properties, and the contents of "child" node interrupts property.

Update tegra20.dtsi to specify the required interrupt-related properties.

Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer gives correct results since the IRQ numbers for GPIOs are dynamically allocated.

Signed-off-by Stephen Warren <[email protected]>

Quoted 11 lines

static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) {

drivers/gpio/gpio-tegra.c +1 -1

-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	int port = GPIO_PORT(gpio);
 	int lvl_type;

@@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 	int i;
 	int j;
 

drivers/gpio/gpio-tegra.c static int __devinit tegra_gpio_probe(struct platform_device *pdev) · line 354

Quoted 7 lines

drivers/gpio/gpio-tegra.c +7 -0

+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops = &irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

Quoted 15 lines · Stephen Warren

Quoted 14 lines

Enhance the driver to dynamically allocate the base IRQ number, and create an IRQ domain for itself. The use of an IRQ domain ensures that any device tree node interrupts properties are correctly parsed.

Fix the DT binding documentation to describe interrupt-related properties, and the contents of "child" node interrupts property.

Update tegra20.dtsi to specify the required interrupt-related properties.

Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer gives correct results since the IRQ numbers for GPIOs are dynamically allocated.

Signed-off-by Stephen Warren <[email protected]>

[...]

Quoted 11 lines

Quoted 11 lines

static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) {

drivers/gpio/gpio-tegra.c +1 -1

-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	int port = GPIO_PORT(gpio);
 	int lvl_type;

@@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 	int i;
 	int j;
 

Quoted 9 lines

drivers/gpio/gpio-tegra.c static int __devinit tegra_gpio_probe(struct platform_device *pdev) · line 354

Quoted 7 lines

drivers/gpio/gpio-tegra.c +7 -0

+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops = &irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

This would need the patch I posted earlier
(https://lkml.org/lkml/2011/12/1/109) so they can work for the !CONFIG_OF case ;-)

Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:

Quoted 38 lines · Rob Herring

Quoted 15 lines · Stephen Warren

Quoted 14 lines

Enhance the driver to dynamically allocate the base IRQ number, and create an IRQ domain for itself. The use of an IRQ domain ensures that any device tree node interrupts properties are correctly parsed.

Fix the DT binding documentation to describe interrupt-related properties, and the contents of "child" node interrupts property.

Update tegra20.dtsi to specify the required interrupt-related properties.

Finally, remove the definition of TEGRA_GPIO_TO_IRQ; this macro no longer gives correct results since the IRQ numbers for GPIOs are dynamically allocated.

Signed-off-by Stephen Warren <[email protected]>

[...]

Quoted 11 lines

Quoted 11 lines

static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) {

drivers/gpio/gpio-tegra.c +1 -1

-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;
 	struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	int port = GPIO_PORT(gpio);
 	int lvl_type;

@@ -343,6 +345,22 @@ static int __devinit tegra_gpio_probe(struct platform_device *pdev)
 	int i;
 	int j;
 

Quoted 9 lines

drivers/gpio/gpio-tegra.c static int __devinit tegra_gpio_probe(struct platform_device *pdev) · line 354

Quoted 7 lines

drivers/gpio/gpio-tegra.c +7 -0

+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops = &irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

Quoted 3 lines

This would need the patch I posted earlier
(https://lkml.org/lkml/2011/12/1/109) so they can work for the !CONFIG_OF case ;-)

Part of my reasoning was that simple_ops appeared to be intended for DT-based controllers; is it safe to use those ops for a controller that wasn't instantiated from DT? I know that right now, the only op in that structure is dt_translate, and that won't ever be called for the non-DT case, but is there a guarantee that more functions won't be added to the simple ops, and that they won't assume DT is in use, and fail if not?

If these are safe to use in the non-DT case, then yet I could build off Jamie's patch, although managing the dependencies might be awkward.

Quoted 3 lines

Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:

Quoted 2 lines · Rob Herring

Quoted 1 line · Stephen Warren

Quoted 22 lines

Quoted 10 lines

Quoted 9 lines

drivers/gpio/gpio-tegra.c static int __devinit tegra_gpio_probe(struct platform_device *pdev) · line 354

Quoted 7 lines

drivers/gpio/gpio-tegra.c +7 -0

+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops = &irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

Quoted 3 lines

This would need the patch I posted earlier
(https://lkml.org/lkml/2011/12/1/109) so they can work for the !CONFIG_OF case ;-)

Part of my reasoning was that simple_ops appeared to be intended for DT-based controllers; is it safe to use those ops for a controller that wasn't instantiated from DT? I know that right now, the only op in that structure is dt_translate, and that won't ever be called for the non-DT case, but is there a guarantee that more functions won't be added to the simple ops, and that they won't assume DT is in use, and fail if not?

Quoted 2 lines

If these are safe to use in the non-DT case, then yet I could build off Jamie's patch, although managing the dependencies might be awkward.

Yes, it's absolutely fine to use it just that irq_simple_ops isn't currently exported unless you have CONFIG_OF_IRQ set so you'd get an undefined reference for !CONFIG_OF at the moment.

Jamie Iles wrote at Thursday, December 01, 2011 9:55 AM:

Quoted 4 lines · Stephen Warren

Quoted 3 lines

Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:

Quoted 2 lines · Rob Herring

Quoted 1 line · Stephen Warren

...

Quoted 22 lines

Quoted 22 lines

Quoted 10 lines

Quoted 9 lines

drivers/gpio/gpio-tegra.c static int __devinit tegra_gpio_probe(struct platform_device *pdev)

Quoted 7 lines

drivers/gpio/gpio-tegra.c +7 -0

+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base<  0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops =&irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

Quoted 3 lines

This would need the patch I posted earlier
(https://lkml.org/lkml/2011/12/1/109) so they can work for the !CONFIG_OF case ;-)

Part of my reasoning was that simple_ops appeared to be intended for DT-based controllers; is it safe to use those ops for a controller that wasn't instantiated from DT? I know that right now, the only op in that structure is dt_translate, and that won't ever be called for the non-DT case, but is there a guarantee that more functions won't be added to the simple ops, and that they won't assume DT is in use, and fail if not?

Quoted 6 lines

Quoted 2 lines

If these are safe to use in the non-DT case, then yet I could build off Jamie's patch, although managing the dependencies might be awkward.

Yes, it's absolutely fine to use it just that irq_simple_ops isn't currently exported unless you have CONFIG_OF_IRQ set so you'd get an undefined reference for !CONFIG_OF at the moment.

OK, sounds good.

So, I think we have a few options:
1) Merge my change as-is, and simplify it later once your patch is in.
2) Put your change in a branch, and merge it into both its usual place, and the Tegra/ARM branches, so I can rebase my patch on top of it.
3) Have the usual maintainer ack it (I see Rob already did, but I think Thomas is the maintainer here right?) and just put both patches into the Tegra/ARM tree. This of course means non-Tegra branches have to wait for it rather than the other way around.

(1) seems simplest, but (2) is probably doable. Thomas?

On 12/01/2011 09:57 PM, Stephen Warren :

Quoted 46 lines

Jamie Iles wrote at Thursday, December 01, 2011 9:55 AM:

Quoted 4 lines · Stephen Warren

Quoted 3 lines

Jamie Iles wrote at Thursday, December 01, 2011 7:11 AM:

Quoted 2 lines · Rob Herring

Quoted 1 line · Stephen Warren

...

Quoted 22 lines

Quoted 22 lines

Quoted 10 lines

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	irq_domain.irq_base = irq_alloc_descs(-1, 0, TEGRA_NR_GPIOS, 0);
+	if (irq_domain.irq_base<  0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+	irq_domain.nr_irq = TEGRA_NR_GPIOS;
+	irq_domain.ops =&irq_domain_ops;

Why don't you just use irq_domain_simple_ops?

Quoted 3 lines

This would need the patch I posted earlier
(https://lkml.org/lkml/2011/12/1/109) so they can work for the !CONFIG_OF case ;-)

Part of my reasoning was that simple_ops appeared to be intended for DT-based controllers; is it safe to use those ops for a controller that wasn't instantiated from DT? I know that right now, the only op in that structure is dt_translate, and that won't ever be called for the non-DT case, but is there a guarantee that more functions won't be added to the simple ops, and that they won't assume DT is in use, and fail if not?

Quoted 6 lines

Quoted 2 lines

If these are safe to use in the non-DT case, then yet I could build off Jamie's patch, although managing the dependencies might be awkward.

Yes, it's absolutely fine to use it just that irq_simple_ops isn't currently exported unless you have CONFIG_OF_IRQ set so you'd get an undefined reference for !CONFIG_OF at the moment.

OK, sounds good.

So, I think we have a few options:
1) Merge my change as-is, and simplify it later once your patch is in.
2) Put your change in a branch, and merge it into both its usual place, and the Tegra/ARM branches, so I can rebase my patch on top of it.
3) Have the usual maintainer ack it (I see Rob already did, but I think Thomas is the maintainer here right?) and just put both patches into the Tegra/ARM tree. This of course means non-Tegra branches have to wait for it rather than the other way around.

Quoted 1 line

(1) seems simplest, but (2) is probably doable. Thomas?

I jump into the discussion to say that I am also interested by Jamie's patch. I am following the same path as Stephen at the moment with Atmel AT91... A chance I can read all your comments that are so valuable for me as well :-)

So, for me (2) will ease things...

tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
}

On Wed, Nov 30, 2011 at 05:45:36PM -0700, Stephen Warren wrote:
[...]

Review

Quoted 5 lines

static void tegra_gpio_irq_ack(struct irq_data *d) {

(quoted diff) +1 -1

-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;

Though it's working right now, I'm not sure it's safe enough. This only works when d->hwirq_base is 0, which is true for now. But I doubt it will be always true. I guess hwirq_base was introduced there for some reason. When some day irqdomain starts using this field, the above code starts being broken. IMO, the way that generic-chip.c is using to calculate the number, d->irq - gc->irq_base, is much more safer.

Shawn,

Quoted 14 lines · Shawn Guo

On Wed, Nov 30, 2011 at 05:45:36PM -0700, Stephen Warren wrote:
[...]

Review

Quoted 5 lines

static void tegra_gpio_irq_ack(struct irq_data *d) {

(quoted diff) +1 -1

-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;

Though it's working right now, I'm not sure it's safe enough. This only works when d->hwirq_base is 0, which is true for now. But I doubt it will be always true. I guess hwirq_base was introduced there for some reason. When some day irqdomain starts using this field, the above code starts being broken. IMO, the way that generic-chip.c is using to calculate the number, d->irq - gc->irq_base, is much more safer.

It does work as the GIC hwirq_base is non-zero. It was introduced exactly so that no conversion of hwirq is needed for functions like this. hwirq_base is the starting point local to the controller numbering. Say you have gpio controller with 16 lines, but only the upper 8 lines have interrupt capability. Then you would set hwirq_base to 8 and nr_irq to 8. Then hwirq will always be set to 8-15.

[...]

Review

Quoted 5 lines

static void tegra_gpio_irq_ack(struct irq_data *d) {

(quoted diff) +1 -1

-	int gpio = d->irq - INT_GPIO_BASE;
+	int gpio = d->hwirq;

Though it's working right now, I'm not sure it's safe enough. This only works when d->hwirq_base is 0, which is true for now. But I doubt it will be always true. I guess hwirq_base was introduced there for some reason. When some day irqdomain starts using this field, the above code starts being broken. IMO, the way that generic-chip.c is using to calculate the number, d->irq - gc->irq_base, is much more safer.