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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> Tested-by: Brian Masney <masneyb@xxxxxxxxxxxxx>

Sent.  See <http://lkml.kernel.org/r/20181126170827.160567-1-dianders@xxxxxxxxxxxx>.
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