| Message ID | 1480677395-14169-2-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016/12/2 19:16, Adrian Hunter wrote: > The JEDEC specification indicates CMD13 can be used after a HS200 switch > to check for errors. However in practice some boards experience CRC errors > in the CMD13 response. Consequently, for HS200, CRC errors are not a > reliable way to know the switch failed. If there really is a problem, we > would expect tuning will fail and the result ends up the same. So change > the error condition to ignore CRC errors in that case. > This makes sense to me, especially for arasan,sdhci-5.1. As it actually needs to update the sd_clk rate to PHY before sending commands when finishing to switch timing. It leads to a dilemma that we should really update the sd_clk to PHY *only after* we actually finish switching timing. But how we need to know we finish switching to e.g HS200? If not using .card_busy, we have to send CMD13 but as I mentioned we need to update sd_clk first, otherwise we expected a CRC there in theory. Which comes first? updating sd_clk to PHY for it to latch the successful result of CMD13 to know we are in HS200 now but updating sd_clk indicates that we already know we are in HS200? :) Although it indeed could work using current code by theoretically it shouldn't. CMD13 shouldn't be used to indicate the status of bus switching at all from my point. This patchs improve it much, Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com> > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/mmc.c | 15 +++++++++++++-- > drivers/mmc/core/mmc_ops.c | 9 ++++++++- > drivers/mmc/core/mmc_ops.h | 1 + > 3 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 033e00abe93f..b61b52f9da3d 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1240,7 +1240,12 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > mmc_set_timing(host, MMC_TIMING_MMC_HS200); > > - err = mmc_switch_status(card); > + /* > + * For HS200, CRC errors are not a reliable way to know the switch > + * failed. If there really is a problem, we would expect tuning will > + * fail and the result ends up the same. > + */ > + err = __mmc_switch_status(card, false); > if (err) > goto out_err; > > @@ -1403,7 +1408,13 @@ static int mmc_select_hs200(struct mmc_card *card) > old_timing = host->ios.timing; > mmc_set_timing(host, MMC_TIMING_MMC_HS200); > > - err = mmc_switch_status(card); > + /* > + * For HS200, CRC errors are not a reliable way to know the > + * switch failed. If there really is a problem, we would expect > + * tuning will fail and the result ends up the same. > + */ > + err = __mmc_switch_status(card, false); > + > /* > * mmc_select_timing() assumes timing has not changed if > * it is a switch error. > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index cb7006feb5c4..81ce63bb0773 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -431,18 +431,25 @@ static int mmc_switch_status_error(struct mmc_host *host, u32 status) > } > > /* Caller must hold re-tuning */ > -int mmc_switch_status(struct mmc_card *card) > +int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal) > { > u32 status; > int err; > > err = mmc_send_status(card, &status); > + if (!crc_err_fatal && err == -EILSEQ) > + return 0; > if (err) > return err; > > return mmc_switch_status_error(card->host, status); > } > > +int mmc_switch_status(struct mmc_card *card) > +{ > + return __mmc_switch_status(card, true); > +} > + > static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms, > bool send_status, bool retry_crc_err) > { > diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h > index 761cb69c46af..abd525ed74be 100644 > --- a/drivers/mmc/core/mmc_ops.h > +++ b/drivers/mmc/core/mmc_ops.h > @@ -28,6 +28,7 @@ > int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status); > int mmc_can_ext_csd(struct mmc_card *card); > int mmc_switch_status(struct mmc_card *card); > +int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal); > int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > unsigned int timeout_ms, unsigned char timing, > bool use_busy_signal, bool send_status, bool retry_crc_err); >
On 2 December 2016 at 12:16, Adrian Hunter <adrian.hunter@intel.com> wrote: > The JEDEC specification indicates CMD13 can be used after a HS200 switch > to check for errors. However in practice some boards experience CRC errors > in the CMD13 response. Consequently, for HS200, CRC errors are not a > reliable way to know the switch failed. If there really is a problem, we > would expect tuning will fail and the result ends up the same. So change > the error condition to ignore CRC errors in that case. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> Thanks, applied for next! I also took the liberty to add the ack from Jaehoon, as he acked v1 (conceptual wise v2 is the same, but not the code). Please tell if you don't like it. Kind regards Uffe > --- > drivers/mmc/core/mmc.c | 15 +++++++++++++-- > drivers/mmc/core/mmc_ops.c | 9 ++++++++- > drivers/mmc/core/mmc_ops.h | 1 + > 3 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 033e00abe93f..b61b52f9da3d 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1240,7 +1240,12 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > mmc_set_timing(host, MMC_TIMING_MMC_HS200); > > - err = mmc_switch_status(card); > + /* > + * For HS200, CRC errors are not a reliable way to know the switch > + * failed. If there really is a problem, we would expect tuning will > + * fail and the result ends up the same. > + */ > + err = __mmc_switch_status(card, false); > if (err) > goto out_err; > > @@ -1403,7 +1408,13 @@ static int mmc_select_hs200(struct mmc_card *card) > old_timing = host->ios.timing; > mmc_set_timing(host, MMC_TIMING_MMC_HS200); > > - err = mmc_switch_status(card); > + /* > + * For HS200, CRC errors are not a reliable way to know the > + * switch failed. If there really is a problem, we would expect > + * tuning will fail and the result ends up the same. > + */ > + err = __mmc_switch_status(card, false); > + > /* > * mmc_select_timing() assumes timing has not changed if > * it is a switch error. > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index cb7006feb5c4..81ce63bb0773 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -431,18 +431,25 @@ static int mmc_switch_status_error(struct mmc_host *host, u32 status) > } > > /* Caller must hold re-tuning */ > -int mmc_switch_status(struct mmc_card *card) > +int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal) > { > u32 status; > int err; > > err = mmc_send_status(card, &status); > + if (!crc_err_fatal && err == -EILSEQ) > + return 0; > if (err) > return err; > > return mmc_switch_status_error(card->host, status); > } > > +int mmc_switch_status(struct mmc_card *card) > +{ > + return __mmc_switch_status(card, true); > +} > + > static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms, > bool send_status, bool retry_crc_err) > { > diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h > index 761cb69c46af..abd525ed74be 100644 > --- a/drivers/mmc/core/mmc_ops.h > +++ b/drivers/mmc/core/mmc_ops.h > @@ -28,6 +28,7 @@ > int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status); > int mmc_can_ext_csd(struct mmc_card *card); > int mmc_switch_status(struct mmc_card *card); > +int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal); > int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > unsigned int timeout_ms, unsigned char timing, > bool use_busy_signal, bool send_status, bool retry_crc_err); > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 033e00abe93f..b61b52f9da3d 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1240,7 +1240,12 @@ int mmc_hs400_to_hs200(struct mmc_card *card) mmc_set_timing(host, MMC_TIMING_MMC_HS200); - err = mmc_switch_status(card); + /* + * For HS200, CRC errors are not a reliable way to know the switch + * failed. If there really is a problem, we would expect tuning will + * fail and the result ends up the same. + */ + err = __mmc_switch_status(card, false); if (err) goto out_err; @@ -1403,7 +1408,13 @@ static int mmc_select_hs200(struct mmc_card *card) old_timing = host->ios.timing; mmc_set_timing(host, MMC_TIMING_MMC_HS200); - err = mmc_switch_status(card); + /* + * For HS200, CRC errors are not a reliable way to know the + * switch failed. If there really is a problem, we would expect + * tuning will fail and the result ends up the same. + */ + err = __mmc_switch_status(card, false); + /* * mmc_select_timing() assumes timing has not changed if * it is a switch error. diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c index cb7006feb5c4..81ce63bb0773 100644 --- a/drivers/mmc/core/mmc_ops.c +++ b/drivers/mmc/core/mmc_ops.c @@ -431,18 +431,25 @@ static int mmc_switch_status_error(struct mmc_host *host, u32 status) } /* Caller must hold re-tuning */ -int mmc_switch_status(struct mmc_card *card) +int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal) { u32 status; int err; err = mmc_send_status(card, &status); + if (!crc_err_fatal && err == -EILSEQ) + return 0; if (err) return err; return mmc_switch_status_error(card->host, status); } +int mmc_switch_status(struct mmc_card *card) +{ + return __mmc_switch_status(card, true); +} + static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms, bool send_status, bool retry_crc_err) { diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h index 761cb69c46af..abd525ed74be 100644 --- a/drivers/mmc/core/mmc_ops.h +++ b/drivers/mmc/core/mmc_ops.h @@ -28,6 +28,7 @@ int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status); int mmc_can_ext_csd(struct mmc_card *card); int mmc_switch_status(struct mmc_card *card); +int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal); int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, unsigned int timeout_ms, unsigned char timing, bool use_busy_signal, bool send_status, bool retry_crc_err);
The JEDEC specification indicates CMD13 can be used after a HS200 switch to check for errors. However in practice some boards experience CRC errors in the CMD13 response. Consequently, for HS200, CRC errors are not a reliable way to know the switch failed. If there really is a problem, we would expect tuning will fail and the result ends up the same. So change the error condition to ignore CRC errors in that case. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/mmc.c | 15 +++++++++++++-- drivers/mmc/core/mmc_ops.c | 9 ++++++++- drivers/mmc/core/mmc_ops.h | 1 + 3 files changed, 22 insertions(+), 3 deletions(-)