From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrian Hunter Subject: Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400 Date: Tue, 02 Dec 2014 14:28:04 +0200 Message-ID: <547DB054.80907@intel.com> References: <1417439766-5063-1-git-send-email-adrian.hunter@intel.com> <1417439766-5063-4-git-send-email-adrian.hunter@intel.com> <547D8FB2.1020400@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com ([134.134.136.24]:32381 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751211AbaLBM3l (ORCPT ); Tue, 2 Dec 2014 07:29:41 -0500 In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Ulf Hansson Cc: Chris Ball , linux-mmc On 02/12/14 13:20, Ulf Hansson wrote: > On 2 December 2014 at 11:08, Adrian Hunter wrote: >> On 02/12/14 11:35, Ulf Hansson wrote: >>> On 1 December 2014 at 14:16, Adrian Hunter wrote: >>>> Re-tuning for HS400 mode must be done in HS200 >>>> mode. Currently there is no support for that. >>>> That needs to be reflected in the code. >>>> Specifically, if tuning is executed in HS400 mode >>>> then return an error, and if the re-tuning timer >>>> is running when switching to HS400 mode, then >>>> disable the timer. >>>> >>>> Note that periodic re-tuning is not expected >>>> to be needed for HS400 but re-tuning is still >>>> needed after the host controller has lost power. >>> >>> Why can't the old values be restored instead of trigger a re-tuning? >> >> The "values" (not sure what you mean by that) are not available to the >> driver. Even if they were the operating conditions may have changed, (i.e. >> temperature change) so the old "values" could still be wrong. > > The "values" I refer to is those which we "calculated" during the > tuning process. > > What I had in mind, was that we should save these values at runtime PM > suspend. And restore them at runtime PM resume. For some mmc > controllers the "values" are typically just a some bits in a > controller register, but that might not be true for all cases. > > Regarding the temperature change, etc. I think that is what the > periodic retuning should be taken care off. > > Could you elaborate on why the "values" is not available to the driver? The "optimal sampling point" has no corresponding register or value in SDHCI. > >> >> Jedec spec. says: >> >> It is recommended to perform tuning procedure while Device wakes >> up, after sleep. >> >> SDHCI spec. says: >> >> If the Host System goes into power down mode, the Host Driver >> should stop the re-tuning timer and set the expiration flag >> to 1 when the Host System resumes from power down mode. > > I am not sure how to interpret this. Is the context about system PM or > runtime PM? I interpret it to mean any situation where the host controller loses power and therefore loses its tuning. > >> >>> >>>> In the case of suspend/resume that is not necessary >>>> because the card is fully re-initialised. That >>>> just leaves runtime suspend/resume with no support >>>> for HS400 re-tuning. >>>> >>>> Signed-off-by: Adrian Hunter >>>> --- >>>> drivers/mmc/host/sdhci.c | 16 +++++++++++++++- >>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>> index 2efa7fe..a7c9e67 100644 >>>> --- a/drivers/mmc/host/sdhci.c >>>> +++ b/drivers/mmc/host/sdhci.c >>>> @@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing) >>>> else if ((timing == MMC_TIMING_UHS_DDR50) || >>>> (timing == MMC_TIMING_MMC_DDR52)) >>>> ctrl_2 |= SDHCI_CTRL_UHS_DDR50; >>>> - else if (timing == MMC_TIMING_MMC_HS400) >>>> + else if (timing == MMC_TIMING_MMC_HS400) { >>>> ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */ >>>> + /* >>>> + * Periodic re-tuning for HS400 is not expected to be needed, so >>>> + * disable it here. >>> >>> Urgh, I don't like that the periodic tuning is handled by the host. We >>> should never had merged that. >>> >>> How about trying to move the periodic tuning to be handled by the mmc >>> core instead? >> >> I have patches for that. I hope to send them today. > > Great! Looking forward to review them! > > Kind regards > Uffe > >