[PATCH v4 1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file

[Doug Anderson]

Hi,

On Fri, Nov 2, 2018 at 3:26 PM Jeykumar Sankaran <jsanka at codeaurora.org> 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
> Changes in v4:
>          - move mdss node to preserve the unit address sort order
>          - remove _clk suffix from dsi clocks
>          (both the comments are from Doug Anderson)
>
> 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 b72bdb0..5728b4c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1248,6 +1248,197 @@
>                         };
>                 };
>
> +               mdss: mdss at ae00000 {
> +                       compatible = "qcom,sdm845-mdss";
> +                       reg = <0xae00000 0x1000>;
> +                       reg-names = "mdss";
> +
> +                       power-domains = <&dispcc 0>;

Could be done in a follow-up patch, but technically the above should be:

power-domains = <&dispcc MDSS_GDSC>;


> +                       dsi0: dsi at ae94000 {
> +                               compatible = "qcom,mdss-dsi-ctrl";
> +                               reg = <0xae94000 0x400>;
> +                               reg-names = "dsi_ctrl";
> +
> +                               interrupt-parent = <&mdss>;
> +                               interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;

Just out of curiousity, where does the 4 comes from?  I guess this matches:

#define MDSS_HW_INTR_STATUS_INTR_DSI0                           0x00000010
#define MDSS_HW_INTR_STATUS_INTR_DSI1                           0x00000020

...where 0x10 == (1 << 4)

I don't see any bindings for this define so I guess hardcoding it is
fine (and the 5 for DSI1).


> +                       dsi0_phy: dsi-phy at ae94400 {
> +                               compatible = "qcom,dsi-phy-10nm";
> +                               reg = <0xae94400 0x200>,
> +                                     <0xae94a00 0x1e0>,
> +                                     <0xae94600 0x280>;
> +                               reg-names = "dsi_phy",
> +                                           "dsi_pll",
> +                                           "dsi_phy_lane";
> +
> +                               #clock-cells = <1>;
> +                               #phy-cells = <0>;
> +
> +                               clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>;
> +                               clock-names = "iface";
> +                       };

+Matthias will probably want to base a future patch on this one since
he's trying to hook the clocks up more properly.  ...but what you have
above matches the current bindings so I think we're good.


Overall: I am not massively familiar with display / bridge / panel
bindings but as far as I can tell this patch is good and ready to
land.  Future work (like fixing the nit about using the power domain
#define) can always be done in a follow-up patch.  Thus:

Reviewed-by: Douglas Anderson <dianders at chromium.org>

I've tested this patch and it's helped me get a working display.  Thus:

Tested-by: Douglas Anderson <dianders at chromium.org>

-Doug