diff mbox series

arm64: dts: qcom: sm8550-qrd: add display and panel

Message ID 20230516151708.213744-1-krzysztof.kozlowski@linaro.org
State New
Headers
Series arm64: dts: qcom: sm8550-qrd: add display and panel |

Commit Message

Krzysztof Kozlowski May 16, 2023, 3:17 p.m. UTC 
  Enable Display Subsystem with Visionox VTDR6130 Panel (same as on
MTP8550).

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Context in the patch depends on:
1. https://lore.kernel.org/linux-arm-msm/20230516133011.108093-1-krzysztof.kozlowski@linaro.org/T/#t
2. https://lore.kernel.org/linux-arm-msm/20230512160452.206585-1-krzysztof.kozlowski@linaro.org/
---
 arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 76 +++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

Comments

Konrad Dybcio May 16, 2023, 3:20 p.m. UTC | #1 
On 16.05.2023 17:17, Krzysztof Kozlowski wrote:
> Enable Display Subsystem with Visionox VTDR6130 Panel (same as on
> MTP8550).
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Context in the patch depends on:
> 1. https://lore.kernel.org/linux-arm-msm/20230516133011.108093-1-krzysztof.kozlowski@linaro.org/T/#t
> 2. https://lore.kernel.org/linux-arm-msm/20230512160452.206585-1-krzysztof.kozlowski@linaro.org/
> ---
>  arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 76 +++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> index 30b36a149125..03bf6bc2db4d 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> @@ -420,6 +420,10 @@ vreg_l3g_1p2: ldo3 {
>  	};
>  };
>  
> +&dispcc {
> +	status = "okay";
> +};
Missed this in the bigpatchdrop review.. It makes no sense to keep
dispcc disabled by default (other than for lazily "solving" UEFI
framebuffer being shut down)

> +
>  &gcc {
>  	clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>  		 <&pcie0_phy>,
> @@ -431,6 +435,50 @@ &gcc {
>  		 <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>  };
>  
> +&mdss {
> +	status = "okay";
> +};
> +
> +&mdss_dsi0 {
> +	vdda-supply = <&vreg_l3e_1p2>;
> +	status = "okay";
> +
> +	panel@0 {
> +		compatible = "visionox,vtdr6130";
> +		reg = <0>;
> +
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&sde_dsi_active>, <&sde_te_active>;
> +		pinctrl-1 = <&sde_dsi_suspend>, <&sde_te_suspend>;
property-n
property-names

> +
> +		vddio-supply = <&vreg_l12b_1p8>;
> +		vci-supply = <&vreg_l13b_3p0>;
> +		vdd-supply = <&vreg_l11b_1p2>;
> +
> +		reset-gpios = <&tlmm 133 GPIO_ACTIVE_LOW>;
> +
> +		port {
> +			panel0_in: endpoint {
> +				remote-endpoint = <&mdss_dsi0_out>;
> +			};
> +		};
> +	};
> +};
> +
> +&mdss_dsi0_out {
> +	remote-endpoint = <&panel0_in>;
> +	data-lanes = <0 1 2 3>;
> +};
> +
> +&mdss_dsi0_phy {
> +	vdds-supply = <&vreg_l1e_0p88>;
> +	status = "okay";
> +};
> +
> +&mdss_mdp {
> +	status = "okay";
> +};
This should also be enabled by default, MDSS is useless when MDP is
disabled.

lgtm otherwise

Konrad

> +
>  &pcie_1_phy_aux_clk {
>  	status = "disabled";
>  };
> @@ -532,6 +580,34 @@ wcd_tx: codec@0,3 {
>  &tlmm {
>  	gpio-reserved-ranges = <32 8>;
>  
> +	sde_dsi_active: sde-dsi-active-state {
> +		pins = "gpio133";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		bias-disable;
> +	};
> +
> +	sde_dsi_suspend: sde-dsi-suspend-state {
> +		pins = "gpio133";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	sde_te_active: sde-te-active-state {
> +		pins = "gpio86";
> +		function = "mdp_vsync";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	sde_te_suspend: sde-te-suspend-state {
> +		pins = "gpio86";
> +		function = "mdp_vsync";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
>  	wcd_default: wcd-reset-n-active-state {
>  		pins = "gpio108";
>  		function = "gpio";
Krzysztof Kozlowski May 16, 2023, 3:23 p.m. UTC | #2 
On 16/05/2023 17:20, Konrad Dybcio wrote:
> 
> 
> On 16.05.2023 17:17, Krzysztof Kozlowski wrote:
>> Enable Display Subsystem with Visionox VTDR6130 Panel (same as on
>> MTP8550).
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Context in the patch depends on:
>> 1. https://lore.kernel.org/linux-arm-msm/20230516133011.108093-1-krzysztof.kozlowski@linaro.org/T/#t
>> 2. https://lore.kernel.org/linux-arm-msm/20230512160452.206585-1-krzysztof.kozlowski@linaro.org/
>> ---
>>  arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 76 +++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
>> index 30b36a149125..03bf6bc2db4d 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
>> +++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
>> @@ -420,6 +420,10 @@ vreg_l3g_1p2: ldo3 {
>>  	};
>>  };
>>  
>> +&dispcc {
>> +	status = "okay";
>> +};
> Missed this in the bigpatchdrop review.. It makes no sense to keep
> dispcc disabled by default (other than for lazily "solving" UEFI
> framebuffer being shut down)

Sure.

> 
>> +
>>  &gcc {
>>  	clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>>  		 <&pcie0_phy>,
>> @@ -431,6 +435,50 @@ &gcc {
>>  		 <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>>  };
>>  
>> +&mdss {
>> +	status = "okay";
>> +};
>> +
>> +&mdss_dsi0 {
>> +	vdda-supply = <&vreg_l3e_1p2>;
>> +	status = "okay";
>> +
>> +	panel@0 {
>> +		compatible = "visionox,vtdr6130";
>> +		reg = <0>;
>> +
>> +		pinctrl-names = "default", "sleep";
>> +		pinctrl-0 = <&sde_dsi_active>, <&sde_te_active>;
>> +		pinctrl-1 = <&sde_dsi_suspend>, <&sde_te_suspend>;
> property-n
> property-names

Sure, copy-pasta from MTP8550.

>> +
>> +&mdss_mdp {
>> +	status = "okay";
>> +};
> This should also be enabled by default, MDSS is useless when MDP is
> disabled.

But don't we want to disable both when display is not used (not connected)?

Best regards,
Krzysztof
Konrad Dybcio May 16, 2023, 3:26 p.m. UTC | #3 
On 16.05.2023 17:23, Krzysztof Kozlowski wrote:
> On 16/05/2023 17:20, Konrad Dybcio wrote:
>>
>>
>> On 16.05.2023 17:17, Krzysztof Kozlowski wrote:
>>> Enable Display Subsystem with Visionox VTDR6130 Panel (same as on
>>> MTP8550).
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>
>>> ---
>>>
>>> Context in the patch depends on:
>>> 1. https://lore.kernel.org/linux-arm-msm/20230516133011.108093-1-krzysztof.kozlowski@linaro.org/T/#t
>>> 2. https://lore.kernel.org/linux-arm-msm/20230512160452.206585-1-krzysztof.kozlowski@linaro.org/
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 76 +++++++++++++++++++++++++
>>>  1 file changed, 76 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
>>> index 30b36a149125..03bf6bc2db4d 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
>>> @@ -420,6 +420,10 @@ vreg_l3g_1p2: ldo3 {
>>>  	};
>>>  };
>>>  
>>> +&dispcc {
>>> +	status = "okay";
>>> +};
>> Missed this in the bigpatchdrop review.. It makes no sense to keep
>> dispcc disabled by default (other than for lazily "solving" UEFI
>> framebuffer being shut down)
> 
> Sure.
> 
>>
>>> +
>>>  &gcc {
>>>  	clocks = <&bi_tcxo_div2>, <&sleep_clk>,
>>>  		 <&pcie0_phy>,
>>> @@ -431,6 +435,50 @@ &gcc {
>>>  		 <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
>>>  };
>>>  
>>> +&mdss {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&mdss_dsi0 {
>>> +	vdda-supply = <&vreg_l3e_1p2>;
>>> +	status = "okay";
>>> +
>>> +	panel@0 {
>>> +		compatible = "visionox,vtdr6130";
>>> +		reg = <0>;
>>> +
>>> +		pinctrl-names = "default", "sleep";
>>> +		pinctrl-0 = <&sde_dsi_active>, <&sde_te_active>;
>>> +		pinctrl-1 = <&sde_dsi_suspend>, <&sde_te_suspend>;
>> property-n
>> property-names
> 
> Sure, copy-pasta from MTP8550.
> 
>>> +
>>> +&mdss_mdp {
>>> +	status = "okay";
>>> +};
>> This should also be enabled by default, MDSS is useless when MDP is
>> disabled.
> 
> But don't we want to disable both when display is not used (not connected)?
The MDSS bus device only has a 0x1000 slice of the 0x90000-long "full MDSS",
the rest is probed with MDP/DPU. It also calls of_something_populate that
make DSI, DSIPHY and DP/HDMI probe. But all of them ultimately need a graph
handle to MDP.

If we have a display (of any kind), MDP has to be enabled (or the display
engine will not have a way to be programmed).

If we don't, enabling MDSS makes no sense as all of the hardware will be
shut down right after probing.

So I'd say either both or none.

Konrad
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski May 16, 2023, 3:35 p.m. UTC | #4 
On 16/05/2023 17:26, Konrad Dybcio wrote:
>>>> +&mdss_mdp {
>>>> +	status = "okay";
>>>> +};
>>> This should also be enabled by default, MDSS is useless when MDP is
>>> disabled.
>>
>> But don't we want to disable both when display is not used (not connected)?
> The MDSS bus device only has a 0x1000 slice of the 0x90000-long "full MDSS",
> the rest is probed with MDP/DPU. It also calls of_something_populate that
> make DSI, DSIPHY and DP/HDMI probe. But all of them ultimately need a graph
> handle to MDP.
> 
> If we have a display (of any kind), MDP has to be enabled (or the display
> engine will not have a way to be programmed).
> 
> If we don't, enabling MDSS makes no sense as all of the hardware will be
> shut down right after probing.
> 
> So I'd say either both or none.

Yes, so the current state - both disabled - is matching it.

Best regards,
Krzysztof
Konrad Dybcio May 16, 2023, 3:36 p.m. UTC | #5 
On 16.05.2023 17:35, Krzysztof Kozlowski wrote:
> On 16/05/2023 17:26, Konrad Dybcio wrote:
>>>>> +&mdss_mdp {
>>>>> +	status = "okay";
>>>>> +};
>>>> This should also be enabled by default, MDSS is useless when MDP is
>>>> disabled.
>>>
>>> But don't we want to disable both when display is not used (not connected)?
>> The MDSS bus device only has a 0x1000 slice of the 0x90000-long "full MDSS",
>> the rest is probed with MDP/DPU. It also calls of_something_populate that
>> make DSI, DSIPHY and DP/HDMI probe. But all of them ultimately need a graph
>> handle to MDP.
>>
>> If we have a display (of any kind), MDP has to be enabled (or the display
>> engine will not have a way to be programmed).
>>
>> If we don't, enabling MDSS makes no sense as all of the hardware will be
>> shut down right after probing.
>>
>> So I'd say either both or none.
> 
> Yes, so the current state - both disabled - is matching it.
Right, but what i was trying to say is that if we leave MDP without
any status properties, it will follow MDSS.

Konrad
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski May 16, 2023, 3:39 p.m. UTC | #6 
On 16/05/2023 17:36, Konrad Dybcio wrote:
>>>> But don't we want to disable both when display is not used (not connected)?
>>> The MDSS bus device only has a 0x1000 slice of the 0x90000-long "full MDSS",
>>> the rest is probed with MDP/DPU. It also calls of_something_populate that
>>> make DSI, DSIPHY and DP/HDMI probe. But all of them ultimately need a graph
>>> handle to MDP.
>>>
>>> If we have a display (of any kind), MDP has to be enabled (or the display
>>> engine will not have a way to be programmed).
>>>
>>> If we don't, enabling MDSS makes no sense as all of the hardware will be
>>> shut down right after probing.
>>>
>>> So I'd say either both or none.
>>
>> Yes, so the current state - both disabled - is matching it.
> Right, but what i was trying to say is that if we leave MDP without
> any status properties, it will follow MDSS.

Ah, I missed that it is MDSS child, so indeed there is no point to have
it explicitly disabled. I'll fix it.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
index 30b36a149125..03bf6bc2db4d 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
@@ -420,6 +420,10 @@  vreg_l3g_1p2: ldo3 {
 	};
 };
 
+&dispcc {
+	status = "okay";
+};
+
 &gcc {
 	clocks = <&bi_tcxo_div2>, <&sleep_clk>,
 		 <&pcie0_phy>,
@@ -431,6 +435,50 @@  &gcc {
 		 <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
 };
 
+&mdss {
+	status = "okay";
+};
+
+&mdss_dsi0 {
+	vdda-supply = <&vreg_l3e_1p2>;
+	status = "okay";
+
+	panel@0 {
+		compatible = "visionox,vtdr6130";
+		reg = <0>;
+
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&sde_dsi_active>, <&sde_te_active>;
+		pinctrl-1 = <&sde_dsi_suspend>, <&sde_te_suspend>;
+
+		vddio-supply = <&vreg_l12b_1p8>;
+		vci-supply = <&vreg_l13b_3p0>;
+		vdd-supply = <&vreg_l11b_1p2>;
+
+		reset-gpios = <&tlmm 133 GPIO_ACTIVE_LOW>;
+
+		port {
+			panel0_in: endpoint {
+				remote-endpoint = <&mdss_dsi0_out>;
+			};
+		};
+	};
+};
+
+&mdss_dsi0_out {
+	remote-endpoint = <&panel0_in>;
+	data-lanes = <0 1 2 3>;
+};
+
+&mdss_dsi0_phy {
+	vdds-supply = <&vreg_l1e_0p88>;
+	status = "okay";
+};
+
+&mdss_mdp {
+	status = "okay";
+};
+
 &pcie_1_phy_aux_clk {
 	status = "disabled";
 };
@@ -532,6 +580,34 @@  wcd_tx: codec@0,3 {
 &tlmm {
 	gpio-reserved-ranges = <32 8>;
 
+	sde_dsi_active: sde-dsi-active-state {
+		pins = "gpio133";
+		function = "gpio";
+		drive-strength = <8>;
+		bias-disable;
+	};
+
+	sde_dsi_suspend: sde-dsi-suspend-state {
+		pins = "gpio133";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	sde_te_active: sde-te-active-state {
+		pins = "gpio86";
+		function = "mdp_vsync";
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	sde_te_suspend: sde-te-suspend-state {
+		pins = "gpio86";
+		function = "mdp_vsync";
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
 	wcd_default: wcd-reset-n-active-state {
 		pins = "gpio108";
 		function = "gpio";