[PATCH v1 1/1] mmc: sdhci-of-dwcmshc: Add runtime PM operations for BlueField-3

[PATCH v1 1/1] mmc: sdhci-of-dwcmshc: Add runtime PM operations for BlueField-3

[Liming Sun]

This commit implements the runtime PM operations for BlueField-3 SoC
to disable eMMC card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 76 ++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..19ce058fc5f0 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>
 
@@ -542,8 +543,10 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	}
 
 #ifdef CONFIG_ACPI
-	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
+	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata) {
 		sdhci_enable_v4_mode(host);
+		pm_runtime_enable(dev);
+	}
 #endif
 
 	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
@@ -646,7 +649,76 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+#ifdef CONFIG_ACPI
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl &= ~SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+#endif
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	const struct sdhci_pltfm_data *pltfm_data;
+	int ret = 0;
+
+	pltfm_data = device_get_match_data(dev);
+	if (!pltfm_data)
+		return -ENODEV;
+
+#ifdef CONFIG_ACPI
+	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata) {
+		ret = sdhci_runtime_suspend_host(host);
+		if (!ret)
+			dwcmshc_disable_card_clk(host);
+	}
+#endif
+
+	return ret;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	const struct sdhci_pltfm_data *pltfm_data;
+	int ret = 0;
+
+	pltfm_data = device_get_match_data(dev);
+	if (!pltfm_data)
+		return -ENODEV;
+
+#ifdef CONFIG_ACPI
+	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata) {
+		dwcmshc_enable_card_clk(host);
+		ret = sdhci_runtime_resume_host(host, 0);
+	}
+#endif
+
+	return ret;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v1 1/1] mmc: sdhci-of-dwcmshc: Add runtime PM operations for BlueField-3

[Ulf Hansson]

On Thu, 11 May 2023 at 21:03, Liming Sun <limings@nvidia.com> wrote:
>
> This commit implements the runtime PM operations for BlueField-3 SoC
> to disable eMMC card clock when idle.
>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 76 ++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..19ce058fc5f0 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>
>
> @@ -542,8 +543,10 @@ static int dwcmshc_probe(struct platform_device *pdev)
>         }
>
>  #ifdef CONFIG_ACPI
> -       if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> +       if (pltfm_data == &sdhci_dwcmshc_bf3_pdata) {
>                 sdhci_enable_v4_mode(host);
> +               pm_runtime_enable(dev);

Why make this ACPI specific? Wouldn't other platforms benefit from
this change too?

[...]

Kind regards
Uffe

RE: [PATCH v1 1/1] mmc: sdhci-of-dwcmshc: Add runtime PM operations for BlueField-3

[Liming Sun]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Friday, May 12, 2023 3:36 AM
> To: Liming Sun <limings@nvidia.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>; David Thompson
> <davthompson@nvidia.com>; linux-mmc@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-dwcmshc: Add runtime PM
> operations for BlueField-3
> 
> On Thu, 11 May 2023 at 21:03, Liming Sun <limings@nvidia.com> wrote:
> >
> > This commit implements the runtime PM operations for BlueField-3 SoC
> > to disable eMMC card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 76
> ++++++++++++++++++++++++++++-
> >  1 file changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
> of-dwcmshc.c
> > index e68cd87998c8..19ce058fc5f0 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>
> >
> > @@ -542,8 +543,10 @@ static int dwcmshc_probe(struct platform_device
> *pdev)
> >         }
> >
> >  #ifdef CONFIG_ACPI
> > -       if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> > +       if (pltfm_data == &sdhci_dwcmshc_bf3_pdata) {
> >                 sdhci_enable_v4_mode(host);
> > +               pm_runtime_enable(dev);
> 
> Why make this ACPI specific? Wouldn't other platforms benefit from
> this change too?

Sure, let me post v2 to make it generic for sdhci-of-dwcmshc.

> 
> [...]
> 
> Kind regards
> Uffe

[PATCH v2] mmc: sdhci-of-dwcmshc: Add runtime PM operations for BlueField-3

[Liming Sun]

This commit implements the runtime PM operations for BlueField-3 SoC
to disable eMMC card clock when idle.

Signed-off-by: Liming Sun <limings@nvidia.com>
---
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 | 56 ++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..a3277f4d250d 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>
 
@@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 		sdhci_enable_v4_mode(host);
 #endif
 
+	pm_runtime_enable(dev);
+
 	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 
 	err = sdhci_setup_host(host);
@@ -646,7 +649,58 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+#ifdef CONFIG_ACPI
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl &= ~SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+#endif
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = sdhci_runtime_suspend_host(host);
+	if (!ret)
+		dwcmshc_disable_card_clk(host);
+
+	return ret;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dwcmshc_enable_card_clk(host);
+	ret = sdhci_runtime_resume_host(host, 0);
+
+	return ret;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

[PATCH v3] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
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 | 56 ++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..a3277f4d250d 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>
 
@@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 		sdhci_enable_v4_mode(host);
 #endif
 
+	pm_runtime_enable(dev);
+
 	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 
 	err = sdhci_setup_host(host);
@@ -646,7 +649,58 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+#ifdef CONFIG_ACPI
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl &= ~SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+#endif
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = sdhci_runtime_suspend_host(host);
+	if (!ret)
+		dwcmshc_disable_card_clk(host);
+
+	return ret;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dwcmshc_enable_card_clk(host);
+	ret = sdhci_runtime_resume_host(host, 0);
+
+	return ret;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v3] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[kernel test robot]

Hi Liming,

kernel test robot noticed the following build errors:

[auto build test ERROR on ulf-hansson-mmc-mirror/next]
[also build test ERROR on linus/master v6.4-rc1 next-20230512]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Liming-Sun/mmc-sdhci-of-dwcmshc-Add-runtime-PM-operations/20230512-202948
base:   https://git.linaro.org/people/ulf.hansson/mmc-mirror.git next
patch link:    https://lore.kernel.org/r/20230512122648.223974-1-limings%40nvidia.com
patch subject: [PATCH v3] mmc: sdhci-of-dwcmshc: Add runtime PM operations
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20230513/202305130116.ynGq0pC5-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/eb5d4c0702ce24630f3d82a37f39437f52607cbb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Liming-Sun/mmc-sdhci-of-dwcmshc-Add-runtime-PM-operations/20230512-202948
        git checkout eb5d4c0702ce24630f3d82a37f39437f52607cbb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305130116.ynGq0pC5-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/mmc/host/sdhci-of-dwcmshc.c: In function 'dwcmshc_runtime_suspend':
>> drivers/mmc/host/sdhci-of-dwcmshc.c:674:17: error: implicit declaration of function 'dwcmshc_disable_card_clk' [-Werror=implicit-function-declaration]
     674 |                 dwcmshc_disable_card_clk(host);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/mmc/host/sdhci-of-dwcmshc.c: In function 'dwcmshc_runtime_resume':
>> drivers/mmc/host/sdhci-of-dwcmshc.c:684:9: error: implicit declaration of function 'dwcmshc_enable_card_clk' [-Werror=implicit-function-declaration]
     684 |         dwcmshc_enable_card_clk(host);
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/dwcmshc_disable_card_clk +674 drivers/mmc/host/sdhci-of-dwcmshc.c

   666	
   667	static int dwcmshc_runtime_suspend(struct device *dev)
   668	{
   669		struct sdhci_host *host = dev_get_drvdata(dev);
   670		int ret = 0;
   671	
   672		ret = sdhci_runtime_suspend_host(host);
   673		if (!ret)
 > 674			dwcmshc_disable_card_clk(host);
   675	
   676		return ret;
   677	}
   678	
   679	static int dwcmshc_runtime_resume(struct device *dev)
   680	{
   681		struct sdhci_host *host = dev_get_drvdata(dev);
   682		int ret = 0;
   683	
 > 684		dwcmshc_enable_card_clk(host);
   685		ret = sdhci_runtime_resume_host(host, 0);
   686	
   687		return ret;
   688	}
   689	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH v4] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
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 | 54 ++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..2d780a5bc8fb 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>
 
@@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 		sdhci_enable_v4_mode(host);
 #endif
 
+	pm_runtime_enable(dev);
+
 	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 
 	err = sdhci_setup_host(host);
@@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl &= ~SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = sdhci_runtime_suspend_host(host);
+	if (!ret)
+		dwcmshc_disable_card_clk(host);
+
+	return ret;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dwcmshc_enable_card_clk(host);
+	ret = sdhci_runtime_resume_host(host, 0);
+
+	return ret;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v4] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 12/05/23 21:15, Liming Sun wrote:
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
> 
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> 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 | 54 ++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..2d780a5bc8fb 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>
>  
> @@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  		sdhci_enable_v4_mode(host);
>  #endif
>  
> +	pm_runtime_enable(dev);

Is there a reason to call it this early?  That raises questions
about runtime PM racing with the rest of the host initialization.
Perhaps below instead (note also using devm):

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 2d780a5bc8fb..5cee42d72257 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -547,8 +547,6 @@ static int dwcmshc_probe(struct platform_device *pdev)
 		sdhci_enable_v4_mode(host);
 #endif
 
-	pm_runtime_enable(dev);
-
 	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 
 	err = sdhci_setup_host(host);
@@ -562,6 +560,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	if (err)
 		goto err_setup_host;
 
+	devm_pm_runtime_enable(dev);
+
 	return 0;
 
 err_setup_host:


> +
>  	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>  
>  	err = sdhci_setup_host(host);
> @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	ctrl |= SDHCI_CLOCK_CARD_EN;
> +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	ret = sdhci_runtime_suspend_host(host);
> +	if (!ret)
> +		dwcmshc_disable_card_clk(host);
> +
> +	return ret;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	dwcmshc_enable_card_clk(host);
> +	ret = sdhci_runtime_resume_host(host, 0);
> +
> +	return ret;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +			   dwcmshc_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver sdhci_dwcmshc_driver = {
>  	.driver	= {

RE: [PATCH v4] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

Done, updated in v5.
(sorry, not sure how I missed this comment earlier).

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Friday, May 19, 2023 9:19 AM
> To: Liming Sun <limings@nvidia.com>; Ulf Hansson <ulf.hansson@linaro.org>;
> David Thompson <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-
> chips.com>
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On 12/05/23 21:15, Liming Sun wrote:
> > This commit implements the runtime PM operations to disable eMMC
> > card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> > 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 | 54
> ++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
> of-dwcmshc.c
> > index e68cd87998c8..2d780a5bc8fb 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>
> >
> > @@ -546,6 +547,8 @@ static int dwcmshc_probe(struct platform_device
> *pdev)
> >  		sdhci_enable_v4_mode(host);
> >  #endif
> >
> > +	pm_runtime_enable(dev);
> 
> Is there a reason to call it this early?  That raises questions
> about runtime PM racing with the rest of the host initialization.
> Perhaps below instead (note also using devm):
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
> of-dwcmshc.c
> index 2d780a5bc8fb..5cee42d72257 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -547,8 +547,6 @@ static int dwcmshc_probe(struct platform_device
> *pdev)
>  		sdhci_enable_v4_mode(host);
>  #endif
> 
> -	pm_runtime_enable(dev);
> -
>  	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> 
>  	err = sdhci_setup_host(host);
> @@ -562,6 +560,8 @@ static int dwcmshc_probe(struct platform_device
> *pdev)
>  	if (err)
>  		goto err_setup_host;
> 
> +	devm_pm_runtime_enable(dev);
> +
>  	return 0;
> 
>  err_setup_host:
> 
> 
> > +
> >  	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> >
> >  	err = sdhci_setup_host(host);
> > @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev)
> >  }
> >  #endif
> >
> > -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend,
> dwcmshc_resume);
> > +#ifdef CONFIG_PM
> > +
> > +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> > +{
> > +	u16 ctrl;
> > +
> > +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +	ctrl |= SDHCI_CLOCK_CARD_EN;
> > +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> > +}
> > +
> > +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> > +{
> > +	u16 ctrl;
> > +
> > +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +	ctrl &= ~SDHCI_CLOCK_CARD_EN;
> > +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> > +}
> > +
> > +static int dwcmshc_runtime_suspend(struct device *dev)
> > +{
> > +	struct sdhci_host *host = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	ret = sdhci_runtime_suspend_host(host);
> > +	if (!ret)
> > +		dwcmshc_disable_card_clk(host);
> > +
> > +	return ret;
> > +}
> > +
> > +static int dwcmshc_runtime_resume(struct device *dev)
> > +{
> > +	struct sdhci_host *host = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	dwcmshc_enable_card_clk(host);
> > +	ret = sdhci_runtime_resume_host(host, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +#endif
> > +
> > +static const struct dev_pm_ops dwcmshc_pmops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> > +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> > +			   dwcmshc_runtime_resume, NULL)
> > +};
> >
> >  static struct platform_driver sdhci_dwcmshc_driver = {
> >  	.driver	= {

[PATCH v5] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
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 | 54 ++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..5cee42d72257 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>
 
@@ -559,6 +560,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	if (err)
 		goto err_setup_host;
 
+	devm_pm_runtime_enable(dev);
+
 	return 0;
 
 err_setup_host:
@@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	ctrl &= ~SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = sdhci_runtime_suspend_host(host);
+	if (!ret)
+		dwcmshc_disable_card_clk(host);
+
+	return ret;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dwcmshc_enable_card_clk(host);
+	ret = sdhci_runtime_resume_host(host, 0);
+
+	return ret;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v5] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 28/07/23 15:20, Liming Sun wrote:
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
> 
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> 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 | 54 ++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..5cee42d72257 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>
>  
> @@ -559,6 +560,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_setup_host;
>  
> +	devm_pm_runtime_enable(dev);

By default, runtime PM regards the device as not active, so
typically drivers will use something like pm_runtime_set_active()
prior to pm_runtime_enable(dev)

In fact it is better to enable before adding the host but
increment the usage count to prevent runtime suspend.  That
means adding some get/puts, ending up with something like:

+	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);

	err = __sdhci_add_host(host);
	if (err)
		goto err_setup_host;

+	pm_runtime_put_sync(dev);

	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);
	if (rk_priv)
		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
					   rk_priv->rockchip_clks);
free_pltfm:
	sdhci_pltfm_free(pdev);
	return err;

> +
>  	return 0;
>  
>  err_setup_host:
> @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);

You could save an mmio write:

	if (ctrl & SDHCI_CLOCK_INT_EN && !(ctrl & SDHCI_CLOCK_CARD_EN)) {

> +	ctrl |= SDHCI_CLOCK_CARD_EN;
> +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);

	}

> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);

You could save an mmio write:

	if (ctrl & SDHCI_CLOCK_CARD_EN) {

> +	ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);

	}

> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret = 0;

ret doesn't need initialization

> +
> +	ret = sdhci_runtime_suspend_host(host);

If you *only* want to disable the card clock, then
it is probably not necessary to call sdhci_runtime_suspend_host()
and sdhci_runtime_resume_host().

> +	if (!ret)
> +		dwcmshc_disable_card_clk(host);
> +
> +	return ret;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret = 0;

ret isn't needed

> +
> +	dwcmshc_enable_card_clk(host);
> +	ret = sdhci_runtime_resume_host(host, 0);

just
	return sdhci_runtime_resume_host(host, 0);

> +
> +	return ret;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)

Typically you need a way to coordinate runtime PM and system PM, refer:

https://www.kernel.org/doc/html/latest/power/runtime_pm.html#runtime-pm-and-system-sleep

> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +			   dwcmshc_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver sdhci_dwcmshc_driver = {
>  	.driver	= {

RE: [PATCH v5] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Tuesday, August 1, 2023 11:37 AM
> To: Liming Sun <limings@nvidia.com>; Ulf Hansson <ulf.hansson@linaro.org>;
> David Thompson <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-
> chips.com>
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On 28/07/23 15:20, Liming Sun wrote:
> > This commit implements the runtime PM operations to disable eMMC
> > card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> > 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 | 54
> ++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
> of-dwcmshc.c
> > index e68cd87998c8..5cee42d72257 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>
> >
> > @@ -559,6 +560,8 @@ static int dwcmshc_probe(struct platform_device
> *pdev)
> >  	if (err)
> >  		goto err_setup_host;
> >
> > +	devm_pm_runtime_enable(dev);
> 
> By default, runtime PM regards the device as not active, so
> typically drivers will use something like pm_runtime_set_active()
> prior to pm_runtime_enable(dev)
> 
> In fact it is better to enable before adding the host but
> increment the usage count to prevent runtime suspend.  That
> means adding some get/puts, ending up with something like:
> 
> +	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);
> 
> 	err = __sdhci_add_host(host);
> 	if (err)
> 		goto err_setup_host;
> 
> +	pm_runtime_put_sync(dev);
> 
> 	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);
> 	if (rk_priv)
> 		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> 					   rk_priv->rockchip_clks);
> free_pltfm:
> 	sdhci_pltfm_free(pdev);
> 	return err;
> 

Updated in v6.

> > +
> >  	return 0;
> >
> >  err_setup_host:
> > @@ -646,7 +649,56 @@ static int dwcmshc_resume(struct device *dev)
> >  }
> >  #endif
> >
> > -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend,
> dwcmshc_resume);
> > +#ifdef CONFIG_PM
> > +
> > +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> > +{
> > +	u16 ctrl;
> > +
> > +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> 
> You could save an mmio write:
> 
> 	if (ctrl & SDHCI_CLOCK_INT_EN && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> 
> > +	ctrl |= SDHCI_CLOCK_CARD_EN;
> > +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> 
> 	}

Updated in v6.

> 
> > +}
> > +
> > +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> > +{
> > +	u16 ctrl;
> > +
> > +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> 
> You could save an mmio write:
> 
> 	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> 
> > +	ctrl &= ~SDHCI_CLOCK_CARD_EN;
> > +	sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> 
> 	}

Updated in v6.

> 
> > +}
> > +
> > +static int dwcmshc_runtime_suspend(struct device *dev)
> > +{
> > +	struct sdhci_host *host = dev_get_drvdata(dev);
> > +	int ret = 0;
> 
> ret doesn't need initialization

Updated in v6.

> 
> > +
> > +	ret = sdhci_runtime_suspend_host(host);
> 
> If you *only* want to disable the card clock, then
> it is probably not necessary to call sdhci_runtime_suspend_host()
> and sdhci_runtime_resume_host().
> 

If, only cares about the card clock. Tested and removed the 
sdhci_runtime_suspend_host() and sdhci_runtime_resume_host()
to keep it simple. 

> > +	if (!ret)
> > +		dwcmshc_disable_card_clk(host);
> > +
> > +	return ret;
> > +}
> > +
> > +static int dwcmshc_runtime_resume(struct device *dev)
> > +{
> > +	struct sdhci_host *host = dev_get_drvdata(dev);
> > +	int ret = 0;
> 
> ret isn't needed

Removed in v6.

> 
> > +
> > +	dwcmshc_enable_card_clk(host);
> > +	ret = sdhci_runtime_resume_host(host, 0);
> 
> just
> 	return sdhci_runtime_resume_host(host, 0);

Updated in v6.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +#endif
> > +
> > +static const struct dev_pm_ops dwcmshc_pmops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> 
> Typically you need a way to coordinate runtime PM and system PM, refer:
> 
> https://www.kernel.org/doc/html/latest/power/runtime_pm.html#runtime-
> pm-and-system-sleep

Thanks. Added pm_runtime_force_suspend() and
pm_runtime_force_resume() in the system sleep() 
and resume() function to make the two states consistent.

> 
> > +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> > +			   dwcmshc_runtime_resume, NULL)
> > +};
> >
> >  static struct platform_driver sdhci_dwcmshc_driver = {
> >  	.driver	= {

[PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
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);
+
 	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);
+	if (ret) {
+		sdhci_resume_host(host);
+		return ret;
+	}
+
 	clk_disable_unprepare(pltfm_host->clk);
 	if (!IS_ERR(priv->bus_clk))
 		clk_disable_unprepare(priv->bus_clk);
@@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
 	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
 	ret = clk_prepare_enable(pltfm_host->clk);
 	if (ret)
 		return ret;
@@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
+		ctrl |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (ctrl & SDHCI_CLOCK_CARD_EN) {
+		ctrl &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_disable_card_clk(host);
+
+	return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_enable_card_clk(host);
+
+	return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Sat, 5 Aug 2023 at 01:30, Liming Sun <limings@nvidia.com> wrote:
>
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> 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".

>         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?

> +       if (ret) {
> +               sdhci_resume_host(host);
> +               return ret;
> +       }
> +
>         clk_disable_unprepare(pltfm_host->clk);
>         if (!IS_ERR(priv->bus_clk))
>                 clk_disable_unprepare(priv->bus_clk);
> @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
>         struct rk35xx_priv *rk_priv = priv->priv;
>         int ret;
>
> +       ret = pm_runtime_force_resume(dev);
> +       if (ret)
> +               return ret;
> +
>         ret = clk_prepare_enable(pltfm_host->clk);
>         if (ret)
>                 return ret;
> @@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +       u16 ctrl;
> +
> +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +               ctrl |= SDHCI_CLOCK_CARD_EN;
> +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +       }
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +       u16 ctrl;
> +
> +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +               ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +       }
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       dwcmshc_disable_card_clk(host);
> +
> +       return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       dwcmshc_enable_card_clk(host);
> +
> +       return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +       SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +                          dwcmshc_runtime_resume, NULL)
> +};
>
>  static struct platform_driver sdhci_dwcmshc_driver = {
>         .driver = {
> --

Kind regards
Uffe

RE: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Tuesday, August 8, 2023 5:40 AM
> To: Liming Sun <limings@nvidia.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>; David Thompson
> <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On Sat, 5 Aug 2023 at 01:30, Liming Sun <limings@nvidia.com> wrote:
> >
> > This commit implements the runtime PM operations to disable eMMC
> > card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> > 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?

> 
> > +       if (ret) {
> > +               sdhci_resume_host(host);
> > +               return ret;
> > +       }
> > +
> >         clk_disable_unprepare(pltfm_host->clk);
> >         if (!IS_ERR(priv->bus_clk))
> >                 clk_disable_unprepare(priv->bus_clk);
> > @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
> >         struct rk35xx_priv *rk_priv = priv->priv;
> >         int ret;
> >
> > +       ret = pm_runtime_force_resume(dev);
> > +       if (ret)
> > +               return ret;
> > +
> >         ret = clk_prepare_enable(pltfm_host->clk);
> >         if (ret)
> >                 return ret;
> > @@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
> >  }
> >  #endif
> >
> > -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend,
> dwcmshc_resume);
> > +#ifdef CONFIG_PM
> > +
> > +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> > +{
> > +       u16 ctrl;
> > +
> > +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +       if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN))
> {
> > +               ctrl |= SDHCI_CLOCK_CARD_EN;
> > +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> > +       }
> > +}
> > +
> > +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> > +{
> > +       u16 ctrl;
> > +
> > +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +       if (ctrl & SDHCI_CLOCK_CARD_EN) {
> > +               ctrl &= ~SDHCI_CLOCK_CARD_EN;
> > +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> > +       }
> > +}
> > +
> > +static int dwcmshc_runtime_suspend(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +
> > +       dwcmshc_disable_card_clk(host);
> > +
> > +       return 0;
> > +}
> > +
> > +static int dwcmshc_runtime_resume(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +
> > +       dwcmshc_enable_card_clk(host);
> > +
> > +       return 0;
> > +}
> > +
> > +#endif
> > +
> > +static const struct dev_pm_ops dwcmshc_pmops = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> > +       SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> > +                          dwcmshc_runtime_resume, NULL)
> > +};
> >
> >  static struct platform_driver sdhci_dwcmshc_driver = {
> >         .driver = {
> > --
> 
> Kind regards
> Uffe

Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Tue, 8 Aug 2023 at 15:21, Liming Sun <limings@nvidia.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Tuesday, August 8, 2023 5:40 AM
> > To: Liming Sun <limings@nvidia.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>; David Thompson
> > <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>; linux-
> > mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> >
> > On Sat, 5 Aug 2023 at 01:30, Liming Sun <limings@nvidia.com> wrote:
> > >
> > > This commit implements the runtime PM operations to disable eMMC
> > > card clock when idle.
> > >
> > > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > > Signed-off-by: Liming Sun <limings@nvidia.com>
> > > ---
> > > 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.

[...]

Kind regards
Uffe

[PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
v6->v7:
    - Address Ulf's comment;
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..c8e145031429 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(dev);
+
 	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);
+	if (ret) {
+		sdhci_resume_host(host);
+		return ret;
+	}
+
 	clk_disable_unprepare(pltfm_host->clk);
 	if (!IS_ERR(priv->bus_clk))
 		clk_disable_unprepare(priv->bus_clk);
@@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
 	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
 	ret = clk_prepare_enable(pltfm_host->clk);
 	if (ret)
 		return ret;
@@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
+		ctrl |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (ctrl & SDHCI_CLOCK_CARD_EN) {
+		ctrl &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_disable_card_clk(host);
+
+	return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_enable_card_clk(host);
+
+	return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

RE: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Tuesday, August 8, 2023 9:57 AM
> To: Liming Sun <limings@nvidia.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>; David Thompson
> <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On Tue, 8 Aug 2023 at 15:21, Liming Sun <limings@nvidia.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > Sent: Tuesday, August 8, 2023 5:40 AM
> > > To: Liming Sun <limings@nvidia.com>
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>; David Thompson
> > > <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>;
> linux-
> > > mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM
> operations
> > >
> > > On Sat, 5 Aug 2023 at 01:30, Liming Sun <limings@nvidia.com> wrote:
> > > >
> > > > This commit implements the runtime PM operations to disable eMMC
> > > > card clock when idle.
> > > >
> > > > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > > > Signed-off-by: Liming Sun <limings@nvidia.com>
> > > > ---
> > > > 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

Re: [PATCH v6] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Sat, 5 Aug 2023 at 01:30, Liming Sun <limings@nvidia.com> wrote:
>
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> 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);
> +
>         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;
>

sdhci_suspend_host() is called a few lines above, which requires the
host to be runtime resumed when called. This may not be the case here.

To fix this there are in principle two options:

1) Call a pm_runtime_get_sync() around here and a corresponding
pm_runtime_put_noidle() somewhere after you have called
pm_runtime_force_suspend().

2) Move away from using sdhci_suspend|resume_host(), but use
sdhci_runtime_suspend|resume_host() instead. This requires some
additional changes, but with the benefit that we can avoid runtime
resuming the device upfront. In this case,
sdhci_runtime_suspend|resume_host() should be called from
dwcmshc_runtime_suspend|resume(). Moreover, we either need to move the
entire clock mgmt from dwcmshc_suspend|resume() into the
dwcmshc_runtime_suspend|resume() or call pm_runtime_get_noresume()
before calling pm_runtime_force_suspend() and pm_runtime_put_noidle()
just after it.

> +       ret = pm_runtime_force_suspend(dev);
> +       if (ret) {
> +               sdhci_resume_host(host);
> +               return ret;
> +       }
> +
>         clk_disable_unprepare(pltfm_host->clk);
>         if (!IS_ERR(priv->bus_clk))
>                 clk_disable_unprepare(priv->bus_clk);
> @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
>         struct rk35xx_priv *rk_priv = priv->priv;
>         int ret;
>
> +       ret = pm_runtime_force_resume(dev);
> +       if (ret)
> +               return ret;
> +
>         ret = clk_prepare_enable(pltfm_host->clk);
>         if (ret)
>                 return ret;
> @@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +       u16 ctrl;
> +
> +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +               ctrl |= SDHCI_CLOCK_CARD_EN;
> +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +       }
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +       u16 ctrl;
> +
> +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +               ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +       }
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       dwcmshc_disable_card_clk(host);
> +
> +       return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       dwcmshc_enable_card_clk(host);
> +
> +       return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +       SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +                          dwcmshc_runtime_resume, NULL)
> +};
>
>  static struct platform_driver sdhci_dwcmshc_driver = {
>         .driver = {

Kind regards
Uffe

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 8/08/23 23:23, Liming Sun wrote:
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
> 
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> v6->v7:
>     - Address Ulf's comment;
> 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..c8e145031429 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(dev);
> +
>  	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);
> +	if (ret) {
> +		sdhci_resume_host(host);
> +		return ret;
> +	}

Since you are only using the runtime PM callbacks to turn off the card
clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
pm_runtime_force_resume() are not needed at all.

sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
(And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
the result is no clock either way)

sdhci_resume_host() does not restore state unless
SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
or runtime resumed.

So it just needs some comments, for example:

+/*
+ * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has no effect on
+ * system suspend.
+ */
 static int dwcmshc_suspend(struct device *dev)
 
+/*
+ * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent with
+ * either runtime suspended or runtime resumed.
+ */
 static int dwcmshc_resume(struct device *dev)


> +
>  	clk_disable_unprepare(pltfm_host->clk);
>  	if (!IS_ERR(priv->bus_clk))
>  		clk_disable_unprepare(priv->bus_clk);
> @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
>  	struct rk35xx_priv *rk_priv = priv->priv;
>  	int ret;
>  
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = clk_prepare_enable(pltfm_host->clk);
>  	if (ret)
>  		return ret;
> @@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +		ctrl |= SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_disable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_enable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +			   dwcmshc_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver sdhci_dwcmshc_driver = {
>  	.driver	= {

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/08/23 23:23, Liming Sun wrote:
> > This commit implements the runtime PM operations to disable eMMC
> > card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> > v6->v7:
> >     - Address Ulf's comment;
> > 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..c8e145031429 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(dev);
> > +
> >       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);
> > +     if (ret) {
> > +             sdhci_resume_host(host);
> > +             return ret;
> > +     }
>
> Since you are only using the runtime PM callbacks to turn off the card
> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> pm_runtime_force_resume() are not needed at all.

Right, it can be done without these too.

>
> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
> the result is no clock either way)
>
> sdhci_resume_host() does not restore state unless
> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
> or runtime resumed.

Even if this may work, to me, it doesn't look like good practice for
how to use runtime PM in combination with system wide suspend/resume.

The point is, sdhci_suspend|resume_host() may end up reading/writing
to sdhci registers - and we should *not* allow that (because it may
not always work), unless the sdhci controller has been runtime resumed
first, right?

[...]

Kind regards
Uffe

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 10/08/23 13:21, Ulf Hansson wrote:
> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 8/08/23 23:23, Liming Sun wrote:
>>> This commit implements the runtime PM operations to disable eMMC
>>> card clock when idle.
>>>
>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
>>> Signed-off-by: Liming Sun <limings@nvidia.com>
>>> ---
>>> v6->v7:
>>>     - Address Ulf's comment;
>>> 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..c8e145031429 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(dev);
>>> +
>>>       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);
>>> +     if (ret) {
>>> +             sdhci_resume_host(host);
>>> +             return ret;
>>> +     }
>>
>> Since you are only using the runtime PM callbacks to turn off the card
>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
>> pm_runtime_force_resume() are not needed at all.
> 
> Right, it can be done without these too.
> 
>>
>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
>> the result is no clock either way)
>>
>> sdhci_resume_host() does not restore state unless
>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
>> or runtime resumed.
> 
> Even if this may work, to me, it doesn't look like good practice for
> how to use runtime PM in combination with system wide suspend/resume.
> 
> The point is, sdhci_suspend|resume_host() may end up reading/writing
> to sdhci registers - and we should *not* allow that (because it may
> not always work), unless the sdhci controller has been runtime resumed
> first, right?

I am OK with drivers that just want to use runtime PM to turn off a
functional clock.  sdhci-tegra.c is also doing that although using the
clock framework.

Certainly that approach assumes that the host controller's power state
is not changed due to runtime PM.

To ensure that the host controller is runtime resumed before calling
sdhci_suspend_host(), we can just call pm_runtime_resume() I think.

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 10/08/23 13:21, Ulf Hansson wrote:
> > On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 8/08/23 23:23, Liming Sun wrote:
> >>> This commit implements the runtime PM operations to disable eMMC
> >>> card clock when idle.
> >>>
> >>> Reviewed-by: David Thompson <davthompson@nvidia.com>
> >>> Signed-off-by: Liming Sun <limings@nvidia.com>
> >>> ---
> >>> v6->v7:
> >>>     - Address Ulf's comment;
> >>> 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..c8e145031429 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(dev);
> >>> +
> >>>       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);
> >>> +     if (ret) {
> >>> +             sdhci_resume_host(host);
> >>> +             return ret;
> >>> +     }
> >>
> >> Since you are only using the runtime PM callbacks to turn off the card
> >> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> >> pm_runtime_force_resume() are not needed at all.
> >
> > Right, it can be done without these too.
> >
> >>
> >> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
> >> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
> >> the result is no clock either way)
> >>
> >> sdhci_resume_host() does not restore state unless
> >> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
> >> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
> >> or runtime resumed.
> >
> > Even if this may work, to me, it doesn't look like good practice for
> > how to use runtime PM in combination with system wide suspend/resume.
> >
> > The point is, sdhci_suspend|resume_host() may end up reading/writing
> > to sdhci registers - and we should *not* allow that (because it may
> > not always work), unless the sdhci controller has been runtime resumed
> > first, right?
>
> I am OK with drivers that just want to use runtime PM to turn off a
> functional clock.  sdhci-tegra.c is also doing that although using the
> clock framework.

Yes, I agree. At least this works for SoC specific drivers.

>
> Certainly that approach assumes that the host controller's power state
> is not changed due to runtime PM.
>
> To ensure that the host controller is runtime resumed before calling
> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.

Yes, that was kind of what I proposed in the other thread as option 1)
(except for the replacement of pm_runtime_force_suspend|resume).

Although, to be clear I would probably use pm_runtime_get_sync()
instead, to make sure the usage count is incremented too.

I don't have a strong opinion here, but from an optimization point of
view I would at least consider what I proposed in option 2) (in the
other thread). The benefit is that it can allow us to potentially
avoid runtime resuming the device, during system suspend.

Kind regards
Uffe

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 10/08/23 19:34, Ulf Hansson wrote:
> On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 10/08/23 13:21, Ulf Hansson wrote:
>>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 8/08/23 23:23, Liming Sun wrote:
>>>>> This commit implements the runtime PM operations to disable eMMC
>>>>> card clock when idle.
>>>>>
>>>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
>>>>> Signed-off-by: Liming Sun <limings@nvidia.com>
>>>>> ---
>>>>> v6->v7:
>>>>>     - Address Ulf's comment;
>>>>> 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..c8e145031429 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(dev);
>>>>> +
>>>>>       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);
>>>>> +     if (ret) {
>>>>> +             sdhci_resume_host(host);
>>>>> +             return ret;
>>>>> +     }
>>>>
>>>> Since you are only using the runtime PM callbacks to turn off the card
>>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
>>>> pm_runtime_force_resume() are not needed at all.
>>>
>>> Right, it can be done without these too.
>>>
>>>>
>>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
>>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
>>>> the result is no clock either way)
>>>>
>>>> sdhci_resume_host() does not restore state unless
>>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
>>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
>>>> or runtime resumed.
>>>
>>> Even if this may work, to me, it doesn't look like good practice for
>>> how to use runtime PM in combination with system wide suspend/resume.
>>>
>>> The point is, sdhci_suspend|resume_host() may end up reading/writing
>>> to sdhci registers - and we should *not* allow that (because it may
>>> not always work), unless the sdhci controller has been runtime resumed
>>> first, right?
>>
>> I am OK with drivers that just want to use runtime PM to turn off a
>> functional clock.  sdhci-tegra.c is also doing that although using the
>> clock framework.
> 
> Yes, I agree. At least this works for SoC specific drivers.
> 
>>
>> Certainly that approach assumes that the host controller's power state
>> is not changed due to runtime PM.
>>
>> To ensure that the host controller is runtime resumed before calling
>> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
> 
> Yes, that was kind of what I proposed in the other thread as option 1)
> (except for the replacement of pm_runtime_force_suspend|resume).
> 
> Although, to be clear I would probably use pm_runtime_get_sync()
> instead, to make sure the usage count is incremented too.

In that case, a matching pm_runtime_put() is needed also at the
end of the resume callback.

> 
> I don't have a strong opinion here, but from an optimization point of
> view I would at least consider what I proposed in option 2) (in the
> other thread). The benefit is that it can allow us to potentially
> avoid runtime resuming the device, during system suspend.
> 
> Kind regards
> Uffe

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Fri, 11 Aug 2023 at 07:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 10/08/23 19:34, Ulf Hansson wrote:
> > On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 10/08/23 13:21, Ulf Hansson wrote:
> >>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>
> >>>> On 8/08/23 23:23, Liming Sun wrote:
> >>>>> This commit implements the runtime PM operations to disable eMMC
> >>>>> card clock when idle.
> >>>>>
> >>>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
> >>>>> Signed-off-by: Liming Sun <limings@nvidia.com>
> >>>>> ---
> >>>>> v6->v7:
> >>>>>     - Address Ulf's comment;
> >>>>> 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..c8e145031429 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(dev);
> >>>>> +
> >>>>>       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);
> >>>>> +     if (ret) {
> >>>>> +             sdhci_resume_host(host);
> >>>>> +             return ret;
> >>>>> +     }
> >>>>
> >>>> Since you are only using the runtime PM callbacks to turn off the card
> >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> >>>> pm_runtime_force_resume() are not needed at all.
> >>>
> >>> Right, it can be done without these too.
> >>>
> >>>>
> >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
> >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
> >>>> the result is no clock either way)
> >>>>
> >>>> sdhci_resume_host() does not restore state unless
> >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
> >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
> >>>> or runtime resumed.
> >>>
> >>> Even if this may work, to me, it doesn't look like good practice for
> >>> how to use runtime PM in combination with system wide suspend/resume.
> >>>
> >>> The point is, sdhci_suspend|resume_host() may end up reading/writing
> >>> to sdhci registers - and we should *not* allow that (because it may
> >>> not always work), unless the sdhci controller has been runtime resumed
> >>> first, right?
> >>
> >> I am OK with drivers that just want to use runtime PM to turn off a
> >> functional clock.  sdhci-tegra.c is also doing that although using the
> >> clock framework.
> >
> > Yes, I agree. At least this works for SoC specific drivers.
> >
> >>
> >> Certainly that approach assumes that the host controller's power state
> >> is not changed due to runtime PM.
> >>
> >> To ensure that the host controller is runtime resumed before calling
> >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
> >
> > Yes, that was kind of what I proposed in the other thread as option 1)
> > (except for the replacement of pm_runtime_force_suspend|resume).
> >
> > Although, to be clear I would probably use pm_runtime_get_sync()
> > instead, to make sure the usage count is incremented too.
>
> In that case, a matching pm_runtime_put() is needed also at the
> end of the resume callback.

Yes, of course. Or depending if we are using the force_suspend|resume
helper, a pm_runtime_put_noidle is sufficient after
pm_runtime_force_suspend() has been called.

[...]

Kind regards
Uffe

RE: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Friday, August 11, 2023 4:36 AM
> To: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Liming Sun <limings@nvidia.com>; David Thompson
> <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On Fri, 11 Aug 2023 at 07:57, Adrian Hunter <adrian.hunter@intel.com>
> wrote:
> >
> > On 10/08/23 19:34, Ulf Hansson wrote:
> > > On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com>
> wrote:
> > >>
> > >> On 10/08/23 13:21, Ulf Hansson wrote:
> > >>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter
> <adrian.hunter@intel.com> wrote:
> > >>>>
> > >>>> On 8/08/23 23:23, Liming Sun wrote:
> > >>>>> This commit implements the runtime PM operations to disable eMMC
> > >>>>> card clock when idle.
> > >>>>>
> > >>>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
> > >>>>> Signed-off-by: Liming Sun <limings@nvidia.com>
> > >>>>> ---
> > >>>>> v6->v7:
> > >>>>>     - Address Ulf's comment;
> > >>>>> 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..c8e145031429 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(dev);
> > >>>>> +
> > >>>>>       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);
> > >>>>> +     if (ret) {
> > >>>>> +             sdhci_resume_host(host);
> > >>>>> +             return ret;
> > >>>>> +     }
> > >>>>
> > >>>> Since you are only using the runtime PM callbacks to turn off the card
> > >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> > >>>> pm_runtime_force_resume() are not needed at all.
> > >>>
> > >>> Right, it can be done without these too.
> > >>>
> > >>>>
> > >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or
> off.
> > >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so
> presumably
> > >>>> the result is no clock either way)
> > >>>>
> > >>>> sdhci_resume_host() does not restore state unless
> > >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the
> internal clock
> > >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime
> suspended
> > >>>> or runtime resumed.
> > >>>
> > >>> Even if this may work, to me, it doesn't look like good practice for
> > >>> how to use runtime PM in combination with system wide
> suspend/resume.
> > >>>
> > >>> The point is, sdhci_suspend|resume_host() may end up reading/writing
> > >>> to sdhci registers - and we should *not* allow that (because it may
> > >>> not always work), unless the sdhci controller has been runtime resumed
> > >>> first, right?
> > >>
> > >> I am OK with drivers that just want to use runtime PM to turn off a
> > >> functional clock.  sdhci-tegra.c is also doing that although using the
> > >> clock framework.
> > >
> > > Yes, I agree. At least this works for SoC specific drivers.
> > >
> > >>
> > >> Certainly that approach assumes that the host controller's power state
> > >> is not changed due to runtime PM.
> > >>
> > >> To ensure that the host controller is runtime resumed before calling
> > >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
> > >
> > > Yes, that was kind of what I proposed in the other thread as option 1)
> > > (except for the replacement of pm_runtime_force_suspend|resume).
> > >
> > > Although, to be clear I would probably use pm_runtime_get_sync()
> > > instead, to make sure the usage count is incremented too.
> >
> > In that case, a matching pm_runtime_put() is needed also at the
> > end of the resume callback.
> 
> Yes, of course. Or depending if we are using the force_suspend|resume
> helper, a pm_runtime_put_noidle is sufficient after
> pm_runtime_force_suspend() has been called.

Thanks Ulf/Adrian! Plan to upload v8 with the following changes:
- Remove pm_runtime_force_suspend/resume() from dwcmshc_suspend()/dwcmshc_resume() (Adrian's comment).
- Add comments for dwcmshc_resume()/dwcmshc_suspend();
(According to Andrian's comment).
- Add pm_runtime_get_sync()/pm_runtime_put() in dwcmshc_suspend(), which is Ulf option-1.  Option-2 seems more efficient, but it involves more changes and I couldn't test the impact on other SoC. Maybe for future enhancement?
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

[...]

> > > >>>>
> > > >>>> Since you are only using the runtime PM callbacks to turn off the card
> > > >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> > > >>>> pm_runtime_force_resume() are not needed at all.
> > > >>>
> > > >>> Right, it can be done without these too.
> > > >>>
> > > >>>>
> > > >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or
> > off.
> > > >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so
> > presumably
> > > >>>> the result is no clock either way)
> > > >>>>
> > > >>>> sdhci_resume_host() does not restore state unless
> > > >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the
> > internal clock
> > > >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime
> > suspended
> > > >>>> or runtime resumed.
> > > >>>
> > > >>> Even if this may work, to me, it doesn't look like good practice for
> > > >>> how to use runtime PM in combination with system wide
> > suspend/resume.
> > > >>>
> > > >>> The point is, sdhci_suspend|resume_host() may end up reading/writing
> > > >>> to sdhci registers - and we should *not* allow that (because it may
> > > >>> not always work), unless the sdhci controller has been runtime resumed
> > > >>> first, right?
> > > >>
> > > >> I am OK with drivers that just want to use runtime PM to turn off a
> > > >> functional clock.  sdhci-tegra.c is also doing that although using the
> > > >> clock framework.
> > > >
> > > > Yes, I agree. At least this works for SoC specific drivers.
> > > >
> > > >>
> > > >> Certainly that approach assumes that the host controller's power state
> > > >> is not changed due to runtime PM.
> > > >>
> > > >> To ensure that the host controller is runtime resumed before calling
> > > >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
> > > >
> > > > Yes, that was kind of what I proposed in the other thread as option 1)
> > > > (except for the replacement of pm_runtime_force_suspend|resume).
> > > >
> > > > Although, to be clear I would probably use pm_runtime_get_sync()
> > > > instead, to make sure the usage count is incremented too.
> > >
> > > In that case, a matching pm_runtime_put() is needed also at the
> > > end of the resume callback.
> >
> > Yes, of course. Or depending if we are using the force_suspend|resume
> > helper, a pm_runtime_put_noidle is sufficient after
> > pm_runtime_force_suspend() has been called.
>
> Thanks Ulf/Adrian! Plan to upload v8 with the following changes:
> - Remove pm_runtime_force_suspend/resume() from dwcmshc_suspend()/dwcmshc_resume() (Adrian's comment).
> - Add comments for dwcmshc_resume()/dwcmshc_suspend();
> (According to Andrian's comment).
> - Add pm_runtime_get_sync()/pm_runtime_put() in dwcmshc_suspend(), which is Ulf option-1.  Option-2 seems more efficient, but it involves more changes and I couldn't test the impact on other SoC. Maybe for future enhancement?

That works for me!

Kind regards
Uffe

[PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
v7->v8:
    - Address Ulf's comment (option-1);
    - Updates for Adrian;s comment to remove the force_suspend/resume
      in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
      dwcmshc_resume()/dwcmshc_suspend();
v6->v7:
    - Address Ulf's comment;
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 | 76 +++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..2196218c9d79 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(dev);
+
 	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);
@@ -594,6 +604,10 @@ static int dwcmshc_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+/*
+ * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has no effect on
+ * system suspend.
+ */
 static int dwcmshc_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
@@ -602,9 +616,11 @@ static int dwcmshc_suspend(struct device *dev)
 	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
+	pm_runtime_get_sync(dev);
+
 	ret = sdhci_suspend_host(host);
 	if (ret)
-		return ret;
+		goto err_suspend;
 
 	clk_disable_unprepare(pltfm_host->clk);
 	if (!IS_ERR(priv->bus_clk))
@@ -614,9 +630,15 @@ static int dwcmshc_suspend(struct device *dev)
 		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
 					   rk_priv->rockchip_clks);
 
+err_suspend:
+	pm_runtime_put_noidle(dev);
 	return ret;
 }
 
+/*
+ * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent with
+ * either runtime suspended or runtime resumed.
+ */
 static int dwcmshc_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
@@ -646,7 +668,55 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
+		ctrl |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (ctrl & SDHCI_CLOCK_CARD_EN) {
+		ctrl &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_disable_card_clk(host);
+
+	return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_enable_card_clk(host);
+
+	return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 17/08/23 04:06, Liming Sun wrote:
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
> 
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> v7->v8:
>     - Address Ulf's comment (option-1);
>     - Updates for Adrian;s comment to remove the force_suspend/resume
>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
>       dwcmshc_resume()/dwcmshc_suspend();
> v6->v7:
>     - Address Ulf's comment;
> 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 | 76 +++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..2196218c9d79 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(dev);
> +
>  	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);
> @@ -594,6 +604,10 @@ static int dwcmshc_remove(struct platform_device *pdev)
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> +/*
> + * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has no effect on
> + * system suspend.
> + */

This comment isn't needed since pm_runtime_get_sync() will
always runtime resume the device if needed.

>  static int dwcmshc_suspend(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -602,9 +616,11 @@ static int dwcmshc_suspend(struct device *dev)
>  	struct rk35xx_priv *rk_priv = priv->priv;
>  	int ret;
>  
> +	pm_runtime_get_sync(dev);

This needs a corresponding pm_runtime_put() at the end of dwcmshc_resume().

> +
>  	ret = sdhci_suspend_host(host);
>  	if (ret)
> -		return ret;
> +		goto err_suspend;
>  
>  	clk_disable_unprepare(pltfm_host->clk);
>  	if (!IS_ERR(priv->bus_clk))
> @@ -614,9 +630,15 @@ static int dwcmshc_suspend(struct device *dev)
>  		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>  					   rk_priv->rockchip_clks);
>  
> +err_suspend:
> +	pm_runtime_put_noidle(dev);
>  	return ret;
>  }
>  
> +/*
> + * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent with
> + * either runtime suspended or runtime resumed.
> + */

As above, this comment isn't needed.

>  static int dwcmshc_resume(struct device *dev)


As mentioned above, dwcmshc_resume() needs a pm_runtime_put()
to balance the pm_runtime_get_sync().

Could fix up the error path too, but that should be a
separate prior patch.

Probably end up looking like:


diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 20aa9b6327d2..c9b020b7a3f6 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -652,17 +652,33 @@ static int dwcmshc_resume(struct device *dev)
 	if (!IS_ERR(priv->bus_clk)) {
 		ret = clk_prepare_enable(priv->bus_clk);
 		if (ret)
-			return ret;
+			goto disable_clk;
 	}
 
 	if (rk_priv) {
 		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
 					      rk_priv->rockchip_clks);
 		if (ret)
-			return ret;
+			goto disable_bus_clk;
 	}
 
-	return sdhci_resume_host(host);
+	ret = sdhci_resume_host(host);
+	if (ret)
+		goto disable_rockchip_clks;
+
+	pm_runtime_put(dev);
+
+	return 0;
+
+disable_rockchip_clks:
+	if (rk_priv)
+		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, rk_priv->rockchip_clks);
+disable_bus_clk:
+	if (!IS_ERR(priv->bus_clk))
+		clk_disable_unprepare(priv->bus_clk);
+disable_clk:
+	clk_disable_unprepare(pltfm_host->clk);
+	return ret;
 }
 #endif
 



>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -646,7 +668,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +		ctrl |= SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_disable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_enable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +			   dwcmshc_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver sdhci_dwcmshc_driver = {
>  	.driver	= {

[PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
v8->v9:
    - Address Adrian's comment to do the pm_runtime_put() in
      dwcmshc_resume() instead; Error path changes not included yet.
v7->v8:
    - Address Ulf's comment (option-1);
    - Updates for Adrian's comment to remove the force_suspend/resume
      in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
      dwcmshc_resume()/dwcmshc_suspend();
v6->v7:
    - Address Ulf's comment;
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 | 76 +++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..3b40f55ce2a4 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(dev);
+
 	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);
@@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev)
 	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
+	pm_runtime_get_sync(dev);
+
 	ret = sdhci_suspend_host(host);
-	if (ret)
+	if (ret) {
+		pm_runtime_put(dev);
 		return ret;
+	}
 
 	clk_disable_unprepare(pltfm_host->clk);
 	if (!IS_ERR(priv->bus_clk))
@@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev)
 			return ret;
 	}
 
-	return sdhci_resume_host(host);
+	ret = sdhci_resume_host(host);
+	if (ret)
+		return ret;
+
+	pm_runtime_put(dev);
+
+	return 0;
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
+		ctrl |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (ctrl & SDHCI_CLOCK_CARD_EN) {
+		ctrl &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_disable_card_clk(host);
+
+	return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_enable_card_clk(host);
+
+	return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

RE: [PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Thursday, August 17, 2023 3:36 AM
> To: Liming Sun <limings@nvidia.com>; Ulf Hansson <ulf.hansson@linaro.org>;
> David Thompson <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-
> chips.com>
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On 17/08/23 04:06, Liming Sun wrote:
> > This commit implements the runtime PM operations to disable eMMC
> > card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> > v7->v8:
> >     - Address Ulf's comment (option-1);
> >     - Updates for Adrian;s comment to remove the force_suspend/resume
> >       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
> >       dwcmshc_resume()/dwcmshc_suspend();
> > v6->v7:
> >     - Address Ulf's comment;
> > 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 | 76
> +++++++++++++++++++++++++++--
> >  1 file changed, 73 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
> of-dwcmshc.c
> > index e68cd87998c8..2196218c9d79 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(dev);
> > +
> >  	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);
> > @@ -594,6 +604,10 @@ static int dwcmshc_remove(struct platform_device
> *pdev)
> >  }
> >
> >  #ifdef CONFIG_PM_SLEEP
> > +/*
> > + * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has
> no effect on
> > + * system suspend.
> > + */
> 
> This comment isn't needed since pm_runtime_get_sync() will
> always runtime resume the device if needed.

Updated in v9.

> 
> >  static int dwcmshc_suspend(struct device *dev)
> >  {
> >  	struct sdhci_host *host = dev_get_drvdata(dev);
> > @@ -602,9 +616,11 @@ static int dwcmshc_suspend(struct device *dev)
> >  	struct rk35xx_priv *rk_priv = priv->priv;
> >  	int ret;
> >
> > +	pm_runtime_get_sync(dev);
> 
> This needs a corresponding pm_runtime_put() at the end of
> dwcmshc_resume().

Updated in v9.

> 
> > +
> >  	ret = sdhci_suspend_host(host);
> >  	if (ret)
> > -		return ret;
> > +		goto err_suspend;
> >
> >  	clk_disable_unprepare(pltfm_host->clk);
> >  	if (!IS_ERR(priv->bus_clk))
> > @@ -614,9 +630,15 @@ static int dwcmshc_suspend(struct device *dev)
> >  		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >  					   rk_priv->rockchip_clks);
> >
> > +err_suspend:
> > +	pm_runtime_put_noidle(dev);
> >  	return ret;
> >  }
> >
> > +/*
> > + * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent
> with
> > + * either runtime suspended or runtime resumed.
> > + */
> 
> As above, this comment isn't needed.

Updated in v9.

> 
> >  static int dwcmshc_resume(struct device *dev)
> 
> 
> As mentioned above, dwcmshc_resume() needs a pm_runtime_put()
> to balance the pm_runtime_get_sync().

Updated in v9.

> 
> Could fix up the error path too, but that should be a
> separate prior patch.
> 
> Probably end up looking like:
> 
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
> of-dwcmshc.c
> index 20aa9b6327d2..c9b020b7a3f6 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -652,17 +652,33 @@ static int dwcmshc_resume(struct device *dev)
>  	if (!IS_ERR(priv->bus_clk)) {
>  		ret = clk_prepare_enable(priv->bus_clk);
>  		if (ret)
> -			return ret;
> +			goto disable_clk;
>  	}
> 
>  	if (rk_priv) {
>  		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
>  					      rk_priv->rockchip_clks);
>  		if (ret)
> -			return ret;
> +			goto disable_bus_clk;
>  	}
> 
> -	return sdhci_resume_host(host);
> +	ret = sdhci_resume_host(host);
> +	if (ret)
> +		goto disable_rockchip_clks;
> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +
> +disable_rockchip_clks:
> +	if (rk_priv)
> +		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, rk_priv-
> >rockchip_clks);
> +disable_bus_clk:
> +	if (!IS_ERR(priv->bus_clk))
> +		clk_disable_unprepare(priv->bus_clk);
> +disable_clk:
> +	clk_disable_unprepare(pltfm_host->clk);
> +	return ret;
>  }
>  #endif
> 
> 

Do you mean a separate 'post' patch? I left it out in v9 and could post another one for this error path. Or do you prefer the same patch series?

> 
> 
> >  {
> >  	struct sdhci_host *host = dev_get_drvdata(dev);
> > @@ -646,7 +668,55 @@ static int dwcmshc_resume(struct device *dev)
> >  }
> >  #endif
> >
> > -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend,
> dwcmshc_resume);
> > +#ifdef CONFIG_PM
> > +
> > +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> > +{
> > +	u16 ctrl;
> > +
> > +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl &
> SDHCI_CLOCK_CARD_EN)) {
> > +		ctrl |= SDHCI_CLOCK_CARD_EN;
> > +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> > +	}
> > +}
> > +
> > +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> > +{
> > +	u16 ctrl;
> > +
> > +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> > +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> > +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> > +	}
> > +}
> > +
> > +static int dwcmshc_runtime_suspend(struct device *dev)
> > +{
> > +	struct sdhci_host *host = dev_get_drvdata(dev);
> > +
> > +	dwcmshc_disable_card_clk(host);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwcmshc_runtime_resume(struct device *dev)
> > +{
> > +	struct sdhci_host *host = dev_get_drvdata(dev);
> > +
> > +	dwcmshc_enable_card_clk(host);
> > +
> > +	return 0;
> > +}
> > +
> > +#endif
> > +
> > +static const struct dev_pm_ops dwcmshc_pmops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> > +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> > +			   dwcmshc_runtime_resume, NULL)
> > +};
> >
> >  static struct platform_driver sdhci_dwcmshc_driver = {
> >  	.driver	= {

Re: [PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 17/08/23 19:22, Liming Sun wrote:
> 
> 
>> -----Original Message-----
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Sent: Thursday, August 17, 2023 3:36 AM
>> To: Liming Sun <limings@nvidia.com>; Ulf Hansson <ulf.hansson@linaro.org>;
>> David Thompson <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-
>> chips.com>
>> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v8] mmc: sdhci-of-dwcmshc: Add runtime PM operations
>>
>> On 17/08/23 04:06, Liming Sun wrote:
>>> This commit implements the runtime PM operations to disable eMMC
>>> card clock when idle.
>>>
>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
>>> Signed-off-by: Liming Sun <limings@nvidia.com>
>>> ---
>>> v7->v8:
>>>     - Address Ulf's comment (option-1);
>>>     - Updates for Adrian;s comment to remove the force_suspend/resume
>>>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
>>>       dwcmshc_resume()/dwcmshc_suspend();
>>> v6->v7:
>>>     - Address Ulf's comment;
>>> 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 | 76
>> +++++++++++++++++++++++++++--
>>>  1 file changed, 73 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
>> of-dwcmshc.c
>>> index e68cd87998c8..2196218c9d79 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(dev);
>>> +
>>>  	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);
>>> @@ -594,6 +604,10 @@ static int dwcmshc_remove(struct platform_device
>> *pdev)
>>>  }
>>>
>>>  #ifdef CONFIG_PM_SLEEP
>>> +/*
>>> + * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has
>> no effect on
>>> + * system suspend.
>>> + */
>>
>> This comment isn't needed since pm_runtime_get_sync() will
>> always runtime resume the device if needed.
> 
> Updated in v9.
> 
>>
>>>  static int dwcmshc_suspend(struct device *dev)
>>>  {
>>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>>> @@ -602,9 +616,11 @@ static int dwcmshc_suspend(struct device *dev)
>>>  	struct rk35xx_priv *rk_priv = priv->priv;
>>>  	int ret;
>>>
>>> +	pm_runtime_get_sync(dev);
>>
>> This needs a corresponding pm_runtime_put() at the end of
>> dwcmshc_resume().
> 
> Updated in v9.
> 
>>
>>> +
>>>  	ret = sdhci_suspend_host(host);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto err_suspend;
>>>
>>>  	clk_disable_unprepare(pltfm_host->clk);
>>>  	if (!IS_ERR(priv->bus_clk))
>>> @@ -614,9 +630,15 @@ static int dwcmshc_suspend(struct device *dev)
>>>  		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>>>  					   rk_priv->rockchip_clks);
>>>
>>> +err_suspend:
>>> +	pm_runtime_put_noidle(dev);
>>>  	return ret;
>>>  }
>>>
>>> +/*
>>> + * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent
>> with
>>> + * either runtime suspended or runtime resumed.
>>> + */
>>
>> As above, this comment isn't needed.
> 
> Updated in v9.
> 
>>
>>>  static int dwcmshc_resume(struct device *dev)
>>
>>
>> As mentioned above, dwcmshc_resume() needs a pm_runtime_put()
>> to balance the pm_runtime_get_sync().
> 
> Updated in v9.
> 
>>
>> Could fix up the error path too, but that should be a
>> separate prior patch.
>>
>> Probably end up looking like:
>>
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-
>> of-dwcmshc.c
>> index 20aa9b6327d2..c9b020b7a3f6 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -652,17 +652,33 @@ static int dwcmshc_resume(struct device *dev)
>>  	if (!IS_ERR(priv->bus_clk)) {
>>  		ret = clk_prepare_enable(priv->bus_clk);
>>  		if (ret)
>> -			return ret;
>> +			goto disable_clk;
>>  	}
>>
>>  	if (rk_priv) {
>>  		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
>>  					      rk_priv->rockchip_clks);
>>  		if (ret)
>> -			return ret;
>> +			goto disable_bus_clk;
>>  	}
>>
>> -	return sdhci_resume_host(host);
>> +	ret = sdhci_resume_host(host);
>> +	if (ret)
>> +		goto disable_rockchip_clks;
>> +
>> +	pm_runtime_put(dev);
>> +
>> +	return 0;
>> +
>> +disable_rockchip_clks:
>> +	if (rk_priv)
>> +		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS, rk_priv-
>>> rockchip_clks);
>> +disable_bus_clk:
>> +	if (!IS_ERR(priv->bus_clk))
>> +		clk_disable_unprepare(priv->bus_clk);
>> +disable_clk:
>> +	clk_disable_unprepare(pltfm_host->clk);
>> +	return ret;
>>  }
>>  #endif
>>
>>
> 
> Do you mean a separate 'post' patch? I left it out in v9 and could post another one for this error path. Or do you prefer the same patch series?

Same patch series, 2 patches.  First patch fixes the error paths in dwcmshc_resume().
Second patch adds runtime pm.

> 
>>
>>
>>>  {
>>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>>> @@ -646,7 +668,55 @@ static int dwcmshc_resume(struct device *dev)
>>>  }
>>>  #endif
>>>
>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend,
>> dwcmshc_resume);
>>> +#ifdef CONFIG_PM
>>> +
>>> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
>>> +{
>>> +	u16 ctrl;
>>> +
>>> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>> +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl &
>> SDHCI_CLOCK_CARD_EN)) {
>>> +		ctrl |= SDHCI_CLOCK_CARD_EN;
>>> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
>>> +	}
>>> +}
>>> +
>>> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
>>> +{
>>> +	u16 ctrl;
>>> +
>>> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>> +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
>>> +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
>>> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
>>> +	}
>>> +}
>>> +
>>> +static int dwcmshc_runtime_suspend(struct device *dev)
>>> +{
>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>> +
>>> +	dwcmshc_disable_card_clk(host);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dwcmshc_runtime_resume(struct device *dev)
>>> +{
>>> +	struct sdhci_host *host = dev_get_drvdata(dev);
>>> +
>>> +	dwcmshc_enable_card_clk(host);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +#endif
>>> +
>>> +static const struct dev_pm_ops dwcmshc_pmops = {
>>> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
>>> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
>>> +			   dwcmshc_runtime_resume, NULL)
>>> +};
>>>
>>>  static struct platform_driver sdhci_dwcmshc_driver = {
>>>  	.driver	= {
> 

Re: [PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote:
>
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> v8->v9:
>     - Address Adrian's comment to do the pm_runtime_put() in
>       dwcmshc_resume() instead; Error path changes not included yet.
> v7->v8:
>     - Address Ulf's comment (option-1);
>     - Updates for Adrian's comment to remove the force_suspend/resume
>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
>       dwcmshc_resume()/dwcmshc_suspend();
> v6->v7:
>     - Address Ulf's comment;
> 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 | 76 +++++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..3b40f55ce2a4 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(dev);
> +
>         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);
> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev)
>         struct rk35xx_priv *rk_priv = priv->priv;
>         int ret;
>
> +       pm_runtime_get_sync(dev);
> +
>         ret = sdhci_suspend_host(host);
> -       if (ret)
> +       if (ret) {
> +               pm_runtime_put(dev);
>                 return ret;
> +       }
>
>         clk_disable_unprepare(pltfm_host->clk);
>         if (!IS_ERR(priv->bus_clk))
> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev)
>                         return ret;
>         }
>
> -       return sdhci_resume_host(host);
> +       ret = sdhci_resume_host(host);
> +       if (ret)
> +               return ret;
> +
> +       pm_runtime_put(dev);

To simplify the error path, I would suggest that you move the call to
pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a
call to pm_runtime_put_noidle(), somewhere after the call to
pm_runtime_get_sync().

This is because runtime suspend is prevented by the PM core as it
bumps the usage count with a pm_runtime_get_noresume() in the
device_prepare() phase.

> +
> +       return 0;
>  }
>  #endif

[...]

Kind regards
Uffe

Re: [PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Adrian Hunter]

On 18/08/23 12:00, Ulf Hansson wrote:
> On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote:
>>
>> This commit implements the runtime PM operations to disable eMMC
>> card clock when idle.
>>
>> Reviewed-by: David Thompson <davthompson@nvidia.com>
>> Signed-off-by: Liming Sun <limings@nvidia.com>
>> ---
>> v8->v9:
>>     - Address Adrian's comment to do the pm_runtime_put() in
>>       dwcmshc_resume() instead; Error path changes not included yet.
>> v7->v8:
>>     - Address Ulf's comment (option-1);
>>     - Updates for Adrian's comment to remove the force_suspend/resume
>>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
>>       dwcmshc_resume()/dwcmshc_suspend();
>> v6->v7:
>>     - Address Ulf's comment;
>> 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 | 76 +++++++++++++++++++++++++++--
>>  1 file changed, 72 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index e68cd87998c8..3b40f55ce2a4 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(dev);
>> +
>>         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);
>> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev)
>>         struct rk35xx_priv *rk_priv = priv->priv;
>>         int ret;
>>
>> +       pm_runtime_get_sync(dev);
>> +
>>         ret = sdhci_suspend_host(host);
>> -       if (ret)
>> +       if (ret) {
>> +               pm_runtime_put(dev);
>>                 return ret;
>> +       }
>>
>>         clk_disable_unprepare(pltfm_host->clk);
>>         if (!IS_ERR(priv->bus_clk))
>> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev)
>>                         return ret;
>>         }
>>
>> -       return sdhci_resume_host(host);
>> +       ret = sdhci_resume_host(host);
>> +       if (ret)
>> +               return ret;
>> +
>> +       pm_runtime_put(dev);
> 
> To simplify the error path, I would suggest that you move the call to
> pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a
> call to pm_runtime_put_noidle(), somewhere after the call to
> pm_runtime_get_sync().
> 
> This is because runtime suspend is prevented by the PM core as it
> bumps the usage count with a pm_runtime_get_noresume() in the
> device_prepare() phase.

I thought you didn't want to assume that, because in that case
it can just be pm_runtime_resume() instead of pm_runtime_get_sync(),
and then no 'put' is needed at all.

> 
>> +
>> +       return 0;
>>  }
>>  #endif
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

On Fri, 18 Aug 2023 at 11:36, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 18/08/23 12:00, Ulf Hansson wrote:
> > On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote:
> >>
> >> This commit implements the runtime PM operations to disable eMMC
> >> card clock when idle.
> >>
> >> Reviewed-by: David Thompson <davthompson@nvidia.com>
> >> Signed-off-by: Liming Sun <limings@nvidia.com>
> >> ---
> >> v8->v9:
> >>     - Address Adrian's comment to do the pm_runtime_put() in
> >>       dwcmshc_resume() instead; Error path changes not included yet.
> >> v7->v8:
> >>     - Address Ulf's comment (option-1);
> >>     - Updates for Adrian's comment to remove the force_suspend/resume
> >>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
> >>       dwcmshc_resume()/dwcmshc_suspend();
> >> v6->v7:
> >>     - Address Ulf's comment;
> >> 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 | 76 +++++++++++++++++++++++++++--
> >>  1 file changed, 72 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> index e68cd87998c8..3b40f55ce2a4 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(dev);
> >> +
> >>         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);
> >> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev)
> >>         struct rk35xx_priv *rk_priv = priv->priv;
> >>         int ret;
> >>
> >> +       pm_runtime_get_sync(dev);
> >> +
> >>         ret = sdhci_suspend_host(host);
> >> -       if (ret)
> >> +       if (ret) {
> >> +               pm_runtime_put(dev);
> >>                 return ret;
> >> +       }
> >>
> >>         clk_disable_unprepare(pltfm_host->clk);
> >>         if (!IS_ERR(priv->bus_clk))
> >> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev)
> >>                         return ret;
> >>         }
> >>
> >> -       return sdhci_resume_host(host);
> >> +       ret = sdhci_resume_host(host);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       pm_runtime_put(dev);
> >
> > To simplify the error path, I would suggest that you move the call to
> > pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a
> > call to pm_runtime_put_noidle(), somewhere after the call to
> > pm_runtime_get_sync().
> >
> > This is because runtime suspend is prevented by the PM core as it
> > bumps the usage count with a pm_runtime_get_noresume() in the
> > device_prepare() phase.
>
> I thought you didn't want to assume that, because in that case
> it can just be pm_runtime_resume() instead of pm_runtime_get_sync(),
> and then no 'put' is needed at all.

I don't really care, but just wanted to keep it as simple as possible.

So yes, I am fine with a pm_runtime_resume() too. Maybe even simpler
in this case, as we are not using pm_runtime_force_suspend|resume()
anymore.

Kind regards
Uffe

RE: [PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Liming Sun]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Friday, August 18, 2023 6:19 AM
> To: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Liming Sun <limings@nvidia.com>; David Thompson
> <davthompson@nvidia.com>; Shawn Lin <shawn.lin@rock-chips.com>; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9] mmc: sdhci-of-dwcmshc: Add runtime PM operations
> 
> On Fri, 18 Aug 2023 at 11:36, Adrian Hunter <adrian.hunter@intel.com>
> wrote:
> >
> > On 18/08/23 12:00, Ulf Hansson wrote:
> > > On Thu, 17 Aug 2023 at 18:22, Liming Sun <limings@nvidia.com> wrote:
> > >>
> > >> This commit implements the runtime PM operations to disable eMMC
> > >> card clock when idle.
> > >>
> > >> Reviewed-by: David Thompson <davthompson@nvidia.com>
> > >> Signed-off-by: Liming Sun <limings@nvidia.com>
> > >> ---
> > >> v8->v9:
> > >>     - Address Adrian's comment to do the pm_runtime_put() in
> > >>       dwcmshc_resume() instead; Error path changes not included yet.
> > >> v7->v8:
> > >>     - Address Ulf's comment (option-1);
> > >>     - Updates for Adrian's comment to remove the force_suspend/resume
> > >>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
> > >>       dwcmshc_resume()/dwcmshc_suspend();
> > >> v6->v7:
> > >>     - Address Ulf's comment;
> > >> 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 | 76
> +++++++++++++++++++++++++++--
> > >>  1 file changed, 72 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c
> b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > >> index e68cd87998c8..3b40f55ce2a4 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(dev);
> > >> +
> > >>         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);
> > >> @@ -602,9 +612,13 @@ static int dwcmshc_suspend(struct device *dev)
> > >>         struct rk35xx_priv *rk_priv = priv->priv;
> > >>         int ret;
> > >>
> > >> +       pm_runtime_get_sync(dev);
> > >> +
> > >>         ret = sdhci_suspend_host(host);
> > >> -       if (ret)
> > >> +       if (ret) {
> > >> +               pm_runtime_put(dev);
> > >>                 return ret;
> > >> +       }
> > >>
> > >>         clk_disable_unprepare(pltfm_host->clk);
> > >>         if (!IS_ERR(priv->bus_clk))
> > >> @@ -642,11 +656,65 @@ static int dwcmshc_resume(struct device *dev)
> > >>                         return ret;
> > >>         }
> > >>
> > >> -       return sdhci_resume_host(host);
> > >> +       ret = sdhci_resume_host(host);
> > >> +       if (ret)
> > >> +               return ret;
> > >> +
> > >> +       pm_runtime_put(dev);
> > >
> > > To simplify the error path, I would suggest that you move the call to
> > > pm_runtime_put() to dwcmshc_suspend(). In fact what you need is just a
> > > call to pm_runtime_put_noidle(), somewhere after the call to
> > > pm_runtime_get_sync().
> > >
> > > This is because runtime suspend is prevented by the PM core as it
> > > bumps the usage count with a pm_runtime_get_noresume() in the
> > > device_prepare() phase.
> >
> > I thought you didn't want to assume that, because in that case
> > it can just be pm_runtime_resume() instead of pm_runtime_get_sync(),
> > and then no 'put' is needed at all.
> 
> I don't really care, but just wanted to keep it as simple as possible.
> 
> So yes, I am fine with a pm_runtime_resume() too. Maybe even simpler
> in this case, as we are not using pm_runtime_force_suspend|resume()
> anymore.

Thanks. Will post v10 with ' pm_runtime_resume(dev);' in dwcmshc_suspend() to keep it simple.

> 
> Kind regards
> Uffe

[PATCH v10 1/2] mmc : sdhci-of-dwcmshc : add error handling in dwcmshc_resume

[Liming Sun]

This commit adds handling in dwcmshc_resume() for different error
cases.

Signed-off-by: Liming Sun <limings@nvidia.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 31c1892f4ecd..bc332a035032 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -630,17 +630,32 @@ static int dwcmshc_resume(struct device *dev)
 	if (!IS_ERR(priv->bus_clk)) {
 		ret = clk_prepare_enable(priv->bus_clk);
 		if (ret)
-			return ret;
+			goto disable_clk;
 	}
 
 	if (rk_priv) {
 		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
 					      rk_priv->rockchip_clks);
 		if (ret)
-			return ret;
+			goto disable_bus_clk;
 	}
 
-	return sdhci_resume_host(host);
+	ret = sdhci_resume_host(host);
+	if (ret)
+		goto disable_rockchip_clks;
+
+	return 0;
+
+disable_rockchip_clks:
+	if (rk_priv)
+		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
+					   rk_priv->rockchip_clks);
+disable_bus_clk:
+	if (!IS_ERR(priv->bus_clk))
+		clk_disable_unprepare(priv->bus_clk);
+disable_clk:
+	clk_disable_unprepare(pltfm_host->clk);
+	return ret;
 }
 #endif
 
-- 
2.30.1

[PATCH v10 2/2] This commit implements the runtime PM operations to disable eMMC card clock when idle.

[Liming Sun]

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
v9->v10:
    - Simplify the changes with pm_runtime_resume() in
      dwcmshc_suspend().
v8->v9:
    - Address Adrian's comment to do the pm_runtime_put() in
      dwcmshc_resume() instead; Error path changes not included yet.
v7->v8:
    - Address Ulf's comment (option-1);
    - Updates for Adrian's comment to remove the force_suspend/resume
      in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
      dwcmshc_resume()/dwcmshc_suspend();
v6->v7:
    - Address Ulf's comment;
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 | 64 ++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index bc332a035032..3a3bae6948a8 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/platform_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(dev);
+
 	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);
@@ -600,6 +610,8 @@ static int dwcmshc_suspend(struct device *dev)
 	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
+	pm_runtime_resume(dev);
+
 	ret = sdhci_suspend_host(host);
 	if (ret)
 		return ret;
@@ -659,7 +671,55 @@ static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
+		ctrl |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (ctrl & SDHCI_CLOCK_CARD_EN) {
+		ctrl &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_disable_card_clk(host);
+
+	return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	dwcmshc_enable_card_clk(host);
+
+	return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops dwcmshc_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
+	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
+			   dwcmshc_runtime_resume, NULL)
+};
 
 static struct platform_driver sdhci_dwcmshc_driver = {
 	.driver	= {
-- 
2.30.1

Re: [PATCH v10 1/2] mmc : sdhci-of-dwcmshc : add error handling in dwcmshc_resume

[Adrian Hunter]

On 22/08/23 22:59, Liming Sun wrote:
> This commit adds handling in dwcmshc_resume() for different error
> cases.
> 
> Signed-off-by: Liming Sun <limings@nvidia.com>

We don't put a space before ":" in the subject line, and usually
capitalize after that i.e.

mmc: sdhci-of-dwcmshc: Add error handling in dwcmshc_resume()

Otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 31c1892f4ecd..bc332a035032 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -630,17 +630,32 @@ static int dwcmshc_resume(struct device *dev)
>  	if (!IS_ERR(priv->bus_clk)) {
>  		ret = clk_prepare_enable(priv->bus_clk);
>  		if (ret)
> -			return ret;
> +			goto disable_clk;
>  	}
>  
>  	if (rk_priv) {
>  		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
>  					      rk_priv->rockchip_clks);
>  		if (ret)
> -			return ret;
> +			goto disable_bus_clk;
>  	}
>  
> -	return sdhci_resume_host(host);
> +	ret = sdhci_resume_host(host);
> +	if (ret)
> +		goto disable_rockchip_clks;
> +
> +	return 0;
> +
> +disable_rockchip_clks:
> +	if (rk_priv)
> +		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> +					   rk_priv->rockchip_clks);
> +disable_bus_clk:
> +	if (!IS_ERR(priv->bus_clk))
> +		clk_disable_unprepare(priv->bus_clk);
> +disable_clk:
> +	clk_disable_unprepare(pltfm_host->clk);
> +	return ret;
>  }
>  #endif
>  

Re: [PATCH v10 2/2] This commit implements the runtime PM operations to disable eMMC card clock when idle.

[Adrian Hunter]

On 22/08/23 22:59, Liming Sun wrote:
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>

Seem the subject line got lost (it was OK in v9), and the description
is in the subject line.

Apart from that:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> v9->v10:
>     - Simplify the changes with pm_runtime_resume() in
>       dwcmshc_suspend().
> v8->v9:
>     - Address Adrian's comment to do the pm_runtime_put() in
>       dwcmshc_resume() instead; Error path changes not included yet.
> v7->v8:
>     - Address Ulf's comment (option-1);
>     - Updates for Adrian's comment to remove the force_suspend/resume
>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
>       dwcmshc_resume()/dwcmshc_suspend();
> v6->v7:
>     - Address Ulf's comment;
> 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 | 64 ++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index bc332a035032..3a3bae6948a8 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/platform_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(dev);
> +
>  	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);
> @@ -600,6 +610,8 @@ static int dwcmshc_suspend(struct device *dev)
>  	struct rk35xx_priv *rk_priv = priv->priv;
>  	int ret;
>  
> +	pm_runtime_resume(dev);
> +
>  	ret = sdhci_suspend_host(host);
>  	if (ret)
>  		return ret;
> @@ -659,7 +671,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +		ctrl |= SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_disable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_enable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +			   dwcmshc_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver sdhci_dwcmshc_driver = {
>  	.driver	= {

Re: [PATCH v10 1/2] mmc : sdhci-of-dwcmshc : add error handling in dwcmshc_resume

[Ulf Hansson]

On Tue, 22 Aug 2023 at 21:59, Liming Sun <limings@nvidia.com> wrote:
>
> This commit adds handling in dwcmshc_resume() for different error
> cases.
>
> Signed-off-by: Liming Sun <limings@nvidia.com>

Applied for next (I amended the patch according to Adrian's
suggestions), thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 31c1892f4ecd..bc332a035032 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -630,17 +630,32 @@ static int dwcmshc_resume(struct device *dev)
>         if (!IS_ERR(priv->bus_clk)) {
>                 ret = clk_prepare_enable(priv->bus_clk);
>                 if (ret)
> -                       return ret;
> +                       goto disable_clk;
>         }
>
>         if (rk_priv) {
>                 ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
>                                               rk_priv->rockchip_clks);
>                 if (ret)
> -                       return ret;
> +                       goto disable_bus_clk;
>         }
>
> -       return sdhci_resume_host(host);
> +       ret = sdhci_resume_host(host);
> +       if (ret)
> +               goto disable_rockchip_clks;
> +
> +       return 0;
> +
> +disable_rockchip_clks:
> +       if (rk_priv)
> +               clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> +                                          rk_priv->rockchip_clks);
> +disable_bus_clk:
> +       if (!IS_ERR(priv->bus_clk))
> +               clk_disable_unprepare(priv->bus_clk);
> +disable_clk:
> +       clk_disable_unprepare(pltfm_host->clk);
> +       return ret;
>  }
>  #endif
>
> --
> 2.30.1
>

Re: [PATCH v10 2/2] This commit implements the runtime PM operations to disable eMMC card clock when idle.

[Ulf Hansson]

On Tue, 22 Aug 2023 at 21:59, Liming Sun <limings@nvidia.com> wrote:
>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>

Applied for next (I amended the patch according to Adrian's
suggestions), thanks!

Kind regards
Uffe


> ---
> v9->v10:
>     - Simplify the changes with pm_runtime_resume() in
>       dwcmshc_suspend().
> v8->v9:
>     - Address Adrian's comment to do the pm_runtime_put() in
>       dwcmshc_resume() instead; Error path changes not included yet.
> v7->v8:
>     - Address Ulf's comment (option-1);
>     - Updates for Adrian's comment to remove the force_suspend/resume
>       in dwcmshc_resume()/dwcmshc_suspend(); Add comments for
>       dwcmshc_resume()/dwcmshc_suspend();
> v6->v7:
>     - Address Ulf's comment;
> 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 | 64 ++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index bc332a035032..3a3bae6948a8 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/platform_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(dev);
> +
>         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);
> @@ -600,6 +610,8 @@ static int dwcmshc_suspend(struct device *dev)
>         struct rk35xx_priv *rk_priv = priv->priv;
>         int ret;
>
> +       pm_runtime_resume(dev);
> +
>         ret = sdhci_suspend_host(host);
>         if (ret)
>                 return ret;
> @@ -659,7 +671,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +       u16 ctrl;
> +
> +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +               ctrl |= SDHCI_CLOCK_CARD_EN;
> +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +       }
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +       u16 ctrl;
> +
> +       ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +               ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +               sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +       }
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       dwcmshc_disable_card_clk(host);
> +
> +       return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +       dwcmshc_enable_card_clk(host);
> +
> +       return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +       SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +                          dwcmshc_runtime_resume, NULL)
> +};
>
>  static struct platform_driver sdhci_dwcmshc_driver = {
>         .driver = {
> --
> 2.30.1
>