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@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxxxxxxx when sending V1/V2
> > >   Let me Cc Guenter Roeck <groeck7@xxxxxxxxx> 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