| Message ID | 1495634044-32102-1-git-send-email-phil.edworthy@renesas.com |
|---|---|
| State | Deferred |
| Delegated to: | Jaehoon Chung |
| Headers | show |
Hi, On 05/24/2017 10:54 PM, Phil Edworthy wrote: > The code currently defaults to the slowest clock speed that can be > achieved, which can be significantly lower than the SD spec. Is there any problem..As i know, it should be changed from 1 to min_clk. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > drivers/mmc/mmc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index 72fc177..dff1be3 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) > #endif > mmc->ddr_mode = 0; > mmc_set_bus_width(mmc, 1); > - mmc_set_clock(mmc, 1); > + mmc_set_clock(mmc, 400000); > > /* Reset the Card */ > err = mmc_go_idle(mmc); >
Hi Jaehoon Chung, On 25 May 2017 14:50 Jaehoon Chung wrote: > Hi, > > On 05/24/2017 10:54 PM, Phil Edworthy wrote: > > The code currently defaults to the slowest clock speed that can be > > achieved, which can be significantly lower than the SD spec. > > Is there any problem..As i know, it should be changed from 1 to min_clk. The only problem is that the initial SD clock can be very slow so it increases the time to start SD. Admittedly, it's a very small increase in time, but we should use the correct initial clock speed. Thanks Phil > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > drivers/mmc/mmc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index > > 72fc177..dff1be3 100644 > > --- a/drivers/mmc/mmc.c > > +++ b/drivers/mmc/mmc.c > > @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) #endif > > mmc->ddr_mode = 0; > > mmc_set_bus_width(mmc, 1); > > - mmc_set_clock(mmc, 1); > > + mmc_set_clock(mmc, 400000); > > > > /* Reset the Card */ > > err = mmc_go_idle(mmc); > >
On 05/25/2017 11:02 PM, Phil Edworthy wrote: > Hi Jaehoon Chung, > > On 25 May 2017 14:50 Jaehoon Chung wrote: >> Hi, >> >> On 05/24/2017 10:54 PM, Phil Edworthy wrote: >>> The code currently defaults to the slowest clock speed that can be >>> achieved, which can be significantly lower than the SD spec. >> >> Is there any problem..As i know, it should be changed from 1 to min_clk. > The only problem is that the initial SD clock can be very slow so it increases > the time to start SD. Admittedly, it's a very small increase in time, but we > should use the correct initial clock speed. Well..i didn't agree yet... If mmc_set_clock(mmc, 400K) and mmc->cfg->f_min is 300K? Initial clock should be always 400K..but spec is mentioned "initial clock is maximum 400K.." It means the clock can be the value under 400K. > > Thanks > Phil > >>> >>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >>> --- >>> drivers/mmc/mmc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index >>> 72fc177..dff1be3 100644 >>> --- a/drivers/mmc/mmc.c >>> +++ b/drivers/mmc/mmc.c >>> @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) #endif >>> mmc->ddr_mode = 0; >>> mmc_set_bus_width(mmc, 1); >>> - mmc_set_clock(mmc, 1); >>> + mmc_set_clock(mmc, 400000); >>> >>> /* Reset the Card */ >>> err = mmc_go_idle(mmc); >>> > > > >
Hi Jaehoon Chung, On 25 May 2017 15:10 Jaehoon Chung wrote: > On 05/25/2017 11:02 PM, Phil Edworthy wrote: > > On 25 May 2017 14:50 Jaehoon Chung wrote: > >> On 05/24/2017 10:54 PM, Phil Edworthy wrote: > >>> The code currently defaults to the slowest clock speed that can be > >>> achieved, which can be significantly lower than the SD spec. > >> > >> Is there any problem..As i know, it should be changed from 1 to min_clk. > > The only problem is that the initial SD clock can be very slow so it > > increases the time to start SD. Admittedly, it's a very small increase > > in time, but we should use the correct initial clock speed. > > Well..i didn't agree yet... > > If mmc_set_clock(mmc, 400K) and mmc->cfg->f_min is 300K? > Initial clock should be always 400K..but spec is mentioned "initial clock is > maximum 400K.." > It means the clock can be the value under 400K. I'm not sure I follow you. The spec means that all SD cards must support an initial clock speed of 400KHz, right? If so, then there is no harm in setting it to 400KHz. Thanks Phil > >>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > >>> --- > >>> drivers/mmc/mmc.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index > >>> 72fc177..dff1be3 100644 > >>> --- a/drivers/mmc/mmc.c > >>> +++ b/drivers/mmc/mmc.c > >>> @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) #endif > >>> mmc->ddr_mode = 0; > >>> mmc_set_bus_width(mmc, 1); > >>> - mmc_set_clock(mmc, 1); > >>> + mmc_set_clock(mmc, 400000); > >>> > >>> /* Reset the Card */ > >>> err = mmc_go_idle(mmc); > >>> > > > > > > > >
On 05/25/2017 11:14 PM, Phil Edworthy wrote: > Hi Jaehoon Chung, > > On 25 May 2017 15:10 Jaehoon Chung wrote: >> On 05/25/2017 11:02 PM, Phil Edworthy wrote: >>> On 25 May 2017 14:50 Jaehoon Chung wrote: >>>> On 05/24/2017 10:54 PM, Phil Edworthy wrote: >>>>> The code currently defaults to the slowest clock speed that can be >>>>> achieved, which can be significantly lower than the SD spec. >>>> >>>> Is there any problem..As i know, it should be changed from 1 to min_clk. >>> The only problem is that the initial SD clock can be very slow so it >>> increases the time to start SD. Admittedly, it's a very small increase >>> in time, but we should use the correct initial clock speed. >> >> Well..i didn't agree yet... >> >> If mmc_set_clock(mmc, 400K) and mmc->cfg->f_min is 300K? >> Initial clock should be always 400K..but spec is mentioned "initial clock is >> maximum 400K.." >> It means the clock can be the value under 400K. > I'm not sure I follow you. > The spec means that all SD cards must support an initial clock speed of 400KHz, right? No..clock frequency range shall be 100KHz - 400KHz during initialization sequence. Some targets should not work fine with 400KHz..So it needs to check f_min value. otherwise, it needs to find the initial clock value like Linux kernel scheme. > If so, then there is no harm in setting it to 400KHz. > > Thanks > Phil > >>>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >>>>> --- >>>>> drivers/mmc/mmc.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index >>>>> 72fc177..dff1be3 100644 >>>>> --- a/drivers/mmc/mmc.c >>>>> +++ b/drivers/mmc/mmc.c >>>>> @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) #endif >>>>> mmc->ddr_mode = 0; >>>>> mmc_set_bus_width(mmc, 1); >>>>> - mmc_set_clock(mmc, 1); >>>>> + mmc_set_clock(mmc, 400000); >>>>> >>>>> /* Reset the Card */ >>>>> err = mmc_go_idle(mmc); >>>>> >>> >>> >>> >>> > > > >
Hi Jaehoon Chung, On 26 May 2017 04:38 Jaehoon Chung wrote: > On 05/25/2017 11:14 PM, Phil Edworthy wrote: > > On 25 May 2017 15:10 Jaehoon Chung wrote: > >> On 05/25/2017 11:02 PM, Phil Edworthy wrote: > >>> On 25 May 2017 14:50 Jaehoon Chung wrote: > >>>> On 05/24/2017 10:54 PM, Phil Edworthy wrote: > >>>>> The code currently defaults to the slowest clock speed that can be > >>>>> achieved, which can be significantly lower than the SD spec. > >>>> > >>>> Is there any problem..As i know, it should be changed from 1 to > min_clk. > >>> The only problem is that the initial SD clock can be very slow so it > >>> increases the time to start SD. Admittedly, it's a very small > >>> increase in time, but we should use the correct initial clock speed. > >> > >> Well..i didn't agree yet... > >> > >> If mmc_set_clock(mmc, 400K) and mmc->cfg->f_min is 300K? > >> Initial clock should be always 400K..but spec is mentioned "initial > >> clock is maximum 400K.." > >> It means the clock can be the value under 400K. > > I'm not sure I follow you. > > The spec means that all SD cards must support an initial clock speed of > 400KHz, right? > > No..clock frequency range shall be 100KHz - 400KHz during initialization > sequence. > Some targets should not work fine with 400KHz..So it needs to check f_min > value. > otherwise, it needs to find the initial clock value like Linux kernel scheme. f_min is not the frequency that must be used during init, it is the minimum frequency that the Controller can operate at. If a controller has f_min=300KHz then there is no problem attempting to set the MMC clock to 400KHz. If the controller cannot do exactly 400KHz, the driver should set the clock to the closest frequency _under_ 400KHz. For example, SDHCI v3 controllers set f_min to f_max / 2046. In my particular case, SDHCI v3 controller f_max is 50MHz, so f_min is approximately 25KHz. Therefore, when the code does this: mmc_set_clock(mmc, 1); it will set the clock rate to approximately 25KHz. Clearly this is outside the valid 100-400KHz range that you quote. Thanks Phil > > If so, then there is no harm in setting it to 400KHz. > > > > Thanks > > Phil > > > >>>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > >>>>> --- > >>>>> drivers/mmc/mmc.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index > >>>>> 72fc177..dff1be3 100644 > >>>>> --- a/drivers/mmc/mmc.c > >>>>> +++ b/drivers/mmc/mmc.c > >>>>> @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) > #endif > >>>>> mmc->ddr_mode = 0; > >>>>> mmc_set_bus_width(mmc, 1); > >>>>> - mmc_set_clock(mmc, 1); > >>>>> + mmc_set_clock(mmc, 400000); > >>>>> > >>>>> /* Reset the Card */ > >>>>> err = mmc_go_idle(mmc); > >>>>> > >>> > >>> > >>> > >>> > > > > > > > >
Hi Phil, On 05/26/2017 05:01 PM, Phil Edworthy wrote: > Hi Jaehoon Chung, > > On 26 May 2017 04:38 Jaehoon Chung wrote: >> On 05/25/2017 11:14 PM, Phil Edworthy wrote: >>> On 25 May 2017 15:10 Jaehoon Chung wrote: >>>> On 05/25/2017 11:02 PM, Phil Edworthy wrote: >>>>> On 25 May 2017 14:50 Jaehoon Chung wrote: >>>>>> On 05/24/2017 10:54 PM, Phil Edworthy wrote: >>>>>>> The code currently defaults to the slowest clock speed that can be >>>>>>> achieved, which can be significantly lower than the SD spec. >>>>>> >>>>>> Is there any problem..As i know, it should be changed from 1 to >> min_clk. >>>>> The only problem is that the initial SD clock can be very slow so it >>>>> increases the time to start SD. Admittedly, it's a very small >>>>> increase in time, but we should use the correct initial clock speed. >>>> >>>> Well..i didn't agree yet... >>>> >>>> If mmc_set_clock(mmc, 400K) and mmc->cfg->f_min is 300K? >>>> Initial clock should be always 400K..but spec is mentioned "initial >>>> clock is maximum 400K.." >>>> It means the clock can be the value under 400K. >>> I'm not sure I follow you. >>> The spec means that all SD cards must support an initial clock speed of >> 400KHz, right? >> >> No..clock frequency range shall be 100KHz - 400KHz during initialization >> sequence. >> Some targets should not work fine with 400KHz..So it needs to check f_min >> value. >> otherwise, it needs to find the initial clock value like Linux kernel scheme. > f_min is not the frequency that must be used during init, it is the minimum > frequency that the Controller can operate at. If a controller has f_min=300KHz > then there is no problem attempting to set the MMC clock to 400KHz. If the > controller cannot do exactly 400KHz, the driver should set the clock to the > closest frequency _under_ 400KHz. > > For example, SDHCI v3 controllers set f_min to f_max / 2046. In my particular > case, SDHCI v3 controller f_max is 50MHz, so f_min is approximately 25KHz. > Therefore, when the code does this: > mmc_set_clock(mmc, 1); > it will set the clock rate to approximately 25KHz. > Clearly this is outside the valid 100-400KHz range that you quote. Well..then i will check the real frequency with oscilloscope.. then it's more clear than now.. :) Thanks for pointing out about this. Best Regards, Jaehoon Chung > > Thanks > Phil > >>> If so, then there is no harm in setting it to 400KHz. >>> >>> Thanks >>> Phil >>> >>>>>>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >>>>>>> --- >>>>>>> drivers/mmc/mmc.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index >>>>>>> 72fc177..dff1be3 100644 >>>>>>> --- a/drivers/mmc/mmc.c >>>>>>> +++ b/drivers/mmc/mmc.c >>>>>>> @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) >> #endif >>>>>>> mmc->ddr_mode = 0; >>>>>>> mmc_set_bus_width(mmc, 1); >>>>>>> - mmc_set_clock(mmc, 1); >>>>>>> + mmc_set_clock(mmc, 400000); >>>>>>> >>>>>>> /* Reset the Card */ >>>>>>> err = mmc_go_idle(mmc); >>>>>>> >>>>> >>>>> >>>>> >>>>> >>> >>> >>> >>> > > > >
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 72fc177..dff1be3 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -1676,7 +1676,7 @@ int mmc_start_init(struct mmc *mmc) #endif mmc->ddr_mode = 0; mmc_set_bus_width(mmc, 1); - mmc_set_clock(mmc, 1); + mmc_set_clock(mmc, 400000); /* Reset the Card */ err = mmc_go_idle(mmc);
The code currently defaults to the slowest clock speed that can be achieved, which can be significantly lower than the SD spec. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- drivers/mmc/mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)