[PATCH v3 0/2]: auto request GPIO as input if used as IRQ via DT

[PATCH v3 0/2]: 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.

Drivers 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. Ideally this has to be handled by the IRQ core and
there are some work-in-progress to add this logic to the core but
until this general solution gets into mainline we need to solve this
on a per irq_chip driver basis.

Some drivers solve this by calling gpio_request() using a custom
.xlate function handler. But .xlate could get called many times
while the irq domain .map function handler is called just once
when a IRQ mapping is created with a call to irq_create_mapping().

This patch-set adds a custom .map function handler for the gpio-omap
irq_chip driver that automatically call gpio_request() when a IRQ
mapping is created for the GPIO line used as interrupt when using DT.
This is just a temporary solution (a.k.a a hack) until a kernel wide
approach is implemented and added to mainline.

This is a third version of a patch-set that addresses some issues pointed
out by Grant Like, it is composed of the following patches:

[PATCH v3 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT
[PATCH v3 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT

This was tested on an OMAP3 DM3735 board (IGEPv2) and all the supported
peripherals are working correctly with both legacy and DT booting. Further
testing will be highly appreciated.

Aaro, could you please test if these changes break any of your OMAP1 boards?
Although it shouldn't do it as far as I can tell.

Many thanks to Jon Hunter and Grant Likely for their feedback and
suggestions on how to solve this.

Best regards,
Javier

[PATCH v3 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>
---

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..42f04ff 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 (!of_have_populated_dt())
+		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

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

[Grant Likely]

On Wed, Jun 26, 2013 at 8:50 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>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>
> 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..42f04ff 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 (!of_have_populated_dt())
> +               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
>

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

[Enric Balletbo Serra]

2013/6/28 Grant Likely <grant.likely@linaro.org>:
> On Wed, Jun 26, 2013 at 8:50 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>
>
> Acked-by: Grant Likely <grant.likely@linaro.org>
>

On IGEPv2 and IGEP COM module ...

Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>



>> ---
>>
>> 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..42f04ff 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 (!of_have_populated_dt())
>> +               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 v3 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>
---

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 |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 42f04ff..98848c9 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,15 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq,
 		set_irq_flags(virq, IRQF_VALID);
 	}
 
+	if (of_have_populated_dt()) {
+		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 v3 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT

[Grant Likely]

On Wed, Jun 26, 2013 at 8:50 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> 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>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>
> 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 |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 42f04ff..98848c9 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,15 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq,
>                 set_irq_flags(virq, IRQF_VALID);
>         }
>
> +       if (of_have_populated_dt()) {
> +               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 v3 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT

[Enric Balletbo Serra]

2013/6/28 Grant Likely <grant.likely@linaro.org>:
> On Wed, Jun 26, 2013 at 8:50 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> 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>
>
> Acked-by: Grant Likely <grant.likely@linaro.org>
>
On IGEPv2 and IGEP COM module ...

Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>

>> ---
>>
>> 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 |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 42f04ff..98848c9 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,15 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq,
>>                 set_irq_flags(virq, IRQF_VALID);
>>         }
>>
>> +       if (of_have_populated_dt()) {
>> +               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 v3 0/2]: auto request GPIO as input if used as IRQ via DT

[Kevin Hilman]

Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:

> 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.
>
> Drivers 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. Ideally this has to be handled by the IRQ core and
> there are some work-in-progress to add this logic to the core but
> until this general solution gets into mainline we need to solve this
> on a per irq_chip driver basis.
>
> Some drivers solve this by calling gpio_request() using a custom
> .xlate function handler. But .xlate could get called many times
> while the irq domain .map function handler is called just once
> when a IRQ mapping is created with a call to irq_create_mapping().
>
> This patch-set adds a custom .map function handler for the gpio-omap
> irq_chip driver that automatically call gpio_request() when a IRQ
> mapping is created for the GPIO line used as interrupt when using DT.
> This is just a temporary solution (a.k.a a hack) until a kernel wide
> approach is implemented and added to mainline.
>
> This is a third version of a patch-set that addresses some issues pointed
> out by Grant Like, it is composed of the following patches:
>
> [PATCH v3 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT
> [PATCH v3 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT
>
> This was tested on an OMAP3 DM3735 board (IGEPv2) and all the supported
> peripherals are working correctly with both legacy and DT booting. Further
> testing will be highly appreciated.

Was there any testing done to hit low-power modes?  suspend/resume?
CPUidle enabled? etc.  It is especially instructive to enable off
mode[1] on OMAP3 platforms and see if things still work as expected.

Kevin

[1] echo 1 > /sys/kerel/debug/pm_debug/enable_off_mode

> Aaro, could you please test if these changes break any of your OMAP1 boards?
> Although it shouldn't do it as far as I can tell.
>
> Many thanks to Jon Hunter and Grant Likely for their feedback and
> suggestions on how to solve this.
>
> Best regards,
> Javier
> --
> 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 v3 0/2]: auto request GPIO as input if used as IRQ via DT

[Javier Martinez Canillas]

On 07/02/2013 01:23 AM, Kevin Hilman wrote:
> Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:
> 
>> 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.
>>
>> Drivers 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. Ideally this has to be handled by the IRQ core and
>> there are some work-in-progress to add this logic to the core but
>> until this general solution gets into mainline we need to solve this
>> on a per irq_chip driver basis.
>>
>> Some drivers solve this by calling gpio_request() using a custom
>> .xlate function handler. But .xlate could get called many times
>> while the irq domain .map function handler is called just once
>> when a IRQ mapping is created with a call to irq_create_mapping().
>>
>> This patch-set adds a custom .map function handler for the gpio-omap
>> irq_chip driver that automatically call gpio_request() when a IRQ
>> mapping is created for the GPIO line used as interrupt when using DT.
>> This is just a temporary solution (a.k.a a hack) until a kernel wide
>> approach is implemented and added to mainline.
>>
>> This is a third version of a patch-set that addresses some issues pointed
>> out by Grant Like, it is composed of the following patches:
>>
>> [PATCH v3 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT
>> [PATCH v3 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT
>>
>> This was tested on an OMAP3 DM3735 board (IGEPv2) and all the supported
>> peripherals are working correctly with both legacy and DT booting. Further
>> testing will be highly appreciated.
> 
> Was there any testing done to hit low-power modes?  suspend/resume?
> CPUidle enabled? etc.  It is especially instructive to enable off
> mode[1] on OMAP3 platforms and see if things still work as expected.
> 
> Kevin
> 
> [1] echo 1 > /sys/kerel/debug/pm_debug/enable_off_mode
>

Hi Kevin,

Thanks a lot for your feedback.

I tested enabling power domain transitions to off mode [1] and suspending to RAM
[2] with both DT and legacy boot. With legacy booting everything works as expected.

In the DT case suspending/resume (without enabling off mode) doesn't affect
system operation. For example the Ethernet chip that uses a GPIO IRQ still is
able to transmit frames after suspending to RAM and waking up the board hitting
Ctrl+C in the serial console.

Now, enabling off mode [2] with DT makes the board to never go out from suspend:

root@igep00x0:~# echo 1 > /sys/kernel/debug/pm_debug/enable_off_mode
root@igep00x0:~# echo mem > /sys/power/state
[  129.833343] PM: Syncing filesystems ... done.
[  129.879211] Freezing user space processes ... (elapsed 0.01 seconds) done.
[  129.905487] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
[  129.935363] Suspending console(s) (use no_console_suspend to debug)

The board just hangs/sleeps and I don't have a way to take it from suspend.

I don't know if there something missing on my board DT file but I think this is
orthogonal with these patches since since I got the same behavior without them.

Best regards,
Javier

[1] echo 1 > /sys/kernel/debug/pm_debug/enable_off_mode
[2] echo mem > /sys/power/state

Re: [PATCH v3 0/2]: auto request GPIO as input if used as IRQ via DT

[Kevin Hilman]

Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:

> On 07/02/2013 01:23 AM, Kevin Hilman wrote:
>> Javier Martinez Canillas <javier.martinez@collabora.co.uk> writes:
>> 
>>> 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.
>>>
>>> Drivers 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. Ideally this has to be handled by the IRQ core and
>>> there are some work-in-progress to add this logic to the core but
>>> until this general solution gets into mainline we need to solve this
>>> on a per irq_chip driver basis.
>>>
>>> Some drivers solve this by calling gpio_request() using a custom
>>> .xlate function handler. But .xlate could get called many times
>>> while the irq domain .map function handler is called just once
>>> when a IRQ mapping is created with a call to irq_create_mapping().
>>>
>>> This patch-set adds a custom .map function handler for the gpio-omap
>>> irq_chip driver that automatically call gpio_request() when a IRQ
>>> mapping is created for the GPIO line used as interrupt when using DT.
>>> This is just a temporary solution (a.k.a a hack) until a kernel wide
>>> approach is implemented and added to mainline.
>>>
>>> This is a third version of a patch-set that addresses some issues pointed
>>> out by Grant Like, it is composed of the following patches:
>>>
>>> [PATCH v3 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT
>>> [PATCH v3 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT
>>>
>>> This was tested on an OMAP3 DM3735 board (IGEPv2) and all the supported
>>> peripherals are working correctly with both legacy and DT booting. Further
>>> testing will be highly appreciated.
>> 
>> Was there any testing done to hit low-power modes?  suspend/resume?
>> CPUidle enabled? etc.  It is especially instructive to enable off
>> mode[1] on OMAP3 platforms and see if things still work as expected.
>> 
>> Kevin
>> 
>> [1] echo 1 > /sys/kerel/debug/pm_debug/enable_off_mode
>>
>
> Hi Kevin,
>
> Thanks a lot for your feedback.
>
> I tested enabling power domain transitions to off mode [1] and suspending to RAM
> [2] with both DT and legacy boot. With legacy booting everything works as expected.

Excellent, thanks.

> In the DT case suspending/resume (without enabling off mode) doesn't affect
> system operation. For example the Ethernet chip that uses a GPIO IRQ still is
> able to transmit frames after suspending to RAM and waking up the board hitting
> Ctrl+C in the serial console.
>
> Now, enabling off mode [2] with DT makes the board to never go out from suspend:
>
> root@igep00x0:~# echo 1 > /sys/kernel/debug/pm_debug/enable_off_mode
> root@igep00x0:~# echo mem > /sys/power/state
> [  129.833343] PM: Syncing filesystems ... done.
> [  129.879211] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [  129.905487] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
> [  129.935363] Suspending console(s) (use no_console_suspend to debug)
>
> The board just hangs/sleeps and I don't have a way to take it from suspend.
>
> I don't know if there something missing on my board DT file but I think this is
> orthogonal with these patches since since I got the same behavior without them.

Yes, you're right.  I forgot we don't have full wakeup capabilities from
low-power states yet (IO pad wakeups) with DT boots.  We have that
mostly working now, but not ready for v3.10.

Thanks for testing,

Once the OMAP1 build failure is fixed (omap1_defconfig build fails
because of this series), feel free to add

Tested-by: Kevin Hilman <khilman@linaro.org>  # OMAP3530: Beagle, Overo
Reviewed-by: Kevin Hilman <khilman@linaro.org>

Kevin

Re: [PATCH v3 0/2]: auto request GPIO as input if used as IRQ via DT

[Kevin Hilman]

On Wed, Jun 26, 2013 at 12:50 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:

[...]

> Aaro, could you please test if these changes break any of your OMAP1 boards?
> Although it shouldn't do it as far as I can tell.

This doesn't build for omap1_defconfig (at least in next-20130702):

/work/kernel/next/drivers/gpio/gpio-omap.c: In function 'omap_gpio_chip_init':
/work/kernel/next/drivers/gpio/gpio-omap.c:1080:17: error: 'struct
gpio_chip' has no member named 'of_node'
/work/kernel/next/drivers/gpio/gpio-omap.c: In function 'omap_gpio_irq_map':
/work/kernel/next/drivers/gpio/gpio-omap.c:1116:16: error: 'struct
gpio_chip' has no member named 'of_node'

Probably because OMAP1 is non-DT?

Kevin

Re: [PATCH v3 0/2]: auto request GPIO as input if used as IRQ via DT

[Javier Martinez Canillas]

On Tue, Jul 2, 2013 at 6:10 PM, Kevin Hilman <khilman@linaro.org> wrote:
> On Wed, Jun 26, 2013 at 12:50 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>
> [...]
>
>> Aaro, could you please test if these changes break any of your OMAP1 boards?
>> Although it shouldn't do it as far as I can tell.
>
> This doesn't build for omap1_defconfig (at least in next-20130702):
>
> /work/kernel/next/drivers/gpio/gpio-omap.c: In function 'omap_gpio_chip_init':
> /work/kernel/next/drivers/gpio/gpio-omap.c:1080:17: error: 'struct
> gpio_chip' has no member named 'of_node'
> /work/kernel/next/drivers/gpio/gpio-omap.c: In function 'omap_gpio_irq_map':
> /work/kernel/next/drivers/gpio/gpio-omap.c:1116:16: error: 'struct
> gpio_chip' has no member named 'of_node'
>
> Probably because OMAP1 is non-DT?
>
> Kevin
> --

Hi Kevin,

Yes, sorry about that. In the a previous version of the patch-set
of_have_populated_dt() was used instead of chip.of_node and it built
correctly with OMAP1 since it just default to return false.

Then I was told to check if the struct gpio_chip has an associated
device node instead and forget to build test for OMAP1 :-(

Could you please tell me if the following patch looks like a good fix
to you so I can do a proper post?

>From 2d14bcc7c300a9451d7d8a37eeff4e0285c977a0 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Tue, 2 Jul 2013 19:04:14 +0200
Subject: [PATCH 1/1] gpio/omap: fix build error when CONFIG_OF_GPIO is not
 defined.

The OMAP GPIO driver check if the chip has an associated
DT device node using the struct gpio_chip of_node member.

But this is only built if CONFIG_OF_GPIO is defined which
leads to the following error when using omap1_defconfig:

linux/drivers/gpio/gpio-omap.c: In function 'omap_gpio_chip_init':
linux/drivers/gpio/gpio-omap.c:1080:17: error: 'struct gpio_chip' has
no member named 'of_node'
linux/drivers/gpio/gpio-omap.c: In function 'omap_gpio_irq_map':
linux/drivers/gpio/gpio-omap.c:1116:16: error: 'struct gpio_chip' has
no member named 'of_node'

Reported-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/gpio/gpio-omap.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c4f0aa2..bd536f1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1037,6 +1037,18 @@ omap_mpuio_alloc_gc(struct gpio_bank *bank,
unsigned int irq_start,
 			       IRQ_NOREQUEST | IRQ_NOPROBE, 0);
 }

+#if defined(CONFIG_OF_GPIO)
+static inline bool omap_gpio_chip_dt_boot(struct gpio_chip *chip)
+{
+	return chip->of_node != NULL;
+}
+#else
+static inline bool omap_gpio_chip_dt_boot(struct gpio_chip *chip)
+{
+	return false;
+}
+#endif
+
 static void omap_gpio_chip_init(struct gpio_bank *bank)
 {
 	int j;
@@ -1077,7 +1089,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 	 * irq_create_of_mapping() only for the GPIO lines that
 	 * are used as interrupts.
 	 */
-	if (!bank->chip.of_node)
+	if (!omap_gpio_chip_dt_boot(&bank->chip))
 		for (j = 0; j < bank->width; j++)
 			irq_create_mapping(bank->domain, j);
 	irq_set_chained_handler(bank->irq, gpio_irq_handler);
@@ -1113,7 +1125,7 @@ static int omap_gpio_irq_map(struct irq_domain
*d, unsigned int virq,
 	 * 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) {
+	if (omap_gpio_chip_dt_boot(&bank->chip)) {
 		gpio = irq_to_gpio(bank, hwirq);
 		ret = gpio_request_one(gpio, GPIOF_IN, NULL);
 		if (ret) {
-- 
1.7.7.6