Date:   Fri, 22 Jul 2022 19:22:18 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Johan Hovold <johan@...nel.org>, devicetree@...r.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] regulator: core: Allow for a base-load per client to
 come from dts

On Thu, Jul 21, 2022 at 06:26:44PM -0700, Douglas Anderson wrote:

> Looking through the current users of regulator_set_load(), all but one
> or two basically follow the pattern:

> 1. Get all the regulators.
> 2. Go through each regulator and call regulator_set_load() on it with
>    some value that's sufficient to get the regulator to run in "HPM"
>    mode.
> 3. Enable / disable the regulator as needed.

Sure...  I guess these devices are just hoping that their idle currents
are near enough to zero to be safe.  Having refreshed my mind about how
all this works that's basically the assumption that Linux is making
here, that any idle currents will be low enough to be satisified by the
lowest power state of any practical regulator and we don't need to keep
track of them.

> Specifically it should be noted that many devices don't really have a
> need for the low power mode of regulators. Unfortunately the current

What exactly do you mean by "a need for the low power mode of
regulators"?

> state of the world means that all drivers are cluttered with tables of
> loads and extra cruft at init time to tell the regulator framework
> about said loads.

We're only talking about a few lines of code here, and we could make a
bulk helper to set loads if this is a serious issue.  You could even
just add some more values in the bulk struct and have it be pure data
for the drivers, regulator_bulk_get() could see a load and set it.  I
think this may actually be the bulk of what's concerning you here?

> There are lots of ways we could combine this "base load" with a load
> that the driver requests. We could:
> - Add them.
> - Take the maximum (assume that the "base" is the min).
> - Only use the "base" load if the driver didn't request one.

This feels a bit mangled, I think largely because you're using the term
"base load" to refer to something which I *think* you want to be the
peak load the device might cause when it's actively doing stuff which is
just very confusing.  If this were being specified per device I'd expect
that to be the idle rather than active current with a name like that.
That's just naming, but it does suggest that if we were to put this in
DT it should be more like the pinctrl equivalents and done as an array
of currents with matching array of names.

> I have chosen the third option here. If the driver knows better then
> it can override. Note that the driver can't override to "0". To do
> that it would just disable the regulator.

I don't like the idea of having platform constraints which we ignore,
this goes against the general safety principal of the regulator API
which is that we won't touch the device state unless given explicit
permission to do so by the platform description.  The general idea is
to ensure that we can't do anything that might damage the platform
without approval from the system integrator.

> NOTE: if we want to keep old binary dtb files working with new kernels
> then we'd still have to keep existing crufty regulator_set_load() in
> drivers, but hopefully we can stop adding new instances of this, and
> perhaps eventually people will agree to deprecate old binary dtb files
> and we can get rid of all the extra code.

To be perfectly honest I'm not sure I see any great advantage here.
It's moving the performance numbers from the driver into DT which makes
them ABI which makes them harder to update, and the peak load is going
to be vulnerable to changes due to driver changes - if we do some
performance work and manage to start driving a device harder the numbers
specified in the DT may become wrong.  In general it seems safer to not
put things into ABI if we don't have to, and a substantial proportion of
things that might use regulators are off-SoC devices which don't
generally want or need DT fragments to describe them so we end up having
to duplicate things in multiple DTs which isn't ideal.

What's big push to put this in DT rather than just make the code pattern
easier to use?  I'm not totally against it but I'm having a hard time
getting enthusiastic about it.

I think the underlying thing here (and the reason you're seeing this a
lot with Qualcomm device specifically) is that Qualcomm have a firmware
interface to specify loads and wanted to implement selection of a low
power mode when things are idle in the way that Linux does with the
get_optimum_mode() interface.  Either they actually have some systems
where they were able to select different modes when things are running
or they don't have the thing we have where they discount loads from
consumers that have removed their enable vote, either way for those
platforms what's going on currently does seem like a sensible way of
setting things up.

Other platforms don't bother, I think that's likely to be some
combination of not caring about anything finer grained than system
suspend (which especially for Android is totally fair) and modern
regulators being much more able to to dynamically adapt to load than
those the code was written for.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists