Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only fornon-fixed regulators

[Marek Szyprowski]

Hello,

On 11/20/2012 12:36 PM, Kevin Liu wrote:
> 2012/11/20 Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>:
> > On 11/20/2012 9:59 AM, Kevin Liu wrote:
> >> 2012/11/20 Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>:
> >> > 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
> >> >> > 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?
> >
> >
> > 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


--
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/