| Message ID | 20210602073435.5955-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: renesas_sdhi: abort tuning when timeout detected | expand |
Hi Wolfram, Thanks for your work. On 2021-06-02 09:34:35 +0200, Wolfram Sang wrote: > We have to bring the eMMC from sending-data state back to transfer state > once we detected a CRC error (timeout) during tuning. So, send a stop > command via mmc_abort_tuning(). > > Fixes: 4f11997773b6 ("mmc: tmio: Add tuning support") > Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > > Ulf, I'd think that mmc_abort_tuning() should be named > mmc_abort_tuning_cmd() instead. Because we don't actually abort the > tuning as a whole in this function. What do you think? I can prepare a > patch if you agree. > > drivers/mmc/host/renesas_sdhi_core.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 635bf31a6735..9029308c4a0f 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -692,14 +692,19 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode) > > /* Issue CMD19 twice for each tap */ > for (i = 0; i < 2 * priv->tap_num; i++) { > + int cmd_error; > + > /* Set sampling clock position */ > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num); > > - if (mmc_send_tuning(mmc, opcode, NULL) == 0) > + if (mmc_send_tuning(mmc, opcode, &cmd_error) == 0) > set_bit(i, priv->taps); > > if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0) > set_bit(i, priv->smpcmp); > + > + if (cmd_error) > + mmc_abort_tuning(mmc, opcode); > } > > ret = renesas_sdhi_select_tuning(host); > -- > 2.30.2 >
Hi Wolfram-san, > From: Wolfram Sang, Sent: Wednesday, June 2, 2021 4:35 PM > > We have to bring the eMMC from sending-data state back to transfer state > once we detected a CRC error (timeout) during tuning. So, send a stop > command via mmc_abort_tuning(). > > Fixes: 4f11997773b6 ("mmc: tmio: Add tuning support") > Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thank you for the patch! Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Also, I tested this patch on my environment (r8a77951-salvator-xs), and I confirmed this patch could send a stop command. So, Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Best regards, Yoshihiro Shimoda
On Wed, 2 Jun 2021 at 09:34, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > We have to bring the eMMC from sending-data state back to transfer state > once we detected a CRC error (timeout) during tuning. So, send a stop > command via mmc_abort_tuning(). > > Fixes: 4f11997773b6 ("mmc: tmio: Add tuning support") > Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied for fixes and by adding a stable tag, thanks! > --- > > Ulf, I'd think that mmc_abort_tuning() should be named > mmc_abort_tuning_cmd() instead. Because we don't actually abort the > tuning as a whole in this function. What do you think? I can prepare a > patch if you agree. Good point. I have no strong opinion, but perhaps mmc_send_abort_tuning() is more consistent with other function names? Kind regards Uffe > > drivers/mmc/host/renesas_sdhi_core.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 635bf31a6735..9029308c4a0f 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -692,14 +692,19 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode) > > /* Issue CMD19 twice for each tap */ > for (i = 0; i < 2 * priv->tap_num; i++) { > + int cmd_error; > + > /* Set sampling clock position */ > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num); > > - if (mmc_send_tuning(mmc, opcode, NULL) == 0) > + if (mmc_send_tuning(mmc, opcode, &cmd_error) == 0) > set_bit(i, priv->taps); > > if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0) > set_bit(i, priv->smpcmp); > + > + if (cmd_error) > + mmc_abort_tuning(mmc, opcode); > } > > ret = renesas_sdhi_select_tuning(host); > -- > 2.30.2 >
> > Ulf, I'd think that mmc_abort_tuning() should be named > > mmc_abort_tuning_cmd() instead. Because we don't actually abort the > > tuning as a whole in this function. What do you think? I can prepare a > > patch if you agree. > > Good point. > > I have no strong opinion, but perhaps mmc_send_abort_tuning() is more > consistent with other function names? Ack. I will prepare it!
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 635bf31a6735..9029308c4a0f 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -692,14 +692,19 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode) /* Issue CMD19 twice for each tap */ for (i = 0; i < 2 * priv->tap_num; i++) { + int cmd_error; + /* Set sampling clock position */ sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num); - if (mmc_send_tuning(mmc, opcode, NULL) == 0) + if (mmc_send_tuning(mmc, opcode, &cmd_error) == 0) set_bit(i, priv->taps); if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0) set_bit(i, priv->smpcmp); + + if (cmd_error) + mmc_abort_tuning(mmc, opcode); } ret = renesas_sdhi_select_tuning(host);
We have to bring the eMMC from sending-data state back to transfer state once we detected a CRC error (timeout) during tuning. So, send a stop command via mmc_abort_tuning(). Fixes: 4f11997773b6 ("mmc: tmio: Add tuning support") Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Ulf, I'd think that mmc_abort_tuning() should be named mmc_abort_tuning_cmd() instead. Because we don't actually abort the tuning as a whole in this function. What do you think? I can prepare a patch if you agree. drivers/mmc/host/renesas_sdhi_core.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)