| Message ID | 20240125064422.347002-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V3] hwmon: scmi-hwmon: implement change_mode for thermal zones | expand |
On 1/24/24 22:44, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > The thermal sensors maybe disabled before kernel boot, so add change_mode > for thermal zones to support configuring the thermal sensor to enabled > state. If reading the temperature when the sensor is disabled, there will > be error reported. > > The cost is an extra config_get all to SCMI firmware to get the status > of the thermal sensor. No function level impact. > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > > V3: > Update commit log to show it only applys to thermal > Add comments in code > Add R-b from Cristian > You didn't address my question regarding the behavior of hwmon attributes if a sensor is disabled. > Guenter, I Cced linux@roeck-us.net when sending V1/V2 > Let me Cc Guenter Roeck <groeck7@gmail.com> in V3, hope you not mind > This time I received it twice ;-). > V2: > Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update config(Thanks Cristian) > > drivers/hwmon/scmi-hwmon.c | 39 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > index 364199b332c0..af2267fea5f0 100644 > --- a/drivers/hwmon/scmi-hwmon.c > +++ b/drivers/hwmon/scmi-hwmon.c > @@ -151,7 +151,46 @@ static int scmi_hwmon_thermal_get_temp(struct thermal_zone_device *tz, > return ret; > } > > +static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz, > + enum thermal_device_mode new_mode) > +{ > + int ret; > + u32 config; > + enum thermal_device_mode cur_mode = THERMAL_DEVICE_DISABLED; > + struct scmi_thermal_sensor *th_sensor = thermal_zone_device_priv(tz); > + > + ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id, > + &config); > + if (ret) > + return ret; > + > + if (SCMI_SENS_CFG_IS_ENABLED(config)) > + cur_mode = THERMAL_DEVICE_ENABLED; > + > + if (cur_mode == new_mode) > + return 0; > + > + /* > + * Per SENSOR_CONFIG_SET sensor_config description: > + * BIT[31:11] should be set to 0 if the sensor update interval does > + * not need to be updated, so clear them. > + * And SENSOR_CONFIG_GET does not return round up/down, so also clear > + * BIT[10:9] round up/down. What does "clear" mean ? Is it going to round up ? Round down ? And why would it be necessary to clear those bits if SENSOR_CONFIG_GET does not return the current setting in the first place ? Thanks, Guenter > + */ > + config &= ~(SCMI_SENS_CFG_UPDATE_SECS_MASK | > + SCMI_SENS_CFG_UPDATE_EXP_MASK | > + SCMI_SENS_CFG_ROUND_MASK); > + if (new_mode == THERMAL_DEVICE_ENABLED) > + config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK; > + else > + config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK; > + > + return sensor_ops->config_set(th_sensor->ph, th_sensor->info->id, > + config); > +} > + > static const struct thermal_zone_device_ops scmi_hwmon_thermal_ops = { > + .change_mode = scmi_hwmon_thermal_change_mode, > .get_temp = scmi_hwmon_thermal_get_temp, > }; >
Hi Guenter, > Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for > thermal zones > > On 1/24/24 22:44, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > The thermal sensors maybe disabled before kernel boot, so add > > change_mode for thermal zones to support configuring the thermal > > sensor to enabled state. If reading the temperature when the sensor is > > disabled, there will be error reported. > > > > The cost is an extra config_get all to SCMI firmware to get the status > > of the thermal sensor. No function level impact. > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > > > V3: > > Update commit log to show it only applys to thermal > > Add comments in code > > Add R-b from Cristian > > > > You didn't address my question regarding the behavior of hwmon attributes if > a sensor is disabled. Would you please share a bit more on what attributes? You mean the files under /sys/class/hwmon/hwmon0? If the sensor is disabled, when cat temp[x]_input, it will report: root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp3_input cat: temp3_input: Protocol error For enabled, it will report value: root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp1_input 31900 > > > Guenter, I Cced linux@roeck-us.net when sending V1/V2 > > Let me Cc Guenter Roeck <groeck7@gmail.com> in V3, hope you not mind > > > This time I received it twice ;-). > > > V2: > > Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update > > config(Thanks Cristian) > > > > drivers/hwmon/scmi-hwmon.c | 39 > ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > > index 364199b332c0..af2267fea5f0 100644 > > --- a/drivers/hwmon/scmi-hwmon.c > > +++ b/drivers/hwmon/scmi-hwmon.c > > @@ -151,7 +151,46 @@ static int scmi_hwmon_thermal_get_temp(struct > thermal_zone_device *tz, > > return ret; > > } > > > > +static int scmi_hwmon_thermal_change_mode(struct > thermal_zone_device *tz, > > + enum thermal_device_mode > new_mode) { > > + int ret; > > + u32 config; > > + enum thermal_device_mode cur_mode = > THERMAL_DEVICE_DISABLED; > > + struct scmi_thermal_sensor *th_sensor = > > +thermal_zone_device_priv(tz); > > + > > + ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id, > > + &config); > > + if (ret) > > + return ret; > > + > > + if (SCMI_SENS_CFG_IS_ENABLED(config)) > > + cur_mode = THERMAL_DEVICE_ENABLED; > > + > > + if (cur_mode == new_mode) > > + return 0; > > + > > + /* > > + * Per SENSOR_CONFIG_SET sensor_config description: > > + * BIT[31:11] should be set to 0 if the sensor update interval does > > + * not need to be updated, so clear them. > > + * And SENSOR_CONFIG_GET does not return round up/down, so also > clear > > + * BIT[10:9] round up/down. > > What does "clear" mean ? Is it going to round up ? Round down ? And why > would it be necessary to clear those bits if SENSOR_CONFIG_GET does not > return the current setting in the first place ? This is to follow Cristian's suggestion to clear [31:9], because we only need to set the sensor to enabled state, no other attributes. My understanding is with BIT[31:11] set to 0, BIT[10:9] will not take effect. Cristian may help comment more since he suggested to clear them in V1/V2 You are right, currently config_get will return [10:2] as reserved, so config_set bit[10:9] no need touch. But config_get bit[10:2] may update to return the value in future SCMI spec? Cristian or Sudeep may help comment here. Thanks, Peng. > > Thanks, > Guenter > > > + */ > > + config &= ~(SCMI_SENS_CFG_UPDATE_SECS_MASK | > > + SCMI_SENS_CFG_UPDATE_EXP_MASK | > > + SCMI_SENS_CFG_ROUND_MASK); > > + if (new_mode == THERMAL_DEVICE_ENABLED) > > + config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK; > > + else > > + config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK; > > + > > + return sensor_ops->config_set(th_sensor->ph, th_sensor->info->id, > > + config); > > +} > > + > > static const struct thermal_zone_device_ops scmi_hwmon_thermal_ops = > > { > > + .change_mode = scmi_hwmon_thermal_change_mode, > > .get_temp = scmi_hwmon_thermal_get_temp, > > }; > >
On Thu, Jan 25, 2024 at 07:06:25AM +0000, Peng Fan wrote: > Hi Guenter, > Hi, > > Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for > > thermal zones > > > > On 1/24/24 22:44, Peng Fan (OSS) wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > The thermal sensors maybe disabled before kernel boot, so add > > > change_mode for thermal zones to support configuring the thermal > > > sensor to enabled state. If reading the temperature when the sensor is > > > disabled, there will be error reported. > > > > > > The cost is an extra config_get all to SCMI firmware to get the status > > > of the thermal sensor. No function level impact. > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > > > > V3: > > > Update commit log to show it only applys to thermal > > > Add comments in code > > > Add R-b from Cristian > > > > > > > You didn't address my question regarding the behavior of hwmon attributes if > > a sensor is disabled. > > Would you please share a bit more on what attributes? > You mean the files under /sys/class/hwmon/hwmon0? > > If the sensor is disabled, when cat temp[x]_input, it will > report: > root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp3_input > cat: temp3_input: Protocol error > > For enabled, it will report value: > root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp1_input > 31900 > > > > > > Guenter, I Cced linux@roeck-us.net when sending V1/V2 > > > Let me Cc Guenter Roeck <groeck7@gmail.com> in V3, hope you not mind > > > > > This time I received it twice ;-). > > > > > V2: > > > Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update > > > config(Thanks Cristian) > > > > > > drivers/hwmon/scmi-hwmon.c | 39 > > ++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 39 insertions(+) > > > > > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > > > index 364199b332c0..af2267fea5f0 100644 > > > --- a/drivers/hwmon/scmi-hwmon.c > > > +++ b/drivers/hwmon/scmi-hwmon.c > > > @@ -151,7 +151,46 @@ static int scmi_hwmon_thermal_get_temp(struct > > thermal_zone_device *tz, > > > return ret; > > > } > > > > > > +static int scmi_hwmon_thermal_change_mode(struct > > thermal_zone_device *tz, > > > + enum thermal_device_mode > > new_mode) { > > > + int ret; > > > + u32 config; > > > + enum thermal_device_mode cur_mode = > > THERMAL_DEVICE_DISABLED; > > > + struct scmi_thermal_sensor *th_sensor = > > > +thermal_zone_device_priv(tz); > > > + > > > + ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id, > > > + &config); > > > + if (ret) > > > + return ret; > > > + > > > + if (SCMI_SENS_CFG_IS_ENABLED(config)) > > > + cur_mode = THERMAL_DEVICE_ENABLED; > > > + > > > + if (cur_mode == new_mode) > > > + return 0; > > > + > > > + /* > > > + * Per SENSOR_CONFIG_SET sensor_config description: > > > + * BIT[31:11] should be set to 0 if the sensor update interval does > > > + * not need to be updated, so clear them. > > > + * And SENSOR_CONFIG_GET does not return round up/down, so also > > clear > > > + * BIT[10:9] round up/down. > > > > What does "clear" mean ? Is it going to round up ? Round down ? And why > > would it be necessary to clear those bits if SENSOR_CONFIG_GET does not > > return the current setting in the first place ? > > This is to follow Cristian's suggestion to clear [31:9], because we only need > to set the sensor to enabled state, no other attributes. > My understanding is with BIT[31:11] set to 0, BIT[10:9] will not take effect. > Cristian may help comment more since he suggested to clear them in V1/V2 > > You are right, currently config_get will return [10:2] as reserved, > so config_set bit[10:9] no need touch. But config_get bit[10:2] > may update to return the value in future SCMI spec? > > Cristian or Sudeep may help comment here. From the spec SENSOR_CONFIG_SET can also be used to set the chosen update-interval for the sensor and the rounding-mode, but the specified rounding mode refers only to the currently issued CONFIG_SET operation, it is not a permanent configuration for the sensor: for this reason when you instead issue a CONFIG_GET it does not make any sense to return the rounding mode, since the interval returned by the GET is the already (previously) rounded final value configured on the sensor. So the spec is right and does not need any change in these regards. By the spec, also, if you set the update-interval to 0 in a CONFIG_SET, the chosen interval will remain unchanged, so the value of the ROUND bits is indeed irrelevant. Now, my (probably ill) advice to anyway clear also the round bits was aimed at using some sort of well-known value in the SET (even though ignored) given that the GET does specify those bits as reserved but you cannot be sure what the previous GET just returned from the fw-of-the-day (maybe by mistake), and if those bits will be effectively ignored on the SET given the zeroed interval value. Indeed, looking at this again, all of this is maybe just overthinking and it just confusing at this point to needlessly clear also those ROUNDING bits, since not required by the spec. Just clear the interval bits, my bad. Thanks, Cristian
On 1/24/24 23:06, Peng Fan wrote: > Hi Guenter, > >> Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for >> thermal zones >> >> On 1/24/24 22:44, Peng Fan (OSS) wrote: >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> The thermal sensors maybe disabled before kernel boot, so add >>> change_mode for thermal zones to support configuring the thermal >>> sensor to enabled state. If reading the temperature when the sensor is >>> disabled, there will be error reported. >>> >>> The cost is an extra config_get all to SCMI firmware to get the status >>> of the thermal sensor. No function level impact. >>> >>> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>> --- >>> >>> V3: >>> Update commit log to show it only applys to thermal >>> Add comments in code >>> Add R-b from Cristian >>> >> >> You didn't address my question regarding the behavior of hwmon attributes if >> a sensor is disabled. > > Would you please share a bit more on what attributes? > You mean the files under /sys/class/hwmon/hwmon0? > > If the sensor is disabled, when cat temp[x]_input, it will > report: > root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp3_input > cat: temp3_input: Protocol error > > For enabled, it will report value: > root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp1_input > 31900 > This is wrong. If the sensor is disabled, there should either be no sensor attribute (if the condition is permanent), or there should be tempX_enable attribute(s) which return the sensor status (and, if appropriate, permit it to be enabled). If the condition is not permanent, attempts to read the sensor value should return -ENODATA. Overall, hwmon functionality is orthogonal to thermal subsystem use. It is not appropriate for the thermal subsystem to disable any temperature sensors in the hwmon subsystem because the user might expect to be able to read temperatures from hwmon devices even if sensors are not in use by the thermal subsystem. Thanks, Guenter
On Thu, Jan 25, 2024 at 06:25:00AM -0800, Guenter Roeck wrote: > On 1/24/24 23:06, Peng Fan wrote: > > Hi Guenter, > > > > > Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for > > > thermal zones > > > > > > On 1/24/24 22:44, Peng Fan (OSS) wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > The thermal sensors maybe disabled before kernel boot, so add > > > > change_mode for thermal zones to support configuring the thermal > > > > sensor to enabled state. If reading the temperature when the sensor is > > > > disabled, there will be error reported. > > > > > > > > The cost is an extra config_get all to SCMI firmware to get the status > > > > of the thermal sensor. No function level impact. > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > --- > > > > > > > > V3: > > > > Update commit log to show it only applys to thermal > > > > Add comments in code > > > > Add R-b from Cristian > > > > > > > > > > You didn't address my question regarding the behavior of hwmon attributes if > > > a sensor is disabled. > > > > Would you please share a bit more on what attributes? > > You mean the files under /sys/class/hwmon/hwmon0? > > > > If the sensor is disabled, when cat temp[x]_input, it will > > report: > > root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp3_input > > cat: temp3_input: Protocol error > > > > For enabled, it will report value: > > root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp1_input > > 31900 > > > > This is wrong. If the sensor is disabled, there should either be no > sensor attribute (if the condition is permanent), or there should be > tempX_enable attribute(s) which return the sensor status (and, if > appropriate, permit it to be enabled). If the condition is not > permanent, attempts to read the sensor value should return -ENODATA. > If the sensor is permanently disabled it should not have been exposed by the SCMI server at all, in first place; in such a case it wont appear at all in hwmon. In this case seems like the sensor is NOT supposed to be ever disabled (not even temporarily), it it just that you want to ensure that is enabled, so I would say @Peng, should not that be done in the fw SCMI server ? (i.e. to turn on, early on, all those resources that are exposed to the agent and meant to be always on) Regarding the non-permanent condition, currently if the SCMI reading failed, for some reason, we return the same error as returned from the SCMI layer, so I suppose I should post a patch to remap that return value to -ENODATA. > Overall, hwmon functionality is orthogonal to thermal subsystem use. > It is not appropriate for the thermal subsystem to disable any > temperature sensors in the hwmon subsystem because the user might > expect to be able to read temperatures from hwmon devices even > if sensors are not in use by the thermal subsystem. Agreed, but it seems that indeed here the attempt is to make sure that an accidentally disabled sensor is turned on. Thanks, Cristian
On 1/25/24 08:09, Cristian Marussi wrote: > Agreed, but it seems that indeed here the attempt is to make sure that > an accidentally disabled sensor is turned on. > From the patch: +static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz, + enum thermal_device_mode new_mode) +{ ... + if (new_mode == THERMAL_DEVICE_ENABLED) + config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK; + else + config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK; This clearly contradicts your statement. It leaves the sensor in full control by the thermal subsystem. If the thermal subsystem decides to turn it off, it is turned off. Guenter
On Thu, Jan 25, 2024 at 08:16:45AM -0800, Guenter Roeck wrote: > On 1/25/24 08:09, Cristian Marussi wrote: > > > Agreed, but it seems that indeed here the attempt is to make sure that > > an accidentally disabled sensor is turned on. > > > > From the patch: > > +static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz, > + enum thermal_device_mode new_mode) > +{ > ... > + if (new_mode == THERMAL_DEVICE_ENABLED) > + config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK; > + else > + config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK; > > This clearly contradicts your statement. It leaves the sensor in full control > by the thermal subsystem. If the thermal subsystem decides to turn it off, > it is turned off. Yes, indeed, and this is wrong as you explained; what I meant is that it seems to me now after this discussion, and from the commit message, that the reason (and the aim of this patch) is different from how it achieves it (as a side effect) "The thermal sensors maybe disabled before kernel boot, so add change_mode for thermal zones to support configuring the thermal sensor to enabled state. If reading the temperature when the sensor is disabled, there will be error reported." So when I said: > > Agreed, but it seems that indeed here the attempt is to make sure that > > an accidentally disabled sensor is turned on. and >> In this case seems like the sensor is NOT supposed to be ever disabled >> (not even temporarily), it it just that you want to ensure that is >>enabled, so I would say @Peng, should not that be done in the fw >> SCMI server ? (i.e. to turn on, early on, all those resources that are >> exposed to the agent and meant to be always on) I implied to drop this patch and instead make sure the visible-and-always-enabled sensor is default-enabled by the SCMI server running in the firmware, given that there won't be any need to ever disable it, from the hwmon interface NOR from the thermal subsystem. Sorry if I have not been clear (but I maybe still well-wrong anyway :D) Thanks, Cristian
diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c index 364199b332c0..af2267fea5f0 100644 --- a/drivers/hwmon/scmi-hwmon.c +++ b/drivers/hwmon/scmi-hwmon.c @@ -151,7 +151,46 @@ static int scmi_hwmon_thermal_get_temp(struct thermal_zone_device *tz, return ret; } +static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz, + enum thermal_device_mode new_mode) +{ + int ret; + u32 config; + enum thermal_device_mode cur_mode = THERMAL_DEVICE_DISABLED; + struct scmi_thermal_sensor *th_sensor = thermal_zone_device_priv(tz); + + ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id, + &config); + if (ret) + return ret; + + if (SCMI_SENS_CFG_IS_ENABLED(config)) + cur_mode = THERMAL_DEVICE_ENABLED; + + if (cur_mode == new_mode) + return 0; + + /* + * Per SENSOR_CONFIG_SET sensor_config description: + * BIT[31:11] should be set to 0 if the sensor update interval does + * not need to be updated, so clear them. + * And SENSOR_CONFIG_GET does not return round up/down, so also clear + * BIT[10:9] round up/down. + */ + config &= ~(SCMI_SENS_CFG_UPDATE_SECS_MASK | + SCMI_SENS_CFG_UPDATE_EXP_MASK | + SCMI_SENS_CFG_ROUND_MASK); + if (new_mode == THERMAL_DEVICE_ENABLED) + config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK; + else + config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK; + + return sensor_ops->config_set(th_sensor->ph, th_sensor->info->id, + config); +} + static const struct thermal_zone_device_ops scmi_hwmon_thermal_ops = { + .change_mode = scmi_hwmon_thermal_change_mode, .get_temp = scmi_hwmon_thermal_get_temp, };