| 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: <60b490d2-30a1-dbff-36c4-992466991d49@mediatek.com>
Date: Thu, 24 Oct 2024 15:44:16 +0800
From: Macpaul Lin <macpaul.lin@...iatek.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.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
On 10/23/24 20:00, AngeloGioacchino Del Regno wrote:
> 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
Okay, will fix it.
>>> + 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)
Okay, it has been complained by checkpatch about this line is too long.
Will fix it in next version.
>> 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?
This port should be able to do both HighSpeed and SuperSpeed.
However our internal reference code set SuperSpeed at port@0 and seems
just work for both high and supoer speeds.
See more detail discussion below.
>>
>> 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.
Ya. That's the question I was wondering, too.
Current DT bindings of MTU3 support only 1 port which is also a required
property. For the SSUSB0 port on MT8195, it indeed 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>;
> }
> };
> };
> };
Sure, but after adding that DT schema before the v3 patch has been send.
The dt_binding_check seems show 'port' is a required property for MTU3.
However I didn't found where it has been defined.
I think to add the 'ports' property in MTU3's DT Schema should be better.
> 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.
Oh! Will fix it.
>> 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";
This port0 is exactly a dual-role port for genio-1200-evk.
It is usually used as gadget device port (like ADB) and
sometimes can do OTG function with xhci0.
>> Cheers,
>> Angelo
>>
>>> + usb-role-switch;
>>> vusb33-supply = <&mt6359_vusb_ldo_reg>;
>>> status = "okay";
>>> +
>>> + port {
>>> + ssusb_ep: endpoint {
>>> + remote-endpoint = <&mt6360_ssusb_ep>;
>>> + };
>>> + };
>>> };
>>> &ssusb2 {
>>
Thanks
Macpaul Lin
Powered by blists - more mailing lists