| Message ID | 1379860848-29020-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- 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
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 -- 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
* Javier Martinez Canillas <javier.martinez@collabora.co.uk> [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 -- 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
On 09/23/2013 06:45 PM, Tony Lindgren wrote: > * Javier Martinez Canillas <javier.martinez@collabora.co.uk> [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 -- 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
* Javier Martinez Canillas <javier.martinez@collabora.co.uk> [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 -- 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
Javier, On Monday 23 September 2013 01:07 PM, Tony Lindgren wrote: > * Javier Martinez Canillas <javier.martinez@collabora.co.uk> [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 -- 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
On Sun, Sep 22, 2013 at 4:40 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> 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 -- 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
On 09/23/2013 10:15 PM, Linus Walleij wrote: > On Sun, Sep 22, 2013 at 4:40 PM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> 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 -- 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
Hi, On Monday 23 September 2013 10:37 PM, Tony Lindgren wrote: > * Javier Martinez Canillas <javier.martinez@collabora.co.uk> [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 -- 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
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 <javier.martinez@collabora.co.uk> [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 -- 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
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 <javier.martinez@collabora.co.uk> [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 -- 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
* Javier Martinez Canillas <javier.martinez@collabora.co.uk> [130923 22:49]: > On 09/23/2013 10:15 PM, Linus Walleij wrote: > > <javier.martinez@collabora.co.uk> 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 -- 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
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;
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 <javier.martinez@collabora.co.uk> --- 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(-)