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

[Chris Ball <cjb@laptop.org>]

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