[lm-sensors] [PATCH 3/3] hwmon: (k8temp) fix temperature reporting

[lm-sensors] [PATCH 3/3] hwmon: (k8temp) fix temperature reporting

[Andreas Herrmann]

Current Temperature for K8 RevG desktop CPUs is a "normalized value"
which can be below ambient temperature.

As a consequence lots of RevG systems report temperatures like:

 sensors
 k8temp-pci-00c3
 Adapter: PCI adapter
 Core0 Temp:
              +17C
 Core0 Temp:
               +3C
 Core1 Temp:
              +21C
 Core1 Temp:
               +5C

being quite below ambient temperature.
There are even reports of negative temperature values.

This patch corrects the temperature reporting of k8temp for
RevG desktop CPUs.

Cc: Rudolf Marek <r.marek@assembler.cz>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 drivers/hwmon/k8temp.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
index ff12281..b138f79 100644
--- a/drivers/hwmon/k8temp.c
+++ b/drivers/hwmon/k8temp.c
@@ -49,6 +49,7 @@ struct k8temp_data {
 	u8 sensorsp;		/* sensor presence bits - SEL_CORE & SEL_PLACE */
 	u32 temp[2][2];		/* core, place */
 	u8 swap_core_select;    /* meaning of SEL_CORE is inverted */
+	u32 temp_offset;
 };
 
 static struct k8temp_data *k8temp_update_device(struct device *dev)
@@ -116,13 +117,15 @@ static ssize_t show_temp(struct device *dev,
 	    to_sensor_dev_attr_2(devattr);
 	int core = attr->nr;
 	int place = attr->index;
+	int temp;
 	struct k8temp_data *data = k8temp_update_device(dev);
 
 	if (data->swap_core_select)
 		core = core ? 0 : 1;
 
-	return sprintf(buf, "%d\n",
-		       TEMP_FROM_REG(data->temp[core][place]));
+	temp = TEMP_FROM_REG(data->temp[core][place]) + data->temp_offset;
+
+	return sprintf(buf, "%d\n", temp);
 }
 
 /* core, place */
@@ -176,6 +179,16 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 				 "wrong - check erratum #141\n");
 		}
 
+		if (((model >= 0x68) && (model != 0xc1)) &&
+		    !(model = 0x68) && !(model = 0x6c) &&
+		    !(model = 0x7c))
+			/*
+			 * RevG desktop CPUs (i.e. no socket S1G1 parts)
+			 * need additional offset, otherwise reported
+			 * temperature is below ambient temperature
+			 */
+			data->temp_offset = 21000;
+
 		break;
 	}
 
-- 
1.6.0.4




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

Re: [lm-sensors] [PATCH 3/3] hwmon: (k8temp) fix temperature

[Jean Delvare]

Hi Andreas,

On Tue, 16 Dec 2008 00:12:13 +0100, Andreas Herrmann wrote:
> Current Temperature for K8 RevG desktop CPUs is a "normalized value"
> which can be below ambient temperature.
> 
> As a consequence lots of RevG systems report temperatures like:
> 
>  sensors
>  k8temp-pci-00c3
>  Adapter: PCI adapter
>  Core0 Temp:
>               +17C
>  Core0 Temp:
>                +3C
>  Core1 Temp:
>               +21C
>  Core1 Temp:
>                +5C
> 
> being quite below ambient temperature.
> There are even reports of negative temperature values.
> 
> This patch corrects the temperature reporting of k8temp for
> RevG desktop CPUs.

Thanks for working on this, that was very needed.

> Cc: Rudolf Marek <r.marek@assembler.cz>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  drivers/hwmon/k8temp.c |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> index ff12281..b138f79 100644
> --- a/drivers/hwmon/k8temp.c
> +++ b/drivers/hwmon/k8temp.c
> @@ -49,6 +49,7 @@ struct k8temp_data {
>  	u8 sensorsp;		/* sensor presence bits - SEL_CORE & SEL_PLACE */
>  	u32 temp[2][2];		/* core, place */
>  	u8 swap_core_select;    /* meaning of SEL_CORE is inverted */
> +	u32 temp_offset;
>  };
>  
>  static struct k8temp_data *k8temp_update_device(struct device *dev)
> @@ -116,13 +117,15 @@ static ssize_t show_temp(struct device *dev,
>  	    to_sensor_dev_attr_2(devattr);
>  	int core = attr->nr;
>  	int place = attr->index;
> +	int temp;
>  	struct k8temp_data *data = k8temp_update_device(dev);
>  
>  	if (data->swap_core_select)
>  		core = core ? 0 : 1;
>  
> -	return sprintf(buf, "%d\n",
> -		       TEMP_FROM_REG(data->temp[core][place]));
> +	temp = TEMP_FROM_REG(data->temp[core][place]) + data->temp_offset;
> +
> +	return sprintf(buf, "%d\n", temp);
>  }
>  
>  /* core, place */
> @@ -176,6 +179,16 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
>  				 "wrong - check erratum #141\n");
>  		}
>  
> +		if (((model >= 0x68) && (model != 0xc1)) &&
> +		    !(model = 0x68) && !(model = 0x6c) &&
> +		    !(model = 0x7c))

This test is pretty confusing, with these extra parentheses and the mix
of (a != b) and !(a = b). What about the following instead? As far as
I can see, it leads to the same results, but is much more readable:

		if (model >= 0x69 &&
		    !(model = 0xc1 || model = 0x6c || model = 0x7c))


> +			/*
> +			 * RevG desktop CPUs (i.e. no socket S1G1 parts)
> +			 * need additional offset, otherwise reported
> +			 * temperature is below ambient temperature
> +			 */
> +			data->temp_offset = 21000;

If you apply the same offset to all sensors, you'll still obtain
something odd:

 k8temp-pci-00c3
 Adapter: PCI adapter
 Core0 Temp:
              +38°C
 Core0 Temp:
              +24°C
 Core1 Temp:
              +42°C
 Core1 Temp:
              +26°C

That's not terribly realistic, is it? Unless both sensors for a given
core are very far apart - but I suspect each core is pretty small,
isn't it?

> +
>  		break;
>  	}
>  


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 3/3] hwmon: (k8temp) fix temperature

[Andreas Herrmann]

On Fri, Jan 09, 2009 at 02:34:39PM +0100, Jean Delvare wrote:
> > @@ -176,6 +179,16 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  				 "wrong - check erratum #141\n");
> >  		}
> >  
> > +		if (((model >= 0x68) && (model != 0xc1)) &&
> > +		    !(model = 0x68) && !(model = 0x6c) &&
> > +		    !(model = 0x7c))
> 
> This test is pretty confusing, with these extra parentheses and the mix
> of (a != b) and !(a = b). What about the following instead? As far as
> I can see, it leads to the same results, but is much more readable:
> 
> 		if (model >= 0x69 &&
> 		    !(model = 0xc1 || model = 0x6c || model = 0x7c))
> 

Well, right you are.

When I wrote the check I had following in mind:

  Check for RevG:

  ((model >= 0x68) && (model != 0xc1)) // excluding ATHLON64 FX, "JH-F3"

  and after that exclude all mobile parts:

  !(model = 0x68) && !(model = 0x6c) && !(model = 0x7c))

Of course this can be simplified.

> > +			/*
> > +			 * RevG desktop CPUs (i.e. no socket S1G1 parts)
> > +			 * need additional offset, otherwise reported
> > +			 * temperature is below ambient temperature
> > +			 */
> > +			data->temp_offset = 21000;
> 
> If you apply the same offset to all sensors, you'll still obtain
> something odd:
> 
>  k8temp-pci-00c3
>  Adapter: PCI adapter
>  Core0 Temp:
>               +38°C
>  Core0 Temp:
>               +24°C
>  Core1 Temp:
>               +42°C
>  Core1 Temp:
>               +26°C
> 
> That's not terribly realistic, is it? Unless both sensors for a given
> core are very far apart -
> but I suspect each core is pretty small,
> isn't it?

"Far" is relative, isn't it ;-)

Guess this is from an idle system.
Please bring load on both cores and look at the temperature
differences again.




Regards,

Andreas



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

Re: [lm-sensors] [PATCH 3/3] hwmon: (k8temp) fix temperature

[Jean Delvare]

On Fri, 9 Jan 2009 15:41:02 +0100, Andreas Herrmann wrote:
> On Fri, Jan 09, 2009 at 02:34:39PM +0100, Jean Delvare wrote:
> > If you apply the same offset to all sensors, you'll still obtain
> > something odd:
> > 
> >  k8temp-pci-00c3
> >  Adapter: PCI adapter
> >  Core0 Temp:
> >               +38°C
> >  Core0 Temp:
> >               +24°C
> >  Core1 Temp:
> >               +42°C
> >  Core1 Temp:
> >               +26°C
> > 
> > That's not terribly realistic, is it? Unless both sensors for a given
> > core are very far apart -
> > but I suspect each core is pretty small,
> > isn't it?
> 
> "Far" is relative, isn't it ;-)
> 
> Guess this is from an idle system.
> Please bring load on both cores and look at the temperature
> differences again.

Well that's not my system, so I can't do the tests. But next time
someone reports with a rev.G K8, we'll ask him/her.

-- 
Jean Delvare

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