| Message ID | 20200312142501.9868-2-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: core: Do not change frequency before switch from HS400 | expand |
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? > if (err) > goto out; > > -- > 2.17.1 > Kind regards Uffe
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? 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,
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, >
On 13/03/20 12:40 pm, Ulf Hansson wrote: > 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? Yes, the additional error handling effectively gives a few more retries, so that should do for now. From: Adrian Hunter <adrian.hunter@intel.com> Date: Fri, 13 Mar 2020 16:01:06 +0200 Subject: [PATCH] mmc: core: Do not change frequency before switch from HS400 Host controllers may support HS400 only at the expected frequency, so do not change frequency before switch from HS400 but in case of CRC error, wait for a busy card and then try again. Also try again after frequency change if the error persists. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/mmc.c | 27 +++++++++++++++++++++------ drivers/mmc/core/mmc_ops.c | 6 +++--- drivers/mmc/core/mmc_ops.h | 3 +++ 3 files changed, 27 insertions(+), 9 deletions(-) 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 aa0cab190cd8..8d9c3b97269c 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,
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); + } if (err) goto out;
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(+)