On 1.07.2024 10:44 AM, Sibi Sankar wrote: > > > On 6/19/24 01:07, Konrad Dybcio wrote: >> >> >> On 2/12/24 11:33, Sibi Sankar wrote: >> >> [...] >> >> >>>> >>>>> + monitor->mon_type = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 1 : 0; BTW: the ternary operator here is unnecessary, but to make it readable, please make an enum / #define describing the two, as magic values are discouraged >>>>> + monitor->ipm_ceil = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 0 : 20000000; Given that you check the same condition here, an if-else block may be more readable, perhaps some comment like: ipm_ceil = 0; /* Always keep a vote, no matter the bus traffic */ >>>> >>>> What does it even mean for a monitor to be a compute mon? >>>> >>> >>> When a monitor is marked compute-mon it means that the table is >>> followed religiously irrespective whether the instruction per miss >>> count threshold (ipm) is exceeded or not. Equivalent to having >>> a cpufreq map -> l3/DDR bw mapping upstream. I.. don't really like that this exists as something that requires OS intervention, but since it does, I suppose it takes a couple lines of code less than adding OPP entries for each and every PSTATE and NUM_SKUs.. >> >> I'm sorta puzzled why the OS would even be required to program this, since >> L3/DDR/CPU frequencies are known by various stages of boot and secure firmware >> too. >> >> What happens if we omit this? Is the default configuration identical to this? >> Or does it need explicit enabling? > > CPUCP isn't expected to know the various ranges supported by the memory > buses it can vote on and from a sandboxing perspective one would want to > control what CPUCP has access to as well. It also can't arrive at the > exact values just from the OPP tables we pass on as well. So it doesn't > have any default values to start off with. For all these reasons, they > need explicit setting up and without it, the algorithm wouldn't function > as expected. Ok, I was thinking more of a scenario where XBL/GH would take care of this.. Throwing in my 5 cents, this could perhaps be moved there in future FW designs (the earlier in the chain the better, especially to keep kicking out gunyah a viable option), as I don't think Linux is the greatest place for storing one-shot configuration data, especially for blocks that already run their own firmware.. I would imagine this could speed up booting as well, if DRAM was appropriately scaled during the boot splash stage (unless it already is either scaled or pinned to FMAX) Konrad
