Re: [PATCH] arm64: dts: qcom: sm8550-qrd: add display and panel

[Konrad Dybcio <konrad.dybcio@linaro.org>]

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
>