* Re: [PATCH v3] mmc: Make ID freq configurable
2010-01-02 23:38 ` Pierre Ossman
@ 2010-01-03 8:00 ` Hein_Tibosch
2010-01-04 21:07 ` Hein_Tibosch
2010-01-04 21:58 ` Hein_Tibosch
2 siblings, 0 replies; 11+ messages in thread
From: Hein_Tibosch @ 2010-01-03 8:00 UTC (permalink / raw)
To: Pierre Ossman
Cc: Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
Pierre Ossman wrote:
> On Sun, 3 Jan 2010 09:23:30 +1100
> Ben Nizette <bn@niasdigital.com> wrote:
>
>
>>> Broken cards seem to be all over the spectrum, so I wouldn't be
>>> suprised if you find ones that break if you go too low as well.
>>>
>> Yea good point, though given there might not even be a One Freq to Rule Them All, how about:
>>
>> ---8<---
>>
>> From: Ben Nizette <bn@niasdigital.com>
>> Subject: [PATCH v3] mmc: Make ID freq configurable
>>
>> While the SD spec specifies a range of frequencies for the ID phase from 100-400kHz, not all cards play by the rules. This patch adds a Kconfig option to allow the user to tweak this to their card/host config, though the default of 250kHz should be fine for all spec-abiding cards and most others besides.
>>
>> Signed-off-by: Ben Nizette <bn@niasdigital.com>
>>
>>
>
> This is not a good solution. We all use the same pool of cards so we
> should all be using the same init sequence. If there isn't a single
> frequency where all cards will work, then we'll have to make something
> more advanced where the kernel will try the init several times with
> different clocking.
>
As I'm lucky to have main-stream cards that break the rule (on
avr32/AP7) I'll dive deeper into it and see if I can make it "automatic"
so that initialization will work for any of cards. Indeed using a kind
of 'bisect approach' approach.
... although I must say that before Sascha Hauer's patch, we never had
problems with any of the cards, using the controller's minimum freq of
137 Khz.
Hein
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] mmc: Make ID freq configurable
2010-01-02 23:38 ` Pierre Ossman
2010-01-03 8:00 ` Hein_Tibosch
@ 2010-01-04 21:07 ` Hein_Tibosch
2010-01-05 12:24 ` Sascha Hauer
2010-01-04 21:58 ` Hein_Tibosch
2 siblings, 1 reply; 11+ messages in thread
From: Hein_Tibosch @ 2010-01-04 21:07 UTC (permalink / raw)
To: Pierre Ossman
Cc: Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
On 3-1-2010 07:38, Pierre Ossman wrote:
>
> <cut>
> We all use the same pool of cards so we
> should all be using the same init sequence. If there isn't a single
> frequency where all cards will work, then we'll have to make something
> more advanced where the kernel will try the init several times with
> different clocking.
On 4-1-2010 20:15, Sascha Hauer wrote:
> The problem was that in some setups the mxcmmc driver allows frequencies
> as low as a few Hz and the card initialization took ages. Increasing the
> frequency to 400kHz seemed sensible since it's the value the spec
> describes. 50kHz should be fine as well.
>
I tried out several main-stream sd-cards and with the Atmel (AP7) controller
they all initialize with F between 138 (=f_min) and 280 Khz.
Below a patch which tries mmc-initialization using several frequencies
from an array 400, 300, 200 and 100. When it comes to 200 Khz, it works.
The two failed attempts lasted about 10 ms together.
It looks like a hell-of-a-patch but that's because the indentation changes.
Note that the new mmc_host member f_init will be used later in power_restore
and resume. As power_up always comes first, it should have a descend value
by then.
But I still wonder, why not just keep using Sascha's patch, changing 400000
to 50000? Has anyone ever had problems with mmc-init on 50 Khz?
Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -891,12 +891,7 @@ static void mmc_power_up(struct mmc_host
*/
mmc_delay(10);
- if (host->f_min > 400000) {
- pr_warning("%s: Minimum clock frequency too high for "
- "identification mode\n", mmc_hostname(host));
- host->ios.clock = host->f_min;
- } else
- host->ios.clock = 400000;
+ host->ios.clock = host->f_init;
host->ios.power_mode = MMC_POWER_ON;
mmc_set_ios(host);
@@ -1041,6 +1036,8 @@ void mmc_rescan(struct work_struct *work
container_of(work, struct mmc_host, detect.work);
u32 ocr;
int err;
+ int i;
+ unsigned freqs[] = { 400000, 300000, 200000, 100000 };
mmc_bus_get(host);
@@ -1070,46 +1067,55 @@ void mmc_rescan(struct work_struct *work
if (host->ops->get_cd && host->ops->get_cd(host) == 0)
goto out;
- mmc_claim_host(host);
+ for (i = 0; i < ARRAY_SIZE(freqs); i++) {
+ mmc_claim_host(host);
- mmc_power_up(host);
- mmc_go_idle(host);
+ if (freqs[i] >= host->f_min)
+ host->f_init = freqs[i];
+ else if (i && freqs[i-1] <= host->f_min)
+ goto out;
+ else
+ host->f_init = host->f_min;
- mmc_send_if_cond(host, host->ocr_avail);
+ printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+ mmc_power_up(host);
+ mmc_go_idle(host);
- /*
- * First we search for SDIO...
- */
- err = mmc_send_io_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_sdio(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ mmc_send_if_cond(host, host->ocr_avail);
- /*
- * ...then normal SD...
- */
- err = mmc_send_app_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_sd(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ /*
+ * First we search for SDIO...
+ */
+ err = mmc_send_io_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sdio(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
- /*
- * ...and finally MMC.
- */
- err = mmc_send_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_mmc(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ /*
+ * ...then normal SD...
+ */
+ err = mmc_send_app_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sd(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
- mmc_release_host(host);
- mmc_power_off(host);
+ /*
+ * ...and finally MMC.
+ */
+ err = mmc_send_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_mmc(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
+ mmc_release_host(host);
+ mmc_power_off(host);
+ }
out:
if (host->caps & MMC_CAP_NEEDS_POLL)
mmc_schedule_delayed_work(&host->detect, HZ);
diff -Nurp a/include/linux/mmc/host.h b/include/linux/mmc/host.h
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -119,6 +119,7 @@ struct mmc_host {
const struct mmc_host_ops *ops;
unsigned int f_min;
unsigned int f_max;
+ unsigned int f_init;
u32 ocr_avail;
#define MMC_VDD_165_195 0x00000080 /* VDD voltage 1.65 - 1.95 */
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] mmc: Make ID freq configurable
2010-01-04 21:07 ` Hein_Tibosch
@ 2010-01-05 12:24 ` Sascha Hauer
0 siblings, 0 replies; 11+ messages in thread
From: Sascha Hauer @ 2010-01-05 12:24 UTC (permalink / raw)
To: Hein_Tibosch
Cc: Pierre Ossman, Ben Nizette, Adrian Hunter, linux-mmc,
linux-kernel, Matt Fleming
On Tue, Jan 05, 2010 at 05:07:21AM +0800, Hein_Tibosch wrote:
> On 3-1-2010 07:38, Pierre Ossman wrote:
>>
>> <cut>
>> We all use the same pool of cards so we
>> should all be using the same init sequence. If there isn't a single
>> frequency where all cards will work, then we'll have to make something
>> more advanced where the kernel will try the init several times with
>> different clocking.
>
> On 4-1-2010 20:15, Sascha Hauer wrote:
>> The problem was that in some setups the mxcmmc driver allows frequencies
>> as low as a few Hz and the card initialization took ages. Increasing the
>> frequency to 400kHz seemed sensible since it's the value the spec
>> describes. 50kHz should be fine as well.
>>
>
> I tried out several main-stream sd-cards and with the Atmel (AP7) controller
> they all initialize with F between 138 (=f_min) and 280 Khz.
>
> Below a patch which tries mmc-initialization using several frequencies
> from an array 400, 300, 200 and 100. When it comes to 200 Khz, it works.
> The two failed attempts lasted about 10 ms together.
>
> It looks like a hell-of-a-patch but that's because the indentation changes.
>
> Note that the new mmc_host member f_init will be used later in power_restore
> and resume. As power_up always comes first, it should have a descend value
> by then.
>
> But I still wonder, why not just keep using Sascha's patch, changing 400000
> to 50000? Has anyone ever had problems with mmc-init on 50 Khz?
I already wrote it to Hein in private, once again for the rest:
The original reason to increase the minimum frequency was that the
Freescale i.MX MMC core allows minimum frequencies of just a few Hz.
With this the card initialisation took ages, so we increased the minimum
frequency. 400kHz seemed like a good value since all cards should
support this, but I'm also fine with any other value between 50kHz and
400kHz.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] mmc: Make ID freq configurable
2010-01-02 23:38 ` Pierre Ossman
2010-01-03 8:00 ` Hein_Tibosch
2010-01-04 21:07 ` Hein_Tibosch
@ 2010-01-04 21:58 ` Hein_Tibosch
2010-08-27 20:44 ` Chris Ball
2 siblings, 1 reply; 11+ messages in thread
From: Hein_Tibosch @ 2010-01-04 21:58 UTC (permalink / raw)
To: Pierre Ossman
Cc: Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
In my last mail the spaces in the patch were mistreated, sorry for that.
Below a patch which tries mmc-initialization using several frequencies
from an array 400, 300, 200 and 100.
Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -891,12 +891,7 @@ static void mmc_power_up(struct mmc_host
*/
mmc_delay(10);
- if (host->f_min > 400000) {
- pr_warning("%s: Minimum clock frequency too high for "
- "identification mode\n", mmc_hostname(host));
- host->ios.clock = host->f_min;
- } else
- host->ios.clock = 400000;
+ host->ios.clock = host->f_init;
host->ios.power_mode = MMC_POWER_ON;
mmc_set_ios(host);
@@ -1041,6 +1036,8 @@ void mmc_rescan(struct work_struct *work
container_of(work, struct mmc_host, detect.work);
u32 ocr;
int err;
+ int i;
+ unsigned freqs[] = { 400000, 300000, 200000, 100000 };
mmc_bus_get(host);
@@ -1070,46 +1067,55 @@ void mmc_rescan(struct work_struct *work
if (host->ops->get_cd && host->ops->get_cd(host) == 0)
goto out;
- mmc_claim_host(host);
+ for (i = 0; i < ARRAY_SIZE(freqs); i++) {
+ mmc_claim_host(host);
- mmc_power_up(host);
- mmc_go_idle(host);
+ if (freqs[i] >= host->f_min)
+ host->f_init = freqs[i];
+ else if (i && freqs[i-1] <= host->f_min)
+ goto out;
+ else
+ host->f_init = host->f_min;
- mmc_send_if_cond(host, host->ocr_avail);
+ printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+ mmc_power_up(host);
+ mmc_go_idle(host);
- /*
- * First we search for SDIO...
- */
- err = mmc_send_io_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_sdio(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ mmc_send_if_cond(host, host->ocr_avail);
- /*
- * ...then normal SD...
- */
- err = mmc_send_app_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_sd(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ /*
+ * First we search for SDIO...
+ */
+ err = mmc_send_io_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sdio(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
- /*
- * ...and finally MMC.
- */
- err = mmc_send_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_mmc(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ /*
+ * ...then normal SD...
+ */
+ err = mmc_send_app_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sd(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
- mmc_release_host(host);
- mmc_power_off(host);
+ /*
+ * ...and finally MMC.
+ */
+ err = mmc_send_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_mmc(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
+ mmc_release_host(host);
+ mmc_power_off(host);
+ }
out:
if (host->caps & MMC_CAP_NEEDS_POLL)
mmc_schedule_delayed_work(&host->detect, HZ);
diff -Nurp a/include/linux/mmc/host.h b/include/linux/mmc/host.h
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -119,6 +119,7 @@ struct mmc_host {
const struct mmc_host_ops *ops;
unsigned int f_min;
unsigned int f_max;
+ unsigned int f_init;
u32 ocr_avail;
#define MMC_VDD_165_195 0x00000080 /* VDD voltage 1.65 - 1.95 */
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] mmc: Make ID freq configurable
2010-01-04 21:58 ` Hein_Tibosch
@ 2010-08-27 20:44 ` Chris Ball
2010-08-28 0:44 ` Hein_Tibosch
0 siblings, 1 reply; 11+ messages in thread
From: Chris Ball @ 2010-08-27 20:44 UTC (permalink / raw)
To: Hein_Tibosch
Cc: Pierre Ossman, Ben Nizette, Sascha Hauer, Adrian Hunter,
linux-mmc, linux-kernel, Matt Fleming, akpm
Hi,
This looks like the best of the min_freq patches I've seen, and is
implementing a suggestion of Pierre's. Should we take it?
- Chris.
On Tue, Jan 05, 2010 at 05:58:34AM +0800, Hein_Tibosch wrote:
>
> In my last mail the spaces in the patch were mistreated, sorry for that.
>
> Below a patch which tries mmc-initialization using several frequencies
> from an array 400, 300, 200 and 100.
>
>
> Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
>
> ---
> diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -891,12 +891,7 @@ static void mmc_power_up(struct mmc_host
> */
> mmc_delay(10);
>
> - if (host->f_min > 400000) {
> - pr_warning("%s: Minimum clock frequency too high for "
> - "identification mode\n", mmc_hostname(host));
> - host->ios.clock = host->f_min;
> - } else
> - host->ios.clock = 400000;
> + host->ios.clock = host->f_init;
>
> host->ios.power_mode = MMC_POWER_ON;
> mmc_set_ios(host);
> @@ -1041,6 +1036,8 @@ void mmc_rescan(struct work_struct *work
> container_of(work, struct mmc_host, detect.work);
> u32 ocr;
> int err;
> + int i;
> + unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>
> mmc_bus_get(host);
>
> @@ -1070,46 +1067,55 @@ void mmc_rescan(struct work_struct *work
> if (host->ops->get_cd && host->ops->get_cd(host) == 0)
> goto out;
>
> - mmc_claim_host(host);
> + for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> + mmc_claim_host(host);
>
> - mmc_power_up(host);
> - mmc_go_idle(host);
> + if (freqs[i] >= host->f_min)
> + host->f_init = freqs[i];
> + else if (i && freqs[i-1] <= host->f_min)
> + goto out;
> + else
> + host->f_init = host->f_min;
>
> - mmc_send_if_cond(host, host->ocr_avail);
> + printk ("mmc_rescan: trying %u Hz\n", host->f_init);
> + mmc_power_up(host);
> + mmc_go_idle(host);
>
> - /*
> - * First we search for SDIO...
> - */
> - err = mmc_send_io_op_cond(host, 0, &ocr);
> - if (!err) {
> - if (mmc_attach_sdio(host, ocr))
> - mmc_power_off(host);
> - goto out;
> - }
> + mmc_send_if_cond(host, host->ocr_avail);
>
> - /*
> - * ...then normal SD...
> - */
> - err = mmc_send_app_op_cond(host, 0, &ocr);
> - if (!err) {
> - if (mmc_attach_sd(host, ocr))
> - mmc_power_off(host);
> - goto out;
> - }
> + /*
> + * First we search for SDIO...
> + */
> + err = mmc_send_io_op_cond(host, 0, &ocr);
> + if (!err) {
> + if (mmc_attach_sdio(host, ocr))
> + mmc_power_off(host);
> + goto out;
> + }
>
> - /*
> - * ...and finally MMC.
> - */
> - err = mmc_send_op_cond(host, 0, &ocr);
> - if (!err) {
> - if (mmc_attach_mmc(host, ocr))
> - mmc_power_off(host);
> - goto out;
> - }
> + /*
> + * ...then normal SD...
> + */
> + err = mmc_send_app_op_cond(host, 0, &ocr);
> + if (!err) {
> + if (mmc_attach_sd(host, ocr))
> + mmc_power_off(host);
> + goto out;
> + }
>
> - mmc_release_host(host);
> - mmc_power_off(host);
> + /*
> + * ...and finally MMC.
> + */
> + err = mmc_send_op_cond(host, 0, &ocr);
> + if (!err) {
> + if (mmc_attach_mmc(host, ocr))
> + mmc_power_off(host);
> + goto out;
> + }
>
> + mmc_release_host(host);
> + mmc_power_off(host);
> + }
> out:
> if (host->caps & MMC_CAP_NEEDS_POLL)
> mmc_schedule_delayed_work(&host->detect, HZ);
> diff -Nurp a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -119,6 +119,7 @@ struct mmc_host {
> const struct mmc_host_ops *ops;
> unsigned int f_min;
> unsigned int f_max;
> + unsigned int f_init;
> u32 ocr_avail;
>
> #define MMC_VDD_165_195 0x00000080 /* VDD voltage 1.65 - 1.95 */
>
>
> --
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] mmc: Make ID freq configurable
2010-08-27 20:44 ` Chris Ball
@ 2010-08-28 0:44 ` Hein_Tibosch
0 siblings, 0 replies; 11+ messages in thread
From: Hein_Tibosch @ 2010-08-28 0:44 UTC (permalink / raw)
To: Chris Ball
Cc: Pierre Ossman, Ben Nizette, Sascha Hauer, Adrian Hunter,
linux-mmc, linux-kernel, Matt Fleming, akpm
On 28-8-2010 04:44, Chris Ball wrote:
> Hi,
>
> This looks like the best of the min_freq patches I've seen, and is
> implementing a suggestion of Pierre's. Should we take it?
>
> - Chris.
>
> On Tue, Jan 05, 2010 at 05:58:34AM +0800, Hein_Tibosch wrote:
>> Below a patch which tries mmc-initialization using several frequencies
>> from an array 400, 300, 200 and 100.
>>
>>
>> Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
>> <cut>
Hi Chris,
Thanks. I can confirm that the above patch has been working properly for
7 months now, using various SD-cards.
Here it tries 3 frequencies, which takes 290 ms:
000 mmc_rescan: trying 400000 Hz
Waiting 1sec before mounting root device...
061 mci: F now 400000 (actual 397727)
usb 1-2: configuration #1 chosen from 1 choice
126 mmc_rescan: trying 300000 Hz
input: eGalax Inc. USB TouchController as /class/input/input1
193 mci: F now 300000 (actual 299145)
227 mmc_rescan: trying 200000 Hz
255 mci: F now 200000 (actual 200000)
290 mmc0: new SD card at address e624
In a meanwhile it is doing other things, so not much time is wasted.
But most importantly: it never failed to initialize a card.
Hein Tibosch
^ permalink raw reply [flat|nested] 11+ messages in thread