| Message ID | 20210222113955.7779-3-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: renesas_sdhi: reset via reset controller | expand |
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
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
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
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)
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(-)