Re: [PATCH] mmc: sdhci-esdhc-imx: make sure usdhc clock enabled while doing suspend

[Dong Aisheng <dongas86@gmail.com>]

Hi Haibo,

On Tue, Oct 18, 2016 at 3:39 PM, Haibo Chen <haibo.chen@nxp.com> wrote:
> When suspend usdhc, it will access usdhc register. So usdhc clock
> should be enabled, otherwise the access usdhc register will return
> error or cause system hung.
>
> Take this into consideration, if system enable a usdhc and do not
> connect any SD/SDIO/MMC card, after system boot up, this usdhc
> will do runtime suspend, and close all usdhc clock. At this time,
> if suspend the system, due to no card persent, usdhc runtime resume
> will not be called. So usdhc clock still closed, then in suspend,
> once access usdhc register, system hung or bus error return.
>
> This patch make sure usdhc clock always enabled while doing usdhc
> suspend.
>

Besides the former comments in my last reply, it would be good
to explain a bit more why the former SoC series do not have this issue
in the patch commit message.
That would make people more clear about it.

And more minor comment below:

> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 7123ef9..1df3846 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1322,17 +1322,28 @@ static int sdhci_esdhc_suspend(struct device *dev)
>  {
>         struct sdhci_host *host = dev_get_drvdata(dev);
>
> +#ifdef CONFIG_PM

Do you really need this?

> +       pm_runtime_get_sync(host->mmc->parent);
> +#endif
> +
>         return sdhci_suspend_host(host);
>  }
>
>  static int sdhci_esdhc_resume(struct device *dev)
>  {
>         struct sdhci_host *host = dev_get_drvdata(dev);
> +       int ret;
>
>         /* re-initialize hw state in case it's lost in low power mode */
>         sdhci_esdhc_imx_hwinit(host);
> +       ret = sdhci_resume_host(host);
>
> -       return sdhci_resume_host(host);
> +#ifdef CONFIG_PM

ditto

> +       pm_runtime_mark_last_busy(host->mmc->parent);
> +       pm_runtime_put_autosuspend(host->mmc->parent);
> +#endif
> +
> +       return ret;
>  }
>  #endif
>
> --
> 1.9.1
>

Regards
Dong Aisheg