Messages in this thread | | | Date | Thu, 29 Feb 2024 14:15:17 +0000 | Subject | Re: [PATCH V3 2/2] cpufreq: scmi: Register for limit change notifications | From | Lukasz Luba <> |
| |
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@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.
| |