diff mbox series

[2/3] arm64: dts: qcom: sm8150: Add DISPCC node

Message ID 20221212093315.11390-2-konrad.dybcio@linaro.org
State New
Headers
Series [1/3] dt-bindings: display/msm: Add SM8150 MDSS & DPU |

Commit Message

Konrad Dybcio Dec. 12, 2022, 9:33 a.m. UTC 
  Years after the SoC support has been added, it's high time for it to
get dispcc going. Add the node to ensure that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Dmitry Baryshkov Dec. 12, 2022, 10:18 a.m. UTC | #1 
On 12 December 2022 12:33:13 GMT+03:00, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>Years after the SoC support has been added, it's high time for it to
>get dispcc going. Add the node to ensure that.
>
>Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>---
> arch/arm64/boot/dts/qcom/sm8150.dtsi | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>index a0c57fb798d3..ff04397777f4 100644
>--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
>+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>@@ -3579,6 +3579,32 @@ camnoc_virt: interconnect@ac00000 {
> 			qcom,bcm-voters = <&apps_bcm_voter>;
> 		};
> 
>+		dispcc: clock-controller@af00000 {
>+			compatible = "qcom,sm8150-dispcc";
>+			reg = <0 0x0af00000 0 0x10000>;
>+			clocks = <&rpmhcc RPMH_CXO_CLK>,
>+				 <0>,
>+				 <0>,
>+				 <0>,
>+				 <0>,
>+				 <0>,
>+				 <0>;
>+			clock-names = "bi_tcxo",
>+				      "dsi0_phy_pll_out_byteclk",
>+				      "dsi0_phy_pll_out_dsiclk",
>+				      "dsi1_phy_pll_out_byteclk",
>+				      "dsi1_phy_pll_out_dsiclk",
>+				      "dp_phy_pll_link_clk",
>+				      "dp_phy_pll_vco_div_clk";
>+			#clock-cells = <1>;
>+			#reset-cells = <1>;
>+			#power-domain-cells = <1>;
>+
>+			power-domains = <&rpmhpd SM8150_MMCX>;
>+			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */
>+			required-opps = <&rpmhpd_opp_low_svs>;

Is it required for the dispcc, for the DSI or for the dpu? We have stumbled upon the similar issue when working on the 8350, see the latest Robert's patchset.


>+		};
>+
> 		pdc: interrupt-controller@b220000 {
> 			compatible = "qcom,sm8150-pdc", "qcom,pdc";
> 			reg = <0 0x0b220000 0 0x400>;
Marijn Suijten Dec. 12, 2022, 10:19 a.m. UTC | #2 
On 2022-12-12 10:33:13, Konrad Dybcio wrote:
> Years after the SoC support has been added, it's high time for it to
> get dispcc going. Add the node to ensure that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

On Sony Xperia 5:
Tested-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  arch/arm64/boot/dts/qcom/sm8150.dtsi | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index a0c57fb798d3..ff04397777f4 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -3579,6 +3579,32 @@ camnoc_virt: interconnect@ac00000 {
>  			qcom,bcm-voters = <&apps_bcm_voter>;
>  		};
>  
> +		dispcc: clock-controller@af00000 {
> +			compatible = "qcom,sm8150-dispcc";
> +			reg = <0 0x0af00000 0 0x10000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>;
> +			clock-names = "bi_tcxo",
> +				      "dsi0_phy_pll_out_byteclk",
> +				      "dsi0_phy_pll_out_dsiclk",
> +				      "dsi1_phy_pll_out_byteclk",
> +				      "dsi1_phy_pll_out_dsiclk",
> +				      "dp_phy_pll_link_clk",
> +				      "dp_phy_pll_vco_div_clk";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +
> +			power-domains = <&rpmhpd SM8150_MMCX>;
> +			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */

Is this still something we want to check/test?  Do we need a reference
manual to be sure?

> +			required-opps = <&rpmhpd_opp_low_svs>;
> +		};
> +
>  		pdc: interrupt-controller@b220000 {
>  			compatible = "qcom,sm8150-pdc", "qcom,pdc";
>  			reg = <0 0x0b220000 0 0x400>;
> -- 
> 2.38.1
>
Konrad Dybcio Dec. 12, 2022, 10:23 a.m. UTC | #3 
On 12.12.2022 11:18, Dmitry Baryshkov wrote:
> 
> 
> On 12 December 2022 12:33:13 GMT+03:00, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>> Years after the SoC support has been added, it's high time for it to
>> get dispcc going. Add the node to ensure that.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>> arch/arm64/boot/dts/qcom/sm8150.dtsi | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>> index a0c57fb798d3..ff04397777f4 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>> @@ -3579,6 +3579,32 @@ camnoc_virt: interconnect@ac00000 {
>> 			qcom,bcm-voters = <&apps_bcm_voter>;
>> 		};
>>
>> +		dispcc: clock-controller@af00000 {
>> +			compatible = "qcom,sm8150-dispcc";
>> +			reg = <0 0x0af00000 0 0x10000>;
>> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
>> +				 <0>,
>> +				 <0>,
>> +				 <0>,
>> +				 <0>,
>> +				 <0>,
>> +				 <0>;
>> +			clock-names = "bi_tcxo",
>> +				      "dsi0_phy_pll_out_byteclk",
>> +				      "dsi0_phy_pll_out_dsiclk",
>> +				      "dsi1_phy_pll_out_byteclk",
>> +				      "dsi1_phy_pll_out_dsiclk",
>> +				      "dp_phy_pll_link_clk",
>> +				      "dp_phy_pll_vco_div_clk";
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +			#power-domain-cells = <1>;
>> +
>> +			power-domains = <&rpmhpd SM8150_MMCX>;
>> +			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */
>> +			required-opps = <&rpmhpd_opp_low_svs>;
> 
> Is it required for the dispcc, for the DSI or for the dpu? We have stumbled upon the similar issue when working on the 8350, see the latest Robert's patchset.
While I don't have any hard evidence, it seems like it is required for
any "interesting" multimedia components, AFAIU even including video and
camera clocks..

Seems like it's a deep down dependency for a lot of things on this
particular SoC (and likely also on newer ones, remember the initial
mess with 8250 mmcx?)

Konrad
> 
> 
>> +		};
>> +
>> 		pdc: interrupt-controller@b220000 {
>> 			compatible = "qcom,sm8150-pdc", "qcom,pdc";
>> 			reg = <0 0x0b220000 0 0x400>;
>
Dmitry Baryshkov Dec. 12, 2022, 10:53 a.m. UTC | #4 
On 12 December 2022 13:23:50 GMT+03:00, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>On 12.12.2022 11:18, Dmitry Baryshkov wrote:
>> 
>> 
>> On 12 December 2022 12:33:13 GMT+03:00, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>> Years after the SoC support has been added, it's high time for it to
>>> get dispcc going. Add the node to ensure that.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>> arch/arm64/boot/dts/qcom/sm8150.dtsi | 26 ++++++++++++++++++++++++++
>>> 1 file changed, 26 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>>> index a0c57fb798d3..ff04397777f4 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>>> @@ -3579,6 +3579,32 @@ camnoc_virt: interconnect@ac00000 {
>>> 			qcom,bcm-voters = <&apps_bcm_voter>;
>>> 		};
>>>
>>> +		dispcc: clock-controller@af00000 {
>>> +			compatible = "qcom,sm8150-dispcc";
>>> +			reg = <0 0x0af00000 0 0x10000>;
>>> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
>>> +				 <0>,
>>> +				 <0>,
>>> +				 <0>,
>>> +				 <0>,
>>> +				 <0>,
>>> +				 <0>;
>>> +			clock-names = "bi_tcxo",
>>> +				      "dsi0_phy_pll_out_byteclk",
>>> +				      "dsi0_phy_pll_out_dsiclk",
>>> +				      "dsi1_phy_pll_out_byteclk",
>>> +				      "dsi1_phy_pll_out_dsiclk",
>>> +				      "dp_phy_pll_link_clk",
>>> +				      "dp_phy_pll_vco_div_clk";
>>> +			#clock-cells = <1>;
>>> +			#reset-cells = <1>;
>>> +			#power-domain-cells = <1>;
>>> +
>>> +			power-domains = <&rpmhpd SM8150_MMCX>;
>>> +			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */
>>> +			required-opps = <&rpmhpd_opp_low_svs>;
>> 
>> Is it required for the dispcc, for the DSI or for the dpu? We have stumbled upon the similar issue when working on the 8350, see the latest Robert's patchset.
>While I don't have any hard evidence, it seems like it is required for
>any "interesting" multimedia components, AFAIU even including video and
>camera clocks..
>
>Seems like it's a deep down dependency for a lot of things on this
>particular SoC (and likely also on newer ones, remember the initial
>mess with 8250 mmcx?)

Yes. I was questioning the opp level rather than the domain itself. Usually the least possible mmcx level is enough to get dispcc going. Then the consumers (via the PLLs) bump this requirement. 

In this particular case the vendor dtsi (sm8150-regulator.dtsi) declares that the minimum level for mmcx is low_svs. So, I think, you can drop the todo completely.


>
>Konrad
>> 
>> 
>>> +		};
>>> +
>>> 		pdc: interrupt-controller@b220000 {
>>> 			compatible = "qcom,sm8150-pdc", "qcom,pdc";
>>> 			reg = <0 0x0b220000 0 0x400>;
>>
Bjorn Andersson Dec. 28, 2022, 4:16 a.m. UTC | #5 
On Mon, Dec 12, 2022 at 10:33:13AM +0100, Konrad Dybcio wrote:
> Years after the SoC support has been added, it's high time for it to
> get dispcc going. Add the node to ensure that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8150.dtsi | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index a0c57fb798d3..ff04397777f4 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -3579,6 +3579,32 @@ camnoc_virt: interconnect@ac00000 {
>  			qcom,bcm-voters = <&apps_bcm_voter>;
>  		};
>  
> +		dispcc: clock-controller@af00000 {
> +			compatible = "qcom,sm8150-dispcc";
> +			reg = <0 0x0af00000 0 0x10000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>;
> +			clock-names = "bi_tcxo",
> +				      "dsi0_phy_pll_out_byteclk",
> +				      "dsi0_phy_pll_out_dsiclk",
> +				      "dsi1_phy_pll_out_byteclk",
> +				      "dsi1_phy_pll_out_dsiclk",
> +				      "dp_phy_pll_link_clk",
> +				      "dp_phy_pll_vco_div_clk";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +
> +			power-domains = <&rpmhpd SM8150_MMCX>;
> +			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */

The power-domain being not disabled should be sufficient for us to
access the dispcc. Beyond that votes would be needed for particular
frequencies, and that goes in the client nodes/opp-tables.

So you should be able to drop this comment and the required-opps.

Regards,
Bjorn

> +			required-opps = <&rpmhpd_opp_low_svs>;
> +		};
> +
>  		pdc: interrupt-controller@b220000 {
>  			compatible = "qcom,sm8150-pdc", "qcom,pdc";
>  			reg = <0 0x0b220000 0 0x400>;
> -- 
> 2.38.1
>
Marijn Suijten Dec. 28, 2022, 11:10 p.m. UTC | #6 
On 2022-12-27 22:16:58, Bjorn Andersson wrote:
> On Mon, Dec 12, 2022 at 10:33:13AM +0100, Konrad Dybcio wrote:
> > [..]
> > +			power-domains = <&rpmhpd SM8150_MMCX>;
> > +			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */
> 
> The power-domain being not disabled should be sufficient for us to
> access the dispcc. Beyond that votes would be needed for particular
> frequencies, and that goes in the client nodes/opp-tables.
> 
> So you should be able to drop this comment and the required-opps.
> 
> Regards,
> Bjorn
> 
> > +			required-opps = <&rpmhpd_opp_low_svs>;

Tested the removal of this on Xperia 5, no regressions.

- Marijn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index a0c57fb798d3..ff04397777f4 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -3579,6 +3579,32 @@  camnoc_virt: interconnect@ac00000 {
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
+		dispcc: clock-controller@af00000 {
+			compatible = "qcom,sm8150-dispcc";
+			reg = <0 0x0af00000 0 0x10000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <0>,
+				 <0>,
+				 <0>,
+				 <0>,
+				 <0>,
+				 <0>;
+			clock-names = "bi_tcxo",
+				      "dsi0_phy_pll_out_byteclk",
+				      "dsi0_phy_pll_out_dsiclk",
+				      "dsi1_phy_pll_out_byteclk",
+				      "dsi1_phy_pll_out_dsiclk",
+				      "dp_phy_pll_link_clk",
+				      "dp_phy_pll_vco_div_clk";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+
+			power-domains = <&rpmhpd SM8150_MMCX>;
+			/* TODO: Maybe rpmhpd_opp_min_svs could work as well? */
+			required-opps = <&rpmhpd_opp_low_svs>;
+		};
+
 		pdc: interrupt-controller@b220000 {
 			compatible = "qcom,sm8150-pdc", "qcom,pdc";
 			reg = <0 0x0b220000 0 0x400>;