To use a GPIO pin as an interrupt line, two previous configurations
have to be made:
a) Map the GPIO pin as an interrupt line into the Linux irq space
b) Enable the GPIO bank and configure the GPIO direction as input
Most GPIO/IRQ chip drivers just create a mapping for every single
GPIO pin with irq_create_mapping() on .probe so users usually can
assume a) and only have to do b) by using the following sequence:
gpio_request(gpio, "foo IRQ");
gpio_direction_input(gpio);
and then request a IRQ with:
irq = gpio_to_irq(gpio);
request_irq(irq, ...);
Some drivers know that their IRQ line is being driven by a GPIO
and use a similar sequence as the described above but others are
not aware or don't care wether their IRQ is a real line from an
interrupt controller or a GPIO pin acting as an IRQ.
In these cases board files did all the necessary GPIO setup so the
drivers could only call request_irq() on the provided IRQ.
Unfortuntaly this does not work when booting with Device Trees since
the OF irq core does neither request the GPIO nor set its direction.
A DTS just defines the GPIO controller phandle as a interrupt-parent
so drivers get the mapped GPIO-IRQ line but request_irq() fails since
the setup was never made.
The first attemp to solve this issue was by adding a custom .map()
irq_domain_ops function handler in the gpio-omap driver that called
gpio_request_one() to request and setup the GPIO direction as input.
This was made on commits:
0e970cec ("gpio/omap: don't create an IRQ mapping for every GPIO on DT")
b4419e1a ("gpio/omap: auto request GPIO as input if used as IRQ via DT")
but they got reverted since broke OMAP1 platforms.
This patch solves this issue with a different approach, by doing all
the needed hardware setup on the GPIO/IRQ chip driver when a call to
request_irq() is made.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
Hello,
This patch is an attempt to solve the long standing issue that we have been
discussing in thread:
"[PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs" [1]
The fix is just for OMAP of course and not a kernel wide solution but Linus
approach (which I think is the best general solution proposed so far) has
some resistance so it seems that it won't be merged and we really need a
solution for OMAP if we want to finish our migration to Device Tree soon.
The patch is based on 3.12-rc1 but is not meant to be merged as a fix for
this -rc cycle. I would like it to be tested ideally on every OMAP platform
supported in mainline. So is better to wait until we are sure that this does
not cause a regression on any platform than ending reverting the patch like
it happened last time.
Thanks a lot and best regards,
Javier
[1]: https://lkml.org/lkml/2013/8/26/260
drivers/gpio/gpio-omap.c | 54 +++++++++++++++++++++++++++---------------------
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0ff4355..218ce3d 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -420,6 +420,29 @@ static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
return 0;
}
+static void _set_gpio_mode(struct gpio_bank *bank, unsigned offset)
+{
+ if (bank->regs->pinctrl) {
+ void __iomem *reg = bank->base + bank->regs->pinctrl;
+
+ /* Claim the pin for MPU */
+ __raw_writel(__raw_readl(reg) | (1 << offset), reg);
+ }
+
+ if (bank->regs->ctrl && !bank->mod_usage) {
+ void __iomem *reg = bank->base + bank->regs->ctrl;
+ u32 ctrl;
+
+ ctrl = __raw_readl(reg);
+ /* Module is enabled, clocks are not gated */
+ ctrl &= ~GPIO_MOD_CTRL_BIT;
+ __raw_writel(ctrl, reg);
+ bank->context.ctrl = ctrl;
+ }
+
+ bank->mod_usage |= 1 << offset;
+}
+
static int gpio_irq_type(struct irq_data *d, unsigned type)
{
struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
@@ -427,8 +450,8 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
int retval;
unsigned long flags;
- if (WARN_ON(!bank->mod_usage))
- return -EINVAL;
+ if (!bank->mod_usage)
+ pm_runtime_get_sync(bank->dev);
#ifdef CONFIG_ARCH_OMAP1
if (d->irq > IH_MPUIO_BASE)
@@ -438,6 +461,11 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
if (!gpio)
gpio = irq_to_gpio(bank, d->hwirq);
+ spin_lock_irqsave(&bank->lock, flags);
+ _set_gpio_mode(bank, GPIO_INDEX(bank, gpio));
+ _set_gpio_direction(bank, GPIO_INDEX(bank, gpio), 1);
+ spin_unlock_irqrestore(&bank->lock, flags);
+
if (type & ~IRQ_TYPE_SENSE_MASK)
return -EINVAL;
@@ -611,27 +639,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
* request_irq() or set_irq_type().
*/
_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-
- if (bank->regs->pinctrl) {
- void __iomem *reg = bank->base + bank->regs->pinctrl;
-
- /* Claim the pin for MPU */
- __raw_writel(__raw_readl(reg) | (1 << offset), reg);
- }
-
- if (bank->regs->ctrl && !bank->mod_usage) {
- void __iomem *reg = bank->base + bank->regs->ctrl;
- u32 ctrl;
-
- ctrl = __raw_readl(reg);
- /* Module is enabled, clocks are not gated */
- ctrl &= ~GPIO_MOD_CTRL_BIT;
- __raw_writel(ctrl, reg);
- bank->context.ctrl = ctrl;
- }
-
- bank->mod_usage |= 1 << offset;
-
+ _set_gpio_mode(bank, offset);
spin_unlock_irqrestore(&bank->lock, flags);
return 0;
--
1.8.4.rc3
On 09/22/2013 08:40 AM, Javier Martinez Canillas wrote:
> To use a GPIO pin as an interrupt line, two previous configurations
> have to be made:
>
> a) Map the GPIO pin as an interrupt line into the Linux irq space
> b) Enable the GPIO bank and configure the GPIO direction as input
>
> Most GPIO/IRQ chip drivers just create a mapping for every single
> GPIO pin with irq_create_mapping() on .probe so users usually can
> assume a) and only have to do b) by using the following sequence:
>
> gpio_request(gpio, "foo IRQ");
> gpio_direction_input(gpio);
>
> and then request a IRQ with:
>
> irq = gpio_to_irq(gpio);
> request_irq(irq, ...);
>
> Some drivers know that their IRQ line is being driven by a GPIO
> and use a similar sequence as the described above but others are
> not aware or don't care wether their IRQ is a real line from an
> interrupt controller or a GPIO pin acting as an IRQ.
> ...
I think that explanation is a bit like retro-actively implying that
drivers /should/ be aware of whether their IRQ is a GPIO or not, and
should be acting differently. However, they should not.
I would much rather see a simpler patch description along the lines of:
The OMAP GPIO controller HW requires that a pin be configured in GPIO
mode in order to operate as an interrupt input. Since drivers should not
be aware of whether an interrupt pin is also a GPIO or not, the HW
should be fully configured/enabled as an IRQ if a driver solely uses IRQ
APIs such as request_irq, and never calls any GPIO-related APIs. As
such, add the missing HW setup to the OMAP GPIO controller's irq_chip
driver.
The code change looks like it does what I would expect though.
On 09/23/2013 06:14 PM, Stephen Warren wrote:
> On 09/22/2013 08:40 AM, Javier Martinez Canillas wrote:
>> To use a GPIO pin as an interrupt line, two previous configurations
>> have to be made:
>>
>> a) Map the GPIO pin as an interrupt line into the Linux irq space
>> b) Enable the GPIO bank and configure the GPIO direction as input
>>
>> Most GPIO/IRQ chip drivers just create a mapping for every single
>> GPIO pin with irq_create_mapping() on .probe so users usually can
>> assume a) and only have to do b) by using the following sequence:
>>
>> gpio_request(gpio, "foo IRQ");
>> gpio_direction_input(gpio);
>>
>> and then request a IRQ with:
>>
>> irq = gpio_to_irq(gpio);
>> request_irq(irq, ...);
>>
>> Some drivers know that their IRQ line is being driven by a GPIO
>> and use a similar sequence as the described above but others are
>> not aware or don't care wether their IRQ is a real line from an
>> interrupt controller or a GPIO pin acting as an IRQ.
>> ...
>
> I think that explanation is a bit like retro-actively implying that
> drivers /should/ be aware of whether their IRQ is a GPIO or not, and
> should be acting differently. However, they should not.
>
I know the patch description is rather verbose but since we have been discussing
this a lot and people have different opinions I wanted to explain some context
and the motivation for the patch.
> I would much rather see a simpler patch description along the lines of:
>
> The OMAP GPIO controller HW requires that a pin be configured in GPIO
> mode in order to operate as an interrupt input. Since drivers should not
> be aware of whether an interrupt pin is also a GPIO or not, the HW
> should be fully configured/enabled as an IRQ if a driver solely uses IRQ
> APIs such as request_irq, and never calls any GPIO-related APIs. As
> such, add the missing HW setup to the OMAP GPIO controller's irq_chip
> driver.
>
Thanks for the suggestion, I'll use something like that when I do a proper post
as a PATCH and not RFC.
> The code change looks like it does what I would expect though.
>
Great, let's see what is the feedback from Santosh and Kevin about the
implementation since they are the maintainers of this driver.
I really hope we can find a solution to this long standing issue.
Thanks a lot and best regards,
Javier
* Javier Martinez Canillas <[email protected]> [130922 07:49]:
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -420,6 +420,29 @@ static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
> return 0;
> }
>
> +static void _set_gpio_mode(struct gpio_bank *bank, unsigned offset)
> +{
> + if (bank->regs->pinctrl) {
> + void __iomem *reg = bank->base + bank->regs->pinctrl;
> +
> + /* Claim the pin for MPU */
> + __raw_writel(__raw_readl(reg) | (1 << offset), reg);
> + }
> +
> + if (bank->regs->ctrl && !bank->mod_usage) {
> + void __iomem *reg = bank->base + bank->regs->ctrl;
> + u32 ctrl;
> +
> + ctrl = __raw_readl(reg);
> + /* Module is enabled, clocks are not gated */
> + ctrl &= ~GPIO_MOD_CTRL_BIT;
> + __raw_writel(ctrl, reg);
> + bank->context.ctrl = ctrl;
> + }
> +
> + bank->mod_usage |= 1 << offset;
> +}
> +
> static int gpio_irq_type(struct irq_data *d, unsigned type)
> {
> struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> @@ -427,8 +450,8 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
> int retval;
> unsigned long flags;
>
> - if (WARN_ON(!bank->mod_usage))
> - return -EINVAL;
> + if (!bank->mod_usage)
> + pm_runtime_get_sync(bank->dev);
>
> #ifdef CONFIG_ARCH_OMAP1
> if (d->irq > IH_MPUIO_BASE)
> @@ -438,6 +461,11 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
> if (!gpio)
> gpio = irq_to_gpio(bank, d->hwirq);
>
> + spin_lock_irqsave(&bank->lock, flags);
> + _set_gpio_mode(bank, GPIO_INDEX(bank, gpio));
> + _set_gpio_direction(bank, GPIO_INDEX(bank, gpio), 1);
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> if (type & ~IRQ_TYPE_SENSE_MASK)
> return -EINVAL;
>
Hmm does this still work for legacy platform data based
drivers that are doing gpio_request() first?
And what's the path for clearing things for PM when free_irq()
gets called? It seems that this would leave the GPIO bank
enabled causing a PM regression?
Other than the two concerns above it seems that this might
be the way to go to fix the regression for the -rc cycle.
Regards,
Tony
On 09/23/2013 06:45 PM, Tony Lindgren wrote:
> * Javier Martinez Canillas <[email protected]> [130922 07:49]:
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -420,6 +420,29 @@ static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
>> return 0;
>> }
>>
>> +static void _set_gpio_mode(struct gpio_bank *bank, unsigned offset)
>> +{
>> + if (bank->regs->pinctrl) {
>> + void __iomem *reg = bank->base + bank->regs->pinctrl;
>> +
>> + /* Claim the pin for MPU */
>> + __raw_writel(__raw_readl(reg) | (1 << offset), reg);
>> + }
>> +
>> + if (bank->regs->ctrl && !bank->mod_usage) {
>> + void __iomem *reg = bank->base + bank->regs->ctrl;
>> + u32 ctrl;
>> +
>> + ctrl = __raw_readl(reg);
>> + /* Module is enabled, clocks are not gated */
>> + ctrl &= ~GPIO_MOD_CTRL_BIT;
>> + __raw_writel(ctrl, reg);
>> + bank->context.ctrl = ctrl;
>> + }
>> +
>> + bank->mod_usage |= 1 << offset;
>> +}
>> +
>> static int gpio_irq_type(struct irq_data *d, unsigned type)
>> {
>> struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> @@ -427,8 +450,8 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
>> int retval;
>> unsigned long flags;
>>
>> - if (WARN_ON(!bank->mod_usage))
>> - return -EINVAL;
>> + if (!bank->mod_usage)
>> + pm_runtime_get_sync(bank->dev);
>>
>> #ifdef CONFIG_ARCH_OMAP1
>> if (d->irq > IH_MPUIO_BASE)
>> @@ -438,6 +461,11 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
>> if (!gpio)
>> gpio = irq_to_gpio(bank, d->hwirq);
>>
>> + spin_lock_irqsave(&bank->lock, flags);
>> + _set_gpio_mode(bank, GPIO_INDEX(bank, gpio));
>> + _set_gpio_direction(bank, GPIO_INDEX(bank, gpio), 1);
>> + spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> if (type & ~IRQ_TYPE_SENSE_MASK)
>> return -EINVAL;
>>
Hi Tony, thanks a lot for your feedback.
>
> Hmm does this still work for legacy platform data based
> drivers that are doing gpio_request() first?
>
Yes it still work when booting using board files. I tested on my OMAP3 board and
it worked in both DT and legacy booting mode.
> And what's the path for clearing things for PM when free_irq()
> gets called? It seems that this would leave the GPIO bank
> enabled causing a PM regression?
>
Indeed, I did set bank->mod_usage |= 1 << offset so the bank is enabled if the
device goes to suspended and then resumed but I completely forget about the
clearing path when the IRQ is freed.
Which makes me think that we should probably maintain two usage variables, one
for GPIO and another one for IRQ and check both of them on the suspend/resume pm
functions.
> Other than the two concerns above it seems that this might
> be the way to go to fix the regression for the -rc cycle.
>
> Regards,
>
> Tony
>
Great, I'll do these changes when posting as a proper PATCH.
Thanks a lot and best regards,
Javier
* Javier Martinez Canillas <[email protected]> [130923 10:09]:
> On 09/23/2013 06:45 PM, Tony Lindgren wrote:
> >
> > Hmm does this still work for legacy platform data based
> > drivers that are doing gpio_request() first?
> >
>
> Yes it still work when booting using board files. I tested on my OMAP3 board and
> it worked in both DT and legacy booting mode.
OK great.
> > And what's the path for clearing things for PM when free_irq()
> > gets called? It seems that this would leave the GPIO bank
> > enabled causing a PM regression?
> >
>
> Indeed, I did set bank->mod_usage |= 1 << offset so the bank is enabled if the
> device goes to suspended and then resumed but I completely forget about the
> clearing path when the IRQ is freed.
>
> Which makes me think that we should probably maintain two usage variables, one
> for GPIO and another one for IRQ and check both of them on the suspend/resume pm
> functions.
Yes that it seems that they should be treated separately.
Regards,
Tony
Javier,
On Monday 23 September 2013 01:07 PM, Tony Lindgren wrote:
> * Javier Martinez Canillas <[email protected]> [130923 10:09]:
>> On 09/23/2013 06:45 PM, Tony Lindgren wrote:
>>>
>>> Hmm does this still work for legacy platform data based
>>> drivers that are doing gpio_request() first?
>>>
>>
>> Yes it still work when booting using board files. I tested on my OMAP3 board and
>> it worked in both DT and legacy booting mode.
>
> OK great.
>
>>> And what's the path for clearing things for PM when free_irq()
>>> gets called? It seems that this would leave the GPIO bank
>>> enabled causing a PM regression?
>>>
>>
>> Indeed, I did set bank->mod_usage |= 1 << offset so the bank is enabled if the
>> device goes to suspended and then resumed but I completely forget about the
>> clearing path when the IRQ is freed.
>>
>> Which makes me think that we should probably maintain two usage variables, one
>> for GPIO and another one for IRQ and check both of them on the suspend/resume pm
>> functions.
>
> Yes that it seems that they should be treated separately.
>
As discussed on IRC, the patch as such is fine after the mentioned fixup,
I would like to hear back if Linus W/Grant is fine with the approach. Not sure
if I missed the discussion, but the proposed patch is deviation from
traditional method of doing gpio_request() first up to perform other
gpio operations.
Regards,
Santosh
On Sun, Sep 22, 2013 at 4:40 PM, Javier Martinez Canillas
<[email protected]> wrote:
> To use a GPIO pin as an interrupt line, two previous configurations
> have to be made:
>
> a) Map the GPIO pin as an interrupt line into the Linux irq space
> b) Enable the GPIO bank and configure the GPIO direction as input
>
> Most GPIO/IRQ chip drivers just create a mapping for every single
> GPIO pin with irq_create_mapping() on .probe so users usually can
> assume a) and only have to do b) by using the following sequence:
>
> gpio_request(gpio, "foo IRQ");
> gpio_direction_input(gpio);
>
> and then request a IRQ with:
>
> irq = gpio_to_irq(gpio);
> request_irq(irq, ...);
I guess I have to live with this approach, but I'd like to - if possible -
address my pet issue.
- It is OK that the HW get set up as GPIO input by the IRQ
request function alone. (Through gpio_irq_type I guess).
- When a second caller calls omap_gpio_request() it should
be OK as well, but only if the flags corresponds to the
previously enabled input mode. Else it should be
disallowed.
- The same should happen for _set_gpio_direction() if a pin
previously set up for IRQ gets a request to be used as
output.
If this cannot be tracked in the driver, it is certainly a candidate
for something that gpiolib should be doing. And then I'm open to
solutions to how we can do that.
If this needs to be applied pronto to fix the regression I'm
happy with that too, if we add a big boilerplate stating the above
problem and that it needs to be *fixed* at some point.
But in either case I want this to be tested on OMAP1 before
I apply it, as in a Tested-by tag.
Yours,
Linus Walleij
On 09/23/2013 10:15 PM, Linus Walleij wrote:
> On Sun, Sep 22, 2013 at 4:40 PM, Javier Martinez Canillas
> <[email protected]> wrote:
>
>> To use a GPIO pin as an interrupt line, two previous configurations
>> have to be made:
>>
>> a) Map the GPIO pin as an interrupt line into the Linux irq space
>> b) Enable the GPIO bank and configure the GPIO direction as input
>>
>> Most GPIO/IRQ chip drivers just create a mapping for every single
>> GPIO pin with irq_create_mapping() on .probe so users usually can
>> assume a) and only have to do b) by using the following sequence:
>>
>> gpio_request(gpio, "foo IRQ");
>> gpio_direction_input(gpio);
>>
>> and then request a IRQ with:
>>
>> irq = gpio_to_irq(gpio);
>> request_irq(irq, ...);
>
> I guess I have to live with this approach, but I'd like to - if possible -
> address my pet issue.
>
> - It is OK that the HW get set up as GPIO input by the IRQ
> request function alone. (Through gpio_irq_type I guess).
>
Yes, this is how is made on this patch indeed.
> - When a second caller calls omap_gpio_request() it should
> be OK as well, but only if the flags corresponds to the
> previously enabled input mode. Else it should be
> disallowed.
>
> - The same should happen for _set_gpio_direction() if a pin
> previously set up for IRQ gets a request to be used as
> output.
>
> If this cannot be tracked in the driver, it is certainly a candidate
> for something that gpiolib should be doing. And then I'm open to
> solutions to how we can do that.
>
Ok, this can be tracked in the driver, will add it when posting v2 soon.
> If this needs to be applied pronto to fix the regression I'm
> happy with that too, if we add a big boilerplate stating the above
> problem and that it needs to be *fixed* at some point.
>
> But in either case I want this to be tested on OMAP1 before
> I apply it, as in a Tested-by tag.
>
Agreed. Even though this is a fix for a long standing issue I prefer it to be
tested extensively than applying the patch in a rush just to learn that causes
regressions and have to be reverted as it happens last time.
So as you said let's wait until we have a few Tested-by tags by people using
different OMAP platforms specially OMAP1.
> Yours,
> Linus Walleij
>
Thanks a lot and best regards,
Javier
Hi,
On Monday 23 September 2013 10:37 PM, Tony Lindgren wrote:
> * Javier Martinez Canillas <[email protected]> [130923 10:09]:
>> On 09/23/2013 06:45 PM, Tony Lindgren wrote:
>>> Hmm does this still work for legacy platform data based
>>> drivers that are doing gpio_request() first?
>>>
>> Yes it still work when booting using board files. I tested on my OMAP3 board and
>> it worked in both DT and legacy booting mode.
> OK great.
>
>>> And what's the path for clearing things for PM when free_irq()
>>> gets called? It seems that this would leave the GPIO bank
>>> enabled causing a PM regression?
>>>
>> Indeed, I did set bank->mod_usage |= 1 << offset so the bank is enabled if the
>> device goes to suspended and then resumed but I completely forget about the
>> clearing path when the IRQ is freed.
>>
>> Which makes me think that we should probably maintain two usage variables, one
>> for GPIO and another one for IRQ and check both of them on the suspend/resume pm
>> functions.
> Yes that it seems that they should be treated separately.
To understand, why cant the flag be cleared in gpio_irq_shutdown ?
Regards,
Sricharan
On 09/24/2013 09:39 AM, Sricharan R wrote:
> Hi,
> On Monday 23 September 2013 10:37 PM, Tony Lindgren wrote:
>> * Javier Martinez Canillas <[email protected]> [130923 10:09]:
>>> On 09/23/2013 06:45 PM, Tony Lindgren wrote:
>>>> Hmm does this still work for legacy platform data based
>>>> drivers that are doing gpio_request() first?
>>>>
>>> Yes it still work when booting using board files. I tested on my OMAP3 board and
>>> it worked in both DT and legacy booting mode.
>> OK great.
>>
>>>> And what's the path for clearing things for PM when free_irq()
>>>> gets called? It seems that this would leave the GPIO bank
>>>> enabled causing a PM regression?
>>>>
>>> Indeed, I did set bank->mod_usage |= 1 << offset so the bank is enabled if the
>>> device goes to suspended and then resumed but I completely forget about the
>>> clearing path when the IRQ is freed.
>>>
>>> Which makes me think that we should probably maintain two usage variables, one
>>> for GPIO and another one for IRQ and check both of them on the suspend/resume pm
>>> functions.
>> Yes that it seems that they should be treated separately.
> To understand, why cant the flag be cleared in gpio_irq_shutdown ?
Hi Sricharan,
Without this patch today drivers do this:
gpio_request(gpio, "foo IRQ"); // bank->mod_usage |= 1 << offset
gpio_direction_input(gpio);
and then request a IRQ with:
irq = gpio_to_irq(gpio);
request_irq(irq, ...);
later on its cleanup path:
free_irq(irq, dev);
gpio_free(gpio) // bank->mod_usage &= ~(1 << offset);
So if you clear the flag on gpio_irq_shutdown then bank module won't be enabled
after a suspend making drivers using the GPIO after freeing the IRQ to fail.
So the idea is to have something like this:
a) Drivers that request both the GPIO and IRQ
gpio_request(gpio, "foo IRQ"); // bank->mod_usage |= 1 << offset
gpio_direction_input(gpio);
irq = gpio_to_irq(gpio);
request_irq(irq, ...); // bank->irq_usage |= 1 << offset
free_irq(irq, dev); // bank->irq_usage &= ~(1 << offset);
gpio_free(gpio) // bank->mod_usage &= ~(1 << offset);
b) Drivers that just request the IRQ:
irq = gpio_to_irq(gpio);
request_irq(irq, ...); // bank->irq_usage |= 1 << offset
free_irq(irq, dev); // bank->irq_usage &= ~(1 << offset);
So irq_usage or mod_usage is set means that the bank has to be enabled and if
both are not set means that the bank module can be disabled.
>
> Regards,
> Sricharan
>
Best regards,
Javier
On Tuesday 24 September 2013 01:24 PM, Javier Martinez Canillas wrote:
> On 09/24/2013 09:39 AM, Sricharan R wrote:
>> Hi,
>> On Monday 23 September 2013 10:37 PM, Tony Lindgren wrote:
>>> * Javier Martinez Canillas <[email protected]> [130923 10:09]:
>>>> On 09/23/2013 06:45 PM, Tony Lindgren wrote:
>>>>> Hmm does this still work for legacy platform data based
>>>>> drivers that are doing gpio_request() first?
>>>>>
>>>> Yes it still work when booting using board files. I tested on my OMAP3 board and
>>>> it worked in both DT and legacy booting mode.
>>> OK great.
>>>
>>>>> And what's the path for clearing things for PM when free_irq()
>>>>> gets called? It seems that this would leave the GPIO bank
>>>>> enabled causing a PM regression?
>>>>>
>>>> Indeed, I did set bank->mod_usage |= 1 << offset so the bank is enabled if the
>>>> device goes to suspended and then resumed but I completely forget about the
>>>> clearing path when the IRQ is freed.
>>>>
>>>> Which makes me think that we should probably maintain two usage variables, one
>>>> for GPIO and another one for IRQ and check both of them on the suspend/resume pm
>>>> functions.
>>> Yes that it seems that they should be treated separately.
>> To understand, why cant the flag be cleared in gpio_irq_shutdown ?
> Hi Sricharan,
>
> Without this patch today drivers do this:
>
> gpio_request(gpio, "foo IRQ"); // bank->mod_usage |= 1 << offset
> gpio_direction_input(gpio);
>
> and then request a IRQ with:
>
> irq = gpio_to_irq(gpio);
> request_irq(irq, ...);
>
> later on its cleanup path:
>
> free_irq(irq, dev);
> gpio_free(gpio) // bank->mod_usage &= ~(1 << offset);
>
> So if you clear the flag on gpio_irq_shutdown then bank module won't be enabled
> after a suspend making drivers using the GPIO after freeing the IRQ to fail.
>
> So the idea is to have something like this:
>
> a) Drivers that request both the GPIO and IRQ
>
> gpio_request(gpio, "foo IRQ"); // bank->mod_usage |= 1 << offset
> gpio_direction_input(gpio);
>
> irq = gpio_to_irq(gpio);
> request_irq(irq, ...); // bank->irq_usage |= 1 << offset
>
> free_irq(irq, dev); // bank->irq_usage &= ~(1 << offset);
> gpio_free(gpio) // bank->mod_usage &= ~(1 << offset);
>
> b) Drivers that just request the IRQ:
>
> irq = gpio_to_irq(gpio);
> request_irq(irq, ...); // bank->irq_usage |= 1 << offset
> free_irq(irq, dev); // bank->irq_usage &= ~(1 << offset);
>
> So irq_usage or mod_usage is set means that the bank has to be enabled and if
> both are not set means that the bank module can be disabled.
>
Ok get it. Thanks.
Regards,
Sricharan
* Javier Martinez Canillas <[email protected]> [130923 22:49]:
> On 09/23/2013 10:15 PM, Linus Walleij wrote:
> > <[email protected]> wrote:
> > - When a second caller calls omap_gpio_request() it should
> > be OK as well, but only if the flags corresponds to the
> > previously enabled input mode. Else it should be
> > disallowed.
> >
> > - The same should happen for _set_gpio_direction() if a pin
> > previously set up for IRQ gets a request to be used as
> > output.
> >
> > If this cannot be tracked in the driver, it is certainly a candidate
> > for something that gpiolib should be doing. And then I'm open to
> > solutions to how we can do that.
> >
>
> Ok, this can be tracked in the driver, will add it when posting v2 soon.
>
> > If this needs to be applied pronto to fix the regression I'm
> > happy with that too, if we add a big boilerplate stating the above
> > problem and that it needs to be *fixed* at some point.
In the mainline kernel, the regression is happening at least for
smsc911x using boards, so that's omap3-igep.dtsi and omap3-tobi.dts
currently. Also MMC card detection for omap4 is failing. Then
of course there are tons of boards where we don't have the .dts
files in the mainline kernel yet.
So maybe let's do a minimal fix for the -rc cycle first?
Then the extra checks can be queued for the next merge window
if it gets too intrusive.
> > But in either case I want this to be tested on OMAP1 before
> > I apply it, as in a Tested-by tag.
> >
>
> Agreed. Even though this is a fix for a long standing issue I prefer it to be
> tested extensively than applying the patch in a rush just to learn that causes
> regressions and have to be reverted as it happens last time.
>
> So as you said let's wait until we have a few Tested-by tags by people using
> different OMAP platforms specially OMAP1.
Yup.
Regards,
Tony