| Message ID | 20250521033134.112671-1-ziniu.wang_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-esdhc-imx: do not change pinctrl state in suspend if function irq is wakeup source | expand |
On Wed, May 21, 2025 at 11:31:34AM +0800, ziniu.wang_1@nxp.com wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > pinctrl sleep state may config the pin mux to certain function to save > power in system PM. But if usdhc is setting as wakeup source, like > the card interrupt(SDIO) or card insert interrupt, it depends on the > related pin mux configured to usdhc function pad. > e.g. To support card interrupt(SDIO interrupt), it need the pin is > config as usdhc DATA[1] function pin. I think it should be dts settings wrong. Does one PAD set as function impact power much? Frank > > Find the issue on imx93-11x11-evk board, SDIO WiFi in band interrupt > can't wakeup system because the pinctrl sleep state config the DATA[1] > pin as GPIO function. > > For this case, do not change the pinctrl state in suspend. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 5f1c45b2bd5d..f206b562a6e3 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -2057,12 +2057,20 @@ static int sdhci_esdhc_suspend(struct device *dev) > ret = sdhci_enable_irq_wakeups(host); > if (!ret) > dev_warn(dev, "Failed to enable irq wakeup\n"); > + } else { > + /* > + * For the device which works as wakeup source, no need > + * to change the pinctrl to sleep state. > + * e.g. For SDIO device, the interrupt share with data pin, > + * but the pinctrl sleep state may config the data pin to > + * other function like GPIO function to save power in PM, > + * which finally block the SDIO wakeup function. > + */ > + ret = pinctrl_pm_select_sleep_state(dev); > + if (ret) > + return ret; > } > > - ret = pinctrl_pm_select_sleep_state(dev); > - if (ret) > - return ret; > - > ret = mmc_gpio_set_cd_wake(host->mmc, true); > > /* > -- > 2.34.1 >
> -----Original Message----- > From: Frank Li <frank.li@nxp.com> > Sent: 2025年5月21日 23:56 > To: Luke Wang <ziniu.wang_1@nxp.com> > Cc: Bough Chen <haibo.chen@nxp.com>; adrian.hunter@intel.com; > ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; shawnguo@kernel.org; > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; > imx@lists.linux.dev; dl-S32 <S32@nxp.com>; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] mmc: sdhci-esdhc-imx: do not change pinctrl state in > suspend if function irq is wakeup source > > On Wed, May 21, 2025 at 11:31:34AM +0800, ziniu.wang_1@nxp.com wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > pinctrl sleep state may config the pin mux to certain function to save > > power in system PM. But if usdhc is setting as wakeup source, like the > > card interrupt(SDIO) or card insert interrupt, it depends on the > > related pin mux configured to usdhc function pad. > > e.g. To support card interrupt(SDIO interrupt), it need the pin is > > config as usdhc DATA[1] function pin. > > I think it should be dts settings wrong. Does one PAD set as function impact > power much? Hi Frank, I double check the power team, on imx93-11x11-evk board, For SD card on usdhc2, switch the PAD to gpio function will save about 3~4mw, For SDIO wifi on usdhc3, switch the PAD to gpio function will save about 0.8~1mw. (without wakeup) Regards Haibo Chen > > Frank > > > > > Find the issue on imx93-11x11-evk board, SDIO WiFi in band interrupt > > can't wakeup system because the pinctrl sleep state config the DATA[1] > > pin as GPIO function. > > > > For this case, do not change the pinctrl state in suspend. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > > --- > > drivers/mmc/host/sdhci-esdhc-imx.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > > b/drivers/mmc/host/sdhci-esdhc-imx.c > > index 5f1c45b2bd5d..f206b562a6e3 100644 > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > @@ -2057,12 +2057,20 @@ static int sdhci_esdhc_suspend(struct device > *dev) > > ret = sdhci_enable_irq_wakeups(host); > > if (!ret) > > dev_warn(dev, "Failed to enable irq wakeup\n"); > > + } else { > > + /* > > + * For the device which works as wakeup source, no need > > + * to change the pinctrl to sleep state. > > + * e.g. For SDIO device, the interrupt share with data pin, > > + * but the pinctrl sleep state may config the data pin to > > + * other function like GPIO function to save power in PM, > > + * which finally block the SDIO wakeup function. > > + */ > > + ret = pinctrl_pm_select_sleep_state(dev); > > + if (ret) > > + return ret; > > } > > > > - ret = pinctrl_pm_select_sleep_state(dev); > > - if (ret) > > - return ret; > > - > > ret = mmc_gpio_set_cd_wake(host->mmc, true); > > > > /* > > -- > > 2.34.1 > >
On Thu, May 22, 2025 at 03:16:11AM +0000, Bough Chen wrote: > > -----Original Message----- > > From: Frank Li <frank.li@nxp.com> > > Sent: 2025年5月21日 23:56 > > To: Luke Wang <ziniu.wang_1@nxp.com> > > Cc: Bough Chen <haibo.chen@nxp.com>; adrian.hunter@intel.com; > > ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; shawnguo@kernel.org; > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; > > imx@lists.linux.dev; dl-S32 <S32@nxp.com>; > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH] mmc: sdhci-esdhc-imx: do not change pinctrl state in > > suspend if function irq is wakeup source > > > > On Wed, May 21, 2025 at 11:31:34AM +0800, ziniu.wang_1@nxp.com wrote: > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > pinctrl sleep state may config the pin mux to certain function to save > > > power in system PM. But if usdhc is setting as wakeup source, like the > > > card interrupt(SDIO) or card insert interrupt, it depends on the > > > related pin mux configured to usdhc function pad. > > > e.g. To support card interrupt(SDIO interrupt), it need the pin is > > > config as usdhc DATA[1] function pin. > > > > I think it should be dts settings wrong. Does one PAD set as function impact > > power much? > > Hi Frank, > > I double check the power team, on imx93-11x11-evk board, For SD card on usdhc2, switch the PAD to gpio function will save about 3~4mw, > For SDIO wifi on usdhc3, switch the PAD to gpio function will save about 0.8~1mw. (without wakeup) Okay, I think it should be add new pinctrl state ("wakeup"), only set DATA[0,2,3] to GPIO and SET DATA[1] to sd function. You can change pad setting to "wakeup" state if wake up enabled. Frank > > Regards > Haibo Chen > > > > Frank > > > > > > > > Find the issue on imx93-11x11-evk board, SDIO WiFi in band interrupt > > > can't wakeup system because the pinctrl sleep state config the DATA[1] > > > pin as GPIO function. > > > > > > For this case, do not change the pinctrl state in suspend. > > > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > > > --- > > > drivers/mmc/host/sdhci-esdhc-imx.c | 16 ++++++++++++---- > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > > > b/drivers/mmc/host/sdhci-esdhc-imx.c > > > index 5f1c45b2bd5d..f206b562a6e3 100644 > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > > @@ -2057,12 +2057,20 @@ static int sdhci_esdhc_suspend(struct device > > *dev) > > > ret = sdhci_enable_irq_wakeups(host); > > > if (!ret) > > > dev_warn(dev, "Failed to enable irq wakeup\n"); > > > + } else { > > > + /* > > > + * For the device which works as wakeup source, no need > > > + * to change the pinctrl to sleep state. > > > + * e.g. For SDIO device, the interrupt share with data pin, > > > + * but the pinctrl sleep state may config the data pin to > > > + * other function like GPIO function to save power in PM, > > > + * which finally block the SDIO wakeup function. > > > + */ > > > + ret = pinctrl_pm_select_sleep_state(dev); > > > + if (ret) > > > + return ret; > > > } > > > > > > - ret = pinctrl_pm_select_sleep_state(dev); > > > - if (ret) > > > - return ret; > > > - > > > ret = mmc_gpio_set_cd_wake(host->mmc, true); > > > > > > /* > > > -- > > > 2.34.1 > > >
> -----Original Message----- > From: Frank Li <frank.li@nxp.com> > Sent: Friday, May 23, 2025 2:29 AM > To: Bough Chen <haibo.chen@nxp.com> > Cc: Luke Wang <ziniu.wang_1@nxp.com>; adrian.hunter@intel.com; > ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; shawnguo@kernel.org; > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; > imx@lists.linux.dev; dl-S32 <S32@nxp.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] mmc: sdhci-esdhc-imx: do not change pinctrl state in > suspend if function irq is wakeup source > > On Thu, May 22, 2025 at 03:16:11AM +0000, Bough Chen wrote: > > > -----Original Message----- > > > From: Frank Li <frank.li@nxp.com> > > > Sent: 2025年5月21日 23:56 > > > To: Luke Wang <ziniu.wang_1@nxp.com> > > > Cc: Bough Chen <haibo.chen@nxp.com>; adrian.hunter@intel.com; > > > ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; > shawnguo@kernel.org; > > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; > > > imx@lists.linux.dev; dl-S32 <S32@nxp.com>; > > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > > Subject: Re: [PATCH] mmc: sdhci-esdhc-imx: do not change pinctrl state in > > > suspend if function irq is wakeup source > > > > > > On Wed, May 21, 2025 at 11:31:34AM +0800, ziniu.wang_1@nxp.com > wrote: > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > > > pinctrl sleep state may config the pin mux to certain function to save > > > > power in system PM. But if usdhc is setting as wakeup source, like the > > > > card interrupt(SDIO) or card insert interrupt, it depends on the > > > > related pin mux configured to usdhc function pad. > > > > e.g. To support card interrupt(SDIO interrupt), it need the pin is > > > > config as usdhc DATA[1] function pin. > > > > > > I think it should be dts settings wrong. Does one PAD set as function > impact > > > power much? > > > > Hi Frank, > > > > I double check the power team, on imx93-11x11-evk board, For SD card on > usdhc2, switch the PAD to gpio function will save about 3~4mw, > > For SDIO wifi on usdhc3, switch the PAD to gpio function will save about > 0.8~1mw. (without wakeup) > > Okay, I think it should be add new pinctrl state ("wakeup"), only set > DATA[0,2,3] > to GPIO and SET DATA[1] to sd function. > > You can change pad setting to "wakeup" state if wake up enabled. > Hi Frank, Adding new pinctrl state will bring both driver and dts changes. What about we only change sleep pinctrl DATA[1] to sd function for SDIO? Regards Luke > Frank > > > > > Regards > > Haibo Chen > > > > > > Frank > > > > > > > > > > > Find the issue on imx93-11x11-evk board, SDIO WiFi in band interrupt > > > > can't wakeup system because the pinctrl sleep state config the DATA[1] > > > > pin as GPIO function. > > > > > > > > For this case, do not change the pinctrl state in suspend. > > > > > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > > > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > > > > --- > > > > drivers/mmc/host/sdhci-esdhc-imx.c | 16 ++++++++++++---- > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c > > > > b/drivers/mmc/host/sdhci-esdhc-imx.c > > > > index 5f1c45b2bd5d..f206b562a6e3 100644 > > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > > > @@ -2057,12 +2057,20 @@ static int sdhci_esdhc_suspend(struct > device > > > *dev) > > > > ret = sdhci_enable_irq_wakeups(host); > > > > if (!ret) > > > > dev_warn(dev, "Failed to enable irq wakeup\n"); > > > > + } else { > > > > + /* > > > > + * For the device which works as wakeup source, no need > > > > + * to change the pinctrl to sleep state. > > > > + * e.g. For SDIO device, the interrupt share with data pin, > > > > + * but the pinctrl sleep state may config the data pin to > > > > + * other function like GPIO function to save power in PM, > > > > + * which finally block the SDIO wakeup function. > > > > + */ > > > > + ret = pinctrl_pm_select_sleep_state(dev); > > > > + if (ret) > > > > + return ret; > > > > } > > > > > > > > - ret = pinctrl_pm_select_sleep_state(dev); > > > > - if (ret) > > > > - return ret; > > > > - > > > > ret = mmc_gpio_set_cd_wake(host->mmc, true); > > > > > > > > /* > > > > -- > > > > 2.34.1 > > > >
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 5f1c45b2bd5d..f206b562a6e3 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -2057,12 +2057,20 @@ static int sdhci_esdhc_suspend(struct device *dev) ret = sdhci_enable_irq_wakeups(host); if (!ret) dev_warn(dev, "Failed to enable irq wakeup\n"); + } else { + /* + * For the device which works as wakeup source, no need + * to change the pinctrl to sleep state. + * e.g. For SDIO device, the interrupt share with data pin, + * but the pinctrl sleep state may config the data pin to + * other function like GPIO function to save power in PM, + * which finally block the SDIO wakeup function. + */ + ret = pinctrl_pm_select_sleep_state(dev); + if (ret) + return ret; } - ret = pinctrl_pm_select_sleep_state(dev); - if (ret) - return ret; - ret = mmc_gpio_set_cd_wake(host->mmc, true); /*