Re: [v4] arm64: dts: sc7180: add display dt nodes

[Doug Anderson]

Hi,

On Tue, Jan 28, 2020 at 5:25 AM Harigovindan P <harigovi@xxxxxxxxxxxxxx> wrote:
>
> Add display, DSI hardware DT nodes for sc7180.
>
> Signed-off-by: Harigovindan P <harigovi@xxxxxxxxxxxxxx>
> ---
>
> 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
> Changes in v4:
>         -Reverting mdp node name
>         -Setting status to disabled in main SOC dtsi file
>         -Replacing _ to - for node names
>         -Adding clock dependency patch link
>         -Splitting idp dt file to a separate patch
>
> This patch has dependency on the below series
> https://lkml.org/lkml/2019/12/27/73
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 128 +++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 3bc3f64..c3883af 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1184,6 +1184,134 @@
>                         #power-domain-cells = <1>;
>                 };
>
> +               mdss: mdss@ae00000 {
> +                       compatible = "qcom,sc7180-mdss";
> +                       reg = <0 0x0ae00000 0 0x1000>;
> +                       reg-names = "mdss";
> +
> +                       power-domains = <&dispcc MDSS_GDSC>;
> +
> +                       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";

The clock "ahb" is not in your bindings.  If it is truly needed,
please update your bindings.

The clock "gcc_bus" is just called "bus" in the bindings.  Assuming
this is the same clock, please use the name from the bindings.


> +                       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;

Do we need a:
  status = "disabled";

I noticed that in sdm845 the top-level mdss node _does_ have that.  If
someone was building a board with your chip and they had no display at
all, would they want this node disabled?  If so then it should be
disabled by default and boards should opt-in.


> +                       mdss_mdp: mdp@ae01000 {
> +                               compatible = "qcom,sc7180-dpu";
> +                               reg = <0 0x0ae01000 0 0x8f000>,
> +                                     <0 0x0aeb0000 0 0x2008>,
> +                                     <0 0x0af03000 0 0x16>;
> +                               reg-names = "mdp", "vbif", "disp_cc";

Did I already ask why you need the "disp_cc" register space?  If I
didn't, can I ask now?  This is not in the bindings and I can't think
of why you'd want it.  Does the code use it?  It doesn't seem to...


> +                               clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_ROT_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_MDP_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
> +                               clock-names = "iface", "rot", "lut", "core",
> +                                             "vsync";

Your bindings doc says that "bus" is required, yet you don't pass it.
Should you?

Your bindings doc says nothing about "rot" and "lut".  Presumably
those should be added if they are actually needed?


> +                               assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> +                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
> +                               assigned-clock-rates = <300000000>,
> +                                                      <19200000>;
> +
> +                               interrupt-parent = <&mdss>;
> +                               interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;

Do we need a:
  status = "disabled";

I noticed that in sdm845 the mdss_mdp node _does_ have that.  NOTE:
you'd only want to add it if it ever made sense to turn on the
top-level mdss node but not this one.  If this should always be
enabled at the exact same time as the top-level mdss node then there's
no need to add the 'status = "disabled";'

If you decide that you don't need to add this, maybe you can submit a
separate patch to remove it from sdm845?


> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       port@0 {
> +                                               reg = <0>;
> +                                               dpu_intf1_out: endpoint {
> +                                                       remote-endpoint = <&dsi0_in>;
> +                                               };
> +                                       };
> +                               };
> +                       };
> +
> +                       dsi_controller: dsi-controller@ae94000 {

nit: Though "dsi-controller" is a sensible name, current binding
examples show "dsi", not "dsi-controller".  The name "dsi" seems
blessed by Rob Herring since it came from commit a3c463e0961c
("dt-bindings: msm/dsi: Some binding doc cleanups") which has his Ack,
so I'd rather go with that.


> +                               compatible = "qcom,mdss-dsi-ctrl";
> +                               reg = <0 0x0ae94000 0 0x400>;
> +                               reg-names = "dsi_ctrl";
> +
> +                               interrupt-parent = <&mdss>;
> +                               interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
> +
> +                               clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_PCLK0_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_ESC0_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +                                        <&gcc GCC_DISP_HF_AXI_CLK>;

Comparing with sdm845 I notice that the last clock used to come from
dispcc.  Now you're getting it from gcc.  Did the architecture
actually change or are you working around a clock that should be
exported by the dispcc but hasn't been finished yet?


> +                               clock-names = "byte",
> +                                             "byte_intf",
> +                                             "pixel",
> +                                             "core",
> +                                             "iface",
> +                                             "bus";

Your bindings doc says this about which clocks you need:

- clock-names: the following clocks are required:
  * "mdp_core"
  * "iface"
  * "bus"
  * "core_mmss"
  * "byte"
  * "pixel"
  * "core"
  For DSIv2, we need an additional clock:
   * "src"
  For DSI6G v2.0 onwards, we need also need the clock:
   * "byte_intf"

...seems like either the binding is wrong or you're missing a few
clocks.  Which is it?


> +                               phys = <&dsi_phy>;
> +                               phy-names = "dsi";
> +
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               status = "disabled";
> +
> +                               ports {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       port@0 {
> +                                               reg = <0>;
> +                                               dsi0_in: endpoint {
> +                                                       remote-endpoint = <&dpu_intf1_out>;
> +                                               };
> +                                       };
> +
> +                                       port@1 {
> +                                               reg = <1>;
> +                                               dsi0_out: endpoint {
> +                                               };
> +                                       };
> +                               };
> +                       };
> +
> +                       dsi_phy: dsi-phy@ae94400 {
> +                               compatible = "qcom,dsi-phy-10nm";
> +                               reg = <0 0x0ae94400 0 0x200>,
> +                                     <0 0x0ae94600 0 0x280>,
> +                                     <0 0x0ae94a00 0 0x1e0>;
> +                               reg-names = "dsi_phy",
> +                                           "dsi_phy_lane",
> +                                           "dsi_pll";
> +
> +                               #clock-cells = <1>;
> +                               #phy-cells = <0>;
> +
> +                               clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>;
> +                               clock-names = "iface";

Your bindings say:

- clock-names: the following clocks are required:
  * "iface"
  * "ref" (only required for new DTS files/entries)

I think you qualify as a "new" DTS file, so you should be providing "ref".


> +                               status = "disabled";

Bindings list "power-domains" as a required property.  Should the
bindings be updated to make this optional, or should you be adding it?


> +                       };
> +               };
> +
>                 pdc: interrupt-controller@b220000 {

nit: your sorting is still slightly off.  I can certainly apply your
patch atop the dispcc device tree patch [1] now, which is good.  But
the context clue in your patch that your stuff comes right before the
'pdc: interrupt-controller@b220000' means that you are being placed
_after_ 'dispcc: clock-controller@af00000'.  You should be before it
since ae00000 < af00000.

...this may sound like making a big deal out of nothing, but keeping
things sorted correctly is the best way to reduce merge conflicts when
landing patches and that's a big deal.

--

Also, in response to your last patch [2] I said:

> ...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>

It doesn't appear that you've done this.  Can you?


NOTE: as you can probably guess, my review was mostly this:
- Compare your nodes with the nodes in a similar SoC (sdm845).
- Compare your nodes with the examples in the bindings.
- Compare your nodes with the text of the bindings.

Those are good things for you to do before you send out future patches
to help make sure you didn't miss anything.


-Doug

[1] 20200130131220.v3.15.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid">https://lore.kernel.org/r/20200130131220.v3.15.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid
[2] https://lore.kernel.org/r/CAD=FV=WKVGq+x1XFvZQvBcKVPdcVxQWJJmsqpAxY3t4dorvMYg@xxxxxxxxxxxxxx/