Skip to contentSkip to conversation

hwmon: (k8temp) adapt cpuid handling,

PATCH In review Andreas Herrmann 2 participants 6 messages Dec 5, 2008 – Dec 15, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/4 hwmon: (k8temp) adapt cpuid handling,

---
 drivers/hwmon/k8temp.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)
drivers/hwmon/k8temp.c +12 -15
index 3b90384..712c208 100644
@@ -31,6 +31,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <asm/processor.h>
 
 #define TEMP_FROM_REG(val)	(((((val) >> 16) & 0xff) - 49) * 1000)
 #define REG_TEMP	0xe4
@@ -48,7 +49,6 @@ struct k8temp_data {
 	/* registers values */
 	u8 sensorsp;		/* sensor presence bits - SEL_CORE & SEL_PLACE */
 	u32 temp[2][2];		/* core, place */
-	u8 fam;
 };
 
 static struct k8temp_data *k8temp_update_device(struct device *dev)
@@ -143,30 +143,27 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 	int err;
 	u8 scfg;
 	u32 temp;
+	u8 model, stepping;
 	struct k8temp_data *data;
-	u32 cpuid = cpuid_eax(1);
-
-	/* this feature should be available since SH-C0 core */
-	if ((cpuid = 0xf40) || (cpuid = 0xf50) || (cpuid = 0xf51)) {
-		err = -ENODEV;
-		goto exit;
-	}
 
 	if (!(data = kzalloc(sizeof(struct k8temp_data), GFP_KERNEL))) {
 		err = -ENOMEM;
 		goto exit;
 	}
 
-	/* get real PCI based cpuid, prior revF of fam 0Fh, this reg is 0 */
-	pci_read_config_dword(pdev, REG_CPUID, &cpuid);
-
-	data->fam = (cpuid & 0x00000f00) >> 8;
-	data->fam += (cpuid & 0x0ff00000) >> 20;
+	model = boot_cpu_data.x86_model;
+	stepping = boot_cpu_data.x86_mask;
 
-	switch (data->fam) {
+	switch (boot_cpu_data.x86) {
 	case 0xf:
+		/* feature available since SH-C0, exclude older revisions */
+		if (((model = 4) && (stepping = 0)) ||
+		    ((model = 5) && ((stepping = 0) || (stepping = 1)))) {
+			err = -ENODEV;
+			goto exit;
+		}
 		dev_warn(&pdev->dev, "Temperature readouts might be wrong"
-					" - check errata #141\n");
+			 " - check erratum #141\n");
 		break;
 	}
 
Signature · 9 lines
1.6.0.4




_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
maintainer
#1 lore raw

Hello,

Sorry for the delay.

Quoted 1 line

Maybe also some desc. Well the reason why the code below looks like is that

1) avoid very old systems - the idea that revs of CPU will be similar/same
2) get the cpuid from PCI. This is good, because some CPU may have the problem, some other may not.

Your patch reduces this to boot CPU checks. The idea was to have the driver know exact CPUID for each PCI driver instance. It was prepared to situation when some CPU steppings will have the fix, some not.

Please tell me why it is not necessary or why you changed that ;)

Thanks,
Rudolf

Quoted 57 lines
---
 drivers/hwmon/k8temp.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)
drivers/hwmon/k8temp.c +8 -14
index 3b90384..712c208 100644
@@ -31,6 +31,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <asm/processor.h>
 
 #define TEMP_FROM_REG(val)	(((((val) >> 16) & 0xff) - 49) * 1000)
 #define REG_TEMP	0xe4
@@ -48,7 +49,6 @@ struct k8temp_data {
 	/* registers values */
 	u8 sensorsp;		/* sensor presence bits - SEL_CORE & SEL_PLACE */
 	u32 temp[2][2];		/* core, place */
-	u8 fam;
 };
 
 static struct k8temp_data *k8temp_update_device(struct device *dev)
@@ -143,30 +143,27 @@ static int __devinit k8temp_probe(struct pci_dev *pdev,
 	int err;
 	u8 scfg;
 	u32 temp;
+	u8 model, stepping;
 	struct k8temp_data *data;
-	u32 cpuid = cpuid_eax(1);
-
-	/* this feature should be available since SH-C0 core */
-	if ((cpuid = 0xf40) || (cpuid = 0xf50) || (cpuid = 0xf51)) {
-		err = -ENODEV;
-		goto exit;
-	}
 
 	if (!(data = kzalloc(sizeof(struct k8temp_data), GFP_KERNEL))) {
 		err = -ENOMEM;
 		goto exit;
 	}
 
-	/* get real PCI based cpuid, prior revF of fam 0Fh, this reg is 0 */
-	pci_read_config_dword(pdev, REG_CPUID, &cpuid);
-
-	data->fam = (cpuid & 0x00000f00) >> 8;
-	data->fam += (cpuid & 0x0ff00000) >> 20;
+	model = boot_cpu_data.x86_model;
+	stepping = boot_cpu_data.x86_mask;
 
-	switch (data->fam) {
+	switch (boot_cpu_data.x86) {
 	case 0xf:
+		/* feature available since SH-C0, exclude older revisions */
+		if (((model = 4) && (stepping = 0)) ||
+		    ((model = 5) && ((stepping = 0) || (stepping = 1)))) {

drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev,

Quoted 10 lines
drivers/hwmon/k8temp.c +4 -2
+			err = -ENODEV;
+			goto exit;
+		}
 		dev_warn(&pdev->dev, "Temperature readouts might be wrong"
-					" - check errata #141\n");
+			 " - check erratum #141\n");
 		break;
 	}
 
-- 1.6.0.4 

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#2 lore raw
In reply to Andreas Herrmann
Quoted 11 lines · Rudolf Marek

Hello,

Sorry for the delay.

Quoted 1 line

Maybe also some desc. Well the reason why the code below looks like is that

1) avoid very old systems - the idea that revs of CPU will be similar/same
2) get the cpuid from PCI. This is good, because some CPU may have the problem, some other may not.

Jean gave examples of a systems where the PCI will not work. Thus it isn't a reliable way to get CPUID. By problem you mean certain erratum. But this can only happen on systems with an unspported mixed silicon setup. E.g. combining K8 RevE with K8 RevF which is (according to AMD's documentation) not a supported setup.

Quoted 5 lines

Your patch reduces this to boot CPU checks. The idea was to have the driver know exact CPUID for each PCI driver instance. It was prepared to situation when some CPU steppings will have the fix, some not.

Please tell me why it is not necessary or why you changed that ;)

For mixed silicon systems certain restrictions apply. The allowed mixed silicon revisions for K8 and family 0x10 show that it completely suffices for k8temp to check the revision of the boot CPU.

E.g. if boot CPU is >= K8 RevC all other processors should be >=RevC <=RevE If boot CPU is RevF all other processors should be RevF. (Similar for family 0x10 RevB or RevC.)

And last not least K8 RevG is desktop/mobile only -- so no mixed setup here at all.

Thus all relevant CPUID checks in k8temp can reliably be performed with boot CPU information and no additional CPUID evaluation code is needed in k8temp.

Regards,
Andreas

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#3 lore raw
In reply to Andreas Herrmann
Quoted 2 lines · Andreas Herrmann
Quoted 1 line · Rudolf Marek

Please tell me why it is not necessary or why you changed that ;)

To be more specific.

There are following CPUID checks necessary in k8temp:

Quoted 1 line

= K8 RevC

to check for support of temperature reporting at all.

Quoted 1 line

= RevF

to check for erratum #141

Quoted 1 line

= RevG (desktop)

to check for temperature skew

(In the future we might have additional checks for family 0x10 >= RevC to check for erratum 319.)

This devides K8 CPUs into 4 classes (and family 10h CPUs into 2 classes).

Now it is important to notice that (by chance) you can't combine processors of different classes in a mixed silicon setup. See the relevant CPU revision guides for those CPUs. E.g., combining RevC with RevF is not supported.

Thus it suffices to check CPUID of boot CPU.
We don't need to care about unsupported processor combinations.

Regards,
Andreas

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors