diff mbox series

mmc: renesas_sdhi: fix hang up in HS400 timing mode selection

Message ID 20190917225023.6035-1-wsa+renesas@sang-engineering.com (mailing list archive)
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series mmc: renesas_sdhi: fix hang up in HS400 timing mode selection | expand

Commit Message

Wolfram Sang Sept. 17, 2019, 10:50 p.m. UTC 
From: Takeshi Saito <takeshi.saito.xv@renesas.com>

In HS400 timing mode selection, SD clock is switched like:

1) HS200 (200MHz) for tuning
2) High Speed (<= 52MHz) for select HS400 mode (card)
3) HS400 (200MHz)

The SDHI controller needs its internal SCC component for HS400 and other
modes which need tuning. However, SCC gets only fed a clock when the
module clk is > 100MHz. Make sure the SCC is always active with tuning
by enforcing at least 100MHz. Note that we only change the module clock.
An internal divider ensures that we will still talk to the card at
52MHz.

Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
[wsa: don't overwrite 'new_freq', use 'mmc_doing_retune', improve docs]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Shimoda-san: can you forward this patch to the BSP team to have a look,
too? I needed to change their version of checking various MMC_TIMING_*
constants because this approach did not work with current mainline for
me. After some testing and researching, I think the solution with
'mmc_doing_retune' is not only working again, but also more future
proof, in general.

 drivers/mmc/host/renesas_sdhi.h               | 2 ++
 drivers/mmc/host/renesas_sdhi_core.c          | 8 +++++++-
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Sept. 18, 2019, 5:54 a.m. UTC | #1 
On Wed, 18 Sep 2019 at 00:50, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> From: Takeshi Saito <takeshi.saito.xv@renesas.com>
>
> In HS400 timing mode selection, SD clock is switched like:
>
> 1) HS200 (200MHz) for tuning
> 2) High Speed (<= 52MHz) for select HS400 mode (card)
> 3) HS400 (200MHz)
>
> The SDHI controller needs its internal SCC component for HS400 and other
> modes which need tuning. However, SCC gets only fed a clock when the
> module clk is > 100MHz. Make sure the SCC is always active with tuning
> by enforcing at least 100MHz. Note that we only change the module clock.
> An internal divider ensures that we will still talk to the card at
> 52MHz.
>
> Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
> [wsa: don't overwrite 'new_freq', use 'mmc_doing_retune', improve docs]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Assuming you want this for stable as well, but perhaps we can also
find a commit to add a fixes tag? Do you have any suggestions for a
commit?

Kind regards
Uffe

> ---
>
> Shimoda-san: can you forward this patch to the BSP team to have a look,
> too? I needed to change their version of checking various MMC_TIMING_*
> constants because this approach did not work with current mainline for
> me. After some testing and researching, I think the solution with
> 'mmc_doing_retune' is not only working again, but also more future
> proof, in general.
>
>  drivers/mmc/host/renesas_sdhi.h               | 2 ++
>  drivers/mmc/host/renesas_sdhi_core.c          | 8 +++++++-
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
> index c0504aa90857..33a1acc67cb4 100644
> --- a/drivers/mmc/host/renesas_sdhi.h
> +++ b/drivers/mmc/host/renesas_sdhi.h
> @@ -27,6 +27,7 @@ struct renesas_sdhi_of_data {
>         dma_addr_t dma_rx_offset;
>         unsigned int bus_shift;
>         int scc_offset;
> +       unsigned int scc_base_f_min;
>         struct renesas_sdhi_scc *taps;
>         int taps_num;
>         unsigned int max_blk_count;
> @@ -49,6 +50,7 @@ struct renesas_sdhi {
>         struct pinctrl *pinctrl;
>         struct pinctrl_state *pins_default, *pins_uhs;
>         void __iomem *scc_ctl;
> +       unsigned int scc_base_f_min;
>         u32 scc_tappos;
>         u32 scc_tappos_hs400;
>  };
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 4a2872f49a60..82a492567016 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -120,16 +120,21 @@ static int renesas_sdhi_clk_enable(struct tmio_mmc_host *host)
>  }
>
>  static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> -                                           unsigned int new_clock)
> +                                           unsigned int req_clock)
>  {
>         struct renesas_sdhi *priv = host_to_priv(host);
>         unsigned int freq, diff, best_freq = 0, diff_min = ~0;
> +       unsigned int new_clock = req_clock;
>         int i, ret;
>
>         /* tested only on R-Car Gen2+ currently; may work for others */
>         if (!(host->pdata->flags & TMIO_MMC_MIN_RCAR2))
>                 return clk_get_rate(priv->clk);
>
> +       /* When SCC is needed, make sure it gets a proper clock */
> +       if (mmc_doing_retune(host->mmc) && new_clock < priv->scc_base_f_min)
> +               new_clock = priv->scc_base_f_min;
> +
>         /*
>          * We want the bus clock to be as close as possible to, but no
>          * greater than, new_clock.  As we can divide by 1 << i for
> @@ -709,6 +714,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>                 mmc_data->max_segs = of_data->max_segs;
>                 dma_priv->dma_buswidth = of_data->dma_buswidth;
>                 host->bus_shift = of_data->bus_shift;
> +               priv->scc_base_f_min = of_data->scc_base_f_min;
>         }
>
>         host->write16_hook      = renesas_sdhi_write16_hook;
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 751fe91c7571..7010c524b180 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -109,6 +109,8 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
>         .capabilities2  = MMC_CAP2_NO_WRITE_PROTECT,
>         .bus_shift      = 2,
>         .scc_offset     = 0x1000,
> +       /* SCC module clock (SDnH) is enabled at 100MHz or more */
> +       .scc_base_f_min = 100000000,
>         .taps           = rcar_gen3_scc_taps,
>         .taps_num       = ARRAY_SIZE(rcar_gen3_scc_taps),
>         /* DMAC can handle 32bit blk count but only 1 segment */
> --
> 2.20.1
>
Yoshihiro Shimoda Sept. 18, 2019, 8:15 a.m. UTC | #2 
Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, September 18, 2019 7:50 AM
<snip>
> 
> In HS400 timing mode selection, SD clock is switched like:
> 
> 1) HS200 (200MHz) for tuning
> 2) High Speed (<= 52MHz) for select HS400 mode (card)
> 3) HS400 (200MHz)
> 
> The SDHI controller needs its internal SCC component for HS400 and other
> modes which need tuning. However, SCC gets only fed a clock when the
> module clk is > 100MHz. Make sure the SCC is always active with tuning
> by enforcing at least 100MHz. Note that we only change the module clock.
> An internal divider ensures that we will still talk to the card at
> 52MHz.
> 
> Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
> [wsa: don't overwrite 'new_freq', use 'mmc_doing_retune', improve docs]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Shimoda-san: can you forward this patch to the BSP team to have a look,
> too? I needed to change their version of checking various MMC_TIMING_*
> constants because this approach did not work with current mainline for
> me. After some testing and researching, I think the solution with
> 'mmc_doing_retune' is not only working again, but also more future
> proof, in general.

I asked the BSP team about this topic, and then they have a concern about
the retune. Since they would like to check whether the software is
doing tune or not, just tuning situation is lacking on this patch.
So, if MMC subsystem has such a new flag as "doing_tune" in struct mmc_host,
it's helpful for it. (Also, perhaps it's helpful for adding extra quirks
on this driver in the future). What do you think?

Best regards,
Yoshihiro Shimoda
Wolfram Sang Sept. 18, 2019, 8:22 a.m. UTC | #3 
> I asked the BSP team about this topic, and then they have a concern about
> the retune. Since they would like to check whether the software is
> doing tune or not, just tuning situation is lacking on this patch.
> So, if MMC subsystem has such a new flag as "doing_tune" in struct mmc_host,
> it's helpful for it. (Also, perhaps it's helpful for adding extra quirks
> on this driver in the future). What do you think?

I understand the concern. I will check this.
Wolfram Sang Oct. 8, 2019, 4:46 p.m. UTC | #4 
On Wed, Sep 18, 2019 at 10:22:10AM +0200, Wolfram Sang wrote:
> 
> > I asked the BSP team about this topic, and then they have a concern about
> > the retune. Since they would like to check whether the software is
> > doing tune or not, just tuning situation is lacking on this patch.
> > So, if MMC subsystem has such a new flag as "doing_tune" in struct mmc_host,
> > it's helpful for it. (Also, perhaps it's helpful for adding extra quirks
> > on this driver in the future). What do you think?
> 
> I understand the concern. I will check this.

For the record, I am still working on this. I am currently researching
if the hs400_downgrade() function has some relevance to it. However,
I wasn't able to trigger the fault condition the last days. Will keep
trying...
diff mbox series

Patch

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index c0504aa90857..33a1acc67cb4 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -27,6 +27,7 @@  struct renesas_sdhi_of_data {
 	dma_addr_t dma_rx_offset;
 	unsigned int bus_shift;
 	int scc_offset;
+	unsigned int scc_base_f_min;
 	struct renesas_sdhi_scc *taps;
 	int taps_num;
 	unsigned int max_blk_count;
@@ -49,6 +50,7 @@  struct renesas_sdhi {
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pins_default, *pins_uhs;
 	void __iomem *scc_ctl;
+	unsigned int scc_base_f_min;
 	u32 scc_tappos;
 	u32 scc_tappos_hs400;
 };
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 4a2872f49a60..82a492567016 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -120,16 +120,21 @@  static int renesas_sdhi_clk_enable(struct tmio_mmc_host *host)
 }
 
 static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
-					    unsigned int new_clock)
+					    unsigned int req_clock)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 	unsigned int freq, diff, best_freq = 0, diff_min = ~0;
+	unsigned int new_clock = req_clock;
 	int i, ret;
 
 	/* tested only on R-Car Gen2+ currently; may work for others */
 	if (!(host->pdata->flags & TMIO_MMC_MIN_RCAR2))
 		return clk_get_rate(priv->clk);
 
+	/* When SCC is needed, make sure it gets a proper clock */
+	if (mmc_doing_retune(host->mmc) && new_clock < priv->scc_base_f_min)
+		new_clock = priv->scc_base_f_min;
+
 	/*
 	 * We want the bus clock to be as close as possible to, but no
 	 * greater than, new_clock.  As we can divide by 1 << i for
@@ -709,6 +714,7 @@  int renesas_sdhi_probe(struct platform_device *pdev,
 		mmc_data->max_segs = of_data->max_segs;
 		dma_priv->dma_buswidth = of_data->dma_buswidth;
 		host->bus_shift = of_data->bus_shift;
+		priv->scc_base_f_min = of_data->scc_base_f_min;
 	}
 
 	host->write16_hook	= renesas_sdhi_write16_hook;
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 751fe91c7571..7010c524b180 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -109,6 +109,8 @@  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 	.bus_shift	= 2,
 	.scc_offset	= 0x1000,
+	/* SCC module clock (SDnH) is enabled at 100MHz or more */
+	.scc_base_f_min = 100000000,
 	.taps		= rcar_gen3_scc_taps,
 	.taps_num	= ARRAY_SIZE(rcar_gen3_scc_taps),
 	/* DMAC can handle 32bit blk count but only 1 segment */