| Message ID | 1384163395-27038-1-git-send-email-arend@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/11/2013 05: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 <arend@broadcom.com> Reviewed-by: Aaron Lu <aaron.lu@intel.com> -Aaron > --- > 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) > -- 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
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. 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
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. 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
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? 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
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. 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 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)
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(-)