public inbox for linux-mmc@vger.kernel.org
 help / color / mirror / Atom feed
* Re: core: negotiate ocr during resume
@ 2013-04-05  8:33 Kevin Liu
  2013-04-05  9:44 ` Prasanna NAVARATNA
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Kevin Liu @ 2013-04-05  8:33 UTC (permalink / raw)
  To: Prasanna NAVARATNA; +Cc: linux-mmc@vger.kernel.org

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Prasanna NAVARATNA
> Sent: Thursday, April 04, 2013 10:35 PM
> To: linux-mmc@vger.kernel.org
> Subject: mmc: core: negotiate ocr during resume
>
> From f43005e05f1e9d93705ec6b3ab98cfa5215c1896 Mon Sep 17 00:00:00 2001
> From: Prasanna NAVARATNA <prasanna.navaratna@broadcom.com>
> Date: Thu, 4 Apr 2013 19:55:19 +0530
> Subject: [PATCH] mmc: core: negotiate ocr during resume
>
> Save the card ocr into struct mmc_card when read from sd card and
> negotiate ocr mask during mmc_sd_init_card with host->ocr & card->ocr.
> ---
>  drivers/mmc/core/sd.c    |    7 +++++++
>  include/linux/mmc/card.h |    1 +
>  2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..8ee27e8 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -910,6 +910,10 @@ static int mmc_sd_init_card(struct mmc_host *host, u32
> ocr,
>         BUG_ON(!host);
>         WARN_ON(!host->claimed);
>
> +       /* Negotiate OCR with oldcard->ocr */
> +       if (oldcard)
> +               ocr &= oldcard->ocr;
> +

I think there are several issues here:
1. with this patch the vmmc voltage should still be 3.3v after resume
back since the vmmc voltage is updated in set_ios which depend on
mmc_select_voltage. But mmc_select_voltage didn't notice your ocr
update.
2. you didn't handle the special case that the card is changed to a
new card during suspended. So the ocr may be different here.
3. mmc.c and sdio.c should also be updated besides sd.c.

I have another old version of this patch attached below for your reference:

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index aaed768..f3c7b33 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1351,6 +1351,7 @@ void mmc_power_off(struct mmc_host *host)
         * Reset ocr mask to be the highest possible voltage supported for
         * this mmc host. This value will be used at next power up.
         */
+       host->ocr_bak = host->ocr;
        host->ocr = 1 << (fls(host->ocr_avail) - 1);

        if (!mmc_host_is_spi(host)) {
@@ -2491,7 +2492,6 @@ int mmc_resume_host(struct mmc_host *host)
        if (host->bus_ops && !host->bus_dead) {
                if (!mmc_card_keep_power(host)) {
                        mmc_power_up(host);
-                       mmc_select_voltage(host, host->ocr);
                        /*
                         * Tell runtime PM core we just powered up the card,
                         * since it still believes the card is powered off.
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index e6e3911..a8e81d2 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -882,7 +882,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
                        err = -ENOENT;
                        goto err;
                }
-
+               /* restore ocr for the same card */
+               if (host->ocr_bak && (host->ocr_bak != host->ocr))
+                       host->ocr = mmc_select_voltage(host, host->ocr_bak);
                card = oldcard;
        } else {
                /*
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 3dafb54..2eea1ae 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -908,7 +908,9 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
        if (oldcard) {
                if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
                        return -ENOENT;
-
+               /* restore ocr for the same card */
+               if (host->ocr_bak && (host->ocr_bak != host->ocr))
+                       host->ocr = mmc_select_voltage(host, host->ocr_bak);
                card = oldcard;
        } else {
                /*
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2273ce6..f66cc9a 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -594,6 +594,11 @@ static int mmc_sdio_init_card(struct mmc_host
*host, u32 ocr,
                err = mmc_send_io_op_cond(host, host->ocr, &ocr);
                if (err)
                        goto err;
+               if (oldcard) {
+                       /* restore ocr for the same card */
+                       if (host->ocr_bak && (host->ocr_bak != host->ocr))
+                               host->ocr = mmc_select_voltage(host,
host->ocr_bak);
+               }
        }

        /*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index a90ea7b..ae5533e 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -285,6 +285,7 @@ struct mmc_host {

        struct mmc_ios          ios;            /* current io bus settings */
        u32                     ocr;            /* the current OCR setting */
+       u32                     ocr_bak;        /* save current OCR setting */

        /* group bitfields together to minimize padding */
        unsigned int            use_spi_crc:1;

Thanks
Kevin

>         err = mmc_sd_get_cid(host, ocr, cid, &rocr);
>         if (err)
>                 return err;
> @@ -1185,6 +1189,9 @@ int mmc_attach_sd(struct mmc_host *host)
>                 ocr &= ~MMC_VDD_165_195;
>         }
>
> +       /* Save the card OCR */
> +       host->card->ocr = ocr;
> +
>         host->ocr = mmc_select_voltage(host, ocr);
>
>         /*
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index f31725b..1bbec2f 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -273,6 +273,7 @@ struct mmc_card {
>         u32                     raw_cid[4];     /* raw card CID */
>         u32                     raw_csd[4];     /* raw card CSD */
>         u32                     raw_scr[2];     /* raw card SCR */
> +       u32                     ocr;            /* card OCR */
>         struct mmc_cid          cid;            /* card identification */
>         struct mmc_csd          csd;            /* card specific */
>         struct mmc_ext_csd      ext_csd;        /* mmc v4 extended card
> specific */
> --
> 1.7.6
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: core: negotiate ocr during resume
@ 2013-04-07  1:55 Kevin Liu
  2013-04-08  6:41 ` Prasanna NAVARATNA
  0 siblings, 1 reply; 6+ messages in thread
From: Kevin Liu @ 2013-04-07  1:55 UTC (permalink / raw)
  To: Prasanna NAVARATNA; +Cc: linux-mmc@vger.kernel.org

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Prasanna NAVARATNA
> Sent: Friday, April 05, 2013 6:50 PM
> To: linux-mmc@vger.kernel.org
> Subject: Re: core: negotiate ocr during resume
>
> Kevin Liu <keyuan.liu <at> gmail.com> writes:
>
> Please find my updated patch and comments under _PNN_ below :-
>
>> I think there are several issues here:
>> 1. with this patch the vmmc voltage should still be 3.3v after resume
>> back since the vmmc voltage is updated in set_ios which depend on
>> mmc_select_voltage. But mmc_select_voltage didn't notice your ocr
>> update.
> _PNN_: Yes, good point. I agree. Accordingly updated the patch.
>
>> 2. you didn't handle the special case that the card is changed to a
>> new card during suspended. So the ocr may be different here.
> _PNN_ : This case doesn't holds good because even when new card is inserted
> druing suspend, cd interrupt is generated and mmc_detect_change will be
> called and mmc_rescan will be scheduled to begin initialization from the
> begining, where ocr is freshly read from new card (mmc_attach_***) and my
> patch will take care to update new ocr in card->ocr.
> Although consider an hypothetical situtaion where within the debounce time
> of cd, suppose resume gets called, it may fail. But fair enough after 200ms,
> fresh initialization will begin and card re-inits. So no need to take
> special consideration here. Don't you agree?
>

But if the card detect can't wakeup system then the cd interrupt may
be discarded if it occured during system suspended. For example, if
the card detect used slot-gpio and the slot power is off during system
suspended or card used polling for card detect.
So there must be a solution to verify whether a same card after system
resumed. In fact, there have been such method in current code:

in mmc_sd_init_card:
        if (oldcard) {
                if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
                        return -ENOENT;
                card = oldcard;

So in my patch, I moved mmc_select_voltage after this verification.
Please consider this case.

>> 3. mmc.c and sdio.c should also be updated besides sd.c.
>>
> _PNN_ : Thanks for pointing this out. Patch updated.
>
>> I have another old version of this patch attached below for your
> reference.
> _PNN_ : I do agree with your patch, but i would suggest to consider my below
> patch rather than yours for following reasons:
>  a. Very simple & less code changes.
>  b. Similar to rca, cid, csa, better to have "ocr" structure member in
> "struct mmc_card" which is card specific.
>  c. mmc_select_voltage is localised in core.c rather than moving it accross
> sd.c sdio.c and mmc.c
>  d. ocr field might be helpful in future too. So better to have it rather
> than "ocr_bak" in struct mmc_host.
>
> From ad70dbd00db355d1e8ca08e7ad12e73cb41df960 Mon Sep 17 00:00:00 2001
> From: Prasanna NAVARATNA <prasanna.navaratna@broadcom.com>
> Date: Thu, 4 Apr 2013 19:55:19 +0530
> Subject: [PATCH] mmc: core: proper ocr negotiation during resume
>
> Save the card ocr into struct mmc_card when read from card
> during initialization of sd/mmc/sdio.
> Druing mmc_resume_host, supply saved card's ocr to
> mmc_select_voltage so as to negotiate voltage appropriately.
> ---
>  drivers/mmc/core/core.c  |    2 +-
>  drivers/mmc/core/mmc.c   |    3 +++
>  drivers/mmc/core/sd.c    |    3 +++
>  drivers/mmc/core/sdio.c  |    3 +++
>  include/linux/mmc/card.h |    1 +
>  5 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index ad7decc..aad511a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2680,7 +2680,7 @@ int mmc_resume_host(struct mmc_host *host)
>         if (host->bus_ops && !host->bus_dead) {
>                 if (!mmc_card_keep_power(host)) {
>                         mmc_power_up(host);
> -                       mmc_select_voltage(host, host->ocr);
> +                       mmc_select_voltage(host, host->card->ocr);
>                         /*
>                          * Tell runtime PM core we just powered up the card,
>                          * since it still believes the card is powered off.
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index d584f7c..923eb53 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1566,6 +1566,9 @@ int mmc_attach_mmc(struct mmc_host *host)
>                 ocr &= ~0x7F;
>         }
>
> +       /* Save the card OCR */
> +       host->card->ocr = ocr;
> +
>         host->ocr = mmc_select_voltage(host, ocr);
>
>         /*
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..965504b 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1185,6 +1185,9 @@ int mmc_attach_sd(struct mmc_host *host)
>                 ocr &= ~MMC_VDD_165_195;
>         }
>
> +       /* Save the card OCR */
> +       host->card->ocr = ocr;
> +
>         host->ocr = mmc_select_voltage(host, ocr);
>
>         /*
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index aa0719a..3f9e08d 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -1090,6 +1090,9 @@ int mmc_attach_sdio(struct mmc_host *host)
>                 ocr &= ~0x7F;
>         }
>
> +       /* Save the card OCR */
> +       host->card->ocr = ocr;
> +
>         host->ocr = mmc_select_voltage(host, ocr);
>
>         /*
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index f31725b..1bbec2f 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -273,6 +273,7 @@ struct mmc_card {
>         u32                     raw_cid[4];     /* raw card CID */
>         u32                     raw_csd[4];     /* raw card CSD */
>         u32                     raw_scr[2];     /* raw card SCR */
> +       u32                     ocr;            /* card OCR */
>         struct mmc_cid          cid;            /* card identification */
>         struct mmc_csd          csd;            /* card specific */
>         struct mmc_ext_csd      ext_csd;        /* mmc v4 extended card
> specific */
> --
> 1.7.6
>
>
> Note : mmc_select_voltage(host, host->card->ocr);
> Here host->card->ocr is directly dereferenced because during resume, we will
> always have host->card !
>
> Regards,
> Prasanna
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-04-08  7:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-05  8:33 core: negotiate ocr during resume Kevin Liu
2013-04-05  9:44 ` Prasanna NAVARATNA
2013-04-05 10:37 ` Prasanna NAVARATNA
2013-04-05 10:49 ` Prasanna NAVARATNA
  -- strict thread matches above, loose matches on Subject: below --
2013-04-07  1:55 Kevin Liu
2013-04-08  6:41 ` Prasanna NAVARATNA

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox