| Message ID | 20250226102507.3743437-1-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] arm64: dts: rockchip: add usb typec host support to rk3588-jaguar | expand |
Hi Heiko, On 2/26/25 11:25 AM, 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. > We're not limiting the mode to host-mode anymore in v3. I tested and indeed peripheral mode doesn't work. While my ACM gadget device is created, I cannot communicate with it. Maybe reword in the commit log that only host mode is supported even though we don't enforce it? The USB2-only issue I experienced in v2 has been fixed with: https://lore.kernel.org/linux-rockchip/20250226103810.3746018-1-heiko@sntech.de/T/#t Tested-by: Quentin Schulz <quentin.schulz@cherry.de> See below for further comments. > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > --- > changes in v3: > - more review comments from Quentin > (sbu-pin pinctrl, comments) > changes in v2: > - address review comments from Quentin > (comments, pinctrl, sbu-gpios and much more) > > .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 218 ++++++++++++++++++ > 1 file changed, 218 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts > index 20b566d4168f..5dbcdf67f0a5 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts [...] > @@ -483,6 +583,26 @@ pcie30x4_waken_m0: pcie30x4-waken-m0 { > rockchip,pins = <0 RK_PC7 12 &pcfg_pull_none>; > }; > }; > + > + usb3 { > + cc_int1: cc-int1 { > + rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + cc_int2: cc-int2 { > + rockchip,pins = <4 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + typec0_sbu_pins: typec0-sbu-pins { Please rename to typec<x>_sbu_dc_pins as they aren't SBU pins, they are pins for DC coupling of SBU as far as I could tell from the DT binding. > + rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>, > + <1 RK_PC3 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + typec1_sbu_pins: typec1-sbu-pins { > + rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>, > + <1 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>; > + }; We're the first ones to declare a pinmux/pinconf for the SBU-DC pins and I'm not too sure if we should let them "float" or not. The default pinconf for those pins is PD, so maybe we should keep that PD. (For C1 they are PU). Rock 5 ITX routes the SBU-DC pins to GPIOs whose pinconf defaults to PU. CM3588 from FriendlyElec, Orange Pi 5, Orange Pi 5 Plus and NanoPC-T6 use GPIOs whose pinconf defaults to PD. I don't see external HW PU/PD in our or their designs so I would recommend to respect the default pinconf and put PD for the sbu-dc pins for USB-C0 and PU for USB-C1? Cheers, Quentin
Am Mittwoch, 26. Februar 2025, 14:17:37 MEZ schrieb Quentin Schulz: > Hi Heiko, > > On 2/26/25 11:25 AM, 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. > > > > We're not limiting the mode to host-mode anymore in v3. > > I tested and indeed peripheral mode doesn't work. While my ACM gadget > device is created, I cannot communicate with it. > > Maybe reword in the commit log that only host mode is supported even > though we don't enforce it? > > The USB2-only issue I experienced in v2 has been fixed with: > https://lore.kernel.org/linux-rockchip/20250226103810.3746018-1-heiko@sntech.de/T/#t > > Tested-by: Quentin Schulz <quentin.schulz@cherry.de> > > See below for further comments. > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > > --- > > changes in v3: > > - more review comments from Quentin > > (sbu-pin pinctrl, comments) > > changes in v2: > > - address review comments from Quentin > > (comments, pinctrl, sbu-gpios and much more) > > > > .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 218 ++++++++++++++++++ > > 1 file changed, 218 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts > > index 20b566d4168f..5dbcdf67f0a5 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts > [...] > > @@ -483,6 +583,26 @@ pcie30x4_waken_m0: pcie30x4-waken-m0 { > > rockchip,pins = <0 RK_PC7 12 &pcfg_pull_none>; > > }; > > }; > > + > > + usb3 { > > + cc_int1: cc-int1 { > > + rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>; > > + }; > > + > > + cc_int2: cc-int2 { > > + rockchip,pins = <4 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>; > > + }; > > + > > + typec0_sbu_pins: typec0-sbu-pins { > > Please rename to typec<x>_sbu_dc_pins as they aren't SBU pins, they are > pins for DC coupling of SBU as far as I could tell from the DT binding. > > > + rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>, > > + <1 RK_PC3 RK_FUNC_GPIO &pcfg_pull_none>; > > + }; > > + > > + typec1_sbu_pins: typec1-sbu-pins { > > + rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>, > > + <1 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>; > > + }; > > We're the first ones to declare a pinmux/pinconf for the SBU-DC pins and > I'm not too sure if we should let them "float" or not. The default > pinconf for those pins is PD, so maybe we should keep that PD. (For C1 > they are PU). > > Rock 5 ITX routes the SBU-DC pins to GPIOs whose pinconf defaults to PU. > CM3588 from FriendlyElec, Orange Pi 5, Orange Pi 5 Plus and NanoPC-T6 > use GPIOs whose pinconf defaults to PD. > > I don't see external HW PU/PD in our or their designs so I would > recommend to respect the default pinconf and put PD for the sbu-dc pins > for USB-C0 and PU for USB-C1? But if you're worried about behaviour wrt. floating, having them pulled in different direction for typec0 and typec1 also wouldn't result in differing behaviour? Also the pins are output-only, so the phy will always set them in some way? But now you made me looks things up ;-) For the TS3USBCA4 USB Type-C SBU Multiplexer [0], the sbu pins on it are described as "This pin has an internal nominally 1.6-MΩ pull-down resistor." In the block-diagram of the NX20P0407 Protection IC [1], it also looks like a pull down is the config of choice. Heiko [0] https://www.ti.com/lit/ds/symlink/ts3usbca4.pdf - page 3 [1] https://www.nxp.com/docs/en/data-sheet/NX20P0407.pdf - page 3
Hi Heiko, On 2/26/25 2:27 PM, Heiko Stübner wrote: > Am Mittwoch, 26. Februar 2025, 14:17:37 MEZ schrieb Quentin Schulz: >> Hi Heiko, >> >> On 2/26/25 11:25 AM, 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. >>> >> >> We're not limiting the mode to host-mode anymore in v3. >> >> I tested and indeed peripheral mode doesn't work. While my ACM gadget >> device is created, I cannot communicate with it. >> >> Maybe reword in the commit log that only host mode is supported even >> though we don't enforce it? >> >> The USB2-only issue I experienced in v2 has been fixed with: >> https://lore.kernel.org/linux-rockchip/20250226103810.3746018-1-heiko@sntech.de/T/#t >> >> Tested-by: Quentin Schulz <quentin.schulz@cherry.de> >> >> See below for further comments. >> >>> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> >>> --- >>> changes in v3: >>> - more review comments from Quentin >>> (sbu-pin pinctrl, comments) >>> changes in v2: >>> - address review comments from Quentin >>> (comments, pinctrl, sbu-gpios and much more) >>> >>> .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 218 ++++++++++++++++++ >>> 1 file changed, 218 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts >>> index 20b566d4168f..5dbcdf67f0a5 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts >> [...] >>> @@ -483,6 +583,26 @@ pcie30x4_waken_m0: pcie30x4-waken-m0 { >>> rockchip,pins = <0 RK_PC7 12 &pcfg_pull_none>; >>> }; >>> }; >>> + >>> + usb3 { >>> + cc_int1: cc-int1 { >>> + rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>; >>> + }; >>> + >>> + cc_int2: cc-int2 { >>> + rockchip,pins = <4 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>; >>> + }; >>> + >>> + typec0_sbu_pins: typec0-sbu-pins { >> >> Please rename to typec<x>_sbu_dc_pins as they aren't SBU pins, they are >> pins for DC coupling of SBU as far as I could tell from the DT binding. >> >>> + rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>, >>> + <1 RK_PC3 RK_FUNC_GPIO &pcfg_pull_none>; >>> + }; >>> + >>> + typec1_sbu_pins: typec1-sbu-pins { >>> + rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>, >>> + <1 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>; >>> + }; >> >> We're the first ones to declare a pinmux/pinconf for the SBU-DC pins and >> I'm not too sure if we should let them "float" or not. The default >> pinconf for those pins is PD, so maybe we should keep that PD. (For C1 >> they are PU). >> >> Rock 5 ITX routes the SBU-DC pins to GPIOs whose pinconf defaults to PU. >> CM3588 from FriendlyElec, Orange Pi 5, Orange Pi 5 Plus and NanoPC-T6 >> use GPIOs whose pinconf defaults to PD. >> >> I don't see external HW PU/PD in our or their designs so I would >> recommend to respect the default pinconf and put PD for the sbu-dc pins >> for USB-C0 and PU for USB-C1? > > But if you're worried about behaviour wrt. floating, having them pulled in > different direction for typec0 and typec1 also wouldn't result in differing > behaviour? > That's fair. I just really don't know what they are supposed to be when not driven :) > Also the pins are output-only, so the phy will always set them in some way? > That's true, there would only be a small time frame between the pinctrl setting the pinconf and the USBDP PHY driver deactivating them (logical low) since that's done as part of the probe of the device. I assume it really doesn't matter which state they are before being driven because nothing from the USB/DP stack is being run at that point? > But now you made me looks things up ;-) > > For the TS3USBCA4 USB Type-C SBU Multiplexer [0], the sbu pins on it are > described as "This pin has an internal nominally 1.6-MΩ pull-down resistor." > > In the block-diagram of the NX20P0407 Protection IC [1], it also looks like > a pull down is the config of choice. > Yeah but we have neither of those on our design, the SBU are directly connected to the USB-C jack without any IC in-between. So I wouldn't trust what one IC defines as internal resistor? So I guess, fine for the no-PU/PD pinconf, but I would really appreciate the renaming of the pinmux to include _dc in it and specify in the commit log that peripheral mode doesn't work yet. With that, Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks! Quentin
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts index 20b566d4168f..5dbcdf67f0a5 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts @@ -333,6 +333,56 @@ rtc_twi: rtc@6f { }; }; + typec-portc@22 { + compatible = "fcs,fusb302"; + reg = <0x22>; + interrupt-parent = <&gpio4>; + interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&cc_int1>; + vbus-supply = <&vcc_5v0_usb_c1>; + + connector { + compatible = "usb-c-connector"; + data-role = "dual"; + label = "USBC-1 P11"; + power-role = "source"; + self-powered; + source-pdos = + <PDO_FIXED(5000, 1500, PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)>; + vbus-supply = <&vcc_5v0_usb_c1>; + + 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 +444,56 @@ &i2c8 { pinctrl-0 = <&i2c8m2_xfer>; status = "okay"; + typec-portc@22 { + compatible = "fcs,fusb302"; + reg = <0x22>; + interrupt-parent = <&gpio4>; + interrupts = <RK_PA4 IRQ_TYPE_LEVEL_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&cc_int2>; + vbus-supply = <&vcc_5v0_usb_c2>; + + connector { + compatible = "usb-c-connector"; + data-role = "dual"; + label = "USBC-2 P12"; + power-role = "source"; + self-powered; + source-pdos = + <PDO_FIXED(5000, 1500, PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)>; + vbus-supply = <&vcc_5v0_usb_c2>; + + 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>; @@ -483,6 +583,26 @@ pcie30x4_waken_m0: pcie30x4-waken-m0 { rockchip,pins = <0 RK_PC7 12 &pcfg_pull_none>; }; }; + + usb3 { + cc_int1: cc-int1 { + rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>; + }; + + cc_int2: cc-int2 { + rockchip,pins = <4 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>; + }; + + typec0_sbu_pins: typec0-sbu-pins { + rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>, + <1 RK_PC3 RK_FUNC_GPIO &pcfg_pull_none>; + }; + + typec1_sbu_pins: typec1-sbu-pins { + rockchip,pins = <0 RK_PD4 RK_FUNC_GPIO &pcfg_pull_none>, + <1 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>; + }; + }; }; &saradc { @@ -850,6 +970,24 @@ &tsadc { status = "okay"; }; +/* USB-C P11 connector */ +&u2phy0 { + status = "okay"; +}; + +&u2phy0_otg { + status = "okay"; +}; + +/* USB-C P12 connector */ +&u2phy1 { + status = "okay"; +}; + +&u2phy1_otg { + status = "okay"; +}; + &u2phy2 { status = "okay"; }; @@ -892,6 +1030,56 @@ &uart7 { status = "okay"; }; +/* Type-C on P11 */ +&usbdp_phy0 { + orientation-switch; + pinctrl-names = "default"; + pinctrl-0 = <&typec0_sbu_pins>; + 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>; + }; + }; +}; + +/* Type-C on P12 */ +&usbdp_phy1 { + orientation-switch; + pinctrl-names = "default"; + pinctrl-0 = <&typec1_sbu_pins>; + 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>; + }; + }; +}; + /* host0 on P10 USB-A */ &usb_host0_ehci { status = "okay"; @@ -902,6 +1090,36 @@ &usb_host0_ohci { status = "okay"; }; +/* host0 on P11 USB-C */ +&usb_host0_xhci { + usb-role-switch; + status = "okay"; + + port { + #address-cells = <1>; + #size-cells = <0>; + + usb_host0_xhci_drd_sw: endpoint { + remote-endpoint = <&usbc0_hs>; + }; + }; +}; + +/* host1 on P12 USB-C */ +&usb_host1_xhci { + usb-role-switch; + 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";