* Re: [PATCH v4] mmc: Make ID freq configurable
2010-09-02 21:34 [PATCH v4] mmc: Make ID freq configurable Hein_Tibosch
@ 2010-09-03 1:47 ` Chris Ball
2010-09-03 23:25 ` Andrew Morton
2010-09-05 20:14 ` Matt Fleming
2010-12-02 11:16 ` [PATCH v4] " Sahitya Tummala
2 siblings, 1 reply; 13+ messages in thread
From: Chris Ball @ 2010-09-03 1:47 UTC (permalink / raw)
To: Hein_Tibosch
Cc: Andrew Morton, Pierre Ossman, Ben Nizette, Sascha Hauer,
Adrian Hunter, linux-mmc, linux-kernel, Matt Fleming
Hi Hein, thanks for resending this,
On Fri, Sep 03, 2010 at 05:34:53AM +0800, Hein_Tibosch wrote:
> In the latest releases of the mmc driver, the freq during initialization
> is set to a fixed 400 Khz. This was reportedly too fast for several
> users. As there doesn't seem to be an ideal frequency which-works-for-all,
> Pierre suggested to let the driver try several frequencies.
>
> This patch implements that idea. It will try mmc-initialization using
> several frequencies from an array 400, 300, 200 and 100.
> I submitted it earlier but it's now adapted to and tested with kernel
> 2.6.36-rc3.
>
> In case SDIO is broken, it'll still try to detect SDMEM, also at different
> freqs.
>
> Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
Reviewed-and-Tested-by: Chris Ball <cjb@laptop.org>
I tested by modifying mmc_send_app_op_cond() to return timeouts for
various frequencies, and verified that the code correctly progresses
through the freqs[] table until reaching host->f_min, and can bring
up the card successfully on any of those retries.
In an earlier mail, you said:
> Here it tries 3 frequencies, which takes 290 ms:
> In a meanwhile it is doing other things, so not much time is wasted.
I decided to check this. If my emulated failing of bringing up a card
at a given frequency is correct, 14ms per frequency attempt is spent in
mdelay() rather than msleep(). I don't think this is a problem, though,
because you only take that hit if you have a card inserted that we're
unable to bring up at the default frequency -- the only alternative
would be to give up on the card.
If a host has no card inserted then we still try to detect a card on
all frequencies, but all of the paths in that case are msleep() rather
than mdelay(), so that's okay.
Two minor checkpatch fixes below:
> ---
> diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> --- a/drivers/mmc/core/core.c 2010-08-29 23:36:04.000000000 +0800
> +++ b/drivers/mmc/core/core.c 2010-09-03 04:28:52.000000000 +0800
> @@ -907,12 +907,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);
> @@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
> u32 ocr;
> int err;
> unsigned long flags;
> + int i;
> + unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>
> spin_lock_irqsave(&host->lock, flags);
>
> @@ -1443,55 +1440,64 @@ 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);
> - sdio_reset(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);
Need a loglevel here, and an mmc_hostname. Something like:
pr_info("%s: %s: trying to init card at %u Hz\n",
mmc_hostname(host), __func__, host->f_init);
> + mmc_power_up(host);
> + sdio_reset(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_claim_host(host);
> - /* try SDMEM (but not MMC) even if SDIO is broken */
> - if (mmc_send_app_op_cond(host, 0, &ocr))
> - goto out_fail;
> + mmc_send_if_cond(host, host->ocr_avail);
> +
> + /*
> + * First we search for SDIO...
> + */
> + err = mmc_send_io_op_cond(host, 0, &ocr);
> + if (!err) {
> + if (mmc_attach_sdio(host, ocr)) {
> + mmc_claim_host(host);
> + /* try SDMEM (but not MMC) even if SDIO is broken */
This breaks 80-chars, so:
/*
* Try SDMEM (but not MMC) even if SDIO
* is broken.
*/
> + if (mmc_send_app_op_cond(host, 0, &ocr))
> + goto out_fail;
> +
> + if (mmc_attach_sd(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;
> }
> - 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;
> - }
> -
> - /*
> - * ...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;
> - }
> + /*
> + * ...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;
> + }
>
> out_fail:
> - mmc_release_host(host);
> - mmc_power_off(host);
> -
> + 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 2010-08-29 23:36:04.000000000 +0800
> +++ b/include/linux/mmc/host.h 2010-09-03 00:52:48.000000000 +0800
> @@ -123,6 +123,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;
> struct notifier_block pm_notify;
>
> ---
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-09-03 1:47 ` Chris Ball
@ 2010-09-03 23:25 ` Andrew Morton
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Morton @ 2010-09-03 23:25 UTC (permalink / raw)
To: Chris Ball
Cc: Hein_Tibosch, Pierre Ossman, Ben Nizette, Sascha Hauer,
Adrian Hunter, linux-mmc, linux-kernel, Matt Fleming
On Fri, 3 Sep 2010 02:47:57 +0100
Chris Ball <cjb@laptop.org> wrote:
> > - mmc_send_if_cond(host, host->ocr_avail);
> > + printk ("mmc_rescan: trying %u Hz\n", host->f_init);
>
> Need a loglevel here, and an mmc_hostname. Something like:
>
> pr_info("%s: %s: trying to init card at %u Hz\n",
> mmc_hostname(host), __func__, host->f_init);
>
> > + mmc_power_up(host);
> > + sdio_reset(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_claim_host(host);
> > - /* try SDMEM (but not MMC) even if SDIO is broken */
> > - if (mmc_send_app_op_cond(host, 0, &ocr))
> > - goto out_fail;
> > + mmc_send_if_cond(host, host->ocr_avail);
> > +
> > + /*
> > + * First we search for SDIO...
> > + */
> > + err = mmc_send_io_op_cond(host, 0, &ocr);
> > + if (!err) {
> > + if (mmc_attach_sdio(host, ocr)) {
> > + mmc_claim_host(host);
> > + /* try SDMEM (but not MMC) even if SDIO is broken */
>
> This breaks 80-chars, so:
>
> /*
> * Try SDMEM (but not MMC) even if SDIO
> * is broken.
> */
yup.
--- a/drivers/mmc/core/core.c~mmc-make-id-freq-configurable-checkpatch-fixes
+++ a/drivers/mmc/core/core.c
@@ -1450,7 +1450,9 @@ void mmc_rescan(struct work_struct *work
else
host->f_init = host->f_min;
- printk ("mmc_rescan: trying %u Hz\n", host->f_init);
+ pr_info("%s: %s: trying to init card at %u Hz\n",
+ mmc_hostname(host), __func__, host->f_init);
+
mmc_power_up(host);
sdio_reset(host);
mmc_go_idle(host);
@@ -1464,7 +1466,10 @@ void mmc_rescan(struct work_struct *work
if (!err) {
if (mmc_attach_sdio(host, ocr)) {
mmc_claim_host(host);
- /* try SDMEM (but not MMC) even if SDIO is broken */
+ /*
+ * Try SDMEM (but not MMC) even if SDIO is
+ * broken.
+ */
if (mmc_send_app_op_cond(host, 0, &ocr))
goto out_fail;
diff -puN include/linux/mmc/host.h~mmc-make-id-freq-configurable-checkpatch-fixes include/linux/mmc/host.h
_
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] mmc: Make ID freq configurable
2010-09-02 21:34 [PATCH v4] mmc: Make ID freq configurable Hein_Tibosch
2010-09-03 1:47 ` Chris Ball
@ 2010-09-05 20:14 ` Matt Fleming
2010-09-05 20:43 ` Chris Ball
2010-12-02 11:16 ` [PATCH v4] " Sahitya Tummala
2 siblings, 1 reply; 13+ messages in thread
From: Matt Fleming @ 2010-09-05 20:14 UTC (permalink / raw)
To: Hein_Tibosch
Cc: Andrew Morton, Pierre Ossman, Chris Ball, Ben Nizette,
Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel
On Fri, Sep 03, 2010 at 05:34:53AM +0800, Hein_Tibosch wrote:
> In the latest releases of the mmc driver, the freq during initialization
> is set to a fixed 400 Khz. This was reportedly too fast for several
> users. As there doesn't seem to be an ideal frequency which-works-for-all,
> Pierre suggested to let the driver try several frequencies.
>
> This patch implements that idea. It will try mmc-initialization using
> several frequencies from an array 400, 300, 200 and 100.
> I submitted it earlier but it's now adapted to and tested with kernel
> 2.6.36-rc3.
>
> In case SDIO is broken, it'll still try to detect SDMEM, also at different
> freqs.
>
> 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 2010-08-29 23:36:04.000000000 +0800
> +++ b/drivers/mmc/core/core.c 2010-09-03 04:28:52.000000000 +0800
> @@ -907,12 +907,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);
> @@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
> u32 ocr;
> int err;
> unsigned long flags;
> + int i;
> + unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>
> spin_lock_irqsave(&host->lock, flags);
>
> @@ -1443,55 +1440,64 @@ 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);
> - sdio_reset(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;
The label 'out' does not release the claim on the host.
> + 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);
> + sdio_reset(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_claim_host(host);
> - /* try SDMEM (but not MMC) even if SDIO is broken */
> - if (mmc_send_app_op_cond(host, 0, &ocr))
> - goto out_fail;
> + mmc_send_if_cond(host, host->ocr_avail);
> +
> + /*
> + * First we search for SDIO...
> + */
> + err = mmc_send_io_op_cond(host, 0, &ocr);
> + if (!err) {
> + if (mmc_attach_sdio(host, ocr)) {
> + mmc_claim_host(host);
> + /* try SDMEM (but not MMC) even if SDIO is broken */
> + if (mmc_send_app_op_cond(host, 0, &ocr))
> + goto out_fail;
> +
> + if (mmc_attach_sd(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;
Same problem here.
> }
> - 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;
> - }
> -
> - /*
> - * ...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;
> - }
> + /*
> + * ...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;
> + }
And here.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-09-05 20:14 ` Matt Fleming
@ 2010-09-05 20:43 ` Chris Ball
2010-09-05 21:22 ` Matt Fleming
0 siblings, 1 reply; 13+ messages in thread
From: Chris Ball @ 2010-09-05 20:43 UTC (permalink / raw)
To: Matt Fleming
Cc: Hein_Tibosch, Andrew Morton, Pierre Ossman, Chris Ball,
Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel
Hi Matt,
On Sun, Sep 05, 2010 at 09:14:48PM +0100, Matt Fleming wrote:
> On Fri, Sep 03, 2010 at 05:34:53AM +0800, Hein_Tibosch wrote:
> > In the latest releases of the mmc driver, the freq during initialization
> > is set to a fixed 400 Khz. This was reportedly too fast for several
> > users. As there doesn't seem to be an ideal frequency which-works-for-all,
> > Pierre suggested to let the driver try several frequencies.
> >
> > This patch implements that idea. It will try mmc-initialization using
> > several frequencies from an array 400, 300, 200 and 100.
> > I submitted it earlier but it's now adapted to and tested with kernel
> > 2.6.36-rc3.
> >
> > In case SDIO is broken, it'll still try to detect SDMEM, also at different
> > freqs.
> >
> > 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 2010-08-29 23:36:04.000000000 +0800
> > +++ b/drivers/mmc/core/core.c 2010-09-03 04:28:52.000000000 +0800
> > @@ -907,12 +907,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);
> > @@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
> > u32 ocr;
> > int err;
> > unsigned long flags;
> > + int i;
> > + unsigned freqs[] = { 400000, 300000, 200000, 100000 };
> >
> > spin_lock_irqsave(&host->lock, flags);
> >
> > @@ -1443,55 +1440,64 @@ 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);
> > - sdio_reset(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;
>
> The label 'out' does not release the claim on the host.
Thanks for catching that.
> > + 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);
> > + sdio_reset(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_claim_host(host);
> > - /* try SDMEM (but not MMC) even if SDIO is broken */
> > - if (mmc_send_app_op_cond(host, 0, &ocr))
> > - goto out_fail;
> > + mmc_send_if_cond(host, host->ocr_avail);
> > +
> > + /*
> > + * First we search for SDIO...
> > + */
> > + err = mmc_send_io_op_cond(host, 0, &ocr);
> > + if (!err) {
> > + if (mmc_attach_sdio(host, ocr)) {
> > + mmc_claim_host(host);
> > + /* try SDMEM (but not MMC) even if SDIO is broken */
> > + if (mmc_send_app_op_cond(host, 0, &ocr))
> > + goto out_fail;
> > +
> > + if (mmc_attach_sd(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;
>
> Same problem here.
>
> > }
> > - 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;
> > - }
> > -
> > - /*
> > - * ...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;
> > - }
> > + /*
> > + * ...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;
> > + }
>
> And here.
Actually, mmc_attach_{sd,mmc}() both do release the host as part of
their failure case, so I don't think there's a problem with these two.
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-09-05 20:43 ` Chris Ball
@ 2010-09-05 21:22 ` Matt Fleming
2010-09-06 1:37 ` [PATCH v5] " Hein_Tibosch
0 siblings, 1 reply; 13+ messages in thread
From: Matt Fleming @ 2010-09-05 21:22 UTC (permalink / raw)
To: Chris Ball
Cc: Hein_Tibosch, Andrew Morton, Pierre Ossman, Ben Nizette,
Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel
On Sun, Sep 05, 2010 at 09:43:36PM +0100, Chris Ball wrote:
>
> Actually, mmc_attach_{sd,mmc}() both do release the host as part of
> their failure case, so I don't think there's a problem with these two.
Ah, yep you're right, I missed that.
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5] mmc: Make ID freq configurable
2010-09-05 21:22 ` Matt Fleming
@ 2010-09-06 1:37 ` Hein_Tibosch
0 siblings, 0 replies; 13+ messages in thread
From: Hein_Tibosch @ 2010-09-06 1:37 UTC (permalink / raw)
To: Andrew Morton
Cc: Chris Ball, Matt Fleming, Pierre Ossman, Ben Nizette,
Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel
Matt, Chris,
Thanks both for reviewing.
I never noticed that the host wasn't released because my f_min was low enough
so that all frequencies would be tried. But yes, you're both right.
Before somebody else makes the remark: "unsigned freqs[]" can be declared as
const.
Below, I changed the order of the if/else/else, and whenever no more frequencies
can be tried, it will release the host before branching out.
Hein Tibosch
---
mmc-make-id-freq-configurable.patch (v5)
In the latest releases of the mmc driver, the freq during initialization
is set to a fixed 400 Khz. This was reportedly too fast for several
users. As there doesn't seem to be an ideal frequency which-works-for-all,
Pierre suggested to let the driver try several frequencies.
This patch implements that idea. It will try mmc-initialization using
several frequencies from an array 400, 300, 200 and 100.
I submitted it earlier but it's now adapted to and tested with kernel
2.6.36-rc3.
In case SDIO is detected but turns out broken, it'll still try to detect
SDMEM, also at different freqs.
Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
Reviewed-and-Tested-by: Chris Ball <cjb@laptop.org>
---
diff -Nurp a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
--- a/drivers/mmc/core/core.c 2010-08-29 23:36:04.000000000 +0800
+++ b/drivers/mmc/core/core.c 2010-09-06 09:16:35.000000000 +0800
@@ -907,12 +907,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);
@@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
u32 ocr;
int err;
unsigned long flags;
+ int i;
+ const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
spin_lock_irqsave(&host->lock, flags);
@@ -1443,55 +1440,70 @@ 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);
- sdio_reset(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)
+ host->f_init = host->f_min;
+ else {
+ mmc_release_host(host);
+ goto out;
+ }
+ pr_info("%s: %s: trying to init card at %u Hz\n",
+ mmc_hostname(host), __func__, host->f_init);
- mmc_send_if_cond(host, host->ocr_avail);
+ mmc_power_up(host);
+ sdio_reset(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_claim_host(host);
- /* try SDMEM (but not MMC) even if SDIO is broken */
- if (mmc_send_app_op_cond(host, 0, &ocr))
- goto out_fail;
+ mmc_send_if_cond(host, host->ocr_avail);
+
+ /*
+ * First we search for SDIO...
+ */
+ err = mmc_send_io_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sdio(host, ocr)) {
+ mmc_claim_host(host);
+ /*
+ * Try SDMEM (but not MMC) even if SDIO
+ * is broken.
+ */
+ if (mmc_send_app_op_cond(host, 0, &ocr))
+ goto out_fail;
+
+ if (mmc_attach_sd(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;
}
- 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;
- }
- /*
- * ...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;
- }
+ /*
+ * ...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;
+ }
out_fail:
- mmc_release_host(host);
- mmc_power_off(host);
-
+ 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 2010-08-29 23:36:04.000000000 +0800
+++ b/include/linux/mmc/host.h 2010-09-03 05:16:21.000000000 +0800
@@ -123,6 +123,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;
struct notifier_block pm_notify;
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] mmc: Make ID freq configurable
2010-09-02 21:34 [PATCH v4] mmc: Make ID freq configurable Hein_Tibosch
2010-09-03 1:47 ` Chris Ball
2010-09-05 20:14 ` Matt Fleming
@ 2010-12-02 11:16 ` Sahitya Tummala
2010-12-02 21:21 ` Hein_Tibosch
2 siblings, 1 reply; 13+ messages in thread
From: Sahitya Tummala @ 2010-12-02 11:16 UTC (permalink / raw)
To: Hein_Tibosch
Cc: Andrew Morton, Pierre Ossman, Chris Ball, Ben Nizette,
Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
Hi Hein Tibosch,
On Fri, 2010-09-03 at 05:34 +0800, Hein_Tibosch wrote:
> In the latest releases of the mmc driver, the freq during initialization
> is set to a fixed 400 Khz. This was reportedly too fast for several
> users. As there doesn't seem to be an ideal frequency which-works-for-all,
> Pierre suggested to let the driver try several frequencies.
>
> This patch implements that idea. It will try mmc-initialization using
> several frequencies from an array 400, 300, 200 and 100.
> I submitted it earlier but it's now adapted to and tested with kernel
> 2.6.36-rc3.
>
> In case SDIO is broken, it'll still try to detect SDMEM, also at different
> freqs.
>
> 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 2010-08-29 23:36:04.000000000 +0800
> +++ b/drivers/mmc/core/core.c 2010-09-03 04:28:52.000000000 +0800
> @@ -907,12 +907,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);
> @@ -1404,6 +1399,8 @@ void mmc_rescan(struct work_struct *work
> u32 ocr;
> int err;
> unsigned long flags;
> + int i;
> + unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>
> spin_lock_irqsave(&host->lock, flags);
>
> @@ -1443,55 +1440,64 @@ 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);
> - sdio_reset(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);
> + sdio_reset(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_claim_host(host);
> - /* try SDMEM (but not MMC) even if SDIO is broken */
> - if (mmc_send_app_op_cond(host, 0, &ocr))
> - goto out_fail;
> + mmc_send_if_cond(host, host->ocr_avail);
> +
> + /*
> + * First we search for SDIO...
> + */
> + err = mmc_send_io_op_cond(host, 0, &ocr);
> + if (!err) {
> + if (mmc_attach_sdio(host, ocr)) {
> + mmc_claim_host(host);
> + /* try SDMEM (but not MMC) even if SDIO is broken */
> + if (mmc_send_app_op_cond(host, 0, &ocr))
In case of SDIO error, mmc_power_off() is getting called as part of
mmc_detach_bus(). Shouldn't we power up the host before checking for
SDMEM? Any comments?
> + goto out_fail;
> +
> + if (mmc_attach_sd(host, ocr))
> + mmc_power_off(host);
> + }
> + goto out;
> + }
Thanks,
Sahitya.
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-12-02 11:16 ` [PATCH v4] " Sahitya Tummala
@ 2010-12-02 21:21 ` Hein_Tibosch
2010-12-02 22:38 ` Michal Miroslaw
0 siblings, 1 reply; 13+ messages in thread
From: Hein_Tibosch @ 2010-12-02 21:21 UTC (permalink / raw)
To: Sahitya Tummala, Michal Miroslaw
Cc: Andrew Morton, Pierre Ossman, Chris Ball, Ben Nizette,
Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
Hi Sahitya,
On 2-12-2010 19:16, Sahitya Tummala wrote:
> Hi Hein Tibosch,
>
> On Fri, 2010-09-03 at 05:34 +0800, Hein_Tibosch wrote:
>> In the latest releases of the mmc driver, the freq during initialization
>> is set to a fixed 400 Khz. This was reportedly too fast for several
>> users. As there doesn't seem to be an ideal frequency which-works-for-all,
>> Pierre suggested to let the driver try several frequencies.
>> <cut>
>> + /*
>> + * First we search for SDIO...
>> + */
>> + err = mmc_send_io_op_cond(host, 0, &ocr);
>> + if (!err) {
>> + if (mmc_attach_sdio(host, ocr)) {
>> + mmc_claim_host(host);
>> + /* try SDMEM (but not MMC) even if SDIO is broken */
>> + if (mmc_send_app_op_cond(host, 0, &ocr))
> In case of SDIO error, mmc_power_off() is getting called as part of
> mmc_detach_bus(). Shouldn't we power up the host before checking for
> SDMEM? Any comments?
>
I think you should ask Michal Miroslaw, as he wrote the patch for the
SD-combo (IO + mem)*
But yes, in case of a failure, mmc_attach_sdio() will call mmc_detach_bus()
which in turn will call mmc_power_off()
And so it should be mmc_power_up() again before trying to detect SD memory.
Hein Tibosch
* http://permalink.gmane.org/gmane.linux.kernel.mmc/2052
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-12-02 21:21 ` Hein_Tibosch
@ 2010-12-02 22:38 ` Michal Miroslaw
2010-12-03 5:05 ` stummala
2010-12-17 1:09 ` Chris Ball
0 siblings, 2 replies; 13+ messages in thread
From: Michal Miroslaw @ 2010-12-02 22:38 UTC (permalink / raw)
To: Hein_Tibosch, Sahitya Tummala
Cc: Andrew Morton, Pierre Ossman, Chris Ball, Ben Nizette,
Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
On Fri, Dec 03, 2010 at 05:21:10AM +0800, Hein_Tibosch wrote:
> Hi Sahitya,
> On 2-12-2010 19:16, Sahitya Tummala wrote:
> > Hi Hein Tibosch,
> >
> > On Fri, 2010-09-03 at 05:34 +0800, Hein_Tibosch wrote:
> >> In the latest releases of the mmc driver, the freq during initialization
> >> is set to a fixed 400 Khz. This was reportedly too fast for several
> >> users. As there doesn't seem to be an ideal frequency which-works-for-all,
> >> Pierre suggested to let the driver try several frequencies.
> >> <cut>
> >> + /*
> >> + * First we search for SDIO...
> >> + */
> >> + err = mmc_send_io_op_cond(host, 0, &ocr);
> >> + if (!err) {
> >> + if (mmc_attach_sdio(host, ocr)) {
> >> + mmc_claim_host(host);
> >> + /* try SDMEM (but not MMC) even if SDIO is broken */
> >> + if (mmc_send_app_op_cond(host, 0, &ocr))
> > In case of SDIO error, mmc_power_off() is getting called as part of
> > mmc_detach_bus(). Shouldn't we power up the host before checking for
> > SDMEM? Any comments?
> I think you should ask Michal Miroslaw, as he wrote the patch for the
> SD-combo (IO + mem)*
>
> But yes, in case of a failure, mmc_attach_sdio() will call mmc_detach_bus()
> which in turn will call mmc_power_off()
>
> And so it should be mmc_power_up() again before trying to detect SD memory.
Yes, you are right! This explains some things...
Please try attached patch. (Not tested, but obvious enough.)
Best Regards,
Michał Mirosław
---
mmc: fix detection of memory part of SD-combo card with broken SDIO
In case of failure, mmc_attach_sdio() will power off the SD bus.
Power it up and reinitialize before trying SD memory detection.
Reported-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/mmc/core/core.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6286898..32a4798 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1564,6 +1564,11 @@ void mmc_rescan(struct work_struct *work)
* Try SDMEM (but not MMC) even if SDIO
* is broken.
*/
+ mmc_power_up(host);
+ sdio_reset(host);
+ mmc_go_idle(host);
+ mmc_send_if_cond(host, host->ocr_avail);
+
if (mmc_send_app_op_cond(host, 0, &ocr))
goto out_fail;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-12-02 22:38 ` Michal Miroslaw
@ 2010-12-03 5:05 ` stummala
2010-12-03 6:56 ` Michal Miroslaw
2010-12-17 1:09 ` Chris Ball
1 sibling, 1 reply; 13+ messages in thread
From: stummala @ 2010-12-03 5:05 UTC (permalink / raw)
To: Michal Miroslaw
Cc: Hein_Tibosch, Sahitya Tummala, Andrew Morton, Pierre Ossman,
Chris Ball, Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc,
linux-kernel, Matt Fleming
Hi Michal,
> On Fri, Dec 03, 2010 at 05:21:10AM +0800, Hein_Tibosch wrote:
>> Hi Sahitya,
>> On 2-12-2010 19:16, Sahitya Tummala wrote:
>> > Hi Hein Tibosch,
>> >
>> > On Fri, 2010-09-03 at 05:34 +0800, Hein_Tibosch wrote:
>> >> In the latest releases of the mmc driver, the freq during
>> initialization
>> >> is set to a fixed 400 Khz. This was reportedly too fast for several
>> >> users. As there doesn't seem to be an ideal frequency
>> which-works-for-all,
>> >> Pierre suggested to let the driver try several frequencies.
>> >> <cut>
>> >> + /*
>> >> + * First we search for SDIO...
>> >> + */
>> >> + err = mmc_send_io_op_cond(host, 0, &ocr);
>> >> + if (!err) {
>> >> + if (mmc_attach_sdio(host, ocr)) {
>> >> + mmc_claim_host(host);
>> >> + /* try SDMEM (but not MMC) even if SDIO is broken */
>> >> + if (mmc_send_app_op_cond(host, 0, &ocr))
>> > In case of SDIO error, mmc_power_off() is getting called as part of
>> > mmc_detach_bus(). Shouldn't we power up the host before checking for
>> > SDMEM? Any comments?
>> I think you should ask Michal Miroslaw, as he wrote the patch for the
>> SD-combo (IO + mem)*
>>
>> But yes, in case of a failure, mmc_attach_sdio() will call
>> mmc_detach_bus()
>> which in turn will call mmc_power_off()
>>
>> And so it should be mmc_power_up() again before trying to detect SD
>> memory.
>
> Yes, you are right! This explains some things...
>
> Please try attached patch. (Not tested, but obvious enough.)
I was looking at the code and figured out this issue. I don't have any
SD-combo card to verify your patch.
Thanks
Sahitya.
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.
> Best Regards,
> Micha³ Miros³aw
>
> ---
>
> mmc: fix detection of memory part of SD-combo card with broken SDIO
>
> In case of failure, mmc_attach_sdio() will power off the SD bus.
> Power it up and reinitialize before trying SD memory detection.
>
> Reported-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Micha³ Miros³aw <mirq-linux@rere.qmqm.pl>
> ---
> drivers/mmc/core/core.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6286898..32a4798 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1564,6 +1564,11 @@ void mmc_rescan(struct work_struct *work)
> * Try SDMEM (but not MMC) even if SDIO
> * is broken.
> */
> + mmc_power_up(host);
> + sdio_reset(host);
> + mmc_go_idle(host);
> + mmc_send_if_cond(host, host->ocr_avail);
> +
> if (mmc_send_app_op_cond(host, 0, &ocr))
> goto out_fail;
>
> --
> 1.7.2.3
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] mmc: Make ID freq configurable
2010-12-03 5:05 ` stummala
@ 2010-12-03 6:56 ` Michal Miroslaw
0 siblings, 0 replies; 13+ messages in thread
From: Michal Miroslaw @ 2010-12-03 6:56 UTC (permalink / raw)
To: stummala
Cc: Hein_Tibosch, Andrew Morton, Pierre Ossman, Chris Ball,
Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
On Thu, Dec 02, 2010 at 09:05:54PM -0800, stummala@codeaurora.org wrote:
> Hi Michal,
[...]
> >> > In case of SDIO error, mmc_power_off() is getting called as part of
> >> > mmc_detach_bus(). Shouldn't we power up the host before checking for
> >> > SDMEM? Any comments?
> >> I think you should ask Michal Miroslaw, as he wrote the patch for the
> >> SD-combo (IO + mem)*
> >>
> >> But yes, in case of a failure, mmc_attach_sdio() will call
> >> mmc_detach_bus()
> >> which in turn will call mmc_power_off()
> >>
> >> And so it should be mmc_power_up() again before trying to detect SD
> >> memory.
> > Yes, you are right! This explains some things...
> >
> > Please try attached patch. (Not tested, but obvious enough.)
> I was looking at the code and figured out this issue. I don't have any
> SD-combo card to verify your patch.
You shouldn't be able to use this code path with normal SD-combo cards.
This is relevant if you have a card that responds to CMD5 but then fails
SDIO part initialization in some circumstances (i.e. time-outs).
Best Regards,
Michał Mirosław
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] mmc: Make ID freq configurable
2010-12-02 22:38 ` Michal Miroslaw
2010-12-03 5:05 ` stummala
@ 2010-12-17 1:09 ` Chris Ball
1 sibling, 0 replies; 13+ messages in thread
From: Chris Ball @ 2010-12-17 1:09 UTC (permalink / raw)
To: Michal Miroslaw
Cc: Hein_Tibosch, Sahitya Tummala, Andrew Morton, Pierre Ossman,
Ben Nizette, Sascha Hauer, Adrian Hunter, linux-mmc, linux-kernel,
Matt Fleming
Hi Michał, Sahitya,
On Thu, Dec 02, 2010 at 11:38:12PM +0100, Michal Miroslaw wrote:
> mmc: fix detection of memory part of SD-combo card with broken SDIO
>
> In case of failure, mmc_attach_sdio() will power off the SD bus.
> Power it up and reinitialize before trying SD memory detection.
>
> Reported-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Thanks, this looks correct to me too; I've pushed it to mmc-next for .38.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 13+ messages in thread