| Message ID | 1518612992-4951-4-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 14 February 2018 at 13:56, Adrian Hunter <adrian.hunter@intel.com> wrote: > Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on > cd_irq") enabled wakeup irrespective of the host controller's PM flags. > However, users also want to control it from sysfs power/wakeup attribute. > That means the driver needs to check the PM flags before enabling it in the > suspend callback. Add helper function mmc_gpio_set_cd_wake() to make it > easy for drivers to do that. Depending on if device_may_wakeup() returns true to allow GPIO card detect IRQ to be configured as a wakeup IRQ is problematic. The reason is related to PM domains (genpd) when it checks the dev->power.wakeup_path in system suspend. In case device_may_wakeup() returns true, it may lead to the PM domain is kept powered during system suspend, while in fact this may not be needed for the GPIO IRQ to be configured as wakeup (because the SoC have external HW logic to deal with wakeup IRQs). So I can't apply this, until we have a solution of how to deal with the above situation and I have been working on that. According to the latest discussions [1], between me and Rafael, it seems like the solution must also take runtime PM wakeups into account. Or perhaps you have some other ideas of how we can move forward? >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/core.c | 3 +-- > drivers/mmc/core/slot-gpio.c | 23 +++++++++++++++++++++++ > include/linux/mmc/slot-gpio.h | 1 + > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index c0ba6d8823b7..7bed23c877de 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2655,8 +2655,7 @@ void mmc_start_host(struct mmc_host *host) > void mmc_stop_host(struct mmc_host *host) > { > if (host->slot.cd_irq >= 0) { > - if (host->slot.cd_wake_enabled) > - disable_irq_wake(host->slot.cd_irq); > + mmc_gpio_set_cd_wake(host, false); > disable_irq(host->slot.cd_irq); > } > > diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c > index 3698b0576009..817d668aae34 100644 > --- a/drivers/mmc/core/slot-gpio.c > +++ b/drivers/mmc/core/slot-gpio.c > @@ -154,6 +154,29 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host) > } > EXPORT_SYMBOL(mmc_gpiod_request_cd_irq); > > +int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on) > +{ > + int ret = 0; > + > + if (!(host->caps & MMC_CAP_CD_WAKE) || > + host->slot.cd_irq < 0 || > + on == host->slot.cd_wake_enabled) > + return 0; > + > + if (on) { > + if (device_may_wakeup(mmc_dev(host))) { > + ret = enable_irq_wake(host->slot.cd_irq); > + host->slot.cd_wake_enabled = !ret; > + } > + } else { > + disable_irq_wake(host->slot.cd_irq); > + host->slot.cd_wake_enabled = false; > + } > + > + return ret; > +} > +EXPORT_SYMBOL(mmc_gpio_set_cd_wake); > + > /* Register an alternate interrupt service routine for > * the card-detect GPIO. > */ > diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h > index 91f1ba0663c8..06607c59c4d0 100644 > --- a/include/linux/mmc/slot-gpio.h > +++ b/include/linux/mmc/slot-gpio.h > @@ -31,6 +31,7 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id, > unsigned int debounce, bool *gpio_invert); > void mmc_gpio_set_cd_isr(struct mmc_host *host, > irqreturn_t (*isr)(int irq, void *dev_id)); > +int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on); > void mmc_gpiod_request_cd_irq(struct mmc_host *host); > bool mmc_can_gpio_cd(struct mmc_host *host); > bool mmc_can_gpio_ro(struct mmc_host *host); > -- > 1.9.1 > Kind regards Uffe [1] https://marc.info/?l=linux-pm&m=151689653022118&w=2
On 27/02/18 10:45, Ulf Hansson wrote: > On 14 February 2018 at 13:56, Adrian Hunter <adrian.hunter@intel.com> wrote: >> Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on >> cd_irq") enabled wakeup irrespective of the host controller's PM flags. >> However, users also want to control it from sysfs power/wakeup attribute. >> That means the driver needs to check the PM flags before enabling it in the >> suspend callback. Add helper function mmc_gpio_set_cd_wake() to make it >> easy for drivers to do that. > > Depending on if device_may_wakeup() returns true to allow GPIO card > detect IRQ to be configured as a wakeup IRQ is problematic. > > The reason is related to PM domains (genpd) when it checks the > dev->power.wakeup_path in system suspend. In case device_may_wakeup() > returns true, it may lead to the PM domain is kept powered during > system suspend, while in fact this may not be needed for the GPIO IRQ > to be configured as wakeup (because the SoC have external HW logic to > deal with wakeup IRQs). > > So I can't apply this, until we have a solution of how to deal with > the above situation and I have been working on that. According to the > latest discussions [1], between me and Rafael, it seems like the > solution must also take runtime PM wakeups into account. > > Or perhaps you have some other ideas of how we can move forward? What about removing device_may_wakeup() from mmc_gpio_set_cd_wake() and leaving it to the driver (sdhci-pci) to decide whether to call device_may_wakeup()? > >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/mmc/core/core.c | 3 +-- >> drivers/mmc/core/slot-gpio.c | 23 +++++++++++++++++++++++ >> include/linux/mmc/slot-gpio.h | 1 + >> 3 files changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index c0ba6d8823b7..7bed23c877de 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -2655,8 +2655,7 @@ void mmc_start_host(struct mmc_host *host) >> void mmc_stop_host(struct mmc_host *host) >> { >> if (host->slot.cd_irq >= 0) { >> - if (host->slot.cd_wake_enabled) >> - disable_irq_wake(host->slot.cd_irq); >> + mmc_gpio_set_cd_wake(host, false); >> disable_irq(host->slot.cd_irq); >> } >> >> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c >> index 3698b0576009..817d668aae34 100644 >> --- a/drivers/mmc/core/slot-gpio.c >> +++ b/drivers/mmc/core/slot-gpio.c >> @@ -154,6 +154,29 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host) >> } >> EXPORT_SYMBOL(mmc_gpiod_request_cd_irq); >> >> +int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on) >> +{ >> + int ret = 0; >> + >> + if (!(host->caps & MMC_CAP_CD_WAKE) || >> + host->slot.cd_irq < 0 || >> + on == host->slot.cd_wake_enabled) >> + return 0; >> + >> + if (on) { >> + if (device_may_wakeup(mmc_dev(host))) { >> + ret = enable_irq_wake(host->slot.cd_irq); >> + host->slot.cd_wake_enabled = !ret; >> + } >> + } else { >> + disable_irq_wake(host->slot.cd_irq); >> + host->slot.cd_wake_enabled = false; >> + } >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(mmc_gpio_set_cd_wake); >> + >> /* Register an alternate interrupt service routine for >> * the card-detect GPIO. >> */ >> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h >> index 91f1ba0663c8..06607c59c4d0 100644 >> --- a/include/linux/mmc/slot-gpio.h >> +++ b/include/linux/mmc/slot-gpio.h >> @@ -31,6 +31,7 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id, >> unsigned int debounce, bool *gpio_invert); >> void mmc_gpio_set_cd_isr(struct mmc_host *host, >> irqreturn_t (*isr)(int irq, void *dev_id)); >> +int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on); >> void mmc_gpiod_request_cd_irq(struct mmc_host *host); >> bool mmc_can_gpio_cd(struct mmc_host *host); >> bool mmc_can_gpio_ro(struct mmc_host *host); >> -- >> 1.9.1 >> > > Kind regards > Uffe > > [1] > https://marc.info/?l=linux-pm&m=151689653022118&w=2 >
On 27 February 2018 at 11:06, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 27/02/18 10:45, Ulf Hansson wrote: >> On 14 February 2018 at 13:56, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on >>> cd_irq") enabled wakeup irrespective of the host controller's PM flags. >>> However, users also want to control it from sysfs power/wakeup attribute. >>> That means the driver needs to check the PM flags before enabling it in the >>> suspend callback. Add helper function mmc_gpio_set_cd_wake() to make it >>> easy for drivers to do that. >> >> Depending on if device_may_wakeup() returns true to allow GPIO card >> detect IRQ to be configured as a wakeup IRQ is problematic. >> >> The reason is related to PM domains (genpd) when it checks the >> dev->power.wakeup_path in system suspend. In case device_may_wakeup() >> returns true, it may lead to the PM domain is kept powered during >> system suspend, while in fact this may not be needed for the GPIO IRQ >> to be configured as wakeup (because the SoC have external HW logic to >> deal with wakeup IRQs). >> >> So I can't apply this, until we have a solution of how to deal with >> the above situation and I have been working on that. According to the >> latest discussions [1], between me and Rafael, it seems like the >> solution must also take runtime PM wakeups into account. >> >> Or perhaps you have some other ideas of how we can move forward? > > What about removing device_may_wakeup() from mmc_gpio_set_cd_wake() > and leaving it to the driver (sdhci-pci) to decide whether to call > device_may_wakeup()? Yes, that's would be fine for now. [...] Kind regards Uffe
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index c0ba6d8823b7..7bed23c877de 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2655,8 +2655,7 @@ void mmc_start_host(struct mmc_host *host) void mmc_stop_host(struct mmc_host *host) { if (host->slot.cd_irq >= 0) { - if (host->slot.cd_wake_enabled) - disable_irq_wake(host->slot.cd_irq); + mmc_gpio_set_cd_wake(host, false); disable_irq(host->slot.cd_irq); } diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c index 3698b0576009..817d668aae34 100644 --- a/drivers/mmc/core/slot-gpio.c +++ b/drivers/mmc/core/slot-gpio.c @@ -154,6 +154,29 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host) } EXPORT_SYMBOL(mmc_gpiod_request_cd_irq); +int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on) +{ + int ret = 0; + + if (!(host->caps & MMC_CAP_CD_WAKE) || + host->slot.cd_irq < 0 || + on == host->slot.cd_wake_enabled) + return 0; + + if (on) { + if (device_may_wakeup(mmc_dev(host))) { + ret = enable_irq_wake(host->slot.cd_irq); + host->slot.cd_wake_enabled = !ret; + } + } else { + disable_irq_wake(host->slot.cd_irq); + host->slot.cd_wake_enabled = false; + } + + return ret; +} +EXPORT_SYMBOL(mmc_gpio_set_cd_wake); + /* Register an alternate interrupt service routine for * the card-detect GPIO. */ diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h index 91f1ba0663c8..06607c59c4d0 100644 --- a/include/linux/mmc/slot-gpio.h +++ b/include/linux/mmc/slot-gpio.h @@ -31,6 +31,7 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id, unsigned int debounce, bool *gpio_invert); void mmc_gpio_set_cd_isr(struct mmc_host *host, irqreturn_t (*isr)(int irq, void *dev_id)); +int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on); void mmc_gpiod_request_cd_irq(struct mmc_host *host); bool mmc_can_gpio_cd(struct mmc_host *host); bool mmc_can_gpio_ro(struct mmc_host *host);
Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on cd_irq") enabled wakeup irrespective of the host controller's PM flags. However, users also want to control it from sysfs power/wakeup attribute. That means the driver needs to check the PM flags before enabling it in the suspend callback. Add helper function mmc_gpio_set_cd_wake() to make it easy for drivers to do that. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/core.c | 3 +-- drivers/mmc/core/slot-gpio.c | 23 +++++++++++++++++++++++ include/linux/mmc/slot-gpio.h | 1 + 3 files changed, 25 insertions(+), 2 deletions(-)