LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: David Collins <collinsd@codeaurora.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 v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
Date: Tue, 22 May 2018 17:55:37 +0100	[thread overview]
Message-ID: <20180522165537.GE24776@sirena.org.uk> (raw)
In-Reply-To: <CAD=FV=XARtQNo5Cyg78XuT26JUFgn=7BjWRnXPjP566=a-sF1w@mail.gmail.com>

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

On Tue, May 22, 2018 at 09:43:02AM -0700, Doug Anderson wrote:
> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:

> > Returning the cached (but not sent) initial voltage equal to the min
> > constraint voltage in get_voltage() calls should not cause any problems.
> > This represents the highest voltage that is guaranteed to be output by the
> > regulator.  Consumer's should call regulator_set_voltage() to specify
> > their voltage needs.  If they simply call regulator_enable(), then the
> > cached voltage will be sent along with the enable request.

> I'm still not seeing the argument for initial-voltage here.  If we
> added a feature like you describe where we don't send the voltage
> until the first enable couldn't we use the minimum voltage here?  If a
> consumer calls regulator_enable() without setting a voltage (which
> seems like a terrible idea for something where the voltage could vary)
> then it would end up at the minimum voltage.

Or if something else has already set a voltage...  though shared voltage
varying regulators aren't a superb idea they do occasionally happen.

> >> I was asking for specific examples.  Do you have any?

> > I was able to track down an example that requires initialization at a
> > non-minimum voltage: PM8998 LDO 13 on SDM845 MTP.  This regulator supplies
> > the microSD card with a voltage in the range 1.8 V to 2.96 V.  The boot
> > loader leaves this regulator enabled at 2.96 V.  It is only guaranteed to
> > be safe to reduce the voltage to 1.8 V on UHS type cards and only after
> > following the proper SD identification and command sequence.

> Ironically, this is also a perfect example of why we _shouldn't_ have
> an "initial-voltage" specified in the device tree.  It is certainly
> plausible to imagine a bootloader that might enable UHS speeds on an
> SD card and leave the rail on at 1.8V.  Having an initial-voltage
> specified in the device tree would be a bad idea here because it's
> even worse to transition to 2.96V when the card was expecting 1.8V.

Right, this sort of situation is why the regulator API has a policy of
leaving things untouched unless it was specifically told to do
something.

> I suppose this is a theoretical example since (as far as I know) no
> bootloaders run the micro SD card at UHS speeds, but it is still

kexec is the most obvious example I can think of here.  You could
probably arrange for something to patch the device tree that's provided
to the kexeced kernel to tell it about the current state but we don't
currently do anything there.

> OK, so how's this for a proposal then:

> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
> specified that the regulator is enabled then we don't send the
> voltage, we just cache it.

> 2. When we see the first enable then we first send the cached voltage
> and then do the enable.

> 3. We don't need an "initial voltage" because any rails that are
> expected to be variable voltage the client should be choosing a
> voltage.

That seems relatively safe.

> You can even come up with a less contrived use case here.  One could
> argue that the SD card framework really ought to be ensuring VMMC and
> VQMMC are off before it starts tweaking with them just in case the
> bootloader left them on.  Thus, it should do:

> A) Turn off VMMC and VQMMC
> B) Program VMMC and VQMMC to defaults
> C) Turn on VMMC and VQMMC

> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
> off, so step A) will be no-op.  Sigh.

> Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
> regulator framework understand that we don't know the state?  I think

Yes, we should be doing that.  Then subsystems where it's an issue can
explicitly turn off the supply.

> that might require a pile of patches to the regulator framework, but
> it can't be helped unless we can somehow get RPMh to give us back the
> status of how the bootloader left us (if we had that, we could return
> 0 for anything the bootloader didn't touch and that would be correct
> from the point of view of the AP).

I think it's fine from a framework point of view, just provide an
is_enabled() operation which returns the error.  The required fiddling
in the core should be fairly minor.

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

  reply	other threads:[~2018-05-22 16:55 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-12  2:28 [PATCH v3 0/2] regulator: add QCOM RPMh regulator driver David Collins
2018-05-12  2:28 ` [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings David Collins
2018-05-17 21:22   ` Doug Anderson
2018-05-18  0:16     ` David Collins
2018-05-18  1:01       ` Doug Anderson
2018-05-19  0:46         ` David Collins
2018-05-21 18:01           ` Doug Anderson
2018-05-22  0:00             ` David Collins
2018-05-22 16:43               ` Doug Anderson
2018-05-22 16:55                 ` Mark Brown [this message]
2018-05-22 22:46                 ` David Collins
2018-05-23  0:08                   ` Doug Anderson
2018-05-23  1:19                     ` David Collins
2018-05-23  5:10                       ` Doug Anderson
2018-05-23  8:29                     ` Mark Brown
2018-05-23 15:23                       ` Doug Anderson
2018-05-23 15:40                         ` Mark Brown
2018-05-23 15:50                           ` Doug Anderson
2018-05-23 15:56                             ` Mark Brown
2018-05-30  5:30                               ` Doug Anderson
2018-05-30  9:37                                 ` Mark Brown
2018-05-30 14:46                                   ` Doug Anderson
2018-05-30 15:02                                     ` Mark Brown
2018-05-30 15:34                                       ` Doug Anderson
2018-05-30 15:48                                         ` Mark Brown
2018-05-30 16:06                                           ` Doug Anderson
2018-05-30 16:07                                             ` Mark Brown
2018-05-30 16:09                                               ` Doug Anderson
2018-05-30 16:13                                                 ` Mark Brown
2018-05-30 16:31                                                   ` Doug Anderson
2018-05-30 16:36                                                     ` Mark Brown
2018-05-30 16:41                                                       ` Doug Anderson
2018-05-30 16:59                                                         ` Mark Brown
2018-05-18 22:24       ` Rob Herring
2018-05-12  2:28 ` [PATCH v3 2/2] regulator: add QCOM RPMh regulator driver David Collins
2018-05-17 21:23   ` Doug Anderson
2018-05-18  0:16     ` David Collins

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=20180522165537.GE24776@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 v3 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).