On 10.03.2023 17:47, Bryan O'Donoghue wrote: > On 10/03/2023 14:26, Konrad Dybcio wrote: >> >> >> On 10.03.2023 15:21, Bryan O'Donoghue wrote: >>> On 08/03/2023 21:40, Konrad Dybcio wrote: >>>> Some (but not all) providers (or their specific nodes) require >>>> specific clocks to be turned on before they can be accessed. Failure >>>> to ensure that results in a seemingly random system crash (which >>>> would usually happen at boot with the interconnect driver built-in), >>>> resulting in the platform not booting up properly. >>> >>> Can you give an example of which clocks on which SoC's ? >> See for example 67fb53745e0b >> >> This was a clock documented downstream under the node-qos clocks here: >> >> https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.5.7.r1/arch/arm/boot/dts/qcom/msm8996-bus.dtsi#L102-L109 >> >> but there are occasions where such clocks are undocumented and downstream >> skips them because it relies on them being on by miracle, such as the case >> of MASTER_IPA and the IPA rpmcc clock on msm8998. Downstream has no >> sync_state, so they would only set the QoS registers when the relevant >> hardware was online, so the clocks were on already. > > What switched the clocks on ? Presumably LK. > > Is this a symptom of using a bootloader other than LK ? If you use the same bootloader, then why hasn't the bootloader/LK already set it up on your platform ? XBL* in this case (qcom fully switched to UEFI with 8998, I'm not using anything other to what came on the device) Setting up these dependencies happens all throughout the boot chain: after the SoC starts up, BIMC/PNoC/CNoC/SNoC(/AnNoC) are set up by some early firmware so that DDR, USB, UFS/eMMC, crypto engines etc. can be reachable. It's *that* level of deep.. Then, they are not shut down at all, leaving USB etc. accessible by default. > >>> >>> Is the intention of this patch to subsequently go through *.dts *.dtsi and start to remove assigned-clocks ? >>> >>> Are we saying that currently there ought to be assigned-clocks for some of these NoC declarations ? >> Not really, assigned-clocks are used for static ratesetting, see >> for example dwc3 nodes where we need it to be fast enough for >> HS/SS operation at all times (though that should have prooobably >> been handled in the driver but it's a separate topic), I don't >> think any of them were used to combat what this commit tries to. > > I think you could use assigned-clocks for that .. > > So its not part of your series but then presumably you have a follow-on patch for the 8998 dts that points your ->intf_clks at these then ? > > clocks = <&clock_gcc clk_aggre2_noc_clk>, > <&clock_gcc clk_gcc_ufs_axi_clk>, > <&clock_gcc clk_gcc_aggre2_ufs_axi_clk>; > > It seems like the right thing to do.. Why so? We're passing all clock references to clocks=<> and we handle them separately. This is akin to treating the "core" clock differently to the "iface" clock on some IP block, except on a wider scale. > > Still not clear why these clocks are off.. your bootchain ? Generally the issue is that icc sync_states goes over *all the possible interconnect paths on the entire SoC*. For QoS register access, you need to be able to interface them. Some of the hardware blocks live on a separate sort of "island". That' part of why clocks such as GCC_CFG_NOC_USB3_PRIM_AXI_CLK exist. They're responsible for clocking the CNoC<->USB connection, which in the bigger picture looks more or less like: 1 2-3 2-3 3-4 3-4 4-5 USB<->CNoC<->CNoC_to_SNoC<->SNoC<->SNoC_to_BIMC<->BIMC<->APSS where: 1 = GCC_CFG_NOC_USB3_PRIM_AXI_CLK 2 = RPM CNOC CLK 3 = RPM SNOC CLK 4 = RPM BIMC CLK 5 = (usually internally managed) HMSS / GNOC CLK or something along these lines, the *NoC names may be in the wrong order but this is the general picture. On msm-4.x there is no such thing as sync_state. The votes are only cast from within the IP-block drivers themselves, using data gathered from qcom,msmbus-blahblah and msmbus calls from within the driver. That way, downstream ensures there's never unclocked QoS register access. After writing this entire email, I got an idea that we could consider not accessing these QoS registers from within sync_state (e.g. use sth like if(is_sync_state_done)).. That would lead to this patch being mostly irrelevant (IF AND ONLY IF all the necessary clocks were handled by the end device drivers AND clk/icc voting was done in correct order - which as we can tell from the sad 8996 example, is not always the case). Not guaranteeing it (like this patch does) would make it worse from the standpoint of representing the hardware needs properly, but it could surely save some headaches. To an extent. Konrad > > --- > bod
