Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* SCMI sensor reads unit scaling
@ 2019-04-20 16:43 Florian Fainelli
  2019-04-20 17:31 ` Guenter Roeck
  2019-04-23 13:06 ` Sudeep Holla
  0 siblings, 2 replies; 5+ messages in thread
From: Florian Fainelli @ 2019-04-20 16:43 UTC (permalink / raw)
  To: Sudeep Holla, Guenter Roeck, linux-hwmon,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

Hi Sudeep, Guenter,

The current SCMI hwmon support does not seem to make use of the sensor
scale/unit as defined in the sensor replies (or the update scale for
that matter).

I came up with the patch below which gets the job done, but I am worried
about possibly breaking people's SCMI deployments and sensors reading
because they may have intentionally or not already decided to return a
value which is scaled the way Linux's hwmon expect it, and may, or may
not have populated a valid unit number in the sensor reply.

Ideally we should probably do two conversions:

- from within scmi_sensor_reading_get(), scale the value as indicated by
the reply
- from within scmi_hwmon_read(), take that scaled value and apply the
necessary conversion expected by Linux's HWMON conventions (e.g.:
reporting voltage as mV values)

What do you think?

---
From: Florian Fainelli <f.fainelli@gmail.com>
Subject: firmware: arm_scmi: Support sensor scaling

The SCMI sensor management protocol includes the following provision:

The power-of-10 multiplier in two’s-complement format that is applied to
the sensor unit specified by the SensorType field.

Add support for scaling the value returned based on what is provided by
the firmware. This requires us to be able to look up a sensor identifier
to its backing scmi_sensor_info structure and apply the necessary scale.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/firmware/arm_scmi/sensors.c | 37
++++++++++++++++++++++++++++++++++++-
 include/linux/scmi_protocol.h       |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c
b/drivers/firmware/arm_scmi/sensors.c
index bbb469f..2eccc6a 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -33,7 +33,8 @@ struct scmi_msg_resp_sensor_description {
 #define NUM_TRIP_POINTS(x)	(((x) >> 4) & 0xff)
 		__le32 attributes_high;
 #define SENSOR_TYPE(x)		((x) & 0xff)
-#define SENSOR_SCALE(x)		(((x) >> 11) & 0x3f)
+#define SENSOR_SCALE_MASK	0x3f
+#define SENSOR_SCALE(x)		(((x) >> 11) & SENSOR_SCALE_MASK)
 #define SENSOR_UPDATE_SCALE(x)	(((x) >> 22) & 0x1f)
 #define SENSOR_UPDATE_BASE(x)	(((x) >> 27) & 0x1f)
 		    u8 name[SCMI_MAX_STR_SIZE];
@@ -72,6 +73,35 @@ struct sensors_info {
 	struct scmi_sensor_info *sensors;
 };

+static
+struct scmi_sensor_info *scmi_sensor_info_from_id(struct sensors_info *si,
+						  u32 sensor_id)
+{
+	struct scmi_sensor_info *s = NULL;
+	int i;
+
+	for (i = 0; i < si->num_sensors; i++) {
+		s = &si->sensors[i];
+		if (s->id == sensor_id)
+			return s;
+	}
+
+	return s;
+}
+
+static void inline scmi_scale_value(u8 scale, u64 *value)
+{
+	unsigned int i;
+
+	if (scale & ((SENSOR_SCALE_MASK + 1) >> 1)) {
+		for (i = 0; i < (~scale) + 1; i++)
+			*value /= 10;
+	} else {
+		for (i = 0; i < scale; i++)
+			*value *= 10;
+	}
+}
+
 static int scmi_sensor_attributes_get(const struct scmi_handle *handle,
 				      struct sensors_info *si)
 {
@@ -140,6 +170,7 @@ static int scmi_sensor_description_get(const struct
scmi_handle *handle,
 			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);
 			memcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
 		}

@@ -205,6 +236,8 @@ static int scmi_sensor_trip_point_set(const struct
scmi_handle *handle,
 static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 				   u32 sensor_id, bool async, u64 *value)
 {
+	struct sensors_info *si = handle->sensor_priv;
+	struct scmi_sensor_info *s = scmi_sensor_info_from_id(si, sensor_id);
 	int ret;
 	struct scmi_xfer *t;
 	struct scmi_msg_sensor_reading_get *sensor;
@@ -225,6 +258,8 @@ static int scmi_sensor_reading_get(const struct
scmi_handle *handle,

 		*value = le32_to_cpu(*pval);
 		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
+		if (s && s->scale)
+			scmi_scale_value(s->scale, value);
 	}

 	scmi_one_xfer_put(handle, t);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b458c87..782b53e 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -140,6 +140,7 @@ struct scmi_power_ops {
 struct scmi_sensor_info {
 	u32 id;
 	u8 type;
+	u8 scale;
 	char name[SCMI_MAX_STR_SIZE];
 };

-- 
-- 
Florian

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: SCMI sensor reads unit scaling
  2019-04-20 16:43 SCMI sensor reads unit scaling Florian Fainelli
@ 2019-04-20 17:31 ` Guenter Roeck
  2019-04-22 18:31   ` Florian Fainelli
  2019-04-23 13:06 ` Sudeep Holla
  1 sibling, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2019-04-20 17:31 UTC (permalink / raw)
  To: Florian Fainelli, Sudeep Holla, linux-hwmon,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On 4/20/19 9:43 AM, Florian Fainelli wrote:
> Hi Sudeep, Guenter,
> 
> The current SCMI hwmon support does not seem to make use of the sensor
> scale/unit as defined in the sensor replies (or the update scale for
> that matter).
> 
> I came up with the patch below which gets the job done, but I am worried
> about possibly breaking people's SCMI deployments and sensors reading
> because they may have intentionally or not already decided to return a
> value which is scaled the way Linux's hwmon expect it, and may, or may
> not have populated a valid unit number in the sensor reply.
> 
> Ideally we should probably do two conversions:
> 
> - from within scmi_sensor_reading_get(), scale the value as indicated by
> the reply
> - from within scmi_hwmon_read(), take that scaled value and apply the
> necessary conversion expected by Linux's HWMON conventions (e.g.:
> reporting voltage as mV values)
> 
> What do you think?
> 

Reported values have to follow HWMON conventions. If not, this is a bug
and needs to get fixed. The argument about not changing the userspace ABI
does not apply to bugs.

Guenter

> ---
> From: Florian Fainelli <f.fainelli@gmail.com>
> Subject: firmware: arm_scmi: Support sensor scaling
> 
> The SCMI sensor management protocol includes the following provision:
> 
> The power-of-10 multiplier in two’s-complement format that is applied to
> the sensor unit specified by the SensorType field.
> 
> Add support for scaling the value returned based on what is provided by
> the firmware. This requires us to be able to look up a sensor identifier
> to its backing scmi_sensor_info structure and apply the necessary scale.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/firmware/arm_scmi/sensors.c | 37
> ++++++++++++++++++++++++++++++++++++-
>   include/linux/scmi_protocol.h       |  1 +
>   2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c
> b/drivers/firmware/arm_scmi/sensors.c
> index bbb469f..2eccc6a 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -33,7 +33,8 @@ struct scmi_msg_resp_sensor_description {
>   #define NUM_TRIP_POINTS(x)	(((x) >> 4) & 0xff)
>   		__le32 attributes_high;
>   #define SENSOR_TYPE(x)		((x) & 0xff)
> -#define SENSOR_SCALE(x)		(((x) >> 11) & 0x3f)
> +#define SENSOR_SCALE_MASK	0x3f
> +#define SENSOR_SCALE(x)		(((x) >> 11) & SENSOR_SCALE_MASK)
>   #define SENSOR_UPDATE_SCALE(x)	(((x) >> 22) & 0x1f)
>   #define SENSOR_UPDATE_BASE(x)	(((x) >> 27) & 0x1f)
>   		    u8 name[SCMI_MAX_STR_SIZE];
> @@ -72,6 +73,35 @@ struct sensors_info {
>   	struct scmi_sensor_info *sensors;
>   };
> 
> +static
> +struct scmi_sensor_info *scmi_sensor_info_from_id(struct sensors_info *si,
> +						  u32 sensor_id)
> +{
> +	struct scmi_sensor_info *s = NULL;
> +	int i;
> +
> +	for (i = 0; i < si->num_sensors; i++) {
> +		s = &si->sensors[i];
> +		if (s->id == sensor_id)
> +			return s;
> +	}
> +
> +	return s;
> +}
> +
> +static void inline scmi_scale_value(u8 scale, u64 *value)
> +{
> +	unsigned int i;
> +
> +	if (scale & ((SENSOR_SCALE_MASK + 1) >> 1)) {
> +		for (i = 0; i < (~scale) + 1; i++)
> +			*value /= 10;
> +	} else {
> +		for (i = 0; i < scale; i++)
> +			*value *= 10;
> +	}
> +}
> +
>   static int scmi_sensor_attributes_get(const struct scmi_handle *handle,
>   				      struct sensors_info *si)
>   {
> @@ -140,6 +170,7 @@ static int scmi_sensor_description_get(const struct
> scmi_handle *handle,
>   			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);
>   			memcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
>   		}
> 
> @@ -205,6 +236,8 @@ static int scmi_sensor_trip_point_set(const struct
> scmi_handle *handle,
>   static int scmi_sensor_reading_get(const struct scmi_handle *handle,
>   				   u32 sensor_id, bool async, u64 *value)
>   {
> +	struct sensors_info *si = handle->sensor_priv;
> +	struct scmi_sensor_info *s = scmi_sensor_info_from_id(si, sensor_id);
>   	int ret;
>   	struct scmi_xfer *t;
>   	struct scmi_msg_sensor_reading_get *sensor;
> @@ -225,6 +258,8 @@ static int scmi_sensor_reading_get(const struct
> scmi_handle *handle,
> 
>   		*value = le32_to_cpu(*pval);
>   		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
> +		if (s && s->scale)
> +			scmi_scale_value(s->scale, value);
>   	}
> 
>   	scmi_one_xfer_put(handle, t);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index b458c87..782b53e 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -140,6 +140,7 @@ struct scmi_power_ops {
>   struct scmi_sensor_info {
>   	u32 id;
>   	u8 type;
> +	u8 scale;
>   	char name[SCMI_MAX_STR_SIZE];
>   };
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: SCMI sensor reads unit scaling
  2019-04-20 17:31 ` Guenter Roeck
@ 2019-04-22 18:31   ` Florian Fainelli
  2019-04-22 19:29     ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2019-04-22 18:31 UTC (permalink / raw)
  To: Guenter Roeck, Sudeep Holla, linux-hwmon,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On 4/20/19 10:31 AM, Guenter Roeck wrote:
> On 4/20/19 9:43 AM, Florian Fainelli wrote:
>> Hi Sudeep, Guenter,
>>
>> The current SCMI hwmon support does not seem to make use of the sensor
>> scale/unit as defined in the sensor replies (or the update scale for
>> that matter).
>>
>> I came up with the patch below which gets the job done, but I am worried
>> about possibly breaking people's SCMI deployments and sensors reading
>> because they may have intentionally or not already decided to return a
>> value which is scaled the way Linux's hwmon expect it, and may, or may
>> not have populated a valid unit number in the sensor reply.
>>
>> Ideally we should probably do two conversions:
>>
>> - from within scmi_sensor_reading_get(), scale the value as indicated by
>> the reply
>> - from within scmi_hwmon_read(), take that scaled value and apply the
>> necessary conversion expected by Linux's HWMON conventions (e.g.:
>> reporting voltage as mV values)
>>
>> What do you think?
>>
> 
> Reported values have to follow HWMON conventions. If not, this is a bug
> and needs to get fixed. The argument about not changing the userspace ABI
> does not apply to bugs.

Upon re-reading the SCMI specification, what is not clear to me is
whether you have the liberty to:

- target the Linux HWMON sensor units as specified by
Documentation/hwmon/sysfs-interface by e.g.: putting a millidegree
Celsius value in the SCMI reply and an power of 10 multiplier value of 0
(which would be treated as not doing any scaling)

- must provide the raw value as provided by the sensor as well as
indicate the scale, e.g.: millidegree celsius and specifying a scale of -3

Sudeep can you clarify how you would approach making use (or not) of the
power of 10 multiplier as defined by the SCMI specification?

Thanks!
-- 
Florian

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: SCMI sensor reads unit scaling
  2019-04-22 18:31   ` Florian Fainelli
@ 2019-04-22 19:29     ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-04-22 19:29 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Sudeep Holla, linux-hwmon,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Mon, Apr 22, 2019 at 11:31:18AM -0700, Florian Fainelli wrote:
> On 4/20/19 10:31 AM, Guenter Roeck wrote:
> > On 4/20/19 9:43 AM, Florian Fainelli wrote:
> >> Hi Sudeep, Guenter,
> >>
> >> The current SCMI hwmon support does not seem to make use of the sensor
> >> scale/unit as defined in the sensor replies (or the update scale for
> >> that matter).
> >>
> >> I came up with the patch below which gets the job done, but I am worried
> >> about possibly breaking people's SCMI deployments and sensors reading
> >> because they may have intentionally or not already decided to return a
> >> value which is scaled the way Linux's hwmon expect it, and may, or may
> >> not have populated a valid unit number in the sensor reply.
> >>
> >> Ideally we should probably do two conversions:
> >>
> >> - from within scmi_sensor_reading_get(), scale the value as indicated by
> >> the reply
> >> - from within scmi_hwmon_read(), take that scaled value and apply the
> >> necessary conversion expected by Linux's HWMON conventions (e.g.:
> >> reporting voltage as mV values)
> >>
> >> What do you think?
> >>
> > 
> > Reported values have to follow HWMON conventions. If not, this is a bug
> > and needs to get fixed. The argument about not changing the userspace ABI
> > does not apply to bugs.
> 
> Upon re-reading the SCMI specification, what is not clear to me is
> whether you have the liberty to:
> 
> - target the Linux HWMON sensor units as specified by
> Documentation/hwmon/sysfs-interface by e.g.: putting a millidegree
> Celsius value in the SCMI reply and an power of 10 multiplier value of 0
> (which would be treated as not doing any scaling)
> 
> - must provide the raw value as provided by the sensor as well as
> indicate the scale, e.g.: millidegree celsius and specifying a scale of -3
> 
> Sudeep can you clarify how you would approach making use (or not) of the
> power of 10 multiplier as defined by the SCMI specification?
> 

Not sure I understand. SCMI and hwmon sysfs ABI requirements are orthogonal.
hwmon expects certain units (mostly milli, but micro for power). What the
hardware reports is irrelevant. The SCMI driver needs to convert values
reported by SCMI into values expected by the hwmon ABI. If it does not do
that today, that is a bug which needs to be fixed.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: SCMI sensor reads unit scaling
  2019-04-20 16:43 SCMI sensor reads unit scaling Florian Fainelli
  2019-04-20 17:31 ` Guenter Roeck
@ 2019-04-23 13:06 ` Sudeep Holla
  1 sibling, 0 replies; 5+ messages in thread
From: Sudeep Holla @ 2019-04-23 13:06 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Guenter Roeck, linux-hwmon,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Sat, Apr 20, 2019 at 09:43:48AM -0700, Florian Fainelli wrote:
> Hi Sudeep, Guenter,
>
> The current SCMI hwmon support does not seem to make use of the sensor
> scale/unit as defined in the sensor replies (or the update scale for
> that matter).
>

That sounds like a bug as I simplified(IOW removed code that I couldn't
test with the reference platform firmware) before I pushed upstream.

> I came up with the patch below which gets the job done, but I am worried
> about possibly breaking people's SCMI deployments and sensors reading
> because they may have intentionally or not already decided to return a
> value which is scaled the way Linux's hwmon expect it, and may, or may
> not have populated a valid unit number in the sensor reply.
>

OK, will try to recollect the details and get back to you, I might have
forgotten few details as it's over  1.5 years since I originally wrote
it.

> Ideally we should probably do two conversions:
>
> - from within scmi_sensor_reading_get(), scale the value as indicated by
> the reply
> - from within scmi_hwmon_read(), take that scaled value and apply the
> necessary conversion expected by Linux's HWMON conventions (e.g.:
> reporting voltage as mV values)
>

At least, the idea I had is to have all the conversion in scmi_sensors.c
and provide sensor data scale to HWMON framework's expectation. Not sure
if I implemented correctly to obey that.

> What do you think?
>
> ---
> From: Florian Fainelli <f.fainelli@gmail.com>
> Subject: firmware: arm_scmi: Support sensor scaling
>
> The SCMI sensor management protocol includes the following provision:
>
> The power-of-10 multiplier in two’s-complement format that is applied to
> the sensor unit specified by the SensorType field.
>
> Add support for scaling the value returned based on what is provided by
> the firmware. This requires us to be able to look up a sensor identifier
> to its backing scmi_sensor_info structure and apply the necessary scale.
>

Had a brief look at the patch and this is what I had initially as mentioned
before. Since the Juno firmware at that time didn't have all these implemented
properly, I dropped. But I am happy to test and merge this if it makes sense
and get the firmware fixed if I find any bugs with it ;).

However I am not sure if we need another conversion in hwmon driver as
we can scale it in scmi itself to whatever Linux HWMON expects. Let me
know if you don't agree with that.

If possible, please post this independently. Also if you can give it a
testing on your platform, that would be great. I will try to run this on
Juno too in the meantime.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-20 16:43 SCMI sensor reads unit scaling Florian Fainelli
2019-04-20 17:31 ` Guenter Roeck
2019-04-22 18:31   ` Florian Fainelli
2019-04-22 19:29     ` Guenter Roeck
2019-04-23 13:06 ` Sudeep Holla

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox