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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxx>

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@xxxxxxxxxxxx>
Signed-off-by: Yow-Shin Liou <yow-shin.liou@xxxxxxxxxxxx>
Signed-off-by: Simon Sun <simon.sun@xxxxxxxxxxxxxxx>
Signed-off-by: Macpaul Lin <macpaul.lin@xxxxxxxxxxxx>
---
  .../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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux