On Wed, 18 Oct 2023 at 04:07, Bough Chen <haibo.chen@xxxxxxx> wrote: > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > > Sent: 2023年10月17日 19:35 > > To: Bough Chen <haibo.chen@xxxxxxx> > > Cc: adrian.hunter@xxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; dl-linux-imx > > <linux-imx@xxxxxxx>; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx > > 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@xxxxxxx> wrote: > > > > > > From: Haibo Chen <haibo.chen@xxxxxxx> > > > > > > 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@xxxxxxx> > > > --- > > > 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
