RE: [PATCH 2/2] mmc: renesas_sdhi: keep SCC clock active when tuning

Yoshihiro Shimoda
2020-06-08T06:35:23Z

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Thursday, June 4, 2020 8:21 PM
>
> Tuning procedure switches to lower frequencies but that will turn the
> SCC off and accessing its register then will hang. So, flag when we are
> tuning and keep the current setup of the external clock if we are doing
> so. Note that we still switch to the lower frequency because of the
> internal divider. We just make sure to not modify the external clock.
> This patch depends on a MMC core patch calling the downgrade function
> earlier.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/mmc/host/renesas_sdhi.h | 1 +
> drivers/mmc/host/renesas_sdhi_core.c | 25 ++++++++++++++++++++++---
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
> index 14c64caefc64..58a8c9133ba4 100644
> --- a/drivers/mmc/host/renesas_sdhi.h
> +++ b/drivers/mmc/host/renesas_sdhi.h
> @@ -59,6 +59,7 @@ struct renesas_sdhi {
> u32 scc_tappos;
> u32 scc_tappos_hs400;
> bool doing_tune;
> + bool keep_scc_freq;
>
> /* Tuning values: 1 for success, 0 for failure */
> DECLARE_BITMAP(taps, BITS_PER_LONG);
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 15e21894bd44..589a59fb70eb 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -117,8 +117,12 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> unsigned int freq, diff, best_freq = 0, diff_min = ~0;
> int i;
>
> - /* tested only on R-Car Gen2+ currently; may work for others */
> - if (!(host->pdata->flags & TMIO_MMC_MIN_RCAR2))
> + /*
> + * We simply return the current rate if a) we are not on a R-Car Gen2+
> + * SoC (may work for others, but untested) or b) if the SCC needs its
> + * clock during tuning, so we don't change the external clock setup.
> + */
> + if (!(host->pdata->flags & TMIO_MMC_MIN_RCAR2) || priv->keep_scc_freq)
> return clk_get_rate(priv->clk);
>
> /*
> @@ -323,6 +327,9 @@ static void renesas_sdhi_hs400_complete(struct mmc_host *mmc)
> u32 bad_taps = priv->quirks ? priv->quirks->hs400_bad_taps : 0;
> bool use_4tap = priv->quirks && priv->quirks->hs400_4taps;
>
> + /* Tuning done, no special handling for SCC clock needed anymore */
> + priv->keep_scc_freq = false;
> +

Setting keep_scc_freq to false is only here. But, I'm thinking
we should set it in some error paths like below somehow too:
- error paths before hs400_complete() in mmc_select_hs400().
- error path of mmc_execute_tuning() in mmc_retune().

Best regards,
Yoshihiro Shimoda