[PATCH RFT] arm64: dts: qcom: sc7280: drop incorrect EUD port on SoC side

[PATCH RFT] arm64: dts: qcom: sc7280: drop incorrect EUD port on SoC side

[Krzysztof Kozlowski]

Qualcomm Embedded USB Debugger (EUD) second port should point to Type-C
USB connector.  Such connector was defined directly in root node of
sc7280.dtsi which is clearly wrong.  SC7280 is a chip, so physically it
does not have USB Type-C port.  The connector is usually accessible
through some USB switch or controller.

Correct the EUD/USB connector topology by removing the top-level fake
USB connector and adding appropriate ports in boards having actual USB
Type-C connector defined (Herobrine, IDP).  All other boards will have
this EUD port missing.

This fixes also dtbs_check warnings:

  sc7280-herobrine-crd.dtb: connector: ports:port@0: 'reg' is a required property

Fixes: 9ee402ccfeb1 ("arm64: dts: qcom: sc7280: Fix EUD dt node syntax")
Cc: Bhupesh Sharma <bhupesh.sharma@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Not tested on hardware.
---
 .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 15 +++++++++++++
 .../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 15 +++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi          | 21 +------------------
 3 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 9ea6636125ad..2a4f239c5632 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -573,6 +573,12 @@ usb_c0: connector@0 {
 				power-role = "dual";
 				data-role = "host";
 				try-power-role = "source";
+
+				port {
+					usb_c0_ep: endpoint {
+						remote-endpoint = <&eud_con>;
+					};
+				};
 			};
 
 			usb_c1: connector@1 {
@@ -590,6 +596,15 @@ usb_c1: connector@1 {
 #include <arm/cros-ec-keyboard.dtsi>
 #include <arm/cros-ec-sbs.dtsi>
 
+&eud_ports {
+	port@1 {
+		reg = <1>;
+		eud_con: endpoint {
+			remote-endpoint = <&usb_c0_ep>;
+		};
+	};
+};
+
 &keyboard_controller {
 	function-row-physmap = <
 		MATRIX_KEY(0x00, 0x02, 0)	/* T1 */
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
index ebae545c587c..ffc469431340 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
@@ -44,6 +44,12 @@ usb_c0: connector@0 {
 				power-role = "dual";
 				data-role = "host";
 				try-power-role = "source";
+
+				port {
+					usb_c0_ep: endpoint {
+						remote-endpoint = <&eud_con>;
+					};
+				};
 			};
 
 			usb_c1: connector@1 {
@@ -78,6 +84,15 @@ cr50: tpm@0 {
 	};
 };
 
+&eud_ports {
+	port@1 {
+		reg = <1>;
+		eud_con: endpoint {
+			remote-endpoint = <&usb_c0_ep>;
+		};
+	};
+};
+
 &tlmm {
 	ap_ec_int_l: ap-ec-int-l-state {
 		pins = "gpio18";
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 925428a5f6ae..af9bb2ebcaa1 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -649,18 +649,6 @@ cpu7_opp_3014mhz: opp-3014400000 {
 		};
 	};
 
-	eud_typec: connector {
-		compatible = "usb-c-connector";
-
-		ports {
-			port@0 {
-				con_eud: endpoint {
-					remote-endpoint = <&eud_con>;
-				};
-			};
-		};
-	};
-
 	memory@80000000 {
 		device_type = "memory";
 		/* We expect the bootloader to fill in the size */
@@ -3624,7 +3612,7 @@ eud: eud@88e0000 {
 			      <0 0x88e2000 0 0x1000>;
 			interrupts-extended = <&pdc 11 IRQ_TYPE_LEVEL_HIGH>;
 
-			ports {
+			eud_ports: ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
 
@@ -3634,13 +3622,6 @@ eud_ep: endpoint {
 						remote-endpoint = <&usb2_role_switch>;
 					};
 				};
-
-				port@1 {
-					reg = <1>;
-					eud_con: endpoint {
-						remote-endpoint = <&con_eud>;
-					};
-				};
 			};
 		};
 
-- 
2.34.1

Re: [PATCH RFT] arm64: dts: qcom: sc7280: drop incorrect EUD port on SoC side

[Doug Anderson]

Hi,

On Tue, Jul 25, 2023 at 1:46 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Qualcomm Embedded USB Debugger (EUD) second port should point to Type-C
> USB connector.  Such connector was defined directly in root node of
> sc7280.dtsi which is clearly wrong.  SC7280 is a chip, so physically it
> does not have USB Type-C port.  The connector is usually accessible
> through some USB switch or controller.
>
> Correct the EUD/USB connector topology by removing the top-level fake
> USB connector and adding appropriate ports in boards having actual USB
> Type-C connector defined (Herobrine, IDP).  All other boards will have
> this EUD port missing.
>
> This fixes also dtbs_check warnings:
>
>   sc7280-herobrine-crd.dtb: connector: ports:port@0: 'reg' is a required property
>
> Fixes: 9ee402ccfeb1 ("arm64: dts: qcom: sc7280: Fix EUD dt node syntax")
> Cc: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Not tested on hardware.
> ---
>  .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 15 +++++++++++++
>  .../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 15 +++++++++++++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi          | 21 +------------------
>  3 files changed, 31 insertions(+), 20 deletions(-)

FWIW, I've always been very intrigued about the embedded USB port but
never managed to find any way to get it actually enabled. :( ...so I'm
probably not the best person to actually review this. That being said:

1. I'm nearly certain that this is completely unusable on herobrine
boards. Specifically on herobrine there's a USB hub between the SoC
and all the physical ports on the device and (I think?) that prevents
EUD from working. It is possible that hoglin/zoglin is an exception
here and Qualcomm might have some backdoor way to access EUD on these
devices since this is hardware that they built.

2. I've always been pretty baffled about the sc7280 EUD stuff since
the device tree shows the EUD on "usb_2". For some background: there
are two USB controllers on sc7280. There's "usb_1" which is USB
2.0/3.0 capable and, at an SoC level, is the "Type C" port.
Specifically the pins on the SoC for the USB 3.0 signals are the same
pins on the SoC as two of the DisplayPort lanes. Then there's "usb_2"
which is USB 2.0 only. If you'll notice, "usb_2" is not set to status
"okay" on any boards except "sc7280-idp.dts".

I asked Qualcomm at least a few times in the past if the EUD is truly
on the USB 2.0 port (which means it isn't connected to anything on
herobrine boards) or if it's actually on the "type C" port (which
means there's a hub in between) and never got a ton of clarify...

Given how baffling everything is, I wouldn't be opposed to just
deleting the EUD from the device tree until there is more clarity
here. If you don't want to just delete it, at least I'd say that it
shouldn't be hooked up for herobrine.

-Doug

Re: [PATCH RFT] arm64: dts: qcom: sc7280: drop incorrect EUD port on SoC side

[Krzysztof Kozlowski]

On 25/07/2023 21:35, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jul 25, 2023 at 1:46 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> Qualcomm Embedded USB Debugger (EUD) second port should point to Type-C
>> USB connector.  Such connector was defined directly in root node of
>> sc7280.dtsi which is clearly wrong.  SC7280 is a chip, so physically it
>> does not have USB Type-C port.  The connector is usually accessible
>> through some USB switch or controller.
>>
>> Correct the EUD/USB connector topology by removing the top-level fake
>> USB connector and adding appropriate ports in boards having actual USB
>> Type-C connector defined (Herobrine, IDP).  All other boards will have
>> this EUD port missing.
>>
>> This fixes also dtbs_check warnings:
>>
>>   sc7280-herobrine-crd.dtb: connector: ports:port@0: 'reg' is a required property
>>
>> Fixes: 9ee402ccfeb1 ("arm64: dts: qcom: sc7280: Fix EUD dt node syntax")
>> Cc: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Not tested on hardware.
>> ---
>>  .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 15 +++++++++++++
>>  .../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 15 +++++++++++++
>>  arch/arm64/boot/dts/qcom/sc7280.dtsi          | 21 +------------------
>>  3 files changed, 31 insertions(+), 20 deletions(-)
> 
> FWIW, I've always been very intrigued about the embedded USB port but
> never managed to find any way to get it actually enabled. :( ...so I'm
> probably not the best person to actually review this. That being said:
> 
> 1. I'm nearly certain that this is completely unusable on herobrine
> boards. Specifically on herobrine there's a USB hub between the SoC
> and all the physical ports on the device and (I think?) that prevents
> EUD from working. It is possible that hoglin/zoglin is an exception
> here and Qualcomm might have some backdoor way to access EUD on these
> devices since this is hardware that they built.
> 
> 2. I've always been pretty baffled about the sc7280 EUD stuff since
> the device tree shows the EUD on "usb_2". For some background: there
> are two USB controllers on sc7280. There's "usb_1" which is USB
> 2.0/3.0 capable and, at an SoC level, is the "Type C" port.
> Specifically the pins on the SoC for the USB 3.0 signals are the same
> pins on the SoC as two of the DisplayPort lanes. Then there's "usb_2"
> which is USB 2.0 only. If you'll notice, "usb_2" is not set to status
> "okay" on any boards except "sc7280-idp.dts".
> 
> I asked Qualcomm at least a few times in the past if the EUD is truly
> on the USB 2.0 port (which means it isn't connected to anything on
> herobrine boards) or if it's actually on the "type C" port (which
> means there's a hub in between) and never got a ton of clarify...
> 
> Given how baffling everything is, I wouldn't be opposed to just
> deleting the EUD from the device tree until there is more clarity
> here. If you don't want to just delete it, at least I'd say that it
> shouldn't be hooked up for herobrine.
> 

Thanks Doug. I forgot to Cc the original author of the code - Souradeep
- but anyway disabling incomplete node seems reasonable.

Best regards,
Krzysztof