From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Tue, 5 Mar 2019 07:10:35 +0000 Subject: [U-Boot] [PATCH v1] mmc: dwmmc: Enable small delay before returning error In-Reply-To: <647e5b79-ddce-73f3-da72-142bc4d76bd2@denx.de> References: <1550463365-28190-1-git-send-email-chee.hong.ang@intel.com> <1550501506.30501.22.camel@intel.com> <1550671074.41995.5.camel@intel.com> <2ce56c7d-f023-0b89-3188-75a0c2f3f0de@denx.de> <1550848755.13281.12.camel@intel.com> <647e5b79-ddce-73f3-da72-142bc4d76bd2@denx.de> Message-ID: <1551769834.18377.4.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Fri, 2019-02-22 at 17:02 +0100, Marek Vasut wrote: > On 2/22/19 4:19 PM, Ang, Chee Hong wrote: > > > > 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" > > > > > > > > > > > > > > > > '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/dbe70c7d4e3d5c705a98d > > > > > > 8295 > > > > > > 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. > Does the SD or MMC JEDEC spec say something about such delay? I checked the SD/MMC JEDEC. Nothing about this delay is mentioned in the spec. > > > > > 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. > So why is there that 100 uS delay ? Does the Linux DWMMC driver have > such a delay too? Linux DWMMC has only the delay for card detection and I think is quite common because this is needed to make sure the memory card is properly inserted before any operations can be started. > > > > > 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 > > Expanding the CC to other DWMMC users, maybe their datasheets contain > something about the 100 uS delay. > > > > > > > > > > > > > > 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. > Hmmmm, try applying > > [PATCH V2] ARM: socfpga: Clear PL310 early in SPL > > I wonder if this might be what you're hitting. Nope. This patch only apply to Aaria 10 and this problem is found on Stratix 10 platform. >