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

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

[Hans de Goede]

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) and an always-on property.

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.

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.

Regards,

Hans

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

[Maxime Ripard]

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

On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
> 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.

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

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

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

> 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. Either you load the
PMIC and describe as accurately as possible which regulator are in
use, or you don't. I'm fine with both, even if have a small preference
for the former, but getting stuck half-way is not really an option.

Maxime

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

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

[Hans de Goede]

Hi,

On 13-01-15 17:46, Maxime Ripard wrote:
> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
>> 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.
>
>> 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 ..

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.

> If an always-on property is needed, then it's in the DTS, not in the
> AXP DTSI.
>
>> 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.
>
>> 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.

Regards,

Hans

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

[Michal Suchanek]

Hello,

On 14 January 2015 at 08:42, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
>
> On 13-01-15 17:46, Maxime Ripard wrote:
>>
>> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
>>>
>>> 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.
>>
>>> 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 ..
>
> 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.
>

We don't really have a CPU and DDR driver so what will mark these
regulators as used when not always on?

There is cpufreq that modifies the voltage but that's optional.
Suspend may even turn off Vddr but that's quite special.

Cpufreq should work within the CPU and memory voltage constraints but
these may vary per SoC and even per board due to trace design. If it's
not possible to override the constraints per-board then setting them
in the include file is counter-productive.

The other regulators, sure. If you have analog audio you have a driver
for it and it should power on the analog part. If it does not do it
now it's a bug and it should be fixed.

Thanks

Michal

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

[Lars Doelle]

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

On Tuesday, January 13, 2015 17:46:06 Maxime Ripard wrote:
> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
> > 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.

It is very unclear to me, what to with this dtsi as it is.

Wrt. axp209, I have a specification like in sun7i-a20-olinuxino-lime2.dts.
If I include this dtsi, what does it help or how is it supposed to be of any
use?

In particular, the following section

|			axp209: pmic@34 {
|				compatible = "x-powers,axp209";
|				reg = <0x34>;
|				interrupt-parent = <&nmi_intc>;
|				interrupts = <0 IRQ_TYPE_LEVEL_LOW>;

|				interrupt-controller;
|				#interrupt-cells = <1>;

|				acin-supply = <&reg_axp_ipsout>;
|				vin2-supply = <&reg_axp_ipsout>;
|				vin3-supply = <&reg_axp_ipsout>;
|				ldo24in-supply = <&reg_axp_ipsout>;
|				ldo3in-supply = <&reg_axp_ipsout>;

would be needed anyway as it is not in sun7i-a20.dtsi, followed by the
regulators, to switch them on. Now a dtsi would make sense, if it shortens
the device specific information.

Should not the section above together with the full regulators be in axp209.dtsi
and only the regulators be mentioned in the boards dts, to switch them on? Isn't
this stuff that should be included in sun7i-a20.dtsi anyway, somehow?

Kind regards, Lars

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

[Chen-Yu Tsai]

On Wed, Jan 14, 2015 at 6:18 PM, Lars Doelle <lars.doelle-RMqqONVpFt+ELgA04lAiVw@public.gmane.org> wrote:
> On Tuesday, January 13, 2015 17:46:06 Maxime Ripard wrote:
>> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
>> > 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.
>
> It is very unclear to me, what to with this dtsi as it is.
>
> Wrt. axp209, I have a specification like in sun7i-a20-olinuxino-lime2.dts.
> If I include this dtsi, what does it help or how is it supposed to be of any
> use?
>
> In particular, the following section
>
> |                       axp209: pmic@34 {
> |                               compatible = "x-powers,axp209";
> |                               reg = <0x34>;
> |                               interrupt-parent = <&nmi_intc>;
> |                               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>
> |                               interrupt-controller;
> |                               #interrupt-cells = <1>;
>
> |                               acin-supply = <&reg_axp_ipsout>;
> |                               vin2-supply = <&reg_axp_ipsout>;
> |                               vin3-supply = <&reg_axp_ipsout>;
> |                               ldo24in-supply = <&reg_axp_ipsout>;
> |                               ldo3in-supply = <&reg_axp_ipsout>;

We should not be using reg_axp_ipsout, as it is an unregulated supply,
and can be left out.

>
> would be needed anyway as it is not in sun7i-a20.dtsi, followed by the
> regulators, to switch them on. Now a dtsi would make sense, if it shortens
> the device specific information.
>
> Should not the section above together with the full regulators be in axp209.dtsi
> and only the regulators be mentioned in the boards dts, to switch them on? Isn't
> this stuff that should be included in sun7i-a20.dtsi anyway, somehow?

It is entirely possible to make a board without using the PMIC, as Olimex
has demonstrated. sun7i-a20 only describes the common SoC bits.

ChenYu

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

[Chen-Yu Tsai]

Hi,

On Wed, Jan 14, 2015 at 3:42 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
>
> On 13-01-15 17:46, Maxime Ripard wrote:
>>
>> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
>>>
>>> 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.
>>
>>> 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 ..
>
> 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.

I think Maxime is only disagreeing to the "always-on" part. And I
somewhat agree with him, but on a technical level. It doesn't seem
possible to negate at the board level the "always-on" set in the dtsi,
or it is not obvious to me how to do so. With properties with values
you just set a new value. How do you "unset" a boolean flag?

As for the constraints, I think we can agree that everyone will use
the reference design if they choose to include the PMIC on a board.

>> If an always-on property is needed, then it's in the DTS, not in the
>> AXP DTSI.
>>
>>> 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.
>>
>>> 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.

I agree that a) the PMIC is an mfd, and b) we can use other bits without
loading the regulator bits. But if adding the regulator bits results in
some sort of crash or issue, just because some regulator got turned off,
it seems to me that the dt is not accurately describing the hardware.

ChenYu

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

[Maxime Ripard]

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

On Fri, Jan 16, 2015 at 12:48:53AM +0800, Chen-Yu Tsai wrote:
> On Wed, Jan 14, 2015 at 6:18 PM, Lars Doelle <lars.doelle-RMqqONVpFt+ELgA04lAiVw@public.gmane.org> wrote:
> > On Tuesday, January 13, 2015 17:46:06 Maxime Ripard wrote:
> >> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
> >> > 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.
> >
> > It is very unclear to me, what to with this dtsi as it is.
> >
> > Wrt. axp209, I have a specification like in sun7i-a20-olinuxino-lime2.dts.
> > If I include this dtsi, what does it help or how is it supposed to be of any
> > use?
> >
> > In particular, the following section
> >
> > |                       axp209: pmic@34 {
> > |                               compatible = "x-powers,axp209";
> > |                               reg = <0x34>;
> > |                               interrupt-parent = <&nmi_intc>;
> > |                               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> >
> > |                               interrupt-controller;
> > |                               #interrupt-cells = <1>;
> >
> > |                               acin-supply = <&reg_axp_ipsout>;
> > |                               vin2-supply = <&reg_axp_ipsout>;
> > |                               vin3-supply = <&reg_axp_ipsout>;
> > |                               ldo24in-supply = <&reg_axp_ipsout>;
> > |                               ldo3in-supply = <&reg_axp_ipsout>;
> 
> We should not be using reg_axp_ipsout, as it is an unregulated supply,
> and can be left out.

Can it? Doesn't the regulator driver need to know the input voltage?

> > would be needed anyway as it is not in sun7i-a20.dtsi, followed by the
> > regulators, to switch them on. Now a dtsi would make sense, if it shortens
> > the device specific information.
> >
> > Should not the section above together with the full regulators be in axp209.dtsi
> > and only the regulators be mentioned in the boards dts, to switch them on? Isn't
> > this stuff that should be included in sun7i-a20.dtsi anyway, somehow?
> 
> It is entirely possible to make a board without using the PMIC, as Olimex
> has demonstrated. sun7i-a20 only describes the common SoC bits.

Or even with a different PMIC.

Maxime

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

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

[Maxime Ripard]

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

On Wed, Jan 14, 2015 at 08:42:11AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 13-01-15 17:46, Maxime Ripard wrote:
> >On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
> >>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.
> >
> >>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

> >If an always-on property is needed, then it's in the DTS, not in the
> >AXP DTSI.
> >
> >>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.
> >
> >>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

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

[Maxime Ripard]

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

On Fri, Jan 16, 2015 at 12:54:58AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Wed, Jan 14, 2015 at 3:42 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Hi,
> >
> >
> > On 13-01-15 17:46, Maxime Ripard wrote:
> >>
> >> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
> >>>
> >>> 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.
> >>
> >>> 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 ..
> >
> > 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.
> 
> I think Maxime is only disagreeing to the "always-on" part. And I
> somewhat agree with him, but on a technical level. It doesn't seem
> possible to negate at the board level the "always-on" set in the dtsi,
> or it is not obvious to me how to do so. With properties with values
> you just set a new value. How do you "unset" a boolean flag?

You don't, but Hans suggestion was to not use the DTSI then and define
the nodes yourself in your DTS.

Another solution would be if this case comes up at some point to just
push the always-on constraints to the boards, but assume for now that
all boards will want it to be always on.

> As for the constraints, I think we can agree that everyone will use
> the reference design if they choose to include the PMIC on a board.

I'm not sure to get what you mean here. The PMIC has constraints of
its own (the min/max voltage it can deliver), and boards will have
some too (ie the min/max voltage the components can handle). So I
guess we will end up with constraints defined at both levels.

> >> If an always-on property is needed, then it's in the DTS, not in the
> >> AXP DTSI.
> >>
> >>> 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.
> >>
> >>> 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.
> 
> I agree that a) the PMIC is an mfd, and b) we can use other bits without
> loading the regulator bits. But if adding the regulator bits results in
> some sort of crash or issue, just because some regulator got turned off,
> it seems to me that the dt is not accurately describing the hardware.

Yeah, that's kind of the same argument than the one we had with
simplefb clocks. So I guess we can agree that we can allow the MFD
part alone to work, but if regulator support is introduced, then *all*
regulators must be described accurately.

Maxime

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

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

[Lars Doelle]

Hi,

> It is entirely possible to make a board without using the PMIC, as Olimex
> has demonstrated. sun7i-a20 only describes the common SoC bits.

I'm sorry, I wrongly thought that axp209 is on the SoC.

Lars

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

[Chen-Yu Tsai]

On Fri, Jan 16, 2015 at 6:04 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Fri, Jan 16, 2015 at 12:54:58AM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Wed, Jan 14, 2015 at 3:42 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > Hi,
>> >
>> >
>> > On 13-01-15 17:46, Maxime Ripard wrote:
>> >>
>> >> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
>> >>>
>> >>> 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.
>> >>
>> >>> 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 ..
>> >
>> > 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.
>>
>> I think Maxime is only disagreeing to the "always-on" part. And I
>> somewhat agree with him, but on a technical level. It doesn't seem
>> possible to negate at the board level the "always-on" set in the dtsi,
>> or it is not obvious to me how to do so. With properties with values
>> you just set a new value. How do you "unset" a boolean flag?
>
> You don't, but Hans suggestion was to not use the DTSI then and define
> the nodes yourself in your DTS.
>
> Another solution would be if this case comes up at some point to just
> push the always-on constraints to the boards, but assume for now that
> all boards will want it to be always on.
>
>> As for the constraints, I think we can agree that everyone will use
>> the reference design if they choose to include the PMIC on a board.
>
> I'm not sure to get what you mean here. The PMIC has constraints of
> its own (the min/max voltage it can deliver), and boards will have
> some too (ie the min/max voltage the components can handle). So I
> guess we will end up with constraints defined at both levels.

I don't know if we need to define the constraints the PMIC itself
has. Aren't these already defined in the driver?

What I meant was that it's not unreasonable to assume that most boards
using axp209 will follow the reference design, so dcdc2 powers the cpu,
dcdc3 powers the rest of the internal logic and dll, and ldo3 powers
avcc. And we could possibly put the the SoC constraints for these in
axp209.dtsi. Maybe except vcc-cpu, which is different across sun[457]i.

ChenYu

>> >> If an always-on property is needed, then it's in the DTS, not in the
>> >> AXP DTSI.
>> >>
>> >>> 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.
>> >>
>> >>> 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.
>>
>> I agree that a) the PMIC is an mfd, and b) we can use other bits without
>> loading the regulator bits. But if adding the regulator bits results in
>> some sort of crash or issue, just because some regulator got turned off,
>> it seems to me that the dt is not accurately describing the hardware.
>
> Yeah, that's kind of the same argument than the one we had with
> simplefb clocks. So I guess we can agree that we can allow the MFD
> part alone to work, but if regulator support is introduced, then *all*
> regulators must be described accurately.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

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

[Hans de Goede]

Hi,

On 15-01-15 23:04, Maxime Ripard wrote:
> On Fri, Jan 16, 2015 at 12:54:58AM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Wed, Jan 14, 2015 at 3:42 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> Hi,
>>>
>>>
>>> On 13-01-15 17:46, Maxime Ripard wrote:
>>>>
>>>> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
>>>>>
>>>>> 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.
>>>>
>>>>> 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 ..
>>>
>>> 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.
>>
>> I think Maxime is only disagreeing to the "always-on" part. And I
>> somewhat agree with him, but on a technical level. It doesn't seem
>> possible to negate at the board level the "always-on" set in the dtsi,
>> or it is not obvious to me how to do so. With properties with values
>> you just set a new value. How do you "unset" a boolean flag?
>
> You don't, but Hans suggestion was to not use the DTSI then and define
> the nodes yourself in your DTS.
>
> Another solution would be if this case comes up at some point to just
> push the always-on constraints to the boards, but assume for now that
> all boards will want it to be always on.
>
>> As for the constraints, I think we can agree that everyone will use
>> the reference design if they choose to include the PMIC on a board.
>
> I'm not sure to get what you mean here. The PMIC has constraints of
> its own (the min/max voltage it can deliver), and boards will have
> some too (ie the min/max voltage the components can handle). So I
> guess we will end up with constraints defined at both levels.
>
>>>> If an always-on property is needed, then it's in the DTS, not in the
>>>> AXP DTSI.
>>>>
>>>>> 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.
>>>>
>>>>> 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.
>>
>> I agree that a) the PMIC is an mfd, and b) we can use other bits without
>> loading the regulator bits. But if adding the regulator bits results in
>> some sort of crash or issue, just because some regulator got turned off,
>> it seems to me that the dt is not accurately describing the hardware.
>
> Yeah, that's kind of the same argument than the one we had with
> simplefb clocks. So I guess we can agree that we can allow the MFD
> part alone to work, but if regulator support is introduced, then *all*
> regulators must be described accurately.

Hmm, I would prefer to have all listed regulators described accurately,
if we do not know how a regulator is used it may be best to just leave it
out (leaving it at whatever setting the bootloader has configured it) AFAIK
there is no technical reason why we must describe all or no regulators.

Regards,

Hans