arm64/dts/qcom/sc7180: Add Display Port dt node
Subsystems (4)
PATCH v2 1/1 arm64/dts/qcom/sc7180: Add Display Port dt node
Add DP device node on sc7180.
Changes in v2:
-- replace msm_dp with dp
-- replace dp_opp_table with opp_table
--- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +9 -0
@@ -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>;
arch/arm64/boot/dts/qcom/sc7180.dtsi +78 -0
90 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 6 lines · Kuogee Hsieh
Add DP device node on sc7180.
Changes in v2:
-- replacemsm_dpwith dp
-- replacedp_opp_tablewithopp_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.
Quoted 11 lines
Signed-off-by Kuogee Hsieh <[email protected]>--- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +0
-0
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi hp_i2c: &i2c9 { · line 793
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +5
-0@@ -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?
Quoted 26 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +4
-0+ 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>;arch/arm64/boot/dts/qcom/sc7180.dtsi +7
-0@@ -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 {
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3159
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +2
-0@@ -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).
Quoted 14 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +14
-0+ 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",
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3183
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ "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?
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ + #sound-dai-cells = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + dp_in: endpoint { + remote-endpoint = <&dpu_intf0_out>;
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3203
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ }; + }; + + 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
Quoted 32 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +25
-0+ 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 {Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 29 lines · Bjorn Andersson
Quoted 6 lines · Kuogee Hsieh
Add DP device node on sc7180.
Changes in v2:
-- replacemsm_dpwith dp
-- replacedp_opp_tablewithopp_tableI'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.
Quoted 15 lines
Signed-off-by Kuogee Hsieh <[email protected]>--- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +0
-0@@ -786,6 +786,15 @@ hp_i2c: &i2c9 { status = "okay"; };
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi hp_i2c: &i2c9 { · line 793
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +5
-0+&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,
Quoted 124 lines
Quoted 27 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +4
-0+ 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>;arch/arm64/boot/dts/qcom/sc7180.dtsi +7
-0@@ -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 {arch/arm64/boot/dts/qcom/sc7180.dtsi line 3159
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +2
-0@@ -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 introgdor.dtsi(which should be sorted alphabetically).Quoted 14 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +14
-0+ 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",arch/arm64/boot/dts/qcom/sc7180.dtsi line 3183
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ "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?Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ + #sound-dai-cells = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + dp_in: endpoint { + remote-endpoint = <&dpu_intf0_out>;arch/arm64/boot/dts/qcom/sc7180.dtsi line 3203
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ }; + }; + + port@1 { + reg = <1>; + dp_out: endpoint { }; + }; + }; + + opp_table: dp-opp-table {The one and only "
opp_table" of the sc7180 :)
Maybe name itdp_opp_tableinstead?Regards,
BjornQuoted 32 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +25
-1+ 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
Quoting [email protected] (2021-06-03 14:28:37)
Quoted 39 lines · Bjorn Andersson
Quoted 30 lines · Kuogee Hsieh
Quoted 7 lines
Add DP device node on sc7180.
Changes in v2:
-- replacemsm_dpwith dp
-- replacedp_opp_tablewithopp_tableI'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.
Quoted 16 lines
Signed-off-by Kuogee Hsieh <[email protected]>--- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +0
-0@@ -786,6 +786,15 @@ hp_i2c: &i2c9 { status = "okay"; };Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi hp_i2c: &i2c9 { · line 793
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +5
-0+&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?
Quoted 2 lines
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.
Quoted 92 lines · Bjorn Andersson
Quoted 6 lines · Kuogee Hsieh
Add DP device node on sc7180.
Changes in v2:
-- replacemsm_dpwith dp
-- replacedp_opp_tablewithopp_tableI'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.
Quoted 12 lines
Signed-off-by Kuogee Hsieh <[email protected]>--- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +0
-0arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi hp_i2c: &i2c9 { · line 793
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +5
-0@@ -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?
Quoted 26 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +4
-0+ 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>;arch/arm64/boot/dts/qcom/sc7180.dtsi +7
-0@@ -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 {arch/arm64/boot/dts/qcom/sc7180.dtsi line 3159
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +2
-0@@ -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 introgdor.dtsi(which should be sorted alphabetically).Quoted 20 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +20
-0+ 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>;
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3183
Quoted 4 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +4
-0+ 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,
Quoted 60 lines
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ + #sound-dai-cells = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + dp_in: endpoint { + remote-endpoint = <&dpu_intf0_out>;arch/arm64/boot/dts/qcom/sc7180.dtsi line 3203
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +10
-0+ }; + }; + + port@1 { + reg = <1>; + dp_out: endpoint { }; + }; + }; + + opp_table: dp-opp-table {The one and only "
opp_table" of the sc7180 :)
Maybe name itdp_opp_tableinstead?Regards,
BjornQuoted 32 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi +25
-1+ 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
Quoted 2 lines · On Thu 03 Jun 16:56 CDT 2021, [email protected] wrote:
Quoted 1 line · Bjorn Andersson
On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0
[..]
Quoted 7 lines
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3183
Quoted 1 line
arch/arm64/boot/dts/qcom/sc7180.dtsi +1
-0+ 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
Quoted 13 lines · Bjorn Andersson
Quoted 2 lines · On Thu 03 Jun 16:56 CDT 2021, [email protected] wrote:
Quoted 1 line · Bjorn Andersson
On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0[..]
Quoted 6 lines
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3183
Quoted 1 line
arch/arm64/boot/dts/qcom/sc7180.dtsi +1
-0+ 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?
Quoted 8 lines
Quoted 1 line
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.
Quoted 23 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 13 lines · Bjorn Andersson
Quoted 2 lines · On Thu 03 Jun 16:56 CDT 2021, [email protected] wrote:
Quoted 1 line · Bjorn Andersson
On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote:
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0[..]
Quoted 6 lines
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180.dtsi line 3183
Quoted 1 line
arch/arm64/boot/dts/qcom/sc7180.dtsi +1
-0+ 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?Quoted 8 lines
Quoted 1 line
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
Quoted 9 lines
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
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 35 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 24 lines · Bjorn Andersson
Quoted 13 lines · On Thu 03 Jun 16:56 CDT 2021, [email protected] wrote:
Quoted 3 lines · Bjorn Andersson
Quoted 1 line · Kuogee Hsieh
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0[..]
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
+ 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?Quoted 8 lines
Quoted 1 line
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,
BjornQuoted 9 lines
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.
Quoted 9 lines
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.
Quoted 43 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 42 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 24 lines · Bjorn Andersson
Quoted 13 lines · On Thu 03 Jun 16:56 CDT 2021, [email protected] wrote:
Quoted 3 lines · Bjorn Andersson
Quoted 1 line · Kuogee Hsieh
[..]
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0[..]
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
+ 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?Quoted 8 lines
Quoted 1 line
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,
BjornQuoted 9 lines
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.
Quoted 7 lines
Quoted 2 lines
@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()andphy_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?
Quoted 5 lines
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_lockis 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
Quoting Bjorn Andersson (2021-06-08 15:26:23)
Quoted 30 lines · Stephen Boyd
Quoted 21 lines
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 19 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
@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()andphy_configure().
Quoted 6 lines
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?
Quoted 35 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-08 15:26:23)
Quoted 34 lines · Stephen Boyd
Quoted 21 lines
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 19 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
@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()andphy_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.
Quoted 5 lines
Quoted 2 lines
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
Quoting Bjorn Andersson (2021-06-08 15:34:01)
Quoted 40 lines · Stephen Boyd
Quoted 39 lines
Quoting Bjorn Andersson (2021-06-08 15:26:23)
Quoted 36 lines · Stephen Boyd
Quoted 21 lines
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 19 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
@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()andphy_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?
Quoted 7 lines
Quoted 2 lines
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?
Quoted 49 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-08 15:34:01)
Quoted 40 lines · Stephen Boyd
Quoted 39 lines
Quoting Bjorn Andersson (2021-06-08 15:26:23)
Quoted 36 lines · Stephen Boyd
Quoted 21 lines
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 19 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
@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()andphy_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?
Quoted 7 lines
Quoted 2 lines
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.
Quoted 5 lines
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...
Quoted 10 lines
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_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand 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
Quoted 83 lines · Bjorn Andersson
Quoted 49 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-08 15:34:01)
Quoted 40 lines · Stephen Boyd
Quoted 39 lines
Quoting Bjorn Andersson (2021-06-08 15:26:23)
Quoted 36 lines · Stephen Boyd
Quoted 21 lines
Quoting Bjorn Andersson (2021-06-07 16:31:47)
Quoted 19 lines · On Mon 07 Jun 12:48 CDT 2021, [email protected] wrote:
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
@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()andphy_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?
Quoted 7 lines
Quoted 2 lines
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.Quoted 5 lines
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...
Quoted 10 lines
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_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand 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_GDSCshould be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain.
Quoted 5 lines
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.
Quoting [email protected] (2021-06-10 09:54:05)
Quoted 17 lines · Bjorn Andersson
Quoted 16 lines · Stephen Boyd
Quoted 14 lines
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. Themdss_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of
the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right?
Quoted 7 lines
Quoted 7 lines
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_GDSCshould 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.
Quoted 14 lines
Quoted 7 lines
PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs) seems to live in MMCX.
Regards,
BjornDp 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.
Quoted 26 lines · Stephen Boyd
Quoting [email protected] (2021-06-10 09:54:05)
Quoted 17 lines · Bjorn Andersson
Quoted 16 lines · Stephen Boyd
Quoted 14 lines
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. Themdss_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of
the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right?Quoted 7 lines
Quoted 7 lines
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_GDSCshould be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain.
Quoted 8 lines
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
Quoted 14 lines
Quoted 14 lines
Quoted 7 lines
PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs) seems to live in MMCX.
Regards,
BjornDp 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.
Quoting Bjorn Andersson (2021-06-18 14:41:50)
Quoted 47 lines · Stephen Boyd
Quoted 27 lines
Quoting [email protected] (2021-06-10 09:54:05)
Quoted 17 lines · Bjorn Andersson
Quoted 16 lines · Stephen Boyd
Quoted 14 lines
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. Themdss_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of
the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right?Quoted 7 lines
Quoted 7 lines
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_GDSCshould be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain.Quoted 8 lines
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 inMDSS_GDSCparented 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.
Quoted 5 lines
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?
Quoted 3 lines
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.
Quoted 54 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-18 14:41:50)
Quoted 47 lines · Stephen Boyd
Quoted 27 lines
Quoting [email protected] (2021-06-10 09:54:05)
Quoted 17 lines · Bjorn Andersson
Quoted 16 lines · Stephen Boyd
Quoted 14 lines
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. Themdss_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of
the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right?Quoted 7 lines
Quoted 7 lines
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_GDSCshould be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain.Quoted 8 lines
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 inMDSS_GDSCparented 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.
Quoted 5 lines
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.
Quoted 6 lines
Right. I'm not sure things could be described any better right now. If we need to change this to be
MDSS_GDSCpower 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.
Quoted 2 lines
Are you happy enough with this current patch?
Yes, I think this looks good.
Quoted 7 lines
Quoted 3 lines
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
Quoted 79 lines · Bjorn Andersson
Quoted 54 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-18 14:41:50)
Quoted 47 lines · Stephen Boyd
Quoted 27 lines
Quoting [email protected] (2021-06-10 09:54:05)
Quoted 17 lines · Bjorn Andersson
Quoted 16 lines · Stephen Boyd
Quoted 14 lines
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. Themdss_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of
the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right?Quoted 7 lines
Quoted 7 lines
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_GDSCshould be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain.Quoted 8 lines
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 inMDSS_GDSCparented 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.
Quoted 5 lines
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.Quoted 6 lines
Right. I'm not sure things could be described any better right now. If we need to change this to be
MDSS_GDSCpower 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.
Quoted 2 lines
Are you happy enough with this current patch?
Yes, I think this looks good.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
Regards,
Bjorn
Quoted 82 lines · On Fri 25 Jun 10:55 CDT 2021, [email protected] wrote:
Quoted 81 lines · Bjorn Andersson
Quoted 54 lines · Stephen Boyd
Quoting Bjorn Andersson (2021-06-18 14:41:50)
Quoted 47 lines · Stephen Boyd
Quoted 27 lines
Quoting [email protected] (2021-06-10 09:54:05)
Quoted 17 lines · Bjorn Andersson
Quoted 16 lines · Stephen Boyd
Quoted 14 lines
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. Themdss_gdscprobably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate thatmdss_gdscis a child of
the CX domain and then make requests from the DP driver for particular link frequencies on themdss_gdscand then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right?Quoted 7 lines
Quoted 7 lines
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_GDSCshould be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain.Quoted 8 lines
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 inMDSS_GDSCparented 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.
Quoted 5 lines
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.Quoted 7 lines
Right. I'm not sure things could be described any better right now. If we need to change this to be
MDSS_GDSCpower 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.
Quoted 2 lines
Are you happy enough with this current patch?
Yes, I think this looks good.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 6 lines
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_tableis 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
Quoted 2 lines
Quoted 2 lines
Regards,
Bjorn