[lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

IT8721 and IT8728 support Intel PECI temperature reporting. Each sensor
can be programmed to display the temperature reported on the PECI interface.

If configured for Intel PECI, the driver reported the respective thermal sensor
as "disabled (0)". Fix the code to correctly report it as "Intel PECI (6)".

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Another potential candidate for -stable.

 Documentation/hwmon/it87 |    3 ++-
 drivers/hwmon/it87.c     |   18 ++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
index 92ce617..3d938aea 100644
--- a/Documentation/hwmon/it87
+++ b/Documentation/hwmon/it87
@@ -217,4 +217,5 @@ Temperature offset attributes
 The driver supports temp[1-3]_offset sysfs attributes to adjust the reported
 temperature for thermal diodes or diode connected thermal transistors.
 If a temperature sensor is configured for thermistors, the attribute values
-are ignored.
+are ignored. If the thermal sensor type is Intel PECI, the temperature offset
+must be programmed to the critical CPU temperature.
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index f3f3e79..f8336be 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -237,6 +237,7 @@ struct it87_devices {
 #define	FEAT_NEWER_AUTOPWM	(1 << 1)
 #define	FEAT_OLD_AUTOPWM	(1 << 2)
 #define	FEAT_16BIT_FAN		(1 << 3)
+#define	FEAT_PECI		(1 << 4)
 
 static const struct it87_devices it87_devices[] = {
 	[it87] = {
@@ -261,11 +262,13 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8721] = {
 		.name = "it8721",
-		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN,
+		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN
+		  | FEAT_PECI,
 	},
 	[it8728] = {
 		.name = "it8728",
-		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN,
+		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN
+		  | FEAT_PECI,
 	},
 	[it8782] = {
 		.name = "it8782",
@@ -281,6 +284,7 @@ static const struct it87_devices it87_devices[] = {
 #define has_12mv_adc(data)	((data)->features & FEAT_12MV_ADC)
 #define has_newer_autopwm(data)	((data)->features & FEAT_NEWER_AUTOPWM)
 #define has_old_autopwm(data)	((data)->features & FEAT_OLD_AUTOPWM)
+#define has_peci(data)		((data)->features & FEAT_PECI)
 
 struct it87_sio_data {
 	enum chips type;
@@ -581,6 +585,8 @@ static ssize_t show_type(struct device *dev, struct device_attribute *attr,
 	struct it87_data *data = it87_update_device(dev);
 	u8 reg = data->sensor;	    /* In case value is updated while used */
 
+	if (has_peci(data) && (reg >> 6 = nr + 1))
+		return sprintf(buf, "6\n");  /* Intel PECI */
 	if (reg & (1 << nr))
 		return sprintf(buf, "3\n");  /* thermal diode */
 	if (reg & (8 << nr))
@@ -604,13 +610,17 @@ static ssize_t set_type(struct device *dev, struct device_attribute *attr,
 	reg = it87_read_value(data, IT87_REG_TEMP_ENABLE);
 	reg &= ~(1 << nr);
 	reg &= ~(8 << nr);
+	if (has_peci(data) && (reg >> 6 = nr + 1 || val = 6))
+		reg &= 0x3f;
 	if (val = 2) {	/* backwards compatibility */
 		dev_warn(dev,
 			 "Sensor type 2 is deprecated, please use 4 instead\n");
 		val = 4;
 	}
-	/* 3 = thermal diode; 4 = thermistor; 0 = disabled */
-	if (val = 3)
+	/* 3 = thermal diode; 4 = thermistor; 6 = Intel PECI; 0 = disabled */
+	if (has_peci(data) && val = 6)
+		reg |= (nr + 1) << 6;
+	else if (val = 3)
 		reg |= 1 << nr;
 	else if (val = 4)
 		reg |= 8 << nr;
-- 
1.7.9.7


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Jean Delvare]

Hi Guenter,

On Sun, 28 Oct 2012 11:20:01 -0700, Guenter Roeck wrote:
> IT8721 and IT8728 support Intel PECI temperature reporting. Each sensor
> can be programmed to display the temperature reported on the PECI interface.
> 
> If configured for Intel PECI, the driver reported the respective thermal sensor
> as "disabled (0)". Fix the code to correctly report it as "Intel PECI (6)".

I think the driver reported it as whatever the other configuration bits
said, not necessarily zero. The configuration scheme for thermal sensor
types on these chips is anything but straightforward, plus you
shouldn't assume every BIOS out there is sane ;)

Anyway, good catch, I had not noticed that newer IT87xxF chips had
support for PECI.

> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Another potential candidate for -stable.

Seems difficult with this patch at the end of the series and depending
on another non-trivial patch earlier in the series. Anyway I consider
this a new feature rather than a bug fix, and some more work seems to
be needed (see below), so no, I don't think pushing this to stable is
the right thing to do.

> 
>  Documentation/hwmon/it87 |    3 ++-
>  drivers/hwmon/it87.c     |   18 ++++++++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
> index 92ce617..3d938aea 100644
> --- a/Documentation/hwmon/it87
> +++ b/Documentation/hwmon/it87
> @@ -217,4 +217,5 @@ Temperature offset attributes
>  The driver supports temp[1-3]_offset sysfs attributes to adjust the reported
>  temperature for thermal diodes or diode connected thermal transistors.
>  If a temperature sensor is configured for thermistors, the attribute values
> -are ignored.
> +are ignored. If the thermal sensor type is Intel PECI, the temperature offset
> +must be programmed to the critical CPU temperature.

I don't quite get this part.. Who should do that, and how?

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index f3f3e79..f8336be 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -237,6 +237,7 @@ struct it87_devices {
>  #define	FEAT_NEWER_AUTOPWM	(1 << 1)
>  #define	FEAT_OLD_AUTOPWM	(1 << 2)
>  #define	FEAT_16BIT_FAN		(1 << 3)
> +#define	FEAT_PECI		(1 << 4)
>  
>  static const struct it87_devices it87_devices[] = {
>  	[it87] = {
> @@ -261,11 +262,13 @@ static const struct it87_devices it87_devices[] = {
>  	},
>  	[it8721] = {
>  		.name = "it8721",
> -		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN,
> +		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN
> +		  | FEAT_PECI,
>  	},
>  	[it8728] = {
>  		.name = "it8728",
> -		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN,
> +		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN
> +		  | FEAT_PECI,
>  	},
>  	[it8782] = {
>  		.name = "it8782",
> @@ -281,6 +284,7 @@ static const struct it87_devices it87_devices[] = {
>  #define has_12mv_adc(data)	((data)->features & FEAT_12MV_ADC)
>  #define has_newer_autopwm(data)	((data)->features & FEAT_NEWER_AUTOPWM)
>  #define has_old_autopwm(data)	((data)->features & FEAT_OLD_AUTOPWM)
> +#define has_peci(data)		((data)->features & FEAT_PECI)
>  
>  struct it87_sio_data {
>  	enum chips type;
> @@ -581,6 +585,8 @@ static ssize_t show_type(struct device *dev, struct device_attribute *attr,
>  	struct it87_data *data = it87_update_device(dev);
>  	u8 reg = data->sensor;	    /* In case value is updated while used */
>  
> +	if (has_peci(data) && (reg >> 6 = nr + 1))
> +		return sprintf(buf, "6\n");  /* Intel PECI */

Looks fine for the IT8728F, but not for the IT8721F.

For the IT8721F, the code above works fine for temp1 and temp3, but as
I understand the datasheet, value 2 isn't possible and the PECI flag
for temp2 would be bit 7 in register 0x55. On that chip, the CPU PECI
temperature can only be mapped to temp1 and temp3 and the south bridge
(PCH) PECI temperature can only be mapped to temp2.

Speaking of PCH PECI temperature, apparently the IT8728F can do this
too, but differently. Relevant register is 0x9E, but to be honest I'm
not sure exactly how to handle it :( I suppose we can start with only
supporting the other registers for the time being, it's already better
than the current situation.

>  	if (reg & (1 << nr))
>  		return sprintf(buf, "3\n");  /* thermal diode */
>  	if (reg & (8 << nr))
> @@ -604,13 +610,17 @@ static ssize_t set_type(struct device *dev, struct device_attribute *attr,
>  	reg = it87_read_value(data, IT87_REG_TEMP_ENABLE);
>  	reg &= ~(1 << nr);
>  	reg &= ~(8 << nr);
> +	if (has_peci(data) && (reg >> 6 = nr + 1 || val = 6))
> +		reg &= 0x3f;
>  	if (val = 2) {	/* backwards compatibility */
>  		dev_warn(dev,
>  			 "Sensor type 2 is deprecated, please use 4 instead\n");
>  		val = 4;
>  	}
> -	/* 3 = thermal diode; 4 = thermistor; 0 = disabled */
> -	if (val = 3)
> +	/* 3 = thermal diode; 4 = thermistor; 6 = Intel PECI; 0 = disabled */
> +	if (has_peci(data) && val = 6)
> +		reg |= (nr + 1) << 6;

If there's no specific reason for handling this first, I'd move it last
for consistency and a slightly smaller patch.

> +	else if (val = 3)
>  		reg |= 1 << nr;
>  	else if (val = 4)
>  		reg |= 8 << nr;


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Mon, Oct 29, 2012 at 05:55:20PM +0100, Jean Delvare wrote:
> Hi Guenter,
> 
> On Sun, 28 Oct 2012 11:20:01 -0700, Guenter Roeck wrote:
> > IT8721 and IT8728 support Intel PECI temperature reporting. Each sensor
> > can be programmed to display the temperature reported on the PECI interface.
> > 
> > If configured for Intel PECI, the driver reported the respective thermal sensor
> > as "disabled (0)". Fix the code to correctly report it as "Intel PECI (6)".
> 
> I think the driver reported it as whatever the other configuration bits
> said, not necessarily zero. The configuration scheme for thermal sensor
> types on these chips is anything but straightforward, plus you
> shouldn't assume every BIOS out there is sane ;)
> 
Good point. I'll change the text.

> Anyway, good catch, I had not noticed that newer IT87xxF chips had
> support for PECI.
> 
Me not either. See below how I found it. Interestingly, IT8782 and IT8783
don't support it (IT8772 does, though, according to coreboot).

> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > Another potential candidate for -stable.
> 
> Seems difficult with this patch at the end of the series and depending
> on another non-trivial patch earlier in the series. Anyway I consider
> this a new feature rather than a bug fix, and some more work seems to
> be needed (see below), so no, I don't think pushing this to stable is
> the right thing to do.
> 
Ok; pretty much why I didn't do it in the first place.

> > 
> >  Documentation/hwmon/it87 |    3 ++-
> >  drivers/hwmon/it87.c     |   18 ++++++++++++++----
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
> > index 92ce617..3d938aea 100644
> > --- a/Documentation/hwmon/it87
> > +++ b/Documentation/hwmon/it87
> > @@ -217,4 +217,5 @@ Temperature offset attributes
> >  The driver supports temp[1-3]_offset sysfs attributes to adjust the reported
> >  temperature for thermal diodes or diode connected thermal transistors.
> >  If a temperature sensor is configured for thermistors, the attribute values
> > -are ignored.
> > +are ignored. If the thermal sensor type is Intel PECI, the temperature offset
> > +must be programmed to the critical CPU temperature.
> 
> I don't quite get this part.. Who should do that, and how?
> 
The BIOS, presumably. It does it on my board, but wrongly. It sets it
to 97 degrees C, while the CPU's Tcrit is 105 degrees C. I fixed the
offset to 105, and now the reported temperature matches the CPU temperature
(and moves with it). This is how I found out about PECI support in the
first place.

> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index f3f3e79..f8336be 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -237,6 +237,7 @@ struct it87_devices {
> >  #define	FEAT_NEWER_AUTOPWM	(1 << 1)
> >  #define	FEAT_OLD_AUTOPWM	(1 << 2)
> >  #define	FEAT_16BIT_FAN		(1 << 3)
> > +#define	FEAT_PECI		(1 << 4)
> >  
> >  static const struct it87_devices it87_devices[] = {
> >  	[it87] = {
> > @@ -261,11 +262,13 @@ static const struct it87_devices it87_devices[] = {
> >  	},
> >  	[it8721] = {
> >  		.name = "it8721",
> > -		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN,
> > +		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN
> > +		  | FEAT_PECI,
> >  	},
> >  	[it8728] = {
> >  		.name = "it8728",
> > -		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN,
> > +		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FAN
> > +		  | FEAT_PECI,
> >  	},
> >  	[it8782] = {
> >  		.name = "it8782",
> > @@ -281,6 +284,7 @@ static const struct it87_devices it87_devices[] = {
> >  #define has_12mv_adc(data)	((data)->features & FEAT_12MV_ADC)
> >  #define has_newer_autopwm(data)	((data)->features & FEAT_NEWER_AUTOPWM)
> >  #define has_old_autopwm(data)	((data)->features & FEAT_OLD_AUTOPWM)
> > +#define has_peci(data)		((data)->features & FEAT_PECI)
> >  
> >  struct it87_sio_data {
> >  	enum chips type;
> > @@ -581,6 +585,8 @@ static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> >  	struct it87_data *data = it87_update_device(dev);
> >  	u8 reg = data->sensor;	    /* In case value is updated while used */
> >  
> > +	if (has_peci(data) && (reg >> 6 = nr + 1))
> > +		return sprintf(buf, "6\n");  /* Intel PECI */
> 
> Looks fine for the IT8728F, but not for the IT8721F.
> 
> For the IT8721F, the code above works fine for temp1 and temp3, but as
> I understand the datasheet, value 2 isn't possible and the PECI flag
> for temp2 would be bit 7 in register 0x55. On that chip, the CPU PECI
> temperature can only be mapped to temp1 and temp3 and the south bridge
> (PCH) PECI temperature can only be mapped to temp2.
> 
Yes, I know, I was lazy :). I think I'll use a bitmap to handle it.
Should only be releevant for writes, though.

> Speaking of PCH PECI temperature, apparently the IT8728F can do this
> too, but differently. Relevant register is 0x9E, but to be honest I'm
> not sure exactly how to handle it :( I suppose we can start with only
> supporting the other registers for the time being, it's already better
> than the current situation.
> 
Let me check on my board. Maybe I can get it to work; if so, I'll submit another
patch to handle it.

> >  	if (reg & (1 << nr))
> >  		return sprintf(buf, "3\n");  /* thermal diode */
> >  	if (reg & (8 << nr))
> > @@ -604,13 +610,17 @@ static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> >  	reg = it87_read_value(data, IT87_REG_TEMP_ENABLE);
> >  	reg &= ~(1 << nr);
> >  	reg &= ~(8 << nr);
> > +	if (has_peci(data) && (reg >> 6 = nr + 1 || val = 6))
> > +		reg &= 0x3f;
> >  	if (val = 2) {	/* backwards compatibility */
> >  		dev_warn(dev,
> >  			 "Sensor type 2 is deprecated, please use 4 instead\n");
> >  		val = 4;
> >  	}
> > -	/* 3 = thermal diode; 4 = thermistor; 0 = disabled */
> > -	if (val = 3)
> > +	/* 3 = thermal diode; 4 = thermistor; 6 = Intel PECI; 0 = disabled */
> > +	if (has_peci(data) && val = 6)
> > +		reg |= (nr + 1) << 6;
> 
> If there's no specific reason for handling this first, I'd move it last
> for consistency and a slightly smaller patch.
> 
Ok.

> > +	else if (val = 3)
> >  		reg |= 1 << nr;
> >  	else if (val = 4)
> >  		reg |= 8 << nr;
> 
> 
> -- 
> Jean Delvare
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Jean Delvare]

On Mon, 29 Oct 2012 10:28:30 -0700, Guenter Roeck wrote:
> On Mon, Oct 29, 2012 at 05:55:20PM +0100, Jean Delvare wrote:
> > On Sun, 28 Oct 2012 11:20:01 -0700, Guenter Roeck wrote:
> > > diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
> > > index 92ce617..3d938aea 100644
> > > --- a/Documentation/hwmon/it87
> > > +++ b/Documentation/hwmon/it87
> > > @@ -217,4 +217,5 @@ Temperature offset attributes
> > >  The driver supports temp[1-3]_offset sysfs attributes to adjust the reported
> > >  temperature for thermal diodes or diode connected thermal transistors.
> > >  If a temperature sensor is configured for thermistors, the attribute values
> > > -are ignored.
> > > +are ignored. If the thermal sensor type is Intel PECI, the temperature offset
> > > +must be programmed to the critical CPU temperature.
> > 
> > I don't quite get this part.. Who should do that, and how?
>
> The BIOS, presumably. It does it on my board, but wrongly. It sets it
> to 97 degrees C, while the CPU's Tcrit is 105 degrees C. I fixed the
> offset to 105, and now the reported temperature matches the CPU temperature
> (and moves with it). This is how I found out about PECI support in the
> first place.

In which register is the PECI temperature offset value stored? I am
looking at the IT8720F datasheet and I can't find it.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Thu, Nov 01, 2012 at 01:56:07PM +0100, Jean Delvare wrote:
> On Mon, 29 Oct 2012 10:28:30 -0700, Guenter Roeck wrote:
> > On Mon, Oct 29, 2012 at 05:55:20PM +0100, Jean Delvare wrote:
> > > On Sun, 28 Oct 2012 11:20:01 -0700, Guenter Roeck wrote:
> > > > diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
> > > > index 92ce617..3d938aea 100644
> > > > --- a/Documentation/hwmon/it87
> > > > +++ b/Documentation/hwmon/it87
> > > > @@ -217,4 +217,5 @@ Temperature offset attributes
> > > >  The driver supports temp[1-3]_offset sysfs attributes to adjust the reported
> > > >  temperature for thermal diodes or diode connected thermal transistors.
> > > >  If a temperature sensor is configured for thermistors, the attribute values
> > > > -are ignored.
> > > > +are ignored. If the thermal sensor type is Intel PECI, the temperature offset
> > > > +must be programmed to the critical CPU temperature.
> > > 
> > > I don't quite get this part.. Who should do that, and how?
> >
> > The BIOS, presumably. It does it on my board, but wrongly. It sets it
> > to 97 degrees C, while the CPU's Tcrit is 105 degrees C. I fixed the
> > offset to 105, and now the reported temperature matches the CPU temperature
> > (and moves with it). This is how I found out about PECI support in the
> > first place.
> 
> In which register is the PECI temperature offset value stored? I am
> looking at the IT8720F datasheet and I can't find it.
> 
On IT8728F it is in the diode zero degree adjust registers.
I found this from experiments; it is not mentioned in the datasheet.
I would guess it is the same on the other chips, though I can not test
it on those.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Jean Delvare]

On Thu, 1 Nov 2012 06:40:51 -0700, Guenter Roeck wrote:
> On Thu, Nov 01, 2012 at 01:56:07PM +0100, Jean Delvare wrote:
> > On Mon, 29 Oct 2012 10:28:30 -0700, Guenter Roeck wrote:
> > > The BIOS, presumably. It does it on my board, but wrongly. It sets it
> > > to 97 degrees C, while the CPU's Tcrit is 105 degrees C. I fixed the
> > > offset to 105, and now the reported temperature matches the CPU temperature
> > > (and moves with it). This is how I found out about PECI support in the
> > > first place.
> > 
> > In which register is the PECI temperature offset value stored? I am
> > looking at the IT8720F datasheet and I can't find it.
>
> On IT8728F it is in the diode zero degree adjust registers.
> I found this from experiments; it is not mentioned in the datasheet.
> I would guess it is the same on the other chips, though I can not test
> it on those.

On an IT8720F chip with temp3 configured for PECI:

temp1_offset: 0
temp2_offset: 0
temp3_offset: -128000

I'm unsure how to interpret this -128000... This value is definitely
not neutral, doesn't sound very realistic. I would expect a positive
number for PECI. Maybe the offset register should be treated as
unsigned in PECI mode? Or maybe its value is irrelevant for the IT8720F
in PECI mode.

# grep . /sys/class/hwmon/hwmon2/device/temp3*
/sys/class/hwmon/hwmon2/device/temp3_alarm:0
/sys/class/hwmon/hwmon2/device/temp3_beep:1
/sys/class/hwmon/hwmon2/device/temp3_input:25000
/sys/class/hwmon/hwmon2/device/temp3_max:70000
/sys/class/hwmon/hwmon2/device/temp3_min:127000
/sys/class/hwmon/hwmon2/device/temp3_offset:-128000
/sys/class/hwmon/hwmon2/device/temp3_type:6

This is with an it87 driver including all your recent patches. It you
have ideas or theories, I'm interested.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Thu, Nov 01, 2012 at 05:37:57PM +0100, Jean Delvare wrote:
> On Thu, 1 Nov 2012 06:40:51 -0700, Guenter Roeck wrote:
> > On Thu, Nov 01, 2012 at 01:56:07PM +0100, Jean Delvare wrote:
> > > On Mon, 29 Oct 2012 10:28:30 -0700, Guenter Roeck wrote:
> > > > The BIOS, presumably. It does it on my board, but wrongly. It sets it
> > > > to 97 degrees C, while the CPU's Tcrit is 105 degrees C. I fixed the
> > > > offset to 105, and now the reported temperature matches the CPU temperature
> > > > (and moves with it). This is how I found out about PECI support in the
> > > > first place.
> > > 
> > > In which register is the PECI temperature offset value stored? I am
> > > looking at the IT8720F datasheet and I can't find it.
> >
> > On IT8728F it is in the diode zero degree adjust registers.
> > I found this from experiments; it is not mentioned in the datasheet.
> > I would guess it is the same on the other chips, though I can not test
> > it on those.
> 
> On an IT8720F chip with temp3 configured for PECI:
> 
> temp1_offset: 0
> temp2_offset: 0
> temp3_offset: -128000
> 
> I'm unsure how to interpret this -128000... This value is definitely
> not neutral, doesn't sound very realistic. I would expect a positive
> number for PECI. Maybe the offset register should be treated as
> unsigned in PECI mode? Or maybe its value is irrelevant for the IT8720F
> in PECI mode.
> 
> # grep . /sys/class/hwmon/hwmon2/device/temp3*
> /sys/class/hwmon/hwmon2/device/temp3_alarm:0
> /sys/class/hwmon/hwmon2/device/temp3_beep:1
> /sys/class/hwmon/hwmon2/device/temp3_input:25000
> /sys/class/hwmon/hwmon2/device/temp3_max:70000
> /sys/class/hwmon/hwmon2/device/temp3_min:127000
> /sys/class/hwmon/hwmon2/device/temp3_offset:-128000
> /sys/class/hwmon/hwmon2/device/temp3_type:6
> 
> This is with an it87 driver including all your recent patches. It you
> have ideas or theories, I'm interested.
> 
Hi Jean,

I agree. The alternative (+128 degrees C) sounds a bit high, though.
Maybe there is another register on IT8720F to store Tjmax ? Did you check
in the register map if there is a register value that happens to match it ?

What happens if you change the offset register value to, say, 100 ?
Does the reported temperature change as well ?

Another maybe dumb question - is this with Intel or AMD CPU ? Because
if it is AMD, it would not be PECI but AMDSI (something to fix if someone
has a system w/ IT87xx and AMD CPU).

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Jean Delvare]

On Thu, 1 Nov 2012 10:17:07 -0700, Guenter Roeck wrote:
> On Thu, Nov 01, 2012 at 05:37:57PM +0100, Jean Delvare wrote:
> > On an IT8720F chip with temp3 configured for PECI:
> > 
> > temp1_offset: 0
> > temp2_offset: 0
> > temp3_offset: -128000
> > 
> > I'm unsure how to interpret this -128000... This value is definitely
> > not neutral, doesn't sound very realistic. I would expect a positive
> > number for PECI. Maybe the offset register should be treated as
> > unsigned in PECI mode? Or maybe its value is irrelevant for the IT8720F
> > in PECI mode.
> > 
> > # grep . /sys/class/hwmon/hwmon2/device/temp3*
> > /sys/class/hwmon/hwmon2/device/temp3_alarm:0
> > /sys/class/hwmon/hwmon2/device/temp3_beep:1
> > /sys/class/hwmon/hwmon2/device/temp3_input:25000
> > /sys/class/hwmon/hwmon2/device/temp3_max:70000
> > /sys/class/hwmon/hwmon2/device/temp3_min:127000
> > /sys/class/hwmon/hwmon2/device/temp3_offset:-128000
> > /sys/class/hwmon/hwmon2/device/temp3_type:6
> > 
> > This is with an it87 driver including all your recent patches. It you
> > have ideas or theories, I'm interested.
> > 
> Hi Jean,
> 
> I agree. The alternative (+128 degrees C) sounds a bit high, though.
> Maybe there is another register on IT8720F to store Tjmax ? Did you check
> in the register map if there is a register value that happens to match it ?

How would I know which value I am looking for? k10temp says this:
high = +70.0°C, crit = +90.0°C, hyst = +87.0°C
but the monitored value doesn't match it87's temp3.

> What happens if you change the offset register value to, say, 100 ?
> Does the reported temperature change as well ?

Yes it does.
temp3_offset =  120000 -> temp3_input = 14000
temp3_offset =  125000 -> temp3_input = 19000
temp3_offset = -128000 -> temp3_input = 22000
temp3_offset = -126000 -> temp3_input = 23000

This leads me to the conclusion that the offset registers are unsigned
in PECI mode.

> Another maybe dumb question - is this with Intel or AMD CPU ? Because
> if it is AMD, it would not be PECI but AMDSI (something to fix if someone
> has a system w/ IT87xx and AMD CPU).

It is AMD indeed, that's another issue I wanted to report. Do you
already know how to fix it?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Thu, Nov 01, 2012 at 07:12:08PM +0100, Jean Delvare wrote:
> On Thu, 1 Nov 2012 10:17:07 -0700, Guenter Roeck wrote:
> > On Thu, Nov 01, 2012 at 05:37:57PM +0100, Jean Delvare wrote:
> > > On an IT8720F chip with temp3 configured for PECI:
> > > 
> > > temp1_offset: 0
> > > temp2_offset: 0
> > > temp3_offset: -128000
> > > 
> > > I'm unsure how to interpret this -128000... This value is definitely
> > > not neutral, doesn't sound very realistic. I would expect a positive
> > > number for PECI. Maybe the offset register should be treated as
> > > unsigned in PECI mode? Or maybe its value is irrelevant for the IT8720F
> > > in PECI mode.
> > > 
> > > # grep . /sys/class/hwmon/hwmon2/device/temp3*
> > > /sys/class/hwmon/hwmon2/device/temp3_alarm:0
> > > /sys/class/hwmon/hwmon2/device/temp3_beep:1
> > > /sys/class/hwmon/hwmon2/device/temp3_input:25000
> > > /sys/class/hwmon/hwmon2/device/temp3_max:70000
> > > /sys/class/hwmon/hwmon2/device/temp3_min:127000
> > > /sys/class/hwmon/hwmon2/device/temp3_offset:-128000
> > > /sys/class/hwmon/hwmon2/device/temp3_type:6
> > > 
> > > This is with an it87 driver including all your recent patches. It you
> > > have ideas or theories, I'm interested.
> > > 
> > Hi Jean,
> > 
> > I agree. The alternative (+128 degrees C) sounds a bit high, though.
> > Maybe there is another register on IT8720F to store Tjmax ? Did you check
> > in the register map if there is a register value that happens to match it ?
> 
> How would I know which value I am looking for? k10temp says this:
> high = +70.0°C, crit = +90.0°C, hyst = +87.0°C
> but the monitored value doesn't match it87's temp3.
> 
> > What happens if you change the offset register value to, say, 100 ?
> > Does the reported temperature change as well ?
> 
> Yes it does.
> temp3_offset =  120000 -> temp3_input = 14000
> temp3_offset =  125000 -> temp3_input = 19000
> temp3_offset = -128000 -> temp3_input = 22000
> temp3_offset = -126000 -> temp3_input = 23000
> 
> This leads me to the conclusion that the offset registers are unsigned
> in PECI mode.
> 
If so, then only for AMD, or it is is a chip dependent.

On Intel (with IT8728F):

temp3_offset = -20000:	temp3_input = -96000
temp3_offset = -10000:	temp3_input = -87000
temp3_offset = 0:	temp3_input = -76000
temp3_offset = 10000:	temp3_input = -67000
temp3_offset = 20000:	temp3_input = -56000
temp3_offset = 105000:	temp3_input = 28000 (matches Tjmax / coretemp)

I don't know what AMD uses as equivalent of Tjmax - maybe they
use a fixed value of 128°C.

> > Another maybe dumb question - is this with Intel or AMD CPU ? Because
> > if it is AMD, it would not be PECI but AMDSI (something to fix if someone
> > has a system w/ IT87xx and AMD CPU).
> 
> It is AMD indeed, that's another issue I wanted to report. Do you
> already know how to fix it?
> 
Yes, I think so, at least I have an idea. Give me a couple of days.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Phil Pokorny]

[-- Attachment #1.1: Type: text/plain, Size: 2078 bytes --]

On Thu, Nov 1, 2012 at 1:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> On Thu, Nov 01, 2012 at 07:12:08PM +0100, Jean Delvare wrote:
> > On Thu, 1 Nov 2012 10:17:07 -0700, Guenter Roeck wrote:
> > > On Thu, Nov 01, 2012 at 05:37:57PM +0100, Jean Delvare wrote:
> > > > On an IT8720F chip with temp3 configured for PECI:
> > > >
> > > > temp1_offset: 0
> > > > temp2_offset: 0
> > > > temp3_offset: -128000
> > > >
> > > > I'm unsure how to interpret this -128000... This value is definitely
> > > > not neutral, doesn't sound very realistic. I would expect a positive
> > > > number for PECI. Maybe the offset register should be treated as
> > > > unsigned in PECI mode? Or maybe its value is irrelevant for the
> IT8720F
> > > > in PECI mode.
> > > >
> > > > # grep . /sys/class/hwmon/hwmon2/device/temp3*
> > > > /sys/class/hwmon/hwmon2/device/temp3_alarm:0
> > > > /sys/class/hwmon/hwmon2/device/temp3_beep:1
> > > > /sys/class/hwmon/hwmon2/device/temp3_input:25000
> > > > /sys/class/hwmon/hwmon2/device/temp3_max:70000
> > > > /sys/class/hwmon/hwmon2/device/temp3_min:127000
> > > > /sys/class/hwmon/hwmon2/device/temp3_offset:-128000
> > > > /sys/class/hwmon/hwmon2/device/temp3_type:6
> > > >
> > > > This is with an it87 driver including all your recent patches. It you
> > > > have ideas or theories, I'm interested.
> > > >
> > > Hi Jean,
> > >
> > > I agree. The alternative (+128 degrees C) sounds a bit high, though.
> > > Maybe there is another register on IT8720F to store Tjmax ? Did you
> check
> > > in the register map if there is a register value that happens to match
> it ?
> >
> > How would I know which value I am looking for? k10temp says this:
> > high = +70.0°C, crit = +90.0°C, hyst = +87.0°C
> > but the monitored value doesn't match it87's temp3.
>


> I don't know what AMD uses as equivalent of Tjmax - maybe they
> use a fixed value of 128°C.
>

Wouldn't it be 70 degC?  That's the "Tcontrol" value they always
reference.  Anything over 70deg C on the "Tcontrol" scale is "HOT"

Phil P.

[-- Attachment #1.2: Type: text/html, Size: 2863 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Thu, Nov 01, 2012 at 02:18:53PM -0700, Phil Pokorny wrote:
> On Thu, Nov 1, 2012 at 1:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On Thu, Nov 01, 2012 at 07:12:08PM +0100, Jean Delvare wrote:
> > > On Thu, 1 Nov 2012 10:17:07 -0700, Guenter Roeck wrote:
> > > > On Thu, Nov 01, 2012 at 05:37:57PM +0100, Jean Delvare wrote:
> > > > > On an IT8720F chip with temp3 configured for PECI:
> > > > >
> > > > > temp1_offset: 0
> > > > > temp2_offset: 0
> > > > > temp3_offset: -128000
> > > > >
> > > > > I'm unsure how to interpret this -128000... This value is definitely
> > > > > not neutral, doesn't sound very realistic. I would expect a positive
> > > > > number for PECI. Maybe the offset register should be treated as
> > > > > unsigned in PECI mode? Or maybe its value is irrelevant for the
> > IT8720F
> > > > > in PECI mode.
> > > > >
> > > > > # grep . /sys/class/hwmon/hwmon2/device/temp3*
> > > > > /sys/class/hwmon/hwmon2/device/temp3_alarm:0
> > > > > /sys/class/hwmon/hwmon2/device/temp3_beep:1
> > > > > /sys/class/hwmon/hwmon2/device/temp3_input:25000
> > > > > /sys/class/hwmon/hwmon2/device/temp3_max:70000
> > > > > /sys/class/hwmon/hwmon2/device/temp3_min:127000
> > > > > /sys/class/hwmon/hwmon2/device/temp3_offset:-128000
> > > > > /sys/class/hwmon/hwmon2/device/temp3_type:6
> > > > >
> > > > > This is with an it87 driver including all your recent patches. It you
> > > > > have ideas or theories, I'm interested.
> > > > >
> > > > Hi Jean,
> > > >
> > > > I agree. The alternative (+128 degrees C) sounds a bit high, though.
> > > > Maybe there is another register on IT8720F to store Tjmax ? Did you
> > check
> > > > in the register map if there is a register value that happens to match
> > it ?
> > >
> > > How would I know which value I am looking for? k10temp says this:
> > > high = +70.0°C, crit = +90.0°C, hyst = +87.0°C
> > > but the monitored value doesn't match it87's temp3.
> >
> 
> 
> > I don't know what AMD uses as equivalent of Tjmax - maybe they
> > use a fixed value of 128°C.
> >
> 
> Wouldn't it be 70 degC?  That's the "Tcontrol" value they always
> reference.  Anything over 70deg C on the "Tcontrol" scale is "HOT"
> 
One would think so, but that doesn't fit with Jean's observation that he has to
enter -128 degC (or possibly 128 degC) for a decent temperature reading.

Jean, does the temperature increase if you put load onto the system ?
And which value in temp3_offset makes the reported temperature match the
temperature reported by k10temp ?

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Jean Delvare]

On Thu, 1 Nov 2012 20:09:00 -0700, Guenter Roeck wrote:
> One would think so, but that doesn't fit with Jean's observation that he has to
> enter -128 degC (or possibly 128 degC) for a decent temperature reading.
> 
> Jean, does the temperature increase if you put load onto the system ?

Yes.

> And which value in temp3_offset makes the reported temperature match the
> temperature reported by k10temp ?

temp3_offset value of 116000 makes them match.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Fri, Nov 02, 2012 at 04:24:04PM +0100, Jean Delvare wrote:
> On Thu, 1 Nov 2012 20:09:00 -0700, Guenter Roeck wrote:
> > One would think so, but that doesn't fit with Jean's observation that he has to
> > enter -128 degC (or possibly 128 degC) for a decent temperature reading.
> > 
> > Jean, does the temperature increase if you put load onto the system ?
> 
> Yes.
> 
> > And which value in temp3_offset makes the reported temperature match the
> > temperature reported by k10temp ?
> 
> temp3_offset value of 116000 makes them match.
> 
Good, so at least we know that we have the correct register. The only remaining
question is if we should make tempX_offset unsigned if a sensor is configured
for AMDTSI. Kind of odd, though, since it is signed for everything else and
might thus change sign when/if the sensor type is changed. Not sure what the
best approach might be.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Jean Delvare]

On Fri, 2 Nov 2012 10:19:25 -0700, Guenter Roeck wrote:
> On Fri, Nov 02, 2012 at 04:24:04PM +0100, Jean Delvare wrote:
> > temp3_offset value of 116000 makes them match.
>
> Good, so at least we know that we have the correct register. The only remaining
> question is if we should make tempX_offset unsigned if a sensor is configured
> for AMDTSI. Kind of odd, though, since it is signed for everything else and
> might thus change sign when/if the sensor type is changed. Not sure what the
> best approach might be.

Treating it as unsigned when the type is set to AMD-SI is probably the
way to go for now, although I would love to get more samples from users
of AMD CPUs to make sure. I suppose we'll get more feedback after the
patch series you send earlier this week hits mainline.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 9/9] hwmon: (it87) Report thermal sensor type as Intel PECI if appropriate

[Guenter Roeck]

On Fri, Nov 02, 2012 at 06:31:34PM +0100, Jean Delvare wrote:
> On Fri, 2 Nov 2012 10:19:25 -0700, Guenter Roeck wrote:
> > On Fri, Nov 02, 2012 at 04:24:04PM +0100, Jean Delvare wrote:
> > > temp3_offset value of 116000 makes them match.
> >
> > Good, so at least we know that we have the correct register. The only remaining
> > question is if we should make tempX_offset unsigned if a sensor is configured
> > for AMDTSI. Kind of odd, though, since it is signed for everything else and
> > might thus change sign when/if the sensor type is changed. Not sure what the
> > best approach might be.
> 
> Treating it as unsigned when the type is set to AMD-SI is probably the
> way to go for now, although I would love to get more samples from users

Agreed.

> of AMD CPUs to make sure. I suppose we'll get more feedback after the
> patch series you send earlier this week hits mainline.
> 
and agreed.

I'll come up with one more patch to add support for AMD-SI to it8728f.
Hopefully sometime this weekend.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors