| Message ID | 1352813534-6795-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek, 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@samsung.com> > --- > 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, - Chris.
Hello, On 11/13/2012 2:45 PM, Chris Ball wrote: > Hi Marek, > > 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@samsung.com> > > --- > > 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). Best regards
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@samsung.com> >> > --- >> > 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? Thanks, - Chris.
Hi Marek, Is the regulator dedicated ? or is it shared ? Is it used for eMMC ? If it cannot be turned off -- then just don't list it in the regulators list for vmmc. If it CAN be turned off then need to get back to you. Philip On Nov 13, 2012, at 2:14 PM, Chris Ball <cjb@laptop.org> wrote: > 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@samsung.com> >>>> --- >>>> 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? > > Thanks, > > - Chris. > -- > Chris Ball <cjb@laptop.org> <http://printf.net/> > One Laptop Per Child -- 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
On 11/13/2012 10:23 PM, Philip Rakity wrote: > Hi Marek, > > Is the regulator dedicated ? or is it shared ? Is it used for eMMC ? > > If it cannot be turned off -- then just don't list it in the regulators list for vmmc. > > If it CAN be turned off then need to get back to you. It is dedicated to eMMC device and can be turned off. Patch "mmc: sdhci: apply voltage range check only for non-fixed regulators" restored sdhci to working state after merging the regulator fix. However there are lots of error messages from sdhci driver: s3c-sdhci s3c-sdhci.0: could not set regulator OCR (-1) The only remaining problem is sdhci driver operation with dummy regulator. Right now it simply fails to initialize if dummy regulator is enabled. Do you have any idea how to fix it properly? Best regards
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)))
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@samsung.com> --- drivers/mmc/host/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)