[PATCH v3 0/6] SCMIv3.0 Sensor Extensions

[PATCH v3 0/6] SCMIv3.0 Sensor Extensions

[Cristian Marussi]

Hi,

this series is meant to add support for the new SCMI Sensor Protocol
features defined by the upcoming SCMIv3.0 specification, whose BETA
release is available at [1].

The series is currently based on for-next/scmi [2] on top of:

commit b141fca08207 ("firmware: arm_scmi: Fix missing destroy_workqueue()")

Any feedback welcome,

Thanks,

Cristian

---
v2 --> v3
- removed stale unused msg payload definition
- moved variable declaration inside switch block
- setting rx_size to 0 in sensor_reading_get to allow fw to send
  both v2 and v3 replies...even if sensor_reading_get() only handles
  v2 spec and returns one single value
- using get_unaligned_le64 in lieu of le64_to_cpu
- removed refs to v2.1
- Fix SCMI_MAX_NUM_SENSOR_AXIS to 63
- added missing Dox comment in resolution
- added common INTVL SEGMENT macros

v1 --> v2
- rebased on for-next/scmi v5.10
- restrict segmented intervals descriptors to single triplet
- add proper usage of scmi_reset_rx_to_maxsz

[1]:https://developer.arm.com/documentation/den0056/c/
[2]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi


Cristian Marussi (6):
  firmware: arm_scmi: rework scmi_sensors_protocol_init
  firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
  hwmon: scmi: update hwmon internal scale data type
  firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads
  firmware: arm_scmi: add SCMIv3.0 Sensor configuration support
  firmware: arm_scmi: add SCMIv3.0 Sensor notifications

 drivers/firmware/arm_scmi/sensors.c | 743 ++++++++++++++++++++++++++--
 drivers/hwmon/scmi-hwmon.c          |   2 +-
 include/linux/scmi_protocol.h       | 291 ++++++++++-
 3 files changed, 977 insertions(+), 59 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] firmware: arm_scmi: rework scmi_sensors_protocol_init

[Cristian Marussi]

Properly handle return values from initialization helpers and avoid
setting sensor_ops before sensor_priv.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/sensors.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index b4232d611033..6aaff478d032 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -2,7 +2,7 @@
 /*
  * System Control and Management Interface (SCMI) Sensor Protocol
  *
- * Copyright (C) 2018 ARM Ltd.
+ * Copyright (C) 2018-2020 ARM Ltd.
  */
 
 #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
@@ -334,6 +334,7 @@ static const struct scmi_event_ops sensor_event_ops = {
 static int scmi_sensors_protocol_init(struct scmi_handle *handle)
 {
 	u32 version;
+	int ret;
 	struct sensors_info *sinfo;
 
 	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);
@@ -344,15 +345,19 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle)
 	sinfo = devm_kzalloc(handle->dev, sizeof(*sinfo), GFP_KERNEL);
 	if (!sinfo)
 		return -ENOMEM;
+	sinfo->version = version;
 
-	scmi_sensor_attributes_get(handle, sinfo);
-
+	ret = scmi_sensor_attributes_get(handle, sinfo);
+	if (ret)
+		return ret;
 	sinfo->sensors = devm_kcalloc(handle->dev, sinfo->num_sensors,
 				      sizeof(*sinfo->sensors), GFP_KERNEL);
 	if (!sinfo->sensors)
 		return -ENOMEM;
 
-	scmi_sensor_description_get(handle, sinfo);
+	ret = scmi_sensor_description_get(handle, sinfo);
+	if (ret)
+		return ret;
 
 	scmi_register_protocol_events(handle,
 				      SCMI_PROTOCOL_SENSOR, SCMI_PROTO_QUEUE_SZ,
@@ -360,9 +365,8 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle)
 				      ARRAY_SIZE(sensor_events),
 				      sinfo->num_sensors);
 
-	sinfo->version = version;
-	handle->sensor_ops = &sensor_ops;
 	handle->sensor_priv = sinfo;
+	handle->sensor_ops = &sensor_ops;
 
 	return 0;
 }
-- 
2.17.1

[PATCH v3 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions

[Cristian Marussi]

Add support for new SCMIv3.0 Sensors extensions related to new sensors'
features, like multiple axis and update intervals, while keeping
compatibility with SCMIv2.0 features.
While at that, refactor and simplify all the internal helpers macros and
move struct scmi_sensor_info to use only non-fixed-size typing.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v2 --> v3
- Fix SCMI_MAX_NUM_SENSOR_AXIS
- added missing Dox comment in resolution
- added common INTVL SEGMENT macros

v1 --> v2
- restrict segmented intervals descriptors to single triplet
- add proper usage of scmi_reset_rx_to_maxsz
---
 drivers/firmware/arm_scmi/sensors.c | 391 ++++++++++++++++++++++++++--
 include/linux/scmi_protocol.h       | 223 +++++++++++++++-
 2 files changed, 588 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 6aaff478d032..1c83aaae0012 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -7,16 +7,21 @@
 
 #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
 
+#include <linux/bitfield.h>
 #include <linux/scmi_protocol.h>
 
 #include "common.h"
 #include "notify.h"
 
+#define SCMI_MAX_NUM_SENSOR_AXIS	63
+
 enum scmi_sensor_protocol_cmd {
 	SENSOR_DESCRIPTION_GET = 0x3,
 	SENSOR_TRIP_POINT_NOTIFY = 0x4,
 	SENSOR_TRIP_POINT_CONFIG = 0x5,
 	SENSOR_READING_GET = 0x6,
+	SENSOR_AXIS_DESCRIPTION_GET = 0x7,
+	SENSOR_LIST_UPDATE_INTERVALS = 0x8,
 };
 
 struct scmi_msg_resp_sensor_attributes {
@@ -28,23 +33,102 @@ struct scmi_msg_resp_sensor_attributes {
 	__le32 reg_size;
 };
 
+/* v3 attributes_low macros */
+#define SUPPORTS_UPDATE_NOTIFY(x)	FIELD_GET(BIT(30), (x))
+#define SENSOR_TSTAMP_EXP(x)		FIELD_GET(GENMASK(14, 10), (x))
+#define SUPPORTS_TIMESTAMP(x)		FIELD_GET(BIT(9), (x))
+#define SUPPORTS_EXTEND_ATTRS(x)	FIELD_GET(BIT(8), (x))
+
+/* v2 attributes_high macros */
+#define SENSOR_UPDATE_BASE(x)		FIELD_GET(GENMASK(31, 27), (x))
+#define SENSOR_UPDATE_SCALE(x)		FIELD_GET(GENMASK(26, 22), (x))
+
+/* v3 attributes_high macros */
+#define SENSOR_AXIS_NUMBER(x)		FIELD_GET(GENMASK(21, 16), (x))
+#define SUPPORTS_AXIS(x)		FIELD_GET(BIT(8), (x))
+
+/* v3 resolution macros */
+#define SENSOR_RES(x)			FIELD_GET(GENMASK(26, 0), (x))
+#define SENSOR_RES_EXP(x)		FIELD_GET(GENMASK(31, 27), (x))
+
+struct scmi_range_attrs_le {
+	__le32 min_range_low;
+	__le32 min_range_high;
+	__le32 max_range_low;
+	__le32 max_range_high;
+};
+
 struct scmi_msg_resp_sensor_description {
 	__le16 num_returned;
 	__le16 num_remaining;
-	struct {
+	struct scmi_sensor_descriptor {
 		__le32 id;
 		__le32 attributes_low;
-#define SUPPORTS_ASYNC_READ(x)	((x) & BIT(31))
-#define NUM_TRIP_POINTS(x)	((x) & 0xff)
+/* Common attributes_low macros */
+#define SUPPORTS_ASYNC_READ(x)		FIELD_GET(BIT(31), (x))
+#define NUM_TRIP_POINTS(x)		FIELD_GET(GENMASK(7, 0), (x))
 		__le32 attributes_high;
-#define SENSOR_TYPE(x)		((x) & 0xff)
-#define SENSOR_SCALE(x)		(((x) >> 11) & 0x1f)
-#define SENSOR_SCALE_SIGN	BIT(4)
-#define SENSOR_SCALE_EXTEND	GENMASK(7, 5)
-#define SENSOR_UPDATE_SCALE(x)	(((x) >> 22) & 0x1f)
-#define SENSOR_UPDATE_BASE(x)	(((x) >> 27) & 0x1f)
-		    u8 name[SCMI_MAX_STR_SIZE];
-	} desc[0];
+/* Common attributes_high macros */
+#define SENSOR_SCALE(x)			FIELD_GET(GENMASK(15, 11), (x))
+#define SENSOR_SCALE_SIGN		BIT(4)
+#define SENSOR_SCALE_EXTEND		GENMASK(31, 5)
+#define SENSOR_TYPE(x)			FIELD_GET(GENMASK(7, 0), (x))
+		u8 name[SCMI_MAX_STR_SIZE];
+		/* only for version > 2.0 */
+		__le32 power;
+		__le32 resolution;
+		struct scmi_range_attrs_le scalar_attrs;
+	} desc[];
+};
+
+/* Sign extend to a full s32 */
+#define	S32_EXT(v)							\
+	({								\
+		int __v = (v);						\
+									\
+		if (__v & SENSOR_SCALE_SIGN)				\
+			__v |= SENSOR_SCALE_EXTEND;			\
+		__v;							\
+	})
+
+#define SCMI_MSG_RESP_SENS_DESCR_BASE_SZ				\
+	(sizeof(struct scmi_sensor_descriptor) -			\
+	  (2 * sizeof(__le32)) - sizeof(struct scmi_range_attrs_le))
+
+struct scmi_msg_sensor_axis_description_get {
+	__le32 id;
+	__le32 axis_desc_index;
+};
+
+struct scmi_msg_resp_sensor_axis_description {
+	__le32 num_axis_flags;
+#define NUM_AXIS_RETURNED(x)		FIELD_GET(GENMASK(5, 0), (x))
+#define NUM_AXIS_REMAINING(x)		FIELD_GET(GENMASK(31, 26), (x))
+	struct scmi_axis_descriptor {
+		__le32 id;
+		__le32 attributes_low;
+		__le32 attributes_high;
+		u8 name[SCMI_MAX_STR_SIZE];
+		__le32 resolution;
+		struct scmi_range_attrs_le attrs;
+	} desc[];
+};
+
+#define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ				\
+		(sizeof(struct scmi_axis_descriptor) -			\
+		  sizeof(__le32) - sizeof(struct scmi_range_attrs_le))
+
+struct scmi_msg_sensor_list_update_intervals {
+	__le32 id;
+	__le32 index;
+};
+
+struct scmi_msg_resp_sensor_list_update_intervals {
+	__le32 num_intervals_flags;
+#define NUM_INTERVALS_RETURNED(x)	FIELD_GET(GENMASK(11, 0), (x))
+#define SEGMENTED_INTVL_FORMAT(x)	FIELD_GET(BIT(12), (x))
+#define NUM_INTERVALS_REMAINING(x)	FIELD_GET(GENMASK(31, 16), (x))
+	__le32 intervals[];
 };
 
 struct scmi_msg_sensor_trip_point_notify {
@@ -114,6 +198,194 @@ static int scmi_sensor_attributes_get(const struct scmi_handle *handle,
 	return ret;
 }
 
+static inline void scmi_parse_range_attrs(struct scmi_range_attrs *out,
+					  struct scmi_range_attrs_le *in)
+{
+	out->min_range = get_unaligned_le64((void *)&in->min_range_low);
+	out->max_range = get_unaligned_le64((void *)&in->max_range_low);
+}
+
+static int scmi_sensor_update_intervals(const struct scmi_handle *handle,
+					struct scmi_sensor_info *s)
+{
+	int ret, cnt;
+	u32 desc_index = 0;
+	u16 num_returned, num_remaining;
+	struct scmi_xfer *ti;
+	struct scmi_msg_resp_sensor_list_update_intervals *buf;
+	struct scmi_msg_sensor_list_update_intervals *msg;
+
+	ret = scmi_xfer_get_init(handle, SENSOR_LIST_UPDATE_INTERVALS,
+				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &ti);
+	if (ret)
+		return ret;
+
+	buf = ti->rx.buf;
+	do {
+		u32 flags;
+
+		msg = ti->tx.buf;
+		/* Set the number of sensors to be skipped/already read */
+		msg->id = cpu_to_le32(s->id);
+		msg->index = cpu_to_le32(desc_index);
+
+		ret = scmi_do_xfer(handle, ti);
+		if (ret)
+			break;
+
+		flags = le32_to_cpu(buf->num_intervals_flags);
+		num_returned = NUM_INTERVALS_RETURNED(flags);
+		num_remaining = NUM_INTERVALS_REMAINING(flags);
+
+		/*
+		 * Max intervals is not declared previously anywhere so we
+		 * assume it's returned+remaining.
+		 */
+		if (!s->intervals.count) {
+			s->intervals.segmented = SEGMENTED_INTVL_FORMAT(flags);
+			s->intervals.count = num_returned + num_remaining;
+			/* segmented intervals are reported in one triplet */
+			if (s->intervals.segmented &&
+			    (num_remaining || num_returned != 3)) {
+				dev_err(handle->dev,
+					"Sensor ID:%d advertises an invalid segmented interval (%d)\n",
+					s->id, s->intervals.count);
+				s->intervals.segmented = false;
+				s->intervals.count = 0;
+				ret = -EINVAL;
+				break;
+			}
+			/* Direct allocation when exceeding pre-allocated */
+			if (s->intervals.count >= SCMI_MAX_PREALLOC_POOL) {
+				s->intervals.desc =
+					devm_kcalloc(handle->dev,
+						     s->intervals.count,
+						     sizeof(*s->intervals.desc),
+						     GFP_KERNEL);
+				if (!s->intervals.desc) {
+					s->intervals.segmented = false;
+					s->intervals.count = 0;
+					ret = -ENOMEM;
+					break;
+				}
+			}
+		} else if (desc_index + num_returned > s->intervals.count) {
+			dev_err(handle->dev,
+				"No. of update intervals can't exceed %d\n",
+				s->intervals.count);
+			ret = -EINVAL;
+			break;
+		}
+
+		for (cnt = 0; cnt < num_returned; cnt++)
+			s->intervals.desc[desc_index + cnt] =
+					le32_to_cpu(buf->intervals[cnt]);
+
+		desc_index += num_returned;
+
+		scmi_reset_rx_to_maxsz(handle, ti);
+		/*
+		 * check for both returned and remaining to avoid infinite
+		 * loop due to buggy firmware
+		 */
+	} while (num_returned && num_remaining);
+
+	scmi_xfer_put(handle, ti);
+	return ret;
+}
+
+static int scmi_sensor_axis_description(const struct scmi_handle *handle,
+					struct scmi_sensor_info *s)
+{
+	int ret, cnt;
+	u32 desc_index = 0;
+	u16 num_returned, num_remaining;
+	struct scmi_xfer *te;
+	struct scmi_msg_resp_sensor_axis_description *buf;
+	struct scmi_msg_sensor_axis_description_get *msg;
+
+	s->axis = devm_kcalloc(handle->dev, s->num_axis,
+			       sizeof(*s->axis), GFP_KERNEL);
+	if (!s->axis)
+		return -ENOMEM;
+
+	ret = scmi_xfer_get_init(handle, SENSOR_AXIS_DESCRIPTION_GET,
+				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &te);
+	if (ret)
+		return ret;
+
+	buf = te->rx.buf;
+	do {
+		u32 flags;
+		struct scmi_axis_descriptor *adesc;
+
+		msg = te->tx.buf;
+		/* Set the number of sensors to be skipped/already read */
+		msg->id = cpu_to_le32(s->id);
+		msg->axis_desc_index = cpu_to_le32(desc_index);
+
+		ret = scmi_do_xfer(handle, te);
+		if (ret)
+			break;
+
+		flags = le32_to_cpu(buf->num_axis_flags);
+		num_returned = NUM_AXIS_RETURNED(flags);
+		num_remaining = NUM_AXIS_REMAINING(flags);
+
+		if (desc_index + num_returned > s->num_axis) {
+			dev_err(handle->dev, "No. of axis can't exceed %d\n",
+				s->num_axis);
+			break;
+		}
+
+		adesc = &buf->desc[0];
+		for (cnt = 0; cnt < num_returned; cnt++) {
+			u32 attrh, attrl;
+			struct scmi_sensor_axis_info *a;
+			size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
+
+			attrl = le32_to_cpu(adesc->attributes_low);
+
+			a = &s->axis[desc_index + cnt];
+
+			a->id = le32_to_cpu(adesc->id);
+			a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
+
+			attrh = le32_to_cpu(adesc->attributes_high);
+			a->scale = S32_EXT(SENSOR_SCALE(attrh));
+			a->type = SENSOR_TYPE(attrh);
+			strlcpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
+
+			if (a->extended_attrs) {
+				unsigned int ares =
+					le32_to_cpu(adesc->resolution);
+
+				a->resolution = SENSOR_RES(ares);
+				a->exponent =
+					S32_EXT(SENSOR_RES_EXP(ares));
+				dsize += sizeof(adesc->resolution);
+
+				scmi_parse_range_attrs(&a->attrs,
+						       &adesc->attrs);
+				dsize += sizeof(adesc->attrs);
+			}
+
+			adesc = (typeof(adesc))((u8 *)adesc + dsize);
+		}
+
+		desc_index += num_returned;
+
+		scmi_reset_rx_to_maxsz(handle, te);
+		/*
+		 * check for both returned and remaining to avoid infinite
+		 * loop due to buggy firmware
+		 */
+	} while (num_returned && num_remaining);
+
+	scmi_xfer_put(handle, te);
+	return ret;
+}
+
 static int scmi_sensor_description_get(const struct scmi_handle *handle,
 				       struct sensors_info *si)
 {
@@ -131,9 +403,10 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 	buf = t->rx.buf;
 
 	do {
+		struct scmi_sensor_descriptor *sdesc;
+
 		/* Set the number of sensors to be skipped/already read */
 		put_unaligned_le32(desc_index, t->tx.buf);
-
 		ret = scmi_do_xfer(handle, t);
 		if (ret)
 			break;
@@ -147,22 +420,97 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 			break;
 		}
 
+		sdesc = &buf->desc[0];
 		for (cnt = 0; cnt < num_returned; cnt++) {
 			u32 attrh, attrl;
 			struct scmi_sensor_info *s;
+			size_t dsize = SCMI_MSG_RESP_SENS_DESCR_BASE_SZ;
 
-			attrl = le32_to_cpu(buf->desc[cnt].attributes_low);
-			attrh = le32_to_cpu(buf->desc[cnt].attributes_high);
 			s = &si->sensors[desc_index + cnt];
-			s->id = le32_to_cpu(buf->desc[cnt].id);
-			s->type = SENSOR_TYPE(attrh);
-			s->scale = SENSOR_SCALE(attrh);
-			/* Sign extend to a full s8 */
-			if (s->scale & SENSOR_SCALE_SIGN)
-				s->scale |= SENSOR_SCALE_EXTEND;
+			s->id = le32_to_cpu(sdesc->id);
+
+			attrl = le32_to_cpu(sdesc->attributes_low);
+			/* common bitfields parsing */
 			s->async = SUPPORTS_ASYNC_READ(attrl);
 			s->num_trip_points = NUM_TRIP_POINTS(attrl);
-			strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
+			/**
+			 * only v3.0 specific bitfield below.
+			 * Such bitfields are assumed to be zeroed on non
+			 * relevant fw versions...assuming fw not buggy !
+			 */
+			s->update = SUPPORTS_UPDATE_NOTIFY(attrl);
+			s->timestamped = SUPPORTS_TIMESTAMP(attrl);
+			if (s->timestamped)
+				s->tstamp_scale =
+					S32_EXT(SENSOR_TSTAMP_EXP(attrl));
+			s->extended_scalar_attrs =
+				SUPPORTS_EXTEND_ATTRS(attrl);
+
+			attrh = le32_to_cpu(sdesc->attributes_high);
+			/* common bitfields parsing */
+			s->scale = S32_EXT(SENSOR_SCALE(attrh));
+			s->type = SENSOR_TYPE(attrh);
+			/* Use pre-allocated pool wherever possible */
+			s->intervals.desc = s->intervals.prealloc_pool;
+			if (si->version == 0x10000) {
+				s->intervals.segmented = false;
+				s->intervals.count = 1;
+				/*
+				 * Convert SCMIv2.0 update interval format to
+				 * SCMIv3.0 to be used as the common exposed
+				 * descriptor, accessible via common macros.
+				 */
+				s->intervals.desc[0] =
+					(SENSOR_UPDATE_BASE(attrh) << 5) |
+					 SENSOR_UPDATE_SCALE(attrh);
+			} else {
+				/*
+				 * From version v3.0 update intervals are
+				 * retrieved via a dedicated (optional) command.
+				 * Since the command is optional, on error carry
+				 * on without any update interval.
+				 */
+				if (scmi_sensor_update_intervals(handle, s))
+					dev_info(handle->dev,
+						 "Update Intervals not available for sensor ID:%d\n",
+						 s->id);
+			}
+			/**
+			 * only > v2.0 specific bitfield below.
+			 * Such bitfields are assumed to be zeroed on non
+			 * relevant fw versions...assuming fw not buggy !
+			 */
+			s->num_axis = min_t(unsigned int,
+					    SUPPORTS_AXIS(attrh) ?
+					    SENSOR_AXIS_NUMBER(attrh) : 0,
+					    SCMI_MAX_NUM_SENSOR_AXIS);
+			strlcpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
+
+			if (s->extended_scalar_attrs) {
+				s->sensor_power = le32_to_cpu(sdesc->power);
+				dsize += sizeof(sdesc->power);
+				/* Only for sensors reporting scalar values */
+				if (s->num_axis == 0) {
+					unsigned int sres =
+						le32_to_cpu(sdesc->resolution);
+
+					s->resolution = SENSOR_RES(sres);
+					s->exponent =
+						S32_EXT(SENSOR_RES_EXP(sres));
+					dsize += sizeof(sdesc->resolution);
+
+					scmi_parse_range_attrs(&s->scalar_attrs,
+							       &sdesc->scalar_attrs);
+					dsize += sizeof(sdesc->scalar_attrs);
+				}
+			}
+			if (s->num_axis > 0) {
+				ret = scmi_sensor_axis_description(handle, s);
+				if (ret)
+					goto out;
+			}
+
+			sdesc = (typeof(sdesc))((u8 *)sdesc + dsize);
 		}
 
 		desc_index += num_returned;
@@ -174,6 +522,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 		 */
 	} while (num_returned && num_remaining);
 
+out:
 	scmi_xfer_put(handle, t);
 	return ret;
 }
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 9cd312a1ff92..53cc5ce0e494 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -8,6 +8,7 @@
 #ifndef _LINUX_SCMI_PROTOCOL_H
 #define _LINUX_SCMI_PROTOCOL_H
 
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/types.h>
@@ -148,26 +149,238 @@ struct scmi_power_ops {
 			 u32 *state);
 };
 
+/**
+ * scmi_range_attrs  - specifies a sensor or axis values' range
+ * @min_range: The minimum value which can be represented by the sensor/axis.
+ * @max_range: The maximum value which can be represented by the sensor/axis.
+ */
+struct scmi_range_attrs {
+	long long min_range;
+	long long max_range;
+};
+
+/**
+ * scmi_sensor_axis_info  - describes one sensor axes
+ * @id: The axes ID.
+ * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
+ * @scale: Power-of-10 multiplier applied to the axis unit.
+ * @name: NULL-terminated string representing axes name as advertised by
+ *	  SCMI platform.
+ * @extended_attrs: Flag to indicate the presence of additional extended
+ *		    attributes for this axes.
+ * @resolution: Extended attribute representing the resolution of the axes.
+ *		Set to 0 if not reported by this axes.
+ * @exponent: Extended attribute representing the power-of-10 multiplier that
+ *	      is applied to the resolution field. Set to 0 if not reported by
+ *	      this axes.
+ * @attrs: Extended attributes representing minimum and maximum values
+ *	   measurable by this axes. Set to 0 if not reported by this sensor.
+ */
+struct scmi_sensor_axis_info {
+	unsigned int id;
+	unsigned int type;
+	int scale;
+	char name[SCMI_MAX_STR_SIZE];
+	bool extended_attrs;
+	unsigned int resolution;
+	int exponent;
+	struct scmi_range_attrs attrs;
+};
+
+/**
+ * scmi_sensor_intervals_info  - describes number and type of available update
+ * intervals
+ * @segmented: Flag for segmented intervals' representation. When True there
+ *	       will be exactly 3 intervals in @desc, with each entry
+ *	       representing a member of a segment in this order:
+ *	       {lowest update interval, highest update interval, step size}
+ * @count: Number of intervals described in @desc.
+ * @desc: Array of @count interval descriptor bitmask represented as detailed in
+ *	  the SCMI specification: it can be accessed using the accompanying
+ *	  macros.
+ * @prealloc_pool: A minimal preallocated pool of desc entries used to avoid
+ *		   lesser-than-64-bytes dynamic allocation for small @count
+ *		   values.
+ */
+struct scmi_sensor_intervals_info {
+	bool segmented;
+	unsigned int count;
+#define SCMI_SENS_INTVL_SEGMENT_LOW	0
+#define SCMI_SENS_INTVL_SEGMENT_HIGH	1
+#define SCMI_SENS_INTVL_SEGMENT_STEP	2
+	unsigned int *desc;
+#define SCMI_SENS_INTVL_GET_SECS(x)		FIELD_GET(GENMASK(20, 5), (x))
+#define SCMI_SENS_INTVL_GET_EXP(x)					\
+	({								\
+		int __signed_exp = FIELD_GET(GENMASK(4, 0), (x));	\
+									\
+		if (__signed_exp & BIT(4))				\
+			__signed_exp |= GENMASK(31, 5);			\
+		__signed_exp;						\
+	})
+#define SCMI_MAX_PREALLOC_POOL			16
+	unsigned int prealloc_pool[SCMI_MAX_PREALLOC_POOL];
+};
+
+/**
+ * struct scmi_sensor_info - represents information related to one of the
+ * available sensors.
+ * @id: Sensor ID.
+ * @type: Sensor type. Chosen amongst one of @enum scmi_sensor_class.
+ * @scale: Power-of-10 multiplier applied to the sensor unit.
+ * @num_trip_points: Number of maximum configurable trip points.
+ * @async: Flag for asynchronous read support.
+ * @update: Flag for continuouos update notification support.
+ * @timestamped: Flag for timestamped read support.
+ * @tstamp_scale: Power-of-10 multiplier applied to the sensor timestamps to
+ *		  represent it in seconds.
+ * @num_axis: Number of supported axis if any. Reported as 0 for scalar sensors.
+ * @axis: Pointer to an array of @num_axis descriptors.
+ * @intervals: Descriptor of available update intervals.
+ * @sensor_config: A bitmask reporting the current sensor configuration as
+ *		   detailed in the SCMI specification: it can accessed and
+ *		   modified through the accompanying macros.
+ * @name: NULL-terminated string representing sensor name as advertised by
+ *	  SCMI platform.
+ * @extended_scalar_attrs: Flag to indicate the presence of additional extended
+ *			   attributes for this sensor.
+ * @sensor_power: Extended attribute representing the average power
+ *		  consumed by the sensor in microwatts (uW) when it is active.
+ *		  Reported here only for scalar sensors.
+ *		  Set to 0 if not reported by this sensor.
+ * @resolution: Extended attribute representing the resolution of the sensor.
+ *		Reported here only for scalar sensors.
+ *		Set to 0 if not reported by this sensor.
+ * @exponent: Extended attribute representing the power-of-10 multiplier that is
+ *	      applied to the resolution field.
+ *	      Reported here only for scalar sensors.
+ *	      Set to 0 if not reported by this sensor.
+ * @scalar_attrs: Extended attributes representing minimum and maximum
+ *		  measurable values by this sensor.
+ *		  Reported here only for scalar sensors.
+ *		  Set to 0 if not reported by this sensor.
+ */
 struct scmi_sensor_info {
-	u32 id;
-	u8 type;
-	s8 scale;
-	u8 num_trip_points;
+	unsigned int id;
+	unsigned int type;
+	int scale;
+	unsigned int num_trip_points;
 	bool async;
+	bool update;
+	bool timestamped;
+	int tstamp_scale;
+	unsigned int num_axis;
+	struct scmi_sensor_axis_info *axis;
+	struct scmi_sensor_intervals_info intervals;
 	char name[SCMI_MAX_STR_SIZE];
+	bool extended_scalar_attrs;
+	unsigned int sensor_power;
+	unsigned int resolution;
+	int exponent;
+	struct scmi_range_attrs scalar_attrs;
 };
 
 /*
  * Partial list from Distributed Management Task Force (DMTF) specification:
- * DSP0249 (Platform Level Data Model specification)
+ * DSP0248 (Platform Level Data Model for Platform Monitoring and Control
+ * specification)
  */
 enum scmi_sensor_class {
 	NONE = 0x0,
+	UNSPEC = 0x1,
 	TEMPERATURE_C = 0x2,
+	TEMPERATURE_F = 0x3,
+	TEMPERATURE_K = 0x4,
 	VOLTAGE = 0x5,
 	CURRENT = 0x6,
 	POWER = 0x7,
 	ENERGY = 0x8,
+	CHARGE = 0x9,
+	VOLTAMPERE = 0xA,
+	NITS = 0xB,
+	LUMENS = 0xC,
+	LUX = 0xD,
+	CANDELAS = 0xE,
+	KPA = 0xF,
+	PSI = 0x10,
+	NEWTON = 0x11,
+	CFM = 0x12,
+	RPM = 0x13,
+	HERTZ = 0x14,
+	SECS = 0x15,
+	MINS = 0x16,
+	HOURS = 0x17,
+	DAYS = 0x18,
+	WEEKS = 0x19,
+	MILS = 0x1A,
+	INCHES = 0x1B,
+	FEET = 0x1C,
+	CUBIC_INCHES = 0x1D,
+	CUBIC_FEET = 0x1E,
+	METERS = 0x1F,
+	CUBIC_CM = 0x20,
+	CUBIC_METERS = 0x21,
+	LITERS = 0x22,
+	FLUID_OUNCES = 0x23,
+	RADIANS = 0x24,
+	STERADIANS = 0x25,
+	REVOLUTIONS = 0x26,
+	CYCLES = 0x27,
+	GRAVITIES = 0x28,
+	OUNCES = 0x29,
+	POUNDS = 0x2A,
+	FOOT_POUNDS = 0x2B,
+	OUNCE_INCHES = 0x2C,
+	GAUSS = 0x2D,
+	GILBERTS = 0x2E,
+	HENRIES = 0x2F,
+	FARADS = 0x30,
+	OHMS = 0x31,
+	SIEMENS = 0x32,
+	MOLES = 0x33,
+	BECQUERELS = 0x34,
+	PPM = 0x35,
+	DECIBELS = 0x36,
+	DBA = 0x37,
+	DBC = 0x38,
+	GRAYS = 0x39,
+	SIEVERTS = 0x3A,
+	COLOR_TEMP_K = 0x3B,
+	BITS = 0x3C,
+	BYTES = 0x3D,
+	WORDS = 0x3E,
+	DWORDS = 0x3F,
+	QWORDS = 0x40,
+	PERCENTAGE = 0x41,
+	PASCALS = 0x42,
+	COUNTS = 0x43,
+	GRAMS = 0x44,
+	NEWTON_METERS = 0x45,
+	HITS = 0x46,
+	MISSES = 0x47,
+	RETRIES = 0x48,
+	OVERRUNS = 0x49,
+	UNDERRUNS = 0x4A,
+	COLLISIONS = 0x4B,
+	PACKETS = 0x4C,
+	MESSAGES = 0x4D,
+	CHARS = 0x4E,
+	ERRORS = 0x4F,
+	CORRECTED_ERRS = 0x50,
+	UNCORRECTABLE_ERRS = 0x51,
+	SQ_MILS = 0x52,
+	SQ_INCHES = 0x53,
+	SQ_FEET = 0x54,
+	SQ_CM = 0x55,
+	SQ_METERS = 0x56,
+	RADIANS_SEC = 0x57,
+	BPM = 0x58,
+	METERS_SEC_SQUARED = 0x59,
+	METERS_SEC = 0x5A,
+	CUBIC_METERS_SEC = 0x5B,
+	MM_MERCURY = 0x5C,
+	RADIANS_SEC_SQUARED = 0x5D,
+	OEM_UNIT = 0xFF
 };
 
 /**
-- 
2.17.1

[PATCH v3 3/6] hwmon: scmi: update hwmon internal scale data type

[Cristian Marussi]

Use an int to calculate scale values inside scmi_hwmon_scale() to match
the updated scale data type in struct scmi_sensor_info.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/hwmon/scmi-hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
index 09ce30cba54b..17d064e58938 100644
--- a/drivers/hwmon/scmi-hwmon.c
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -30,7 +30,7 @@ static inline u64 __pow10(u8 x)
 
 static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value)
 {
-	s8 scale = sensor->scale;
+	int scale = sensor->scale;
 	u64 f;
 
 	switch (sensor->type) {
-- 
2.17.1

[PATCH v3 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads

[Cristian Marussi]

Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0
timestamped reads.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
V2 --> v3
- setting rx_size to 0 in sensor_reading_get to allow fw to send
  both v2 and v3 replies...even if sensor_reading_get() only handles
  v2 spec and returns one single value
- using get_unaligned_le64 in lieu of le64_to_cpu
- removed refs to v2.1
---
 drivers/firmware/arm_scmi/sensors.c | 137 ++++++++++++++++++++++++++--
 include/linux/scmi_protocol.h       |  22 +++++
 2 files changed, 152 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 1c83aaae0012..0adc545116a4 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get {
 #define SENSOR_READ_ASYNC	BIT(0)
 };
 
+struct scmi_resp_sensor_reading_get {
+	__le64 readings;
+};
+
+struct scmi_resp_sensor_reading_complete {
+	__le32 id;
+	__le64 readings;
+};
+
+struct scmi_sensor_reading_le {
+	__le32 sensor_value_low;
+	__le32 sensor_value_high;
+	__le32 timestamp_low;
+	__le32 timestamp_high;
+};
+
+struct scmi_resp_sensor_reading_complete_v3 {
+	__le32 id;
+	struct scmi_sensor_reading_le readings[];
+};
+
 struct scmi_sensor_trip_notify_payld {
 	__le32 agent_id;
 	__le32 sensor_id;
@@ -576,6 +597,21 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id,
 	return ret;
 }
 
+/**
+ * scmi_sensor_reading_get  - Read scalar sensor value
+ * @handle: Platform handle
+ * @sensor_id: Sensor ID
+ * @value: The 64bit value sensor reading
+ *
+ * This function returns a single 64 bit reading value representing the sensor
+ * value; if the platform SCMI Protocol implementation and the sensor support
+ * multiple axis and timestamped-reads, this just returns the first axis while
+ * dropping the timestamp value.
+ * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of
+ * timestamped multi-axis values.
+ *
+ * Return: 0 on Success
+ */
 static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 				   u32 sensor_id, u64 *value)
 {
@@ -586,25 +622,111 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 	struct scmi_sensor_info *s = si->sensors + sensor_id;
 
 	ret = scmi_xfer_get_init(handle, SENSOR_READING_GET,
-				 SCMI_PROTOCOL_SENSOR, sizeof(*sensor),
-				 sizeof(u64), &t);
+				 SCMI_PROTOCOL_SENSOR, sizeof(*sensor), 0, &t);
 	if (ret)
 		return ret;
 
 	sensor = t->tx.buf;
 	sensor->id = cpu_to_le32(sensor_id);
+	if (s->async) {
+		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
+		ret = scmi_do_xfer_with_response(handle, t);
+		if (!ret) {
+			struct scmi_resp_sensor_reading_complete *resp;
+
+			resp = t->rx.buf;
+			if (le32_to_cpu(resp->id) == sensor_id)
+				*value = get_unaligned_le64(&resp->readings);
+			else
+				ret = -EPROTO;
+		}
+	} else {
+		sensor->flags = cpu_to_le32(0);
+		ret = scmi_do_xfer(handle, t);
+		if (!ret) {
+			struct scmi_resp_sensor_reading_get *resp;
+
+			resp = t->rx.buf;
+			*value = get_unaligned_le64(&resp->readings);
+		}
+	}
 
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static inline void
+scmi_parse_sensor_readings(struct scmi_sensor_reading *out,
+			   const struct scmi_sensor_reading_le *in)
+{
+	out->value = get_unaligned_le64((void *)&in->sensor_value_low);
+	out->timestamp = get_unaligned_le64((void *)&in->timestamp_low);
+}
+
+/**
+ * scmi_sensor_reading_get_timestamped  - Read multiple-axis timestamped values
+ * @handle: Platform handle
+ * @sensor_id: Sensor ID
+ * @count: The length of the provided @readings array
+ * @readings: An array of elements each representing a timestamped per-axis
+ *	      reading of type @struct scmi_sensor_reading.
+ *	      Returned readings are ordered as the @axis descriptors array
+ *	      included in @struct scmi_sensor_info and the max number of
+ *	      returned elements is min(@count, @num_axis); ideally the provided
+ *	      array should be of length @count equal to @num_axis.
+ *
+ * Return: 0 on Success
+ */
+static int
+scmi_sensor_reading_get_timestamped(const struct scmi_handle *handle,
+				    u32 sensor_id, u8 count,
+				    struct scmi_sensor_reading *readings)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_sensor_reading_get *sensor;
+	struct sensors_info *si = handle->sensor_priv;
+	struct scmi_sensor_info *s = si->sensors + sensor_id;
+
+	if (!count || !readings ||
+	    (!s->num_axis && count > 1) || (s->num_axis && count > s->num_axis))
+		return -EINVAL;
+
+	ret = scmi_xfer_get_init(handle, SENSOR_READING_GET,
+				 SCMI_PROTOCOL_SENSOR, sizeof(*sensor), 0, &t);
+	if (ret)
+		return ret;
+
+	sensor = t->tx.buf;
+	sensor->id = cpu_to_le32(sensor_id);
 	if (s->async) {
 		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
 		ret = scmi_do_xfer_with_response(handle, t);
-		if (!ret)
-			*value = get_unaligned_le64((void *)
-						    ((__le32 *)t->rx.buf + 1));
+		if (!ret) {
+			int i;
+			struct scmi_resp_sensor_reading_complete_v3 *resp;
+
+			resp = t->rx.buf;
+			/* Retrieve only the number of requested axis anyway */
+			if (le32_to_cpu(resp->id) == sensor_id)
+				for (i = 0; i < count; i++)
+					scmi_parse_sensor_readings(&readings[i],
+								   &resp->readings[i]);
+			else
+				ret = -EPROTO;
+		}
 	} else {
 		sensor->flags = cpu_to_le32(0);
 		ret = scmi_do_xfer(handle, t);
-		if (!ret)
-			*value = get_unaligned_le64(t->rx.buf);
+		if (!ret) {
+			int i;
+			struct scmi_sensor_reading_le *resp_readings;
+
+			resp_readings = t->rx.buf;
+			for (i = 0; i < count; i++)
+				scmi_parse_sensor_readings(&readings[i],
+							   &resp_readings[i]);
+		}
 	}
 
 	scmi_xfer_put(handle, t);
@@ -631,6 +753,7 @@ static const struct scmi_sensor_ops sensor_ops = {
 	.info_get = scmi_sensor_info_get,
 	.trip_point_config = scmi_sensor_trip_point_config,
 	.reading_get = scmi_sensor_reading_get,
+	.reading_get_timestamped = scmi_sensor_reading_get_timestamped,
 };
 
 static int scmi_sensor_set_notify_enabled(const struct scmi_handle *handle,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 53cc5ce0e494..072d8f9fc42a 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -149,6 +149,20 @@ struct scmi_power_ops {
 			 u32 *state);
 };
 
+/**
+ * scmi_sensor_reading  - represent a timestamped read
+ *
+ * Used by @reading_get_timestamped method.
+ *
+ * @value: The signed value sensor read.
+ * @timestamp: An unsigned timestamp for the sensor read, as provided by
+ *	       SCMI platform. Set to zero when not available.
+ */
+struct scmi_sensor_reading {
+	long long value;
+	unsigned long long timestamp;
+};
+
 /**
  * scmi_range_attrs  - specifies a sensor or axis values' range
  * @min_range: The minimum value which can be represented by the sensor/axis.
@@ -391,6 +405,11 @@ enum scmi_sensor_class {
  * @info_get: get the information of the specified sensor
  * @trip_point_config: selects and configures a trip-point of interest
  * @reading_get: gets the current value of the sensor
+ * @reading_get_timestamped: gets the current value and timestamp, when
+ *			     available, of the sensor. (as of v3.0 spec)
+ *			     Supports multi-axis sensors for sensors which
+ *			     supports it and if the @reading array size of
+ *			     @count entry equals the sensor num_axis
  */
 struct scmi_sensor_ops {
 	int (*count_get)(const struct scmi_handle *handle);
@@ -400,6 +419,9 @@ struct scmi_sensor_ops {
 				 u32 sensor_id, u8 trip_id, u64 trip_value);
 	int (*reading_get)(const struct scmi_handle *handle, u32 sensor_id,
 			   u64 *value);
+	int (*reading_get_timestamped)(const struct scmi_handle *handle,
+				       u32 sensor_id, u8 count,
+				       struct scmi_sensor_reading *readings);
 };
 
 /**
-- 
2.17.1

[PATCH v3 5/6] firmware: arm_scmi: add SCMIv3.0 Sensor configuration support

[Cristian Marussi]

Add SCMIv3.0 Sensor support for CONFIG_GET/CONFIG_SET commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/sensors.c | 75 +++++++++++++++++++++++++++++
 include/linux/scmi_protocol.h       | 37 ++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 0adc545116a4..fa3385045361 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -22,6 +22,8 @@ enum scmi_sensor_protocol_cmd {
 	SENSOR_READING_GET = 0x6,
 	SENSOR_AXIS_DESCRIPTION_GET = 0x7,
 	SENSOR_LIST_UPDATE_INTERVALS = 0x8,
+	SENSOR_CONFIG_GET = 0x9,
+	SENSOR_CONFIG_SET = 0xA,
 };
 
 struct scmi_msg_resp_sensor_attributes {
@@ -150,6 +152,19 @@ struct scmi_msg_set_sensor_trip_point {
 	__le32 value_high;
 };
 
+struct scmi_msg_sensor_config_get {
+	__le32 id;
+};
+
+struct scmi_resp_sensor_config_get {
+	__le32 sensor_config;
+};
+
+struct scmi_msg_sensor_config_set {
+	__le32 id;
+	__le32 sensor_config;
+};
+
 struct scmi_msg_sensor_reading_get {
 	__le32 id;
 	__le32 flags;
@@ -597,6 +612,64 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id,
 	return ret;
 }
 
+static int scmi_sensor_config_get(const struct scmi_handle *handle,
+				  u32 sensor_id, u32 *sensor_config)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_sensor_config_get *msg;
+	struct scmi_resp_sensor_config_get *resp;
+
+	ret = scmi_xfer_get_init(handle, SENSOR_CONFIG_GET,
+				 SCMI_PROTOCOL_SENSOR, sizeof(*msg),
+				 sizeof(*resp), &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->id = cpu_to_le32(sensor_id);
+	ret = scmi_do_xfer(handle, t);
+	if (!ret) {
+		struct sensors_info *si = handle->sensor_priv;
+		struct scmi_sensor_info *s = si->sensors + sensor_id;
+
+		resp = t->rx.buf;
+		*sensor_config = le32_to_cpu(resp->sensor_config);
+		s->sensor_config = *sensor_config;
+	}
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int scmi_sensor_config_set(const struct scmi_handle *handle,
+				  u32 sensor_id, u32 sensor_config)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_sensor_config_set *msg;
+
+	ret = scmi_xfer_get_init(handle, SENSOR_CONFIG_SET,
+				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->id = cpu_to_le32(sensor_id);
+	msg->sensor_config = cpu_to_le32(sensor_config);
+
+	ret = scmi_do_xfer(handle, t);
+	if (!ret) {
+		struct sensors_info *si = handle->sensor_priv;
+		struct scmi_sensor_info *s = si->sensors + sensor_id;
+
+		s->sensor_config = sensor_config;
+	}
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
 /**
  * scmi_sensor_reading_get  - Read scalar sensor value
  * @handle: Platform handle
@@ -754,6 +827,8 @@ static const struct scmi_sensor_ops sensor_ops = {
 	.trip_point_config = scmi_sensor_trip_point_config,
 	.reading_get = scmi_sensor_reading_get,
 	.reading_get_timestamped = scmi_sensor_reading_get_timestamped,
+	.config_get = scmi_sensor_config_get,
+	.config_set = scmi_sensor_config_set,
 };
 
 static int scmi_sensor_set_notify_enabled(const struct scmi_handle *handle,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 072d8f9fc42a..32d18457b6c6 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -286,7 +286,38 @@ struct scmi_sensor_info {
 	unsigned int num_axis;
 	struct scmi_sensor_axis_info *axis;
 	struct scmi_sensor_intervals_info intervals;
+	unsigned int sensor_config;
+#define SCMI_SENS_CFG_UPDATE_SECS_MASK		GENMASK(31, 16)
+#define SCMI_SENS_CFG_GET_UPDATE_SECS(x)				\
+	FIELD_GET(SCMI_SENS_CFG_UPDATE_SECS_MASK, (x))
+
+#define SCMI_SENS_CFG_UPDATE_EXP_MASK		GENMASK(15, 11)
+#define SCMI_SENS_CFG_GET_UPDATE_EXP(x)					\
+	({								\
+		int __signed_exp =					\
+			FIELD_GET(SCMI_SENS_CFG_UPDATE_EXP_MASK, (x));	\
+									\
+		if (__signed_exp & BIT(4))				\
+			__signed_exp |= GENMASK(31, 5);			\
+		__signed_exp;						\
+	})
+
+#define SCMI_SENS_CFG_ROUND_MASK		GENMASK(10, 9)
+#define SCMI_SENS_CFG_ROUND_AUTO		2
+#define SCMI_SENS_CFG_ROUND_UP			1
+#define SCMI_SENS_CFG_ROUND_DOWN		0
+
+#define SCMI_SENS_CFG_TSTAMP_ENABLED_MASK	BIT(1)
+#define SCMI_SENS_CFG_TSTAMP_ENABLE		1
+#define SCMI_SENS_CFG_TSTAMP_DISABLE		0
+#define SCMI_SENS_CFG_IS_TSTAMP_ENABLED(x)				\
+	FIELD_GET(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, (x))
+
+#define SCMI_SENS_CFG_SENSOR_ENABLED_MASK	BIT(0)
+#define SCMI_SENS_CFG_SENSOR_ENABLE		1
+#define SCMI_SENS_CFG_SENSOR_DISABLE		0
 	char name[SCMI_MAX_STR_SIZE];
+#define SCMI_SENS_CFG_IS_ENABLED(x)		FIELD_GET(BIT(0), (x))
 	bool extended_scalar_attrs;
 	unsigned int sensor_power;
 	unsigned int resolution;
@@ -410,6 +441,8 @@ enum scmi_sensor_class {
  *			     Supports multi-axis sensors for sensors which
  *			     supports it and if the @reading array size of
  *			     @count entry equals the sensor num_axis
+ * @config_get: Get sensor current configuration
+ * @config_set: Set sensor current configuration
  */
 struct scmi_sensor_ops {
 	int (*count_get)(const struct scmi_handle *handle);
@@ -422,6 +455,10 @@ struct scmi_sensor_ops {
 	int (*reading_get_timestamped)(const struct scmi_handle *handle,
 				       u32 sensor_id, u8 count,
 				       struct scmi_sensor_reading *readings);
+	int (*config_get)(const struct scmi_handle *handle,
+			  u32 sensor_id, u32 *sensor_config);
+	int (*config_set)(const struct scmi_handle *handle,
+			  u32 sensor_id, u32 sensor_config);
 };
 
 /**
-- 
2.17.1

[PATCH v3 6/6] firmware: arm_scmi: add SCMIv3.0 Sensor notifications

[Cristian Marussi]

Add support for new SCMIv3.0 SENSOR_UPDATE notification.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v2 --> v3
- removed stale unused msg payload definition
- moved variable declaration inside switch block
---
 drivers/firmware/arm_scmi/sensors.c | 124 +++++++++++++++++++++++-----
 include/linux/scmi_protocol.h       |   9 ++
 2 files changed, 114 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index fa3385045361..6d6b48032c8c 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -24,6 +24,7 @@ enum scmi_sensor_protocol_cmd {
 	SENSOR_LIST_UPDATE_INTERVALS = 0x8,
 	SENSOR_CONFIG_GET = 0x9,
 	SENSOR_CONFIG_SET = 0xA,
+	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
 };
 
 struct scmi_msg_resp_sensor_attributes {
@@ -133,10 +134,10 @@ struct scmi_msg_resp_sensor_list_update_intervals {
 	__le32 intervals[];
 };
 
-struct scmi_msg_sensor_trip_point_notify {
+struct scmi_msg_sensor_request_notify {
 	__le32 id;
 	__le32 event_control;
-#define SENSOR_TP_NOTIFY_ALL	BIT(0)
+#define SENSOR_NOTIFY_ALL	BIT(0)
 };
 
 struct scmi_msg_set_sensor_trip_point {
@@ -198,6 +199,12 @@ struct scmi_sensor_trip_notify_payld {
 	__le32 trip_point_desc;
 };
 
+struct scmi_sensor_update_notify_payld {
+	__le32 agent_id;
+	__le32 sensor_id;
+	struct scmi_sensor_reading_le readings[];
+};
+
 struct sensors_info {
 	u32 version;
 	int num_sensors;
@@ -563,15 +570,16 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 	return ret;
 }
 
-static int scmi_sensor_trip_point_notify(const struct scmi_handle *handle,
-					 u32 sensor_id, bool enable)
+static inline int
+scmi_sensor_request_notify(const struct scmi_handle *handle, u32 sensor_id,
+			   u8 message_id, bool enable)
 {
 	int ret;
-	u32 evt_cntl = enable ? SENSOR_TP_NOTIFY_ALL : 0;
+	u32 evt_cntl = enable ? SENSOR_NOTIFY_ALL : 0;
 	struct scmi_xfer *t;
-	struct scmi_msg_sensor_trip_point_notify *cfg;
+	struct scmi_msg_sensor_request_notify *cfg;
 
-	ret = scmi_xfer_get_init(handle, SENSOR_TRIP_POINT_NOTIFY,
+	ret = scmi_xfer_get_init(handle, message_id,
 				 SCMI_PROTOCOL_SENSOR, sizeof(*cfg), 0, &t);
 	if (ret)
 		return ret;
@@ -586,6 +594,23 @@ static int scmi_sensor_trip_point_notify(const struct scmi_handle *handle,
 	return ret;
 }
 
+static int scmi_sensor_trip_point_notify(const struct scmi_handle *handle,
+					 u32 sensor_id, bool enable)
+{
+	return scmi_sensor_request_notify(handle, sensor_id,
+					  SENSOR_TRIP_POINT_NOTIFY,
+					  enable);
+}
+
+static int
+scmi_sensor_continuous_update_notify(const struct scmi_handle *handle,
+				     u32 sensor_id, bool enable)
+{
+	return scmi_sensor_request_notify(handle, sensor_id,
+					  SENSOR_CONTINUOUS_UPDATE_NOTIFY,
+					  enable);
+}
+
 static int
 scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id,
 			      u8 trip_id, u64 trip_value)
@@ -836,7 +861,19 @@ static int scmi_sensor_set_notify_enabled(const struct scmi_handle *handle,
 {
 	int ret;
 
-	ret = scmi_sensor_trip_point_notify(handle, src_id, enable);
+	switch (evt_id) {
+	case SCMI_EVENT_SENSOR_TRIP_POINT_EVENT:
+		ret = scmi_sensor_trip_point_notify(handle, src_id, enable);
+		break;
+	case SCMI_EVENT_SENSOR_UPDATE:
+		ret = scmi_sensor_continuous_update_notify(handle, src_id,
+							   enable);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
 	if (ret)
 		pr_debug("FAIL_ENABLED - evt[%X] dom[%d] - ret:%d\n",
 			 evt_id, src_id, ret);
@@ -849,20 +886,59 @@ static void *scmi_sensor_fill_custom_report(const struct scmi_handle *handle,
 					    const void *payld, size_t payld_sz,
 					    void *report, u32 *src_id)
 {
-	const struct scmi_sensor_trip_notify_payld *p = payld;
-	struct scmi_sensor_trip_point_report *r = report;
+	void *rep = NULL;
 
-	if (evt_id != SCMI_EVENT_SENSOR_TRIP_POINT_EVENT ||
-	    sizeof(*p) != payld_sz)
-		return NULL;
+	switch (evt_id) {
+	case SCMI_EVENT_SENSOR_TRIP_POINT_EVENT:
+	{
+		const struct scmi_sensor_trip_notify_payld *p = payld;
+		struct scmi_sensor_trip_point_report *r = report;
 
-	r->timestamp = timestamp;
-	r->agent_id = le32_to_cpu(p->agent_id);
-	r->sensor_id = le32_to_cpu(p->sensor_id);
-	r->trip_point_desc = le32_to_cpu(p->trip_point_desc);
-	*src_id = r->sensor_id;
+		if (sizeof(*p) != payld_sz)
+			break;
 
-	return r;
+		r->timestamp = timestamp;
+		r->agent_id = le32_to_cpu(p->agent_id);
+		r->sensor_id = le32_to_cpu(p->sensor_id);
+		r->trip_point_desc = le32_to_cpu(p->trip_point_desc);
+		*src_id = r->sensor_id;
+		rep = r;
+		break;
+	}
+	case SCMI_EVENT_SENSOR_UPDATE:
+	{
+		int i;
+		struct scmi_sensor_info *s;
+		const struct scmi_sensor_update_notify_payld *p = payld;
+		struct scmi_sensor_update_report *r = report;
+		struct sensors_info *sinfo = handle->sensor_priv;
+
+		/* payld_sz is variable for this event */
+		r->sensor_id = le32_to_cpu(p->sensor_id);
+		if (r->sensor_id >= sinfo->num_sensors)
+			break;
+		r->timestamp = timestamp;
+		r->agent_id = le32_to_cpu(p->agent_id);
+		s = &sinfo->sensors[r->sensor_id];
+		/*
+		 * The generated report r (@struct scmi_sensor_update_report)
+		 * was pre-allocated to contain up to SCMI_MAX_NUM_SENSOR_AXIS
+		 * readings: here it is filled with the effective @num_axis
+		 * readings defined for this sensor or 1 for scalar sensors.
+		 */
+		r->readings_count = s->num_axis ?: 1;
+		for (i = 0; i < r->readings_count; i++)
+			scmi_parse_sensor_readings(&r->readings[i],
+						   &p->readings[i]);
+		*src_id = r->sensor_id;
+		rep = r;
+		break;
+	}
+	default:
+		break;
+	}
+
+	return rep;
 }
 
 static const struct scmi_event sensor_events[] = {
@@ -871,6 +947,16 @@ static const struct scmi_event sensor_events[] = {
 		.max_payld_sz = sizeof(struct scmi_sensor_trip_notify_payld),
 		.max_report_sz = sizeof(struct scmi_sensor_trip_point_report),
 	},
+	{
+		.id = SCMI_EVENT_SENSOR_UPDATE,
+		.max_payld_sz =
+			sizeof(struct scmi_sensor_update_notify_payld) +
+			 SCMI_MAX_NUM_SENSOR_AXIS *
+			 sizeof(struct scmi_sensor_reading_le),
+		.max_report_sz = sizeof(struct scmi_sensor_update_report) +
+				  SCMI_MAX_NUM_SENSOR_AXIS *
+				  sizeof(struct scmi_sensor_reading),
+	},
 };
 
 static const struct scmi_event_ops sensor_event_ops = {
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 32d18457b6c6..dcdd1d10bbcc 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -658,6 +658,7 @@ enum scmi_notification_events {
 	SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED = 0x0,
 	SCMI_EVENT_PERFORMANCE_LEVEL_CHANGED = 0x1,
 	SCMI_EVENT_SENSOR_TRIP_POINT_EVENT = 0x0,
+	SCMI_EVENT_SENSOR_UPDATE = 0x1,
 	SCMI_EVENT_RESET_ISSUED = 0x0,
 	SCMI_EVENT_BASE_ERROR_EVENT = 0x0,
 	SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER = 0x0,
@@ -699,6 +700,14 @@ struct scmi_sensor_trip_point_report {
 	unsigned int	trip_point_desc;
 };
 
+struct scmi_sensor_update_report {
+	ktime_t				timestamp;
+	unsigned int			agent_id;
+	unsigned int			sensor_id;
+	unsigned int			readings_count;
+	struct scmi_sensor_reading	readings[];
+};
+
 struct scmi_reset_issued_report {
 	ktime_t		timestamp;
 	unsigned int	agent_id;
-- 
2.17.1

Re: [PATCH v3 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions

[Sudeep Holla]

On Wed, Nov 18, 2020 at 04:29:01PM +0000, Cristian Marussi wrote:
> Add support for new SCMIv3.0 Sensors extensions related to new sensors'
> features, like multiple axis and update intervals, while keeping
> compatibility with SCMIv2.0 features.
> While at that, refactor and simplify all the internal helpers macros and
> move struct scmi_sensor_info to use only non-fixed-size typing.
>

Sorry for late review.

> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v2 --> v3
> - Fix SCMI_MAX_NUM_SENSOR_AXIS
> - added missing Dox comment in resolution
> - added common INTVL SEGMENT macros
>
> v1 --> v2
> - restrict segmented intervals descriptors to single triplet
> - add proper usage of scmi_reset_rx_to_maxsz
> ---
>  drivers/firmware/arm_scmi/sensors.c | 391 ++++++++++++++++++++++++++--
>  include/linux/scmi_protocol.h       | 223 +++++++++++++++-
>  2 files changed, 588 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 6aaff478d032..1c83aaae0012 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -7,16 +7,21 @@
>
>  #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
>
> +#include <linux/bitfield.h>
>  #include <linux/scmi_protocol.h>
>
>  #include "common.h"
>  #include "notify.h"
>
> +#define SCMI_MAX_NUM_SENSOR_AXIS	63
> +
>  enum scmi_sensor_protocol_cmd {
>  	SENSOR_DESCRIPTION_GET = 0x3,
>  	SENSOR_TRIP_POINT_NOTIFY = 0x4,
>  	SENSOR_TRIP_POINT_CONFIG = 0x5,
>  	SENSOR_READING_GET = 0x6,
> +	SENSOR_AXIS_DESCRIPTION_GET = 0x7,
> +	SENSOR_LIST_UPDATE_INTERVALS = 0x8,
>  };
>
>  struct scmi_msg_resp_sensor_attributes {
> @@ -28,23 +33,102 @@ struct scmi_msg_resp_sensor_attributes {
>  	__le32 reg_size;
>  };
>
> +/* v3 attributes_low macros */
> +#define SUPPORTS_UPDATE_NOTIFY(x)	FIELD_GET(BIT(30), (x))
> +#define SENSOR_TSTAMP_EXP(x)		FIELD_GET(GENMASK(14, 10), (x))
> +#define SUPPORTS_TIMESTAMP(x)		FIELD_GET(BIT(9), (x))
> +#define SUPPORTS_EXTEND_ATTRS(x)	FIELD_GET(BIT(8), (x))
> +
> +/* v2 attributes_high macros */
> +#define SENSOR_UPDATE_BASE(x)		FIELD_GET(GENMASK(31, 27), (x))
> +#define SENSOR_UPDATE_SCALE(x)		FIELD_GET(GENMASK(26, 22), (x))
> +
> +/* v3 attributes_high macros */
> +#define SENSOR_AXIS_NUMBER(x)		FIELD_GET(GENMASK(21, 16), (x))
> +#define SUPPORTS_AXIS(x)		FIELD_GET(BIT(8), (x))
> +
> +/* v3 resolution macros */
> +#define SENSOR_RES(x)			FIELD_GET(GENMASK(26, 0), (x))
> +#define SENSOR_RES_EXP(x)		FIELD_GET(GENMASK(31, 27), (x))
> +
> +struct scmi_range_attrs_le {

[nit] Does "_le" above indicate little endian ? If so, please drop it here
and elsewhere in the series as it is not consistent with other structure
names. LE is assumed throughout SCMI.

> +	__le32 min_range_low;
> +	__le32 min_range_high;
> +	__le32 max_range_low;
> +	__le32 max_range_high;
> +};
> +
>  struct scmi_msg_resp_sensor_description {
>  	__le16 num_returned;
>  	__le16 num_remaining;
> -	struct {
> +	struct scmi_sensor_descriptor {
>  		__le32 id;
>  		__le32 attributes_low;
> -#define SUPPORTS_ASYNC_READ(x)	((x) & BIT(31))
> -#define NUM_TRIP_POINTS(x)	((x) & 0xff)
> +/* Common attributes_low macros */
> +#define SUPPORTS_ASYNC_READ(x)		FIELD_GET(BIT(31), (x))
> +#define NUM_TRIP_POINTS(x)		FIELD_GET(GENMASK(7, 0), (x))
>  		__le32 attributes_high;
> -#define SENSOR_TYPE(x)		((x) & 0xff)
> -#define SENSOR_SCALE(x)		(((x) >> 11) & 0x1f)
> -#define SENSOR_SCALE_SIGN	BIT(4)
> -#define SENSOR_SCALE_EXTEND	GENMASK(7, 5)
> -#define SENSOR_UPDATE_SCALE(x)	(((x) >> 22) & 0x1f)
> -#define SENSOR_UPDATE_BASE(x)	(((x) >> 27) & 0x1f)
> -		    u8 name[SCMI_MAX_STR_SIZE];
> -	} desc[0];
> +/* Common attributes_high macros */
> +#define SENSOR_SCALE(x)			FIELD_GET(GENMASK(15, 11), (x))
> +#define SENSOR_SCALE_SIGN		BIT(4)
> +#define SENSOR_SCALE_EXTEND		GENMASK(31, 5)
> +#define SENSOR_TYPE(x)			FIELD_GET(GENMASK(7, 0), (x))
> +		u8 name[SCMI_MAX_STR_SIZE];
> +		/* only for version > 2.0 */
> +		__le32 power;
> +		__le32 resolution;
> +		struct scmi_range_attrs_le scalar_attrs;
> +	} desc[];
> +};
> +
> +/* Sign extend to a full s32 */
> +#define	S32_EXT(v)							\
> +	({								\
> +		int __v = (v);						\
> +									\
> +		if (__v & SENSOR_SCALE_SIGN)				\
> +			__v |= SENSOR_SCALE_EXTEND;			\
> +		__v;							\
> +	})
> +
> +#define SCMI_MSG_RESP_SENS_DESCR_BASE_SZ				\
> +	(sizeof(struct scmi_sensor_descriptor) -			\
> +	  (2 * sizeof(__le32)) - sizeof(struct scmi_range_attrs_le))

Why can't we just hardcode the offset ? This needs changes when we modify
the structure in future for additional elements right ?

> +
> +struct scmi_msg_sensor_axis_description_get {
> +	__le32 id;
> +	__le32 axis_desc_index;
> +};
> +
> +struct scmi_msg_resp_sensor_axis_description {
> +	__le32 num_axis_flags;
> +#define NUM_AXIS_RETURNED(x)		FIELD_GET(GENMASK(5, 0), (x))
> +#define NUM_AXIS_REMAINING(x)		FIELD_GET(GENMASK(31, 26), (x))
> +	struct scmi_axis_descriptor {
> +		__le32 id;
> +		__le32 attributes_low;
> +		__le32 attributes_high;
> +		u8 name[SCMI_MAX_STR_SIZE];
> +		__le32 resolution;
> +		struct scmi_range_attrs_le attrs;
> +	} desc[];
> +};
> +
> +#define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ				\
> +		(sizeof(struct scmi_axis_descriptor) -			\
> +		  sizeof(__le32) - sizeof(struct scmi_range_attrs_le))
> +
> +struct scmi_msg_sensor_list_update_intervals {
> +	__le32 id;
> +	__le32 index;
> +};
> +
> +struct scmi_msg_resp_sensor_list_update_intervals {
> +	__le32 num_intervals_flags;
> +#define NUM_INTERVALS_RETURNED(x)	FIELD_GET(GENMASK(11, 0), (x))
> +#define SEGMENTED_INTVL_FORMAT(x)	FIELD_GET(BIT(12), (x))
> +#define NUM_INTERVALS_REMAINING(x)	FIELD_GET(GENMASK(31, 16), (x))
> +	__le32 intervals[];
>  };
>
>  struct scmi_msg_sensor_trip_point_notify {
> @@ -114,6 +198,194 @@ static int scmi_sensor_attributes_get(const struct scmi_handle *handle,
>  	return ret;
>  }
>
> +static inline void scmi_parse_range_attrs(struct scmi_range_attrs *out,
> +					  struct scmi_range_attrs_le *in)
> +{
> +	out->min_range = get_unaligned_le64((void *)&in->min_range_low);
> +	out->max_range = get_unaligned_le64((void *)&in->max_range_low);
> +}
> +
> +static int scmi_sensor_update_intervals(const struct scmi_handle *handle,
> +					struct scmi_sensor_info *s)
> +{
> +	int ret, cnt;
> +	u32 desc_index = 0;
> +	u16 num_returned, num_remaining;
> +	struct scmi_xfer *ti;
> +	struct scmi_msg_resp_sensor_list_update_intervals *buf;
> +	struct scmi_msg_sensor_list_update_intervals *msg;
> +
> +	ret = scmi_xfer_get_init(handle, SENSOR_LIST_UPDATE_INTERVALS,
> +				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &ti);
> +	if (ret)
> +		return ret;
> +
> +	buf = ti->rx.buf;
> +	do {
> +		u32 flags;
> +
> +		msg = ti->tx.buf;
> +		/* Set the number of sensors to be skipped/already read */
> +		msg->id = cpu_to_le32(s->id);
> +		msg->index = cpu_to_le32(desc_index);
> +
> +		ret = scmi_do_xfer(handle, ti);
> +		if (ret)
> +			break;
> +
> +		flags = le32_to_cpu(buf->num_intervals_flags);
> +		num_returned = NUM_INTERVALS_RETURNED(flags);
> +		num_remaining = NUM_INTERVALS_REMAINING(flags);
> +
> +		/*
> +		 * Max intervals is not declared previously anywhere so we
> +		 * assume it's returned+remaining.
> +		 */
> +		if (!s->intervals.count) {
> +			s->intervals.segmented = SEGMENTED_INTVL_FORMAT(flags);
> +			s->intervals.count = num_returned + num_remaining;
> +			/* segmented intervals are reported in one triplet */
> +			if (s->intervals.segmented &&
> +			    (num_remaining || num_returned != 3)) {
> +				dev_err(handle->dev,
> +					"Sensor ID:%d advertises an invalid segmented interval (%d)\n",
> +					s->id, s->intervals.count);
> +				s->intervals.segmented = false;
> +				s->intervals.count = 0;
> +				ret = -EINVAL;
> +				break;
> +			}
> +			/* Direct allocation when exceeding pre-allocated */
> +			if (s->intervals.count >= SCMI_MAX_PREALLOC_POOL) {
> +				s->intervals.desc =
> +					devm_kcalloc(handle->dev,
> +						     s->intervals.count,
> +						     sizeof(*s->intervals.desc),
> +						     GFP_KERNEL);
> +				if (!s->intervals.desc) {
> +					s->intervals.segmented = false;
> +					s->intervals.count = 0;
> +					ret = -ENOMEM;
> +					break;
> +				}
> +			}
> +		} else if (desc_index + num_returned > s->intervals.count) {
> +			dev_err(handle->dev,
> +				"No. of update intervals can't exceed %d\n",
> +				s->intervals.count);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		for (cnt = 0; cnt < num_returned; cnt++)
> +			s->intervals.desc[desc_index + cnt] =
> +					le32_to_cpu(buf->intervals[cnt]);
> +
> +		desc_index += num_returned;
> +
> +		scmi_reset_rx_to_maxsz(handle, ti);
> +		/*
> +		 * check for both returned and remaining to avoid infinite
> +		 * loop due to buggy firmware
> +		 */
> +	} while (num_returned && num_remaining);
> +
> +	scmi_xfer_put(handle, ti);
> +	return ret;
> +}
> +
> +static int scmi_sensor_axis_description(const struct scmi_handle *handle,
> +					struct scmi_sensor_info *s)
> +{
> +	int ret, cnt;
> +	u32 desc_index = 0;
> +	u16 num_returned, num_remaining;
> +	struct scmi_xfer *te;
> +	struct scmi_msg_resp_sensor_axis_description *buf;
> +	struct scmi_msg_sensor_axis_description_get *msg;
> +
> +	s->axis = devm_kcalloc(handle->dev, s->num_axis,
> +			       sizeof(*s->axis), GFP_KERNEL);
> +	if (!s->axis)
> +		return -ENOMEM;
> +
> +	ret = scmi_xfer_get_init(handle, SENSOR_AXIS_DESCRIPTION_GET,
> +				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &te);
> +	if (ret)
> +		return ret;
> +
> +	buf = te->rx.buf;
> +	do {
> +		u32 flags;
> +		struct scmi_axis_descriptor *adesc;
> +
> +		msg = te->tx.buf;
> +		/* Set the number of sensors to be skipped/already read */
> +		msg->id = cpu_to_le32(s->id);
> +		msg->axis_desc_index = cpu_to_le32(desc_index);
> +
> +		ret = scmi_do_xfer(handle, te);
> +		if (ret)
> +			break;
> +
> +		flags = le32_to_cpu(buf->num_axis_flags);
> +		num_returned = NUM_AXIS_RETURNED(flags);
> +		num_remaining = NUM_AXIS_REMAINING(flags);
> +
> +		if (desc_index + num_returned > s->num_axis) {
> +			dev_err(handle->dev, "No. of axis can't exceed %d\n",
> +				s->num_axis);
> +			break;
> +		}
> +
> +		adesc = &buf->desc[0];
> +		for (cnt = 0; cnt < num_returned; cnt++) {
> +			u32 attrh, attrl;
> +			struct scmi_sensor_axis_info *a;
> +			size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
> +
> +			attrl = le32_to_cpu(adesc->attributes_low);
> +
> +			a = &s->axis[desc_index + cnt];
> +
> +			a->id = le32_to_cpu(adesc->id);
> +			a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
> +
> +			attrh = le32_to_cpu(adesc->attributes_high);
> +			a->scale = S32_EXT(SENSOR_SCALE(attrh));
> +			a->type = SENSOR_TYPE(attrh);
> +			strlcpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> +
> +			if (a->extended_attrs) {
> +				unsigned int ares =
> +					le32_to_cpu(adesc->resolution);
> +
> +				a->resolution = SENSOR_RES(ares);
> +				a->exponent =
> +					S32_EXT(SENSOR_RES_EXP(ares));
> +				dsize += sizeof(adesc->resolution);
> +
> +				scmi_parse_range_attrs(&a->attrs,
> +						       &adesc->attrs);
> +				dsize += sizeof(adesc->attrs);
> +			}
> +
> +			adesc = (typeof(adesc))((u8 *)adesc + dsize);

Just thinking if we can avoid this my having union comprising of v1 and v2
structures ?

> +		}
> +
> +		desc_index += num_returned;
> +
> +		scmi_reset_rx_to_maxsz(handle, te);
> +		/*
> +		 * check for both returned and remaining to avoid infinite
> +		 * loop due to buggy firmware
> +		 */
> +	} while (num_returned && num_remaining);
> +
> +	scmi_xfer_put(handle, te);
> +	return ret;
> +}
> +
>  static int scmi_sensor_description_get(const struct scmi_handle *handle,
>  				       struct sensors_info *si)
>  {
> @@ -131,9 +403,10 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
>  	buf = t->rx.buf;
>
>  	do {
> +		struct scmi_sensor_descriptor *sdesc;
> +
>  		/* Set the number of sensors to be skipped/already read */
>  		put_unaligned_le32(desc_index, t->tx.buf);
> -
>  		ret = scmi_do_xfer(handle, t);
>  		if (ret)
>  			break;
> @@ -147,22 +420,97 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
>  			break;
>  		}
>
> +		sdesc = &buf->desc[0];
>  		for (cnt = 0; cnt < num_returned; cnt++) {
>  			u32 attrh, attrl;
>  			struct scmi_sensor_info *s;
> +			size_t dsize = SCMI_MSG_RESP_SENS_DESCR_BASE_SZ;
>
> -			attrl = le32_to_cpu(buf->desc[cnt].attributes_low);
> -			attrh = le32_to_cpu(buf->desc[cnt].attributes_high);
>  			s = &si->sensors[desc_index + cnt];
> -			s->id = le32_to_cpu(buf->desc[cnt].id);
> -			s->type = SENSOR_TYPE(attrh);
> -			s->scale = SENSOR_SCALE(attrh);
> -			/* Sign extend to a full s8 */
> -			if (s->scale & SENSOR_SCALE_SIGN)
> -				s->scale |= SENSOR_SCALE_EXTEND;
> +			s->id = le32_to_cpu(sdesc->id);
> +
> +			attrl = le32_to_cpu(sdesc->attributes_low);
> +			/* common bitfields parsing */
>  			s->async = SUPPORTS_ASYNC_READ(attrl);
>  			s->num_trip_points = NUM_TRIP_POINTS(attrl);
> -			strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
> +			/**
> +			 * only v3.0 specific bitfield below.
> +			 * Such bitfields are assumed to be zeroed on non
> +			 * relevant fw versions...assuming fw not buggy !
> +			 */
> +			s->update = SUPPORTS_UPDATE_NOTIFY(attrl);
> +			s->timestamped = SUPPORTS_TIMESTAMP(attrl);
> +			if (s->timestamped)
> +				s->tstamp_scale =
> +					S32_EXT(SENSOR_TSTAMP_EXP(attrl));
> +			s->extended_scalar_attrs =
> +				SUPPORTS_EXTEND_ATTRS(attrl);
> +
> +			attrh = le32_to_cpu(sdesc->attributes_high);
> +			/* common bitfields parsing */
> +			s->scale = S32_EXT(SENSOR_SCALE(attrh));
> +			s->type = SENSOR_TYPE(attrh);
> +			/* Use pre-allocated pool wherever possible */
> +			s->intervals.desc = s->intervals.prealloc_pool;
> +			if (si->version == 0x10000) {
> +				s->intervals.segmented = false;
> +				s->intervals.count = 1;
> +				/*
> +				 * Convert SCMIv2.0 update interval format to
> +				 * SCMIv3.0 to be used as the common exposed
> +				 * descriptor, accessible via common macros.
> +				 */
> +				s->intervals.desc[0] =
> +					(SENSOR_UPDATE_BASE(attrh) << 5) |
> +					 SENSOR_UPDATE_SCALE(attrh);
> +			} else {
> +				/*
> +				 * From version v3.0 update intervals are

The version comment might be confusing and the check for si->version.
We need to clarify until SCMIv3.0, sensor protocol version = v1.0 and
it is v2.0 from SCMI v3.0 onwards.

> +				 * retrieved via a dedicated (optional) command.
> +				 * Since the command is optional, on error carry
> +				 * on without any update interval.
> +				 */
> +				if (scmi_sensor_update_intervals(handle, s))
> +					dev_info(handle->dev,
> +						 "Update Intervals not available for sensor ID:%d\n",
> +						 s->id);

Can we drop the logging or make it _dbg ? Make flood in a system with 100s of
sensors.

> +			}
> +			/**
> +			 * only > v2.0 specific bitfield below.
> +			 * Such bitfields are assumed to be zeroed on non
> +			 * relevant fw versions...assuming fw not buggy !
> +			 */
> +			s->num_axis = min_t(unsigned int,
> +					    SUPPORTS_AXIS(attrh) ?
> +					    SENSOR_AXIS_NUMBER(attrh) : 0,
> +					    SCMI_MAX_NUM_SENSOR_AXIS);
> +			strlcpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
> +
> +			if (s->extended_scalar_attrs) {
> +				s->sensor_power = le32_to_cpu(sdesc->power);
> +				dsize += sizeof(sdesc->power);
> +				/* Only for sensors reporting scalar values */
> +				if (s->num_axis == 0) {
> +					unsigned int sres =
> +						le32_to_cpu(sdesc->resolution);
> +
> +					s->resolution = SENSOR_RES(sres);
> +					s->exponent =
> +						S32_EXT(SENSOR_RES_EXP(sres));
> +					dsize += sizeof(sdesc->resolution);
> +
> +					scmi_parse_range_attrs(&s->scalar_attrs,
> +							       &sdesc->scalar_attrs);
> +					dsize += sizeof(sdesc->scalar_attrs);
> +				}
> +			}
> +			if (s->num_axis > 0) {
> +				ret = scmi_sensor_axis_description(handle, s);
> +				if (ret)
> +					goto out;
> +			}
> +
> +			sdesc = (typeof(sdesc))((u8 *)sdesc + dsize);
>  		}
>
>  		desc_index += num_returned;
> @@ -174,6 +522,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
>  		 */
>  	} while (num_returned && num_remaining);
>
> +out:
>  	scmi_xfer_put(handle, t);
>  	return ret;
>  }
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 9cd312a1ff92..53cc5ce0e494 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -8,6 +8,7 @@
>  #ifndef _LINUX_SCMI_PROTOCOL_H
>  #define _LINUX_SCMI_PROTOCOL_H
>
> +#include <linux/bitfield.h>
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/types.h>
> @@ -148,26 +149,238 @@ struct scmi_power_ops {
>  			 u32 *state);
>  };
>
> +/**
> + * scmi_range_attrs  - specifies a sensor or axis values' range
> + * @min_range: The minimum value which can be represented by the sensor/axis.
> + * @max_range: The maximum value which can be represented by the sensor/axis.
> + */
> +struct scmi_range_attrs {
> +	long long min_range;
> +	long long max_range;
> +};
> +
> +/**
> + * scmi_sensor_axis_info  - describes one sensor axes
> + * @id: The axes ID.
> + * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> + * @scale: Power-of-10 multiplier applied to the axis unit.
> + * @name: NULL-terminated string representing axes name as advertised by
> + *	  SCMI platform.
> + * @extended_attrs: Flag to indicate the presence of additional extended
> + *		    attributes for this axes.
> + * @resolution: Extended attribute representing the resolution of the axes.
> + *		Set to 0 if not reported by this axes.
> + * @exponent: Extended attribute representing the power-of-10 multiplier that
> + *	      is applied to the resolution field. Set to 0 if not reported by
> + *	      this axes.
> + * @attrs: Extended attributes representing minimum and maximum values
> + *	   measurable by this axes. Set to 0 if not reported by this sensor.
> + */
> +struct scmi_sensor_axis_info {
> +	unsigned int id;
> +	unsigned int type;
> +	int scale;
> +	char name[SCMI_MAX_STR_SIZE];
> +	bool extended_attrs;
> +	unsigned int resolution;
> +	int exponent;
> +	struct scmi_range_attrs attrs;
> +};
> +
> +/**
> + * scmi_sensor_intervals_info  - describes number and type of available update
> + * intervals
> + * @segmented: Flag for segmented intervals' representation. When True there
> + *	       will be exactly 3 intervals in @desc, with each entry
> + *	       representing a member of a segment in this order:
> + *	       {lowest update interval, highest update interval, step size}
> + * @count: Number of intervals described in @desc.
> + * @desc: Array of @count interval descriptor bitmask represented as detailed in
> + *	  the SCMI specification: it can be accessed using the accompanying
> + *	  macros.
> + * @prealloc_pool: A minimal preallocated pool of desc entries used to avoid
> + *		   lesser-than-64-bytes dynamic allocation for small @count
> + *		   values.
> + */
> +struct scmi_sensor_intervals_info {
> +	bool segmented;
> +	unsigned int count;
> +#define SCMI_SENS_INTVL_SEGMENT_LOW	0
> +#define SCMI_SENS_INTVL_SEGMENT_HIGH	1
> +#define SCMI_SENS_INTVL_SEGMENT_STEP	2
> +	unsigned int *desc;
> +#define SCMI_SENS_INTVL_GET_SECS(x)		FIELD_GET(GENMASK(20, 5), (x))
> +#define SCMI_SENS_INTVL_GET_EXP(x)					\
> +	({								\
> +		int __signed_exp = FIELD_GET(GENMASK(4, 0), (x));	\
> +									\
> +		if (__signed_exp & BIT(4))				\
> +			__signed_exp |= GENMASK(31, 5);			\
> +		__signed_exp;						\
> +	})
> +#define SCMI_MAX_PREALLOC_POOL			16
> +	unsigned int prealloc_pool[SCMI_MAX_PREALLOC_POOL];
> +};
> +
> +/**
> + * struct scmi_sensor_info - represents information related to one of the
> + * available sensors.
> + * @id: Sensor ID.
> + * @type: Sensor type. Chosen amongst one of @enum scmi_sensor_class.
> + * @scale: Power-of-10 multiplier applied to the sensor unit.
> + * @num_trip_points: Number of maximum configurable trip points.
> + * @async: Flag for asynchronous read support.
> + * @update: Flag for continuouos update notification support.
> + * @timestamped: Flag for timestamped read support.
> + * @tstamp_scale: Power-of-10 multiplier applied to the sensor timestamps to
> + *		  represent it in seconds.
> + * @num_axis: Number of supported axis if any. Reported as 0 for scalar sensors.
> + * @axis: Pointer to an array of @num_axis descriptors.
> + * @intervals: Descriptor of available update intervals.
> + * @sensor_config: A bitmask reporting the current sensor configuration as
> + *		   detailed in the SCMI specification: it can accessed and
> + *		   modified through the accompanying macros.
> + * @name: NULL-terminated string representing sensor name as advertised by
> + *	  SCMI platform.
> + * @extended_scalar_attrs: Flag to indicate the presence of additional extended
> + *			   attributes for this sensor.
> + * @sensor_power: Extended attribute representing the average power
> + *		  consumed by the sensor in microwatts (uW) when it is active.
> + *		  Reported here only for scalar sensors.
> + *		  Set to 0 if not reported by this sensor.
> + * @resolution: Extended attribute representing the resolution of the sensor.
> + *		Reported here only for scalar sensors.
> + *		Set to 0 if not reported by this sensor.
> + * @exponent: Extended attribute representing the power-of-10 multiplier that is
> + *	      applied to the resolution field.
> + *	      Reported here only for scalar sensors.
> + *	      Set to 0 if not reported by this sensor.
> + * @scalar_attrs: Extended attributes representing minimum and maximum
> + *		  measurable values by this sensor.
> + *		  Reported here only for scalar sensors.
> + *		  Set to 0 if not reported by this sensor.
> + */
>  struct scmi_sensor_info {
> -	u32 id;
> -	u8 type;
> -	s8 scale;
> -	u8 num_trip_points;
> +	unsigned int id;
> +	unsigned int type;
> +	int scale;
> +	unsigned int num_trip_points;
>  	bool async;
> +	bool update;
> +	bool timestamped;
> +	int tstamp_scale;
> +	unsigned int num_axis;
> +	struct scmi_sensor_axis_info *axis;
> +	struct scmi_sensor_intervals_info intervals;
>  	char name[SCMI_MAX_STR_SIZE];
> +	bool extended_scalar_attrs;
> +	unsigned int sensor_power;
> +	unsigned int resolution;
> +	int exponent;
> +	struct scmi_range_attrs scalar_attrs;
>  };
>

Below changes can go in a separate patch as it is just update of entires
as per latest SCMIv2.0 spec and not entirely related to v3.0.

>  /*
>   * Partial list from Distributed Management Task Force (DMTF) specification:
> - * DSP0249 (Platform Level Data Model specification)
> + * DSP0248 (Platform Level Data Model for Platform Monitoring and Control
> + * specification)
>   */

--
Regards,
Sudeep

Re: [PATCH v3 3/6] hwmon: scmi: update hwmon internal scale data type

[Sudeep Holla]

On Wed, Nov 18, 2020 at 04:29:02PM +0000, Cristian Marussi wrote:
> Use an int to calculate scale values inside scmi_hwmon_scale() to match
> the updated scale data type in struct scmi_sensor_info.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/hwmon/scmi-hwmon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 09ce30cba54b..17d064e58938 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -30,7 +30,7 @@ static inline u64 __pow10(u8 x)
>  
>  static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value)
>  {
> -	s8 scale = sensor->scale;
> +	int scale = sensor->scale;

Can this go independent of the series ? You haven't cc-ed the hwmon maintainer
and the list. We need their ack if we have to take it as part of the series.

-- 
Regards,
Sudeep

Re: [PATCH v3 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads

[Sudeep Holla]

On Wed, Nov 18, 2020 at 04:29:03PM +0000, Cristian Marussi wrote:
> Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0
> timestamped reads.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> V2 --> v3
> - setting rx_size to 0 in sensor_reading_get to allow fw to send
>   both v2 and v3 replies...even if sensor_reading_get() only handles
>   v2 spec and returns one single value
> - using get_unaligned_le64 in lieu of le64_to_cpu
> - removed refs to v2.1
> ---
>  drivers/firmware/arm_scmi/sensors.c | 137 ++++++++++++++++++++++++++--
>  include/linux/scmi_protocol.h       |  22 +++++
>  2 files changed, 152 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 1c83aaae0012..0adc545116a4 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get {
>  #define SENSOR_READ_ASYNC	BIT(0)
>  };
>  
> +struct scmi_resp_sensor_reading_get {
> +	__le64 readings;

Generally I have avoided such single element structures so far. Any
particular reasons for having it ?

-- 
Regards,
Sudeep

Re: [PATCH v3 5/6] firmware: arm_scmi: add SCMIv3.0 Sensor configuration support

[Sudeep Holla]

On Wed, Nov 18, 2020 at 04:29:04PM +0000, Cristian Marussi wrote:
> Add SCMIv3.0 Sensor support for CONFIG_GET/CONFIG_SET commands.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/sensors.c | 75 +++++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h       | 37 ++++++++++++++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 0adc545116a4..fa3385045361 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -22,6 +22,8 @@ enum scmi_sensor_protocol_cmd {
>  	SENSOR_READING_GET = 0x6,
>  	SENSOR_AXIS_DESCRIPTION_GET = 0x7,
>  	SENSOR_LIST_UPDATE_INTERVALS = 0x8,
> +	SENSOR_CONFIG_GET = 0x9,
> +	SENSOR_CONFIG_SET = 0xA,
>  };
>  
>  struct scmi_msg_resp_sensor_attributes {
> @@ -150,6 +152,19 @@ struct scmi_msg_set_sensor_trip_point {
>  	__le32 value_high;
>  };
>  
> +struct scmi_msg_sensor_config_get {
> +	__le32 id;
> +};
> +
> +struct scmi_resp_sensor_config_get {
> +	__le32 sensor_config;
> +};
> +

Same comment about single element structure as in previous patch.

-- 
Regards,
Sudeep

Re: [PATCH v3 6/6] firmware: arm_scmi: add SCMIv3.0 Sensor notifications

[Sudeep Holla]

On Wed, Nov 18, 2020 at 04:29:05PM +0000, Cristian Marussi wrote:
> Add support for new SCMIv3.0 SENSOR_UPDATE notification.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v2 --> v3
> - removed stale unused msg payload definition
> - moved variable declaration inside switch block

Other than few minor comments in the previous patches, this series look OK.

-- 
Regards,
Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions

[Cristian Marussi]

Hi Sudeep

thanks for the review.

On Thu, Nov 19, 2020 at 11:39:00AM +0000, Sudeep Holla wrote:
> On Wed, Nov 18, 2020 at 04:29:01PM +0000, Cristian Marussi wrote:
> > Add support for new SCMIv3.0 Sensors extensions related to new sensors'
> > features, like multiple axis and update intervals, while keeping
> > compatibility with SCMIv2.0 features.
> > While at that, refactor and simplify all the internal helpers macros and
> > move struct scmi_sensor_info to use only non-fixed-size typing.
> >
> 
> Sorry for late review.
> 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v2 --> v3
> > - Fix SCMI_MAX_NUM_SENSOR_AXIS
> > - added missing Dox comment in resolution
> > - added common INTVL SEGMENT macros
> >
> > v1 --> v2
> > - restrict segmented intervals descriptors to single triplet
> > - add proper usage of scmi_reset_rx_to_maxsz
> > ---
> >  drivers/firmware/arm_scmi/sensors.c | 391 ++++++++++++++++++++++++++--
> >  include/linux/scmi_protocol.h       | 223 +++++++++++++++-
> >  2 files changed, 588 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> > index 6aaff478d032..1c83aaae0012 100644
> > --- a/drivers/firmware/arm_scmi/sensors.c
> > +++ b/drivers/firmware/arm_scmi/sensors.c
> > @@ -7,16 +7,21 @@
> >
> >  #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
> >
> > +#include <linux/bitfield.h>
> >  #include <linux/scmi_protocol.h>
> >
> >  #include "common.h"
> >  #include "notify.h"
> >
> > +#define SCMI_MAX_NUM_SENSOR_AXIS	63
> > +
> >  enum scmi_sensor_protocol_cmd {
> >  	SENSOR_DESCRIPTION_GET = 0x3,
> >  	SENSOR_TRIP_POINT_NOTIFY = 0x4,
> >  	SENSOR_TRIP_POINT_CONFIG = 0x5,
> >  	SENSOR_READING_GET = 0x6,
> > +	SENSOR_AXIS_DESCRIPTION_GET = 0x7,
> > +	SENSOR_LIST_UPDATE_INTERVALS = 0x8,
> >  };
> >
> >  struct scmi_msg_resp_sensor_attributes {
> > @@ -28,23 +33,102 @@ struct scmi_msg_resp_sensor_attributes {
> >  	__le32 reg_size;
> >  };
> >
> > +/* v3 attributes_low macros */
> > +#define SUPPORTS_UPDATE_NOTIFY(x)	FIELD_GET(BIT(30), (x))
> > +#define SENSOR_TSTAMP_EXP(x)		FIELD_GET(GENMASK(14, 10), (x))
> > +#define SUPPORTS_TIMESTAMP(x)		FIELD_GET(BIT(9), (x))
> > +#define SUPPORTS_EXTEND_ATTRS(x)	FIELD_GET(BIT(8), (x))
> > +
> > +/* v2 attributes_high macros */
> > +#define SENSOR_UPDATE_BASE(x)		FIELD_GET(GENMASK(31, 27), (x))
> > +#define SENSOR_UPDATE_SCALE(x)		FIELD_GET(GENMASK(26, 22), (x))
> > +
> > +/* v3 attributes_high macros */
> > +#define SENSOR_AXIS_NUMBER(x)		FIELD_GET(GENMASK(21, 16), (x))
> > +#define SUPPORTS_AXIS(x)		FIELD_GET(BIT(8), (x))
> > +
> > +/* v3 resolution macros */
> > +#define SENSOR_RES(x)			FIELD_GET(GENMASK(26, 0), (x))
> > +#define SENSOR_RES_EXP(x)		FIELD_GET(GENMASK(31, 27), (x))
> > +
> > +struct scmi_range_attrs_le {
> 
> [nit] Does "_le" above indicate little endian ? If so, please drop it here
> and elsewhere in the series as it is not consistent with other structure
> names. LE is assumed throughout SCMI.
> 
Yes, the idea really was to have a different naming for this internal
structure which is mapped out to an scmi_range_attrs exposed in scmi_protocol.h
with low/high values merged into u64 for the externally visible fields...
...so I can rename as wished....it was just a way to represent here that it is
a msg structure. I'll rename it.

> > +	__le32 min_range_low;
> > +	__le32 min_range_high;
> > +	__le32 max_range_low;
> > +	__le32 max_range_high;
> > +};
> > +
> >  struct scmi_msg_resp_sensor_description {
> >  	__le16 num_returned;
> >  	__le16 num_remaining;
> > -	struct {
> > +	struct scmi_sensor_descriptor {
> >  		__le32 id;
> >  		__le32 attributes_low;
> > -#define SUPPORTS_ASYNC_READ(x)	((x) & BIT(31))
> > -#define NUM_TRIP_POINTS(x)	((x) & 0xff)
> > +/* Common attributes_low macros */
> > +#define SUPPORTS_ASYNC_READ(x)		FIELD_GET(BIT(31), (x))
> > +#define NUM_TRIP_POINTS(x)		FIELD_GET(GENMASK(7, 0), (x))
> >  		__le32 attributes_high;
> > -#define SENSOR_TYPE(x)		((x) & 0xff)
> > -#define SENSOR_SCALE(x)		(((x) >> 11) & 0x1f)
> > -#define SENSOR_SCALE_SIGN	BIT(4)
> > -#define SENSOR_SCALE_EXTEND	GENMASK(7, 5)
> > -#define SENSOR_UPDATE_SCALE(x)	(((x) >> 22) & 0x1f)
> > -#define SENSOR_UPDATE_BASE(x)	(((x) >> 27) & 0x1f)
> > -		    u8 name[SCMI_MAX_STR_SIZE];
> > -	} desc[0];
> > +/* Common attributes_high macros */
> > +#define SENSOR_SCALE(x)			FIELD_GET(GENMASK(15, 11), (x))
> > +#define SENSOR_SCALE_SIGN		BIT(4)
> > +#define SENSOR_SCALE_EXTEND		GENMASK(31, 5)
> > +#define SENSOR_TYPE(x)			FIELD_GET(GENMASK(7, 0), (x))
> > +		u8 name[SCMI_MAX_STR_SIZE];
> > +		/* only for version > 2.0 */
> > +		__le32 power;
> > +		__le32 resolution;
> > +		struct scmi_range_attrs_le scalar_attrs;
> > +	} desc[];
> > +};
> > +
> > +/* Sign extend to a full s32 */
> > +#define	S32_EXT(v)							\
> > +	({								\
> > +		int __v = (v);						\
> > +									\
> > +		if (__v & SENSOR_SCALE_SIGN)				\
> > +			__v |= SENSOR_SCALE_EXTEND;			\
> > +		__v;							\
> > +	})
> > +
> > +#define SCMI_MSG_RESP_SENS_DESCR_BASE_SZ				\
> > +	(sizeof(struct scmi_sensor_descriptor) -			\
> > +	  (2 * sizeof(__le32)) - sizeof(struct scmi_range_attrs_le))
> 
> Why can't we just hardcode the offset ? This needs changes when we modify
> the structure in future for additional elements right ?
> 

Ok, I'll do I thought was more self explaining to see how it is
calculated but I can add a comment.

> > +
> > +struct scmi_msg_sensor_axis_description_get {
> > +	__le32 id;
> > +	__le32 axis_desc_index;
> > +};
> > +
> > +struct scmi_msg_resp_sensor_axis_description {
> > +	__le32 num_axis_flags;
> > +#define NUM_AXIS_RETURNED(x)		FIELD_GET(GENMASK(5, 0), (x))
> > +#define NUM_AXIS_REMAINING(x)		FIELD_GET(GENMASK(31, 26), (x))
> > +	struct scmi_axis_descriptor {
> > +		__le32 id;
> > +		__le32 attributes_low;
> > +		__le32 attributes_high;
> > +		u8 name[SCMI_MAX_STR_SIZE];
> > +		__le32 resolution;
> > +		struct scmi_range_attrs_le attrs;
> > +	} desc[];
> > +};
> > +
> > +#define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ				\
> > +		(sizeof(struct scmi_axis_descriptor) -			\
> > +		  sizeof(__le32) - sizeof(struct scmi_range_attrs_le))
> > +
> > +struct scmi_msg_sensor_list_update_intervals {
> > +	__le32 id;
> > +	__le32 index;
> > +};
> > +
> > +struct scmi_msg_resp_sensor_list_update_intervals {
> > +	__le32 num_intervals_flags;
> > +#define NUM_INTERVALS_RETURNED(x)	FIELD_GET(GENMASK(11, 0), (x))
> > +#define SEGMENTED_INTVL_FORMAT(x)	FIELD_GET(BIT(12), (x))
> > +#define NUM_INTERVALS_REMAINING(x)	FIELD_GET(GENMASK(31, 16), (x))
> > +	__le32 intervals[];
> >  };
> >
> >  struct scmi_msg_sensor_trip_point_notify {
> > @@ -114,6 +198,194 @@ static int scmi_sensor_attributes_get(const struct scmi_handle *handle,
> >  	return ret;
> >  }
> >
> > +static inline void scmi_parse_range_attrs(struct scmi_range_attrs *out,
> > +					  struct scmi_range_attrs_le *in)
> > +{
> > +	out->min_range = get_unaligned_le64((void *)&in->min_range_low);
> > +	out->max_range = get_unaligned_le64((void *)&in->max_range_low);
> > +}
> > +
> > +static int scmi_sensor_update_intervals(const struct scmi_handle *handle,
> > +					struct scmi_sensor_info *s)
> > +{
> > +	int ret, cnt;
> > +	u32 desc_index = 0;
> > +	u16 num_returned, num_remaining;
> > +	struct scmi_xfer *ti;
> > +	struct scmi_msg_resp_sensor_list_update_intervals *buf;
> > +	struct scmi_msg_sensor_list_update_intervals *msg;
> > +
> > +	ret = scmi_xfer_get_init(handle, SENSOR_LIST_UPDATE_INTERVALS,
> > +				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &ti);
> > +	if (ret)
> > +		return ret;
> > +
> > +	buf = ti->rx.buf;
> > +	do {
> > +		u32 flags;
> > +
> > +		msg = ti->tx.buf;
> > +		/* Set the number of sensors to be skipped/already read */
> > +		msg->id = cpu_to_le32(s->id);
> > +		msg->index = cpu_to_le32(desc_index);
> > +
> > +		ret = scmi_do_xfer(handle, ti);
> > +		if (ret)
> > +			break;
> > +
> > +		flags = le32_to_cpu(buf->num_intervals_flags);
> > +		num_returned = NUM_INTERVALS_RETURNED(flags);
> > +		num_remaining = NUM_INTERVALS_REMAINING(flags);
> > +
> > +		/*
> > +		 * Max intervals is not declared previously anywhere so we
> > +		 * assume it's returned+remaining.
> > +		 */
> > +		if (!s->intervals.count) {
> > +			s->intervals.segmented = SEGMENTED_INTVL_FORMAT(flags);
> > +			s->intervals.count = num_returned + num_remaining;
> > +			/* segmented intervals are reported in one triplet */
> > +			if (s->intervals.segmented &&
> > +			    (num_remaining || num_returned != 3)) {
> > +				dev_err(handle->dev,
> > +					"Sensor ID:%d advertises an invalid segmented interval (%d)\n",
> > +					s->id, s->intervals.count);
> > +				s->intervals.segmented = false;
> > +				s->intervals.count = 0;
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +			/* Direct allocation when exceeding pre-allocated */
> > +			if (s->intervals.count >= SCMI_MAX_PREALLOC_POOL) {
> > +				s->intervals.desc =
> > +					devm_kcalloc(handle->dev,
> > +						     s->intervals.count,
> > +						     sizeof(*s->intervals.desc),
> > +						     GFP_KERNEL);
> > +				if (!s->intervals.desc) {
> > +					s->intervals.segmented = false;
> > +					s->intervals.count = 0;
> > +					ret = -ENOMEM;
> > +					break;
> > +				}
> > +			}
> > +		} else if (desc_index + num_returned > s->intervals.count) {
> > +			dev_err(handle->dev,
> > +				"No. of update intervals can't exceed %d\n",
> > +				s->intervals.count);
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +
> > +		for (cnt = 0; cnt < num_returned; cnt++)
> > +			s->intervals.desc[desc_index + cnt] =
> > +					le32_to_cpu(buf->intervals[cnt]);
> > +
> > +		desc_index += num_returned;
> > +
> > +		scmi_reset_rx_to_maxsz(handle, ti);
> > +		/*
> > +		 * check for both returned and remaining to avoid infinite
> > +		 * loop due to buggy firmware
> > +		 */
> > +	} while (num_returned && num_remaining);
> > +
> > +	scmi_xfer_put(handle, ti);
> > +	return ret;
> > +}
> > +
> > +static int scmi_sensor_axis_description(const struct scmi_handle *handle,
> > +					struct scmi_sensor_info *s)
> > +{
> > +	int ret, cnt;
> > +	u32 desc_index = 0;
> > +	u16 num_returned, num_remaining;
> > +	struct scmi_xfer *te;
> > +	struct scmi_msg_resp_sensor_axis_description *buf;
> > +	struct scmi_msg_sensor_axis_description_get *msg;
> > +
> > +	s->axis = devm_kcalloc(handle->dev, s->num_axis,
> > +			       sizeof(*s->axis), GFP_KERNEL);
> > +	if (!s->axis)
> > +		return -ENOMEM;
> > +
> > +	ret = scmi_xfer_get_init(handle, SENSOR_AXIS_DESCRIPTION_GET,
> > +				 SCMI_PROTOCOL_SENSOR, sizeof(*msg), 0, &te);
> > +	if (ret)
> > +		return ret;
> > +
> > +	buf = te->rx.buf;
> > +	do {
> > +		u32 flags;
> > +		struct scmi_axis_descriptor *adesc;
> > +
> > +		msg = te->tx.buf;
> > +		/* Set the number of sensors to be skipped/already read */
> > +		msg->id = cpu_to_le32(s->id);
> > +		msg->axis_desc_index = cpu_to_le32(desc_index);
> > +
> > +		ret = scmi_do_xfer(handle, te);
> > +		if (ret)
> > +			break;
> > +
> > +		flags = le32_to_cpu(buf->num_axis_flags);
> > +		num_returned = NUM_AXIS_RETURNED(flags);
> > +		num_remaining = NUM_AXIS_REMAINING(flags);
> > +
> > +		if (desc_index + num_returned > s->num_axis) {
> > +			dev_err(handle->dev, "No. of axis can't exceed %d\n",
> > +				s->num_axis);
> > +			break;
> > +		}
> > +
> > +		adesc = &buf->desc[0];
> > +		for (cnt = 0; cnt < num_returned; cnt++) {
> > +			u32 attrh, attrl;
> > +			struct scmi_sensor_axis_info *a;
> > +			size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
> > +
> > +			attrl = le32_to_cpu(adesc->attributes_low);
> > +
> > +			a = &s->axis[desc_index + cnt];
> > +
> > +			a->id = le32_to_cpu(adesc->id);
> > +			a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
> > +
> > +			attrh = le32_to_cpu(adesc->attributes_high);
> > +			a->scale = S32_EXT(SENSOR_SCALE(attrh));
> > +			a->type = SENSOR_TYPE(attrh);
> > +			strlcpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> > +
> > +			if (a->extended_attrs) {
> > +				unsigned int ares =
> > +					le32_to_cpu(adesc->resolution);
> > +
> > +				a->resolution = SENSOR_RES(ares);
> > +				a->exponent =
> > +					S32_EXT(SENSOR_RES_EXP(ares));
> > +				dsize += sizeof(adesc->resolution);
> > +
> > +				scmi_parse_range_attrs(&a->attrs,
> > +						       &adesc->attrs);
> > +				dsize += sizeof(adesc->attrs);
> > +			}
> > +
> > +			adesc = (typeof(adesc))((u8 *)adesc + dsize);
> 
> Just thinking if we can avoid this my having union comprising of v1 and v2
> structures ?
> 

Not sure to understand, axis_descr are only v3.0 and in fact this is
called only for v > 2 I think, BUT the problem is that both this and the
main sensor descriptor v3 msg payloads are runtime variable, so that it
is stated in the msg->extended_attrs itself if that particular sensor desc
response that I'm parsing has the additional extended fields or not:
so the dance with dsize to keep track of where the current response ends
and when the next starts...but maybe I've not got really what you meant.

> > +		}
> > +
> > +		desc_index += num_returned;
> > +
> > +		scmi_reset_rx_to_maxsz(handle, te);
> > +		/*
> > +		 * check for both returned and remaining to avoid infinite
> > +		 * loop due to buggy firmware
> > +		 */
> > +	} while (num_returned && num_remaining);
> > +
> > +	scmi_xfer_put(handle, te);
> > +	return ret;
> > +}
> > +
> >  static int scmi_sensor_description_get(const struct scmi_handle *handle,
> >  				       struct sensors_info *si)
> >  {
> > @@ -131,9 +403,10 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
> >  	buf = t->rx.buf;
> >
> >  	do {
> > +		struct scmi_sensor_descriptor *sdesc;
> > +
> >  		/* Set the number of sensors to be skipped/already read */
> >  		put_unaligned_le32(desc_index, t->tx.buf);
> > -
> >  		ret = scmi_do_xfer(handle, t);
> >  		if (ret)
> >  			break;
> > @@ -147,22 +420,97 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
> >  			break;
> >  		}
> >
> > +		sdesc = &buf->desc[0];
> >  		for (cnt = 0; cnt < num_returned; cnt++) {
> >  			u32 attrh, attrl;
> >  			struct scmi_sensor_info *s;
> > +			size_t dsize = SCMI_MSG_RESP_SENS_DESCR_BASE_SZ;
> >
> > -			attrl = le32_to_cpu(buf->desc[cnt].attributes_low);
> > -			attrh = le32_to_cpu(buf->desc[cnt].attributes_high);
> >  			s = &si->sensors[desc_index + cnt];
> > -			s->id = le32_to_cpu(buf->desc[cnt].id);
> > -			s->type = SENSOR_TYPE(attrh);
> > -			s->scale = SENSOR_SCALE(attrh);
> > -			/* Sign extend to a full s8 */
> > -			if (s->scale & SENSOR_SCALE_SIGN)
> > -				s->scale |= SENSOR_SCALE_EXTEND;
> > +			s->id = le32_to_cpu(sdesc->id);
> > +
> > +			attrl = le32_to_cpu(sdesc->attributes_low);
> > +			/* common bitfields parsing */
> >  			s->async = SUPPORTS_ASYNC_READ(attrl);
> >  			s->num_trip_points = NUM_TRIP_POINTS(attrl);
> > -			strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
> > +			/**
> > +			 * only v3.0 specific bitfield below.
> > +			 * Such bitfields are assumed to be zeroed on non
> > +			 * relevant fw versions...assuming fw not buggy !
> > +			 */
> > +			s->update = SUPPORTS_UPDATE_NOTIFY(attrl);
> > +			s->timestamped = SUPPORTS_TIMESTAMP(attrl);
> > +			if (s->timestamped)
> > +				s->tstamp_scale =
> > +					S32_EXT(SENSOR_TSTAMP_EXP(attrl));
> > +			s->extended_scalar_attrs =
> > +				SUPPORTS_EXTEND_ATTRS(attrl);
> > +
> > +			attrh = le32_to_cpu(sdesc->attributes_high);
> > +			/* common bitfields parsing */
> > +			s->scale = S32_EXT(SENSOR_SCALE(attrh));
> > +			s->type = SENSOR_TYPE(attrh);
> > +			/* Use pre-allocated pool wherever possible */
> > +			s->intervals.desc = s->intervals.prealloc_pool;
> > +			if (si->version == 0x10000) {
> > +				s->intervals.segmented = false;
> > +				s->intervals.count = 1;
> > +				/*
> > +				 * Convert SCMIv2.0 update interval format to
> > +				 * SCMIv3.0 to be used as the common exposed
> > +				 * descriptor, accessible via common macros.
> > +				 */
> > +				s->intervals.desc[0] =
> > +					(SENSOR_UPDATE_BASE(attrh) << 5) |
> > +					 SENSOR_UPDATE_SCALE(attrh);
> > +			} else {
> > +				/*
> > +				 * From version v3.0 update intervals are
> 
> The version comment might be confusing and the check for si->version.
> We need to clarify until SCMIv3.0, sensor protocol version = v1.0 and
> it is v2.0 from SCMI v3.0 onwards.

Ok I'll introduce some define if it's fine for you.

> 
> > +				 * retrieved via a dedicated (optional) command.
> > +				 * Since the command is optional, on error carry
> > +				 * on without any update interval.
> > +				 */
> > +				if (scmi_sensor_update_intervals(handle, s))
> > +					dev_info(handle->dev,
> > +						 "Update Intervals not available for sensor ID:%d\n",
> > +						 s->id);
> 
> Can we drop the logging or make it _dbg ? Make flood in a system with 100s of
> sensors.
> 

Sure, I was wondering in fact what to do with this: because the command
is optional but it seemed odd to me that an SCMIv3.0 sensor does not
expose any update interval so I wanted to log somehow this anomaly.
(but maybe it's just not an anomaly)

> > +			}
> > +			/**
> > +			 * only > v2.0 specific bitfield below.
> > +			 * Such bitfields are assumed to be zeroed on non
> > +			 * relevant fw versions...assuming fw not buggy !
> > +			 */
> > +			s->num_axis = min_t(unsigned int,
> > +					    SUPPORTS_AXIS(attrh) ?
> > +					    SENSOR_AXIS_NUMBER(attrh) : 0,
> > +					    SCMI_MAX_NUM_SENSOR_AXIS);
> > +			strlcpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
> > +
> > +			if (s->extended_scalar_attrs) {
> > +				s->sensor_power = le32_to_cpu(sdesc->power);
> > +				dsize += sizeof(sdesc->power);
> > +				/* Only for sensors reporting scalar values */
> > +				if (s->num_axis == 0) {
> > +					unsigned int sres =
> > +						le32_to_cpu(sdesc->resolution);
> > +
> > +					s->resolution = SENSOR_RES(sres);
> > +					s->exponent =
> > +						S32_EXT(SENSOR_RES_EXP(sres));
> > +					dsize += sizeof(sdesc->resolution);
> > +
> > +					scmi_parse_range_attrs(&s->scalar_attrs,
> > +							       &sdesc->scalar_attrs);
> > +					dsize += sizeof(sdesc->scalar_attrs);
> > +				}
> > +			}
> > +			if (s->num_axis > 0) {
> > +				ret = scmi_sensor_axis_description(handle, s);
> > +				if (ret)
> > +					goto out;
> > +			}
> > +
> > +			sdesc = (typeof(sdesc))((u8 *)sdesc + dsize);
> >  		}
> >
> >  		desc_index += num_returned;
> > @@ -174,6 +522,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
> >  		 */
> >  	} while (num_returned && num_remaining);
> >
> > +out:
> >  	scmi_xfer_put(handle, t);
> >  	return ret;
> >  }
> > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > index 9cd312a1ff92..53cc5ce0e494 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -8,6 +8,7 @@
> >  #ifndef _LINUX_SCMI_PROTOCOL_H
> >  #define _LINUX_SCMI_PROTOCOL_H
> >
> > +#include <linux/bitfield.h>
> >  #include <linux/device.h>
> >  #include <linux/notifier.h>
> >  #include <linux/types.h>
> > @@ -148,26 +149,238 @@ struct scmi_power_ops {
> >  			 u32 *state);
> >  };
> >
> > +/**
> > + * scmi_range_attrs  - specifies a sensor or axis values' range
> > + * @min_range: The minimum value which can be represented by the sensor/axis.
> > + * @max_range: The maximum value which can be represented by the sensor/axis.
> > + */
> > +struct scmi_range_attrs {
> > +	long long min_range;
> > +	long long max_range;
> > +};
> > +
> > +/**
> > + * scmi_sensor_axis_info  - describes one sensor axes
> > + * @id: The axes ID.
> > + * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> > + * @scale: Power-of-10 multiplier applied to the axis unit.
> > + * @name: NULL-terminated string representing axes name as advertised by
> > + *	  SCMI platform.
> > + * @extended_attrs: Flag to indicate the presence of additional extended
> > + *		    attributes for this axes.
> > + * @resolution: Extended attribute representing the resolution of the axes.
> > + *		Set to 0 if not reported by this axes.
> > + * @exponent: Extended attribute representing the power-of-10 multiplier that
> > + *	      is applied to the resolution field. Set to 0 if not reported by
> > + *	      this axes.
> > + * @attrs: Extended attributes representing minimum and maximum values
> > + *	   measurable by this axes. Set to 0 if not reported by this sensor.
> > + */
> > +struct scmi_sensor_axis_info {
> > +	unsigned int id;
> > +	unsigned int type;
> > +	int scale;
> > +	char name[SCMI_MAX_STR_SIZE];
> > +	bool extended_attrs;
> > +	unsigned int resolution;
> > +	int exponent;
> > +	struct scmi_range_attrs attrs;
> > +};
> > +
> > +/**
> > + * scmi_sensor_intervals_info  - describes number and type of available update
> > + * intervals
> > + * @segmented: Flag for segmented intervals' representation. When True there
> > + *	       will be exactly 3 intervals in @desc, with each entry
> > + *	       representing a member of a segment in this order:
> > + *	       {lowest update interval, highest update interval, step size}
> > + * @count: Number of intervals described in @desc.
> > + * @desc: Array of @count interval descriptor bitmask represented as detailed in
> > + *	  the SCMI specification: it can be accessed using the accompanying
> > + *	  macros.
> > + * @prealloc_pool: A minimal preallocated pool of desc entries used to avoid
> > + *		   lesser-than-64-bytes dynamic allocation for small @count
> > + *		   values.
> > + */
> > +struct scmi_sensor_intervals_info {
> > +	bool segmented;
> > +	unsigned int count;
> > +#define SCMI_SENS_INTVL_SEGMENT_LOW	0
> > +#define SCMI_SENS_INTVL_SEGMENT_HIGH	1
> > +#define SCMI_SENS_INTVL_SEGMENT_STEP	2
> > +	unsigned int *desc;
> > +#define SCMI_SENS_INTVL_GET_SECS(x)		FIELD_GET(GENMASK(20, 5), (x))
> > +#define SCMI_SENS_INTVL_GET_EXP(x)					\
> > +	({								\
> > +		int __signed_exp = FIELD_GET(GENMASK(4, 0), (x));	\
> > +									\
> > +		if (__signed_exp & BIT(4))				\
> > +			__signed_exp |= GENMASK(31, 5);			\
> > +		__signed_exp;						\
> > +	})
> > +#define SCMI_MAX_PREALLOC_POOL			16
> > +	unsigned int prealloc_pool[SCMI_MAX_PREALLOC_POOL];
> > +};
> > +
> > +/**
> > + * struct scmi_sensor_info - represents information related to one of the
> > + * available sensors.
> > + * @id: Sensor ID.
> > + * @type: Sensor type. Chosen amongst one of @enum scmi_sensor_class.
> > + * @scale: Power-of-10 multiplier applied to the sensor unit.
> > + * @num_trip_points: Number of maximum configurable trip points.
> > + * @async: Flag for asynchronous read support.
> > + * @update: Flag for continuouos update notification support.
> > + * @timestamped: Flag for timestamped read support.
> > + * @tstamp_scale: Power-of-10 multiplier applied to the sensor timestamps to
> > + *		  represent it in seconds.
> > + * @num_axis: Number of supported axis if any. Reported as 0 for scalar sensors.
> > + * @axis: Pointer to an array of @num_axis descriptors.
> > + * @intervals: Descriptor of available update intervals.
> > + * @sensor_config: A bitmask reporting the current sensor configuration as
> > + *		   detailed in the SCMI specification: it can accessed and
> > + *		   modified through the accompanying macros.
> > + * @name: NULL-terminated string representing sensor name as advertised by
> > + *	  SCMI platform.
> > + * @extended_scalar_attrs: Flag to indicate the presence of additional extended
> > + *			   attributes for this sensor.
> > + * @sensor_power: Extended attribute representing the average power
> > + *		  consumed by the sensor in microwatts (uW) when it is active.
> > + *		  Reported here only for scalar sensors.
> > + *		  Set to 0 if not reported by this sensor.
> > + * @resolution: Extended attribute representing the resolution of the sensor.
> > + *		Reported here only for scalar sensors.
> > + *		Set to 0 if not reported by this sensor.
> > + * @exponent: Extended attribute representing the power-of-10 multiplier that is
> > + *	      applied to the resolution field.
> > + *	      Reported here only for scalar sensors.
> > + *	      Set to 0 if not reported by this sensor.
> > + * @scalar_attrs: Extended attributes representing minimum and maximum
> > + *		  measurable values by this sensor.
> > + *		  Reported here only for scalar sensors.
> > + *		  Set to 0 if not reported by this sensor.
> > + */
> >  struct scmi_sensor_info {
> > -	u32 id;
> > -	u8 type;
> > -	s8 scale;
> > -	u8 num_trip_points;
> > +	unsigned int id;
> > +	unsigned int type;
> > +	int scale;
> > +	unsigned int num_trip_points;
> >  	bool async;
> > +	bool update;
> > +	bool timestamped;
> > +	int tstamp_scale;
> > +	unsigned int num_axis;
> > +	struct scmi_sensor_axis_info *axis;
> > +	struct scmi_sensor_intervals_info intervals;
> >  	char name[SCMI_MAX_STR_SIZE];
> > +	bool extended_scalar_attrs;
> > +	unsigned int sensor_power;
> > +	unsigned int resolution;
> > +	int exponent;
> > +	struct scmi_range_attrs scalar_attrs;
> >  };
> >
> 
> Below changes can go in a separate patch as it is just update of entires
> as per latest SCMIv2.0 spec and not entirely related to v3.0.
> 

Sure.

Thanks

Cristian

> >  /*
> >   * Partial list from Distributed Management Task Force (DMTF) specification:
> > - * DSP0249 (Platform Level Data Model specification)
> > + * DSP0248 (Platform Level Data Model for Platform Monitoring and Control
> > + * specification)
> >   */
> 
> --
> Regards,
> Sudeep

Re: [PATCH v3 3/6] hwmon: scmi: update hwmon internal scale data type

[Cristian Marussi]

On Thu, Nov 19, 2020 at 11:40:29AM +0000, Sudeep Holla wrote:
> On Wed, Nov 18, 2020 at 04:29:02PM +0000, Cristian Marussi wrote:
> > Use an int to calculate scale values inside scmi_hwmon_scale() to match
> > the updated scale data type in struct scmi_sensor_info.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/hwmon/scmi-hwmon.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> > index 09ce30cba54b..17d064e58938 100644
> > --- a/drivers/hwmon/scmi-hwmon.c
> > +++ b/drivers/hwmon/scmi-hwmon.c
> > @@ -30,7 +30,7 @@ static inline u64 __pow10(u8 x)
> >  
> >  static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value)
> >  {
> > -	s8 scale = sensor->scale;
> > +	int scale = sensor->scale;
> 
> Can this go independent of the series ? You haven't cc-ed the hwmon maintainer
> and the list. We need their ack if we have to take it as part of the series.
> 
Sure, I did not know in fact what's better to in this case (maybe
bouncing/fwd the single patch ? CC the whole series ? ).
I'll split and send to the maintainer

Cheers

Cristian

> -- 
> Regards,
> Sudeep

Re: [PATCH v3 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads

[Cristian Marussi]

On Thu, Nov 19, 2020 at 11:42:16AM +0000, Sudeep Holla wrote:
> On Wed, Nov 18, 2020 at 04:29:03PM +0000, Cristian Marussi wrote:
> > Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0
> > timestamped reads.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > V2 --> v3
> > - setting rx_size to 0 in sensor_reading_get to allow fw to send
> >   both v2 and v3 replies...even if sensor_reading_get() only handles
> >   v2 spec and returns one single value
> > - using get_unaligned_le64 in lieu of le64_to_cpu
> > - removed refs to v2.1
> > ---
> >  drivers/firmware/arm_scmi/sensors.c | 137 ++++++++++++++++++++++++++--
> >  include/linux/scmi_protocol.h       |  22 +++++
> >  2 files changed, 152 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> > index 1c83aaae0012..0adc545116a4 100644
> > --- a/drivers/firmware/arm_scmi/sensors.c
> > +++ b/drivers/firmware/arm_scmi/sensors.c
> > @@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get {
> >  #define SENSOR_READ_ASYNC	BIT(0)
> >  };
> >  
> > +struct scmi_resp_sensor_reading_get {
> > +	__le64 readings;
> 
> Generally I have avoided such single element structures so far. Any
> particular reasons for having it ?
> 
 Just because there are a few of them so I found it easy and preferable
 to have some typing to distinguish them, but I can drop it.

 Cristian

> -- 
> Regards,
> Sudeep

Re: [PATCH v3 3/6] hwmon: scmi: update hwmon internal scale data type

[Sudeep Holla]

On Thu, Nov 19, 2020 at 12:22:49PM +0000, Cristian Marussi wrote:
> On Thu, Nov 19, 2020 at 11:40:29AM +0000, Sudeep Holla wrote:
> > On Wed, Nov 18, 2020 at 04:29:02PM +0000, Cristian Marussi wrote:
> > > Use an int to calculate scale values inside scmi_hwmon_scale() to match
> > > the updated scale data type in struct scmi_sensor_info.
> > > 
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > >  drivers/hwmon/scmi-hwmon.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> > > index 09ce30cba54b..17d064e58938 100644
> > > --- a/drivers/hwmon/scmi-hwmon.c
> > > +++ b/drivers/hwmon/scmi-hwmon.c
> > > @@ -30,7 +30,7 @@ static inline u64 __pow10(u8 x)
> > >  
> > >  static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value)
> > >  {
> > > -	s8 scale = sensor->scale;
> > > +	int scale = sensor->scale;
> > 
> > Can this go independent of the series ? You haven't cc-ed the hwmon maintainer
> > and the list. We need their ack if we have to take it as part of the series.
> > 
> Sure, I did not know in fact what's better to in this case (maybe
> bouncing/fwd the single patch ? CC the whole series ? ).
> I'll split and send to the maintainer

I see this can be independent of the series. I can provide my review/ack
tag once the maintainer is in cc. No point giving that here 😄 

-- 
Regards,
Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions

[Sudeep Holla]

On Thu, Nov 19, 2020 at 12:20:29PM +0000, Cristian Marussi wrote:
> Hi Sudeep
>

[...]

> > > +					S32_EXT(SENSOR_RES_EXP(ares));
> > > +				dsize += sizeof(adesc->resolution);
> > > +
> > > +				scmi_parse_range_attrs(&a->attrs,
> > > +						       &adesc->attrs);
> > > +				dsize += sizeof(adesc->attrs);
> > > +			}
> > > +
> > > +			adesc = (typeof(adesc))((u8 *)adesc + dsize);
> >
> > Just thinking if we can avoid this my having union comprising of v1 and v2
> > structures ?
> >
>
> Not sure to understand, axis_descr are only v3.0 and in fact this is
> called only for v > 2 I think, BUT the problem is that both this and the
> main sensor descriptor v3 msg payloads are runtime variable, so that it
> is stated in the msg->extended_attrs itself if that particular sensor desc
> response that I'm parsing has the additional extended fields or not:
> so the dance with dsize to keep track of where the current response ends
> and when the next starts...but maybe I've not got really what you meant.
>

No worries, we can always improve later if possible, you can keep it as
for now.

[...]

> > > +				 * retrieved via a dedicated (optional) command.
> > > +				 * Since the command is optional, on error carry
> > > +				 * on without any update interval.
> > > +				 */
> > > +				if (scmi_sensor_update_intervals(handle, s))
> > > +					dev_info(handle->dev,
> > > +						 "Update Intervals not available for sensor ID:%d\n",
> > > +						 s->id);
> >
> > Can we drop the logging or make it _dbg ? Make flood in a system with 100s of
> > sensors.
> >
>
> Sure, I was wondering in fact what to do with this: because the command
> is optional but it seemed odd to me that an SCMIv3.0 sensor does not
> expose any update interval so I wanted to log somehow this anomaly.
> (but maybe it's just not an anomaly)
>

Anything optional can never be anomaly, there is high chance that f/w
authors will drop it as it is optional unless it is absolutely necessary.

--
Regards,
Sudeep