diff mbox

[linux-sunxi,1/3] sunxi: A20-OLinuXino-LIME2: Fix ldo3/ldo4 in DTS

Message ID 56F5DEB9.7070207@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Iain Paton March 26, 2016, 12:58 a.m. UTC
On 25/03/16 19:04, Michael Haas wrote:

> This patch implements the suggested changes. It is not enough
> to set the voltage to 2800000 to avoid the crash. Hence, I have
> also removed regulator-always-on.

NAK.

If setting both min and max to 2800000 isn't working then I'd suggest you're 
just papering over the issue and not actually solving it.

It should be fairly clear that whenever those regulators are turned on by 
some driver you're going to see the same issue, you've just delayed the 
problem by removing the regulator-always-on item.

The real problem here is people monkeying with the regulators without 
understanding what they're doing, bothering to test the results, or caring 
about the consequences.

Anyway, Hans was correct in that the datasheet we have access to says that 
the defaults for these regulators are undefined. 
I suspect that's not the whole story though, if these regulators had random 
values at powerup we'd have seen problems before now. 
Most likely the values we think we know are simply incorrect plus there's 
probably some element of sequencing involved - perhaps LDO4 needs turned on 
first. 
However I don't see sufficient detail in the datasheets to confirm any of 
that, so we shouldn't assume anything.
Electrically on the lime2 LDO3 goes to VCC-PE and LDO4 to VCC-PG. Neither 
groups E or G appear to be connected to anything other than gpio headers, 
so this has little to do with board design, it's something internal to 
the SoC that the datasheet is lacking on detail about.

One of the reasons for the regulator-always-on being in the dts was so that 
the regulator didn't get touched and so didn't break anything.

All said and done, it only takes a minute with a multimeter to determine 
actual defaults. Helpful to have multiple boards to gain some confidence 
that they behave the same, but still basing values like this on tiny 
sample sizes is probably unwise.

Looking at other A20 based boards, Cubieboards for example, shows LDO3/4 
being unused and the VCC-PE/PG pins they power on the lime2 being tied 
directly to 3.3v. No issues there simply because there's no way to mess 
with the power to those blocks.

The patch you really want is something like the one below which works 
on my boards regardless of the presence of regulator-always-on.
Verified on four boards with u-boot 2016.03 and kernel 4.5.0.

Whether you decide to remove regulator-always-on or not is a different 
question, but please use 2.3v, or some other proven working value, for 
ldo3. At least that way you have a fighting chance of the board 
remaining functional when something wants to turn the regulator on.

It's a good bet that the same u-boot change will have implications 
for other boards, if not now then at some future point.

Hans, take note, the schematic is wrong. You shouldn't assume either 
the schematic or you are correct. Ill considered changes to regulators 
can cause real damage to peoples boards.
I know we've discussed regulators before and that my opinion falls on 
deaf ears, but worth one last try..



Author: Iain Paton <ipaton0@gmail.com>
Date:   Fri Mar 25 22:07:55 2016 +0000

sunxi: A20-OLinuXino-LIME2: Update ldo3/ldo4 in DTS

A recent ill-considered u-boot change:

>commit 02cc27c74f9b884b538bcd1b93342a4c05b5d608
>Author: Hans de Goede <hdegoede@redhat.com>
>Date:   Sat Oct 3 15:29:24 2015 +0200
>
>    sunxi: power: Change axp209 LDO3 and LDO4 default to disabled

causes the lime2 to lockup/crash when LDO3/4 are re-enabled by the
kernel.

The AXP209 datasheet shows the powerup values for these regulators 
to be undefined and the 2.8v suggested by the lime2 schematic also 
causes a lockup when the regulators are re-enabled so is obviously 
also incorrect.

Empirical measurement suggests LDO3 @ 2.3v and LDO4 @ 2.8v are the 
actual defaults, but is based on a small sample size. 

Verified to work with u-boot 2016.03, kernel 4.5.0 on four lime2 
boards.

Signed-off-by: Iain Paton <ipaton0@gmail.com>
---

Comments

Iain Paton March 26, 2016, 8:56 a.m. UTC | #1
Having reread the A20 datasheet yesterday, something in the back of my 
mind was bothering me overnight so I took some time to check this morning.

On the lime2 schematic we see the pins labeled as follows:

F19 : VCC_CSI0
E18 : VCC_CSI1

However the A20 datasheet doesn't label them this way, instead using:

F19 : VCC-PE : Port E Power Supply
E18 : VCC-PG : Port G Power Supply

no mention at all of CSI. CSI just happens to be a function that can be 
multiplexed onto ports E & G.

So lets assume for a moment we don't have a CSI device, or a CSI driver 
and are not using those pins for CSI functions but are instead using 
their default GPIO purpose, or in the case of PG perhaps for UART3 
or UART4.

What happens when you disable the regulator?  Hopefully from the above 
you'll already have worked out that you lose ports E & G when you turn 
their I/O power supply off and anything multiplexed onto those ports 
becomes unuseable.

I haven't checked other schematics, but all of the Olimex A10/A20 
boards along with Cubieboard & CubieTruck label these pins as VCC_CSIn
It would be my guess that there's a reference design being given to 
manufacturers, perhaps the original Cubieboard even, that makes this 
mistake and everyone else has blindly copied it.

So, unless we want to lose access to 24 GPIO pins for no real reason 
I'd suggest that the u-boot patch is reverted and/or the dts is altered 
such that the gpio driver claims the regulators and prevents them being 
turned off.

The u-boot commit message and reasoning given on list are trivially 
proved incorrect. Ports E & G are not CSI-only, losing all other 
functions on these ports is not acceptable.

I've also just tested building u-boot with LDO3 & 4 voltages set to 
3.3v to be the same as CubieBoard, this works for lime2.
So it's unlikely that the 2.8v & 2.3v settings from my previous patch 
make sense. The crash is actually being caused by something else, 
probably sequencing, due to unnecessarily turning the regulators off.
Whatever the reason, we don't have enough information in the 
datasheets to know for sure so we simply shouldn't turn them off to 
begin with.

The people behind this need to take responsibility and clean up their 
mess.

Iain
Michael Haas March 27, 2016, 8:08 a.m. UTC | #2
On 03/26/2016 09:56 AM, Iain Paton wrote:
> Having reread the A20 datasheet yesterday, something in the back of my 
> mind was bothering me overnight so I took some time to check this morning.
>
> On the lime2 schematic we see the pins labeled as follows:
>
> F19 : VCC_CSI0
> E18 : VCC_CSI1
>
> However the A20 datasheet doesn't label them this way, instead using:
>
> F19 : VCC-PE : Port E Power Supply
> E18 : VCC-PG : Port G Power Supply
>
> no mention at all of CSI. CSI just happens to be a function that can be 
> multiplexed onto ports E & G.
>
> So lets assume for a moment we don't have a CSI device, or a CSI driver 
> and are not using those pins for CSI functions but are instead using 
> their default GPIO purpose, or in the case of PG perhaps for UART3 
> or UART4.
>
> What happens when you disable the regulator?  Hopefully from the above 
> you'll already have worked out that you lose ports E & G when you turn 
> their I/O power supply off and anything multiplexed onto those ports 
> becomes unuseable.
>

Hi Iain,

I took some time to go through the data sheets and your analysis is on
point.

I did raise an issue with Olimex via github [0]. Maybe they will update
their schematics.


I do have my breakout board connected to GPIO-1.. and it turns out that
mostly port G is exposed on GPIO-1. I'll be reverting Hans' patch locally
so I can toggle some pins today. I am sure he will contribute his own
thoughts on the matter.


> I've also just tested building u-boot with LDO3 & 4 voltages set to 
> 3.3v to be the same as CubieBoard, this works for lime2.
> So it's unlikely that the 2.8v & 2.3v settings from my previous patch 
> make sense. The crash is actually being caused by something else, 
> probably sequencing, due to unnecessarily turning the regulators off.
> Whatever the reason, we don't have enough information in the 
> datasheets to know for sure so we simply shouldn't turn them off to 
> begin with.


I just tried that on linux and axp20x-regulator.ko refuses to set the
voltage
on ldo4:

[   88.914653] at24 1-0050: 2048 byte 24c16 EEPROM, writable, 16 bytes/write
[   96.893312] axp20x-i2c 0-0034: AXP20x variant AXP209 found
[   96.910631] axp20x-i2c 0-0034: AXP20X driver loaded
[   96.955579] input: axp20x-pek as
/devices/platform/soc@01c00000/1c2ac00.i2c/i2c-0/0-0034/axp20x-pek/input/input0
[   96.971734] ldo4: failed to apply 3300000uV constraint(-22)
[   96.990646] axp20x-regulator axp20x-regulator: Failed to register ldo4
[   97.012686] axp20x-regulator: probe of axp20x-regulator failed with
error -22

Perhaps this requires another patch [1], but unless there's a real need
for me to verify this,
I'll simply revert the u-boot patch and enjoy working GPIO.

Michael

[0] https://github.com/OLIMEX/OLINUXINO/issues/35
[1] https://www.spinics.net/lists/devicetree/msg118871.html
Hans de Goede March 28, 2016, 1:01 p.m. UTC | #3
Hi,

On 27-03-16 10:08, Michael Haas wrote:
> On 03/26/2016 09:56 AM, Iain Paton wrote:
>> Having reread the A20 datasheet yesterday, something in the back of my
>> mind was bothering me overnight so I took some time to check this morning.
>>
>> On the lime2 schematic we see the pins labeled as follows:
>>
>> F19 : VCC_CSI0
>> E18 : VCC_CSI1
>>
>> However the A20 datasheet doesn't label them this way, instead using:
>>
>> F19 : VCC-PE : Port E Power Supply
>> E18 : VCC-PG : Port G Power Supply
>>
>> no mention at all of CSI. CSI just happens to be a function that can be
>> multiplexed onto ports E & G.
>>
>> So lets assume for a moment we don't have a CSI device, or a CSI driver
>> and are not using those pins for CSI functions but are instead using
>> their default GPIO purpose, or in the case of PG perhaps for UART3
>> or UART4.
>>
>> What happens when you disable the regulator?  Hopefully from the above
>> you'll already have worked out that you lose ports E & G when you turn
>> their I/O power supply off and anything multiplexed onto those ports
>> becomes unuseable.
>>
>
> Hi Iain,
>
> I took some time to go through the data sheets and your analysis is on
> point.
>
> I did raise an issue with Olimex via github [0]. Maybe they will update
> their schematics.
>
>
> I do have my breakout board connected to GPIO-1.. and it turns out that
> mostly port G is exposed on GPIO-1. I'll be reverting Hans' patch locally
> so I can toggle some pins today. I am sure he will contribute his own
> thoughts on the matter.
>
>
>> I've also just tested building u-boot with LDO3 & 4 voltages set to
>> 3.3v to be the same as CubieBoard, this works for lime2.
>> So it's unlikely that the 2.8v & 2.3v settings from my previous patch
>> make sense. The crash is actually being caused by something else,
>> probably sequencing, due to unnecessarily turning the regulators off.
>> Whatever the reason, we don't have enough information in the
>> datasheets to know for sure so we simply shouldn't turn them off to
>> begin with.
>
>
> I just tried that on linux and axp20x-regulator.ko refuses to set the
> voltage
> on ldo4:
>
> [   88.914653] at24 1-0050: 2048 byte 24c16 EEPROM, writable, 16 bytes/write
> [   96.893312] axp20x-i2c 0-0034: AXP20x variant AXP209 found
> [   96.910631] axp20x-i2c 0-0034: AXP20X driver loaded
> [   96.955579] input: axp20x-pek as
> /devices/platform/soc@01c00000/1c2ac00.i2c/i2c-0/0-0034/axp20x-pek/input/input0
> [   96.971734] ldo4: failed to apply 3300000uV constraint(-22)
> [   96.990646] axp20x-regulator axp20x-regulator: Failed to register ldo4
> [   97.012686] axp20x-regulator: probe of axp20x-regulator failed with
> error -22
>
> Perhaps this requires another patch [1],

Yes setting the constraints to 3.3v requires the patch you link for things to work.

> but unless there's a real need
> for me to verify this,
> I'll simply revert the u-boot patch and enjoy working GPIO.

Given the entire discussion in this thread, I agree that fixing this in u-boot
is best. But I do not see reverting the u-boot patch disabling ldo3/4 by default
as the solution. ldo3/4 are unused on most boards, so disabling them by default
clearly is the right thing todo IMHO.

Instead please submit a patch for configs/A20-OLinuXino-Lime2_defconfig which
configures them at 2800 mV there (per the Lime2 schematic).

It would be good to also check the Lime1 schematic if they are used in the same
way there, I would also welcome a patch for:

configs/A10-OLinuXino-Lime_defconfig
configs/A20-OLinuXino-Lime_defconfig

Regards,

Hans
Maxime Ripard March 29, 2016, 12:03 p.m. UTC | #4
On Mon, Mar 28, 2016 at 03:01:27PM +0200, Hans de Goede wrote:
> >but unless there's a real need
> >for me to verify this,
> >I'll simply revert the u-boot patch and enjoy working GPIO.
> 
> Given the entire discussion in this thread, I agree that fixing this in u-boot
> is best. But I do not see reverting the u-boot patch disabling ldo3/4 by default
> as the solution. ldo3/4 are unused on most boards, so disabling them by default
> clearly is the right thing todo IMHO.
> 
> Instead please submit a patch for configs/A20-OLinuXino-Lime2_defconfig which
> configures them at 2800 mV there (per the Lime2 schematic).
> 
> It would be good to also check the Lime1 schematic if they are used in the same
> way there, I would also welcome a patch for:
> 
> configs/A10-OLinuXino-Lime_defconfig
> configs/A20-OLinuXino-Lime_defconfig

Adding a comment to the DT summing up that whole discussion would be
great too.

Maxime
diff mbox

Patch

diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
index d5c796c..e665d22 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
@@ -140,14 +140,14 @@ 
                        };
 
                        vcc_csi0: ldo3 {
-                               regulator-min-microvolt = <700000>;
-                               regulator-max-microvolt = <3500000>;
+                               regulator-min-microvolt = <2300000>;
+                               regulator-max-microvolt = <2300000>;
                                regulator-always-on;
                        };
 
                        vcc_csi1: ldo4 {
-                               regulator-min-microvolt = <1250000>;
-                               regulator-max-microvolt = <3300000>;
+                               regulator-min-microvolt = <2800000>;
+                               regulator-max-microvolt = <2800000>;
                                regulator-always-on;
                        };