Hello Mark,

On 05/31/2018 04:48 AM, Mark Brown wrote:

On Wed, May 30, 2018 at 04:39:10PM -0700, David Collins wrote:

quoted

The DRMS modes to use and max allowed current per mode need to be
specified at the board level in device tree instead of hard-coded per
regulator type in the driver.  There are at least two use cases driving
this need: LDOs shared between RPMh client processors and SMPSes requiring
PWM mode in certain circumstances.

Is there really no way to improve the RPM firmware?

This aggregation takes place in a discrete hardware block, not in a
general purpose processor.  Theoretically, future chips could have
redesigned VRM hardware; however, there is no plan to make such a change.

quoted

Consider the case of a regulator with physical 10 mA LPM max current. Say
that modem and application processors each have a load on the regulator
that draws 9 mA. If they each respect the 10 mA limit, then they'd each
vote for LPM. The VRM block in RPMh hardware will aggregate these requests

This is, of course, why the regulator API aggregates this stuff based on
the current not based on having every individual user select a mode.

quoted

together using a max function which will result in the regulator being set
to LPM, even though the total load is 18 mA (which would require high
power mode (HPM)). To get around this corner case, a LPM max current of 1
uA can be used for all LDO supplies that have non-application processor
consumers. Thus, any non-zero regulator_set_load() current request will
result in setting the regulator to HPM (which is always safe).

That's obviously just never going to work well, the best you can do here
is just pretend that the other components are always operating at full
power (which I assume all the other components are doing too...) or not
try to use load based mode switching in the first place.

I will remove the DT-based mode and max load current mapping support.  In
its place, I'll implement hard coded LPM current limits for LDOs of 10 mA
or 30 mA (depending upon subtype) like is done in other regulator drivers.

If we actually encounter an issue caused by the APPS processor and another
RPMh client both voting for LPM when HPM is needed for the combination,
then we can disable load-based mode control for the affected regulator in
DT and configure its initial mode as HPM.

quoted

The second situation that needs board-level DRMS mode and current limit
specification is SMPS regulator AUTO mode to PWM (HPM) mode switching.
SMPS regulators should theoretically always be able to operate in AUTO
mode as it switches automatically between PWM mode (which can provide the
maximum current) and PFM mode (which supports lower current but has higher
efficiency). However, there may be board/system issues that require
switching to PWM mode for certain use cases as it has better load
regulation (i.e. no PFM ripple for lower loads) and supports more
aggressive load current steps (i.e. greater A/ns).

quoted

If a Linux consumer requires the ability to force a given SMPS regulator
from AUTO mode into PWM mode and that SMPS is shared by other Linux
consumers (which may be the case, but at least must be guaranteed to work
architecturally), then regulator_set_load() is the only option since it
provides aggregation, where as regulator_set_mode() does not.

That's obviously broken though, what you're describing is just clearly
nothing to do with load so trying to configure it using load is just
going to lead to problems later on.  Honestly it sounds like you just
want to force the regulator into forced PWM mode all the time, otherwise
you need to look into implementing support for describing the thing
you're actually trying to do and add a mechanism for per consumer mode
configuration.

This has been a frequent pattern with these RPM drivers, trying to find
some way to shoehorn something that happens to work right now into the
code.  That's going to make things fragile and hard to maintain, we need
code that does the thing it's saying it does so that it's easier to
understand and work with - getting things running isn't enough, they
need to be clear.

I agree that using regulator_set_load() to handle SMPS AUTO mode to PWM
mode switching is hacky.  Since there is no natural way to pick SMPS modes
based on load current, I will remove the functionality completely.  In its
place, we can configure the SMPSes to have an initial mode of AUTO.  If a
use case requiring PWM mode arises, then the consumer driver responsible
for it can call regulator_set_mode() to switch between AUTO and PWM modes
explicitly.

Since regulator_set_mode() does not support aggregation currently, this
technique would work only if there is exactly one consumer per regulator
that needs explicit mode control.  Should we hit a situation that needs
multiple consumers to have such mode control, then we could simply default
the SMPS to PWM mode.

I'll also start looking into per-consumer mode configuration and
regulator_set_mode() aggregation within the regulator framework.  I think
that we should be able to function without it for now; however, it would
be good to have going forward.

Take care,
David

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