[PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

When a GPIO is defined as an interrupt line using Device
Tree, a call to irq_create_of_mapping() is made that calls
irq_create_mapping(). So, is not necessary to do the mapping
for all OMAP GPIO lines and explicitly call irq_create_mapping()
on the driver probe() when booting with Device Tree.

Add a custom IRQ domain .map function handler that will be
called by irq_create_mapping() to map the GPIO lines used as IRQ.
This also allows to execute needed setup code such as configuring
a GPIO as input and enabling the GPIO bank.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---

Changes since v3:
  - Use bank->chip.of_node instead of_have_populated_dt() to check
    DT or legacy boot as suggested by Jean-Christophe PLAGNIOL-VILLARD

Changes since v2:
  - Unconditionally do the IRQ setup in the .map() function and
    only call irq_create_mapping() in the gpio chip init to avoid
    code duplication as suggested by Grant Likely.

Changes since v1:
  - Split the addition of the .map function handler and the
    automatic gpio request in two different patches.
  - Add GPIO IRQ setup logic to the irq domain mapping function.
  - Only call irq_create_mapping for every GPIO on legacy boot.
  - Only setup a GPIO IRQ on the .map function for DeviceTree boot.

 drivers/gpio/gpio-omap.c |   54 ++++++++++++++++++++++++++++++++++------------
 1 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4a43036..e260590 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1068,24 +1068,50 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 
 	gpiochip_add(&bank->chip);
 
-	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, irq, bank->width);
-		} else {
-			irq_set_chip_and_handler(irq, &gpio_irq_chip,
-						 handle_simple_irq);
-			set_irq_flags(irq, IRQF_VALID);
-		}
-	}
+	/*
+	 * REVISIT these explicit calls to irq_create_mapping()
+	 * to do the GPIO to IRQ domain mapping for each GPIO in
+	 * the bank can be removed once all OMAP platforms have
+	 * been migrated to Device Tree boot only.
+	 * Since in DT boot irq_create_mapping() is called from
+	 * irq_create_of_mapping() only for the GPIO lines that
+	 * are used as interrupts.
+	 */
+	if (!bank->chip.of_node)
+		for (j = 0; j < bank->width; j++)
+			irq_create_mapping(bank->domain, j);
 	irq_set_chained_handler(bank->irq, gpio_irq_handler);
 	irq_set_handler_data(bank->irq, bank);
 }
 
 static const struct of_device_id omap_gpio_match[];
 
+static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq,
+			     irq_hw_number_t hwirq)
+{
+	struct gpio_bank *bank = d->host_data;
+
+	if (!bank)
+		return -EINVAL;
+
+	irq_set_lockdep_class(virq, &gpio_lock_class);
+	irq_set_chip_data(virq, bank);
+	if (bank->is_mpuio) {
+		omap_mpuio_alloc_gc(bank, virq, bank->width);
+	} else {
+		irq_set_chip_and_handler(virq, &gpio_irq_chip,
+					 handle_simple_irq);
+		set_irq_flags(virq, IRQF_VALID);
+	}
+
+	return 0;
+}
+
+static struct irq_domain_ops omap_gpio_irq_ops = {
+	.xlate  = irq_domain_xlate_onetwocell,
+	.map    = omap_gpio_irq_map,
+};
+
 static int omap_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1151,10 +1177,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	}
 
 	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
-					     0, &irq_domain_simple_ops, NULL);
+					     0, &omap_gpio_irq_ops, bank);
 #else
 	bank->domain = irq_domain_add_linear(node, bank->width,
-					     &irq_domain_simple_ops, NULL);
+					     &omap_gpio_irq_ops, bank);
 #endif
 	if (!bank->domain) {
 		dev_err(dev, "Couldn't register an IRQ domain\n");
-- 
1.7.7.6

[PATCH v4 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT

[Javier Martinez Canillas]

When an OMAP GPIO is used as an IRQ line, a call to gpio_request()
has to be made to initialize the OMAP GPIO bank before a driver
request the IRQ. Otherwise the call to request_irq() fails.

Drives should not be aware of this neither care wether an IRQ line
is a GPIO or not. They should just request the IRQ and this has to
be handled by the irq_chip driver.

With the current OMAP GPIO DT binding, if we define:

    gpio6: gpio@49058000 {
    	   compatible = "ti,omap3-gpio";
	   reg = <0x49058000 0x200>;
	   interrupts = <34>;
	   ti,hwmods = "gpio6";
	   gpio-controller;
	   #gpio-cells = <2>;
	   interrupt-controller;
	   #interrupt-cells = <2>;
    };

	   interrupt-parent = <&gpio6>;
           interrupts = <16 8>;

The GPIO is correctly mapped as an IRQ but a call to gpio_request()
is never made. Since a call to the custom IRQ domain .map function
handler is made for each GPIO used as an IRQ, the GPIO can be setup
and configured as input there automatically.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---

Changes since v3:
  - Use bank->chip.of_node instead of_have_populated_dt() to check
    DT or legacy boot as suggested by Jean-Christophe PLAGNIOL-VILLARD
  - Add a comment that this is just a temporary solution until and
    that it has to be removed once is handled by the IRQ core.

Changes since v2:
 - Only make the call to gpio_request_one() conditional in the DT
   case as suggested by Grant Likely.

Changes since v1:
  - Split the irq domain mapping function handler and the GPIO
    request in two different patches.

 drivers/gpio/gpio-omap.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e260590..c4f0aa2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1090,6 +1090,8 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq,
 			     irq_hw_number_t hwirq)
 {
 	struct gpio_bank *bank = d->host_data;
+	int gpio;
+	int ret;
 
 	if (!bank)
 		return -EINVAL;
@@ -1104,6 +1106,22 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq,
 		set_irq_flags(virq, IRQF_VALID);
 	}
 
+	/*
+	 * REVISIT most GPIO IRQ chip drivers need to call
+	 * gpio_request() before a GPIO line can be used as an
+	 * IRQ. Ideally this should be handled by the IRQ core
+	 * but until then this has to be done on a per driver
+	 * basis. Remove this once this is managed by the core.
+	 */
+	if (bank->chip.of_node) {
+		gpio = irq_to_gpio(bank, hwirq);
+		ret = gpio_request_one(gpio, GPIOF_IN, NULL);
+		if (ret) {
+			dev_err(bank->dev, "Could not request GPIO%d\n", gpio);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
-- 
1.7.7.6

Re: [PATCH v4 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT

[Santosh Shilimkar]

On Friday 28 June 2013 11:27 AM, Javier Martinez Canillas wrote:
> When an OMAP GPIO is used as an IRQ line, a call to gpio_request()
> has to be made to initialize the OMAP GPIO bank before a driver
> request the IRQ. Otherwise the call to request_irq() fails.
> 
> Drives should not be aware of this neither care wether an IRQ line
> is a GPIO or not. They should just request the IRQ and this has to
> be handled by the irq_chip driver.
> 
> With the current OMAP GPIO DT binding, if we define:
> 
>     gpio6: gpio@49058000 {
>     	   compatible = "ti,omap3-gpio";
> 	   reg = <0x49058000 0x200>;
> 	   interrupts = <34>;
> 	   ti,hwmods = "gpio6";
> 	   gpio-controller;
> 	   #gpio-cells = <2>;
> 	   interrupt-controller;
> 	   #interrupt-cells = <2>;
>     };
> 
> 	   interrupt-parent = <&gpio6>;
>            interrupts = <16 8>;
> 
> The GPIO is correctly mapped as an IRQ but a call to gpio_request()
> is never made. Since a call to the custom IRQ domain .map function
> handler is made for each GPIO used as an IRQ, the GPIO can be setup
> and configured as input there automatically.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
> 
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Santosh Shilimkar]

On Friday 28 June 2013 11:27 AM, Javier Martinez Canillas wrote:
> When a GPIO is defined as an interrupt line using Device
> Tree, a call to irq_create_of_mapping() is made that calls
> irq_create_mapping(). So, is not necessary to do the mapping
> for all OMAP GPIO lines and explicitly call irq_create_mapping()
> on the driver probe() when booting with Device Tree.
> 
> Add a custom IRQ domain .map function handler that will be
> called by irq_create_mapping() to map the GPIO lines used as IRQ.
> This also allows to execute needed setup code such as configuring
> a GPIO as input and enabling the GPIO bank.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
> 
Thanks for the patches.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Fri, Jun 28, 2013 at 5:27 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:

> When a GPIO is defined as an interrupt line using Device
> Tree, a call to irq_create_of_mapping() is made that calls
> irq_create_mapping(). So, is not necessary to do the mapping
> for all OMAP GPIO lines and explicitly call irq_create_mapping()
> on the driver probe() when booting with Device Tree.
>
> Add a custom IRQ domain .map function handler that will be
> called by irq_create_mapping() to map the GPIO lines used as IRQ.
> This also allows to execute needed setup code such as configuring
> a GPIO as input and enabling the GPIO bank.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>
> Changes since v3:
>   - Use bank->chip.of_node instead of_have_populated_dt() to check
>     DT or legacy boot as suggested by Jean-Christophe PLAGNIOL-VILLARD

This does not apply to the "next" branch on my GPIO tree,
i.e:
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/

Not even with fuzzing :-(

Can you rebase them?

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On Sun, Jun 30, 2013 at 1:44 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jun 28, 2013 at 5:27 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>
>> When a GPIO is defined as an interrupt line using Device
>> Tree, a call to irq_create_of_mapping() is made that calls
>> irq_create_mapping(). So, is not necessary to do the mapping
>> for all OMAP GPIO lines and explicitly call irq_create_mapping()
>> on the driver probe() when booting with Device Tree.
>>
>> Add a custom IRQ domain .map function handler that will be
>> called by irq_create_mapping() to map the GPIO lines used as IRQ.
>> This also allows to execute needed setup code such as configuring
>> a GPIO as input and enabling the GPIO bank.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
>> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> ---
>>
>> Changes since v3:
>>   - Use bank->chip.of_node instead of_have_populated_dt() to check
>>     DT or legacy boot as suggested by Jean-Christophe PLAGNIOL-VILLARD
>
> This does not apply to the "next" branch on my GPIO tree,
> i.e:
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/
>
> Not even with fuzzing :-(
>
> Can you rebase them?
>
> Yours,
> Linus Walleij
> --

Hi Linus,

Yes, It doesn't apply cleanly to your "next" branch cleanly because
this patch-set depends on the following bugfix patch merged late on
the -rc cycle (3.10-rc7):

397eada9 ("gpio/omap: don't use linear domain mapping for OMAP1")

but your next branch is based on Linux 3.10-rc3.

So, I could change the patches so they can be applied cleanly on your
branch but then it will not apply cleanly when you send your pull
request to Torvalds.

Thanks a lot and best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jun 30, 2013 at 2:25 AM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:

> Yes, It doesn't apply cleanly to your "next" branch cleanly because
> this patch-set depends on the following bugfix patch merged late on
> the -rc cycle (3.10-rc7):
>
> 397eada9 ("gpio/omap: don't use linear domain mapping for OMAP1")

Aha, well this fix was only CC:ed to Grant so I never saw it
happen. Obviously it cannot be merged through my GPIO tree
then.

> So, I could change the patches so they can be applied cleanly on your
> branch but then it will not apply cleanly when you send your pull
> request to Torvalds.

That will probably not work as it would cause even more conflicts
upstream, and now the merge window is open so no way can I
rebase the tree either.

Let's see if we can cram it in as part of a late v3.11 merge or
if we'll have to defer to v3.12.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On 01/07/2013, at 10:04, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sun, Jun 30, 2013 at 2:25 AM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
> 
>> Yes, It doesn't apply cleanly to your "next" branch cleanly because
>> this patch-set depends on the following bugfix patch merged late on
>> the -rc cycle (3.10-rc7):
>> 
>> 397eada9 ("gpio/omap: don't use linear domain mapping for OMAP1")
> 
> Aha, well this fix was only CC:ed to Grant so I never saw it
> happen. Obviously it cannot be merged through my GPIO tree
> then.
> 

Yes, sorry about that. I didn't do a proper post of that patch but it was proposed on a very long thread [1] about a regression for GPIO in OMAP1 platforms.

Then Tony took the patch and send a pull request to Grant. I just saw you were not cc'ed.

>> So, I could change the patches so they can be applied cleanly on your
>> branch but then it will not apply cleanly when you send your pull
>> request to Torvalds.
> 
> That will probably not work as it would cause even more conflicts
> upstream, and now the merge window is open so no way can I
> rebase the tree either.
> 
> Let's see if we can cram it in as part of a late v3.11 merge or
> if we'll have to defer to v3.12.
> 

Ok no worries. Since these patches fixes an actual issue (using a GPIO as IRQ) I guess they still can be sent as part of the v3.11 -rc cycle gpio fixes.

> Yours,
> Linus Walleij

Thanks a lot and best regards,
Javier

[1]: http://comments.gmane.org/gmane.linux.ports.arm.kernel/236198

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Mon, Jul 1, 2013 at 1:01 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> On 01/07/2013, at 10:04, Linus Walleij <linus.walleij@linaro.org> wrote:

>> Let's see if we can cram it in as part of a late v3.11 merge or
>> if we'll have to defer to v3.12.
>
> Ok no worries. Since these patches fixes an actual issue
> (using a GPIO as IRQ) I guess they still can be sent as
> part of the v3.11 -rc cycle gpio fixes.

Yes of course, we'll fix that up.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Grant Likely]

On Mon, Jul 1, 2013 at 9:04 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Jun 30, 2013 at 2:25 AM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
>
>> Yes, It doesn't apply cleanly to your "next" branch cleanly because
>> this patch-set depends on the following bugfix patch merged late on
>> the -rc cycle (3.10-rc7):
>>
>> 397eada9 ("gpio/omap: don't use linear domain mapping for OMAP1")
>
> Aha, well this fix was only CC:ed to Grant so I never saw it
> happen. Obviously it cannot be merged through my GPIO tree
> then.
>
>> So, I could change the patches so they can be applied cleanly on your
>> branch but then it will not apply cleanly when you send your pull
>> request to Torvalds.
>
> That will probably not work as it would cause even more conflicts
> upstream, and now the merge window is open so no way can I
> rebase the tree either.
>
> Let's see if we can cram it in as part of a late v3.11 merge or
> if we'll have to defer to v3.12.

Linus, you can actually merge in the final v3.10 tag into your
gpio-next branch and then apply on top of that. I would send it as two
separate pull requests though. One pull with all the stuff that was in
linux-next, and a second that includes the v3.10 merge and the OMAP
patches. Include an explaination in that pull request that it is an
important bug fix, but it took a bit of time to get it worked out
correctly; hence the reason for the merge. That will let Linus T make
a decision about whether or not to merge it without affecting the bulk
of the gpio changes.

g.

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Mon, Jul 1, 2013 at 2:49 PM, Grant Likely <grant.likely@linaro.org> wrote:

> Linus, you can actually merge in the final v3.10 tag into your
> gpio-next branch and then apply on top of that. I would send it as two
> separate pull requests though. One pull with all the stuff that was in
> linux-next, and a second that includes the v3.10 merge and the OMAP
> patches.

The first is already done :-D

I'll queue the OMAP stuff on a branch based off v3.10 sharp and
put into -next right now so we can propose it for fixes or as a
second pull request during the merge window.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.06.2013 17:27, schrieb Javier Martinez Canillas:
> When a GPIO is defined as an interrupt line using Device
> Tree, a call to irq_create_of_mapping() is made that calls
> irq_create_mapping(). So, is not necessary to do the mapping
> for all OMAP GPIO lines and explicitly call irq_create_mapping()
> on the driver probe() when booting with Device Tree.
>
> Add a custom IRQ domain .map function handler that will be
> called by irq_create_mapping() to map the GPIO lines used as IRQ.
> This also allows to execute needed setup code such as configuring
> a GPIO as input and enabling the GPIO bank.

This patch basically broke every usage of

irq = gpio_to_irq(gpio);
request[_threaded]_irq(irq, ...);

because request[_threaded]_irq(irq, ...) now fails because of a missing 
irq_domain (no mapping => no domain).

A prominent victim of this is the omap_hsmmc driver with the cd-gpio 
option enabled. To reproduce it, just add "cd-gpios = <&gpio0 6 
GPIO_ACTIVE_HIGH>;" (or similiar) to the omap-mmc part in the DT.

I don't know what else might fail, but I would assume there are still 
many other places which do request an IRQ for a GPIO with the above, now 
failing, sequence.

Regards,

Alexander

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 12:58 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 28.06.2013 17:27, schrieb Javier Martinez Canillas:
>
>> When a GPIO is defined as an interrupt line using Device
>> Tree, a call to irq_create_of_mapping() is made that calls
>> irq_create_mapping(). So, is not necessary to do the mapping
>> for all OMAP GPIO lines and explicitly call irq_create_mapping()
>> on the driver probe() when booting with Device Tree.
>>
>> Add a custom IRQ domain .map function handler that will be
>> called by irq_create_mapping() to map the GPIO lines used as IRQ.
>> This also allows to execute needed setup code such as configuring
>> a GPIO as input and enabling the GPIO bank.
>
>
> This patch basically broke every usage of
>
> irq = gpio_to_irq(gpio);
> request[_threaded]_irq(irq, ...);
>
> because request[_threaded]_irq(irq, ...) now fails because of a missing
> irq_domain (no mapping => no domain).

OK and I had ACKs from Santosh and even a Test-by tag from Enric
on that thing.

What shall we do with this mess now?

I was told that these patches really needed to be applied because
they were fixing a regression in v3.11, and now it seems they are
causing another regression.

What I need to know is what is worst: having these three patches
there or reverting them?

Or should I simply revert *all* the TI GPIO stuff merged for v3.11
now that is seems like this is a can of worms? :-/

Tony, Kevin, Santosh, someone? What will make all happy?

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 13:14, schrieb Linus Walleij:
> On Sun, Jul 28, 2013 at 12:58 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 28.06.2013 17:27, schrieb Javier Martinez Canillas:
>>
>>> When a GPIO is defined as an interrupt line using Device
>>> Tree, a call to irq_create_of_mapping() is made that calls
>>> irq_create_mapping(). So, is not necessary to do the mapping
>>> for all OMAP GPIO lines and explicitly call irq_create_mapping()
>>> on the driver probe() when booting with Device Tree.
>>>
>>> Add a custom IRQ domain .map function handler that will be
>>> called by irq_create_mapping() to map the GPIO lines used as IRQ.
>>> This also allows to execute needed setup code such as configuring
>>> a GPIO as input and enabling the GPIO bank.
>>
>>
>> This patch basically broke every usage of
>>
>> irq = gpio_to_irq(gpio);
>> request[_threaded]_irq(irq, ...);
>>
>> because request[_threaded]_irq(irq, ...) now fails because of a missing
>> irq_domain (no mapping => no domain).
> 
> OK and I had ACKs from Santosh and even a Test-by tag from Enric
> on that thing.
> 
> What shall we do with this mess now?
> 
> I was told that these patches really needed to be applied because
> they were fixing a regression in v3.11, and now it seems they are
> causing another regression.
> 
> What I need to know is what is worst: having these three patches
> there or reverting them?
> 
> Or should I simply revert *all* the TI GPIO stuff merged for v3.11
> now that is seems like this is a can of worms? :-/
> 
> Tony, Kevin, Santosh, someone? What will make all happy?

Hmm, maybe something which adds a mapping for a IRQ when gpio_to_irq()
is called would help.

I've just reverted the 3 patches on that topic because I was to lazy to
find out how to add a mapping for an irq in gpio_to_irq().

And just in case, I've only seen a breakage with that cd-signal for MMC
and nothing else. But I haven't really tested 3.11-rc much and only just
added that cd-gpio to my DT which is the reason I've seen the problem.
And my stripped down kernel doesn't seem to use a gpio-irq somewhere else.

But nevertheless, I assume it is a problem.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 2:59 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 28.07.2013 13:14, schrieb Linus Walleij:

>> What shall we do with this mess now?
(...)
> Hmm, maybe something which adds a mapping for a IRQ when gpio_to_irq()
> is called would help.

I think this bug is pointing back to my initial remark that
we need to flag, in the DT node for each GPIOchip
that provides "auto-IRQs" exactly which IRQ lines that
will be used as simple IRQ lines. This is what I called
"GPIO hogs" a while back.

If this had been done, gpio_request_one() would
*only* have been called on the GPIOs marked as
being used for IRQs simply. And we wouldn't have
had this bug.

I am leaning towards reverting the whole shebang
right now, and using that as an argument to hammer
in my opinion that we need to do it my way unless
something incredibly clever comes up.

Yours,
Linus Walleij

RE: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Shilimkar, Santosh]

(Sorry for the  top posting)

Linus,
Auto request GPIO as an input line when used as IRQ line for DT case has been broken and we did
discuss the issue on couple of threads. Javier's intermediate approach with $subject series was addressing
this long standing issue. But while fixing that case,  unfortunately breaks another case as highlighted
by Alexander. I think the default OMAP DT files will continue to work with these patches applied and
mostly doesn't break anything in default configuration. Ofcourse with the DT modification as done
by Alexander will expose the issue.

So either case we will something broken. I really wanted to have the auto request GPIO supported
when used as IRQ line but surely not at expense of breaking the client drivers. So I am fine 
with whatever you decide.

Regards,
Santosh
________________________________________
From: Linus Walleij [linus.walleij@linaro.org]
Sent: Sunday, July 28, 2013 10:11 AM
To: Alexander Holler
Cc: ext Tony Lindgren; Grant Likely; Shilimkar, Santosh; Kevin Hilman; Javier Martinez Canillas; Jon Hunter; Jean-Christophe PLAGNIOL-VILLARD; Enric Balletbo Serra; Linux-OMAP; Florian Vaussard; Aaro Koskinen; Krishnamoorthy, Balaji T
Subject: Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

On Sun, Jul 28, 2013 at 2:59 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 28.07.2013 13:14, schrieb Linus Walleij:

>> What shall we do with this mess now?
(...)
> Hmm, maybe something which adds a mapping for a IRQ when gpio_to_irq()
> is called would help.

I think this bug is pointing back to my initial remark that
we need to flag, in the DT node for each GPIOchip
that provides "auto-IRQs" exactly which IRQ lines that
will be used as simple IRQ lines. This is what I called
"GPIO hogs" a while back.

If this had been done, gpio_request_one() would
*only* have been called on the GPIOs marked as
being used for IRQs simply. And we wouldn't have
had this bug.

I am leaning towards reverting the whole shebang
right now, and using that as an argument to hammer
in my opinion that we need to do it my way unless
something incredibly clever comes up.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 4:37 PM, Shilimkar, Santosh
<santosh.shilimkar@ti.com> wrote:

> I think the default OMAP DT files will continue to work with
>  these patches applied and mostly doesn't break anything
> in default configuration.

What does "mostly" mean? Hm hm. OK I feel a little
bit better about this now...

> Ofcourse with the DT modification as done
> by Alexander will expose the issue.

So this is all caused by non-upstream code or
non-upstream DTS files?

> I really wanted to have the auto request GPIO supported
> when used as IRQ line but surely not at expense of
> breaking the client drivers.

If things are working for the default DTS files in the
kernel then I am OK with it.

However moving forward to support the cases that
Alexander is highlighting, I really really thin we need
to put the information about taken GPIOs "input-hogs"
into the gpio DT node.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 18:29, schrieb Linus Walleij:
> On Sun, Jul 28, 2013 at 4:37 PM, Shilimkar, Santosh
> <santosh.shilimkar@ti.com> wrote:
> 
>> I think the default OMAP DT files will continue to work with
>>  these patches applied and mostly doesn't break anything
>> in default configuration.
> 
> What does "mostly" mean? Hm hm. OK I feel a little
> bit better about this now...
> 
>> Ofcourse with the DT modification as done
>> by Alexander will expose the issue.
> 
> So this is all caused by non-upstream code or
> non-upstream DTS files?
> 
>> I really wanted to have the auto request GPIO supported
>> when used as IRQ line but surely not at expense of
>> breaking the client drivers.
> 
> If things are working for the default DTS files in the
> kernel then I am OK with it.

Sorry, but that isn't and can't be a solution. Most upstream DTS files
are just for a small number of developer boards and DT got introduced to
support a lot of boards while only providing a few DTs.

Having a quick look git grep gpio_to_irq, I've quickly spotted e.g. this
in drivers/input/keyboard/gpio_keys.c:

                irq = gpio_to_irq(button->gpio);
                if (irq < 0) {

(again <0 not <=0)

and I assume that gpio-keys now fails too.

But I haven't tested it (yet).

Maybe my dts entry is wrong or incomplete, but I don't see a way how to
define the irq-domain besides the one which is already defined by the
definition of the gpio. And I don't see why that should be necessary, in
the dts the domain is already defined, it just (now) isn't used anymore.

Besides that, I feel free to ignore me. I don't know what the new
patches do fix and I don't really have a need for a solution.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 7:13 PM, Alexander Holler <holler@ahsoftware.de> wrote:

> Having a quick look git grep gpio_to_irq, I've quickly spotted e.g. this
> in drivers/input/keyboard/gpio_keys.c:
>
>                 irq = gpio_to_irq(button->gpio);
>                 if (irq < 0) {
>
> (again <0 not <=0)

Hm I think this should atleast be augmented to spit a warning if
IRQ 0 is returned.

> Besides that, I feel free to ignore me. I don't know what the new
> patches do fix and I don't really have a need for a solution.

No one shall be ignored because I expect the OMAP GPIO
maintainers to have very clear answers to how they want to
proceed with this, and the issue with gpio_to_irq() *is* complex
so we shall indeed discuss it. Better to have the discussion
now and not when it gets deployed in a million systems.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

Hello,

Being the author of these patches I'm ashamed that they are causing a
regression. I did my best to make sure that it would work on OMAP1 and
OMAP2+ platforms booting with both legacy board files and DT.

Also, I didn't come with this solution in a vacuum. This was deeply
discussed with Jon Hunter, Grant Likely, Jean-Christophe
PLAGNIOL-VILLARD and others on a 90+ emails linux-omap thread [1] and
over IRC in #linaro-kernel channel.

The final agreement was that when a GPIO line is mapped in the IRQ
domain with irq_create_of_mapping(), the core has to take care to
request the GPIO and configure it as input.

But until we have this general solution we have to do it on a per irq
chip driver basis and the less hack-ish solution is to have a custom
.map function handler that request the GPIO used as IRQ.

Now, this assumes that irq_create_of_mapping() will always be called
for all GPIO lines that are going to be used as an IRQ. But this
doesn't seem to be true for the omap_hsmmc driver when the "cd-gpios"
mmc property is used in the omap-mmc device node.

I'm ok if you decide to revert these patches but I think we should do
it with care and be sure that the assumption that a IRQ-GPIO mapping
can happen without an explicit call to irq_create_of_mapping() is
actually true and this is not and issue in the omap_hsmmc driver or
its DT bindings.

According to Documentation/devicetree/bindings/mmc/mmc.txt:

cd-gpios: Specify GPIOs for card detection, see gpio binding

So it just says that it is a GPIO for card detection and not an IRQ so
this assumption comes from either the omap_hsmmc driver or Alexander'
DTS is missing something like:

                interrupt-parent = <&gpio6>;
                interrupts = <16 8>;

which will call irq_create_of_mapping() and the mapping for the
GPIO-IRQ will be created, thus making

irq = gpio_to_irq(gpio);
request[_threaded]_irq(irq, ...);

to succeed.

On Sun, Jul 28, 2013 at 6:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Jul 28, 2013 at 4:37 PM, Shilimkar, Santosh
> <santosh.shilimkar@ti.com> wrote:
>
>> I think the default OMAP DT files will continue to work with
>>  these patches applied and mostly doesn't break anything
>> in default configuration.
>
> What does "mostly" mean? Hm hm. OK I feel a little
> bit better about this now...
>

I think Santosh meant that

$ grep cd-gpios arch/arm/boot/dts/omap*

doesn't find any usage of the cd-gpios property so that's why this
issue was not found on my tests.

>> Ofcourse with the DT modification as done
>> by Alexander will expose the issue.
>
> So this is all caused by non-upstream code or
> non-upstream DTS files?
>

It seems so, I'm not familiar with the omap_hsmmc so I'm not sure if
this issue is with Alexander DTS, the omap_hsmmc driver or my
assumption that irq_create_of_mapping() will always be called for GPIO
mapped as IRQ is wrong.

>> I really wanted to have the auto request GPIO supported
>> when used as IRQ line but surely not at expense of
>> breaking the client drivers.
>
> If things are working for the default DTS files in the
> kernel then I am OK with it.
>

It's working with the default DTS afaict. At least I didn't find any
regression with my tests using an OMAP3 and OMAP4 boards and neither
did Enric whose tested-by I have in the patches.

> However moving forward to support the cases that
> Alexander is highlighting, I really really thin we need
> to put the information about taken GPIOs "input-hogs"
> into the gpio DT node.
>

I do agree with you that this information should be added to the DT to
prevent this kind of issues.

> Yours,
> Linus Walleij
> --

Hope this helps...

Thanks a lot and best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:
> Hello,
>
> Being the author of these patches I'm ashamed that they are causing a
> regression. I did my best to make sure that it would work on OMAP1 and
> OMAP2+ platforms booting with both legacy board files and DT.
>
> Also, I didn't come with this solution in a vacuum. This was deeply
> discussed with Jon Hunter, Grant Likely, Jean-Christophe
> PLAGNIOL-VILLARD and others on a 90+ emails linux-omap thread [1] and
> over IRC in #linaro-kernel channel.
>

Just realized I forgot to add the reference to the original thread
were the issue that $subject fixes was discussed, sorry for the
noise...

[1]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85544.html

Best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:

> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>
> cd-gpios: Specify GPIOs for card detection, see gpio binding
>
> So it just says that it is a GPIO for card detection and not an IRQ so
> this assumption comes from either the omap_hsmmc driver or Alexander'
> DTS is missing something like:
>
>                 interrupt-parent = <&gpio6>;
>                 interrupts = <16 8>;
>
> which will call irq_create_of_mapping() and the mapping for the
> GPIO-IRQ will be created, thus making
>
> irq = gpio_to_irq(gpio);
> request[_threaded]_irq(irq, ...);
>
> to succeed.

So if I understand this correctly, the issue is that some out-of-tree,
i.e. unreviewed DTS files are not following the style of the in-tree
DTS files, which mandate attributing the consumer drivers with
interrupts=  and therefore they fail?

I don't feel we have an obligation to support any strange DTS
files out there, and the ones that are in-tree for the reference
designs should showcase the vast majority of expected
usecases for DT boots. Else please add more DTS files for
ever more systems so we get proper coverage, don't be shy :-)

I wish we already had the DTS schema parser and checks
in place. In that case I could just ask if the DTS is well-formed,
and you could say "yes" or "no", and if it was no, it's just an
invalid DTS file.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
>
>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>
>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>
>> So it just says that it is a GPIO for card detection and not an IRQ so
>> this assumption comes from either the omap_hsmmc driver or Alexander'
>> DTS is missing something like:
>>
>>                 interrupt-parent = <&gpio6>;
>>                 interrupts = <16 8>;
>>
>> which will call irq_create_of_mapping() and the mapping for the
>> GPIO-IRQ will be created, thus making
>>
>> irq = gpio_to_irq(gpio);
>> request[_threaded]_irq(irq, ...);
>>
>> to succeed.
>
> So if I understand this correctly, the issue is that some out-of-tree,
> i.e. unreviewed DTS files are not following the style of the in-tree
> DTS files, which mandate attributing the consumer drivers with
> interrupts=  and therefore they fail?
>

Yes, that's my understanding of the issue. The fact that it was
working before is that the gpio-omap driver used to call
irq_create_mapping() for all the GPIO in the bank. e.g:

(j = 0; j < bank->width; j++) {
               int irq = irq_create_mapping(bank->domain, j);

But for DT this is wrong since we use the "interrupt-parent" property
to specify the controller to which interrupts are routed (an OMAP GPIO
controller in this case) and defining an "interrupts" property will
make the IRQ core to call irq_create_of_mapping().

So, $subject changes the gpio-omap driver to not do an explicit
irq_create_mapping() call for each GPIO and let the core handle that.

The sequence:

irq = gpio_to_irq(gpio);
request[_threaded]_irq(irq, ...);

only works with $subject reverted because a mapping for *every* GPIO
was created so gpio_to_irq() worked but that is wrong in the DT case
IMHO.

> I don't feel we have an obligation to support any strange DTS
> files out there, and the ones that are in-tree for the reference
> designs should showcase the vast majority of expected
> usecases for DT boots. Else please add more DTS files for
> ever more systems so we get proper coverage, don't be shy :-)
>
> I wish we already had the DTS schema parser and checks
> in place. In that case I could just ask if the DTS is well-formed,
> and you could say "yes" or "no", and if it was no, it's just an
> invalid DTS file.
>
> Yours,
> Linus Walleij

Thanks a lot and best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>> <martinez.javier@gmail.com> wrote:
>>
>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>
>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>
>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>> DTS is missing something like:
>>>
>>>                  interrupt-parent = <&gpio6>;
>>>                  interrupts = <16 8>;

What do the values 16 and 8 mean here? GPIO numbers?
And where do I have to place that?

I've now tried the following:

--
&mmc1 {
	vmmc-supply = <&vmmcsd_fixed>;
	status = "okay";
	pinctrl-names = "default";
	pinctrl-0 = <&mmc1_pins>; /* pinmux for GPIO0__6 */
	interrupt-parent = <&gpio0>;
	interrupts = <6>;
	cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
	cd-inverted;
};
--
(the gpio which should be used for the IRQ is GPIO0_6)

The result was that the gpio_request() failed with -EBUSY.
Then I've commented out that, because it isn't necessary anymore as the
gpio should be set as input automatically (as I've understood the commit
msg).

The result was

[    1.397100] genirq: Flags mismatch irq 144. 00002003 (mmc0) vs.
00000000 (mmc0)

and request_threaded_irq() returned with -EBUSY.


To stop that discussion about some "non-standard" dts I'm using (I 
wonder where the standard is), I try to formulate a clear question:

If a driver uses
--
irq = gpio_to_irq(some_gpio_number);
/*
gpio_request();
gpio_direction_input();
*/
request_threaded_irq(irq);
--

How should the dts or the driver be changed that this works with 3.11-rc2?

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

Hi Alexander,

On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
>
>> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org>
>> wrote:
>>>
>>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>>> <martinez.javier@gmail.com> wrote:
>>>
>>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>>
>>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>>
>>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>>> DTS is missing something like:
>>>>
>>>>                  interrupt-parent = <&gpio6>;
>>>>                  interrupts = <16 8>;
>
>
> What do the values 16 and 8 mean here? GPIO numbers?
> And where do I have to place that?
>

If you look at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
for the two cells interrupt controllers definition:

  b) two cells
  ------------
  The #interrupt-cells property is set to 2 and the first cell defines the
  index of the interrupt within the controller, while the second cell is used
  to specify any of the following 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

So, the first cell in this example means the 16th GPIO in the omap
gpio6 controller using the edge/level flag IRQ_TYPE_LEVEL_LOW.

> I've now tried the following:
>
> --
> &mmc1 {
>         vmmc-supply = <&vmmcsd_fixed>;
>         status = "okay";
>         pinctrl-names = "default";
>         pinctrl-0 = <&mmc1_pins>; /* pinmux for GPIO0__6 */
>         interrupt-parent = <&gpio0>;
>         interrupts = <6>;
>
>         cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>         cd-inverted;
> };
> --
> (the gpio which should be used for the IRQ is GPIO0_6)
>
> The result was that the gpio_request() failed with -EBUSY.
> Then I've commented out that, because it isn't necessary anymore as the
> gpio should be set as input automatically (as I've understood the commit
> msg).
>

Indeed, drivers shouldn't explicitly call gpio_request() when booting
with DT. This only made sense with legacy boot. So maybe the
omap_hsmmc driver has not been completely converted to be used with DT
booting when using the "cd-gpios" property?

> The result was
>
> [    1.397100] genirq: Flags mismatch irq 144. 00002003 (mmc0) vs.
> 00000000 (mmc0)
>
> and request_threaded_irq() returned with -EBUSY.

By looking at kernel/irq/manage.c, this error comes from __setup_irq()
because the same interrupt line is tried to be shared by two users
with different trigger (edge/level) type flags. Don't know what is
triggering that with your DTS though.

>
>
> To stop that discussion about some "non-standard" dts I'm using (I wonder
> where the standard is), I try to formulate a clear question:
>
> If a driver uses
> --
> irq = gpio_to_irq(some_gpio_number);
> /*
> gpio_request();
> gpio_direction_input();
> */
> request_threaded_irq(irq);
> --
>
> How should the dts or the driver be changed that this works with 3.11-rc2?
>

Drivers shouldn't do that IMHO, they should just use IRQ (virtual)
numbers and request them. Drivers shouldn't care whether it is a GPIO
line acting as an IRQ number or a real IRQ from an interrupt
controller.

My understanding is that defining:

interrupt-parent = <&phandle>;
interrupts = <index type_flags>;

*should* be enough to make the sequence you are mentioning to work.
Now, you said that this breaks your DTS when using the "cd-gpios"
property with the omap hsmmc driver. Unfortunately I'm not familiar
with neither the omap hsmmc driver nor the mmc "cd-gpios" property so
I can't be of too much help here. I guess Balaji can take a step
forward here and shed some light on this.

My guess is that it was working on your setup because the omap hsmmc
driver is handling GPIO-IRQ in the same way that board files do and it
was not even described in your DTS because support for GPIO-IRQ was
not really working for DT in OMAP (an explicit call to gpio_request
had to be made anyways).

So, I would like to first check if this really is a regression (bug)
introduced by these changes or is an issue with the omap hsmmc driver,
how it handles the "cd-gpios" binding or your DTS definition and
reverting the patch-set will just hide the real problem.

As I said, I don't mind if these patches are reverted although many
people (Jon, Santosh, Grant and myself) think that this is the right
approach and reverting them will break DTS for OMAP boards that are
already in mainline.

Yes, it is a PITA that things get broken but is the price of progress
I think, I'll prefer fixing them than just blindly reverting patches
because they show hidden issues. If I'm proven to be wrong I'll be
happy if these patches get reverted though.

> Regards,
>
> Alexander Holler

Thanks a lot and best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

På måndagen den 29 juli 2013 vid 10:17 AM, skrev Javier Martinez
Canillas <martinez.javier@gmail.com>:
> On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:

>>>>>                  interrupt-parent = <&gpio6>;
>>>>>                  interrupts = <16 8>;
>>
>>
>> What do the values 16 and 8 mean here? GPIO numbers?
>> And where do I have to place that?
(...)
>     - 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
>
> So, the first cell in this example means the 16th GPIO in the omap
> gpio6 controller using the edge/level flag IRQ_TYPE_LEVEL_LOW.

We have a better way to express this to avoid confusion.
In your DTS file, using the new DTS preprocessor:

#include <dt-bindings/interrupt-controller/irq.h>

interrupt-parent = <&gpio6>;
interrupts = <16 IRQ_TYPE_LEVEL_LOW>;

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 29.07.2013 10:17, schrieb Javier Martinez Canillas:
> Hi Alexander,
> 
> On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
>>
>>> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org>
>>> wrote:
>>>>
>>>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>>>> <martinez.javier@gmail.com> wrote:
>>>>
>>>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>>>
>>>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>>>
>>>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>>>> DTS is missing something like:
>>>>>
>>>>>                  interrupt-parent = <&gpio6>;
>>>>>                  interrupts = <16 8>;
>>
>>
>> What do the values 16 and 8 mean here? GPIO numbers?
>> And where do I have to place that?
>>
> 
> If you look at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> for the two cells interrupt controllers definition:
> 
>   b) two cells
>   ------------
>   The #interrupt-cells property is set to 2 and the first cell defines the
>   index of the interrupt within the controller, while the second cell is used

I had read that. And there is written, "index of the interrupt". So may
I ask the question how you think people are translating GPIO number to
interrupt index?. And if it's the same, will GPIO0_0 map to IRQ 0 or how
is the magic formular people are supposed to use?

Besides that, the existing standard DTS I've modified into my
"non-standard" dts is arch/arm/boot/dts/am335x-bone.dts.

if you look at the definition of the GPIO bank 0 in
arch/arm/boot/dts/am33xx.dtsi, you see

#interrupt-cells = <1>;

Anyway, thanks for your time, but I don't want to spend more time on
that topic.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On 29/07/2013, at 12:27, Alexander Holler <holler@ahsoftware.de> wrote:

> Am 29.07.2013 10:17, schrieb Javier Martinez Canillas:
>> Hi Alexander,
>> 
>> On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>>> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
>>> 
>>>> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org>
>>>> wrote:
>>>>> 
>>>>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>>>>> <martinez.javier@gmail.com> wrote:
>>>>> 
>>>>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>>>> 
>>>>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>>>> 
>>>>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>>>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>>>>> DTS is missing something like:
>>>>>> 
>>>>>>                 interrupt-parent = <&gpio6>;
>>>>>>                 interrupts = <16 8>;
>>> 
>>> 
>>> What do the values 16 and 8 mean here? GPIO numbers?
>>> And where do I have to place that?
>> 
>> If you look at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> for the two cells interrupt controllers definition:
>> 
>>  b) two cells
>>  ------------
>>  The #interrupt-cells property is set to 2 and the first cell defines the
>>  index of the interrupt within the controller, while the second cell is used
> 
> I had read that. And there is written, "index of the interrupt". So may
> I ask the question how you think people are translating GPIO number to
> interrupt index?. And if it's the same, will GPIO0_0 map to IRQ 0 or how
> is the magic formular people are supposed to use?
> 

Yes, the GPIO-IRQ is a especial case since the chip is both a GPIO and a IRQ controller. In this context the "index of the interrupt" is the physical index of the GPIO line within that GPIO bank. Since each OMAP GPIO bank has 32 GPIO lines, in this example, gpio6 + 16 = GPIO 176.

You define the hardware interrupt (GPIO line in this case) but that is not what drivers see, they see the virtual IRQ number mapped to the hardware IRQ which is just a cookie and the actual number depends on what GPIO to IRQ domain mapping is being used.

> Besides that, the existing standard DTS I've modified into my
> "non-standard" dts is arch/arm/boot/dts/am335x-bone.dts.
> 
> if you look at the definition of the GPIO bank 0 in
> arch/arm/boot/dts/am33xx.dtsi, you see
> 
> #interrupt-cells = <1>;
> 

Yes, that's probably because no one is using the edge/level flags on am33xx but that is a bug and has to be changed to #interrupt-cells = <2>

This has been fixed recently for omap3.dtsi recently too.

> Anyway, thanks for your time, but I don't want to spend more time on
> that topic.
> 

Thanks a lot for your time and sorry for the inconvenience. It seems these patches are going to be reverted anyways since it causes another regression on older OMAP1 platforms so you probably can forget about this issue.

A different "GPIO hogs" approach will be used to pre-define which GPIO are going to be used as IRQ so the core can request them and setup as input.

I expect you will have the same issue with this new approach though since the omap hsmmc driver will try to call gpio_request() on a previously requested GPIO.

So I think the driver has to be changed independently of the approach for auto request GPIO used.

> Regards,
> 
> Alexander Holler

Best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 29.07.2013 13:11, schrieb Javier Martinez Canillas:
> On 29/07/2013, at 12:27, Alexander Holler <holler@ahsoftware.de> wrote:
>
>> Am 29.07.2013 10:17, schrieb Javier Martinez Canillas:
>>> Hi Alexander,
>>>
>>> On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>>>> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
>>>>
>>>>> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org>
>>>>> wrote:
>>>>>>
>>>>>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>>>>>> <martinez.javier@gmail.com> wrote:
>>>>>>
>>>>>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>>>>>
>>>>>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>>>>>
>>>>>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>>>>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>>>>>> DTS is missing something like:
>>>>>>>
>>>>>>>                  interrupt-parent = <&gpio6>;
>>>>>>>                  interrupts = <16 8>;
>>>>
>>>>
>>>> What do the values 16 and 8 mean here? GPIO numbers?
>>>> And where do I have to place that?
>>>
>>> If you look at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>> for the two cells interrupt controllers definition:
>>>
>>>   b) two cells
>>>   ------------
>>>   The #interrupt-cells property is set to 2 and the first cell defines the
>>>   index of the interrupt within the controller, while the second cell is used
>>
>> I had read that. And there is written, "index of the interrupt". So may
>> I ask the question how you think people are translating GPIO number to
>> interrupt index?. And if it's the same, will GPIO0_0 map to IRQ 0 or how
>> is the magic formular people are supposed to use?
>>
>
> Yes, the GPIO-IRQ is a especial case since the chip is both a GPIO and a IRQ controller. In this context the "index of the interrupt" is the physical index of the GPIO line within that GPIO bank. Since each OMAP GPIO bank has 32 GPIO lines, in this example, gpio6 + 16 = GPIO 176.
>
> You define the hardware interrupt (GPIO line in this case) but that is not what drivers see, they see the virtual IRQ number mapped to the hardware IRQ which is just a cookie and the actual number depends on what GPIO to IRQ domain mapping is being used.
>
>> Besides that, the existing standard DTS I've modified into my
>> "non-standard" dts is arch/arm/boot/dts/am335x-bone.dts.
>>
>> if you look at the definition of the GPIO bank 0 in
>> arch/arm/boot/dts/am33xx.dtsi, you see
>>
>> #interrupt-cells = <1>;
>>
>
> Yes, that's probably because no one is using the edge/level flags on am33xx but that is a bug and has to be changed to #interrupt-cells = <2>
>
> This has been fixed recently for omap3.dtsi recently too.
>
>> Anyway, thanks for your time, but I don't want to spend more time on
>> that topic.
>>
>
> Thanks a lot for your time and sorry for the inconvenience. It seems these patches are going to be reverted anyways since it causes another regression on older OMAP1 platforms so you probably can forget about this issue.

Hopefully I'm not the reason for the revert. I don't want to block new 
approaches and it seems to make sense to not map an irq for every 
possible gpio. I've just tried to dig out how one is supposed to use the 
new feature with existing drivers which do use gpio_to_irq().

>
> A different "GPIO hogs" approach will be used to pre-define which GPIO are going to be used as IRQ so the core can request them and setup as input.
>
> I expect you will have the same issue with this new approach though since the omap hsmmc driver will try to call gpio_request() on a previously requested GPIO.
>
> So I think the driver has to be changed independently of the approach for auto request GPIO used.

Maybe, but that isn't much effort. I had done that in 3 minutes (ok, 
without the would be necessary #ifdef CONFIG_OF_GPIO). But you have a 
chicken and egg situation here, because you can't change drivers without 
the new patch and you can't introduce the new patch without changes to 
drivers.

As I had mentioned before, another approach could be to map the irq when 
gpio_to_irq() is called. I think that would be downward compatible 
without the need for some changes in a dts.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 29.07.2013 13:30, schrieb Alexander Holler:

>> So I think the driver has to be changed independently of the approach
>> for auto request GPIO used.
>
> Maybe, but that isn't much effort. I had done that in 3 minutes (ok,
> without the would be necessary #ifdef CONFIG_OF_GPIO). But you have a
> chicken and egg situation here, because you can't change drivers without
> the new patch and you can't introduce the new patch without changes to
> drivers.
>
> As I had mentioned before, another approach could be to map the irq when
> gpio_to_irq() is called. I think that would be downward compatible
> without the need for some changes in a dts.

Having send that, I had another idea: introduce a config option to 
enable/disable the new behaviour. That would make it possible to change 
drivers while not breaking compatibility, and when all are changed, the 
config option could go away.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Mon, Jul 29, 2013 at 1:11 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:

> A different "GPIO hogs" approach will be used to pre-define which GPIO
> are going to be used as IRQ so the core can request them and setup as input.
>
> I expect you will have the same issue with this new approach though since
> the omap hsmmc driver will try to call gpio_request() on a previously
> requested GPIO.

This should not happen.

I've realized that GPIO interrupt hogs will just redefine information
that already exist in the device tree.

The right way is to traverse the tree, find all references to the GPIO
based interrupt-controller, and request each of these lines as
GPIO (i.e. hog them) in the gpiolib OF core.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Balaji T K]

On Monday 29 July 2013 01:47 PM, Javier Martinez Canillas wrote:
> Hi Alexander,
>
> On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
>>
>>> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org>
>>> wrote:
>>>>
>>>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>>>> <martinez.javier@gmail.com> wrote:
>>>>
>>>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>>>
>>>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>>>
>>>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>>>> DTS is missing something like:
>>>>>
>>>>>                   interrupt-parent = <&gpio6>;
>>>>>                   interrupts = <16 8>;
>>
>>
>> What do the values 16 and 8 mean here? GPIO numbers?
>> And where do I have to place that?
>>
>
> If you look at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> for the two cells interrupt controllers definition:
>
>    b) two cells
>    ------------
>    The #interrupt-cells property is set to 2 and the first cell defines the
>    index of the interrupt within the controller, while the second cell is used
>    to specify any of the following 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
>
> So, the first cell in this example means the 16th GPIO in the omap
> gpio6 controller using the edge/level flag IRQ_TYPE_LEVEL_LOW.
>
>> I've now tried the following:
>>
>> --
>> &mmc1 {
>>          vmmc-supply = <&vmmcsd_fixed>;
>>          status = "okay";
>>          pinctrl-names = "default";
>>          pinctrl-0 = <&mmc1_pins>; /* pinmux for GPIO0__6 */
>>          interrupt-parent = <&gpio0>;
>>          interrupts = <6>;
>>
>>          cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>          cd-inverted;
>> };
>> --
>> (the gpio which should be used for the IRQ is GPIO0_6)
>>
>> The result was that the gpio_request() failed with -EBUSY.
>> Then I've commented out that, because it isn't necessary anymore as the
>> gpio should be set as input automatically (as I've understood the commit
>> msg).
>>
>
> Indeed, drivers shouldn't explicitly call gpio_request() when booting
> with DT. This only made sense with legacy boot. So maybe the
> omap_hsmmc driver has not been completely converted to be used with DT
> booting when using the "cd-gpios" property?
>
>> The result was
>>
>> [    1.397100] genirq: Flags mismatch irq 144. 00002003 (mmc0) vs.
>> 00000000 (mmc0)
>>
>> and request_threaded_irq() returned with -EBUSY.
>
> By looking at kernel/irq/manage.c, this error comes from __setup_irq()
> because the same interrupt line is tried to be shared by two users
> with different trigger (edge/level) type flags. Don't know what is
> triggering that with your DTS though.
>
>>
>>
>> To stop that discussion about some "non-standard" dts I'm using (I wonder
>> where the standard is), I try to formulate a clear question:
>>
>> If a driver uses
>> --
>> irq = gpio_to_irq(some_gpio_number);
>> /*
>> gpio_request();
>> gpio_direction_input();
>> */
>> request_threaded_irq(irq);
>> --
>>
>> How should the dts or the driver be changed that this works with 3.11-rc2?
>>
>
> Drivers shouldn't do that IMHO, they should just use IRQ (virtual)
> numbers and request them. Drivers shouldn't care whether it is a GPIO
> line acting as an IRQ number or a real IRQ from an interrupt
> controller.
>
> My understanding is that defining:
>
> interrupt-parent = <&phandle>;
> interrupts = <index type_flags>;
>
> *should* be enough to make the sequence you are mentioning to work.
> Now, you said that this breaks your DTS when using the "cd-gpios"
> property with the omap hsmmc driver. Unfortunately I'm not familiar
> with neither the omap hsmmc driver nor the mmc "cd-gpios" property so
> I can't be of too much help here. I guess Balaji can take a step
> forward here and shed some light on this.
>
> My guess is that it was working on your setup because the omap hsmmc
> driver is handling GPIO-IRQ in the same way that board files do and it
> was not even described in your DTS because support for GPIO-IRQ was
> not really working for DT in OMAP (an explicit call to gpio_request
> had to be made anyways).
>
> So, I would like to first check if this really is a regression (bug)
> introduced by these changes or is an issue with the omap hsmmc driver,
> how it handles the "cd-gpios" binding or your DTS definition and
> reverting the patch-set will just hide the real problem.
>
Hi,

omap_hsmmc received the gpio number from cd-gpios and configured
it for card detect gpio interrupt. omap_hsmmc driver did gpio_request(),
gpio_direction_input() as it was required to do so, as in legacy board
case.

> As I said, I don't mind if these patches are reverted although many
> people (Jon, Santosh, Grant and myself) think that this is the right
> approach and reverting them will break DTS for OMAP boards that are
> already in mainline.

Since cd-gpios dts binding for omap boards is not in mainline,
omap_hsmmc driver can be patched with new binding added.

>
> Yes, it is a PITA that things get broken but is the price of progress
> I think, I'll prefer fixing them than just blindly reverting patches
> because they show hidden issues. If I'm proven to be wrong I'll be
> happy if these patches get reverted though.
>
>> Regards,
>>
>> Alexander Holler
>
> Thanks a lot and best regards,
> Javier
>

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
>
>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>
>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>
>> So it just says that it is a GPIO for card detection and not an IRQ so
>> this assumption comes from either the omap_hsmmc driver or Alexander'
>> DTS is missing something like:
>>
>>                 interrupt-parent = <&gpio6>;
>>                 interrupts = <16 8>;
>>
>> which will call irq_create_of_mapping() and the mapping for the
>> GPIO-IRQ will be created, thus making
>>
>> irq = gpio_to_irq(gpio);
>> request[_threaded]_irq(irq, ...);
>>
>> to succeed.
>
> So if I understand this correctly, the issue is that some out-of-tree,
> i.e. unreviewed DTS files are not following the style of the in-tree
> DTS files, which mandate attributing the consumer drivers with
> interrupts=  and therefore they fail?
>
> I don't feel we have an obligation to support any strange DTS
> files out there, and the ones that are in-tree for the reference
> designs should showcase the vast majority of expected
> usecases for DT boots. Else please add more DTS files for
> ever more systems so we get proper coverage, don't be shy :-)
>

Yes, we are trying to add more DTS for OMAP boards to have a better
coverage. In fact adding support for an ethernet chip connected to the
OMAP GPMC memory controller was what showed the need for the GPIO auto
request feature that led to this patch-set.

> I wish we already had the DTS schema parser and checks
> in place. In that case I could just ask if the DTS is well-formed,
> and you could say "yes" or "no", and if it was no, it's just an
> invalid DTS file.
>

Indeed, hopefully we will have schema validation in dtc soon so we
could validate if DTS are well-formed and test if our changes breaks
valid DTS or not.

> Yours,
> Linus Walleij

Best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 21:30, schrieb Javier Martinez Canillas:
> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>> <martinez.javier@gmail.com> wrote:
>>

>
>> I wish we already had the DTS schema parser and checks
>> in place. In that case I could just ask if the DTS is well-formed,
>> and you could say "yes" or "no", and if it was no, it's just an
>> invalid DTS file.
>>
>
> Indeed, hopefully we will have schema validation in dtc soon so we
> could validate if DTS are well-formed and test if our changes breaks
> valid DTS or not.

That won't help with the current problem because the DTS doesn't know 
that some code want's to use a gpio as irq, so that can't be verified.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 12:58, schrieb Alexander Holler:

> This patch basically broke every usage of
> 
> irq = gpio_to_irq(gpio);
> request[_threaded]_irq(irq, ...);
> 
> because request[_threaded]_irq(irq, ...) now fails because of a missing
> irq_domain (no mapping => no domain).
> 
> A prominent victim of this is the omap_hsmmc driver with the cd-gpio
> option enabled. To reproduce it, just add "cd-gpios = <&gpio0 6
> GPIO_ACTIVE_HIGH>;" (or similiar) to the omap-mmc part in the DT.

By the way, if someone decides to touch omap_hsmmc, the driver wrongly
assumes that 0 is not a valid IRQ number and it doesn't check if
gpio_to_irq() returns a negative value. ;)

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 4:25 PM, Alexander Holler <holler@ahsoftware.de> wrote:

> By the way, if someone decides to touch omap_hsmmc, the driver wrongly
> assumes that 0 is not a valid IRQ number and it doesn't check if
> gpio_to_irq() returns a negative value. ;)

Zero *is* *not* a valid IRQ number.

But the latter should be checked.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 18:25, schrieb Linus Walleij:
> On Sun, Jul 28, 2013 at 4:25 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> 
>> By the way, if someone decides to touch omap_hsmmc, the driver wrongly
>> assumes that 0 is not a valid IRQ number and it doesn't check if
>> gpio_to_irq() returns a negative value. ;)
> 
> Zero *is* *not* a valid IRQ number.

Where is that mentioned?

gpio.txt states:

----
Non-error values returned from gpio_to_irq() can be passed to request_irq()
or free_irq().  They will often be stored into IRQ resources for platform
----

With the new patches gpio_to_irq() returns 0.

Documentation/IRQ-domain.txt:
----
The legacy map should only be used if fixed IRQ mappings must be
supported.  For example, ISA controllers would use the legacy map for
mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
numbers.
----

You see the 0 too?

And while browsing some other source I had the impression zero might be
a valid irq number too, at least in regard to the IRQ apis. If it's a
valid IRQ nuber on ARM is something else.

Of ourse, I might be wrong, but you just stated that 0 isn't valid, and
I would be happy to find a source for your statement.

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On Sun, Jul 28, 2013 at 6:45 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 28.07.2013 18:25, schrieb Linus Walleij:
>> On Sun, Jul 28, 2013 at 4:25 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>
>>> By the way, if someone decides to touch omap_hsmmc, the driver wrongly
>>> assumes that 0 is not a valid IRQ number and it doesn't check if
>>> gpio_to_irq() returns a negative value. ;)
>>
>> Zero *is* *not* a valid IRQ number.
>
> Where is that mentioned?
>
> gpio.txt states:
>
> ----
> Non-error values returned from gpio_to_irq() can be passed to request_irq()
> or free_irq().  They will often be stored into IRQ resources for platform
> ----
>
> With the new patches gpio_to_irq() returns 0.
>

Hi Alexander,

gpio_to_irq() returns 0 because omap_gpio_to_irq() calls
irq_find_mapping() and this does:

if (domain == NULL)
       return 0;

So gpio_to_irq() returning zero means that there isn't a GPIO-IRQ
mapping domain not that the 0 is the virtual IRQ number mapped to this
GPIO.

> Documentation/IRQ-domain.txt:
> ----
> The legacy map should only be used if fixed IRQ mappings must be
> supported.  For example, ISA controllers would use the legacy map for
> mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
> numbers.
> ----
>
> You see the 0 too?
>
> And while browsing some other source I had the impression zero might be
> a valid irq number too, at least in regard to the IRQ apis. If it's a
> valid IRQ nuber on ARM is something else.
>
> Of ourse, I might be wrong, but you just stated that 0 isn't valid, and
> I would be happy to find a source for your statement.
>
> Alexander Holler
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Sun, Jul 28, 2013 at 6:45 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 28.07.2013 18:25, schrieb Linus Walleij:
>> On Sun, Jul 28, 2013 at 4:25 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>
>>> By the way, if someone decides to touch omap_hsmmc, the driver wrongly
>>> assumes that 0 is not a valid IRQ number and it doesn't check if
>>> gpio_to_irq() returns a negative value. ;)
>>
>> Zero *is* *not* a valid IRQ number.
>
> Where is that mentioned?

This has been a major debate in the kernel in recent months, and we are
agreed to remove 0 as a valid Linux IRQ number. The fact that up until
two years ago the ARM kernel allowed it is a historical artifact.

Please see this article for background:
http://lwn.net/Articles/470820/

Which falls back to this posting from Torvalds:
http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-11/7628.html

> gpio.txt states:
>
> ----
> Non-error values returned from gpio_to_irq() can be passed to request_irq()
> or free_irq().  They will often be stored into IRQ resources for platform

While IRQ 0 is not an error, it means that this particular GPIO line
does not have an IRQ, or cannot be translated into an IRQ, and
should not be passed to request_irq().

Patches to the documentation is welcome.

> With the new patches gpio_to_irq() returns 0.

This is not good. Under which circumstances does that happen?

> Documentation/IRQ-domain.txt:
> ----
> The legacy map should only be used if fixed IRQ mappings must be
> supported.  For example, ISA controllers would use the legacy map for
> mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
> numbers.
> ----
>
> You see the 0 too?

Is OMAP still using the legacy map? I don't think that works
on any system utilizing the GIC driver. It is called legacy because
it is not supposed to be used :-/

> Of ourse, I might be wrong, but you just stated that 0 isn't valid, and
> I would be happy to find a source for your statement.

See above.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Javier Martinez Canillas]

On Sun, Jul 28, 2013 at 8:06 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Jul 28, 2013 at 6:45 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 28.07.2013 18:25, schrieb Linus Walleij:
>>> On Sun, Jul 28, 2013 at 4:25 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>>
>>>> By the way, if someone decides to touch omap_hsmmc, the driver wrongly
>>>> assumes that 0 is not a valid IRQ number and it doesn't check if
>>>> gpio_to_irq() returns a negative value. ;)
>>>
>>> Zero *is* *not* a valid IRQ number.
>>
>> Where is that mentioned?
>
> This has been a major debate in the kernel in recent months, and we are
> agreed to remove 0 as a valid Linux IRQ number. The fact that up until
> two years ago the ARM kernel allowed it is a historical artifact.
>
> Please see this article for background:
> http://lwn.net/Articles/470820/
>
> Which falls back to this posting from Torvalds:
> http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-11/7628.html
>
>> gpio.txt states:
>>
>> ----
>> Non-error values returned from gpio_to_irq() can be passed to request_irq()
>> or free_irq().  They will often be stored into IRQ resources for platform
>
> While IRQ 0 is not an error, it means that this particular GPIO line
> does not have an IRQ, or cannot be translated into an IRQ, and
> should not be passed to request_irq().
>
> Patches to the documentation is welcome.
>
>> With the new patches gpio_to_irq() returns 0.
>
> This is not good. Under which circumstances does that happen?
>
>> Documentation/IRQ-domain.txt:
>> ----
>> The legacy map should only be used if fixed IRQ mappings must be
>> supported.  For example, ISA controllers would use the legacy map for
>> mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
>> numbers.
>> ----
>>
>> You see the 0 too?
>
> Is OMAP still using the legacy map? I don't think that works
> on any system utilizing the GIC driver. It is called legacy because
> it is not supposed to be used :-/
>

Not anymore, it was changed recently to use the linear domain mapping
instead in commit ede4d7a5 ("gpio/omap: convert gpio irq domain to
linear mapping").

The commit message says Reported-by: Linus Walleij
<linus.walleij@linaro.org> ;-)

>> Of ourse, I might be wrong, but you just stated that 0 isn't valid, and
>> I would be happy to find a source for your statement.
>
> See above.
>
> Yours,
> Linus Walleij
> --

Best regards,
Javier

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 28.07.2013 20:50, schrieb Javier Martinez Canillas:
> On Sun, Jul 28, 2013 at 8:06 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Sun, Jul 28, 2013 at 6:45 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>> Am 28.07.2013 18:25, schrieb Linus Walleij:
>>>> On Sun, Jul 28, 2013 at 4:25 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>>>
>>>>> By the way, if someone decides to touch omap_hsmmc, the driver wrongly
>>>>> assumes that 0 is not a valid IRQ number and it doesn't check if
>>>>> gpio_to_irq() returns a negative value. ;)
>>>>
>>>> Zero *is* *not* a valid IRQ number.
>>>
>>> Where is that mentioned?
>>
>> This has been a major debate in the kernel in recent months, and we are
>> agreed to remove 0 as a valid Linux IRQ number. The fact that up until
>> two years ago the ARM kernel allowed it is a historical artifact.
>>
>> Please see this article for background:
>> http://lwn.net/Articles/470820/
>>
>> Which falls back to this posting from Torvalds:
>> http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-11/7628.html
>>
>>> gpio.txt states:
>>>
>>> ----
>>> Non-error values returned from gpio_to_irq() can be passed to request_irq()
>>> or free_irq().  They will often be stored into IRQ resources for platform
>>
>> While IRQ 0 is not an error, it means that this particular GPIO line
>> does not have an IRQ, or cannot be translated into an IRQ, and
>> should not be passed to request_irq().
>>
>> Patches to the documentation is welcome.
>>
>>> With the new patches gpio_to_irq() returns 0.
>>
>> This is not good. Under which circumstances does that happen?
>>
>>> Documentation/IRQ-domain.txt:
>>> ----
>>> The legacy map should only be used if fixed IRQ mappings must be
>>> supported.  For example, ISA controllers would use the legacy map for
>>> mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
>>> numbers.
>>> ----
>>>
>>> You see the 0 too?
>>
>> Is OMAP still using the legacy map? I don't think that works
>> on any system utilizing the GIC driver. It is called legacy because
>> it is not supposed to be used :-/
>>
>
> Not anymore, it was changed recently to use the linear domain mapping
> instead in commit ede4d7a5 ("gpio/omap: convert gpio irq domain to
> linear mapping").
>

I don't really care which mapping is in place. I just care if zero is a 
valid IRQ number for the IRQ and related APIs. And I have to assume that 
those APIs don't switch their internal handling because of the irq 
mapping in place.

Of course, having zero as an invalid IRQ number is a handy thing, but I 
just didn't find a place where this is written down. And the usually 
numbering in IT (starting at zero) makes it very likely that zero could 
be valid IRQ number.

> The commit message says Reported-by: Linus Walleij
> <linus.walleij@linaro.org> ;-)
>
>>> Of ourse, I might be wrong, but you just stated that 0 isn't valid, and
>>> I would be happy to find a source for your statement.
>>
>> See above.

Yeah, as usual, some mail, discussion, thread, article or similiar in 
the small internet everyone has memorized in full. ;)

Once, a maintainer refered me to a discussion and commit which happend 
10 years ago and it sounded like I'm a morron because I didn't know that. ;)

Thanks a lot for the article about NO_IRQ, it describes the pit I fell 
into too, very good.

Maybe it might be worth to suggest using/returning NO_IRQ in (new) 
patches instead of zero. That would make it very clear that the value 0 
isn't to be used later.

Regards,

Alexander Holler

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Linus Walleij]

On Mon, Jul 29, 2013 at 7:24 AM, Alexander Holler <holler@ahsoftware.de> wrote:

> Maybe it might be worth to suggest using/returning NO_IRQ in (new) patches
> instead of zero. That would make it very clear that the value 0 isn't to be
> used later.

Using NO_IRQ is also discouraged, and this is because we should
use the if (!irq) design pattern and not if (irq == NO_IRQ).

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Alexander Holler]

Am 29.07.2013 11:05, schrieb Linus Walleij:
> On Mon, Jul 29, 2013 at 7:24 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>
>> Maybe it might be worth to suggest using/returning NO_IRQ in (new) patches
>> instead of zero. That would make it very clear that the value 0 isn't to be
>> used later.
>
> Using NO_IRQ is also discouraged, and this is because we should
> use the if (!irq) design pattern and not if (irq == NO_IRQ).

So the article on lkml is already outdated. After reading the article, I 
had the impression that using NO_IRQ (at least for return codes) is 
encouraged.

Regards,

Alexander Holler