| Message ID | 1360656096-1671-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek On Tue, 12 Feb 2013, Marek Szyprowski wrote: > Some regulators don't report any voltage values, so checking supported > voltage range results in disabling all SDHCI_CAN_VDD_* flags and > registration failure. This patch finally provides a correct fix for the > registration of SDHCI driver with all possible voltage regulators: > dummy, fixed and regulated without using regulator_count_voltages() > hacks. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/mmc/host/sdhci.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index ba586ae..735526b 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host) > } > > #ifdef CONFIG_REGULATOR > - if (host->vmmc) { > + /* > + * Voltage range check makes sense only if regulator reports > + * any voltage value. > + */ > + if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) { > ret = regulator_is_supported_voltage(host->vmmc, 2700000, > 3600000); Wouldn't using mmc_regulator_get_ocrmask() be a better option? Thanks Guennadi > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
Hello, On 2/12/2013 11:10 PM, Guennadi Liakhovetski wrote: > Hi Marek > > On Tue, 12 Feb 2013, Marek Szyprowski wrote: > > > Some regulators don't report any voltage values, so checking supported > > voltage range results in disabling all SDHCI_CAN_VDD_* flags and > > registration failure. This patch finally provides a correct fix for the > > registration of SDHCI driver with all possible voltage regulators: > > dummy, fixed and regulated without using regulator_count_voltages() > > hacks. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > --- > > drivers/mmc/host/sdhci.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index ba586ae..735526b 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host) > > } > > > > #ifdef CONFIG_REGULATOR > > - if (host->vmmc) { > > + /* > > + * Voltage range check makes sense only if regulator reports > > + * any voltage value. > > + */ > > + if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) { > > ret = regulator_is_supported_voltage(host->vmmc, 2700000, > > 3600000); > > Wouldn't using mmc_regulator_get_ocrmask() be a better option? The idea behind this patch it to avoid messing ocr mask and voltage regulators when voltage regulator is a simple on/off switch, which doesn't report any value. This solves the serious problems with sdhci driver when dummy regulator is enabled in kconfig, otherwise the sdhci driver concludes that no supported voltage is available and fails to initialize. Using mmc_regulator_get_ocrmask() won't solve this problem. BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators. Best regards
On Wed, 13 Feb 2013, Marek Szyprowski wrote: > Hello, > > On 2/12/2013 11:10 PM, Guennadi Liakhovetski wrote: > > Hi Marek > > > > On Tue, 12 Feb 2013, Marek Szyprowski wrote: > > > > > Some regulators don't report any voltage values, so checking supported > > > voltage range results in disabling all SDHCI_CAN_VDD_* flags and > > > registration failure. This patch finally provides a correct fix for the > > > registration of SDHCI driver with all possible voltage regulators: > > > dummy, fixed and regulated without using regulator_count_voltages() > > > hacks. > > > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > --- > > > drivers/mmc/host/sdhci.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > > index ba586ae..735526b 100644 > > > --- a/drivers/mmc/host/sdhci.c > > > +++ b/drivers/mmc/host/sdhci.c > > > @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host) > > > } > > > > > > #ifdef CONFIG_REGULATOR > > > - if (host->vmmc) { > > > + /* > > > + * Voltage range check makes sense only if regulator reports > > > + * any voltage value. > > > + */ > > > + if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) { > > > ret = regulator_is_supported_voltage(host->vmmc, 2700000, > > > 3600000); > > > > Wouldn't using mmc_regulator_get_ocrmask() be a better option? > > The idea behind this patch it to avoid messing ocr mask and voltage > regulators when voltage regulator is a simple on/off switch, which doesn't > report any value. Wouldn't mmc_regulator_get_ocrmask() also report an error back in this case? > This solves the serious problems with sdhci driver when > dummy regulator is enabled in kconfig, otherwise the sdhci driver > concludes that no supported voltage is available and fails to initialize. > Using mmc_regulator_get_ocrmask() won't solve this problem. > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators. This seems like a problem, that has to be fixed... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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 Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote: > On Wed, 13 Feb 2013, Marek Szyprowski wrote: > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators. > This seems like a problem, that has to be fixed... Indeed, what's the issue?
On 2/13/2013 12:35 PM, Mark Brown wrote: > On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote: > > On Wed, 13 Feb 2013, Marek Szyprowski wrote: > > > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range regulators. > > > This seems like a problem, that has to be fixed... > > Indeed, what's the issue? There are probably 2 issues: 1. mmc_regulator_get_ocrmask() works only with regulators which support regulator_count_voltages() and regulator_list_voltage(). Recently support for continuous regulators have been merged. Such regulators doesn't provide regulator_list_voltage() method, but are able to change/set voltage to the given value. I agree that they are not very common, so right now we can probably ignore them until the first board, which uses them arrives. 2. The second issue might be related to the testing of precise voltage values in the ocr mask, not the whole allowed ranges. Such issues in sdhci.c driver has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503 ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I don't know MMC core code to judge if ocr mask is used for exact voltage checking or only for checking the voltage ranges. However someone with good mmc subsystem knowledge should check it. Best regards
On Thu, Feb 14, 2013 at 09:05:43AM +0100, Marek Szyprowski wrote: > 1. mmc_regulator_get_ocrmask() works only with regulators which support > regulator_count_voltages() and regulator_list_voltage(). Recently > support for > continuous regulators have been merged. Such regulators doesn't provide > regulator_list_voltage() method, but are able to change/set voltage to the > given value. I agree that they are not very common, so right now we can > probably ignore them until the first board, which uses them arrives. OK, I think this should be changed to use regulator_is_supported_voltage() to pick a range if list_voltage() isn't there, we don't want to list an extremely large number of voltages so using list_voltage() for continous regulators wouldn't make sense. > 2. The second issue might be related to the testing of precise > voltage values > in the ocr mask, not the whole allowed ranges. Such issues in > sdhci.c driver > has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503 > ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I > don't know MMC core code to judge if ocr mask is used for exact voltage > checking or only for checking the voltage ranges. However someone with good > mmc subsystem knowledge should check it. Looking at the code I'd expect it to work with continuous regulators, if it doesn't we should fix that.
On 14 February 2013 09:05, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 2/13/2013 12:35 PM, Mark Brown wrote: >> >> On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote: >> > On Wed, 13 Feb 2013, Marek Szyprowski wrote: >> >> > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range >> > > regulators. >> >> > This seems like a problem, that has to be fixed... >> >> Indeed, what's the issue? > > > There are probably 2 issues: > > 1. mmc_regulator_get_ocrmask() works only with regulators which support > regulator_count_voltages() and regulator_list_voltage(). Recently support > for > continuous regulators have been merged. Such regulators doesn't provide > regulator_list_voltage() method, but are able to change/set voltage to the > given value. I agree that they are not very common, so right now we can > probably ignore them until the first board, which uses them arrives. > > 2. The second issue might be related to the testing of precise voltage > values > in the ocr mask, not the whole allowed ranges. Such issues in sdhci.c driver > has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503 > ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I > don't know MMC core code to judge if ocr mask is used for exact voltage > checking or only for checking the voltage ranges. However someone with good > mmc subsystem knowledge should check it. > Not 100% sure what your problem relates too here, but I am aware of an issue for how the mmc protocol layer are handling ocr_masks. Let me try to describe it here: 1. During "card init" mmc_power_up will be called for telling the host driver to provide power to the card. The level of voltage will be set to "ocr_avail" which means the highest supported voltage by the host. 2. At the protocol layer the card init sequence tries to negotiate to lowest possible ocr value from what the card and the host together supports. Once done, the ocr mask value will be cached into the host struct. 3. The host will informed about the new ocr mask from the protocol layer with mmc_select_voltage and it somewhere here the problems starts. No host are actually changing the voltage level at this state (MMC_POWER_ON) which is correct since it would likely mean violation of the spec. At the same time the protocol layer still believes the host has switched to operate at the new voltage level. 4. So the host and the protocol layer are out of sync with regards to the ocr mask, which is why the cached ocr_mask in the host struct is reset when doing mmc_power_off. Otherwise the suspend/resume sequence would have been broken. I have been looking into a solution for the above problem, but has not yet been able to finalize the task. Hope this did not become more fussy now. :-) > > Best regards > -- > Marek Szyprowski > Samsung Poland R&D Center > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Kind regards Ulf Hansson -- 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
2013/2/14 Ulf Hansson <ulf.hansson@linaro.org>: > On 14 February 2013 09:05, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> >> On 2/13/2013 12:35 PM, Mark Brown wrote: >>> >>> On Wed, Feb 13, 2013 at 08:45:56AM +0100, Guennadi Liakhovetski wrote: >>> > On Wed, 13 Feb 2013, Marek Szyprowski wrote: >>> >>> > > BTW, mmc_regulator_get_ocrmask() won't work with continuous range >>> > > regulators. >>> >>> > This seems like a problem, that has to be fixed... >>> >>> Indeed, what's the issue? >> >> >> There are probably 2 issues: >> >> 1. mmc_regulator_get_ocrmask() works only with regulators which support >> regulator_count_voltages() and regulator_list_voltage(). Recently support >> for >> continuous regulators have been merged. Such regulators doesn't provide >> regulator_list_voltage() method, but are able to change/set voltage to the >> given value. I agree that they are not very common, so right now we can >> probably ignore them until the first board, which uses them arrives. >> >> 2. The second issue might be related to the testing of precise voltage >> values >> in the ocr mask, not the whole allowed ranges. Such issues in sdhci.c driver >> has been recently fixed by commit cec2e216f72c6b5ccdadb60aadbe99821d744503 >> ("mmc: sdhci: Use regulator min/max voltage range according to spec"), but I >> don't know MMC core code to judge if ocr mask is used for exact voltage >> checking or only for checking the voltage ranges. However someone with good >> mmc subsystem knowledge should check it. >> > > Not 100% sure what your problem relates too here, but I am aware of an > issue for how the mmc protocol layer are handling ocr_masks. Let me > try to describe it here: > > 1. During "card init" mmc_power_up will be called for telling the host > driver to provide power to the card. The level of voltage will be set > to "ocr_avail" which means the highest supported voltage by the host. > 2. At the protocol layer the card init sequence tries to negotiate to > lowest possible ocr value from what the card and the host together > supports. Once done, the ocr mask value will be cached into the host > struct. > 3. The host will informed about the new ocr mask from the protocol > layer with mmc_select_voltage and it somewhere here the problems > starts. No host are actually changing the voltage level at this state > (MMC_POWER_ON) which is correct since it would likely mean violation > of the spec. At the same time the protocol layer still believes the > host has switched to operate at the new voltage level. According to the spec, only during the initialization procedure, the host is not allowed to change the operating voltage range. In fact, mmc_select_voltage is called before init start in mmc_attach_sd/mmc_attach_mmc/mmc_attach_sdio. So voltage level should can be changed during MMC_POWER_ON. sdhci.c did this way and worked well. But I noticed some other hosts didn't change voltage for MMC_POWER_ON. I think it should be changed. > 4. So the host and the protocol layer are out of sync with regards to > the ocr mask, which is why the cached ocr_mask in the host struct is > reset when doing mmc_power_off. Otherwise the suspend/resume sequence > would have been broken. > But there indeed exist issues that the host->ocr doesn't restore after resume back and operation voltage use the highest voltage. Just submitted below patch to fix this. Please help to review. "[PATCH] mmc: core: restore ocr and operation voltage in resume" Thanks Kevin -- 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
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index ba586ae..735526b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2976,7 +2976,11 @@ int sdhci_add_host(struct sdhci_host *host) } #ifdef CONFIG_REGULATOR - if (host->vmmc) { + /* + * Voltage range check makes sense only if regulator reports + * any voltage value. + */ + if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) { ret = regulator_is_supported_voltage(host->vmmc, 2700000, 3600000); if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
Some regulators don't report any voltage values, so checking supported voltage range results in disabling all SDHCI_CAN_VDD_* flags and registration failure. This patch finally provides a correct fix for the registration of SDHCI driver with all possible voltage regulators: dummy, fixed and regulated without using regulator_count_voltages() hacks. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/mmc/host/sdhci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)