Messages in this thread | | | Date | Wed, 18 Jan 2023 11:50:20 +0000 | Subject | Re: [PATCH v3 5/8] arm64: dts: qcom: Add msm8939 SoC | From | Bryan O'Donoghue <> |
| |
On 18/01/2023 09:59, Stephan Gerhold wrote: > On Tue, Jan 17, 2023 at 02:48:43AM +0000, Bryan O'Donoghue wrote: >> Add msm8939 a derivative SoC of msm8916. This SoC contains a number of key >> differences to msm8916. >> >> - big.LITTLE Octa Core - quad 1.5GHz + quad 1.0GHz >> - DRAM 1x800 LPDDR3 >> - Camera 4+4 lane CSI >> - Venus @ 1080p60 HEVC >> - DSI x 2 >> - Adreno A405 >> - WiFi wcn3660/wcn3680b 802.11ac >> >> Co-developed-by: Shawn Guo <shawn.guo@linaro.org> >> Signed-off-by: Shawn Guo <shawn.guo@linaro.org> >> Co-developed-by: Jun Nie <jun.nie@linaro.org> >> Signed-off-by: Jun Nie <jun.nie@linaro.org> >> Co-developed-by: Benjamin Li <benl@squareup.com> >> Signed-off-by: Benjamin Li <benl@squareup.com> >> Co-developed-by: James Willcox <jwillcox@squareup.com> >> Signed-off-by: James Willcox <jwillcox@squareup.com> >> Co-developed-by: Leo Yan <leo.yan@linaro.org> >> Signed-off-by: Leo Yan <leo.yan@linaro.org> >> Co-developed-by: Joseph Gates <jgates@squareup.com> >> Signed-off-by: Joseph Gates <jgates@squareup.com> >> Co-developed-by: Max Chen <mchen@squareup.com> >> Signed-off-by: Max Chen <mchen@squareup.com> >> Co-developed-by: Zac Crosby <zac@squareup.com> >> Signed-off-by: Zac Crosby <zac@squareup.com> >> Co-developed-by: Vincent Knecht <vincent.knecht@mailoo.org> >> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org> >> Co-developed-by: Stephan Gerhold <stephan@gerhold.net> >> Signed-off-by: Stephan Gerhold <stephan@gerhold.net> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> arch/arm64/boot/dts/qcom/msm8939.dtsi | 2393 +++++++++++++++++++++++++ >> 1 file changed, 2393 insertions(+) >> create mode 100644 arch/arm64/boot/dts/qcom/msm8939.dtsi >> >> diff --git a/arch/arm64/boot/dts/qcom/msm8939.dtsi b/arch/arm64/boot/dts/qcom/msm8939.dtsi >> new file mode 100644 >> index 0000000000000..8cd358a9fe623 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/msm8939.dtsi >> @@ -0,0 +1,2393 @@ >> [...] >> + cpus { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + cpu0: cpu@100 { >> + compatible = "arm,cortex-a53"; >> + device_type = "cpu"; >> + enable-method = "spin-table"; >> + reg = <0x100>; >> + next-level-cache = <&L2_1>; >> + power-domains = <&vreg_dummy>; >> + power-domain-names = "cpr"; > > Why are you adding a dummy power domain here? IMO this would be better > added together with CPR. Especially because I would expect two power > domains here later ("mx", "cpr"). For cpufreq you also need to make > votes for the "MSM8939_VDDMX" power domain.
I'm pretty sure there's binding checks that demand this but, I will re-verify it.
> >> + qcom,acc = <&acc0>; >> + qcom,saw = <&saw0>; >> + cpu-idle-states = <&CPU_SLEEP_0>; >> + clocks = <&apcs1_mbox>; >> + #cooling-cells = <2>; >> + L2_1: l2-cache { >> + compatible = "cache"; >> + cache-level = <2>; >> + }; >> + }; >> [...] >> + soc: soc@0 { >> + compatible = "simple-bus"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges = <0 0 0 0xffffffff>; >> + >> + rng@22000 { >> + compatible = "qcom,prng"; >> + reg = <0x00022000 0x200>; >> + clocks = <&gcc GCC_PRNG_AHB_CLK>; >> + clock-names = "core"; >> + }; >> + >> + qfprom: qfprom@5c000 { >> + compatible = "qcom,msm8916-qfprom", "qcom,qfprom"; >> + reg = <0x0005c000 0x1000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + tsens_caldata: caldata@a0 { >> + reg = <0xa0 0x5c>; >> + }; >> + cpr_efuse_init_voltage1: ivoltage1@dc { >> + reg = <0xdc 0x4>; >> + bits = <4 6>; >> + }; >> + cpr_efuse_init_voltage2: ivoltage2@da { >> + reg = <0xda 0x4>; >> + bits = <2 6>; >> + }; >> + cpr_efuse_init_voltage3: ivoltage3@d8 { >> + reg = <0xd8 0x4>; >> + bits = <0 6>; >> + }; >> + cpr_efuse_quot1: quot1@dd { >> + reg = <0xdd 0x8>; >> + bits = <2 12>; >> + }; >> + cpr_efuse_quot2: quot2@db { >> + reg = <0xdb 0x8>; >> + bits = <0x0 12>; >> + }; >> + cpr_efuse_ring1: ring1@de { >> + reg = <0xde 0x4>; >> + bits = <6 3>; >> + }; >> + cpr_efuse_revision: revision@5 { >> + reg = <0x5 0x1>; >> + bits = <5 1>; >> + }; >> + cpr_efuse_revision_high: revision-high@7 { >> + reg = <0x7 0x1>; >> + bits = <0 1>; >> + }; >> + cpr_efuse_pvs_version: pvs@3 { >> + reg = <0x3 0x1>; >> + bits = <5 1>; >> + }; >> + cpr_efuse_pvs_version_high: pvs-high@6 { >> + reg = <0x6 0x1>; >> + bits = <2 2>; >> + }; >> + cpr_efuse_speedbin: speedbin@c { >> + reg = <0xc 0x1>; >> + bits = <2 3>; >> + }; > > Please add the CPR items later together with CPR. This will make the > review a bit easier because we don't need to check that these are right > for the initial submission.
I will excise this if I can, i.e. if the system will still boot once done.
> >> + }; >> [...] >> + mdss: display-subsystem@1a00000 { >> + compatible = "qcom,mdss"; >> + reg = <0x01a00000 0x1000>, >> + <0x01ac8000 0x3000>; >> + reg-names = "mdss_phys", "vbif_phys"; >> + >> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + >> + clocks = <&gcc GCC_MDSS_AHB_CLK>, >> + <&gcc GCC_MDSS_AXI_CLK>, >> + <&gcc GCC_MDSS_VSYNC_CLK>; >> + clock-names = "iface", >> + "bus", >> + "vsync"; >> + >> + power-domains = <&gcc MDSS_GDSC>; >> + >> + #address-cells = <1>; >> + #size-cells = <1>; >> + #interrupt-cells = <1>; >> + ranges; >> + >> + mdp: display-controller@1a01000 { >> + compatible = "qcom,mdp5"; >> + reg = <0x01a01000 0x89000>; >> + reg-names = "mdp_phys"; >> + >> + interrupt-parent = <&mdss>; >> + interrupts = <0>; >> + >> + clocks = <&gcc GCC_MDSS_AHB_CLK>, >> + <&gcc GCC_MDSS_AXI_CLK>, >> + <&gcc GCC_MDSS_MDP_CLK>, >> + <&gcc GCC_MDSS_VSYNC_CLK>, >> + <&gcc GCC_MDP_TBU_CLK>, >> + <&gcc GCC_MDP_RT_TBU_CLK>; >> + clock-names = "iface", >> + "bus", >> + "core", >> + "vsync", >> + "tbu", >> + "tbu_rt"; >> + >> + iommus = <&apps_iommu 4>; >> + >> + interconnects = <&snoc_mm MASTER_MDP_PORT0 &bimc SLAVE_EBI_CH0>, >> + <&snoc_mm MASTER_MDP_PORT1 &bimc SLAVE_EBI_CH0>, >> + <&pcnoc MASTER_SPDM &snoc SLAVE_IMEM>; >> + interconnect-names = "mdp0-mem", "mdp1-mem", "register-mem"; > > As I mentioned a already in a direct email at some point, AFAIU adding > interconnects should be an [almost-] all or nothing step. If you only > add interconnects for MDP then everything else that needs bandwidth will > either break or only continue working as a mere side effect of MDP > voting for permanent high bandwidth.
We did discuss that. You'll also recall we concluded we would have to revert this patch to make that happen.
commit 76a748e2c1aa976d0c7fef872fa6ff93ce334a8a Author: Leo Yan <leo.yan@linaro.org> Date: Sat Apr 16 09:26:34 2022 +0800
interconnect: qcom: msm8939: Use icc_sync_state
but then why not revert for all of these SoCs too ?
drivers/interconnect/qcom/msm8939.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/msm8974.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/msm8996.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/osm-l3.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sc7180.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sc7280.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sc8180x.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sc8280xp.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sdm845.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sdx55.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sdx65.c: .sync_state = icc_sync_state, drivers/interconnect/qcom/sm6350.c: .sync_state = icc_sync_state,
until such time as we have an all or nothing interconnect setup for each of those SoCs ?
Yes I take your point "some peripherals will appear to work only as a result of the AHB vote the MDP casts here" but, that is a bug in the definition of that hypothetical peripheral.
The MDP/display won't run without the interconnect here and the only way to pull it is to remove sync_state which begs the question why not pull sync_state for all SoCs without a perfect interconnect description ?
I think that would be a retrograde step.
> (Semi-related side note: "register-mem" is neither documented nor used > anywhere in the code?)
Oh do you have me there though, this is a holdover from the Android dtsi. I'll see if it makes a difference dropping this.
> >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + mdp5_intf1_out: endpoint { >> + remote-endpoint = <&dsi0_in>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + mdp5_intf2_out: endpoint { >> + remote-endpoint = <&dsi1_in>; >> + }; >> + }; >> + }; >> + }; >> + >> + dsi0: dsi@1a98000 { >> + compatible = "qcom,msm8916-dsi-ctrl", >> + "qcom,mdss-dsi-ctrl"; >> + reg = <0x01a98000 0x25c>; >> + reg-names = "dsi_ctrl"; >> + >> + interrupt-parent = <&mdss>; >> + interrupts = <4>; >> + >> + power-domains = <&gcc MDSS_GDSC>; > > Why is MDSS_GDSC defined again here? The parent-child relationship of > MDSS->MDP should ensure that the MDSS_GDSC from the parent mdss node > is on when dsi is. > >> + >> + clocks = <&gcc GCC_MDSS_MDP_CLK>, >> + <&gcc GCC_MDSS_AHB_CLK>, >> + <&gcc GCC_MDSS_AXI_CLK>, >> + <&gcc GCC_MDSS_BYTE0_CLK>, >> + <&gcc GCC_MDSS_PCLK0_CLK>, >> + <&gcc GCC_MDSS_ESC0_CLK>; >> + clock-names = "mdp_core", >> + "iface", >> + "bus", >> + "byte", >> + "pixel", >> + "core"; >> + assigned-clocks = <&gcc BYTE0_CLK_SRC>, >> + <&gcc PCLK0_CLK_SRC>; >> + assigned-clock-parents = <&dsi_phy0 0>, >> + <&dsi_phy0 1>; >> + >> + phys = <&dsi_phy0>; >> + status = "disabled"; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + dsi0_in: endpoint { >> + remote-endpoint = <&mdp5_intf1_out>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + dsi0_out: endpoint { >> + }; >> + }; >> + }; >> + }; >> + >> + dsi_phy0: phy@1a98300 { >> + compatible = "qcom,dsi-phy-28nm-lp"; >> + reg = <0x01a98300 0xd4>, >> + <0x01a98500 0x280>, >> + <0x01a98780 0x30>; >> + reg-names = "dsi_pll", >> + "dsi_phy", >> + "dsi_phy_regulator"; >> + >> + clocks = <&gcc GCC_MDSS_AHB_CLK>, >> + <&rpmcc RPM_SMD_XO_CLK_SRC>; >> + clock-names = "iface", "ref"; >> + >> + #clock-cells = <1>; >> + #phy-cells = <0>; >> + status = "disabled"; >> + }; >> + >> + dsi1: dsi@1aa0000 { >> + compatible = "qcom,msm8916-dsi-ctrl", >> + "qcom,mdss-dsi-ctrl"; >> + reg = <0x01aa0000 0x25c>; >> + reg-names = "dsi_ctrl"; >> + >> + interrupt-parent = <&mdss>; >> + interrupts = <5>; >> + >> + power-domains = <&gcc MDSS_GDSC>; >> + >> + clocks = <&gcc GCC_MDSS_MDP_CLK>, >> + <&gcc GCC_MDSS_AHB_CLK>, >> + <&gcc GCC_MDSS_AXI_CLK>, >> + <&gcc GCC_MDSS_BYTE1_CLK>, >> + <&gcc GCC_MDSS_PCLK1_CLK>, >> + <&gcc GCC_MDSS_ESC1_CLK>; >> + clock-names = "mdp_core", >> + "iface", >> + "bus", >> + "byte", >> + "pixel", >> + "core"; >> + assigned-clocks = <&gcc BYTE1_CLK_SRC>, >> + <&gcc PCLK1_CLK_SRC>; >> + assigned-clock-parents = <&dsi_phy1 0>, >> + <&dsi_phy1 1>; > > Does this work? Confusingly, BYTE1/PCLK1_CLK_SRC can only have dsi0pll > as parent in gcc-msm8939 and not the dsi1pll. <&dsi_phy1 0/1> is not a > valid parent for those clocks.
No you're right, its all wrong. I will correct it
mdss_dsi0: qcom,mdss_dsi@1a98000 { compatible = "qcom,mdss-dsi-ctrl"; label = "MDSS DSI CTRL->0"; cell-index = <0>; reg = <0x1a98000 0x25c>, <0x1a98500 0x2b0>, <0x193e000 0x30>; reg-names = "dsi_ctrl", "dsi_phy", "mmss_misc_phys"; qcom,mdss-fb-map = <&mdss_fb0>; qcom,mdss-mdp = <&mdss_mdp>; gdsc-supply = <&gdsc_mdss>; vdda-supply = <&pm8916_l2>; vdd-supply = <&pm8916_l17>; vddio-supply = <&pm8916_l6>; clocks = <&clock_gcc clk_gcc_mdss_mdp_clk>, <&clock_gcc clk_gcc_mdss_ahb_clk>, <&clock_gcc clk_gcc_mdss_axi_clk>, <&clock_gcc_mdss clk_gcc_mdss_byte0_clk>, <&clock_gcc_mdss clk_gcc_mdss_pclk0_clk>, <&clock_gcc clk_gcc_mdss_esc0_clk>; --- bod
| |