| Message ID | 20231017073129.1406748-1-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: host: sdhci-esdhc-imx: let usr to config the wakeup for gpio cd pin through sysfs | expand |
On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > Currently default enable the gpio cd pin wakeup, this will waste > power after system suspend if usr do not need this cd pin wakeup > feature. Now let usr to config the wakeup for gpio cd pin through > sysfs. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 40a6e2f8145a..2e46648344ba 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > /* > * Setup the wakeup capability here, let user to decide > * whether need to enable this wakeup through sysfs interface. > + * First check the SDIO device, second check the gpio CD pin. > */ > - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > - (host->mmc->pm_caps & MMC_PM_WAKE_SDIO_IRQ)) > + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > + (host->mmc->pm_caps & MMC_PM_WAKE_SDIO_IRQ)) || > + ((host->mmc->caps & MMC_CAP_CD_WAKE) && > + host->mmc->slot.cd_irq >= 0)) > device_set_wakeup_capable(&pdev->dev, true); If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, so this is wrong. In principle this must be managed through the GPIO irqchip driver instead. > > pm_runtime_set_active(&pdev->dev); > @@ -1878,7 +1881,8 @@ static int sdhci_esdhc_suspend(struct device *dev) > if (ret) > return ret; > > - ret = mmc_gpio_set_cd_wake(host->mmc, true); > + if (device_may_wakeup(dev)) As I indicated above. It's not really the device that corresponds to the mmc controller that can wake up the system, but rather the gpio irqchip. > + ret = mmc_gpio_set_cd_wake(host->mmc, true); > > return ret; > } > @@ -1901,8 +1905,10 @@ static int sdhci_esdhc_resume(struct device *dev) > > if (host->mmc->caps2 & MMC_CAP2_CQE) > ret = cqhci_resume(host->mmc); > + if (ret) > + return ret; > > - if (!ret) > + if (device_may_wakeup(dev)) Ditto. > ret = mmc_gpio_set_cd_wake(host->mmc, false); > > return ret; > -- > 2.34.1 > Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: 2023年10月17日 19:35 > To: Bough Chen <haibo.chen@nxp.com> > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de; > kernel@pengutronix.de; festevam@gmail.com > Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup > for gpio cd pin through sysfs > > On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > Currently default enable the gpio cd pin wakeup, this will waste power > > after system suspend if usr do not need this cd pin wakeup feature. > > Now let usr to config the wakeup for gpio cd pin through sysfs. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > --- > > drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > > b/drivers/mmc/host/sdhci-esdhc-imx.c > > index 40a6e2f8145a..2e46648344ba 100644 > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct > platform_device *pdev) > > /* > > * Setup the wakeup capability here, let user to decide > > * whether need to enable this wakeup through sysfs interface. > > + * First check the SDIO device, second check the gpio CD pin. > > */ > > - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > > - (host->mmc->pm_caps & > MMC_PM_WAKE_SDIO_IRQ)) > > + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > > + (host->mmc->pm_caps & > MMC_PM_WAKE_SDIO_IRQ)) || > > + ((host->mmc->caps & MMC_CAP_CD_WAKE) && > > + host->mmc->slot.cd_irq >= 0)) > > device_set_wakeup_capable(&pdev->dev, true); > > If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, > so this is wrong. > > In principle this must be managed through the GPIO irqchip driver instead. Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() Seems PCI subsystem combine GPIO wakeup to PCI controller. Anyway, I will try to implement this in GPIO irqchip driver. Best Regards Haibo Chen > > > > > pm_runtime_set_active(&pdev->dev); > > @@ -1878,7 +1881,8 @@ static int sdhci_esdhc_suspend(struct device *dev) > > if (ret) > > return ret; > > > > - ret = mmc_gpio_set_cd_wake(host->mmc, true); > > + if (device_may_wakeup(dev)) > > As I indicated above. It's not really the device that corresponds to the mmc > controller that can wake up the system, but rather the gpio irqchip. > > > + ret = mmc_gpio_set_cd_wake(host->mmc, true); > > > > return ret; > > } > > @@ -1901,8 +1905,10 @@ static int sdhci_esdhc_resume(struct device > > *dev) > > > > if (host->mmc->caps2 & MMC_CAP2_CQE) > > ret = cqhci_resume(host->mmc); > > + if (ret) > > + return ret; > > > > - if (!ret) > > + if (device_may_wakeup(dev)) > > Ditto. > > > ret = mmc_gpio_set_cd_wake(host->mmc, false); > > > > return ret; > > -- > > 2.34.1 > > > > Kind regards > Uffe
On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@nxp.com> wrote: > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: 2023年10月17日 19:35 > > To: Bough Chen <haibo.chen@nxp.com> > > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; dl-linux-imx > > <linux-imx@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de; > > kernel@pengutronix.de; festevam@gmail.com > > Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup > > for gpio cd pin through sysfs > > > > On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: > > > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > Currently default enable the gpio cd pin wakeup, this will waste power > > > after system suspend if usr do not need this cd pin wakeup feature. > > > Now let usr to config the wakeup for gpio cd pin through sysfs. > > > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > > --- > > > drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > > > b/drivers/mmc/host/sdhci-esdhc-imx.c > > > index 40a6e2f8145a..2e46648344ba 100644 > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > > @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct > > platform_device *pdev) > > > /* > > > * Setup the wakeup capability here, let user to decide > > > * whether need to enable this wakeup through sysfs interface. > > > + * First check the SDIO device, second check the gpio CD pin. > > > */ > > > - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > > > - (host->mmc->pm_caps & > > MMC_PM_WAKE_SDIO_IRQ)) > > > + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > > > + (host->mmc->pm_caps & > > MMC_PM_WAKE_SDIO_IRQ)) || > > > + ((host->mmc->caps & MMC_CAP_CD_WAKE) && > > > + host->mmc->slot.cd_irq >= 0)) > > > device_set_wakeup_capable(&pdev->dev, true); > > > > If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, > > so this is wrong. > > > > In principle this must be managed through the GPIO irqchip driver instead. > > Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. > I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() > > Seems PCI subsystem combine GPIO wakeup to PCI controller. Good point! I am not sure that is the correct way to do it either, but maybe it's fine for PCI based devices. Adrian probably knows this better. The main problem I see with your approach, is that it may prevent a PM domain (genpd), if the USDHC device is attached to one, from being powered-off during system suspend. That would be wrong as the USDHC device doesn't need to stay powered-on to allow a GPIO to be configured as a system wakeup irq. > > Anyway, I will try to implement this in GPIO irqchip driver. That sounds like the right thing to do. Although, it does mean that we won't be able to allow userspace to decide on a per GPIO pin/irq basis, if system wakeup should be enabled or disabled. As far as I know, we don't have a solution in place to support that. [...] Kind regards Uffe
On 18/10/23 17:42, Ulf Hansson wrote: > On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@nxp.com> wrote: >> >>> -----Original Message----- >>> From: Ulf Hansson <ulf.hansson@linaro.org> >>> Sent: 2023年10月17日 19:35 >>> To: Bough Chen <haibo.chen@nxp.com> >>> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; dl-linux-imx >>> <linux-imx@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de; >>> kernel@pengutronix.de; festevam@gmail.com >>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup >>> for gpio cd pin through sysfs >>> >>> On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: >>>> >>>> From: Haibo Chen <haibo.chen@nxp.com> >>>> >>>> Currently default enable the gpio cd pin wakeup, this will waste power >>>> after system suspend if usr do not need this cd pin wakeup feature. >>>> Now let usr to config the wakeup for gpio cd pin through sysfs. >>>> >>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> >>>> --- >>>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- >>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c >>>> b/drivers/mmc/host/sdhci-esdhc-imx.c >>>> index 40a6e2f8145a..2e46648344ba 100644 >>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >>>> @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct >>> platform_device *pdev) >>>> /* >>>> * Setup the wakeup capability here, let user to decide >>>> * whether need to enable this wakeup through sysfs interface. >>>> + * First check the SDIO device, second check the gpio CD pin. >>>> */ >>>> - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && >>>> - (host->mmc->pm_caps & >>> MMC_PM_WAKE_SDIO_IRQ)) >>>> + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && >>>> + (host->mmc->pm_caps & >>> MMC_PM_WAKE_SDIO_IRQ)) || >>>> + ((host->mmc->caps & MMC_CAP_CD_WAKE) && >>>> + host->mmc->slot.cd_irq >= 0)) >>>> device_set_wakeup_capable(&pdev->dev, true); >>> >>> If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, >>> so this is wrong. >>> >>> In principle this must be managed through the GPIO irqchip driver instead. >> >> Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. >> I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() >> >> Seems PCI subsystem combine GPIO wakeup to PCI controller. > > Good point! I am not sure that is the correct way to do it either, but > maybe it's fine for PCI based devices. Adrian probably knows this > better. PCI / ACPI layers handle configuration of wakeup signaling, so a device will be put to the appropriate power state. It won't be forced to be "on" by registering a wakeup source. > > The main problem I see with your approach, is that it may prevent a PM > domain (genpd), if the USDHC device is attached to one, from being > powered-off during system suspend. That would be wrong as the USDHC > device doesn't need to stay powered-on to allow a GPIO to be > configured as a system wakeup irq. > >> >> Anyway, I will try to implement this in GPIO irqchip driver. > > That sounds like the right thing to do. > > Although, it does mean that we won't be able to allow userspace to > decide on a per GPIO pin/irq basis, if system wakeup should be enabled > or disabled. As far as I know, we don't have a solution in place to > support that. > > [...] > > Kind regards > Uffe
On Thu, 19 Oct 2023 at 16:48, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 18/10/23 17:42, Ulf Hansson wrote: > > On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@nxp.com> wrote: > >> > >>> -----Original Message----- > >>> From: Ulf Hansson <ulf.hansson@linaro.org> > >>> Sent: 2023年10月17日 19:35 > >>> To: Bough Chen <haibo.chen@nxp.com> > >>> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; dl-linux-imx > >>> <linux-imx@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de; > >>> kernel@pengutronix.de; festevam@gmail.com > >>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup > >>> for gpio cd pin through sysfs > >>> > >>> On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: > >>>> > >>>> From: Haibo Chen <haibo.chen@nxp.com> > >>>> > >>>> Currently default enable the gpio cd pin wakeup, this will waste power > >>>> after system suspend if usr do not need this cd pin wakeup feature. > >>>> Now let usr to config the wakeup for gpio cd pin through sysfs. > >>>> > >>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > >>>> --- > >>>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- > >>>> 1 file changed, 10 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > >>>> b/drivers/mmc/host/sdhci-esdhc-imx.c > >>>> index 40a6e2f8145a..2e46648344ba 100644 > >>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c > >>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > >>>> @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct > >>> platform_device *pdev) > >>>> /* > >>>> * Setup the wakeup capability here, let user to decide > >>>> * whether need to enable this wakeup through sysfs interface. > >>>> + * First check the SDIO device, second check the gpio CD pin. > >>>> */ > >>>> - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > >>>> - (host->mmc->pm_caps & > >>> MMC_PM_WAKE_SDIO_IRQ)) > >>>> + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > >>>> + (host->mmc->pm_caps & > >>> MMC_PM_WAKE_SDIO_IRQ)) || > >>>> + ((host->mmc->caps & MMC_CAP_CD_WAKE) && > >>>> + host->mmc->slot.cd_irq >= 0)) > >>>> device_set_wakeup_capable(&pdev->dev, true); > >>> > >>> If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, > >>> so this is wrong. > >>> > >>> In principle this must be managed through the GPIO irqchip driver instead. > >> > >> Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. > >> I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() > >> > >> Seems PCI subsystem combine GPIO wakeup to PCI controller. > > > > Good point! I am not sure that is the correct way to do it either, but > > maybe it's fine for PCI based devices. Adrian probably knows this > > better. > > PCI / ACPI layers handle configuration of wakeup signaling, so a > device will be put to the appropriate power state. It won't be > forced to be "on" by registering a wakeup source. > Okay, I see. So you are saying that PCI bus/layers are aware of the GPIO pin/irq being used for wakeup for the device in question? Kind regards Uffe
On 19/10/23 18:24, Ulf Hansson wrote: > On Thu, 19 Oct 2023 at 16:48, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 18/10/23 17:42, Ulf Hansson wrote: >>> On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@nxp.com> wrote: >>>> >>>>> -----Original Message----- >>>>> From: Ulf Hansson <ulf.hansson@linaro.org> >>>>> Sent: 2023年10月17日 19:35 >>>>> To: Bough Chen <haibo.chen@nxp.com> >>>>> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; dl-linux-imx >>>>> <linux-imx@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de; >>>>> kernel@pengutronix.de; festevam@gmail.com >>>>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup >>>>> for gpio cd pin through sysfs >>>>> >>>>> On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: >>>>>> >>>>>> From: Haibo Chen <haibo.chen@nxp.com> >>>>>> >>>>>> Currently default enable the gpio cd pin wakeup, this will waste power >>>>>> after system suspend if usr do not need this cd pin wakeup feature. >>>>>> Now let usr to config the wakeup for gpio cd pin through sysfs. >>>>>> >>>>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> >>>>>> --- >>>>>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- >>>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> b/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> index 40a6e2f8145a..2e46648344ba 100644 >>>>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >>>>>> @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct >>>>> platform_device *pdev) >>>>>> /* >>>>>> * Setup the wakeup capability here, let user to decide >>>>>> * whether need to enable this wakeup through sysfs interface. >>>>>> + * First check the SDIO device, second check the gpio CD pin. >>>>>> */ >>>>>> - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && >>>>>> - (host->mmc->pm_caps & >>>>> MMC_PM_WAKE_SDIO_IRQ)) >>>>>> + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && >>>>>> + (host->mmc->pm_caps & >>>>> MMC_PM_WAKE_SDIO_IRQ)) || >>>>>> + ((host->mmc->caps & MMC_CAP_CD_WAKE) && >>>>>> + host->mmc->slot.cd_irq >= 0)) >>>>>> device_set_wakeup_capable(&pdev->dev, true); >>>>> >>>>> If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, >>>>> so this is wrong. >>>>> >>>>> In principle this must be managed through the GPIO irqchip driver instead. >>>> >>>> Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. >>>> I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() >>>> >>>> Seems PCI subsystem combine GPIO wakeup to PCI controller. >>> >>> Good point! I am not sure that is the correct way to do it either, but >>> maybe it's fine for PCI based devices. Adrian probably knows this >>> better. >> >> PCI / ACPI layers handle configuration of wakeup signaling, so a >> device will be put to the appropriate power state. It won't be >> forced to be "on" by registering a wakeup source. >> > > Okay, I see. > > So you are saying that PCI bus/layers are aware of the GPIO pin/irq > being used for wakeup for the device in question? No, it is more like PCI / ACPI take care of most things but the wakeup framework provides sysfs wakeup API. We can add that and use it to determine if wakeup is enabled (by the user) to then enable the CD GPIO IRQ wakeup. That way, the sysfs wakeup API is on the correct device, and the user doesn't need to know anything about GPIO.
On Thu, 19 Oct 2023 at 18:59, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 19/10/23 18:24, Ulf Hansson wrote: > > On Thu, 19 Oct 2023 at 16:48, Adrian Hunter <adrian.hunter@intel.com> wrote: > >> > >> On 18/10/23 17:42, Ulf Hansson wrote: > >>> On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@nxp.com> wrote: > >>>> > >>>>> -----Original Message----- > >>>>> From: Ulf Hansson <ulf.hansson@linaro.org> > >>>>> Sent: 2023年10月17日 19:35 > >>>>> To: Bough Chen <haibo.chen@nxp.com> > >>>>> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; dl-linux-imx > >>>>> <linux-imx@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de; > >>>>> kernel@pengutronix.de; festevam@gmail.com > >>>>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: let usr to config the wakeup > >>>>> for gpio cd pin through sysfs > >>>>> > >>>>> On Tue, 17 Oct 2023 at 09:26, <haibo.chen@nxp.com> wrote: > >>>>>> > >>>>>> From: Haibo Chen <haibo.chen@nxp.com> > >>>>>> > >>>>>> Currently default enable the gpio cd pin wakeup, this will waste power > >>>>>> after system suspend if usr do not need this cd pin wakeup feature. > >>>>>> Now let usr to config the wakeup for gpio cd pin through sysfs. > >>>>>> > >>>>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > >>>>>> --- > >>>>>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++++---- > >>>>>> 1 file changed, 10 insertions(+), 4 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > >>>>>> b/drivers/mmc/host/sdhci-esdhc-imx.c > >>>>>> index 40a6e2f8145a..2e46648344ba 100644 > >>>>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c > >>>>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > >>>>>> @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct > >>>>> platform_device *pdev) > >>>>>> /* > >>>>>> * Setup the wakeup capability here, let user to decide > >>>>>> * whether need to enable this wakeup through sysfs interface. > >>>>>> + * First check the SDIO device, second check the gpio CD pin. > >>>>>> */ > >>>>>> - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > >>>>>> - (host->mmc->pm_caps & > >>>>> MMC_PM_WAKE_SDIO_IRQ)) > >>>>>> + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && > >>>>>> + (host->mmc->pm_caps & > >>>>> MMC_PM_WAKE_SDIO_IRQ)) || > >>>>>> + ((host->mmc->caps & MMC_CAP_CD_WAKE) && > >>>>>> + host->mmc->slot.cd_irq >= 0)) > >>>>>> device_set_wakeup_capable(&pdev->dev, true); > >>>>> > >>>>> If the wakeup is GPIO based, it doesn't mean that the device is wakeup capable, > >>>>> so this is wrong. > >>>>> > >>>>> In principle this must be managed through the GPIO irqchip driver instead. > >>>> > >>>> Yes, I know GPIO and USDHC are different device, combine GPIO wakeup to USDHC is not that reasonable. > >>>> I send this patch because I notice there is similar code in sdhci-pci-core.c, refer to sdhci_pci_suspend_host() > >>>> > >>>> Seems PCI subsystem combine GPIO wakeup to PCI controller. > >>> > >>> Good point! I am not sure that is the correct way to do it either, but > >>> maybe it's fine for PCI based devices. Adrian probably knows this > >>> better. > >> > >> PCI / ACPI layers handle configuration of wakeup signaling, so a > >> device will be put to the appropriate power state. It won't be > >> forced to be "on" by registering a wakeup source. > >> > > > > Okay, I see. > > > > So you are saying that PCI bus/layers are aware of the GPIO pin/irq > > being used for wakeup for the device in question? > > No, it is more like PCI / ACPI take care of most things but the > wakeup framework provides sysfs wakeup API. We can add that and > use it to determine if wakeup is enabled (by the user) to then > enable the CD GPIO IRQ wakeup. That way, the sysfs wakeup API is > on the correct device, and the user doesn't need to know anything > about GPIO. > Okay, so it's really ACPI that is needed here too. Anyway, thanks for sharing! Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 40a6e2f8145a..2e46648344ba 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -1797,9 +1797,12 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) /* * Setup the wakeup capability here, let user to decide * whether need to enable this wakeup through sysfs interface. + * First check the SDIO device, second check the gpio CD pin. */ - if ((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && - (host->mmc->pm_caps & MMC_PM_WAKE_SDIO_IRQ)) + if (((host->mmc->pm_caps & MMC_PM_KEEP_POWER) && + (host->mmc->pm_caps & MMC_PM_WAKE_SDIO_IRQ)) || + ((host->mmc->caps & MMC_CAP_CD_WAKE) && + host->mmc->slot.cd_irq >= 0)) device_set_wakeup_capable(&pdev->dev, true); pm_runtime_set_active(&pdev->dev); @@ -1878,7 +1881,8 @@ static int sdhci_esdhc_suspend(struct device *dev) if (ret) return ret; - ret = mmc_gpio_set_cd_wake(host->mmc, true); + if (device_may_wakeup(dev)) + ret = mmc_gpio_set_cd_wake(host->mmc, true); return ret; } @@ -1901,8 +1905,10 @@ static int sdhci_esdhc_resume(struct device *dev) if (host->mmc->caps2 & MMC_CAP2_CQE) ret = cqhci_resume(host->mmc); + if (ret) + return ret; - if (!ret) + if (device_may_wakeup(dev)) ret = mmc_gpio_set_cd_wake(host->mmc, false); return ret;