[PATCH 3/5 v3] ESDHC: Power management for ESDHC

[PATCH 3/5 v3] ESDHC: Power management for ESDHC

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

For FSL ESDHC controllor, when enter the sleep, the controller will power off,
therefore the register will lost its valuse, and driver should save value of
register during suspend and used during resume.

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
Signed-off-by: Jiang Yutang <b14898@freescale.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- change the property to compatible for quirks
changes for v3:
	- fix one compile error

 drivers/mmc/host/sdhci-pltfm.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 1c254b1..6791a2a 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -53,6 +53,10 @@ static bool sdhci_of_wp_inverted(struct device_node *np)
 #endif /* CONFIG_PPC */
 }
 
+#ifdef CONFIG_PM
+static int sdhc_pmsaveproctlreg;
+static u32 esdhc_proctl;
+#endif
 void sdhci_get_of_property(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -79,6 +83,11 @@ void sdhci_get_of_property(struct platform_device *pdev)
 			|| of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
 			host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
+#ifdef CONFIG_PM
+		if (of_device_is_compatible(np, "fsl,p1022-esdhc"))
+			sdhc_pmsaveproctlreg = 1;
+#endif
+
 		clk = of_get_property(np, "clock-frequency", &size);
 		if (clk && size == sizeof(*clk) && *clk)
 			pltfm_host->clock = be32_to_cpup(clk);
@@ -206,15 +215,25 @@ int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state)
 {
 	struct sdhci_host *host = platform_get_drvdata(dev);
 
+	if (sdhc_pmsaveproctlreg == 1)
+		esdhc_proctl = sdhci_readl(host, SDHCI_HOST_CONTROL);
+
 	return sdhci_suspend_host(host, state);
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_suspend);
 
 int sdhci_pltfm_resume(struct platform_device *dev)
 {
+	int ret;
 	struct sdhci_host *host = platform_get_drvdata(dev);
 
-	return sdhci_resume_host(host);
+	 host->ops->enable_dma(host);
+
+	ret = mmc_resume_host(host->mmc);
+	if (sdhc_pmsaveproctlreg == 1)
+		sdhci_writel(host, esdhc_proctl, SDHCI_HOST_CONTROL);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_resume);
 #endif	/* CONFIG_PM */
-- 
1.7.5.4

Re: [PATCH 3/5 v3] ESDHC: Power management for ESDHC

[Chris Ball]

Hi,

Wolfram, do you have time to look at this?  Looks like we need to expose
suspend/resume hooks to -pltfm users for this use case -- I don't think
any esdhc code should be in sdhci-pltfm.c.  (I don't mind writing the
patch if you agree that that's the correct solution here.)

Jerry, some style comments below.

Thanks,

- Chris.

On Mon, Dec 26 2011, r66093@freescale.com wrote:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> For FSL ESDHC controllor, when enter the sleep, the controller will power off,
                controller,      entering sleep,
> therefore the register will lost its valuse, and driver should save value of
                              lose its values,
> register during suspend and used during resume.
  registers                   restore them during resume.
>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> Signed-off-by: Jiang Yutang <b14898@freescale.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- change the property to compatible for quirks
> changes for v3:
> 	- fix one compile error
>
>  drivers/mmc/host/sdhci-pltfm.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
> index 1c254b1..6791a2a 100644
> --- a/drivers/mmc/host/sdhci-pltfm.c
> +++ b/drivers/mmc/host/sdhci-pltfm.c
> @@ -53,6 +53,10 @@ static bool sdhci_of_wp_inverted(struct device_node *np)
>  #endif /* CONFIG_PPC */
>  }
>  
> +#ifdef CONFIG_PM
> +static int sdhc_pmsaveproctlreg;
> +static u32 esdhc_proctl;
> +#endif
>  void sdhci_get_of_property(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -79,6 +83,11 @@ void sdhci_get_of_property(struct platform_device *pdev)
>  			|| of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
>  			host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>  
> +#ifdef CONFIG_PM
> +		if (of_device_is_compatible(np, "fsl,p1022-esdhc"))
> +			sdhc_pmsaveproctlreg = 1;
> +#endif
> +
>  		clk = of_get_property(np, "clock-frequency", &size);
>  		if (clk && size == sizeof(*clk) && *clk)
>  			pltfm_host->clock = be32_to_cpup(clk);

error: patch failed: drivers/mmc/host/sdhci-pltfm.c:79
error: drivers/mmc/host/sdhci-pltfm.c: patch does not apply

Needs to be rebased against mmc-next.  But wait until we figure out
what to do with this patch.

> @@ -206,15 +215,25 @@ int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state)
>  {
>  	struct sdhci_host *host = platform_get_drvdata(dev);
>  
> +	if (sdhc_pmsaveproctlreg == 1)
> +		esdhc_proctl = sdhci_readl(host, SDHCI_HOST_CONTROL);
> +

You can skip the "== 1".

>  	return sdhci_suspend_host(host, state);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_pltfm_suspend);
>  
>  int sdhci_pltfm_resume(struct platform_device *dev)
>  {
> +	int ret;
>  	struct sdhci_host *host = platform_get_drvdata(dev);
>  
> -	return sdhci_resume_host(host);
> +	 host->ops->enable_dma(host);

(The indentation looks incorrect here.)

You shouldn't be calling an ops pointer without testing it first.
And you haven't explained why you're calling this unconditionally
for *every* -pltfm driver.  How do you know it doesn't break one
of them?

> +
> +	ret = mmc_resume_host(host->mmc);
> +	if (sdhc_pmsaveproctlreg == 1)
> +		sdhci_writel(host, esdhc_proctl, SDHCI_HOST_CONTROL);
> +
> +	return ret;

== 1 not needed.

>  }
>  EXPORT_SYMBOL_GPL(sdhci_pltfm_resume);
>  #endif	/* CONFIG_PM */

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 3/5 v3] ESDHC: Power management for ESDHC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Mon, Jan 02, 2012 at 07:00:51PM -0500, Chris Ball wrote:

> Wolfram, do you have time to look at this?  Looks like we need to expose
> suspend/resume hooks to -pltfm users for this use case -- I don't think
> any esdhc code should be in sdhci-pltfm.c.  (I don't mind writing the
> patch if you agree that that's the correct solution here.)

I won't have time to look at it, but what you say sounds reasonable to me.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5 v3] ESDHC: Power management for ESDHC

[Chris Ball]

Hi,

On Mon, Jan 02 2012, Wolfram Sang wrote:
>> Wolfram, do you have time to look at this?  Looks like we need to expose
>> suspend/resume hooks to -pltfm users for this use case -- I don't think
>> any esdhc code should be in sdhci-pltfm.c.  (I don't mind writing the
>> patch if you agree that that's the correct solution here.)
>
> I won't have time to look at it, but what you say sounds reasonable to me.

Thanks, Wolfram.  Jerry, does this patch give you the hook you need to
handle this inside the driver instead of sdhci/sdhci-pltfm?

From: Chris Ball <cjb@laptop.org>
Subject: [PATCH] mmc: sdhci: Add platform suspend/resume hooks.

Some platforms require saving/restoring registers across suspend/resume;
this hook allows them to do that inside their driver.

Signed-off-by: Chris Ball <cjb@laptop.org>

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 384adda..3425c9a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2351,6 +2351,9 @@ int sdhci_suspend_host(struct sdhci_host *host)
 {
 	int ret;
 
+	if (host->ops->platform_suspend)
+		host->ops->platform_suspend(host);
+
 	sdhci_disable_card_detection(host);
 
 	/* Disable tuning since we are suspending */
@@ -2391,6 +2394,9 @@ int sdhci_resume_host(struct sdhci_host *host)
 	ret = mmc_resume_host(host->mmc);
 	sdhci_enable_card_detection(host);
 
+	if (host->ops->platform_resume)
+		host->ops->platform_resume(host);
+
 	/* Set the re-tuning expiration flag */
 	if ((host->version >= SDHCI_SPEC_300) && host->tuning_count &&
 	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index a04d4d0..ae05a80 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -274,6 +274,8 @@ struct sdhci_ops {
 	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
 	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
 	void	(*hw_reset)(struct sdhci_host *host);
+	void	(*platform_suspend)(struct sdhci_host *host);
+	void	(*platform_resume)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH 3/5 v3] ESDHC: Power management for ESDHC

[Huang Changming-R66093]

Thanks, Chris.
I will try it.
If it is ok, I will post the new patch.

> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Tuesday, January 03, 2012 10:12 AM
> To: Wolfram Sang
> Cc: Huang Changming-R66093; linux-mmc@vger.kernel.org; Huang Changming-
> R66093; Jiang Yutang-B14898
> Subject: Re: [PATCH 3/5 v3] ESDHC: Power management for ESDHC
> 
> Hi,
> 
> On Mon, Jan 02 2012, Wolfram Sang wrote:
> >> Wolfram, do you have time to look at this?  Looks like we need to
> expose
> >> suspend/resume hooks to -pltfm users for this use case -- I don't
> think
> >> any esdhc code should be in sdhci-pltfm.c.  (I don't mind writing the
> >> patch if you agree that that's the correct solution here.)
> >
> > I won't have time to look at it, but what you say sounds reasonable to
> me.
> 
> Thanks, Wolfram.  Jerry, does this patch give you the hook you need to
> handle this inside the driver instead of sdhci/sdhci-pltfm?
> 
> From: Chris Ball <cjb@laptop.org>
> Subject: [PATCH] mmc: sdhci: Add platform suspend/resume hooks.
> 
> Some platforms require saving/restoring registers across suspend/resume;
> this hook allows them to do that inside their driver.
> 
> Signed-off-by: Chris Ball <cjb@laptop.org>
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 384adda..3425c9a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2351,6 +2351,9 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  {
>  	int ret;
> 
> +	if (host->ops->platform_suspend)
> +		host->ops->platform_suspend(host);
> +
>  	sdhci_disable_card_detection(host);
> 
>  	/* Disable tuning since we are suspending */
> @@ -2391,6 +2394,9 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	ret = mmc_resume_host(host->mmc);
>  	sdhci_enable_card_detection(host);
> 
> +	if (host->ops->platform_resume)
> +		host->ops->platform_resume(host);
> +
>  	/* Set the re-tuning expiration flag */
>  	if ((host->version >= SDHCI_SPEC_300) && host->tuning_count &&
>  	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index a04d4d0..ae05a80 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -274,6 +274,8 @@ struct sdhci_ops {
>  	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
>  	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int
> uhs);
>  	void	(*hw_reset)(struct sdhci_host *host);
> +	void	(*platform_suspend)(struct sdhci_host *host);
> +	void	(*platform_resume)(struct sdhci_host *host);
>  };
> 
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child

RE: [PATCH 3/5 v3] ESDHC: Power management for ESDHC

[Huang Changming-R66093]

Hi, Chris
I worked one patch to replace this patch based on yours.
But, the other two patches will be posted because they has been affected by the new hooks.

> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Tuesday, January 03, 2012 10:12 AM
> To: Wolfram Sang
> Cc: Huang Changming-R66093; linux-mmc@vger.kernel.org; Huang Changming-
> R66093; Jiang Yutang-B14898
> Subject: Re: [PATCH 3/5 v3] ESDHC: Power management for ESDHC
> 
> Hi,
> 
> On Mon, Jan 02 2012, Wolfram Sang wrote:
> >> Wolfram, do you have time to look at this?  Looks like we need to
> expose
> >> suspend/resume hooks to -pltfm users for this use case -- I don't
> think
> >> any esdhc code should be in sdhci-pltfm.c.  (I don't mind writing the
> >> patch if you agree that that's the correct solution here.)
> >
> > I won't have time to look at it, but what you say sounds reasonable to
> me.
> 
> Thanks, Wolfram.  Jerry, does this patch give you the hook you need to
> handle this inside the driver instead of sdhci/sdhci-pltfm?
> 
> From: Chris Ball <cjb@laptop.org>
> Subject: [PATCH] mmc: sdhci: Add platform suspend/resume hooks.
> 
> Some platforms require saving/restoring registers across suspend/resume;
> this hook allows them to do that inside their driver.
> 
> Signed-off-by: Chris Ball <cjb@laptop.org>
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 384adda..3425c9a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2351,6 +2351,9 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  {
>  	int ret;
> 
> +	if (host->ops->platform_suspend)
> +		host->ops->platform_suspend(host);
> +
>  	sdhci_disable_card_detection(host);
> 
>  	/* Disable tuning since we are suspending */
> @@ -2391,6 +2394,9 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	ret = mmc_resume_host(host->mmc);
>  	sdhci_enable_card_detection(host);
> 
> +	if (host->ops->platform_resume)
> +		host->ops->platform_resume(host);
> +
>  	/* Set the re-tuning expiration flag */
>  	if ((host->version >= SDHCI_SPEC_300) && host->tuning_count &&
>  	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index a04d4d0..ae05a80 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -274,6 +274,8 @@ struct sdhci_ops {
>  	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
>  	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int
> uhs);
>  	void	(*hw_reset)(struct sdhci_host *host);
> +	void	(*platform_suspend)(struct sdhci_host *host);
> +	void	(*platform_resume)(struct sdhci_host *host);
>  };
> 
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child

RE: [PATCH 3/5 v3] ESDHC: Power management for ESDHC

[Huang Changming-R66093]

And I have one question,
This group patches has five, and this version will only include 3/5, 4/5, 5/5.
Can I repost the 1/5 and 2/5? These two patches has no changes.

> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Tuesday, January 03, 2012 10:12 AM
> To: Wolfram Sang
> Cc: Huang Changming-R66093; linux-mmc@vger.kernel.org; Huang Changming-
> R66093; Jiang Yutang-B14898
> Subject: Re: [PATCH 3/5 v3] ESDHC: Power management for ESDHC
> 
> Hi,
> 
> On Mon, Jan 02 2012, Wolfram Sang wrote:
> >> Wolfram, do you have time to look at this?  Looks like we need to
> expose
> >> suspend/resume hooks to -pltfm users for this use case -- I don't
> think
> >> any esdhc code should be in sdhci-pltfm.c.  (I don't mind writing the
> >> patch if you agree that that's the correct solution here.)
> >
> > I won't have time to look at it, but what you say sounds reasonable to
> me.
> 
> Thanks, Wolfram.  Jerry, does this patch give you the hook you need to
> handle this inside the driver instead of sdhci/sdhci-pltfm?
> 
> From: Chris Ball <cjb@laptop.org>
> Subject: [PATCH] mmc: sdhci: Add platform suspend/resume hooks.
> 
> Some platforms require saving/restoring registers across suspend/resume;
> this hook allows them to do that inside their driver.
> 
> Signed-off-by: Chris Ball <cjb@laptop.org>
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 384adda..3425c9a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2351,6 +2351,9 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  {
>  	int ret;
> 
> +	if (host->ops->platform_suspend)
> +		host->ops->platform_suspend(host);
> +
>  	sdhci_disable_card_detection(host);
> 
>  	/* Disable tuning since we are suspending */
> @@ -2391,6 +2394,9 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	ret = mmc_resume_host(host->mmc);
>  	sdhci_enable_card_detection(host);
> 
> +	if (host->ops->platform_resume)
> +		host->ops->platform_resume(host);
> +
>  	/* Set the re-tuning expiration flag */
>  	if ((host->version >= SDHCI_SPEC_300) && host->tuning_count &&
>  	    (host->tuning_mode == SDHCI_TUNING_MODE_1))
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index a04d4d0..ae05a80 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -274,6 +274,8 @@ struct sdhci_ops {
>  	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
>  	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int
> uhs);
>  	void	(*hw_reset)(struct sdhci_host *host);
> +	void	(*platform_suspend)(struct sdhci_host *host);
> +	void	(*platform_resume)(struct sdhci_host *host);
>  };
> 
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child