linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* coretemp support for Core i5 CPU
@ 2009-09-20  7:47 Robert Hancock
  2009-09-20 18:19 ` [PATCH] coretemp: add " Robert Hancock
  2009-09-20 19:16 ` [lm-sensors] coretemp " Jean Delvare
  0 siblings, 2 replies; 6+ messages in thread
From: Robert Hancock @ 2009-09-20  7:47 UTC (permalink / raw)
  To: linux-kernel, lm-sensors; +Cc: r.marek

Trying to load the coretemp driver for CPU temperature monitoring on a 
Core i5 750 CPU gives:

coretemp: Unknown CPU model 1e
coretemp: Unknown CPU model 1e
coretemp: Unknown CPU model 1e
coretemp: Unknown CPU model 1e

Looks like this model needs to be added to the list in 
drivers/hwmon/coretemp.c. Though this doesn't really seem like a very 
good approach as we'll keep having to update this driver whenever a new 
CPU model gets released. Intel has a CPUID field that indicates if the 
thermal sensor is supported: "The processor supports a digital thermal 
sensor if CPUID.06H.EAX[0] = 1." We should likely be using this instead 
of hard-coded CPU model checks..

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] coretemp: add support for Core i5 CPU
  2009-09-20  7:47 coretemp support for Core i5 CPU Robert Hancock
@ 2009-09-20 18:19 ` Robert Hancock
  2009-09-23 21:19   ` Jean Delvare
  2009-09-20 19:16 ` [lm-sensors] coretemp " Jean Delvare
  1 sibling, 1 reply; 6+ messages in thread
From: Robert Hancock @ 2009-09-20 18:19 UTC (permalink / raw)
  To: linux-kernel, lm-sensors; +Cc: r.marek

Add coretemp support for Core i5 (Lynnfield) CPUs with model 0x1E.

Signed-off-by: Robert Hancock <hancockrwd@gmail.com>

---

This minimal patch works to add support for these CPUs, though we should likely
still look into using the CPUID flags instead of the model check.

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 93c1722..a9a21dc 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -413,11 +413,11 @@ static int __init coretemp_init(void)
 	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, 0x1E */
 		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 == 0x1E))) {
 
 			/* supported CPU not found, but report the unknown
 			   family 6 CPU */

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [lm-sensors] coretemp support for Core i5 CPU
  2009-09-20  7:47 coretemp support for Core i5 CPU Robert Hancock
  2009-09-20 18:19 ` [PATCH] coretemp: add " Robert Hancock
@ 2009-09-20 19:16 ` Jean Delvare
  2009-09-20 22:53   ` Robert Hancock
  1 sibling, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2009-09-20 19:16 UTC (permalink / raw)
  To: Robert Hancock; +Cc: linux-kernel, lm-sensors

Hi Robert,

On Sun, 20 Sep 2009 01:47:43 -0600, Robert Hancock wrote:
> Trying to load the coretemp driver for CPU temperature monitoring on a 
> Core i5 750 CPU gives:
> 
> coretemp: Unknown CPU model 1e
> coretemp: Unknown CPU model 1e
> coretemp: Unknown CPU model 1e
> coretemp: Unknown CPU model 1e
> 
> Looks like this model needs to be added to the list in 
> drivers/hwmon/coretemp.c. Though this doesn't really seem like a very 
> good approach as we'll keep having to update this driver whenever a new 
> CPU model gets released. Intel has a CPUID field that indicates if the 
> thermal sensor is supported: "The processor supports a digital thermal 
> sensor if CPUID.06H.EAX[0] = 1." We should likely be using this instead 
> of hard-coded CPU model checks..

Supporting the digital thermal sensor is one thing, but it is totally
insufficient. If you look at the coretemp driver, you'll see we need to
figure out a number of calibration values which are model-dependent. So
we will need to check this part before adding support for new CPU
models anyway.

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [lm-sensors] coretemp support for Core i5 CPU
  2009-09-20 19:16 ` [lm-sensors] coretemp " Jean Delvare
@ 2009-09-20 22:53   ` Robert Hancock
  2009-09-21  7:50     ` Jean Delvare
  0 siblings, 1 reply; 6+ messages in thread
From: Robert Hancock @ 2009-09-20 22:53 UTC (permalink / raw)
  To: Jean Delvare; +Cc: linux-kernel, lm-sensors

On Sun, Sep 20, 2009 at 1:16 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Robert,
>
> On Sun, 20 Sep 2009 01:47:43 -0600, Robert Hancock wrote:
>> Trying to load the coretemp driver for CPU temperature monitoring on a
>> Core i5 750 CPU gives:
>>
>> coretemp: Unknown CPU model 1e
>> coretemp: Unknown CPU model 1e
>> coretemp: Unknown CPU model 1e
>> coretemp: Unknown CPU model 1e
>>
>> Looks like this model needs to be added to the list in
>> drivers/hwmon/coretemp.c. Though this doesn't really seem like a very
>> good approach as we'll keep having to update this driver whenever a new
>> CPU model gets released. Intel has a CPUID field that indicates if the
>> thermal sensor is supported: "The processor supports a digital thermal
>> sensor if CPUID.06H.EAX[0] = 1." We should likely be using this instead
>> of hard-coded CPU model checks..
>
> Supporting the digital thermal sensor is one thing, but it is totally
> insufficient. If you look at the coretemp driver, you'll see we need to
> figure out a number of calibration values which are model-dependent. So
> we will need to check this part before adding support for new CPU
> models anyway.

Well, I see there is the TCC activation temperature which the reading
is relative to, which is detected on the mobile CPUs via an MSR read,
so it would only be a problem if they changed this temperature on
other CPUs..

It might make sense to warn "unknown CPU model, temperature reading
may be suspect" or something, but I should think it should still load
as the temperature reading is likely still correct.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [lm-sensors] coretemp support for Core i5 CPU
  2009-09-20 22:53   ` Robert Hancock
@ 2009-09-21  7:50     ` Jean Delvare
  0 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-09-21  7:50 UTC (permalink / raw)
  To: Robert Hancock; +Cc: linux-kernel, lm-sensors

On Sun, 20 Sep 2009 16:53:53 -0600, Robert Hancock wrote:
> On Sun, Sep 20, 2009 at 1:16 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > Supporting the digital thermal sensor is one thing, but it is totally
> > insufficient. If you look at the coretemp driver, you'll see we need to
> > figure out a number of calibration values which are model-dependent. So
> > we will need to check this part before adding support for new CPU
> > models anyway.
> 
> Well, I see there is the TCC activation temperature which the reading
> is relative to, which is detected on the mobile CPUs via an MSR read,
> so it would only be a problem if they changed this temperature on
> other CPUs..

They actually did change it on almost every CPU model so far. I fail to
see why that would suddenly change (even though I'd really love it.)

> It might make sense to warn "unknown CPU model, temperature reading
> may be suspect" or something, but I should think it should still load
> as the temperature reading is likely still correct.

Our experience is the exact opposite. The temperature reading is likely
not correct. And we prefer not telling the user anything than to tell
him/her something incorrect. Most users won't notice warnings in their
kernel logs, all they see is a CPU temperature value in their desktop
tray area.

I hope Intel will finally take over the driver maintenance and deal
with their own mess. It has been a pain for us to maintain since day 2.

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] coretemp: add support for Core i5 CPU
  2009-09-20 18:19 ` [PATCH] coretemp: add " Robert Hancock
@ 2009-09-23 21:19   ` Jean Delvare
  0 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-09-23 21:19 UTC (permalink / raw)
  To: Robert Hancock; +Cc: linux-kernel

On Sun, 20 Sep 2009 12:19:54 -0600, Robert Hancock wrote:
> Add coretemp support for Core i5 (Lynnfield) CPUs with model 0x1E.
> 
> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
> 
> ---
> 
> This minimal patch works to add support for these CPUs, though we should likely
> still look into using the CPUID flags instead of the model check.
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 93c1722..a9a21dc 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -413,11 +413,11 @@ static int __init coretemp_init(void)
>  	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, 0x1E */
>  		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 == 0x1E))) {
>  
>  			/* supported CPU not found, but report the unknown
>  			   family 6 CPU */

We've received a similar patch from Intel themselves a few days ago,
and I've just pushed it upstream.

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2009-09-23 21:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-20  7:47 coretemp support for Core i5 CPU Robert Hancock
2009-09-20 18:19 ` [PATCH] coretemp: add " Robert Hancock
2009-09-23 21:19   ` Jean Delvare
2009-09-20 19:16 ` [lm-sensors] coretemp " Jean Delvare
2009-09-20 22:53   ` Robert Hancock
2009-09-21  7:50     ` Jean Delvare

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).