[U-Boot] [PATCH v1] mmc: dwmmc: Enable small delay before returning error

[Ang, Chee Hong <chee.hong.ang@intel.com>]

On Thu, 2019-02-21 at 11:06 +0100, Marek Vasut wrote:
> On 2/20/19 2:57 PM, Ang, Chee Hong wrote:
> > 
> > On Mon, 2019-02-18 at 21:38 +0100, Marek Vasut wrote:
> > > 
> > > On 2/18/19 3:51 PM, Ang, Chee Hong wrote:
> > > > 
> > > > 
> > > > On Mon, 2019-02-18 at 12:57 +0100, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 2/18/19 5:16 AM, chee.hong.ang at intel.com wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > > > > > 
> > > > > > 'SET_BLOCKLEN' may occasionally fail on first attempt.
> > > > > Why ?
> > > > This is part of the workaround of mmc driver which is enabled
> > > > with
> > > > 'CONFIG_MMC_QUIRKS' config.
> > > > https://github.com/u-boot/u-boot/blob/dbe70c7d4e3d5c705a98d8295
> > > > 2e05
> > > > a591
> > > > efd0683/drivers/mmc/mmc.c#L272
> > > OK, let's take a step back. What problem do you observe, that
> > > you're
> > > trying to fix ?
> > See my detail explanation below.
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > This patch enable a small delay in dwmci_send_cmd() on
> > > > > > busy, I/O or CRC error to allow the MMC controller recovers
> > > > > > from the failure/error on subsequent retries.
> > > > > Why 100 uS ?
> > > > This is a good question. Perhaps the driver's authors can
> > > > explain
> > > > this.
> > > > The driver delay 100us after dwmci_send_cmd() success with the
> > > > command
> > > > sent. But it never delay 100us whenever mmc controller response
> > > > to
> > > > the
> > > > command sent with I/O or CRC errors. MMC drivers expect the
> > > > first
> > > > 'SET_BLOCKEN' command issued by dwmci_send_cmd() may fail
> > > > intermittently so it will retry up to 4 times before it gave up
> > > > and
> > > > return error. Without this 100us delay after error response,
> > > > 'SET_BLOCKEN' may sometime fail in 4 attempts in a row.
> > > > Therefore,
> > > > we
> > > > encountered intermittent failure in loading u-boot (SSBL) from
> > > > MMC.
> > > Can you be more specific about the failure you saw ?
> > Here are the steps for booting u-boot (SSBL) from MMC:
> > 
> > 1) SPL (FSBL) get loaded to OCRAM
> > 2) SPL try to load uboot as SSBL from MMC
> > 3) SPL issue 'SET_BLOCKEN' to MMC controller
> s/SPL/MMC framework via DWMMC driver/
Yes. It is done in drivers/mmc/mmc.c (Line 276)
> 
> > 
> > 4) If MMC controller response with error continue to step 5 else go
> > to
> > step 7
> > 5) If is less than 4 attempts go to step 3 else continue to step 6
> > 6) Indicate to user there was an error loading uboot as SSBL from
> > MMC
> > and stop here
> > 7) Send command to read blocks of data from MMC
> > 8) uboot successfully loaded to DDR memory
> > 9) SPL jumps to uboot and continue the boot flow.
> > 
> > So this patch actually introduce a small delay at step 4. Without
> > this
> > small delay, step 4 might sometime fail in all 4 attempts to issue
> > the
> > 'SET_BLOCKEN' command to MMC controller.
> Isn't what you observe here some sort of timeout which you need to
> respect when the card rejects a command ? If so, such timeout should
> be
> described in either of the SD or MMC specification and it should be
> in
> the MMC framework core instead of a driver.
I don't have chance to test other MMC drivers and I am not sure other
MMC drivers need such small delay for next retry if the card reject
this command. This DW MMC driver already enforce a small delay even the
command is success, so this makes me think that this 'time out' issue
is only specific to this DW MMC driver.
Is it necessary to 'enforce' this small delay in common MMC framework
(affecting all other MMC drivers as well) as shown below:

drivers/mmc/mmc.c

#ifdef CONFIG_MMC_QUIRKS
	if (err && (mmc->quirks & MMC_QUIRK_RETRY_SET_BLOCKLEN)) {
		int retries = 4;
		/*
		 * It has been seen that SET_BLOCKLEN may fail on the
first
		 * attempt, let's try a few more time
		 */
		do {
			err = mmc_send_cmd(mmc, &cmd, NULL);
			if (!err)
				break;
+			/* Enforce small delay before retry */
+			udelay(100);
		} while (retries--);
	}
#endif

> 
> > 
> > Note that this failure is
> > intermittent. If it fails at step 3 and there is no small delay in
> > step
> > 4, it will most likely fail in subsequent attempts.
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > Can we rather detect whether the controller recovered by
> > > > > polling
> > > > > some
> > > > > bit?
> > > > Hmmm...I can take a look at the databook of this controller and
> > > > dig
> > > > further. Probably this is the limitation of the controller
> > > > itself.
> > > > The
> > > > mmc driver code which introduce 100us delay after every command
> > > > sent in
> > > > dwmci_send_cmd() looks iffy.
> > > If you have access to it, please do.
> > Unfortunately, I can't find any registers I can poll for the status
> > of
> > the readiness of the controller from DesignWare storage controller
> > databook.
> > > 
> > > 
> > > btw do you have the same problem if you disable caches ?
> > The error happen while SPL trying to load uboot as SSBL from MMC
> > into
> > DDR memory.At this point, the caches are disabled.
> I-Cache is enabled in SPL, D-cache _might_ be too.
> 
> Do you get this error after cold boot too ?
Yes. It happened after cold boot too.
>