arm: dts: sun50i-h6-orangepi-3: disable aldo2 regulator

Mainline TF-A has a broken behavior - it enables all used AXP regulator
outputs, without much reason.

It breaks PHY on Orange Pi 3 and other boards, because they a specific
power-on sequence is required. aldo2 which is enabled by TF-A is just a
half of PHY's power and the other half that is untouched by TF-A must be
enabled simultaneously (even a small difference may cause a break-down).
If not TF-A, kernel driver would do everything correctly.

However, some boards rely on this behavior, so it's impossible to get
rid of it.

TF-A recently started checking regulator status, and now it enables a
regulator only if it's status is "okay". Disabling regulator in U-Boot's
dts workarounds the problem.
---
 arch/arm/dts/sun50i-h6-orangepi-3.dts | 4 ++++
 1 file changed, 4 insertions(+)

On Tue,  7 Sep 2021 19:23:26 +0300
Maxim Karasev <begs@disroot.org> wrote:

Hi Maxim,

please add the respective maintainers in To: or CC:, as reported by
scripts/get_maintainer.pl.
Also please add at least "sunxi" or "allwinner" somewhere in the subject
line, that helps the reduce the response time ;-)

> Mainline TF-A has a broken behavior - it enables all used AXP regulator
> outputs, without much reason.

Without that code you could not use most peripherals powered by the PMIC
in U-Boot, the PHY being the most prominent one.
But yes, it's a long standing issue - at least for the OPi3, which seems
to be one of the few boards really having an issue with that.
And the real solution is to drop the regulator handling in TF-A, and use a
DM compliant AXP driver in U-Boot. Samuel's recent work should have
brought that a bit closer, but it's still quite some work ahead:
https://oftc.irclog.whitequark.org/linux-sunxi/2021-10-11#30291278;

> It breaks PHY on Orange Pi 3 and other boards, because they a specific
> power-on sequence is required. aldo2 which is enabled by TF-A is just a
> half of PHY's power and the other half that is untouched by TF-A must be
> enabled simultaneously (even a small difference may cause a break-down).

I wish OrangePi would have designed a bit less of a special snowflake
here.

> If not TF-A, kernel driver would do everything correctly.
> 
> However, some boards rely on this behavior, so it's impossible to get
> rid of it.
> 
> TF-A recently started checking regulator status, and now it enables a
> regulator only if it's status is "okay". Disabling regulator in U-Boot's
> dts workarounds the problem.

The problem is that there is no such thing as "U-Boot's dts". I always use
that DT to pass it on to the kernel (via $fdtcontroladdr), and this is the
designated way to use UEFI. And that is just one reason we don't accept DT
hacks just in U-Boot.

I was thinking about adding a TF-A build option that would skip regulator
setup altogether, which would then become standard in some happy feature
when U-Boot takes proper care of regulators.
Maybe you can just disable this already in your TF-A build, to see if that
fixes your issues?

In any case your patch below is unfortunately not a solution.

Cheers,
Andre

> ---
>  arch/arm/dts/sun50i-h6-orangepi-3.dts | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/dts/sun50i-h6-orangepi-3.dts b/arch/arm/dts/sun50i-h6-orangepi-3.dts
> index 7e83f6146f..9f91e4ec88 100644
> --- a/arch/arm/dts/sun50i-h6-orangepi-3.dts
> +++ b/arch/arm/dts/sun50i-h6-orangepi-3.dts
> @@ -207,6 +207,10 @@
>  				regulator-min-microvolt = <3300000>;
>  				regulator-max-microvolt = <3300000>;
>  				regulator-name = "vcc33-audio-tv-ephy-mac";
> +				/* This regulator must be enabled by the kernel,
> +				 * not by u-boot or TF-A, otherwise power-on
> +				 * sequence will be wrong and PHY won't work */
> +				status = "disabled";
>  			};
>  
>  			/* ALDO3 is shorted to CLDO1 */

Hi. Thanks for explanation, I'm new to mailing lists, so I appreciate this.

Also I'm happy to hear that any work is done to address that issue.

>I was thinking about adding a TF-A build option that would skip regulator
>setup altogether, which would then become standard in some happy feature
>when U-Boot takes proper care of regulators.
>Maybe you can just disable this already in your TF-A build, to see if that
>fixes your issues?

It believe it should fix that exact issue, but in my situation (I'm a distro packager) such solutions (having a several atf versions) are unacceptable.

>In any case your patch below is unfortunately not a solution.

It's rather the most painless workaround in my case.

On October 13, 2021 7:25:36 PM GMT+03:00, Andre Przywara <andre.przywara@arm.com> wrote:
>On Tue,  7 Sep 2021 19:23:26 +0300
>Maxim Karasev <begs@disroot.org> wrote:
>
>Hi Maxim,
>
>please add the respective maintainers in To: or CC:, as reported by
>scripts/get_maintainer.pl.
>Also please add at least "sunxi" or "allwinner" somewhere in the subject
>line, that helps the reduce the response time ;-)
>
>> Mainline TF-A has a broken behavior - it enables all used AXP regulator
>> outputs, without much reason.
>
>Without that code you could not use most peripherals powered by the PMIC
>in U-Boot, the PHY being the most prominent one.
>But yes, it's a long standing issue - at least for the OPi3, which seems
>to be one of the few boards really having an issue with that.
>And the real solution is to drop the regulator handling in TF-A, and use a
>DM compliant AXP driver in U-Boot. Samuel's recent work should have
>brought that a bit closer, but it's still quite some work ahead:
>https://oftc.irclog.whitequark.org/linux-sunxi/2021-10-11#30291278;
>
>> It breaks PHY on Orange Pi 3 and other boards, because they a specific
>> power-on sequence is required. aldo2 which is enabled by TF-A is just a
>> half of PHY's power and the other half that is untouched by TF-A must be
>> enabled simultaneously (even a small difference may cause a break-down).
>
>I wish OrangePi would have designed a bit less of a special snowflake
>here.
>
>> If not TF-A, kernel driver would do everything correctly.
>> 
>> However, some boards rely on this behavior, so it's impossible to get
>> rid of it.
>> 
>> TF-A recently started checking regulator status, and now it enables a
>> regulator only if it's status is "okay". Disabling regulator in U-Boot's
>> dts workarounds the problem.
>
>The problem is that there is no such thing as "U-Boot's dts". I always use
>that DT to pass it on to the kernel (via $fdtcontroladdr), and this is the
>designated way to use UEFI. And that is just one reason we don't accept DT
>hacks just in U-Boot.
>

>
>Cheers,
>Andre
>
>> ---
>>  arch/arm/dts/sun50i-h6-orangepi-3.dts | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/arch/arm/dts/sun50i-h6-orangepi-3.dts b/arch/arm/dts/sun50i-h6-orangepi-3.dts
>> index 7e83f6146f..9f91e4ec88 100644
>> --- a/arch/arm/dts/sun50i-h6-orangepi-3.dts
>> +++ b/arch/arm/dts/sun50i-h6-orangepi-3.dts
>> @@ -207,6 +207,10 @@
>>  				regulator-min-microvolt = <3300000>;
>>  				regulator-max-microvolt = <3300000>;
>>  				regulator-name = "vcc33-audio-tv-ephy-mac";
>> +				/* This regulator must be enabled by the kernel,
>> +				 * not by u-boot or TF-A, otherwise power-on
>> +				 * sequence will be wrong and PHY won't work */
>> +				status = "disabled";
>>  			};
>>  
>>  			/* ALDO3 is shorted to CLDO1 */
>

On Wed, 20 Oct 2021 21:31:09 +0300
Maxim Karasev <begs@disroot.org> wrote:

Hi Maxim,

> Hi. Thanks for explanation, I'm new to mailing lists, so I appreciate this.
> 
> Also I'm happy to hear that any work is done to address that issue.
> 
> >I was thinking about adding a TF-A build option that would skip regulator
> >setup altogether, which would then become standard in some happy feature
> >when U-Boot takes proper care of regulators.
> >Maybe you can just disable this already in your TF-A build, to see if that
> >fixes your issues?  
> 
> It believe it should fix that exact issue, but in my situation (I'm a distro packager) such solutions (having a several atf versions) are unacceptable.

What are you packaging, exactly? (And for what distro?) I guess it's some
kind of U-Boot/firmware package [1]? So you build this per device then?

And this might not have been clear, but I was more asking for a *test* of
this approach (rather than a random hack in your tree). I don't have
this device, so I cannot test that easily.
I made an easy patch the other day which introduces a TF-A build time
option, so you would just build it for the OPi3 with
"SUNXI_SKIP_REGULATOR_SETUP=1" on the make command line. Eventually, once
U-Boot takes over the regulator setup, this would probably become the
default, but for now this would just be a workaround for OPi3 users.
It would be good to know if that fixes your problem.

> >In any case your patch below is unfortunately not a solution.  
> 
> It's rather the most painless workaround in my case.

It's a workaround, yes. But what I was actually wondering about: where in
the DT you use the MAC and those regulators? I don't see any mentioning of
that in the kernel or U-Boot tree. So is this some off-tree .dts?

Cheers,
Andre

[1] Please note that I consider *distros* packaging *firmware* the wrong
approach. Firmware is device specific, not distro-specific. There should
be no reason that the Debian firmware for some device is different from
the Arch Linux firmware.
I am asking for years for people to coordinate, and publish distro-generic
firmware. This mostly works already with current mainline U-Boot & TF-A,
but would need some testing and fine-tuning.

> 
> On October 13, 2021 7:25:36 PM GMT+03:00, Andre Przywara <andre.przywara@arm.com> wrote:
> >On Tue,  7 Sep 2021 19:23:26 +0300
> >Maxim Karasev <begs@disroot.org> wrote:
> >
> >Hi Maxim,
> >
> >please add the respective maintainers in To: or CC:, as reported by
> >scripts/get_maintainer.pl.
> >Also please add at least "sunxi" or "allwinner" somewhere in the subject
> >line, that helps the reduce the response time ;-)
> >  
> >> Mainline TF-A has a broken behavior - it enables all used AXP regulator
> >> outputs, without much reason.  
> >
> >Without that code you could not use most peripherals powered by the PMIC
> >in U-Boot, the PHY being the most prominent one.
> >But yes, it's a long standing issue - at least for the OPi3, which seems
> >to be one of the few boards really having an issue with that.
> >And the real solution is to drop the regulator handling in TF-A, and use a
> >DM compliant AXP driver in U-Boot. Samuel's recent work should have
> >brought that a bit closer, but it's still quite some work ahead:
> >https://oftc.irclog.whitequark.org/linux-sunxi/2021-10-11#30291278;
> >  
> >> It breaks PHY on Orange Pi 3 and other boards, because they a specific
> >> power-on sequence is required. aldo2 which is enabled by TF-A is just a
> >> half of PHY's power and the other half that is untouched by TF-A must be
> >> enabled simultaneously (even a small difference may cause a break-down).  
> >
> >I wish OrangePi would have designed a bit less of a special snowflake
> >here.
> >  
> >> If not TF-A, kernel driver would do everything correctly.
> >> 
> >> However, some boards rely on this behavior, so it's impossible to get
> >> rid of it.
> >> 
> >> TF-A recently started checking regulator status, and now it enables a
> >> regulator only if it's status is "okay". Disabling regulator in U-Boot's
> >> dts workarounds the problem.  
> >
> >The problem is that there is no such thing as "U-Boot's dts". I always use
> >that DT to pass it on to the kernel (via $fdtcontroladdr), and this is the
> >designated way to use UEFI. And that is just one reason we don't accept DT
> >hacks just in U-Boot.
> >  
> 
> >
> >Cheers,
> >Andre
> >  
> >> ---
> >>  arch/arm/dts/sun50i-h6-orangepi-3.dts | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/arch/arm/dts/sun50i-h6-orangepi-3.dts b/arch/arm/dts/sun50i-h6-orangepi-3.dts
> >> index 7e83f6146f..9f91e4ec88 100644
> >> --- a/arch/arm/dts/sun50i-h6-orangepi-3.dts
> >> +++ b/arch/arm/dts/sun50i-h6-orangepi-3.dts
> >> @@ -207,6 +207,10 @@
> >>  				regulator-min-microvolt = <3300000>;
> >>  				regulator-max-microvolt = <3300000>;
> >>  				regulator-name = "vcc33-audio-tv-ephy-mac";
> >> +				/* This regulator must be enabled by the kernel,
> >> +				 * not by u-boot or TF-A, otherwise power-on
> >> +				 * sequence will be wrong and PHY won't work */
> >> +				status = "disabled";
> >>  			};
> >>  
> >>  			/* ALDO3 is shorted to CLDO1 */  
> >

>What are you packaging, exactly? (And for what distro?) I guess it's some
>kind of U-Boot/firmware package [1]?

Yes, it's U-Boot + ATF at Alpine and Linux + WiFi/BT firmware at postmarketOS.

>So you build this per device then?

Well, not exactly. Alpine builds one shared ATF package that includes TF-A for several SoCs, plus U-Boot is a metapackage, which means that all device targets are built at the same time from the same source and then split into subpackages. Alpine wouldn't want to include separate board-specific U-Boot or ATF packages.

pmOS accepts such workarounds on the other hand, but it's still unwanted way.

>And this might not have been clear, but I was more asking for a *test* of
>this approach (rather than a random hack in your tree). I don't have
>this device, so I cannot test that easily.
>I made an easy patch the other day which introduces a TF-A build time
>option, so you would just build it for the OPi3 with
>"SUNXI_SKIP_REGULATOR_SETUP=1" on the make command line. Eventually, once
>U-Boot takes over the regulator setup, this would probably become the
>default, but for now this would just be a workaround for OPi3 users.
>It would be good to know if that fixes your problem.

Following patch workarounds the issue:

diff --git a/drivers/allwinner/axp/common.c b/drivers/allwinner/axp/common.c
index e98b16fdb..111a4d0d7 100644
--- a/drivers/allwinner/axp/common.c
+++ b/drivers/allwinner/axp/common.c
@@ -98,8 +98,9 @@ static int setup_regulator(const void *fdt, int node,

 static bool should_enable_regulator(const void *fdt, int node)
 {
-       if (fdt_getprop(fdt, node, "phandle", NULL) != NULL)
-               return true;
+//XXX: no need to enable referenced regulators
+//     if (fdt_getprop(fdt, node, "phandle", NULL) != NULL)
+//             return true;
        if (fdt_getprop(fdt, node, "regulator-always-on", NULL) != NULL)
                return true;
        return false;

However, idk if disabling regulator setup entirely won't break anything. I may test it, but I haven't got your patch.

Such config option would be pretty usable, e.g. creating a arm-trusted-firmware-no-reg-setup subpackage, which is built from same source, but with that config option, would be clean enough to be accepted in Alpine.

>It's a workaround, yes. But what I was actually wondering about: where in
>the DT you use the MAC and those regulators? I don't see any mentioning of
>that in the kernel or U-Boot tree. So is this some off-tree .dts?

Yes, kernel source we use at pmOS for allwinner is megous.com/git/linux plus some extra device-specific patches.

>Please note that I consider *distros* packaging *firmware* the wrong
>approach. Firmware is device specific, not distro-specific. There should
>be no reason that the Debian firmware for some device is different from
>the Arch Linux firmware.
>I am asking for years for people to coordinate, and publish distro-generic
>firmware. This mostly works already with current mainline U-Boot & TF-A,
>but would need some testing and fine-tuning.

Hm, I've heard about UEFI on U-Boot before and it sounds interesting, but I've never seen it in reality.

However, I see nothing wrong with distro providing a pre-build mainline U-Boot and ATF binaries somewhere at /usr/share to save user's time. It needs to be installed to the board somehow anyway, so, if not packaging, everyone would have to compile it for themselves.

pmOS just automates the process of creating a ready-to-go system images and provides useful tools and tolerance to hacks for supporting as many devices as possible.