Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <aa2bd1f6-74b4-a63b-d115-88d6e16ed95d@codeaurora.org>
References: <20180814170617.100087-1-dianders@chromium.org>
 <20180814170617.100087-2-dianders@chromium.org> <bf70f01d-41ac-dcdc-b503-5e0eb1e70329@codeaurora.org>
 <CAD=FV=VNCOyyq5+xvSudTmtBgPm4-s7gj7GRCvuL5vSmA6yDVA@mail.gmail.com> <aa2bd1f6-74b4-a63b-d115-88d6e16ed95d@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 14 Aug 2018 16:56:42 -0700
Message-ID: <CAD=FV=WBw2SKgPq8Atb+SkziUku8p7N4GWemHYZZvoMSE0bK9g@mail.gmail.com>
Subject: Re: [PATCH 1/4] regulator: core: If consumers don't call
 regulator_set_load() assume max
To:     David Collins <collinsd@codeaurora.org>
Cc:     Mark Brown <broonie@kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Tue, Aug 14, 2018 at 2:59 PM, David Collins <collinsd@codeaurora.org> wrote:
> Hi,
>
> On 08/14/2018 01:03 PM, Doug Anderson wrote:
>> On Tue, Aug 14, 2018 at 11:30 AM, David Collins <collinsd@codeaurora.org> wrote:>>> --- a/drivers/regulator/core.c
>>>> +++ b/drivers/regulator/core.c
>>>> @@ -732,6 +732,7 @@ static int drms_uA_update(struct regulator_dev *rdev)
>>>>       struct regulator *sibling;
>>>>       int current_uA = 0, output_uV, input_uV, err;
>>>>       unsigned int mode;
>>>> +     bool any_unset = false;
>>>>
>>>>       lockdep_assert_held_once(&rdev->mutex);
>>>>
>>>> @@ -751,11 +752,17 @@ static int drms_uA_update(struct regulator_dev *rdev)
>>>>               return -EINVAL;
>>>>
>>>>       /* calc total requested load */
>>>> -     list_for_each_entry(sibling, &rdev->consumer_list, list)
>>>> +     list_for_each_entry(sibling, &rdev->consumer_list, list) {
>>>>               current_uA += sibling->uA_load;
>>>> +             if (!sibling->uA_load_set)
>>>> +                     any_unset = true;
>>>> +     }
>>>>
>>>>       current_uA += rdev->constraints->system_load;
>>>>
>>>> +     if (any_unset)
>>>> +             current_uA = INT_MAX;
>>>> +
>>>
>>> This check will incorrectly result in a constant load request of INT_MAX
>>> for all regulators that have at least one child regulator.  This is the
>>> case because such child regulators are present in rdev->consumer_list and
>>> because regulator_set_load() requests are not propagated up to parent
>>> regulators.  Thus, the regulator structs for child regulators will always
>>> have uA_load==0 and uA_load_set==false.
>>
>> Ah, interesting.
>>
>> ...but doesn't this same bug exist anyway, just in the opposite
>> direction?  Without my patch we'll try to request a 0 mA load in this
>> case which seems just as wrong (or perhaps worse?).  I guess on RPMh
>> regulator you're "saved" because the RPMh hardware itself knows the
>> parent/child relationship and knows to ignore this 0 mA load, but it's
>> still a bug in the overall Linux framework...
>
> I'm not sure what existing bug you are referring to here.  Where is a 0 mA
> load being requested?  Could you list the consumer function calls and the
> behavior on the framework side that you're envisioning?

Imagine you've got a tree like this:

- regulatorParent (1.8 V)
  - regulatorChildA (1.7 V)
  - regulatorChildB (1.2 V)
  - regulatorChildC (1.5 V)

...and this set of calls:

regulator_set_load(regulatorChildA, 1000);
regulator_set_load(regulatorChildB, 2000);
regulator_set_load(regulatorChildC, 3000);

regulator_enable(regulatorChildA);
regulator_enable(regulatorChildB);
regulator_enable(regulatorChildC);


With the existing code in Mark's tree then ChildA / ChildB / ChildC
will presumably have a load high enough to be in high power mode.
However, as you said, loads aren't propagated.  ...so "Parent" will
see no load requested (which translates to a load request of 0 mA).

The "bug" is that when you did the
"regulator_enable(regulatorChildA);" then that will propagate up and
cause a "regulator_enable(regulatorParent)".  In _regulator_enable()
you can see drms_uA_update().  That will cause it to set the mode of
regulatorParent based on a load of 0 mA.  That is the bug.  You may
respond "but REGULATOR_CHANGE_DRMS isn't set for the parent!  See
below and for now imagine that REGULATOR_CHANGE_DRMS _is_ set for the
parent.

With my code in the same situation the parent will end up with a load
of "MAX_INT" mA which I think is the bug you pointed out.  ...while it
would be good to fix this it does seem to be better to set the parent
to a mode based on MAX_INT mA rather than 0 mA?


> The RPMh hardware never sees requests with units of mA (though its
> predecessor RPM SMD did).  Instead, mode requests sent to RPMh are raw
> PMIC MODE_CTL register values.  RPMh then applies max aggregation of these
> register values across masters (APPS, modem, etc).  Also note that the
> RPMh knowledge of parent/child relationships is only utilized in enable
> control and voltage headroom management.  It does not apply to mode control.

Yeah, I know RPMh only sees modes.  I was sorta assuming that perhaps
RPMh would use its parent/child relationship knowledge to say that if
a child is in HPM then the parent should automatically go to HPM.

...but as I dug further I figured out why we're not running into this
bug (at least with the regulator setup I have in my tree).  All of the
"parent" regulators are always configured such that
"REGULATOR_CHANGE_DRMS" is not valid.  Thus we never end up calling
into drms_uA_update() and never update the mode.


So tl;dr: your original comment was that my patch had problems
whenever one regulator is the supply for another regulator if parent
has REGULATOR_CHANGE_DRMS set.  I believe I have shown that even
without my patch we have problems in this exact same case.


>> I have no idea how we ought to propagate regulator_set_load() to
>> parents though.  That seems like a tough thing to try to handle
>> automagically.
>
> I attempted it seven years ago with two revisions of a patch series [1]
> and [2].  The feature wasn't completed though.  Perhaps something along
> the same lines could be reintroduced now that child regulators are handled
> the same way as ordinary consumers within the regulator framework.
>
> Thanks,
> David
>
> [1]: https://lkml.org/lkml/2011/3/28/246
> [2]: https://lkml.org/lkml/2011/3/28/530

Thanks for the links.  My first instinct is just what Mark said there:
you can't really take the child load and map it accurately to a parent
load without loads of per-device complexity and even then you'd
probably get nothing more than an approximation.

IMO about the best we could hope to do would be to map "mode" from
children to parent.  AKA: perhaps you could assume that if a child is
in a higher power mode that perhaps a parent should be too?


-Doug