| Message ID | 1479293481-20186-9-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16/11/16 12:51, Ulf Hansson wrote: > In cases when the mmc host doesn't support HW busy detection, polling for > busy by using CMD13 is beneficial. The reasons have already been explained > in earlier change logs. > > Moreover, when polling with CMD13 during bus timing changes, we should > retry instead of fail when we get CRC errors. > > Switching from HS200 to HS400 and reverse includes several steps, where > each step changes the bus speed timing. Let's improve the behaviour during > these sequences, by allowing CMD13 polling for each of the step. Let's also > make sure the CMD13 polling becomes retried, while receiving a CRC error. I don't know we need to try to get polling for HS400. The support of HS400 suggests a relatively sophisticated host controller which really should support busy waiting as well. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/core/mmc.c | 87 +++++++++++++++----------------------------------- > 1 file changed, 26 insertions(+), 61 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 0b26383..24b9e72 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1094,8 +1094,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card) > static int mmc_select_hs400(struct mmc_card *card) > { > struct mmc_host *host = card->host; > - unsigned int max_dtr; > - int err = 0; > + int err; > u8 val; > > /* > @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card) > host->ios.bus_width == MMC_BUS_WIDTH_8)) > return 0; > > - /* Switch card to HS mode */ > - 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, > - true, false, true); > + /* > + * Switch card to HS mode. > + * First reduce to the HS frequency as CMD13 are sent to poll for busy > + * and/or to validate the switch status. > + */ > + mmc_set_clock(host, card->ext_csd.hs_max_dtr); That was moved by commit 649c6059d2371fef886a8f967e21416204723d63 ("mmc: mmc: Fix HS switch failure in mmc_select_hs400()") i.e. if you put it back I would expect the gru board problem to reappear. > + err = mmc_select_hs(card); > if (err) { > pr_err("%s: switch to high-speed from hs200 failed, err:%d\n", > mmc_hostname(host), err); > return err; > } > > - /* Set host controller to HS timing */ > - mmc_set_timing(card->host, MMC_TIMING_MMC_HS); > - > - /* Reduce frequency to HS frequency */ > - max_dtr = card->ext_csd.hs_max_dtr; > - mmc_set_clock(host, max_dtr); > - > - err = mmc_switch_status(card); > - if (err) > - goto out_err; > - > - /* Switch card to DDR */ > - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > - EXT_CSD_BUS_WIDTH, > - EXT_CSD_DDR_BUS_WIDTH_8, > - card->ext_csd.generic_cmd6_time); > + /* Switch card to HS DDR */ > + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > + EXT_CSD_BUS_WIDTH, > + EXT_CSD_DDR_BUS_WIDTH_8, > + card->ext_csd.generic_cmd6_time, > + MMC_TIMING_MMC_DDR52, > + true, true, true); > if (err) { > pr_err("%s: switch to bus width for hs400 failed, err:%d\n", > mmc_hostname(host), err); > @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card) > card->drive_strength << EXT_CSD_DRV_STR_SHIFT; > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > EXT_CSD_HS_TIMING, val, > - card->ext_csd.generic_cmd6_time, 0, > - true, false, true); > + card->ext_csd.generic_cmd6_time, > + MMC_TIMING_MMC_HS400, > + true, true, true); This is not exactly right. Tuning has been done at the HS400 operating frequency. That is why we set the bus speed before sending any more commands. i.e. mmc_switch_status(card) is below mmc_set_bus_speed(card) > if (err) { > pr_err("%s: switch to hs400 failed, err:%d\n", > mmc_hostname(host), err); > return err; > } > > - /* Set host controller to HS400 timing and frequency */ > - mmc_set_timing(host, MMC_TIMING_MMC_HS400); > mmc_set_bus_speed(card); > - > - err = mmc_switch_status(card); > - if (err) > - goto out_err; > - > return 0; > - > -out_err: > - pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host), > - __func__, err); > - return err; > } > > int mmc_hs200_to_hs400(struct mmc_card *card) > @@ -1187,27 +1167,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > /* 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, > - true, false, true); > - if (err) > - goto out_err; > - > - mmc_set_timing(host, MMC_TIMING_MMC_DDR52); > - > - err = mmc_switch_status(card); > + val, card->ext_csd.generic_cmd6_time, > + MMC_TIMING_MMC_DDR52, > + true, true, true); > if (err) > goto out_err; > > /* Switch HS DDR to HS */ > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH, > EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time, > - 0, true, false, true); > - if (err) > - goto out_err; > - > - mmc_set_timing(host, MMC_TIMING_MMC_HS); > - > - err = mmc_switch_status(card); > + MMC_TIMING_MMC_HS, > + true, true, true); > if (err) > goto out_err; > > @@ -1215,14 +1185,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > val = EXT_CSD_TIMING_HS200 | > card->drive_strength << EXT_CSD_DRV_STR_SHIFT; > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, > - val, card->ext_csd.generic_cmd6_time, 0, > - true, false, true); > - if (err) > - goto out_err; > - > - mmc_set_timing(host, MMC_TIMING_MMC_HS200); > - > - err = mmc_switch_status(card); > + val, card->ext_csd.generic_cmd6_time, > + MMC_TIMING_MMC_HS200, > + true, true, true); > if (err) > goto out_err; > > -- 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
On 18 November 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 16/11/16 12:51, Ulf Hansson wrote: >> In cases when the mmc host doesn't support HW busy detection, polling for >> busy by using CMD13 is beneficial. The reasons have already been explained >> in earlier change logs. >> >> Moreover, when polling with CMD13 during bus timing changes, we should >> retry instead of fail when we get CRC errors. >> >> Switching from HS200 to HS400 and reverse includes several steps, where >> each step changes the bus speed timing. Let's improve the behaviour during >> these sequences, by allowing CMD13 polling for each of the step. Let's also >> make sure the CMD13 polling becomes retried, while receiving a CRC error. > > I don't know we need to try to get polling for HS400. The support of HS400 > suggests a relatively sophisticated host controller which really should > support busy waiting as well. That's a reasonable argument. Moreover, I do also think that argument stands for a controller supporting HS200. So I have no problem dropping the CMD13 polling support for that bus speed mode as well, if people wants that. > >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/mmc/core/mmc.c | 87 +++++++++++++++----------------------------------- >> 1 file changed, 26 insertions(+), 61 deletions(-) >> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >> index 0b26383..24b9e72 100644 >> --- a/drivers/mmc/core/mmc.c >> +++ b/drivers/mmc/core/mmc.c >> @@ -1094,8 +1094,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card) >> static int mmc_select_hs400(struct mmc_card *card) >> { >> struct mmc_host *host = card->host; >> - unsigned int max_dtr; >> - int err = 0; >> + int err; >> u8 val; >> >> /* >> @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card) >> host->ios.bus_width == MMC_BUS_WIDTH_8)) >> return 0; >> >> - /* Switch card to HS mode */ >> - 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, >> - true, false, true); >> + /* >> + * Switch card to HS mode. >> + * First reduce to the HS frequency as CMD13 are sent to poll for busy >> + * and/or to validate the switch status. >> + */ >> + mmc_set_clock(host, card->ext_csd.hs_max_dtr); > > That was moved by commit 649c6059d2371fef886a8f967e21416204723d63 > ("mmc: mmc: Fix HS switch failure in mmc_select_hs400()") i.e. if you put it > back I would expect the gru board problem to reappear. Correct, thanks for pointing it out! According to below comments, it's clear that the JEDEC spec lacks a good description of the HS200/400 switch behaviour. > >> + err = mmc_select_hs(card); >> if (err) { >> pr_err("%s: switch to high-speed from hs200 failed, err:%d\n", >> mmc_hostname(host), err); >> return err; >> } >> >> - /* Set host controller to HS timing */ >> - mmc_set_timing(card->host, MMC_TIMING_MMC_HS); >> - >> - /* Reduce frequency to HS frequency */ >> - max_dtr = card->ext_csd.hs_max_dtr; >> - mmc_set_clock(host, max_dtr); >> - >> - err = mmc_switch_status(card); >> - if (err) >> - goto out_err; >> - >> - /* Switch card to DDR */ >> - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> - EXT_CSD_BUS_WIDTH, >> - EXT_CSD_DDR_BUS_WIDTH_8, >> - card->ext_csd.generic_cmd6_time); >> + /* Switch card to HS DDR */ >> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> + EXT_CSD_BUS_WIDTH, >> + EXT_CSD_DDR_BUS_WIDTH_8, >> + card->ext_csd.generic_cmd6_time, >> + MMC_TIMING_MMC_DDR52, >> + true, true, true); >> if (err) { >> pr_err("%s: switch to bus width for hs400 failed, err:%d\n", >> mmc_hostname(host), err); >> @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card) >> card->drive_strength << EXT_CSD_DRV_STR_SHIFT; >> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> EXT_CSD_HS_TIMING, val, >> - card->ext_csd.generic_cmd6_time, 0, >> - true, false, true); >> + card->ext_csd.generic_cmd6_time, >> + MMC_TIMING_MMC_HS400, >> + true, true, true); > > This is not exactly right. Tuning has been done at the HS400 operating > frequency. That is why we set the bus speed before sending any more > commands. i.e. mmc_switch_status(card) is below mmc_set_bus_speed(card) Right, I see! I had another look at the JEDEC spec for HS400 mode switch: --- 6) Perform the Tuning Process at the HS400 target operating frequency, NOTE Tuning process in HS200 mode is required to synchronize the command response on the CMD line to CLK for HS400 operation. --- This confirms your point. Although on the other hand by looking at the HS400 switch flow chart, it says that checking the switch status shall be done *before* bumping the clock rate. :-) Anyway, I will move back to the behaviour where CMD13 polling is not allowed for HS400, as that seems like the only thing we can do! Kind regards Uffe -- 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 0b26383..24b9e72 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1094,8 +1094,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card) static int mmc_select_hs400(struct mmc_card *card) { struct mmc_host *host = card->host; - unsigned int max_dtr; - int err = 0; + int err; u8 val; /* @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card) host->ios.bus_width == MMC_BUS_WIDTH_8)) return 0; - /* Switch card to HS mode */ - 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, - true, false, true); + /* + * Switch card to HS mode. + * First reduce to the HS frequency as CMD13 are sent to poll for busy + * and/or to validate the switch status. + */ + mmc_set_clock(host, card->ext_csd.hs_max_dtr); + err = mmc_select_hs(card); if (err) { pr_err("%s: switch to high-speed from hs200 failed, err:%d\n", mmc_hostname(host), err); return err; } - /* Set host controller to HS timing */ - mmc_set_timing(card->host, MMC_TIMING_MMC_HS); - - /* Reduce frequency to HS frequency */ - max_dtr = card->ext_csd.hs_max_dtr; - mmc_set_clock(host, max_dtr); - - err = mmc_switch_status(card); - if (err) - goto out_err; - - /* Switch card to DDR */ - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, - EXT_CSD_BUS_WIDTH, - EXT_CSD_DDR_BUS_WIDTH_8, - card->ext_csd.generic_cmd6_time); + /* Switch card to HS DDR */ + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, + EXT_CSD_BUS_WIDTH, + EXT_CSD_DDR_BUS_WIDTH_8, + card->ext_csd.generic_cmd6_time, + MMC_TIMING_MMC_DDR52, + true, true, true); if (err) { pr_err("%s: switch to bus width for hs400 failed, err:%d\n", mmc_hostname(host), err); @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card) card->drive_strength << EXT_CSD_DRV_STR_SHIFT; err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, val, - card->ext_csd.generic_cmd6_time, 0, - true, false, true); + card->ext_csd.generic_cmd6_time, + MMC_TIMING_MMC_HS400, + true, true, true); if (err) { pr_err("%s: switch to hs400 failed, err:%d\n", mmc_hostname(host), err); return err; } - /* Set host controller to HS400 timing and frequency */ - mmc_set_timing(host, MMC_TIMING_MMC_HS400); mmc_set_bus_speed(card); - - err = mmc_switch_status(card); - if (err) - goto out_err; - return 0; - -out_err: - pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host), - __func__, err); - return err; } int mmc_hs200_to_hs400(struct mmc_card *card) @@ -1187,27 +1167,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card) /* 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, - true, false, true); - if (err) - goto out_err; - - mmc_set_timing(host, MMC_TIMING_MMC_DDR52); - - err = mmc_switch_status(card); + val, card->ext_csd.generic_cmd6_time, + MMC_TIMING_MMC_DDR52, + true, true, true); if (err) goto out_err; /* Switch HS DDR to HS */ err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH, EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time, - 0, true, false, true); - if (err) - goto out_err; - - mmc_set_timing(host, MMC_TIMING_MMC_HS); - - err = mmc_switch_status(card); + MMC_TIMING_MMC_HS, + true, true, true); if (err) goto out_err; @@ -1215,14 +1185,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card) val = EXT_CSD_TIMING_HS200 | card->drive_strength << EXT_CSD_DRV_STR_SHIFT; err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, - val, card->ext_csd.generic_cmd6_time, 0, - true, false, true); - if (err) - goto out_err; - - mmc_set_timing(host, MMC_TIMING_MMC_HS200); - - err = mmc_switch_status(card); + val, card->ext_csd.generic_cmd6_time, + MMC_TIMING_MMC_HS200, + true, true, true); if (err) goto out_err;
In cases when the mmc host doesn't support HW busy detection, polling for busy by using CMD13 is beneficial. The reasons have already been explained in earlier change logs. Moreover, when polling with CMD13 during bus timing changes, we should retry instead of fail when we get CRC errors. Switching from HS200 to HS400 and reverse includes several steps, where each step changes the bus speed timing. Let's improve the behaviour during these sequences, by allowing CMD13 polling for each of the step. Let's also make sure the CMD13 polling becomes retried, while receiving a CRC error. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/core/mmc.c | 87 +++++++++++++++----------------------------------- 1 file changed, 26 insertions(+), 61 deletions(-) -- 1.9.1