| Message ID | 20210309092332.30705-3-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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.
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
> 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.
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
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)
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(-)