2010-12-02 11:32:02

by Sahitya Tummala

[permalink] [raw]
Subject: Re: [PATCH v4] mmc: Make ID freq configurable

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 <[email protected]>
>
> ---
> 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.


2010-12-02 21:53:53

by Hein_Tibosch

[permalink] [raw]
Subject: Re: [PATCH v4] mmc: Make ID freq configurable

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

2010-12-02 22:38:18

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH v4] mmc: Make ID freq configurable

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 <[email protected]>
Signed-off-by: Micha? Miros?aw <[email protected]>
---
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

2010-12-03 05:06:09

by Sahitya Tummala

[permalink] [raw]
Subject: Re: [PATCH v4] mmc: Make ID freq configurable

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 <[email protected]>
> Signed-off-by: Micha? Miros?aw <[email protected]>
> ---
> 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
>
>

2010-12-03 06:56:16

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH v4] mmc: Make ID freq configurable

On Thu, Dec 02, 2010 at 09:05:54PM -0800, [email protected] 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

2010-12-17 01:10:14

by Chris Ball

[permalink] [raw]
Subject: Re: [PATCH v4] mmc: Make ID freq configurable

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 <[email protected]>
> Signed-off-by: Michał Mirosław <[email protected]>

Thanks, this looks correct to me too; I've pushed it to mmc-next for .38.

--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child