* [PATCH] gpio: pl061: add support for wakeup configuration @ 2015-11-27 17:19 Sudeep Holla 2015-12-09 15:08 ` Linus Walleij 2015-12-14 14:02 ` Linus Walleij 0 siblings, 2 replies; 7+ messages in thread From: Sudeep Holla @ 2015-11-27 17:19 UTC (permalink / raw) To: Linus Walleij, linux-gpio; +Cc: Sudeep Holla, linux-kernel, Alexandre Courbot The PL061 supports interrupts and those can be wakeup interrupts. We need to provide support for configuring those interrupts as wakeup sources. This patch adds irq_set_wake callback for PL061 so that GPIO interrupts can be configured as wakeup. Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Alexandre Courbot <gnurou@gmail.com> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/gpio/gpio-pl061.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c index 4d4b37676702..8b1cbd5767f9 100644 --- a/drivers/gpio/gpio-pl061.c +++ b/drivers/gpio/gpio-pl061.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/io.h> #include <linux/ioport.h> +#include <linux/interrupt.h> #include <linux/irq.h> #include <linux/irqchip/chained_irq.h> #include <linux/bitops.h> @@ -269,12 +270,20 @@ static void pl061_irq_ack(struct irq_data *d) spin_unlock(&chip->lock); } +static int pl061_irq_set_wake(struct irq_data *d, unsigned int state) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + + return irq_set_irq_wake(gc->irq_parent, state); +} + static struct irq_chip pl061_irqchip = { .name = "pl061", .irq_ack = pl061_irq_ack, .irq_mask = pl061_irq_mask, .irq_unmask = pl061_irq_unmask, .irq_set_type = pl061_irq_type, + .irq_set_wake = pl061_irq_set_wake, }; static int pl061_probe(struct amba_device *adev, const struct amba_id *id) -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] gpio: pl061: add support for wakeup configuration 2015-11-27 17:19 [PATCH] gpio: pl061: add support for wakeup configuration Sudeep Holla @ 2015-12-09 15:08 ` Linus Walleij 2015-12-09 15:20 ` Sudeep Holla 2015-12-14 14:02 ` Linus Walleij 1 sibling, 1 reply; 7+ messages in thread From: Linus Walleij @ 2015-12-09 15:08 UTC (permalink / raw) To: Sudeep Holla, Aubrey Li, Dmitry Torokhov, Rafael J. Wysocki Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexandre Courbot, Ulf Hansson On Fri, Nov 27, 2015 at 6:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > The PL061 supports interrupts and those can be wakeup interrupts. We > need to provide support for configuring those interrupts as wakeup > sources. > > This patch adds irq_set_wake callback for PL061 so that GPIO interrupts > can be configured as wakeup. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/gpio/gpio-pl061.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c > index 4d4b37676702..8b1cbd5767f9 100644 > --- a/drivers/gpio/gpio-pl061.c > +++ b/drivers/gpio/gpio-pl061.c > @@ -14,6 +14,7 @@ > #include <linux/module.h> > #include <linux/io.h> > #include <linux/ioport.h> > +#include <linux/interrupt.h> > #include <linux/irq.h> > #include <linux/irqchip/chained_irq.h> > #include <linux/bitops.h> > @@ -269,12 +270,20 @@ static void pl061_irq_ack(struct irq_data *d) > spin_unlock(&chip->lock); > } > > +static int pl061_irq_set_wake(struct irq_data *d, unsigned int state) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + > + return irq_set_irq_wake(gc->irq_parent, state); > +} > + > static struct irq_chip pl061_irqchip = { > .name = "pl061", > .irq_ack = pl061_irq_ack, > .irq_mask = pl061_irq_mask, > .irq_unmask = pl061_irq_unmask, > .irq_set_type = pl061_irq_type, > + .irq_set_wake = pl061_irq_set_wake, > }; Is this really all that is needed? Don't you need to call device_wakeup_enable(&adev->dev, 1); on the amba (primecell) device providing this GPIO, lest it may be suspended itself and render this exercise pointless. And if not, why not? How are GPIO chips providing IRQs actually going to do this, provided that they need to be kept awake themselves? Adding some people to the conversation. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpio: pl061: add support for wakeup configuration 2015-12-09 15:08 ` Linus Walleij @ 2015-12-09 15:20 ` Sudeep Holla 2015-12-10 18:22 ` Linus Walleij 0 siblings, 1 reply; 7+ messages in thread From: Sudeep Holla @ 2015-12-09 15:20 UTC (permalink / raw) To: Linus Walleij Cc: Aubrey Li, Dmitry Torokhov, Rafael J. Wysocki, Sudeep Holla, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexandre Courbot, Ulf Hansson On 09/12/15 15:08, Linus Walleij wrote: > On Fri, Nov 27, 2015 at 6:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> The PL061 supports interrupts and those can be wakeup interrupts. We >> need to provide support for configuring those interrupts as wakeup >> sources. >> >> This patch adds irq_set_wake callback for PL061 so that GPIO interrupts >> can be configured as wakeup. >> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Alexandre Courbot <gnurou@gmail.com> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/gpio/gpio-pl061.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c >> index 4d4b37676702..8b1cbd5767f9 100644 >> --- a/drivers/gpio/gpio-pl061.c >> +++ b/drivers/gpio/gpio-pl061.c >> @@ -14,6 +14,7 @@ >> #include <linux/module.h> >> #include <linux/io.h> >> #include <linux/ioport.h> >> +#include <linux/interrupt.h> >> #include <linux/irq.h> >> #include <linux/irqchip/chained_irq.h> >> #include <linux/bitops.h> >> @@ -269,12 +270,20 @@ static void pl061_irq_ack(struct irq_data *d) >> spin_unlock(&chip->lock); >> } >> >> +static int pl061_irq_set_wake(struct irq_data *d, unsigned int state) >> +{ >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> + >> + return irq_set_irq_wake(gc->irq_parent, state); >> +} >> + >> static struct irq_chip pl061_irqchip = { >> .name = "pl061", >> .irq_ack = pl061_irq_ack, >> .irq_mask = pl061_irq_mask, >> .irq_unmask = pl061_irq_unmask, >> .irq_set_type = pl061_irq_type, >> + .irq_set_wake = pl061_irq_set_wake, >> }; > > Is this really all that is needed? Don't you need to call > device_wakeup_enable(&adev->dev, 1); on the amba (primecell) > device providing this GPIO, lest it may be suspended itself > and render this exercise pointless. > Do you mean something like : -->8-- diff --git i/drivers/gpio/gpio-pl061.c w/drivers/gpio/gpio-pl061.c index 4d4b37676702..467e0b278cf0 100644 --- i/drivers/gpio/gpio-pl061.c +++ w/drivers/gpio/gpio-pl061.c @@ -354,6 +354,7 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id) } amba_set_drvdata(adev, chip); + dev_pm_set_wake_irq(&adev->dev, irq); dev_info(&adev->dev, "PL061 GPIO chip @%pa registered\n", &adev->res.start); -->8-- If yes, I agree that's one possible solution. > And if not, why not? > The only reason I didn't go for the above solution is that some platforms may not have any gpio pins as wakeup source, so it may be unnecessary to do that. Also in order to that, we also implicitly force platforms to mark the GPIO controller as wakeup source rather than just the few GPIO pins that are wakeup capable. > How are GPIO chips providing IRQs actually going to do this, > provided that they need to be kept awake themselves? > Not gone through all the implementations. Just started with PL061 when I tested on Juno(Beware it doesn't work with std. firmware as there are some know issues still under investigation) -- Regards, Sudeep ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] gpio: pl061: add support for wakeup configuration 2015-12-09 15:20 ` Sudeep Holla @ 2015-12-10 18:22 ` Linus Walleij 2015-12-10 22:28 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Linus Walleij @ 2015-12-10 18:22 UTC (permalink / raw) To: Sudeep Holla Cc: Aubrey Li, Dmitry Torokhov, Rafael J. Wysocki, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexandre Courbot, Ulf Hansson On Wed, Dec 9, 2015 at 4:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 09/12/15 15:08, Linus Walleij wrote: >> On Fri, Nov 27, 2015 at 6:19 PM, Sudeep Holla <sudeep.holla@arm.com> >> wrote: >> >>> The PL061 supports interrupts and those can be wakeup interrupts. We >>> need to provide support for configuring those interrupts as wakeup >>> sources. >>> >>> This patch adds irq_set_wake callback for PL061 so that GPIO interrupts >>> can be configured as wakeup. >>> >>> Cc: Linus Walleij <linus.walleij@linaro.org> >>> Cc: Alexandre Courbot <gnurou@gmail.com> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> drivers/gpio/gpio-pl061.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c >>> index 4d4b37676702..8b1cbd5767f9 100644 >>> --- a/drivers/gpio/gpio-pl061.c >>> +++ b/drivers/gpio/gpio-pl061.c >>> @@ -14,6 +14,7 @@ >>> #include <linux/module.h> >>> #include <linux/io.h> >>> #include <linux/ioport.h> >>> +#include <linux/interrupt.h> >>> #include <linux/irq.h> >>> #include <linux/irqchip/chained_irq.h> >>> #include <linux/bitops.h> >>> @@ -269,12 +270,20 @@ static void pl061_irq_ack(struct irq_data *d) >>> spin_unlock(&chip->lock); >>> } >>> >>> +static int pl061_irq_set_wake(struct irq_data *d, unsigned int state) >>> +{ >>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >>> + >>> + return irq_set_irq_wake(gc->irq_parent, state); >>> +} >>> + >>> static struct irq_chip pl061_irqchip = { >>> .name = "pl061", >>> .irq_ack = pl061_irq_ack, >>> .irq_mask = pl061_irq_mask, >>> .irq_unmask = pl061_irq_unmask, >>> .irq_set_type = pl061_irq_type, >>> + .irq_set_wake = pl061_irq_set_wake, >>> }; >> >> >> Is this really all that is needed? Don't you need to call >> device_wakeup_enable(&adev->dev, 1); on the amba (primecell) >> device providing this GPIO, lest it may be suspended itself >> and render this exercise pointless. >> > > Do you mean something like : > > > -->8-- > > diff --git i/drivers/gpio/gpio-pl061.c w/drivers/gpio/gpio-pl061.c > index 4d4b37676702..467e0b278cf0 100644 > --- i/drivers/gpio/gpio-pl061.c > +++ w/drivers/gpio/gpio-pl061.c > @@ -354,6 +354,7 @@ static int pl061_probe(struct amba_device *adev, const > struct amba_id *id) > } > > amba_set_drvdata(adev, chip); > + dev_pm_set_wake_irq(&adev->dev, irq); I don't really know :( I was under the impression that we may need *both*, because if the GPIO line is wakeup-capable, the wakeup event will not be detected unless the GPIO controller is also online, simply. Maybe: if (any_lines_on_me_wakeup_capable) dev_pm_set_wake()... I need the help from the power maintainers to figure this out... Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpio: pl061: add support for wakeup configuration 2015-12-10 18:22 ` Linus Walleij @ 2015-12-10 22:28 ` Rafael J. Wysocki 0 siblings, 0 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2015-12-10 22:28 UTC (permalink / raw) To: Linus Walleij Cc: Sudeep Holla, Aubrey Li, Dmitry Torokhov, Rafael J. Wysocki, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexandre Courbot, Ulf Hansson, Linux PM list On Thursday, December 10, 2015 07:22:12 PM Linus Walleij wrote: > On Wed, Dec 9, 2015 at 4:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On 09/12/15 15:08, Linus Walleij wrote: > >> On Fri, Nov 27, 2015 at 6:19 PM, Sudeep Holla <sudeep.holla@arm.com> > >> wrote: > >> > >>> The PL061 supports interrupts and those can be wakeup interrupts. We > >>> need to provide support for configuring those interrupts as wakeup > >>> sources. > >>> > >>> This patch adds irq_set_wake callback for PL061 so that GPIO interrupts > >>> can be configured as wakeup. > >>> > >>> Cc: Linus Walleij <linus.walleij@linaro.org> > >>> Cc: Alexandre Courbot <gnurou@gmail.com> > >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >>> --- > >>> drivers/gpio/gpio-pl061.c | 9 +++++++++ > >>> 1 file changed, 9 insertions(+) > >>> > >>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c > >>> index 4d4b37676702..8b1cbd5767f9 100644 > >>> --- a/drivers/gpio/gpio-pl061.c > >>> +++ b/drivers/gpio/gpio-pl061.c > >>> @@ -14,6 +14,7 @@ > >>> #include <linux/module.h> > >>> #include <linux/io.h> > >>> #include <linux/ioport.h> > >>> +#include <linux/interrupt.h> > >>> #include <linux/irq.h> > >>> #include <linux/irqchip/chained_irq.h> > >>> #include <linux/bitops.h> > >>> @@ -269,12 +270,20 @@ static void pl061_irq_ack(struct irq_data *d) > >>> spin_unlock(&chip->lock); > >>> } > >>> > >>> +static int pl061_irq_set_wake(struct irq_data *d, unsigned int state) > >>> +{ > >>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > >>> + > >>> + return irq_set_irq_wake(gc->irq_parent, state); > >>> +} > >>> + > >>> static struct irq_chip pl061_irqchip = { > >>> .name = "pl061", > >>> .irq_ack = pl061_irq_ack, > >>> .irq_mask = pl061_irq_mask, > >>> .irq_unmask = pl061_irq_unmask, > >>> .irq_set_type = pl061_irq_type, > >>> + .irq_set_wake = pl061_irq_set_wake, > >>> }; > >> > >> > >> Is this really all that is needed? Don't you need to call > >> device_wakeup_enable(&adev->dev, 1); on the amba (primecell) > >> device providing this GPIO, lest it may be suspended itself > >> and render this exercise pointless. > >> > > > > Do you mean something like : > > > > > > -->8-- > > > > diff --git i/drivers/gpio/gpio-pl061.c w/drivers/gpio/gpio-pl061.c > > index 4d4b37676702..467e0b278cf0 100644 > > --- i/drivers/gpio/gpio-pl061.c > > +++ w/drivers/gpio/gpio-pl061.c > > @@ -354,6 +354,7 @@ static int pl061_probe(struct amba_device *adev, const > > struct amba_id *id) > > } > > > > amba_set_drvdata(adev, chip); > > + dev_pm_set_wake_irq(&adev->dev, irq); > > I don't really know :( > > I was under the impression that we may need *both*, because if > the GPIO line is wakeup-capable, the wakeup event will not be > detected unless the GPIO controller is also online, simply. I would think that the GPIO controller wouldn't be suspended in that case and that its driver would decide. So, it would return 0 from its system suspend callbacks, but wouldn't actually suspend the device. Of course, this means that its parent might need to do the same and so on. We don't really have a common way for handling that in the PM core. Thanks, Rafael ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpio: pl061: add support for wakeup configuration 2015-11-27 17:19 [PATCH] gpio: pl061: add support for wakeup configuration Sudeep Holla 2015-12-09 15:08 ` Linus Walleij @ 2015-12-14 14:02 ` Linus Walleij 2015-12-14 14:04 ` Sudeep Holla 1 sibling, 1 reply; 7+ messages in thread From: Linus Walleij @ 2015-12-14 14:02 UTC (permalink / raw) To: Sudeep Holla Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexandre Courbot On Fri, Nov 27, 2015 at 6:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > The PL061 supports interrupts and those can be wakeup interrupts. We > need to provide support for configuring those interrupts as wakeup > sources. > > This patch adds irq_set_wake callback for PL061 so that GPIO interrupts > can be configured as wakeup. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Patch applied, since I can't think of anything better. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpio: pl061: add support for wakeup configuration 2015-12-14 14:02 ` Linus Walleij @ 2015-12-14 14:04 ` Sudeep Holla 0 siblings, 0 replies; 7+ messages in thread From: Sudeep Holla @ 2015-12-14 14:04 UTC (permalink / raw) To: Linus Walleij Cc: Sudeep Holla, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexandre Courbot On 14/12/15 14:02, Linus Walleij wrote: > On Fri, Nov 27, 2015 at 6:19 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> The PL061 supports interrupts and those can be wakeup interrupts. We >> need to provide support for configuring those interrupts as wakeup >> sources. >> >> This patch adds irq_set_wake callback for PL061 so that GPIO interrupts >> can be configured as wakeup. >> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Alexandre Courbot <gnurou@gmail.com> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > Patch applied, since I can't think of anything better. > Thanks and I agree but we can always fix/enhance if/whenever required :) -- Regards, Sudeep ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-14 14:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-27 17:19 [PATCH] gpio: pl061: add support for wakeup configuration Sudeep Holla 2015-12-09 15:08 ` Linus Walleij 2015-12-09 15:20 ` Sudeep Holla 2015-12-10 18:22 ` Linus Walleij 2015-12-10 22:28 ` Rafael J. Wysocki 2015-12-14 14:02 ` Linus Walleij 2015-12-14 14:04 ` Sudeep Holla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).