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

[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@xxxxxxxxxx>
> > 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