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

From: Quentin Schulz
Date: Tue Feb 18 2025 - 06:24:53 EST

Hi Heiko,

On 2/13/25 5:30 PM, Heiko Stuebner wrote:
From: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>

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@xxxxxxxxx>
---
.../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