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

[RFC PATCH v2 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 (internal) RFC v1:

* moved from probe() to the reset() callback so we use it for every
  reset, not only when initializing
* patch 1 is new to make sure aborting DMA will work
* added a comment about (unexpected) polling for the reset status
* don't use atomic version for polling; reset is not a hot path

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

 drivers/mmc/host/renesas_sdhi.h      |  2 ++
 drivers/mmc/host/renesas_sdhi_core.c | 13 ++++++++++++-
 drivers/mmc/host/tmio_mmc_core.c     |  4 ++--
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.30.0

[RFC PATCH v2 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 2478a91e84b2..0e7a2faa5238 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

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

[Wolfram Sang]

Some SDHI instances can be reset via the reset controller. If one is
found, use it instead of the custom reset.

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

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 158c21e5a942..a1de5c431f07 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 (!IS_ERR(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;
@@ -1076,6 +1085,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	if (ret)
 		goto efree;
 
+	priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+
 	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: [RFC PATCH v2 2/2] mmc: renesas_sdhi: do hard reset if possible

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Feb 22, 2021 at 12:41 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Some SDHI instances can be reset via the reset controller. If one is
> found, use it instead of the custom reset.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c

> @@ -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 (!IS_ERR(priv->rstc)) {

"if (priv->rstc)" if the reset is made optional.

> +               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;
> @@ -1076,6 +1085,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>         if (ret)
>                 goto efree;
>
> +       priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);

devm_reset_control_get_optional_exclusive()?
+ missing error handling (real errors and -EPROBE_DEFER).

Perhaps you want to add a "select RESET_CONTROLLER" to "config
MMC_SDHI"?

> +
>         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)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Wolfram Sang]

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

Hi Geert,

> "if (priv->rstc)" if the reset is made optional.

Yes, that would be better.

> > +       priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> 
> devm_reset_control_get_optional_exclusive()?
> + missing error handling (real errors and -EPROBE_DEFER).

OK.

> Perhaps you want to add a "select RESET_CONTROLLER" to "config
> MMC_SDHI"?

Isn't "select" too strong for an optional feature? I'd think so.

Thanks,

   Wolfram

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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Feb 23, 2021 at 11:17 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > Perhaps you want to add a "select RESET_CONTROLLER" to "config
> > MMC_SDHI"?
>
> Isn't "select" too strong for an optional feature? I'd think so.

It depends.  Why would you want to use the reset controller instead
of the custom reset in the first place?

Hmm, reminds me your patch doesn't explain the "why" part ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds