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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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