> From: [email protected] [mailto:[email protected]] On Behalf Of Chris Ball
> Sent: Tuesday, November 13, 2012 10:14 PM
> To: Marek Szyprowski
> Cc: [email protected]; [email protected]; Kyungmin Park; Mark Brown; Liam Girdwood; Philip Rakity
> Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
>
> Hi,
>
> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>>> > Fixed regulators cannot change their voltage, so disable all voltage
>>> > range checking for them, otherwise the driver fails to operate with
>>> > fixed regulators. Up to now it worked only by luck, because
>>> > regulator_is_supported_voltage() function returned incorrect values.
>>> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
>>> > fixed that function and now additional check is needed for fixed
>>> > regulators.
>>> >
>>> > Signed-off-by: Marek Szyprowski <[email protected]>
>>> > ---
>>> > drivers/mmc/host/sdhci.c | 2 +-
>>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> > index c7851c0..6f6534e 100644
>>> > --- a/drivers/mmc/host/sdhci.c
>>> > +++ b/drivers/mmc/host/sdhci.c
>>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>>> > regulator_enable(host->vmmc);
>>> >
>>> > #ifdef CONFIG_REGULATOR
>>> > - if (host->vmmc) {
>>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>>> > ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>>> > 3300000);
>>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>>
>>> Thanks for the longer explanation. I'm still missing something, though;
>>> what's wrong with running the check as it was with the new regulator code?
>>> (I haven't tried it yet.)
>>>
>>> #ifdef CONFIG_REGULATOR
>>> if (host->vmmc) {
>>> ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>>> 3300000);
>>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>> caps[0] &= ~SDHCI_CAN_VDD_330;
>>> ret = regulator_is_supported_voltage(host->vmmc, 3000000,
>>> 3000000);
>>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>>> caps[0] &= ~SDHCI_CAN_VDD_300;
>>> ret = regulator_is_supported_voltage(host->vmmc, 1800000,
>>> 1800000);
>>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
>>> caps[0] &= ~SDHCI_CAN_VDD_180;
>>> }
>>> #endif /* CONFIG_REGULATOR */
>>>
>>> The point is to remove unsupported voltages, so if someone sets up a
>>> fixed regulator at 3300000, all of the other caps are disabled. Why
>>> wouldn't that work without this change, and how are we supposed to
>>> remove those caps on a fixed regulator after your patchset?
>>>
>>> Thanks, sorry if I'm missing something obvious,
>>
>> On our boards eMMC is connected to fixed 2.8V regulator, what results in
>> clearing all available voltages and fail. The same situation is when one
>> enable dummy regulator and try to use sdhci with it. My patch fixes this
>> and restores sdhci to working state as it was before (before fixing
>> regulator regulator_is_supported_voltage() function and earlier when
>> MMC_BROKEN_VOLATGE capability was used).
>
> I see. Sounds like a separate bug -- Philip (or anyone else), any
> idea how we should be treating eMMCs with a fixed voltage here?
>
I think we should check the voltage range rather than the voltage
point accoring to the spec.
Otherwise some valid voltage like 2.8v will be discarded by mistake.
My below old patch aim to fix this issue.
How do you think?
-----Original Message-----
From: Kevin Liu [mailto:[email protected]]
Sent: Friday, September 28, 2012 3:56 PM
To: [email protected]; [email protected]; [email protected];
[email protected]; Zhangfei Gao
Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
range according to spec
From: Kevin Liu <[email protected]>
For regulator vmmc/vmmcq, use voltage range as below
3.3v/3.0v: (2.7v, 3.6v)
1.8v: (1.7v, 1.95v)
Original code use the specific value which may fail in regulator
driver if it does NOT support the specific voltage.
Signed-off-by: Jialing Fu <[email protected]>
Signed-off-by: Kevin Liu <[email protected]>
---
drivers/mmc/host/sdhci.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3aef580..36afd47 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1628,7 +1628,7 @@ static int
sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host,
sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
if (host->vqmmc) {
- ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000);
+ ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
if (ret) {
pr_warning("%s: Switching to 3.3V signalling voltage "
" failed\n", mmc_hostname(host->mmc));
@@ -1672,7 +1672,7 @@ static int
sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host,
*/
if (host->vqmmc)
ret = regulator_set_voltage(host->vqmmc,
- 1800000, 1800000);
+ 1700000, 1950000);
else
ret = 0;
@@ -2856,7 +2856,7 @@ int sdhci_add_host(struct sdhci_host *host)
pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
host->vqmmc = NULL;
}
- else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
+ else if (regulator_is_supported_voltage(host->vqmmc, 1700000, 1950000))
regulator_enable(host->vqmmc);
else
caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
@@ -2927,16 +2927,14 @@ int sdhci_add_host(struct sdhci_host *host)
#ifdef CONFIG_REGULATOR
if (host->vmmc) {
- ret = regulator_is_supported_voltage(host->vmmc, 3300000,
- 3300000);
+ ret = regulator_is_supported_voltage(host->vmmc, 2700000,
+ 3600000);
if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_330))
caps[0] &= ~SDHCI_CAN_VDD_330;
- ret = regulator_is_supported_voltage(host->vmmc, 3000000,
- 3000000);
if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_300))
caps[0] &= ~SDHCI_CAN_VDD_300;
- ret = regulator_is_supported_voltage(host->vmmc, 1800000,
- 1800000);
+ ret = regulator_is_supported_voltage(host->vmmc, 1700000,
+ 1950000);
if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180))
caps[0] &= ~SDHCI_CAN_VDD_180;
}
--
1.7.0.4
On Wed, Nov 14, 2012 at 03:11:37PM +0800, Kevin Liu wrote:
> - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000);
> + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
Should this be regulator_set_voltage_tol()? Otherwise it'd be good to
explain where the numbers come from.
> + ret = regulator_is_supported_voltage(host->vmmc, 1700000,
> + 1950000);
We should really add a regulator_is_supported_voltage_tol... let me
just do that.
2012/11/14 Mark Brown <[email protected]>:
> On Wed, Nov 14, 2012 at 03:11:37PM +0800, Kevin Liu wrote:
>
>> - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000);
>> + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
>
> Should this be regulator_set_voltage_tol()? Otherwise it'd be good to
> explain where the numbers come from.
In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for
voltage range is defined as below:
Vdd(min) = 2.7V while Vdd(max) = 3.6V.
The card should work within the voltage range.
If you are afraid the voltage value is too aggressive, maybe we can
use regulator_set_voltage_tol() to set a smaller range.
But which range should be reasonable?
>> + ret = regulator_is_supported_voltage(host->vmmc, 1700000,
>> + 1950000);
>
> We should really add a regulator_is_supported_voltage_tol... let me
> just do that.
On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote:
> 2012/11/14 Mark Brown <[email protected]>:
> > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to
> > explain where the numbers come from.
> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for
> voltage range is defined as below:
> Vdd(min) = 2.7V while Vdd(max) = 3.6V.
> The card should work within the voltage range.
> If you are afraid the voltage value is too aggressive, maybe we can
> use regulator_set_voltage_tol() to set a smaller range.
> But which range should be reasonable?
The above makes total sense - thanks! I just wasn't aware that the
range was specified in this fashion in the spec. Might be worth a
comment in the code if you need to respin.
2012/11/14 Mark Brown <[email protected]>:
> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote:
>> 2012/11/14 Mark Brown <[email protected]>:
>
>> > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to
>> > explain where the numbers come from.
>
>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for
>> voltage range is defined as below:
>> Vdd(min) = 2.7V while Vdd(max) = 3.6V.
>> The card should work within the voltage range.
>
>> If you are afraid the voltage value is too aggressive, maybe we can
>> use regulator_set_voltage_tol() to set a smaller range.
>> But which range should be reasonable?
>
> The above makes total sense - thanks! I just wasn't aware that the
> range was specified in this fashion in the spec. Might be worth a
> comment in the code if you need to respin.
Sure, I will update the patch. Thanks!
On Nov 14, 2012, at 8:57 AM, Kevin Liu <[email protected]> wrote:
> 2012/11/14 Mark Brown <[email protected]>:
>> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote:
>>> 2012/11/14 Mark Brown <[email protected]>:
>>
>>>> Should this be regulator_set_voltage_tol()? Otherwise it'd be good to
>>>> explain where the numbers come from.
>>
>>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for
>>> voltage range is defined as below:
>>> Vdd(min) = 2.7V while Vdd(max) = 3.6V.
>>> The card should work within the voltage range.
>>
>>> If you are afraid the voltage value is too aggressive, maybe we can
>>> use regulator_set_voltage_tol() to set a smaller range.
>>> But which range should be reasonable?
>>
>> The above makes total sense - thanks! I just wasn't aware that the
>> range was specified in this fashion in the spec. Might be worth a
>> comment in the code if you need to respin.
>
> Sure, I will update the patch. Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
re-read spec. Please apply Kevin;s patch.
Reviewed-by: Philip Rakity <[email protected]>
2012/11/14 Philip Rakity <[email protected]>:
>
> On Nov 14, 2012, at 8:57 AM, Kevin Liu <[email protected]> wrote:
>
>> 2012/11/14 Mark Brown <[email protected]>:
>>> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote:
>>>> 2012/11/14 Mark Brown <[email protected]>:
>>>
>>>>> Should this be regulator_set_voltage_tol()? Otherwise it'd be good to
>>>>> explain where the numbers come from.
>>>
>>>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for
>>>> voltage range is defined as below:
>>>> Vdd(min) = 2.7V while Vdd(max) = 3.6V.
>>>> The card should work within the voltage range.
>>>
>>>> If you are afraid the voltage value is too aggressive, maybe we can
>>>> use regulator_set_voltage_tol() to set a smaller range.
>>>> But which range should be reasonable?
>>>
>>> The above makes total sense - thanks! I just wasn't aware that the
>>> range was specified in this fashion in the spec. Might be worth a
>>> comment in the code if you need to respin.
>>
>> Sure, I will update the patch. Thanks!
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> re-read spec. Please apply Kevin;s patch.
>
> Reviewed-by: Philip Rakity <[email protected]>
Philip, thanks!
Hello,
On 11/14/2012 8:11 AM, Kevin Liu wrote:
> > From: [email protected] [mailto:[email protected]] On Behalf Of Chris Ball
> > Sent: Tuesday, November 13, 2012 10:14 PM
> > To: Marek Szyprowski
> > Cc: [email protected]; [email protected]; Kyungmin Park; Mark Brown; Liam Girdwood; Philip Rakity
> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
> >
> > Hi,
> >
> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >>> > Fixed regulators cannot change their voltage, so disable all voltage
> >>> > range checking for them, otherwise the driver fails to operate with
> >>> > fixed regulators. Up to now it worked only by luck, because
> >>> > regulator_is_supported_voltage() function returned incorrect values.
> >>> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
> >>> > fixed that function and now additional check is needed for fixed
> >>> > regulators.
> >>> >
> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
> >>> > ---
> >>> > drivers/mmc/host/sdhci.c | 2 +-
> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >>> >
> >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>> > index c7851c0..6f6534e 100644
> >>> > --- a/drivers/mmc/host/sdhci.c
> >>> > +++ b/drivers/mmc/host/sdhci.c
> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >>> > regulator_enable(host->vmmc);
> >>> >
> >>> > #ifdef CONFIG_REGULATOR
> >>> > - if (host->vmmc) {
> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> >>> > ret = regulator_is_supported_voltage(host->vmmc, 3300000,
> >>> > 3300000);
> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> >>>
> >>> Thanks for the longer explanation. I'm still missing something, though;
> >>> what's wrong with running the check as it was with the new regulator code?
> >>> (I haven't tried it yet.)
> >>>
> >>> #ifdef CONFIG_REGULATOR
> >>> if (host->vmmc) {
> >>> ret = regulator_is_supported_voltage(host->vmmc, 3300000,
> >>> 3300000);
> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
> >>> ret = regulator_is_supported_voltage(host->vmmc, 3000000,
> >>> 3000000);
> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
> >>> ret = regulator_is_supported_voltage(host->vmmc, 1800000,
> >>> 1800000);
> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
> >>> }
> >>> #endif /* CONFIG_REGULATOR */
> >>>
> >>> The point is to remove unsupported voltages, so if someone sets up a
> >>> fixed regulator at 3300000, all of the other caps are disabled. Why
> >>> wouldn't that work without this change, and how are we supposed to
> >>> remove those caps on a fixed regulator after your patchset?
> >>>
> >>> Thanks, sorry if I'm missing something obvious,
> >>
> >> On our boards eMMC is connected to fixed 2.8V regulator, what results in
> >> clearing all available voltages and fail. The same situation is when one
> >> enable dummy regulator and try to use sdhci with it. My patch fixes this
> >> and restores sdhci to working state as it was before (before fixing
> >> regulator regulator_is_supported_voltage() function and earlier when
> >> MMC_BROKEN_VOLATGE capability was used).
> >
> > I see. Sounds like a separate bug -- Philip (or anyone else), any
> > idea how we should be treating eMMCs with a fixed voltage here?
> >
>
> I think we should check the voltage range rather than the voltage
> point accoring to the spec.
> Otherwise some valid voltage like 2.8v will be discarded by mistake.
> My below old patch aim to fix this issue.
> How do you think?
>
> -----Original Message-----
> From: Kevin Liu [mailto:[email protected]]
> Sent: Friday, September 28, 2012 3:56 PM
> To: [email protected]; [email protected]; [email protected];
> [email protected]; Zhangfei Gao
> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
> range according to spec
>
> From: Kevin Liu <[email protected]>
>
> For regulator vmmc/vmmcq, use voltage range as below
> 3.3v/3.0v: (2.7v, 3.6v)
> 1.8v: (1.7v, 1.95v)
> Original code use the specific value which may fail in regulator
> driver if it does NOT support the specific voltage.
>
> Signed-off-by: Jialing Fu <[email protected]>
> Signed-off-by: Kevin Liu <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
This patch restores sdhci devices to working state on Samsung boards
(tested on GONI and UniversalC210) after merging "regulator: fix voltage
check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
merged before the final v3.7 is out.
> ---
> drivers/mmc/host/sdhci.c | 16 +++++++---------
> 1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 3aef580..36afd47 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1628,7 +1628,7 @@ static int
> sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host,
> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>
> if (host->vqmmc) {
> - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000);
> + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000);
> if (ret) {
> pr_warning("%s: Switching to 3.3V signalling voltage "
> " failed\n", mmc_hostname(host->mmc));
> @@ -1672,7 +1672,7 @@ static int
> sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host,
> */
> if (host->vqmmc)
> ret = regulator_set_voltage(host->vqmmc,
> - 1800000, 1800000);
> + 1700000, 1950000);
> else
> ret = 0;
>
> @@ -2856,7 +2856,7 @@ int sdhci_add_host(struct sdhci_host *host)
> pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> host->vqmmc = NULL;
> }
> - else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
> + else if (regulator_is_supported_voltage(host->vqmmc, 1700000, 1950000))
> regulator_enable(host->vqmmc);
> else
> caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> @@ -2927,16 +2927,14 @@ int sdhci_add_host(struct sdhci_host *host)
>
> #ifdef CONFIG_REGULATOR
> if (host->vmmc) {
> - ret = regulator_is_supported_voltage(host->vmmc, 3300000,
> - 3300000);
> + ret = regulator_is_supported_voltage(host->vmmc, 2700000,
> + 3600000);
> if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_330))
> caps[0] &= ~SDHCI_CAN_VDD_330;
> - ret = regulator_is_supported_voltage(host->vmmc, 3000000,
> - 3000000);
> if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_300))
> caps[0] &= ~SDHCI_CAN_VDD_300;
> - ret = regulator_is_supported_voltage(host->vmmc, 1800000,
> - 1800000);
> + ret = regulator_is_supported_voltage(host->vmmc, 1700000,
> + 1950000);
> if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180))
> caps[0] &= ~SDHCI_CAN_VDD_180;
> }
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
2012/11/20 Marek Szyprowski <[email protected]>:
> Hello,
>
>
> On 11/14/2012 8:11 AM, Kevin Liu wrote:
>>
>> > From: [email protected]
>> > [mailto:[email protected]] On Behalf Of Chris Ball
>> > Sent: Tuesday, November 13, 2012 10:14 PM
>> > To: Marek Szyprowski
>> > Cc: [email protected]; [email protected]; Kyungmin
>> > Park; Mark Brown; Liam Girdwood; Philip Rakity
>> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only for
>> > non-fixed regulators
>> >
>> > Hi,
>> >
>> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >>> > Fixed regulators cannot change their voltage, so disable all voltage
>> >>> > range checking for them, otherwise the driver fails to operate with
>> >>> > fixed regulators. Up to now it worked only by luck, because
>> >>> > regulator_is_supported_voltage() function returned incorrect values.
>> >>> > Commit "regulator: fix voltage check in
>> >>> > regulator_is_supported_voltage()"
>> >>> > fixed that function and now additional check is needed for fixed
>> >>> > regulators.
>> >>> >
>> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
>> >>> > ---
>> >>> > drivers/mmc/host/sdhci.c | 2 +-
>> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>> >
>> >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> >>> > index c7851c0..6f6534e 100644
>> >>> > --- a/drivers/mmc/host/sdhci.c
>> >>> > +++ b/drivers/mmc/host/sdhci.c
>> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>> >>> > regulator_enable(host->vmmc);
>> >>> >
>> >>> > #ifdef CONFIG_REGULATOR
>> >>> > - if (host->vmmc) {
>> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>> >>> > ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>> >>> > 3300000);
>> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>> >>>
>> >>> Thanks for the longer explanation. I'm still missing something,
>> >>> though;
>> >>> what's wrong with running the check as it was with the new regulator
>> >>> code?
>> >>> (I haven't tried it yet.)
>> >>>
>> >>> #ifdef CONFIG_REGULATOR
>> >>> if (host->vmmc) {
>> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >>> 3300000,
>> >>> 3300000);
>> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >>> 3000000,
>> >>> 3000000);
>> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >>> 1800000,
>> >>> 1800000);
>> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
>> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
>> >>> }
>> >>> #endif /* CONFIG_REGULATOR */
>> >>>
>> >>> The point is to remove unsupported voltages, so if someone sets up a
>> >>> fixed regulator at 3300000, all of the other caps are disabled. Why
>> >>> wouldn't that work without this change, and how are we supposed to
>> >>> remove those caps on a fixed regulator after your patchset?
>> >>>
>> >>> Thanks, sorry if I'm missing something obvious,
>> >>
>> >> On our boards eMMC is connected to fixed 2.8V regulator, what results
>> >> in
>> >> clearing all available voltages and fail. The same situation is when
>> >> one
>> >> enable dummy regulator and try to use sdhci with it. My patch fixes
>> >> this
>> >> and restores sdhci to working state as it was before (before fixing
>> >> regulator regulator_is_supported_voltage() function and earlier when
>> >> MMC_BROKEN_VOLATGE capability was used).
>> >
>> > I see. Sounds like a separate bug -- Philip (or anyone else), any
>> > idea how we should be treating eMMCs with a fixed voltage here?
>> >
>>
>> I think we should check the voltage range rather than the voltage
>> point accoring to the spec.
>> Otherwise some valid voltage like 2.8v will be discarded by mistake.
>> My below old patch aim to fix this issue.
>> How do you think?
>>
>> -----Original Message-----
>> From: Kevin Liu [mailto:[email protected]]
>> Sent: Friday, September 28, 2012 3:56 PM
>> To: [email protected]; [email protected]; [email protected];
>> [email protected]; Zhangfei Gao
>> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
>> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
>> range according to spec
>>
>> From: Kevin Liu <[email protected]>
>>
>> For regulator vmmc/vmmcq, use voltage range as below
>> 3.3v/3.0v: (2.7v, 3.6v)
>> 1.8v: (1.7v, 1.95v)
>> Original code use the specific value which may fail in regulator
>> driver if it does NOT support the specific voltage.
>>
>> Signed-off-by: Jialing Fu <[email protected]>
>> Signed-off-by: Kevin Liu <[email protected]>
>
>
> Tested-by: Marek Szyprowski <[email protected]>
>
> This patch restores sdhci devices to working state on Samsung boards
> (tested on GONI and UniversalC210) after merging "regulator: fix voltage
> check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
> f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
> merged before the final v3.7 is out.
>
Marek,
thanks a lot for the verification!
And your patch "mmc: sdhci: apply voltage range check only for
non-fixed regulators" (commit
d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
mmc-next) can be reverted if this patch merged?
Kevin
>> ---
>> drivers/mmc/host/sdhci.c | 16 +++++++---------
>> 1 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 3aef580..36afd47 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1628,7 +1628,7 @@ static int
>> sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host,
>> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>
>> if (host->vqmmc) {
>> - ret = regulator_set_voltage(host->vqmmc, 3300000,
>> 3300000);
>> + ret = regulator_set_voltage(host->vqmmc, 2700000,
>> 3600000);
>> if (ret) {
>> pr_warning("%s: Switching to 3.3V signalling
>> voltage "
>> " failed\n", mmc_hostname(host->mmc));
>> @@ -1672,7 +1672,7 @@ static int
>> sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host,
>> */
>> if (host->vqmmc)
>> ret = regulator_set_voltage(host->vqmmc,
>> - 1800000, 1800000);
>> + 1700000, 1950000);
>> else
>> ret = 0;
>>
>> @@ -2856,7 +2856,7 @@ int sdhci_add_host(struct sdhci_host *host)
>> pr_info("%s: no vqmmc regulator found\n",
>> mmc_hostname(mmc));
>> host->vqmmc = NULL;
>> }
>> - else if (regulator_is_supported_voltage(host->vqmmc, 1800000,
>> 1800000))
>> + else if (regulator_is_supported_voltage(host->vqmmc, 1700000,
>> 1950000))
>> regulator_enable(host->vqmmc);
>> else
>> caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>> @@ -2927,16 +2927,14 @@ int sdhci_add_host(struct sdhci_host *host)
>>
>> #ifdef CONFIG_REGULATOR
>> if (host->vmmc) {
>> - ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>> - 3300000);
>> + ret = regulator_is_supported_voltage(host->vmmc, 2700000,
>> + 3600000);
>> if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_330))
>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> - ret = regulator_is_supported_voltage(host->vmmc, 3000000,
>> - 3000000);
>> if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_300))
>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> - ret = regulator_is_supported_voltage(host->vmmc, 1800000,
>> - 1800000);
>> + ret = regulator_is_supported_voltage(host->vmmc, 1700000,
>> + 1950000);
>> if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180))
>> caps[0] &= ~SDHCI_CAN_VDD_180;
>> }
>
>
> Best regards
> --
> Marek Szyprowski
> Samsung Poland R&D Center
>
>
Hello,
On 11/20/2012 9:59 AM, Kevin Liu wrote:
> 2012/11/20 Marek Szyprowski <[email protected]>:
> > Hello,
> >
> >
> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
> >>
> >> > From: [email protected]
> >> > [mailto:[email protected]] On Behalf Of Chris Ball
> >> > Sent: Tuesday, November 13, 2012 10:14 PM
> >> > To: Marek Szyprowski
> >> > Cc: [email protected]; [email protected]; Kyungmin
> >> > Park; Mark Brown; Liam Girdwood; Philip Rakity
> >> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only for
> >> > non-fixed regulators
> >> >
> >> > Hi,
> >> >
> >> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >> >>> > Fixed regulators cannot change their voltage, so disable all voltage
> >> >>> > range checking for them, otherwise the driver fails to operate with
> >> >>> > fixed regulators. Up to now it worked only by luck, because
> >> >>> > regulator_is_supported_voltage() function returned incorrect values.
> >> >>> > Commit "regulator: fix voltage check in
> >> >>> > regulator_is_supported_voltage()"
> >> >>> > fixed that function and now additional check is needed for fixed
> >> >>> > regulators.
> >> >>> >
> >> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
> >> >>> > ---
> >> >>> > drivers/mmc/host/sdhci.c | 2 +-
> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>> >
> >> >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> >>> > index c7851c0..6f6534e 100644
> >> >>> > --- a/drivers/mmc/host/sdhci.c
> >> >>> > +++ b/drivers/mmc/host/sdhci.c
> >> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >> >>> > regulator_enable(host->vmmc);
> >> >>> >
> >> >>> > #ifdef CONFIG_REGULATOR
> >> >>> > - if (host->vmmc) {
> >> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> >> >>> > ret = regulator_is_supported_voltage(host->vmmc, 3300000,
> >> >>> > 3300000);
> >> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> >> >>>
> >> >>> Thanks for the longer explanation. I'm still missing something,
> >> >>> though;
> >> >>> what's wrong with running the check as it was with the new regulator
> >> >>> code?
> >> >>> (I haven't tried it yet.)
> >> >>>
> >> >>> #ifdef CONFIG_REGULATOR
> >> >>> if (host->vmmc) {
> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
> >> >>> 3300000,
> >> >>> 3300000);
> >> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
> >> >>> 3000000,
> >> >>> 3000000);
> >> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
> >> >>> 1800000,
> >> >>> 1800000);
> >> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
> >> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
> >> >>> }
> >> >>> #endif /* CONFIG_REGULATOR */
> >> >>>
> >> >>> The point is to remove unsupported voltages, so if someone sets up a
> >> >>> fixed regulator at 3300000, all of the other caps are disabled. Why
> >> >>> wouldn't that work without this change, and how are we supposed to
> >> >>> remove those caps on a fixed regulator after your patchset?
> >> >>>
> >> >>> Thanks, sorry if I'm missing something obvious,
> >> >>
> >> >> On our boards eMMC is connected to fixed 2.8V regulator, what results
> >> >> in
> >> >> clearing all available voltages and fail. The same situation is when
> >> >> one
> >> >> enable dummy regulator and try to use sdhci with it. My patch fixes
> >> >> this
> >> >> and restores sdhci to working state as it was before (before fixing
> >> >> regulator regulator_is_supported_voltage() function and earlier when
> >> >> MMC_BROKEN_VOLATGE capability was used).
> >> >
> >> > I see. Sounds like a separate bug -- Philip (or anyone else), any
> >> > idea how we should be treating eMMCs with a fixed voltage here?
> >> >
> >>
> >> I think we should check the voltage range rather than the voltage
> >> point accoring to the spec.
> >> Otherwise some valid voltage like 2.8v will be discarded by mistake.
> >> My below old patch aim to fix this issue.
> >> How do you think?
> >>
> >> -----Original Message-----
> >> From: Kevin Liu [mailto:[email protected]]
> >> Sent: Friday, September 28, 2012 3:56 PM
> >> To: [email protected]; [email protected]; [email protected];
> >> [email protected]; Zhangfei Gao
> >> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
> >> range according to spec
> >>
> >> From: Kevin Liu <[email protected]>
> >>
> >> For regulator vmmc/vmmcq, use voltage range as below
> >> 3.3v/3.0v: (2.7v, 3.6v)
> >> 1.8v: (1.7v, 1.95v)
> >> Original code use the specific value which may fail in regulator
> >> driver if it does NOT support the specific voltage.
> >>
> >> Signed-off-by: Jialing Fu <[email protected]>
> >> Signed-off-by: Kevin Liu <[email protected]>
> >
> >
> > Tested-by: Marek Szyprowski <[email protected]>
> >
> > This patch restores sdhci devices to working state on Samsung boards
> > (tested on GONI and UniversalC210) after merging "regulator: fix voltage
> > check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
> > merged before the final v3.7 is out.
> >
> Marek,
>
> thanks a lot for the verification!
> And your patch "mmc: sdhci: apply voltage range check only for
> non-fixed regulators" (commit
> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
> mmc-next) can be reverted if this patch merged?
Yes, it can be replaced with it, although there is still an issue that need
to be resolved somehow. Right now SDHCI driver fails to initialize if
support
for dummy regulator is enabled.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
2012/11/20 Marek Szyprowski <[email protected]>:
> Hello,
>
>
> On 11/20/2012 9:59 AM, Kevin Liu wrote:
>>
>> 2012/11/20 Marek Szyprowski <[email protected]>:
>> > Hello,
>> >
>> >
>> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
>> >>
>> >> > From: [email protected]
>> >> > [mailto:[email protected]] On Behalf Of Chris Ball
>> >> > Sent: Tuesday, November 13, 2012 10:14 PM
>> >> > To: Marek Szyprowski
>> >> > Cc: [email protected]; [email protected]; Kyungmin
>> >> > Park; Mark Brown; Liam Girdwood; Philip Rakity
>> >> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only
>> >> > for
>> >> > non-fixed regulators
>> >> >
>> >> > Hi,
>> >> >
>> >> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >> >>> > Fixed regulators cannot change their voltage, so disable all
>> >> >>> > voltage
>> >> >>> > range checking for them, otherwise the driver fails to operate
>> >> >>> > with
>> >> >>> > fixed regulators. Up to now it worked only by luck, because
>> >> >>> > regulator_is_supported_voltage() function returned incorrect
>> >> >>> > values.
>> >> >>> > Commit "regulator: fix voltage check in
>> >> >>> > regulator_is_supported_voltage()"
>> >> >>> > fixed that function and now additional check is needed for fixed
>> >> >>> > regulators.
>> >> >>> >
>> >> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
>> >> >>> > ---
>> >> >>> > drivers/mmc/host/sdhci.c | 2 +-
>> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >>> >
>> >> >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> >> >>> > index c7851c0..6f6534e 100644
>> >> >>> > --- a/drivers/mmc/host/sdhci.c
>> >> >>> > +++ b/drivers/mmc/host/sdhci.c
>> >> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>> >> >>> > regulator_enable(host->vmmc);
>> >> >>> >
>> >> >>> > #ifdef CONFIG_REGULATOR
>> >> >>> > - if (host->vmmc) {
>> >> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>> >> >>> > ret = regulator_is_supported_voltage(host->vmmc,
>> >> >>> > 3300000,
>> >> >>> > 3300000);
>> >> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>> >> >>>
>> >> >>> Thanks for the longer explanation. I'm still missing something,
>> >> >>> though;
>> >> >>> what's wrong with running the check as it was with the new
>> >> >>> regulator
>> >> >>> code?
>> >> >>> (I haven't tried it yet.)
>> >> >>>
>> >> >>> #ifdef CONFIG_REGULATOR
>> >> >>> if (host->vmmc) {
>> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >> >>> 3300000,
>> >> >>> 3300000);
>> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >>> SDHCI_CAN_VDD_330)))
>> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >> >>> 3000000,
>> >> >>> 3000000);
>> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >>> SDHCI_CAN_VDD_300)))
>> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >> >>> 1800000,
>> >> >>> 1800000);
>> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >>> SDHCI_CAN_VDD_180)))
>> >> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
>> >> >>> }
>> >> >>> #endif /* CONFIG_REGULATOR */
>> >> >>>
>> >> >>> The point is to remove unsupported voltages, so if someone sets up
>> >> >>> a
>> >> >>> fixed regulator at 3300000, all of the other caps are disabled.
>> >> >>> Why
>> >> >>> wouldn't that work without this change, and how are we supposed to
>> >> >>> remove those caps on a fixed regulator after your patchset?
>> >> >>>
>> >> >>> Thanks, sorry if I'm missing something obvious,
>> >> >>
>> >> >> On our boards eMMC is connected to fixed 2.8V regulator, what
>> >> >> results
>> >> >> in
>> >> >> clearing all available voltages and fail. The same situation is when
>> >> >> one
>> >> >> enable dummy regulator and try to use sdhci with it. My patch fixes
>> >> >> this
>> >> >> and restores sdhci to working state as it was before (before fixing
>> >> >> regulator regulator_is_supported_voltage() function and earlier when
>> >> >> MMC_BROKEN_VOLATGE capability was used).
>> >> >
>> >> > I see. Sounds like a separate bug -- Philip (or anyone else), any
>> >> > idea how we should be treating eMMCs with a fixed voltage here?
>> >> >
>> >>
>> >> I think we should check the voltage range rather than the voltage
>> >> point accoring to the spec.
>> >> Otherwise some valid voltage like 2.8v will be discarded by mistake.
>> >> My below old patch aim to fix this issue.
>> >> How do you think?
>> >>
>> >> -----Original Message-----
>> >> From: Kevin Liu [mailto:[email protected]]
>> >> Sent: Friday, September 28, 2012 3:56 PM
>> >> To: [email protected]; [email protected]; [email protected];
>> >> [email protected]; Zhangfei Gao
>> >> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
>> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
>> >> range according to spec
>> >>
>> >> From: Kevin Liu <[email protected]>
>> >>
>> >> For regulator vmmc/vmmcq, use voltage range as below
>> >> 3.3v/3.0v: (2.7v, 3.6v)
>> >> 1.8v: (1.7v, 1.95v)
>> >> Original code use the specific value which may fail in regulator
>> >> driver if it does NOT support the specific voltage.
>> >>
>> >> Signed-off-by: Jialing Fu <[email protected]>
>> >> Signed-off-by: Kevin Liu <[email protected]>
>> >
>> >
>> > Tested-by: Marek Szyprowski <[email protected]>
>> >
>> > This patch restores sdhci devices to working state on Samsung boards
>> > (tested on GONI and UniversalC210) after merging "regulator: fix voltage
>> > check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
>> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
>> > merged before the final v3.7 is out.
>> >
>> Marek,
>>
>> thanks a lot for the verification!
>> And your patch "mmc: sdhci: apply voltage range check only for
>> non-fixed regulators" (commit
>> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
>> mmc-next) can be reverted if this patch merged?
>
>
> Yes, it can be replaced with it, although there is still an issue that need
> to be resolved somehow. Right now SDHCI driver fails to initialize if
> support
> for dummy regulator is enabled.
>
Then I think the dummy issue can be resolved with your patch merged
and if you can just update your patch from
"regulator_count_voltages(host->vmmc) > 1"
to
"regulator_count_voltages(host->vmmc) > 0"
to let fix regulator work.
Thanks
Kevin
Hello,
On 11/20/2012 12:36 PM, Kevin Liu wrote:
> 2012/11/20 Marek Szyprowski <[email protected]>:
> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
> >> 2012/11/20 Marek Szyprowski <[email protected]>:
> >> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
> >> >>
> >> >> > From: [email protected]
> >> >> > [mailto:[email protected]] On Behalf Of Chris Ball
> >> >> > Sent: Tuesday, November 13, 2012 10:14 PM
> >> >> > To: Marek Szyprowski
> >> >> > Cc: [email protected]; [email protected]; Kyungmin
> >> >> > Park; Mark Brown; Liam Girdwood; Philip Rakity
> >> >> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only
> >> >> > for non-fixed regulators
> >> >> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >> >> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >> >> >>> > Fixed regulators cannot change their voltage, so disable all
> >> >> >>> > voltage
> >> >> >>> > range checking for them, otherwise the driver fails to operate
> >> >> >>> > with
> >> >> >>> > fixed regulators. Up to now it worked only by luck, because
> >> >> >>> > regulator_is_supported_voltage() function returned incorrect
> >> >> >>> > values.
> >> >> >>> > Commit "regulator: fix voltage check in
> >> >> >>> > regulator_is_supported_voltage()"
> >> >> >>> > fixed that function and now additional check is needed for fixed
> >> >> >>> > regulators.
> >> >> >>> >
> >> >> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
> >> >> >>> > ---
> >> >> >>> > drivers/mmc/host/sdhci.c | 2 +-
> >> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> >>> >
> >> >> >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> >> >>> > index c7851c0..6f6534e 100644
> >> >> >>> > --- a/drivers/mmc/host/sdhci.c
> >> >> >>> > +++ b/drivers/mmc/host/sdhci.c
> >> >> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >> >> >>> > regulator_enable(host->vmmc);
> >> >> >>> >
> >> >> >>> > #ifdef CONFIG_REGULATOR
> >> >> >>> > - if (host->vmmc) {
> >> >> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> >> >> >>> > ret = regulator_is_supported_voltage(host->vmmc,
> >> >> >>> > 3300000,
> >> >> >>> > 3300000);
> >> >> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> >> >> >>>
> >> >> >>> Thanks for the longer explanation. I'm still missing something,
> >> >> >>> though;
> >> >> >>> what's wrong with running the check as it was with the new
> >> >> >>> regulator
> >> >> >>> code?
> >> >> >>> (I haven't tried it yet.)
> >> >> >>>
> >> >> >>> #ifdef CONFIG_REGULATOR
> >> >> >>> if (host->vmmc) {
> >> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
> >> >> >>> 3300000,
> >> >> >>> 3300000);
> >> >> >>> if ((ret <= 0) || (!(caps[0] &
> >> >> >>> SDHCI_CAN_VDD_330)))
> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
> >> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
> >> >> >>> 3000000,
> >> >> >>> 3000000);
> >> >> >>> if ((ret <= 0) || (!(caps[0] &
> >> >> >>> SDHCI_CAN_VDD_300)))
> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
> >> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
> >> >> >>> 1800000,
> >> >> >>> 1800000);
> >> >> >>> if ((ret <= 0) || (!(caps[0] &
> >> >> >>> SDHCI_CAN_VDD_180)))
> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
> >> >> >>> }
> >> >> >>> #endif /* CONFIG_REGULATOR */
> >> >> >>>
> >> >> >>> The point is to remove unsupported voltages, so if someone sets up
> >> >> >>> a
> >> >> >>> fixed regulator at 3300000, all of the other caps are disabled.
> >> >> >>> Why
> >> >> >>> wouldn't that work without this change, and how are we supposed to
> >> >> >>> remove those caps on a fixed regulator after your patchset?
> >> >> >>>
> >> >> >>> Thanks, sorry if I'm missing something obvious,
> >> >> >>
> >> >> >> On our boards eMMC is connected to fixed 2.8V regulator, what
> >> >> >> results
> >> >> >> in
> >> >> >> clearing all available voltages and fail. The same situation is when
> >> >> >> one
> >> >> >> enable dummy regulator and try to use sdhci with it. My patch fixes
> >> >> >> this
> >> >> >> and restores sdhci to working state as it was before (before fixing
> >> >> >> regulator regulator_is_supported_voltage() function and earlier when
> >> >> >> MMC_BROKEN_VOLATGE capability was used).
> >> >> >
> >> >> > I see. Sounds like a separate bug -- Philip (or anyone else), any
> >> >> > idea how we should be treating eMMCs with a fixed voltage here?
> >> >> >
> >> >>
> >> >> I think we should check the voltage range rather than the voltage
> >> >> point accoring to the spec.
> >> >> Otherwise some valid voltage like 2.8v will be discarded by mistake.
> >> >> My below old patch aim to fix this issue.
> >> >> How do you think?
> >> >>
> >> >> -----Original Message-----
> >> >> From: Kevin Liu [mailto:[email protected]]
> >> >> Sent: Friday, September 28, 2012 3:56 PM
> >> >> To: [email protected]; [email protected]; [email protected];
> >> >> [email protected]; Zhangfei Gao
> >> >> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
> >> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
> >> >> range according to spec
> >> >>
> >> >> From: Kevin Liu <[email protected]>
> >> >>
> >> >> For regulator vmmc/vmmcq, use voltage range as below
> >> >> 3.3v/3.0v: (2.7v, 3.6v)
> >> >> 1.8v: (1.7v, 1.95v)
> >> >> Original code use the specific value which may fail in regulator
> >> >> driver if it does NOT support the specific voltage.
> >> >>
> >> >> Signed-off-by: Jialing Fu <[email protected]>
> >> >> Signed-off-by: Kevin Liu <[email protected]>
> >> >
> >> >
> >> > Tested-by: Marek Szyprowski <[email protected]>
> >> >
> >> > This patch restores sdhci devices to working state on Samsung boards
> >> > (tested on GONI and UniversalC210) after merging "regulator: fix voltage
> >> > check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
> >> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
> >> > merged before the final v3.7 is out.
> >> >
> >> Marek,
> >>
> >> thanks a lot for the verification!
> >> And your patch "mmc: sdhci: apply voltage range check only for
> >> non-fixed regulators" (commit
> >> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
> >> mmc-next) can be reverted if this patch merged?
> >
> >
> > Yes, it can be replaced with it, although there is still an issue that need
> > to be resolved somehow. Right now SDHCI driver fails to initialize if
> > support
> > for dummy regulator is enabled.
> >
> Then I think the dummy issue can be resolved with your patch merged
> and if you can just update your patch from
> "regulator_count_voltages(host->vmmc) > 1"
> to
> "regulator_count_voltages(host->vmmc) > 0"
> to let fix regulator work.
regulator_count_voltages() returns 1 for both fixed regulators and for
virtual dummy regulator, so the above change makes no sense.
However I was so focused on fixing the 2.8V supply case that I missed the
fact that my "mmc: sdhci: apply voltage range check only for non-fixed
regulators" patch also fixed the dummy regulator case.
The conclusion is that applying both patches should finally fix the
regulator issues with for the Samsung boards (2.8V supply for eMMC) and
'dummy-regulators' cases.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
Hi,
On Tue, Nov 20 2012, Marek Szyprowski wrote:
>> >> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
>> >> >> range according to spec
>> >> >>
>> >> >> From: Kevin Liu <[email protected]>
>> >> >>
>> >> >> For regulator vmmc/vmmcq, use voltage range as below
>> >> >> 3.3v/3.0v: (2.7v, 3.6v)
>> >> >> 1.8v: (1.7v, 1.95v)
>> >> >> Original code use the specific value which may fail in regulator
>> >> >> driver if it does NOT support the specific voltage.
>> >> >>
>> >> >> Signed-off-by: Jialing Fu <[email protected]>
>> >> >> Signed-off-by: Kevin Liu <[email protected]>
>> >> >
>> >> >
>> >> > Tested-by: Marek Szyprowski <[email protected]>
>> >> >
>> >> > This patch restores sdhci devices to working state on Samsung boards
>> >> > (tested on GONI and UniversalC210) after merging "regulator: fix voltage
>> >> > check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
>> >> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
>> >> > merged before the final v3.7 is out.
>> >> >
>> >> Marek,
>> >>
>> >> thanks a lot for the verification!
>> >> And your patch "mmc: sdhci: apply voltage range check only for
>> >> non-fixed regulators" (commit
>> >> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
>> >> mmc-next) can be reverted if this patch merged?
>> >
>> >
>> > Yes, it can be replaced with it, although there is still an issue that need
>> > to be resolved somehow. Right now SDHCI driver fails to initialize if
>> > support
>> > for dummy regulator is enabled.
>> >
>> Then I think the dummy issue can be resolved with your patch merged
>> and if you can just update your patch from
>> "regulator_count_voltages(host->vmmc) > 1"
>> to
>> "regulator_count_voltages(host->vmmc) > 0"
>> to let fix regulator work.
>
> regulator_count_voltages() returns 1 for both fixed regulators and for
> virtual dummy regulator, so the above change makes no sense.
>
> However I was so focused on fixing the 2.8V supply case that I missed the
> fact that my "mmc: sdhci: apply voltage range check only for non-fixed
> regulators" patch also fixed the dummy regulator case.
>
> The conclusion is that applying both patches should finally fix the
> regulator issues with for the Samsung boards (2.8V supply for eMMC) and
> 'dummy-regulators' cases.
Thanks, I've pushed v5 of "mmc: sdhci: use regulator min/max voltage
range according to spec" to mmc-next for 3.7 with Marek's Tested-by now.
- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
2012/11/20 Marek Szyprowski <[email protected]>:
> Hello,
>
>
> On 11/20/2012 12:36 PM, Kevin Liu wrote:
>>
>> 2012/11/20 Marek Szyprowski <[email protected]>:
>> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
>> >> 2012/11/20 Marek Szyprowski <[email protected]>:
>> >> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
>> >> >>
>> >> >> > From: [email protected]
>> >> >> > [mailto:[email protected]] On Behalf Of Chris Ball
>> >> >> > Sent: Tuesday, November 13, 2012 10:14 PM
>> >> >> > To: Marek Szyprowski
>> >> >> > Cc: [email protected]; [email protected];
>> >> >> > Kyungmin
>> >> >> > Park; Mark Brown; Liam Girdwood; Philip Rakity
>> >> >> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only
>> >> >> > for non-fixed regulators
>> >> >> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >> >> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >> >> >>> > Fixed regulators cannot change their voltage, so disable all
>> >> >> >>> > voltage
>> >> >> >>> > range checking for them, otherwise the driver fails to operate
>> >> >> >>> > with
>> >> >> >>> > fixed regulators. Up to now it worked only by luck, because
>> >> >> >>> > regulator_is_supported_voltage() function returned incorrect
>> >> >> >>> > values.
>> >> >> >>> > Commit "regulator: fix voltage check in
>> >> >> >>> > regulator_is_supported_voltage()"
>> >> >> >>> > fixed that function and now additional check is needed for
>> >> >> >>> > fixed
>> >> >> >>> > regulators.
>> >> >> >>> >
>> >> >> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
>> >> >> >>> > ---
>> >> >> >>> > drivers/mmc/host/sdhci.c | 2 +-
>> >> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >> >>> >
>> >> >> >>> > diff --git a/drivers/mmc/host/sdhci.c
>> >> >> >>> > b/drivers/mmc/host/sdhci.c
>> >> >> >>> > index c7851c0..6f6534e 100644
>> >> >> >>> > --- a/drivers/mmc/host/sdhci.c
>> >> >> >>> > +++ b/drivers/mmc/host/sdhci.c
>> >> >> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host
>> >> >> >>> > *host)
>> >> >> >>> > regulator_enable(host->vmmc);
>> >> >> >>> >
>> >> >> >>> > #ifdef CONFIG_REGULATOR
>> >> >> >>> > - if (host->vmmc) {
>> >> >> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1)
>> >> >> >>> > {
>> >> >> >>> > ret = regulator_is_supported_voltage(host->vmmc,
>> >> >> >>> > 3300000,
>> >> >> >>> > 3300000);
>> >> >> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>> >> >> >>>
>> >> >> >>> Thanks for the longer explanation. I'm still missing something,
>> >> >> >>> though;
>> >> >> >>> what's wrong with running the check as it was with the new
>> >> >> >>> regulator
>> >> >> >>> code?
>> >> >> >>> (I haven't tried it yet.)
>> >> >> >>>
>> >> >> >>> #ifdef CONFIG_REGULATOR
>> >> >> >>> if (host->vmmc) {
>> >> >> >>> ret =
>> >> >> >>> regulator_is_supported_voltage(host->vmmc,
>> >> >> >>> 3300000,
>> >> >> >>> 3300000);
>> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >>> SDHCI_CAN_VDD_330)))
>> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> >> >> >>> ret =
>> >> >> >>> regulator_is_supported_voltage(host->vmmc,
>> >> >> >>> 3000000,
>> >> >> >>> 3000000);
>> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >>> SDHCI_CAN_VDD_300)))
>> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> >> >> >>> ret =
>> >> >> >>> regulator_is_supported_voltage(host->vmmc,
>> >> >> >>> 1800000,
>> >> >> >>> 1800000);
>> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >>> SDHCI_CAN_VDD_180)))
>> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
>> >> >> >>> }
>> >> >> >>> #endif /* CONFIG_REGULATOR */
>> >> >> >>>
>> >> >> >>> The point is to remove unsupported voltages, so if someone sets
>> >> >> >>> up
>> >> >> >>> a
>> >> >> >>> fixed regulator at 3300000, all of the other caps are disabled.
>> >> >> >>> Why
>> >> >> >>> wouldn't that work without this change, and how are we supposed
>> >> >> >>> to
>> >> >> >>> remove those caps on a fixed regulator after your patchset?
>> >> >> >>>
>> >> >> >>> Thanks, sorry if I'm missing something obvious,
>> >> >> >>
>> >> >> >> On our boards eMMC is connected to fixed 2.8V regulator, what
>> >> >> >> results
>> >> >> >> in
>> >> >> >> clearing all available voltages and fail. The same situation is
>> >> >> >> when
>> >> >> >> one
>> >> >> >> enable dummy regulator and try to use sdhci with it. My patch
>> >> >> >> fixes
>> >> >> >> this
>> >> >> >> and restores sdhci to working state as it was before (before
>> >> >> >> fixing
>> >> >> >> regulator regulator_is_supported_voltage() function and earlier
>> >> >> >> when
>> >> >> >> MMC_BROKEN_VOLATGE capability was used).
>> >> >> >
>> >> >> > I see. Sounds like a separate bug -- Philip (or anyone else), any
>> >> >> > idea how we should be treating eMMCs with a fixed voltage here?
>> >> >> >
>> >> >>
>> >> >> I think we should check the voltage range rather than the voltage
>> >> >> point accoring to the spec.
>> >> >> Otherwise some valid voltage like 2.8v will be discarded by mistake.
>> >> >> My below old patch aim to fix this issue.
>> >> >> How do you think?
>> >> >>
>> >> >> -----Original Message-----
>> >> >> From: Kevin Liu [mailto:[email protected]]
>> >> >> Sent: Friday, September 28, 2012 3:56 PM
>> >> >> To: [email protected]; [email protected]; [email protected];
>> >> >> [email protected]; Zhangfei Gao
>> >> >> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
>> >> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
>> >> >> range according to spec
>> >> >>
>> >> >> From: Kevin Liu <[email protected]>
>> >> >>
>> >> >> For regulator vmmc/vmmcq, use voltage range as below
>> >> >> 3.3v/3.0v: (2.7v, 3.6v)
>> >> >> 1.8v: (1.7v, 1.95v)
>> >> >> Original code use the specific value which may fail in regulator
>> >> >> driver if it does NOT support the specific voltage.
>> >> >>
>> >> >> Signed-off-by: Jialing Fu <[email protected]>
>> >> >> Signed-off-by: Kevin Liu <[email protected]>
>> >> >
>> >> >
>> >> > Tested-by: Marek Szyprowski <[email protected]>
>> >> >
>> >> > This patch restores sdhci devices to working state on Samsung boards
>> >> > (tested on GONI and UniversalC210) after merging "regulator: fix
>> >> > voltage
>> >> > check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
>> >> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
>> >> > merged before the final v3.7 is out.
>> >> >
>> >> Marek,
>> >>
>> >> thanks a lot for the verification!
>> >> And your patch "mmc: sdhci: apply voltage range check only for
>> >> non-fixed regulators" (commit
>> >> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
>> >> mmc-next) can be reverted if this patch merged?
>> >
>> >
>> > Yes, it can be replaced with it, although there is still an issue that
>> > need
>> > to be resolved somehow. Right now SDHCI driver fails to initialize if
>> > support
>> > for dummy regulator is enabled.
>> >
>> Then I think the dummy issue can be resolved with your patch merged
>> and if you can just update your patch from
>> "regulator_count_voltages(host->vmmc) > 1"
>> to
>> "regulator_count_voltages(host->vmmc) > 0"
>> to let fix regulator work.
>
>
> regulator_count_voltages() returns 1 for both fixed regulators and for
> virtual dummy regulator, so the above change makes no sense.
I think regulator_count_voltage should return -EINVAL for dummy
regulator since n_voltages is not defined for dummy regulator:
int regulator_count_voltages(struct regulator *regulator)
{
struct regulator_dev *rdev = regulator->rdev;
return rdev->desc->n_voltages ? : -EINVAL;
}
can you double check this?
> However I was so focused on fixing the 2.8V supply case that I missed the
> fact that my "mmc: sdhci: apply voltage range check only for non-fixed
> regulators" patch also fixed the dummy regulator case.
>
> The conclusion is that applying both patches should finally fix the
> regulator issues with for the Samsung boards (2.8V supply for eMMC) and
> 'dummy-regulators' cases.
>
After thinking again, I think we don't need the check for
regulator_count_voltages.
In fact, dummy regulator should NOT be used for the sd/mmc power
supply. We should use controllable or fixed regulator. If dummy
regulator is used, then it means we won't control it and we don't know
its voltage. It's not reasonable for sd/mmc power supply. If dummy
regulator is used, I think it's ok to return error and disable all
voltage support caps.
Thanks
Kevin
Hello,
On 11/20/2012 3:14 PM, Kevin Liu wrote:
> 2012/11/20 Marek Szyprowski <[email protected]>:
> > Hello,
> >
> >
> > On 11/20/2012 12:36 PM, Kevin Liu wrote:
> >>
> >> 2012/11/20 Marek Szyprowski <[email protected]>:
> >> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
> >> >> 2012/11/20 Marek Szyprowski <[email protected]>:
> >> >> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
> >> >> >>
> >> >> >> > From: [email protected]
> >> >> >> > [mailto:[email protected]] On Behalf Of Chris Ball
> >> >> >> > Sent: Tuesday, November 13, 2012 10:14 PM
> >> >> >> > To: Marek Szyprowski
> >> >> >> > Cc: [email protected]; [email protected];
> >> >> >> > Kyungmin
> >> >> >> > Park; Mark Brown; Liam Girdwood; Philip Rakity
> >> >> >> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only
> >> >> >> > for non-fixed regulators
> >> >> >> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >> >> >> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
> >> >> >> >>> > Fixed regulators cannot change their voltage, so disable all
> >> >> >> >>> > voltage
> >> >> >> >>> > range checking for them, otherwise the driver fails to operate
> >> >> >> >>> > with
> >> >> >> >>> > fixed regulators. Up to now it worked only by luck, because
> >> >> >> >>> > regulator_is_supported_voltage() function returned incorrect
> >> >> >> >>> > values.
> >> >> >> >>> > Commit "regulator: fix voltage check in
> >> >> >> >>> > regulator_is_supported_voltage()"
> >> >> >> >>> > fixed that function and now additional check is needed for
> >> >> >> >>> > fixed
> >> >> >> >>> > regulators.
> >> >> >> >>> >
> >> >> >> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
> >> >> >> >>> > ---
> >> >> >> >>> > drivers/mmc/host/sdhci.c | 2 +-
> >> >> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> >> >>> >
> >> >> >> >>> > diff --git a/drivers/mmc/host/sdhci.c
> >> >> >> >>> > b/drivers/mmc/host/sdhci.c
> >> >> >> >>> > index c7851c0..6f6534e 100644
> >> >> >> >>> > --- a/drivers/mmc/host/sdhci.c
> >> >> >> >>> > +++ b/drivers/mmc/host/sdhci.c
> >> >> >> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host
> >> >> >> >>> > *host)
> >> >> >> >>> > regulator_enable(host->vmmc);
> >> >> >> >>> >
> >> >> >> >>> > #ifdef CONFIG_REGULATOR
> >> >> >> >>> > - if (host->vmmc) {
> >> >> >> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1)
> >> >> >> >>> > {
> >> >> >> >>> > ret = regulator_is_supported_voltage(host->vmmc,
> >> >> >> >>> > 3300000,
> >> >> >> >>> > 3300000);
> >> >> >> >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> >> >> >> >>>
> >> >> >> >>> Thanks for the longer explanation. I'm still missing something,
> >> >> >> >>> though;
> >> >> >> >>> what's wrong with running the check as it was with the new
> >> >> >> >>> regulator
> >> >> >> >>> code?
> >> >> >> >>> (I haven't tried it yet.)
> >> >> >> >>>
> >> >> >> >>> #ifdef CONFIG_REGULATOR
> >> >> >> >>> if (host->vmmc) {
> >> >> >> >>> ret =
> >> >> >> >>> regulator_is_supported_voltage(host->vmmc,
> >> >> >> >>> 3300000,
> >> >> >> >>> 3300000);
> >> >> >> >>> if ((ret <= 0) || (!(caps[0] &
> >> >> >> >>> SDHCI_CAN_VDD_330)))
> >> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
> >> >> >> >>> ret =
> >> >> >> >>> regulator_is_supported_voltage(host->vmmc,
> >> >> >> >>> 3000000,
> >> >> >> >>> 3000000);
> >> >> >> >>> if ((ret <= 0) || (!(caps[0] &
> >> >> >> >>> SDHCI_CAN_VDD_300)))
> >> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
> >> >> >> >>> ret =
> >> >> >> >>> regulator_is_supported_voltage(host->vmmc,
> >> >> >> >>> 1800000,
> >> >> >> >>> 1800000);
> >> >> >> >>> if ((ret <= 0) || (!(caps[0] &
> >> >> >> >>> SDHCI_CAN_VDD_180)))
> >> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
> >> >> >> >>> }
> >> >> >> >>> #endif /* CONFIG_REGULATOR */
> >> >> >> >>>
> >> >> >> >>> The point is to remove unsupported voltages, so if someone sets
> >> >> >> >>> up
> >> >> >> >>> a
> >> >> >> >>> fixed regulator at 3300000, all of the other caps are disabled.
> >> >> >> >>> Why
> >> >> >> >>> wouldn't that work without this change, and how are we supposed
> >> >> >> >>> to
> >> >> >> >>> remove those caps on a fixed regulator after your patchset?
> >> >> >> >>>
> >> >> >> >>> Thanks, sorry if I'm missing something obvious,
> >> >> >> >>
> >> >> >> >> On our boards eMMC is connected to fixed 2.8V regulator, what
> >> >> >> >> results
> >> >> >> >> in
> >> >> >> >> clearing all available voltages and fail. The same situation is
> >> >> >> >> when
> >> >> >> >> one
> >> >> >> >> enable dummy regulator and try to use sdhci with it. My patch
> >> >> >> >> fixes
> >> >> >> >> this
> >> >> >> >> and restores sdhci to working state as it was before (before
> >> >> >> >> fixing
> >> >> >> >> regulator regulator_is_supported_voltage() function and earlier
> >> >> >> >> when
> >> >> >> >> MMC_BROKEN_VOLATGE capability was used).
> >> >> >> >
> >> >> >> > I see. Sounds like a separate bug -- Philip (or anyone else), any
> >> >> >> > idea how we should be treating eMMCs with a fixed voltage here?
> >> >> >> >
> >> >> >>
> >> >> >> I think we should check the voltage range rather than the voltage
> >> >> >> point accoring to the spec.
> >> >> >> Otherwise some valid voltage like 2.8v will be discarded by mistake.
> >> >> >> My below old patch aim to fix this issue.
> >> >> >> How do you think?
> >> >> >>
> >> >> >> -----Original Message-----
> >> >> >> From: Kevin Liu [mailto:[email protected]]
> >> >> >> Sent: Friday, September 28, 2012 3:56 PM
> >> >> >> To: [email protected]; [email protected]; [email protected];
> >> >> >> [email protected]; Zhangfei Gao
> >> >> >> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
> >> >> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage
> >> >> >> range according to spec
> >> >> >>
> >> >> >> From: Kevin Liu <[email protected]>
> >> >> >>
> >> >> >> For regulator vmmc/vmmcq, use voltage range as below
> >> >> >> 3.3v/3.0v: (2.7v, 3.6v)
> >> >> >> 1.8v: (1.7v, 1.95v)
> >> >> >> Original code use the specific value which may fail in regulator
> >> >> >> driver if it does NOT support the specific voltage.
> >> >> >>
> >> >> >> Signed-off-by: Jialing Fu <[email protected]>
> >> >> >> Signed-off-by: Kevin Liu <[email protected]>
> >> >> >
> >> >> >
> >> >> > Tested-by: Marek Szyprowski <[email protected]>
> >> >> >
> >> >> > This patch restores sdhci devices to working state on Samsung boards
> >> >> > (tested on GONI and UniversalC210) after merging "regulator: fix
> >> >> > voltage
> >> >> > check in regulator_is_supported_voltage()" patch to v3.7-rc6 (commit
> >> >> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have it
> >> >> > merged before the final v3.7 is out.
> >> >> >
> >> >> Marek,
> >> >>
> >> >> thanks a lot for the verification!
> >> >> And your patch "mmc: sdhci: apply voltage range check only for
> >> >> non-fixed regulators" (commit
> >> >> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
> >> >> mmc-next) can be reverted if this patch merged?
> >> >
> >> >
> >> > Yes, it can be replaced with it, although there is still an issue that
> >> > need
> >> > to be resolved somehow. Right now SDHCI driver fails to initialize if
> >> > support
> >> > for dummy regulator is enabled.
> >> >
> >> Then I think the dummy issue can be resolved with your patch merged
> >> and if you can just update your patch from
> >> "regulator_count_voltages(host->vmmc) > 1"
> >> to
> >> "regulator_count_voltages(host->vmmc) > 0"
> >> to let fix regulator work.
> >
> >
> > regulator_count_voltages() returns 1 for both fixed regulators and for
> > virtual dummy regulator, so the above change makes no sense.
>
> I think regulator_count_voltage should return -EINVAL for dummy
> regulator since n_voltages is not defined for dummy regulator:
> int regulator_count_voltages(struct regulator *regulator)
> {
> struct regulator_dev *rdev = regulator->rdev;
>
> return rdev->desc->n_voltages ? : -EINVAL;
> }
> can you double check this?
Err, right. I looked at the wrong SDHCI device :/ I have 3 of them on my
board - one with fixed regulator, one with 'real' and one without (with
virtual dummy regulator). I've applied my earlier patch and noticed that
it cured sdhci driver with dummy regulator, so I concluded that it did
the right job. Sorry for the confusion.
> > However I was so focused on fixing the 2.8V supply case that I missed the
> > fact that my "mmc: sdhci: apply voltage range check only for non-fixed
> > regulators" patch also fixed the dummy regulator case.
> >
> > The conclusion is that applying both patches should finally fix the
> > regulator issues with for the Samsung boards (2.8V supply for eMMC) and
> > 'dummy-regulators' cases.
> >
>
> After thinking again, I think we don't need the check for
> regulator_count_voltages.
> In fact, dummy regulator should NOT be used for the sd/mmc power
> supply. We should use controllable or fixed regulator. If dummy
> regulator is used, then it means we won't control it and we don't know
> its voltage. It's not reasonable for sd/mmc power supply. If dummy
> regulator is used, I think it's ok to return error and disable all
> voltage support caps.
The problem with dummy regulator is the fact that it can be enabled only
globally for all devices in the system. I think that the best solution
would be to introduce regulator_can_change_voltage() as Mark suggested.
I will post patches soon.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
2012/11/20 Marek Szyprowski <[email protected]>:
> Hello,
>
>
> On 11/20/2012 3:14 PM, Kevin Liu wrote:
>>
>> 2012/11/20 Marek Szyprowski <[email protected]>:
>> > Hello,
>> >
>> >
>> > On 11/20/2012 12:36 PM, Kevin Liu wrote:
>> >>
>> >> 2012/11/20 Marek Szyprowski <[email protected]>:
>> >> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
>> >> >> 2012/11/20 Marek Szyprowski <[email protected]>:
>> >> >> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
>> >> >> >>
>> >> >> >> > From: [email protected]
>> >> >> >> > [mailto:[email protected]] On Behalf Of Chris
>> >> >> >> > Ball
>> >> >> >> > Sent: Tuesday, November 13, 2012 10:14 PM
>> >> >> >> > To: Marek Szyprowski
>> >> >> >> > Cc: [email protected]; [email protected];
>> >> >> >> > Kyungmin
>> >> >> >> > Park; Mark Brown; Liam Girdwood; Philip Rakity
>> >> >> >> > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check
>> >> >> >> > only
>> >> >> >> > for non-fixed regulators
>> >> >> >> > On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >> >> >> >>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> >> >> >> >>> > Fixed regulators cannot change their voltage, so disable
>> >> >> >> >>> > all
>> >> >> >> >>> > voltage
>> >> >> >> >>> > range checking for them, otherwise the driver fails to
>> >> >> >> >>> > operate
>> >> >> >> >>> > with
>> >> >> >> >>> > fixed regulators. Up to now it worked only by luck, because
>> >> >> >> >>> > regulator_is_supported_voltage() function returned
>> >> >> >> >>> > incorrect
>> >> >> >> >>> > values.
>> >> >> >> >>> > Commit "regulator: fix voltage check in
>> >> >> >> >>> > regulator_is_supported_voltage()"
>> >> >> >> >>> > fixed that function and now additional check is needed for
>> >> >> >> >>> > fixed
>> >> >> >> >>> > regulators.
>> >> >> >> >>> >
>> >> >> >> >>> > Signed-off-by: Marek Szyprowski <[email protected]>
>> >> >> >> >>> > ---
>> >> >> >> >>> > drivers/mmc/host/sdhci.c | 2 +-
>> >> >> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >> >> >>> >
>> >> >> >> >>> > diff --git a/drivers/mmc/host/sdhci.c
>> >> >> >> >>> > b/drivers/mmc/host/sdhci.c
>> >> >> >> >>> > index c7851c0..6f6534e 100644
>> >> >> >> >>> > --- a/drivers/mmc/host/sdhci.c
>> >> >> >> >>> > +++ b/drivers/mmc/host/sdhci.c
>> >> >> >> >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host
>> >> >> >> >>> > *host)
>> >> >> >> >>> > regulator_enable(host->vmmc);
>> >> >> >> >>> >
>> >> >> >> >>> > #ifdef CONFIG_REGULATOR
>> >> >> >> >>> > - if (host->vmmc) {
>> >> >> >> >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) >
>> >> >> >> >>> > 1)
>> >> >> >> >>> > {
>> >> >> >> >>> > ret = regulator_is_supported_voltage(host->vmmc,
>> >> >> >> >>> > 3300000,
>> >> >> >> >>> > 3300000);
>> >> >> >> >>> > if ((ret <= 0) || (!(caps[0] &
>> >> >> >> >>> > SDHCI_CAN_VDD_330)))
>> >> >> >> >>>
>> >> >> >> >>> Thanks for the longer explanation. I'm still missing
>> >> >> >> >>> something,
>> >> >> >> >>> though;
>> >> >> >> >>> what's wrong with running the check as it was with the new
>> >> >> >> >>> regulator
>> >> >> >> >>> code?
>> >> >> >> >>> (I haven't tried it yet.)
>> >> >> >> >>>
>> >> >> >> >>> #ifdef CONFIG_REGULATOR
>> >> >> >> >>> if (host->vmmc) {
>> >> >> >> >>> ret =
>> >> >> >> >>> regulator_is_supported_voltage(host->vmmc,
>> >> >> >> >>> 3300000,
>> >> >> >> >>> 3300000);
>> >> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >> >>> SDHCI_CAN_VDD_330)))
>> >> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> >> >> >> >>> ret =
>> >> >> >> >>> regulator_is_supported_voltage(host->vmmc,
>> >> >> >> >>> 3000000,
>> >> >> >> >>> 3000000);
>> >> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >> >>> SDHCI_CAN_VDD_300)))
>> >> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> >> >> >> >>> ret =
>> >> >> >> >>> regulator_is_supported_voltage(host->vmmc,
>> >> >> >> >>> 1800000,
>> >> >> >> >>> 1800000);
>> >> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >> >>> SDHCI_CAN_VDD_180)))
>> >> >> >> >>> caps[0] &= ~SDHCI_CAN_VDD_180;
>> >> >> >> >>> }
>> >> >> >> >>> #endif /* CONFIG_REGULATOR */
>> >> >> >> >>>
>> >> >> >> >>> The point is to remove unsupported voltages, so if someone
>> >> >> >> >>> sets
>> >> >> >> >>> up
>> >> >> >> >>> a
>> >> >> >> >>> fixed regulator at 3300000, all of the other caps are
>> >> >> >> >>> disabled.
>> >> >> >> >>> Why
>> >> >> >> >>> wouldn't that work without this change, and how are we
>> >> >> >> >>> supposed
>> >> >> >> >>> to
>> >> >> >> >>> remove those caps on a fixed regulator after your patchset?
>> >> >> >> >>>
>> >> >> >> >>> Thanks, sorry if I'm missing something obvious,
>> >> >> >> >>
>> >> >> >> >> On our boards eMMC is connected to fixed 2.8V regulator, what
>> >> >> >> >> results
>> >> >> >> >> in
>> >> >> >> >> clearing all available voltages and fail. The same situation
>> >> >> >> >> is
>> >> >> >> >> when
>> >> >> >> >> one
>> >> >> >> >> enable dummy regulator and try to use sdhci with it. My patch
>> >> >> >> >> fixes
>> >> >> >> >> this
>> >> >> >> >> and restores sdhci to working state as it was before (before
>> >> >> >> >> fixing
>> >> >> >> >> regulator regulator_is_supported_voltage() function and
>> >> >> >> >> earlier
>> >> >> >> >> when
>> >> >> >> >> MMC_BROKEN_VOLATGE capability was used).
>> >> >> >> >
>> >> >> >> > I see. Sounds like a separate bug -- Philip (or anyone else),
>> >> >> >> > any
>> >> >> >> > idea how we should be treating eMMCs with a fixed voltage here?
>> >> >> >> >
>> >> >> >>
>> >> >> >> I think we should check the voltage range rather than the voltage
>> >> >> >> point accoring to the spec.
>> >> >> >> Otherwise some valid voltage like 2.8v will be discarded by
>> >> >> >> mistake.
>> >> >> >> My below old patch aim to fix this issue.
>> >> >> >> How do you think?
>> >> >> >>
>> >> >> >> -----Original Message-----
>> >> >> >> From: Kevin Liu [mailto:[email protected]]
>> >> >> >> Sent: Friday, September 28, 2012 3:56 PM
>> >> >> >> To: [email protected]; [email protected]; [email protected];
>> >> >> >> [email protected]; Zhangfei Gao
>> >> >> >> Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing
>> >> >> >> Fu
>> >> >> >> Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max
>> >> >> >> voltage
>> >> >> >> range according to spec
>> >> >> >>
>> >> >> >> From: Kevin Liu <[email protected]>
>> >> >> >>
>> >> >> >> For regulator vmmc/vmmcq, use voltage range as below
>> >> >> >> 3.3v/3.0v: (2.7v, 3.6v)
>> >> >> >> 1.8v: (1.7v, 1.95v)
>> >> >> >> Original code use the specific value which may fail in regulator
>> >> >> >> driver if it does NOT support the specific voltage.
>> >> >> >>
>> >> >> >> Signed-off-by: Jialing Fu <[email protected]>
>> >> >> >> Signed-off-by: Kevin Liu <[email protected]>
>> >> >> >
>> >> >> >
>> >> >> > Tested-by: Marek Szyprowski <[email protected]>
>> >> >> >
>> >> >> > This patch restores sdhci devices to working state on Samsung
>> >> >> > boards
>> >> >> > (tested on GONI and UniversalC210) after merging "regulator: fix
>> >> >> > voltage
>> >> >> > check in regulator_is_supported_voltage()" patch to v3.7-rc6
>> >> >> > (commit
>> >> >> > f0f98b19e23d4426ca185e3d4ca80e6aff5ef51b). Would be great to have
>> >> >> > it
>> >> >> > merged before the final v3.7 is out.
>> >> >> >
>> >> >> Marek,
>> >> >>
>> >> >> thanks a lot for the verification!
>> >> >> And your patch "mmc: sdhci: apply voltage range check only for
>> >> >> non-fixed regulators" (commit
>> >> >> d5b5205f2d480a47863dda0772d2d9dc47c2b51b, which has been merged in
>> >> >> mmc-next) can be reverted if this patch merged?
>> >> >
>> >> >
>> >> > Yes, it can be replaced with it, although there is still an issue
>> >> > that
>> >> > need
>> >> > to be resolved somehow. Right now SDHCI driver fails to initialize if
>> >> > support
>> >> > for dummy regulator is enabled.
>> >> >
>> >> Then I think the dummy issue can be resolved with your patch merged
>> >> and if you can just update your patch from
>> >> "regulator_count_voltages(host->vmmc) > 1"
>> >> to
>> >> "regulator_count_voltages(host->vmmc) > 0"
>> >> to let fix regulator work.
>> >
>> >
>> > regulator_count_voltages() returns 1 for both fixed regulators and for
>> > virtual dummy regulator, so the above change makes no sense.
>>
>> I think regulator_count_voltage should return -EINVAL for dummy
>> regulator since n_voltages is not defined for dummy regulator:
>> int regulator_count_voltages(struct regulator *regulator)
>> {
>> struct regulator_dev *rdev = regulator->rdev;
>>
>> return rdev->desc->n_voltages ? : -EINVAL;
>> }
>> can you double check this?
>
>
> Err, right. I looked at the wrong SDHCI device :/ I have 3 of them on my
> board - one with fixed regulator, one with 'real' and one without (with
> virtual dummy regulator). I've applied my earlier patch and noticed that
> it cured sdhci driver with dummy regulator, so I concluded that it did
> the right job. Sorry for the confusion.
>
No problem, thanks for the check.
>> > However I was so focused on fixing the 2.8V supply case that I missed
>> > the
>> > fact that my "mmc: sdhci: apply voltage range check only for non-fixed
>> > regulators" patch also fixed the dummy regulator case.
>> >
>> > The conclusion is that applying both patches should finally fix the
>> > regulator issues with for the Samsung boards (2.8V supply for eMMC) and
>> > 'dummy-regulators' cases.
>> >
>>
>> After thinking again, I think we don't need the check for
>> regulator_count_voltages.
>> In fact, dummy regulator should NOT be used for the sd/mmc power
>> supply. We should use controllable or fixed regulator. If dummy
>> regulator is used, then it means we won't control it and we don't know
>> its voltage. It's not reasonable for sd/mmc power supply. If dummy
>> regulator is used, I think it's ok to return error and disable all
>> voltage support caps.
>
>
> The problem with dummy regulator is the fact that it can be enabled only
> globally for all devices in the system. I think that the best solution
> would be to introduce regulator_can_change_voltage() as Mark suggested.
> I will post patches soon.
>
I think both controllable and fixed regulator should check the voltage
range (fixed regulator also need to check its voltage whether resides
in the valid range). But regulator_can_change_voltage will return
false for both fixed and dummy regulator while return true for
controllable regulator.
Thanks
Kevin
Hi Marek,
On Tue, Nov 20 2012, Marek Szyprowski wrote:
> The problem with dummy regulator is the fact that it can be enabled only
> globally for all devices in the system. I think that the best solution
> would be to introduce regulator_can_change_voltage() as Mark suggested.
> I will post patches soon.
Does this mean that I shouldn't merge either yours or Kevin's patch for
3.8, while we wait for this? Any ETA on it?
Thanks very much,
- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child