From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aaron Lu Subject: Re: [PATCH V2] sdhci: only reprogram retuning timer when flag is set Date: Wed, 13 Nov 2013 21:01:26 +0800 Message-ID: <52837826.7060000@gmail.com> References: <1383821960-2533-1-git-send-email-arend@broadcom.com> <1384163395-27038-1-git-send-email-arend@broadcom.com> <528352AA.6000507@broadcom.com> <52836004.8030404@broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f50.google.com ([209.85.220.50]:38160 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751221Ab3KMNBo (ORCPT ); Wed, 13 Nov 2013 08:01:44 -0500 Received: by mail-pa0-f50.google.com with SMTP id hz1so387445pad.23 for ; Wed, 13 Nov 2013 05:01:44 -0800 (PST) In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Dong Aisheng , Arend van Spriel Cc: Chris Ball , "linux-mmc@vger.kernel.org" On 11/13/2013 07:25 PM, Dong Aisheng wrote: > On Wed, Nov 13, 2013 at 7:18 PM, Arend van Spriel wrote: >> 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. >> > > Right, so that means we could remove the tuning_mode check in the > else-statement. I agree. -Aaron > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 2d55e6a..b2928ef 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2015,12 +2015,11 @@ 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 timeout workqueue */ > - if (host->tuning_mode == SDHCI_TUNING_MODE_1) > - schedule_delayed_work(&host->tuning_timeout_work, > - host->tuning_count * HZ); > + schedule_delayed_work(&host->tuning_timeout_work, > + host->tuning_count * HZ); > } > > /* > > Regards > Dong Aisheng > >> >> 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 >>>>> >>>>> >>>>> >>>> >>>> >>> >> >> > -- > 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 >