| Message ID | 1482213199-29152-3-git-send-email-riteshh@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 2016/12/20 13:53, Ritesh Harjani wrote: > Currently mmc_select_hs400es is not setting the desired frequency > before sending switch command. This makes CMD13 to timeout on > some controllers. > Thus add a change to set the desired HS400 frequency > in mmc_select_hs400es when the timing is switched to HS400. What is the desired HS400 freq? I guess it is 200MHz, right? Well, per the spec, it just say "host *may* changes frequency to <= 200Mhz". So before setting timing to MMC_TIMING_MMC_HS400, we already reach ext_csd.hs_max_dtr. So it should meet the spec. If the fact that some controllers would see CMD13 timeout here, I guess you wouldn't let folks add max-frequency to the DT as if the max freq is set to just lower than your *desired HS400 freq* , it the same failure result even you add this patch, right? the root cause the controllers see failure for CMD13 is it didn't configure the right delay phase or something similar to fit the timing under the freq of hs_max_dtr, I guess. > > Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> > --- > drivers/mmc/core/mmc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index b61b52f9..eb69497 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1329,6 +1329,7 @@ static int mmc_select_hs400es(struct mmc_card *card) > > /* Set host controller to HS400 timing and frequency */ > mmc_set_timing(host, MMC_TIMING_MMC_HS400); > + mmc_set_bus_speed(card); > > /* Controller enable enhanced strobe function */ > host->ios.enhanced_strobe = true; >
Hi Shawn, On 12/20/2016 12:59 PM, Shawn Lin wrote: > On 2016/12/20 13:53, Ritesh Harjani wrote: >> Currently mmc_select_hs400es is not setting the desired frequency >> before sending switch command. This makes CMD13 to timeout on >> some controllers. >> Thus add a change to set the desired HS400 frequency >> in mmc_select_hs400es when the timing is switched to HS400. > > What is the desired HS400 freq? I guess it is 200MHz, right? > > Well, per the spec, it just say "host *may* changes frequency to > <= 200Mhz". So before setting timing to MMC_TIMING_MMC_HS400, we > already reach ext_csd.hs_max_dtr. So it should meet the spec. If > the fact that some controllers would see CMD13 timeout here, I guess > you wouldn't let folks add max-frequency to the DT as if the max freq > is set to just lower than your *desired HS400 freq* , it the same > failure result even you add this patch, right? > > the root cause the controllers see failure for CMD13 is it didn't > configure the right delay phase or something similar to fit the timing > under the freq of hs_max_dtr, I guess. Actually this may be limitation of sdhc msm controller that it cannot work at lower frequency while in HS400 mode. I would have to see more on this to confirm on why this limitation is there (mostly it was with some DLL limitation). In that case maybe I should not generalize this for other controllers. But still the below code change to set the bus_speed after setting the timing should be fine right ? Since host controller may as well change the frequency to 200MHz. Do you think the below change is fine in this sense? I can change the commit message to just - "set both timing mode and frequency before sending CMD13 in mmc_select_hs400es." Or do you see any other approach to this ? Regards Ritesh > >> >> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >> --- >> drivers/mmc/core/mmc.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >> index b61b52f9..eb69497 100644 >> --- a/drivers/mmc/core/mmc.c >> +++ b/drivers/mmc/core/mmc.c >> @@ -1329,6 +1329,7 @@ static int mmc_select_hs400es(struct mmc_card >> *card) >> >> /* Set host controller to HS400 timing and frequency */ >> mmc_set_timing(host, MMC_TIMING_MMC_HS400); >> + mmc_set_bus_speed(card); >> >> /* Controller enable enhanced strobe function */ >> host->ios.enhanced_strobe = true; >> > >
On 2016/12/20 18:33, Ritesh Harjani wrote: > Hi Shawn, > > On 12/20/2016 12:59 PM, Shawn Lin wrote: >> On 2016/12/20 13:53, Ritesh Harjani wrote: >>> Currently mmc_select_hs400es is not setting the desired frequency >>> before sending switch command. This makes CMD13 to timeout on >>> some controllers. >>> Thus add a change to set the desired HS400 frequency >>> in mmc_select_hs400es when the timing is switched to HS400. >> >> What is the desired HS400 freq? I guess it is 200MHz, right? >> >> Well, per the spec, it just say "host *may* changes frequency to >> <= 200Mhz". So before setting timing to MMC_TIMING_MMC_HS400, we >> already reach ext_csd.hs_max_dtr. So it should meet the spec. If >> the fact that some controllers would see CMD13 timeout here, I guess >> you wouldn't let folks add max-frequency to the DT as if the max freq >> is set to just lower than your *desired HS400 freq* , it the same >> failure result even you add this patch, right? >> >> the root cause the controllers see failure for CMD13 is it didn't >> configure the right delay phase or something similar to fit the timing >> under the freq of hs_max_dtr, I guess. > Actually this may be limitation of sdhc msm controller that it cannot > work at lower frequency while in HS400 mode. I would have to see more on > this to confirm on why this limitation is there (mostly it was with some > DLL limitation). > In that case maybe I should not generalize this for other controllers. > > > But still the below code change to set the bus_speed after setting the > timing should be fine right ? Since host controller may as well change > the frequency to 200MHz. maybe not, I was guessing what would happend when folks add max-frequency = <100000000> as well as mmc-hs400-enhanced-strobe in the DT? It won't work for your case. So the question is how you would make that case work? > > Do you think the below change is fine in this sense? I can change the > commit message to just - > "set both timing mode and frequency before sending CMD13 in > mmc_select_hs400es." > > Or do you see any other approach to this ? > I haven't had clear thought about how to deal with it but I guess you are not alone for this case maybe. Now there will be more and more mmc controllers using PHY or DLL and some more special limitions is ineluctable, so we need to discuss a more general/legit way to make it work. Your patch is a good begin to open this topic. :) I would like to hear some input from Ulf and Adian. > > Regards > Ritesh > >> >>> >>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>> --- >>> drivers/mmc/core/mmc.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index b61b52f9..eb69497 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -1329,6 +1329,7 @@ static int mmc_select_hs400es(struct mmc_card >>> *card) >>> >>> /* Set host controller to HS400 timing and frequency */ >>> mmc_set_timing(host, MMC_TIMING_MMC_HS400); >>> + mmc_set_bus_speed(card); >>> >>> /* Controller enable enhanced strobe function */ >>> host->ios.enhanced_strobe = true; >>> >> >> >
Hi Shawn, On 12/20/2016 4:33 PM, Shawn Lin wrote: > On 2016/12/20 18:33, Ritesh Harjani wrote: >> Hi Shawn, >> >> On 12/20/2016 12:59 PM, Shawn Lin wrote: >>> On 2016/12/20 13:53, Ritesh Harjani wrote: >>>> Currently mmc_select_hs400es is not setting the desired frequency >>>> before sending switch command. This makes CMD13 to timeout on >>>> some controllers. >>>> Thus add a change to set the desired HS400 frequency >>>> in mmc_select_hs400es when the timing is switched to HS400. >>> >>> What is the desired HS400 freq? I guess it is 200MHz, right? >>> >>> Well, per the spec, it just say "host *may* changes frequency to >>> <= 200Mhz". So before setting timing to MMC_TIMING_MMC_HS400, we >>> already reach ext_csd.hs_max_dtr. So it should meet the spec. If >>> the fact that some controllers would see CMD13 timeout here, I guess >>> you wouldn't let folks add max-frequency to the DT as if the max freq >>> is set to just lower than your *desired HS400 freq* , it the same >>> failure result even you add this patch, right? >>> >>> the root cause the controllers see failure for CMD13 is it didn't >>> configure the right delay phase or something similar to fit the timing >>> under the freq of hs_max_dtr, I guess. >> Actually this may be limitation of sdhc msm controller that it cannot >> work at lower frequency while in HS400 mode. I would have to see more on >> this to confirm on why this limitation is there (mostly it was with some >> DLL limitation). >> In that case maybe I should not generalize this for other controllers. >> >> >> But still the below code change to set the bus_speed after setting the >> timing should be fine right ? Since host controller may as well change >> the frequency to 200MHz. > > maybe not, I was guessing what would happend when folks add > max-frequency = <100000000> as well as mmc-hs400-enhanced-strobe in > the DT? It won't work for your case. So the question is how you would > make that case work? > >> >> Do you think the below change is fine in this sense? I can change the >> commit message to just - >> "set both timing mode and frequency before sending CMD13 in >> mmc_select_hs400es." >> >> Or do you see any other approach to this ? >> > > I haven't had clear thought about how to deal with it but > I guess you are not alone for this case maybe. Now there > will be more and more mmc controllers using PHY or DLL and > some more special limitions is ineluctable, so we need to > discuss a more general/legit way to make it work. > > Your patch is a good begin to open this topic. :) > I would like to hear some input from Ulf and Adian. I see there is a miss from my side here. Although there is still some limitation on msm host controller which I cannot recollect. But for this case we can get it done by configuring the DLL properly. I missed that particular check of clock >= 100MHz in sdhci_msm_set_uhs_signaling which configures the DLL. I guess if I could use the host->mmc_host_ops.hs400_enhanced_strobe ops and provide a callback to configure the DLL without any restriction on clock frequency, then this can be worked out. I will check more on this and get back. Thanks for your inputs. Regards Ritesh > >> >> Regards >> Ritesh >> >>> >>>> >>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> >>>> --- >>>> drivers/mmc/core/mmc.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>> index b61b52f9..eb69497 100644 >>>> --- a/drivers/mmc/core/mmc.c >>>> +++ b/drivers/mmc/core/mmc.c >>>> @@ -1329,6 +1329,7 @@ static int mmc_select_hs400es(struct mmc_card >>>> *card) >>>> >>>> /* Set host controller to HS400 timing and frequency */ >>>> mmc_set_timing(host, MMC_TIMING_MMC_HS400); >>>> + mmc_set_bus_speed(card); >>>> >>>> /* Controller enable enhanced strobe function */ >>>> host->ios.enhanced_strobe = true; >>>> >>> >>> >> > >
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index b61b52f9..eb69497 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1329,6 +1329,7 @@ static int mmc_select_hs400es(struct mmc_card *card) /* Set host controller to HS400 timing and frequency */ mmc_set_timing(host, MMC_TIMING_MMC_HS400); + mmc_set_bus_speed(card); /* Controller enable enhanced strobe function */ host->ios.enhanced_strobe = true;
Currently mmc_select_hs400es is not setting the desired frequency before sending switch command. This makes CMD13 to timeout on some controllers. Thus add a change to set the desired HS400 frequency in mmc_select_hs400es when the timing is switched to HS400. Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org> --- drivers/mmc/core/mmc.c | 1 + 1 file changed, 1 insertion(+)