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 13:24:38 +0100 Message-ID: <528F4D06.3080400@mentor.com> References: <1385046445-29711-1-git-send-email-vladimir_zapolskiy@mentor.com> <528F4242.8040408@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]:59451 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751956Ab3KVMYz (ORCPT ); Fri, 22 Nov 2013 07:24:55 -0500 In-Reply-To: <528F4242.8040408@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 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. >> >> 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. >> >> 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 */ >> >