[PATCH v2 0/3] Fix fixed regulators support

[PATCH v2 0/3] Fix fixed regulators support

[Marek Szyprowski]

Hello,

Commit f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b revealed that the support for
fixed regulators in SDHCI driver was working only by pure luck. My previous
patch set for fixing this issue resulted in a discussion, which has been
concluded that this issue should by fixed by extending regulators core by
regualtor_can_change_voltage() function and using it in the mmc/sdhci drivers.

For more information, please refer to the following threads:
http://thread.gmane.org/gmane.linux.kernel.mmc/17643
https://patchwork.kernel.org/patch/1739061/

Best regards
Marek Szyprowski
Samsung Poland R&D Center


Patch summary:

Marek Szyprowski (3):
  regulators: add regulator_can_change_voltage() function
  mmc: use regulator_can_change_voltage() instead of
    regulator_count_voltages() hacks
  mmc: sdhci: check voltage range only on regulators aware of voltage
    value

 drivers/mmc/core/core.c            |    2 +-
 drivers/mmc/host/sdhci.c           |    6 +++++-
 drivers/regulator/core.c           |   22 ++++++++++++++++++++++
 include/linux/regulator/consumer.h |    1 +
 4 files changed, 29 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] regulators: add regulator_can_change_voltage() function

[Marek Szyprowski]

Introduce a regulator_can_change_voltage() function for the subsytems or
drivers which might check if applying voltage change is possible and use
special workaround code when the driver is used with fixed regulators or
regulators with disabled ability to change the voltage.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c           |   22 ++++++++++++++++++++++
 include/linux/regulator/consumer.h |    1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5c4829c..24623b8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1861,6 +1861,28 @@ int regulator_is_enabled(struct regulator *regulator)
 EXPORT_SYMBOL_GPL(regulator_is_enabled);
 
 /**
+ * regulator_can_change_voltage - check if regulator can change voltage
+ * @regulator: regulator source
+ *
+ * Returns positive if the regulator driver backing the source/client
+ * can change its voltage, false otherwise. Usefull for detecting fixed
+ * or dummy regulators and disabling voltage change logic in the client
+ * driver.
+ */
+int regulator_can_change_voltage(struct regulator *regulator)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+
+	if (rdev->constraints &&
+	    rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE &&
+	    rdev->desc->n_voltages > 1)
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(regulator_can_change_voltage);
+
+/**
  * regulator_count_voltages - count regulator_list_voltage() selectors
  * @regulator: regulator source
  *
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index c43cd35..5d0f7c1 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -160,6 +160,7 @@ int regulator_bulk_force_disable(int num_consumers,
 void regulator_bulk_free(int num_consumers,
 			 struct regulator_bulk_data *consumers);
 
+int regulator_can_change_voltage(struct regulator *regulator);
 int regulator_count_voltages(struct regulator *regulator);
 int regulator_list_voltage(struct regulator *regulator, unsigned selector);
 int regulator_is_supported_voltage(struct regulator *regulator,
-- 
1.7.9.5

Re: [PATCH 1/3] regulators: add regulator_can_change_voltage() function

[Mark Brown]

On Tue, Dec 04, 2012 at 03:01:01PM +0100, Marek Szyprowski wrote:
> Introduce a regulator_can_change_voltage() function for the subsytems or
> drivers which might check if applying voltage change is possible and use
> special workaround code when the driver is used with fixed regulators or
> regulators with disabled ability to change the voltage.

Applied, thanks.  I've put it on a topic branch and will push a signed
tag called can-change-voltage or similar as soon as I can, feel free to
merge elsewhere (ideally the signed tag).

[PATCH 2/3] mmc: use regulator_can_change_voltage() instead of regulator_count_voltages() hacks

[Marek Szyprowski]

mmc_regulator_set_ocr() depends on the ability of regulator to change the
voltage value. When regulator cannot change its voltage output, some code is
skipped to avoid reporting false errors on some boards, which use MMC hosts
with fixed regulators (example: Samsung Goni and UniversalC210 boards).

This patch replaces a hacky workaround based on regulator_count_voltages()
value with the correct call to recently introduced
regulator_can_change_voltage() function in regulators core.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/mmc/core/core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index aaed768..f5a0493 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1137,7 +1137,7 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
 		 */
 		voltage = regulator_get_voltage(supply);
 
-		if (regulator_count_voltages(supply) == 1)
+		if (!regulator_can_change_voltage(supply))
 			min_uV = max_uV = voltage;
 
 		if (voltage < 0)
-- 
1.7.9.5

Re: [PATCH 2/3] mmc: use regulator_can_change_voltage() instead of regulator_count_voltages() hacks

[Marek Szyprowski]

Hello,

On 12/4/2012 3:01 PM, Marek Szyprowski wrote:
> mmc_regulator_set_ocr() depends on the ability of regulator to change the
> voltage value. When regulator cannot change its voltage output, some code is
> skipped to avoid reporting false errors on some boards, which use MMC hosts
> with fixed regulators (example: Samsung Goni and UniversalC210 boards).
>
> This patch replaces a hacky workaround based on regulator_count_voltages()
> value with the correct call to recently introduced
> regulator_can_change_voltage() function in regulators core.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Any chance to get this patch scheduled for v3.9?

> ---
>   drivers/mmc/core/core.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index aaed768..f5a0493 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1137,7 +1137,7 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
>   		 */
>   		voltage = regulator_get_voltage(supply);
>   
> -		if (regulator_count_voltages(supply) == 1)
> +		if (!regulator_can_change_voltage(supply))
>   			min_uV = max_uV = voltage;
>   
>   		if (voltage < 0)

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH 2/3] mmc: use regulator_can_change_voltage() instead of regulator_count_voltages() hacks

[Chris Ball]

Hi Marek,

On Fri, Feb 01 2013, Marek Szyprowski wrote:
> On 12/4/2012 3:01 PM, Marek Szyprowski wrote:
>> mmc_regulator_set_ocr() depends on the ability of regulator to change the
>> voltage value. When regulator cannot change its voltage output, some code is
>> skipped to avoid reporting false errors on some boards, which use MMC hosts
>> with fixed regulators (example: Samsung Goni and UniversalC210 boards).
>>
>> This patch replaces a hacky workaround based on regulator_count_voltages()
>> value with the correct call to recently introduced
>> regulator_can_change_voltage() function in regulators core.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Any chance to get this patch scheduled for v3.9?

Thanks, pushed to mmc-next for 3.9.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Marek Szyprowski]

Some regulators don't report any voltage values, so checking supported
voltage range results in disabling all SDHCI_CAN_VDD_* flags and
registration failure. This patch finally provides a correct fix for the
registration of SDHCI driver with all possible voltage regulators:
dummy, fixed and regulated without using regulator_count_voltages()
hacks.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/mmc/host/sdhci.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a9ad2cd..d244dc0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2930,7 +2930,11 @@ int sdhci_add_host(struct sdhci_host *host)
 	}
 
 #ifdef CONFIG_REGULATOR
-	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
+	/*
+	 * Voltage range check makes sense only if regulator reports
+	 * any voltage value.
+	 */
+	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
 		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
 			3600000);
 		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
-- 
1.7.9.5

Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Kevin Liu]

2012/12/4 Marek Szyprowski <m.szyprowski@samsung.com>:
> Some regulators don't report any voltage values, so checking supported
> voltage range results in disabling all SDHCI_CAN_VDD_* flags and
> registration failure. This patch finally provides a correct fix for the
> registration of SDHCI driver with all possible voltage regulators:
> dummy, fixed and regulated without using regulator_count_voltages()
> hacks.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/mmc/host/sdhci.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a9ad2cd..d244dc0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2930,7 +2930,11 @@ int sdhci_add_host(struct sdhci_host *host)
>         }
>
>  #ifdef CONFIG_REGULATOR
> -       if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> +       /*
> +        * Voltage range check makes sense only if regulator reports
> +        * any voltage value.
> +        */
> +       if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
>                 ret = regulator_is_supported_voltage(host->vmmc, 2700000,
>                         3600000);
>                 if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))

Good idea. But how about the regulator is disabled at this point? So I
suggest to change to
     if (host->vmmc && regulator_get_voltage(host->vmmc) >= 0)

Thanks
Kevin

Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Mark Brown]

On Tue, Dec 04, 2012 at 10:50:03PM +0800, Kevin Liu wrote:
> 2012/12/4 Marek Szyprowski <m.szyprowski@samsung.com>:

> > +       if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> >                 ret = regulator_is_supported_voltage(host->vmmc, 2700000,
> >                         3600000);
> >                 if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))

> Good idea. But how about the regulator is disabled at this point? So I
> suggest to change to
>      if (host->vmmc && regulator_get_voltage(host->vmmc) >= 0)

I'd not expect regulator_get_voltage() to return 0 for disabled
regulators, I'd expect it to return the voltage the regulator will have
when enabled.

Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Kevin Liu]

2012/12/5 Mark Brown <broonie@opensource.wolfsonmicro.com>
>
> On Tue, Dec 04, 2012 at 10:50:03PM +0800, Kevin Liu wrote:
> > 2012/12/4 Marek Szyprowski <m.szyprowski@samsung.com>:
>
> > > +       if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> > >                 ret = regulator_is_supported_voltage(host->vmmc,
> > > 2700000,
> > >                         3600000);
> > >                 if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>
> > Good idea. But how about the regulator is disabled at this point? So I
> > suggest to change to
> >      if (host->vmmc && regulator_get_voltage(host->vmmc) >= 0)
>
> I'd not expect regulator_get_voltage() to return 0 for disabled
> regulators, I'd expect it to return the voltage the regulator will have
> when enabled.
Understand.

Thanks
Kevin

Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Marek Szyprowski]

Hello,

On 12/4/2012 3:01 PM, Marek Szyprowski wrote:
> Some regulators don't report any voltage values, so checking supported
> voltage range results in disabling all SDHCI_CAN_VDD_* flags and
> registration failure. This patch finally provides a correct fix for the
> registration of SDHCI driver with all possible voltage regulators:
> dummy, fixed and regulated without using regulator_count_voltages()
> hacks.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Any chance to get this patch scheduled for v3.9?

> ---
>   drivers/mmc/host/sdhci.c |    6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a9ad2cd..d244dc0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2930,7 +2930,11 @@ int sdhci_add_host(struct sdhci_host *host)
>   	}
>   
>   #ifdef CONFIG_REGULATOR
> -	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> +	/*
> +	 * Voltage range check makes sense only if regulator reports
> +	 * any voltage value.
> +	 */
> +	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
>   		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
>   			3600000);
>   		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Chris Ball]

Hi Marek,

On Fri, Feb 01 2013, Marek Szyprowski wrote:
> On 12/4/2012 3:01 PM, Marek Szyprowski wrote:
>> Some regulators don't report any voltage values, so checking supported
>> voltage range results in disabling all SDHCI_CAN_VDD_* flags and
>> registration failure. This patch finally provides a correct fix for the
>> registration of SDHCI driver with all possible voltage regulators:
>> dummy, fixed and regulated without using regulator_count_voltages()
>> hacks.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Any chance to get this patch scheduled for v3.9?

This one doesn't apply -- please could you rebase/resend?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Marek Szyprowski]

Some regulators don't report any voltage values, so checking supported
voltage range results in disabling all SDHCI_CAN_VDD_* flags and
registration failure. This patch finally provides a correct fix for the
registration of SDHCI driver with all possible voltage regulators:
dummy, fixed and regulated without using regulator_count_voltages()
hacks.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/mmc/host/sdhci.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ba586ae..735526b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host)
 	}
 
 #ifdef CONFIG_REGULATOR
-	if (host->vmmc) {
+	/*
+	 * Voltage range check makes sense only if regulator reports
+	 * any voltage value.
+	 */
+	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
 		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
 			3600000);
 		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
-- 
1.7.9.5

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Guennadi Liakhovetski]

Hi Marek

On Tue, 12 Feb 2013, Marek Szyprowski wrote:

> Some regulators don't report any voltage values, so checking supported
> voltage range results in disabling all SDHCI_CAN_VDD_* flags and
> registration failure. This patch finally provides a correct fix for the
> registration of SDHCI driver with all possible voltage regulators:
> dummy, fixed and regulated without using regulator_count_voltages()
> hacks.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/mmc/host/sdhci.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ba586ae..735526b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host)
>  	}
>  
>  #ifdef CONFIG_REGULATOR
> -	if (host->vmmc) {
> +	/*
> +	 * Voltage range check makes sense only if regulator reports
> +	 * any voltage value.
> +	 */
> +	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
>  		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
>  			3600000);

Wouldn't using mmc_regulator_get_ocrmask() be a better option?

Thanks
Guennadi

>  		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> -- 
> 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 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Marek Szyprowski]

Hello,

On 2/12/2013 11:10 PM, Guennadi Liakhovetski wrote:
> Hi Marek
>
> On Tue, 12 Feb 2013, Marek Szyprowski wrote:
>
> > Some regulators don't report any voltage values, so checking supported
> > voltage range results in disabling all SDHCI_CAN_VDD_* flags and
> > registration failure. This patch finally provides a correct fix for the
> > registration of SDHCI driver with all possible voltage regulators:
> > dummy, fixed and regulated without using regulator_count_voltages()
> > hacks.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/mmc/host/sdhci.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index ba586ae..735526b 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host)
> >  	}
> >
> >  #ifdef CONFIG_REGULATOR
> > -	if (host->vmmc) {
> > +	/*
> > +	 * Voltage range check makes sense only if regulator reports
> > +	 * any voltage value.
> > +	 */
> > +	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> >  		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
> >  			3600000);
>
> Wouldn't using mmc_regulator_get_ocrmask() be a better option?

The idea behind this patch it to avoid messing ocr mask and voltage
regulators when voltage regulator is a simple on/off switch, which doesn't
report any value. This solves the serious problems with sdhci driver when
dummy regulator is enabled in kconfig, otherwise the sdhci driver
concludes that no supported voltage is available and fails to initialize.
Using mmc_regulator_get_ocrmask() won't solve this problem.

BTW, mmc_regulator_get_ocrmask() won't work with continuous range 
regulators.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center
  

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Guennadi Liakhovetski]

On Wed, 13 Feb 2013, Marek Szyprowski wrote:

> Hello,
> 
> On 2/12/2013 11:10 PM, Guennadi Liakhovetski wrote:
> > Hi Marek
> > 
> > On Tue, 12 Feb 2013, Marek Szyprowski wrote:
> > 
> > > Some regulators don't report any voltage values, so checking supported
> > > voltage range results in disabling all SDHCI_CAN_VDD_* flags and
> > > registration failure. This patch finally provides a correct fix for the
> > > registration of SDHCI driver with all possible voltage regulators:
> > > dummy, fixed and regulated without using regulator_count_voltages()
> > > hacks.
> > >
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > ---
> > >  drivers/mmc/host/sdhci.c |    6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index ba586ae..735526b 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host)
> > >  	}
> > >
> > >  #ifdef CONFIG_REGULATOR
> > > -	if (host->vmmc) {
> > > +	/*
> > > +	 * Voltage range check makes sense only if regulator reports
> > > +	 * any voltage value.
> > > +	 */
> > > +	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> > >  		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
> > >  			3600000);
> > 
> > Wouldn't using mmc_regulator_get_ocrmask() be a better option?
> 
> The idea behind this patch it to avoid messing ocr mask and voltage
> regulators when voltage regulator is a simple on/off switch, which doesn't
> report any value.

Wouldn't mmc_regulator_get_ocrmask() also report an error back in this 
case?

> This solves the serious problems with sdhci driver when
> dummy regulator is enabled in kconfig, otherwise the sdhci driver
> concludes that no supported voltage is available and fails to initialize.
> Using mmc_regulator_get_ocrmask() won't solve this problem.
> 
> BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators.

This seems like a problem, that has to be fixed...

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

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Mark Brown]

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

On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote:
> On Wed, 13 Feb 2013, Marek Szyprowski wrote:

> > BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators.

> This seems like a problem, that has to be fixed...

Indeed, what's the issue?

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

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Marek Szyprowski]

On 2/13/2013 12:35 PM, Mark Brown wrote:
> On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote:
> > On Wed, 13 Feb 2013, Marek Szyprowski wrote:
>
> > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators.
>
> > This seems like a problem, that has to be fixed...
>
> Indeed, what's the issue?

There are probably 2 issues:

1. mmc_regulator_get_ocrmask() works only with regulators which support
regulator_count_voltages() and regulator_list_voltage(). Recently 
support for
continuous regulators have been merged. Such regulators doesn't provide
regulator_list_voltage() method, but are able to change/set voltage to the
given value. I agree that they are not very common, so right now we can
probably ignore them until the first board, which uses them arrives.

2. The second issue might be related to the testing of precise voltage 
values
in the ocr mask, not the whole allowed ranges. Such issues in sdhci.c 
driver
has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503
("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I
don't know MMC core code to judge if ocr mask is used for exact voltage
checking or only for checking the voltage ranges. However someone with good
mmc subsystem knowledge should check it.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Mark Brown]

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

On Thu, Feb 14, 2013 at 09:05:43AM +0100, Marek Szyprowski wrote:

> 1. mmc_regulator_get_ocrmask() works only with regulators which support
> regulator_count_voltages() and regulator_list_voltage(). Recently
> support for
> continuous regulators have been merged. Such regulators doesn't provide
> regulator_list_voltage() method, but are able to change/set voltage to the
> given value. I agree that they are not very common, so right now we can
> probably ignore them until the first board, which uses them arrives.

OK, I think this should be changed to use regulator_is_supported_voltage()
to pick a range if list_voltage() isn't there, we don't want to list an
extremely large number of voltages so using list_voltage() for continous
regulators wouldn't make sense.

> 2. The second issue might be related to the testing of precise
> voltage values
> in the ocr mask, not the whole allowed ranges. Such issues in
> sdhci.c driver
> has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503
> ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I
> don't know MMC core code to judge if ocr mask is used for exact voltage
> checking or only for checking the voltage ranges. However someone with good
> mmc subsystem knowledge should check it.

Looking at the code I'd expect it to work with continuous regulators, if
it doesn't we should fix that.

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

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Ulf Hansson]

On 14 February 2013 09:05, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 2/13/2013 12:35 PM, Mark Brown wrote:
>>
>> On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote:
>> > On Wed, 13 Feb 2013, Marek Szyprowski wrote:
>>
>> > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range
>> > > regulators.
>>
>> > This seems like a problem, that has to be fixed...
>>
>> Indeed, what's the issue?
>
>
> There are probably 2 issues:
>
> 1. mmc_regulator_get_ocrmask() works only with regulators which support
> regulator_count_voltages() and regulator_list_voltage(). Recently support
> for
> continuous regulators have been merged. Such regulators doesn't provide
> regulator_list_voltage() method, but are able to change/set voltage to the
> given value. I agree that they are not very common, so right now we can
> probably ignore them until the first board, which uses them arrives.
>
> 2. The second issue might be related to the testing of precise voltage
> values
> in the ocr mask, not the whole allowed ranges. Such issues in sdhci.c driver
> has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503
> ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I
> don't know MMC core code to judge if ocr mask is used for exact voltage
> checking or only for checking the voltage ranges. However someone with good
> mmc subsystem knowledge should check it.
>

Not 100% sure what your problem relates too here, but I am aware of an
issue for how the mmc protocol layer are handling ocr_masks. Let me
try to describe it here:

1. During "card init" mmc_power_up will be called for telling the host
driver to provide power to the card. The level of voltage will be set
to "ocr_avail" which means the highest supported voltage by the host.
2. At the protocol layer the card init sequence tries to negotiate to
lowest possible ocr value from what the card and the host together
supports. Once done, the ocr mask value will be cached into the host
struct.
3. The host will informed about the new ocr mask from the protocol
layer with mmc_select_voltage and it somewhere here the problems
starts. No host are actually changing the voltage level at this state
(MMC_POWER_ON) which is correct since it would likely mean violation
of the spec. At the same time the protocol layer still believes the
host has switched to operate at the new voltage level.
4. So the host and the protocol layer are out of sync with regards to
the ocr mask, which is why the cached ocr_mask in the host struct is
reset when doing mmc_power_off. Otherwise the suspend/resume sequence
would have been broken.

I have been looking into a solution for the above problem, but has not
yet been able to finalize the task.

Hope this did not become more fussy now. :-)

>
> Best regards
> --
> Marek Szyprowski
> Samsung Poland R&D Center
>
>
> --
> 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

Kind regards
Ulf Hansson

Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value

[Kevin Liu]

2013/2/14 Ulf Hansson <ulf.hansson@linaro.org>:
> On 14 February 2013 09:05, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>
>> On 2/13/2013 12:35 PM, Mark Brown wrote:
>>>
>>> On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote:
>>> > On Wed, 13 Feb 2013, Marek Szyprowski wrote:
>>>
>>> > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range
>>> > > regulators.
>>>
>>> > This seems like a problem, that has to be fixed...
>>>
>>> Indeed, what's the issue?
>>
>>
>> There are probably 2 issues:
>>
>> 1. mmc_regulator_get_ocrmask() works only with regulators which support
>> regulator_count_voltages() and regulator_list_voltage(). Recently support
>> for
>> continuous regulators have been merged. Such regulators doesn't provide
>> regulator_list_voltage() method, but are able to change/set voltage to the
>> given value. I agree that they are not very common, so right now we can
>> probably ignore them until the first board, which uses them arrives.
>>
>> 2. The second issue might be related to the testing of precise voltage
>> values
>> in the ocr mask, not the whole allowed ranges. Such issues in sdhci.c driver
>> has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503
>> ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I
>> don't know MMC core code to judge if ocr mask is used for exact voltage
>> checking or only for checking the voltage ranges. However someone with good
>> mmc subsystem knowledge should check it.
>>
>
> Not 100% sure what your problem relates too here, but I am aware of an
> issue for how the mmc protocol layer are handling ocr_masks. Let me
> try to describe it here:
>
> 1. During "card init" mmc_power_up will be called for telling the host
> driver to provide power to the card. The level of voltage will be set
> to "ocr_avail" which means the highest supported voltage by the host.
> 2. At the protocol layer the card init sequence tries to negotiate to
> lowest possible ocr value from what the card and the host together
> supports. Once done, the ocr mask value will be cached into the host
> struct.
> 3. The host will informed about the new ocr mask from the protocol
> layer with mmc_select_voltage and it somewhere here the problems
> starts. No host are actually changing the voltage level at this state
> (MMC_POWER_ON) which is correct since it would likely mean violation
> of the spec. At the same time the protocol layer still believes the
> host has switched to operate at the new voltage level.

According to the spec, only during the initialization procedure, the
host is not allowed to change the operating voltage range.
In fact, mmc_select_voltage is called before init start in
mmc_attach_sd/mmc_attach_mmc/mmc_attach_sdio.
So voltage level should can be changed during MMC_POWER_ON.
sdhci.c did this way and worked well. But I noticed some other hosts
didn't change voltage for MMC_POWER_ON.
I think it should be changed.

> 4. So the host and the protocol layer are out of sync with regards to
> the ocr mask, which is why the cached ocr_mask in the host struct is
> reset when doing mmc_power_off. Otherwise the suspend/resume sequence
> would have been broken.
>

But there indeed exist issues that the host->ocr doesn't restore after
resume back and operation voltage use the highest voltage.
Just submitted below patch to fix this. Please help to review.
"[PATCH] mmc: core: restore ocr and operation voltage in resume"

Thanks
Kevin