Re: FW: Regulator API ignored return values
> -Original Message-
> From: linux-mmc-ow...@vger.kernel.org
> [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Chris Ball
> Sent: Tuesday, March 12, 2013 5:56 AM
> To: Arnd Bergmann
> Cc: linux-kernel@vger.kernel.org; Stephen Warren;
> linux-arm-ker...@lists.infradead.org; Mark Brown; Linus Walleij; Axel Lin;
> Jingoo Han; Felipe Balbi; Dmitry Torokhov; linux-...@vger.kernel.org
> Subject: Re: Regulator API ignored return values
>
> Hi,
>
> On Mon, Mar 11 2013, Arnd Bergmann wrote:
>> ==> build/dove_defconfig/faillog <==
>> /git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_add_host':
>> /git/arm-soc/drivers/mmc/host/sdhci.c:2910:19: warning: ignoring
>> return value of 'regulator_enable', declared with attribute
>> warn_unused_result [-Wunused-result]
>
> Thanks, this looks like the right fix to me:
>
>
> Subject: [PATCH] mmc: sdhci: Don't ignore regulator_enable() return value
>
> Fixes:
> /git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_add_host':
> /git/arm-soc/drivers/mmc/host/sdhci.c:2910:19: warning: ignoring
> return value of 'regulator_enable', declared with attribute
> warn_unused_result [-Wunused-result]
>
> Reported-by: Arnd Bergmann
> Signed-off-by: Chris Ball
> ---
> drivers/mmc/host/sdhci.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 51bbba4..fbf0b93 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2907,12 +2907,17 @@ int sdhci_add_host(struct sdhci_host *host)
> host->vqmmc = NULL;
> }
> } else {
> - regulator_enable(host->vqmmc);
> + ret = regulator_enable(host->vqmmc);
> if (!regulator_is_supported_voltage(host->vqmmc, 170,
> 195))
> caps[1] &= ~(SDHCI_SUPPORT_SDR104 |
> SDHCI_SUPPORT_SDR50 |
> SDHCI_SUPPORT_DDR50);
> + if (ret) {
> + pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> + mmc_hostname(mmc), ret);
Need add regulator_put here since regulator_get has succeed?
> + host->vqmmc = NULL;
> + }
> }
>
> if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
> --
> Chris Ball
Re: [PATCH 3/3 RESEND] mmc: sdhci: check voltage range only on regulators aware of voltage value
2013/2/14 Ulf Hansson :
> On 14 February 2013 09:05, Marek Szyprowski 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value
2012/12/5 Mark Brown
>
> On Tue, Dec 04, 2012 at 10:50:03PM +0800, Kevin Liu wrote:
> > 2012/12/4 Marek Szyprowski :
>
> > > + if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> > > ret = regulator_is_supported_voltage(host->vmmc,
> > > 270,
> > > 360);
> > > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>
> > Good idea. But how about the regulator is disabled at this point? So I
> > suggest to change to
> > if (host->vmmc && regulator_get_voltage(host->vmmc) >= 0)
>
> I'd not expect regulator_get_voltage() to return 0 for disabled
> regulators, I'd expect it to return the voltage the regulator will have
> when enabled.
Understand.
Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mmc: sdhci: check voltage range only on regulators aware of voltage value
2012/12/4 Marek Szyprowski :
> Some regulators don't report any voltage values, so checking supported
> voltage range results in disabling all SDHCI_CAN_VDD_* flags and
> registration failure. This patch finally provides a correct fix for the
> registration of SDHCI driver with all possible voltage regulators:
> dummy, fixed and regulated without using regulator_count_voltages()
> hacks.
>
> Signed-off-by: Marek Szyprowski
> ---
> drivers/mmc/host/sdhci.c |6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a9ad2cd..d244dc0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2930,7 +2930,11 @@ int sdhci_add_host(struct sdhci_host *host)
> }
>
> #ifdef CONFIG_REGULATOR
> - if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> + /*
> +* Voltage range check makes sense only if regulator reports
> +* any voltage value.
> +*/
> + if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> ret = regulator_is_supported_voltage(host->vmmc, 270,
> 360);
> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
Good idea. But how about the regulator is disabled at this point? So I
suggest to change to
if (host->vmmc && regulator_get_voltage(host->vmmc) >= 0)
Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/20 Marek Szyprowski :
> Hello,
>
>
> On 11/20/2012 3:14 PM, Kevin Liu wrote:
>>
>> 2012/11/20 Marek Szyprowski :
>> > Hello,
>> >
>> >
>> > On 11/20/2012 12:36 PM, Kevin Liu wrote:
>> >>
>> >> 2012/11/20 Marek Szyprowski :
>> >> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
>> >> >> 2012/11/20 Marek Szyprowski :
>> >> >> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
>> >> >> >>
>> >> >> >> > From: linux-mmc-ow...@vger.kernel.org
>> >> >> >> > [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Chris
>> >> >> >> > Ball
>> >> >> >> > Sent: Tuesday, November 13, 2012 10:14 PM
>> >> >> >> > To: Marek Szyprowski
>> >> >> >> > Cc: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org;
>> >> >> >> > 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
>> >> >> >> >>> > ---
>> >> >> >> >>> > 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,
>> >> >> >> >>> > 330,
>> >> >> >> >>> >330);
>> >> >> >> >>> >if ((ret <= 0) || (!(caps[0] &
>> >> >> >> >>> > SDHCI_CAN_VDD_330)))
>> >> >> >> >>>
>> >> >> >> >>> Thanks for the longer explanation. I'm still missing
>&g
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/20 Marek Szyprowski :
> Hello,
>
>
> On 11/20/2012 12:36 PM, Kevin Liu wrote:
>>
>> 2012/11/20 Marek Szyprowski :
>> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
>> >> 2012/11/20 Marek Szyprowski :
>> >> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
>> >> >>
>> >> >> > From: linux-mmc-ow...@vger.kernel.org
>> >> >> > [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Chris Ball
>> >> >> > Sent: Tuesday, November 13, 2012 10:14 PM
>> >> >> > To: Marek Szyprowski
>> >> >> > Cc: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org;
>> >> >> > 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
>> >> >> >>> > ---
>> >> >> >>> > 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,
>> >> >> >>> > 330,
>> >> >> >>> >330);
>> >> >> >>> >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,
>> >> >> >>> 330,
>> >> >> >>> 330);
>> >> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >> >>> SDHCI_CAN_VDD_330)))
>> >> >> >>> caps[
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/20 Marek Szyprowski :
> Hello,
>
>
> On 11/20/2012 9:59 AM, Kevin Liu wrote:
>>
>> 2012/11/20 Marek Szyprowski :
>> > Hello,
>> >
>> >
>> > On 11/14/2012 8:11 AM, Kevin Liu wrote:
>> >>
>> >> > From: linux-mmc-ow...@vger.kernel.org
>> >> > [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Chris Ball
>> >> > Sent: Tuesday, November 13, 2012 10:14 PM
>> >> > To: Marek Szyprowski
>> >> > Cc: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; 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
>> >> >>> > ---
>> >> >>> > 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,
>> >> >>> > 330,
>> >> >>> >330);
>> >> >>> >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,
>> >> >>> 330,
>> >> >>> 330);
>> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >>> SDHCI_CAN_VDD_330)))
>> >> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >> >>> 300,
>> >> >>> 300);
>> >> >>> if ((ret <= 0) || (!(caps[0] &
>> >> >>> SDHCI_CAN_VDD_300)))
>> >> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> >> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >> >>> 180,
>> >> >>> 180);
>> >> >>> if ((ret <=
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/20 Marek Szyprowski :
> Hello,
>
>
> On 11/14/2012 8:11 AM, Kevin Liu wrote:
>>
>> > From: linux-mmc-ow...@vger.kernel.org
>> > [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Chris Ball
>> > Sent: Tuesday, November 13, 2012 10:14 PM
>> > To: Marek Szyprowski
>> > Cc: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; 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
>> >>> > ---
>> >>> > 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, 330,
>> >>> >330);
>> >>> >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,
>> >>> 330,
>> >>> 330);
>> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>> >>> caps[0] &= ~SDHCI_CAN_VDD_330;
>> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >>> 300,
>> >>> 300);
>> >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>> >>> caps[0] &= ~SDHCI_CAN_VDD_300;
>> >>> ret = regulator_is_supported_voltage(host->vmmc,
>> >>> 180,
>> >>> 180);
>> >>> 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 330, 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
Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/14 Philip Rakity : > > On Nov 14, 2012, at 8:57 AM, Kevin Liu wrote: > >> 2012/11/14 Mark Brown : >>> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >>>> 2012/11/14 Mark Brown : >>> >>>>> 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 majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > re-read spec. Please apply Kevin;s patch. > > Reviewed-by: Philip Rakity Philip, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/14 Mark Brown : > On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >> 2012/11/14 Mark Brown : > >> > 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/14 Mark Brown : > On Wed, Nov 14, 2012 at 03:11:37PM +0800, Kevin Liu wrote: > >> - ret = regulator_set_voltage(host->vqmmc, 330, 330); >> + ret = regulator_set_voltage(host->vqmmc, 270, 360); > > 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, 170, >> + 195); > > We should really add a regulator_is_supported_voltage_tol... let me > just do that. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
> From: linux-mmc-ow...@vger.kernel.org
> [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Chris Ball
> Sent: Tuesday, November 13, 2012 10:14 PM
> To: Marek Szyprowski
> Cc: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; 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
>>> > ---
>>> > 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, 330,
>>> >330);
>>> >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, 330,
>>> 330);
>>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>> caps[0] &= ~SDHCI_CAN_VDD_330;
>>> ret = regulator_is_supported_voltage(host->vmmc, 300,
>>> 300);
>>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>>> caps[0] &= ~SDHCI_CAN_VDD_300;
>>> ret = regulator_is_supported_voltage(host->vmmc, 180,
>>> 180);
>>> 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 330, 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@gmail.com]
Sent: Friday, September 28, 2012 3:56 PM
To: linux-...@vger.kernel.org; c...@laptop.org; pie...@ossman.eu;
ulf.hans...@linaro.org; Zhangfei Gao
Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu
Subject: [PATCH v5 03/13] mmc:
