Re: Including empty regulator nodes in axp209.dtsi is a BAD idea

On Wed, Jan 14, 2015 at 08:42:11AM +0100, Hans de Goede wrote:

Hi,

On 13-01-15 17:46, Maxime Ripard wrote:

quoted

On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:

quoted

Hi ChenYu, Maxime,

During the review of a few dts files for new boards Maxime asked me to use
axp209.dtsi to avoid the standard axp209 "boilerplate" present in most
boards using the axp209 pmic.

But axp209.dtsi includes empty regulator nodes, e.g. :

                reg_dcdc3: dcdc3 {
                        regulator-name = "dcdc3";
                };

This is a BAD idea, the presence of these empty nodes causes the
axp20x-regulator driver to actually register regulators for them,
and then on late_init the regulator subsys turns them off, since
they have absolutely no constraints set (nor users registered)
and the regulator subsys assumes that when devicetree is used their
is always a compete set of constraints and that thus turning them
off is safe.

So when I switched to using axp209.dtsi for the bananapro.dts,
and booted the bananapro this is the last message I got from the
kernel while booting:

[    2.314014] dcdc3: disabling

And away went our DRAM power-supply, oops.

So for dcdc2 (CPU) and dcdc3 (DRAM), the boilerplate
should contain reasonable constraints (eg the operating range
from the datasheet)

Indeed.

quoted

and an always-on property.

I disagree. The regulator disabling is a feature, and how the board is
wired is, well, up to the board.

And here I was thinking you wanted to reduce the amount of boilerplate
in our dts files ..

I want to reduce the boilerplate of doing the reasonable thing with
regulators: turning them off if unused.

IOW I disagree with your disagreeing all boards we know of have dcdc2
wired to Vcpu and dcdc3 wired to Vddr, so not having this in the dtsi
will lead to a lot of extra boilerplate in each dts file. We're not
talking about our main dtsi file here, if we ever encounter a board
which is wired in a different way, then its dts can simply not use
axp209.dtsi and instead define the nodes itself, it needs to do that
anyways if we do include the standard CPU and DDR constrains in the
dtsi since those will not make sense either in that case.

Ok. Let's do that and see how it turns out then

quoted

If an always-on property is needed, then it's in the DTS, not in the
AXP DTSI.

quoted

The ldo-s are trickier, since we simply do not know how those
are used, I think ldo2 is used for Avcc on most boards, so it
too should be always on, since almost any board will have some
analog parts on it (be it the ir receiver, lradc, rtp, lvds, vga,
or analog audio in/out). Assuming that we're willing to assume
that ldo2 is used this way, we should give it matching constraints
and always mark it always-on.

Ideally, all the drivers that have a analogic component should have a
reference to the regulator they use. But again, at the board
level. And more realistically, putting always-on should also happen at
the board level.

quoted

As for ldo3 - 5 I've no idea when / for what these are used, but
if we do not know it is better to just leave them be then to turn
them off IMHO, so we should remove the nodes for these from axp209.dtsi

Anyways sorting this all out is going to take some time, so I'm
not going to use axp209.dtsi in dts files for new boards for now.

I'm afraid it's an "all or nothing" situation.

No it is not, the PMIC is a mfd, and we can use some of its functions
fine without actually loading the regulator bits. This is already
done on most boards with the axp209, even without touching the regulators
it is nice to have the axp209 mfd driver loaded so that we get support
for the powerbutton, and support for poweroff, esp. the latter is quite
nice to have.

Hmmm, good point. We won't enforce the DTSI inclusion as a rule
then.

But I still believe that any DTS defining its regulators should use
the DTSI and a complete regulator description.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com