diff mbox series

mmc: core: retry CMD1 in mmc_send_op_cond() even if the ocr = 0

Message ID 1553764807-5778-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State New, archived
Headers show
Series mmc: core: retry CMD1 in mmc_send_op_cond() even if the ocr = 0 | expand

Commit Message

Yoshihiro Shimoda March 28, 2019, 9:20 a.m. UTC
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(-)

Comments

Wolfram Sang April 3, 2019, 1:13 p.m. UTC | #1
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
Yoshihiro Shimoda April 9, 2019, 10:48 a.m. UTC | #2
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 mbox series

Patch

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))