[RFC PATCH 0/4] mmc: renesas_sdhi: reset SCC only when available

[RFC PATCH 0/4] mmc: renesas_sdhi: reset SCC only when available

[Wolfram Sang]

While working on another improvement for the reset routine, I noted an
issue with an old Gen2 SDHI instance which did not have a SCC. It turned
out that we never distinguished between the SCC and non-SCC versions on
Gen2 when it came to resetting. So far, it went OK but my upcoming
change broke. So, this series fixes the underlying issue by only
resetting the SCC when one is available.

I made very fine-grained patches because this driver is so fragile.
Nonetheless, my tests on a Renesas Lager board (R-Car H2) and
Salvator-XS (R-Car M3-N) were successful. Debug output showed that
proper code paths were taken and checksumming large files worked as well
as reinserting cards. The patches are based on mmc/next and 5.11 will
do because there seems to be no issue with current kernels.

I'd be very happy about further review and testing!

Thanks and all the best,

   Wolfram


Wolfram Sang (4):
  mmc: renesas_sdhi: only reset SCC when its pointer is populated
  mmc: renesas_sdhi: probe into TMIO after SCC parameters have been
    setup
  mmc: renesas_sdhi: populate SCC pointer at the proper place
  mmc: renesas_sdhi: simplify reset routine a little

 drivers/mmc/host/renesas_sdhi_core.c | 38 +++++++++++++---------------
 1 file changed, 18 insertions(+), 20 deletions(-)

-- 
2.28.0

[RFC PATCH 1/4] mmc: renesas_sdhi: only reset SCC when its pointer is populated

[Wolfram Sang]

Only re-initialize SCC and tuning when an SCC was found during probe().
This is currently a noop because all R-Car Gen2+ are considered to have
an SCC. But this will change in a later patch, so we need this
preparation.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index acb9c81a4e45..a395f835e836 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -556,16 +556,18 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 
-	renesas_sdhi_reset_scc(host, priv);
-	renesas_sdhi_reset_hs400_mode(host, priv);
-	priv->needs_adjust_hs400 = false;
+	if (priv->scc_ctl) {
+		renesas_sdhi_reset_scc(host, priv);
+		renesas_sdhi_reset_hs400_mode(host, priv);
+		priv->needs_adjust_hs400 = false;
 
-	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
-			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
+				sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
 
-	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
-		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
-		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
+			       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+			       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+	}
 
 	if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
 		sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK,
-- 
2.28.0

[RFC PATCH 2/4] mmc: renesas_sdhi: probe into TMIO after SCC parameters have been setup

[Wolfram Sang]

Setting up the SCC parameters does not need a probed TMIO device. But in
the near future, probing the TMIO device needs the SCC parameters setup.
So, fix the ordering.

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

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index a395f835e836..17ebaa324ca1 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -1067,10 +1067,6 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 			quirks->hs400_calib_table + 1);
 	}
 
-	ret = tmio_mmc_host_probe(host);
-	if (ret < 0)
-		goto edisclk;
-
 	/* Enable tuning iff we have an SCC and a supported mode */
 	if (of_data && of_data->scc_offset &&
 	    (host->mmc->caps & MMC_CAP_UHS_SDR104 ||
@@ -1102,6 +1098,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 		host->ops.hs400_complete = renesas_sdhi_hs400_complete;
 	}
 
+	ret = tmio_mmc_host_probe(host);
+	if (ret < 0)
+		goto edisclk;
+
 	num_irqs = platform_irq_count(pdev);
 	if (num_irqs < 0) {
 		ret = num_irqs;
-- 
2.28.0

[RFC PATCH 3/4] mmc: renesas_sdhi: populate SCC pointer at the proper place

[Wolfram Sang]

The SCC pointer is currently filled whenever the SoC is Gen2+. This is
wrong because there is a Gen2-variant without SCC (SDHI_VER_GEN2_SDR50).
We have been lucky because the writes to unintended registers have not
caused problems so far. But further refactoring work exposed the
problem. So, move the pointer initialization to the place where we know
that the SDHI instance supports tuning. And also populate the 'reset'
pointer unconditionally to make sure the interrupt enable register is
always properly set for Gen2+.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 17ebaa324ca1..4a1ed5bf216c 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -1007,11 +1007,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 		host->ops.start_signal_voltage_switch =
 			renesas_sdhi_start_signal_voltage_switch;
 		host->sdcard_irq_setbit_mask = TMIO_STAT_ALWAYS_SET_27;
-
-		if (of_data && of_data->scc_offset) {
-			priv->scc_ctl = host->ctl + of_data->scc_offset;
-			host->reset = renesas_sdhi_reset;
-		}
+		host->reset = renesas_sdhi_reset;
 	}
 
 	/* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */
@@ -1091,6 +1087,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 		if (!hit)
 			dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n");
 
+		priv->scc_ctl = host->ctl + of_data->scc_offset;
 		host->check_retune = renesas_sdhi_check_scc_error;
 		host->ops.execute_tuning = renesas_sdhi_execute_tuning;
 		host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning;
-- 
2.28.0

[RFC PATCH 4/4] mmc: renesas_sdhi: simplify reset routine a little

[Wolfram Sang]

The 'reset' pointer is only populated for Gen2+. So, we don't need to
check for that flag inside the routine.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 4a1ed5bf216c..1fabf8c5773c 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -552,6 +552,7 @@ static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_io
 	return 0;
 }
 
+/* only populated for TMIO_MMC_MIN_RCAR2 */
 static void renesas_sdhi_reset(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
@@ -569,9 +570,7 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
 			       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
 	}
 
-	if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
-		sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK,
-					     TMIO_MASK_INIT_RCAR2);
+	sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK, TMIO_MASK_INIT_RCAR2);
 }
 
 #define SH_MOBILE_SDHI_MIN_TAP_ROW 3
-- 
2.28.0

Re: [RFC PATCH 1/4] mmc: renesas_sdhi: only reset SCC when its pointer is populated

[Niklas Söderlund]

Hi Wolfram,

Thanks for your work.

On 2020-11-10 15:20:55 +0100, Wolfram Sang wrote:
> Only re-initialize SCC and tuning when an SCC was found during probe().
> This is currently a noop because all R-Car Gen2+ are considered to have
> an SCC. But this will change in a later patch, so we need this
> preparation.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index acb9c81a4e45..a395f835e836 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -556,16 +556,18 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
>  
> -	renesas_sdhi_reset_scc(host, priv);
> -	renesas_sdhi_reset_hs400_mode(host, priv);
> -	priv->needs_adjust_hs400 = false;
> +	if (priv->scc_ctl) {
> +		renesas_sdhi_reset_scc(host, priv);
> +		renesas_sdhi_reset_hs400_mode(host, priv);
> +		priv->needs_adjust_hs400 = false;
>  
> -	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
> -			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
> +		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, CLK_CTL_SCLKEN |
> +				sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>  
> -	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
> -		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
> -		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
> +		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
> +			       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
> +			       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
> +	}
>  
>  	if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
>  		sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK,
> -- 
> 2.28.0
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC PATCH 2/4] mmc: renesas_sdhi: probe into TMIO after SCC parameters have been setup

[Niklas Söderlund]

Hi Wolfram,

Thanks for your work.

On 2020-11-10 15:20:56 +0100, Wolfram Sang wrote:
> Setting up the SCC parameters does not need a probed TMIO device. But in
> the near future, probing the TMIO device needs the SCC parameters setup.
> So, fix the ordering.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index a395f835e836..17ebaa324ca1 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -1067,10 +1067,6 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  			quirks->hs400_calib_table + 1);
>  	}
>  
> -	ret = tmio_mmc_host_probe(host);
> -	if (ret < 0)
> -		goto edisclk;
> -
>  	/* Enable tuning iff we have an SCC and a supported mode */
>  	if (of_data && of_data->scc_offset &&
>  	    (host->mmc->caps & MMC_CAP_UHS_SDR104 ||
> @@ -1102,6 +1098,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  		host->ops.hs400_complete = renesas_sdhi_hs400_complete;
>  	}
>  
> +	ret = tmio_mmc_host_probe(host);
> +	if (ret < 0)
> +		goto edisclk;
> +
>  	num_irqs = platform_irq_count(pdev);
>  	if (num_irqs < 0) {
>  		ret = num_irqs;
> -- 
> 2.28.0
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC PATCH 3/4] mmc: renesas_sdhi: populate SCC pointer at the proper place

[Niklas Söderlund]

Hi Wolfram,

Thanks for your patch.

On 2020-11-10 15:20:57 +0100, Wolfram Sang wrote:
> The SCC pointer is currently filled whenever the SoC is Gen2+. This is
> wrong because there is a Gen2-variant without SCC (SDHI_VER_GEN2_SDR50).
> We have been lucky because the writes to unintended registers have not
> caused problems so far. But further refactoring work exposed the
> problem. So, move the pointer initialization to the place where we know
> that the SDHI instance supports tuning. And also populate the 'reset'
> pointer unconditionally to make sure the interrupt enable register is
> always properly set for Gen2+.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 17ebaa324ca1..4a1ed5bf216c 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -1007,11 +1007,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  		host->ops.start_signal_voltage_switch =
>  			renesas_sdhi_start_signal_voltage_switch;
>  		host->sdcard_irq_setbit_mask = TMIO_STAT_ALWAYS_SET_27;
> -
> -		if (of_data && of_data->scc_offset) {
> -			priv->scc_ctl = host->ctl + of_data->scc_offset;
> -			host->reset = renesas_sdhi_reset;
> -		}
> +		host->reset = renesas_sdhi_reset;
>  	}
>  
>  	/* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */
> @@ -1091,6 +1087,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  		if (!hit)
>  			dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n");
>  
> +		priv->scc_ctl = host->ctl + of_data->scc_offset;
>  		host->check_retune = renesas_sdhi_check_scc_error;
>  		host->ops.execute_tuning = renesas_sdhi_execute_tuning;
>  		host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning;
> -- 
> 2.28.0
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC PATCH 4/4] mmc: renesas_sdhi: simplify reset routine a little

[Niklas Söderlund]

Hi Wolfram,

Thanks for your work.

On 2020-11-10 15:20:58 +0100, Wolfram Sang wrote:
> The 'reset' pointer is only populated for Gen2+. So, we don't need to
> check for that flag inside the routine.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 4a1ed5bf216c..1fabf8c5773c 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -552,6 +552,7 @@ static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_io
>  	return 0;
>  }
>  
> +/* only populated for TMIO_MMC_MIN_RCAR2 */
>  static void renesas_sdhi_reset(struct tmio_mmc_host *host)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
> @@ -569,9 +570,7 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
>  			       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
>  	}
>  
> -	if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
> -		sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK,
> -					     TMIO_MASK_INIT_RCAR2);
> +	sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK, TMIO_MASK_INIT_RCAR2);
>  }
>  
>  #define SH_MOBILE_SDHI_MIN_TAP_ROW 3
> -- 
> 2.28.0
> 

-- 
Regards,
Niklas Söderlund

RE: [RFC PATCH 0/4] mmc: renesas_sdhi: reset SCC only when available

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Tuesday, November 10, 2020 11:21 PM
> 
> While working on another improvement for the reset routine, I noted an
> issue with an old Gen2 SDHI instance which did not have a SCC. It turned
> out that we never distinguished between the SCC and non-SCC versions on
> Gen2 when it came to resetting. So far, it went OK but my upcoming
> change broke. So, this series fixes the underlying issue by only
> resetting the SCC when one is available.
> 
> I made very fine-grained patches because this driver is so fragile.
> Nonetheless, my tests on a Renesas Lager board (R-Car H2) and
> Salvator-XS (R-Car M3-N) were successful. Debug output showed that
> proper code paths were taken and checksumming large files worked as well
> as reinserting cards. The patches are based on mmc/next and 5.11 will
> do because there seems to be no issue with current kernels.
> 
> I'd be very happy about further review and testing!

Thank you for the patch!

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

And, I tested on Salvator-XS (R-Car H3 and R-Car M3-W+). So,

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

Best regards,
Yoshihiro Shimoda

Re: [RFC PATCH 0/4] mmc: renesas_sdhi: reset SCC only when available

[Ulf Hansson]

On Tue, 10 Nov 2020 at 15:21, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> While working on another improvement for the reset routine, I noted an
> issue with an old Gen2 SDHI instance which did not have a SCC. It turned
> out that we never distinguished between the SCC and non-SCC versions on
> Gen2 when it came to resetting. So far, it went OK but my upcoming
> change broke. So, this series fixes the underlying issue by only
> resetting the SCC when one is available.
>
> I made very fine-grained patches because this driver is so fragile.
> Nonetheless, my tests on a Renesas Lager board (R-Car H2) and
> Salvator-XS (R-Car M3-N) were successful. Debug output showed that
> proper code paths were taken and checksumming large files worked as well
> as reinserting cards. The patches are based on mmc/next and 5.11 will
> do because there seems to be no issue with current kernels.
>
> I'd be very happy about further review and testing!
>
> Thanks and all the best,
>
>    Wolfram
>
>
> Wolfram Sang (4):
>   mmc: renesas_sdhi: only reset SCC when its pointer is populated
>   mmc: renesas_sdhi: probe into TMIO after SCC parameters have been
>     setup
>   mmc: renesas_sdhi: populate SCC pointer at the proper place
>   mmc: renesas_sdhi: simplify reset routine a little
>
>  drivers/mmc/host/renesas_sdhi_core.c | 38 +++++++++++++---------------
>  1 file changed, 18 insertions(+), 20 deletions(-)

Applied for next, thanks!

Kind regards
Uffe