| 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
| ||
|
Message-ID: <6cac5ab1-0b76-4f28-b8e1-72760464b3d1@cherry.de>
Date: Tue, 18 Feb 2025 12:24:38 +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, Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH] arm64: dts: rockchip: add usb typec host support to
rk3588-jaguar
Hi Heiko,
On 2/13/25 5:30 PM, 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.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@...rry.de>
> ---
> .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 178 ++++++++++++++++++
> 1 file changed, 178 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> index 90f823b2c219..329d98011c60 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> @@ -333,6 +333,52 @@ rtc_twi: rtc@6f {
> };
> };
>
> + usb-typec@22 {
We have a mix of node names in the Rockchip tree, some call it usb-typec
some call it typec-portc, including the device tree binding.
> + compatible = "fcs,fusb302";
> + reg = <0x22>;
> + interrupt-parent = <&gpio4>;
> + interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>;
Should we have a pinmux for the interrupt line in GPIO mode maybe?
> + vbus-supply = <&vcc_5v0_usb_c1>;
> +
> + connector {
> + compatible = "usb-c-connector";
Reading the binding, I'm wondering if we shouldn't set self-powered
property in there as well? Jaguar cannot be powered (or at least wasn't
designed for being powered) via USB-C and I think self-powered means
that? Not sure to be honest.
> + data-role = "dual";
> + label = "USBC-1 P11";
> + power-role = "source";
> + source-pdos =
> + <PDO_FIXED(5000, 1500, PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)>;
> +
Should we have vbus-supply = <&vcc_5v0_usb_c1>; here too?
> + 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 +440,52 @@ &i2c8 {
> pinctrl-0 = <&i2c8m2_xfer>;
> status = "okay";
>
> + usb-typec@22 {
All the same remarks as for P11 above.
> + compatible = "fcs,fusb302";
> + reg = <0x22>;
> + interrupt-parent = <&gpio4>;
> + interrupts = <RK_PA4 IRQ_TYPE_LEVEL_LOW>;
> + vbus-supply = <&vcc_5v0_usb_c2>;
> +
> + connector {
> + compatible = "usb-c-connector";
> + data-role = "dual";
> + label = "USBC-2 P12";
> + power-role = "source";
> + source-pdos =
> + <PDO_FIXED(5000, 1500, PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)>;
> +
> + 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>;
> @@ -851,6 +943,24 @@ &tsadc {
> status = "okay";
> };
>
Please add a comment here that this is for USB-C P11 connector so it
gets easier to figure out what's for what.
> +&u2phy0 {
> + status = "okay";
> +};
> +
> +&u2phy0_otg {
> + phy-supply = <&vcc_5v0_usb_c1>;
This is a bit confusing at we have the OTG port needing to specify the
VBUS supply on the port, while the FUSB also specifies it and the
usb-c-connector node can as well.
> + status = "okay";
> +};
> +
Comment for USB-C P12 connector.
> +&u2phy1 {
> + status = "okay";
> +};
> +
> +&u2phy1_otg {
> + phy-supply = <&vcc_5v0_usb_c2>;
> + status = "okay";
> +};
> +
> &u2phy2 {
> status = "okay";
> };
> @@ -893,6 +1003,46 @@ &uart7 {
> status = "okay";
> };
>
Comment for USB-C P11 connector.
> +&usbdp_phy0 {
> + orientation-switch;
It seems like we have SBU1 and SBU2 GPIOs as well. So I guess we want
something like:
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>;
> + };
Something's wrong with the dt-binding here as it only lists one possible
port, for the orientation.
> + };
> +};
> +
> +&usbdp_phy1 {
> + orientation-switch;
It seems like we have SBU1 and SBU2 GPIOs as well. So I guess we want
something like:
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>;
> + };
Something's wrong with the dt-binding here as it only lists one possible
port, for the orientation.
> + };
> +};
> +
> /* host0 on P10 USB-A */
> &usb_host0_ehci {
> status = "okay";
> @@ -903,6 +1053,34 @@ &usb_host0_ohci {
> status = "okay";
> };
>
Comment for USB-C P11 connector.
> +&usb_host0_xhci {
Add a comment for highlighting it supports DRD, just that we aren't
ready to support it just yet.
> + dr_mode = "host";
> + status = "okay";
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + usb_host0_xhci_drd_sw: endpoint {
> + remote-endpoint = <&usbc0_hs>;
> + };
Does this actually make sense without usb-role-switch; set? The binding
seems to indicate port is only useful when that is set.
> + };
> +};
> +
Comment for USB-C P12 connector.
> +&usb_host1_xhci {
Add a comment for highlighting it supports DRD, just that we aren't
ready to support it just yet.
> + dr_mode = "host";
> + status = "okay";
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + usb_host1_xhci_drd_sw: endpoint {
> + remote-endpoint = <&usbc1_hs>;
> + };
Does this actually make sense without usb-role-switch; set? The binding
seems to indicate port is only useful when that is set.
Cheers,
Quentin
Powered by blists - more mailing lists