From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaehoon Chung Date: Mon, 28 Jan 2013 16:16:32 +0900 Subject: [U-Boot] [RFC] mmc:fix: Increase the timeout value for SDHCI_send_command() In-Reply-To: <20130128080220.4ff50ede@amdc308.digital.local> References: <1357665792-8141-1-git-send-email-l.majewski@samsung.com> <20130109201201.5F4E9202B9C@gemini.denx.de> <20130111161929.6ecd75e9@amdc308.digital.local> <510323EE.8070100@samsung.com> <20130128080220.4ff50ede@amdc308.digital.local> Message-ID: <510625D0.2020100@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Lukasz, On 01/28/2013 04:02 PM, Lukasz Majewski wrote: > Hi Jaehoon, > >> On 01/25/2013 08:44 PM, Jagan Teki wrote: >>> Hi, >>> >>> On Fri, Jan 11, 2013 at 8:49 PM, Lukasz Majewski >>> wrote: >>>> Hi Wolfgang, >>>> >>>>> Dear Lukasz Majewski, >>>>> >>>>> In message >>>>> <1357665792-8141-1-git-send-email-l.majewski@samsung.com> you >>>>> wrote: >>>>>> I'd like to ask for your opinion about the following problem: >>>>> >>>>> I cannot comment on the problem - only a bit about the proposed >>>>> patch ;-) >>>>> >>>>>> From a brief checking I can say that it happens when we are doing >>>>>> consecutive MMC operations (i.e. many reads), and the 10ms >>>>>> timeout might be too short when eMMC firmware is forced to do >>>>>> some internal time consuming operations (e.g. flash blocks >>>>>> management, wear leveling). >>>>>> In this situation, the SDHCI_CMD_INHIBIT bit is set, which means >>>>>> that SDHCI controller didn't received response from eMMC. >>>>>> >>>>>> One proposition would be to define the per device/per memory chip >>>>>> specific timeouts, to replace those defined >>>>>> at ./drivers/mmc/sdhci.c file. >>>>> >>>>> Is there no way to ask the device and/or controller when it is >>>>> done, so we can poll for ready state instead of adding delays, >>>>> which will always have to be tailored for the so far known worst >>>>> case, i. e. they will be always too long on all almost all >>>>> systems. >>>> >>>> We are doing this already - the SDHCI_PRESENT_STATE register's bit >>>> 0 (SDHCI_CMD_INHIBIT) and bit 1 (DATA_INHIBIT) are for this >>>> purpose. Those indicate when host controller can send further >>>> command/data to the card. >>>> >>>> Moreover, there are also timeouts in the case when someone pull >>>> out SD card inserted to the slot (or any other use case which I'm >>>> not aware). >>>> >>>> >>>>> >>>>>> --- a/drivers/mmc/sdhci.c >>>>>> +++ b/drivers/mmc/sdhci.c >>>>>> @@ -137,8 +137,8 @@ int sdhci_send_command(struct mmc *mmc, >>>>>> struct mmc_cmd *cmd, unsigned int timeout, start_addr = 0; >>>>>> unsigned int retry = 10000; >>>>>> >>>>>> - /* Wait max 10 ms */ >>>>>> - timeout = 10; >>>>>> + /* Wait max 100 ms */ >>>>>> + timeout = 100; >>>>> >>>>> We have cases where we struggle for sub-second boot times. Adding >>>>> 100 ms delay here is clearly prohbitive. [Even the 10 ms are way >>>>> too long IMHO.] There must be a better way to handle this. >>>> >>>> That's why I'm asking. >>>> >>>> It is strange that, when I'm increasing delay it works. >>>> >>>> Maybe we will find some areas of optimization? >>> >>> BTW: I am also finding the similar issue. >>> >>> But when I enabled CONFIG_MMC_TRACE for log traces, i never see the >>> issue..it's pretty much working fine. >> It's not important to enable the MMC_TRACE. It should be increased >> the delay. > > You don't see problem, since CONFIG_MMC_TRACE causes extra delays to > write log information to serial console. Right...So i mentioned that CONFIG_MMC_TRACE isn't important.(added delay). > >>> As per my latest debug, the issue is fire for CMD6 (SWITCH_FUNC). >> Right, i also find the error log for CMD6. >> Could you test this point? >> >> sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS); >> - mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT; >> + mask = SDHCI_CMD_INHIBIT; >> + >> + if ((data != NULL) || (cmd->resp_type & MMC_RSP_BUSY)) >> + mask |= SDHCI_DATA_INHIBIT; > > I've tested it on trats, but mentioned errors also appear (with lower > frequency though). Right..But I think that this patch is correct regardless of this problem. if you agree this, i will send the patch with this. > > >> >> Best Regards, >> Jaehoon Chung >>> >>> May be we need to update the logic on this while loop... how about using get_timer()? Best Regards, Jaehoon Chung > > Yep. > >>> >>> Thanks, >>> Jagan. >>> >>>> >>>>> >>>>> Best regards, >>>>> >>>>> Wolfgang Denk >>>>> >>>> >>>> >>>> >>>> -- >>>> Best regards, >>>> >>>> Lukasz Majewski >>>> >>>> Samsung R&D Poland (SRPOL) | Linux Platform Group >>>> _______________________________________________ >>>> U-Boot mailing list >>>> U-Boot at lists.denx.de >>>> http://lists.denx.de/mailman/listinfo/u-boot >>> _______________________________________________ >>> U-Boot mailing list >>> U-Boot at lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >>> >> > > >