hwmon,
Subsystems (2)
PATCH 1/1 hwmon,
From: Andreas Herrmann <[email protected]>
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
--- 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
drivers/hwmon/k8temp.c +6 -4
@@ -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 ; }
Signature · 7 lines
1.7.1 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 65 lines · Jean Delvare
Hi Andreas,
Quoted 28 lines · Andreas Herrmann
From: Andreas Herrmann <[email protected]>
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°CWith 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°CQuoted 8 lines
Cc <[email protected]> # .32.x .33.x, .34.xCc Rudolf Marek <[email protected]>Signed-off-by Andreas Herrmann <[email protected]>--- 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.
Quoted 3 lines
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.
Quoted 17 lines
drivers/hwmon/k8temp.c +4
-4@@ -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,
Quoted 8 lines
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 192
Quoted 5 lines
drivers/hwmon/k8temp.c +2
-0+* 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.
Quoted 4 lines
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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Andreas,
Quoted 28 lines · Andreas Herrmann
From: Andreas Herrmann <[email protected]>
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°CWith 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
Quoted 8 lines
Cc <[email protected]> # .32.x .33.x, .34.xCc Rudolf Marek <[email protected]>Signed-off-by Andreas Herrmann <[email protected]>--- 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.
Quoted 3 lines
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.
Quoted 12 lines
drivers/hwmon/k8temp.c +2
-1@@ -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 )) {/*
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 192
Quoted 10 lines
drivers/hwmon/k8temp.c +4
-3-* 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,
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Andreas,
Quoted 14 lines · Andreas Herrmann
Quoted 2 lines · Jean Delvare
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.
Quoted 4 lines
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.
Quoted 6 lines
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,
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
PATCH 1/1 hwmon,
In reply to Andreas HerrmannFrom: Andreas Herrmann <[email protected]>
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).
--- 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
drivers/hwmon/k8temp.c +23 -4
@@ -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 ; }
Signature · 7 lines
1.7.2 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
PATCH 1/1 hwmon,
In reply to Andreas HerrmannFrom: Andreas Herrmann <[email protected]>
The family check in k8temp is not required because the driver is already bound to a northbridge device only used with K8 CPUs.
--- drivers/hwmon/k8temp.c | 49 +++++++++++++++++++++-------------------------- 1 files changed, 22 insertions(+), 27 deletions(-)
drivers/hwmon/k8temp.c +22 -27
@@ -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 */
Signature · 7 lines
1.7.2 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Andreas,
Quoted 18 lines · Andreas Herrmann
From: Andreas Herrmann <[email protected]>
Commit
8bf0223ed515be24de0c671eedaff49e78bebc9c(hwmon, k8temp: Fix temperature reporting for ASB1 processor revisions) fixed temperature reporting for ASB1 CPUs. But one dual core CPU model (model0x6b) 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 <[email protected]>Cc [email protected] [.32.x .34.x, .35.x]Reported-by Vladislav Guberinic <[email protected]>Signed-off-by Andreas Herrmann <[email protected]>
Quoted 6 lines
--- drivers/hwmon/k8temp.c | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-)Sorry, but I missed that model
0x6bwas also packaged as AM2. Please apply this fix.
Thanks for the heads up. I'm a little confused though, see below.
Quoted 14 lines
drivers/hwmon/k8temp.c +6
-0@@ -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 ;
drivers/hwmon/k8temp.c static const struct pci_device_id k8temp_ids[] = { · line 161
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-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?
Quoted 21 lines
drivers/hwmon/k8temp.c +7
-3+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,
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 214
Quoted 7 lines
drivers/hwmon/k8temp.c +0
-1@@ -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.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Jean Delvare
AppliedQuoted 4 lines · Andreas Herrmann
From: Andreas Herrmann <[email protected]>
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.
Quoted 58 lines
Signed-off-by Andreas Herrmann <[email protected]>--- drivers/hwmon/k8temp.c | 49 +++++++++++++++++++++-------------------------- 1 files changed, 22 insertions(+), 27 deletions(-)drivers/hwmon/k8temp.c +21
-21@@ -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
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 212
Quoted 10 lines
drivers/hwmon/k8temp.c +1
-6-* 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.)
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 11 lines · Jean Delvare
Quoted 4 lines · Andreas Herrmann
From: Andreas Herrmann <[email protected]>
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»9a35f293a3c8b5d57253cdfe2f29fa2627e1b9But 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.)
Quoted 66 lines
Quoted 66 lines
Signed-off-by Andreas Herrmann <[email protected]>--- drivers/hwmon/k8temp.c | 49 +++++++++++++++++++++-------------------------- 1 files changed, 22 insertions(+), 27 deletions(-)drivers/hwmon/k8temp.c +22
-27@@ -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 ;
Quoted 5 lines
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 212
Quoted 2 lines
drivers/hwmon/k8temp.c +0
-0pci_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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
On Tue, Aug 17, 2010 at 06:21:38PM +0200, Jean Delvare wrote:
[...]
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+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; +
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-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
Quoted 3 lines
Is this correct? What about the 3 other ASB1 entries in table 8? And why are you comparing with
0xawhile 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.
Quoted 26 lines
Quoted 26 lines
(quoted diff) +7
-4+ 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; - }
Quoted 7 lines
Review static int __devinit k8temp_probe(struct pci_dev *pdev, · line 214
Quoted 2 lines
(quoted diff) +0
-0break; }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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
PATCH 1/1 hwmon,
In reply to Andreas HerrmannFrom: Andreas Herrmann <[email protected]>
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).
--- 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
drivers/hwmon/k8temp.c +34 -10
@@ -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 ; }
Signature · 7 lines
1.7.2 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Andreas,
Quoted 24 lines · Andreas Herrmann
On Tue, Aug 17, 2010 at 06:21:38PM +0200, Jean Delvare wrote:
[...]
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+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; +Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-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?
Quoted 7 lines
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.
Quoted 7 lines
Quoted 3 lines
Is this correct? What about the 3 other ASB1 entries in table 8? And why are you comparing with
0xawhile 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)
Quoted 7 lines
Damn!
Need to update this patch. I did not check table 2 which lists single-core AM2 CPUs.
model
0x6fand0x7f(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.
Quoted 35 lines
Quoted 27 lines
Quoted 27 lines
(quoted diff) +7
-4+ 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; - }Quoted 7 lines
Review static int __devinit k8temp_probe(struct pci_dev *pdev, · line 214
Quoted 2 lines
(quoted diff) +0
-0break; }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.
Quoted 4 lines
Ok.
Thanks a lot for reviewing.
I'll come up with an updated patch.
OK, I'll wait for it.
Thanks,
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Andreas,
Quoted 19 lines · Andreas Herrmann
From: Andreas Herrmann <[email protected]>
Commit
8bf0223ed515be24de0c671eedaff49e78bebc9c(hwmon, k8temp: Fix temperature reporting for ASB1 processor revisions) fixed temperature reporting for ASB1 CPUs. But those CPU models (model0x6b,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 <[email protected]>Cc [email protected] [.32.x .34.x, .35.x]Reported-by Vladislav Guberinic <[email protected]>Signed-off-by Andreas Herrmann <[email protected]>
Quoted 7 lines
--- 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
0x6bASB1 CPU. I don't have instant access to model0x6fand0x7fCPU 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? :(
drivers/hwmon/k8temp.c static const struct pci_device_id k8temp_ids[] = { · line 146
Quoted 9 lines
drivers/hwmon/k8temp.c +1
-0@@ -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.
Quoted 11 lines
drivers/hwmon/k8temp.c +11
-0+{ +u32 brandidx; + +if (model <0x69 ) +return 0 ; + +if (model =0xc1 || model =0x6c || model =0x7c ) +return 0 ; + +/* +* differentiate between AM2 and ASB1
drivers/hwmon/k8temp.c static const struct pci_device_id k8temp_ids[] = { · line 167
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+*/ + + 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.
Quoted 23 lines
drivers/hwmon/k8temp.c +7
-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 -*/ +/*
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 219
Quoted 10 lines
drivers/hwmon/k8temp.c +5
-1+* 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.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
On Thu, Aug 19, 2010 at 10:34:42AM +0200, Jean Delvare wrote:
[...]
Quoted 8 lines
Quoted 3 lines
Here is the updated patch considering single core and dual core ASB1 CPUs. Tested with model
0x6bASB1 CPU. I don't have instant access to model0x6fand0x7fCPU 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.
Quoted 11 lines
drivers/hwmon/k8temp.c static const struct pci_device_id k8temp_ids[] = { · line 146
Quoted 9 lines
drivers/hwmon/k8temp.c +1
-0@@ -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.
Quoted 17 lines
Quoted 17 lines
drivers/hwmon/k8temp.c +17
-0+{ +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 ))
Quoted 8 lines
drivers/hwmon/k8temp.c static const struct pci_device_id k8temp_ids[] = { · line 167
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+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.
Quoted 31 lines
Quoted 31 lines
drivers/hwmon/k8temp.c +12
-10+ 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 ; - }
Quoted 7 lines
drivers/hwmon/k8temp.c static int __devinit k8temp_probe(struct pci_dev *pdev, · line 219
Quoted 2 lines
drivers/hwmon/k8temp.c +0
-0break ; }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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors