From patchwork Wed Nov 13 11:25:54 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dong Aisheng X-Patchwork-Id: 3177951 Return-Path: X-Original-To: patchwork-linux-mmc@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 674F89F43F for ; Wed, 13 Nov 2013 11:26:03 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AC1B9205F1 for ; Wed, 13 Nov 2013 11:25:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 658A4204B5 for ; Wed, 13 Nov 2013 11:25:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758761Ab3KMLZ4 (ORCPT ); Wed, 13 Nov 2013 06:25:56 -0500 Received: from mail-qe0-f53.google.com ([209.85.128.53]:48908 "EHLO mail-qe0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758744Ab3KMLZ4 (ORCPT ); Wed, 13 Nov 2013 06:25:56 -0500 Received: by mail-qe0-f53.google.com with SMTP id cy11so132353qeb.12 for ; Wed, 13 Nov 2013 03:25:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=225yYZTPX/qHrUwrfP3iKhlshy1qX2r8+zlySWwVjCk=; b=WpbdjbDeOGEwFKtMj6mdwFRwh43cXijz8XwL76ozDf3k8b2zXZNsQRi833wpcZ0Tyq SUB1RCOkMaRVIpNHUia/Zkjyiepmbj+blG3LnX+j5VM+PwgfUgWwsOjOqSwC3byRcw2B Gr8Ia3dM9vqLGljdVVsd12m9FULTYCnJj9B0sG14oseFCYEPrYIh2u+2Zw6oJXLTA/yJ +EH3+cQSZDRofFGs4AuOkLdr/i8nWUYOo4FFv3wEfHWm9tsFNTmJyJj7NB5yd9nF+mDe WcDiNjzTotMe51+9S2iXwh8mc24UEdTelunr5lhHY8jkMGXWoefMUE1CRZzhE3mFszSG hZ6g== MIME-Version: 1.0 X-Received: by 10.224.131.67 with SMTP id w3mr67369482qas.62.1384341954372; Wed, 13 Nov 2013 03:25:54 -0800 (PST) Received: by 10.140.84.135 with HTTP; Wed, 13 Nov 2013 03:25:54 -0800 (PST) In-Reply-To: <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> <52836004.8030404@broadcom.com> Date: Wed, 13 Nov 2013 19:25:54 +0800 Message-ID: Subject: Re: [PATCH V2] sdhci: only reprogram retuning timer when flag is set From: Dong Aisheng To: Arend van Spriel Cc: Chris Ball , "linux-mmc@vger.kernel.org" Sender: linux-mmc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. 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 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); } /*