| Message ID | DB6PR0402MB2760B71254EEA50DC78DDBCB88B90@DB6PR0402MB2760.eurprd04.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: retry CMD1 in mmc_send_op_cond() until the eMMC is ready | expand |
On 5/19/20 12:37 PM, Peng Fan wrote: >> Subject: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the eMMC >> is ready >> >> From: Haibo Chen <haibo.chen at nxp.com> >> >> According to eMMC specification v5.1 section 6.4.3, we should issue >> CMD1 repeatedly in the idle state until the eMMC is ready even if >> mmc_send_op_cond() send CMD1 with argument = 0. Otherwise some >> eMMC devices seems to enter the inactive mode after >> mmc_complete_op_cond() issued CMD0 when the eMMC device is busy. This >> patch also align with Linux 5.7. Well, i think that it can't compare to linux driver. is your issue fixed with this patch? As i know, this sequence is not follow correct spec. To enhance boot, it was skipping the polling time. If there is a problem, could you explain in more detail? Then I will also check on my targets. >> >> Signed-off-by: Haibo Chen <haibo.chen at nxp.com> >> --- >> drivers/mmc/mmc.c | 41 +++++++++++++++++++++++++++++++++-------- >> 1 file changed, 33 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index >> 523c055967..509549756b 100644 >> --- a/drivers/mmc/mmc.c >> +++ b/drivers/mmc/mmc.c >> @@ -664,21 +664,46 @@ static int mmc_send_op_cond_iter(struct mmc >> *mmc, int use_arg) >> >> static int mmc_send_op_cond(struct mmc *mmc) { >> + struct mmc_cmd cmd; >> int err, i; >> + int timeout = 1000; >> + ulong start; >> >> /* Some cards seem to need this */ >> mmc_go_idle(mmc); >> >> - /* Asking to the card its capabilities */ >> - for (i = 0; i < 2; i++) { >> - err = mmc_send_op_cond_iter(mmc, i != 0); >> + cmd.cmdidx = MMC_CMD_SEND_OP_COND; >> + cmd.resp_type = MMC_RSP_R3; >> + cmd.cmdarg = 0; >> + >> + start = get_timer(0); >> + /* >> + * According to eMMC specification v5.1 section 6.4.3, we >> + * should issue CMD1 repeatedly in the idle state until >> + * the eMMC is ready. Otherwise some eMMC devices seem to enter >> + * the inactive mode after mmc_complete_op_cond() issued CMD0 >> when >> + * the eMMC device is busy. >> + */ > > Could we just extend the previous for loop to fix the issue? > Your piece code duplicate much of the code and changed > the original behavior. > >> + while (1) { >> + err = mmc_send_cmd(mmc, &cmd, NULL); >> if (err) >> return err; >> - >> - /* exit if not busy (flag seems to be inverted) */ >> - if (mmc->ocr & OCR_BUSY) >> - break; >> + if (mmc_host_is_spi(mmc)) { >> + if (!(cmd.response[0] & (1 << 0))) >> + break; >> + } else { >> + mmc->ocr = cmd.response[0]; >> + /* exit if not busy (flag seems to be inverted) */ >> + if (mmc->ocr & OCR_BUSY) >> + break; >> + } >> + if (get_timer(start) > timeout) >> + return -EOPNOTSUPP; > > TIMEOUT, please. > >> + udelay(100); > > This will change the original behavior by adding a delay. > >> + if (!mmc_host_is_spi(mmc)) >> + cmd.cmdarg = cmd.response[0] | (1 << 30); > > Do we need "1 << 30" ? > >> } >> + >> mmc->op_cond_pending = 1; >> return 0; >> } >> @@ -691,7 +716,7 @@ static int mmc_complete_op_cond(struct mmc >> *mmc) >> int err; >> >> mmc->op_cond_pending = 0; >> - if (!(mmc->ocr & OCR_BUSY)) { >> + if (mmc->ocr & OCR_BUSY) { > > When card not go out busy, it means card not > finish power up seq. Why drop !? Right. It doesn't finish power-ups seq. If it's working with this patch, it seems something wrong. > >> /* Some cards seem to need this */ >> mmc_go_idle(mmc); >> >> -- > > Would the following patch help? yes. it's more better. If need to increase polling time, i think it's correct place in mmc_complete_op_cond(). Best Regards, Jaehoon Chung > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index 523c055967..d3a0538cdb 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -665,12 +665,15 @@ static int mmc_send_op_cond_iter(struct mmc *mmc, int use_arg) > static int mmc_send_op_cond(struct mmc *mmc) > { > int err, i; > + int timeout = 1000; > + ulong start; > > /* Some cards seem to need this */ > mmc_go_idle(mmc); > > + start = get_timer(0); > /* Asking to the card its capabilities */ > - for (i = 0; i < 2; i++) { > + for (i = 0; ; i++) { > err = mmc_send_op_cond_iter(mmc, i != 0); > if (err) > return err; > @@ -678,6 +681,9 @@ static int mmc_send_op_cond(struct mmc *mmc) > /* exit if not busy (flag seems to be inverted) */ > if (mmc->ocr & OCR_BUSY) > break; > + > + if (get_timer(start) > timeout) > + return -ETIMEDOUT; > } > mmc->op_cond_pending = 1; > return 0; > > Thanks, > Peng. >> 2.17.1 > >
> -----Original Message----- > From: Peng Fan <peng.fan at nxp.com> > Sent: 2020?5?19? 11:38 > To: BOUGH CHEN <haibo.chen at nxp.com>; u-boot at lists.denx.de > Cc: dl-uboot-imx <uboot-imx at nxp.com> > Subject: RE: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the > eMMC is ready > > > Subject: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the eMMC > > is ready > > > > From: Haibo Chen <haibo.chen at nxp.com> > > > > According to eMMC specification v5.1 section 6.4.3, we should issue > > CMD1 repeatedly in the idle state until the eMMC is ready even if > > mmc_send_op_cond() send CMD1 with argument = 0. Otherwise some > eMMC > > devices seems to enter the inactive mode after > > mmc_complete_op_cond() issued CMD0 when the eMMC device is busy. This > > patch also align with Linux 5.7. > > > > Signed-off-by: Haibo Chen <haibo.chen at nxp.com> > > --- > > drivers/mmc/mmc.c | 41 +++++++++++++++++++++++++++++++++-------- > > 1 file changed, 33 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index > > 523c055967..509549756b 100644 > > --- a/drivers/mmc/mmc.c > > +++ b/drivers/mmc/mmc.c > > @@ -664,21 +664,46 @@ static int mmc_send_op_cond_iter(struct mmc > > *mmc, int use_arg) > > > > static int mmc_send_op_cond(struct mmc *mmc) { > > + struct mmc_cmd cmd; > > int err, i; > > + int timeout = 1000; > > + ulong start; > > > > /* Some cards seem to need this */ > > mmc_go_idle(mmc); > > > > - /* Asking to the card its capabilities */ > > - for (i = 0; i < 2; i++) { > > - err = mmc_send_op_cond_iter(mmc, i != 0); > > + cmd.cmdidx = MMC_CMD_SEND_OP_COND; > > + cmd.resp_type = MMC_RSP_R3; > > + cmd.cmdarg = 0; > > + > > + start = get_timer(0); > > + /* > > + * According to eMMC specification v5.1 section 6.4.3, we > > + * should issue CMD1 repeatedly in the idle state until > > + * the eMMC is ready. Otherwise some eMMC devices seem to enter > > + * the inactive mode after mmc_complete_op_cond() issued CMD0 > > when > > + * the eMMC device is busy. > > + */ > > Could we just extend the previous for loop to fix the issue? > Your piece code duplicate much of the code and changed the original behavior. > > > + while (1) { > > + err = mmc_send_cmd(mmc, &cmd, NULL); > > if (err) > > return err; > > - > > - /* exit if not busy (flag seems to be inverted) */ > > - if (mmc->ocr & OCR_BUSY) > > - break; > > + if (mmc_host_is_spi(mmc)) { > > + if (!(cmd.response[0] & (1 << 0))) > > + break; > > + } else { > > + mmc->ocr = cmd.response[0]; > > + /* exit if not busy (flag seems to be inverted) */ > > + if (mmc->ocr & OCR_BUSY) > > + break; > > + } > > + if (get_timer(start) > timeout) > > + return -EOPNOTSUPP; > > TIMEOUT, please. > > > + udelay(100); > > This will change the original behavior by adding a delay. > > > + if (!mmc_host_is_spi(mmc)) > > + cmd.cmdarg = cmd.response[0] | (1 << 30); > > Do we need "1 << 30" ? > > > } > > + > > mmc->op_cond_pending = 1; > > return 0; > > } > > @@ -691,7 +716,7 @@ static int mmc_complete_op_cond(struct mmc > > *mmc) > > int err; > > > > mmc->op_cond_pending = 0; > > - if (!(mmc->ocr & OCR_BUSY)) { > > + if (mmc->ocr & OCR_BUSY) { > > When card not go out busy, it means card not finish power up seq. Why drop !? > > > /* Some cards seem to need this */ > > mmc_go_idle(mmc); > > > > -- > > Would the following patch help? > Hi Peng, Thanks for your carefully review and suggestion, most eMMC chip can work normal on the original code, only some Toshiba eMMC need more time to get out of busy after cmd1. The patch I send out just refer to the Linux driver code, and pass on the customer side. Currently I'm not sure whether the bit 30 of the argument for cmd1 is necessary for this eMMC, I will get the reworked board which soldered this Toshiba eMMC in a few days, then I can test on my hand. Will test this then and give you response. Best Regards Haibo Chen > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index > 523c055967..d3a0538cdb 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -665,12 +665,15 @@ static int mmc_send_op_cond_iter(struct mmc > *mmc, int use_arg) static int mmc_send_op_cond(struct mmc *mmc) { > int err, i; > + int timeout = 1000; > + ulong start; > > /* Some cards seem to need this */ > mmc_go_idle(mmc); > > + start = get_timer(0); > /* Asking to the card its capabilities */ > - for (i = 0; i < 2; i++) { > + for (i = 0; ; i++) { > err = mmc_send_op_cond_iter(mmc, i != 0); > if (err) > return err; > @@ -678,6 +681,9 @@ static int mmc_send_op_cond(struct mmc *mmc) > /* exit if not busy (flag seems to be inverted) */ > if (mmc->ocr & OCR_BUSY) > break; > + > + if (get_timer(start) > timeout) > + return -ETIMEDOUT; > } > mmc->op_cond_pending = 1; > return 0; > > Thanks, > Peng. > > 2.17.1
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 523c055967..d3a0538cdb 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -665,12 +665,15 @@ static int mmc_send_op_cond_iter(struct mmc *mmc, int use_arg) static int mmc_send_op_cond(struct mmc *mmc) { int err, i; + int timeout = 1000; + ulong start; /* Some cards seem to need this */ mmc_go_idle(mmc); + start = get_timer(0); /* Asking to the card its capabilities */ - for (i = 0; i < 2; i++) { + for (i = 0; ; i++) { err = mmc_send_op_cond_iter(mmc, i != 0); if (err) return err; @@ -678,6 +681,9 @@ static int mmc_send_op_cond(struct mmc *mmc) /* exit if not busy (flag seems to be inverted) */ if (mmc->ocr & OCR_BUSY) break; + + if (get_timer(start) > timeout) + return -ETIMEDOUT; } mmc->op_cond_pending = 1; return 0;