Question about "regulator: core: Only count load for enabled consumers" in -next

Question about "regulator: core: Only count load for enabled consumers" in -next

[Brian Masney]

Hi all,

I see errors like the following in linux next-20181123 when trying to
boot a mainline kernel on a LG Nexus 5 phone:

[   14.495056] mmc1: Card stuck in wrong state! mmcblk1 card_busy_detect status: 0xe00
[   14.495185] mmc1: cache flush error -110
[   14.601547] mmc1: Reset 0x1 never completed.
[   14.601572] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
[   14.604882] mmc1: sdhci: Sys addr:  0x00000000 | Version:  0x00003802
[   14.611240] mmc1: sdhci: Blk size:  0x00004000 | Blk cnt:  0x00000000
[   14.617664] mmc1: sdhci: Argument:  0x00000000 | Trn mode: 0x00000000
[   14.624064] mmc1: sdhci: Present:   0x01e80000 | Host ctl: 0x00000000
[   14.630508] mmc1: sdhci: Power:     0x00000000 | Blk gap:  0x00000000
[   14.636930] mmc1: sdhci: Wake-up:   0x00000000 | Clock:    0x00000003
[   14.643335] mmc1: sdhci: Timeout:   0x00000000 | Int stat: 0x00000000
[   14.649778] mmc1: sdhci: Int enab:  0x00000000 | Sig enab: 0x00000000
[   14.656200] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[   14.662606] mmc1: sdhci: Caps:      0x642dc8b2 | Caps_1:   0x00008007
[   14.669051] mmc1: sdhci: Cmd:       0x00000000 | Max curr: 0x00000000
[   14.675469] mmc1: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
[   14.681877] mmc1: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
[   14.688320] mmc1: sdhci: Host ctl2: 0x00000000
[   14.694720] mmc1: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0x70042330
[   14.699080] mmc1: sdhci: ============================================

I bisected the issue to the following commit:

5451781dadf8 ("regulator: core: Only count load for enabled consumers")

We have to increase the load for the sdhci in device tree in order for
the phone to boot properly. This change was made with the commit:

03864e57770a ("ARM: dts: qcom: msm8974-hammerhead: increase load on l20
for sdhci")

Is there a change that I should make in device tree to get the SD card
working again on the phone?

Thanks,

Brian

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Doug Anderson]

Hi,

On Sun, Nov 25, 2018 at 1:37 AM Brian Masney <masneyb@onstation.org> wrote:
> I bisected the issue to the following commit:
>
> 5451781dadf8 ("regulator: core: Only count load for enabled consumers")
>
> We have to increase the load for the sdhci in device tree in order for
> the phone to boot properly. This change was made with the commit:
>
> 03864e57770a ("ARM: dts: qcom: msm8974-hammerhead: increase load on l20
> for sdhci")

You have a 200 mA system load on this regulator?  I guess this is a
workaround for drivers that don't set the load properly themselves?

I wonder if there is a bug in my patch where the system load doesn't
take effect if nobody ever calls set_load.  Let's see...  Does the
below fix things for you?  It's totally untested and whitespace
damaged but I wanted to get a response out quick and I'm just walking
out the door.  I'll test more / dig more either tonight or at work
tomorrow:

+++ b/drivers/regulator/core.c
@@ -1344,6 +1344,12 @@ static int set_machine_constraints(struct
regulator_dev *rdev,
                        rdev_err(rdev, "failed to set initial mode: %d\n", ret);
                        return ret;
                }
+       } else if (rdev->constraints->system_load) {
+               /*
+                * We'll only apply the initial system load if an
+                * initial mode wasn't specified.
+                */
+               drms_uA_update(rdev);
        }

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Brian Masney]

Hi Doug,

On Sun, Nov 25, 2018 at 09:20:02AM -0800, Doug Anderson wrote:
> On Sun, Nov 25, 2018 at 1:37 AM Brian Masney <masneyb@onstation.org> wrote:
> > I bisected the issue to the following commit:
> >
> > 5451781dadf8 ("regulator: core: Only count load for enabled consumers")
> >
> > We have to increase the load for the sdhci in device tree in order for
> > the phone to boot properly. This change was made with the commit:
> >
> > 03864e57770a ("ARM: dts: qcom: msm8974-hammerhead: increase load on l20
> > for sdhci")
> 
> You have a 200 mA system load on this regulator?

Yes.

> I guess this is a workaround for drivers that don't set the load
> properly themselves?

I'm honestly not sure when the load should be set in the driver or in
device tree. None of the drivers in drivers/mmc/ call
regulator_set_load. The dt bindings describes the regulator-system-load
property in Documentation/devicetree/bindings/regulator/regulator.txt.

I see that there are 8 users of regulator-system-load but most are all
addressing this same issue with the SD card. 
qcom-msm8974-sony-xperia-castor.dts sets the load to 500 mA but all of
the other msm8974-based SOCs use 200 mA. I'm not sure if this is
correct.

> I wonder if there is a bug in my patch where the system load doesn't
> take effect if nobody ever calls set_load.  Let's see...  Does the
> below fix things for you?  It's totally untested and whitespace
> damaged but I wanted to get a response out quick and I'm just walking
> out the door.  I'll test more / dig more either tonight or at work
> tomorrow:
> 
> +++ b/drivers/regulator/core.c
> @@ -1344,6 +1344,12 @@ static int set_machine_constraints(struct
> regulator_dev *rdev,
>                         rdev_err(rdev, "failed to set initial mode: %d\n", ret);
>                         return ret;
>                 }
> +       } else if (rdev->constraints->system_load) {
> +               /*
> +                * We'll only apply the initial system load if an
> +                * initial mode wasn't specified.
> +                */
> +               drms_uA_update(rdev);
>         }

Yes, this patch corrects the issue for me. You can add my tags if you
end up applying it:

Reported-by: Brian Masney <masneyb@onstation.org>
Tested-by: Brian Masney <masneyb@onstation.org>

Feel free to send me any other patches if you'd like me to do
additional testing.

Thanks for the quick response!

Brian

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Mark Brown]

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

On Sun, Nov 25, 2018 at 06:24:50PM -0500, Brian Masney wrote:
> On Sun, Nov 25, 2018 at 09:20:02AM -0800, Doug Anderson wrote:

> > I guess this is a workaround for drivers that don't set the load
> > properly themselves?

> I'm honestly not sure when the load should be set in the driver or in
> device tree. None of the drivers in drivers/mmc/ call
> regulator_set_load. The dt bindings describes the regulator-system-load
> property in Documentation/devicetree/bindings/regulator/regulator.txt.

Very few vendors disclose power numbers, ideally the drivers would be
providing power information especially if the load caused by the device
varies substantially at runtime (as is normal if things can go idle).

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

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Doug Anderson]

Hi,

On Sun, Nov 25, 2018 at 3:24 PM Brian Masney <masneyb@onstation.org> wrote:
>
> > I guess this is a workaround for drivers that don't set the load
> > properly themselves?
>
> I'm honestly not sure when the load should be set in the driver or in
> device tree. None of the drivers in drivers/mmc/ call
> regulator_set_load. The dt bindings describes the regulator-system-load
> property in Documentation/devicetree/bindings/regulator/regulator.txt.

My initial thought is that I'd expect that the load should be
associated with the consumer, not with the regulator.  The way you
have it specified in the device tree it's associated with the
regulator.  The distinction would only matter if:

A) there was another consumer of this rail

B) that other consumer could actually make due with a lower power mode
of the regulator.

C) we knew for sure that the SD card wouldn't draw (much) power from
this rail when the SD driver told it to be off.

In your system A) isn't true so thus it doesn't matter.  Even if A)
and B) were true though, I'm not sure I'd trust random SD cards
plugged into the system to not draw power.  Given that a random SD
card might draw power from the rail even if the driver wants the
regulator off then perhaps your "system-load" solution is the most
correct thing after all.

NOTE: another option would be to change the regulator driver to just
force this rail to a high power mode and never let it change.  That's
what we're doing on a SDM845-based board.  When the regulator is off
the mode doesn't matter and as per the above argument we always want
it in high power mode when it's on.


> I see that there are 8 users of regulator-system-load but most are all
> addressing this same issue with the SD card.
> qcom-msm8974-sony-xperia-castor.dts sets the load to 500 mA but all of
> the other msm8974-based SOCs use 200 mA. I'm not sure if this is
> correct.

Interestingly enough I think the max load here is specified by the SD
card specification.  My quick reading of the SD spec shows that you
could do all sorts of complex negotiation with the card about how much
load it could take up but Linux didn't actually support that.  If I'm
reading it right the default is 200 mA.

It's unlikely that really matters though.  I doubt your regulator has
a different mode for 200 mA vs. 500 mA.


> > +++ b/drivers/regulator/core.c
> > @@ -1344,6 +1344,12 @@ static int set_machine_constraints(struct
> > regulator_dev *rdev,
> >                         rdev_err(rdev, "failed to set initial mode: %d\n", ret);
> >                         return ret;
> >                 }
> > +       } else if (rdev->constraints->system_load) {
> > +               /*
> > +                * We'll only apply the initial system load if an
> > +                * initial mode wasn't specified.
> > +                */
> > +               drms_uA_update(rdev);
> >         }
>
> Yes, this patch corrects the issue for me. You can add my tags if you
> end up applying it:
>
> Reported-by: Brian Masney <masneyb@onstation.org>
> Tested-by: Brian Masney <masneyb@onstation.org>

Sent.  See <http://lkml.kernel.org/r/20181126170827.160567-1-dianders@chromium.org>.
Looks like Mark already applied it.  Thanks, Mark!


> Thanks for the quick response!

Thanks for the quick report and testing.  I'm very happy that it was
something as simple as this and not some complex interaction.

-Doug

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Mark Brown]

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

On Mon, Nov 26, 2018 at 09:43:42AM -0800, Doug Anderson wrote:

> NOTE: another option would be to change the regulator driver to just
> force this rail to a high power mode and never let it change.  That's
> what we're doing on a SDM845-based board.  When the regulator is off
> the mode doesn't matter and as per the above argument we always want
> it in high power mode when it's on.

You should never need to modify the driver for this, the regulator
framework will only change things if it's been given permission to do
so - simply don't specify a regulator-allowed-modes property and the
mode will be left alone (we should probably still use -initial-mode if
it's specified but I'd need to check if we actually do).

> > I see that there are 8 users of regulator-system-load but most are all
> > addressing this same issue with the SD card.
> > qcom-msm8974-sony-xperia-castor.dts sets the load to 500 mA but all of
> > the other msm8974-based SOCs use 200 mA. I'm not sure if this is
> > correct.

> Interestingly enough I think the max load here is specified by the SD
> card specification.  My quick reading of the SD spec shows that you
> could do all sorts of complex negotiation with the card about how much
> load it could take up but Linux didn't actually support that.  If I'm
> reading it right the default is 200 mA.

I'd like to see how closely hardware adheres to the spec too...

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

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Doug Anderson]

Hi,

On Mon, Nov 26, 2018 at 9:59 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 26, 2018 at 09:43:42AM -0800, Doug Anderson wrote:
>
> > NOTE: another option would be to change the regulator driver to just
> > force this rail to a high power mode and never let it change.  That's
> > what we're doing on a SDM845-based board.  When the regulator is off
> > the mode doesn't matter and as per the above argument we always want
> > it in high power mode when it's on.
>
> You should never need to modify the driver for this, the regulator
> framework will only change things if it's been given permission to do
> so - simply don't specify a regulator-allowed-modes property and the
> mode will be left alone (we should probably still use -initial-mode if
> it's specified but I'd need to check if we actually do).

Ah, right, there's nothing actually in rpmh and it's all in the core.
So it's just removing 'regulator-allowed-modes' and adding a
'regulator-initial-mode' property.

...hrm, but it looks like this won't just magically apply to the older
RPM driver.  For one it looks like the conversion from load to mode
happens outside of Linux (on the AOP I guess?).  rpm_reg_set_load()
just takes the load you pass it and sends it on.  It looks like you
can still force the mode but it looks like it's using a custom
attribute for that (qcom,force-mode).  I wonder if that should be
changed to use 'regulator-initial-mode'...

-Doug

Re: Question about "regulator: core: Only count load for enabled consumers" in -next

[Mark Brown]

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

On Mon, Nov 26, 2018 at 10:11:48AM -0800, Doug Anderson wrote:

> just takes the load you pass it and sends it on.  It looks like you
> can still force the mode but it looks like it's using a custom
> attribute for that (qcom,force-mode).  I wonder if that should be
> changed to use 'regulator-initial-mode'...

Yes (with compatibility for old DTs of course).

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