Re: [PATCH v3 1/2] arm64: dts: mediatek: mt8395-genio-1200-evk: add support for TCPC port

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@baylibre.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@baylibre.com>
>> Signed-off-by: Yow-Shin Liou <yow-shin.liou@mediatek.com>
>> Signed-off-by: Simon Sun <simon.sun@yunjingtech.com>
>> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.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 {
>