LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: David Collins <collinsd@codeaurora.org>
Cc: Doug Anderson <dianders@chromium.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-arm-msm@vger.kernel.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	devicetree@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
Date: Thu, 31 May 2018 12:48:16 +0100	[thread overview]
Message-ID: <20180531114816.GC13319@sirena.org.uk> (raw)
In-Reply-To: <75088820-f20d-32ac-780a-5e7dacdf20ff@codeaurora.org>

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

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

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

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

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

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

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

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

  parent reply	other threads:[~2018-05-31 11:48 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-23  2:43 [PATCH v4 0/2] regulator: add QCOM RPMh regulator driver David Collins
2018-05-23  2:43 ` [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings David Collins
2018-05-23 16:22   ` Rob Herring
2018-05-30  5:23   ` Doug Anderson
2018-05-30 10:37     ` Mark Brown
2018-05-30 14:54       ` Doug Anderson
2018-05-30 15:50         ` Mark Brown
2018-05-30 16:12           ` Doug Anderson
2018-05-30 16:20             ` Mark Brown
2018-05-30 16:24               ` Doug Anderson
2018-05-30 23:39                 ` David Collins
2018-05-31  0:34                   ` Doug Anderson
2018-05-31  1:03                     ` David Collins
2018-05-31 11:48                   ` Mark Brown [this message]
2018-06-01 21:41                     ` David Collins
2018-05-23  2:43 ` [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver David Collins
2018-05-30  5:32   ` Doug Anderson
2018-05-30 23:58     ` David Collins
2018-05-30 16:33 ` [PATCH v4 0/2] " Mark Brown
2018-05-31  0:11   ` David Collins
2018-05-31 10:38     ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180531114816.GC13319@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=collinsd@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=rnayak@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).