* Re: [lm-sensors] [PATCH] Add support for Atom CPUs
2009-07-06 16:34 [lm-sensors] [PATCH] Add support for Atom CPUs Rudolf Marek
@ 2009-07-06 20:38 ` Rudolf Marek
2009-07-07 10:52 ` Jean Delvare
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Rudolf Marek @ 2009-07-06 20:38 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 546 bytes --]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi all,
Thanks Martin for a test. It seems that TAGET MSR is not implemented, also
driver incorrectly reports that it uses relative temperature scale. Fix it with
updated patch.
Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkpSYLEACgkQ3J9wPJqZRNX2qwCfeF0+WAidbXs/cterief56qV/
i4EAn1zchUyNP54Qas3KEqV5RGGi40Kj
=ye/T
-----END PGP SIGNATURE-----
[-- Attachment #2: add_atoms.patch --]
[-- Type: text/x-diff, Size: 4092 bytes --]
Index: linux-2.6.30.1/drivers/hwmon/coretemp.c
===================================================================
--- linux-2.6.30.1.orig/drivers/hwmon/coretemp.c 2009-07-06 17:07:16.458757525 +0200
+++ linux-2.6.30.1/drivers/hwmon/coretemp.c 2009-07-06 22:34:59.417758968 +0200
@@ -157,17 +157,24 @@
/* The 100C is default for both mobile and non mobile CPUs */
int tjmax = 100000;
- int ismobile = 1;
+ int usemsr_ee = 1;
int err;
u32 eax, edx;
/* Early chips have no MSR for TjMax */
if ((c->x86_model == 0xf) && (c->x86_mask < 4)) {
- ismobile = 0;
+ usemsr_ee = 0;
}
- if ((c->x86_model > 0xe) && (ismobile)) {
+ /* Atoms seems to have TjMax at 90C */
+
+ if (c->x86_model == 0x1c) {
+ usemsr_ee = 0;
+ tjmax = 90000;
+ }
+
+ if ((c->x86_model > 0xe) && (usemsr_ee)) {
/* Now we can detect the mobile CPU using Intel provided table
http://softwarecommunity.intel.com/Wiki/Mobility/720.htm
@@ -179,13 +186,13 @@
dev_warn(dev,
"Unable to access MSR 0x17, assuming desktop"
" CPU\n");
- ismobile = 0;
+ usemsr_ee = 0;
} else if (!(eax & 0x10000000)) {
- ismobile = 0;
+ usemsr_ee = 0;
}
}
- if (ismobile) {
+ if (usemsr_ee) {
err = rdmsr_safe_on_cpu(id, 0xee, &eax, &edx);
if (err) {
@@ -195,7 +202,8 @@
} else if (eax & 0x40000000) {
tjmax = 85000;
}
- } else {
+ /* if we dont use msr EE it means we are desktop CPU (with exeception of Atom) */
+ } else if (tjmax == 100000) {
dev_warn(dev, "Using relative temperature scale!\n");
}
@@ -248,9 +256,9 @@
platform_set_drvdata(pdev, data);
/* read the still undocumented IA32_TEMPERATURE_TARGET it exists
- on older CPUs but not in this register */
+ on older CPUs but not in this register, Atoms don't have it too */
- if (c->x86_model > 0xe) {
+ if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
err = rdmsr_safe_on_cpu(data->id, 0x1a2, &eax, &edx);
if (err) {
dev_warn(&pdev->dev, "Unable to read"
@@ -413,11 +421,11 @@
for_each_online_cpu(i) {
struct cpuinfo_x86 *c = &cpu_data(i);
- /* check if family 6, models 0xe, 0xf, 0x16, 0x17, 0x1A */
+ /* check if family 6, models 0xe, 0xf, 0x16, 0x17, 0x1A, 0x1c */
if ((c->cpuid_level < 0) || (c->x86 != 0x6) ||
!((c->x86_model == 0xe) || (c->x86_model == 0xf) ||
(c->x86_model == 0x16) || (c->x86_model == 0x17) ||
- (c->x86_model == 0x1A))) {
+ (c->x86_model == 0x1A) || (c->x86_model == 0x1c))) {
/* supported CPU not found, but report the unknown
family 6 CPU */
Index: linux-2.6.30.1/Documentation/hwmon/coretemp
===================================================================
--- linux-2.6.30.1.orig/Documentation/hwmon/coretemp 2009-07-06 18:31:56.262759644 +0200
+++ linux-2.6.30.1/Documentation/hwmon/coretemp 2009-07-06 22:31:01.905898848 +0200
@@ -4,7 +4,7 @@
Supported chips:
* All Intel Core family
Prefix: 'coretemp'
- CPUID: family 0x6, models 0xe, 0xf, 0x16, 0x17
+ CPUID: family 0x6, models 0xe, 0xf, 0x16, 0x17, 0x1c (Atom)
Datasheet: Intel 64 and IA-32 Architectures Software Developer's Manual
Volume 3A: System Programming Guide
http://softwarecommunity.intel.com/Wiki/Mobility/720.htm
Index: linux-2.6.30.1/drivers/hwmon/Kconfig
===================================================================
--- linux-2.6.30.1.orig/drivers/hwmon/Kconfig 2009-07-06 18:32:31.974757310 +0200
+++ linux-2.6.30.1/drivers/hwmon/Kconfig 2009-07-06 18:33:48.566759515 +0200
@@ -402,12 +402,12 @@
will be called gl520sm.
config SENSORS_CORETEMP
- tristate "Intel Core (2) Duo/Solo temperature sensor"
+ tristate "Intel Core/Core2/Atom temperature sensor"
depends on X86 && EXPERIMENTAL
help
If you say yes here you get support for the temperature
- sensor inside your CPU. Supported all are all known variants
- of Intel Core family.
+ sensor inside your CPU. Most of the family 6 CPUs
+ are supported. Check documentation/driver for details.
config SENSORS_IBMAEM
tristate "IBM Active Energy Manager temperature/power sensors and control"
[-- Attachment #3: add_atoms.patch.sig --]
[-- Type: application/octet-stream, Size: 72 bytes --]
[-- Attachment #4: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] Add support for Atom CPUs
2009-07-06 16:34 [lm-sensors] [PATCH] Add support for Atom CPUs Rudolf Marek
2009-07-06 20:38 ` Rudolf Marek
@ 2009-07-07 10:52 ` Jean Delvare
2009-07-08 20:38 ` Rudolf Marek
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-07-07 10:52 UTC (permalink / raw)
To: lm-sensors
Hi Rudolf,
On Mon, 06 Jul 2009 22:38:10 +0200, Rudolf Marek wrote:
> Thanks Martin for a test. It seems that TAGET MSR is not implemented, also
> driver incorrectly reports that it uses relative temperature scale. Fix it with
> updated patch.
>
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
> Index: linux-2.6.30.1/drivers/hwmon/coretemp.c
> =================================> --- linux-2.6.30.1.orig/drivers/hwmon/coretemp.c 2009-07-06 17:07:16.458757525 +0200
> +++ linux-2.6.30.1/drivers/hwmon/coretemp.c 2009-07-06 22:34:59.417758968 +0200
> @@ -157,17 +157,24 @@
> /* The 100C is default for both mobile and non mobile CPUs */
>
> int tjmax = 100000;
> - int ismobile = 1;
> + int usemsr_ee = 1;
> int err;
> u32 eax, edx;
>
> /* Early chips have no MSR for TjMax */
>
> if ((c->x86_model = 0xf) && (c->x86_mask < 4)) {
> - ismobile = 0;
> + usemsr_ee = 0;
> }
>
> - if ((c->x86_model > 0xe) && (ismobile)) {
> + /* Atoms seems to have TjMax at 90C */
> +
> + if (c->x86_model = 0x1c) {
> + usemsr_ee = 0;
> + tjmax = 90000;
> + }
> +
> + if ((c->x86_model > 0xe) && (usemsr_ee)) {
>
> /* Now we can detect the mobile CPU using Intel provided table
> http://softwarecommunity.intel.com/Wiki/Mobility/720.htm
> @@ -179,13 +186,13 @@
> dev_warn(dev,
> "Unable to access MSR 0x17, assuming desktop"
> " CPU\n");
> - ismobile = 0;
> + usemsr_ee = 0;
> } else if (!(eax & 0x10000000)) {
> - ismobile = 0;
> + usemsr_ee = 0;
> }
> }
>
> - if (ismobile) {
> + if (usemsr_ee) {
>
> err = rdmsr_safe_on_cpu(id, 0xee, &eax, &edx);
> if (err) {
> @@ -195,7 +202,8 @@
> } else if (eax & 0x40000000) {
> tjmax = 85000;
> }
> - } else {
> + /* if we dont use msr EE it means we are desktop CPU (with exeception of Atom) */
Line too long.
> + } else if (tjmax = 100000) {
> dev_warn(dev, "Using relative temperature scale!\n");
> }
>
I think function adjust_tjmax() has reached its limit of readability. I
don't understand why it needs to be so complex. For example, the case
of the Atom is very simply always return 90 degree C. So why not just:
/* Atoms seems to have TjMax at 90C */
if (c->x86_model = 0x1c)
return 90000;
at the beginning of the function, and be done with that case? And I
suspect the rest can be simplified a bit too.
> @@ -248,9 +256,9 @@
> platform_set_drvdata(pdev, data);
>
> /* read the still undocumented IA32_TEMPERATURE_TARGET it exists
> - on older CPUs but not in this register */
> + on older CPUs but not in this register, Atoms don't have it too */
s/too/either/
>
> - if (c->x86_model > 0xe) {
> + if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
> err = rdmsr_safe_on_cpu(data->id, 0x1a2, &eax, &edx);
> if (err) {
> dev_warn(&pdev->dev, "Unable to read"
> @@ -413,11 +421,11 @@
> for_each_online_cpu(i) {
> struct cpuinfo_x86 *c = &cpu_data(i);
>
> - /* check if family 6, models 0xe, 0xf, 0x16, 0x17, 0x1A */
> + /* check if family 6, models 0xe, 0xf, 0x16, 0x17, 0x1A, 0x1c */
> if ((c->cpuid_level < 0) || (c->x86 != 0x6) ||
> !((c->x86_model = 0xe) || (c->x86_model = 0xf) ||
> (c->x86_model = 0x16) || (c->x86_model = 0x17) ||
> - (c->x86_model = 0x1A))) {
> + (c->x86_model = 0x1A) || (c->x86_model = 0x1c))) {
>
> /* supported CPU not found, but report the unknown
> family 6 CPU */
sensors-detect needs to be updated as well.
> Index: linux-2.6.30.1/Documentation/hwmon/coretemp
> =================================> --- linux-2.6.30.1.orig/Documentation/hwmon/coretemp 2009-07-06 18:31:56.262759644 +0200
> +++ linux-2.6.30.1/Documentation/hwmon/coretemp 2009-07-06 22:31:01.905898848 +0200
> @@ -4,7 +4,7 @@
> Supported chips:
> * All Intel Core family
> Prefix: 'coretemp'
> - CPUID: family 0x6, models 0xe, 0xf, 0x16, 0x17
> + CPUID: family 0x6, models 0xe, 0xf, 0x16, 0x17, 0x1c (Atom)
> Datasheet: Intel 64 and IA-32 Architectures Software Developer's Manual
> Volume 3A: System Programming Guide
> http://softwarecommunity.intel.com/Wiki/Mobility/720.htm
> Index: linux-2.6.30.1/drivers/hwmon/Kconfig
> =================================> --- linux-2.6.30.1.orig/drivers/hwmon/Kconfig 2009-07-06 18:32:31.974757310 +0200
> +++ linux-2.6.30.1/drivers/hwmon/Kconfig 2009-07-06 18:33:48.566759515 +0200
> @@ -402,12 +402,12 @@
> will be called gl520sm.
>
> config SENSORS_CORETEMP
> - tristate "Intel Core (2) Duo/Solo temperature sensor"
> + tristate "Intel Core/Core2/Atom temperature sensor"
> depends on X86 && EXPERIMENTAL
> help
> If you say yes here you get support for the temperature
> - sensor inside your CPU. Supported all are all known variants
> - of Intel Core family.
> + sensor inside your CPU. Most of the family 6 CPUs
> + are supported. Check documentation/driver for details.
>
> config SENSORS_IBMAEM
> tristate "IBM Active Energy Manager temperature/power sensors and control"
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] Add support for Atom CPUs
2009-07-06 16:34 [lm-sensors] [PATCH] Add support for Atom CPUs Rudolf Marek
2009-07-06 20:38 ` Rudolf Marek
2009-07-07 10:52 ` Jean Delvare
@ 2009-07-08 20:38 ` Rudolf Marek
2009-07-08 21:18 ` Jean Delvare
2009-09-20 8:28 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Rudolf Marek @ 2009-07-08 20:38 UTC (permalink / raw)
To: lm-sensors
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
>> + /* if we dont use msr EE it means we are desktop CPU (with exeception of Atom) */
>
> Line too long.
Well I thought the 80 lines limit is not so strict anymore.
>
>> + } else if (tjmax = 100000) {
>> dev_warn(dev, "Using relative temperature scale!\n");
>> }
>>
>
> I think function adjust_tjmax() has reached its limit of readability. I
> don't understand why it needs to be so complex. For example, the case
> of the Atom is very simply always return 90 degree C. So why not just:
Yes right, but it is going to be complex for penryns. I will try to simplify it.
>
> /* Atoms seems to have TjMax at 90C */
> if (c->x86_model = 0x1c)
> return 90000;
Because of multiple returns? If you are OK with multiple returns I will fix it.
>
> at the beginning of the function, and be done with that case? And I
> suspect the rest can be simplified a bit too.
>
>> @@ -248,9 +256,9 @@
>> platform_set_drvdata(pdev, data);
>>
>> /* read the still undocumented IA32_TEMPERATURE_TARGET it exists
>> - on older CPUs but not in this register */
>> + on older CPUs but not in this register, Atoms don't have it too */
>
> s/too/either/
ok
> sensors-detect needs to be updated as well.
yes good catch.
Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkpVA6YACgkQ3J9wPJqZRNVM6wCghNTnneKd3ta0M7X9GO+EeRNZ
OH4An3uuSHgvH/rB+8leDYf7zFAOI9mc
=HIzj
-----END PGP SIGNATURE-----
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] Add support for Atom CPUs
2009-07-06 16:34 [lm-sensors] [PATCH] Add support for Atom CPUs Rudolf Marek
` (2 preceding siblings ...)
2009-07-08 20:38 ` Rudolf Marek
@ 2009-07-08 21:18 ` Jean Delvare
2009-09-20 8:28 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-07-08 21:18 UTC (permalink / raw)
To: lm-sensors
On Wed, 08 Jul 2009 22:38:05 +0200, Rudolf Marek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> >> + /* if we dont use msr EE it means we are desktop CPU (with exeception of Atom) */
> >
> > Line too long.
>
> Well I thought the 80 lines limit is not so strict anymore.
Quoting CodingStyle:
"The only exception to this is where exceeding 80 columns significantly
increases readability and does not hide information."
Do you really believe that splitting this comment will hinder
readability?
> >> + } else if (tjmax = 100000) {
> >> dev_warn(dev, "Using relative temperature scale!\n");
> >> }
> >>
> >
> > I think function adjust_tjmax() has reached its limit of readability. I
> > don't understand why it needs to be so complex. For example, the case
> > of the Atom is very simply always return 90 degree C. So why not just:
>
> Yes right, but it is going to be complex for penryns. I will try to simplify it.
>
> >
> > /* Atoms seems to have TjMax at 90C */
> > if (c->x86_model = 0x1c)
> > return 90000;
>
> Because of multiple returns? If you are OK with multiple returns I will fix it.
Last time I checked, the C language supported it ;) I see no point in
limiting yourself if this means your code becomes harder to read.
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] Add support for Atom CPUs
2009-07-06 16:34 [lm-sensors] [PATCH] Add support for Atom CPUs Rudolf Marek
` (3 preceding siblings ...)
2009-07-08 21:18 ` Jean Delvare
@ 2009-09-20 8:28 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-09-20 8:28 UTC (permalink / raw)
To: lm-sensors
On Mon, 06 Jul 2009 22:38:10 +0200, Rudolf Marek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi all,
>
> Thanks Martin for a test. It seems that TAGET MSR is not implemented, also
> driver incorrectly reports that it uses relative temperature scale. Fix it with
> updated patch.
>
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>
As there is increasing pressure to get support for the Atom CPU, I've
included this patch in my unofficial hwmon tree, fixed the two
comments, and will send it to Linus for kernel 2.6.32. I still believe
adjust_tjmax() could and should be simplified, but this is left for a
future patch.
I've also updated sensors-detect.
--
Jean Delvare
http://khali.linux-fr.org/wishlist.html
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread