[BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Peter Geis]

Good Morning,

I've encountered a fun issue with the dw-mmc driver while working on
enabling support for the Quartz-64 Model A.
The regulator-fixed driver supports enabling via a gpio, but does not
have the ops to set voltage as it is fixed.
The dw-mmc calls mmc_regulator_set_ocr for vmmc, which attempts to set
the voltage first but fails due to the lack of the voltage ops. It
then bails returning -EINVAL.
This leads to the following message :
dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)

This can be fixed by switching to regulator-gpio for the vmmc supply
to the sdmmc controller, however the sdio controller vmmc is provided
by a fixed regulator that is always on. Obviously the regulator-gpio
isn't an option, as it has no gpio to enable.

Removing the vmmc phandle from the sdio node is an option, but then it
doesn't fully describe the hardware (it's also a non-standard 4.4v).
I had considered changing the check in dw-mmc.c [1] to continue in the
case of -EINVAL, but there are other places in the regulator framework
that can also return that and it doesn't address the underlying issue.

As such I'm reaching out to the experts to see what the best course of
action is here.
Please weigh in with what you think.

Very Respectfully,
Peter Geis

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On Wed, Aug 04, 2021 at 10:32:52AM -0400, Peter Geis wrote:

> Removing the vmmc phandle from the sdio node is an option, but then it
> doesn't fully describe the hardware (it's also a non-standard 4.4v).
> I had considered changing the check in dw-mmc.c [1] to continue in the
> case of -EINVAL, but there are other places in the regulator framework
> that can also return that and it doesn't address the underlying issue.

What is the underlying issue that you don't see as being fixed if the
MMC code is able to cope with not being able to read the voltage?  If we
don't know what voltage the regulator has then no amount of wishful
thinking is going to give us that knowledge, if we want to proceed with
controlling the device then the MMC code is going to need to make some
decisions about what it's safe to do given the limited information it
has available to it.  Otherwise there's no option other than providing
the information about the voltage.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Jaehoon Chung]

Hi,

On 8/4/21 11:32 PM, Peter Geis wrote:
> Good Morning,
> 
> I've encountered a fun issue with the dw-mmc driver while working on
> enabling support for the Quartz-64 Model A.
> The regulator-fixed driver supports enabling via a gpio, but does not
> have the ops to set voltage as it is fixed.
> The dw-mmc calls mmc_regulator_set_ocr for vmmc, which attempts to set
> the voltage first but fails due to the lack of the voltage ops. It
> then bails returning -EINVAL.
> This leads to the following message :
> dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)

What is vdd value (ocr_avail value) on your target?
I didn't see its case until now. If there is a real bug, I will try to check again.

Best Regards,
Jaehoon Chung

> 
> This can be fixed by switching to regulator-gpio for the vmmc supply
> to the sdmmc controller, however the sdio controller vmmc is provided
> by a fixed regulator that is always on. Obviously the regulator-gpio
> isn't an option, as it has no gpio to enable.
> 
> Removing the vmmc phandle from the sdio node is an option, but then it
> doesn't fully describe the hardware (it's also a non-standard 4.4v).
> I had considered changing the check in dw-mmc.c [1] to continue in the
> case of -EINVAL, but there are other places in the regulator framework
> that can also return that and it doesn't address the underlying issue.
> 
> As such I'm reaching out to the experts to see what the best course of
> action is here.
> Please weigh in with what you think.
> 
> Very Respectfully,
> Peter Geis
> 

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Peter Geis]

On Thu, Aug 5, 2021 at 5:59 AM Jaehoon Chung <jh80.chung@samsung.com> wrote:
>
> Hi,
>
> On 8/4/21 11:32 PM, Peter Geis wrote:
> > Good Morning,
> >
> > I've encountered a fun issue with the dw-mmc driver while working on
> > enabling support for the Quartz-64 Model A.
> > The regulator-fixed driver supports enabling via a gpio, but does not
> > have the ops to set voltage as it is fixed.
> > The dw-mmc calls mmc_regulator_set_ocr for vmmc, which attempts to set
> > the voltage first but fails due to the lack of the voltage ops. It
> > then bails returning -EINVAL.
> > This leads to the following message :
> > dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)
>
> What is vdd value (ocr_avail value) on your target?
> I didn't see its case until now. If there is a real bug, I will try to check again.

What brought this front and center is actually another bug with the
dw-mmc-rk driver (I don't know if it affects other dw-mmc drivers)
vmmc is 3.3v on this board, but with broken-cd set polling causes the
following splat constantly.

[   30.719846] dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)
[   30.720488] dwmmc_rockchip fe2b0000.mmc: failed to enable vmmc regulator
[   30.763736] dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)
[   30.764379] dwmmc_rockchip fe2b0000.mmc: failed to enable vmmc regulator
[   30.778231] dwmmc_rockchip fe2b0000.mmc: failed to set rate 300000Hz
[   30.795042] dwmmc_rockchip fe2b0000.mmc: failed to set rate 300000Hz
[   30.798735] dwmmc_rockchip fe2b0000.mmc: failed to set rate 300000Hz
[   30.816591] dwmmc_rockchip fe2b0000.mmc: failed to set rate 300000Hz
[   30.820303] dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)
[   30.820944] dwmmc_rockchip fe2b0000.mmc: failed to enable vmmc regulator
[   30.834794] dwmmc_rockchip fe2b0000.mmc: failed to set rate 200000Hz
[   30.851606] dwmmc_rockchip fe2b0000.mmc: failed to set rate 200000Hz
[   30.855264] dwmmc_rockchip fe2b0000.mmc: failed to set rate 200000Hz
[   30.873197] dwmmc_rockchip fe2b0000.mmc: failed to set rate 200000Hz
[   30.876908] dwmmc_rockchip fe2b0000.mmc: could not set regulator OCR (-22)
[   30.877549] dwmmc_rockchip fe2b0000.mmc: failed to enable vmmc regulator
[   30.891396] dwmmc_rockchip fe2b0000.mmc: failed to set rate 100000Hz
[   30.911704] dwmmc_rockchip fe2b0000.mmc: failed to set rate 100000Hz
[   30.915586] dwmmc_rockchip fe2b0000.mmc: failed to set rate 100000Hz
[   30.948637] dwmmc_rockchip fe2b0000.mmc: failed to set rate 100000Hz

I looked through the code path and if the OCR voltage is set to the
fixed regulator voltage it should just return success. I think I need
to change that error to output the voltage it tried to set, just to
see if something weird is going on.

Also, I've got a possible fix to the dw-mmc issue, the following patch
changes the behavior to only enable a fixed regulator, not try to set
the voltage. It's a split between the behavior when vmmc isn't defined
at all and when its a variable regulator:

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d333130d1531..b30102980261 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1446,11 +1446,13 @@ static void dw_mci_set_ios(struct mmc_host
*mmc, struct mmc_ios *ios)
  switch (ios->power_mode) {
  case MMC_POWER_UP:
  if (!IS_ERR(mmc->supply.vmmc)) {
- ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
- ios->vdd);
+ if (mmc->ocr_avail > 0)
+ ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd);
+ else
+ ret = regulator_enable(mmc->supply.vmmc);
+
  if (ret) {
- dev_err(slot->host->dev,
- "failed to enable vmmc regulator\n");
+ dev_err(slot->host->dev, "failed to enable vmmc regulator\n");
  /*return, if failed turn on vmmc*/
  return;
  }

>
> Best Regards,
> Jaehoon Chung
>
> >
> > This can be fixed by switching to regulator-gpio for the vmmc supply
> > to the sdmmc controller, however the sdio controller vmmc is provided
> > by a fixed regulator that is always on. Obviously the regulator-gpio
> > isn't an option, as it has no gpio to enable.
> >
> > Removing the vmmc phandle from the sdio node is an option, but then it
> > doesn't fully describe the hardware (it's also a non-standard 4.4v).
> > I had considered changing the check in dw-mmc.c [1] to continue in the
> > case of -EINVAL, but there are other places in the regulator framework
> > that can also return that and it doesn't address the underlying issue.
> >
> > As such I'm reaching out to the experts to see what the best course of
> > action is here.
> > Please weigh in with what you think.
> >
> > Very Respectfully,
> > Peter Geis
> >
>

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Thu, Aug 05, 2021 at 07:38:06AM -0400, Peter Geis wrote:

> Also, I've got a possible fix to the dw-mmc issue, the following patch
> changes the behavior to only enable a fixed regulator, not try to set
> the voltage. It's a split between the behavior when vmmc isn't defined
> at all and when its a variable regulator:

One thing to watch out for with this approach is if there's things that
really need a specific voltage to be set then you'll have to stop those
things happening if you've got a voltage regulator that can't deliver a
voltage in the required range.  I don't know if this affects MMC or not,
if it's just a case of being less efficient it's not such an issue.

> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index d333130d1531..b30102980261 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1446,11 +1446,13 @@ static void dw_mci_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
>   switch (ios->power_mode) {
>   case MMC_POWER_UP:
>   if (!IS_ERR(mmc->supply.vmmc)) {
> - ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,

This patch is very whitespace damaged FWIW.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Peter Geis]

On Thu, Aug 5, 2021 at 8:47 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Aug 05, 2021 at 07:38:06AM -0400, Peter Geis wrote:
>
> > Also, I've got a possible fix to the dw-mmc issue, the following patch
> > changes the behavior to only enable a fixed regulator, not try to set
> > the voltage. It's a split between the behavior when vmmc isn't defined
> > at all and when its a variable regulator:
>
> One thing to watch out for with this approach is if there's things that
> really need a specific voltage to be set then you'll have to stop those
> things happening if you've got a voltage regulator that can't deliver a
> voltage in the required range.  I don't know if this affects MMC or not,
> if it's just a case of being less efficient it's not such an issue.

Yeah, but if this is a fixed regulator and it's a problem, then the
hardware is screwed anyways.

>
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index d333130d1531..b30102980261 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1446,11 +1446,13 @@ static void dw_mci_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >   switch (ios->power_mode) {
> >   case MMC_POWER_UP:
> >   if (!IS_ERR(mmc->supply.vmmc)) {
> > - ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
>
> This patch is very whitespace damaged FWIW.

Unfortunately gmail doesn't let you reply with patch sets without
mangling them, so it's only useful as an example.
If it seems sane I'll send it as a proper patch.

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On Thu, Aug 05, 2021 at 08:58:58AM -0400, Peter Geis wrote:
> On Thu, Aug 5, 2021 at 8:47 AM Mark Brown <broonie@kernel.org> wrote:

> > One thing to watch out for with this approach is if there's things that
> > really need a specific voltage to be set then you'll have to stop those
> > things happening if you've got a voltage regulator that can't deliver a
> > voltage in the required range.  I don't know if this affects MMC or not,
> > if it's just a case of being less efficient it's not such an issue.

> Yeah, but if this is a fixed regulator and it's a problem, then the
> hardware is screwed anyways.

Well, the fact that the voltage is being changed at runtime indicates
that we're changing something from whatever was in the fixed setup - it
can sometimes be that we don't have access to some higher performance or
lower power features for example.  That's not ideal but works perfectly
safely.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Ravikumar Kattekola]

Hi,
Resending my reply as my Mail client settings prevented delivery

On 05/08/21 6:38 pm, Mark Brown wrote:
> On Thu, Aug 05, 2021 at 08:58:58AM -0400, Peter Geis wrote:
>> On Thu, Aug 5, 2021 at 8:47 AM Mark Brown <broonie@kernel.org> wrote:
> 
>>> One thing to watch out for with this approach is if there's things that
>>> really need a specific voltage to be set then you'll have to stop those
>>> things happening if you've got a voltage regulator that can't deliver a
>>> voltage in the required range.  I don't know if this affects MMC or not,
>>> if it's just a case of being less efficient it's not such an issue.
> 
>> Yeah, but if this is a fixed regulator and it's a problem, then the
>> hardware is screwed anyways.
> 
> Well, the fact that the voltage is being changed at runtime indicates
> that we're changing something from whatever was in the fixed setup - it
> can sometimes be that we don't have access to some higher performance or
> lower power features for example.  That's not ideal but works perfectly
> safely.
> 
Suggested approach of checking "mmc->ocr_avail" might work.

But, IMO mmc core should check if the voltage can be changed or not

before trying to do regulator_set_voltage() in mmc_regulator_set_ocr().

Wouldn't that be better and solve this issue for other hosts as well.

Something like below in mmc_regulator_set_ocr ():

+               result = regulator_check_voltage_constraints(supply,
+                       min_uV, max_uV);
+               if(!result) {
+                       result = regulator_set_voltage(supply, min_uV, 
max_uV);
+                       if (result != -EINVAL && !mmc->regulator_enabled) {
+                               result = regulator_enable(supply);
+                               if (!result)
+                                       mmc->regulator_enabled = true;
+                       }

We could wrap the existing check_voltage function

+/* Check voltage constraints helper function */
+int regulator_check_voltage_constraints(struct regulator *regulator,
+                                       int min_uV, int max_uV)
+{
+       return regulator_check_voltage(regulator->rdev, &min_uV, &max_uV);
+}
+EXPORT_SYMBOL_GPL(regulator_check_voltage_constraints);

I hope this makes sense.

Regards,
RK

Re: [BUG] mmc_regulator_set_ocr can't cope with regulator-fixed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

On Fri, Aug 06, 2021 at 01:44:45PM +0530, Ravikumar Kattekola wrote:

> But, IMO mmc core should check if the voltage can be changed or not
> 
> before trying to do regulator_set_voltage() in mmc_regulator_set_ocr().

It does exactly that in mmc_regulator_get_ocrmask().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]