public inbox for devicetree@vger.kernel.org
 help / color / mirror / Atom feed
From: Ze Huang <huang.ze@linux.dev>
To: Yixun Lan <dlan@gentoo.org>, Ze Huang <huang.ze@linux.dev>
Cc: Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Paul Walmsley <pjw@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Alexandre Ghiti <alex@ghiti.fr>,
	devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
	spacemit@lists.linux.dev, linux-kernel@vger.kernel.org,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH v2 3/3] riscv: dts: spacemit: Enable USB3.0 on BananaPi-F3
Date: Fri, 9 Jan 2026 12:07:05 +0800	[thread overview]
Message-ID: <aWB-6UkudFit3ZBX@monica.localdomain> (raw)
In-Reply-To: <20260108113605-GYA4052@gentoo.org>

On Thu, Jan 08, 2026 at 07:36:05PM +0800, Yixun Lan wrote:
> Hi Ze,
> 
> I've done a quick check of the "regulator-always-on" issue, 
> it's ok to drop the one from hub, but not the vbus one..
> 

Thanks for your double check

> On 09:21 Thu 08 Jan     , Yixun Lan wrote:
> > Hi Ze,
> > 
> >   thanks for your patch, I have few comments, see below..
> > 
> > On 20:05 Wed 07 Jan     , Ze Huang wrote:
> > > Enable the DWC3 USB 3.0 controller and its associated usbphy2 on the
> > > Banana Pi F3 board.
> > > 
> > > The board utilizes a VLI VL817 hub, which requires two separate power
> > > supplies: one VBUS and one for hub itself. Add two GPIO-controlled
> > > fixed-regulators to manage this.
> > > 
> > > Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> > > Signed-off-by: Ze Huang <huang.ze@linux.dev>
> > > ---
> > >  arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 48 +++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > > 
> > > diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > > index 3f10efd925dc..013df91c6a4c 100644
> > > --- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > > +++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > > @@ -59,6 +59,26 @@ reg_vcc_4v: vcc-4v {
> > >  		regulator-always-on;
> > >  		vin-supply = <&reg_dc_in>;
> > >  	};
> > > +
> > > +	usb3_vbus: regulator-vbus-5v {
> > I've checked the schematics, the name is 5V_VBUS there, so for the consistency
> > with previous naming convention, let's change to:
> > 	usb3_vbus_5v: usb3-vbus-5v 
> > 
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "USB30_VBUS";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > ..
> > > +		regulator-always-on;
> > do you think the "always-on" property really necessary? it indicate the
> > power regulator is critical, and should never been disabled even during
> > suspend/resume state, for the case of USB, I think it should be totally
> > fine to poweroff once the device is not used(suspended) or even disabled
> > 
> > besides, the regulator is designed with a gpio enabling/disabling control
> > which means it can be powered to on/off state?
> > 


> not ok to drop, see comment below at &usb_dwc3 

> 
> > > +		gpio = <&gpio K1_GPIO(97) GPIO_ACTIVE_HIGH>;
> > > +		enable-active-high;
> > > +	};
> > > +
> > > +	usb3_vhub: regulator-vhub-5v {
> > why use vhub, but not hub? where does this name come from?
> > 
> > and for same reason, the name in schematics is VCC5V0_HUB, so how about
> > change it to:
> > 	usb3_hub_5v: usb3-hub-5v 
> > 
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "USB30_VHUB";
> >                                     ~~~need to fix too, if above is valid
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > ..
> > > +		regulator-always-on;
> > ditto

> ok to drop, as it's already handled at drivers/usb/misc/onboard_usb_dev.c
> which will explicitly request the regulator and enable it

> 
> > > +		gpio = <&gpio K1_GPIO(123) GPIO_ACTIVE_HIGH>;
> > > +		enable-active-high;
> > > +	};
> > >  };
> > >  
> > >  &combo_phy {
> > > @@ -67,6 +87,34 @@ &combo_phy {
> > >  	status = "okay";
> > >  };
> > >  
> > ..
> > > +&usbphy2 {
> > > +	status = "okay";
> > > +};
> > > +
> > > +&usb_dwc3 {
> > > +	dr_mode = "host";
> > > +	vbus-supply = <&usb3_vbus>;
> due to drivers/usb/dwc3/dwc3-generic-plat.c has no handler to request regulator
> there will be problem if "regulator-always-on" property is removed..

On the BananaPi-F3 and to Jupiter, the VL817 hub setup uses two distinct controls:

GPIO97  controls the physical VBUS supply.
GPIO123 controls the VCC5V0_HUB supply for the hub logic.

(Note: This differs from the Orange Pi RV2, where only GPIO123 is used to
control the VBUS supply).

dwc3-generic-plat can not currently handle the regulator request explicitly,
keeping "regulator-always-on" forces the VBUS to remain active even when
suspended, which consumes unnecessary power.

I prefer to drop regulator-always-on in the DTS to accurately describe the
hardware capability, and let the driver manage the VBUS state. This ensures
better power management.

If anyone have other ideas, please let me know.

> 
> > > +	#address-cells = <1>;
> > > +	#size-cells = <0>;
> > > +	status = "okay";
> > > +
> 
> -- 
> Yixun Lan (dlan)

  reply	other threads:[~2026-01-09  4:07 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-07 12:05 [PATCH v2 0/3] riscv: dts: spacemit: Add USB 3.0 support for K1 Ze Huang
2026-01-07 12:05 ` [PATCH v2 1/3] riscv: dts: spacemit: Add USB2 PHY node " Ze Huang
2026-01-07 12:05 ` [PATCH v2 2/3] riscv: dts: spacemit: Add DWC3 USB 3.0 controller " Ze Huang
2026-01-07 12:05 ` [PATCH v2 3/3] riscv: dts: spacemit: Enable USB3.0 on BananaPi-F3 Ze Huang
2026-01-08  1:21   ` Yixun Lan
2026-01-08 11:36     ` Yixun Lan
2026-01-09  4:07       ` Ze Huang [this message]
2026-01-09  5:21         ` Samuel Holland
2026-01-10 12:04           ` Ze Huang
2026-01-10 12:32             ` Yixun Lan
2026-01-09  4:00     ` Ze Huang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aWB-6UkudFit3ZBX@monica.localdomain \
    --to=huang.ze@linux.dev \
    --cc=alex@ghiti.fr \
    --cc=aou@eecs.berkeley.edu \
    --cc=aurelien@aurel32.net \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlan@gentoo.org \
    --cc=krzk+dt@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=pjw@kernel.org \
    --cc=robh@kernel.org \
    --cc=spacemit@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox