All of lore.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

> -----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-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
  2 siblings, 0 replies; 6+ messages in thread
From: Prasanna NAVARATNA @ 2013-04-05  9:44 UTC (permalink / raw)
  To: linux-mmc

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?

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. Obviously 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: core: negotiate ocr during resume
  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
  2 siblings, 0 replies; 6+ messages in thread
From: Prasanna NAVARATNA @ 2013-04-05 10:37 UTC (permalink / raw)
  To: linux-mmc

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?

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. Obviously 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



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: core: negotiate ocr during resume
  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
  2 siblings, 0 replies; 6+ messages in thread
From: Prasanna NAVARATNA @ 2013-04-05 10:49 UTC (permalink / raw)
  To: linux-mmc

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?

> 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



^ 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, 0 replies; 6+ messages in thread
From: Prasanna NAVARATNA @ 2013-04-08  6:41 UTC (permalink / raw)
  To: linux-mmc


Kevin : 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
during suspend, cd interrupt is generated and mmc_detect_change will be
called and mmc_rescan will be scheduled to begin initialization from the
beginning, 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 situation 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?

Kevin : But if the card detect can't wakeup system then the cd interrupt may
be discarded if it occurred 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.

_PNN_ : I personally haven’t seen any system design where card insertion or 
removal doesn't generate interrupt to resume the system (I may not have seen 
enough variety of systems)
But suppose assume, mmc_detect_change, for various reasons as you mentioned 
above is not called during card insertion or removal.
Then still “oldcard” pointer is valid but old card has been replaced with 
new card. Your patch will still consider old ocr saved of old card. Right?
I didn't understand, how does your patch works and takes care of this 
situation? Please elaborate.

Thanks & Regards,
Prasanna NAVARATNA


^ permalink raw reply	[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

> -----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
2013-04-07  1:55 Kevin Liu
2013-04-08  6:41 ` Prasanna NAVARATNA

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.