| Message ID | 1383821960-2533-1-git-send-email-arend@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/07/2013 06:59 PM, Arend van Spriel wrote: > When the host->tuning_count is zero it means that the If the tuning_count is zero, then the retuning timer shouldn't be started in the first place and not possible to run code there. Or is the tuning_count dynamically changed? Thanks, Aaron > retuning is disabled. Doing a mod_timer() with a zero > tuning_count does something else. > > Signed-off-by: Arend van Spriel <arend@broadcom.com> > --- > drivers/mmc/host/sdhci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 7a7fb4f..9803e7a 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2007,7 +2007,8 @@ out: > } else { > host->flags &= ~SDHCI_NEEDS_RETUNING; > /* Reload the new initial value for timer */ > - if (host->tuning_mode == SDHCI_TUNING_MODE_1) > + if (host->tuning_count && > + host->tuning_mode == SDHCI_TUNING_MODE_1) > mod_timer(&host->tuning_timer, jiffies + > host->tuning_count * HZ); > } > -- 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/08/2013 07:50 AM, Aaron Lu wrote: > On 11/07/2013 06:59 PM, Arend van Spriel wrote: >> When the host->tuning_count is zero it means that the > > If the tuning_count is zero, then the retuning timer shouldn't be > started in the first place and not possible to run code there. Or is the > tuning_count dynamically changed? Actually, the sdhci_execute_tuning() must run once to do the initial tuning procedure. This is mandatory for SDR104. However, *re*tuning is not and a zero tuning_count disables it. The function is executed initially. The 'if' statement above the patched 'else' statement is actually responsible for programming the retuning timer for the first time. However, it requires tuning_count to be non-zero. The 'else' statement is actually for reloading the retuning timer, which is not the case. Adding the non-zero check assures the retuning timer is never started. I guess the fact that this needs explaining indicates that the commit message should be updated. I will send a V2 for this. Regards, Arend > Thanks, > Aaron > >> retuning is disabled. Doing a mod_timer() with a zero >> tuning_count does something else. >> >> Signed-off-by: Arend van Spriel <arend@broadcom.com> >> --- >> drivers/mmc/host/sdhci.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 7a7fb4f..9803e7a 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -2007,7 +2007,8 @@ out: >> } else { >> host->flags &= ~SDHCI_NEEDS_RETUNING; >> /* Reload the new initial value for timer */ >> - if (host->tuning_mode == SDHCI_TUNING_MODE_1) >> + if (host->tuning_count && >> + host->tuning_mode == SDHCI_TUNING_MODE_1) >> mod_timer(&host->tuning_timer, jiffies + >> host->tuning_count * HZ); >> } >> > > -- 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/08/2013 05:10 PM, Arend van Spriel wrote: > On 11/08/2013 07:50 AM, Aaron Lu wrote: >> On 11/07/2013 06:59 PM, Arend van Spriel wrote: >>> When the host->tuning_count is zero it means that the >> >> If the tuning_count is zero, then the retuning timer shouldn't be >> started in the first place and not possible to run code there. Or is the >> tuning_count dynamically changed? > > Actually, the sdhci_execute_tuning() must run once to do the initial > tuning procedure. This is mandatory for SDR104. However, *re*tuning is > not and a zero tuning_count disables it. So the host in question doesn't need do retuning while in SDR104 mode? It seems your host's retuning mode is mode_1. > > The function is executed initially. The 'if' statement above the patched > 'else' statement is actually responsible for programming the retuning > timer for the first time. However, it requires tuning_count to be > non-zero. The 'else' statement is actually for reloading the retuning > timer, which is not the case. Adding the non-zero check assures the > retuning timer is never started. OK, I see. The SDHCI_USING_RETUNING_TIMER flag is supposed to mean if the host is currently using retuning timer to do retuning, it could also be used to decide if retuning timer needs be re-programmed. Anyway, a host in retuning mode 1 does not have tuning_count set seems a little odd to me... Thanks, Aaron > > I guess the fact that this needs explaining indicates that the commit > message should be updated. I will send a V2 for this. > > Regards, > Arend > >> Thanks, >> Aaron >> >>> retuning is disabled. Doing a mod_timer() with a zero >>> tuning_count does something else. >>> >>> Signed-off-by: Arend van Spriel <arend@broadcom.com> >>> --- >>> drivers/mmc/host/sdhci.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index 7a7fb4f..9803e7a 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -2007,7 +2007,8 @@ out: >>> } else { >>> host->flags &= ~SDHCI_NEEDS_RETUNING; >>> /* Reload the new initial value for timer */ >>> - if (host->tuning_mode == SDHCI_TUNING_MODE_1) >>> + if (host->tuning_count && >>> + host->tuning_mode == SDHCI_TUNING_MODE_1) >>> mod_timer(&host->tuning_timer, jiffies + >>> host->tuning_count * HZ); >>> } >>> >> >> > > -- 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/08/2013 12:49 PM, Aaron Lu wrote: > On 11/08/2013 05:10 PM, Arend van Spriel wrote: >> On 11/08/2013 07:50 AM, Aaron Lu wrote: >>> On 11/07/2013 06:59 PM, Arend van Spriel wrote: >>>> When the host->tuning_count is zero it means that the >>> >>> If the tuning_count is zero, then the retuning timer shouldn't be >>> started in the first place and not possible to run code there. Or is the >>> tuning_count dynamically changed? >> >> Actually, the sdhci_execute_tuning() must run once to do the initial >> tuning procedure. This is mandatory for SDR104. However, *re*tuning is >> not and a zero tuning_count disables it. > > So the host in question doesn't need do retuning while in SDR104 mode? > It seems your host's retuning mode is mode_1. > >> >> The function is executed initially. The 'if' statement above the patched >> 'else' statement is actually responsible for programming the retuning >> timer for the first time. However, it requires tuning_count to be >> non-zero. The 'else' statement is actually for reloading the retuning >> timer, which is not the case. Adding the non-zero check assures the >> retuning timer is never started. > > OK, I see. > The SDHCI_USING_RETUNING_TIMER flag is supposed to mean if the host is > currently using retuning timer to do retuning, it could also be used to > decide if retuning timer needs be re-programmed. True. I can go for that approach. > Anyway, a host in retuning mode 1 does not have tuning_count set seems a > little odd to me... Looking at sdhci.h it actually seems sdhci code is missing support for the other modes: unsigned int tuning_count; /* Timer count for re-tuning */ unsigned int tuning_mode; /* Re-tuning mode supported by host */ #define SDHCI_TUNING_MODE_1 0 struct timer_list tuning_timer; /* Timer for tuning */ At least the other modes are not defined here. Regards, Arend > Thanks, > Aaron > >> >> I guess the fact that this needs explaining indicates that the commit >> message should be updated. I will send a V2 for this. >> >> Regards, >> Arend >> >>> Thanks, >>> Aaron >>> >>>> retuning is disabled. Doing a mod_timer() with a zero >>>> tuning_count does something else. >>>> >>>> Signed-off-by: Arend van Spriel <arend@broadcom.com> >>>> --- >>>> drivers/mmc/host/sdhci.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>> index 7a7fb4f..9803e7a 100644 >>>> --- a/drivers/mmc/host/sdhci.c >>>> +++ b/drivers/mmc/host/sdhci.c >>>> @@ -2007,7 +2007,8 @@ out: >>>> } else { >>>> host->flags &= ~SDHCI_NEEDS_RETUNING; >>>> /* Reload the new initial value for timer */ >>>> - if (host->tuning_mode == SDHCI_TUNING_MODE_1) >>>> + if (host->tuning_count && >>>> + host->tuning_mode == SDHCI_TUNING_MODE_1) >>>> mod_timer(&host->tuning_timer, jiffies + >>>> host->tuning_count * HZ); >>>> } >>>> >>> >>> >> >> > > -- 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/08/2013 07:55 PM, Arend van Spriel wrote: > On 11/08/2013 12:49 PM, Aaron Lu wrote: >> On 11/08/2013 05:10 PM, Arend van Spriel wrote: >>> On 11/08/2013 07:50 AM, Aaron Lu wrote: >>>> On 11/07/2013 06:59 PM, Arend van Spriel wrote: >>>>> When the host->tuning_count is zero it means that the >>>> >>>> If the tuning_count is zero, then the retuning timer shouldn't be >>>> started in the first place and not possible to run code there. Or is the >>>> tuning_count dynamically changed? >>> >>> Actually, the sdhci_execute_tuning() must run once to do the initial >>> tuning procedure. This is mandatory for SDR104. However, *re*tuning is >>> not and a zero tuning_count disables it. >> >> So the host in question doesn't need do retuning while in SDR104 mode? >> It seems your host's retuning mode is mode_1. >> >>> >>> The function is executed initially. The 'if' statement above the patched >>> 'else' statement is actually responsible for programming the retuning >>> timer for the first time. However, it requires tuning_count to be >>> non-zero. The 'else' statement is actually for reloading the retuning >>> timer, which is not the case. Adding the non-zero check assures the >>> retuning timer is never started. >> >> OK, I see. >> The SDHCI_USING_RETUNING_TIMER flag is supposed to mean if the host is >> currently using retuning timer to do retuning, it could also be used to >> decide if retuning timer needs be re-programmed. > > True. I can go for that approach. > >> Anyway, a host in retuning mode 1 does not have tuning_count set seems a >> little odd to me... > > Looking at sdhci.h it actually seems sdhci code is missing support for > the other modes: > > unsigned int tuning_count; /* Timer count for re-tuning */ > unsigned int tuning_mode; /* Re-tuning mode supported by host */ > #define SDHCI_TUNING_MODE_1 0 > struct timer_list tuning_timer; /* Timer for tuning */ > > At least the other modes are not defined here. Right, only retuning mode 1 is implemented currently. Thanks, Aaron > > Regards, > Arend > >> Thanks, >> Aaron >> >>> >>> I guess the fact that this needs explaining indicates that the commit >>> message should be updated. I will send a V2 for this. >>> >>> Regards, >>> Arend >>> >>>> Thanks, >>>> Aaron >>>> >>>>> retuning is disabled. Doing a mod_timer() with a zero >>>>> tuning_count does something else. >>>>> >>>>> Signed-off-by: Arend van Spriel <arend@broadcom.com> >>>>> --- >>>>> drivers/mmc/host/sdhci.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>>> index 7a7fb4f..9803e7a 100644 >>>>> --- a/drivers/mmc/host/sdhci.c >>>>> +++ b/drivers/mmc/host/sdhci.c >>>>> @@ -2007,7 +2007,8 @@ out: >>>>> } else { >>>>> host->flags &= ~SDHCI_NEEDS_RETUNING; >>>>> /* Reload the new initial value for timer */ >>>>> - if (host->tuning_mode == SDHCI_TUNING_MODE_1) >>>>> + if (host->tuning_count && >>>>> + host->tuning_mode == SDHCI_TUNING_MODE_1) >>>>> mod_timer(&host->tuning_timer, jiffies + >>>>> host->tuning_count * HZ); >>>>> } >>>>> >>>> >>>> >>> >>> >> >> > > -- 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 7a7fb4f..9803e7a 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2007,7 +2007,8 @@ out: } else { host->flags &= ~SDHCI_NEEDS_RETUNING; /* Reload the new initial value for timer */ - if (host->tuning_mode == SDHCI_TUNING_MODE_1) + if (host->tuning_count && + host->tuning_mode == SDHCI_TUNING_MODE_1) mod_timer(&host->tuning_timer, jiffies + host->tuning_count * HZ); }
When the host->tuning_count is zero it means that the retuning is disabled. Doing a mod_timer() with a zero tuning_count does something else. Signed-off-by: Arend van Spriel <arend@broadcom.com> --- drivers/mmc/host/sdhci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)