| Message ID | CAA+hA=QELe=C852Xemh8JbvVjin75OiZM=a87y2kBRPtcpHbkw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/13/2013 07:25 PM, Dong Aisheng wrote: > On Wed, Nov 13, 2013 at 7:18 PM, Arend van Spriel <arend@broadcom.com> 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 <arend@broadcom.com> >>> 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 <arend@broadcom.com> >>>>> 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 <arend@broadcom.com> >>>>>> --- >>>>>> 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 > -- 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); } /*