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

[Stephan Gerhold]

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. It's probably not necessary to have the interconnect setup
absolutely perfect before enabling it. However to avoid frustration for
people with slightly different board setups it should at the very least
cover more than one component.

Should the icc_sync_state() change be reverted for some of these SoCs?
If you ask me: Yes!

Perhaps a real example makes my concern more understandable: As I
mentioned, you rely on MDP providing the necessary bandwidth for the
entire system. This works fine in your case, but it can happen easily
that MDSS/MDP is not enabled at all, e.g.:

 - On a board without display.
 - During early bring-up: I usually start with UART, USB and SDHCI
   before I even think about enabling the display.

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):

 -> Boots into rootfs in about *18 seconds*, feels fine

Now I just disable MDSS in the device tree and boot again:

	&mdss {
		status = "disabled";
	};

 -> Boots into rootfs in about *80 seconds*, everything feels sluggish

This is 4 times the normal boot time, and nothing in dmesg tells me that
it's because I don't have display enabled. Someone porting a new device,
especially without UART, might have given up already before waiting so
long. Plus, what would I do to fix this on a board without display? :/

Now I try removing icc_sync_state:

 -> Boots into rootfs in about 17 seconds, feels fine

IMO it is clear that adding icc_sync_state() too early is a bad idea,
and *will* break some setups.

Thanks,
Stephan