Re: [PATCH v2] arm64: dts: qcom: sc7180: Add Display Port dt node

[tanmay@codeaurora.org]

Thanks Stephen for reviews.
I will post new change addressing your all comments.

On 2020-06-18 18:12, Stephen Boyd wrote:
> Quoting Tanmay Shah (2020-06-18 16:21:13)
>> Enable DP driver for sc7180.
> 
> Add DP device node on sc7180? This isn't a driver.
> 
Done.
>> 
>> This change depends-on following series:
>> https://patchwork.freedesktop.org/series/78583/
>> and https://patchwork.freedesktop.org/patch/351990/
>> 
>> Changes in v2:
>> 
>> - Add assigned-clocks and assigned-clock-parents
>> - Remove cell-index and pixel_rcg
>> - Change compatible to qcom,sc7180-dp
>> 
>> Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 57 
>> ++++++++++++++++++++++++++--
>>  1 file changed, 53 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 916401f7e87c..26fe623e3b0f 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -2216,10 +2216,19 @@ ports {
>>                                         #address-cells = <1>;
>>                                         #size-cells = <0>;
>> 
>> +                                       port@1 {
>> +                                               reg = <1>;
>> +                                               dpu_intf1_out: 
>> endpoint {
>> +                                                       
>> remote-endpoint =
>> +                                                                
>> <&dsi0_in>;
>> +                                               };
>> +                                       };
>> +
>>                                         port@0 {
>>                                                 reg = <0>;
>> -                                               dpu_intf1_out: 
>> endpoint {
>> -                                                       
>> remote-endpoint = <&dsi0_in>;
>> +                                               dpu_intf0_out: 
>> endpoint {
>> +                                                       
>> remote-endpoint =
>> +                                                                
>> <&dp_in>;
>>                                                 };
>>                                         };
>>                                 };
> 
> I thought this wasn't supposed to change? At least according to the
> binding it shouldn't be needed.
> 
Thanks. Yes it should be port@2 as per new dpu-sc7180.yaml bindings in 
below patch.
https://patchwork.freedesktop.org/patch/371087/?series=78583&rev=1

>> @@ -2293,6 +2302,46 @@ dsi_phy: dsi-phy@ae94400 {
>>                         };
>>                 };
>> 
>> +               msm_dp: displayport-controller@ae90000{
>> +                       status = "ok";
> 
> Please use status = "disabled";
> 
Ok got it. Board's DT will enable it.

>> +                       compatible = "qcom,sc7180-dp";
>> +
>> +                       reg = <0 0xae90000 0 0x1400>;
>> +                       reg-names = "dp_controller";
>> +
>> +                       interrupt-parent = <&mdss>;
> 
> Any reason why this isn't under the mdss node like the other display
> device nodes?
> 
Done. Moved msm_dp node under mdss.
>> +                       interrupts = <12 0>;
>> +
>> +                       clocks = <&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_aux", "ctrl_link",
>> +                                     "ctrl_link_iface", 
>> "stream_pixel";
>> +                       #clock-cells = <1>;
>> +                       assigned-clocks = <&dispcc 
>> DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +                       assigned-clock-parents = <&msm_dp 1>;
>> +
>> +                       data-lanes = <0 1>;
> 
> This can and should be left to the board files. At the SoC level my
> understanding is that there are four lanes possible, so no need to
> artificially limit it here.
> 
Removed from here. Driver changes I will be posting accordingly.
>> +
>> +                       ports {
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                               port@0 {
>> +                                       reg = <0>;
>> +                                       dp_in: endpoint {
>> +                                               remote-endpoint =
>> +                                                        
>> <&dpu_intf0_out>;
> 
> I'd prefer these were on one line, regardless of the 80 character line
> limit/suggestion.
> 
ok done.
>> +                                       };
>> +                               };
>> +
>> +                               port@1 {
>> +                                       reg = <1>;
>> +                                       dp_out: endpoint { };
>> +                               };
>> +                       };
>> +               };
>> +
>>                 dispcc: clock-controller@af00000 {
>>                         compatible = "qcom,sc7180-dispcc";
>>                         reg = <0 0x0af00000 0 0x200000>;