Re: [RFC 4/7] soc: qcom: Utilize qcom scmi vendor protocol for bus dvfs

[Konrad Dybcio <konrad.dybcio@linaro.org>]

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