On 2/28/24 18:54, Lukasz Luba wrote:

On 2/27/24 18:16, Sibi Sankar wrote:

quoted

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!

quoted

Signed-off-by: Sibi Sankar <redacted>
---

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

quoted

+
+    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...

quoted

+    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

quoted

      if (!handle)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel