Re: [PATCH] arm64: dts: rockchip: add usb typec host support to rk3588-jaguar

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Heiko,

On 2/13/25 5:30 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Jaguar has two type-c ports connected to fusb302 controllers that can
> work both in host and device mode and can also run in display-port
> altmode.
> 
> While these ports can work in dual-role data mode, they do not support
> powering the device itself as power-sink. This causes issues because
> the current infrastructure does not cope well with dual-role data
> without dual-role power.
> 
> So add the necessary nodes for the type-c controllers as well
> as enable the relevant core usb nodes, but limit the mode to host-mode
> for now until we figure out device mode.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>   .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 178 ++++++++++++++++++
>   1 file changed, 178 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> index 90f823b2c219..329d98011c60 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> @@ -333,6 +333,52 @@ rtc_twi: rtc@6f {
>   		};
>   	};
>   
> +	usb-typec@22 {

We have a mix of node names in the Rockchip tree, some call it usb-typec 
some call it typec-portc, including the device tree binding.

> +		compatible = "fcs,fusb302";
> +		reg = <0x22>;
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>;

Should we have a pinmux for the interrupt line in GPIO mode maybe?

> +		vbus-supply = <&vcc_5v0_usb_c1>;
> +
> +		connector {
> +			compatible = "usb-c-connector";

Reading the binding, I'm wondering if we shouldn't set self-powered 
property in there as well? Jaguar cannot be powered (or at least wasn't 
designed for being powered) via USB-C and I think self-powered means 
that? Not sure to be honest.

> +			data-role = "dual";
> +			label = "USBC-1 P11";
> +			power-role = "source";
> +			source-pdos =
> +				<PDO_FIXED(5000, 1500, PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)>;
> +

Should we have vbus-supply = <&vcc_5v0_usb_c1>; here too?

> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					usbc0_hs: endpoint {
> +						remote-endpoint = <&usb_host0_xhci_drd_sw>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					usbc0_ss: endpoint {
> +						remote-endpoint = <&usbdp_phy0_typec_ss>;
> +					};
> +				};
> +
> +				port@2 {
> +					reg = <2>;
> +
> +					usbc0_sbu: endpoint {
> +						remote-endpoint = <&usbdp_phy0_typec_sbu>;
> +					};
> +				};
> +			};
> +		};
> +	};
> +
>   	vdd_npu_s0: regulator@42 {
>   		compatible = "rockchip,rk8602";
>   		reg = <0x42>;
> @@ -394,6 +440,52 @@ &i2c8 {
>   	pinctrl-0 = <&i2c8m2_xfer>;
>   	status = "okay";
>   
> +	usb-typec@22 {

All the same remarks as for P11 above.

> +		compatible = "fcs,fusb302";
> +		reg = <0x22>;
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <RK_PA4 IRQ_TYPE_LEVEL_LOW>;
> +		vbus-supply = <&vcc_5v0_usb_c2>;
> +
> +		connector {
> +			compatible = "usb-c-connector";
> +			data-role = "dual";
> +			label = "USBC-2 P12";
> +			power-role = "source";
> +			source-pdos =
> +				<PDO_FIXED(5000, 1500, PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					usbc1_hs: endpoint {
> +						remote-endpoint = <&usb_host1_xhci_drd_sw>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					usbc1_ss: endpoint {
> +						remote-endpoint = <&usbdp_phy1_typec_ss>;
> +					};
> +				};
> +
> +				port@2 {
> +					reg = <2>;
> +
> +					usbc1_sbu: endpoint {
> +						remote-endpoint = <&usbdp_phy1_typec_sbu>;
> +					};
> +				};
> +			};
> +		};
> +	};
> +
>   	vdd_cpu_big0_s0: regulator@42 {
>   		compatible = "rockchip,rk8602";
>   		reg = <0x42>;
> @@ -851,6 +943,24 @@ &tsadc {
>   	status = "okay";
>   };
>   

Please add a comment here that this is for USB-C P11 connector so it 
gets easier to figure out what's for what.

> +&u2phy0 {
> +	status = "okay";
> +};
> +
> +&u2phy0_otg {
> +	phy-supply = <&vcc_5v0_usb_c1>;

This is a bit confusing at we have the OTG port needing to specify the 
VBUS supply on the port, while the FUSB also specifies it and the 
usb-c-connector node can as well.

> +	status = "okay";
> +};
> +

Comment for USB-C P12 connector.

> +&u2phy1 {
> +	status = "okay";
> +};
> +
> +&u2phy1_otg {
> +	phy-supply = <&vcc_5v0_usb_c2>;
> +	status = "okay";
> +};
> +
>   &u2phy2 {
>   	status = "okay";
>   };
> @@ -893,6 +1003,46 @@ &uart7 {
>   	status = "okay";
>   };
>   

Comment for USB-C P11 connector.

> +&usbdp_phy0 {
> +	orientation-switch;

It seems like we have SBU1 and SBU2 GPIOs as well. So I guess we want 
something like:

sbu1-dc-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; /* Q7_USB_C0_SBU1_DC */
sbu2-dc-gpios = <&gpio1 RK_PC3 GPIO_ACTIVE_HIGH>; /* Q7_USB_C0_SBU2_DC */

> +	status = "okay";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		usbdp_phy0_typec_ss: endpoint@0 {
> +			reg = <0>;
> +			remote-endpoint = <&usbc0_ss>;
> +		};
> +
> +		usbdp_phy0_typec_sbu: endpoint@1 {
> +			reg = <1>;
> +			remote-endpoint = <&usbc0_sbu>;
> +		};

Something's wrong with the dt-binding here as it only lists one possible 
port, for the orientation.

> +	};
> +};
> +
> +&usbdp_phy1 {
> +	orientation-switch;

It seems like we have SBU1 and SBU2 GPIOs as well. So I guess we want 
something like:

sbu1-dc-gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; /* Q7_USB_C1_SBU1_DC */
sbu2-dc-gpios = <&gpio1 RK_PB5 GPIO_ACTIVE_HIGH>; /* Q7_USB_C1_SBU2_DC */

> +	status = "okay";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		usbdp_phy1_typec_ss: endpoint@0 {
> +			reg = <0>;
> +			remote-endpoint = <&usbc1_ss>;
> +		};
> +
> +		usbdp_phy1_typec_sbu: endpoint@1 {
> +			reg = <1>;
> +			remote-endpoint = <&usbc1_sbu>;
> +		};

Something's wrong with the dt-binding here as it only lists one possible 
port, for the orientation.

> +	};
> +};
> +
>   /* host0 on P10 USB-A */
>   &usb_host0_ehci {
>   	status = "okay";
> @@ -903,6 +1053,34 @@ &usb_host0_ohci {
>   	status = "okay";
>   };
>   

Comment for USB-C P11 connector.

> +&usb_host0_xhci {

Add a comment for highlighting it supports DRD, just that we aren't 
ready to support it just yet.

> +	dr_mode = "host";
> +	status = "okay";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		usb_host0_xhci_drd_sw: endpoint {
> +			remote-endpoint = <&usbc0_hs>;
> +		};

Does this actually make sense without usb-role-switch; set? The binding 
seems to indicate port is only useful when that is set.

> +	};
> +};
> +

Comment for USB-C P12 connector.

> +&usb_host1_xhci {

Add a comment for highlighting it supports DRD, just that we aren't 
ready to support it just yet.

> +	dr_mode = "host";
> +	status = "okay";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		usb_host1_xhci_drd_sw: endpoint {
> +			remote-endpoint = <&usbc1_hs>;
> +		};

Does this actually make sense without usb-role-switch; set? The binding 
seems to indicate port is only useful when that is set.

Cheers,
Quentin