| Message ID | 20250213163013.1616467-1-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: add usb typec host support to rk3588-jaguar | expand |
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
Am Dienstag, 18. Februar 2025, 12:24:38 MEZ schrieb Quentin Schulz: > 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. when in doubt, follow the binding :-) ... so changed to typec-portc > > + 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? added a pinctrl for each, as this the interrupt is triggered level-low, pinctrl got a pull-up > > + 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. added self-powered ... reading other DTS this seems used in a number of boards and reading the code which uses this, it seems to ease a number of places where the code would check if the device might loose power when not self-powered. > > + 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? binding says so, so it shouldn't hurt ... added vbus-supply. > > @@ -394,6 +440,52 @@ &i2c8 { > > pinctrl-0 = <&i2c8m2_xfer>; > > status = "okay"; > > > > + usb-typec@22 { > > All the same remarks as for P11 above. done everything 2 times :-) . > > @@ -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. added comment > > +&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. Actually (or with the vbus-supply being part of the usb-c-connector now) this isn't actually needed and the vbus regulator seems to be handled correctly. So dropped that. > > +&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 */ added these > > + 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. I would assume that stems from the thing that the dp-phy is always just at the far-end of any of_graph lookup. The dpphy does not itself do any "thinking" but instead just gets told what to do from fusb302, dwc3 etc. So the port needs to be present as the "other end" of an of-graph link but the phy itself does not even use it at all. > > @@ -903,6 +1053,34 @@ &usb_host0_ohci { > > status = "okay"; > > }; > > > > Comment for USB-C P11 connector. ok > > +&usb_host0_xhci { > > Add a comment for highlighting it supports DRD, just that we aren't > ready to support it just yet. ok > > + 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. The port _can_ do role-switch, so we will need that later anyway once I figured out the switch part, so no harm in already having the link. Heiko
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 { + compatible = "fcs,fusb302"; + reg = <0x22>; + interrupt-parent = <&gpio4>; + interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>; + vbus-supply = <&vcc_5v0_usb_c1>; + + connector { + compatible = "usb-c-connector"; + data-role = "dual"; + label = "USBC-1 P11"; + 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>; + + 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 { + 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"; }; +&u2phy0 { + status = "okay"; +}; + +&u2phy0_otg { + phy-supply = <&vcc_5v0_usb_c1>; + status = "okay"; +}; + +&u2phy1 { + status = "okay"; +}; + +&u2phy1_otg { + phy-supply = <&vcc_5v0_usb_c2>; + status = "okay"; +}; + &u2phy2 { status = "okay"; }; @@ -893,6 +1003,46 @@ &uart7 { status = "okay"; }; +&usbdp_phy0 { + orientation-switch; + 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>; + }; + }; +}; + +&usbdp_phy1 { + orientation-switch; + 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>; + }; + }; +}; + /* host0 on P10 USB-A */ &usb_host0_ehci { status = "okay"; @@ -903,6 +1053,34 @@ &usb_host0_ohci { status = "okay"; }; +&usb_host0_xhci { + dr_mode = "host"; + status = "okay"; + + port { + #address-cells = <1>; + #size-cells = <0>; + + usb_host0_xhci_drd_sw: endpoint { + remote-endpoint = <&usbc0_hs>; + }; + }; +}; + +&usb_host1_xhci { + dr_mode = "host"; + status = "okay"; + + port { + #address-cells = <1>; + #size-cells = <0>; + + usb_host1_xhci_drd_sw: endpoint { + remote-endpoint = <&usbc1_hs>; + }; + }; +}; + /* host1 on M.2 E-key */ &usb_host1_ehci { status = "okay";