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

[tanmay]

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@xxxxxxxxxxxxxx>
> > ---
> >  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>;