From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Arend van Spriel" Subject: Re: [PATCH V2] sdhci: only reprogram retuning timer when flag is set Date: Wed, 13 Nov 2013 12:18:28 +0100 Message-ID: <52836004.8030404@broadcom.com> References: <1383821960-2533-1-git-send-email-arend@broadcom.com> <1384163395-27038-1-git-send-email-arend@broadcom.com> <528352AA.6000507@broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mms1.broadcom.com ([216.31.210.17]:2513 "EHLO mms1.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751687Ab3KMLSi (ORCPT ); Wed, 13 Nov 2013 06:18:38 -0500 In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Dong Aisheng Cc: Chris Ball , "linux-mmc@vger.kernel.org" On 11/13/2013 12:12 PM, Dong Aisheng wrote: > Hi Arend, > > On Wed, Nov 13, 2013 at 6:21 PM, Arend van Spriel wrote: >> On 11/13/2013 06:02 AM, Dong Aisheng wrote: >>> >>> Hi Arend, >>> >>> On Mon, Nov 11, 2013 at 5:49 PM, Arend van Spriel >>> wrote: >>>> >>>> When the host->tuning_count is zero it means that the >>>> retuning is disabled. This is checked on the first >>>> run of sdhci_execute_tuning() by the if statement below: >>>> >>>> if (!(host->flags & SDHCI_NEEDS_RETUNING) && host->tuning_count >>>> && >>>> (host->tuning_mode == SDHCI_TUNING_MODE_1)) { >>>> >>>> So only when tuning_count is non-zero it will set the host >>>> flag SDHCI_USING_RETUNING_TIMER. The else statement is only >>>> for re-programming the timer, which means that flag must be >>>> set. Because that is not checked the else statement is executed >>>> in the first run when tuning_count is zero. >>>> >>>> This was seen on a host controller which indicated >>>> SDHCI_TUNING_MODE_1 (0) and tuning_count being zero. Suspect >>>> that (one of) these registers is not properly set. >>>> >>>> Signed-off-by: Arend van Spriel >>>> --- >>>> This patch applies to the mmc-next branch. >>>> >>>> V2: >>>> - add more explanation to the commit message >>>> - check host flag SDHCI_USING_RETUNING_TIMER >>>> --- >>>> drivers/mmc/host/sdhci.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>> index bd8a098..5974599 100644 >>>> --- a/drivers/mmc/host/sdhci.c >>>> +++ b/drivers/mmc/host/sdhci.c >>>> @@ -2014,7 +2014,7 @@ out: >>>> host->tuning_count * HZ); >>>> /* Tuning mode 1 limits the maximum data length to 4MB >>>> */ >>>> mmc->max_blk_count = (4 * 1024 * 1024) / >>>> mmc->max_blk_size; >>>> - } else { >>>> + } else if (host->flags & SDHCI_USING_RETUNING_TIMER) { >>>> host->flags &= ~SDHCI_NEEDS_RETUNING; >>>> /* Reload the new initial value for timer */ >>>> if (host->tuning_mode == SDHCI_TUNING_MODE_1) >>> >>> >>> I wonder if we could also remove this line? >>> It looks to me it's not neccesary to check the tuning_mode again since >>> we already check the flag >>> above and SDHCI_TUNING_MODE_1 seems like the prerequisite of >>> SDHCI_USING_RETUNING_TIMER. >> >> >> According the spec the other tuning modes also can use retuning timer. >> Currently, the mmc stack in upstream linux only supports tuning mode 1. When >> adding the other modes this if statement will probably go. >> > > For currently code, it looks like also not necessary to check it since > SDHCI_USING_RETUNING_TIMER will only be set when tunning_mode is > SDHCI_TUNING_MODE_1. > And SDHCI_TUNING_MODE_1 just indicates the tuning mode while the flag > SDHCI_USING_RETUNING_TIMER represents the retuning timer implementation. > So check the flag to invoke the timer seems make more sense to me. > do you agree? The flag SDHCI_USING_RETUNING_TIMER is only set after the initial tuning run so in the if-statement. So currently in the else-statement the fact that SDHCI_USING_RETUNING_TIMER is set implies SDHCI_TUNING_MODE_1. Regards, Arend > Regards > Dong Aisheng > >> Regards, >> Arend >> >> >>> Regards >>> Dong Aisheng >>> >>>> -- >>>> 1.7.10.4 >>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >> >> >