[v3] arm64: dts: sc7180: add display dt nodes

[Doug Anderson]

Hi,

On Fri, Jan 24, 2020 at 4:07 AM Harigovindan P <harigovi at codeaurora.org> wrote:
>
> Add display, DSI hardware DT nodes for sc7180.
>
> Signed-off-by: Harigovindan P <harigovi at codeaurora.org>
> ---
>
> Changes in v1:
>         -Added display DT nodes for sc7180
> Changes in v2:
>         -Renamed node names
>         -Corrected code alignments
>         -Removed extra new line
>         -Added DISP AHB clock for register access
>         under display_subsystem node for global settings
> Changes in v3:
>         -Modified node names
>         -Modified hard coded values
>         -Removed mdss reg entry
>
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts |  58 +++++++++++++++
>  arch/arm64/boot/dts/qcom/sc7180.dtsi    | 124 ++++++++++++++++++++++++++++++++
>  2 files changed, 182 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 388f50a..c77aab7 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -7,6 +7,7 @@
>
>  /dts-v1/;
>
> +#include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>  #include "sc7180.dtsi"
>  #include "pm6150.dtsi"
> @@ -232,6 +233,50 @@
>         };
>  };
>
> +&dsi_controller {
> +       status = "okay";
> +
> +       vdda-supply = <&vreg_l3c_1p2>;
> +
> +       panel at 0 {
> +               compatible = "visionox,rm69299-1080p-display";

I don't think the bindings for this panel have landed anywhere, have
they?  Maybe keep the IDP patch separate from the main sc7180 patch so
that we can land the main sc7180 patch even if the idp patch isn't
quite ready?  ...and maybe ping whoever is supposed to add support for
this panel to tell them to get working on it.


> +               reg = <0>;
> +
> +               vdda-supply = <&vreg_l8c_1p8>;
> +               vdd3p3-supply = <&vreg_l18a_2p8>;
> +
> +               pinctrl-names = "default", "suspend";
> +               pinctrl-0 = <&disp_pins_default>;
> +               pinctrl-1 = <&disp_pins_default>;
> +
> +               reset-gpios = <&pm6150l_gpio 3 GPIO_ACTIVE_HIGH>;
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       port at 0 {
> +                               reg = <0>;
> +                               panel0_in: endpoint {
> +                                       remote-endpoint = <&dsi0_out>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       ports {
> +               port at 1 {
> +                       endpoint {
> +                               remote-endpoint = <&panel0_in>;
> +                               data-lanes = <0 1 2 3>;
> +                       };
> +               };
> +       };
> +};
> +
> +&dsi_phy {
> +       status = "okay";

The above doesn't do anything because the dsi_phy you add to
sc7180.dtsi doesn't have a
  status = "disabled";

...but probably it (and most of the other components that you're
adding) should.  The idea is that if it ever makes sense that a board
might be built with this SoC but _not_ hook up a given component that
it should start out "disabled" in the main SoC dtsi file.

> +};
> +
>  &qspi {
>         status = "okay";
>         pinctrl-names = "default";
> @@ -289,6 +334,19 @@
>
>  /* PINCTRL - additions to nodes defined in sc7180.dtsi */
>
> +&pm6150l_gpio {
> +       disp_pins {
> +               disp_pins_default: disp_pins_default{

As Bjorn mentioned, node name should use "-" instead of "_".  Also add
a space before your "{"


> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 3bc3f64..3ebc45b 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1184,6 +1184,130 @@
>                         #power-domain-cells = <1>;
>                 };
>
> +               mdss: display_subsystem at ae00000 {
> +                       compatible = "qcom,sc7180-mdss";
> +                       reg = <0 0x0ae00000 0 0x1000>;
> +                       reg-names = "mdss";
> +
> +                       power-domains = <&dispcc MDSS_GDSC>;

You definitely can't land your patch until the "dispcc" node is added,
but it's not.  You should be mentioning somewhere (after the cut?)
that you depend on the series to add dispcc.

...speaking of which, I just posted up a v2 of that.  See:

https://lore.kernel.org/r/20200124144154.v2.10.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid

...speaking of which, can you please change your patch to replace the
bogus <0> in the dispcc for the DSI PHY, providing the clocks for
"dsi_phy_pll_byte" and "dsi_phy_pll_pixel"?  See
<https://crrev.com/c/2017974/3>


> +
> +                       clocks = <&gcc GCC_DISP_AHB_CLK>,
> +                                <&gcc GCC_DISP_HF_AXI_CLK>,
> +                                <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +                                <&dispcc DISP_CC_MDSS_MDP_CLK>;
> +                       clock-names = "iface", "gcc_bus", "ahb", "core";
> +
> +                       assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>;
> +                       assigned-clock-rates = <300000000>;
> +
> +                       interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> +                       interrupt-controller;
> +                       #interrupt-cells = <1>;
> +
> +                       iommus = <&apps_smmu 0x800 0x2>;
> +
> +                       #address-cells = <2>;
> +                       #size-cells = <2>;
> +                       ranges;
> +
> +                       mdp: display_controller at ae01000 {

Did you test this?  As far I can tell this change between v2 and v3
broke things because the node name "mdp" is magic.  In
"drivers/gpu/drm/msm/msm_drv.c" you can find the function:

static int compare_name_mdp(struct device *dev, void *data)
{
  return (strstr(dev_name(dev), "mdp") != NULL);
}

...so unless that function changes your device tree won't work.  Maybe
Bjorn can comment since I think he's the one that suggested you change
this, but IMO change it back to "mdp at ae01000" for now and then start a
separate patch series about transitioning this if people still want
it.  See <https://crrev.com/c/2020395/1>.

-Doug