| 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: <9087FE0C88CD8335+4505c82a-33dd-45cf-941d-aa818068f19c@radxa.com>
Date: Wed, 11 Dec 2024 09:04:37 +0900
From: FUKAUMI Naoki <naoki@...xa.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Heiko Stuebner <heiko@...ech.de>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, linux-rockchip@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v1 1/1] arm64: dts: rockchip: Add USB-C support to ROCK 5B
Hi Sebastian,
On 12/11/24 08:08, Sebastian Reichel wrote:
> Hello Naoki,
>
> On Wed, Dec 11, 2024 at 07:10:55AM +0900, FUKAUMI Naoki wrote:
>> Hi Sebastian,
>>
>> Thank you very much for your work!
>>
>> $ cat /sys/class/power_supply/tcpm-source-psy-4-0022/{current_max,current_now,online,type,usb_type,voltage_max,voltage_min,voltage_now}
>> 1500000
>> 1500000
>> 1
>> USB
>> C [PD] PD_PPS
>> 20000000
>> 20000000
>> 20000000
>>
>> $ cat /sys/class/power_supply/tcpm-source-psy-4-0022/{current_max,current_now,online,type,usb_type,voltage_max,voltage_min,voltage_now}
>> 5000000
>> 5000000
>> 1
>> USB
>> C PD [PD_PPS]
>> 20000000
>> 20000000
>> 20000000
>>
>> $ ls /sys/class/udc/
>> fc000000.usb
>>
>> I can configure it as CDC-NCM and host detects it.
>> But I could not use it as a HOST port. How to use it?
>
> You can switch between host and peripheral for Type-C ports like
> this depending on the remote sides capabilities:
>
> * echo host > /sys/class/typec/<port>/data_role
> * echo device > /sys/class/typec/<port>/data_role
thanks!
I tested both data_role and both orientation. all works.
> I tested this with a USB-C hub connected to the port, which works
> in host mode.
>
>> some minor nitpick is below:
>>
>> On 12/11/24 01:36, Sebastian Reichel wrote:
>>> Add hardware description for the USB-C port in the Radxa Rock 5 Model B.
>>> This describes the OHCI, EHCI and XHCI USB parts, but not yet the
>>> DisplayPort AltMode (bindings are not yet upstream).
>>>
>>> The fusb302 node is marked with status "fail", since the board is usually
>>> powered through the USB-C port. Handling of errors can result in hard
>>> resets, which removed the bus power for some time resulting in a board
>>> reset.
>>>
>>> The main problem is that devices are supposed to interact with the
>>> power-supply within 5 seconds after the plug event according to the
>>> USB PD specification. This is more or less impossible to achieve when
>>> the kernel is the first software communicating with the power-supply.
>>>
>>> Recent U-Boot (v2025.01) will start doing USB-PD communication, which
>>> solves this issue. Upstream U-Boot doing USB-PD communication will also
>>> set the fusb302 node status to "okay". That way booting a kernel with
>>> the updated DT on an old U-Boot avoids a reset loop.
>>>
>>> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
>>> ---
>>> .../boot/dts/rockchip/rk3588-rock-5b.dts | 121 ++++++++++++++++++
>>> 1 file changed, 121 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> index d597112f1d5b..cb5990df6ccb 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> @@ -5,6 +5,7 @@
>>> #include <dt-bindings/gpio/gpio.h>
>>> #include <dt-bindings/leds/common.h>
>>> #include <dt-bindings/soc/rockchip,vop2.h>
>>> +#include <dt-bindings/usb/pd.h>
>>> #include "rk3588.dtsi"
>>> / {
>>> @@ -84,6 +85,15 @@ rfkill-bt {
>>> shutdown-gpios = <&gpio3 RK_PD5 GPIO_ACTIVE_HIGH>;
>>> };
>>> + vcc12v_dcin: regulator-vcc12v-dcin {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "vcc12v_dcin";
>>
>> typec_vin by schematic.
>
> Ack. Will update in v2.
>
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + regulator-min-microvolt = <12000000>;
>>> + regulator-max-microvolt = <12000000>;
both microvolt line can be removed.
>>> + };
>>> +
>>> vcc3v3_pcie2x1l0: regulator-vcc3v3-pcie2x1l0 {
>>> compatible = "regulator-fixed";
>>> enable-active-high;
>>> @@ -142,6 +152,7 @@ vcc5v0_sys: regulator-vcc5v0-sys {
>>> regulator-boot-on;
>>> regulator-min-microvolt = <5000000>;
>>> regulator-max-microvolt = <5000000>;
>>> + vin-supply = <&vcc12v_dcin>;
>>
>> typec_vin.
>>
>>> };
>>> vcc_1v1_nldo_s3: regulator-vcc-1v1-nldo-s3 {
>>> @@ -264,6 +275,67 @@ regulator-state-mem {
>>> };
>>> };
>>> +&i2c4 {
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&i2c4m1_xfer>;
>>> + status = "okay";
>>> +
>>> + usbc0: usb-typec@22 {
>>
>> Is "usbc0" label necessary?
>
> no, but does it hurt?
sorry, please keep it.
>>> + compatible = "fcs,fusb302";
>>> + reg = <0x22>;
>>> + interrupt-parent = <&gpio3>;
>>> + interrupts = <RK_PB4 IRQ_TYPE_LEVEL_LOW>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&usbc0_int>;
>>
>> cc_int_l by schematic.
>
> Ack. I intentionally switched away from this naming, since cc prefix
> is imho a way worse prefix than usbc0. The _l suffix just means
> active low, which is already encoded in DT.
>
> But I don't have a strong opinion and can fix this in v2.
>
>>> + vbus-supply = <&vcc12v_dcin>;
>>
>> typec_vin
>>
>>> + /*
>>> + * When the board is starting to send power-delivery messages
>>> + * too late (5 seconds according to the specification), the
>>> + * power-supply reacts with a hard-reset. That removes the
>>> + * power from VBUS for some time, which resets te whole board.
>>
>> ... resets "the" whole board.
>
> Ack.
>
>>
>>> + */
>>> + status = "fail";
>>> +
>>> + usb_con: connector {
>>
>> Is "usb_con" label necessary?
>
> No. It should either be changed to "usbc0_con" or removed. In
> general I tend to add some labels when they might be needed by
> something in the future. They are more or less free anyways.
+1 for "usbc0_con".
Best regards,
--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.
> -- Sebastian
Powered by blists - more mailing lists