[PATCH V3 2/2] cpufreq: scmi: Register for limit change notifications

Thu Feb 29 06:15:17 PST 2024

On 2/29/24 12:11, Cristian Marussi wrote:
> On Thu, Feb 29, 2024 at 11:45:41AM +0000, Lukasz Luba wrote:
>>
>>
>> On 2/29/24 11:28, Cristian Marussi wrote:
>>> On Thu, Feb 29, 2024 at 10:22:39AM +0000, Lukasz Luba wrote:
>>>>
>>>>
>>>> On 2/29/24 09:59, Lukasz Luba wrote:
>>>>>
>>>>>
>>>>> On 2/28/24 17:00, Sibi Sankar wrote:
>>>>>>
>>>>>>
>>>>>> On 2/28/24 18:54, Lukasz Luba wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/27/24 18:16, Sibi Sankar wrote:
>>>>>>>> Register for limit change notifications if supported and use
>>>>>>>> the throttled
>>>>>>>> frequency from the notification to apply HW pressure.
>>>>>>
>>>>>> Lukasz,
>>>>>>
>>>>>> Thanks for taking time to review the series!
>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Sibi Sankar <quic_sibis at quicinc.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> v3:
>>>>>>>> * Sanitize range_max received from the notifier. [Pierre]
>>>>>>>> * Update commit message.
>>>>>>>>
>>>>>>>> ï¿½ drivers/cpufreq/scmi-cpufreq.c | 29 ++++++++++++++++++++++++++++-
>>>>>>>> ï¿½ 1 file changed, 28 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/cpufreq/scmi-cpufreq.c
>>>>>>>> b/drivers/cpufreq/scmi-cpufreq.c
>>>>>>>> index 76a0ddbd9d24..78b87b72962d 100644
>>>>>>>> --- a/drivers/cpufreq/scmi-cpufreq.c
>>>>>>>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>>>>>>>> @@ -25,9 +25,13 @@ struct scmi_data {
>>>>>>>> ï¿½ï¿½ï¿½ï¿½ï¿½ int domain_id;
>>>>>>>> ï¿½ï¿½ï¿½ï¿½ï¿½ int nr_opp;
>>>>>>>> ï¿½ï¿½ï¿½ï¿½ï¿½ struct device *cpu_dev;
>>>>>>>> +ï¿½ï¿½ï¿½ struct cpufreq_policy *policy;
>>>>>>>> ï¿½ï¿½ï¿½ï¿½ï¿½ cpumask_var_t opp_shared_cpus;
>>>>>>>> +ï¿½ï¿½ï¿½ struct notifier_block limit_notify_nb;
>>>>>>>> ï¿½ };
>>>>>>>> +const struct scmi_handle *handle;
>>>>>
>>>>> I've missed this bit here.
>>>>
>>>> So for this change we actually have to ask Cristian or Sudeep
>>>> because I'm not sure if we have only one 'handle' instance
>>>> for all cpufreq devices.
>>>>
>>>> If we have different 'handle' we cannot move it to the
>>>> global single pointer.
>>>>
>>>> Sudeep, Cristian what do you think?
>>>
>>> I was just replying noticing this :D .... since SCMI drivers can be
>>> probed multiple times IF you defined multiple scmi top nodes in your DT
>>> containing the same protocol nodes, they receive a distinct sdev/handle/ph
>>> for each probe...so any attempt to globalize these wont work...BUT...
>>>
>>> ...this is a bit of a weird setup BUT it is not against the spec and it can
>>> be used to parallelize more the SCMI accesses to disjont set of resources
>>> within the same protocol (a long story here...) AND this type of setup is
>>> something that it is already used by some other colleagues of Sibi working
>>> on a different line of products (AFAIK)...
>>>
>>> So, for these reasons, usually, all the other SCMI drivers have per-instance
>>> non-global references to handle/sdev/ph....
>>>
>>> ...having said that, thought, looking at the structure of CPUFReq
>>> drivers, I am not sure that they can stand such a similar setup
>>> where multiple instances of this same driver are probed
>>>
>>> .... indeed the existent *ph refs above is already global....so it wont already
>>> work anyway in case of multiple instances now...
>>>
>>> ...and if I look at how CPUFreq expects the signature of scmi_cpufreq_get_rate()
>>> to be annd how it is implemented now using the global *ph reference, it is
>>> clearly already not working cleanly on a multi-instance setup...
>>>
>>> ...now...I can imagine how to (maybe) fix the above removing the globals and
>>> fixing this, BUT the question, more generally, is CPUFreq supposed to work at all in
>>> this multi-probed mode of operation ?
>>> Does it even make sense to be able to support this in CPUFREQ ?
>>>
>>> (as an example in cpufreq,c there is static global cpufreq_driver
>>>    pointing to the arch-specific configured driver BUT that also holds
>>>    some .driver_data AND that cleraly wont be instance specific if you
>>>    probe multiple times and register with CPUFreq multiple times...)
>>>
>>>    More questions than answers here :D
>>>
>>
>> Thanks Cristian for instant response. Yes, indeed now we have more
>> questions :) (which is good). But that's good description of the
>> situation.
>>
>> So lets consider a few option what we could do now:
>> 1. Let Sibi add another global state the 'handle' but add
>>     a BUG_ON() or WARN_ON() in the probe path if the next
>>     'handle' instance is different than already set in global.
>>     This would simply mean that we don't support (yet)
>>     such configuration in a platform. As you said, we
>>     already have the *ph global, so maybe such platforms
>>     with multiple instances for this particular cpufreq and
>>     performance protocol don't exist yet.
> 
> Yes this is the quickst way (and a WARN_ON() is better I'd say) but there
> are similar issues of "unicity" currently already with another vendor SCMI
> drivers and custom protocol currently under review, so I was thinking to
> add a new common mechanism in SCMI to handle this ... not thought about
> this really in depth and I want to chat with Sudeep about this...
> 
>> 2. Ask Sibi to wait with this change, till we refactor the
>>     exiting driver such that it could support easily those
>>     multiple instances. Then pick up this patch set.
>>     Although, we would also like to have those notifications from our
>>     Juno SCP reference FW, so the feature is useful.
>> 3. Ask Sibi to refactor his patch to somehow get the 'handle'
>>     in different way, using exiting code and not introduce this global.
>>
> 
>> IHMO we could do this in steps: 1. and then 2. When
>> we create some mock platform to test this refactoring we can
>> start cleaning it.
>>
> 
> Both of these options really beg an answer to my original previous q
> question...if we somehow enable this multi-probe support in the
> scmi-cpufreq.c driver by avoiding glbals refs, does this work at all in
> the context of CPUFreq ?

I don't know yet.

> 
> ...or it is just that CPUFreq cannot handle such a configuration (and
> maybe dont want to) and so the only solution here is just 1. at first and
> then a common refined mechanism (as mentioned above) to ensure this "unicity"
> of the probes for some drivers ?

This sounds reasonable.

> 
> I'm not familiar enough to grasp if this "multi-probed" mode of operation is
> allowed/supported by CPUFreq and, more important, if it makes any sense
> at all to be a supported mode...
> 

OK, let me check some stuff in the code and think for a while on that.
Thanks Cristian!

Sibi, please give me a few days. In the meantime you can continue
on the 'boost' patch set probably.