Re: [PATCH v2 3/3] riscv: dts: spacemit: Enable USB3.0 on BananaPi-F3

[Yixun Lan <dlan@gentoo.org>]

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?

> +		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
> +		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>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	hub_2_0: hub@1 {
> +		compatible = "usb2109,2817";
> +		reg = <0x1>;
> +		vdd-supply = <&usb3_vhub>;
> +		peer-hub = <&hub_3_0>;
> +		reset-gpios = <&gpio K1_GPIO(124) GPIO_ACTIVE_LOW>;
> +	};
> +
> +	hub_3_0: hub@2 {
> +		compatible = "usb2109,817";
> +		reg = <0x2>;
> +		vdd-supply = <&usb3_vhub>;
> +		peer-hub = <&hub_2_0>;
> +		reset-gpios = <&gpio K1_GPIO(124) GPIO_ACTIVE_LOW>;
> +	};
> +};
> +
can you move these two after &uart0? I'd like to keep them sorted by 
label alphabet order..

>  &emmc {
>  	bus-width = <8>;
>  	mmc-hs400-1_8v;
> 
> -- 
> 2.52.0
> 

-- 
Yixun Lan (dlan)