diff mbox series

[v6,12/13] arm64: dts: qcom: qrb5165-rb5: Switch on TCPM usb-role-switching for usb_1

Message ID 20230501121111.1058190-13-bryan.odonoghue@linaro.org
State Superseded
Headers show
Series [v6,01/13] dt-bindings: regulator: qcom,usb-vbus-regulator: Mark reg as required | expand

Commit Message

Bryan O'Donoghue May 1, 2023, 12:11 p.m. UTC
Switch on usb-role-switching for usb_1 via TCPM. We need to declare
usb-role-switch in &usb_1 and associate with the remote-endpoint in TCPM
which provides the necessary signal.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 19 ++++++++++++++++++-
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  4 ++++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Konrad Dybcio May 2, 2023, 11 a.m. UTC | #1
On 1.05.2023 14:11, Bryan O'Donoghue wrote:
> Switch on usb-role-switching for usb_1 via TCPM. We need to declare
> usb-role-switch in &usb_1 and associate with the remote-endpoint in TCPM
> which provides the necessary signal.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 19 ++++++++++++++++++-
>  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  4 ++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> index 1e0b6fd59abc9..b5cc45358a474 100644
> --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> @@ -1273,7 +1273,13 @@ &usb_1 {
>  };
>  
>  &usb_1_dwc3 {
> -	dr_mode = "peripheral";
> +	dr_mode = "otg";
> +	usb-role-switch;
> +	port {
> +		dwc3_role_switch_in: endpoint {
Any reason you're redefining it? You can just do

&dwc3_role_switch_in {
	remote-endpoint = <&pm8150b_role_switch_out>;
};


> +			remote-endpoint = <&pm8150b_role_switch_out>;
> +		};
> +	};
>  };
>  
>  &usb_1_hsphy {
> @@ -1359,5 +1365,16 @@ connector {
>  					 PDO_FIXED_DUAL_ROLE |
>  					 PDO_FIXED_USB_COMM |
>  					 PDO_FIXED_DATA_SWAP)>;
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				pm8150b_role_switch_out: endpoint {
Similarly to the QMPPHY, the port definition can be moved to
the common node in the SoC DTSI

Konrad

> +					remote-endpoint = <&dwc3_role_switch_in>;
> +				};
> +			};
> +		};
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index af16d3ba76b8e..af988328db6b9 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -3740,6 +3740,10 @@ usb_1_dwc3: usb@a600000 {
>  				snps,dis_enblslpm_quirk;
>  				phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
>  				phy-names = "usb2-phy", "usb3-phy";
> +
> +				port {
> +					dwc3_role_switch_in: endpoint {};
> +				};
>  			};
>  		};
>
Bryan O'Donoghue May 2, 2023, 11:03 a.m. UTC | #2
On 02/05/2023 12:00, Konrad Dybcio wrote:
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				pm8150b_role_switch_out: endpoint {
> Similarly to the QMPPHY, the port definition can be moved to
> the common node in the SoC DTSI

But then the connector would have to be defined in the SoC dtsi and not 
all derivatives of SoC can be assumed to have a usb-c-connector.

grep "usb-c-connector" arch/arm64/boot/dts/qcom/*

---
bod
Konrad Dybcio May 2, 2023, 11:13 a.m. UTC | #3
On 2.05.2023 13:03, Bryan O'Donoghue wrote:
> On 02/05/2023 12:00, Konrad Dybcio wrote:
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            port@0 {
>>> +                reg = <0>;
>>> +                pm8150b_role_switch_out: endpoint {
>> Similarly to the QMPPHY, the port definition can be moved to
>> the common node in the SoC DTSI
> 
> But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector.
Not quite!

You can define an empty endpoint (like we do with DSI<->panel ones) and
fill it in on the device side.

Konrad
> 
> grep "usb-c-connector" arch/arm64/boot/dts/qcom/*
> 
> ---
> bod
Bryan O'Donoghue May 2, 2023, 11:16 a.m. UTC | #4
On 02/05/2023 12:13, Konrad Dybcio wrote:
> 
> 
> On 2.05.2023 13:03, Bryan O'Donoghue wrote:
>> On 02/05/2023 12:00, Konrad Dybcio wrote:
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <0>;
>>>> +
>>>> +            port@0 {
>>>> +                reg = <0>;
>>>> +                pm8150b_role_switch_out: endpoint {
>>> Similarly to the QMPPHY, the port definition can be moved to
>>> the common node in the SoC DTSI
>>
>> But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector.
> Not quite!
> 
> You can define an empty endpoint (like we do with DSI<->panel ones) and
> fill it in on the device side.

Sorry you're saying to define as an example the port here in the dtsi

&usb_1_dwc3 {
         dr_mode = "otg";
         usb-role-switch;
         port {
                 dwc3_role_switch_in: endpoint {
                         remote-endpoint = <&pm8150b_role_switch_out>;
                 };
         };
};

and to leave the reciprocal definition in the connector to the dts ?

&pm8150b_typec {

         connector {
                 compatible = "usb-c-connector";
                 ports {
                         #address-cells = <1>;
                         #size-cells = <0>;

                         port@0 {
                                 reg = <0>;
                                 pm8150b_role_switch_out: endpoint {
                                         remote-endpoint = 
<&dwc3_role_switch_in>;
                                 };
                         };
Konrad Dybcio May 2, 2023, 11:47 a.m. UTC | #5
On 2.05.2023 13:16, Bryan O'Donoghue wrote:
> On 02/05/2023 12:13, Konrad Dybcio wrote:
>>
>>
>> On 2.05.2023 13:03, Bryan O'Donoghue wrote:
>>> On 02/05/2023 12:00, Konrad Dybcio wrote:
>>>>> +            #address-cells = <1>;
>>>>> +            #size-cells = <0>;
>>>>> +
>>>>> +            port@0 {
>>>>> +                reg = <0>;
>>>>> +                pm8150b_role_switch_out: endpoint {
>>>> Similarly to the QMPPHY, the port definition can be moved to
>>>> the common node in the SoC DTSI
>>>
>>> But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector.
>> Not quite!
>>
>> You can define an empty endpoint (like we do with DSI<->panel ones) and
>> fill it in on the device side.
> 
> Sorry you're saying to define as an example the port here in the dtsi
> 
> &usb_1_dwc3 {
>         dr_mode = "otg";
>         usb-role-switch;

===
>         port {
>                 dwc3_role_switch_in: endpoint {
>                         remote-endpoint = <&pm8150b_role_switch_out>;
>                 };
>         };
===

this part (minus remote-endpoint) would go to SoC dtsi

remote-endpoint would be assigned on a per-device basis

> };
> 
> and to leave the reciprocal definition in the connector to the dts ?
> 
> &pm8150b_typec {
> 

====
>         connector {
>                 compatible = "usb-c-connector";
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         port@0 {
>                                 reg = <0>;
>                                 pm8150b_role_switch_out: endpoint {
>                                         remote-endpoint = <&dwc3_role_switch_in>;
>                                 };
>                         };
====

this part (minus remote-endpoint) would go to pm8150b.dtsi

remote-endpoint would be assigned (or left empty) on a per-device basis

Konrad
>
Bryan O'Donoghue May 2, 2023, 12:10 p.m. UTC | #6
On 02/05/2023 12:47, Konrad Dybcio wrote:
> 
> this part (minus remote-endpoint) would go to pm8150b.dtsi
> 
> remote-endpoint would be assigned (or left empty) on a per-device basis

Hmm.

Yes, actually I think I can locate the connector {} defintion in the 
pm8150b.dtsi.. you would need to have one tcpm {typec, pdphy} block per 
type-c port so.. yes

done

---
bod
Bryan O'Donoghue May 7, 2023, 9:09 p.m. UTC | #7
On 02/05/2023 12:47, Konrad Dybcio wrote:
>>          connector {
>>                  compatible = "usb-c-connector";
>>                  ports {
>>                          #address-cells = <1>;
>>                          #size-cells = <0>;
>>
>>                          port@0 {
>>                                  reg = <0>;
>>                                  pm8150b_role_switch_out: endpoint {
>>                                          remote-endpoint = <&dwc3_role_switch_in>;
>>                                  };
>>                          };
> ====

Hmm, I'm actually recoiling from doing this now that I look at it.

- Every other implementation defines the connector in the platfrom
   dts not the core dtsi

- The connector would have to be named something like
   pm8150b_type_connector which is not consistent with the
   rest of the Type-C connector namespace


Yes it can be done that the connector with the ports can be defined in 
the pm8150b dtsi but TBH I think it is more confusing that way and not 
consistent with other implementations.

I'm citing prior art here I just think this is nicer/easier to follow

- arch/arm64/boot/dts/qcom/sc8280xp.dtsi
- arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts

So for now

https://imgflip.com/i/7kw6ck

---
bod
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index 1e0b6fd59abc9..b5cc45358a474 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -1273,7 +1273,13 @@  &usb_1 {
 };
 
 &usb_1_dwc3 {
-	dr_mode = "peripheral";
+	dr_mode = "otg";
+	usb-role-switch;
+	port {
+		dwc3_role_switch_in: endpoint {
+			remote-endpoint = <&pm8150b_role_switch_out>;
+		};
+	};
 };
 
 &usb_1_hsphy {
@@ -1359,5 +1365,16 @@  connector {
 					 PDO_FIXED_DUAL_ROLE |
 					 PDO_FIXED_USB_COMM |
 					 PDO_FIXED_DATA_SWAP)>;
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				pm8150b_role_switch_out: endpoint {
+					remote-endpoint = <&dwc3_role_switch_in>;
+				};
+			};
+		};
 	};
 };
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index af16d3ba76b8e..af988328db6b9 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -3740,6 +3740,10 @@  usb_1_dwc3: usb@a600000 {
 				snps,dis_enblslpm_quirk;
 				phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
 				phy-names = "usb2-phy", "usb3-phy";
+
+				port {
+					dwc3_role_switch_in: endpoint {};
+				};
 			};
 		};