| Message ID | 20180814170617.100087-2-dianders@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | regulator: core: A few useful patches for regulators that need load set | expand |
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
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
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
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
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
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.
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.
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.
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
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
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
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
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).
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;
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(-)