| 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: <ba44b369-bcd7-4226-b75a-f3c5000e97ec@collabora.com>
Date: Wed, 23 Oct 2024 14:00:04 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Macpaul Lin <macpaul.lin@...iatek.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
Alexandre Mergnat <amergnat@...libre.com>
Cc: Bear Wang <bear.wang@...iatek.com>, Pablo Sun <pablo.sun@...iatek.com>,
Macpaul Lin <macpaul@...il.com>,
Project_Global_Chrome_Upstream_Group@...iatek.com,
linux-usb@...r.kernel.org, Chris-qj chen <chris-qj.chen@...iatek.com>,
Fabien Parent <fparent@...libre.com>,
Yow-Shin Liou <yow-shin.liou@...iatek.com>,
Simon Sun <simon.sun@...jingtech.com>
Subject: Re: [PATCH v3 1/2] arm64: dts: mediatek: mt8395-genio-1200-evk: add
support for TCPC port
Il 23/10/24 13:07, AngeloGioacchino Del Regno ha scritto:
> Il 23/10/24 10:09, Macpaul Lin ha scritto:
>> From: Fabien Parent <fparent@...libre.com>
>>
>> Enable USB Type-C support on MediaTek MT8395 Genio 1200 EVK by adding
>> configuration for TCPC Port, USB-C connector, and related settings.
>>
>> Configure dual role switch capability, set up PD (Power Delivery) profiles,
>> and establish endpoints for SSUSB (SuperSpeed USB).
>>
>> Update pinctrl configurations for U3 P0 VBus default pins and set dr_mode
>> to "otg" for OTG (On-The-Go) mode operation.
>>
>> Signed-off-by: Fabien Parent <fparent@...libre.com>
>> Signed-off-by: Yow-Shin Liou <yow-shin.liou@...iatek.com>
>> Signed-off-by: Simon Sun <simon.sun@...jingtech.com>
>> Signed-off-by: Macpaul Lin <macpaul.lin@...iatek.com>
>> ---
>> .../dts/mediatek/mt8395-genio-1200-evk.dts | 54 +++++++++++++++++++
>> 1 file changed, 54 insertions(+)
>>
>> Changes for v2:
>> - Drop the no need '1/2' DT Schema update patch in the 1st version.
>> - Fix intent for 'ports' node, it should under the 'connector' node.
>> - Correct the index for 'port@0' and 'port@1' node.
>>
>> Changes for v3:
>> - Correct the order between new added nodes.
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8395-genio-1200-evk.dts b/arch/arm64/
>> boot/dts/mediatek/mt8395-genio-1200-evk.dts
>> index 5f16fb820580..83d520226302 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8395-genio-1200-evk.dts
>> +++ b/arch/arm64/boot/dts/mediatek/mt8395-genio-1200-evk.dts
>> @@ -335,6 +335,43 @@ mt6360_ldo7: ldo7 {
>> regulator-always-on;
>> };
>> };
>> +
>> + tcpc {
>> + compatible = "mediatek,mt6360-tcpc";
>> + interrupts-extended = <&pio 17 IRQ_TYPE_LEVEL_LOW>;
>> + interrupt-names = "PD_IRQB";
>> +
>> + connector {
>> + compatible = "usb-c-connector";
>> + label = "USB-C";
>> + data-role = "dual";
>
> op-sink-microwatt goes here
>
>> + power-role = "dual";
>> + try-power-role = "sink";
>> + source-pdos = <PDO_FIXED(5000, 1000, \
>> + PDO_FIXED_DUAL_ROLE | \
>> + PDO_FIXED_DATA_SWAP)>;
>
> Please fix the indentation (and also you don't need the escaping)
>
> source-pdos = <PDO_FIXED(5000, 1000,
> PDO_FIXED_DUAL_ROLE |
> PDO_FIXED_DATA_SWAP)>;
>
>> + sink-pdos = <PDO_FIXED(5000, 2000, \
>> + PDO_FIXED_DUAL_ROLE | \
>> + PDO_FIXED_DATA_SWAP)>;
>> + op-sink-microwatt = <10000000>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@0 {
>> + reg = <0>;
>
> Just to make sure that this is ok: are you sure that this port supports
> SuperSpeed (physical connector too) and that it's not limited to HighSpeed?
>
> I have seen Rob's comment stating that ssusb_ep goes to port@1, but I think
> that his comment came after reading "ss" in "ssusb": while the controller
> does surely support SS, if the port does not, this should still go to port@0.
>
> P.S.: I didn't check the schematics - just please make sure it's correct, and
> that this actually works.
>
Sorry for the double email, but I've done some fast research on this as something
came to mind right after sending this review.
usb-connector.yaml says that the `ports` property models a data bus to the USB
connector, and that a single connector can have multiple data buses, of course.
The last statement doesn't come as a surprise, and actually makes me think about
what MTU3 actually provides: a High Speed data bus, and a SuperSpeed data bus.
Now, I see in MTU3 that only `port` is allowed and that only the HS part is
is exposed... so to resolve this, you want to add a binding to connect the
data bus of the MTU3 controller to the usb-c-connector, and obviously only then
you should use it here.
That should look like this:
mediatek,mtu3.yaml:
ports:
$ref: /schemas/graph.yaml#/properties/ports
properties:
port@0:
$ref: /schemas/graph.yaml#/properties/port
description: High Speed (HS) data bus.
port@1:
$ref: /schemas/graph.yaml#/properties/port
description: Super Speed (SS) data bus.
some_devicetree.dts
&ssusb0 {
ports {
port@0 {
reg = <0>;
/* High Speed data bus */
mtu3_hs0_ep: endpoint {
/* connects to port@0 (HS) of usb-c-connector */
remote-endpoint = <&typec_con_hs>;
}
};
port@1 {
reg = <1>;
/* SuperSpeed data bus */
mtu3_ss0_ep: endpoint {
/* connects to port@1 (SS) of usb-c-connector */
remote-endpoint = <&typec_con_sss>;
}
};
};
};
I don't have time to do any testing in this precise moment, so, if you want
to go on and test - cool; otherwise, I can check that at a later time, but
on Genio 700 EVK instead (which is the same thing for this specific topic anyway).
Cheers,
Angelo
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> + mt6360_ssusb_ep: endpoint {
>> + remote-endpoint = <&ssusb_ep>;
>> + };
>> + };
>> + };
>> + };
>> + };
>> };
>> };
>> @@ -770,6 +807,13 @@ pins-reset {
>> };
>> };
>> + u3_p0_vbus: u3-p0-vbus-default-pins {
>> + pins-cmd-dat {
>
> That's not a command nor data pin.
>
> pins-vbus {
>
>> + pinmux = <PINMUX_GPIO63__FUNC_VBUSVALID>;
>> + input-enable;
>> + };
>> + };
>> +
>> uart0_pins: uart0-pins {
>> pins {
>> pinmux = <PINMUX_GPIO98__FUNC_UTXD0>,
>> @@ -900,8 +944,18 @@ &ufsphy {
>> };
>> &ssusb0 {
>> + dr_mode = "otg";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&u3_p0_vbus>;
>
> Is this port usb host by default? If it is:
>
> role-switch-default-mode = "host";
>
> Cheers,
> Angelo
>
>> + usb-role-switch;
>> vusb33-supply = <&mt6359_vusb_ldo_reg>;
>> status = "okay";
>> +
>> + port {
>> + ssusb_ep: endpoint {
>> + remote-endpoint = <&mt6360_ssusb_ep>;
>> + };
>> + };
>> };
>> &ssusb2 {
>
Powered by blists - more mailing lists