linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: maxime.ripard@free-electrons.com (Maxime Ripard)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] sunxi: a10-lime: add regulator nodes
Date: Tue, 7 Apr 2015 12:17:57 +0200	[thread overview]
Message-ID: <20150407101757.GC15823@lukather> (raw)
In-Reply-To: <20150404184716.GZ6023@sirena.org.uk>

On Sat, Apr 04, 2015 at 07:47:16PM +0100, Mark Brown wrote:
> On Sat, Apr 04, 2015 at 02:56:45PM +0200, Maxime Ripard wrote:
> > On Sat, Apr 04, 2015 at 01:33:02PM +0100, Mark Brown wrote:
> 
> > > This is broken - think about what this means.  If you are defining a
> > > voltage (or any other constraint) you're saying that it's safe to use on
> > > a given board. If you provide a voltage constraint saying that the
> > > maximum allowable range for a voltage regulator is safe.  That's
> > > unlikely to be true on any given board, usually only a limited set of
> > > regulators can vary voltages at runtime safely at all and then rarely
> > > over their full supported range.  Similarly for other constraints, for
> > > example allowing a regulator to be disabled when there are driverless
> > > things (or drivers without regulator support or mappings for that board)
> > > relying on it is going to break.
> 
> > If this model is broken, then I don't see how the full regulator
> > support is not broken.
> 
> > Our PMIC DTSI sets up the regulators with the ranges supported by the
> > PMIC. We have two cases then:
> >   - Either the regulator is not used on the board, and it will be
> >     disabled by the framework.
> 
> Or the regulator is used by some passive component on the board that
> doesn't appear in DT (or some driver that doesn't yet have DT support
> doesn't claim the regulator) and it gets switched off.  Or we power it
> off and then later discover that we've been placing physical stress on
> the system.

Isn't that what the regulator-always-on property is here for?

> >   - Or the regulator is actually used on the board, it will be defined
> >     in the DTS, possibly with additional constraints.
> 
> Especially for voltage range constraints you're assuming that every
> system integrator will get everything right from their initial attempt
> to power up Linux onwards and will anticipate future enhancements to the
> drivers which try to vary voltages.  For any given design you can pretty
> much guarantee that the electrical engineers would not approve of
> software making use of the maximum allowable voltage variation but that
> is what you're proposing to enable.  That is clearly not a sensible or
> reasonable default, the best we can do is just not vary the voltage and
> let the system integrator say if it's safe to do so.
> 
> Just listing the regulators is less likely to be an issue, it's more
> just not particularly useful than anything else, though it can
> potentially cause long term stresses too.

My bad, that's actually just what we are doing, plus defining the
regulators that are at a fixed voltage and can't be disabled.

> Providing .dtsis for reference designs can make sense, if there's a
> widely cloned board with lots of common design elements then reusing
> that .dtsi is fine but clearly that .dtsi isn't going to enable the full
> flexibility of the regulators since there will be constraints from that
> reference design.

That PMIC is used with various SoCs, so that's not really an
option. And especially when it comes to regulators, I don't think
there's really a pattern that emerged yet...

> > I really don't see how your above comments apply to that
> > situation. What I do see though, is that if we drop the DTSI, all our
> > boards will have to define all the PMIC regulators, even if they're
> > not used at all on the board, just to have the right behaviour. And
> > that's clearly a no-go for me.
> 
> Nothing else is safe.  Remember, if we get this wrong we risk system
> instability and at worst physical damage to the system either in rare
> cases immediately due to getting something badly wrong or more likely
> from long term electrical stresses.  If what you were proposing were
> safe then we should still not be enumerating everything in device trees,
> we should just be enabling all operations for every regulator in the
> system by default.  The fact that you have to override this should be a
> warning sign that you shouldn't be doing it.

Well, we're doing low level stuff and (from experience) things like a
poor muxing option can cause physical damage too, and we don't have
any kind of protection or policy against that.

So, what do you suggest for the kernel to have a behaviour independant
of what the bootloader state was, without adding a lot of churn in
each and every DTS?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150407/f828df80/attachment.sig>

  reply	other threads:[~2015-04-07 10:17 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-27 10:58 [PATCH] sunxi: a10-lime: add regulator nodes Iain Paton
2015-03-27 11:02 ` Hans de Goede
2015-03-28 11:53   ` Iain Paton
2015-03-28 23:50     ` Hans de Goede
2015-04-04 10:20       ` Iain Paton
2015-03-30 22:23     ` Maxime Ripard
2015-04-04 10:30       ` Iain Paton
2015-04-04 11:06         ` Hans de Goede
2015-04-04 12:18       ` Javier Martinez Canillas
2015-04-04 12:33         ` Mark Brown
2015-04-04 12:56           ` Maxime Ripard
2015-04-04 13:05             ` Hans de Goede
2015-04-04 18:54               ` Mark Brown
2015-04-04 18:47             ` Mark Brown
2015-04-07 10:17               ` Maxime Ripard [this message]
2015-04-08 12:35                 ` Mark Brown
2015-04-13  8:15                   ` Maxime Ripard
2015-04-13 11:44                     ` Mark Brown
2015-04-14 16:27                       ` Maxime Ripard
2015-04-14 20:06                         ` Mark Brown
2015-04-04 12:58           ` Hans de Goede
2015-04-04 18:52             ` 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=20150407101757.GC15823@lukather \
    --to=maxime.ripard@free-electrons.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).