Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only fornon-fixed regulators
From: Kevin Liu
Date: Tue Nov 20 2012 - 03:59:32 EST
2012/11/20 Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>:
> Hello,
>
>
> On 11/14/2012 8:11 AM, Kevin Liu wrote:
>>
>> > From: linux-mmc-owner@xxxxxxxxxxxxxxx
>> > [mailto:linux-mmc-owner@xxxxxxxxxxxxxxx] On Behalf Of Chris Ball
>> > Sent: Tuesday, November 13, 2012 10:14 PM
>> > To: Marek Szyprowski
>> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; 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 <m.szyprowski@xxxxxxxxxxx>
>> >>> > ---
>> >>> > 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:keyuan.liu@xxxxxxxxx]
>> Sent: Friday, September 28, 2012 3:56 PM
>> To: linux-mmc@xxxxxxxxxxxxxxx; cjb@xxxxxxxxxx; pierre@xxxxxxxxx;
>> ulf.hansson@xxxxxxxxxx; 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 <kliu5@xxxxxxxxxxx>
>>
>> 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 <jlfu@xxxxxxxxxxx>
>> Signed-off-by: Kevin Liu <kliu5@xxxxxxxxxxx>
>
>
> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>
> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/