| Message ID | 1494613000-8156-23-git-send-email-jjhiblot@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > From: Vignesh R <vigneshr@ti.com> > > With certain SD cards like Kingston 8GB/16GB UHS card, it is seen that > MMC_CMD_ALL_SEND_CID cmd fails on first attempt, but succeeds > subsequently. Therefore, retry MMC_CMD_ALL_SEND_CID cmd at least thrice > as done in Linux kernel. > Similarly, it is seen that MMC_CMD_SET_BLOCKLEN may fail on first > attempt, therefore retry this cmd five times as done in kernel. > > Signed-off-by: Vignesh R <vigneshr@ti.com> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/mmc/mmc.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > So sad to see this sort of thing. Can we enable this via a quirk and a Kconfig? Could default to on, but I'm not sure we want this code in regardless. Reviewed-by: Simon Glass <sjg@chromium.org> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index c7dda64..49edf52 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -275,6 +275,8 @@ int mmc_send_status(struct mmc *mmc, int timeout) > int mmc_set_blocklen(struct mmc *mmc, int len) > { > struct mmc_cmd cmd; > + int retries = 5; > + int err; > > if (mmc->ddr_mode) > return 0; > @@ -282,8 +284,13 @@ int mmc_set_blocklen(struct mmc *mmc, int len) > cmd.cmdidx = MMC_CMD_SET_BLOCKLEN; > cmd.resp_type = MMC_RSP_R1; > cmd.cmdarg = len; > + do { > + err = mmc_send_cmd(mmc, &cmd, NULL); > + if (!err) > + break; > + } while (retries--); > > - return mmc_send_cmd(mmc, &cmd, NULL); > + return err; > } > > static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start, > @@ -1867,6 +1874,7 @@ static int mmc_startup(struct mmc *mmc) > u64 cmult, csize; > struct mmc_cmd cmd; > struct blk_desc *bdesc; > + int retries = 3; > > #ifdef CONFIG_MMC_SPI_CRC_ON > if (mmc_host_is_spi(mmc)) { /* enable CRC check for spi */ > @@ -1874,7 +1882,6 @@ static int mmc_startup(struct mmc *mmc) > cmd.resp_type = MMC_RSP_R1; > cmd.cmdarg = 1; > err = mmc_send_cmd(mmc, &cmd, NULL); > - > if (err) > return err; > } > @@ -1886,7 +1893,9 @@ static int mmc_startup(struct mmc *mmc) > cmd.resp_type = MMC_RSP_R2; > cmd.cmdarg = 0; > > - err = mmc_send_cmd(mmc, &cmd, NULL); > + do { > + err = mmc_send_cmd(mmc, &cmd, NULL); > + } while (err && retries-- > 0); > > if (err) > return err; > -- > 1.9.1 >
On 15/05/2017 05:28, Simon Glass wrote: > On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >> From: Vignesh R <vigneshr@ti.com> >> >> With certain SD cards like Kingston 8GB/16GB UHS card, it is seen that >> MMC_CMD_ALL_SEND_CID cmd fails on first attempt, but succeeds >> subsequently. Therefore, retry MMC_CMD_ALL_SEND_CID cmd at least thrice >> as done in Linux kernel. >> Similarly, it is seen that MMC_CMD_SET_BLOCKLEN may fail on first >> attempt, therefore retry this cmd five times as done in kernel. >> >> Signed-off-by: Vignesh R <vigneshr@ti.com> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> drivers/mmc/mmc.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> > So sad to see this sort of thing. > > Can we enable this via a quirk and a Kconfig? Could default to on, but > I'm not sure we want this code in regardless. > > Reviewed-by: Simon Glass <sjg@chromium.org> I admit that it's ugly... no clean code survives contact with a few hundred different hardware parts. We could add KConfig options to enable/disable those quirks but I don't see the point of being able to. It's just a basic retry that has no impact on things that work well already and might make other work more reliably. > > >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >> index c7dda64..49edf52 100644 >> --- a/drivers/mmc/mmc.c >> +++ b/drivers/mmc/mmc.c >> @@ -275,6 +275,8 @@ int mmc_send_status(struct mmc *mmc, int timeout) >> int mmc_set_blocklen(struct mmc *mmc, int len) >> { >> struct mmc_cmd cmd; >> + int retries = 5; >> + int err; >> >> if (mmc->ddr_mode) >> return 0; >> @@ -282,8 +284,13 @@ int mmc_set_blocklen(struct mmc *mmc, int len) >> cmd.cmdidx = MMC_CMD_SET_BLOCKLEN; >> cmd.resp_type = MMC_RSP_R1; >> cmd.cmdarg = len; >> + do { >> + err = mmc_send_cmd(mmc, &cmd, NULL); >> + if (!err) >> + break; >> + } while (retries--); >> >> - return mmc_send_cmd(mmc, &cmd, NULL); >> + return err; >> } >> >> static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start, >> @@ -1867,6 +1874,7 @@ static int mmc_startup(struct mmc *mmc) >> u64 cmult, csize; >> struct mmc_cmd cmd; >> struct blk_desc *bdesc; >> + int retries = 3; >> >> #ifdef CONFIG_MMC_SPI_CRC_ON >> if (mmc_host_is_spi(mmc)) { /* enable CRC check for spi */ >> @@ -1874,7 +1882,6 @@ static int mmc_startup(struct mmc *mmc) >> cmd.resp_type = MMC_RSP_R1; >> cmd.cmdarg = 1; >> err = mmc_send_cmd(mmc, &cmd, NULL); >> - >> if (err) >> return err; >> } >> @@ -1886,7 +1893,9 @@ static int mmc_startup(struct mmc *mmc) >> cmd.resp_type = MMC_RSP_R2; >> cmd.cmdarg = 0; >> >> - err = mmc_send_cmd(mmc, &cmd, NULL); >> + do { >> + err = mmc_send_cmd(mmc, &cmd, NULL); >> + } while (err && retries-- > 0); >> >> if (err) >> return err; >> -- >> 1.9.1 >>
Hi Jean-Jacques, On 15 May 2017 at 09:49, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > > > On 15/05/2017 05:28, Simon Glass wrote: >> >> On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >>> >>> From: Vignesh R <vigneshr@ti.com> >>> >>> With certain SD cards like Kingston 8GB/16GB UHS card, it is seen that >>> MMC_CMD_ALL_SEND_CID cmd fails on first attempt, but succeeds >>> subsequently. Therefore, retry MMC_CMD_ALL_SEND_CID cmd at least thrice >>> as done in Linux kernel. >>> Similarly, it is seen that MMC_CMD_SET_BLOCKLEN may fail on first >>> attempt, therefore retry this cmd five times as done in kernel. >>> >>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >>> --- >>> drivers/mmc/mmc.c | 15 ++++++++++++--- >>> 1 file changed, 12 insertions(+), 3 deletions(-) >>> >> So sad to see this sort of thing. >> >> Can we enable this via a quirk and a Kconfig? Could default to on, but >> I'm not sure we want this code in regardless. >> >> Reviewed-by: Simon Glass <sjg@chromium.org> > > I admit that it's ugly... no clean code survives contact with a few hundred > different hardware parts. > We could add KConfig options to enable/disable those quirks but I don't see > the point of being able to. It's just a basic retry that has no impact on > things that work well already and might make other work more reliably. I'm OK with it being there. But I suggest it should be a quirk flag in struct mmc. You can always have it enabled (by default), but that way it becomes very clear that this is a work-around, since it can be disabled at run-time. Regards, Simon
On 17/05/2017 03:38, Simon Glass wrote: > Hi Jean-Jacques, > > On 15 May 2017 at 09:49, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >> >> On 15/05/2017 05:28, Simon Glass wrote: >>> On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >>>> From: Vignesh R <vigneshr@ti.com> >>>> >>>> With certain SD cards like Kingston 8GB/16GB UHS card, it is seen that >>>> MMC_CMD_ALL_SEND_CID cmd fails on first attempt, but succeeds >>>> subsequently. Therefore, retry MMC_CMD_ALL_SEND_CID cmd at least thrice >>>> as done in Linux kernel. >>>> Similarly, it is seen that MMC_CMD_SET_BLOCKLEN may fail on first >>>> attempt, therefore retry this cmd five times as done in kernel. >>>> >>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >>>> --- >>>> drivers/mmc/mmc.c | 15 ++++++++++++--- >>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>> >>> So sad to see this sort of thing. >>> >>> Can we enable this via a quirk and a Kconfig? Could default to on, but >>> I'm not sure we want this code in regardless. >>> >>> Reviewed-by: Simon Glass <sjg@chromium.org> >> I admit that it's ugly... no clean code survives contact with a few hundred >> different hardware parts. >> We could add KConfig options to enable/disable those quirks but I don't see >> the point of being able to. It's just a basic retry that has no impact on >> things that work well already and might make other work more reliably. > I'm OK with it being there. But I suggest it should be a quirk flag in > struct mmc. You can always have it enabled (by default), but that way > it becomes very clear that this is a work-around, since it can be > disabled at run-time. OK. It'll be in v2 > > Regards, > Simon >
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index c7dda64..49edf52 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -275,6 +275,8 @@ int mmc_send_status(struct mmc *mmc, int timeout) int mmc_set_blocklen(struct mmc *mmc, int len) { struct mmc_cmd cmd; + int retries = 5; + int err; if (mmc->ddr_mode) return 0; @@ -282,8 +284,13 @@ int mmc_set_blocklen(struct mmc *mmc, int len) cmd.cmdidx = MMC_CMD_SET_BLOCKLEN; cmd.resp_type = MMC_RSP_R1; cmd.cmdarg = len; + do { + err = mmc_send_cmd(mmc, &cmd, NULL); + if (!err) + break; + } while (retries--); - return mmc_send_cmd(mmc, &cmd, NULL); + return err; } static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start, @@ -1867,6 +1874,7 @@ static int mmc_startup(struct mmc *mmc) u64 cmult, csize; struct mmc_cmd cmd; struct blk_desc *bdesc; + int retries = 3; #ifdef CONFIG_MMC_SPI_CRC_ON if (mmc_host_is_spi(mmc)) { /* enable CRC check for spi */ @@ -1874,7 +1882,6 @@ static int mmc_startup(struct mmc *mmc) cmd.resp_type = MMC_RSP_R1; cmd.cmdarg = 1; err = mmc_send_cmd(mmc, &cmd, NULL); - if (err) return err; } @@ -1886,7 +1893,9 @@ static int mmc_startup(struct mmc *mmc) cmd.resp_type = MMC_RSP_R2; cmd.cmdarg = 0; - err = mmc_send_cmd(mmc, &cmd, NULL); + do { + err = mmc_send_cmd(mmc, &cmd, NULL); + } while (err && retries-- > 0); if (err) return err;