Re: [PATCH v3 5/8] arm64: dts: qcom: Add msm8939 SoC

Bryan O'Donoghue
2023-01-19T00:53:25Z

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