Re: [PATCH v3] arm64: dts: rockchip: add usb typec host support to rk3588-jaguar
From: Quentin Schulz
Date: Wed Feb 26 2025 - 09:31:42 EST
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@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.
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@xxxxxxxxx/T/#t
Tested-by: Quentin Schulz <quentin.schulz@xxxxxxxxx>
See below for further comments.
Signed-off-by: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>
---
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@xxxxxxxxx>
Thanks!
Quentin