| Message ID | 1590253873-11556-2-git-send-email-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip: qcom: pdc: Introduce irq_set_wake call | expand |
On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote: > With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib > overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable > callback is implemented then genirq takes unlazy path to disable irq. > > Underlying irqchip may not want to implement irq_disable callback to lazy > disable irq when client drivers invokes disable_irq(). By overriding > irq_disable callback, gpiolib ends up always unlazy disabling IRQ. > > Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only > if irqchip implemented irq_disable. In cases where irq_disable is not > implemented irq_mask is overridden. Similarly override irq_enable callback > only if irqchip implemented irq_enable otherwise irq_unmask is overridden. > > Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> I definitely want Hans Verkuils test and review on this, since it is a usecase that he is really dependent on. Also the irqchip people preferredly. But it does seem to mop up my mistakes and fix this up properly! So with some testing I'll be happy to merge it, even this one patch separately if Hans can verify that it works. Yours, Linus Walleij
On 25/05/2020 13:55, Linus Walleij wrote: > On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote: > >> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib >> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable >> callback is implemented then genirq takes unlazy path to disable irq. >> >> Underlying irqchip may not want to implement irq_disable callback to lazy >> disable irq when client drivers invokes disable_irq(). By overriding >> irq_disable callback, gpiolib ends up always unlazy disabling IRQ. >> >> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only >> if irqchip implemented irq_disable. In cases where irq_disable is not >> implemented irq_mask is overridden. Similarly override irq_enable callback >> only if irqchip implemented irq_enable otherwise irq_unmask is overridden. >> >> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > I definitely want Hans Verkuils test and review on this, since it > is a usecase that he is really dependent on. Maulik, since I am no longer subscribed to linux-gpio, can you mail the series to me? I have two use-cases, but I can only test one (I don't have access to the SBC I need to test the other use-case for the next few months). Once I have the whole series I'll try to test the first use-case and at least look into the code if this series could affect the second use-case. Regards, Hans > > Also the irqchip people preferredly. > > But it does seem to mop up my mistakes and fix this up properly! > > So with some testing I'll be happy to merge it, even this one > patch separately if Hans can verify that it works. > > Yours, > Linus Walleij >
Hi, On 5/25/2020 5:52 PM, Hans Verkuil wrote: > On 25/05/2020 13:55, Linus Walleij wrote: >> On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote: >> >>> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib >>> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable >>> callback is implemented then genirq takes unlazy path to disable irq. >>> >>> Underlying irqchip may not want to implement irq_disable callback to lazy >>> disable irq when client drivers invokes disable_irq(). By overriding >>> irq_disable callback, gpiolib ends up always unlazy disabling IRQ. >>> >>> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only >>> if irqchip implemented irq_disable. In cases where irq_disable is not >>> implemented irq_mask is overridden. Similarly override irq_enable callback >>> only if irqchip implemented irq_enable otherwise irq_unmask is overridden. >>> >>> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) >>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> I definitely want Hans Verkuils test and review on this, since it >> is a usecase that he is really dependent on. > Maulik, since I am no longer subscribed to linux-gpio, can you mail the > series to me? > > I have two use-cases, but I can only test one (I don't have access to the > SBC I need to test the other use-case for the next few months). > > Once I have the whole series I'll try to test the first use-case and at > least look into the code if this series could affect the second use-case. > > Regards, > > Hans Hi Hans, Mailed you the entire series. Thanks, Maulik > >> Also the irqchip people preferredly. >> >> But it does seem to mop up my mistakes and fix this up properly! >> >> So with some testing I'll be happy to merge it, even this one >> patch separately if Hans can verify that it works. >> >> Yours, >> Linus Walleij >>
Quoting Maulik Shah (2020-05-23 10:11:10) > With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib > overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable > callback is implemented then genirq takes unlazy path to disable irq. > > Underlying irqchip may not want to implement irq_disable callback to lazy > disable irq when client drivers invokes disable_irq(). By overriding > irq_disable callback, gpiolib ends up always unlazy disabling IRQ. > > Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only > if irqchip implemented irq_disable. In cases where irq_disable is not > implemented irq_mask is overridden. Similarly override irq_enable callback > only if irqchip implemented irq_enable otherwise irq_unmask is overridden. > > Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) This isn't a proper Fixes line. Should have quotes Fixes: 461c1a7d4733 ("gpiolib: override irq_enable/disable") > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > drivers/gpio/gpiolib.c | 55 +++++++++++++++++++++++++++++---------------- > include/linux/gpio/driver.h | 13 +++++++++++ > 2 files changed, 49 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index eaa0e20..3810cd0 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) > gpiochip_relres_irq(gc, d->hwirq); > } > > +static void gpiochip_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + > + if (gc->irq.irq_mask) > + gc->irq.irq_mask(d); > + gpiochip_disable_irq(gc, d->hwirq); How does this work in the lazy case when I want to drive the GPIO? Say I have a GPIO that is also an interrupt. The code would look like struct gpio_desc *gpio = gpiod_get(...) unsigned int girq = gpiod_to_irq(gpio) request_irq(girq, ...); disable_irq(girq); gpiod_direction_output(gpio, 1); In the lazy case genirq wouldn't call the mask function until the first interrupt arrived on the GPIO line. If that never happened then wouldn't we be blocked in gpiod_direction_output() when the test_bit() sees FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving gpios? > +} > + > +static void gpiochip_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + > + gpiochip_enable_irq(gc, d->hwirq); > + if (gc->irq.irq_unmask) > + gc->irq.irq_unmask(d); > +} > +
On 25/05/2020 13:55, Linus Walleij wrote: > On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote: > >> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib >> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable >> callback is implemented then genirq takes unlazy path to disable irq. >> >> Underlying irqchip may not want to implement irq_disable callback to lazy >> disable irq when client drivers invokes disable_irq(). By overriding >> irq_disable callback, gpiolib ends up always unlazy disabling IRQ. >> >> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only >> if irqchip implemented irq_disable. In cases where irq_disable is not >> implemented irq_mask is overridden. Similarly override irq_enable callback >> only if irqchip implemented irq_enable otherwise irq_unmask is overridden. >> >> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > I definitely want Hans Verkuils test and review on this, since it > is a usecase that he is really dependent on. For this patch: Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting with kernel v5.5. The CEC pin is an open drain pin that is used in both input and output directions and has an interrupt (which is of course disabled while in output mode). With this patch the interrupt can no longer be requested: [ 4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ [ 4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ [ 4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835 [ 4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5 You can easily test this with a RPi by enabling cec-gpio: ------------------------------------------------------ diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts index 054ecaa355c9..87d6ed711ce2 100644 --- a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts +++ b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts @@ -29,6 +29,13 @@ wifi_pwrseq: wifi-pwrseq { compatible = "mmc-pwrseq-simple"; reset-gpios = <&expgpio 1 GPIO_ACTIVE_LOW>; }; + + cec-gpio@7 { + compatible = "cec-gpio"; + cec-gpios = <&gpio 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; + hpd-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; + v5-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; + }; }; &firmware { ------------------------------------------------------ Reverting that patch makes it work again. I'm happy to test a fix for this. Regards, Hans > > Also the irqchip people preferredly. > > But it does seem to mop up my mistakes and fix this up properly! > > So with some testing I'll be happy to merge it, even this one > patch separately if Hans can verify that it works. > > Yours, > Linus Walleij >
Hi, On 5/27/2020 3:14 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-05-23 10:11:10) >> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib >> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable >> callback is implemented then genirq takes unlazy path to disable irq. >> >> Underlying irqchip may not want to implement irq_disable callback to lazy >> disable irq when client drivers invokes disable_irq(). By overriding >> irq_disable callback, gpiolib ends up always unlazy disabling IRQ. >> >> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only >> if irqchip implemented irq_disable. In cases where irq_disable is not >> implemented irq_mask is overridden. Similarly override irq_enable callback >> only if irqchip implemented irq_enable otherwise irq_unmask is overridden. >> >> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) > This isn't a proper Fixes line. Should have quotes > > Fixes: 461c1a7d4733 ("gpiolib: override irq_enable/disable") Thanks for pointing this, i will address in next revision. > >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> drivers/gpio/gpiolib.c | 55 +++++++++++++++++++++++++++++---------------- >> include/linux/gpio/driver.h | 13 +++++++++++ >> 2 files changed, 49 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index eaa0e20..3810cd0 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) >> gpiochip_relres_irq(gc, d->hwirq); >> } >> >> +static void gpiochip_irq_mask(struct irq_data *d) >> +{ >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> + >> + if (gc->irq.irq_mask) >> + gc->irq.irq_mask(d); >> + gpiochip_disable_irq(gc, d->hwirq); > How does this work in the lazy case when I want to drive the GPIO? Say I > have a GPIO that is also an interrupt. The code would look like > > struct gpio_desc *gpio = gpiod_get(...) > unsigned int girq = gpiod_to_irq(gpio) > > request_irq(girq, ...); > > disable_irq(girq); > gpiod_direction_output(gpio, 1); > > In the lazy case genirq wouldn't call the mask function until the first > interrupt arrived on the GPIO line. If that never happened then wouldn't > we be blocked in gpiod_direction_output() when the test_bit() sees > FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving > gpios? The client driver can decide to unlazy disable IRQ with below API... irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY); This will immediatly invoke mask function (unlazy disable) from genirq, even though irq_disable is not implemented. Thanks, Maulik > >> +} >> + >> +static void gpiochip_irq_unmask(struct irq_data *d) >> +{ >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> + >> + gpiochip_enable_irq(gc, d->hwirq); >> + if (gc->irq.irq_unmask) >> + gc->irq.irq_unmask(d); >> +} >> +
On Wed, May 27, 2020 at 12:38 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated > open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting > with kernel v5.5. > > The CEC pin is an open drain pin that is used in both input and output > directions and has an interrupt (which is of course disabled while in > output mode). > > With this patch the interrupt can no longer be requested: > > [ 4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ > > [ 4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ > [ 4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835 > [ 4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5 There is nothing conceptually wrong with that patch so I think we need to have the irqchip code check if it is input *OR* open drain. I'll send a separate patch for this, it should be an easy fix. Yours, Linus Walleij
On Wed, May 27, 2020 at 03:56:16PM +0200, Linus Walleij wrote: > On Wed, May 27, 2020 at 12:38 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated > > open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting > > with kernel v5.5. > > > > The CEC pin is an open drain pin that is used in both input and output > > directions and has an interrupt (which is of course disabled while in > > output mode). > > > > With this patch the interrupt can no longer be requested: > > > > [ 4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ > > > > [ 4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ > > [ 4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835 > > [ 4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5 > > There is nothing conceptually wrong with that patch so I think we > need to have the irqchip code check if it is input *OR* open drain. Right - because, before this patch: - if you have hardware that is capable of open-drain outputs, gpiolib will report that the GPIO is in *output* mode. - if you have hardware that is not capable of open-drain outputs, and gpiolib emulates the open-drain nature by switching the GPIO direction, then gpiolib will report that the GPIO is in input mode. What my patch does is provide consistent behaviour across all cases by making open-drain outputs consistently report output mode irrespective of the underlying hardware. Whether an open-drain GPIO should be viewed as an input or as an output is an interesting question, but the important thing as far as this subsystem goes is to have consistent behaviour, otherwise having a subsystem is utterly pointless. However, consider whether it is sane to request a change of state via gpiod_set_value() of a gpio that reports itself as input, and have that request honoured and cause a change of state of the "input" - clearly that is not sane. In any case: cec->cec_gpio = devm_gpiod_get(dev, "cec", GPIOD_OUT_HIGH_OPEN_DRAIN); if (IS_ERR(cec->cec_gpio)) return PTR_ERR(cec->cec_gpio); cec->cec_irq = gpiod_to_irq(cec->cec_gpio); ... ret = devm_request_irq(dev, cec->cec_irq, cec_gpio_irq_handler, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, cec->adap->name, cec); So, the GPIO is requested in an _output_ mode, so it would be weird if it were subsequently to be reported as an input just because it ended up being an emulated open-drain output. Hence, the above code would have failed if cec-gpio were used with hardware that had open-drain semantics without needing the gpiolib emulation for the same reason that's now triggering; such hardware would report that the pin is in output mode, and the interrupt allocation would fail. While it's regrettable that fixing the inconsistency has caused a regression, I think that has found another place where the semantics aren't entirely sane, and as Linus says, _that_ needs fixing.
Quoting Maulik Shah (2020-05-27 04:26:14) > On 5/27/2020 3:14 PM, Stephen Boyd wrote: > > Quoting Maulik Shah (2020-05-23 10:11:10) > >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > >> index eaa0e20..3810cd0 100644 > >> --- a/drivers/gpio/gpiolib.c > >> +++ b/drivers/gpio/gpiolib.c > >> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) > >> gpiochip_relres_irq(gc, d->hwirq); > >> } > >> > >> +static void gpiochip_irq_mask(struct irq_data *d) > >> +{ > >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > >> + > >> + if (gc->irq.irq_mask) > >> + gc->irq.irq_mask(d); > >> + gpiochip_disable_irq(gc, d->hwirq); > > How does this work in the lazy case when I want to drive the GPIO? Say I > > have a GPIO that is also an interrupt. The code would look like > > > > struct gpio_desc *gpio = gpiod_get(...) > > unsigned int girq = gpiod_to_irq(gpio) > > > > request_irq(girq, ...); > > > > disable_irq(girq); > > gpiod_direction_output(gpio, 1); > > > > In the lazy case genirq wouldn't call the mask function until the first > > interrupt arrived on the GPIO line. If that never happened then wouldn't > > we be blocked in gpiod_direction_output() when the test_bit() sees > > FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving > > gpios? > > The client driver can decide to unlazy disable IRQ with below API... > > irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY); > > This will immediatly invoke mask function (unlazy disable) from genirq, > even though irq_disable is not implemented. > Sure a consumer can disable the lazy feature, but that shouldn't be required to make this work. The flag was introduced in commit e9849777d0e2 ("genirq: Add flag to force mask in disable_irq[_nosync]()") specifically to help devices that can't disable the interrupt in their own device avoid a double interrupt.
Hi, On 5/28/2020 6:38 AM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-05-27 04:26:14) >> On 5/27/2020 3:14 PM, Stephen Boyd wrote: >>> Quoting Maulik Shah (2020-05-23 10:11:10) >>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >>>> index eaa0e20..3810cd0 100644 >>>> --- a/drivers/gpio/gpiolib.c >>>> +++ b/drivers/gpio/gpiolib.c >>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) >>>> gpiochip_relres_irq(gc, d->hwirq); >>>> } >>>> >>>> +static void gpiochip_irq_mask(struct irq_data *d) >>>> +{ >>>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >>>> + >>>> + if (gc->irq.irq_mask) >>>> + gc->irq.irq_mask(d); >>>> + gpiochip_disable_irq(gc, d->hwirq); >>> How does this work in the lazy case when I want to drive the GPIO? Say I >>> have a GPIO that is also an interrupt. The code would look like >>> >>> struct gpio_desc *gpio = gpiod_get(...) >>> unsigned int girq = gpiod_to_irq(gpio) >>> >>> request_irq(girq, ...); >>> >>> disable_irq(girq); >>> gpiod_direction_output(gpio, 1); >>> >>> In the lazy case genirq wouldn't call the mask function until the first >>> interrupt arrived on the GPIO line. If that never happened then wouldn't >>> we be blocked in gpiod_direction_output() when the test_bit() sees >>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving >>> gpios? >> The client driver can decide to unlazy disable IRQ with below API... >> >> irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY); >> >> This will immediatly invoke mask function (unlazy disable) from genirq, >> even though irq_disable is not implemented. >> > Sure a consumer can disable the lazy feature, but that shouldn't be > required to make this work. The flag was introduced in commit > e9849777d0e2 ("genirq: Add flag to force mask in > disable_irq[_nosync]()") specifically to help devices that can't disable > the interrupt in their own device avoid a double interrupt. i don't think this will be a problem. Case 1) Client driver have locked gpio to be used as IRQ using gpiochip_lock_as_irq() In this case, When client driver want to change the direction for a gpio, they will invoke gpiod_direction_output(). I see it checks for two flags (pasted below), if GPIO is used as IRQ and whether its enabled IRQ or not. /* GPIOs used for enabled IRQs shall not be set as output */ if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) && test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) { The first one (FLAG_USED_AS_IRQ) is set only if client driver in past have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq() then it never gets unlocked until clients invoke gpiochip_unlock_as_irq(). So i presume the client driver which in past locked gpio to be used as IRQ, now wants to change direction then it will a. first unlock to use as IRQ b. then change the direction. Once it unlocks in step (a), both these flags will be cleared and there won't be any error in changing direction in step (b). Case 2) Client driver did not lock gpio to be used as IRQ. In this case, it will be straight forward to change the direction, as FLAG_USED_AS_IRQ is never set. Thanks, Maulik
On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote: > With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib > overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable > callback is implemented then genirq takes unlazy path to disable irq. > > Underlying irqchip may not want to implement irq_disable callback to lazy > disable irq when client drivers invokes disable_irq(). By overriding > irq_disable callback, gpiolib ends up always unlazy disabling IRQ. > > Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only > if irqchip implemented irq_disable. In cases where irq_disable is not > implemented irq_mask is overridden. Similarly override irq_enable callback > only if irqchip implemented irq_enable otherwise irq_unmask is overridden. > > Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> I applied this patch 1/4 to the GPIO tree since it is nice on its own and it is soon merge window. Yours, Linus Walleij
Quoting Maulik Shah (2020-05-28 06:11:23) > Hi, > > On 5/28/2020 6:38 AM, Stephen Boyd wrote: > > Quoting Maulik Shah (2020-05-27 04:26:14) > >> On 5/27/2020 3:14 PM, Stephen Boyd wrote: > >>> Quoting Maulik Shah (2020-05-23 10:11:10) > >>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > >>>> index eaa0e20..3810cd0 100644 > >>>> --- a/drivers/gpio/gpiolib.c > >>>> +++ b/drivers/gpio/gpiolib.c > >>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) > >>>> gpiochip_relres_irq(gc, d->hwirq); > >>>> } > >>>> > >>>> +static void gpiochip_irq_mask(struct irq_data *d) > >>>> +{ > >>>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > >>>> + > >>>> + if (gc->irq.irq_mask) > >>>> + gc->irq.irq_mask(d); > >>>> + gpiochip_disable_irq(gc, d->hwirq); > >>> How does this work in the lazy case when I want to drive the GPIO? Say I > >>> have a GPIO that is also an interrupt. The code would look like > >>> > >>> struct gpio_desc *gpio = gpiod_get(...) > >>> unsigned int girq = gpiod_to_irq(gpio) > >>> > >>> request_irq(girq, ...); > >>> > >>> disable_irq(girq); > >>> gpiod_direction_output(gpio, 1); > >>> > >>> In the lazy case genirq wouldn't call the mask function until the first > >>> interrupt arrived on the GPIO line. If that never happened then wouldn't > >>> we be blocked in gpiod_direction_output() when the test_bit() sees > >>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving > >>> gpios? > >> The client driver can decide to unlazy disable IRQ with below API... > >> > >> irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY); > >> > >> This will immediatly invoke mask function (unlazy disable) from genirq, > >> even though irq_disable is not implemented. > >> > > Sure a consumer can disable the lazy feature, but that shouldn't be > > required to make this work. The flag was introduced in commit > > e9849777d0e2 ("genirq: Add flag to force mask in > > disable_irq[_nosync]()") specifically to help devices that can't disable > > the interrupt in their own device avoid a double interrupt. > i don't think this will be a problem. > > Case 1) Client driver have locked gpio to be used as IRQ using > gpiochip_lock_as_irq() > > In this case, When client driver want to change the direction for a > gpio, they will invoke gpiod_direction_output(). > I see it checks for two flags (pasted below), if GPIO is used as IRQ and > whether its enabled IRQ or not. > > /* GPIOs used for enabled IRQs shall not be set as output */ > if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) && > test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) { > > The first one (FLAG_USED_AS_IRQ) is set only if client driver in past > have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq() > then it never gets unlocked until clients invoke gpiochip_unlock_as_irq(). > > So i presume the client driver which in past locked gpio to be used as > IRQ, now wants to change direction then it will > a. first unlock to use as IRQ > b. then change the direction. How does a client driver unlock to use as an IRQ though? I don't understand how that is done. gpiochip_lock_as_irq() isn't a gpio consumer API, it's a gpiochip/gpio provider API. > > Once it unlocks in step (a), both these flags will be cleared and there > won't be any error in changing direction in step (b).
Hi, On 5/29/2020 3:52 AM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-05-28 06:11:23) >> Hi, >> >> On 5/28/2020 6:38 AM, Stephen Boyd wrote: >>> Quoting Maulik Shah (2020-05-27 04:26:14) >>>> On 5/27/2020 3:14 PM, Stephen Boyd wrote: >>>>> Quoting Maulik Shah (2020-05-23 10:11:10) >>>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >>>>>> index eaa0e20..3810cd0 100644 >>>>>> --- a/drivers/gpio/gpiolib.c >>>>>> +++ b/drivers/gpio/gpiolib.c >>>>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) >>>>>> gpiochip_relres_irq(gc, d->hwirq); >>>>>> } >>>>>> >>>>>> +static void gpiochip_irq_mask(struct irq_data *d) >>>>>> +{ >>>>>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >>>>>> + >>>>>> + if (gc->irq.irq_mask) >>>>>> + gc->irq.irq_mask(d); >>>>>> + gpiochip_disable_irq(gc, d->hwirq); >>>>> How does this work in the lazy case when I want to drive the GPIO? Say I >>>>> have a GPIO that is also an interrupt. The code would look like >>>>> >>>>> struct gpio_desc *gpio = gpiod_get(...) >>>>> unsigned int girq = gpiod_to_irq(gpio) >>>>> >>>>> request_irq(girq, ...); >>>>> >>>>> disable_irq(girq); >>>>> gpiod_direction_output(gpio, 1); >>>>> >>>>> In the lazy case genirq wouldn't call the mask function until the first >>>>> interrupt arrived on the GPIO line. If that never happened then wouldn't >>>>> we be blocked in gpiod_direction_output() when the test_bit() sees >>>>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving >>>>> gpios? >>>> The client driver can decide to unlazy disable IRQ with below API... >>>> >>>> irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY); >>>> >>>> This will immediatly invoke mask function (unlazy disable) from genirq, >>>> even though irq_disable is not implemented. >>>> >>> Sure a consumer can disable the lazy feature, but that shouldn't be >>> required to make this work. The flag was introduced in commit >>> e9849777d0e2 ("genirq: Add flag to force mask in >>> disable_irq[_nosync]()") specifically to help devices that can't disable >>> the interrupt in their own device avoid a double interrupt. >> i don't think this will be a problem. >> >> Case 1) Client driver have locked gpio to be used as IRQ using >> gpiochip_lock_as_irq() >> >> In this case, When client driver want to change the direction for a >> gpio, they will invoke gpiod_direction_output(). >> I see it checks for two flags (pasted below), if GPIO is used as IRQ and >> whether its enabled IRQ or not. >> >> /* GPIOs used for enabled IRQs shall not be set as output */ >> if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) && >> test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) { >> >> The first one (FLAG_USED_AS_IRQ) is set only if client driver in past >> have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq() >> then it never gets unlocked until clients invoke gpiochip_unlock_as_irq(). >> >> So i presume the client driver which in past locked gpio to be used as >> IRQ, now wants to change direction then it will >> a. first unlock to use as IRQ >> b. then change the direction. > How does a client driver unlock to use as an IRQ though? I don't > understand how that is done. gpiochip_lock_as_irq() isn't a gpio > consumer API, it's a gpiochip/gpio provider API. >>In the lazy case genirq wouldn't call the mask function until the first >>interrupt arrived on the GPIO line. If that never happened then wouldn't >>we be blocked in gpiod_direction_output() when the test_bit() sees >>FLAG_USED_AS_IRQ? What i was trying to explain in above two cases is.. FLAG_USED_AS_IRQ is set only when client driver locks GPIO to be used as IRQ with gpiochip_lock_as_irq() API call. Coming to query of test_bit() seeing this flag as set won't be a problem. Lets take an example... 1. Client driver locks gpio to be used as IRQ gpiochip_lock_as_irq() This makes gpiolib set two flags.. a. FLAG_USED_AS_IRQ b. FLAG_IRQ_IS_ENABLED Note that this is the only API which sets the flag (a) FLAG_USED_AS_IRQ. 2. During gpiochip_disable_irq() it only clears the flag (b) but the flag (a) is still set. 3. Now client driver does disable_irq()... With this patch, in case the irq_chip did not implement irq_disable callback then irq_mask will be overridden. so during first disable_irq() call, the gpiolib won't come to immediatly know that interrupt is disabled (lazy disable) hence both these flags are still set. 4. After disabling irq, client driver want to change the direction, so it wants to now call gpiod_direction_output() But before calling this, client driver knows that in step (1), client driver locked GPIO to be used only as IRQ. So GPIO cannot be used as any other purpose other than IRQ till the point its locked. hence before calling gpiod_direction_output(), it has to first invoke gpiochip_unlock_as_irq(). Calling this will clear both flags and allow GPIO to change the direction. Now client can invoke gpiod_direction_output() and the test_bit() check inside this won't complain. Case 1) in my previous mail was where in above example client driver did step (1) which locks the gpio as IRQ then its expected to do unlock in step (4) before changing direction. so no issue in case 1) regarding test_bit complain. Case 2) is where driver didn't even do step-1, so in step-4, they can directly invoke gpiod_direction_output() client didn't lock, so the flag FLAG_USED_AS_IRQ is never set, so test_bit for this flag won't complain. So in both cases it looks to be working as expected to me. Hope that its answers your query. Thanks, Maulik >> Once it unlocks in step (a), both these flags will be cleared and there >> won't be any error in changing direction in step (b).
On Fri, May 29, 2020 at 12:27 AM Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Linus Walleij (2020-05-28 14:33:36) > > On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote: > > > > > With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib > > > overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable > > > callback is implemented then genirq takes unlazy path to disable irq. > > > > > > Underlying irqchip may not want to implement irq_disable callback to lazy > > > disable irq when client drivers invokes disable_irq(). By overriding > > > irq_disable callback, gpiolib ends up always unlazy disabling IRQ. > > > > > > Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only > > > if irqchip implemented irq_disable. In cases where irq_disable is not > > > implemented irq_mask is overridden. Similarly override irq_enable callback > > > only if irqchip implemented irq_enable otherwise irq_unmask is overridden. > > > > > > Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) > > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > > > I applied this patch 1/4 to the GPIO tree since it is nice on its own > > and it is soon merge window. > > > > Can you please clarify how this doesn't break things as discussed in a > fork of this thread[1]? > > [1] https://lore.kernel.org/r/948defc1-5ea0-adbb-185b-5f5a81f2e28a@codeaurora.org I don't really understand I'm afraid. Hans tested this on the one system that uses the method to disable the IRQ and turn the line into an output, play around with it, then switch it back to input (actually open drain) and then use the IRQ again. Is something broken in practice or in theory? Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index eaa0e20..3810cd0 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d) gpiochip_relres_irq(gc, d->hwirq); } +static void gpiochip_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + + if (gc->irq.irq_mask) + gc->irq.irq_mask(d); + gpiochip_disable_irq(gc, d->hwirq); +} + +static void gpiochip_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + + gpiochip_enable_irq(gc, d->hwirq); + if (gc->irq.irq_unmask) + gc->irq.irq_unmask(d); +} + static void gpiochip_irq_enable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); gpiochip_enable_irq(gc, d->hwirq); - if (gc->irq.irq_enable) - gc->irq.irq_enable(d); - else - gc->irq.chip->irq_unmask(d); + gc->irq.irq_enable(d); } static void gpiochip_irq_disable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); - /* - * Since we override .irq_disable() we need to mimic the - * behaviour of __irq_disable() in irq/chip.c. - * First call .irq_disable() if it exists, else mimic the - * behaviour of mask_irq() which calls .irq_mask() if - * it exists. - */ - if (gc->irq.irq_disable) - gc->irq.irq_disable(d); - else if (gc->irq.chip->irq_mask) - gc->irq.chip->irq_mask(d); + gc->irq.irq_disable(d); gpiochip_disable_irq(gc, d->hwirq); } @@ -2515,10 +2520,22 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gc) "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n"); return; } - gc->irq.irq_enable = irqchip->irq_enable; - gc->irq.irq_disable = irqchip->irq_disable; - irqchip->irq_enable = gpiochip_irq_enable; - irqchip->irq_disable = gpiochip_irq_disable; + + if (irqchip->irq_disable) { + gc->irq.irq_disable = irqchip->irq_disable; + irqchip->irq_disable = gpiochip_irq_disable; + } else { + gc->irq.irq_mask = irqchip->irq_mask; + irqchip->irq_mask = gpiochip_irq_mask; + } + + if (irqchip->irq_enable) { + gc->irq.irq_enable = irqchip->irq_enable; + irqchip->irq_enable = gpiochip_irq_enable; + } else { + gc->irq.irq_unmask = irqchip->irq_unmask; + irqchip->irq_unmask = gpiochip_irq_unmask; + } } /** diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 8c41ae4..c8bcaf3 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -253,6 +253,19 @@ struct gpio_irq_chip { * Store old irq_chip irq_disable callback */ void (*irq_disable)(struct irq_data *data); + /** + * @irq_unmask: + * + * Store old irq_chip irq_unmask callback + */ + void (*irq_unmask)(struct irq_data *data); + + /** + * @irq_mask: + * + * Store old irq_chip irq_mask callback + */ + void (*irq_mask)(struct irq_data *data); }; /**
With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable callback is implemented then genirq takes unlazy path to disable irq. Underlying irqchip may not want to implement irq_disable callback to lazy disable irq when client drivers invokes disable_irq(). By overriding irq_disable callback, gpiolib ends up always unlazy disabling IRQ. Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only if irqchip implemented irq_disable. In cases where irq_disable is not implemented irq_mask is overridden. Similarly override irq_enable callback only if irqchip implemented irq_enable otherwise irq_unmask is overridden. Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/gpio/gpiolib.c | 55 +++++++++++++++++++++++++++++---------------- include/linux/gpio/driver.h | 13 +++++++++++ 2 files changed, 49 insertions(+), 19 deletions(-)