[PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Peng Fan (OSS)]

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

 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

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.
+	 */
+	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,
 };
 
-- 
2.37.1

Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Guenter Roeck]

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,
>   };
>   

RE: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Peng Fan]

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,
> >   };
> >

Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Cristian Marussi]

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

Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Guenter Roeck]

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

Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Cristian Marussi]

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

Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Guenter Roeck]

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

Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Cristian Marussi]

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

RE: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Peng Fan]

> Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for
> thermal zones
> 
> 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)

Sorry to bring back this old topic.

The tempsensor is disabled at boot, I will check with FW owner to enable it
by default. But the tempsensor will consume some power, if leaving
it always enabled.

Do we need to export a HWMON_T_ENABLE to temperature sensor if
leaving thermal framework only reading temp?

Thanks,
Peng.
> 
> Thanks,
> Cristian
>