| Message ID | 53E4FD1B.6060600@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote: > > > It seems as if the first call to exynos_irq_set_type() that is made by OF is a > no-op while the second call is the one that actually setups the hw correctly. > Does this make any sense? Maybe is related to the pin not being muxed in the > correct function when the "interrupts" property is parsed by OF? > So after a conversation with Tomasz Figa over IRC the problem was after all that the pin was reconfigured. The IRQ trigger type resulted to be just a red herring and not a direct cause. The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type function handler. So what happened was that OF parsed the "interrupts" property and called exynos_irq_set_type() which did the pinmux setup. But after that, due a DTS pinctrl configuration the pin function was changed as a GPIO input and that happened before the atmel driver was probed. So when the driver called request_threaded_irq(), the correct flags were used but the pin was not configured as an IRQ anymore so IRQ were not fired. Setting a trigger type just had the side effect of calling exynos_irq_set_type() which again setup the pin as an IRQ. To fix the issue a variation of patch [0] will be posted that moves the IRQ pinmux setup from .irq_set_type to the .irq_request_resources function handler. That way the pin will be setup as IRQ regardless of the the trigger type [1] when someone calls request_[threaded]_irq(). Only the mentioned patch fixes the issue but Tomasz said that even a call to gpio_direction_{input,output} can change the pin configuration so he will post another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux reconfiguration. Thanks a lot and best regards, Javier [0]: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/34259/focus=34261 [1]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1162 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+CC Linus, as this became also pinctrl related. On 08.08.2014 20:29, Javier Martinez Canillas wrote: > Hello, > > On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote: >> >> >> It seems as if the first call to exynos_irq_set_type() that is made by OF is a >> no-op while the second call is the one that actually setups the hw correctly. >> Does this make any sense? Maybe is related to the pin not being muxed in the >> correct function when the "interrupts" property is parsed by OF? >> > > So after a conversation with Tomasz Figa over IRC the problem was after all that > the pin was reconfigured. The IRQ trigger type resulted to be just a red herring > and not a direct cause. > > The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type > function handler. So what happened was that OF parsed the "interrupts" property > and called exynos_irq_set_type() which did the pinmux setup. > > But after that, due a DTS pinctrl configuration the pin function was changed as > a GPIO input and that happened before the atmel driver was probed. So when the > driver called request_threaded_irq(), the correct flags were used but the pin > was not configured as an IRQ anymore so IRQ were not fired. > > Setting a trigger type just had the side effect of calling exynos_irq_set_type() > which again setup the pin as an IRQ. > > To fix the issue a variation of patch [0] will be posted that moves the IRQ > pinmux setup from .irq_set_type to the .irq_request_resources function handler. > That way the pin will be setup as IRQ regardless of the the trigger type [1] > when someone calls request_[threaded]_irq(). > > Only the mentioned patch fixes the issue but Tomasz said that even a call to > gpio_direction_{input,output} can change the pin configuration so he will post > another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux > reconfiguration. To add a bit more information about the hardware, setting up a GPIO interrupt on Samsung SoCs is a two-step operation - in addition to trigger configuration in a dedicated register, the pinmux must be also reconfigured to GPIO interrupt, which is a different function than normal GPIO input, although I/O-wise they both behave in the same way and gpio_get_value() can be used on a pin configured as IRQ as well. I'm afraid that such design implies that handling of this in the driver must be done on a very low level, because it involves three possible interfaces changing the pinmux - pinctrl, GPIO and irqchip and such subtletes like gpio_direction_input() that shouldn't neither fail if a pin is already configured as an interrupt nor change the configuration to normal input. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Aug 8, 2014 at 11:29 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello, > > On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote: >> >> >> It seems as if the first call to exynos_irq_set_type() that is made by OF is a >> no-op while the second call is the one that actually setups the hw correctly. >> Does this make any sense? Maybe is related to the pin not being muxed in the >> correct function when the "interrupts" property is parsed by OF? >> > > So after a conversation with Tomasz Figa over IRC the problem was after all that > the pin was reconfigured. The IRQ trigger type resulted to be just a red herring > and not a direct cause. > > The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type > function handler. So what happened was that OF parsed the "interrupts" property > and called exynos_irq_set_type() which did the pinmux setup. > > But after that, due a DTS pinctrl configuration the pin function was changed as > a GPIO input and that happened before the atmel driver was probed. So when the > driver called request_threaded_irq(), the correct flags were used but the pin > was not configured as an IRQ anymore so IRQ were not fired. > > Setting a trigger type just had the side effect of calling exynos_irq_set_type() > which again setup the pin as an IRQ. > > To fix the issue a variation of patch [0] will be posted that moves the IRQ > pinmux setup from .irq_set_type to the .irq_request_resources function handler. > That way the pin will be setup as IRQ regardless of the the trigger type [1] > when someone calls request_[threaded]_irq(). > > Only the mentioned patch fixes the issue but Tomasz said that even a call to > gpio_direction_{input,output} can change the pin configuration so he will post > another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux > reconfiguration. Would just making a device tree change fix this? AKA for the pin, do: samsung,pin-function = <0xf>; I have some vague recollection that I set interrupts to pin-function "0" by default for some reason (assuming they would go to 0xf when interrupts were enabled). ...but I can't for the life of me remember if it was a good reason or just seemed like the right thing to do. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Doug, On 08/08/2014 10:54 PM, Doug Anderson wrote: > Hi, >> >> To fix the issue a variation of patch [0] will be posted that moves the IRQ >> pinmux setup from .irq_set_type to the .irq_request_resources function handler. >> That way the pin will be setup as IRQ regardless of the the trigger type [1] >> when someone calls request_[threaded]_irq(). >> >> Only the mentioned patch fixes the issue but Tomasz said that even a call to >> gpio_direction_{input,output} can change the pin configuration so he will post >> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux >> reconfiguration. > > Would just making a device tree change fix this? AKA for the pin, do: > > samsung,pin-function = <0xf>; > Yes, when configuring the pin as 0xf (GPIO interrupt) instead of 0x0 (GPIO input) the issue is not present indeed. So after Nick answer my question about what I got wrong with the "linux,gpio-keymap" property, I will change the pin function when re-posing the DTS changes for the atmel touchpad. > I have some vague recollection that I set interrupts to pin-function > "0" by default for some reason (assuming they would go to 0xf when > interrupts were enabled). ...but I can't for the life of me remember > if it was a good reason or just seemed like the right thing to do. > It would be great to know if there is a good reason. I see indeed that all pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a difference between GPIO-IRQ and GPIO input I guess that it's better to change those to 0xf instead. What do you think? Regardless of this though I think that both the patch to move the IRQ pinmux setup from .irq_set_type to the .irq_request_resources and the patch to to prevent any pinmux reconfiguration are good improvements to avoid future issues like the one we found here. > -Doug > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Javier, On Fri, Aug 8, 2014 at 3:26 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >> I have some vague recollection that I set interrupts to pin-function >> "0" by default for some reason (assuming they would go to 0xf when >> interrupts were enabled). ...but I can't for the life of me remember >> if it was a good reason or just seemed like the right thing to do. >> > > It would be great to know if there is a good reason. I see indeed that all > pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are > using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a > difference between GPIO-IRQ and GPIO input I guess that it's better to change > those to 0xf instead. What do you think? I think it's worth trying out. If there are no problems with it then let's do it. My vague recollection is that I was worried that pinctrl would take effect right at driver probe time (maybe this used to happen? or maybe I imagined it?) and that configuring to 0xf at this point in time would cause a spurious interrupt. I can't remember ever testing it so it was probably just something I imagined. Even if it was configured as 0xf I'd imagine that the interrupt would be masked anyway so there should be no spurious interrupt, right? > Regardless of this though I think that both the patch to move the IRQ > pinmux setup from .irq_set_type to the .irq_request_resources and the patch to > to prevent any pinmux reconfiguration are good improvements to avoid future > issues like the one we found here. OK. I'll let you, Tomasz, and Linus figure out what's best here since I haven't done extensive thinking on it. ;) -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I guess this discussion is about drivers/pinctrl/samsung/pinctrl-exynos.c? Or else I'm not really following this... $SUBJECT is a bit confusing. On Sat, Aug 9, 2014 at 12:26 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Regardless of this though I think that both the patch to move the IRQ > pinmux setup from .irq_set_type to the .irq_request_resources and the patch to > to prevent any pinmux reconfiguration are good improvements to avoid future > issues like the one we found here. I think someone should look into switching the Samsung/Exynos pinctrl driver to the gpiolib irqchip helpers, I looked at it but was scared by the special wkup chip and stuff I can't test. The irqchip helpers will atleast help out in flagging GPIO lines as used for IRQs so the core can keep track of stuff and show that properly in debugfs. The orthogonality compliance between GPIO and irqchip must however be solved in the driver itself, the core only helps out in blocking some abuse of the API. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Linus, On 08/11/2014 10:32 AM, Linus Walleij wrote: > I guess this discussion is about drivers/pinctrl/samsung/pinctrl-exynos.c? > > Or else I'm not really following this... $SUBJECT is a bit confusing. > Yes, the thing is that at the beginning I (wrongly) thought that the IRQ trigger type flags defined in the DTS were discarded if someone called request_threaded_irq() with a flags != 0. And since the atmel touchpad driver was pass pdata->irqflags | IRQF_ONESHOT I posted this patch to get the trigger type parsed from DT and pass it to request_threaded_irq(). But after digging further I found that the issue was in the pinctrl-exynos driver and Tomasz explained to me that the real cause was that the pin was being configured from GPIO IRQ to GPIO input (which on other platforms is just the same mode but on Exynos are two different modes). At the end passing the trigger type to request_threaded_irq() just had the side effect to call the exynos .irq_set_type function handler that reconfigured the pin as GPIO-IRQ and that is why it made it to work. So I posted a patch from Tomasz that fixes the real cause [0]. I'm so sorry for the confusion. > Yours, > Linus Walleij > Best regards, Javier [0]: https://lkml.org/lkml/2014/8/8/962 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/kernel/irq/manage.c b/kernel/irq/manage.c index 3dc6a61..ed76b25 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1176,6 +1176,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (ret) goto out_mask; + } else if (irq == 283 /* mapped IRQ number for touchpad */) { + struct irq_chip *chip = desc->irq_data.chip; + chip->irq_set_type(&desc->irq_data, + irq_get_trigger_type(irq)); }