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

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

[fuyao]

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

Signed-off-by: fuyao <fuyao1697@cyg.com>
---
 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.

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

[Andre Przywara]

On Wed, 20 Dec 2023 16:18:43 +0800
fuyao <fuyao1697@cyg.com> 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 <fuyao1697@cyg.com>
> ---
>  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>;

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

[fuyao]

On Wed, Dec 20, 2023 at 03:04:00PM +0000, Andre Przywara wrote:
> On Wed, 20 Dec 2023 16:18:43 +0800
> fuyao <fuyao1697@cyg.com> 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 <fuyao1697@cyg.com>
> > ---
> >  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>;
> 

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

[Andre Przywara]

On Thu, 21 Dec 2023 10:20:49 +0800
fuyao <fuyao@sjterm.com> 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 <fuyao1697@cyg.com> 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 <fuyao1697@cyg.com>
> > > ---
> > >  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>;  
> >   
> 

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

[fuyao]

On Thu, Dec 21, 2023 at 10:39:06AM +0000, Andre Przywara wrote:
> On Thu, 21 Dec 2023 10:20:49 +0800
> fuyao <fuyao@sjterm.com> 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 <fuyao1697@cyg.com> 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 <fuyao1697@cyg.com>
> > > > ---
> > > >  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 [付尧]

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

[Andre Przywara]

On Mon, 25 Dec 2023 18:11:24 +0800
fuyao <fuyao@sjterm.com> 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 <fuyao@sjterm.com> 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 <fuyao1697@cyg.com> 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 <fuyao1697@cyg.com>
> > > > > ---
> > > > >  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>;    
> > > >     
> > >   
> 

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

[Jernej Škrabec]

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 <fuyao1697@cyg.com>

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