From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Zapolskiy Subject: Re: [PATCH] mmc: sdhci: don't limit discard timeout by data line timeout Date: Fri, 22 Nov 2013 16:21:20 +0100 Message-ID: <528F7670.9040101@mentor.com> References: <1385046445-29711-1-git-send-email-vladimir_zapolskiy@mentor.com> <528F4242.8040408@intel.com> <528F4D06.3080400@mentor.com> <528F5663.2050800@intel.com> <528F6113.2070009@mentor.com> <528F728D.3040004@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from relay1.mentorg.com ([192.94.38.131]:45776 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753034Ab3KVPVg (ORCPT ); Fri, 22 Nov 2013 10:21:36 -0500 In-Reply-To: <528F728D.3040004@intel.com> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Adrian Hunter Cc: linux-mmc@vger.kernel.org, Ed Sutter , Chris Ball On 22.11.2013 16:04, Adrian Hunter wrote: > On 22/11/13 15:50, Vladimir Zapolskiy wrote: >> On 22.11.2013 14:04, Adrian Hunter wrote: >>> On 22/11/13 14:24, Vladimir Zapolskiy wrote: >>>> On 22.11.2013 12:38, Adrian Hunter wrote: >>>>> On 21/11/13 17:07, Vladimir Zapolskiy wrote: >>>>>> JEDEC specification defines quite high erase timeout value for 300ms >>>>>> multiplied by erase group number, and SD Host Controller specification >>>>>> data line timeout may be much less, e.g. 2^13 / 52MHz ~ 160us. >>>>>> >>>>>>> From block layer and MMC perfromance perspective it is desirable that >>>>>> millions of erase groups are discarded at once, so there is no much >>>>>> sense to limit maximum erase timeout by data line timeout, if a >>>>>> controller handles correctly erase operation without indication of >>>>>> data line timeout. >>>>> >>>>> Would you explain that some more. Do you mean that: >>>>> a) it does not have a timeout >>>> >>>> JEDEC defines a timeout on erase/trim operations, also in >>>> drivers/mmc/core/core.c >>>> there is a reasonable enough 10 minutes limitation for discard operations. >>>> >>>>> b) it has a timeout which is less than the timeout specified by the >>>>> standard but the operation nevertheless completes >>>> >>>> SDHC data line timeout is enormously less than erase group timeout, and >>>> trivial testing shows that those two timeouts are independent, probably >>>> except some particular cases of controllers not known before commits >>>> 58d1246db3 and e056a1b5b. >>>> >>>> According to the currently implemented logic, mmc_do_erase() commonly is >>>> instructed to discard 1-2 erase groups at maximum, however it tends to be >>>> capable to successfully discard millions of erase groups at once ignoring >>>> that SDHC data line timeout limitation. >>>> >>> >>> You seem to be trying to say that the SDHCI spec. says that the host >>> controller does not timeout erase operations or uses a different timeout >>> than the one programmed in the "Timeout Control Register". Where is that is >>> the SDHCI spec? >> >> According to the spec a host controller timeouts erase operations like any >> other R1B command. >> >> So in your opinion, should there be SDHCI_QUIRK_BROKEN_TIMEOUT_VAL instead >> of the new quirk? > > I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help. It just > sets the timeout to maximum but max_discard_to is the maximum timeout. Here I meant to do something like: if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL) mmc->max_discard_to = 0; Again I'm not sure that this applies well to all SDHCI_QUIRK_BROKEN_TIMEOUT_VAL controllers, therefore a new quirk might be better. > As I understand it you don't want to limit the discard size, either because > your controller does not timeout, or because you are happy that the maximum > timeout is enough for your users and their use-cases. > > If that is the case then the original patch just needs the quirk the other > way around. i.e. > > if (host->quirks2& SDHCI_QUIRK2_NO_DISCARD_LIMIT) > mmc->max_discard_to = 0; > else > mmc->max_discard_to = (1<< 27) / host->timeout_clk; This suits me fine, thanks for review, and I'll resend a change based on this. Also I'd like to pay your attention to (1 << 27) / host->timeout_clk part of calculation, following the spec it might be better to account the actual value of Data Timeout Counter, otherwise a controller may get unintentional Data Timeout Error pretty soon. Please correct me, if I'm mistaken here. >> >>>>>> >>>>>> Potentially the change may break some of the SDHCs on discard of mmc, >>>>>> and for backward compatibility a new quirk is introduced, which is NOT >>>>>> set by default. >>>>> >>>>> It sounds to me that what you want to do is not standard so the quirk >>>>> should >>>>> be the other way around. >>>> >>>> Please take a look at commits 58d1246db3 and e056a1b5b, I'd be glad, if you >>>> could elaborate to which "some host controllers" the quirk in my definition >>>> applies, I believe all other host controllers present at that time in >>>> drivers/mmc/host/* are capable to discard without introduced limitation. >>> >>> "some host controllers" == SDHCI i.e. to all of the ones you are applying >>> the change. >>> >>>> >>>>>> >>>>>> Signed-off-by: Vladimir Zapolskiy >>>>>> Reported-by: Ed Sutter >>>>>> Cc: Chris Ball >>>>>> Cc: Adrian Hunter >>>>>> --- >>>>>> drivers/mmc/host/sdhci.c | 5 ++++- >>>>>> include/linux/mmc/sdhci.h | 1 + >>>>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>>>> index bd8a098..b1fdddb 100644 >>>>>> --- a/drivers/mmc/host/sdhci.c >>>>>> +++ b/drivers/mmc/host/sdhci.c >>>>>> @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host) >>>>>> if (host->quirks& SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) >>>>>> host->timeout_clk = mmc->f_max / 1000; >>>>>> >>>>>> - mmc->max_discard_to = (1<< 27) / host->timeout_clk; >>>>>> + if (host->quirks2& SDHCI_QUIRK2_DATA_TIMEOUT_ON_DISCARD) >>>>>> + mmc->max_discard_to = (1<< 27) / host->timeout_clk; >>>>>> + else >>>>>> + mmc->max_discard_to = 0; >>>>>> >>>>>> mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; >>>>>> >>>>>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h >>>>>> index 3e781b8..e7f6bd2 100644 >>>>>> --- a/include/linux/mmc/sdhci.h >>>>>> +++ b/include/linux/mmc/sdhci.h >>>>>> @@ -98,6 +98,7 @@ struct sdhci_host { >>>>>> #define SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON (1<<4) >>>>>> /* Controller has a non-standard host control register */ >>>>>> #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL (1<<5) >>>>>> +#define SDHCI_QUIRK2_DATA_TIMEOUT_ON_DISCARD (1<<6) >>>>>> >>>>>> int irq; /* Device IRQ */ >>>>>> void __iomem *ioaddr; /* Mapped address */ >>>>>> >>>>> >>>> >>>> >>>