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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3119048.xgJ6IN8ObU@diego>
Date: Wed, 26 Feb 2025 14:27:49 +0100
From: Heiko Stübner <heiko@...ech.de>
To: Quentin Schulz <quentin.schulz@...rry.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

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?

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