[PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Ulf Hansson]

According to the eMMC/SD/SDIO specs the VDD voltage level must not be changed
during the initialization, without a complete power cycle of the card.

Before this patchset, some host drivers were trying to change voltage level
at MMC_POWER_ON state, which is also what the protocol layer advised them to.
This was not correctly done and is the reason to why quite messy code in
the protocol layer has been needed, to handle a re-initialization sequence,
typically triggered from a power_restore or resume.

I have tried to make each patch in the patchset as small and separate as
possible, surely the is room for improvments. Any suggestions are appreciated.

An important note; MMC_CAP2_FULL_PWR_CYCLE, will need to be set by a host
to tell the protocol layer that the host is able to perform a complete power
cycle. Thus the protocol layer will try to negotiate the lowest possible VDD
voltage level between the card and host.


Ulf Hansson (7):
  mmc: core: Let mmc_power_up|cycle take ocr as parameter
  mmc: core: Let mmc_set_signal_voltage take ocr as parameter
  mmc: core: Remove unnecessary retry mechanism at SDIO attach
  mmc: core: Cleanup code for setting ocr mask for SDIO
  mmc: core: Move cached value of the negotiated ocr mask to card
    struct
  mmc: core: Prevent violation of specs while initializing cards
  mmc: core: Collect common code for card ocr validation

 drivers/mmc/core/core.c  |   66 +++++++++++++++++--------------------
 drivers/mmc/core/core.h  |    6 ++--
 drivers/mmc/core/mmc.c   |   29 +++++-----------
 drivers/mmc/core/sd.c    |   41 +++++++----------------
 drivers/mmc/core/sdio.c  |   82 ++++++++++++++--------------------------------
 include/linux/mmc/card.h |    1 +
 include/linux/mmc/host.h |    1 -
 7 files changed, 79 insertions(+), 147 deletions(-)

-- 
1.7.9.5

[PATCH 1/7] mmc: core: Let mmc_power_up|cycle take ocr as parameter

[Ulf Hansson]

As a step to fixup the setup of the negotiated ocr mask, we need the
mmc_power_up|cycle functions to take the ocr as a parameter.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c |   24 ++++++++----------------
 drivers/mmc/core/core.h |    4 ++--
 drivers/mmc/core/mmc.c  |    4 ++--
 drivers/mmc/core/sd.c   |    4 ++--
 drivers/mmc/core/sdio.c |    4 ++--
 5 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b9b9fb6..07e83c5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1504,7 +1504,7 @@ power_cycle:
 	if (err) {
 		pr_debug("%s: Signal voltage switch failed, "
 			"power cycling card\n", mmc_hostname(host));
-		mmc_power_cycle(host);
+		mmc_power_cycle(host, host->ocr);
 	}
 
 	mmc_host_clk_release(host);
@@ -1545,22 +1545,14 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
  * If a host does all the power sequencing itself, ignore the
  * initial MMC_POWER_UP stage.
  */
-void mmc_power_up(struct mmc_host *host)
+void mmc_power_up(struct mmc_host *host, u32 ocr)
 {
-	int bit;
-
 	if (host->ios.power_mode == MMC_POWER_ON)
 		return;
 
 	mmc_host_clk_hold(host);
 
-	/* If ocr is set, we use it */
-	if (host->ocr)
-		bit = ffs(host->ocr) - 1;
-	else
-		bit = fls(host->ocr_avail) - 1;
-
-	host->ios.vdd = bit;
+	host->ios.vdd = fls(ocr) - 1;
 	if (mmc_host_is_spi(host))
 		host->ios.chip_select = MMC_CS_HIGH;
 	else
@@ -1630,12 +1622,12 @@ void mmc_power_off(struct mmc_host *host)
 	mmc_host_clk_release(host);
 }
 
-void mmc_power_cycle(struct mmc_host *host)
+void mmc_power_cycle(struct mmc_host *host, u32 ocr)
 {
 	mmc_power_off(host);
 	/* Wait at least 1 ms according to SD spec */
 	mmc_delay(1);
-	mmc_power_up(host);
+	mmc_power_up(host, ocr);
 }
 
 /*
@@ -2334,7 +2326,7 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 	pr_info("%s: %s: trying to init card at %u Hz\n",
 		mmc_hostname(host), __func__, host->f_init);
 #endif
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr_avail);
 
 	/*
 	 * Some eMMCs (with VCCQ always on) may not be reset after power up, so
@@ -2504,7 +2496,7 @@ void mmc_start_host(struct mmc_host *host)
 	if (host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)
 		mmc_power_off(host);
 	else
-		mmc_power_up(host);
+		mmc_power_up(host, host->ocr_avail);
 	mmc_detect_change(host, 0);
 }
 
@@ -2583,7 +2575,7 @@ int mmc_power_restore_host(struct mmc_host *host)
 		return -EINVAL;
 	}
 
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr);
 	ret = host->bus_ops->power_restore(host);
 
 	mmc_bus_put(host);
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 5345d15..502769a 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -46,9 +46,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
 int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
 void mmc_set_timing(struct mmc_host *host, unsigned int timing);
 void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
-void mmc_power_up(struct mmc_host *host);
+void mmc_power_up(struct mmc_host *host, u32 ocr);
 void mmc_power_off(struct mmc_host *host);
-void mmc_power_cycle(struct mmc_host *host);
+void mmc_power_cycle(struct mmc_host *host, u32 ocr);
 
 static inline void mmc_delay(unsigned int ms)
 {
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 6d02012..56e1b04 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1533,7 +1533,7 @@ static int mmc_resume(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr);
 	mmc_select_voltage(host, host->ocr);
 	err = mmc_init_card(host, host->ocr, host->card);
 	mmc_release_host(host);
@@ -1579,7 +1579,7 @@ static int mmc_runtime_resume(struct mmc_host *host)
 
 	mmc_claim_host(host);
 
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr);
 	err = mmc_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 5e8823d..af55e9e 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1099,7 +1099,7 @@ static int mmc_sd_resume(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr);
 	mmc_select_voltage(host, host->ocr);
 	err = mmc_sd_init_card(host, host->ocr, host->card);
 	mmc_release_host(host);
@@ -1144,7 +1144,7 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
 
 	mmc_claim_host(host);
 
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr);
 	err = mmc_sd_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 80d89cff..355c952 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -981,7 +981,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 
 	/* Restore power if needed */
 	if (!mmc_card_keep_power(host)) {
-		mmc_power_up(host);
+		mmc_power_up(host, host->ocr);
 		mmc_select_voltage(host, host->ocr);
 		/*
 		 * Tell runtime PM core we just powered up the card,
@@ -1108,7 +1108,7 @@ static int mmc_sdio_runtime_suspend(struct mmc_host *host)
 static int mmc_sdio_runtime_resume(struct mmc_host *host)
 {
 	/* Restore power and re-initialize. */
-	mmc_power_up(host);
+	mmc_power_up(host, host->ocr);
 	return mmc_sdio_power_restore(host);
 }
 
-- 
1.7.9.5

[PATCH 2/7] mmc: core: Let mmc_set_signal_voltage take ocr as parameter

[Ulf Hansson]

This is yet another step of restructure code to be able to fixup the
setup of the negotiated ocr mask.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c |    4 ++--
 drivers/mmc/core/core.h |    2 +-
 drivers/mmc/core/sd.c   |    3 ++-
 drivers/mmc/core/sdio.c |    3 ++-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 07e83c5..8f89e11 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1422,7 +1422,7 @@ int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
 
 }
 
-int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
+int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
 {
 	struct mmc_command cmd = {0};
 	int err = 0;
@@ -1504,7 +1504,7 @@ power_cycle:
 	if (err) {
 		pr_debug("%s: Signal voltage switch failed, "
 			"power cycling card\n", mmc_hostname(host));
-		mmc_power_cycle(host, host->ocr);
+		mmc_power_cycle(host, ocr);
 	}
 
 	mmc_host_clk_release(host);
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 502769a..443a584 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -42,7 +42,7 @@ void mmc_set_ungated(struct mmc_host *host);
 void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode);
 void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
 u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
-int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
+int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr);
 int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
 void mmc_set_timing(struct mmc_host *host, unsigned int timing);
 void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index af55e9e..5818887 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -773,7 +773,8 @@ try_again:
 	 */
 	if (!mmc_host_is_spi(host) && rocr &&
 	   ((*rocr & 0x41000000) == 0x41000000)) {
-		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
+		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
+					host->ocr);
 		if (err == -EAGAIN) {
 			retries--;
 			goto try_again;
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 355c952..15cbc418 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -664,7 +664,8 @@ try_again:
 	 * it.
 	 */
 	if (!powered_resume && (ocr & R4_18V_PRESENT) && mmc_host_uhs(host)) {
-		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
+		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
+					host->ocr);
 		if (err == -EAGAIN) {
 			sdio_reset(host);
 			mmc_go_idle(host);
-- 
1.7.9.5

[PATCH 3/7] mmc: core: Remove unnecessary retry mechanism at SDIO attach

[Ulf Hansson]

The retry and fallback mechanism when failing to switch to 1.8V
signaling voltage is handled by the SDIO card init function. Thus we
can remove the duplicated old code from the attach function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 15cbc418..26b1316 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1175,17 +1175,9 @@ int mmc_attach_sdio(struct mmc_host *host)
 		host->ocr |= R4_18V_PRESENT;
 
 	err = mmc_sdio_init_card(host, host->ocr, NULL, 0);
-	if (err) {
-		if (err == -EAGAIN) {
-			/*
-			 * Retry initialization with S18R set to 0.
-			 */
-			host->ocr &= ~R4_18V_PRESENT;
-			err = mmc_sdio_init_card(host, host->ocr, NULL, 0);
-		}
-		if (err)
-			goto err;
-	}
+	if (err)
+		goto err;
+
 	card = host->card;
 
 	/*
-- 
1.7.9.5

[PATCH 4/7] mmc: core: Cleanup code for setting ocr mask for SDIO

[Ulf Hansson]

At several places in mmc_sdio_init_card function the cached mask in
host->ocr were being updated. To simplify code, we make use of an
local ocr parameter instead.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 26b1316..9587d9f 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -593,23 +593,27 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	struct mmc_card *card;
 	int err;
 	int retries = 10;
+	u32 rocr = 0;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
+	/* to query card if 1.8V signalling is supported */
+	if (mmc_host_uhs(host))
+		ocr |= R4_18V_PRESENT;
+
 try_again:
 	if (!retries) {
 		pr_warning("%s: Skipping voltage switch\n",
 				mmc_hostname(host));
 		ocr &= ~R4_18V_PRESENT;
-		host->ocr &= ~R4_18V_PRESENT;
 	}
 
 	/*
 	 * Inform the card of the voltage
 	 */
 	if (!powered_resume) {
-		err = mmc_send_io_op_cond(host, host->ocr, &ocr);
+		err = mmc_send_io_op_cond(host, ocr, &rocr);
 		if (err)
 			goto err;
 	}
@@ -632,8 +636,8 @@ try_again:
 		goto err;
 	}
 
-	if ((ocr & R4_MEMORY_PRESENT) &&
-	    mmc_sd_get_cid(host, host->ocr & ocr, card->raw_cid, NULL) == 0) {
+	if ((rocr & R4_MEMORY_PRESENT) &&
+	    mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) == 0) {
 		card->type = MMC_TYPE_SD_COMBO;
 
 		if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
@@ -663,9 +667,9 @@ try_again:
 	 * systems that claim 1.8v signalling in fact do not support
 	 * it.
 	 */
-	if (!powered_resume && (ocr & R4_18V_PRESENT) && mmc_host_uhs(host)) {
+	if (!powered_resume && (rocr & ocr & R4_18V_PRESENT)) {
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
-					host->ocr);
+					ocr);
 		if (err == -EAGAIN) {
 			sdio_reset(host);
 			mmc_go_idle(host);
@@ -675,12 +679,10 @@ try_again:
 			goto try_again;
 		} else if (err) {
 			ocr &= ~R4_18V_PRESENT;
-			host->ocr &= ~R4_18V_PRESENT;
 		}
 		err = 0;
 	} else {
 		ocr &= ~R4_18V_PRESENT;
-		host->ocr &= ~R4_18V_PRESENT;
 	}
 
 	/*
@@ -1084,10 +1086,6 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 		goto out;
 	}
 
-	if (mmc_host_uhs(host))
-		/* to query card if 1.8V signalling is supported */
-		host->ocr |= R4_18V_PRESENT;
-
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
@@ -1170,10 +1168,6 @@ int mmc_attach_sdio(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
-	if (mmc_host_uhs(host))
-		/* to query card if 1.8V signalling is supported */
-		host->ocr |= R4_18V_PRESENT;
-
 	err = mmc_sdio_init_card(host, host->ocr, NULL, 0);
 	if (err)
 		goto err;
-- 
1.7.9.5

[PATCH 5/7] mmc: core: Move cached value of the negotiated ocr mask to card struct

[Ulf Hansson]

The negotiated ocr mask is directly related to the card. Once a card
gets removed, the mask shall be dropped. By moving the cache of the ocr
mask from the host struct to the card struct we have accomplished this.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c  |    7 ++++---
 drivers/mmc/core/mmc.c   |   19 ++++++++++---------
 drivers/mmc/core/sd.c    |   22 ++++++++++++----------
 drivers/mmc/core/sdio.c  |   26 ++++++++++++++------------
 include/linux/mmc/card.h |    1 +
 include/linux/mmc/host.h |    1 -
 6 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8f89e11..4a1e3ca 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1599,9 +1599,10 @@ 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.
+	 * this card. This value will be used at next power up.
 	 */
-	host->ocr = 1 << (fls(host->ocr_avail) - 1);
+	if (host->card)
+		host->card->ocr = 1 << (fls(host->ocr_avail) - 1);
 
 	if (!mmc_host_is_spi(host)) {
 		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
@@ -2575,7 +2576,7 @@ int mmc_power_restore_host(struct mmc_host *host)
 		return -EINVAL;
 	}
 
-	mmc_power_up(host, host->ocr);
+	mmc_power_up(host, host->card->ocr);
 	ret = host->bus_ops->power_restore(host);
 
 	mmc_bus_put(host);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 56e1b04..efc9f6f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -934,6 +934,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 			goto err;
 		}
 
+		card->ocr = ocr;
 		card->type = MMC_TYPE_MMC;
 		card->rca = 1;
 		memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
@@ -1533,9 +1534,9 @@ static int mmc_resume(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
-	mmc_power_up(host, host->ocr);
-	mmc_select_voltage(host, host->ocr);
-	err = mmc_init_card(host, host->ocr, host->card);
+	mmc_power_up(host, host->card->ocr);
+	mmc_select_voltage(host, host->card->ocr);
+	err = mmc_init_card(host, host->card->ocr, host->card);
 	mmc_release_host(host);
 
 	return err;
@@ -1579,7 +1580,7 @@ static int mmc_runtime_resume(struct mmc_host *host)
 
 	mmc_claim_host(host);
 
-	mmc_power_up(host, host->ocr);
+	mmc_power_up(host, host->card->ocr);
 	err = mmc_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
@@ -1595,7 +1596,7 @@ static int mmc_power_restore(struct mmc_host *host)
 
 	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
 	mmc_claim_host(host);
-	ret = mmc_init_card(host, host->ocr, host->card);
+	ret = mmc_init_card(host, host->card->ocr, host->card);
 	mmc_release_host(host);
 
 	return ret;
@@ -1640,7 +1641,7 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 int mmc_attach_mmc(struct mmc_host *host)
 {
 	int err;
-	u32 ocr;
+	u32 ocr, rocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
@@ -1677,12 +1678,12 @@ int mmc_attach_mmc(struct mmc_host *host)
 		ocr &= ~0x7F;
 	}
 
-	host->ocr = mmc_select_voltage(host, ocr);
+	rocr = mmc_select_voltage(host, ocr);
 
 	/*
 	 * Can we support the voltage of the card?
 	 */
-	if (!host->ocr) {
+	if (!rocr) {
 		err = -EINVAL;
 		goto err;
 	}
@@ -1690,7 +1691,7 @@ int mmc_attach_mmc(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
-	err = mmc_init_card(host, host->ocr, NULL);
+	err = mmc_init_card(host, rocr, NULL);
 	if (err)
 		goto err;
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 5818887..398065c 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -721,6 +721,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
 	int err;
 	u32 max_current;
 	int retries = 10;
+	u32 pocr = ocr;
 
 try_again:
 	if (!retries) {
@@ -774,7 +775,7 @@ try_again:
 	if (!mmc_host_is_spi(host) && rocr &&
 	   ((*rocr & 0x41000000) == 0x41000000)) {
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
-					host->ocr);
+					pocr);
 		if (err == -EAGAIN) {
 			retries--;
 			goto try_again;
@@ -936,6 +937,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		if (IS_ERR(card))
 			return PTR_ERR(card);
 
+		card->ocr = ocr;
 		card->type = MMC_TYPE_SD;
 		memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
 	}
@@ -1100,9 +1102,9 @@ static int mmc_sd_resume(struct mmc_host *host)
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
-	mmc_power_up(host, host->ocr);
-	mmc_select_voltage(host, host->ocr);
-	err = mmc_sd_init_card(host, host->ocr, host->card);
+	mmc_power_up(host, host->card->ocr);
+	mmc_select_voltage(host, host->card->ocr);
+	err = mmc_sd_init_card(host, host->card->ocr, host->card);
 	mmc_release_host(host);
 
 	return err;
@@ -1145,7 +1147,7 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
 
 	mmc_claim_host(host);
 
-	mmc_power_up(host, host->ocr);
+	mmc_power_up(host, host->card->ocr);
 	err = mmc_sd_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
@@ -1161,7 +1163,7 @@ static int mmc_sd_power_restore(struct mmc_host *host)
 
 	host->card->state &= ~MMC_STATE_HIGHSPEED;
 	mmc_claim_host(host);
-	ret = mmc_sd_init_card(host, host->ocr, host->card);
+	ret = mmc_sd_init_card(host, host->card->ocr, host->card);
 	mmc_release_host(host);
 
 	return ret;
@@ -1206,7 +1208,7 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
-	u32 ocr;
+	u32 ocr, rocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
@@ -1249,12 +1251,12 @@ int mmc_attach_sd(struct mmc_host *host)
 		ocr &= ~MMC_VDD_165_195;
 	}
 
-	host->ocr = mmc_select_voltage(host, ocr);
+	rocr = mmc_select_voltage(host, ocr);
 
 	/*
 	 * Can we support the voltage(s) of the card(s)?
 	 */
-	if (!host->ocr) {
+	if (!rocr) {
 		err = -EINVAL;
 		goto err;
 	}
@@ -1262,7 +1264,7 @@ int mmc_attach_sd(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
-	err = mmc_sd_init_card(host, host->ocr, NULL);
+	err = mmc_sd_init_card(host, rocr, NULL);
 	if (err)
 		goto err;
 
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 9587d9f..e0a135a 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -594,6 +594,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	int err;
 	int retries = 10;
 	u32 rocr = 0;
+	u32 ocr_card = ocr;
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
@@ -762,6 +763,7 @@ try_again:
 
 		card = oldcard;
 	}
+	card->ocr = ocr_card;
 	mmc_fixup_device(card, NULL);
 
 	if (card->type == MMC_TYPE_SD_COMBO) {
@@ -984,8 +986,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
 
 	/* Restore power if needed */
 	if (!mmc_card_keep_power(host)) {
-		mmc_power_up(host, host->ocr);
-		mmc_select_voltage(host, host->ocr);
+		mmc_power_up(host, host->card->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.
@@ -1003,7 +1005,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
 		sdio_reset(host);
 		mmc_go_idle(host);
-		err = mmc_sdio_init_card(host, host->ocr, host->card,
+		err = mmc_sdio_init_card(host, host->card->ocr, host->card,
 					mmc_card_keep_power(host));
 	} else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
 		/* We may have switched to 1-bit mode during suspend */
@@ -1043,7 +1045,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 static int mmc_sdio_power_restore(struct mmc_host *host)
 {
 	int ret;
-	u32 ocr;
+	u32 ocr, rocr;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
@@ -1080,13 +1082,13 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 	if (host->ocr_avail_sdio)
 		host->ocr_avail = host->ocr_avail_sdio;
 
-	host->ocr = mmc_select_voltage(host, ocr & ~0x7F);
-	if (!host->ocr) {
+	rocr = mmc_select_voltage(host, ocr & ~0x7F);
+	if (!rocr) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	ret = mmc_sdio_init_card(host, host->ocr, host->card,
+	ret = mmc_sdio_init_card(host, rocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
@@ -1107,7 +1109,7 @@ static int mmc_sdio_runtime_suspend(struct mmc_host *host)
 static int mmc_sdio_runtime_resume(struct mmc_host *host)
 {
 	/* Restore power and re-initialize. */
-	mmc_power_up(host, host->ocr);
+	mmc_power_up(host, host->card->ocr);
 	return mmc_sdio_power_restore(host);
 }
 
@@ -1130,7 +1132,7 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 int mmc_attach_sdio(struct mmc_host *host)
 {
 	int err, i, funcs;
-	u32 ocr;
+	u32 ocr, rocr;
 	struct mmc_card *card;
 
 	BUG_ON(!host);
@@ -1155,12 +1157,12 @@ int mmc_attach_sdio(struct mmc_host *host)
 		ocr &= ~0x7F;
 	}
 
-	host->ocr = mmc_select_voltage(host, ocr);
+	rocr = mmc_select_voltage(host, ocr);
 
 	/*
 	 * Can we support the voltage(s) of the card(s)?
 	 */
-	if (!host->ocr) {
+	if (!rocr) {
 		err = -EINVAL;
 		goto err;
 	}
@@ -1168,7 +1170,7 @@ int mmc_attach_sdio(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
-	err = mmc_sdio_init_card(host, host->ocr, NULL, 0);
+	err = mmc_sdio_init_card(host, rocr, NULL, 0);
 	if (err)
 		goto err;
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 842de3e..2ebf743 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -240,6 +240,7 @@ struct mmc_part {
 struct mmc_card {
 	struct mmc_host		*host;		/* the host this device belongs to */
 	struct device		dev;		/* the device */
+	u32			ocr;		/* the current OCR setting */
 	unsigned int		rca;		/* relative card address of device */
 	unsigned int		type;		/* card type */
 #define MMC_TYPE_MMC		0		/* MMC card */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 3b0c33a..1c91bbb 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -309,7 +309,6 @@ struct mmc_host {
 	spinlock_t		lock;		/* lock for claim and bus ops */
 
 	struct mmc_ios		ios;		/* current io bus settings */
-	u32			ocr;		/* the current OCR setting */
 
 	/* group bitfields together to minimize padding */
 	unsigned int		use_spi_crc:1;
-- 
1.7.9.5

[PATCH 6/7] mmc: core: Prevent violation of specs while initializing cards

[Ulf Hansson]

According to eMMC/SD/SDIO specs, the VDD (VCC) voltage level must be
maintained during the initialization sequence. If we want/need to tune
the voltage level, a complete power cycle of the card must be executed.

Most host drivers conforms to the specifications by only allowing to
change VDD voltage level at the MMC_POWER_UP state, but some also cares
about MMC_POWER_ON state, which they should'nt. This patch will not
break those drivers, but they could clean up code to better reflect
what is expected from the protocol layer.

A big re-work of the mmc_select_voltage function is done to only change
VDD voltage level if the host supports MMC_CAP2_FULL_PWR_CYCLE.
Otherwise only validation of the host and card ocr mask will be done.

A very nice side-effect of this patch is that we now don't need to
reset the negotiated ocr mask at the mmc_power_off function, since now
it will actually reflect the present voltage level, which safely can be
used at the next power up and re-initialization. Moreover, we then only
need to execute mmc_select_voltage from the attach sequence.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c |   31 +++++++++++--------------------
 drivers/mmc/core/mmc.c  |    1 -
 drivers/mmc/core/sd.c   |    1 -
 drivers/mmc/core/sdio.c |   17 ++---------------
 4 files changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 4a1e3ca..0d89ccc 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1383,21 +1383,20 @@ u32 mmc_select_voltage(struct mmc_host *host, u32 ocr)
 	int bit;
 
 	ocr &= host->ocr_avail;
+	if (!ocr) {
+		dev_warn(mmc_dev(host), "no support for card's volts\n");
+		return 0;
+	}
 
-	bit = ffs(ocr);
-	if (bit) {
-		bit -= 1;
-
+	if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) {
+		bit = ffs(ocr) - 1;
 		ocr &= 3 << bit;
-
-		mmc_host_clk_hold(host);
-		host->ios.vdd = bit;
-		mmc_set_ios(host);
-		mmc_host_clk_release(host);
+		mmc_power_cycle(host, ocr);
 	} else {
-		pr_warning("%s: host doesn't support card's voltages\n",
-				mmc_hostname(host));
-		ocr = 0;
+		bit = fls(ocr) - 1;
+		ocr &= 3 << bit;
+		if (bit != host->ios.vdd)
+			dev_warn(mmc_dev(host), "exceeding card's volts\n");
 	}
 
 	return ocr;
@@ -1596,14 +1595,6 @@ void mmc_power_off(struct mmc_host *host)
 	host->ios.clock = 0;
 	host->ios.vdd = 0;
 
-
-	/*
-	 * Reset ocr mask to be the highest possible voltage supported for
-	 * this card. This value will be used at next power up.
-	 */
-	if (host->card)
-		host->card->ocr = 1 << (fls(host->ocr_avail) - 1);
-
 	if (!mmc_host_is_spi(host)) {
 		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
 		host->ios.chip_select = MMC_CS_DONTCARE;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index efc9f6f..9149eab 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1535,7 +1535,6 @@ static int mmc_resume(struct mmc_host *host)
 
 	mmc_claim_host(host);
 	mmc_power_up(host, host->card->ocr);
-	mmc_select_voltage(host, host->card->ocr);
 	err = mmc_init_card(host, host->card->ocr, host->card);
 	mmc_release_host(host);
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 398065c..53db60a 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1103,7 +1103,6 @@ static int mmc_sd_resume(struct mmc_host *host)
 
 	mmc_claim_host(host);
 	mmc_power_up(host, host->card->ocr);
-	mmc_select_voltage(host, host->card->ocr);
 	err = mmc_sd_init_card(host, host->card->ocr, host->card);
 	mmc_release_host(host);
 
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index e0a135a..b7c19e8 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -987,7 +987,6 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	/* Restore power if needed */
 	if (!mmc_card_keep_power(host)) {
 		mmc_power_up(host, host->card->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.
@@ -1045,7 +1044,6 @@ static int mmc_sdio_resume(struct mmc_host *host)
 static int mmc_sdio_power_restore(struct mmc_host *host)
 {
 	int ret;
-	u32 ocr, rocr;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
@@ -1067,28 +1065,17 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 	 * for OLPC SD8686 (which expects a [CMD5,5,3,7] init sequence), and
 	 * harmless in other situations.
 	 *
-	 * With these steps taken, mmc_select_voltage() is also required to
-	 * restore the correct voltage setting of the card.
 	 */
 
 	sdio_reset(host);
 	mmc_go_idle(host);
 	mmc_send_if_cond(host, host->ocr_avail);
 
-	ret = mmc_send_io_op_cond(host, 0, &ocr);
+	ret = mmc_send_io_op_cond(host, 0, NULL);
 	if (ret)
 		goto out;
 
-	if (host->ocr_avail_sdio)
-		host->ocr_avail = host->ocr_avail_sdio;
-
-	rocr = mmc_select_voltage(host, ocr & ~0x7F);
-	if (!rocr) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	ret = mmc_sdio_init_card(host, rocr, host->card,
+	ret = mmc_sdio_init_card(host, host->card->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
-- 
1.7.9.5

[PATCH 7/7] mmc: core: Collect common code for card ocr validation

[Ulf Hansson]

Since mmc_select_voltage now only gets called from the attach sequence,
it makes sense to move the out of spec validations of the card ocr into
this function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c |   10 ++++++++++
 drivers/mmc/core/mmc.c  |   11 -----------
 drivers/mmc/core/sd.c   |   19 -------------------
 drivers/mmc/core/sdio.c |   10 ----------
 4 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0d89ccc..5806bfc 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1382,6 +1382,16 @@ u32 mmc_select_voltage(struct mmc_host *host, u32 ocr)
 {
 	int bit;
 
+	/*
+	 * Sanity check the voltages that the card claims to
+	 * support.
+	 */
+	if (ocr & 0x7F) {
+		dev_warn(mmc_dev(host),
+		"card claims to support voltages below defined range\n");
+		ocr &= ~0x7F;
+	}
+
 	ocr &= host->ocr_avail;
 	if (!ocr) {
 		dev_warn(mmc_dev(host), "no support for card's volts\n");
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 9149eab..69398f4 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1666,17 +1666,6 @@ int mmc_attach_mmc(struct mmc_host *host)
 			goto err;
 	}
 
-	/*
-	 * Sanity check the voltages that the card claims to
-	 * support.
-	 */
-	if (ocr & 0x7F) {
-		pr_warning("%s: card claims to support voltages "
-		       "below the defined range. These will be ignored.\n",
-		       mmc_hostname(host));
-		ocr &= ~0x7F;
-	}
-
 	rocr = mmc_select_voltage(host, ocr);
 
 	/*
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 53db60a..6ef84d0 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1231,25 +1231,6 @@ int mmc_attach_sd(struct mmc_host *host)
 			goto err;
 	}
 
-	/*
-	 * Sanity check the voltages that the card claims to
-	 * support.
-	 */
-	if (ocr & 0x7F) {
-		pr_warning("%s: card claims to support voltages "
-		       "below the defined range. These will be ignored.\n",
-		       mmc_hostname(host));
-		ocr &= ~0x7F;
-	}
-
-	if ((ocr & MMC_VDD_165_195) &&
-	    !(host->ocr_avail_sd & MMC_VDD_165_195)) {
-		pr_warning("%s: SD card claims to support the "
-		       "incompletely defined 'low voltage range'. This "
-		       "will be ignored.\n", mmc_hostname(host));
-		ocr &= ~MMC_VDD_165_195;
-	}
-
 	rocr = mmc_select_voltage(host, ocr);
 
 	/*
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index b7c19e8..4d721c6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1133,16 +1133,6 @@ int mmc_attach_sdio(struct mmc_host *host)
 	if (host->ocr_avail_sdio)
 		host->ocr_avail = host->ocr_avail_sdio;
 
-	/*
-	 * Sanity check the voltages that the card claims to
-	 * support.
-	 */
-	if (ocr & 0x7F) {
-		pr_warning("%s: card claims to support voltages "
-		       "below the defined range. These will be ignored.\n",
-		       mmc_hostname(host));
-		ocr &= ~0x7F;
-	}
 
 	rocr = mmc_select_voltage(host, ocr);
 
-- 
1.7.9.5

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Guennadi Liakhovetski]

Hi Ulf

On Mon, 16 Sep 2013, Ulf Hansson wrote:

> According to the eMMC/SD/SDIO specs the VDD voltage level must not be changed
> during the initialization, without a complete power cycle of the card.
> 
> Before this patchset, some host drivers were trying to change voltage level
> at MMC_POWER_ON state, which is also what the protocol layer advised them to.
> This was not correctly done and is the reason to why quite messy code in
> the protocol layer has been needed, to handle a re-initialization sequence,
> typically triggered from a power_restore or resume.
> 
> I have tried to make each patch in the patchset as small and separate as
> possible, surely the is room for improvments. Any suggestions are appreciated.
> 
> An important note; MMC_CAP2_FULL_PWR_CYCLE, will need to be set by a host
> to tell the protocol layer that the host is able to perform a complete power
> cycle. Thus the protocol layer will try to negotiate the lowest possible VDD
> voltage level between the card and host.

I've got a question to this one. You mean a power cycle of a card, right? 
What if the card power is supplied by a regulator. How do you tell whether 
you can power cycle it or not? Maybe you can theoretically switch that 
regulator - sometimes. On other occasions other users might be preventing 
it from really powering off. Do you really need a guarantee, that every 
time you issue a power down .set_ios() the card will really be switched 
off? I don't see how this can be possible in my example?

Thanks
Guennadi

> 
> 
> Ulf Hansson (7):
>   mmc: core: Let mmc_power_up|cycle take ocr as parameter
>   mmc: core: Let mmc_set_signal_voltage take ocr as parameter
>   mmc: core: Remove unnecessary retry mechanism at SDIO attach
>   mmc: core: Cleanup code for setting ocr mask for SDIO
>   mmc: core: Move cached value of the negotiated ocr mask to card
>     struct
>   mmc: core: Prevent violation of specs while initializing cards
>   mmc: core: Collect common code for card ocr validation
> 
>  drivers/mmc/core/core.c  |   66 +++++++++++++++++--------------------
>  drivers/mmc/core/core.h  |    6 ++--
>  drivers/mmc/core/mmc.c   |   29 +++++-----------
>  drivers/mmc/core/sd.c    |   41 +++++++----------------
>  drivers/mmc/core/sdio.c  |   82 ++++++++++++++--------------------------------
>  include/linux/mmc/card.h |    1 +
>  include/linux/mmc/host.h |    1 -
>  7 files changed, 79 insertions(+), 147 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> --
> 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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Ulf Hansson]

On 16 September 2013 22:57, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Ulf
>
> On Mon, 16 Sep 2013, Ulf Hansson wrote:
>
>> According to the eMMC/SD/SDIO specs the VDD voltage level must not be changed
>> during the initialization, without a complete power cycle of the card.
>>
>> Before this patchset, some host drivers were trying to change voltage level
>> at MMC_POWER_ON state, which is also what the protocol layer advised them to.
>> This was not correctly done and is the reason to why quite messy code in
>> the protocol layer has been needed, to handle a re-initialization sequence,
>> typically triggered from a power_restore or resume.
>>
>> I have tried to make each patch in the patchset as small and separate as
>> possible, surely the is room for improvments. Any suggestions are appreciated.
>>
>> An important note; MMC_CAP2_FULL_PWR_CYCLE, will need to be set by a host
>> to tell the protocol layer that the host is able to perform a complete power
>> cycle. Thus the protocol layer will try to negotiate the lowest possible VDD
>> voltage level between the card and host.

Hi Guennadi,

>
> I've got a question to this one. You mean a power cycle of a card, right?

Correct!

> What if the card power is supplied by a regulator. How do you tell whether
> you can power cycle it or not? Maybe you can theoretically switch that
> regulator - sometimes. On other occasions other users might be preventing
> it from really powering off. Do you really need a guarantee, that every
> time you issue a power down .set_ios() the card will really be switched
> off? I don't see how this can be possible in my example?

Yes, we need a guarantee to be able to conform to the specs.

In one of my cases I am also using a regulator, but from a board
configuration point of view I know I am the only user on this
regulator, thus I can be sure I can switch it off when I want.

I see your point though, and I am not sure how to adapt to this
configuration. I suggest you to not set the MMC_CAP2_FULL_PWR_CYCLE
for that mmc host - unless you can find a way to make sure you can cut
and restore power when needed.

Kind regards
Ulf Hansson

>
> Thanks
> Guennadi
>
>>
>>
>> Ulf Hansson (7):
>>   mmc: core: Let mmc_power_up|cycle take ocr as parameter
>>   mmc: core: Let mmc_set_signal_voltage take ocr as parameter
>>   mmc: core: Remove unnecessary retry mechanism at SDIO attach
>>   mmc: core: Cleanup code for setting ocr mask for SDIO
>>   mmc: core: Move cached value of the negotiated ocr mask to card
>>     struct
>>   mmc: core: Prevent violation of specs while initializing cards
>>   mmc: core: Collect common code for card ocr validation
>>
>>  drivers/mmc/core/core.c  |   66 +++++++++++++++++--------------------
>>  drivers/mmc/core/core.h  |    6 ++--
>>  drivers/mmc/core/mmc.c   |   29 +++++-----------
>>  drivers/mmc/core/sd.c    |   41 +++++++----------------
>>  drivers/mmc/core/sdio.c  |   82 ++++++++++++++--------------------------------
>>  include/linux/mmc/card.h |    1 +
>>  include/linux/mmc/host.h |    1 -
>>  7 files changed, 79 insertions(+), 147 deletions(-)
>>
>> --
>> 1.7.9.5
>>
>> --
>> 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
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Guennadi Liakhovetski]

(adding Mark to cc)

On Tue, 17 Sep 2013, Ulf Hansson wrote:

> On 16 September 2013 22:57, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Ulf
> >
> > On Mon, 16 Sep 2013, Ulf Hansson wrote:
> >
> >> According to the eMMC/SD/SDIO specs the VDD voltage level must not be changed
> >> during the initialization, without a complete power cycle of the card.
> >>
> >> Before this patchset, some host drivers were trying to change voltage level
> >> at MMC_POWER_ON state, which is also what the protocol layer advised them to.
> >> This was not correctly done and is the reason to why quite messy code in
> >> the protocol layer has been needed, to handle a re-initialization sequence,
> >> typically triggered from a power_restore or resume.
> >>
> >> I have tried to make each patch in the patchset as small and separate as
> >> possible, surely the is room for improvments. Any suggestions are appreciated.
> >>
> >> An important note; MMC_CAP2_FULL_PWR_CYCLE, will need to be set by a host
> >> to tell the protocol layer that the host is able to perform a complete power
> >> cycle. Thus the protocol layer will try to negotiate the lowest possible VDD
> >> voltage level between the card and host.
> 
> Hi Guennadi,
> 
> >
> > I've got a question to this one. You mean a power cycle of a card, right?
> 
> Correct!
> 
> > What if the card power is supplied by a regulator. How do you tell whether
> > you can power cycle it or not? Maybe you can theoretically switch that
> > regulator - sometimes. On other occasions other users might be preventing
> > it from really powering off. Do you really need a guarantee, that every
> > time you issue a power down .set_ios() the card will really be switched
> > off? I don't see how this can be possible in my example?
> 
> Yes, we need a guarantee to be able to conform to the specs.
> 
> In one of my cases I am also using a regulator, but from a board
> configuration point of view I know I am the only user on this
> regulator, thus I can be sure I can switch it off when I want.
> 
> I see your point though, and I am not sure how to adapt to this
> configuration. I suggest you to not set the MMC_CAP2_FULL_PWR_CYCLE
> for that mmc host - unless you can find a way to make sure you can cut
> and restore power when needed.

So, do I understand correctly, that if we get a regulator exclusively and 
it is capable of changing status (REGULATOR_CHANGE_STATUS) and it is not 
always on, then we can assume, that every call to regulator_enable() / 
regulator_disable() with a suitable use count _will_ switch power off or 
on? Maybe then we could adjust mmc_regulator_get_supply() accordingly - 
first try to get the regulator exclusively, if it fails, try shared. If 
succeeded and the conditions are satisfied - set the new PWR_CYCLE flag.

Thanks
Guennadi

> Kind regards
> Ulf Hansson
> 
> >
> > Thanks
> > Guennadi
> >
> >>
> >>
> >> Ulf Hansson (7):
> >>   mmc: core: Let mmc_power_up|cycle take ocr as parameter
> >>   mmc: core: Let mmc_set_signal_voltage take ocr as parameter
> >>   mmc: core: Remove unnecessary retry mechanism at SDIO attach
> >>   mmc: core: Cleanup code for setting ocr mask for SDIO
> >>   mmc: core: Move cached value of the negotiated ocr mask to card
> >>     struct
> >>   mmc: core: Prevent violation of specs while initializing cards
> >>   mmc: core: Collect common code for card ocr validation
> >>
> >>  drivers/mmc/core/core.c  |   66 +++++++++++++++++--------------------
> >>  drivers/mmc/core/core.h  |    6 ++--
> >>  drivers/mmc/core/mmc.c   |   29 +++++-----------
> >>  drivers/mmc/core/sd.c    |   41 +++++++----------------
> >>  drivers/mmc/core/sdio.c  |   82 ++++++++++++++--------------------------------
> >>  include/linux/mmc/card.h |    1 +
> >>  include/linux/mmc/host.h |    1 -
> >>  7 files changed, 79 insertions(+), 147 deletions(-)
> >>
> >> --
> >> 1.7.9.5

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Ulf Hansson]

On 17 September 2013 10:16, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> (adding Mark to cc)
>
> On Tue, 17 Sep 2013, Ulf Hansson wrote:
>
>> On 16 September 2013 22:57, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> > Hi Ulf
>> >
>> > On Mon, 16 Sep 2013, Ulf Hansson wrote:
>> >
>> >> According to the eMMC/SD/SDIO specs the VDD voltage level must not be changed
>> >> during the initialization, without a complete power cycle of the card.
>> >>
>> >> Before this patchset, some host drivers were trying to change voltage level
>> >> at MMC_POWER_ON state, which is also what the protocol layer advised them to.
>> >> This was not correctly done and is the reason to why quite messy code in
>> >> the protocol layer has been needed, to handle a re-initialization sequence,
>> >> typically triggered from a power_restore or resume.
>> >>
>> >> I have tried to make each patch in the patchset as small and separate as
>> >> possible, surely the is room for improvments. Any suggestions are appreciated.
>> >>
>> >> An important note; MMC_CAP2_FULL_PWR_CYCLE, will need to be set by a host
>> >> to tell the protocol layer that the host is able to perform a complete power
>> >> cycle. Thus the protocol layer will try to negotiate the lowest possible VDD
>> >> voltage level between the card and host.
>>
>> Hi Guennadi,
>>
>> >
>> > I've got a question to this one. You mean a power cycle of a card, right?
>>
>> Correct!
>>
>> > What if the card power is supplied by a regulator. How do you tell whether
>> > you can power cycle it or not? Maybe you can theoretically switch that
>> > regulator - sometimes. On other occasions other users might be preventing
>> > it from really powering off. Do you really need a guarantee, that every
>> > time you issue a power down .set_ios() the card will really be switched
>> > off? I don't see how this can be possible in my example?
>>
>> Yes, we need a guarantee to be able to conform to the specs.
>>
>> In one of my cases I am also using a regulator, but from a board
>> configuration point of view I know I am the only user on this
>> regulator, thus I can be sure I can switch it off when I want.
>>
>> I see your point though, and I am not sure how to adapt to this
>> configuration. I suggest you to not set the MMC_CAP2_FULL_PWR_CYCLE
>> for that mmc host - unless you can find a way to make sure you can cut
>> and restore power when needed.
>
> So, do I understand correctly, that if we get a regulator exclusively and
> it is capable of changing status (REGULATOR_CHANGE_STATUS) and it is not
> always on, then we can assume, that every call to regulator_enable() /
> regulator_disable() with a suitable use count _will_ switch power off or
> on? Maybe then we could adjust mmc_regulator_get_supply() accordingly -
> first try to get the regulator exclusively, if it fails, try shared. If
> succeeded and the conditions are satisfied - set the new PWR_CYCLE flag.

For SD and SDIO cards, that would be enough - but not for eMMC since
we have both VCC and VCCQ.

If you want to change voltage level for eMMC, both VCC and VCCQ must
be possible to power cycle, unless you have a hw-reset wire connected
to the eMMC which is implemented by a host->ops callback. This
complicates things for SD/SDIO as well, while trying to setup the
conditions for when to set the PWR_CYCLE flag.

BTW, thanks for bringing up this discussion! I have been thinking a
bit around this as well. :-)

Kind regards
Ulf Hansson

>
> Thanks
> Guennadi
>
>> Kind regards
>> Ulf Hansson
>>
>> >
>> > Thanks
>> > Guennadi
>> >
>> >>
>> >>
>> >> Ulf Hansson (7):
>> >>   mmc: core: Let mmc_power_up|cycle take ocr as parameter
>> >>   mmc: core: Let mmc_set_signal_voltage take ocr as parameter
>> >>   mmc: core: Remove unnecessary retry mechanism at SDIO attach
>> >>   mmc: core: Cleanup code for setting ocr mask for SDIO
>> >>   mmc: core: Move cached value of the negotiated ocr mask to card
>> >>     struct
>> >>   mmc: core: Prevent violation of specs while initializing cards
>> >>   mmc: core: Collect common code for card ocr validation
>> >>
>> >>  drivers/mmc/core/core.c  |   66 +++++++++++++++++--------------------
>> >>  drivers/mmc/core/core.h  |    6 ++--
>> >>  drivers/mmc/core/mmc.c   |   29 +++++-----------
>> >>  drivers/mmc/core/sd.c    |   41 +++++++----------------
>> >>  drivers/mmc/core/sdio.c  |   82 ++++++++++++++--------------------------------
>> >>  include/linux/mmc/card.h |    1 +
>> >>  include/linux/mmc/host.h |    1 -
>> >>  7 files changed, 79 insertions(+), 147 deletions(-)
>> >>
>> >> --
>> >> 1.7.9.5
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Tue, Sep 17, 2013 at 11:29:21AM +0200, Ulf Hansson wrote:

> If you want to change voltage level for eMMC, both VCC and VCCQ must
> be possible to power cycle, unless you have a hw-reset wire connected
> to the eMMC which is implemented by a host->ops callback. This
> complicates things for SD/SDIO as well, while trying to setup the
> conditions for when to set the PWR_CYCLE flag.

Should the hardware resets not be being converted over to be represented
as GPIOs (or the reset controller API) to factor things out?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Ulf Hansson]

On 17 September 2013 13:04, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 17, 2013 at 11:29:21AM +0200, Ulf Hansson wrote:
>
>> If you want to change voltage level for eMMC, both VCC and VCCQ must
>> be possible to power cycle, unless you have a hw-reset wire connected
>> to the eMMC which is implemented by a host->ops callback. This
>> complicates things for SD/SDIO as well, while trying to setup the
>> conditions for when to set the PWR_CYCLE flag.
>
> Should the hardware resets not be being converted over to be represented
> as GPIOs (or the reset controller API) to factor things out?

In some cases the power is controlled from inside the mmc controller,
thus writing to some of it's registers will affect the power to the
card. In other cases I believe we should be able to consider them as
GPIOs. I think this is the reason to why a specific mmc host->ops is
being used, to cover all cases.

Maybe converting them to the "reset controller API" would be suitable
for all cases, not sure.

Note that, as of today only some sdhci variants have implemented the
hw-reset support.

Kind regards
Ulf Hansson

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Ulf Hansson]

On 17 September 2013 14:50, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 17 September 2013 13:04, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Sep 17, 2013 at 11:29:21AM +0200, Ulf Hansson wrote:
>>
>>> If you want to change voltage level for eMMC, both VCC and VCCQ must
>>> be possible to power cycle, unless you have a hw-reset wire connected
>>> to the eMMC which is implemented by a host->ops callback. This
>>> complicates things for SD/SDIO as well, while trying to setup the
>>> conditions for when to set the PWR_CYCLE flag.
>>
>> Should the hardware resets not be being converted over to be represented
>> as GPIOs (or the reset controller API) to factor things out?
>
> In some cases the power is controlled from inside the mmc controller,
> thus writing to some of it's registers will affect the power to the
> card. In other cases I believe we should be able to consider them as
> GPIOs. I think this is the reason to why a specific mmc host->ops is
> being used, to cover all cases.
>
> Maybe converting them to the "reset controller API" would be suitable
> for all cases, not sure.
>
> Note that, as of today only some sdhci variants have implemented the
> hw-reset support.
>
> Kind regards
> Ulf Hansson

Hi Mark & Guennadi,

I appreciated you ideas and thoughts around this patchset.

To go forward, I suppose our discussion shall not prevent Chris from
merging this patchset? We shall be able to address your input as work
on top if this patchset, right?

Kind regards
Ulf Hansson

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

On Fri, Sep 27, 2013 at 11:56:26AM +0200, Ulf Hansson wrote:

> To go forward, I suppose our discussion shall not prevent Chris from
> merging this patchset? We shall be able to address your input as work
> on top if this patchset, right?

To be honest I don't think I saw the original patches so I can't offer
much comment either way but that seems like a sensible idea.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/7] mmc: core: Fixup ocr mask setup to prevent spec violation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

On Tue, Sep 17, 2013 at 10:16:06AM +0200, Guennadi Liakhovetski wrote:

*Please* remember to trim context when replying.

> On Tue, 17 Sep 2013, Ulf Hansson wrote:
> 
> > In one of my cases I am also using a regulator, but from a board
> > configuration point of view I know I am the only user on this
> > regulator, thus I can be sure I can switch it off when I want.

> So, do I understand correctly, that if we get a regulator exclusively and 
> it is capable of changing status (REGULATOR_CHANGE_STATUS) and it is not 
> always on, then we can assume, that every call to regulator_enable() / 
> regulator_disable() with a suitable use count _will_ switch power off or 
> on? Maybe then we could adjust mmc_regulator_get_supply() accordingly - 
> first try to get the regulator exclusively, if it fails, try shared. If 
> succeeded and the conditions are satisfied - set the new PWR_CYCLE flag.

This use case is what regulator_get_exclusive() was added for.  The
method you are suggesting there is not a good one, though - if the
supply is shared it will cause the first MMC controller to get the
regulator and the others to fail.  However I think we should probably
fix this by having code in the regulator core which checks if multiple
consumers are mapped and refuses to allow get_exclusive() in that case.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]