[Freedreno] [PATCH v3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file

[Doug Anderson]

Hi,

On Wed, Nov 28, 2018 at 3:44 PM Jordan Crouse <jcrouse at codeaurora.org> wrote:
>
> On Thu, Nov 01, 2018 at 07:25:23PM -0700, Jeykumar Sankaran wrote:
> > DPU is short for the Display Processing Unit. It is the display
> > controller on Qualcomm SDM845 chips.
> >
> > This change adds MDSS and DSI nodes to enable display on the
> > target device.
> >
> > Changes in v2:
> >        - Beefed up commit message
> >        - Use SoC specific compatibles for mdss and dpu (Rob H)
> >        - Use assigned-clocks to set initial clock frequency(Rob H)
> > Changes in v3:
> >        - added IOMMU node
> >        - Fix device naming (remove _phys)
> >        - Use correct IRQ_TYPE in interrupt specifiers
> >
> > Signed-off-by: Jeykumar Sankaran <jsanka at codeaurora.org>
> > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 191 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 191 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 0c9a2aa..dd612ac 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -978,6 +978,197 @@
> >                       #thermal-sensor-cells = <1>;
> >               };
> >
> > +             mdss: mdss at ae00000 {
> > +                     compatible = "qcom,sdm845-mdss";
> > +                     reg = <0xae00000 0x1000>;
> > +                     reg-names = "mdss";
> > +
> > +                     power-domains = <&dispcc 0>;
>
> Would the powers-that-be in the power domain prefer this to have a symbolic
> value? I see MDSS_GDSC in the bindings header.

You're one patch version behind.  Can you look at:

https://patchwork.kernel.org/patch/10666253/ AKA
https://lkml.kernel.org/r/1541197576-19730-2-git-send-email-jsanka@codeaurora.org

...that addresses the _clk issues and I commented about the #define too.


> > +                             phys = <&dsi0_phy>;
> > +                             phy-names = "dsi-phy";
>
> This too, I reckon.

I didn't notice this one, so it's still "dsi-phy" in v4.


> > +                     dsi0_phy: dsi-phy at ae94400 {
> > +                             compatible = "qcom,dsi-phy-10nm";
> > +                             reg = <0xae94400 0x200>,
> > +                                   <0xae94a00 0x1e0>,
> > +                                   <0xae94600 0x280>;
>
> I think it would be better if these were in numerical order.

Still broken in v4.


Also Bjorn's thought about adding a 'status = "disabled"' also makes sense.

So I guess it's time for a v5 Jeykumar.


-Doug