* [PATCH v4 0/2] hwmon: scmi: Scale values to target desired HWMON units @ 2019-05-08 17:00 Florian Fainelli 2019-05-08 17:00 ` [PATCH v4 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli 2019-05-08 17:00 ` [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli 0 siblings, 2 replies; 7+ messages in thread From: Florian Fainelli @ 2019-05-08 17:00 UTC (permalink / raw) To: linux-kernel Cc: bcm-kernel-feedback-list, Florian Fainelli, Sudeep Holla, Jean Delvare, Guenter Roeck, linux-arm-kernel, open list:HARDWARE MONITORING Hi Sudeep, Guenter, This patch series adds support for scaling SCMI sensor values read from firmware. Sudeep, let me know if you think we should be treating scale == 0 as a special value to preserve some firmware compatibility (not that this would be desired). Changes in v4: - deal with overflow in the caller of __pow10() as suggested by Guenter which makes us rework a bit how the value argument/return value are passed - don't harcode the latest power of 10 factor to be 18, just rely on overflowing the u64 value instead Changes in v3: - add a local __pow10 function to scmi-hwmon.c while a plan to provide a generic function is figured out. - add check on power > 18 which would overflow a 64-bit unsigned integer - use div64_u64() to properly divide a 64-bit quantity with an unsigned 64-bit quantity Changes in v2: - added a helper function in kernel.h: __pow10() - made the scale in scmi_sensor_info an s8 type, added defines for checking the sign bit and sign extending with a mask - simplify computations in hwmon driver Florian Fainelli (2): firmware: arm_scmi: Fetch and store sensor scale hwmon: scmi: Scale values to target desired HWMON units drivers/firmware/arm_scmi/sensors.c | 6 ++++ drivers/hwmon/scmi-hwmon.c | 46 +++++++++++++++++++++++++++++ include/linux/scmi_protocol.h | 1 + 3 files changed, 53 insertions(+) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/2] firmware: arm_scmi: Fetch and store sensor scale 2019-05-08 17:00 [PATCH v4 0/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli @ 2019-05-08 17:00 ` Florian Fainelli 2019-05-08 18:33 ` Guenter Roeck 2019-05-08 17:00 ` [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli 1 sibling, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2019-05-08 17:00 UTC (permalink / raw) To: linux-kernel Cc: bcm-kernel-feedback-list, Florian Fainelli, Sudeep Holla, Jean Delvare, Guenter Roeck, linux-arm-kernel, open list:HARDWARE MONITORING In preparation for dealing with scales within the SCMI HWMON driver, fetch and store the sensor unit scale into the scmi_sensor_info structure. In order to simplify computations for upper layer, take care of sign extending the scale to a full 8-bit signed value. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/firmware/arm_scmi/sensors.c | 6 ++++++ include/linux/scmi_protocol.h | 1 + 2 files changed, 7 insertions(+) diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c index b53d5cc9c9f6..21353470a740 100644 --- a/drivers/firmware/arm_scmi/sensors.c +++ b/drivers/firmware/arm_scmi/sensors.c @@ -34,6 +34,8 @@ struct scmi_msg_resp_sensor_description { __le32 attributes_high; #define SENSOR_TYPE(x) ((x) & 0xff) #define SENSOR_SCALE(x) (((x) >> 11) & 0x3f) +#define SENSOR_SCALE_SIGN BIT(5) +#define SENSOR_SCALE_EXTEND GENMASK(7, 6) #define SENSOR_UPDATE_SCALE(x) (((x) >> 22) & 0x1f) #define SENSOR_UPDATE_BASE(x) (((x) >> 27) & 0x1f) u8 name[SCMI_MAX_STR_SIZE]; @@ -140,6 +142,10 @@ 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); + /* Sign extend to a full s8 */ + if (s->scale & SENSOR_SCALE_SIGN) + s->scale |= SENSOR_SCALE_EXTEND; strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE); } diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index 3105055c00a7..9ff2e9357e9a 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h @@ -144,6 +144,7 @@ struct scmi_power_ops { struct scmi_sensor_info { u32 id; u8 type; + s8 scale; char name[SCMI_MAX_STR_SIZE]; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] firmware: arm_scmi: Fetch and store sensor scale 2019-05-08 17:00 ` [PATCH v4 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli @ 2019-05-08 18:33 ` Guenter Roeck 0 siblings, 0 replies; 7+ messages in thread From: Guenter Roeck @ 2019-05-08 18:33 UTC (permalink / raw) To: Florian Fainelli Cc: linux-kernel, bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare, linux-arm-kernel, open list:HARDWARE MONITORING On Wed, May 08, 2019 at 10:00:34AM -0700, Florian Fainelli wrote: > In preparation for dealing with scales within the SCMI HWMON driver, > fetch and store the sensor unit scale into the scmi_sensor_info > structure. In order to simplify computations for upper layer, take care > of sign extending the scale to a full 8-bit signed value. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/firmware/arm_scmi/sensors.c | 6 ++++++ > include/linux/scmi_protocol.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c > index b53d5cc9c9f6..21353470a740 100644 > --- a/drivers/firmware/arm_scmi/sensors.c > +++ b/drivers/firmware/arm_scmi/sensors.c > @@ -34,6 +34,8 @@ struct scmi_msg_resp_sensor_description { > __le32 attributes_high; > #define SENSOR_TYPE(x) ((x) & 0xff) > #define SENSOR_SCALE(x) (((x) >> 11) & 0x3f) > +#define SENSOR_SCALE_SIGN BIT(5) > +#define SENSOR_SCALE_EXTEND GENMASK(7, 6) > #define SENSOR_UPDATE_SCALE(x) (((x) >> 22) & 0x1f) > #define SENSOR_UPDATE_BASE(x) (((x) >> 27) & 0x1f) > u8 name[SCMI_MAX_STR_SIZE]; > @@ -140,6 +142,10 @@ 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); > + /* Sign extend to a full s8 */ > + if (s->scale & SENSOR_SCALE_SIGN) > + s->scale |= SENSOR_SCALE_EXTEND; > strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE); > } > > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > index 3105055c00a7..9ff2e9357e9a 100644 > --- a/include/linux/scmi_protocol.h > +++ b/include/linux/scmi_protocol.h > @@ -144,6 +144,7 @@ struct scmi_power_ops { > struct scmi_sensor_info { > u32 id; > u8 type; > + s8 scale; > char name[SCMI_MAX_STR_SIZE]; > }; > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units 2019-05-08 17:00 [PATCH v4 0/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli 2019-05-08 17:00 ` [PATCH v4 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli @ 2019-05-08 17:00 ` Florian Fainelli 2019-05-08 18:32 ` Guenter Roeck 1 sibling, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2019-05-08 17:00 UTC (permalink / raw) To: linux-kernel Cc: bcm-kernel-feedback-list, Florian Fainelli, Sudeep Holla, Jean Delvare, Guenter Roeck, linux-arm-kernel, open list:HARDWARE MONITORING If the SCMI firmware implementation is reporting values in a scale that is different from the HWMON units, we need to scale up or down the value according to how far appart they are. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c index a80183a488c5..4399372e2131 100644 --- a/drivers/hwmon/scmi-hwmon.c +++ b/drivers/hwmon/scmi-hwmon.c @@ -7,6 +7,7 @@ */ #include <linux/hwmon.h> +#include <linux/limits.h> #include <linux/module.h> #include <linux/scmi_protocol.h> #include <linux/slab.h> @@ -18,6 +19,47 @@ struct scmi_sensors { const struct scmi_sensor_info **info[hwmon_max]; }; +static inline u64 __pow10(u8 x) +{ + u64 r = 1; + + while (x--) + r *= 10; + + return r; +} + +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) +{ + s8 scale = sensor->scale; + u64 f; + + switch (sensor->type) { + case TEMPERATURE_C: + case VOLTAGE: + case CURRENT: + scale += 3; + break; + case POWER: + case ENERGY: + scale += 6; + break; + default: + break; + } + + f = __pow10(abs(scale)); + if (f == U64_MAX) + return -E2BIG; + + if (scale > 0) + *value *= f; + else + *value = div64_u64(*value, f); + + return 0; +} + static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, long *val) { @@ -29,6 +71,10 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, sensor = *(scmi_sensors->info[type] + channel); ret = h->sensor_ops->reading_get(h, sensor->id, false, &value); + if (ret) + return ret; + + ret = scmi_hwmon_scale(sensor, value); if (!ret) *val = value; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units 2019-05-08 17:00 ` [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli @ 2019-05-08 18:32 ` Guenter Roeck 2019-05-08 18:34 ` Florian Fainelli 0 siblings, 1 reply; 7+ messages in thread From: Guenter Roeck @ 2019-05-08 18:32 UTC (permalink / raw) To: Florian Fainelli Cc: linux-kernel, bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare, linux-arm-kernel, open list:HARDWARE MONITORING Hi Florian, On Wed, May 08, 2019 at 10:00:35AM -0700, Florian Fainelli wrote: > If the SCMI firmware implementation is reporting values in a scale that > is different from the HWMON units, we need to scale up or down the value > according to how far appart they are. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > index a80183a488c5..4399372e2131 100644 > --- a/drivers/hwmon/scmi-hwmon.c > +++ b/drivers/hwmon/scmi-hwmon.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/hwmon.h> > +#include <linux/limits.h> > #include <linux/module.h> > #include <linux/scmi_protocol.h> > #include <linux/slab.h> > @@ -18,6 +19,47 @@ struct scmi_sensors { > const struct scmi_sensor_info **info[hwmon_max]; > }; > > +static inline u64 __pow10(u8 x) > +{ > + u64 r = 1; > + > + while (x--) > + r *= 10; > + > + return r; > +} > + > +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) > +{ > + s8 scale = sensor->scale; > + u64 f; > + > + switch (sensor->type) { > + case TEMPERATURE_C: > + case VOLTAGE: > + case CURRENT: > + scale += 3; > + break; > + case POWER: > + case ENERGY: > + scale += 6; > + break; > + default: > + break; > + } > + > + f = __pow10(abs(scale)); > + if (f == U64_MAX) > + return -E2BIG; Unfortunately that is not how integer overflows work. A test program with increasing values of scale reports: 0: 1 ... 18: 1000000000000000000 19: 10000000000000000000 20: 7766279631452241920 21: 3875820019684212736 22: 1864712049423024128 23: 200376420520689664 24: 2003764205206896640 ... 61: 11529215046068469760 62: 4611686018427387904 63: 9223372036854775808 64: 0 ... You'll have to check for abs(scale) > 19 if you want to report overflows. Guenter > + > + if (scale > 0) > + *value *= f; > + else > + *value = div64_u64(*value, f); > + > + return 0; > +} > + > static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > u32 attr, int channel, long *val) > { > @@ -29,6 +71,10 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > sensor = *(scmi_sensors->info[type] + channel); > ret = h->sensor_ops->reading_get(h, sensor->id, false, &value); > + if (ret) > + return ret; > + > + ret = scmi_hwmon_scale(sensor, value); > if (!ret) > *val = value; > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units 2019-05-08 18:32 ` Guenter Roeck @ 2019-05-08 18:34 ` Florian Fainelli 2019-05-08 19:40 ` Guenter Roeck 0 siblings, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2019-05-08 18:34 UTC (permalink / raw) To: Guenter Roeck, Florian Fainelli Cc: linux-kernel, bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare, linux-arm-kernel, open list:HARDWARE MONITORING On 5/8/19 11:32 AM, Guenter Roeck wrote: > Hi Florian, > > On Wed, May 08, 2019 at 10:00:35AM -0700, Florian Fainelli wrote: >> If the SCMI firmware implementation is reporting values in a scale that >> is different from the HWMON units, we need to scale up or down the value >> according to how far appart they are. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c >> index a80183a488c5..4399372e2131 100644 >> --- a/drivers/hwmon/scmi-hwmon.c >> +++ b/drivers/hwmon/scmi-hwmon.c >> @@ -7,6 +7,7 @@ >> */ >> >> #include <linux/hwmon.h> >> +#include <linux/limits.h> >> #include <linux/module.h> >> #include <linux/scmi_protocol.h> >> #include <linux/slab.h> >> @@ -18,6 +19,47 @@ struct scmi_sensors { >> const struct scmi_sensor_info **info[hwmon_max]; >> }; >> >> +static inline u64 __pow10(u8 x) >> +{ >> + u64 r = 1; >> + >> + while (x--) >> + r *= 10; >> + >> + return r; >> +} >> + >> +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) >> +{ >> + s8 scale = sensor->scale; >> + u64 f; >> + >> + switch (sensor->type) { >> + case TEMPERATURE_C: >> + case VOLTAGE: >> + case CURRENT: >> + scale += 3; >> + break; >> + case POWER: >> + case ENERGY: >> + scale += 6; >> + break; >> + default: >> + break; >> + } >> + >> + f = __pow10(abs(scale)); >> + if (f == U64_MAX) >> + return -E2BIG; > > Unfortunately that is not how integer overflows work. > > A test program with increasing values of scale reports: > > 0: 1 > ... > 18: 1000000000000000000 > 19: 10000000000000000000 > 20: 7766279631452241920 > 21: 3875820019684212736 > 22: 1864712049423024128 > 23: 200376420520689664 > 24: 2003764205206896640 > ... > 61: 11529215046068469760 > 62: 4611686018427387904 > 63: 9223372036854775808 > 64: 0 > ... > > You'll have to check for abs(scale) > 19 if you want to report overflows. Yes silly me, my test program was flawed, thanks for pointing out that. You are okay with returning E2BIG when we overflow? -- Florian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units 2019-05-08 18:34 ` Florian Fainelli @ 2019-05-08 19:40 ` Guenter Roeck 0 siblings, 0 replies; 7+ messages in thread From: Guenter Roeck @ 2019-05-08 19:40 UTC (permalink / raw) To: Florian Fainelli Cc: linux-kernel, bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare, linux-arm-kernel, open list:HARDWARE MONITORING On Wed, May 08, 2019 at 11:34:50AM -0700, Florian Fainelli wrote: > On 5/8/19 11:32 AM, Guenter Roeck wrote: > > Hi Florian, > > > > On Wed, May 08, 2019 at 10:00:35AM -0700, Florian Fainelli wrote: > >> If the SCMI firmware implementation is reporting values in a scale that > >> is different from the HWMON units, we need to scale up or down the value > >> according to how far appart they are. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 46 insertions(+) > >> > >> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > >> index a80183a488c5..4399372e2131 100644 > >> --- a/drivers/hwmon/scmi-hwmon.c > >> +++ b/drivers/hwmon/scmi-hwmon.c > >> @@ -7,6 +7,7 @@ > >> */ > >> > >> #include <linux/hwmon.h> > >> +#include <linux/limits.h> > >> #include <linux/module.h> > >> #include <linux/scmi_protocol.h> > >> #include <linux/slab.h> > >> @@ -18,6 +19,47 @@ struct scmi_sensors { > >> const struct scmi_sensor_info **info[hwmon_max]; > >> }; > >> > >> +static inline u64 __pow10(u8 x) > >> +{ > >> + u64 r = 1; > >> + > >> + while (x--) > >> + r *= 10; > >> + > >> + return r; > >> +} > >> + > >> +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) > >> +{ > >> + s8 scale = sensor->scale; > >> + u64 f; > >> + > >> + switch (sensor->type) { > >> + case TEMPERATURE_C: > >> + case VOLTAGE: > >> + case CURRENT: > >> + scale += 3; > >> + break; > >> + case POWER: > >> + case ENERGY: > >> + scale += 6; > >> + break; > >> + default: > >> + break; > >> + } > >> + > >> + f = __pow10(abs(scale)); > >> + if (f == U64_MAX) > >> + return -E2BIG; > > > > Unfortunately that is not how integer overflows work. > > > > A test program with increasing values of scale reports: > > > > 0: 1 > > ... > > 18: 1000000000000000000 > > 19: 10000000000000000000 > > 20: 7766279631452241920 > > 21: 3875820019684212736 > > 22: 1864712049423024128 > > 23: 200376420520689664 > > 24: 2003764205206896640 > > ... > > 61: 11529215046068469760 > > 62: 4611686018427387904 > > 63: 9223372036854775808 > > 64: 0 > > ... > > > > You'll have to check for abs(scale) > 19 if you want to report overflows. > > Yes silly me, my test program was flawed, thanks for pointing out that. > You are okay with returning E2BIG when we overflow? Yes. Thanks, Guenter ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-08 19:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-08 17:00 [PATCH v4 0/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli 2019-05-08 17:00 ` [PATCH v4 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli 2019-05-08 18:33 ` Guenter Roeck 2019-05-08 17:00 ` [PATCH v4 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli 2019-05-08 18:32 ` Guenter Roeck 2019-05-08 18:34 ` Florian Fainelli 2019-05-08 19:40 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).