| Message ID | 20241023080912.15349-1-macpaul.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] arm64: dts: mediatek: mt8395-genio-1200-evk: add support for TCPC port | expand |
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. > + }; > + > + 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 {
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 { >
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@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 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
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"; + power-role = "dual"; + try-power-role = "sink"; + 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>; + }; + + 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 { + 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>; + usb-role-switch; vusb33-supply = <&mt6359_vusb_ldo_reg>; status = "okay"; + + port { + ssusb_ep: endpoint { + remote-endpoint = <&mt6360_ssusb_ep>; + }; + }; }; &ssusb2 {