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
