[PATCH v2 0/4] renesas_sdhi: fix hang when SCC loses its clock

[PATCH v2 0/4] renesas_sdhi: fix hang when SCC loses its clock

[Wolfram Sang]

This again took a while since v1 because the issue was so hard to
trigger. But I finally found a way to inject the flaw, so this series
could be tested and it fixes the issue.

Changes since v1:
	* introduce a new flag to MMC core indicating any kind of tuning
	  not only retune
	* use the new flag to keep SCC flag active
	* new patch 4, minor cleanup to MMC core

A branch including the DEBUG patch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib-for-5.10

If you revert patch 3, you should have the SCC hang during boot again.
For the record, let me copy some findings I mentioned in another thread:

===
Interesting news: The hang comes from a code path I would have not
expected. It is not because of accessing an SCC register, it is this
line from renesas_sdhi_set_clock() which causes the issue:

186         sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & CLK_CTL_DIV_MASK);

I mean I can guess that the clock setting has something to do with the
SCC, but I can't see the direct connection with the documentation I
have.
===

Tested on R-Car H3 ES2.0 and M3-N and patches based on mmc/next.

Another hope this is gone for good now...

Kind regards,

   Wolfram


Wolfram Sang (4):
  mmc: core: when downgrading HS400, callback into drivers earlier
  mmc: core: add a 'doing_init_tune' flag and a 'mmc_doing_tune' helper
  mmc: renesas_sdhi: keep SCC clock active when tuning
  mmc: core: simplify an expression

 drivers/mmc/core/mmc.c               | 16 ++++++++++------
 drivers/mmc/host/renesas_sdhi_core.c |  8 ++++++--
 include/linux/mmc/host.h             |  6 ++++++
 3 files changed, 22 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] mmc: core: when downgrading HS400, callback into drivers earlier

[Wolfram Sang]

The driver specific downgrade function makes more sense if we run it
before we set the timing to something lower, not after. Otherwise some
non-HS400 communication has already happened.

No need to convert users. There is only one currently which needs this
change in a following patch.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/mmc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index b3fa193de846..ba2852b684b1 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1168,13 +1168,13 @@ static int mmc_select_hs400(struct mmc_card *card)
 		return err;
 	}
 
-	/* Set host controller to HS timing */
-	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-
 	/* Prepare host to downgrade to HS timing */
 	if (host->ops->hs400_downgrade)
 		host->ops->hs400_downgrade(host);
 
+	/* Set host controller to HS timing */
+	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
+
 	/* Reduce frequency to HS frequency */
 	max_dtr = card->ext_csd.hs_max_dtr;
 	mmc_set_clock(host, max_dtr);
@@ -1253,6 +1253,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	if (err)
 		goto out_err;
 
+	if (host->ops->hs400_downgrade)
+		host->ops->hs400_downgrade(host);
+
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
 	err = mmc_switch_status(card, true);
@@ -1268,9 +1271,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 
 	mmc_set_timing(host, MMC_TIMING_MMC_HS);
 
-	if (host->ops->hs400_downgrade)
-		host->ops->hs400_downgrade(host);
-
 	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
-- 
2.20.1

[PATCH v2 2/4] mmc: core: add a 'doing_init_tune' flag and a 'mmc_doing_tune' helper

[Wolfram Sang]

Our driver needs to know when tuning is in progress. 'doing_retune' only
covers re-tuning, not the initial tuning. Add another flag to detect the
initial tuning state and add a helper which tells us if any kind of
tuning is going on. Only implemented for MMC currently because that's
where we need it. SD can be added later if it becomes necessary.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/mmc.c   | 4 ++++
 include/linux/mmc/host.h | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index ba2852b684b1..216bd1aed373 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1763,6 +1763,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 		goto free_card;
 
 	if (mmc_card_hs200(card)) {
+		host->doing_init_tune = 1;
+
 		err = mmc_hs200_tuning(card);
 		if (err)
 			goto free_card;
@@ -1770,6 +1772,8 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 		err = mmc_select_hs400(card);
 		if (err)
 			goto free_card;
+
+		host->doing_init_tune = 0;
 	} else if (!mmc_card_hs400es(card)) {
 		/* Select the desired bus width optionally */
 		err = mmc_select_bus_width(card);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 799e23b0a23c..c079b932330f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -400,6 +400,7 @@ struct mmc_host {
 	unsigned int		use_spi_crc:1;
 	unsigned int		claimed:1;	/* host exclusively claimed */
 	unsigned int		bus_dead:1;	/* bus has been released */
+	unsigned int		doing_init_tune:1; /* initial tuning in progress */
 	unsigned int		can_retune:1;	/* re-tuning can be used */
 	unsigned int		doing_retune:1;	/* re-tuning in progress */
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
@@ -595,6 +596,11 @@ static inline bool mmc_doing_retune(struct mmc_host *host)
 	return host->doing_retune == 1;
 }
 
+static inline bool mmc_doing_tune(struct mmc_host *host)
+{
+	return host->doing_retune == 1 || host->doing_init_tune == 1;
+}
+
 static inline enum dma_data_direction mmc_get_dma_dir(struct mmc_data *data)
 {
 	return data->flags & MMC_DATA_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
-- 
2.20.1

[PATCH v2 3/4] mmc: renesas_sdhi: keep SCC clock active when tuning

[Wolfram Sang]

Tuning procedure switches to lower frequencies but that will turn the
SCC off and accessing its register then will hang. So, check 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_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 29148fa25d82..c5cba0a1b112 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) || mmc_doing_tune(host->mmc))
 		return clk_get_rate(priv->clk);
 
 	/*
-- 
2.20.1

[PATCH v2 4/4] mmc: core: simplify an expression

[Wolfram Sang]

We already have 'host' as a variable, so use it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 216bd1aed373..67e95eba0e82 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1173,7 +1173,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 		host->ops->hs400_downgrade(host);
 
 	/* Set host controller to HS timing */
-	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
+	mmc_set_timing(host, MMC_TIMING_MMC_HS);
 
 	/* Reduce frequency to HS frequency */
 	max_dtr = card->ext_csd.hs_max_dtr;
-- 
2.20.1

RE: [PATCH v2 0/4] renesas_sdhi: fix hang when SCC loses its clock

[Yoshihiro Shimoda]

Hello Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, September 2, 2020 12:03 AM
> 
> This again took a while since v1 because the issue was so hard to
> trigger. But I finally found a way to inject the flaw, so this series
> could be tested and it fixes the issue.
> 
> Changes since v1:
> 	* introduce a new flag to MMC core indicating any kind of tuning
> 	  not only retune
> 	* use the new flag to keep SCC flag active
> 	* new patch 4, minor cleanup to MMC core
> 
> A branch including the DEBUG patch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib-for-5.10
> 
> If you revert patch 3, you should have the SCC hang during boot again.
<snip>
> 
> Tested on R-Car H3 ES2.0 and M3-N and patches based on mmc/next.

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

And, I tested this series on R-Car H3 ES3.0 and M3-W+. And,
I confirmed this series could fix the SCC hang issue. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 0/4] renesas_sdhi: fix hang when SCC loses its clock

[Ulf Hansson]

On Tue, 1 Sep 2020 at 17:03, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> This again took a while since v1 because the issue was so hard to
> trigger. But I finally found a way to inject the flaw, so this series
> could be tested and it fixes the issue.
>
> Changes since v1:
>         * introduce a new flag to MMC core indicating any kind of tuning
>           not only retune
>         * use the new flag to keep SCC flag active
>         * new patch 4, minor cleanup to MMC core
>
> A branch including the DEBUG patch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib-for-5.10
>
> If you revert patch 3, you should have the SCC hang during boot again.
> For the record, let me copy some findings I mentioned in another thread:
>
> ===
> Interesting news: The hang comes from a code path I would have not
> expected. It is not because of accessing an SCC register, it is this
> line from renesas_sdhi_set_clock() which causes the issue:
>
> 186         sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk & CLK_CTL_DIV_MASK);
>
> I mean I can guess that the clock setting has something to do with the
> SCC, but I can't see the direct connection with the documentation I
> have.
> ===
>
> Tested on R-Car H3 ES2.0 and M3-N and patches based on mmc/next.
>
> Another hope this is gone for good now...
>
> Kind regards,
>
>    Wolfram
>
>
> Wolfram Sang (4):
>   mmc: core: when downgrading HS400, callback into drivers earlier
>   mmc: core: add a 'doing_init_tune' flag and a 'mmc_doing_tune' helper
>   mmc: renesas_sdhi: keep SCC clock active when tuning
>   mmc: core: simplify an expression
>
>  drivers/mmc/core/mmc.c               | 16 ++++++++++------
>  drivers/mmc/host/renesas_sdhi_core.c |  8 ++++++--
>  include/linux/mmc/host.h             |  6 ++++++
>  3 files changed, 22 insertions(+), 8 deletions(-)
>
> --
> 2.20.1
>

Applied for next, thanks!

Kind regards
Uffe