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

[harigovi@codeaurora.org]

On 2020-02-01 01:02, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jan 28, 2020 at 5:25 AM Harigovindan P 
> <harigovi@codeaurora.org> wrote:
>> 
>> Add display, DSI hardware DT nodes for sc7180.
>> 
>> Signed-off-by: Harigovindan P <harigovi@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
>> 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?

"bus" is optional due to architecture change. Will update bindings in a 
new patch.

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

Yes the architecture has changed.

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

Not needed. The supply will be picked from parent node. I will be 
removing power-domain as required property from dsi bindings in a new 
patch.

> 
> 
>> +                       };
>> +               };
>> +
>>                 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]
> https://lore.kernel.org/r/20200130131220.v3.15.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid
> [2]
> https://lore.kernel.org/r/CAD=FV=WKVGq+x1XFvZQvBcKVPdcVxQWJJmsqpAxY3t4dorvMYg@mail.gmail.com/