[PATCH V2 0/4] firmware: arm_scmi: Register and handle limits change notification

[PATCH V2 0/4] firmware: arm_scmi: Register and handle limits change notification

[Sibi Sankar]

This series registers for scmi limits change notifications and adds
perf_notify_support/perf_opp_xlate interfaces which are used by the
scmi cpufreq driver to determine the throttled frequency and apply HW
pressure.

V2:
* Rename opp_xlate -> freq_xlate [Viresh]
* Export cpufreq_update_pressure and use it directly [Lukasz]

Depends on:
HW pressure v4: https://patchwork.kernel.org/project/linux-arm-msm/cover/20240109164655.626085-1-vincent.guittot@linaro.org/

Sibi Sankar (4):
  firmware: arm_scmi: Add perf_notify_support interface
  firmware: arm_scmi: Add perf_freq_xlate interface
  cpufreq: Export cpufreq_update_pressure
  cpufreq: scmi: Register for limit change notifications

 drivers/cpufreq/cpufreq.c        |  3 ++-
 drivers/cpufreq/scmi-cpufreq.c   | 42 +++++++++++++++++++++++++++++++-
 drivers/firmware/arm_scmi/perf.c | 37 ++++++++++++++++++++++++++++
 include/linux/cpufreq.h          |  2 ++
 include/linux/scmi_protocol.h    | 11 +++++++++
 5 files changed, 93 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Sibi Sankar]

Add a new perf_notify_support interface to the existing perf_ops to export
info regarding limit/level change notification support.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 drivers/firmware/arm_scmi/perf.c | 16 ++++++++++++++++
 include/linux/scmi_protocol.h    |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 211e8e0aef2c..ae7681eda276 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -962,6 +962,21 @@ scmi_power_scale_get(const struct scmi_protocol_handle *ph)
 	return pi->power_scale;
 }
 
+static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain,
+			       struct scmi_perf_notify_info *info)
+{
+	struct perf_dom_info *dom;
+
+	dom = scmi_perf_domain_lookup(ph, domain);
+	if (IS_ERR(dom))
+		return -EINVAL;
+
+	info->perf_limit_notify = dom->perf_limit_notify;
+	info->perf_level_notify = dom->perf_level_notify;
+
+	return 0;
+}
+
 static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.num_domains_get = scmi_perf_num_domains_get,
 	.info_get = scmi_perf_info_get,
@@ -976,6 +991,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.est_power_get = scmi_dvfs_est_power_get,
 	.fast_switch_possible = scmi_fast_switch_possible,
 	.power_scale_get = scmi_power_scale_get,
+	.perf_notify_support = scmi_notify_support,
 };
 
 static int scmi_perf_set_notify_enabled(const struct scmi_protocol_handle *ph,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f2f05fb42d28..b0947d004826 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -117,6 +117,11 @@ struct scmi_perf_domain_info {
 	bool set_perf;
 };
 
+struct scmi_perf_notify_info {
+	bool perf_limit_notify;
+	bool perf_level_notify;
+};
+
 /**
  * struct scmi_perf_proto_ops - represents the various operations provided
  *	by SCMI Performance Protocol
@@ -139,6 +144,7 @@ struct scmi_perf_domain_info {
  *	for a given device
  * @power_scale_mw_get: indicates if the power values provided are in milliWatts
  *	or in some other (abstract) scale
+ * @perf_notify_support: indicates if limit and level change notification is supported
  */
 struct scmi_perf_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
@@ -165,6 +171,8 @@ struct scmi_perf_proto_ops {
 	bool (*fast_switch_possible)(const struct scmi_protocol_handle *ph,
 				     u32 domain);
 	enum scmi_power_scale (*power_scale_get)(const struct scmi_protocol_handle *ph);
+	int (*perf_notify_support)(const struct scmi_protocol_handle *ph, u32 domain,
+				   struct scmi_perf_notify_info *info);
 };
 
 /**
-- 
2.34.1

[PATCH V2 2/4] firmware: arm_scmi: Add perf_freq_xlate interface

[Sibi Sankar]

Add a new perf_freq_xlate interface to the existing perf_ops to translate
a given perf index to frequency.

This can be used by the cpufreq driver and framework to determine the
throttled frequency from a given perf index and apply HW pressure
accordingly.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v2:
* Rename opp_xlate -> freq_xlate [Viresh]

 drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
 include/linux/scmi_protocol.h    |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ae7681eda276..e286f04ee6e3 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -977,6 +977,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
 	return 0;
 }
 
+static int scmi_perf_freq_xlate(const struct scmi_protocol_handle *ph, u32 domain,
+				int idx, unsigned long *freq)
+{
+	struct perf_dom_info *dom;
+
+	dom = scmi_perf_domain_lookup(ph, domain);
+	if (IS_ERR(dom))
+		return PTR_ERR(dom);
+
+	if (idx >= dom->opp_count)
+		return -ERANGE;
+
+	if (!dom->level_indexing_mode)
+		*freq = dom->opp[idx].perf * dom->mult_factor;
+	else
+		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
+
+	return 0;
+}
+
 static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.num_domains_get = scmi_perf_num_domains_get,
 	.info_get = scmi_perf_info_get,
@@ -992,6 +1012,7 @@ static const struct scmi_perf_proto_ops perf_proto_ops = {
 	.fast_switch_possible = scmi_fast_switch_possible,
 	.power_scale_get = scmi_power_scale_get,
 	.perf_notify_support = scmi_notify_support,
+	.perf_freq_xlate = scmi_perf_freq_xlate,
 };
 
 static int scmi_perf_set_notify_enabled(const struct scmi_protocol_handle *ph,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b0947d004826..6221d391386c 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -145,6 +145,7 @@ struct scmi_perf_notify_info {
  * @power_scale_mw_get: indicates if the power values provided are in milliWatts
  *	or in some other (abstract) scale
  * @perf_notify_support: indicates if limit and level change notification is supported
+ * @perf_freq_xlate: translates the given perf index to frequency in Hz
  */
 struct scmi_perf_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
@@ -173,6 +174,8 @@ struct scmi_perf_proto_ops {
 	enum scmi_power_scale (*power_scale_get)(const struct scmi_protocol_handle *ph);
 	int (*perf_notify_support)(const struct scmi_protocol_handle *ph, u32 domain,
 				   struct scmi_perf_notify_info *info);
+	int (*perf_freq_xlate)(const struct scmi_protocol_handle *ph, u32 domain,
+			       int idx, unsigned long *freq);
 };
 
 /**
-- 
2.34.1

[PATCH V2 3/4] cpufreq: Export cpufreq_update_pressure

[Sibi Sankar]

The SCMI cpufreq driver doesn't require any additional signal
smoothing provided by arch_update_hw_pressure interface, export
cpufreq_update_pressure so that it can be called upon directly
instead.

Suggested-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v2:
* Export cpufreq_update_pressure and use it directly [Lukasz]

 drivers/cpufreq/cpufreq.c | 3 ++-
 include/linux/cpufreq.h   | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f4eee3d107f1..c051d1719a06 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2571,7 +2571,7 @@ DEFINE_PER_CPU(unsigned long, cpufreq_pressure);
  *
  * Update the value of cpufreq pressure for all @cpus in the policy.
  */
-static void cpufreq_update_pressure(struct cpufreq_policy *policy)
+void cpufreq_update_pressure(struct cpufreq_policy *policy)
 {
 	unsigned long max_capacity, capped_freq, pressure;
 	u32 max_freq;
@@ -2596,6 +2596,7 @@ static void cpufreq_update_pressure(struct cpufreq_policy *policy)
 	for_each_cpu(cpu, policy->related_cpus)
 		WRITE_ONCE(per_cpu(cpufreq_pressure, cpu), pressure);
 }
+EXPORT_SYMBOL(cpufreq_update_pressure);
 
 /**
  * cpufreq_set_policy - Modify cpufreq policy parameters.
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index b1d97edd3253..c6395b698863 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -241,6 +241,7 @@ struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
 void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
 void cpufreq_disable_fast_switch(struct cpufreq_policy *policy);
 bool has_target_index(void);
+void cpufreq_update_pressure(struct cpufreq_policy *policy);
 
 DECLARE_PER_CPU(unsigned long, cpufreq_pressure);
 static inline unsigned long cpufreq_get_pressure(int cpu)
@@ -269,6 +270,7 @@ static inline bool cpufreq_supports_freq_invariance(void)
 	return false;
 }
 static inline void disable_cpufreq(void) { }
+static inline void cpufreq_update_pressure(struct cpufreq_policy *policy) { }
 static inline unsigned long cpufreq_get_pressure(int cpu)
 {
 	return 0;
-- 
2.34.1

[PATCH V2 4/4] cpufreq: scmi: Register for limit change notifications

[Sibi Sankar]

Register for limit change notifications if supported with the help of
perf_notify_support interface and determine the throttled frequency
using the perf_freq_xlate to apply HW pressure.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v2:
* Export cpufreq_update_pressure and use it directly [Lukasz]

 drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 4ee23f4ebf4a..e0aa85764451 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;
 
@@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
 	return 0;
 }
 
+static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data)
+{
+	unsigned long freq_hz;
+	struct scmi_perf_limits_report *limit_notify = data;
+	struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb);
+	struct cpufreq_policy *policy = priv->policy;
+
+	if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz))
+		return NOTIFY_OK;
+
+	policy->max = freq_hz / HZ_PER_KHZ;
+	cpufreq_update_pressure(policy);
+
+	return NOTIFY_OK;
+}
+
 static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 {
 	int ret, nr_opp, domain;
@@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	struct device *cpu_dev;
 	struct scmi_data *priv;
 	struct cpufreq_frequency_table *freq_table;
+	struct scmi_perf_notify_info info = {};
 
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
@@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	policy->fast_switch_possible =
 		perf_ops->fast_switch_possible(ph, domain);
 
+	ret = perf_ops->perf_notify_support(ph, domain, &info);
+	if (ret)
+		dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret);
+
+	if (info.perf_limit_notify) {
+		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_err(cpu_dev, "Error in registering limit change notifier for domain %d\n",
+				domain);
+			return ret;
+		}
+	}
+
+	priv->policy = policy;
+
 	return 0;
 
 out_free_opp:
@@ -321,8 +361,8 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
 {
 	int ret;
 	struct device *dev = &sdev->dev;
-	const struct scmi_handle *handle;
 
+	scmi_dev = sdev;
 	handle = sdev->handle;
 
 	if (!handle)
-- 
2.34.1

Re: [PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Cristian Marussi]

On Wed, Jan 17, 2024 at 04:11:13PM +0530, Sibi Sankar wrote:
> Add a new perf_notify_support interface to the existing perf_ops to export
> info regarding limit/level change notification support.
> 

Hi Sibi,

as I mentioned previously, in order not to add a needless stream of SCMI
Perf accessors I posted this:

https://lore.kernel.org/linux-arm-kernel/20240129151002.1215333-1-cristian.marussi@arm.com/T/#u

to expose all the Perf domains infos via the usual info_get(), similarly
to how other SCMI protocols do already.

I think that reworking this series on that, you can certainly drop this patch and just
check the _notify booleans on the retrieved domain info.

Thanks,
Cristian

Re: [PATCH V2 2/4] firmware: arm_scmi: Add perf_freq_xlate interface

[Cristian Marussi]

On Wed, Jan 17, 2024 at 04:11:14PM +0530, Sibi Sankar wrote:
> Add a new perf_freq_xlate interface to the existing perf_ops to translate
> a given perf index to frequency.
> 
> This can be used by the cpufreq driver and framework to determine the
> throttled frequency from a given perf index and apply HW pressure
> accordingly.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
> * Rename opp_xlate -> freq_xlate [Viresh]
> 
>  drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
>  include/linux/scmi_protocol.h    |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index ae7681eda276..e286f04ee6e3 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -977,6 +977,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
>  	return 0;
>  }
>  
> +static int scmi_perf_freq_xlate(const struct scmi_protocol_handle *ph, u32 domain,
> +				int idx, unsigned long *freq)
> +{
> +	struct perf_dom_info *dom;
> +
> +	dom = scmi_perf_domain_lookup(ph, domain);
> +	if (IS_ERR(dom))
> +		return PTR_ERR(dom);
> +
> +	if (idx >= dom->opp_count)
> +		return -ERANGE;
> +
> +	if (!dom->level_indexing_mode)
> +		*freq = dom->opp[idx].perf * dom->mult_factor;
> +	else
> +		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
> +
> +	return 0;
> +}

Potentially, once the dom info are exposed you can also drop this
accessor and move all of these checks/calculations in the the SCMI cpufreq
driver...but maybe Sudeep/Viresh prefer to keep this accessor exposed
like others.

Thanks,
Cristian

Re: [PATCH V2 4/4] cpufreq: scmi: Register for limit change notifications

[Cristian Marussi]

On Wed, Jan 17, 2024 at 04:11:16PM +0530, Sibi Sankar wrote:
> Register for limit change notifications if supported with the help of
> perf_notify_support interface and determine the throttled frequency
> using the perf_freq_xlate to apply HW pressure.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
> * Export cpufreq_update_pressure and use it directly [Lukasz]
> 
>  drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 4ee23f4ebf4a..e0aa85764451 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;
>  
> @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
>  	return 0;
>  }
>  
> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data)
> +{
> +	unsigned long freq_hz;
> +	struct scmi_perf_limits_report *limit_notify = data;
> +	struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb);
> +	struct cpufreq_policy *policy = priv->policy;
> +
> +	if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz))
> +		return NOTIFY_OK;
> +
> +	policy->max = freq_hz / HZ_PER_KHZ;
> +	cpufreq_update_pressure(policy);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	int ret, nr_opp, domain;
> @@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  	struct device *cpu_dev;
>  	struct scmi_data *priv;
>  	struct cpufreq_frequency_table *freq_table;
> +	struct scmi_perf_notify_info info = {};
>  
>  	cpu_dev = get_cpu_device(policy->cpu);
>  	if (!cpu_dev) {
> @@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->fast_switch_possible =
>  		perf_ops->fast_switch_possible(ph, domain);
>  
> +	ret = perf_ops->perf_notify_support(ph, domain, &info);
> +	if (ret)
> +		dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret);
> +
> +	if (info.perf_limit_notify) {
> +		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_err(cpu_dev, "Error in registering limit change notifier for domain %d\n",
> +				domain);
> +			return ret;
> +		}

Is there a reason to fail completely here if it was not possible to register
the notifier ? (even though expected to succeed given perf_limit_notify
was true...)

Maybe a big fat warn that the system perf could be degraded, but
carrying on ?

Or maybe you have in mind a good reason to fail like you did, so please
explain in that case in a comment.

Thanks,
Cristian

Re: [PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Cristian Marussi]

On Mon, Jan 29, 2024 at 03:50:20PM +0000, Cristian Marussi wrote:
> On Wed, Jan 17, 2024 at 04:11:13PM +0530, Sibi Sankar wrote:
> > Add a new perf_notify_support interface to the existing perf_ops to export
> > info regarding limit/level change notification support.
> > 
> 
> Hi Sibi,
> 
> as I mentioned previously, in order not to add a needless stream of SCMI
> Perf accessors I posted this:
> 
> https://lore.kernel.org/linux-arm-kernel/20240129151002.1215333-1-cristian.marussi@arm.com/T/#u
> 
> to expose all the Perf domains infos via the usual info_get(), similarly
> to how other SCMI protocols do already.
> 
> I think that reworking this series on that, you can certainly drop this patch and just
> check the _notify booleans on the retrieved domain info.

Sorry, but hold on with this change, I will probably post an updated version
my patch above.

Thanks,
Cristian

Re: [PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Sudeep Holla]

On Mon, Jan 29, 2024 at 05:33:42PM +0000, Cristian Marussi wrote:
> On Mon, Jan 29, 2024 at 03:50:20PM +0000, Cristian Marussi wrote:
> > On Wed, Jan 17, 2024 at 04:11:13PM +0530, Sibi Sankar wrote:
> > > Add a new perf_notify_support interface to the existing perf_ops to export
> > > info regarding limit/level change notification support.
> > > 
> > 
> > Hi Sibi,
> > 
> > as I mentioned previously, in order not to add a needless stream of SCMI
> > Perf accessors I posted this:
> > 
> > https://lore.kernel.org/linux-arm-kernel/20240129151002.1215333-1-cristian.marussi@arm.com/T/#u
> > 
> > to expose all the Perf domains infos via the usual info_get(), similarly
> > to how other SCMI protocols do already.
> > 
> > I think that reworking this series on that, you can certainly drop this patch and just
> > check the _notify booleans on the retrieved domain info.
> 
> Sorry, but hold on with this change, I will probably post an updated version
> my patch above.
> 

As discussed in private, I would prefer to avoid exposing all the internals
to the users of SCMI perf. At the same time may we can do better if we can
check the availability of notification as part of notification enablement
from the SCMI driver, I need to think the details yet.

-- 
Regards,
Sudeep

Re: [PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Cristian Marussi]

On Wed, Jan 31, 2024 at 11:28:54AM +0000, Sudeep Holla wrote:
> On Mon, Jan 29, 2024 at 05:33:42PM +0000, Cristian Marussi wrote:
> > On Mon, Jan 29, 2024 at 03:50:20PM +0000, Cristian Marussi wrote:
> > > On Wed, Jan 17, 2024 at 04:11:13PM +0530, Sibi Sankar wrote:
> > > > Add a new perf_notify_support interface to the existing perf_ops to export
> > > > info regarding limit/level change notification support.
> > > > 
> > > 
> > > Hi Sibi,
> > > 
> > > as I mentioned previously, in order not to add a needless stream of SCMI
> > > Perf accessors I posted this:
> > > 
> > > https://lore.kernel.org/linux-arm-kernel/20240129151002.1215333-1-cristian.marussi@arm.com/T/#u
> > > 
> > > to expose all the Perf domains infos via the usual info_get(), similarly
> > > to how other SCMI protocols do already.
> > > 
> > > I think that reworking this series on that, you can certainly drop this patch and just
> > > check the _notify booleans on the retrieved domain info.
> > 
> > Sorry, but hold on with this change, I will probably post an updated version
> > my patch above.
> > 
> 
> As discussed in private, I would prefer to avoid exposing all the internals
> to the users of SCMI perf. At the same time may we can do better if we can
> check the availability of notification as part of notification enablement
> from the SCMI driver, I need to think the details yet.

Yes a patch is under-work to avoid exposing too much Perf info AND to
avoid adding ad-hoc accessors like xlate, in this case.

Thanks,
Cristian

Re: [PATCH V2 2/4] firmware: arm_scmi: Add perf_freq_xlate interface

[Cristian Marussi]

On Wed, Jan 17, 2024 at 04:11:14PM +0530, Sibi Sankar wrote:
> Add a new perf_freq_xlate interface to the existing perf_ops to translate
> a given perf index to frequency.
> 
> This can be used by the cpufreq driver and framework to determine the
> throttled frequency from a given perf index and apply HW pressure
> accordingly.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
> * Rename opp_xlate -> freq_xlate [Viresh]
> 
>  drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
>  include/linux/scmi_protocol.h    |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index ae7681eda276..e286f04ee6e3 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -977,6 +977,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
>  	return 0;
>  }
>  
> +static int scmi_perf_freq_xlate(const struct scmi_protocol_handle *ph, u32 domain,
> +				int idx, unsigned long *freq)
> +{
> +	struct perf_dom_info *dom;
> +
> +	dom = scmi_perf_domain_lookup(ph, domain);
> +	if (IS_ERR(dom))
> +		return PTR_ERR(dom);
> +
> +	if (idx >= dom->opp_count)
> +		return -ERANGE;
> +
> +	if (!dom->level_indexing_mode)
> +		*freq = dom->opp[idx].perf * dom->mult_factor;
> +	else
> +		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
> +

As said elsewhere the plan would be to change slightly the SCMI core to
avoid the need for this patch and the previous one (while NOT exposing
too much Perf info)...

... anyway just looking at the above freq calc logic in this patch, be
aware that as it stands it seems to me broken, since the idx you use to
peek into the opp array comes (in the next patch) from the range_max
carried by the notification and that can be, indeed, a perf_level OR a
perf_index BUT it is absolutely NOT guaranteed to be an index into the
opp[] array...so it may work in your case if you have a platform
defining level or indexes matching the opp[] indexes BUT it is not true
in general. (but as said, this will be handled by the core and possibly
this patch dropped...)

Thanks,
Cristian

Re: [PATCH V2 4/4] cpufreq: scmi: Register for limit change notifications

[Pierre Gondois]

Hello Sibi,

On 1/17/24 11:41, Sibi Sankar wrote:
> Register for limit change notifications if supported with the help of
> perf_notify_support interface and determine the throttled frequency
> using the perf_freq_xlate to apply HW pressure.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
> * Export cpufreq_update_pressure and use it directly [Lukasz]
> 
>   drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++-
>   1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 4ee23f4ebf4a..e0aa85764451 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;
>   
> @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
>   	return 0;
>   }
>   
> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data)
> +{
> +	unsigned long freq_hz;
> +	struct scmi_perf_limits_report *limit_notify = data;
> +	struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb);
> +	struct cpufreq_policy *policy = priv->policy;
> +
> +	if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz))
> +		return NOTIFY_OK;
> +
> +	policy->max = freq_hz / HZ_PER_KHZ;

Maybe 'policy->max' should be checked. The limits received by SCMI is blindly
trusted. This might be ok, but could also lead to some inconsistency.

The scmi_cpufreq_driver's verify() callback could be used.

---

I think there might also be corner cases where the SCP might advertise
the maximum boosted frequency as the max limit, but boosting might not
be enabled on the kernel side.
So I think this should be checked when setting 'policy->max',

Regards,
Pierre

> +	cpufreq_update_pressure(policy);
> +
> +	return NOTIFY_OK;
> +}
> +
>   static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>   {
>   	int ret, nr_opp, domain;
> @@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>   	struct device *cpu_dev;
>   	struct scmi_data *priv;
>   	struct cpufreq_frequency_table *freq_table;
> +	struct scmi_perf_notify_info info = {};
>   
>   	cpu_dev = get_cpu_device(policy->cpu);
>   	if (!cpu_dev) {
> @@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>   	policy->fast_switch_possible =
>   		perf_ops->fast_switch_possible(ph, domain);
>   
> +	ret = perf_ops->perf_notify_support(ph, domain, &info);
> +	if (ret)
> +		dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret);
> +
> +	if (info.perf_limit_notify) {
> +		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_err(cpu_dev, "Error in registering limit change notifier for domain %d\n",
> +				domain);
> +			return ret;
> +		}
> +	}
> +
> +	priv->policy = policy;
> +
>   	return 0;
>   
>   out_free_opp:
> @@ -321,8 +361,8 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
>   {
>   	int ret;
>   	struct device *dev = &sdev->dev;
> -	const struct scmi_handle *handle;
>   
> +	scmi_dev = sdev;
>   	handle = sdev->handle;
>   
>   	if (!handle)

Re: [PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Cristian Marussi]

On Wed, Jan 31, 2024 at 11:28:54AM +0000, Sudeep Holla wrote:
> On Mon, Jan 29, 2024 at 05:33:42PM +0000, Cristian Marussi wrote:

Hi Sibi,

> > On Mon, Jan 29, 2024 at 03:50:20PM +0000, Cristian Marussi wrote:
> > > On Wed, Jan 17, 2024 at 04:11:13PM +0530, Sibi Sankar wrote:
> > > > Add a new perf_notify_support interface to the existing perf_ops to export
> > > > info regarding limit/level change notification support.
> > > > 
> > > 
> > > Hi Sibi,
> > > 
> > > as I mentioned previously, in order not to add a needless stream of SCMI
> > > Perf accessors I posted this:
> > > 
> > > https://lore.kernel.org/linux-arm-kernel/20240129151002.1215333-1-cristian.marussi@arm.com/T/#u
> > > 
> > > to expose all the Perf domains infos via the usual info_get(), similarly
> > > to how other SCMI protocols do already.
> > > 
> > > I think that reworking this series on that, you can certainly drop this patch and just
> > > check the _notify booleans on the retrieved domain info.
> > 
> > Sorry, but hold on with this change, I will probably post an updated version
> > my patch above.
> > 
> 
> As discussed in private, I would prefer to avoid exposing all the internals
> to the users of SCMI perf. At the same time may we can do better if we can
> check the availability of notification as part of notification enablement
> from the SCMI driver, I need to think the details yet.
> 

as previously mentioned, after speaking with Sudeep, I posted a new
series at [1], that aims to solve your problem with registering
notifications and looking up reported Perf frequencies in a new way.

Using the changes at [1] you should be able to:

- register your notifier without caring to check if the notification
  is supported, since the SCMI core will take care of checking that and
  report an error if not supported, without sending any unneeded
  attempted notification enable message  (so you can drop 1/4 in this
  series)

- retrieve the pre-calculated OPPs frequencies from the new extended
  Perf notifications reports no matter if the system if operating in
  level_indexing_mode or not. (so you can drop 2/4 in this series)

Thanks,
Cristian

[1]: https://lore.kernel.org/linux-arm-kernel/20240212123233.1230090-1-cristian.marussi@arm.com/T/#ma68cefd753e34ba3e1f2d4392e978026a87e1bf8

Re: [PATCH V2 1/4] firmware: arm_scmi: Add perf_notify_support interface

[Sibi Sankar]

On 2/12/24 18:14, Cristian Marussi wrote:
> On Wed, Jan 31, 2024 at 11:28:54AM +0000, Sudeep Holla wrote:
>> On Mon, Jan 29, 2024 at 05:33:42PM +0000, Cristian Marussi wrote:
> 
> Hi Sibi,
> 
>>> On Mon, Jan 29, 2024 at 03:50:20PM +0000, Cristian Marussi wrote:
>>>> On Wed, Jan 17, 2024 at 04:11:13PM +0530, Sibi Sankar wrote:
>>>>> Add a new perf_notify_support interface to the existing perf_ops to export
>>>>> info regarding limit/level change notification support.
>>>>>
>>>>
>>>> Hi Sibi,
>>>>
>>>> as I mentioned previously, in order not to add a needless stream of SCMI
>>>> Perf accessors I posted this:
>>>>
>>>> https://lore.kernel.org/linux-arm-kernel/20240129151002.1215333-1-cristian.marussi@arm.com/T/#u
>>>>
>>>> to expose all the Perf domains infos via the usual info_get(), similarly
>>>> to how other SCMI protocols do already.
>>>>
>>>> I think that reworking this series on that, you can certainly drop this patch and just
>>>> check the _notify booleans on the retrieved domain info.
>>>
>>> Sorry, but hold on with this change, I will probably post an updated version
>>> my patch above.
>>>
>>
>> As discussed in private, I would prefer to avoid exposing all the internals
>> to the users of SCMI perf. At the same time may we can do better if we can
>> check the availability of notification as part of notification enablement
>> from the SCMI driver, I need to think the details yet.
>>
> 
> as previously mentioned, after speaking with Sudeep, I posted a new
> series at [1], that aims to solve your problem with registering
> notifications and looking up reported Perf frequencies in a new way.
> 
> Using the changes at [1] you should be able to:
> 
> - register your notifier without caring to check if the notification
>    is supported, since the SCMI core will take care of checking that and
>    report an error if not supported, without sending any unneeded
>    attempted notification enable message  (so you can drop 1/4 in this
>    series)
> 
> - retrieve the pre-calculated OPPs frequencies from the new extended
>    Perf notifications reports no matter if the system if operating in
>    level_indexing_mode or not. (so you can drop 2/4 in this series)

Christian/Sudeep,

Thanks a lot for spending time on this and simplifying the series.

Will re-spin the series with your recommendations.

-Sibi

> 
> Thanks,
> Cristian
> 
> [1]: https://lore.kernel.org/linux-arm-kernel/20240212123233.1230090-1-cristian.marussi@arm.com/T/#ma68cefd753e34ba3e1f2d4392e978026a87e1bf8
> 

Re: [PATCH V2 4/4] cpufreq: scmi: Register for limit change notifications

[Sibi Sankar]

On 1/29/24 21:29, Cristian Marussi wrote:
> On Wed, Jan 17, 2024 at 04:11:16PM +0530, Sibi Sankar wrote:
>> Register for limit change notifications if supported with the help of
>> perf_notify_support interface and determine the throttled frequency
>> using the perf_freq_xlate to apply HW pressure.
>>

Christian,

Thanks for taking time to review the series.

>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v2:
>> * Export cpufreq_update_pressure and use it directly [Lukasz]
>>
>>   drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index 4ee23f4ebf4a..e0aa85764451 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;
>>   
>> @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
>>   	return 0;
>>   }
>>   
>> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event, void *data)
>> +{
>> +	unsigned long freq_hz;
>> +	struct scmi_perf_limits_report *limit_notify = data;
>> +	struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb);
>> +	struct cpufreq_policy *policy = priv->policy;
>> +
>> +	if (perf_ops->perf_freq_xlate(ph, priv->domain_id, limit_notify->range_max, &freq_hz))
>> +		return NOTIFY_OK;
>> +
>> +	policy->max = freq_hz / HZ_PER_KHZ;
>> +	cpufreq_update_pressure(policy);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>>   static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>   {
>>   	int ret, nr_opp, domain;
>> @@ -151,6 +171,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>   	struct device *cpu_dev;
>>   	struct scmi_data *priv;
>>   	struct cpufreq_frequency_table *freq_table;
>> +	struct scmi_perf_notify_info info = {};
>>   
>>   	cpu_dev = get_cpu_device(policy->cpu);
>>   	if (!cpu_dev) {
>> @@ -250,6 +271,25 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>   	policy->fast_switch_possible =
>>   		perf_ops->fast_switch_possible(ph, domain);
>>   
>> +	ret = perf_ops->perf_notify_support(ph, domain, &info);
>> +	if (ret)
>> +		dev_warn(cpu_dev, "failed to get supported notifications: %d\n", ret);
>> +
>> +	if (info.perf_limit_notify) {
>> +		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_err(cpu_dev, "Error in registering limit change notifier for domain %d\n",
>> +				domain);
>> +			return ret;
>> +		}
> 
> Is there a reason to fail completely here if it was not possible to register
> the notifier ? (even though expected to succeed given perf_limit_notify
> was true...)
> 
> Maybe a big fat warn that the system perf could be degraded, but
> carrying on ?
> 
> Or maybe you have in mind a good reason to fail like you did, so please
> explain in that case in a comment.

ack a warn should suffice here

-Sibi

> 
> Thanks,
> Cristian

Re: [PATCH V2 4/4] cpufreq: scmi: Register for limit change notifications

[Sibi Sankar]

On 1/31/24 19:59, Pierre Gondois wrote:
> Hello Sibi,
> 
> On 1/17/24 11:41, Sibi Sankar wrote:
>> Register for limit change notifications if supported with the help of
>> perf_notify_support interface and determine the throttled frequency
>> using the perf_freq_xlate to apply HW pressure.
>>

Hey Pierre,

Thanks for taking time to review the series.

>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v2:
>> * Export cpufreq_update_pressure and use it directly [Lukasz]
>>
>>   drivers/cpufreq/scmi-cpufreq.c | 42 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c 
>> b/drivers/cpufreq/scmi-cpufreq.c
>> index 4ee23f4ebf4a..e0aa85764451 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;
>> @@ -144,6 +148,22 @@ scmi_get_cpu_power(struct device *cpu_dev, 
>> unsigned long *power,
>>       return 0;
>>   }
>> +static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned 
>> long event, void *data)
>> +{
>> +    unsigned long freq_hz;
>> +    struct scmi_perf_limits_report *limit_notify = data;
>> +    struct scmi_data *priv = container_of(nb, struct scmi_data, 
>> limit_notify_nb);
>> +    struct cpufreq_policy *policy = priv->policy;
>> +
>> +    if (perf_ops->perf_freq_xlate(ph, priv->domain_id, 
>> limit_notify->range_max, &freq_hz))
>> +        return NOTIFY_OK;
>> +
>> +    policy->max = freq_hz / HZ_PER_KHZ;
> 
> Maybe 'policy->max' should be checked. The limits received by SCMI is 
> blindly
> trusted. This might be ok, but could also lead to some inconsistency.
> 
> The scmi_cpufreq_driver's verify() callback could be used.

ack, will fix this in the next re-spin.

> 
> ---
> 
> I think there might also be corner cases where the SCP might advertise
> the maximum boosted frequency as the max limit, but boosting might not
> be enabled on the kernel side.
> So I think this should be checked when setting 'policy->max',

ack

-Sibi
> 
> Regards,
> Pierre
> 
[...]
>>       if (!handle)