| Message ID | 1401090486-4414-4-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 26, 2014 at 09:47:58AM +0200, Hans de Goede wrote: > With level triggered interrupt mask / unmask will get called for each > interrupt, doing the somewhat expensive mux setting on each unmask thus is > not a good idea. Instead move it to the set_type callback, which is typically > done only once for each irq. *This* is the bad idea. Nothing prevents you from calling gpio_get_value whenever you just got your interrupt, that will change the muxing, and never change it back, effectively breaking the interrupts. Maxime
Hi, On 05/27/2014 10:09 AM, Maxime Ripard wrote: > On Mon, May 26, 2014 at 09:47:58AM +0200, Hans de Goede wrote: >> With level triggered interrupt mask / unmask will get called for each >> interrupt, doing the somewhat expensive mux setting on each unmask thus is >> not a good idea. Instead move it to the set_type callback, which is typically >> done only once for each irq. > > *This* is the bad idea. Nothing prevents you from calling > gpio_get_value whenever you just got your interrupt, that will change > the muxing, and never change it back, effectively breaking the > interrupts. Hmm, interesting point, but your assuming 2 things which are both not true: 1) That calling gpiod_get_value changes the muxing, which is not true, all gpiod_get_value variants end up in drivers/gpio/gpiolib.c _gpiod_get_raw_value() which does no such thing 2) That unmask will always get called after the gpio_get_value to restore the mux setting for us, which is not guaranteed at all. Before this patch set pinctrl-sunxi.c was using handle_simple_irq which does not mask / unmask at all. And even with an irq-handler which masks / unmasks, what if the irq wakes up a thread and that thread then does the gpio_get_value ? Note this is *exactly* what e.g. the mmc gpio card-detect code does, it uses a thread to read the gpio for debouncing. Luckily 2. is not a problem, since 1. means that the mux won't get changed at all so we don't need to change it back. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: > With level triggered interrupt mask / unmask will get called for each > interrupt, doing the somewhat expensive mux setting on each unmask thus is > not a good idea. Instead move it to the set_type callback, which is typically > done only once for each irq. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Yes move it out of mask/unmask but no, not into set_type(). Can you not use the irqchip startup()/shutdown() callbacks instead? And how come we have no clean separation between gpiochip and the pinmux parts here, why cant we just call pinctrl_request_gpio() and pinctrl_free_gpio() here instead? Or maybe pinctrl_gpio_direction_input() and have that set up the muxing in the pinctrl driver side? It looks slightly convoluted. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 27, 2014 at 04:18:29PM +0200, Linus Walleij wrote: > On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > > With level triggered interrupt mask / unmask will get called for each > > interrupt, doing the somewhat expensive mux setting on each unmask thus is > > not a good idea. Instead move it to the set_type callback, which is typically > > done only once for each irq. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Yes move it out of mask/unmask but no, not into set_type(). > > Can you not use the irqchip startup()/shutdown() callbacks > instead? I think we can use irq_request_resources then https://lkml.org/lkml/2014/3/12/307 We could even merge the gpio_to_irq code into it. It's called in __setup_irq, so it's guaranteed to be called, and it will bail out early without doing anything, since it's one of the first thing __setup_irq does. > And how come we have no clean separation between gpiochip > and the pinmux parts here, why cant we just call > pinctrl_request_gpio() and pinctrl_free_gpio() here instead? > Or maybe pinctrl_gpio_direction_input() and have that set > up the muxing in the pinctrl driver side? Because the function it has to be muxed to is neither gpio_in or gpio_out, and it's not even considered a gpio. It really is just another muxing function, like i2c or mmc. Maxime
On Tue, May 27, 2014 at 11:01:03AM +0200, Hans de Goede wrote: > Hi, > > On 05/27/2014 10:09 AM, Maxime Ripard wrote: > > On Mon, May 26, 2014 at 09:47:58AM +0200, Hans de Goede wrote: > >> With level triggered interrupt mask / unmask will get called for each > >> interrupt, doing the somewhat expensive mux setting on each unmask thus is > >> not a good idea. Instead move it to the set_type callback, which is typically > >> done only once for each irq. > > > > *This* is the bad idea. Nothing prevents you from calling > > gpio_get_value whenever you just got your interrupt, that will change > > the muxing, and never change it back, effectively breaking the > > interrupts. > > Hmm, interesting point, but your assuming 2 things which are both not true: > > 1) That calling gpiod_get_value changes the muxing, which is not true, all > gpiod_get_value variants end up in drivers/gpio/gpiolib.c _gpiod_get_raw_value() > which does no such thing Somehow I was convinced it was the case, but yeah, it doesn't really make much sense, especially since you can get the value of an output, without wanting to change it to input. > > 2) That unmask will always get called after the gpio_get_value to restore the mux > setting for us, which is not guaranteed at all. Before this patch set pinctrl-sunxi.c > was using handle_simple_irq which does not mask / unmask at all. > > And even with an irq-handler which masks / unmasks, what if the irq wakes up > a thread and that thread then does the gpio_get_value ? > > Note this is *exactly* what e.g. the mmc gpio card-detect code does, it uses > a thread to read the gpio for debouncing. > > Luckily 2. is not a problem, since 1. means that the mux won't get changed at all > so we don't need to change it back. Ok. Maxime
Hi, On 05/28/2014 11:36 AM, Maxime Ripard wrote: > On Tue, May 27, 2014 at 04:18:29PM +0200, Linus Walleij wrote: >> On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> >>> With level triggered interrupt mask / unmask will get called for each >>> interrupt, doing the somewhat expensive mux setting on each unmask thus is >>> not a good idea. Instead move it to the set_type callback, which is typically >>> done only once for each irq. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> Yes move it out of mask/unmask but no, not into set_type(). >> >> Can you not use the irqchip startup()/shutdown() callbacks >> instead? > > I think we can use irq_request_resources then > https://lkml.org/lkml/2014/3/12/307 Sounds good, I'll modify the patch to move it here before posting a v2 of this series. Note v2 likely won't happen till this weekend, -ENOTIME. > We could even merge the gpio_to_irq code into it. Erm, no we need that as a separate function for the gpio_chip's to_irq callback. > It's called in __setup_irq, so it's guaranteed to be called, and it > will bail out early without doing anything, since it's one of the > first thing __setup_irq does. Right, as I said above, this sounds good. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 28, 2014 at 11:51:52AM +0200, Hans de Goede wrote: > Hi, > > On 05/28/2014 11:36 AM, Maxime Ripard wrote: > > On Tue, May 27, 2014 at 04:18:29PM +0200, Linus Walleij wrote: > >> On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >>> With level triggered interrupt mask / unmask will get called for each > >>> interrupt, doing the somewhat expensive mux setting on each unmask thus is > >>> not a good idea. Instead move it to the set_type callback, which is typically > >>> done only once for each irq. > >>> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> > >> Yes move it out of mask/unmask but no, not into set_type(). > >> > >> Can you not use the irqchip startup()/shutdown() callbacks > >> instead? > > > > I think we can use irq_request_resources then > > https://lkml.org/lkml/2014/3/12/307 > > Sounds good, I'll modify the patch to move it here before posting a v2 of > this series. Note v2 likely won't happen till this weekend, -ENOTIME. > > > We could even merge the gpio_to_irq code into it. > > Erm, no we need that as a separate function for the gpio_chip's to_irq > callback. Linus sent a patch stating otherwise a few weeks ago, and was suggesting moving it to irq_startup. https://lkml.org/lkml/2014/5/9/50 Maxime
Hi, On 05/28/2014 12:33 PM, Maxime Ripard wrote: > On Wed, May 28, 2014 at 11:51:52AM +0200, Hans de Goede wrote: >> Hi, >> >> On 05/28/2014 11:36 AM, Maxime Ripard wrote: >>> On Tue, May 27, 2014 at 04:18:29PM +0200, Linus Walleij wrote: >>>> On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>>> With level triggered interrupt mask / unmask will get called for each >>>>> interrupt, doing the somewhat expensive mux setting on each unmask thus is >>>>> not a good idea. Instead move it to the set_type callback, which is typically >>>>> done only once for each irq. >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> >>>> Yes move it out of mask/unmask but no, not into set_type(). >>>> >>>> Can you not use the irqchip startup()/shutdown() callbacks >>>> instead? >>> >>> I think we can use irq_request_resources then >>> https://lkml.org/lkml/2014/3/12/307 >> >> Sounds good, I'll modify the patch to move it here before posting a v2 of >> this series. Note v2 likely won't happen till this weekend, -ENOTIME. >> >>> We could even merge the gpio_to_irq code into it. >> >> Erm, no we need that as a separate function for the gpio_chip's to_irq >> callback. > > Linus sent a patch stating otherwise a few weeks ago, and was > suggesting moving it to irq_startup. > > https://lkml.org/lkml/2014/5/9/50 That is not going to work, that patch uses gpiochip_irqchip_add, which in turn uses gpiochip_to_irq as to_irq handler, which assumes that gpio offset == irq offset, which is not true for sunxi-pinctrl. Specifically gpio_chio_to_irq does: static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { return irq_find_mapping(chip->irqdomain, offset); } Where as the sunxi code does (simplified): static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset) { struct sunxi_desc_function *desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, offset, "irq"); return irq_find_mapping(pctl->domain, desc->irqnum);\ } Note the extra level of indirection. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 31, 2014 at 11:13:05AM +0200, Hans de Goede wrote: > Hi, > > On 05/28/2014 12:33 PM, Maxime Ripard wrote: > >On Wed, May 28, 2014 at 11:51:52AM +0200, Hans de Goede wrote: > >>Hi, > >> > >>On 05/28/2014 11:36 AM, Maxime Ripard wrote: > >>>On Tue, May 27, 2014 at 04:18:29PM +0200, Linus Walleij wrote: > >>>>On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: > >>>> > >>>>>With level triggered interrupt mask / unmask will get called for each > >>>>>interrupt, doing the somewhat expensive mux setting on each unmask thus is > >>>>>not a good idea. Instead move it to the set_type callback, which is typically > >>>>>done only once for each irq. > >>>>> > >>>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> > >>>>Yes move it out of mask/unmask but no, not into set_type(). > >>>> > >>>>Can you not use the irqchip startup()/shutdown() callbacks > >>>>instead? > >>> > >>>I think we can use irq_request_resources then > >>>https://lkml.org/lkml/2014/3/12/307 > >> > >>Sounds good, I'll modify the patch to move it here before posting a v2 of > >>this series. Note v2 likely won't happen till this weekend, -ENOTIME. > >> > >>>We could even merge the gpio_to_irq code into it. > >> > >>Erm, no we need that as a separate function for the gpio_chip's to_irq > >>callback. > > > >Linus sent a patch stating otherwise a few weeks ago, and was > >suggesting moving it to irq_startup. > > > >https://lkml.org/lkml/2014/5/9/50 > > That is not going to work, that patch uses gpiochip_irqchip_add, > which in turn uses gpiochip_to_irq as to_irq handler, which > assumes that gpio offset == irq offset, which is not true for > sunxi-pinctrl. > > Specifically gpio_chio_to_irq does: > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > return irq_find_mapping(chip->irqdomain, offset); > } > > Where as the sunxi code does (simplified): > > static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset) > { > struct sunxi_desc_function *desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, offset, "irq"); > return irq_find_mapping(pctl->domain, desc->irqnum);\ > } Yes, I know it's not going to work, but my point was that gpio_to_irq might not be called by the drivers, and it's still valid not to do it. So we might end up with a driver requesting an interrupt that won't be muxed to it. But thinking more about it, it would be wrong to remove the .to_irq callback completely either, since drivers might need a "secondary" interrupt (like the card detect one for an MMC driver), and a lot of these use an additional gpio property to do so. So we need to keep it. Nevermind then. Maxime
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c index ec60c2e..d1675c9 100644 --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c @@ -542,6 +542,7 @@ static int sunxi_pinctrl_irq_set_type(struct irq_data *d, struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d); u32 reg = sunxi_irq_cfg_reg(d->hwirq); u8 index = sunxi_irq_cfg_offset(d->hwirq); + struct sunxi_desc_function *func; unsigned long flags; u32 regval; u8 mode; @@ -574,6 +575,12 @@ static int sunxi_pinctrl_irq_set_type(struct irq_data *d, spin_unlock_irqrestore(&pctl->lock, flags); + func = sunxi_pinctrl_desc_find_function_by_pin(pctl, + pctl->irq_array[d->hwirq], "irq"); + + /* Change muxing to INT mode */ + sunxi_pmx_set(pctl->pctl_dev, pctl->irq_array[d->hwirq], func->muxval); + return 0; } @@ -619,19 +626,11 @@ static void sunxi_pinctrl_irq_mask(struct irq_data *d) static void sunxi_pinctrl_irq_unmask(struct irq_data *d) { struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d); - struct sunxi_desc_function *func; u32 reg = sunxi_irq_ctrl_reg(d->hwirq); u8 idx = sunxi_irq_ctrl_offset(d->hwirq); unsigned long flags; u32 val; - func = sunxi_pinctrl_desc_find_function_by_pin(pctl, - pctl->irq_array[d->hwirq], - "irq"); - - /* Change muxing to INT mode */ - sunxi_pmx_set(pctl->pctl_dev, pctl->irq_array[d->hwirq], func->muxval); - spin_lock_irqsave(&pctl->lock, flags); /* Unmask the IRQ */
With level triggered interrupt mask / unmask will get called for each interrupt, doing the somewhat expensive mux setting on each unmask thus is not a good idea. Instead move it to the set_type callback, which is typically done only once for each irq. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/pinctrl/sunxi/pinctrl-sunxi.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)