> -----Original Message----- > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > Sent: Tuesday, August 8, 2023 9:57 AM > To: Liming Sun <limings@xxxxxxxxxx> > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>; David Thompson > <davthompson@xxxxxxxxxx>; Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>; linux- > mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations > > On Tue, 8 Aug 2023 at 15:21, Liming Sun <limings@xxxxxxxxxx> wrote: > > > > > > > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > > > Sent: Tuesday, August 8, 2023 5:40 AM > > > To: Liming Sun <limings@xxxxxxxxxx> > > > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>; David Thompson > > > <davthompson@xxxxxxxxxx>; Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>; > linux- > > > mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM > operations > > > > > > On Sat, 5 Aug 2023 at 01:30, Liming Sun <limings@xxxxxxxxxx> wrote: > > > > > > > > This commit implements the runtime PM operations to disable eMMC > > > > card clock when idle. > > > > > > > > Reviewed-by: David Thompson <davthompson@xxxxxxxxxx> > > > > Signed-off-by: Liming Sun <limings@xxxxxxxxxx> > > > > --- > > > > v5->v6: > > > > - Address Adrian's more comments and add coordination between > > > > runtime PM and system PM; > > > > v4->v5: > > > > - Address Adrian's comment to move the pm_enable to the end to > > > > avoid race; > > > > v3->v4: > > > > - Fix compiling reported by 'kernel test robot'; > > > > v2->v3: > > > > - Revise the commit message; > > > > v1->v2: > > > > Updates for comments from Ulf: > > > > - Make the runtime PM logic generic for sdhci-of-dwcmshc; > > > > v1: Initial version. > > > > --- > > > > drivers/mmc/host/sdhci-of-dwcmshc.c | 72 > > > ++++++++++++++++++++++++++++- > > > > 1 file changed, 70 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c > b/drivers/mmc/host/sdhci- > > > of-dwcmshc.c > > > > index e68cd87998c8..aaf66358f626 100644 > > > > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > > > > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > > > > @@ -15,6 +15,7 @@ > > > > #include <linux/module.h> > > > > #include <linux/of.h> > > > > #include <linux/of_device.h> > > > > +#include <linux/pm_runtime.h> > > > > #include <linux/reset.h> > > > > #include <linux/sizes.h> > > > > > > > > @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct > platform_device > > > *pdev) > > > > > > > > host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > > > > > > > > + pm_runtime_get_noresume(dev); > > > > + pm_runtime_set_active(dev); > > > > + pm_runtime_enable(dev); > > > > + > > > > err = sdhci_setup_host(host); > > > > if (err) > > > > - goto err_clk; > > > > + goto err_rpm; > > > > > > > > if (rk_priv) > > > > dwcmshc_rk35xx_postinit(host, priv); > > > > @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct > platform_device > > > *pdev) > > > > if (err) > > > > goto err_setup_host; > > > > > > > > + pm_runtime_put_sync(dev); > > > > + > > > > > > The async pm_runtime_put() is probably sufficient - and it would allow > > > the probe to complete a bit "sooner". > > > > Sure, will test and update the line in v7. > > > > > > > > > return 0; > > > > > > > > err_setup_host: > > > > sdhci_cleanup_host(host); > > > > +err_rpm: > > > > + pm_runtime_disable(dev); > > > > + pm_runtime_put_noidle(dev); > > > > err_clk: > > > > clk_disable_unprepare(pltfm_host->clk); > > > > clk_disable_unprepare(priv->bus_clk); > > > > @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev) > > > > if (ret) > > > > return ret; > > > > > > > > + ret = pm_runtime_force_suspend(dev); > > > > > > Can dwcmshc_suspend() be called for a device that may be attached to > > > the ACPI PM domain? > > > > BlueField SoC is the only chip that uses ACPI in this driver for now and it > doesn't support System Sleep. Thus, the dwcmshc_suspend() won't be called. > But I guess it might be possible when other new chip support is added into this > driver. Is it a concern? > > The ACPI PM domain doesn't support drivers using > pm_runtime_force_suspend|resume(). Unless that has been changed > without me knowing about it. > > Anyway, it looks like it shouldn't be a problem at this point. We can > also add separate callbacks for other SoCs, if that comes into play > going forward. Thanks. So no further changes for now. Posted v7 (for the other comment). > > [...] > > Kind regards > Uffe
