[PATCH 0/4] regulator: core: A few useful patches for regulators that need load set

[PATCH 0/4] regulator: core: A few useful patches for regulators that need load set

[Douglas Anderson]

These patches ought to be useful for those people who need to work
with regulator drivers where regulator_set_load() is important.  The
first patch in this series actually implements real functionality.
After that patch then devices whose drivers don't know to call
regulator_set_load() are more likely to work.  The rest of the patches
just work on regulator_summary to try to make it so we can confirm
that the first patch works.


Douglas Anderson (4):
  regulator: core: If consumers don't call regulator_set_load() assume
    max
  regulator: core: Add the opmode to regulator_summary
  regulator: core: Add consumer-requested load in regulator_summary
  regulator: core: Add locking to debugfs regulator_summary

 drivers/regulator/core.c     | 98 ++++++++++++++++++++++++------------
 drivers/regulator/internal.h |  1 +
 2 files changed, 66 insertions(+), 33 deletions(-)

-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Douglas Anderson]

Not all regulator consumers call regulator_set_load().  On some
regulators (like on RPMh-regulator) this could be bad since the
regulator framework will treat this as if consumer needs no load.
It's much better to assume that a dumb client needs the maximum
possible load so we get correctness first.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c     | 10 +++++++++-
 drivers/regulator/internal.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ed568b96c0e..a4da68775b49 100644
--- 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;
+
 	if (rdev->desc->ops->set_load) {
 		/* set the optimum mode for our new total regulator load */
 		err = rdev->desc->ops->set_load(rdev, current_uA);
@@ -3631,6 +3638,7 @@ int regulator_set_load(struct regulator *regulator, int uA_load)
 
 	regulator_lock(rdev);
 	regulator->uA_load = uA_load;
+	regulator->uA_load_set = true;
 	ret = drms_uA_update(rdev);
 	regulator_unlock(rdev);
 
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 943926a156f2..f05c75c59ef4 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -41,6 +41,7 @@ struct regulator {
 	struct list_head list;
 	unsigned int always_on:1;
 	unsigned int bypass:1;
+	bool uA_load_set;
 	int uA_load;
 	struct regulator_voltage voltage[REGULATOR_STATES_NUM];
 	const char *supply_name;
-- 
2.18.0.865.gffc8e1a3cd6-goog

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[David Collins]

Hello Doug,

On 08/14/2018 10:06 AM, Douglas Anderson wrote:
> Not all regulator consumers call regulator_set_load().  On some
> regulators (like on RPMh-regulator) this could be bad since the
> regulator framework will treat this as if consumer needs no load.
> It's much better to assume that a dumb client needs the maximum
> possible load so we get correctness first.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

The behavior introduced by this patch seems like an undesirable hack to
me.  It goes against the general philosophy within the regulator framework
of taking no action unless directed to do so by an explicit consumer
request (or special device tree property).  We should assume that
consumers make requests to meet their needs instead of assuming that they
are missing important votes required by their hardware.


>  drivers/regulator/core.c     | 10 +++++++++-
>  drivers/regulator/internal.h |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 6ed568b96c0e..a4da68775b49 100644
> --- 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.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Doug Anderson]

Hi,

On Tue, Aug 14, 2018 at 11:30 AM, David Collins <collinsd@codeaurora.org> wrote:
> The behavior introduced by this patch seems like an undesirable hack to
> me.  It goes against the general philosophy within the regulator framework
> of taking no action unless directed to do so by an explicit consumer
> request (or special device tree property).  We should assume that
> consumers make requests to meet their needs instead of assuming that they
> are missing important votes required by their hardware.

I don't agree so I guess it will be up to Mark to decide.

Specifically I will note that there are boatloads of drivers out there
that use the regulator framework but don't have a call to
regulator_set_load() in them.  Are these drivers all broken?  I don't
think so.  IMO the regulator_set_load() API is an optional call for
drivers that they can use to optimize power usage, not a required API.


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


-Doug

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[David Collins]

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?

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.


> 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

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Doug Anderson]

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

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[David Collins]

On 08/14/2018 04:56 PM, Doug Anderson wrote:
> On Tue, Aug 14, 2018 at 2:59 PM, David Collins <collinsd@codeaurora.org> wrote:
>> 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?

I agree that the existing behavior resulting from a lack of load current
propagation from child to parent regulators is wrong.  However, I do not
agree that replacing it with something else that results in different
wrong behavior is the way forward.

Adding load current propagation would resolve your 0 mA concern but does
not necessary require making MAX_INT the default for all consumers.


>>> 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.
>>
>> [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?

Propagating regulator framework modes (Standby, Idle, Normal, Fast) from
child to parent regulators is definitely not feasible in the general case.
 The meaning of these modes varies between different types of regulators
as do load current thresholds between modes.  I.e. just because a child
regulator changed from Idle to Normal mode does not imply that the parent
regulator needs to operate in Normal mode now.

As an example, consider the QTI PM8998 FTSMPS and NLDO regulators.  The
FTSMPS regulators can supply up to 800 mA in PFM mode (Idle) and 4000 mA
in PWM mode (Fast).  The child NLDO type regulators supplied by the
FTSMPSes can source up to 30 mA in LPM (Idle) and 300 - 1200 mA in HPM
(Normal) (depending upon subtype).  Clearly, an NLDO switching from Idle
to Normal mode because 50 mA was requested does not mean that its parent
needs to switch to Fast mode.

Also note that in this case, the FTSMPS regulators have AUTO mode in
hardware what maps to Linux mode Normal.  In practice, the FTSMPSes will
be configured to be in AUTO mode all the time unless there is a special
case that requires PWM mode (e.g. hardware that is particularly
susceptible to PFM voltage ripple).  They will completely ignore load
current aggregation.

I agree that calculating the load current required from a parent based
upon the current output by a child regulator is challenging.  However,
microamps are physically meaningful and consistent across different types
of regulators.  Therefore, it is technically possible to propagate load
currents from child regulators up to parents.  The same is not true of
regulator framework modes.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Mark Brown]

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

On Tue, Aug 14, 2018 at 04:56:42PM -0700, Doug Anderson wrote:

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

That's not going to work well - different regulators have wildly
different abilities to deliver current which is the whole reason why
modes are so fuzzy and hard to use in the first place.  A high power
load for a low noise regulator designed to feed analogue circuits might
not even make it out of the lowest power LDO mode of a DCDC designed to
supply the main application processors in the system or (more
relevantly) provide the main step down for a bunch of LDOs.

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

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Doug Anderson]

Hi,

On Wed, Aug 15, 2018 at 4:13 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Aug 14, 2018 at 04:56:42PM -0700, Doug Anderson wrote:
>
>> 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?
>
> That's not going to work well - different regulators have wildly
> different abilities to deliver current which is the whole reason why
> modes are so fuzzy and hard to use in the first place.  A high power
> load for a low noise regulator designed to feed analogue circuits might
> not even make it out of the lowest power LDO mode of a DCDC designed to
> supply the main application processors in the system or (more
> relevantly) provide the main step down for a bunch of LDOs.

OK, fair enough.  I'll drop this patch and rebase the later patches in
the series without it since I think they're still useful.

I'll work on either adding more regulator_set_load() calls to clients
or perhaps disabling the "regulator-allow-set-load" for a bunch of
rails.  David: presumably if we have a rail that we never need to be
on-and-in-low-power-mode can just be left in high power mode all the
time?  There should be no advantage of being in low power mode for a
regulator that is off, right?


-Doug

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[David Collins]

Hello Doug,

On 08/16/2018 01:07 PM, Doug Anderson wrote:
> I'll work on either adding more regulator_set_load() calls to clients
> or perhaps disabling the "regulator-allow-set-load" for a bunch of
> rails.  David: presumably if we have a rail that we never need to be
> on-and-in-low-power-mode can just be left in high power mode all the
> time?  There should be no advantage of being in low power mode for a
> regulator that is off, right?

Generally speaking, yes, that is true on both points.  The only caveat is
that there could be a minor power penalty if APPS votes for OFF+HPM and a
non-HLOS processor votes for ON+LPM for the same regulator.  This would
lead to an aggregated state of ON+HPM when only ON+LPM is really needed.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Doug Anderson]

Hi,

On Thu, Aug 16, 2018 at 1:58 PM, David Collins <collinsd@codeaurora.org> wrote:
> Hello Doug,
>
> On 08/16/2018 01:07 PM, Doug Anderson wrote:
>> I'll work on either adding more regulator_set_load() calls to clients
>> or perhaps disabling the "regulator-allow-set-load" for a bunch of
>> rails.  David: presumably if we have a rail that we never need to be
>> on-and-in-low-power-mode can just be left in high power mode all the
>> time?  There should be no advantage of being in low power mode for a
>> regulator that is off, right?
>
> Generally speaking, yes, that is true on both points.  The only caveat is
> that there could be a minor power penalty if APPS votes for OFF+HPM and a
> non-HLOS processor votes for ON+LPM for the same regulator.  This would
> lead to an aggregated state of ON+HPM when only ON+LPM is really needed.

OK, thanks for the confirmation.  ...so if we know that this is a rail
that the non HLOS has no business dealing with then this would be a
nice simplification so we don't need to go add code to all drivers
everywhere when all they want is a simple regulator that turns on and
off.

Presumably we could also add code somewhere in Linux that would
automatically vote for LPM for a regulator that has been disabled if
we had to.

-Doug

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Mark Brown]

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

On Tue, Aug 14, 2018 at 01:03:07PM -0700, Doug Anderson wrote:

> Specifically I will note that there are boatloads of drivers out there
> that use the regulator framework but don't have a call to
> regulator_set_load() in them.  Are these drivers all broken?  I don't
> think so.  IMO the regulator_set_load() API is an optional call for
> drivers that they can use to optimize power usage, not a required API.

Very few systems dynamically change modes in the first place, if we were
doing this as a matter of course you could claim the drivers were buggy
but really it's unusual for it to even be a useful thing to do - as it
is it's more an accomodation for a small subset of systems.  This does
mean that those systems have to pay attention to making sure that
everything works well which is unfortunate but it's not unreasonable.

Forcing the mode to the highest available would be a step backwards for
most systems.

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

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Mark Brown]

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

On Tue, Aug 14, 2018 at 10:06:14AM -0700, Douglas Anderson wrote:
> Not all regulator consumers call regulator_set_load().  On some
> regulators (like on RPMh-regulator) this could be bad since the
> regulator framework will treat this as if consumer needs no load.
> It's much better to assume that a dumb client needs the maximum
> possible load so we get correctness first.

If you take this to its logical conclusion we should never have runtime
mode setting - there may be passive components in the system which don't
appear in DT but do affect the load.  It's why we require the machines
to explicitlly enable any changes that the framework does, if we don't
do this then things aren't safe.  You may also have a situation where
some of the drivers that are registered are included in a fixed load
specified by the machine integration.

Dynamic mode setting is just a very hard feature to use usefully,
there's a reason why few systems do it.

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

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Bjorn Andersson]

On Tue 14 Aug 10:06 PDT 2018, Douglas Anderson wrote:

> Not all regulator consumers call regulator_set_load().  On some
> regulators (like on RPMh-regulator) this could be bad since the
> regulator framework will treat this as if consumer needs no load.
> It's much better to assume that a dumb client needs the maximum
> possible load so we get correctness first.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

FWIW, we've had this problem on other devices as well; where the eMMC
won't operate properly unless the supply operates in HPM. We've worked
around this by specifying regulator-system-load for said regulators.

Only drawback with that is that we use DT to work around missing
features in the code. But at least the improved implementation will be
backwards compatible with existing DT.

Regards,
Bjorn

Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

[Mark Brown]

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

On Fri, Aug 17, 2018 at 02:36:01PM -0700, Bjorn Andersson wrote:

> FWIW, we've had this problem on other devices as well; where the eMMC
> won't operate properly unless the supply operates in HPM. We've worked
> around this by specifying regulator-system-load for said regulators.

You can set the regulator to work in a single mode all the time, that's
not a problem.  If you know the system needs to be in a specific mode
it's easy to configure that (some might require a mode below the highest
power one since that's sometimes the noisiest).

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

[PATCH 2/4] regulator: core: Add the opmode to regulator_summary

[Douglas Anderson]

It's handy to know what opmode a regulator has been configured to in
the summary.  Add it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a4da68775b49..b8b8048efbdd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -426,19 +426,24 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(name);
 
-static ssize_t regulator_print_opmode(char *buf, int mode)
+static const char *regulator_opmode_to_str(int mode)
 {
 	switch (mode) {
 	case REGULATOR_MODE_FAST:
-		return sprintf(buf, "fast\n");
+		return "fast";
 	case REGULATOR_MODE_NORMAL:
-		return sprintf(buf, "normal\n");
+		return "normal";
 	case REGULATOR_MODE_IDLE:
-		return sprintf(buf, "idle\n");
+		return "idle";
 	case REGULATOR_MODE_STANDBY:
-		return sprintf(buf, "standby\n");
+		return "standby";
 	}
-	return sprintf(buf, "unknown\n");
+	return "unknown";
+}
+
+static ssize_t regulator_print_opmode(char *buf, int mode)
+{
+	return sprintf(buf, "%s\n", regulator_opmode_to_str(mode));
 }
 
 static ssize_t regulator_opmode_show(struct device *dev,
@@ -4668,10 +4673,11 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 	if (!rdev)
 		return;
 
-	seq_printf(s, "%*s%-*s %3d %4d %6d ",
+	seq_printf(s, "%*s%-*s %3d %4d %6d %7s ",
 		   level * 3 + 1, "",
 		   30 - level * 3, rdev_get_name(rdev),
-		   rdev->use_count, rdev->open_count, rdev->bypass_count);
+		   rdev->use_count, rdev->open_count, rdev->bypass_count,
+		   regulator_opmode_to_str(_regulator_get_mode(rdev)));
 
 	seq_printf(s, "%5dmV ", _regulator_get_voltage(rdev) / 1000);
 	seq_printf(s, "%5dmA ", _regulator_get_current_limit(rdev) / 1000);
@@ -4703,7 +4709,7 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 
 		switch (rdev->desc->type) {
 		case REGULATOR_VOLTAGE:
-			seq_printf(s, "%37dmV %5dmV",
+			seq_printf(s, "%45dmV %5dmV",
 				   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
 				   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
 			break;
@@ -4735,8 +4741,8 @@ static int regulator_summary_show_roots(struct device *dev, void *data)
 
 static int regulator_summary_show(struct seq_file *s, void *data)
 {
-	seq_puts(s, " regulator                      use open bypass voltage current     min     max\n");
-	seq_puts(s, "-------------------------------------------------------------------------------\n");
+	seq_puts(s, " regulator                      use open bypass  opmode voltage current     min     max\n");
+	seq_puts(s, "---------------------------------------------------------------------------------------\n");
 
 	class_for_each_device(&regulator_class, NULL, s,
 			      regulator_summary_show_roots);
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 3/4] regulator: core: Add consumer-requested load in regulator_summary

[Douglas Anderson]

It's handy to see the load requested by a regulator consumer in the
regulator_summary.  Add it.  Since this patch comes after the patch
("regulator: core: If consumers don't call regulator_set_load() assume
max") we'll also differentiate between the case where a consumer
hasn't called regulator_set_load() and the case where a consumer
called it but the load is currently 0 mA.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b8b8048efbdd..23f64faa6961 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4709,9 +4709,16 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 
 		switch (rdev->desc->type) {
 		case REGULATOR_VOLTAGE:
-			seq_printf(s, "%45dmV %5dmV",
-				   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
-				   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
+			if (consumer->uA_load_set) {
+				seq_printf(s, "%37dmA %5dmV %5dmV",
+					   consumer->uA_load / 1000,
+					   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
+					   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
+			} else {
+				seq_printf(s, "%45dmV %5dmV",
+					   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
+					   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
+			}
 			break;
 		case REGULATOR_CURRENT:
 			break;
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 4/4] regulator: core: Add locking to debugfs regulator_summary

[Douglas Anderson]

Most functions that access the rdev lock the rdev mutex before looking
at data.  ...but not the code that implements the debugfs
regulator_summary.  It probably should though, so let's do it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 51 ++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 23f64faa6961..5950b44c10d5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3468,21 +3468,23 @@ int regulator_set_current_limit(struct regulator *regulator,
 }
 EXPORT_SYMBOL_GPL(regulator_set_current_limit);
 
+static int _regulator_get_current_limit_unlocked(struct regulator_dev *rdev)
+{
+	/* sanity check */
+	if (!rdev->desc->ops->get_current_limit)
+		return -EINVAL;
+
+	return rdev->desc->ops->get_current_limit(rdev);
+}
+
 static int _regulator_get_current_limit(struct regulator_dev *rdev)
 {
 	int ret;
 
 	regulator_lock(rdev);
-
-	/* sanity check */
-	if (!rdev->desc->ops->get_current_limit) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	ret = rdev->desc->ops->get_current_limit(rdev);
-out:
+	ret = _regulator_get_current_limit_unlocked(rdev);
 	regulator_unlock(rdev);
+
 	return ret;
 }
 
@@ -3547,21 +3549,23 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 }
 EXPORT_SYMBOL_GPL(regulator_set_mode);
 
+static unsigned int _regulator_get_mode_unlocked(struct regulator_dev *rdev)
+{
+	/* sanity check */
+	if (!rdev->desc->ops->get_mode)
+		return -EINVAL;
+
+	return rdev->desc->ops->get_mode(rdev);
+}
+
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev)
 {
 	int ret;
 
 	regulator_lock(rdev);
-
-	/* sanity check */
-	if (!rdev->desc->ops->get_mode) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	ret = rdev->desc->ops->get_mode(rdev);
-out:
+	ret = _regulator_get_mode_unlocked(rdev);
 	regulator_unlock(rdev);
+
 	return ret;
 }
 
@@ -4669,18 +4673,23 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 	struct regulation_constraints *c;
 	struct regulator *consumer;
 	struct summary_data summary_data;
+	unsigned int opmode;
 
 	if (!rdev)
 		return;
 
+	regulator_lock_nested(rdev, level);
+
+	opmode = _regulator_get_mode_unlocked(rdev);
 	seq_printf(s, "%*s%-*s %3d %4d %6d %7s ",
 		   level * 3 + 1, "",
 		   30 - level * 3, rdev_get_name(rdev),
 		   rdev->use_count, rdev->open_count, rdev->bypass_count,
-		   regulator_opmode_to_str(_regulator_get_mode(rdev)));
+		   regulator_opmode_to_str(opmode));
 
 	seq_printf(s, "%5dmV ", _regulator_get_voltage(rdev) / 1000);
-	seq_printf(s, "%5dmA ", _regulator_get_current_limit(rdev) / 1000);
+	seq_printf(s, "%5dmA ",
+		   _regulator_get_current_limit_unlocked(rdev) / 1000);
 
 	c = rdev->constraints;
 	if (c) {
@@ -4733,6 +4742,8 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 
 	class_for_each_device(&regulator_class, NULL, &summary_data,
 			      regulator_summary_show_children);
+
+	regulator_unlock(rdev);
 }
 
 static int regulator_summary_show_roots(struct device *dev, void *data)
-- 
2.18.0.865.gffc8e1a3cd6-goog