On 18/10/23 17:42, Ulf Hansson wrote: > 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. 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
