2023-12-20 08:51:38

by fuyao

[permalink] [raw]
Subject: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

the aldo1 is connect regulator pin which power the TV.
The USB core use TV ref as reference Voltage.

Signed-off-by: fuyao <[email protected]>
---
arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
index 9f39b5a2bb35..8906170461df 100644
--- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
+++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
@@ -42,6 +42,13 @@ &pio {
vcc-pg-supply = <&reg_dldo1>;
};

+&reg_aldo1 {
+ regulator-always-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-name = "vcc-aldo1";
+};
+
&reg_aldo2 {
regulator-always-on;
regulator-min-microvolt = <1800000>;
--
2.39.2


--
CYG Technology.


2023-12-20 15:04:17

by Andre Przywara

[permalink] [raw]
Subject: Re: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

On Wed, 20 Dec 2023 16:18:43 +0800
fuyao <[email protected]> wrote:

Hi,

> the aldo1 is connect regulator pin which power the TV.

What do you mean with that? That ALDO1 is connected to VCC-TVOUT and/or
VCC-TVIN on the R40 SoC?

> The USB core use TV ref as reference Voltage.

The USB core in the SoC? So pin VCC-USB, which requires 3.3V, the same
voltage as the TV pins?
Which means this doesn't really have much to do with TV, it's just that
USB and also "TV" are supplied by ALDO1?

> Signed-off-by: fuyao <[email protected]>
> ---
> arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> index 9f39b5a2bb35..8906170461df 100644
> --- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> +++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> @@ -42,6 +42,13 @@ &pio {
> vcc-pg-supply = <&reg_dldo1>;
> };
>
> +&reg_aldo1 {
> + regulator-always-on;

So did USB never work before, with the DT as in mainline?

For always-on regulators it would be good to see some rationale why this
cannot be referenced by its consumer. If it is really supplying the USB
core, that would be a reason, because we don't have a good way of
describing this.

> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-aldo1";

Regulators should be named after their users, so use something like:
regulator-name = "vcc-3v3-tv-usb";

That then also serves as documentation of why this is always on.

Cheers,
Andre

> +};
> +
> &reg_aldo2 {
> regulator-always-on;
> regulator-min-microvolt = <1800000>;


2023-12-21 02:21:07

by Fuyao Kashizuku

[permalink] [raw]
Subject: Re: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

On Wed, Dec 20, 2023 at 03:04:00PM +0000, Andre Przywara wrote:
> On Wed, 20 Dec 2023 16:18:43 +0800
> fuyao <[email protected]> wrote:
>
> Hi,
>
> > the aldo1 is connect regulator pin which power the TV.
>
> What do you mean with that? That ALDO1 is connected to VCC-TVOUT and/or
> VCC-TVIN on the R40 SoC?

The ALDO1 is connected to VCC-TVOUT on the R40 Soc.

>
> > The USB core use TV ref as reference Voltage.
>
> The USB core in the SoC? So pin VCC-USB, which requires 3.3V, the same
> voltage as the TV pins?
> Which means this doesn't really have much to do with TV, it's just that
> USB and also "TV" are supplied by ALDO1?

The internal USB PHY requires a reference voltage. It seems that in
order to save costs, the reference voltage of the TVOUT module is used.

>
> > Signed-off-by: fuyao <[email protected]>
> > ---
> > arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > index 9f39b5a2bb35..8906170461df 100644
> > --- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > @@ -42,6 +42,13 @@ &pio {
> > vcc-pg-supply = <&reg_dldo1>;
> > };
> >
> > +&reg_aldo1 {
> > + regulator-always-on;
>
> So did USB never work before, with the DT as in mainline?
>

The USB can work, but is unstable. Occasionally disconnected because of
the D+/D- electrical characteristics.

> For always-on regulators it would be good to see some rationale why this
> cannot be referenced by its consumer. If it is really supplying the USB
> core, that would be a reason, because we don't have a good way of
> describing this.
>
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-name = "vcc-aldo1";
>
> Regulators should be named after their users, so use something like:
> regulator-name = "vcc-3v3-tv-usb";
>

thanks.

> That then also serves as documentation of why this is always on.
>
> Cheers,
> Andre
>
> > +};
> > +
> > &reg_aldo2 {
> > regulator-always-on;
> > regulator-min-microvolt = <1800000>;
>


2023-12-21 10:40:42

by Andre Przywara

[permalink] [raw]
Subject: Re: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

On Thu, 21 Dec 2023 10:20:49 +0800
fuyao <[email protected]> wrote:

Hi,

thanks for the reply!

> On Wed, Dec 20, 2023 at 03:04:00PM +0000, Andre Przywara wrote:
> > On Wed, 20 Dec 2023 16:18:43 +0800
> > fuyao <[email protected]> wrote:
> >
> > Hi,
> >
> > > the aldo1 is connect regulator pin which power the TV.
> >
> > What do you mean with that? That ALDO1 is connected to VCC-TVOUT and/or
> > VCC-TVIN on the R40 SoC?
>
> The ALDO1 is connected to VCC-TVOUT on the R40 Soc.

Ah, thanks for the confirmation.

> > > The USB core use TV ref as reference Voltage.
> >
> > The USB core in the SoC? So pin VCC-USB, which requires 3.3V, the same
> > voltage as the TV pins?
> > Which means this doesn't really have much to do with TV, it's just that
> > USB and also "TV" are supplied by ALDO1?
>
> The internal USB PHY requires a reference voltage. It seems that in
> order to save costs, the reference voltage of the TVOUT module is used.

Do you mean a USB *reference* voltage that is separate from the USB PHY
power supply voltage, so pin VCC-USB on the SoC? And that it is internally
connected to some TV-OUT related circuits? So that would apply to all
devices using the R40 SoC then?

Or is it simply that the SoC pins VCC-TVOUT and VCC-USB are connected
together, on this SoM?
Do you have access to some schematic? I couldn't find one online easily,
so cannot check this myself.

Thanks,
Andre

> > > Signed-off-by: fuyao <[email protected]>
> > > ---
> > > arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > index 9f39b5a2bb35..8906170461df 100644
> > > --- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > +++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > @@ -42,6 +42,13 @@ &pio {
> > > vcc-pg-supply = <&reg_dldo1>;
> > > };
> > >
> > > +&reg_aldo1 {
> > > + regulator-always-on;
> >
> > So did USB never work before, with the DT as in mainline?
> >
>
> The USB can work, but is unstable. Occasionally disconnected because of
> the D+/D- electrical characteristics.
>
> > For always-on regulators it would be good to see some rationale why this
> > cannot be referenced by its consumer. If it is really supplying the USB
> > core, that would be a reason, because we don't have a good way of
> > describing this.
> >
> > > + regulator-min-microvolt = <3300000>;
> > > + regulator-max-microvolt = <3300000>;
> > > + regulator-name = "vcc-aldo1";
> >
> > Regulators should be named after their users, so use something like:
> > regulator-name = "vcc-3v3-tv-usb";
> >
>
> thanks.
>
> > That then also serves as documentation of why this is always on.
> >
> > Cheers,
> > Andre
> >
> > > +};
> > > +
> > > &reg_aldo2 {
> > > regulator-always-on;
> > > regulator-min-microvolt = <1800000>;
> >
>


2023-12-22 17:52:27

by Jernej Škrabec

[permalink] [raw]
Subject: Re: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

Dne sreda, 20. december 2023 ob 09:18:43 CET je fuyao napisal(a):
> the aldo1 is connect regulator pin which power the TV.
> The USB core use TV ref as reference Voltage.
>
> Signed-off-by: fuyao <[email protected]>

Subject line must have board name in it. Please also update message
as it's unclear.

Best regards,
Jernej

> ---
> arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> index 9f39b5a2bb35..8906170461df 100644
> --- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> +++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> @@ -42,6 +42,13 @@ &pio {
> vcc-pg-supply = <&reg_dldo1>;
> };
>
> +&reg_aldo1 {
> + regulator-always-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc-aldo1";
> +};
> +
> &reg_aldo2 {
> regulator-always-on;
> regulator-min-microvolt = <1800000>;
>





2023-12-25 10:11:48

by Fuyao Kashizuku

[permalink] [raw]
Subject: Re: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

On Thu, Dec 21, 2023 at 10:39:06AM +0000, Andre Przywara wrote:
> On Thu, 21 Dec 2023 10:20:49 +0800
> fuyao <[email protected]> wrote:
>
> Hi,
>
> thanks for the reply!
>
> > On Wed, Dec 20, 2023 at 03:04:00PM +0000, Andre Przywara wrote:
> > > On Wed, 20 Dec 2023 16:18:43 +0800
> > > fuyao <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > > the aldo1 is connect regulator pin which power the TV.
> > >
> > > What do you mean with that? That ALDO1 is connected to VCC-TVOUT and/or
> > > VCC-TVIN on the R40 SoC?
> >
> > The ALDO1 is connected to VCC-TVOUT on the R40 Soc.
>
> Ah, thanks for the confirmation.
>
> > > > The USB core use TV ref as reference Voltage.
> > >
> > > The USB core in the SoC? So pin VCC-USB, which requires 3.3V, the same
> > > voltage as the TV pins?
> > > Which means this doesn't really have much to do with TV, it's just that
> > > USB and also "TV" are supplied by ALDO1?
> >
> > The internal USB PHY requires a reference voltage. It seems that in
> > order to save costs, the reference voltage of the TVOUT module is used.
>
> Do you mean a USB *reference* voltage that is separate from the USB PHY
> power supply voltage, so pin VCC-USB on the SoC? And that it is internally
> connected to some TV-OUT related circuits? So that would apply to all
> devices using the R40 SoC then?
yes, The usb need a power from TV module insides.
>
> Or is it simply that the SoC pins VCC-TVOUT and VCC-USB are connected
> together, on this SoM?
no
> Do you have access to some schematic? I couldn't find one online easily,
> so cannot check this myself.
>
It has up to https://file.io/VSUL4FDrapDY
> Thanks,
> Andre
>
> > > > Signed-off-by: fuyao <[email protected]>
> > > > ---
> > > > arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > > index 9f39b5a2bb35..8906170461df 100644
> > > > --- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > > @@ -42,6 +42,13 @@ &pio {
> > > > vcc-pg-supply = <&reg_dldo1>;
> > > > };
> > > >
> > > > +&reg_aldo1 {
> > > > + regulator-always-on;
> > >
> > > So did USB never work before, with the DT as in mainline?
> > >
> >
> > The USB can work, but is unstable. Occasionally disconnected because of
> > the D+/D- electrical characteristics.
> >
> > > For always-on regulators it would be good to see some rationale why this
> > > cannot be referenced by its consumer. If it is really supplying the USB
> > > core, that would be a reason, because we don't have a good way of
> > > describing this.
> > >
> > > > + regulator-min-microvolt = <3300000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > + regulator-name = "vcc-aldo1";
> > >
> > > Regulators should be named after their users, so use something like:
> > > regulator-name = "vcc-3v3-tv-usb";
> > >
> >
> > thanks.
> >
> > > That then also serves as documentation of why this is always on.
> > >
> > > Cheers,
> > > Andre
> > >
> > > > +};
> > > > +
> > > > &reg_aldo2 {
> > > > regulator-always-on;
> > > > regulator-min-microvolt = <1800000>;
> > >
> >

--
fuyao [付尧]

2023-12-26 01:17:21

by Andre Przywara

[permalink] [raw]
Subject: Re: [PATCH RESEND] ARM: dts: sun8i: r40: open the regulator aldo1

On Mon, 25 Dec 2023 18:11:24 +0800
fuyao <[email protected]> wrote:

Hi,

thanks for the reply, with insightful information!

> On Thu, Dec 21, 2023 at 10:39:06AM +0000, Andre Przywara wrote:
> > On Thu, 21 Dec 2023 10:20:49 +0800
> > fuyao <[email protected]> wrote:
> >
> > Hi,
> >
> > thanks for the reply!
> >
> > > On Wed, Dec 20, 2023 at 03:04:00PM +0000, Andre Przywara wrote:
> > > > On Wed, 20 Dec 2023 16:18:43 +0800
> > > > fuyao <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > > the aldo1 is connect regulator pin which power the TV.
> > > >
> > > > What do you mean with that? That ALDO1 is connected to VCC-TVOUT and/or
> > > > VCC-TVIN on the R40 SoC?
> > >
> > > The ALDO1 is connected to VCC-TVOUT on the R40 Soc.
> >
> > Ah, thanks for the confirmation.
> >
> > > > > The USB core use TV ref as reference Voltage.
> > > >
> > > > The USB core in the SoC? So pin VCC-USB, which requires 3.3V, the same
> > > > voltage as the TV pins?
> > > > Which means this doesn't really have much to do with TV, it's just that
> > > > USB and also "TV" are supplied by ALDO1?
> > >
> > > The internal USB PHY requires a reference voltage. It seems that in
> > > order to save costs, the reference voltage of the TVOUT module is used.
> >
> > Do you mean a USB *reference* voltage that is separate from the USB PHY
> > power supply voltage, so pin VCC-USB on the SoC? And that it is internally
> > connected to some TV-OUT related circuits? So that would apply to all
> > devices using the R40 SoC then?
> yes, The usb need a power from TV module insides.

Ah, alright. I dimly remember hearing reports about unstable USB
operation on some (custom?) R40 boards, that might as well explain it.
On the BananaPis (the only other officially supported R40 boards),
TVOUT and TVOUT are connected to DCDC1, so are effectively always
powered by 3.3V already, which would explain why we didn't observe USB
issues there.

> > Or is it simply that the SoC pins VCC-TVOUT and VCC-USB are connected
> > together, on this SoM?
> no

Thanks for the confirmation!

> > Do you have access to some schematic? I couldn't find one online easily,
> > so cannot check this myself.
> >
> It has up to https://file.io/VSUL4FDrapDY

Ah, many thanks, that's really useful! That indeed confirms that both
TVIN and TVOUT are exclusively powered by ALDO1.

So if you resend the patch as v2, with the regulator-name changed, and
possibly with the following commit message, I'd support it:

=============
The USB PHY in the Allwinner R40 SoC seems to rely on voltage on the
VCC-TVIN/OUT supply pins for proper operation, on top of its own supply
voltage on VCC-USB. Without a 3.3V voltage supplied to VCC-TV*, USB
operation becomes unstable and can result in disconnects.

The Forlinx FETA40i-C SoM connects both the VCC-TVOUT and VCC-TVIN pins
to the ALDO1 rail of the PMIC, so we need to enable that rail for USB
operation. Since there is no supply property in the DT bindings for
the USB core, we need to always enable the regulator.

This fixes unstable USB operation on boards using the Forlinx FETA40i-C
module.
================

Cheers,
Andre


> > Thanks,
> > Andre
> >
> > > > > Signed-off-by: fuyao <[email protected]>
> > > > > ---
> > > > > arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi | 7 +++++++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > > > index 9f39b5a2bb35..8906170461df 100644
> > > > > --- a/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-r40-feta40i.dtsi
> > > > > @@ -42,6 +42,13 @@ &pio {
> > > > > vcc-pg-supply = <&reg_dldo1>;
> > > > > };
> > > > >
> > > > > +&reg_aldo1 {
> > > > > + regulator-always-on;
> > > >
> > > > So did USB never work before, with the DT as in mainline?
> > > >
> > >
> > > The USB can work, but is unstable. Occasionally disconnected because of
> > > the D+/D- electrical characteristics.
> > >
> > > > For always-on regulators it would be good to see some rationale why this
> > > > cannot be referenced by its consumer. If it is really supplying the USB
> > > > core, that would be a reason, because we don't have a good way of
> > > > describing this.
> > > >
> > > > > + regulator-min-microvolt = <3300000>;
> > > > > + regulator-max-microvolt = <3300000>;
> > > > > + regulator-name = "vcc-aldo1";
> > > >
> > > > Regulators should be named after their users, so use something like:
> > > > regulator-name = "vcc-3v3-tv-usb";
> > > >
> > >
> > > thanks.
> > >
> > > > That then also serves as documentation of why this is always on.
> > > >
> > > > Cheers,
> > > > Andre
> > > >
> > > > > +};
> > > > > +
> > > > > &reg_aldo2 {
> > > > > regulator-always-on;
> > > > > regulator-min-microvolt = <1800000>;
> > > >
> > >
>