Message-ID: <Yt/aLGJqIRxam1G9@sirena.org.uk>
Date:   Tue, 26 Jul 2022 13:12:28 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Johan Hovold <johan@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] regulator: core: Allow for a base-load per client to
 come from dts

On Mon, Jul 25, 2022 at 05:31:34PM -0700, Doug Anderson wrote:
> On Sat, Jul 23, 2022 at 12:11 PM Mark Brown <broonie@...nel.org> wrote:

> I guess the answer is that if it's a micro-optimization then we should
> be ripping more of this code out. ;-) That would go contrary to
> Dmitry's request that all regulators have a load set on them...

I've not seen this request.

> > To the extent this is needed it does smell a bit like "these
> > regulators should default to setting their load to 0 when
> > disabled" or something more along those lines TBH, some of your
> > previous comments suggested that the per device loads being
> > specified in a lot of the driver are just random values intended
> > to trigger a specific behaviour in the regulator but forced to be
> > done in terms of a load due to the firmware inteface that's
> > available on these platforms having an interface in those terms.

> It's actually _not_ the firmware interface as far as I can tell, at
> least for newer Qualcomm chips (those using RPMH). The firmware
> interface seems to be for modes. See, for instance,
> rpmh_regulator_vrm_set_load() which translates loads into modes before
> passing to the firmware. Ironically, my memory tells me that Qualcomm
> actually said that this turned out to be a problem in the past for
> them, though, since some rails went to both the main apps processor
> (AP) and the modem processor. Each could independently decide that low
> power mode was fine but the total of both usages could bump you into
> needing high power mode...

Oh, that's just completely broken.  The set_load() support should be
removed and the drivers should be implementing get_optimum_mode(), it's
actually got an open coded version of it in there already.  That'll only
have snuck past due to being hidden behind a layer of abstraction that
shouldn't be there.

> consumers") but is still instructive of Qualcomm's thinking. Taking a
> sampling of the loads in the tables in the DSI driver / phy, I see:
> * Many specify 100 uA.
> * Some seem to pick based on throwing a dart at a dartboard. 16 uA, 2
> uA, 4 uA, 32 uA, etc.

The load for a PHY is going to be immaterial when you're driving a
panel.

> If we happen to be using an LDO that changes over at 30 mA, though,
> these ones _could_ use LDO. I guess this is where the whole
> "specifying in uA" makes sense? If you've got a regulator that changes
> at 30mA and only one ~20mA consumer is active then it can stay in LPM.
> When two ~20mA consumers are active then it needs to switch to HPM?
> Having lots of consumers on a given rail is really common w/ Qulaocmm
> setups. On trogdor, rail "L4A" is all of these:

We specify in uA and uV partly to ensure we never run into the bottom
limit of resolution and partly because for loads uA is a very relevant
number, as you've noticed it's where things tend to transition into
their lowest power modes and it's also commonly a relevant unit for the
draw from devices in idle modes.  Something like an audio CODEC doing
jack detection will probably be able to get into a low power mode for
example.

> I guess the above doesn't really give us a lot of good answers. :(

All the load setting that doesn't get passed to the firmware in the
regulator drivers should be removed and replaced with get_optimum_mode()
operations, this has no impact on the client drivers and removes some
open coding from the regulator drivers.  Load management in client
drivers that doesn't go below say 100uA is probably noise but it's also
fairly harmless.

> Perhaps the low hanging fruit is to just accept that the current API
> of setting the load is here to stay, even if it does seem mostly
> pointless in many cases. I can submit a patch that adds the load to
> the "bulk" API and at least it would clean up a bunch of stuff even if
> it doesn't fundamentally overhaul the system...

Adding loads to the bulk API would also be useful.

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

Powered by blists - more mailing lists