[PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Kuogee Hsieh]

Add DP device node on sc7180.

Changes in v2:
-- replace msm_dp with dp
-- replace dp_opp_table with opp_table

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  9 ++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi         | 78 ++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 24d293e..40367a2 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -786,6 +786,15 @@ hp_i2c: &i2c9 {
 	status = "okay";
 };
 
+&dp {
+        status = "okay";
+        pinctrl-names = "default";
+        pinctrl-0 = <&dp_hot_plug_det>;
+        data-lanes = <0 1>;
+        vdda-1p2-supply = <&vdda_usb_ss_dp_1p2>;
+        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
+};
+
 &pm6150_adc {
 	charger-thermistor@4f {
 		reg = <ADC5_AMUX_THM3_100K_PU>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6228ba2..05a4133 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3032,6 +3032,13 @@
 							remote-endpoint = <&dsi0_in>;
 						};
 					};
+
+					port@2 {
+						reg = <2>;
+						dpu_intf0_out: endpoint {
+							remote-endpoint = <&dp_in>;
+						};
+					};
 				};
 
 				mdp_opp_table: mdp-opp-table {
@@ -3148,6 +3155,77 @@
 
 				status = "disabled";
 			};
+
+			dp: displayport-controller@ae90000 {
+				compatible = "qcom,sc7180-dp";
+				status = "disabled";
+
+				reg = <0 0x0ae90000 0 0x1400>;
+
+				interrupt-parent = <&mdss>;
+				interrupts = <12>;
+
+				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
+					 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
+					 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
+					 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
+					 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
+				clock-names = "core_iface", "core_aux", "ctrl_link",
+					      "ctrl_link_iface", "stream_pixel";
+				#clock-cells = <1>;
+				assigned-clocks = <&dispcc DISP_CC_MDSS_DP_LINK_CLK_SRC>,
+						  <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
+				assigned-clock-parents = <&dp_phy 0>, <&dp_phy 1>;
+				phys = <&dp_phy>;
+				phy-names = "dp";
+
+				operating-points-v2 = <&opp_table>;
+				power-domains = <&rpmhpd SC7180_CX>;
+
+				#sound-dai-cells = <0>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					port@0 {
+						reg = <0>;
+						dp_in: endpoint {
+							remote-endpoint = <&dpu_intf0_out>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						dp_out: endpoint { };
+					};
+				};
+
+				opp_table: dp-opp-table {
+					compatible = "operating-points-v2";
+
+					opp-160000000 {
+						opp-hz = /bits/ 64 <160000000>;
+						required-opps = <&rpmhpd_opp_low_svs>;
+					};
+
+					opp-270000000 {
+						opp-hz = /bits/ 64 <270000000>;
+						required-opps = <&rpmhpd_opp_svs>;
+					};
+
+					opp-540000000 {
+						opp-hz = /bits/ 64 <540000000>;
+						required-opps = <&rpmhpd_opp_svs_l1>;
+					};
+
+					opp-810000000 {
+						opp-hz = /bits/ 64 <810000000>;
+						required-opps = <&rpmhpd_opp_nom>;
+					};
+				};
+			};
+
+
 		};
 
 		dispcc: clock-controller@af00000 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:

> Add DP device node on sc7180.
> 
> Changes in v2:
> -- replace msm_dp with dp
> -- replace dp_opp_table with opp_table
> 

I'm sorry for those suggestions, I don't like either one of them.

And for everything but changes to the DRM code the changelog goes below
the --- line, so it's not part of the git history.

> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  9 ++++
>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 78 ++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 24d293e..40367a2 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -786,6 +786,15 @@ hp_i2c: &i2c9 {
>  	status = "okay";
>  };
>  
> +&dp {
> +        status = "okay";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&dp_hot_plug_det>;
> +        data-lanes = <0 1>;

Is it a limitation of the EC in Trogdor that you can only do 2 lanes?

> +        vdda-1p2-supply = <&vdda_usb_ss_dp_1p2>;
> +        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
> +};
> +
>  &pm6150_adc {
>  	charger-thermistor@4f {
>  		reg = <ADC5_AMUX_THM3_100K_PU>;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 6228ba2..05a4133 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -3032,6 +3032,13 @@
>  							remote-endpoint = <&dsi0_in>;
>  						};
>  					};
> +
> +					port@2 {
> +						reg = <2>;
> +						dpu_intf0_out: endpoint {
> +							remote-endpoint = <&dp_in>;
> +						};
> +					};
>  				};
>  
>  				mdp_opp_table: mdp-opp-table {
> @@ -3148,6 +3155,77 @@
>  
>  				status = "disabled";
>  			};
> +
> +			dp: displayport-controller@ae90000 {

If you label this "mdss_dp", then it will naturally group with other
mdss properties in trogdor.dtsi (which should be sorted alphabetically).

> +				compatible = "qcom,sc7180-dp";
> +				status = "disabled";
> +
> +				reg = <0 0x0ae90000 0 0x1400>;
> +
> +				interrupt-parent = <&mdss>;
> +				interrupts = <12>;
> +
> +				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +					 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
> +					 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
> +					 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +					 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
> +				clock-names = "core_iface", "core_aux", "ctrl_link",
> +					      "ctrl_link_iface", "stream_pixel";
> +				#clock-cells = <1>;
> +				assigned-clocks = <&dispcc DISP_CC_MDSS_DP_LINK_CLK_SRC>,
> +						  <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +				assigned-clock-parents = <&dp_phy 0>, <&dp_phy 1>;
> +				phys = <&dp_phy>;
> +				phy-names = "dp";
> +
> +				operating-points-v2 = <&opp_table>;
> +				power-domains = <&rpmhpd SC7180_CX>;

Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
mention CX here in order for the opp framework to apply required-opps
of CX?

> +
> +				#sound-dai-cells = <0>;
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					port@0 {
> +						reg = <0>;
> +						dp_in: endpoint {
> +							remote-endpoint = <&dpu_intf0_out>;
> +						};
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +						dp_out: endpoint { };
> +					};
> +				};
> +
> +				opp_table: dp-opp-table {

The one and only "opp_table" of the sc7180 :)
Maybe name it dp_opp_table instead?

Regards,
Bjorn

> +					compatible = "operating-points-v2";
> +
> +					opp-160000000 {
> +						opp-hz = /bits/ 64 <160000000>;
> +						required-opps = <&rpmhpd_opp_low_svs>;
> +					};
> +
> +					opp-270000000 {
> +						opp-hz = /bits/ 64 <270000000>;
> +						required-opps = <&rpmhpd_opp_svs>;
> +					};
> +
> +					opp-540000000 {
> +						opp-hz = /bits/ 64 <540000000>;
> +						required-opps = <&rpmhpd_opp_svs_l1>;
> +					};
> +
> +					opp-810000000 {
> +						opp-hz = /bits/ 64 <810000000>;
> +						required-opps = <&rpmhpd_opp_nom>;
> +					};
> +				};
> +			};
> +
> +
>  		};
>  
>  		dispcc: clock-controller@af00000 {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[khsieh]

On 2021-06-03 09:53, Bjorn Andersson wrote:
> On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> 
>> Add DP device node on sc7180.
>> 
>> Changes in v2:
>> -- replace msm_dp with dp
>> -- replace dp_opp_table with opp_table
>> 
> 
> I'm sorry for those suggestions, I don't like either one of them.
> 
> And for everything but changes to the DRM code the changelog goes below
> the --- line, so it's not part of the git history.
> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  9 ++++
>>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 78 
>> ++++++++++++++++++++++++++++
>>  2 files changed, 87 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index 24d293e..40367a2 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -786,6 +786,15 @@ hp_i2c: &i2c9 {
>>  	status = "okay";
>>  };
>> 
>> +&dp {
>> +        status = "okay";
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&dp_hot_plug_det>;
>> +        data-lanes = <0 1>;
> 
> Is it a limitation of the EC in Trogdor that you can only do 2 lanes?

yes,

> 
>> +        vdda-1p2-supply = <&vdda_usb_ss_dp_1p2>;
>> +        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
>> +};
>> +
>>  &pm6150_adc {
>>  	charger-thermistor@4f {
>>  		reg = <ADC5_AMUX_THM3_100K_PU>;
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 6228ba2..05a4133 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -3032,6 +3032,13 @@
>>  							remote-endpoint = <&dsi0_in>;
>>  						};
>>  					};
>> +
>> +					port@2 {
>> +						reg = <2>;
>> +						dpu_intf0_out: endpoint {
>> +							remote-endpoint = <&dp_in>;
>> +						};
>> +					};
>>  				};
>> 
>>  				mdp_opp_table: mdp-opp-table {
>> @@ -3148,6 +3155,77 @@
>> 
>>  				status = "disabled";
>>  			};
>> +
>> +			dp: displayport-controller@ae90000 {
> 
> If you label this "mdss_dp", then it will naturally group with other
> mdss properties in trogdor.dtsi (which should be sorted 
> alphabetically).
> 
>> +				compatible = "qcom,sc7180-dp";
>> +				status = "disabled";
>> +
>> +				reg = <0 0x0ae90000 0 0x1400>;
>> +
>> +				interrupt-parent = <&mdss>;
>> +				interrupts = <12>;
>> +
>> +				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
>> +				clock-names = "core_iface", "core_aux", "ctrl_link",
>> +					      "ctrl_link_iface", "stream_pixel";
>> +				#clock-cells = <1>;
>> +				assigned-clocks = <&dispcc DISP_CC_MDSS_DP_LINK_CLK_SRC>,
>> +						  <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +				assigned-clock-parents = <&dp_phy 0>, <&dp_phy 1>;
>> +				phys = <&dp_phy>;
>> +				phy-names = "dp";
>> +
>> +				operating-points-v2 = <&opp_table>;
>> +				power-domains = <&rpmhpd SC7180_CX>;
> 
> Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
> mention CX here in order for the opp framework to apply required-opps
> of CX?
> 
>> +
>> +				#sound-dai-cells = <0>;
>> +
>> +				ports {
>> +					#address-cells = <1>;
>> +					#size-cells = <0>;
>> +					port@0 {
>> +						reg = <0>;
>> +						dp_in: endpoint {
>> +							remote-endpoint = <&dpu_intf0_out>;
>> +						};
>> +					};
>> +
>> +					port@1 {
>> +						reg = <1>;
>> +						dp_out: endpoint { };
>> +					};
>> +				};
>> +
>> +				opp_table: dp-opp-table {
> 
> The one and only "opp_table" of the sc7180 :)
> Maybe name it dp_opp_table instead?
> 
> Regards,
> Bjorn
> 
>> +					compatible = "operating-points-v2";
>> +
>> +					opp-160000000 {
>> +						opp-hz = /bits/ 64 <160000000>;
>> +						required-opps = <&rpmhpd_opp_low_svs>;
>> +					};
>> +
>> +					opp-270000000 {
>> +						opp-hz = /bits/ 64 <270000000>;
>> +						required-opps = <&rpmhpd_opp_svs>;
>> +					};
>> +
>> +					opp-540000000 {
>> +						opp-hz = /bits/ 64 <540000000>;
>> +						required-opps = <&rpmhpd_opp_svs_l1>;
>> +					};
>> +
>> +					opp-810000000 {
>> +						opp-hz = /bits/ 64 <810000000>;
>> +						required-opps = <&rpmhpd_opp_nom>;
>> +					};
>> +				};
>> +			};
>> +
>> +
>>  		};
>> 
>>  		dispcc: clock-controller@af00000 {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-06-03 14:28:37)
> On 2021-06-03 09:53, Bjorn Andersson wrote:
> > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> >
> >> Add DP device node on sc7180.
> >>
> >> Changes in v2:
> >> -- replace msm_dp with dp
> >> -- replace dp_opp_table with opp_table
> >>
> >
> > I'm sorry for those suggestions, I don't like either one of them.
> >
> > And for everything but changes to the DRM code the changelog goes below
> > the --- line, so it's not part of the git history.
> >
> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> >> ---
> >>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  9 ++++
> >>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 78
> >> ++++++++++++++++++++++++++++
> >>  2 files changed, 87 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> >> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> >> index 24d293e..40367a2 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> >> @@ -786,6 +786,15 @@ hp_i2c: &i2c9 {
> >>      status = "okay";
> >>  };
> >>
> >> +&dp {
> >> +        status = "okay";
> >> +        pinctrl-names = "default";
> >> +        pinctrl-0 = <&dp_hot_plug_det>;
> >> +        data-lanes = <0 1>;
> >
> > Is it a limitation of the EC in Trogdor that you can only do 2 lanes?
>
> yes,
>

It's not an EC limitation. It's a hardware design decision. We have one
type-c PHY on the sc7180 SoC and we have two type-c ports on the board
so we have decided to only use two lanes for DP and two lanes for USB on
the type-c ports so that both type-c ports work all the time.

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[khsieh]

On 2021-06-03 09:53, Bjorn Andersson wrote:
> On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> 
>> Add DP device node on sc7180.
>> 
>> Changes in v2:
>> -- replace msm_dp with dp
>> -- replace dp_opp_table with opp_table
>> 
> 
> I'm sorry for those suggestions, I don't like either one of them.
> 
> And for everything but changes to the DRM code the changelog goes below
> the --- line, so it's not part of the git history.
> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |  9 ++++
>>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 78 
>> ++++++++++++++++++++++++++++
>>  2 files changed, 87 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index 24d293e..40367a2 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -786,6 +786,15 @@ hp_i2c: &i2c9 {
>>  	status = "okay";
>>  };
>> 
>> +&dp {
>> +        status = "okay";
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&dp_hot_plug_det>;
>> +        data-lanes = <0 1>;
> 
> Is it a limitation of the EC in Trogdor that you can only do 2 lanes?
> 
>> +        vdda-1p2-supply = <&vdda_usb_ss_dp_1p2>;
>> +        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
>> +};
>> +
>>  &pm6150_adc {
>>  	charger-thermistor@4f {
>>  		reg = <ADC5_AMUX_THM3_100K_PU>;
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 6228ba2..05a4133 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -3032,6 +3032,13 @@
>>  							remote-endpoint = <&dsi0_in>;
>>  						};
>>  					};
>> +
>> +					port@2 {
>> +						reg = <2>;
>> +						dpu_intf0_out: endpoint {
>> +							remote-endpoint = <&dp_in>;
>> +						};
>> +					};
>>  				};
>> 
>>  				mdp_opp_table: mdp-opp-table {
>> @@ -3148,6 +3155,77 @@
>> 
>>  				status = "disabled";
>>  			};
>> +
>> +			dp: displayport-controller@ae90000 {
> 
> If you label this "mdss_dp", then it will naturally group with other
> mdss properties in trogdor.dtsi (which should be sorted 
> alphabetically).
> 
>> +				compatible = "qcom,sc7180-dp";
>> +				status = "disabled";
>> +
>> +				reg = <0 0x0ae90000 0 0x1400>;
>> +
>> +				interrupt-parent = <&mdss>;
>> +				interrupts = <12>;
>> +
>> +				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> +					 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
>> +				clock-names = "core_iface", "core_aux", "ctrl_link",
>> +					      "ctrl_link_iface", "stream_pixel";
>> +				#clock-cells = <1>;
>> +				assigned-clocks = <&dispcc DISP_CC_MDSS_DP_LINK_CLK_SRC>,
>> +						  <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +				assigned-clock-parents = <&dp_phy 0>, <&dp_phy 1>;
>> +				phys = <&dp_phy>;
>> +				phy-names = "dp";
>> +
>> +				operating-points-v2 = <&opp_table>;
>> +				power-domains = <&rpmhpd SC7180_CX>;
> 
> Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
> mention CX here in order for the opp framework to apply required-opps
> of CX?

yes,
> 
>> +
>> +				#sound-dai-cells = <0>;
>> +
>> +				ports {
>> +					#address-cells = <1>;
>> +					#size-cells = <0>;
>> +					port@0 {
>> +						reg = <0>;
>> +						dp_in: endpoint {
>> +							remote-endpoint = <&dpu_intf0_out>;
>> +						};
>> +					};
>> +
>> +					port@1 {
>> +						reg = <1>;
>> +						dp_out: endpoint { };
>> +					};
>> +				};
>> +
>> +				opp_table: dp-opp-table {
> 
> The one and only "opp_table" of the sc7180 :)
> Maybe name it dp_opp_table instead?
> 
> Regards,
> Bjorn
> 
>> +					compatible = "operating-points-v2";
>> +
>> +					opp-160000000 {
>> +						opp-hz = /bits/ 64 <160000000>;
>> +						required-opps = <&rpmhpd_opp_low_svs>;
>> +					};
>> +
>> +					opp-270000000 {
>> +						opp-hz = /bits/ 64 <270000000>;
>> +						required-opps = <&rpmhpd_opp_svs>;
>> +					};
>> +
>> +					opp-540000000 {
>> +						opp-hz = /bits/ 64 <540000000>;
>> +						required-opps = <&rpmhpd_opp_svs_l1>;
>> +					};
>> +
>> +					opp-810000000 {
>> +						opp-hz = /bits/ 64 <810000000>;
>> +						required-opps = <&rpmhpd_opp_nom>;
>> +					};
>> +				};
>> +			};
>> +
>> +
>>  		};
>> 
>>  		dispcc: clock-controller@af00000 {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Thu 03 Jun 16:56 CDT 2021, khsieh@codeaurora.org wrote:

> On 2021-06-03 09:53, Bjorn Andersson wrote:
> > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
[..]
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
[..]
> > > +				power-domains = <&rpmhpd SC7180_CX>;
> > 
> > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
> > mention CX here in order for the opp framework to apply required-opps
> > of CX?
> 
> yes,

If you want me, or other maintainers, to spend any time reviewing or
applying your patches going forward then you need to actually bother
replying properly to the questions asked.

Thanks,
Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[khsieh]

On 2021-06-05 22:07, Bjorn Andersson wrote:
> On Thu 03 Jun 16:56 CDT 2021, khsieh@codeaurora.org wrote:
> 
>> On 2021-06-03 09:53, Bjorn Andersson wrote:
>> > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> [..]
>> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> [..]
>> > > +				power-domains = <&rpmhpd SC7180_CX>;
>> >
>> > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
>> > mention CX here in order for the opp framework to apply required-opps
>> > of CX?
>> 
>> yes,
> 
> If you want me, or other maintainers, to spend any time reviewing or
> applying your patches going forward then you need to actually bother
> replying properly to the questions asked.
> 
> Thanks,
> Bjorn

Sorry about the confusion. What I meant is that even though DP 
controller is in the MDSS_GDSC
power domain, DP PHY/PLL sources out of CX. The DP link clocks have a 
direct impact
on the CX voltage corners. Therefore, we need to mention the CX power 
domain here. And, since
we can associate only one OPP table with one device, we picked the DP 
link clock over other
clocks.

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:

> On 2021-06-05 22:07, Bjorn Andersson wrote:
> > On Thu 03 Jun 16:56 CDT 2021, khsieh@codeaurora.org wrote:
> > 
> > > On 2021-06-03 09:53, Bjorn Andersson wrote:
> > > > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> > [..]
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > [..]
> > > > > +				power-domains = <&rpmhpd SC7180_CX>;
> > > >
> > > > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
> > > > mention CX here in order for the opp framework to apply required-opps
> > > > of CX?
> > > 
> > > yes,
> > 
> > If you want me, or other maintainers, to spend any time reviewing or
> > applying your patches going forward then you need to actually bother
> > replying properly to the questions asked.
> > 
> > Thanks,
> > Bjorn
> 
> Sorry about the confusion. What I meant is that even though DP controller is
> in the MDSS_GDSC
> power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> impact
> on the CX voltage corners. Therefore, we need to mention the CX power domain
> here. And, since
> we can associate only one OPP table with one device, we picked the DP link
> clock over other
> clocks.

Thank you, that's a much more useful answer.

Naturally I would think it would make more sense for the PHY/PLL driver
to ensure that CX is appropriately voted for then, but I think that
would result in it being the clock driver performing such vote and I'm
unsure how the opp table for that would look.

@Stephen, what do you say?

Regards,
Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Stephen Boyd]

Quoting Bjorn Andersson (2021-06-07 16:31:47)
> On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
>
> > On 2021-06-05 22:07, Bjorn Andersson wrote:
> > > On Thu 03 Jun 16:56 CDT 2021, khsieh@codeaurora.org wrote:
> > >
> > > > On 2021-06-03 09:53, Bjorn Andersson wrote:
> > > > > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> > > [..]
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > [..]
> > > > > > +                             power-domains = <&rpmhpd SC7180_CX>;
> > > > >
> > > > > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
> > > > > mention CX here in order for the opp framework to apply required-opps
> > > > > of CX?
> > > >
> > > > yes,
> > >
> > > If you want me, or other maintainers, to spend any time reviewing or
> > > applying your patches going forward then you need to actually bother
> > > replying properly to the questions asked.
> > >
> > > Thanks,
> > > Bjorn
> >
> > Sorry about the confusion. What I meant is that even though DP controller is
> > in the MDSS_GDSC
> > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > impact
> > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > here. And, since
> > we can associate only one OPP table with one device, we picked the DP link
> > clock over other
> > clocks.
>
> Thank you, that's a much more useful answer.
>
> Naturally I would think it would make more sense for the PHY/PLL driver
> to ensure that CX is appropriately voted for then, but I think that
> would result in it being the clock driver performing such vote and I'm
> unsure how the opp table for that would look.
>
> @Stephen, what do you say?
>

Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
clk driver, and probably not from the clk ops, but instead come from the
phy ops via phy_enable() and phy_configure().

By the way, there's nothing wrong with a clk device doing power domain
"stuff", except for that we haven't plumbed it into the clk framework
properly and I'm fairly certain our usage of runtime PM in the clk
framework today underneath the prepare_lock is getting us into trouble
or will get us there soon.

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-07 16:31:47)
> > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
> >
> > > On 2021-06-05 22:07, Bjorn Andersson wrote:
> > > > On Thu 03 Jun 16:56 CDT 2021, khsieh@codeaurora.org wrote:
> > > >
> > > > > On 2021-06-03 09:53, Bjorn Andersson wrote:
> > > > > > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
> > > > [..]
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > [..]
> > > > > > > +                             power-domains = <&rpmhpd SC7180_CX>;
> > > > > >
> > > > > > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need to
> > > > > > mention CX here in order for the opp framework to apply required-opps
> > > > > > of CX?
> > > > >
> > > > > yes,
> > > >
> > > > If you want me, or other maintainers, to spend any time reviewing or
> > > > applying your patches going forward then you need to actually bother
> > > > replying properly to the questions asked.
> > > >
> > > > Thanks,
> > > > Bjorn
> > >
> > > Sorry about the confusion. What I meant is that even though DP controller is
> > > in the MDSS_GDSC
> > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > > impact
> > > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > > here. And, since
> > > we can associate only one OPP table with one device, we picked the DP link
> > > clock over other
> > > clocks.
> >
> > Thank you, that's a much more useful answer.
> >
> > Naturally I would think it would make more sense for the PHY/PLL driver
> > to ensure that CX is appropriately voted for then, but I think that
> > would result in it being the clock driver performing such vote and I'm
> > unsure how the opp table for that would look.
> >
> > @Stephen, what do you say?
> >
> 
> Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
> clk driver, and probably not from the clk ops, but instead come from the
> phy ops via phy_enable() and phy_configure().
> 

If I understand the logic correctly *_configure_dp_phy() will both
configure the vco clock and "request" the clock framework to change the
rate.

So I presume what you're suggesting is that that would be the place to
cast the CX corner vote?

> By the way, there's nothing wrong with a clk device doing power domain
> "stuff", except for that we haven't plumbed it into the clk framework
> properly and I'm fairly certain our usage of runtime PM in the clk
> framework today underneath the prepare_lock is getting us into trouble
> or will get us there soon.

On the bright side, it's wonderful that we're at a point where this is
not only a theoretical problem :)

Regards,
Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Stephen Boyd]

Quoting Bjorn Andersson (2021-06-08 15:26:23)
> On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-06-07 16:31:47)
> > > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
> > >
> > > > Sorry about the confusion. What I meant is that even though DP controller is
> > > > in the MDSS_GDSC
> > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > > > impact
> > > > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > > > here. And, since
> > > > we can associate only one OPP table with one device, we picked the DP link
> > > > clock over other
> > > > clocks.
> > >
> > > Thank you, that's a much more useful answer.
> > >
> > > Naturally I would think it would make more sense for the PHY/PLL driver
> > > to ensure that CX is appropriately voted for then, but I think that
> > > would result in it being the clock driver performing such vote and I'm
> > > unsure how the opp table for that would look.
> > >
> > > @Stephen, what do you say?
> > >
> >
> > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
> > clk driver, and probably not from the clk ops, but instead come from the
> > phy ops via phy_enable() and phy_configure().
> >
>
> If I understand the logic correctly *_configure_dp_phy() will both
> configure the vco clock and "request" the clock framework to change the
> rate.
>
> So I presume what you're suggesting is that that would be the place to
> cast the CX corner vote?

Yes that would be a place to make the CX vote. The problem is then I
don't know where to drop the vote. Is that when the phy is disabled?

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-08 15:26:23)
> > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-06-07 16:31:47)
> > > > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
> > > >
> > > > > Sorry about the confusion. What I meant is that even though DP controller is
> > > > > in the MDSS_GDSC
> > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > > > > impact
> > > > > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > > > > here. And, since
> > > > > we can associate only one OPP table with one device, we picked the DP link
> > > > > clock over other
> > > > > clocks.
> > > >
> > > > Thank you, that's a much more useful answer.
> > > >
> > > > Naturally I would think it would make more sense for the PHY/PLL driver
> > > > to ensure that CX is appropriately voted for then, but I think that
> > > > would result in it being the clock driver performing such vote and I'm
> > > > unsure how the opp table for that would look.
> > > >
> > > > @Stephen, what do you say?
> > > >
> > >
> > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
> > > clk driver, and probably not from the clk ops, but instead come from the
> > > phy ops via phy_enable() and phy_configure().
> > >
> >
> > If I understand the logic correctly *_configure_dp_phy() will both
> > configure the vco clock and "request" the clock framework to change the
> > rate.
> >
> > So I presume what you're suggesting is that that would be the place to
> > cast the CX corner vote?
> 
> Yes that would be a place to make the CX vote. The problem is then I
> don't know where to drop the vote. Is that when the phy is disabled?

We do pass qcom_qmp_phy_power_off() and power down the DP part as DP
output is being disabled. So that sounds like a reasonable place to drop
the vote for the lowest performance state.

Regards,
Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Stephen Boyd]

Quoting Bjorn Andersson (2021-06-08 15:34:01)
> On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-06-08 15:26:23)
> > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-06-07 16:31:47)
> > > > > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
> > > > >
> > > > > > Sorry about the confusion. What I meant is that even though DP controller is
> > > > > > in the MDSS_GDSC
> > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > > > > > impact
> > > > > > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > > > > > here. And, since
> > > > > > we can associate only one OPP table with one device, we picked the DP link
> > > > > > clock over other
> > > > > > clocks.
> > > > >
> > > > > Thank you, that's a much more useful answer.
> > > > >
> > > > > Naturally I would think it would make more sense for the PHY/PLL driver
> > > > > to ensure that CX is appropriately voted for then, but I think that
> > > > > would result in it being the clock driver performing such vote and I'm
> > > > > unsure how the opp table for that would look.
> > > > >
> > > > > @Stephen, what do you say?
> > > > >
> > > >
> > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
> > > > clk driver, and probably not from the clk ops, but instead come from the
> > > > phy ops via phy_enable() and phy_configure().
> > > >
> > >
> > > If I understand the logic correctly *_configure_dp_phy() will both
> > > configure the vco clock and "request" the clock framework to change the
> > > rate.
> > >
> > > So I presume what you're suggesting is that that would be the place to
> > > cast the CX corner vote?
> >
> > Yes that would be a place to make the CX vote. The problem is then I
> > don't know where to drop the vote. Is that when the phy is disabled?
>
> We do pass qcom_qmp_phy_power_off() and power down the DP part as DP
> output is being disabled. So that sounds like a reasonable place to drop
> the vote for the lowest performance state.
>

So then will the corner vote be in place when the PHY isn't actually
powered up? That will be bad for power. The phy configure code will need
to know if the phy is enabled and then only put in the vote when the phy
is enabled, otherwise wait for enable to make the corner vote.

Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
digital logic. Probably the PLL is the hardware that has some minimum CX
requirement, and that flows down into the various display clks like the
link clk that actually clock the DP controller hardware. The mdss_gdsc
probably gates CX for the display subsystem (mdss) so if we had proper
corner aggregation logic we could indicate that mdss_gdsc is a child of
the CX domain and then make requests from the DP driver for particular
link frequencies on the mdss_gdsc and then have that bubble up to CX
appropriately. I don't think any of that sort of code is in place
though, right?

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-08 15:34:01)
> > On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-06-08 15:26:23)
> > > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47)
> > > > > > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
> > > > > >
> > > > > > > Sorry about the confusion. What I meant is that even though DP controller is
> > > > > > > in the MDSS_GDSC
> > > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > > > > > > impact
> > > > > > > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > > > > > > here. And, since
> > > > > > > we can associate only one OPP table with one device, we picked the DP link
> > > > > > > clock over other
> > > > > > > clocks.
> > > > > >
> > > > > > Thank you, that's a much more useful answer.
> > > > > >
> > > > > > Naturally I would think it would make more sense for the PHY/PLL driver
> > > > > > to ensure that CX is appropriately voted for then, but I think that
> > > > > > would result in it being the clock driver performing such vote and I'm
> > > > > > unsure how the opp table for that would look.
> > > > > >
> > > > > > @Stephen, what do you say?
> > > > > >
> > > > >
> > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
> > > > > clk driver, and probably not from the clk ops, but instead come from the
> > > > > phy ops via phy_enable() and phy_configure().
> > > > >
> > > >
> > > > If I understand the logic correctly *_configure_dp_phy() will both
> > > > configure the vco clock and "request" the clock framework to change the
> > > > rate.
> > > >
> > > > So I presume what you're suggesting is that that would be the place to
> > > > cast the CX corner vote?
> > >
> > > Yes that would be a place to make the CX vote. The problem is then I
> > > don't know where to drop the vote. Is that when the phy is disabled?
> >
> > We do pass qcom_qmp_phy_power_off() and power down the DP part as DP
> > output is being disabled. So that sounds like a reasonable place to drop
> > the vote for the lowest performance state.
> >
> 
> So then will the corner vote be in place when the PHY isn't actually
> powered up? That will be bad for power. The phy configure code will need
> to know if the phy is enabled and then only put in the vote when the phy
> is enabled, otherwise wait for enable to make the corner vote.
> 

If we vote for a corner based on the link rate in *_configure_dp_phy()
and put the vote for lowest corner we'd get the corner part sorted out
afaict.

We'd still have to make sure that the PHY doesn't hang on to the cx vote
beyond that though - and implicitly in the non-DP cases...

> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> digital logic. Probably the PLL is the hardware that has some minimum CX
> requirement, and that flows down into the various display clks like the
> link clk that actually clock the DP controller hardware. The mdss_gdsc
> probably gates CX for the display subsystem (mdss) so if we had proper
> corner aggregation logic we could indicate that mdss_gdsc is a child of
> the CX domain and then make requests from the DP driver for particular
> link frequencies on the mdss_gdsc and then have that bubble up to CX
> appropriately. I don't think any of that sort of code is in place
> though, right?

I haven't checked sc7180, but I'm guessing that it's following the other
modern platforms, where all the MDSS related pieces (including e.g.
dispcc) lives in the MMCX domain, which is separate from CX.

So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
the dp-opp-table) tells us that the PLL lives in the CX domain.


PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs)
seems to live in MMCX.

Regards,
Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[khsieh]

On 2021-06-08 16:10, Bjorn Andersson wrote:
> On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
> 
>> Quoting Bjorn Andersson (2021-06-08 15:34:01)
>> > On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote:
>> >
>> > > Quoting Bjorn Andersson (2021-06-08 15:26:23)
>> > > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:
>> > > >
>> > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47)
>> > > > > > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
>> > > > > >
>> > > > > > > Sorry about the confusion. What I meant is that even though DP controller is
>> > > > > > > in the MDSS_GDSC
>> > > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
>> > > > > > > impact
>> > > > > > > on the CX voltage corners. Therefore, we need to mention the CX power domain
>> > > > > > > here. And, since
>> > > > > > > we can associate only one OPP table with one device, we picked the DP link
>> > > > > > > clock over other
>> > > > > > > clocks.
>> > > > > >
>> > > > > > Thank you, that's a much more useful answer.
>> > > > > >
>> > > > > > Naturally I would think it would make more sense for the PHY/PLL driver
>> > > > > > to ensure that CX is appropriately voted for then, but I think that
>> > > > > > would result in it being the clock driver performing such vote and I'm
>> > > > > > unsure how the opp table for that would look.
>> > > > > >
>> > > > > > @Stephen, what do you say?
>> > > > > >
>> > > > >
>> > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
>> > > > > clk driver, and probably not from the clk ops, but instead come from the
>> > > > > phy ops via phy_enable() and phy_configure().
>> > > > >
>> > > >
>> > > > If I understand the logic correctly *_configure_dp_phy() will both
>> > > > configure the vco clock and "request" the clock framework to change the
>> > > > rate.
>> > > >
>> > > > So I presume what you're suggesting is that that would be the place to
>> > > > cast the CX corner vote?
>> > >
>> > > Yes that would be a place to make the CX vote. The problem is then I
>> > > don't know where to drop the vote. Is that when the phy is disabled?
>> >
>> > We do pass qcom_qmp_phy_power_off() and power down the DP part as DP
>> > output is being disabled. So that sounds like a reasonable place to drop
>> > the vote for the lowest performance state.
>> >
>> 
>> So then will the corner vote be in place when the PHY isn't actually
>> powered up? That will be bad for power. The phy configure code will 
>> need
>> to know if the phy is enabled and then only put in the vote when the 
>> phy
>> is enabled, otherwise wait for enable to make the corner vote.
>> 
> 
> If we vote for a corner based on the link rate in *_configure_dp_phy()
> and put the vote for lowest corner we'd get the corner part sorted out
> afaict.
> 
> We'd still have to make sure that the PHY doesn't hang on to the cx 
> vote
> beyond that though - and implicitly in the non-DP cases...
> 
>> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
>> digital logic. Probably the PLL is the hardware that has some minimum 
>> CX
>> requirement, and that flows down into the various display clks like 
>> the
>> link clk that actually clock the DP controller hardware. The mdss_gdsc
>> probably gates CX for the display subsystem (mdss) so if we had proper
>> corner aggregation logic we could indicate that mdss_gdsc is a child 
>> of
>> the CX domain and then make requests from the DP driver for particular
>> link frequencies on the mdss_gdsc and then have that bubble up to CX
>> appropriately. I don't think any of that sort of code is in place
>> though, right?
> 
> I haven't checked sc7180, but I'm guessing that it's following the 
> other
> modern platforms, where all the MDSS related pieces (including e.g.
> dispcc) lives in the MMCX domain, which is separate from CX.
> 
> So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
> the dp-opp-table) tells us that the PLL lives in the CX domain.
> 
> 
> PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs)
> seems to live in MMCX.
> 
> Regards,
> Bjorn

Dp link clock rate is sourced from phy/pll (vco). However it is possible 
that different link clock rate
are sourced from same vco (phy/pll) rate. Therefore I think CX rail 
voltage level is more proper to
be decided base on link clock rate.

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-06-10 09:54:05)
> On 2021-06-08 16:10, Bjorn Andersson wrote:
> > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
> >
> >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> >> digital logic. Probably the PLL is the hardware that has some minimum
> >> CX
> >> requirement, and that flows down into the various display clks like
> >> the
> >> link clk that actually clock the DP controller hardware. The mdss_gdsc
> >> probably gates CX for the display subsystem (mdss) so if we had proper
> >> corner aggregation logic we could indicate that mdss_gdsc is a child
> >> of
> >> the CX domain and then make requests from the DP driver for particular
> >> link frequencies on the mdss_gdsc and then have that bubble up to CX
> >> appropriately. I don't think any of that sort of code is in place
> >> though, right?
> >
> > I haven't checked sc7180, but I'm guessing that it's following the
> > other
> > modern platforms, where all the MDSS related pieces (including e.g.
> > dispcc) lives in the MMCX domain, which is separate from CX.
> >
> > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
> > the dp-opp-table) tells us that the PLL lives in the CX domain.

Isn't MMCX a "child" of CX? At least my understanding is that MMCX is
basically a GDSC that clamps all of multimedia hardware block power
logic so that the leakage is minimized when multimedia isn't in use,
i.e. the device is suspended. In terms of bumping up the voltage we have
to pin that on CX though as far as I know because that's the only power
domain that can actually change voltage, while MMCX merely gates that
voltage for multimedia.

> >
> >
> > PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs)
> > seems to live in MMCX.
> >
> > Regards,
> > Bjorn
>
> Dp link clock rate is sourced from phy/pll (vco). However it is possible
> that different link clock rate
> are sourced from same vco (phy/pll) rate. Therefore I think CX rail
> voltage level is more proper to
> be decided base on link clock rate.
>

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Fri 18 Jun 15:49 CDT 2021, Stephen Boyd wrote:

> Quoting khsieh@codeaurora.org (2021-06-10 09:54:05)
> > On 2021-06-08 16:10, Bjorn Andersson wrote:
> > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
> > >
> > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> > >> digital logic. Probably the PLL is the hardware that has some minimum
> > >> CX
> > >> requirement, and that flows down into the various display clks like
> > >> the
> > >> link clk that actually clock the DP controller hardware. The mdss_gdsc
> > >> probably gates CX for the display subsystem (mdss) so if we had proper
> > >> corner aggregation logic we could indicate that mdss_gdsc is a child
> > >> of
> > >> the CX domain and then make requests from the DP driver for particular
> > >> link frequencies on the mdss_gdsc and then have that bubble up to CX
> > >> appropriately. I don't think any of that sort of code is in place
> > >> though, right?
> > >
> > > I haven't checked sc7180, but I'm guessing that it's following the
> > > other
> > > modern platforms, where all the MDSS related pieces (including e.g.
> > > dispcc) lives in the MMCX domain, which is separate from CX.
> > >
> > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
> > > the dp-opp-table) tells us that the PLL lives in the CX domain.
> 
> Isn't MMCX a "child" of CX? At least my understanding is that MMCX is
> basically a GDSC that clamps all of multimedia hardware block power
> logic so that the leakage is minimized when multimedia isn't in use,
> i.e. the device is suspended. In terms of bumping up the voltage we have
> to pin that on CX though as far as I know because that's the only power
> domain that can actually change voltage, while MMCX merely gates that
> voltage for multimedia.
> 

No, MMCX is a separate rail from CX, which powers the display blocks and
is parent of MDSS_GDSC. But I see in rpmhpd that sc7180 is not one of
these platforms, so I presume this means that the displayport controller
thereby sits in MDSS_GDSC parented by CX.

But in line with what you're saying, the naming of the supplies to the
QMP indicates that the power for the PLLs is static. As such the only
moving things would be the clock rates in the DP controller and as such
that's what needs to scale the voltage.

So if the resources we're scaling is the clocks in the DP controller
then the gist of the patch is correct. The only details I see is that
the DP controller actually sits in MDSS_GDSC - while it should control
the level of its parent (CX). Not sure if we can describe that in a
simple way.


PS. Why does the node name of the opp-table have to be globally unique?

Regards,
Bjorn

> > >
> > >
> > > PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs)
> > > seems to live in MMCX.
> > >
> > > Regards,
> > > Bjorn
> >
> > Dp link clock rate is sourced from phy/pll (vco). However it is possible
> > that different link clock rate
> > are sourced from same vco (phy/pll) rate. Therefore I think CX rail
> > voltage level is more proper to
> > be decided base on link clock rate.
> >

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Stephen Boyd]

Quoting Bjorn Andersson (2021-06-18 14:41:50)
> On Fri 18 Jun 15:49 CDT 2021, Stephen Boyd wrote:
>
> > Quoting khsieh@codeaurora.org (2021-06-10 09:54:05)
> > > On 2021-06-08 16:10, Bjorn Andersson wrote:
> > > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
> > > >
> > > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> > > >> digital logic. Probably the PLL is the hardware that has some minimum
> > > >> CX
> > > >> requirement, and that flows down into the various display clks like
> > > >> the
> > > >> link clk that actually clock the DP controller hardware. The mdss_gdsc
> > > >> probably gates CX for the display subsystem (mdss) so if we had proper
> > > >> corner aggregation logic we could indicate that mdss_gdsc is a child
> > > >> of
> > > >> the CX domain and then make requests from the DP driver for particular
> > > >> link frequencies on the mdss_gdsc and then have that bubble up to CX
> > > >> appropriately. I don't think any of that sort of code is in place
> > > >> though, right?
> > > >
> > > > I haven't checked sc7180, but I'm guessing that it's following the
> > > > other
> > > > modern platforms, where all the MDSS related pieces (including e.g.
> > > > dispcc) lives in the MMCX domain, which is separate from CX.
> > > >
> > > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
> > > > the dp-opp-table) tells us that the PLL lives in the CX domain.
> >
> > Isn't MMCX a "child" of CX? At least my understanding is that MMCX is
> > basically a GDSC that clamps all of multimedia hardware block power
> > logic so that the leakage is minimized when multimedia isn't in use,
> > i.e. the device is suspended. In terms of bumping up the voltage we have
> > to pin that on CX though as far as I know because that's the only power
> > domain that can actually change voltage, while MMCX merely gates that
> > voltage for multimedia.
> >
>
> No, MMCX is a separate rail from CX, which powers the display blocks and
> is parent of MDSS_GDSC. But I see in rpmhpd that sc7180 is not one of
> these platforms, so I presume this means that the displayport controller
> thereby sits in MDSS_GDSC parented by CX.
>
> But in line with what you're saying, the naming of the supplies to the
> QMP indicates that the power for the PLLs is static. As such the only
> moving things would be the clock rates in the DP controller and as such
> that's what needs to scale the voltage.
>
> So if the resources we're scaling is the clocks in the DP controller
> then the gist of the patch is correct. The only details I see is that
> the DP controller actually sits in MDSS_GDSC - while it should control
> the level of its parent (CX). Not sure if we can describe that in a
> simple way.

Right. I'm not sure things could be described any better right now. If
we need to change this to be MDSS_GDSC power domain and control the
level of the parent then I suppose we'll have to make some sort of DT
change and pair that with a driver change. Maybe if that happens we can
just pick a new compatible and leave the old code in place.

Are you happy enough with this current patch?

>
>
> PS. Why does the node name of the opp-table have to be globally unique?

Presumably the opp table node name can be 'opp-table' as long as it
lives under the node that's using it. If the opp table is at / or /soc
then it will need to be unique. I'd prefer just 'opp-table' if possible.

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Tue 22 Jun 15:23 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-18 14:41:50)
> > On Fri 18 Jun 15:49 CDT 2021, Stephen Boyd wrote:
> >
> > > Quoting khsieh@codeaurora.org (2021-06-10 09:54:05)
> > > > On 2021-06-08 16:10, Bjorn Andersson wrote:
> > > > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
> > > > >
> > > > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> > > > >> digital logic. Probably the PLL is the hardware that has some minimum
> > > > >> CX
> > > > >> requirement, and that flows down into the various display clks like
> > > > >> the
> > > > >> link clk that actually clock the DP controller hardware. The mdss_gdsc
> > > > >> probably gates CX for the display subsystem (mdss) so if we had proper
> > > > >> corner aggregation logic we could indicate that mdss_gdsc is a child
> > > > >> of
> > > > >> the CX domain and then make requests from the DP driver for particular
> > > > >> link frequencies on the mdss_gdsc and then have that bubble up to CX
> > > > >> appropriately. I don't think any of that sort of code is in place
> > > > >> though, right?
> > > > >
> > > > > I haven't checked sc7180, but I'm guessing that it's following the
> > > > > other
> > > > > modern platforms, where all the MDSS related pieces (including e.g.
> > > > > dispcc) lives in the MMCX domain, which is separate from CX.
> > > > >
> > > > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
> > > > > the dp-opp-table) tells us that the PLL lives in the CX domain.
> > >
> > > Isn't MMCX a "child" of CX? At least my understanding is that MMCX is
> > > basically a GDSC that clamps all of multimedia hardware block power
> > > logic so that the leakage is minimized when multimedia isn't in use,
> > > i.e. the device is suspended. In terms of bumping up the voltage we have
> > > to pin that on CX though as far as I know because that's the only power
> > > domain that can actually change voltage, while MMCX merely gates that
> > > voltage for multimedia.
> > >
> >
> > No, MMCX is a separate rail from CX, which powers the display blocks and
> > is parent of MDSS_GDSC. But I see in rpmhpd that sc7180 is not one of
> > these platforms, so I presume this means that the displayport controller
> > thereby sits in MDSS_GDSC parented by CX.
> >
> > But in line with what you're saying, the naming of the supplies to the
> > QMP indicates that the power for the PLLs is static. As such the only
> > moving things would be the clock rates in the DP controller and as such
> > that's what needs to scale the voltage.
> >
> > So if the resources we're scaling is the clocks in the DP controller
> > then the gist of the patch is correct. The only details I see is that
> > the DP controller actually sits in MDSS_GDSC - while it should control
> > the level of its parent (CX). Not sure if we can describe that in a
> > simple way.
> 
> Right. I'm not sure things could be described any better right now. If
> we need to change this to be MDSS_GDSC power domain and control the
> level of the parent then I suppose we'll have to make some sort of DT
> change and pair that with a driver change. Maybe if that happens we can
> just pick a new compatible and leave the old code in place.
> 

I would prefer that we stay away from making up a new compatible for
that, but let's see when we get there.

> Are you happy enough with this current patch?
> 

Yes, I think this looks good.

> >
> >
> > PS. Why does the node name of the opp-table have to be globally unique?
> 
> Presumably the opp table node name can be 'opp-table' as long as it
> lives under the node that's using it. If the opp table is at / or /soc
> then it will need to be unique. I'd prefer just 'opp-table' if possible.

I asked the same question (if it has to be globally unique) in the patch
adding sdhci nodes for sc7280 and I didn't get a sufficient answer...

So now I do want to know why "opp-table" wouldn't be sufficient name for
these device-internal nodes.

Regards,
Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[khsieh]

On 2021-06-22 19:52, Bjorn Andersson wrote:
> On Tue 22 Jun 15:23 CDT 2021, Stephen Boyd wrote:
> 
>> Quoting Bjorn Andersson (2021-06-18 14:41:50)
>> > On Fri 18 Jun 15:49 CDT 2021, Stephen Boyd wrote:
>> >
>> > > Quoting khsieh@codeaurora.org (2021-06-10 09:54:05)
>> > > > On 2021-06-08 16:10, Bjorn Andersson wrote:
>> > > > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
>> > > > >
>> > > > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
>> > > > >> digital logic. Probably the PLL is the hardware that has some minimum
>> > > > >> CX
>> > > > >> requirement, and that flows down into the various display clks like
>> > > > >> the
>> > > > >> link clk that actually clock the DP controller hardware. The mdss_gdsc
>> > > > >> probably gates CX for the display subsystem (mdss) so if we had proper
>> > > > >> corner aggregation logic we could indicate that mdss_gdsc is a child
>> > > > >> of
>> > > > >> the CX domain and then make requests from the DP driver for particular
>> > > > >> link frequencies on the mdss_gdsc and then have that bubble up to CX
>> > > > >> appropriately. I don't think any of that sort of code is in place
>> > > > >> though, right?
>> > > > >
>> > > > > I haven't checked sc7180, but I'm guessing that it's following the
>> > > > > other
>> > > > > modern platforms, where all the MDSS related pieces (including e.g.
>> > > > > dispcc) lives in the MMCX domain, which is separate from CX.
>> > > > >
>> > > > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
>> > > > > the dp-opp-table) tells us that the PLL lives in the CX domain.
>> > >
>> > > Isn't MMCX a "child" of CX? At least my understanding is that MMCX is
>> > > basically a GDSC that clamps all of multimedia hardware block power
>> > > logic so that the leakage is minimized when multimedia isn't in use,
>> > > i.e. the device is suspended. In terms of bumping up the voltage we have
>> > > to pin that on CX though as far as I know because that's the only power
>> > > domain that can actually change voltage, while MMCX merely gates that
>> > > voltage for multimedia.
>> > >
>> >
>> > No, MMCX is a separate rail from CX, which powers the display blocks and
>> > is parent of MDSS_GDSC. But I see in rpmhpd that sc7180 is not one of
>> > these platforms, so I presume this means that the displayport controller
>> > thereby sits in MDSS_GDSC parented by CX.
>> >
>> > But in line with what you're saying, the naming of the supplies to the
>> > QMP indicates that the power for the PLLs is static. As such the only
>> > moving things would be the clock rates in the DP controller and as such
>> > that's what needs to scale the voltage.
>> >
>> > So if the resources we're scaling is the clocks in the DP controller
>> > then the gist of the patch is correct. The only details I see is that
>> > the DP controller actually sits in MDSS_GDSC - while it should control
>> > the level of its parent (CX). Not sure if we can describe that in a
>> > simple way.
>> 
>> Right. I'm not sure things could be described any better right now. If
>> we need to change this to be MDSS_GDSC power domain and control the
>> level of the parent then I suppose we'll have to make some sort of DT
>> change and pair that with a driver change. Maybe if that happens we 
>> can
>> just pick a new compatible and leave the old code in place.
>> 
> 
> I would prefer that we stay away from making up a new compatible for
> that, but let's see when we get there.
> 
>> Are you happy enough with this current patch?
>> 
> 
> Yes, I think this looks good.
> 
>> >
>> >
>> > PS. Why does the node name of the opp-table have to be globally unique?
>> 
>> Presumably the opp table node name can be 'opp-table' as long as it
>> lives under the node that's using it. If the opp table is at / or /soc
>> then it will need to be unique. I'd prefer just 'opp-table' if 
>> possible.
> 
> I asked the same question (if it has to be globally unique) in the 
> patch
> adding sdhci nodes for sc7280 and I didn't get a sufficient answer...
> 
> So now I do want to know why "opp-table" wouldn't be sufficient name 
> for
> these device-internal nodes.
> 
my opinion is dp_opp_table is more consistency with mdp and dsi.
Either one is fine. Please let me know asap.
> Regards,
> Bjorn

Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

[Bjorn Andersson]

On Fri 25 Jun 10:55 CDT 2021, khsieh@codeaurora.org wrote:

> On 2021-06-22 19:52, Bjorn Andersson wrote:
> > On Tue 22 Jun 15:23 CDT 2021, Stephen Boyd wrote:
> > 
> > > Quoting Bjorn Andersson (2021-06-18 14:41:50)
> > > > On Fri 18 Jun 15:49 CDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting khsieh@codeaurora.org (2021-06-10 09:54:05)
> > > > > > On 2021-06-08 16:10, Bjorn Andersson wrote:
> > > > > > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:
> > > > > > >
> > > > > > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> > > > > > >> digital logic. Probably the PLL is the hardware that has some minimum
> > > > > > >> CX
> > > > > > >> requirement, and that flows down into the various display clks like
> > > > > > >> the
> > > > > > >> link clk that actually clock the DP controller hardware. The mdss_gdsc
> > > > > > >> probably gates CX for the display subsystem (mdss) so if we had proper
> > > > > > >> corner aggregation logic we could indicate that mdss_gdsc is a child
> > > > > > >> of
> > > > > > >> the CX domain and then make requests from the DP driver for particular
> > > > > > >> link frequencies on the mdss_gdsc and then have that bubble up to CX
> > > > > > >> appropriately. I don't think any of that sort of code is in place
> > > > > > >> though, right?
> > > > > > >
> > > > > > > I haven't checked sc7180, but I'm guessing that it's following the
> > > > > > > other
> > > > > > > modern platforms, where all the MDSS related pieces (including e.g.
> > > > > > > dispcc) lives in the MMCX domain, which is separate from CX.
> > > > > > >
> > > > > > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
> > > > > > > the dp-opp-table) tells us that the PLL lives in the CX domain.
> > > > >
> > > > > Isn't MMCX a "child" of CX? At least my understanding is that MMCX is
> > > > > basically a GDSC that clamps all of multimedia hardware block power
> > > > > logic so that the leakage is minimized when multimedia isn't in use,
> > > > > i.e. the device is suspended. In terms of bumping up the voltage we have
> > > > > to pin that on CX though as far as I know because that's the only power
> > > > > domain that can actually change voltage, while MMCX merely gates that
> > > > > voltage for multimedia.
> > > > >
> > > >
> > > > No, MMCX is a separate rail from CX, which powers the display blocks and
> > > > is parent of MDSS_GDSC. But I see in rpmhpd that sc7180 is not one of
> > > > these platforms, so I presume this means that the displayport controller
> > > > thereby sits in MDSS_GDSC parented by CX.
> > > >
> > > > But in line with what you're saying, the naming of the supplies to the
> > > > QMP indicates that the power for the PLLs is static. As such the only
> > > > moving things would be the clock rates in the DP controller and as such
> > > > that's what needs to scale the voltage.
> > > >
> > > > So if the resources we're scaling is the clocks in the DP controller
> > > > then the gist of the patch is correct. The only details I see is that
> > > > the DP controller actually sits in MDSS_GDSC - while it should control
> > > > the level of its parent (CX). Not sure if we can describe that in a
> > > > simple way.
> > > 
> > > Right. I'm not sure things could be described any better right now. If
> > > we need to change this to be MDSS_GDSC power domain and control the
> > > level of the parent then I suppose we'll have to make some sort of DT
> > > change and pair that with a driver change. Maybe if that happens we
> > > can
> > > just pick a new compatible and leave the old code in place.
> > > 
> > 
> > I would prefer that we stay away from making up a new compatible for
> > that, but let's see when we get there.
> > 
> > > Are you happy enough with this current patch?
> > > 
> > 
> > Yes, I think this looks good.
> > 
> > > >
> > > >
> > > > PS. Why does the node name of the opp-table have to be globally unique?
> > > 
> > > Presumably the opp table node name can be 'opp-table' as long as it
> > > lives under the node that's using it. If the opp table is at / or /soc
> > > then it will need to be unique. I'd prefer just 'opp-table' if
> > > possible.
> > 
> > I asked the same question (if it has to be globally unique) in the patch
> > adding sdhci nodes for sc7280 and I didn't get a sufficient answer...
> > 
> > So now I do want to know why "opp-table" wouldn't be sufficient name for
> > these device-internal nodes.
> > 
> my opinion is dp_opp_table is more consistency with mdp and dsi.
> Either one is fine. Please let me know asap.

I presume you mean dp-opp-table, and you're right, that is perfectly in
line with gpu-opp-table, mdp-opp-table and dsi-opp-table. But there's
also a few examples showing me that there's no need for it to be
globally unique.

So "dp_opp_table: opp-table" is the form I want and we should fix all
those other cases.

I'll update your patch as I apply it, no need to respin it for that.

Thanks,
Bjorn

> > Regards,
> > Bjorn