Messages in this thread |  | | Date | Tue, 04 Feb 2020 19:44:32 +0530 | From | harigovi@codeauro ... | Subject | Re: [v4] arm64: dts: sc7180: add display dt nodes |
| |
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/
|  |