Re: [PATCH RFC 1/3] mmc: core: Try harder if transfer mode switch fails

[Ulf Hansson <ulf.hansson@linaro.org>]

On Fri, 13 Mar 2020 at 10:53, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 12/03/20 5:45 pm, Ulf Hansson wrote:
> > On Thu, 12 Mar 2020 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> Add extra busy wait and retries if transfer mode switch fails.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> ---
> >>  drivers/mmc/core/mmc_ops.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> >> index aa0cab190cd8..619088a94688 100644
> >> --- a/drivers/mmc/core/mmc_ops.c
> >> +++ b/drivers/mmc/core/mmc_ops.c
> >> @@ -599,6 +599,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
> >>                 cmd.sanitize_busy = true;
> >>
> >>         err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> >> +       if (err && index == EXT_CSD_HS_TIMING) {
> >> +               /* Try harder for timing changes */
> >> +               __mmc_poll_for_busy(card, timeout_ms, send_status,
> >> +                                   retry_crc_err, MMC_BUSY_CMD6);
> >> +               err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
> >> +       }
> >
> > Hmm, what do you think of moving this to the caller(s) of
> > __mmc_switch() and in particular only at those places were we find it
> > useful. Me personally, would prefer that option.
> >
> > To do that, we may need to have the possibility of specifying the
> > number of retries that should be used in the mmc_wait_for_cmd() call
> > to the caller can check the error code better.
> >
> > Moreover, it looks a bit risky to do the polling for all kinds of
> > errors - shouldn't we do for CRC errors?
> >
>
> What about this then?

Looks good to me!

Is the retry in __mmc_switch() with MMC_CMD_RETRIES okay for now you think?

Kind regards
Uffe

>
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c2abd417a84a..faa5d30ed891 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1235,20 +1235,35 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>         int err;
>         u8 val;
>
> -       /* Reduce frequency to HS */
> -       max_dtr = card->ext_csd.hs_max_dtr;
> -       mmc_set_clock(host, max_dtr);
> -
>         /* Switch HS400 to HS DDR */
>         val = EXT_CSD_TIMING_HS;
>         err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
>                            val, card->ext_csd.generic_cmd6_time, 0,
>                            false, true);
> -       if (err)
> -               goto out_err;
> +       if (err == -EILSEQ) {
> +               __mmc_poll_for_busy(card, card->ext_csd.generic_cmd6_time,
> +                                   false, true, MMC_BUSY_CMD6);
> +               err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +                                  EXT_CSD_HS_TIMING, val,
> +                                  card->ext_csd.generic_cmd6_time, 0, false,
> +                                  true);
> +       }
>
>         mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
>
> +       /* Reduce frequency to HS */
> +       max_dtr = card->ext_csd.hs_max_dtr;
> +       mmc_set_clock(host, max_dtr);
> +
> +       if (err) {
> +               err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +                                  EXT_CSD_HS_TIMING, val,
> +                                  card->ext_csd.generic_cmd6_time, 0, false,
> +                                  true);
> +       }
> +       if (err)
> +               goto out_err;
> +
>         err = mmc_switch_status(card, true);
>         if (err)
>                 goto out_err;
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 6eb87833d478..54afee7c34ae 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -484,9 +484,9 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
>         return 0;
>  }
>
> -static int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> -                              bool send_status, bool retry_crc_err,
> -                              enum mmc_busy_cmd busy_cmd)
> +int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> +                       bool send_status, bool retry_crc_err,
> +                       enum mmc_busy_cmd busy_cmd)
>  {
>         struct mmc_host *host = card->host;
>         int err;
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 38dcfeeaf6d5..649985507f87 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -36,6 +36,9 @@ int mmc_interrupt_hpi(struct mmc_card *card);
>  int mmc_can_ext_csd(struct mmc_card *card);
>  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>  int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
> +int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> +                       bool send_status, bool retry_crc_err,
> +                       enum mmc_busy_cmd busy_cmd);
>  int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>                       enum mmc_busy_cmd busy_cmd);
>  int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>