[PATCH 0/2] mmc: renesas_sdhi: reset via reset controller

[PATCH 0/2] mmc: renesas_sdhi: reset via reset controller

[Wolfram Sang]

This series enables SDHI to be reset via a reset controller.

Please let me know about your (and BSP team's) thoughts.

Changes since RFC v2 (thanks Geert for the review!):

* use devm_reset_control_get_optional_exclusive()
* select RESET_CONTROLLER
* given reason in commit message of patch 2

Tested on a Salvator-XS with a Renesas R-Car M3-N.

Shimoda-san: I think we need to get this series upstream first. On top
of this, I will work on the better irq_mask handling.


Wolfram Sang (2):
  mmc: tmio: abort DMA before reset
  mmc: renesas_sdhi: do hard reset if possible

 drivers/mmc/host/Kconfig             |  1 +
 drivers/mmc/host/renesas_sdhi.h      |  2 ++
 drivers/mmc/host/renesas_sdhi_core.c | 15 ++++++++++++++-
 drivers/mmc/host/tmio_mmc_core.c     |  4 ++--
 4 files changed, 19 insertions(+), 3 deletions(-)

-- 
2.30.0

[PATCH 1/2] mmc: tmio: abort DMA before reset

[Wolfram Sang]

We will soon allow resetting the whole IP core via a reset controller.
For this case, DMA must be terminated before the actual reset. For the
other cases, it is probably better, too.

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

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 5aa57640d0e6..eca767dcabba 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -172,11 +172,11 @@ static void tmio_mmc_reset(struct tmio_mmc_host *host)
 	sd_ctrl_write16(host, CTL_RESET_SD, 0x0001);
 	usleep_range(10000, 11000);
 
+	tmio_mmc_abort_dma(host);
+
 	if (host->reset)
 		host->reset(host);
 
-	tmio_mmc_abort_dma(host);
-
 	if (host->pdata->flags & TMIO_MMC_SDIO_IRQ) {
 		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
-- 
2.30.0

[PATCH 2/2] mmc: renesas_sdhi: do hard reset if possible

[Wolfram Sang]

All recent SDHI instances can be reset via the reset controller. If one
is found, use it instead of the open coded reset. This is to get a
future-proof sane reset state.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/Kconfig             |  1 +
 drivers/mmc/host/renesas_sdhi.h      |  2 ++
 drivers/mmc/host/renesas_sdhi_core.c | 15 ++++++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index b236dfe2e879..1f1b691f10ce 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -707,6 +707,7 @@ config MMC_SDHI
 	tristate "Renesas SDHI SD/SDIO controller support"
 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
 	select MMC_TMIO_CORE
+	select RESET_CONTROLLER
 	help
 	  This provides support for the SDHI SD/SDIO controller found in
 	  Renesas SuperH, ARM and ARM64 based SoCs
diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index cb962c7883dc..53eded81a53e 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -70,6 +70,8 @@ struct renesas_sdhi {
 	DECLARE_BITMAP(smpcmp, BITS_PER_LONG);
 	unsigned int tap_num;
 	unsigned int tap_set;
+
+	struct reset_control *rstc;
 };
 
 #define host_to_priv(host) \
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 09a5e0dafbef..473f155f6d3d 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -20,6 +20,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mfd/tmio.h>
 #include <linux/mmc/host.h>
@@ -32,6 +33,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/sh_dma.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
@@ -561,9 +563,16 @@ static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_io
 static void renesas_sdhi_reset(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
+	int ret;
 	u16 val;
 
-	if (priv->scc_ctl) {
+	if (priv->rstc) {
+		reset_control_reset(priv->rstc);
+		/* Unknown why but without polling reset status, it will hang */
+		read_poll_timeout(reset_control_status, ret, ret == 0, 1, 100,
+				  false, priv->rstc);
+		priv->needs_adjust_hs400 = false;
+	} else if (priv->scc_ctl) {
 		renesas_sdhi_disable_scc(host->mmc);
 		renesas_sdhi_reset_hs400_mode(host, priv);
 		priv->needs_adjust_hs400 = false;
@@ -1077,6 +1086,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	if (ret)
 		goto efree;
 
+	priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(priv->rstc))
+		return PTR_ERR(priv->rstc);
+
 	ver = sd_ctrl_read16(host, CTL_VERSION);
 	/* GEN2_SDR104 is first known SDHI to use 32bit block count */
 	if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX)
-- 
2.30.0

Re: [PATCH 0/2] mmc: renesas_sdhi: reset via reset controller

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Tue, Mar 09, 2021 at 10:23:30AM +0100, Wolfram Sang wrote:
> This series enables SDHI to be reset via a reset controller.
> 
> Please let me know about your (and BSP team's) thoughts.
> 
> Changes since RFC v2 (thanks Geert for the review!):
> 
> * use devm_reset_control_get_optional_exclusive()
> * select RESET_CONTROLLER
> * given reason in commit message of patch 2
> 
> Tested on a Salvator-XS with a Renesas R-Car M3-N.

Patches are on top of mmc/next.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] mmc: tmio: abort DMA before reset

[Niklas Söderlund]

Hi Wolfram,

Thanks for your patch.

On 2021-03-09 10:23:31 +0100, Wolfram Sang wrote:
> We will soon allow resetting the whole IP core via a reset controller.
> For this case, DMA must be terminated before the actual reset. For the
> other cases, it is probably better, too.

I agree I think it makes more sens to abort dma before reseting the 
device.

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

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

> ---
>  drivers/mmc/host/tmio_mmc_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 5aa57640d0e6..eca767dcabba 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -172,11 +172,11 @@ static void tmio_mmc_reset(struct tmio_mmc_host *host)
>  	sd_ctrl_write16(host, CTL_RESET_SD, 0x0001);
>  	usleep_range(10000, 11000);
>  
> +	tmio_mmc_abort_dma(host);
> +
>  	if (host->reset)
>  		host->reset(host);
>  
> -	tmio_mmc_abort_dma(host);
> -
>  	if (host->pdata->flags & TMIO_MMC_SDIO_IRQ) {
>  		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
>  		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
> -- 
> 2.30.0
> 

-- 
Regards,
Niklas Söderlund

RE: [PATCH 2/2] mmc: renesas_sdhi: do hard reset if possible

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Tuesday, March 9, 2021 6:24 PM
> 
> All recent SDHI instances can be reset via the reset controller. If one
> is found, use it instead of the open coded reset. This is to get a
> future-proof sane reset state.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
<snip>
> @@ -561,9 +563,16 @@ static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_io
>  static void renesas_sdhi_reset(struct tmio_mmc_host *host)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
> +	int ret;
>  	u16 val;
> 
> -	if (priv->scc_ctl) {
> +	if (priv->rstc) {
> +		reset_control_reset(priv->rstc);
> +		/* Unknown why but without polling reset status, it will hang */
> +		read_poll_timeout(reset_control_status, ret, ret == 0, 1, 100,
> +				  false, priv->rstc);
> +		priv->needs_adjust_hs400 = false;

After we did hard reset here, sometimes tmio_mmc_reset_work() cannot recover
again with "mmcblk0: recovery failed!" message... So, I investigated this
issue and I found a reason.

> +	} else if (priv->scc_ctl) {
>  		renesas_sdhi_disable_scc(host->mmc);

I realized this renesas_sdhi_disable_scc() will set CLK_CTL_SCLKEN.
So, the previous code can issue CMD13 after tmio_mmc_reset_work() was called.
But, after we applied this patch, the CMD13 failed because the clock was disabled.
# In other words, if a controller doesn't have scc, the previous code cannot issue
# CMD13 in such a case, I guess.

So, before we apply this patch, we have to add ->set_clock() calling in
tmio_mmc_reset_work() like below:
---
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index eca767dcabba..a05ccfc7aa0d 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -222,6 +222,7 @@ static void tmio_mmc_reset_work(struct work_struct *work)
 	spin_unlock_irqrestore(&host->lock, flags);
 
 	tmio_mmc_reset(host);
+	host->set_clock(host, host->clk_cache);
 
 	/* Ready for new calls */
 	host->mrq = NULL;
---

Best regards,
Yoshihiro Shimoda

Re: [PATCH 2/2] mmc: renesas_sdhi: do hard reset if possible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

Hi Shimoda-san,

> # In other words, if a controller doesn't have scc, the previous code cannot issue
> # CMD13 in such a case, I guess.

Makes sense.

>  	tmio_mmc_reset(host);
> +	host->set_clock(host, host->clk_cache);

What about putting it into the reset function itself, so it will be
always enabled (like for the scc_ctl case)?

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 473f155f6d3d..672953e3362d 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -572,6 +572,7 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
 		read_poll_timeout(reset_control_status, ret, ret == 0, 1, 100,
 				  false, priv->rstc);
 		priv->needs_adjust_hs400 = false;
+		renesas_sdhi_set_clock(host, host->clk_cache);
 	} else if (priv->scc_ctl) {
 		renesas_sdhi_disable_scc(host->mmc);
 		renesas_sdhi_reset_hs400_mode(host, priv);

If you agree, I will fold this into v2 of this series.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] mmc: renesas_sdhi: do hard reset if possible

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Thursday, March 11, 2021 10:18 PM
> >  	tmio_mmc_reset(host);
> > +	host->set_clock(host, host->clk_cache);
> 
> What about putting it into the reset function itself, so it will be
> always enabled (like for the scc_ctl case)?
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 473f155f6d3d..672953e3362d 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -572,6 +572,7 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host)
>  		read_poll_timeout(reset_control_status, ret, ret == 0, 1, 100,
>  				  false, priv->rstc);
>  		priv->needs_adjust_hs400 = false;
> +		renesas_sdhi_set_clock(host, host->clk_cache);
>  	} else if (priv->scc_ctl) {
>  		renesas_sdhi_disable_scc(host->mmc);
>  		renesas_sdhi_reset_hs400_mode(host, priv);
> 
> If you agree, I will fold this into v2 of this series.

Adding the function itself seems OK. However, I checked the code, and then
adding hard reset into renesas_sdhi_reset() seems to break the following:
-----
commit 5b0739d76227fd5a3f02f014385bfa9c86e0404b
Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date:   Thu Aug 20 15:25:37 2020 +0200

    mmc: tmio: don't reset whole IP core when tuning fails

    SDHI needs to reset the SCC only, not the whole IP core. So, if tuning
    fails, don't handle specifics in the generic TMIO core, but in the
    specific drivers. For SDHI, we need to move around the reset routine a
    bit. It is not modified.
------

So, perhaps, we have to fix renesas_sdhi_execute_tuning() somehow before
adding hard reset. But, what do you think?

Best regards,
Yoshihiro Shimoda

Re: [PATCH 2/2] mmc: renesas_sdhi: do hard reset if possible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]


> Adding the function itself seems OK. However, I checked the code, and then
> adding hard reset into renesas_sdhi_reset() seems to break the following:
> -----
> commit 5b0739d76227fd5a3f02f014385bfa9c86e0404b
> Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date:   Thu Aug 20 15:25:37 2020 +0200
> 
>     mmc: tmio: don't reset whole IP core when tuning fails
> 
>     SDHI needs to reset the SCC only, not the whole IP core. So, if tuning
>     fails, don't handle specifics in the generic TMIO core, but in the
>     specific drivers. For SDHI, we need to move around the reset routine a
>     bit. It is not modified.
> ------
> 
> So, perhaps, we have to fix renesas_sdhi_execute_tuning() somehow before
> adding hard reset. But, what do you think?

Thanks for the pointer, Shimoda-san! It seems it gets messy again, so
time for getting clearer, I think. I will introduce a reset_scc funtion
again which is seperate from reset_sdhi, so we have a clear distinction.
Then, I will call reset_scc from renesas_sdhi_execute_tuning() and the
rest stays with reset_sdhi. I have a prototype patch which I will test
some more and  hopefully send out later.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] mmc: renesas_sdhi: do hard reset if possible

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Monday, March 15, 2021 9:31 PM
> 
> > Adding the function itself seems OK. However, I checked the code, and then
> > adding hard reset into renesas_sdhi_reset() seems to break the following:
> > -----
> > commit 5b0739d76227fd5a3f02f014385bfa9c86e0404b
> > Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Date:   Thu Aug 20 15:25:37 2020 +0200
> >
> >     mmc: tmio: don't reset whole IP core when tuning fails
> >
> >     SDHI needs to reset the SCC only, not the whole IP core. So, if tuning
> >     fails, don't handle specifics in the generic TMIO core, but in the
> >     specific drivers. For SDHI, we need to move around the reset routine a
> >     bit. It is not modified.
> > ------
> >
> > So, perhaps, we have to fix renesas_sdhi_execute_tuning() somehow before
> > adding hard reset. But, what do you think?
> 
> Thanks for the pointer, Shimoda-san! It seems it gets messy again, so
> time for getting clearer, I think. I will introduce a reset_scc funtion
> again which is seperate from reset_sdhi, so we have a clear distinction.
> Then, I will call reset_scc from renesas_sdhi_execute_tuning() and the
> rest stays with reset_sdhi. I have a prototype patch which I will test
> some more and  hopefully send out later.

It's a nice idea! And, thank you for submitting a new patch series.
I'll test it later.

Best regards,
Yoshihiro Shimoda