| Message ID | 1553764807-5778-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | mmc: core: retry CMD1 in mmc_send_op_cond() even if the ocr = 0 | expand |
Hi Shimoda-san, thank you for this bugfix! Looks like a nice catch. > + > + /* > + * According to eMMC specification, we should issue CMD1 Maybe it is good to specify where? I found it in the standard v5.1, section 6.4.3. > + * repeatidly in the idle state until the eMMC is ready even if "repeatedly" > + * the mmc_attach_mmc() calls this function with ocr = 0. I would suggest to remove the "if ..." part from the comment. Because you remove the 'if (ocr == 0)' code, so it becomes less obvious what that part of the comment relates to. > + * Otherwise some eMMC devices seem to enter the inactive mode > + * after mmc_init_card() issued CMD0 when the eMMC device is > + * busy. > + */ Other than that, good documentation! > + if (!ocr && !mmc_host_is_spi(host)) > + cmd.arg = cmd.resp[0] | BIT(30); I think BIT(30) is okay for this bugfix. But as a follow up, we should turn this into a define. It could be used in mmc_init_card() as well and will make the code way more readable. All the best, Wolfram
Hi Wolfram-san, Thank you for your review! > From: Wolfram Sang, Sent: Wednesday, April 3, 2019 10:13 PM > > Hi Shimoda-san, > > thank you for this bugfix! Looks like a nice catch. > > > + > > + /* > > + * According to eMMC specification, we should issue CMD1 > > Maybe it is good to specify where? I found it in the standard v5.1, > section 6.4.3. I got it. I'll add such a comment. > > + * repeatidly in the idle state until the eMMC is ready even if > > "repeatedly" Oops. I'll revise it. > > + * the mmc_attach_mmc() calls this function with ocr = 0. > > I would suggest to remove the "if ..." part from the comment. Because > you remove the 'if (ocr == 0)' code, so it becomes less obvious what > that part of the comment relates to. I got it. I'll remove it. > > + * Otherwise some eMMC devices seem to enter the inactive mode > > + * after mmc_init_card() issued CMD0 when the eMMC device is > > + * busy. > > + */ > > Other than that, good documentation! > > > + if (!ocr && !mmc_host_is_spi(host)) > > + cmd.arg = cmd.resp[0] | BIT(30); > > I think BIT(30) is okay for this bugfix. But as a follow up, we should > turn this into a define. It could be used in mmc_init_card() as well and > will make the code way more readable. I think so. I found hardcoded value in ./drivers/mmc/core like below (I don't check all the lines are related to the bit though): $ git grep -e "1 << 30" -e "BIT(30)" ./drivers/mmc/core/ drivers/mmc/core/mmc.c: err = mmc_send_op_cond(host, ocr | (1 << 30), &rocr); drivers/mmc/core/mmc.c: if (rocr & BIT(30)) drivers/mmc/core/mmc_ops.c: cmd.arg = highcap ? (1 << 30) : 0; drivers/mmc/core/sd_ops.c: cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */ Best regards, Yoshihiro Shimoda > All the best, > > Wolfram
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c index c5208fb..d068eed 100644 --- a/drivers/mmc/core/mmc_ops.c +++ b/drivers/mmc/core/mmc_ops.c @@ -184,11 +184,7 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) if (err) break; - /* if we're just probing, do a single pass */ - if (ocr == 0) - break; - - /* otherwise wait until reset completes */ + /* wait until reset completes */ if (mmc_host_is_spi(host)) { if (!(cmd.resp[0] & R1_SPI_IDLE)) break; @@ -200,6 +196,17 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) err = -ETIMEDOUT; mmc_delay(10); + + /* + * According to eMMC specification, we should issue CMD1 + * repeatidly in the idle state until the eMMC is ready even if + * the mmc_attach_mmc() calls this function with ocr = 0. + * Otherwise some eMMC devices seem to enter the inactive mode + * after mmc_init_card() issued CMD0 when the eMMC device is + * busy. + */ + if (!ocr && !mmc_host_is_spi(host)) + cmd.arg = cmd.resp[0] | BIT(30); } if (rocr && !mmc_host_is_spi(host))
According to eMMC specification, we should issue CMD1 repeatidly in the idle state until the eMMC is ready even if the mmc_attach_mmc() calls this function with ocr = 0. Otherwise some eMMC devices seems to enter the inactive mode after mmc_init_card() issued CMD0 when the eMMC device is busy. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- We can reproduce this issue if: - we add no-sd and no-sdio property into sdhi2 node of salvator-common.dtsi, - the renesas_sdhi driver is kernel module, - Using a Salvator-XS board that has Samsung eMMC device, - enter suspend and exit it, - and then insmod renesas_sdhi_{core,internal_dmac}.ko. After that, the following error happened and any partitions are not detected. mmc1: error -110 whilst initialising MMC card I tested this patch on: - M3-N Salvator-XS with a Samsung eMMC device, - M3-W Salvator-X with SiliconMotion eMMC device, - and M3-W Starter Kit with Micron eMMC device. drivers/mmc/core/mmc_ops.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)