Messages in this thread | | | Date | Thu, 19 Jan 2023 00:53:15 +0000 | Subject | Re: [PATCH v3 5/8] arm64: dts: qcom: Add msm8939 SoC | From | Bryan O'Donoghue <> |
| |
On 18/01/2023 17:33, Stephan Gerhold wrote: > On Wed, Jan 18, 2023 at 11:50:20AM +0000, Bryan O'Donoghue wrote: >> On 18/01/2023 09:59, Stephan Gerhold wrote: >>> On Tue, Jan 17, 2023 at 02:48:43AM +0000, Bryan O'Donoghue wrote: >> [...] >>>> + 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. >> > > Most of the SoCs you list do have "interconnects" defined for most > components, which means the situation for them is quite a different > level.
8974 defines two interconnects one for the mdp, one of the gpu. So a headless setup as you describe would encounter the same situation potentially.
> I simulated this on the BQ Aquaris M5 (MSM8939) that has most > functionality set up already in postmarketOS. First the results without > any changes (interconnects enabled like in your patch here):
To me, that is indicative of more work being required to vote appropriately for required bandwidth - AHB clocks basically in our hypothetical setup.
The display certainly won't work without voting for bandwidth it needs. If there's work to be done to _enable_ headless mode - and there is, we can do the work to figure out who isn't voting for bandwidth.
Probably the CPU - absent cpufreq, CPR, the operating points. A good - probably correct guess is we aren't ramping cpufreq, aren't ramping CCI and aren't voting for the inter-chip CCI "front side" so when the system boots headless and "does stuff" the cpufrequency stays low, the votes aren't cast and everything seems to crawl.
I still think its a contrived example though. CPR will come right after the core dtsi and we can put the theory to the test.
;)
--- bod
| |