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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <28a16ca5-42f7-4aa1-9bc7-fc6b72364556@cherry.de>
Date: Wed, 26 Feb 2025 14:17:37 +0100
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Heiko Stuebner <heiko@...ech.de>
Cc: linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, lukasz.czechowski@...umatec.com,
 Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH v3] arm64: dts: rockchip: add usb typec host support to
 rk3588-jaguar

Hi Heiko,

On 2/26/25 11:25 AM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@...rry.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@...rry.de>

See below for further comments.

> Signed-off-by: Heiko Stuebner <heiko.stuebner@...rry.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