| Message ID | 1622736555-15775-1-git-send-email-khsieh@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] arm64/dts/qcom/sc7180: Add Display Port dt node | expand |
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 >
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 >>
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.
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 >>
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
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.
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
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.
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
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?
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
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?
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
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.
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. >
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. > >
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.
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
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
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
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 {
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(+)