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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b793928e-8310-4bba-a9d1-50d30f898c17@cherry.de>
Date: Wed, 26 Feb 2025 15:30:26 +0100
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Heiko Stübner <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 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@...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?
> 
> 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@...rry.de>

Thanks!
Quentin