[lm-sensors] [PATCH v3] hwmon-vid: Fix AMD K8 VID decoding

[lm-sensors] [PATCH v3] hwmon-vid: Fix AMD K8 VID decoding

[Jean Delvare]

Not all AMD K8 have 6 VID pins, contrary to what was assumed in
commit 116d0486bdefc11f71e567cadf0c47f788b4dd06. This commit broke
support of older CPU models which have only 5 VID pins:
http://bugzilla.kernel.org/show_bug.cgi?id\x11329

We need two entries in the hwmon-vid table, one for 5-bit VID models
(K8 revision <= E) and one for 6-bit VID models (K8 revision >= F).
This fixes bug #11329.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Frank Myhr <fmyhr@fhmtech.com>
Tested-by: Jean-Luc Coulon <jean.luc.coulon@gmail.com>
---
 drivers/hwmon/hwmon-vid.c |   36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

--- linux-2.6.27-rc3.orig/drivers/hwmon/hwmon-vid.c	2008-08-13 09:50:50.000000000 +0200
+++ linux-2.6.27-rc3/drivers/hwmon/hwmon-vid.c	2008-08-15 09:48:17.000000000 +0200
@@ -37,13 +37,21 @@
  * For VRD 10.0 and up, "VRD x.y Design Guide",
  * available at http://developer.intel.com/.
  *
- * AMD NPT 0Fh (Athlon64 & Opteron), AMD Publication 32559,
+ * AMD Athlon 64 and AMD Opteron Processors, AMD Publication 26094,
+ * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/26094.PDF
+ * Table 74. VID Code Voltages
+ * This corresponds to an arbitrary VRM code of 24 in the functions below.
+ * These CPU models (K8 revision <= E) have 5 VID pins. See also:
+ * Revision Guide for AMD Athlon 64 and AMD Opteron Processors, AMD Publication 25759,
+ * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/25759.pdf
+ *
+ * AMD NPT Family 0Fh Processors, AMD Publication 32559,
  * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/32559.pdf
  * Table 71. VID Code Voltages
- * AMD Opteron processors don't follow the Intel specifications.
- * I'm going to "make up" 2.4 as the spec number for the Opterons.
- * No good reason just a mnemonic for the 24x Opteron processor
- * series.
+ * This corresponds to an arbitrary VRM code of 25 in the functions below.
+ * These CPU models (K8 revision >= F) have 6 VID pins. See also:
+ * Revision Guide for AMD NPT Family 0Fh Processors, AMD Publication 33610,
+ * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/33610.pdf
  *
  * The 17 specification is in fact Intel Mobile Voltage Positioning -
  * (IMVP-II). You can find more information in the datasheet of Max1718
@@ -95,7 +103,12 @@ int vid_from_reg(int val, u8 vrm)
 			return 0;
 		return((1600000 - (val - 2) * 6250 + 500) / 1000);
 
-	case 24:		/* AMD NPT 0Fh (Athlon64 & Opteron) */
+	case 24:		/* Athlon64 & Opteron */
+		val &= 0x1f;
+		if (val = 0x1f)
+			return 0;
+				/* fall through */
+	case 25:		/* AMD NPT 0Fh */
 		val &= 0x3f;
 		return (val < 32) ? 1550 - 25 * val
 			: 775 - (25 * (val - 31)) / 2;
@@ -157,11 +170,16 @@ struct vrm_model {
 
 #ifdef CONFIG_X86
 
-/* the stepping parameter is highest acceptable stepping for current line */
+/*
+ * The stepping parameter is highest acceptable stepping for current line.
+ * The model match must be exact for 4-bit values. For model values 0x10
+ * and above (extended model), all models below the parameter will match.
+ */
 
 static struct vrm_model vrm_models[] = {
 	{X86_VENDOR_AMD, 0x6, ANY, ANY, 90},		/* Athlon Duron etc */
-	{X86_VENDOR_AMD, 0xF, ANY, ANY, 24},		/* Athlon 64, Opteron and above VRM 24 */
+	{X86_VENDOR_AMD, 0xF, 0x3F, ANY, 24},		/* Athlon 64, Opteron */
+	{X86_VENDOR_AMD, 0xF, ANY, ANY, 25},		/* NPT family 0Fh */
 	{X86_VENDOR_INTEL, 0x6, 0x9, ANY, 13},		/* Pentium M (130 nm) */
 	{X86_VENDOR_INTEL, 0x6, 0xB, ANY, 85},		/* Tualatin */
 	{X86_VENDOR_INTEL, 0x6, 0xD, ANY, 13},		/* Pentium M (90 nm) */
@@ -189,6 +207,8 @@ static u8 find_vrm(u8 eff_family, u8 eff
 		if (vrm_models[i].vendor=vendor)
 			if ((vrm_models[i].eff_family=eff_family)
 			 && ((vrm_models[i].eff_model=eff_model) ||
+			     (vrm_models[i].eff_model >= 0x10 &&
+			      eff_model <= vrm_models[i].eff_model) ||
 			     (vrm_models[i].eff_model=ANY)) &&
 			     (eff_stepping <= vrm_models[i].eff_stepping))
 				return vrm_models[i].vrm_type;

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v3] hwmon-vid: Fix AMD K8 VID decoding

[Frank Myhr]

Hi Jean,

Your v3 patch looks good to me. A few comments below:


Jean Delvare wrote:
> Not all AMD K8 have 6 VID pins, contrary to what was assumed in
> commit 116d0486bdefc11f71e567cadf0c47f788b4dd06. This commit broke
> support of older CPU models which have only 5 VID pins:
> http://bugzilla.kernel.org/show_bug.cgi?id\x11329
> 
> We need two entries in the hwmon-vid table, one for 5-bit VID models
> (K8 revision <= E) and one for 6-bit VID models (K8 revision >= F).
> This fixes bug #11329.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Acked-by: Frank Myhr <fmyhr@fhmtech.com>
> Tested-by: Jean-Luc Coulon <jean.luc.coulon@gmail.com>
> ---
>  drivers/hwmon/hwmon-vid.c |   36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> --- linux-2.6.27-rc3.orig/drivers/hwmon/hwmon-vid.c	2008-08-13 09:50:50.000000000 +0200
> +++ linux-2.6.27-rc3/drivers/hwmon/hwmon-vid.c	2008-08-15 09:48:17.000000000 +0200
> @@ -37,13 +37,21 @@
>   * For VRD 10.0 and up, "VRD x.y Design Guide",
>   * available at http://developer.intel.com/.
>   *
> - * AMD NPT 0Fh (Athlon64 & Opteron), AMD Publication 32559,
> + * AMD Athlon 64 and AMD Opteron Processors, AMD Publication 26094,
> + * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/26094.PDF
> + * Table 74. VID Code Voltages
> + * This corresponds to an arbitrary VRM code of 24 in the functions below.
> + * These CPU models (K8 revision <= E) have 5 VID pins. See also:
> + * Revision Guide for AMD Athlon 64 and AMD Opteron Processors, AMD Publication 25759,
> + * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/25759.pdf
> + *
> + * AMD NPT Family 0Fh Processors, AMD Publication 32559,
>   * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/32559.pdf
>   * Table 71. VID Code Voltages
> - * AMD Opteron processors don't follow the Intel specifications.
> - * I'm going to "make up" 2.4 as the spec number for the Opterons.
> - * No good reason just a mnemonic for the 24x Opteron processor
> - * series.
> + * This corresponds to an arbitrary VRM code of 25 in the functions below.
> + * These CPU models (K8 revision >= F) have 6 VID pins. See also:
> + * Revision Guide for AMD NPT Family 0Fh Processors, AMD Publication 33610,
> + * http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/33610.pdf

Succinct explanation with refs, I like it!


> -	case 24:		/* AMD NPT 0Fh (Athlon64 & Opteron) */
> +	case 24:		/* Athlon64 & Opteron */
> +		val &= 0x1f;
> +		if (val = 0x1f)
> +			return 0;

Nice catch that all bits set is an error (voltage "off", impossible if code is
running) for the 5-vid-bit cpu's but not the 6-vid-bit ones in case 25


> +				/* fall through */
> +	case 25:		/* AMD NPT 0Fh */
>  		val &= 0x3f;
>  		return (val < 32) ? 1550 - 25 * val
>  			: 775 - (25 * (val - 31)) / 2;

The above formula is correct, and I'm the one who put it here in this form. But
in looking at AMD cpu specs yesterday I came across:

AMD 31116, BIOS and Kernel Developer's Guide (BKDG) For AMD Family 10h Processors
http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/31116.PDF
section 2.4.1.5.2, p. 30

This gives the canonical formula for K10's, which happens to be the same as for
the 6-vid-pin K8's:

return (val >= 0x20) ? (7625 - 125 * (val - 0x20)) / 10
	: 1550 - 25 * val;

This gives the same result and is easier to compare with the AMD reference
above. Perhaps we should make this change, I'll leave it up to you.


Thanks,
Frank

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

Re: [lm-sensors] [PATCH v3] hwmon-vid: Fix AMD K8 VID decoding

[Jean Delvare]

Hi Frank,

On Fri, 15 Aug 2008 08:00:13 -0400, Frank Myhr wrote:
> Jean Delvare wrote:
> > -	case 24:		/* AMD NPT 0Fh (Athlon64 & Opteron) */
> > +	case 24:		/* Athlon64 & Opteron */
> > +		val &= 0x1f;
> > +		if (val = 0x1f)
> > +			return 0;
> 
> Nice catch that all bits set is an error (voltage "off", impossible if code is
> running) for the 5-vid-bit cpu's but not the 6-vid-bit ones in case 25

Not necessarily impossible. Think of hot-pluggable CPUs...

> > +				/* fall through */
> > +	case 25:		/* AMD NPT 0Fh */
> >  		val &= 0x3f;
> >  		return (val < 32) ? 1550 - 25 * val
> >  			: 775 - (25 * (val - 31)) / 2;
> 
> The above formula is correct, and I'm the one who put it here in this form. But
> in looking at AMD cpu specs yesterday I came across:
> 
> AMD 31116, BIOS and Kernel Developer's Guide (BKDG) For AMD Family 10h Processors
> http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/31116.PDF
> section 2.4.1.5.2, p. 30
> 
> This gives the canonical formula for K10's, which happens to be the same as for
> the 6-vid-pin K8's:
> 
> return (val >= 0x20) ? (7625 - 125 * (val - 0x20)) / 10
> 	: 1550 - 25 * val;
> 
> This gives the same result and is easier to compare with the AMD reference
> above. Perhaps we should make this change, I'll leave it up to you.

Why not, but let's not push this now. We're already late in the release
cycle, all we want to do at this point is fix regressions in the code.

So, feel free to send an incremental patch changing the formula in the
code; we can have this in kernel 2.6.28.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v3] hwmon-vid: Fix AMD K8 VID decoding

[Frank Myhr]

Jean Delvare wrote:
> > Hi Frank,
> >
> > On Fri, 15 Aug 2008 08:00:13 -0400, Frank Myhr wrote:
>> >> Jean Delvare wrote:
>>> >>> -	case 24:		/* AMD NPT 0Fh (Athlon64 & Opteron) */
>>> >>> +	case 24:		/* Athlon64 & Opteron */
>>> >>> +		val &= 0x1f;
>>> >>> +		if (val = 0x1f)
>>> >>> +			return 0;
>> >> Nice catch that all bits set is an error (voltage "off", impossible if code is
>> >> running) for the 5-vid-bit cpu's but not the 6-vid-bit ones in case 25
> >
> > Not necessarily impossible. Think of hot-pluggable CPUs...

Good point! I've got to try harder to think outside my particular box...


> >
>>> >>> +				/* fall through */
>>> >>> +	case 25:		/* AMD NPT 0Fh */
>>> >>>  		val &= 0x3f;
>>> >>>  		return (val < 32) ? 1550 - 25 * val
>>> >>>  			: 775 - (25 * (val - 31)) / 2;
>> >> The above formula is correct, and I'm the one who put it here in this
form. But
>> >> in looking at AMD cpu specs yesterday I came across:
>> >>
>> >> AMD 31116, BIOS and Kernel Developer's Guide (BKDG) For AMD Family 10h
Processors
>> >>
http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/31116.PDF
>> >> section 2.4.1.5.2, p. 30
>> >>
>> >> This gives the canonical formula for K10's, which happens to be the same
as for
>> >> the 6-vid-pin K8's:
>> >>
>> >> return (val >= 0x20) ? (7625 - 125 * (val - 0x20)) / 10
>> >> 	: 1550 - 25 * val;
>> >>
>> >> This gives the same result and is easier to compare with the AMD reference
>> >> above. Perhaps we should make this change, I'll leave it up to you.
> >
> > Why not, but let's not push this now. We're already late in the release
> > cycle, all we want to do at this point is fix regressions in the code.
> >
> > So, feel free to send an incremental patch changing the formula in the
> > code; we can have this in kernel 2.6.28.

Makes sense to me. I'll plan to submit patch against released 2.6.27.


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