Re: [PATCH v2 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
On Tue, Nov 10, 2020 at 06:50:04PM +0100, Peter Hilber wrote:
> Hi Cristian,
>
> sorry, I mistakenly used the wrong sender ("Mailing Lists") for the
> original comment mail. Please see below for my reply.
>
> On 10.11.20 18:21, Cristian Marussi wrote:
> > On Tue, Nov 10, 2020 at 05:00:05PM +0100, Mailing Lists wrote:
> >> On 26.10.20 21:10, 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.
> >>>
> >>> Signed-off-by: Cristian Marussi
> >>> ---
> >>> 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 | 219 +++-
> >>> 2 files changed, 584 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/drivers/firmware/arm_scmi/sensors.c
> >>> b/drivers/firmware/arm_scmi/sensors.c
> >>> index 6aaff478d032..5a18f8c84bef 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
> >>> #include
> >>>
> >>> #include "common.h"
> >>> #include "notify.h"
> >>>
> >>> +#define SCMI_MAX_NUM_SENSOR_AXIS 64
> >>
> >> IMHO the related 6 bit wide fields, like SENSOR_DESCRIPTION_GET "Number
> >> of axes", should determine the maximum value, so 64 -> 63.
> >>
> >
> > Yes, bits [21:16] 'Number of Axes' in sensor_attributes_high, but this
> > #define was meant to represent the maximum number of sensor axis
> > (64...ranging
> > from 0 to 63) not the highest possible numbered (63).
> >
>
> But in my understanding the actual maximum number of sensor axes is 63
> due to the maximum value 63 of 'Number of Axes', 64 would overflow
> already. The ids would range from 0 to 62.
Ah damn, you're right ... maximum that I can set in 5 bits is anyway 63.
I'll fix.
Thanks
Cristian
>
> That said, in my understanding there is no problem with retaining a
> higher value ATM.
>
> Best regards,
>
> Peter
Re: [PATCH v2 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
Hi Cristian,
sorry, I mistakenly used the wrong sender ("Mailing Lists") for the
original comment mail. Please see below for my reply.
On 10.11.20 18:21, Cristian Marussi wrote:
> On Tue, Nov 10, 2020 at 05:00:05PM +0100, Mailing Lists wrote:
>> On 26.10.20 21:10, 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.
>>>
>>> Signed-off-by: Cristian Marussi
>>> ---
>>> 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 | 219 +++-
>>> 2 files changed, 584 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/sensors.c
>>> b/drivers/firmware/arm_scmi/sensors.c
>>> index 6aaff478d032..5a18f8c84bef 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
>>> #include
>>>
>>> #include "common.h"
>>> #include "notify.h"
>>>
>>> +#define SCMI_MAX_NUM_SENSOR_AXIS 64
>>
>> IMHO the related 6 bit wide fields, like SENSOR_DESCRIPTION_GET "Number
>> of axes", should determine the maximum value, so 64 -> 63.
>>
>
> Yes, bits [21:16] 'Number of Axes' in sensor_attributes_high, but this
> #define was meant to represent the maximum number of sensor axis (64...ranging
> from 0 to 63) not the highest possible numbered (63).
>
But in my understanding the actual maximum number of sensor axes is 63
due to the maximum value 63 of 'Number of Axes', 64 would overflow
already. The ids would range from 0 to 62.
That said, in my understanding there is no problem with retaining a
higher value ATM.
Best regards,
Peter
Re: [PATCH v2 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
On Tue, Nov 10, 2020 at 05:00:05PM +0100, Mailing Lists wrote:
> On 26.10.20 21:10, 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.
> >
> > Signed-off-by: Cristian Marussi
> > ---
> > 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 | 219 +++-
> > 2 files changed, 584 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/sensors.c
> > b/drivers/firmware/arm_scmi/sensors.c
> > index 6aaff478d032..5a18f8c84bef 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
> > #include
> >
> > #include "common.h"
> > #include "notify.h"
> >
> > +#define SCMI_MAX_NUM_SENSOR_AXIS 64
>
> IMHO the related 6 bit wide fields, like SENSOR_DESCRIPTION_GET "Number
> of axes", should determine the maximum value, so 64 -> 63.
>
Yes, bits [21:16] 'Number of Axes' in sensor_attributes_high, but this
#define was meant to represent the maximum number of sensor axis (64...ranging
from 0 to 63) not the highest possible numbered (63).
> [...]
>
> > +
> > +/**
> > + * 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.
>
> Above line should go to @resolution below.
>
Ah, right.
Regards
Cristian
> Best regards,
>
> Peter
>
> > + * Set to 0 if not reported by this sensor.
> > + * @resolution: Extended attribute representing the resolution of the
> > sensor.
> > + * 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;
> > };
Re: [PATCH v2 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
On 26.10.20 21:10, 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.
>
> Signed-off-by: Cristian Marussi
> ---
> 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 | 219 +++-
> 2 files changed, 584 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c
> b/drivers/firmware/arm_scmi/sensors.c
> index 6aaff478d032..5a18f8c84bef 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
> #include
>
> #include "common.h"
> #include "notify.h"
>
> +#define SCMI_MAX_NUM_SENSOR_AXIS 64
IMHO the related 6 bit wide fields, like SENSOR_DESCRIPTION_GET "Number
of axes", should determine the maximum value, so 64 -> 63.
[...]
> +
> +/**
> + * 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.
Above line should go to @resolution below.
Best regards,
Peter
> + * Set to 0 if not reported by this sensor.
> + * @resolution: Extended attribute representing the resolution of the sensor.
> + * 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;
> };
[PATCH v2 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
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
---
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 | 219 +++-
2 files changed, 584 insertions(+), 26 deletions(-)
diff --git a/drivers/firmware/arm_scmi/sensors.c
b/drivers/firmware/arm_scmi/sensors.c
index 6aaff478d032..5a18f8c84bef 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
#include
#include "common.h"
#include "notify.h"
+#define SCMI_MAX_NUM_SENSOR_AXIS 64
+
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_EXTENDGENMASK(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_EXTENDGENMASK(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 */
+#defineS32_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 scm
