[lm-sensors] [PATCH] hwmon,

[lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

From: Andreas Herrmann <andreas.herrmann3@amd.com>

Reported temperature for ASB1 CPUs is too high.
Add ASB1 CPU revisions (these are also non-desktop variants) to the
list of CPUs for which the temperature fixup is not required.

Example: (from LENOVO ThinkPad Edge 13, 01972NG, system was idle)

  Current kernel reports

  $ sensors
  k8temp-pci-00c3
  Adapter: PCI adapter
  Core0 Temp:  +74.0°C
  Core0 Temp:  +70.0°C
  Core1 Temp:  +69.0°C
  Core1 Temp:  +70.0°C

  With this patch I have

  $ sensors
  k8temp-pci-00c3
  Adapter: PCI adapter
  Core0 Temp:  +54.0°C
  Core0 Temp:  +51.0°C
  Core1 Temp:  +48.0°C
  Core1 Temp:  +49.0°C

Cc: <stable@kernel.org> # .32.x .33.x, .34.x
Cc: Rudolf Marek <r.marek@assembler.cz>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 drivers/hwmon/k8temp.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

I think this patch should go into .35.
Patch was built against Linus' git v2.6.35-rc3-3-g9a3cde2.
(Hope that there is no conflict with your recent fix for single core
CPUs.)


Thanks,

Andreas


diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
index 0ceb6d6..1fdd63e 100644
--- a/drivers/hwmon/k8temp.c
+++ b/drivers/hwmon/k8temp.c
@@ -180,11 +180,13 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 		}
 
 		if ((model >= 0x69) &&
-		    !(model = 0xc1 || model = 0x6c || model = 0x7c)) {
+		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
+		      model = 0x6b || model = 0x6f || model = 0x7f)) {
 			/*
-			 * RevG desktop CPUs (i.e. no socket S1G1 parts)
-			 * need additional offset, otherwise reported
-			 * temperature is below ambient temperature
+			 * RevG desktop CPUs (i.e. no socket S1G1 or
+			 * ASB1 parts) need additional offset,
+			 * otherwise reported temperature is below
+			 * ambient temperature
 			 */
 			data->temp_offset = 21000;
 		}
-- 
1.7.1


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

Re: [lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

On Tue, Jun 22, 2010 at 01:35:33PM +0200, Jean Delvare wrote:
> Hi Andreas,
> 
> On Tue, 22 Jun 2010 10:45:57 +0200, Andreas Herrmann wrote:
> > From: Andreas Herrmann <andreas.herrmann3@amd.com>
> > 
> > Reported temperature for ASB1 CPUs is too high.
> > Add ASB1 CPU revisions (these are also non-desktop variants) to the
> > list of CPUs for which the temperature fixup is not required.
> > 
> > Example: (from LENOVO ThinkPad Edge 13, 01972NG, system was idle)
> > 
> >   Current kernel reports
> > 
> >   $ sensors
> >   k8temp-pci-00c3
> >   Adapter: PCI adapter
> >   Core0 Temp:  +74.0°C
> >   Core0 Temp:  +70.0°C
> >   Core1 Temp:  +69.0°C
> >   Core1 Temp:  +70.0°C
> > 
> >   With this patch I have
> > 
> >   $ sensors
> >   k8temp-pci-00c3
> >   Adapter: PCI adapter
> >   Core0 Temp:  +54.0°C
> >   Core0 Temp:  +51.0°C
> >   Core1 Temp:  +48.0°C
> >   Core1 Temp:  +49.0°C
> > 
> > Cc: <stable@kernel.org> # .32.x .33.x, .34.x
> > Cc: Rudolf Marek <r.marek@assembler.cz>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> > ---
> >  drivers/hwmon/k8temp.c |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > I think this patch should go into .35.
> 
> I agree, I've applied it, thanks for your contribution.
> 
> > Patch was built against Linus' git v2.6.35-rc3-3-g9a3cde2.
> > (Hope that there is no conflict with your recent fix for single core
> > CPUs.)
> 
> No, there is not.
> 
> > diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> > index 0ceb6d6..1fdd63e 100644
> > --- a/drivers/hwmon/k8temp.c
> > +++ b/drivers/hwmon/k8temp.c
> > @@ -180,11 +180,13 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  		}
> >  
> >  		if ((model >= 0x69) &&
> > -		    !(model = 0xc1 || model = 0x6c || model = 0x7c)) {
> > +		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> > +		      model = 0x6b || model = 0x6f || model = 0x7f)) {
> >  			/*
> > -			 * RevG desktop CPUs (i.e. no socket S1G1 parts)
> > -			 * need additional offset, otherwise reported
> > -			 * temperature is below ambient temperature
> > +			 * RevG desktop CPUs (i.e. no socket S1G1 or
> > +			 * ASB1 parts) need additional offset,
> > +			 * otherwise reported temperature is below
> > +			 * ambient temperature
> >  			 */
> >  			data->temp_offset = 21000;
> >  		}
> 
> I would like to avoid having to update this list each time a new CPU
> model is released. Are there more K8 models scheduled to be released?

I am not aware of other models. I think the ASB1 parts were the last
updates of that CPU family. Those Athlon/Turion Neo CPUs are on the
market for quite some time now. I've encountered that problem because
I have such a CPU in my Laptop.

> If there are, we should come up with a better test, either by
> defaulting to no offset if all newer models don't need it, or by using
> a socket-based detection as we do in the k10temp driver, assuming this
> is available on the K8 as well.

Unfortunately on K8 socket type information is not provided with CPUID
like its done for family 0x10. Thus the socket type must be derived
from the CPU revision and information contained in the CPU revision
guide.

I think (hope) that this list needs no update anymore.


Thanks,


Andreas

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

Re: [lm-sensors] [PATCH] hwmon,

[Jean Delvare]

Hi Andreas,

On Tue, 22 Jun 2010 10:45:57 +0200, Andreas Herrmann wrote:
> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> 
> Reported temperature for ASB1 CPUs is too high.
> Add ASB1 CPU revisions (these are also non-desktop variants) to the
> list of CPUs for which the temperature fixup is not required.
> 
> Example: (from LENOVO ThinkPad Edge 13, 01972NG, system was idle)
> 
>   Current kernel reports
> 
>   $ sensors
>   k8temp-pci-00c3
>   Adapter: PCI adapter
>   Core0 Temp:  +74.0°C
>   Core0 Temp:  +70.0°C
>   Core1 Temp:  +69.0°C
>   Core1 Temp:  +70.0°C
> 
>   With this patch I have
> 
>   $ sensors
>   k8temp-pci-00c3
>   Adapter: PCI adapter
>   Core0 Temp:  +54.0°C
>   Core0 Temp:  +51.0°C
>   Core1 Temp:  +48.0°C
>   Core1 Temp:  +49.0°C
> 
> Cc: <stable@kernel.org> # .32.x .33.x, .34.x
> Cc: Rudolf Marek <r.marek@assembler.cz>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  drivers/hwmon/k8temp.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> I think this patch should go into .35.

I agree, I've applied it, thanks for your contribution.

> Patch was built against Linus' git v2.6.35-rc3-3-g9a3cde2.
> (Hope that there is no conflict with your recent fix for single core
> CPUs.)

No, there is not.

> diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> index 0ceb6d6..1fdd63e 100644
> --- a/drivers/hwmon/k8temp.c
> +++ b/drivers/hwmon/k8temp.c
> @@ -180,11 +180,13 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
>  		}
>  
>  		if ((model >= 0x69) &&
> -		    !(model = 0xc1 || model = 0x6c || model = 0x7c)) {
> +		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> +		      model = 0x6b || model = 0x6f || model = 0x7f)) {
>  			/*
> -			 * RevG desktop CPUs (i.e. no socket S1G1 parts)
> -			 * need additional offset, otherwise reported
> -			 * temperature is below ambient temperature
> +			 * RevG desktop CPUs (i.e. no socket S1G1 or
> +			 * ASB1 parts) need additional offset,
> +			 * otherwise reported temperature is below
> +			 * ambient temperature
>  			 */
>  			data->temp_offset = 21000;
>  		}

I would like to avoid having to update this list each time a new CPU
model is released. Are there more K8 models scheduled to be released?

If there are, we should come up with a better test, either by
defaulting to no offset if all newer models don't need it, or by using
a socket-based detection as we do in the k10temp driver, assuming this
is available on the K8 as well.

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] hwmon,

[Jean Delvare]

Hi Andreas,

On Tue, 22 Jun 2010 13:24:03 +0200, Andreas Herrmann wrote:
> On Tue, Jun 22, 2010 at 01:35:33PM +0200, Jean Delvare wrote:
> > I would like to avoid having to update this list each time a new CPU
> > model is released. Are there more K8 models scheduled to be released?
> 
> I am not aware of other models. I think the ASB1 parts were the last
> updates of that CPU family. Those Athlon/Turion Neo CPUs are on the
> market for quite some time now. I've encountered that problem because
> I have such a CPU in my Laptop.
> 
> > If there are, we should come up with a better test, either by
> > defaulting to no offset if all newer models don't need it, or by using
> > a socket-based detection as we do in the k10temp driver, assuming this
> > is available on the K8 as well.
> 
> Unfortunately on K8 socket type information is not provided with CPUID
> like its done for family 0x10. Thus the socket type must be derived
> from the CPU revision and information contained in the CPU revision
> guide.
> 
> I think (hope) that this list needs no update anymore.

OK, let's keep the code the way it is then. I'll send the patch to
Linus for 2.6.35 with the next batch.

Thanks,
-- 
Jean Delvare

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

[lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

From: Andreas Herrmann <andreas.herrmann3@amd.com>

Commit 8bf0223ed515be24de0c671eedaff49e78bebc9c (hwmon, k8temp: Fix
temperature reporting for ASB1 processor revisions) fixed temperature
reporting for ASB1 CPUs. But one dual core CPU model (model 0x6b) was
packaged both as AM2 (desktop) and ASB1 (mobile). Thus the commit
leads to wrong temperature reporting for the AM2 CPU part.

The solution is to determine the package type for model 0x6b.

This is done using BrandId from CPUID Fn8000_0001_EBX[15:0]. See
"Constructing the processor Name String" in "Revision Guide for AMD
NPT Family 0Fh Processors" (Rev. 3.46).

Cc: Rudolf Marek <r.marek@assembler.cz>
Cc: stable@kernel.org [.32.x .34.x, .35.x]
Reported-by: Vladislav Guberinic <neosisani@gmail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 drivers/hwmon/k8temp.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

Sorry, but I missed that model 0x6b was also packaged as AM2.
Please apply this fix.


Thanks,

Andreas


diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
index 1fdd63e..bcb1a35 100644
--- a/drivers/hwmon/k8temp.c
+++ b/drivers/hwmon/k8temp.c
@@ -143,6 +143,28 @@ static const struct pci_device_id k8temp_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, k8temp_ids);
 
+static int is_rev_g_desktop(u8 model)
+{
+	u32 brandidx;
+
+	if (model < 0x69)
+		return 0;
+
+	if (model = 0xc1 || model = 0x6c || model = 0x7c ||
+	    model = 0x6f || model = 0x7f)
+		return 0;
+
+	if (model = 0x6b) {
+		/* differentiate between AM2 and ASB1 */
+		brandidx = cpuid_ebx(0x80000001);
+		brandidx = (brandidx >> 9) & 0x1f;
+		if (brandidx > 0xa)
+			return 0;
+	}
+
+	return 1;
+}
+
 static int __devinit k8temp_probe(struct pci_dev *pdev,
 				  const struct pci_device_id *id)
 {
@@ -179,9 +201,7 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 				 "wrong - check erratum #141\n");
 		}
 
-		if ((model >= 0x69) &&
-		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
-		      model = 0x6b || model = 0x6f || model = 0x7f)) {
+		if (is_rev_g_desktop(model))
 			/*
 			 * RevG desktop CPUs (i.e. no socket S1G1 or
 			 * ASB1 parts) need additional offset,
@@ -189,7 +209,6 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 			 * ambient temperature
 			 */
 			data->temp_offset = 21000;
-		}
 
 		break;
 	}
-- 
1.7.2


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

[lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

From: Andreas Herrmann <andreas.herrmann3@amd.com>

The family check in k8temp is not required because the driver is
already bound to a northbridge device only used with K8 CPUs.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 drivers/hwmon/k8temp.c |   49 +++++++++++++++++++++--------------------------
 1 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
index bcb1a35..2210b21 100644
--- a/drivers/hwmon/k8temp.c
+++ b/drivers/hwmon/k8temp.c
@@ -182,36 +182,31 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 	model = boot_cpu_data.x86_model;
 	stepping = boot_cpu_data.x86_mask;
 
-	switch (boot_cpu_data.x86) {
-	case 0xf:
-		/* feature available since SH-C0, exclude older revisions */
-		if (((model = 4) && (stepping = 0)) ||
-		    ((model = 5) && (stepping <= 1))) {
-			err = -ENODEV;
-			goto exit_free;
-		}
+	/* feature available since SH-C0, exclude older revisions */
+	if (((model = 4) && (stepping = 0)) ||
+	    ((model = 5) && (stepping <= 1))) {
+		err = -ENODEV;
+		goto exit_free;
+	}
+
+	/*
+	 * AMD NPT family 0fh, i.e. RevF and RevG:
+	 * meaning of SEL_CORE bit is inverted
+	 */
+	if (model >= 0x40) {
+		data->swap_core_select = 1;
+		dev_warn(&pdev->dev, "Temperature readouts might be "
+			 "wrong - check erratum #141\n");
+	}
 
+	if (is_rev_g_desktop(model))
 		/*
-		 * AMD NPT family 0fh, i.e. RevF and RevG:
-		 * meaning of SEL_CORE bit is inverted
+		 * RevG desktop CPUs (i.e. no socket S1G1 or
+		 * ASB1 parts) need additional offset,
+		 * otherwise reported temperature is below
+		 * ambient temperature
 		 */
-		if (model >= 0x40) {
-			data->swap_core_select = 1;
-			dev_warn(&pdev->dev, "Temperature readouts might be "
-				 "wrong - check erratum #141\n");
-		}
-
-		if (is_rev_g_desktop(model))
-			/*
-			 * RevG desktop CPUs (i.e. no socket S1G1 or
-			 * ASB1 parts) need additional offset,
-			 * otherwise reported temperature is below
-			 * ambient temperature
-			 */
-			data->temp_offset = 21000;
-
-		break;
-	}
+		data->temp_offset = 21000;
 
 	pci_read_config_byte(pdev, REG_TEMP, &scfg);
 	scfg &= ~(SEL_PLACE | SEL_CORE);		/* Select sensor 0, core0 */
-- 
1.7.2


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

Re: [lm-sensors] [PATCH] hwmon,

[Jean Delvare]

Hi Andreas,

On Mon, 16 Aug 2010 11:17:36 +0200, Andreas Herrmann wrote:
> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> 
> Commit 8bf0223ed515be24de0c671eedaff49e78bebc9c (hwmon, k8temp: Fix
> temperature reporting for ASB1 processor revisions) fixed temperature
> reporting for ASB1 CPUs. But one dual core CPU model (model 0x6b) was
> packaged both as AM2 (desktop) and ASB1 (mobile). Thus the commit
> leads to wrong temperature reporting for the AM2 CPU part.
> 
> The solution is to determine the package type for model 0x6b.
> 
> This is done using BrandId from CPUID Fn8000_0001_EBX[15:0]. See
> "Constructing the processor Name String" in "Revision Guide for AMD
> NPT Family 0Fh Processors" (Rev. 3.46).
> 
> Cc: Rudolf Marek <r.marek@assembler.cz>
> Cc: stable@kernel.org [.32.x .34.x, .35.x]
> Reported-by: Vladislav Guberinic <neosisani@gmail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  drivers/hwmon/k8temp.c |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
> 
> Sorry, but I missed that model 0x6b was also packaged as AM2.
> Please apply this fix.

Thanks for the heads up. I'm a little confused though, see below.

> diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> index 1fdd63e..bcb1a35 100644
> --- a/drivers/hwmon/k8temp.c
> +++ b/drivers/hwmon/k8temp.c
> @@ -143,6 +143,28 @@ static const struct pci_device_id k8temp_ids[] = {
>  
>  MODULE_DEVICE_TABLE(pci, k8temp_ids);
>  
> +static int is_rev_g_desktop(u8 model)
> +{
> +	u32 brandidx;
> +
> +	if (model < 0x69)
> +		return 0;
> +
> +	if (model = 0xc1 || model = 0x6c || model = 0x7c ||
> +	    model = 0x6f || model = 0x7f)
> +		return 0;
> +
> +	if (model = 0x6b) {
> +		/* differentiate between AM2 and ASB1 */
> +		brandidx = cpuid_ebx(0x80000001);
> +		brandidx = (brandidx >> 9) & 0x1f;
> +		if (brandidx > 0xa)

This will only catch the "AMD Sempron(tm) Processor 2RRU" entry, if I
read both the code and the datasheet right? Is this correct? What about
the 3 other ASB1 entries in table 8? And why are you comparing with 0xa
while this specific value doesn't match any CPU model?

> +			return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  static int __devinit k8temp_probe(struct pci_dev *pdev,
>  				  const struct pci_device_id *id)
>  {
> @@ -179,9 +201,7 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
>  				 "wrong - check erratum #141\n");
>  		}
>  
> -		if ((model >= 0x69) &&
> -		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> -		      model = 0x6b || model = 0x6f || model = 0x7f)) {
> +		if (is_rev_g_desktop(model))
>  			/*
>  			 * RevG desktop CPUs (i.e. no socket S1G1 or
>  			 * ASB1 parts) need additional offset,
> @@ -189,7 +209,6 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
>  			 * ambient temperature
>  			 */
>  			data->temp_offset = 21000;
> -		}
>  
>  		break;
>  	}

I also do not like the brace change. While technically correct, it
could lead to mistakes on future changes, because the large comment
doesn't make it immediately obvious that this is currently a
single-statement branch.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon,

[Jean Delvare]

On Mon, 16 Aug 2010 11:21:38 +0200, Andreas Herrmann wrote:
> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> 
> The family check in k8temp is not required because the driver is
> already bound to a northbridge device only used with K8 CPUs.

This check was added by... you:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h»9a35f293a3c8b5d57253cdfe2f29fa2627e1b9

But yes, I agree it's not needed. And actually, the driver wasn't even
failing to bind if a different CPU model was found.

> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  drivers/hwmon/k8temp.c |   49 +++++++++++++++++++++--------------------------
>  1 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> index bcb1a35..2210b21 100644
> --- a/drivers/hwmon/k8temp.c
> +++ b/drivers/hwmon/k8temp.c
> @@ -182,36 +182,31 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
>  	model = boot_cpu_data.x86_model;
>  	stepping = boot_cpu_data.x86_mask;
>  
> -	switch (boot_cpu_data.x86) {
> -	case 0xf:
> -		/* feature available since SH-C0, exclude older revisions */
> -		if (((model = 4) && (stepping = 0)) ||
> -		    ((model = 5) && (stepping <= 1))) {
> -			err = -ENODEV;
> -			goto exit_free;
> -		}
> +	/* feature available since SH-C0, exclude older revisions */
> +	if (((model = 4) && (stepping = 0)) ||
> +	    ((model = 5) && (stepping <= 1))) {
> +		err = -ENODEV;
> +		goto exit_free;
> +	}
> +
> +	/*
> +	 * AMD NPT family 0fh, i.e. RevF and RevG:
> +	 * meaning of SEL_CORE bit is inverted
> +	 */
> +	if (model >= 0x40) {
> +		data->swap_core_select = 1;
> +		dev_warn(&pdev->dev, "Temperature readouts might be "
> +			 "wrong - check erratum #141\n");
> +	}
>  
> +	if (is_rev_g_desktop(model))
>  		/*
> -		 * AMD NPT family 0fh, i.e. RevF and RevG:
> -		 * meaning of SEL_CORE bit is inverted
> +		 * RevG desktop CPUs (i.e. no socket S1G1 or
> +		 * ASB1 parts) need additional offset,
> +		 * otherwise reported temperature is below
> +		 * ambient temperature
>  		 */
> -		if (model >= 0x40) {
> -			data->swap_core_select = 1;
> -			dev_warn(&pdev->dev, "Temperature readouts might be "
> -				 "wrong - check erratum #141\n");
> -		}
> -
> -		if (is_rev_g_desktop(model))
> -			/*
> -			 * RevG desktop CPUs (i.e. no socket S1G1 or
> -			 * ASB1 parts) need additional offset,
> -			 * otherwise reported temperature is below
> -			 * ambient temperature
> -			 */
> -			data->temp_offset = 21000;
> -
> -		break;
> -	}
> +		data->temp_offset = 21000;
>  
>  	pci_read_config_byte(pdev, REG_TEMP, &scfg);
>  	scfg &= ~(SEL_PLACE | SEL_CORE);		/* Select sensor 0, core0 */

Applied, thanks (modulo the missing curly braces, which I've added back
myself.)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

On Tue, Aug 17, 2010 at 06:52:18PM +0200, Jean Delvare wrote:
> On Mon, 16 Aug 2010 11:21:38 +0200, Andreas Herrmann wrote:
> > From: Andreas Herrmann <andreas.herrmann3@amd.com>
> > 
> > The family check in k8temp is not required because the driver is
> > already bound to a northbridge device only used with K8 CPUs.
> 
> This check was added by... you:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h»9a35f293a3c8b5d57253cdfe2f29fa2627e1b9
> 
> But yes, I agree it's not needed. And actually, the driver wasn't even
> failing to bind if a different CPU model was found.

Shame on me.

(Maybe I've done this in preparation to add family 10h support. But we
have a separate driver for this. I can't remember the exact reason.)

> > Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> > ---
> >  drivers/hwmon/k8temp.c |   49 +++++++++++++++++++++--------------------------
> >  1 files changed, 22 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> > index bcb1a35..2210b21 100644
> > --- a/drivers/hwmon/k8temp.c
> > +++ b/drivers/hwmon/k8temp.c
> > @@ -182,36 +182,31 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  	model = boot_cpu_data.x86_model;
> >  	stepping = boot_cpu_data.x86_mask;
> >  
> > -	switch (boot_cpu_data.x86) {
> > -	case 0xf:
> > -		/* feature available since SH-C0, exclude older revisions */
> > -		if (((model = 4) && (stepping = 0)) ||
> > -		    ((model = 5) && (stepping <= 1))) {
> > -			err = -ENODEV;
> > -			goto exit_free;
> > -		}
> > +	/* feature available since SH-C0, exclude older revisions */
> > +	if (((model = 4) && (stepping = 0)) ||
> > +	    ((model = 5) && (stepping <= 1))) {
> > +		err = -ENODEV;
> > +		goto exit_free;
> > +	}
> > +
> > +	/*
> > +	 * AMD NPT family 0fh, i.e. RevF and RevG:
> > +	 * meaning of SEL_CORE bit is inverted
> > +	 */
> > +	if (model >= 0x40) {
> > +		data->swap_core_select = 1;
> > +		dev_warn(&pdev->dev, "Temperature readouts might be "
> > +			 "wrong - check erratum #141\n");
> > +	}
> >  
> > +	if (is_rev_g_desktop(model))
> >  		/*
> > -		 * AMD NPT family 0fh, i.e. RevF and RevG:
> > -		 * meaning of SEL_CORE bit is inverted
> > +		 * RevG desktop CPUs (i.e. no socket S1G1 or
> > +		 * ASB1 parts) need additional offset,
> > +		 * otherwise reported temperature is below
> > +		 * ambient temperature
> >  		 */
> > -		if (model >= 0x40) {
> > -			data->swap_core_select = 1;
> > -			dev_warn(&pdev->dev, "Temperature readouts might be "
> > -				 "wrong - check erratum #141\n");
> > -		}
> > -
> > -		if (is_rev_g_desktop(model))
> > -			/*
> > -			 * RevG desktop CPUs (i.e. no socket S1G1 or
> > -			 * ASB1 parts) need additional offset,
> > -			 * otherwise reported temperature is below
> > -			 * ambient temperature
> > -			 */
> > -			data->temp_offset = 21000;
> > -
> > -		break;
> > -	}
> > +		data->temp_offset = 21000;
> >  
> >  	pci_read_config_byte(pdev, REG_TEMP, &scfg);
> >  	scfg &= ~(SEL_PLACE | SEL_CORE);		/* Select sensor 0, core0 */
> 
> Applied, thanks (modulo the missing curly braces, which I've added back
> myself.)

Oops.

Thanks,

Andreas

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

Re: [lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

On Tue, Aug 17, 2010 at 06:21:38PM +0200, Jean Delvare wrote:

 [...]

> > +static int is_rev_g_desktop(u8 model)
> > +{
> > +	u32 brandidx;
> > +
> > +	if (model < 0x69)
> > +		return 0;
> > +
> > +	if (model = 0xc1 || model = 0x6c || model = 0x7c ||
> > +	    model = 0x6f || model = 0x7f)
> > +		return 0;
> > +
> > +	if (model = 0x6b) {
> > +		/* differentiate between AM2 and ASB1 */
> > +		brandidx = cpuid_ebx(0x80000001);
> > +		brandidx = (brandidx >> 9) & 0x1f;
> > +		if (brandidx > 0xa)
> 
> This will only catch the "AMD Sempron(tm) Processor 2RRU" entry, if I
> read both the code and the datasheet right?

No, it will catch only the dual-core CPUs

	0Bh 0h AMD Turion(tm) Neo X2 Dual Core Processor L6RR (ASB1)
	0Ch 0h AMD Athlon(tm) Neo X2 Dual Core Processor L3RR (ASB1)

See revision 3.46 of the RG:
http://support.amd.com/us/Processor_TechDocs/33610.pdf


> Is this correct? What about the 3 other ASB1 entries in table 8? And
> why are you comparing with 0xa while this specific value doesn't
> match any CPU model?

Hmm, wait let me double check this. (Thought the single core variants were
not affected by this problem)

Damn!

Need to update this patch. I did not check table 2 which lists
single-core AM2 CPUs.

model 0x6f and 0x7f (both single core) are also provided as ASB1 and
AM2.

> > +			return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> >  static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  				  const struct pci_device_id *id)
> >  {
> > @@ -179,9 +201,7 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  				 "wrong - check erratum #141\n");
> >  		}
> >  
> > -		if ((model >= 0x69) &&
> > -		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> > -		      model = 0x6b || model = 0x6f || model = 0x7f)) {
> > +		if (is_rev_g_desktop(model))
> >  			/*
> >  			 * RevG desktop CPUs (i.e. no socket S1G1 or
> >  			 * ASB1 parts) need additional offset,
> > @@ -189,7 +209,6 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  			 * ambient temperature
> >  			 */
> >  			data->temp_offset = 21000;
> > -		}
> >  
> >  		break;
> >  	}
> 
> I also do not like the brace change. While technically correct, it
> could lead to mistakes on future changes, because the large comment
> doesn't make it immediately obvious that this is currently a
> single-statement branch.

Ok.

Thanks a lot for reviewing.
I'll come up with an updated patch.



Andreas

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

[lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

From: Andreas Herrmann <andreas.herrmann3@amd.com>

Commit 8bf0223ed515be24de0c671eedaff49e78bebc9c (hwmon, k8temp: Fix
temperature reporting for ASB1 processor revisions) fixed temperature
reporting for ASB1 CPUs. But those CPU models (model 0x6b, 0x6f, 0x7f)
were packaged both as AM2 (desktop) and ASB1 (mobile). Thus the commit
leads to wrong temperature reporting for AM2 CPU parts.

The solution is to determine the package type for models 0x6b, 0x6f,
0x7f.

This is done using BrandId from CPUID Fn8000_0001_EBX[15:0]. See
"Constructing the processor Name String" in "Revision Guide for AMD
NPT Family 0Fh Processors" (Rev. 3.46).

Cc: Rudolf Marek <r.marek@assembler.cz>
Cc: stable@kernel.org [.32.x .34.x, .35.x]
Reported-by: Vladislav Guberinic <neosisani@gmail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 drivers/hwmon/k8temp.c |   44 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 34 insertions(+), 10 deletions(-)

Here is the updated patch considering single core and dual core ASB1
CPUs. Tested with model 0x6b ASB1 CPU. I don't have instant access to
model 0x6f and 0x7f CPU models.


Andreas


diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
index b9bb3e0..bbfc0ad 100644
--- a/drivers/hwmon/k8temp.c
+++ b/drivers/hwmon/k8temp.c
@@ -143,6 +143,34 @@ static const struct pci_device_id k8temp_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, k8temp_ids);
 
+static int is_rev_g_desktop(u8 model)
+{
+	u32 brandidx;
+
+	if (model < 0x69)
+		return 0;
+
+	if (model = 0xc1 || model = 0x6c || model = 0x7c)
+		return 0;
+
+	/*
+	 * differentiate between AM2 and ASB1
+	 */
+
+	brandidx = cpuid_ebx(0x80000001);
+	brandidx = (brandidx >> 9) & 0x1f;
+	if (model = 0x6b &&
+	    (brandidx = 0xb || brandidx = 0xc))
+		return 0;
+
+	if ((model = 0x6f || model = 0x7f) &&
+	    (brandidx = 7 || brandidx = 9 ||
+	     brandidx = 0xc))
+		return 0;
+
+	return 1;
+}
+
 static int __devinit k8temp_probe(struct pci_dev *pdev,
 				  const struct pci_device_id *id)
 {
@@ -179,17 +207,13 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 				 "wrong - check erratum #141\n");
 		}
 
-		if ((model >= 0x69) &&
-		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
-		      model = 0x6b || model = 0x6f || model = 0x7f)) {
-			/*
-			 * RevG desktop CPUs (i.e. no socket S1G1 or
-			 * ASB1 parts) need additional offset,
-			 * otherwise reported temperature is below
-			 * ambient temperature
-			 */
+		/*
+		 * RevG desktop CPUs (i.e. no socket S1G1 or ASB1
+		 * parts) need additional offset, otherwise reported
+		 * temperature is below ambient temperature
+		 */
+		if (is_rev_g_desktop(model))
 			data->temp_offset = 21000;
-		}
 
 		break;
 	}
-- 
1.7.2


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

Re: [lm-sensors] [PATCH] hwmon,

[Jean Delvare]

Hi Andreas,

On Wed, 18 Aug 2010 11:10:52 +0200, Andreas Herrmann wrote:
> On Tue, Aug 17, 2010 at 06:21:38PM +0200, Jean Delvare wrote:
> 
>  [...]
> 
> > > +static int is_rev_g_desktop(u8 model)
> > > +{
> > > +	u32 brandidx;
> > > +
> > > +	if (model < 0x69)
> > > +		return 0;
> > > +
> > > +	if (model = 0xc1 || model = 0x6c || model = 0x7c ||
> > > +	    model = 0x6f || model = 0x7f)
> > > +		return 0;
> > > +
> > > +	if (model = 0x6b) {
> > > +		/* differentiate between AM2 and ASB1 */
> > > +		brandidx = cpuid_ebx(0x80000001);
> > > +		brandidx = (brandidx >> 9) & 0x1f;
> > > +		if (brandidx > 0xa)
> > 
> > This will only catch the "AMD Sempron(tm) Processor 2RRU" entry, if I
> > read both the code and the datasheet right?
> 
> No, it will catch only the dual-core CPUs
> 
> 	0Bh 0h AMD Turion(tm) Neo X2 Dual Core Processor L6RR (ASB1)
> 	0Ch 0h AMD Athlon(tm) Neo X2 Dual Core Processor L3RR (ASB1)
> 
> See revision 3.46 of the RG:
> http://support.amd.com/us/Processor_TechDocs/33610.pdf

Oh, my bad. I didn't see that table 8 was spanning over a 3rd page. Now
it makes more sense.

> > Is this correct? What about the 3 other ASB1 entries in table 8? And
> > why are you comparing with 0xa while this specific value doesn't
> > match any CPU model?
> 
> Hmm, wait let me double check this. (Thought the single core variants were
> not affected by this problem)
> 
> Damn!
> 
> Need to update this patch. I did not check table 2 which lists
> single-core AM2 CPUs.
> 
> model 0x6f and 0x7f (both single core) are also provided as ASB1 and
> AM2.

Oh, I'm glad my mistake helped somehow ;) We'd rather get it right this
time. We already introduced a regression in stable kernels, which is
very bad and should never happen, please let's not do it again.

> 
> > > +			return 0;
> > > +	}
> > > +
> > > +	return 1;
> > > +}
> > > +
> > >  static int __devinit k8temp_probe(struct pci_dev *pdev,
> > >  				  const struct pci_device_id *id)
> > >  {
> > > @@ -179,9 +201,7 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> > >  				 "wrong - check erratum #141\n");
> > >  		}
> > >  
> > > -		if ((model >= 0x69) &&
> > > -		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> > > -		      model = 0x6b || model = 0x6f || model = 0x7f)) {
> > > +		if (is_rev_g_desktop(model))
> > >  			/*
> > >  			 * RevG desktop CPUs (i.e. no socket S1G1 or
> > >  			 * ASB1 parts) need additional offset,
> > > @@ -189,7 +209,6 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> > >  			 * ambient temperature
> > >  			 */
> > >  			data->temp_offset = 21000;
> > > -		}
> > >  
> > >  		break;
> > >  	}
> > 
> > I also do not like the brace change. While technically correct, it
> > could lead to mistakes on future changes, because the large comment
> > doesn't make it immediately obvious that this is currently a
> > single-statement branch.
> 
> Ok.
> 
> Thanks a lot for reviewing.
> I'll come up with an updated patch.

OK, I'll wait for it.

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] hwmon,

[Jean Delvare]

Hi Andreas,

On Wed, 18 Aug 2010 14:13:00 +0200, Andreas Herrmann wrote:
> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> 
> Commit 8bf0223ed515be24de0c671eedaff49e78bebc9c (hwmon, k8temp: Fix
> temperature reporting for ASB1 processor revisions) fixed temperature
> reporting for ASB1 CPUs. But those CPU models (model 0x6b, 0x6f, 0x7f)
> were packaged both as AM2 (desktop) and ASB1 (mobile). Thus the commit
> leads to wrong temperature reporting for AM2 CPU parts.
> 
> The solution is to determine the package type for models 0x6b, 0x6f,
> 0x7f.
> 
> This is done using BrandId from CPUID Fn8000_0001_EBX[15:0]. See
> "Constructing the processor Name String" in "Revision Guide for AMD
> NPT Family 0Fh Processors" (Rev. 3.46).
> 
> Cc: Rudolf Marek <r.marek@assembler.cz>
> Cc: stable@kernel.org [.32.x .34.x, .35.x]
> Reported-by: Vladislav Guberinic <neosisani@gmail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  drivers/hwmon/k8temp.c |   44 ++++++++++++++++++++++++++++++++++----------
>  1 files changed, 34 insertions(+), 10 deletions(-)
> 
> Here is the updated patch considering single core and dual core ASB1
> CPUs. Tested with model 0x6b ASB1 CPU. I don't have instant access to
> model 0x6f and 0x7f CPU models.

Great, thanks. Looks much more in line with the datasheet.

I presume we will have to update is_rev_g_desktop() every time a new
ASB1 K8 processor is released? :(

> diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> index b9bb3e0..bbfc0ad 100644
> --- a/drivers/hwmon/k8temp.c
> +++ b/drivers/hwmon/k8temp.c
> @@ -143,6 +143,34 @@ static const struct pci_device_id k8temp_ids[] = {
>  
>  MODULE_DEVICE_TABLE(pci, k8temp_ids);
>  
> +static int is_rev_g_desktop(u8 model)

Can be marked __devinit.

> +{
> +	u32 brandidx;
> +
> +	if (model < 0x69)
> +		return 0;
> +
> +	if (model = 0xc1 || model = 0x6c || model = 0x7c)
> +		return 0;
> +
> +	/*
> +	 * differentiate between AM2 and ASB1
> +	 */
> +
> +	brandidx = cpuid_ebx(0x80000001);
> +	brandidx = (brandidx >> 9) & 0x1f;
> +	if (model = 0x6b &&
> +	    (brandidx = 0xb || brandidx = 0xc))
> +		return 0;
> +
> +	if ((model = 0x6f || model = 0x7f) &&
> +	    (brandidx = 7 || brandidx = 9 ||

I've made these constants hexadecimal for consistency. I've also
inverted the tests so they are in the same order as in the datasheet,
and added some comments to make the code easier to follow.

> +	     brandidx = 0xc))
> +		return 0;
> +
> +	return 1;
> +}
> +
>  static int __devinit k8temp_probe(struct pci_dev *pdev,
>  				  const struct pci_device_id *id)
>  {
> @@ -179,17 +207,13 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
>  				 "wrong - check erratum #141\n");
>  		}
>  
> -		if ((model >= 0x69) &&
> -		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> -		      model = 0x6b || model = 0x6f || model = 0x7f)) {
> -			/*
> -			 * RevG desktop CPUs (i.e. no socket S1G1 or
> -			 * ASB1 parts) need additional offset,
> -			 * otherwise reported temperature is below
> -			 * ambient temperature
> -			 */
> +		/*
> +		 * RevG desktop CPUs (i.e. no socket S1G1 or ASB1
> +		 * parts) need additional offset, otherwise reported
> +		 * temperature is below ambient temperature
> +		 */
> +		if (is_rev_g_desktop(model))
>  			data->temp_offset = 21000;
> -		}
>  
>  		break;
>  	}

I've adjusted your second patch so that it applies cleanly. Both
patches after my changes are available at:
  ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/
Please check.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon,

[Andreas Herrmann]

On Thu, Aug 19, 2010 at 10:34:42AM +0200, Jean Delvare wrote:
  [...]
> > Here is the updated patch considering single core and dual core ASB1
> > CPUs. Tested with model 0x6b ASB1 CPU. I don't have instant access to
> > model 0x6f and 0x7f CPU models.
> 
> Great, thanks. Looks much more in line with the datasheet.
> 
> I presume we will have to update is_rev_g_desktop() every time a new
> ASB1 K8 processor is released? :(

My personnel assumption is that the number of new ASB1 K8 models (if
any) is quite low.

> > diff --git a/drivers/hwmon/k8temp.c b/drivers/hwmon/k8temp.c
> > index b9bb3e0..bbfc0ad 100644
> > --- a/drivers/hwmon/k8temp.c
> > +++ b/drivers/hwmon/k8temp.c
> > @@ -143,6 +143,34 @@ static const struct pci_device_id k8temp_ids[] = {
> >  
> >  MODULE_DEVICE_TABLE(pci, k8temp_ids);
> >  
> > +static int is_rev_g_desktop(u8 model)
> 
> Can be marked __devinit.

Ok.

> > +{
> > +	u32 brandidx;
> > +
> > +	if (model < 0x69)
> > +		return 0;
> > +
> > +	if (model = 0xc1 || model = 0x6c || model = 0x7c)
> > +		return 0;
> > +
> > +	/*
> > +	 * differentiate between AM2 and ASB1
> > +	 */
> > +
> > +	brandidx = cpuid_ebx(0x80000001);
> > +	brandidx = (brandidx >> 9) & 0x1f;
> > +	if (model = 0x6b &&
> > +	    (brandidx = 0xb || brandidx = 0xc))
> > +		return 0;
> > +
> > +	if ((model = 0x6f || model = 0x7f) &&
> > +	    (brandidx = 7 || brandidx = 9 ||
> 
> I've made these constants hexadecimal for consistency. I've also
> inverted the tests so they are in the same order as in the datasheet,
> and added some comments to make the code easier to follow.

Fine with me.

> > +	     brandidx = 0xc))
> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +
> >  static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  				  const struct pci_device_id *id)
> >  {
> > @@ -179,17 +207,13 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
> >  				 "wrong - check erratum #141\n");
> >  		}
> >  
> > -		if ((model >= 0x69) &&
> > -		    !(model = 0xc1 || model = 0x6c || model = 0x7c ||
> > -		      model = 0x6b || model = 0x6f || model = 0x7f)) {
> > -			/*
> > -			 * RevG desktop CPUs (i.e. no socket S1G1 or
> > -			 * ASB1 parts) need additional offset,
> > -			 * otherwise reported temperature is below
> > -			 * ambient temperature
> > -			 */
> > +		/*
> > +		 * RevG desktop CPUs (i.e. no socket S1G1 or ASB1
> > +		 * parts) need additional offset, otherwise reported
> > +		 * temperature is below ambient temperature
> > +		 */
> > +		if (is_rev_g_desktop(model))
> >  			data->temp_offset = 21000;
> > -		}
> >  
> >  		break;
> >  	}
> 
> I've adjusted your second patch so that it applies cleanly. Both
> patches after my changes are available at:
>   ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/
> Please check.

It looks good.


Thanks,

Andreas

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