| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <384572129dc818773eb0ae37617ff8a1@codeaurora.org>
Date: Tue, 04 Feb 2020 19:44:32 +0530
From: harigovi@...eaurora.org
To: Doug Anderson <dianders@...omium.org>
Cc: dri-devel <dri-devel@...ts.freedesktop.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
freedreno <freedreno@...ts.freedesktop.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
Rob Clark <robdclark@...il.com>,
Sean Paul <seanpaul@...omium.org>,
"Kristian H. Kristensen" <hoegsberg@...omium.org>,
Kalyan Thota <kalyan_t@...eaurora.org>, nganji@...eaurora.org
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@...eaurora.org> wrote:
>>
>> Add display, DSI hardware DT nodes for sc7180.
>>
>> Signed-off-by: Harigovindan P <harigovi@...eaurora.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@...0000 {
>> + 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@...1000 {
>> + 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@...4000 {
>
> 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@...4400 {
>> + 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@...0000 {
>
> 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@...0000' means that you are being placed
> _after_ 'dispcc: clock-controller@...0000'. 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/
Powered by blists - more mailing lists