diff mbox series

[6/7] arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph

Message ID 20230425034010.3789376-7-quic_bjorande@quicinc.com
State New
Headers
Series phy: qcom-qmp-combo: Support orientation switching |

Commit Message

Bjorn Andersson April 25, 2023, 3:40 a.m. UTC 
  With support for the QMP combo phy to react to USB Type-C switch events,
introduce it as the next hop for the SuperSpeed lanes of the two USB
Type-C connectors, and connect the output of the DisplayPort controller
to the QMP combo phy.

This allows the TCPM to perform orientation switching of both USB and
DisplayPort signals.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 34 +++++++++++++++++++++++
 2 files changed, 58 insertions(+), 4 deletions(-)

Comments

Konrad Dybcio April 26, 2023, 11:33 p.m. UTC | #1 
On 4/25/23 04:40, Bjorn Andersson wrote:
> With support for the QMP combo phy to react to USB Type-C switch events,
> introduce it as the next hop for the SuperSpeed lanes of the two USB
> Type-C connectors, and connect the output of the DisplayPort controller
> to the QMP combo phy.
>
> This allows the TCPM to perform orientation switching of both USB and
> DisplayPort signals.
>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 34 +++++++++++++++++++++++
>   2 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 547277924ea3..33c973661fa5 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -64,7 +64,7 @@ port@1 {
>   					reg = <1>;
>   
>   					pmic_glink_con0_ss: endpoint {
> -						remote-endpoint = <&mdss0_dp0_out>;
> +						remote-endpoint = <&usb_0_qmpphy_out>;
>   					};
>   				};
>   
> @@ -99,7 +99,7 @@ port@1 {
>   					reg = <1>;
>   
>   					pmic_glink_con1_ss: endpoint {
> -						remote-endpoint = <&mdss0_dp1_out>;
> +						remote-endpoint = <&usb_1_qmpphy_out>;
>   					};
>   				};
>   
> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>   
>   &mdss0_dp0_out {
>   	data-lanes = <0 1>;
> -	remote-endpoint = <&pmic_glink_con0_ss>;
> +	remote-endpoint = <&usb_0_qmpphy_dp_in>;
>   };
>   
>   &mdss0_dp1 {
> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>   
>   &mdss0_dp1_out {
>   	data-lanes = <0 1>;
> -	remote-endpoint = <&pmic_glink_con1_ss>;
> +	remote-endpoint = <&usb_1_qmpphy_dp_in>;
>   };
>   
>   &mdss0_dp3 {
> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
>   	vdda-phy-supply = <&vreg_l9d>;
>   	vdda-pll-supply = <&vreg_l4d>;
>   
> +	orientation-switch;

I believe this belongs in the SoC DTSI, as it's supported by
the PHY block itself


The rest seems to lgtm..


On a note, why did we end up placing pmic_glink in device
DTs? It's already assumed that we're using the full Qualcomm
stack as we use PAS for remoteprocs so I *think* we can always
assume pmic_glink would be there!

Konrad

> +
>   	status = "okay";
>   };
>   
> +&usb_0_qmpphy_dp_in {
> +	remote-endpoint = <&mdss0_dp0_out>;
> +};
> +
> +&usb_0_qmpphy_out {
> +	remote-endpoint = <&pmic_glink_con0_ss>;
> +};
> +
>   &usb_0_role_switch {
>   	remote-endpoint = <&pmic_glink_con0_hs>;
>   };
> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
>   	vdda-phy-supply = <&vreg_l4b>;
>   	vdda-pll-supply = <&vreg_l3b>;
>   
> +	orientation-switch;
> +
>   	status = "okay";
>   };
>   
> +&usb_1_qmpphy_dp_in {
> +	remote-endpoint = <&mdss0_dp1_out>;
> +};
> +
> +&usb_1_qmpphy_out {
> +	remote-endpoint = <&pmic_glink_con1_ss>;
> +};
> +
>   &usb_1_role_switch {
>   	remote-endpoint = <&pmic_glink_con1_hs>;
>   };
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 0e691bb0120c..1eb3a295e8fa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
>   			#phy-cells = <1>;
>   
>   			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					usb_0_qmpphy_out: endpoint {};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					usb_0_qmpphy_dp_in: endpoint {};
> +				};
> +			};
>   		};
>   
>   		usb_1_hsphy: phy@8902000 {
> @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
>   			#phy-cells = <1>;
>   
>   			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					usb_1_qmpphy_out: endpoint {};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					usb_1_qmpphy_dp_in: endpoint {};
> +				};
> +			};
>   		};
>   
>   		mdss1_dp0_phy: phy@8909a00 {
Neil Armstrong April 27, 2023, 1:27 p.m. UTC | #2 
On 27/04/2023 01:33, Konrad Dybcio wrote:
> 
> On 4/25/23 04:40, Bjorn Andersson wrote:
>> With support for the QMP combo phy to react to USB Type-C switch events,
>> introduce it as the next hop for the SuperSpeed lanes of the two USB
>> Type-C connectors, and connect the output of the DisplayPort controller
>> to the QMP combo phy.
>>
>> This allows the TCPM to perform orientation switching of both USB and
>> DisplayPort signals.
>>
>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
>>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 34 +++++++++++++++++++++++
>>   2 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>> index 547277924ea3..33c973661fa5 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>> @@ -64,7 +64,7 @@ port@1 {
>>                       reg = <1>;
>>                       pmic_glink_con0_ss: endpoint {
>> -                        remote-endpoint = <&mdss0_dp0_out>;
>> +                        remote-endpoint = <&usb_0_qmpphy_out>;
>>                       };
>>                   };
>> @@ -99,7 +99,7 @@ port@1 {
>>                       reg = <1>;
>>                       pmic_glink_con1_ss: endpoint {
>> -                        remote-endpoint = <&mdss0_dp1_out>;
>> +                        remote-endpoint = <&usb_1_qmpphy_out>;
>>                       };
>>                   };
>> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>>   &mdss0_dp0_out {
>>       data-lanes = <0 1>;
>> -    remote-endpoint = <&pmic_glink_con0_ss>;
>> +    remote-endpoint = <&usb_0_qmpphy_dp_in>;
>>   };
>>   &mdss0_dp1 {
>> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>>   &mdss0_dp1_out {
>>       data-lanes = <0 1>;
>> -    remote-endpoint = <&pmic_glink_con1_ss>;
>> +    remote-endpoint = <&usb_1_qmpphy_dp_in>;
>>   };
>>   &mdss0_dp3 {
>> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
>>       vdda-phy-supply = <&vreg_l9d>;
>>       vdda-pll-supply = <&vreg_l4d>;
>> +    orientation-switch;
> 
> I believe this belongs in the SoC DTSI, as it's supported by
> the PHY block itself
> 
> 
> The rest seems to lgtm..
> 
> 
> On a note, why did we end up placing pmic_glink in device
> DTs? It's already assumed that we're using the full Qualcomm
> stack as we use PAS for remoteprocs so I *think* we can always
> assume pmic_glink would be there!

As we did on other board, I think because having pmic_glink depends
on the board firmware capabilities ? Boards without USB-C won't need/have
pmic_link right ?

Neil

> 
> Konrad
> 
>> +
>>       status = "okay";
>>   };
>> +&usb_0_qmpphy_dp_in {
>> +    remote-endpoint = <&mdss0_dp0_out>;
>> +};
>> +
>> +&usb_0_qmpphy_out {
>> +    remote-endpoint = <&pmic_glink_con0_ss>;
>> +};
>> +
>>   &usb_0_role_switch {
>>       remote-endpoint = <&pmic_glink_con0_hs>;
>>   };
>> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
>>       vdda-phy-supply = <&vreg_l4b>;
>>       vdda-pll-supply = <&vreg_l3b>;
>> +    orientation-switch;
>> +
>>       status = "okay";
>>   };
>> +&usb_1_qmpphy_dp_in {
>> +    remote-endpoint = <&mdss0_dp1_out>;
>> +};
>> +
>> +&usb_1_qmpphy_out {
>> +    remote-endpoint = <&pmic_glink_con1_ss>;
>> +};
>> +
>>   &usb_1_role_switch {
>>       remote-endpoint = <&pmic_glink_con1_hs>;
>>   };
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index 0e691bb0120c..1eb3a295e8fa 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
>>               #phy-cells = <1>;
>>               status = "disabled";
>> +
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                port@0 {
>> +                    reg = <0>;
>> +
>> +                    usb_0_qmpphy_out: endpoint {};
>> +                };
>> +
>> +                port@1 {
>> +                    reg = <1>;
>> +
>> +                    usb_0_qmpphy_dp_in: endpoint {};
>> +                };
>> +            };
>>           };
>>           usb_1_hsphy: phy@8902000 {
>> @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
>>               #phy-cells = <1>;
>>               status = "disabled";
>> +
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                port@0 {
>> +                    reg = <0>;
>> +
>> +                    usb_1_qmpphy_out: endpoint {};
>> +                };
>> +
>> +                port@1 {
>> +                    reg = <1>;
>> +
>> +                    usb_1_qmpphy_dp_in: endpoint {};
>> +                };
>> +            };
>>           };
>>           mdss1_dp0_phy: phy@8909a00 {
Bjorn Andersson April 27, 2023, 7:48 p.m. UTC | #3 
On Thu, Apr 27, 2023 at 12:33:44AM +0100, Konrad Dybcio wrote:
> 
> On 4/25/23 04:40, Bjorn Andersson wrote:
> > With support for the QMP combo phy to react to USB Type-C switch events,
> > introduce it as the next hop for the SuperSpeed lanes of the two USB
> > Type-C connectors, and connect the output of the DisplayPort controller
> > to the QMP combo phy.
> > 
> > This allows the TCPM to perform orientation switching of both USB and
> > DisplayPort signals.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >   arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
> >   arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 34 +++++++++++++++++++++++
> >   2 files changed, 58 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > index 547277924ea3..33c973661fa5 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > @@ -64,7 +64,7 @@ port@1 {
> >   					reg = <1>;
> >   					pmic_glink_con0_ss: endpoint {
> > -						remote-endpoint = <&mdss0_dp0_out>;
> > +						remote-endpoint = <&usb_0_qmpphy_out>;
> >   					};
> >   				};
> > @@ -99,7 +99,7 @@ port@1 {
> >   					reg = <1>;
> >   					pmic_glink_con1_ss: endpoint {
> > -						remote-endpoint = <&mdss0_dp1_out>;
> > +						remote-endpoint = <&usb_1_qmpphy_out>;
> >   					};
> >   				};
> > @@ -412,7 +412,7 @@ &mdss0_dp0 {
> >   &mdss0_dp0_out {
> >   	data-lanes = <0 1>;
> > -	remote-endpoint = <&pmic_glink_con0_ss>;
> > +	remote-endpoint = <&usb_0_qmpphy_dp_in>;
> >   };
> >   &mdss0_dp1 {
> > @@ -421,7 +421,7 @@ &mdss0_dp1 {
> >   &mdss0_dp1_out {
> >   	data-lanes = <0 1>;
> > -	remote-endpoint = <&pmic_glink_con1_ss>;
> > +	remote-endpoint = <&usb_1_qmpphy_dp_in>;
> >   };
> >   &mdss0_dp3 {
> > @@ -670,9 +670,19 @@ &usb_0_qmpphy {
> >   	vdda-phy-supply = <&vreg_l9d>;
> >   	vdda-pll-supply = <&vreg_l4d>;
> > +	orientation-switch;
> 
> I believe this belongs in the SoC DTSI, as it's supported by
> the PHY block itself
> 

Sounds reasonable to me, will move it.

> 
> The rest seems to lgtm..
> 
> 
> On a note, why did we end up placing pmic_glink in device
> DTs? It's already assumed that we're using the full Qualcomm
> stack as we use PAS for remoteprocs so I *think* we can always
> assume pmic_glink would be there!
> 

In this particular case, sa8295p and sa8540p are derived from sc8280xp
dtsi, but those does not implement pmic_glink...
In most other cases I think your expectation would hold though.

Perhaps would be suitable to put the pmic_glink in sc8280xp.dtsi but
disable it, and then add connectors in the device.

Regards,
Bjorn

> Konrad
> 
> > +
> >   	status = "okay";
> >   };
> > +&usb_0_qmpphy_dp_in {
> > +	remote-endpoint = <&mdss0_dp0_out>;
> > +};
> > +
> > +&usb_0_qmpphy_out {
> > +	remote-endpoint = <&pmic_glink_con0_ss>;
> > +};
> > +
> >   &usb_0_role_switch {
> >   	remote-endpoint = <&pmic_glink_con0_hs>;
> >   };
> > @@ -697,9 +707,19 @@ &usb_1_qmpphy {
> >   	vdda-phy-supply = <&vreg_l4b>;
> >   	vdda-pll-supply = <&vreg_l3b>;
> > +	orientation-switch;
> > +
> >   	status = "okay";
> >   };
> > +&usb_1_qmpphy_dp_in {
> > +	remote-endpoint = <&mdss0_dp1_out>;
> > +};
> > +
> > +&usb_1_qmpphy_out {
> > +	remote-endpoint = <&pmic_glink_con1_ss>;
> > +};
> > +
> >   &usb_1_role_switch {
> >   	remote-endpoint = <&pmic_glink_con1_hs>;
> >   };
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index 0e691bb0120c..1eb3a295e8fa 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
> >   			#phy-cells = <1>;
> >   			status = "disabled";
> > +
> > +			ports {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				port@0 {
> > +					reg = <0>;
> > +
> > +					usb_0_qmpphy_out: endpoint {};
> > +				};
> > +
> > +				port@1 {
> > +					reg = <1>;
> > +
> > +					usb_0_qmpphy_dp_in: endpoint {};
> > +				};
> > +			};
> >   		};
> >   		usb_1_hsphy: phy@8902000 {
> > @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
> >   			#phy-cells = <1>;
> >   			status = "disabled";
> > +
> > +			ports {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				port@0 {
> > +					reg = <0>;
> > +
> > +					usb_1_qmpphy_out: endpoint {};
> > +				};
> > +
> > +				port@1 {
> > +					reg = <1>;
> > +
> > +					usb_1_qmpphy_dp_in: endpoint {};
> > +				};
> > +			};
> >   		};
> >   		mdss1_dp0_phy: phy@8909a00 {
Konrad Dybcio May 2, 2023, 11:03 a.m. UTC | #4 
On 27.04.2023 15:27, Neil Armstrong wrote:
> On 27/04/2023 01:33, Konrad Dybcio wrote:
>>
>> On 4/25/23 04:40, Bjorn Andersson wrote:
>>> With support for the QMP combo phy to react to USB Type-C switch events,
>>> introduce it as the next hop for the SuperSpeed lanes of the two USB
>>> Type-C connectors, and connect the output of the DisplayPort controller
>>> to the QMP combo phy.
>>>
>>> This allows the TCPM to perform orientation switching of both USB and
>>> DisplayPort signals.
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
>>>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 34 +++++++++++++++++++++++
>>>   2 files changed, 58 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>>> index 547277924ea3..33c973661fa5 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>>> @@ -64,7 +64,7 @@ port@1 {
>>>                       reg = <1>;
>>>                       pmic_glink_con0_ss: endpoint {
>>> -                        remote-endpoint = <&mdss0_dp0_out>;
>>> +                        remote-endpoint = <&usb_0_qmpphy_out>;
>>>                       };
>>>                   };
>>> @@ -99,7 +99,7 @@ port@1 {
>>>                       reg = <1>;
>>>                       pmic_glink_con1_ss: endpoint {
>>> -                        remote-endpoint = <&mdss0_dp1_out>;
>>> +                        remote-endpoint = <&usb_1_qmpphy_out>;
>>>                       };
>>>                   };
>>> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>>>   &mdss0_dp0_out {
>>>       data-lanes = <0 1>;
>>> -    remote-endpoint = <&pmic_glink_con0_ss>;
>>> +    remote-endpoint = <&usb_0_qmpphy_dp_in>;
>>>   };
>>>   &mdss0_dp1 {
>>> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>>>   &mdss0_dp1_out {
>>>       data-lanes = <0 1>;
>>> -    remote-endpoint = <&pmic_glink_con1_ss>;
>>> +    remote-endpoint = <&usb_1_qmpphy_dp_in>;
>>>   };
>>>   &mdss0_dp3 {
>>> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
>>>       vdda-phy-supply = <&vreg_l9d>;
>>>       vdda-pll-supply = <&vreg_l4d>;
>>> +    orientation-switch;
>>
>> I believe this belongs in the SoC DTSI, as it's supported by
>> the PHY block itself
>>
>>
>> The rest seems to lgtm..
>>
>>
>> On a note, why did we end up placing pmic_glink in device
>> DTs? It's already assumed that we're using the full Qualcomm
>> stack as we use PAS for remoteprocs so I *think* we can always
>> assume pmic_glink would be there!
> 
> As we did on other board, I think because having pmic_glink depends
> on the board firmware capabilities ? Boards without USB-C won't need/have
> pmic_link right ?
PMIC_GLINK takes care of USB-C, but also enables battmgr and friends

Konrad
> 
> Neil
> 
>>
>> Konrad
>>
>>> +
>>>       status = "okay";
>>>   };
>>> +&usb_0_qmpphy_dp_in {
>>> +    remote-endpoint = <&mdss0_dp0_out>;
>>> +};
>>> +
>>> +&usb_0_qmpphy_out {
>>> +    remote-endpoint = <&pmic_glink_con0_ss>;
>>> +};
>>> +
>>>   &usb_0_role_switch {
>>>       remote-endpoint = <&pmic_glink_con0_hs>;
>>>   };
>>> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
>>>       vdda-phy-supply = <&vreg_l4b>;
>>>       vdda-pll-supply = <&vreg_l3b>;
>>> +    orientation-switch;
>>> +
>>>       status = "okay";
>>>   };
>>> +&usb_1_qmpphy_dp_in {
>>> +    remote-endpoint = <&mdss0_dp1_out>;
>>> +};
>>> +
>>> +&usb_1_qmpphy_out {
>>> +    remote-endpoint = <&pmic_glink_con1_ss>;
>>> +};
>>> +
>>>   &usb_1_role_switch {
>>>       remote-endpoint = <&pmic_glink_con1_hs>;
>>>   };
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> index 0e691bb0120c..1eb3a295e8fa 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
>>>               #phy-cells = <1>;
>>>               status = "disabled";
>>> +
>>> +            ports {
>>> +                #address-cells = <1>;
>>> +                #size-cells = <0>;
>>> +
>>> +                port@0 {
>>> +                    reg = <0>;
>>> +
>>> +                    usb_0_qmpphy_out: endpoint {};
>>> +                };
>>> +
>>> +                port@1 {
>>> +                    reg = <1>;
>>> +
>>> +                    usb_0_qmpphy_dp_in: endpoint {};
>>> +                };
>>> +            };
>>>           };
>>>           usb_1_hsphy: phy@8902000 {
>>> @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
>>>               #phy-cells = <1>;
>>>               status = "disabled";
>>> +
>>> +            ports {
>>> +                #address-cells = <1>;
>>> +                #size-cells = <0>;
>>> +
>>> +                port@0 {
>>> +                    reg = <0>;
>>> +
>>> +                    usb_1_qmpphy_out: endpoint {};
>>> +                };
>>> +
>>> +                port@1 {
>>> +                    reg = <1>;
>>> +
>>> +                    usb_1_qmpphy_dp_in: endpoint {};
>>> +                };
>>> +            };
>>>           };
>>>           mdss1_dp0_phy: phy@8909a00 {
>
Johan Hovold May 2, 2023, 12:22 p.m. UTC | #5 
On Mon, Apr 24, 2023 at 08:40:09PM -0700, Bjorn Andersson wrote:
> With support for the QMP combo phy to react to USB Type-C switch events,
> introduce it as the next hop for the SuperSpeed lanes of the two USB
> Type-C connectors, and connect the output of the DisplayPort controller
> to the QMP combo phy.
> 
> This allows the TCPM to perform orientation switching of both USB and
> DisplayPort signals.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi    | 34 +++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 547277924ea3..33c973661fa5 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -64,7 +64,7 @@ port@1 {
>  					reg = <1>;
>  
>  					pmic_glink_con0_ss: endpoint {
> -						remote-endpoint = <&mdss0_dp0_out>;
> +						remote-endpoint = <&usb_0_qmpphy_out>;
>  					};
>  				};
>  
> @@ -99,7 +99,7 @@ port@1 {
>  					reg = <1>;
>  
>  					pmic_glink_con1_ss: endpoint {
> -						remote-endpoint = <&mdss0_dp1_out>;
> +						remote-endpoint = <&usb_1_qmpphy_out>;
>  					};
>  				};
>  
> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>  
>  &mdss0_dp0_out {
>  	data-lanes = <0 1>;
> -	remote-endpoint = <&pmic_glink_con0_ss>;
> +	remote-endpoint = <&usb_0_qmpphy_dp_in>;
>  };

It's a bit hard to follow what going on when using place holder nodes
from the dtsi like this (instead of describing all the ports directly in
the board dts). IIRC we went a bit back and forth over this earlier and
we already use this scheme for the display port controllers, so I guess
this is the price we pay for being consistent.

>  &mdss0_dp1 {
> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>  
>  &mdss0_dp1_out {
>  	data-lanes = <0 1>;
> -	remote-endpoint = <&pmic_glink_con1_ss>;
> +	remote-endpoint = <&usb_1_qmpphy_dp_in>;
>  };
>  
>  &mdss0_dp3 {
> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
>  	vdda-phy-supply = <&vreg_l9d>;
>  	vdda-pll-supply = <&vreg_l4d>;
>  
> +	orientation-switch;
> +
>  	status = "okay";
>  };
>  
> +&usb_0_qmpphy_dp_in {
> +	remote-endpoint = <&mdss0_dp0_out>;
> +};
> +
> +&usb_0_qmpphy_out {
> +	remote-endpoint = <&pmic_glink_con0_ss>;
> +};
> +
>  &usb_0_role_switch {
>  	remote-endpoint = <&pmic_glink_con0_hs>;
>  };
> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
>  	vdda-phy-supply = <&vreg_l4b>;
>  	vdda-pll-supply = <&vreg_l3b>;
>  
> +	orientation-switch;
> +
>  	status = "okay";
>  };
>  
> +&usb_1_qmpphy_dp_in {
> +	remote-endpoint = <&mdss0_dp1_out>;
> +};
> +
> +&usb_1_qmpphy_out {
> +	remote-endpoint = <&pmic_glink_con1_ss>;
> +};
> +
>  &usb_1_role_switch {
>  	remote-endpoint = <&pmic_glink_con1_hs>;
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 0e691bb0120c..1eb3a295e8fa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
>  			#phy-cells = <1>;
>  
>  			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					usb_0_qmpphy_out: endpoint {};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					usb_0_qmpphy_dp_in: endpoint {};
> +				};
> +			};
>  		};

The binding describes three ports, where dp-in is port 2.

Perhaps you don't need to describe ss-in yet, but shouldn't the port
numbers match? Should some of these be described as required in the
binding?

Johan
Bjorn Andersson May 4, 2023, 3:07 a.m. UTC | #6 
On Tue, May 02, 2023 at 02:22:22PM +0200, Johan Hovold wrote:
> On Mon, Apr 24, 2023 at 08:40:09PM -0700, Bjorn Andersson wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
[..]
> >  &mdss0_dp0_out {
> >  	data-lanes = <0 1>;
> > -	remote-endpoint = <&pmic_glink_con0_ss>;
> > +	remote-endpoint = <&usb_0_qmpphy_dp_in>;
> >  };
> 
> It's a bit hard to follow what going on when using place holder nodes
> from the dtsi like this (instead of describing all the ports directly in
> the board dts). IIRC we went a bit back and forth over this earlier and
> we already use this scheme for the display port controllers, so I guess
> this is the price we pay for being consistent.
> 

I agree, this is why I argued in favour of keeping the of_graphs
together in a single node. But as long as we label things appropriately
it's pretty ok - and the alternative would be yet another undocumented
"rule".

So let's stick with this...

> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
[..]
> > +			ports {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				port@0 {
> > +					reg = <0>;
> > +
> > +					usb_0_qmpphy_out: endpoint {};
> > +				};
> > +
> > +				port@1 {
> > +					reg = <1>;
> > +
> > +					usb_0_qmpphy_dp_in: endpoint {};
> > +				};
> > +			};
> >  		};
> 
> The binding describes three ports, where dp-in is port 2.
> 
> Perhaps you don't need to describe ss-in yet, but shouldn't the port
> numbers match? Should some of these be described as required in the
> binding?
> 

This should certainly be port@2, thanks for spotting.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 547277924ea3..33c973661fa5 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -64,7 +64,7 @@  port@1 {
 					reg = <1>;
 
 					pmic_glink_con0_ss: endpoint {
-						remote-endpoint = <&mdss0_dp0_out>;
+						remote-endpoint = <&usb_0_qmpphy_out>;
 					};
 				};
 
@@ -99,7 +99,7 @@  port@1 {
 					reg = <1>;
 
 					pmic_glink_con1_ss: endpoint {
-						remote-endpoint = <&mdss0_dp1_out>;
+						remote-endpoint = <&usb_1_qmpphy_out>;
 					};
 				};
 
@@ -412,7 +412,7 @@  &mdss0_dp0 {
 
 &mdss0_dp0_out {
 	data-lanes = <0 1>;
-	remote-endpoint = <&pmic_glink_con0_ss>;
+	remote-endpoint = <&usb_0_qmpphy_dp_in>;
 };
 
 &mdss0_dp1 {
@@ -421,7 +421,7 @@  &mdss0_dp1 {
 
 &mdss0_dp1_out {
 	data-lanes = <0 1>;
-	remote-endpoint = <&pmic_glink_con1_ss>;
+	remote-endpoint = <&usb_1_qmpphy_dp_in>;
 };
 
 &mdss0_dp3 {
@@ -670,9 +670,19 @@  &usb_0_qmpphy {
 	vdda-phy-supply = <&vreg_l9d>;
 	vdda-pll-supply = <&vreg_l4d>;
 
+	orientation-switch;
+
 	status = "okay";
 };
 
+&usb_0_qmpphy_dp_in {
+	remote-endpoint = <&mdss0_dp0_out>;
+};
+
+&usb_0_qmpphy_out {
+	remote-endpoint = <&pmic_glink_con0_ss>;
+};
+
 &usb_0_role_switch {
 	remote-endpoint = <&pmic_glink_con0_hs>;
 };
@@ -697,9 +707,19 @@  &usb_1_qmpphy {
 	vdda-phy-supply = <&vreg_l4b>;
 	vdda-pll-supply = <&vreg_l3b>;
 
+	orientation-switch;
+
 	status = "okay";
 };
 
+&usb_1_qmpphy_dp_in {
+	remote-endpoint = <&mdss0_dp1_out>;
+};
+
+&usb_1_qmpphy_out {
+	remote-endpoint = <&pmic_glink_con1_ss>;
+};
+
 &usb_1_role_switch {
 	remote-endpoint = <&pmic_glink_con1_hs>;
 };
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 0e691bb0120c..1eb3a295e8fa 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3006,6 +3006,23 @@  usb_0_qmpphy: phy@88eb000 {
 			#phy-cells = <1>;
 
 			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					usb_0_qmpphy_out: endpoint {};
+				};
+
+				port@1 {
+					reg = <1>;
+
+					usb_0_qmpphy_dp_in: endpoint {};
+				};
+			};
 		};
 
 		usb_1_hsphy: phy@8902000 {
@@ -3042,6 +3059,23 @@  usb_1_qmpphy: phy@8903000 {
 			#phy-cells = <1>;
 
 			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					usb_1_qmpphy_out: endpoint {};
+				};
+
+				port@1 {
+					reg = <1>;
+
+					usb_1_qmpphy_dp_in: endpoint {};
+				};
+			};
 		};
 
 		mdss1_dp0_phy: phy@8909a00 {