Messages in this thread | | | Date | Wed, 28 Feb 2024 22:30:50 +0530 | Subject | Re: [PATCH V3 2/2] cpufreq: scmi: Register for limit change notifications | From | Sibi Sankar <> |
| |
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; >> +static struct scmi_device *scmi_dev; >> static struct scmi_protocol_handle *ph; >> static const struct scmi_perf_proto_ops *perf_ops; >> static struct cpufreq_driver scmi_cpufreq_driver; >> @@ -151,6 +155,20 @@ static struct freq_attr *scmi_cpufreq_hw_attr[] = { >> NULL, >> }; >> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned >> long event, void *data) >> +{ >> + struct scmi_data *priv = container_of(nb, struct scmi_data, >> limit_notify_nb); >> + struct scmi_perf_limits_report *limit_notify = data; >> + struct cpufreq_policy *policy = priv->policy; >> + >> + policy->max = clamp(limit_notify->range_max_freq/HZ_PER_KHZ, >> policy->cpuinfo.min_freq, >> + policy->cpuinfo.max_freq); > > Please take the division operation out of this clamp() call, somewhere > above. Currently it 'blurs' these stuff, while it's important convertion > to khz. You can call it e.g.: > > limit_freq_khz = limit_notify->range_max_freq / HZ_PER_KHZ; > > then use in clamp(limit_freq_khz, ...)
ack
> >> + >> + cpufreq_update_pressure(policy); >> + >> + return NOTIFY_OK; >> +} >> + >> static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> { >> int ret, nr_opp, domain; >> @@ -269,6 +287,15 @@ static int scmi_cpufreq_init(struct >> cpufreq_policy *policy) >> } >> } >> + priv->limit_notify_nb.notifier_call = scmi_limit_notify_cb; >> + ret = handle->notify_ops->devm_event_notifier_register(scmi_dev, >> SCMI_PROTOCOL_PERF, >> + SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED, >> + &domain, >> + &priv->limit_notify_nb); >> + if (ret) >> + dev_warn(cpu_dev, >> + "failed to register for limits change notifier for >> domain %d\n", domain); >> + >> priv->policy = policy; >> return 0; >> @@ -342,8 +369,8 @@ static int scmi_cpufreq_probe(struct scmi_device >> *sdev) >> { >> int ret; >> struct device *dev = &sdev->dev; >> - const struct scmi_handle *handle; > > It should be a compilation error... > >> + scmi_dev = sdev; >> handle = sdev->handle; > > due to usage here, wasn't it?
Not really, isn't it getting the first initialization here? Are there any compiler options that I need to turn on to catch these?
-Sibi
> >> if (!handle)
| |