[PATCH 0/3] Add ARM SCMI Powercap driver

[PATCH 0/3] Add ARM SCMI Powercap driver

[Cristian Marussi]

Hi all,

this short series builts on top of recently introduced SCMI v3.1 Powercap
protocol to implement an ARM SCMI Powercap driver which takes care to
expose via the Powercap framework all the SCMI Powercap zones that have
been discovered asking the SCMI platform firmware.

Basic testing has been performed against an emulated SCMI platform
supporting SCMIv3.1 Powercap protocol using powercap-utils.

A couple of fixes spotted with Smatch by Dan are queued on top of the
driver itself; these may have to be squashed finally into the driver itself
(with the Fixes tags removed) but for now I have just added them here as
patches on top, for clarity and to preserve authorship.

The series is based on v6.0-rc1:

Thanks,
Cristian

Cristian Marussi (1):
  powercap: arm_scmi: Add SCMI Powercap based driver

Dan Carpenter (2):
  powercap: arm_scmi: Fix signedness bug in probe
  powercap: arm_scmi: Fix a NULL vs IS_ERR() bug

 drivers/powercap/Kconfig             |  13 +
 drivers/powercap/Makefile            |   1 +
 drivers/powercap/arm_scmi_powercap.c | 545 +++++++++++++++++++++++++++
 3 files changed, 559 insertions(+)
 create mode 100644 drivers/powercap/arm_scmi_powercap.c

-- 
2.32.0

[PATCH 1/3] powercap: arm_scmi: Add SCMI Powercap based driver

[Cristian Marussi]

Add a powercap driver that, using the ARM SCMI Protocol to query the SCMI
platform firmware for the list of existing Powercap domains, registers all
of such discovered domains under the new 'arm-scmi' powercap control type.

A new simple powercap zone and constraint is registered for all the SCMI
powercap zones that are found.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v4 --> v5
- fix _init cleaning up powerctrl zone on failure
- removed uneeded casting
- simplified normalization helpers
v3 --> v4
- get rid of scmi setup/teardwn and use std module_init/module_exit
v1 --> v2
- fix sparse warning about missing static on global  *scmi_top_pcntrl;
---
 drivers/powercap/Kconfig             |  13 +
 drivers/powercap/Makefile            |   1 +
 drivers/powercap/arm_scmi_powercap.c | 544 +++++++++++++++++++++++++++
 3 files changed, 558 insertions(+)
 create mode 100644 drivers/powercap/arm_scmi_powercap.c

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index 515e3ceb3393..90d33cd1b670 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -44,6 +44,19 @@ config IDLE_INJECT
 	  synchronously on a set of specified CPUs or alternatively
 	  on a per CPU basis.
 
+config ARM_SCMI_POWERCAP
+	tristate "ARM SCMI Powercap driver"
+	depends on ARM_SCMI_PROTOCOL
+	help
+	  This enables support for the ARM Powercap based on ARM SCMI
+	  Powercap protocol.
+
+	  ARM SCMI Powercap protocol allows power limits to be enforced
+	  and monitored against the SCMI Powercap domains advertised as
+	  available by the SCMI platform firmware.
+
+	  When compiled as module it will be called arm_scmi_powercap.ko.
+
 config DTPM
 	bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)"
 	depends on OF
diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
index 494617cdad88..4474201b4aa7 100644
--- a/drivers/powercap/Makefile
+++ b/drivers/powercap/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
 obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
 obj-$(CONFIG_INTEL_RAPL) += intel_rapl_msr.o
 obj-$(CONFIG_IDLE_INJECT) += idle_inject.o
+obj-$(CONFIG_ARM_SCMI_POWERCAP) += arm_scmi_powercap.o
diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
new file mode 100644
index 000000000000..9f3b8386b4d8
--- /dev/null
+++ b/drivers/powercap/arm_scmi_powercap.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SCMI Powercap support.
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ */
+
+#include <linux/device.h>
+#include <linux/math.h>
+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/powercap.h>
+#include <linux/scmi_protocol.h>
+
+#define to_scmi_powercap_zone(z)		\
+	container_of(z, struct scmi_powercap_zone, zone)
+
+static const struct scmi_powercap_proto_ops *powercap_ops;
+
+struct scmi_powercap_zone {
+	unsigned int height;
+	struct device *dev;
+	struct scmi_protocol_handle *ph;
+	const struct scmi_powercap_info *info;
+	struct scmi_powercap_zone *spzones;
+	struct powercap_zone zone;
+	struct list_head node;
+};
+
+struct scmi_powercap_root {
+	unsigned int num_zones;
+	struct scmi_powercap_zone *spzones;
+	struct list_head *registered_zones;
+};
+
+static struct powercap_control_type *scmi_top_pcntrl;
+
+static int scmi_powercap_zone_release(struct powercap_zone *pz)
+{
+	return 0;
+}
+
+static int scmi_powercap_get_max_power_range_uw(struct powercap_zone *pz,
+						u64 *max_power_range_uw)
+{
+	*max_power_range_uw = U32_MAX;
+	return 0;
+}
+
+static int scmi_powercap_get_power_uw(struct powercap_zone *pz,
+				      u64 *power_uw)
+{
+	int ret;
+	u32 avg_power, pai;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (!spz->info->powercap_monitoring)
+		return -EINVAL;
+
+	ret = powercap_ops->measurements_get(spz->ph, spz->info->id, &avg_power,
+					     &pai);
+	if (ret)
+		return ret;
+
+	if (spz->info->powercap_scale_mw)
+		*power_uw = avg_power * 1000;
+	else
+		*power_uw = avg_power;
+
+	return 0;
+}
+
+static const struct powercap_zone_ops zone_ops = {
+	.get_max_power_range_uw = scmi_powercap_get_max_power_range_uw,
+	.get_power_uw = scmi_powercap_get_power_uw,
+	.release = scmi_powercap_zone_release,
+};
+
+static inline void
+scmi_powercap_normalize_cap(const struct scmi_powercap_info *info,
+			    u64 power_limit_uw, u32 *normalized)
+{
+	u64 req_power;
+
+	if (info->powercap_scale_mw)
+		req_power = DIV_ROUND_UP_ULL(power_limit_uw, 1000);
+	else
+		req_power = power_limit_uw;
+
+	if (req_power > info->max_power_cap)
+		*normalized = info->max_power_cap;
+	else if (req_power < info->min_power_cap)
+		*normalized = info->min_power_cap;
+	else
+		/*
+		 * This cast is lossless since here @req_power is certain to be
+		 * within the range [min_power_cap, max_power_cap] whose bounds
+		 * are assured to be two unsigned 32bits quantities.
+		 */
+		*normalized = (u32)req_power;
+
+	*normalized = rounddown(*normalized, info->power_cap_step);
+}
+
+static int scmi_powercap_set_power_limit_uw(struct powercap_zone *pz, int cid,
+					    u64 power_uw)
+{
+	u32 norm_power;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (!spz->info->powercap_cap_config)
+		return -EINVAL;
+
+	scmi_powercap_normalize_cap(spz->info, power_uw, &norm_power);
+
+	return powercap_ops->cap_set(spz->ph, spz->info->id, norm_power, false);
+}
+
+static int scmi_powercap_get_power_limit_uw(struct powercap_zone *pz, int cid,
+					    u64 *power_limit_uw)
+{
+	int ret;
+	u32 power;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	ret = powercap_ops->cap_get(spz->ph, spz->info->id, &power);
+	if (ret)
+		return ret;
+
+	if (spz->info->powercap_scale_mw)
+		*power_limit_uw = power * 1000;
+	else
+		*power_limit_uw = power;
+
+	return 0;
+}
+
+static inline void
+scmi_powercap_normalize_time(const struct scmi_powercap_info *info,
+			     u64 time_us, u32 *normalized)
+{
+	if (time_us > info->max_pai)
+		*normalized = info->max_pai;
+	else if (time_us < info->min_pai)
+		*normalized = info->min_pai;
+	else
+		/*
+		 * This cast is lossless since here @time_us is certain to be
+		 * within the range [min_pai, max_pai] whose bounds
+		 * are assured to be two unsigned 32bits quantities.
+		 */
+		*normalized = (u32)time_us;
+
+	*normalized = rounddown(*normalized, info->pai_step);
+}
+
+static int scmi_powercap_set_time_window_us(struct powercap_zone *pz, int cid,
+					    u64 time_window_us)
+{
+	u32 norm_pai;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (!spz->info->powercap_pai_config)
+		return -EINVAL;
+
+	scmi_powercap_normalize_time(spz->info, time_window_us, &norm_pai);
+
+	return powercap_ops->pai_set(spz->ph, spz->info->id, norm_pai);
+}
+
+static int scmi_powercap_get_time_window_us(struct powercap_zone *pz, int cid,
+					    u64 *time_window_us)
+{
+	int ret;
+	u32 pai;
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	ret = powercap_ops->pai_get(spz->ph, spz->info->id, &pai);
+	if (ret)
+		return ret;
+
+	*time_window_us = pai;
+
+	return 0;
+}
+
+static int scmi_powercap_get_max_power_uw(struct powercap_zone *pz, int cid,
+					  u64 *max_power_uw)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (spz->info->powercap_scale_uw)
+		*max_power_uw = spz->info->max_power_cap;
+	else
+		*max_power_uw = spz->info->max_power_cap * 1000;
+
+	return 0;
+}
+
+static int scmi_powercap_get_min_power_uw(struct powercap_zone *pz, int cid,
+					  u64 *min_power_uw)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	if (spz->info->powercap_scale_uw)
+		*min_power_uw = spz->info->min_power_cap;
+	else
+		*min_power_uw = spz->info->min_power_cap * 1000;
+
+	return 0;
+}
+
+static int scmi_powercap_get_max_time_window_us(struct powercap_zone *pz,
+						int cid, u64 *time_window_us)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	*time_window_us = spz->info->max_pai;
+
+	return 0;
+}
+
+static int scmi_powercap_get_min_time_window_us(struct powercap_zone *pz,
+						int cid, u64 *time_window_us)
+{
+	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
+
+	if (!spz->info)
+		return -ENODEV;
+
+	*time_window_us = (u64)spz->info->min_pai;
+
+	return 0;
+}
+
+static const char *scmi_powercap_get_name(struct powercap_zone *pz, int cid)
+{
+	return "SCMI power-cap";
+}
+
+static const struct powercap_zone_constraint_ops constraint_ops  = {
+	.set_power_limit_uw = scmi_powercap_set_power_limit_uw,
+	.get_power_limit_uw = scmi_powercap_get_power_limit_uw,
+	.set_time_window_us = scmi_powercap_set_time_window_us,
+	.get_time_window_us = scmi_powercap_get_time_window_us,
+	.get_max_power_uw = scmi_powercap_get_max_power_uw,
+	.get_min_power_uw = scmi_powercap_get_min_power_uw,
+	.get_max_time_window_us = scmi_powercap_get_max_time_window_us,
+	.get_min_time_window_us = scmi_powercap_get_min_time_window_us,
+	.get_name = scmi_powercap_get_name,
+};
+
+static void scmi_powercap_unregister_all_zones(struct scmi_powercap_root *pr)
+{
+	int i;
+
+	/* Un-register children zones first starting from the leaves */
+	for (i = pr->num_zones - 1; i >= 0; i--) {
+		if (!list_empty(&pr->registered_zones[i])) {
+			struct scmi_powercap_zone *spz;
+
+			list_for_each_entry(spz, &pr->registered_zones[i], node)
+				powercap_unregister_zone(scmi_top_pcntrl,
+							 &spz->zone);
+		}
+	}
+}
+
+static inline bool
+scmi_powercap_is_zone_registered(struct scmi_powercap_zone *spz)
+{
+	return !list_empty(&spz->node);
+}
+
+static inline unsigned int
+scmi_powercap_get_zone_height(struct scmi_powercap_zone *spz)
+{
+	if (spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID)
+		return 0;
+
+	return spz->spzones[spz->info->parent_id].height + 1;
+}
+
+static inline struct scmi_powercap_zone *
+scmi_powercap_get_parent_zone(struct scmi_powercap_zone *spz)
+{
+	if (spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID)
+		return NULL;
+
+	return &spz->spzones[spz->info->parent_id];
+}
+
+/**
+ * scmi_powercap_register_zone  - Register an SCMI powercap zone recursively
+ *
+ * @pr: A reference to the root powercap zones descriptors
+ * @spz: A reference to the SCMI powercap zone to register
+ *
+ * When registering SCMI powercap zones with the powercap framework we should
+ * take care to always register zones starting from the root ones and to
+ * deregister starting from the leaves.
+ *
+ * Unfortunately we cannot assume that the array of available SCMI powercap
+ * zones provided by the SCMI platform firmware is built to comply with such
+ * requirement.
+ *
+ * This function, given an SCMI powercap zone to register, takes care to walk
+ * the SCMI powercap zones tree up to the root looking recursively for
+ * unregistered parent zones before registering the provided zone; at the same
+ * time each registered zone height in such a tree is accounted for and each
+ * zone, once registered, is stored in the @registered_zones array that is
+ * indexed by zone height: this way will be trivial, at unregister time, to walk
+ * the @registered_zones array backward and unregister all the zones starting
+ * from the leaves, removing children zones before parents.
+ *
+ * While doing this, we prune away any zone marked as invalid (like the ones
+ * sporting an SCMI abstract power scale) as long as they are positioned as
+ * leaves in the SCMI powercap zones hierarchy: any non-leaf invalid zone causes
+ * the entire process to fail since we cannot assume the correctness of an SCMI
+ * powercap zones hierarchy if some of the internal nodes are missing.
+ *
+ * Note that the array of SCMI powercap zones as returned by the SCMI platform
+ * is known to be sane, i.e. zones relationships have been validated at the
+ * protocol layer.
+ *
+ * Return: 0 on Success
+ */
+static int scmi_powercap_register_zone(struct scmi_powercap_root *pr,
+				       struct scmi_powercap_zone *spz)
+{
+	int ret = 0;
+	struct scmi_powercap_zone *parent;
+
+	if (!spz->info)
+		return ret;
+
+	parent = scmi_powercap_get_parent_zone(spz);
+	if (parent && !scmi_powercap_is_zone_registered(parent)) {
+		/*
+		 * Bail out if a parent domain was marked as unsupported:
+		 * only domains participating as leaves can be skipped.
+		 */
+		if (!parent->info)
+			return -ENODEV;
+
+		ret = scmi_powercap_register_zone(pr, parent);
+		if (ret)
+			return ret;
+	}
+
+	if (!scmi_powercap_is_zone_registered(spz)) {
+		struct powercap_zone *z;
+
+		z = powercap_register_zone(&spz->zone,
+					   scmi_top_pcntrl,
+					   spz->info->name,
+					   parent ? &parent->zone : NULL,
+					   &zone_ops, 1, &constraint_ops);
+		if (!IS_ERR(z)) {
+			spz->height = scmi_powercap_get_zone_height(spz);
+			list_add(&spz->node,
+				 &pr->registered_zones[spz->height]);
+			dev_dbg(spz->dev,
+				"Registered node %s - parent %s - height:%d\n",
+				spz->info->name,
+				parent ? parent->info->name : "ROOT",
+				spz->height);
+			ret = 0;
+		} else {
+			ret = PTR_ERR(z);
+			dev_err(spz->dev,
+				"Error registering node:%s - parent:%s - h:%d - ret:%d\n",
+				 spz->info->name,
+				 parent ? parent->info->name : "ROOT",
+				 spz->height, ret);
+		}
+	}
+
+	return ret;
+}
+
+static int scmi_powercap_probe(struct scmi_device *sdev)
+{
+	int ret, i;
+	struct scmi_powercap_root *pr;
+	struct scmi_powercap_zone *spz;
+	struct scmi_protocol_handle *ph;
+	struct device *dev = &sdev->dev;
+	const struct scmi_handle *handle = sdev->handle;
+
+	if (!handle)
+		return -ENODEV;
+
+	powercap_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_POWERCAP,
+						 &ph);
+	if (IS_ERR(powercap_ops))
+		return PTR_ERR(powercap_ops);
+
+	pr = devm_kzalloc(dev, sizeof(*pr), GFP_KERNEL);
+	if (!pr)
+		return -ENOMEM;
+
+	pr->num_zones = powercap_ops->num_domains_get(ph);
+	if (pr->num_zones < 0) {
+		dev_err(dev, "number of powercap domains not found\n");
+		return pr->num_zones;
+	}
+
+	pr->spzones = devm_kcalloc(dev, pr->num_zones,
+				   sizeof(*pr->spzones), GFP_KERNEL);
+	if (!pr->spzones)
+		return -ENOMEM;
+
+	/* Allocate for worst possible scenario of maximum tree height. */
+	pr->registered_zones = devm_kcalloc(dev, pr->num_zones,
+					    sizeof(*pr->registered_zones),
+					    GFP_KERNEL);
+	if (!pr->registered_zones)
+		return -ENOMEM;
+
+	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
+		/*
+		 * Powercap domains are validate by the protocol layer, i.e.
+		 * when only non-NULL domains are returned here, whose
+		 * parent_id is assured to point to another valid domain.
+		 */
+		spz->info = powercap_ops->info_get(ph, i);
+
+		spz->dev = dev;
+		spz->ph = ph;
+		spz->spzones = pr->spzones;
+		INIT_LIST_HEAD(&spz->node);
+		INIT_LIST_HEAD(&pr->registered_zones[i]);
+
+		/*
+		 * Forcibly skip powercap domains using an abstract scale.
+		 * Note that only leaves domains can be skipped, so this could
+		 * lead later to a global failure.
+		 */
+		if (!spz->info->powercap_scale_uw &&
+		    !spz->info->powercap_scale_mw) {
+			dev_warn(dev,
+				 "Abstract power scale not supported. Skip %s.\n",
+				 spz->info->name);
+			spz->info = NULL;
+			continue;
+		}
+	}
+
+	/*
+	 * Scan array of retrieved SCMI powercap domains and register them
+	 * recursively starting from the root domains.
+	 */
+	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
+		ret = scmi_powercap_register_zone(pr, spz);
+		if (ret) {
+			dev_err(dev,
+				"Failed to register powercap zone %s - ret:%d\n",
+				spz->info->name, ret);
+			scmi_powercap_unregister_all_zones(pr);
+			return ret;
+		}
+	}
+
+	dev_set_drvdata(dev, pr);
+
+	dev_info(dev, "Registered %d SCMI Powercap domains !\n", pr->num_zones);
+
+	return ret;
+}
+
+static void scmi_powercap_remove(struct scmi_device *sdev)
+{
+	struct device *dev = &sdev->dev;
+	struct scmi_powercap_root *pr = dev_get_drvdata(dev);
+
+	scmi_powercap_unregister_all_zones(pr);
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_POWERCAP, "powercap" },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver scmi_powercap_driver = {
+	.name = "scmi-powercap",
+	.probe = scmi_powercap_probe,
+	.remove = scmi_powercap_remove,
+	.id_table = scmi_id_table,
+};
+
+static int __init scmi_powercap_init(void)
+{
+	int ret;
+
+	scmi_top_pcntrl = powercap_register_control_type(NULL, "arm-scmi", NULL);
+	if (!scmi_top_pcntrl)
+		return -ENODEV;
+
+	ret = scmi_register(&scmi_powercap_driver);
+	if (ret)
+		powercap_unregister_control_type(scmi_top_pcntrl);
+
+	return ret;
+}
+module_init(scmi_powercap_init);
+
+static void __exit scmi_powercap_exit(void)
+{
+	scmi_unregister(&scmi_powercap_driver);
+
+	powercap_unregister_control_type(scmi_top_pcntrl);
+}
+module_exit(scmi_powercap_exit);
+
+MODULE_AUTHOR("Cristian Marussi <cristian.marussi@arm.com>");
+MODULE_DESCRIPTION("ARM SCMI Powercap driver");
+MODULE_LICENSE("GPL");
-- 
2.32.0

Re: [PATCH 1/3] powercap: arm_scmi: Add SCMI Powercap based driver

[Lukasz Luba]

Hi Cristian,

On 8/17/22 11:54, Cristian Marussi wrote:
> Add a powercap driver that, using the ARM SCMI Protocol to query the SCMI
> platform firmware for the list of existing Powercap domains, registers all
> of such discovered domains under the new 'arm-scmi' powercap control type.
> 
> A new simple powercap zone and constraint is registered for all the SCMI
> powercap zones that are found.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v4 --> v5
> - fix _init cleaning up powerctrl zone on failure
> - removed uneeded casting
> - simplified normalization helpers
> v3 --> v4
> - get rid of scmi setup/teardwn and use std module_init/module_exit
> v1 --> v2
> - fix sparse warning about missing static on global  *scmi_top_pcntrl;
> ---
>   drivers/powercap/Kconfig             |  13 +
>   drivers/powercap/Makefile            |   1 +
>   drivers/powercap/arm_scmi_powercap.c | 544 +++++++++++++++++++++++++++
>   3 files changed, 558 insertions(+)
>   create mode 100644 drivers/powercap/arm_scmi_powercap.c
> 
> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> index 515e3ceb3393..90d33cd1b670 100644
> --- a/drivers/powercap/Kconfig
> +++ b/drivers/powercap/Kconfig
> @@ -44,6 +44,19 @@ config IDLE_INJECT
>   	  synchronously on a set of specified CPUs or alternatively
>   	  on a per CPU basis.
>   
> +config ARM_SCMI_POWERCAP
> +	tristate "ARM SCMI Powercap driver"
> +	depends on ARM_SCMI_PROTOCOL
> +	help
> +	  This enables support for the ARM Powercap based on ARM SCMI
> +	  Powercap protocol.
> +
> +	  ARM SCMI Powercap protocol allows power limits to be enforced
> +	  and monitored against the SCMI Powercap domains advertised as
> +	  available by the SCMI platform firmware.
> +
> +	  When compiled as module it will be called arm_scmi_powercap.ko.
> +
>   config DTPM
>   	bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)"
>   	depends on OF
> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
> index 494617cdad88..4474201b4aa7 100644
> --- a/drivers/powercap/Makefile
> +++ b/drivers/powercap/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
>   obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
>   obj-$(CONFIG_INTEL_RAPL) += intel_rapl_msr.o
>   obj-$(CONFIG_IDLE_INJECT) += idle_inject.o
> +obj-$(CONFIG_ARM_SCMI_POWERCAP) += arm_scmi_powercap.o
> diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> new file mode 100644
> index 000000000000..9f3b8386b4d8
> --- /dev/null
> +++ b/drivers/powercap/arm_scmi_powercap.c
> @@ -0,0 +1,544 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SCMI Powercap support.
> + *
> + * Copyright (C) 2022 ARM Ltd.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/math.h>
> +#include <linux/limits.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/powercap.h>
> +#include <linux/scmi_protocol.h>
> +
> +#define to_scmi_powercap_zone(z)		\
> +	container_of(z, struct scmi_powercap_zone, zone)
> +
> +static const struct scmi_powercap_proto_ops *powercap_ops;
> +
> +struct scmi_powercap_zone {
> +	unsigned int height;
> +	struct device *dev;
> +	struct scmi_protocol_handle *ph;
> +	const struct scmi_powercap_info *info;
> +	struct scmi_powercap_zone *spzones;
> +	struct powercap_zone zone;
> +	struct list_head node;
> +};
> +
> +struct scmi_powercap_root {
> +	unsigned int num_zones;
> +	struct scmi_powercap_zone *spzones;
> +	struct list_head *registered_zones;
> +};
> +
> +static struct powercap_control_type *scmi_top_pcntrl;
> +
> +static int scmi_powercap_zone_release(struct powercap_zone *pz)
> +{
> +	return 0;
> +}
> +
> +static int scmi_powercap_get_max_power_range_uw(struct powercap_zone *pz,
> +						u64 *max_power_range_uw)
> +{
> +	*max_power_range_uw = U32_MAX;

Shouldn't be calculated based on pai info from the platform FW?
e.g.
*max_power_range_uw = spz->info->max_power_cap - spz->info->min_power_cap

(but with uW conversion in mind if needed)

> +	return 0;
> +}
> +
> +static int scmi_powercap_get_power_uw(struct powercap_zone *pz,
> +				      u64 *power_uw)
> +{
> +	int ret;
> +	u32 avg_power, pai;
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);

Please order these lines from longest (on top) to shortest.

> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	if (!spz->info->powercap_monitoring)
> +		return -EINVAL;

Those checks might be done during setup. Please find my comment there.

> +
> +	ret = powercap_ops->measurements_get(spz->ph, spz->info->id, &avg_power,
> +					     &pai);
> +	if (ret)
> +		return ret;
> +
> +	if (spz->info->powercap_scale_mw)
> +		*power_uw = avg_power * 1000;
> +	else
> +		*power_uw = avg_power;

Maybe:

	*power_uw = avg_power;
	if (spz->info->powercap_scale_mw)
		*power_uw *= 1000;

> +
> +	return 0;
> +}
> +
> +static const struct powercap_zone_ops zone_ops = {
> +	.get_max_power_range_uw = scmi_powercap_get_max_power_range_uw,
> +	.get_power_uw = scmi_powercap_get_power_uw,
> +	.release = scmi_powercap_zone_release,
> +};
> +
> +static inline void

Drop inline here, compiler will figure out.

> +scmi_powercap_normalize_cap(const struct scmi_powercap_info *info,
> +			    u64 power_limit_uw, u32 *normalized)
> +{
> +	u64 req_power;
> +
> +	if (info->powercap_scale_mw)
> +		req_power = DIV_ROUND_UP_ULL(power_limit_uw, 1000);
> +	else
> +		req_power = power_limit_uw;
> +
> +	if (req_power > info->max_power_cap)
> +		*normalized = info->max_power_cap;
> +	else if (req_power < info->min_power_cap)
> +		*normalized = info->min_power_cap;
> +	else
> +		/*
> +		 * This cast is lossless since here @req_power is certain to be
> +		 * within the range [min_power_cap, max_power_cap] whose bounds
> +		 * are assured to be two unsigned 32bits quantities.
> +		 */
> +		*normalized = (u32)req_power;

clamp() or clamp_t() might simplify those lines

> +
> +	*normalized = rounddown(*normalized, info->power_cap_step);
> +}
> +
> +static int scmi_powercap_set_power_limit_uw(struct powercap_zone *pz, int cid,
> +					    u64 power_uw)
> +{
> +	u32 norm_power;
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);

sorting ^^^

> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	if (!spz->info->powercap_cap_config)
> +		return -EINVAL;
> +
> +	scmi_powercap_normalize_cap(spz->info, power_uw, &norm_power);
> +
> +	return powercap_ops->cap_set(spz->ph, spz->info->id, norm_power, false);
> +}
> +
> +static int scmi_powercap_get_power_limit_uw(struct powercap_zone *pz, int cid,
> +					    u64 *power_limit_uw)
> +{
> +	int ret;
> +	u32 power;
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);

sorting ^^^

> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	ret = powercap_ops->cap_get(spz->ph, spz->info->id, &power);
> +	if (ret)
> +		return ret;
> +
> +	if (spz->info->powercap_scale_mw)
> +		*power_limit_uw = power * 1000;
> +	else
> +		*power_limit_uw = power;

	*power_limit_uw = power;
	if (spz->info->powercap_scale_mw)
		*power_limit_uw *= 1000;

> +
> +	return 0;
> +}
> +
> +static inline void

IMO inline not needed here, compiler will know.

> +scmi_powercap_normalize_time(const struct scmi_powercap_info *info,
> +			     u64 time_us, u32 *normalized)
> +{
> +	if (time_us > info->max_pai)
> +		*normalized = info->max_pai;
> +	else if (time_us < info->min_pai)
> +		*normalized = info->min_pai;
> +	else
> +		/*
> +		 * This cast is lossless since here @time_us is certain to be
> +		 * within the range [min_pai, max_pai] whose bounds
> +		 * are assured to be two unsigned 32bits quantities.
> +		 */
> +		*normalized = (u32)time_us;

clamp() ?

> +
> +	*normalized = rounddown(*normalized, info->pai_step);
> +}
> +
> +static int scmi_powercap_set_time_window_us(struct powercap_zone *pz, int cid,
> +					    u64 time_window_us)
> +{
> +	u32 norm_pai;
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	if (!spz->info->powercap_pai_config)
> +		return -EINVAL;
> +
> +	scmi_powercap_normalize_time(spz->info, time_window_us, &norm_pai);
> +
> +	return powercap_ops->pai_set(spz->ph, spz->info->id, norm_pai);
> +}
> +
> +static int scmi_powercap_get_time_window_us(struct powercap_zone *pz, int cid,
> +					    u64 *time_window_us)
> +{
> +	int ret;
> +	u32 pai;
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);

sorting

> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	ret = powercap_ops->pai_get(spz->ph, spz->info->id, &pai);
> +	if (ret)
> +		return ret;
> +
> +	*time_window_us = pai;
> +
> +	return 0;
> +}
> +
> +static int scmi_powercap_get_max_power_uw(struct powercap_zone *pz, int cid,
> +					  u64 *max_power_uw)
> +{
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	if (spz->info->powercap_scale_uw)
> +		*max_power_uw = spz->info->max_power_cap;
> +	else
> +		*max_power_uw = spz->info->max_power_cap * 1000;

	*max_power_uw = spz->info->max_power_cap;
	if (spz->info->powercap_scale_mw)
		*max_power_uw *= 1000;

> +
> +	return 0;
> +}
> +
> +static int scmi_powercap_get_min_power_uw(struct powercap_zone *pz, int cid,
> +					  u64 *min_power_uw)
> +{
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	if (spz->info->powercap_scale_uw)
> +		*min_power_uw = spz->info->min_power_cap;
> +	else
> +		*min_power_uw = spz->info->min_power_cap * 1000;

	*min_power_uw = spz->info->min_power_cap;
	if (spz->info->powercap_scale_mw)
		*min_power_uw *= 1000;

> +
> +	return 0;
> +}
> +
> +static int scmi_powercap_get_max_time_window_us(struct powercap_zone *pz,
> +						int cid, u64 *time_window_us)
> +{
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	*time_window_us = spz->info->max_pai;
> +
> +	return 0;
> +}
> +
> +static int scmi_powercap_get_min_time_window_us(struct powercap_zone *pz,
> +						int cid, u64 *time_window_us)
> +{
> +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> +
> +	if (!spz->info)
> +		return -ENODEV;
> +
> +	*time_window_us = (u64)spz->info->min_pai;
> +
> +	return 0;
> +}
> +
> +static const char *scmi_powercap_get_name(struct powercap_zone *pz, int cid)
> +{
> +	return "SCMI power-cap";
> +}
> +
> +static const struct powercap_zone_constraint_ops constraint_ops  = {
> +	.set_power_limit_uw = scmi_powercap_set_power_limit_uw,
> +	.get_power_limit_uw = scmi_powercap_get_power_limit_uw,
> +	.set_time_window_us = scmi_powercap_set_time_window_us,
> +	.get_time_window_us = scmi_powercap_get_time_window_us,
> +	.get_max_power_uw = scmi_powercap_get_max_power_uw,
> +	.get_min_power_uw = scmi_powercap_get_min_power_uw,
> +	.get_max_time_window_us = scmi_powercap_get_max_time_window_us,
> +	.get_min_time_window_us = scmi_powercap_get_min_time_window_us,
> +	.get_name = scmi_powercap_get_name,
> +};
> +
> +static void scmi_powercap_unregister_all_zones(struct scmi_powercap_root *pr)
> +{
> +	int i;
> +
> +	/* Un-register children zones first starting from the leaves */
> +	for (i = pr->num_zones - 1; i >= 0; i--) {
> +		if (!list_empty(&pr->registered_zones[i])) {
> +			struct scmi_powercap_zone *spz;
> +
> +			list_for_each_entry(spz, &pr->registered_zones[i], node)
> +				powercap_unregister_zone(scmi_top_pcntrl,
> +							 &spz->zone);
> +		}
> +	}
> +}
> +
> +static inline bool
> +scmi_powercap_is_zone_registered(struct scmi_powercap_zone *spz)
> +{
> +	return !list_empty(&spz->node);
> +}
> +
> +static inline unsigned int
> +scmi_powercap_get_zone_height(struct scmi_powercap_zone *spz)
> +{
> +	if (spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID)
> +		return 0;
> +
> +	return spz->spzones[spz->info->parent_id].height + 1;
> +}
> +
> +static inline struct scmi_powercap_zone *
> +scmi_powercap_get_parent_zone(struct scmi_powercap_zone *spz)
> +{
> +	if (spz->info->parent_id == SCMI_POWERCAP_ROOT_ZONE_ID)
> +		return NULL;
> +
> +	return &spz->spzones[spz->info->parent_id];
> +}
> +
> +/**
> + * scmi_powercap_register_zone  - Register an SCMI powercap zone recursively
> + *
> + * @pr: A reference to the root powercap zones descriptors
> + * @spz: A reference to the SCMI powercap zone to register
> + *
> + * When registering SCMI powercap zones with the powercap framework we should
> + * take care to always register zones starting from the root ones and to
> + * deregister starting from the leaves.
> + *
> + * Unfortunately we cannot assume that the array of available SCMI powercap
> + * zones provided by the SCMI platform firmware is built to comply with such
> + * requirement.
> + *
> + * This function, given an SCMI powercap zone to register, takes care to walk
> + * the SCMI powercap zones tree up to the root looking recursively for
> + * unregistered parent zones before registering the provided zone; at the same
> + * time each registered zone height in such a tree is accounted for and each
> + * zone, once registered, is stored in the @registered_zones array that is
> + * indexed by zone height: this way will be trivial, at unregister time, to walk
> + * the @registered_zones array backward and unregister all the zones starting
> + * from the leaves, removing children zones before parents.
> + *
> + * While doing this, we prune away any zone marked as invalid (like the ones
> + * sporting an SCMI abstract power scale) as long as they are positioned as
> + * leaves in the SCMI powercap zones hierarchy: any non-leaf invalid zone causes
> + * the entire process to fail since we cannot assume the correctness of an SCMI
> + * powercap zones hierarchy if some of the internal nodes are missing.
> + *
> + * Note that the array of SCMI powercap zones as returned by the SCMI platform
> + * is known to be sane, i.e. zones relationships have been validated at the
> + * protocol layer.
> + *
> + * Return: 0 on Success
> + */
> +static int scmi_powercap_register_zone(struct scmi_powercap_root *pr,
> +				       struct scmi_powercap_zone *spz)
> +{
> +	int ret = 0;
> +	struct scmi_powercap_zone *parent;
> +
> +	if (!spz->info)
> +		return ret;
> +
> +	parent = scmi_powercap_get_parent_zone(spz);
> +	if (parent && !scmi_powercap_is_zone_registered(parent)) {
> +		/*
> +		 * Bail out if a parent domain was marked as unsupported:
> +		 * only domains participating as leaves can be skipped.
> +		 */
> +		if (!parent->info)
> +			return -ENODEV;
> +
> +		ret = scmi_powercap_register_zone(pr, parent);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!scmi_powercap_is_zone_registered(spz)) {
> +		struct powercap_zone *z;
> +
> +		z = powercap_register_zone(&spz->zone,
> +					   scmi_top_pcntrl,
> +					   spz->info->name,
> +					   parent ? &parent->zone : NULL,
> +					   &zone_ops, 1, &constraint_ops);
> +		if (!IS_ERR(z)) {
> +			spz->height = scmi_powercap_get_zone_height(spz);
> +			list_add(&spz->node,
> +				 &pr->registered_zones[spz->height]);
> +			dev_dbg(spz->dev,
> +				"Registered node %s - parent %s - height:%d\n",
> +				spz->info->name,
> +				parent ? parent->info->name : "ROOT",
> +				spz->height);
> +			ret = 0;
> +		} else {
> +			ret = PTR_ERR(z);
> +			dev_err(spz->dev,
> +				"Error registering node:%s - parent:%s - h:%d - ret:%d\n",
> +				 spz->info->name,
> +				 parent ? parent->info->name : "ROOT",
> +				 spz->height, ret);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int scmi_powercap_probe(struct scmi_device *sdev)
> +{
> +	int ret, i;
> +	struct scmi_powercap_root *pr;
> +	struct scmi_powercap_zone *spz;
> +	struct scmi_protocol_handle *ph;
> +	struct device *dev = &sdev->dev;
> +	const struct scmi_handle *handle = sdev->handle;

I would drop 'handle' since it's used once only, then sort those
lines in order.

> +
> +	if (!handle)

maybe:
	if (!sdev->handle)

> +		return -ENODEV;
> +
> +	powercap_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_POWERCAP,

and here 'sdev->handle'

> +						 &ph);
> +	if (IS_ERR(powercap_ops))
> +		return PTR_ERR(powercap_ops);
> +
> +	pr = devm_kzalloc(dev, sizeof(*pr), GFP_KERNEL);
> +	if (!pr)
> +		return -ENOMEM;
> +
> +	pr->num_zones = powercap_ops->num_domains_get(ph);
> +	if (pr->num_zones < 0) {
> +		dev_err(dev, "number of powercap domains not found\n");
> +		return pr->num_zones;
> +	}
> +
> +	pr->spzones = devm_kcalloc(dev, pr->num_zones,
> +				   sizeof(*pr->spzones), GFP_KERNEL);
> +	if (!pr->spzones)
> +		return -ENOMEM;
> +
> +	/* Allocate for worst possible scenario of maximum tree height. */
> +	pr->registered_zones = devm_kcalloc(dev, pr->num_zones,
> +					    sizeof(*pr->registered_zones),
> +					    GFP_KERNEL);
> +	if (!pr->registered_zones)
> +		return -ENOMEM;
> +
> +	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
> +		/*
> +		 * Powercap domains are validate by the protocol layer, i.e.
> +		 * when only non-NULL domains are returned here, whose
> +		 * parent_id is assured to point to another valid domain.
> +		 */
> +		spz->info = powercap_ops->info_get(ph, i);
> +
> +		spz->dev = dev;
> +		spz->ph = ph;
> +		spz->spzones = pr->spzones;
> +		INIT_LIST_HEAD(&spz->node);
> +		INIT_LIST_HEAD(&pr->registered_zones[i]);
> +
> +		/*
> +		 * Forcibly skip powercap domains using an abstract scale.
> +		 * Note that only leaves domains can be skipped, so this could
> +		 * lead later to a global failure.
> +		 */
> +		if (!spz->info->powercap_scale_uw &&
> +		    !spz->info->powercap_scale_mw) {
> +			dev_warn(dev,
> +				 "Abstract power scale not supported. Skip %s.\n",
> +				 spz->info->name);
> +			spz->info = NULL;
> +			continue;
> +		}

We can say that the power scale should be consistent in
a platform. Then we can bail out when abstract scale has
been found. This could also simplify code by a bit.

Can we also validate here some those lines, which are
checked in many callback funcitons?

These are the lines, which could be then removed if we bail
out here earlier:
	if (!spz->info)
		return -ENODEV;
	if (!spz->info->powercap_pai_config)
		return -EINVAL;
	if (!spz->info->powercap_monitoring)
		return -EINVAL;

> +	}
> +
> +	/*
> +	 * Scan array of retrieved SCMI powercap domains and register them
> +	 * recursively starting from the root domains.
> +	 */
> +	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
> +		ret = scmi_powercap_register_zone(pr, spz);
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to register powercap zone %s - ret:%d\n",
> +				spz->info->name, ret);
> +			scmi_powercap_unregister_all_zones(pr);
> +			return ret;
> +		}
> +	}
> +
> +	dev_set_drvdata(dev, pr);
> +
> +	dev_info(dev, "Registered %d SCMI Powercap domains !\n", pr->num_zones);
> +
> +	return ret;
> +}
> +


Regards,
Lukasz

Re: [PATCH 1/3] powercap: arm_scmi: Add SCMI Powercap based driver

[Cristian Marussi]

On Tue, Aug 30, 2022 at 02:16:42PM +0100, Lukasz Luba wrote:
> Hi Cristian,
> 

Hi Lukasz,

thanks for having a look first of all !

> On 8/17/22 11:54, Cristian Marussi wrote:
> > Add a powercap driver that, using the ARM SCMI Protocol to query the SCMI
> > platform firmware for the list of existing Powercap domains, registers all
> > of such discovered domains under the new 'arm-scmi' powercap control type.
> > 
> > A new simple powercap zone and constraint is registered for all the SCMI
> > powercap zones that are found.
> > 
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v4 --> v5
> > - fix _init cleaning up powerctrl zone on failure
> > - removed uneeded casting
> > - simplified normalization helpers
> > v3 --> v4
> > - get rid of scmi setup/teardwn and use std module_init/module_exit
> > v1 --> v2
> > - fix sparse warning about missing static on global  *scmi_top_pcntrl;
> > ---
> >   drivers/powercap/Kconfig             |  13 +
> >   drivers/powercap/Makefile            |   1 +
> >   drivers/powercap/arm_scmi_powercap.c | 544 +++++++++++++++++++++++++++
> >   3 files changed, 558 insertions(+)
> >   create mode 100644 drivers/powercap/arm_scmi_powercap.c
> > 
> > diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> > index 515e3ceb3393..90d33cd1b670 100644
> > --- a/drivers/powercap/Kconfig
> > +++ b/drivers/powercap/Kconfig
> > @@ -44,6 +44,19 @@ config IDLE_INJECT
> >   	  synchronously on a set of specified CPUs or alternatively
> >   	  on a per CPU basis.
> > +config ARM_SCMI_POWERCAP
> > +	tristate "ARM SCMI Powercap driver"
> > +	depends on ARM_SCMI_PROTOCOL
> > +	help
> > +	  This enables support for the ARM Powercap based on ARM SCMI
> > +	  Powercap protocol.
> > +
> > +	  ARM SCMI Powercap protocol allows power limits to be enforced
> > +	  and monitored against the SCMI Powercap domains advertised as
> > +	  available by the SCMI platform firmware.
> > +
> > +	  When compiled as module it will be called arm_scmi_powercap.ko.
> > +
> >   config DTPM
> >   	bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)"
> >   	depends on OF
> > diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
> > index 494617cdad88..4474201b4aa7 100644
> > --- a/drivers/powercap/Makefile
> > +++ b/drivers/powercap/Makefile
> > @@ -6,3 +6,4 @@ obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
> >   obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
> >   obj-$(CONFIG_INTEL_RAPL) += intel_rapl_msr.o
> >   obj-$(CONFIG_IDLE_INJECT) += idle_inject.o
> > +obj-$(CONFIG_ARM_SCMI_POWERCAP) += arm_scmi_powercap.o
> > diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> > new file mode 100644
> > index 000000000000..9f3b8386b4d8
> > --- /dev/null
> > +++ b/drivers/powercap/arm_scmi_powercap.c
> > @@ -0,0 +1,544 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SCMI Powercap support.
> > + *
> > + * Copyright (C) 2022 ARM Ltd.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/math.h>
> > +#include <linux/limits.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/powercap.h>
> > +#include <linux/scmi_protocol.h>
> > +
> > +#define to_scmi_powercap_zone(z)		\
> > +	container_of(z, struct scmi_powercap_zone, zone)
> > +
> > +static const struct scmi_powercap_proto_ops *powercap_ops;
> > +
> > +struct scmi_powercap_zone {
> > +	unsigned int height;
> > +	struct device *dev;
> > +	struct scmi_protocol_handle *ph;
> > +	const struct scmi_powercap_info *info;
> > +	struct scmi_powercap_zone *spzones;
> > +	struct powercap_zone zone;
> > +	struct list_head node;
> > +};
> > +
> > +struct scmi_powercap_root {
> > +	unsigned int num_zones;
> > +	struct scmi_powercap_zone *spzones;
> > +	struct list_head *registered_zones;
> > +};
> > +
> > +static struct powercap_control_type *scmi_top_pcntrl;
> > +
> > +static int scmi_powercap_zone_release(struct powercap_zone *pz)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int scmi_powercap_get_max_power_range_uw(struct powercap_zone *pz,
> > +						u64 *max_power_range_uw)
> > +{
> > +	*max_power_range_uw = U32_MAX;
> 
> Shouldn't be calculated based on pai info from the platform FW?
> e.g.
> *max_power_range_uw = spz->info->max_power_cap - spz->info->min_power_cap
> 
> (but with uW conversion in mind if needed)
> 

I double checked this and in include/linux/powercap.h these
powercap_zone_ops are defined as:

 * @get_max_power_range_uw:	Get maximum range of power counter in
 *				micro-watts.
 * @get_power_uw:		Get current power counter in micro-watts.

so these are really data related to average power consumed, i.e. in SCMI
parlance, info counters I can retrieve for a powercapping domain with
POWERCAP_MEASUREMENTS_GET, which returns a uint32 representing the
"average power consumption of the powercapping domain in the last PAI"

It seemed to me that this was unrelated to min/max powercap but more
something used to report actual powercap domain consumption, so I use
UINT32_MAX to represent the max range...on the other side in Linux these
powercap ops may seem more to expect to report a sort of progressive
accumulated comsuption value while I can only expose the average consumption
as calculated and reported by fw across the last PAI. (SCMI 4.10.3.10)

Looking again at this, I'm not sure really if this is ok for the powercap
Linux framework or should I instead try to keep a running accumulated value
inside this driver (built from the values I get from
POWERCAP_MEASUREMENTS_GET) and expose that....

... so thanks for pointing this out because it triggered more doubts :D
...any hint about this welcome.

> > +	return 0;
> > +}
> > +
> > +static int scmi_powercap_get_power_uw(struct powercap_zone *pz,
> > +				      u64 *power_uw)
> > +{
> > +	int ret;
> > +	u32 avg_power, pai;
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> 
> Please order these lines from longest (on top) to shortest.
> 

Ah ok..I was convinced to have to order those exactly the other way around :P
(from shortest to the longest, from unitialized to initialized)...I'll fix

> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	if (!spz->info->powercap_monitoring)
> > +		return -EINVAL;
> 
> Those checks might be done during setup. Please find my comment there.
> 

I agree partially, see my reply below near to your comment.

> > +
> > +	ret = powercap_ops->measurements_get(spz->ph, spz->info->id, &avg_power,
> > +					     &pai);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (spz->info->powercap_scale_mw)
> > +		*power_uw = avg_power * 1000;
> > +	else
> > +		*power_uw = avg_power;
> 
> Maybe:
> 
> 	*power_uw = avg_power;
> 	if (spz->info->powercap_scale_mw)
> 		*power_uw *= 1000;
> 

Yes definetely better, I'll fix in V6.

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct powercap_zone_ops zone_ops = {
> > +	.get_max_power_range_uw = scmi_powercap_get_max_power_range_uw,
> > +	.get_power_uw = scmi_powercap_get_power_uw,
> > +	.release = scmi_powercap_zone_release,
> > +};
> > +
> > +static inline void
> 
> Drop inline here, compiler will figure out.
> 

Ok.
> > +scmi_powercap_normalize_cap(const struct scmi_powercap_info *info,
> > +			    u64 power_limit_uw, u32 *normalized)
> > +{
> > +	u64 req_power;
> > +
> > +	if (info->powercap_scale_mw)
> > +		req_power = DIV_ROUND_UP_ULL(power_limit_uw, 1000);
> > +	else
> > +		req_power = power_limit_uw;
> > +
> > +	if (req_power > info->max_power_cap)
> > +		*normalized = info->max_power_cap;
> > +	else if (req_power < info->min_power_cap)
> > +		*normalized = info->min_power_cap;
> > +	else
> > +		/*
> > +		 * This cast is lossless since here @req_power is certain to be
> > +		 * within the range [min_power_cap, max_power_cap] whose bounds
> > +		 * are assured to be two unsigned 32bits quantities.
> > +		 */
> > +		*normalized = (u32)req_power;
> 
> clamp() or clamp_t() might simplify those lines
> 

Yes indeed I wondered if there was not some Kernel macros to do that in
a simpler way...thanks for pointing me at that ! I'll rework.

> > +
> > +	*normalized = rounddown(*normalized, info->power_cap_step);
> > +}
> > +
> > +static int scmi_powercap_set_power_limit_uw(struct powercap_zone *pz, int cid,
> > +					    u64 power_uw)
> > +{
> > +	u32 norm_power;
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> 
> sorting ^^^
> 

Ok.
> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	if (!spz->info->powercap_cap_config)
> > +		return -EINVAL;
> > +
> > +	scmi_powercap_normalize_cap(spz->info, power_uw, &norm_power);
> > +
> > +	return powercap_ops->cap_set(spz->ph, spz->info->id, norm_power, false);
> > +}
> > +
> > +static int scmi_powercap_get_power_limit_uw(struct powercap_zone *pz, int cid,
> > +					    u64 *power_limit_uw)
> > +{
> > +	int ret;
> > +	u32 power;
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> 
> sorting ^^^
> 
Ok.

> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	ret = powercap_ops->cap_get(spz->ph, spz->info->id, &power);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (spz->info->powercap_scale_mw)
> > +		*power_limit_uw = power * 1000;
> > +	else
> > +		*power_limit_uw = power;
> 
> 	*power_limit_uw = power;
> 	if (spz->info->powercap_scale_mw)
> 		*power_limit_uw *= 1000;
> 

Ditto.

> > +
> > +	return 0;
> > +}
> > +
> > +static inline void
> 
> IMO inline not needed here, compiler will know.
> 

Ok.

> > +scmi_powercap_normalize_time(const struct scmi_powercap_info *info,
> > +			     u64 time_us, u32 *normalized)
> > +{
> > +	if (time_us > info->max_pai)
> > +		*normalized = info->max_pai;
> > +	else if (time_us < info->min_pai)
> > +		*normalized = info->min_pai;
> > +	else
> > +		/*
> > +		 * This cast is lossless since here @time_us is certain to be
> > +		 * within the range [min_pai, max_pai] whose bounds
> > +		 * are assured to be two unsigned 32bits quantities.
> > +		 */
> > +		*normalized = (u32)time_us;
> 
> clamp() ?
> 

Ditto.

> > +
> > +	*normalized = rounddown(*normalized, info->pai_step);
> > +}
> > +
> > +static int scmi_powercap_set_time_window_us(struct powercap_zone *pz, int cid,
> > +					    u64 time_window_us)
> > +{
> > +	u32 norm_pai;
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	if (!spz->info->powercap_pai_config)
> > +		return -EINVAL;
> > +
> > +	scmi_powercap_normalize_time(spz->info, time_window_us, &norm_pai);
> > +
> > +	return powercap_ops->pai_set(spz->ph, spz->info->id, norm_pai);
> > +}
> > +
> > +static int scmi_powercap_get_time_window_us(struct powercap_zone *pz, int cid,
> > +					    u64 *time_window_us)
> > +{
> > +	int ret;
> > +	u32 pai;
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> 
> sorting
> 

Ok.

> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	ret = powercap_ops->pai_get(spz->ph, spz->info->id, &pai);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*time_window_us = pai;
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_powercap_get_max_power_uw(struct powercap_zone *pz, int cid,
> > +					  u64 *max_power_uw)
> > +{
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	if (spz->info->powercap_scale_uw)
> > +		*max_power_uw = spz->info->max_power_cap;
> > +	else
> > +		*max_power_uw = spz->info->max_power_cap * 1000;
> 
> 	*max_power_uw = spz->info->max_power_cap;
> 	if (spz->info->powercap_scale_mw)
> 		*max_power_uw *= 1000;
> 

Ditto.

> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_powercap_get_min_power_uw(struct powercap_zone *pz, int cid,
> > +					  u64 *min_power_uw)
> > +{
> > +	struct scmi_powercap_zone *spz = to_scmi_powercap_zone(pz);
> > +
> > +	if (!spz->info)
> > +		return -ENODEV;
> > +
> > +	if (spz->info->powercap_scale_uw)
> > +		*min_power_uw = spz->info->min_power_cap;
> > +	else
> > +		*min_power_uw = spz->info->min_power_cap * 1000;
> 
> 	*min_power_uw = spz->info->min_power_cap;
> 	if (spz->info->powercap_scale_mw)
> 		*min_power_uw *= 1000;
> 

Ditto.

> > +
> > +	return 0;

[snip]

> > +
> > +static int scmi_powercap_probe(struct scmi_device *sdev)
> > +{
> > +	int ret, i;
> > +	struct scmi_powercap_root *pr;
> > +	struct scmi_powercap_zone *spz;
> > +	struct scmi_protocol_handle *ph;
> > +	struct device *dev = &sdev->dev;
> > +	const struct scmi_handle *handle = sdev->handle;
> 
> I would drop 'handle' since it's used once only, then sort those
> lines in order.
> 
> > +
> > +	if (!handle)
> 
> maybe:
> 	if (!sdev->handle)
> 
> > +		return -ENODEV;
> > +
> > +	powercap_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_POWERCAP,
> 
> and here 'sdev->handle'
> 

Yes indeed it's better, I'll do.

> > +						 &ph);
> > +	if (IS_ERR(powercap_ops))
> > +		return PTR_ERR(powercap_ops);
> > +
> > +	pr = devm_kzalloc(dev, sizeof(*pr), GFP_KERNEL);
> > +	if (!pr)
> > +		return -ENOMEM;
> > +
> > +	pr->num_zones = powercap_ops->num_domains_get(ph);
> > +	if (pr->num_zones < 0) {
> > +		dev_err(dev, "number of powercap domains not found\n");
> > +		return pr->num_zones;
> > +	}
> > +
> > +	pr->spzones = devm_kcalloc(dev, pr->num_zones,
> > +				   sizeof(*pr->spzones), GFP_KERNEL);
> > +	if (!pr->spzones)
> > +		return -ENOMEM;
> > +
> > +	/* Allocate for worst possible scenario of maximum tree height. */
> > +	pr->registered_zones = devm_kcalloc(dev, pr->num_zones,
> > +					    sizeof(*pr->registered_zones),
> > +					    GFP_KERNEL);
> > +	if (!pr->registered_zones)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
> > +		/*
> > +		 * Powercap domains are validate by the protocol layer, i.e.
> > +		 * when only non-NULL domains are returned here, whose
> > +		 * parent_id is assured to point to another valid domain.
> > +		 */
> > +		spz->info = powercap_ops->info_get(ph, i);
> > +
> > +		spz->dev = dev;
> > +		spz->ph = ph;
> > +		spz->spzones = pr->spzones;
> > +		INIT_LIST_HEAD(&spz->node);
> > +		INIT_LIST_HEAD(&pr->registered_zones[i]);
> > +
> > +		/*
> > +		 * Forcibly skip powercap domains using an abstract scale.
> > +		 * Note that only leaves domains can be skipped, so this could
> > +		 * lead later to a global failure.
> > +		 */
> > +		if (!spz->info->powercap_scale_uw &&
> > +		    !spz->info->powercap_scale_mw) {
> > +			dev_warn(dev,
> > +				 "Abstract power scale not supported. Skip %s.\n",
> > +				 spz->info->name);
> > +			spz->info = NULL;
> > +			continue;
> > +		}
> 
> We can say that the power scale should be consistent in
> a platform. Then we can bail out when abstract scale has
> been found. This could also simplify code by a bit.
> 

I do NOT agree on this since I do NOT think from the SCMI spec we can
assume this semplification: Linux powercap has indeed this limitation on
scales BUT other non-Linux agents could indeed support abstract scales and
the SCMI server could advertise a well built hierarchy of powercap domains
including some abstract scale ones tha, if placed as leaves of the hierarchy,
could be ignored by Linux but used instead by other agents...or in the future
used by Linux too ?

I'll double check with Archs since I had already an internal exchange on
this and seemed to me that the current approach (of only bailing out when
non-leaves abstract scale domains are found) was fine, i.e. that I could
not just assume to receive only uw/mv scale domains.

> Can we also validate here some those lines, which are
> checked in many callback funcitons?
> 

Partially yes....see below...

> These are the lines, which could be then removed if we bail
> out here earlier:
> 	if (!spz->info)
> 		return -ENODEV;

I can remove this surely from everywhere since I really never register a
zone with NULL spx->info, this check all-around, my bad, is redundant.

> 	if (!spz->info->powercap_pai_config)
> 		return -EINVAL;
> 	if (!spz->info->powercap_monitoring)
> 		return -EINVAL;
>

Instead I cannot see why a powercap domain missing this capabilities
(PAI configuration and power consumption monitoring) should be
excluded as a whole...for this reason (if valid from the scale
perspective as said above) I currently register these powercap SCMI
zones even if lacking these supports and then return -EINVAL only for
the related Powercap unsupported callbacks...while still supporting as
an example setting min/max powercaps.

Thanks,
Regards,
Cristian

Re: [PATCH 1/3] powercap: arm_scmi: Add SCMI Powercap based driver

[Lukasz Luba]

On 9/4/22 16:41, Cristian Marussi wrote:
> On Tue, Aug 30, 2022 at 02:16:42PM +0100, Lukasz Luba wrote:
>> Hi Cristian,
>>
> 
> Hi Lukasz,
> 

[snip]

>>> +static int scmi_powercap_get_max_power_range_uw(struct powercap_zone *pz,
>>> +						u64 *max_power_range_uw)
>>> +{
>>> +	*max_power_range_uw = U32_MAX;
>>
>> Shouldn't be calculated based on pai info from the platform FW?
>> e.g.
>> *max_power_range_uw = spz->info->max_power_cap - spz->info->min_power_cap
>>
>> (but with uW conversion in mind if needed)
>>
> 
> I double checked this and in include/linux/powercap.h these
> powercap_zone_ops are defined as:
> 
>   * @get_max_power_range_uw:	Get maximum range of power counter in
>   *				micro-watts.
>   * @get_power_uw:		Get current power counter in micro-watts.
> 
> so these are really data related to average power consumed, i.e. in SCMI
> parlance, info counters I can retrieve for a powercapping domain with
> POWERCAP_MEASUREMENTS_GET, which returns a uint32 representing the
> "average power consumption of the powercapping domain in the last PAI"
> 
> It seemed to me that this was unrelated to min/max powercap but more
> something used to report actual powercap domain consumption, so I use
> UINT32_MAX to represent the max range...on the other side in Linux these
> powercap ops may seem more to expect to report a sort of progressive
> accumulated comsuption value while I can only expose the average consumption
> as calculated and reported by fw across the last PAI. (SCMI 4.10.3.10)
> 
> Looking again at this, I'm not sure really if this is ok for the powercap
> Linux framework or should I instead try to keep a running accumulated value
> inside this driver (built from the values I get from
> POWERCAP_MEASUREMENTS_GET) and expose that....
> 
> ... so thanks for pointing this out because it triggered more doubts :D
> ...any hint about this welcome.

I recalled this code in DTPM [1]. Although, I have checked the
documentation of Powercap sysfs for this file [2]. This 'range'
for power (or energy) describes the values for related: 'power_uw'
or 'energy_uj'. Which means the 'power_uw' value can be actually
lower that setting in 'min_power_cap' (e.g. due to lightly loaded CPU).
I'm not sure for the upper bound: 'max_power_cap'. In real world
we can get a power spike which is bigger than that, so probably
your original U32_MAX is OK.

Therefore, probably the DTPM [1] could be adjusted not your aproach.


[snip]

>>> +	for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
>>> +		/*
>>> +		 * Powercap domains are validate by the protocol layer, i.e.
>>> +		 * when only non-NULL domains are returned here, whose
>>> +		 * parent_id is assured to point to another valid domain.
>>> +		 */
>>> +		spz->info = powercap_ops->info_get(ph, i);
>>> +
>>> +		spz->dev = dev;
>>> +		spz->ph = ph;
>>> +		spz->spzones = pr->spzones;
>>> +		INIT_LIST_HEAD(&spz->node);
>>> +		INIT_LIST_HEAD(&pr->registered_zones[i]);
>>> +
>>> +		/*
>>> +		 * Forcibly skip powercap domains using an abstract scale.
>>> +		 * Note that only leaves domains can be skipped, so this could
>>> +		 * lead later to a global failure.
>>> +		 */
>>> +		if (!spz->info->powercap_scale_uw &&
>>> +		    !spz->info->powercap_scale_mw) {
>>> +			dev_warn(dev,
>>> +				 "Abstract power scale not supported. Skip %s.\n",
>>> +				 spz->info->name);
>>> +			spz->info = NULL;
>>> +			continue;
>>> +		}
>>
>> We can say that the power scale should be consistent in
>> a platform. Then we can bail out when abstract scale has
>> been found. This could also simplify code by a bit.
>>
> 
> I do NOT agree on this since I do NOT think from the SCMI spec we can
> assume this semplification: Linux powercap has indeed this limitation on
> scales BUT other non-Linux agents could indeed support abstract scales and
> the SCMI server could advertise a well built hierarchy of powercap domains
> including some abstract scale ones tha, if placed as leaves of the hierarchy,
> could be ignored by Linux but used instead by other agents...or in the future
> used by Linux too ?

This diversity makes me a headache ;) I would hope the SCMI spec would
restrict the span of variety. Although, I cannot find in the spec that
all powercap domains must use the same power scale...

It looks like, you will have to implement it this way.

> 
> I'll double check with Archs since I had already an internal exchange on
> this and seemed to me that the current approach (of only bailing out when
> non-leaves abstract scale domains are found) was fine, i.e. that I could
> not just assume to receive only uw/mv scale domains.
> 
>> Can we also validate here some those lines, which are
>> checked in many callback funcitons?
>>
> 
> Partially yes....see below...
> 
>> These are the lines, which could be then removed if we bail
>> out here earlier:
>> 	if (!spz->info)
>> 		return -ENODEV;
> 
> I can remove this surely from everywhere since I really never register a
> zone with NULL spx->info, this check all-around, my bad, is redundant.
> 
>> 	if (!spz->info->powercap_pai_config)
>> 		return -EINVAL;
>> 	if (!spz->info->powercap_monitoring)
>> 		return -EINVAL;
>>
> 
> Instead I cannot see why a powercap domain missing this capabilities
> (PAI configuration and power consumption monitoring) should be
> excluded as a whole...for this reason (if valid from the scale
> perspective as said above) I currently register these powercap SCMI
> zones even if lacking these supports and then return -EINVAL only for
> the related Powercap unsupported callbacks...while still supporting as
> an example setting min/max powercaps.

It's a bit more complicated than I thought. We cannot simplify too much
and make weak assumption. You're right, please keep your approach.



[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/powercap/dtpm.c#L54
[2] 
https://elixir.bootlin.com/linux/latest/source/Documentation/power/powercap/powercap.rst#L206

[PATCH 2/3] powercap: arm_scmi: Fix signedness bug in probe

[Cristian Marussi]

From: Dan Carpenter <dan.carpenter@oracle.com>

The "pr->num_zones" variable is an unsigned int so it can't be less than
zero.

Fixes: 31afdd34f2b9 ("powercap: arm_scmi: Add SCMI powercap based driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/powercap/arm_scmi_powercap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
index 9f3b8386b4d8..dc26cbb32b35 100644
--- a/drivers/powercap/arm_scmi_powercap.c
+++ b/drivers/powercap/arm_scmi_powercap.c
@@ -425,11 +425,12 @@ static int scmi_powercap_probe(struct scmi_device *sdev)
 	if (!pr)
 		return -ENOMEM;
 
-	pr->num_zones = powercap_ops->num_domains_get(ph);
-	if (pr->num_zones < 0) {
+	ret = powercap_ops->num_domains_get(ph);
+	if (ret < 0) {
 		dev_err(dev, "number of powercap domains not found\n");
-		return pr->num_zones;
+		return ret;
 	}
+	pr->num_zones = ret;
 
 	pr->spzones = devm_kcalloc(dev, pr->num_zones,
 				   sizeof(*pr->spzones), GFP_KERNEL);
-- 
2.32.0

[PATCH 3/3] powercap: arm_scmi: Fix a NULL vs IS_ERR() bug

[Cristian Marussi]

From: Dan Carpenter <dan.carpenter@oracle.com>

The powercap_register_control_type() return error pointers.  It never
returns NULL.

Fixes: 31afdd34f2b9 ("powercap: arm_scmi: Add SCMI powercap based driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/powercap/arm_scmi_powercap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
index dc26cbb32b35..9993b7fa4993 100644
--- a/drivers/powercap/arm_scmi_powercap.c
+++ b/drivers/powercap/arm_scmi_powercap.c
@@ -521,8 +521,8 @@ static int __init scmi_powercap_init(void)
 	int ret;
 
 	scmi_top_pcntrl = powercap_register_control_type(NULL, "arm-scmi", NULL);
-	if (!scmi_top_pcntrl)
-		return -ENODEV;
+	if (IS_ERR(scmi_top_pcntrl))
+		return PTR_ERR(scmi_top_pcntrl);
 
 	ret = scmi_register(&scmi_powercap_driver);
 	if (ret)
-- 
2.32.0