Currently the mmc_regulator_set_ocr() fiddles with the regulator
refcount by selectively calling regulator_[enable|disable]
depending on the state of the regulator. This will confuse the
reference count if case the regulator is for example shared with
other MMC slots or user for other stuff than the MMC card.
Push regulator_[enable|disable] out into the MMC host drivers
and remove this from the MMC core so the reference count can be
trusted.
Cc: Andrew Morton <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Robert Jarzmik <[email protected]>
Cc: Sundar Iyer <[email protected]>
Cc: Bengt Jonsson <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
This has been regression compiled for U300, OMAP3, PXA3XX
defconfigs, tested on U300.
We're facing problems with our regulator reference counters if
this is not fixed.
---
drivers/mmc/core/core.c | 66 ++++++++++++++++++-----------------------
drivers/mmc/host/mmci.c | 10 ++++--
drivers/mmc/host/omap_hsmmc.c | 40 ++++++++++++++++++------
drivers/mmc/host/pxamci.c | 17 ++++++++--
4 files changed, 78 insertions(+), 55 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 569e94d..904f245 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -784,47 +784,39 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
{
int result = 0;
int min_uV, max_uV;
- int enabled;
+ int tmp;
+ int voltage;
- enabled = regulator_is_enabled(supply);
- if (enabled < 0)
- return enabled;
-
- if (vdd_bit) {
- int tmp;
- int voltage;
-
- /* REVISIT mmc_vddrange_to_ocrmask() may have set some
- * bits this regulator doesn't quite support ... don't
- * be too picky, most cards and regulators are OK with
- * a 0.1V range goof (it's a small error percentage).
- */
- tmp = vdd_bit - ilog2(MMC_VDD_165_195);
- if (tmp == 0) {
- min_uV = 1650 * 1000;
- max_uV = 1950 * 1000;
- } else {
- min_uV = 1900 * 1000 + tmp * 100 * 1000;
- max_uV = min_uV + 100 * 1000;
- }
-
- /* avoid needless changes to this voltage; the regulator
- * might not allow this operation
- */
- voltage = regulator_get_voltage(supply);
- if (voltage < 0)
- result = voltage;
- else if (voltage < min_uV || voltage > max_uV)
- result = regulator_set_voltage(supply, min_uV, max_uV);
- else
- result = 0;
+ if (!vdd_bit)
+ return 0;
- if (result == 0 && !enabled)
- result = regulator_enable(supply);
- } else if (enabled) {
- result = regulator_disable(supply);
+ /*
+ * REVISIT mmc_vddrange_to_ocrmask() may have set some
+ * bits this regulator doesn't quite support ... don't
+ * be too picky, most cards and regulators are OK with
+ * a 0.1V range goof (it's a small error percentage).
+ */
+ tmp = vdd_bit - ilog2(MMC_VDD_165_195);
+ if (tmp == 0) {
+ min_uV = 1650 * 1000;
+ max_uV = 1950 * 1000;
+ } else {
+ min_uV = 1900 * 1000 + tmp * 100 * 1000;
+ max_uV = min_uV + 100 * 1000;
}
+ /*
+ * Avoid needless changes to this voltage; the regulator
+ * might not allow this operation
+ */
+ voltage = regulator_get_voltage(supply);
+ if (voltage < 0)
+ result = voltage;
+ else if (voltage < min_uV || voltage > max_uV)
+ result = regulator_set_voltage(supply, min_uV, max_uV);
+ else
+ result = 0;
+
return result;
}
EXPORT_SYMBOL(mmc_regulator_set_ocr);
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 4917af9..5f530b1 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -467,15 +467,17 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
switch (ios->power_mode) {
case MMC_POWER_OFF:
- if(host->vcc &&
- regulator_is_enabled(host->vcc))
+ if (host->vcc)
regulator_disable(host->vcc);
break;
case MMC_POWER_UP:
#ifdef CONFIG_REGULATOR
- if (host->vcc)
- /* This implicitly enables the regulator */
+ if (host->vcc) {
+ if (regulator_enable(host->vcc))
+ dev_err(mmc_dev(mmc),
+ "could not enable regulator\n");
mmc_regulator_set_ocr(host->vcc, ios->vdd);
+ }
#endif
/*
* The translate_vdd function is not used if you have
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b032828..d8d07e1 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -247,10 +247,17 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
if (mmc_slot(host).before_set_reg)
mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
- if (power_on)
- ret = mmc_regulator_set_ocr(host->vcc, vdd);
- else
- ret = mmc_regulator_set_ocr(host->vcc, 0);
+ if (power_on) {
+ ret = regulator_enable(host->vcc);
+ if (ret)
+ dev_err(dev, "could not enable regulator\n");
+ else
+ ret = mmc_regulator_set_ocr(host->vcc, vdd);
+ } else {
+ ret = regulator_disable(host->vcc);
+ if (ret)
+ dev_err(dev, "could not disable regulator\n");
+ }
if (mmc_slot(host).after_set_reg)
mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
@@ -289,7 +296,11 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
* chips/cards need an interface voltage rail too.
*/
if (power_on) {
- ret = mmc_regulator_set_ocr(host->vcc, vdd);
+ ret = regulator_enable(host->vcc);
+ if (ret < 0)
+ dev_err(dev, "could not enable regulator\n");
+ else
+ ret = mmc_regulator_set_ocr(host->vcc, vdd);
/* Enable interface voltage rail, if needed */
if (ret == 0 && host->vcc_aux) {
ret = regulator_enable(host->vcc_aux);
@@ -297,10 +308,15 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
ret = mmc_regulator_set_ocr(host->vcc, 0);
}
} else {
+ /* Shut down the rail */
if (host->vcc_aux)
ret = regulator_disable(host->vcc_aux);
- if (ret == 0)
- ret = mmc_regulator_set_ocr(host->vcc, 0);
+ if (ret == 0) {
+ /* Then proeeed to shut down the local regulator */
+ ret = regulator_disable(host->vcc);
+ if (ret == 0)
+ ret = mmc_regulator_set_ocr(host->vcc, 0);
+ }
}
if (mmc_slot(host).after_set_reg)
@@ -340,10 +356,14 @@ static int omap_hsmmc_23_set_sleep(struct device *dev, int slot, int sleep,
if (cardsleep) {
/* VCC can be turned off if card is asleep */
- if (sleep)
- err = mmc_regulator_set_ocr(host->vcc, 0);
- else
+ if (sleep) {
+ err = regulator_disable(host->vcc);
+ } else {
+ err = regulator_enable(host->vcc);
+ if (err)
+ return err;
err = mmc_regulator_set_ocr(host->vcc, vdd);
+ }
} else
err = regulator_set_mode(host->vcc, mode);
if (err)
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 0a4e43f..b7e0216 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -99,13 +99,22 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
}
}
-static inline void pxamci_set_power(struct pxamci_host *host, unsigned int vdd)
+static inline void pxamci_set_power(struct pxamci_host *host,
+ unsigned char power_mode,
+ unsigned int vdd)
{
int on;
#ifdef CONFIG_REGULATOR
- if (host->vcc)
- mmc_regulator_set_ocr(host->vcc, vdd);
+ if (host->vcc) {
+ if (power_mode == MMC_POWER_UP) {
+ if (regulator_enable(host->vcc))
+ dev_err(mmc_dev(host->mmc),
+ "could not enable regulator\n");
+ mmc_regulator_set_ocr(host->vcc, vdd);
+ } else if (power_mode == MMC_POWER_OFF)
+ regulator_disable(host->vcc);
+ }
#endif
if (!host->vcc && host->pdata &&
gpio_is_valid(host->pdata->gpio_power)) {
@@ -492,7 +501,7 @@ static void pxamci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
if (host->power_mode != ios->power_mode) {
host->power_mode = ios->power_mode;
- pxamci_set_power(host, ios->vdd);
+ pxamci_set_power(host, ios->power_mode, ios->vdd);
if (ios->power_mode == MMC_POWER_ON)
host->cmdat |= CMDAT_INIT;
--
1.6.3.3
On Thu, Jun 24, 2010 at 05:12:17PM +0200, Linus Walleij wrote:
> Currently the mmc_regulator_set_ocr() fiddles with the regulator
> refcount by selectively calling regulator_[enable|disable]
> depending on the state of the regulator. This will confuse the
> reference count if case the regulator is for example shared with
> other MMC slots or user for other stuff than the MMC card.
> Push regulator_[enable|disable] out into the MMC host drivers
> and remove this from the MMC core so the reference count can be
> trusted.
So, the feedback from folks at the time this was originally written was
that the MMC code was unable to cope with sharing regulators since it
really needs to be able to set specific voltages. This needn't be a
showstopper since people can force a single voltage in the constraints
but it does need to be considered here.
2010/6/25 Mark Brown <[email protected]>:
> On Thu, Jun 24, 2010 at 05:12:17PM +0200, Linus Walleij wrote:
>
>> Currently the mmc_regulator_set_ocr() fiddles with the regulator
>> refcount by selectively calling regulator_[enable|disable]
>> depending on the state of the regulator. This will confuse the
>> reference count if case the regulator is for example shared with
>> other MMC slots or user for other stuff than the MMC card.
>
>> Push regulator_[enable|disable] out into the MMC host drivers
>> and remove this from the MMC core so the reference count can be
>> trusted.
>
> So, the feedback from folks at the time this was originally written was
> that the MMC code was unable to cope with sharing regulators since it
> really needs to be able to set specific voltages. ?This needn't be a
> showstopper since people can force a single voltage in the constraints
> but it does need to be considered here.
Well hm, that's not strictly true. If you only provide one standard
voltage ONLY in your OCR mask, i.e. MMC_VDD_* then you can use
the same regulator for two or more MMC cards.
Further that's a perfectly reasonable thing to do if you have e.g.
two embedded eMMC cards and you know which voltage they like
to operate on ... so share the same regulator, why not. The above
assumption comes from a slot-based world.
Another argument is that a function named
mmc_regulator_set_ocr() shouldn't be enabling/disabling regulators
anyway because it's hopeless to read the code, and the other
functions in mmc/core.c only deals with voltages, not on/off:ing.
(Maybe it's just me who have a hard time reading code like that.)
Yours,
Linus Walleij
On 26 Jun 2010, at 18:37, Linus Walleij wrote:
> 2010/6/25 Mark Brown <[email protected]>:
>>
>> So, the feedback from folks at the time this was originally written was
>> that the MMC code was unable to cope with sharing regulators since it
>> really needs to be able to set specific voltages. This needn't be a
>> showstopper since people can force a single voltage in the constraints
>> but it does need to be considered here.
> Well hm, that's not strictly true. If you only provide one standard
> voltage ONLY in your OCR mask, i.e. MMC_VDD_* then you can use
> the same regulator for two or more MMC cards.
This is what I'm saying about forcing a voltage in the constraints - the
existing code should i believe implement the above automatically.
> Further that's a perfectly reasonable thing to do if you have e.g.
> two embedded eMMC cards and you know which voltage they like
> to operate on ... so share the same regulator, why not. The above
> assumption comes from a slot-based world.
Right, but this code supports all MMC cards. To repeat what I said
above this does need to be considered here. I don't think it's a
particular problem, probably just turning it into a consumer capable
of sharing the regulator would be enough.
> Another argument is that a function named
> mmc_regulator_set_ocr() shouldn't be enabling/disabling regulators
> anyway because it's hopeless to read the code, and the other
> functions in mmc/core.c only deals with voltages, not on/off:ing.
> (Maybe it's just me who have a hard time reading code like that.)
This seems rather surprising - are you saying that no other MMC drivers
are able to manage power to the slot? There was a strong insistence
when this code was originally written that it was essential to be able to
power up and down the regulators for MMC applications.
Linus Walleij wrote:
> Currently the mmc_regulator_set_ocr() fiddles with the regulator
> refcount by selectively calling regulator_[enable|disable]
> depending on the state of the regulator. This will confuse the
> reference count if case the regulator is for example shared with
> other MMC slots or user for other stuff than the MMC card.
>
> Push regulator_[enable|disable] out into the MMC host drivers
> and remove this from the MMC core so the reference count can be
> trusted.
That is not a technical reason. It would be better to provide
more regulator support in core so there is less duplication in
the drivers.
At least it should be a separate patch.
>
> Cc: Andrew Morton <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Robert Jarzmik <[email protected]>
> Cc: Sundar Iyer <[email protected]>
> Cc: Bengt Jonsson <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> This has been regression compiled for U300, OMAP3, PXA3XX
> defconfigs, tested on U300.
>
> We're facing problems with our regulator reference counters if
> this is not fixed.
> ---
> drivers/mmc/core/core.c | 66 ++++++++++++++++++-----------------------
> drivers/mmc/host/mmci.c | 10 ++++--
> drivers/mmc/host/omap_hsmmc.c | 40 ++++++++++++++++++------
> drivers/mmc/host/pxamci.c | 17 ++++++++--
> 4 files changed, 78 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 569e94d..904f245 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -784,47 +784,39 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
> {
> int result = 0;
> int min_uV, max_uV;
> - int enabled;
> + int tmp;
> + int voltage;
>
> - enabled = regulator_is_enabled(supply);
> - if (enabled < 0)
> - return enabled;
> -
> - if (vdd_bit) {
> - int tmp;
> - int voltage;
> -
> - /* REVISIT mmc_vddrange_to_ocrmask() may have set some
> - * bits this regulator doesn't quite support ... don't
> - * be too picky, most cards and regulators are OK with
> - * a 0.1V range goof (it's a small error percentage).
> - */
> - tmp = vdd_bit - ilog2(MMC_VDD_165_195);
> - if (tmp == 0) {
> - min_uV = 1650 * 1000;
> - max_uV = 1950 * 1000;
> - } else {
> - min_uV = 1900 * 1000 + tmp * 100 * 1000;
> - max_uV = min_uV + 100 * 1000;
> - }
> -
> - /* avoid needless changes to this voltage; the regulator
> - * might not allow this operation
> - */
> - voltage = regulator_get_voltage(supply);
> - if (voltage < 0)
> - result = voltage;
> - else if (voltage < min_uV || voltage > max_uV)
> - result = regulator_set_voltage(supply, min_uV, max_uV);
> - else
> - result = 0;
> + if (!vdd_bit)
> + return 0;
>
> - if (result == 0 && !enabled)
> - result = regulator_enable(supply);
> - } else if (enabled) {
> - result = regulator_disable(supply);
> + /*
> + * REVISIT mmc_vddrange_to_ocrmask() may have set some
> + * bits this regulator doesn't quite support ... don't
> + * be too picky, most cards and regulators are OK with
> + * a 0.1V range goof (it's a small error percentage).
> + */
> + tmp = vdd_bit - ilog2(MMC_VDD_165_195);
> + if (tmp == 0) {
> + min_uV = 1650 * 1000;
> + max_uV = 1950 * 1000;
> + } else {
> + min_uV = 1900 * 1000 + tmp * 100 * 1000;
> + max_uV = min_uV + 100 * 1000;
> }
>
> + /*
> + * Avoid needless changes to this voltage; the regulator
> + * might not allow this operation
> + */
> + voltage = regulator_get_voltage(supply);
> + if (voltage < 0)
> + result = voltage;
> + else if (voltage < min_uV || voltage > max_uV)
> + result = regulator_set_voltage(supply, min_uV, max_uV);
> + else
> + result = 0;
> +
> return result;
> }
> EXPORT_SYMBOL(mmc_regulator_set_ocr);
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 4917af9..5f530b1 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -467,15 +467,17 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> switch (ios->power_mode) {
> case MMC_POWER_OFF:
> - if(host->vcc &&
> - regulator_is_enabled(host->vcc))
> + if (host->vcc)
> regulator_disable(host->vcc);
> break;
> case MMC_POWER_UP:
> #ifdef CONFIG_REGULATOR
> - if (host->vcc)
> - /* This implicitly enables the regulator */
> + if (host->vcc) {
> + if (regulator_enable(host->vcc))
> + dev_err(mmc_dev(mmc),
> + "could not enable regulator\n");
> mmc_regulator_set_ocr(host->vcc, ios->vdd);
> + }
> #endif
> /*
> * The translate_vdd function is not used if you have
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index b032828..d8d07e1 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -247,10 +247,17 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
> if (mmc_slot(host).before_set_reg)
> mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
>
> - if (power_on)
> - ret = mmc_regulator_set_ocr(host->vcc, vdd);
> - else
> - ret = mmc_regulator_set_ocr(host->vcc, 0);
> + if (power_on) {
> + ret = regulator_enable(host->vcc);
> + if (ret)
> + dev_err(dev, "could not enable regulator\n");
> + else
> + ret = mmc_regulator_set_ocr(host->vcc, vdd);
> + } else {
> + ret = regulator_disable(host->vcc);
> + if (ret)
> + dev_err(dev, "could not disable regulator\n");
> + }
Enabling the regulator before setting the voltage seems like it could
create problems, for example, enabling at an invalid voltage before
ramping to the correct voltage. Also, while regulator_enable typically
waits for the regulator to stablize, regulator_set_voltage typically
does not.
The original code was the other way around.
>
> if (mmc_slot(host).after_set_reg)
> mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
> @@ -289,7 +296,11 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
> * chips/cards need an interface voltage rail too.
> */
> if (power_on) {
> - ret = mmc_regulator_set_ocr(host->vcc, vdd);
> + ret = regulator_enable(host->vcc);
> + if (ret < 0)
> + dev_err(dev, "could not enable regulator\n");
> + else
> + ret = mmc_regulator_set_ocr(host->vcc, vdd);
> /* Enable interface voltage rail, if needed */
> if (ret == 0 && host->vcc_aux) {
> ret = regulator_enable(host->vcc_aux);
> @@ -297,10 +308,15 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
> ret = mmc_regulator_set_ocr(host->vcc, 0);
> }
> } else {
> + /* Shut down the rail */
> if (host->vcc_aux)
> ret = regulator_disable(host->vcc_aux);
> - if (ret == 0)
> - ret = mmc_regulator_set_ocr(host->vcc, 0);
> + if (ret == 0) {
> + /* Then proeeed to shut down the local regulator */
> + ret = regulator_disable(host->vcc);
> + if (ret == 0)
> + ret = mmc_regulator_set_ocr(host->vcc, 0);
> + }
> }
>
> if (mmc_slot(host).after_set_reg)
> @@ -340,10 +356,14 @@ static int omap_hsmmc_23_set_sleep(struct device *dev, int slot, int sleep,
>
> if (cardsleep) {
> /* VCC can be turned off if card is asleep */
> - if (sleep)
> - err = mmc_regulator_set_ocr(host->vcc, 0);
> - else
> + if (sleep) {
> + err = regulator_disable(host->vcc);
> + } else {
> + err = regulator_enable(host->vcc);
> + if (err)
> + return err;
> err = mmc_regulator_set_ocr(host->vcc, vdd);
> + }
> } else
> err = regulator_set_mode(host->vcc, mode);
> if (err)
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 0a4e43f..b7e0216 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -99,13 +99,22 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
> }
> }
>
> -static inline void pxamci_set_power(struct pxamci_host *host, unsigned int vdd)
> +static inline void pxamci_set_power(struct pxamci_host *host,
> + unsigned char power_mode,
> + unsigned int vdd)
> {
> int on;
>
> #ifdef CONFIG_REGULATOR
> - if (host->vcc)
> - mmc_regulator_set_ocr(host->vcc, vdd);
> + if (host->vcc) {
> + if (power_mode == MMC_POWER_UP) {
> + if (regulator_enable(host->vcc))
> + dev_err(mmc_dev(host->mmc),
> + "could not enable regulator\n");
> + mmc_regulator_set_ocr(host->vcc, vdd);
> + } else if (power_mode == MMC_POWER_OFF)
> + regulator_disable(host->vcc);
> + }
> #endif
> if (!host->vcc && host->pdata &&
> gpio_is_valid(host->pdata->gpio_power)) {
> @@ -492,7 +501,7 @@ static void pxamci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> if (host->power_mode != ios->power_mode) {
> host->power_mode = ios->power_mode;
>
> - pxamci_set_power(host, ios->vdd);
> + pxamci_set_power(host, ios->power_mode, ios->vdd);
>
> if (ios->power_mode == MMC_POWER_ON)
> host->cmdat |= CMDAT_INIT;