add alternate VCORE calculations for w83627thf and

PATCH New Mark M. Hoffman 4 participants 11 messages May 19, 2005

lore

Share & tools

lore

Mark M. Hoffman

author

2005-05-19 06:25 UTC #0 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

Hi Greg:

This can wait until after 2.6.7. It applies to 2.6.7-rc2.

* * * * *

This patch adds support for the alternate in0/VCORE calculation which is available for 2 of 4 chips this driver supports. It also fixes a minor bug in the standard voltage input calculation. Please apply.

drivers/i2c/chips/w83627hf.c renamed from drivers/i2c/chips/w83627hf.c.orig +91 -3

@@ -199,7 +199,7 @@
 #define W83627THF_REG_PWM2		0x03	/* 697HF and 637HF too */
 #define W83627THF_REG_PWM3		0x11	/* 637HF too */
 
-#define W83627THF_REG_VRM_OVT_CFG 	0x18	/* 637HF too, unused yet */
+#define W83627THF_REG_VRM_OVT_CFG 	0x18	/* 637HF too */
 
 static const u8 regpwm_627hf[] = { W83627HF_REG_PWM1, W83627HF_REG_PWM2 };
 static const u8 regpwm[] = { W83627THF_REG_PWM1, W83627THF_REG_PWM2,

@@ -222,7 +222,7 @@
    these macros are called: arguments may be evaluated more than once.
    Fixing this is just not worth it. */
 #define IN_TO_REG(val)  (SENSORS_LIMIT((((val) + 8)/16),0,255))
-#define IN_FROM_REG(val) ((val) * 16 + 5)
+#define IN_FROM_REG(val) ((val) * 16)
 
 static inline u8 FAN_TO_REG(long rpm, int div)
 {

@@ -312,6 +312,7 @@
 				   Default = 3435.
 				   Other Betas unimplemented */
 	u8 vrm;
+	u8 vrm_ovt;		/* Register value, 627thf & 637hf only */
 };

@@ -391,7 +392,6 @@
 sysfs_in_reg_offset(min, offset) \
 sysfs_in_reg_offset(max, offset)
 
-sysfs_in_offsets(0)
 sysfs_in_offsets(1)
 sysfs_in_offsets(2)
 sysfs_in_offsets(3)

@@ -401,6 +401,89 @@
 sysfs_in_offsets(7)
 sysfs_in_offsets(8)
 
+/* use a different set of functions for in0 */
+static ssize_t show_in_0(struct w83627hf_data *data, char *buf, u8 reg)
+{
+	long in0;
+
+	if ((data->vrm_ovt & 0x01) &&
+		(w83627thf = data->type || w83637hf = data->type))
+
+		/* use VRM9 calculation */
+		in0 = (long)((reg * 488 + 70000 + 50) / 100);
+	else
+		/* use VRM8 (standard) calculation */
+		in0 = (long)IN_FROM_REG(reg);
+
+	return sprintf(buf,"%ld\n", in0);
+}
+
+static ssize_t show_regs_in_0(struct device *dev, char *buf)
+{
+	struct w83627hf_data *data = w83627hf_update_device(dev);
+	return show_in_0(data, buf, data->in[0]);
+}
+
+static ssize_t show_regs_in_min0(struct device *dev, char *buf)
+{
+	struct w83627hf_data *data = w83627hf_update_device(dev);
+	return show_in_0(data, buf, data->in_min[0]);
+}
+
+static ssize_t show_regs_in_max0(struct device *dev, char *buf)
+{
+	struct w83627hf_data *data = w83627hf_update_device(dev);
+	return show_in_0(data, buf, data->in_max[0]);
+}
+
+static ssize_t store_regs_in_min0(struct device *dev,
+	const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct w83627hf_data *data = i2c_get_clientdata(client);
+	u32 val;
+
+	val = simple_strtoul(buf, NULL, 10);
+	if ((data->vrm_ovt & 0x01) &&
+		(w83627thf = data->type || w83637hf = data->type))
+
+		/* use VRM9 calculation */
+		data->in_min[0] = (u8)(((val * 100) - 70000 + 244) / 488);
+	else
+		/* use VRM8 (standard) calculation */
+		data->in_min[0] = IN_TO_REG(val);
+
+	w83627hf_write_value(client, W83781D_REG_IN_MIN(0), data->in_min[0]);
+	return count;
+}
+
+static ssize_t store_regs_in_max0(struct device *dev,
+	const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct w83627hf_data *data = i2c_get_clientdata(client);
+	u32 val;
+
+	val = simple_strtoul(buf, NULL, 10);
+	if ((data->vrm_ovt & 0x01) &&
+		(w83627thf = data->type || w83637hf = data->type))
+		
+		/* use VRM9 calculation */
+		data->in_max[0] = (u8)(((val * 100) - 70000 + 244) / 488);
+	else
+		/* use VRM8 (standard) calculation */
+		data->in_max[0] = IN_TO_REG(val);
+
+	w83627hf_write_value(client, W83781D_REG_IN_MAX(0), data->in_max[0]);
+	return count;
+}
+
+static DEVICE_ATTR(in0_input, S_IRUGO, show_regs_in_0, NULL)
+static DEVICE_ATTR(in0_min, S_IRUGO | S_IWUSR,
+	show_regs_in_min0, store_regs_in_min0)
+static DEVICE_ATTR(in0_max, S_IRUGO | S_IWUSR,
+	show_regs_in_max0, store_regs_in_max0)
+
 #define device_create_file_in(client, offset) \
 do { \
 device_create_file(&client->dev, &dev_attr_in##offset##_input); \

@@ -1191,6 +1274,11 @@
 		data->vid = w83627thf_read_gpio5(client) & 0x1f;
 	}
 
+	/* Read VRM & OVT Config only once */
+	if (w83627thf = data->type || w83637hf = data->type)
+		data->vrm_ovt = 
+			w83627hf_read_value(client, W83627THF_REG_VRM_OVT_CFG);
+
 	/* Convert VID to voltage based on default VRM */
 	data->vrm = DEFAULT_VRM;
 	if (type != w83697hf)

Signature · 2 lines

Mark M. Hoffman
[email protected]

Jean Delvare

2005-05-19 06:25 UTC #1 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

Hi Mark,

drivers/i2c/chips/w83627hf.c line 1283

Quoted 7 lines

drivers/i2c/chips/w83627hf.c renamed from drivers/i2c/chips/w83627hf.c.orig +5 -0

+	/* Read VRM & OVT Config only once */
+	if (w83627thf = data->type || w83637hf = data->type)
+		data->vrm_ovt = 
+			w83627hf_read_value(client, W83627THF_REG_VRM_OVT_CFG);
+
 	/* Convert VID to voltage based on default VRM */
 	data->vrm = DEFAULT_VRM;

What about also using the VRM configuration bit to select the correct data->vrm upon load for 627thf and 637hf (instead of default)? The code could look like:

	/* Read VRM & OVT Config only once */
	if (w83627thf = data->type || w83637hf = data->type) {
		data->vrm_ovt = 
			w83627hf_read_value(client, W83627THF_REG_VRM_OVT_CFG);
		data->vrm = (data->vrm_ovt & 0x01) ? 90 : 82;
	} else {
		/* Convert VID to voltage based on default VRM */
		data->vrm = DEFAULT_VRM;
	}

(As a side note, I don't much see the interest of DEFAULT_VRM. Having the same default for all chips doesn't make much sense since older chips will most likely need VRM8 and newer chips will most likely need VRM9. So I'd propose to get rid of that define and let every driver pick whatever is relevant.)

Maybe it would also make sense to (physically) change this configuration bit whenever the user changes VRM versions? We certainly want VID pins and in0 reading to refer to the same VRM version.

Nice patch BTW, thanks :)

Signature · 2 lines

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

Greg KH

2005-05-19 06:25 UTC #2 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Quoted 3 lines · Mark M. Hoffman

Hi Greg:
This can wait until after 2.6.7. It applies to 2.6.7-rc2.

Are you sure? I get the following errors when trying to apply it:
drivers/i2c/chips/w83627hf.c 1.5: 1359 lines
patching file drivers/i2c/chips/w83627hf.c
Hunk #4 FAILED at 392.
Hunk #5 succeeded at 401 with fuzz 2.
1 out of 6 hunks FAILED -- saving rejects to file drivers/i2c/chips/w83627hf.c.rej

thanks,

greg k-h

Greg KH

2005-05-19 06:25 UTC #3 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Quoted 11 lines · Greg KH

Quoted 3 lines · Mark M. Hoffman
Hi Greg:
This can wait until after 2.6.7. It applies to 2.6.7-rc2.
Are you sure? I get the following errors when trying to apply it:
drivers/i2c/chips/w83627hf.c 1.5: 1359 lines
patching file drivers/i2c/chips/w83627hf.c
Hunk #4 FAILED at 392.
Hunk #5 succeeded at 401 with fuzz 2.
1 out of 6 hunks FAILED -- saving rejects to file drivers/i2c/chips/w83627hf.c.rej

Hm, ok, that's wierd, I just got it to work.

Nevermind... Applied, thanks.

greg k-h

Mark M. Hoffman

author

2005-05-19 06:25 UTC #4 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Hi Jean:

Quoted 4 lines · Jean Delvare

What about also using the VRM configuration bit to select the correct data->vrm upon load for 627thf and 637hf (instead of default)? The code could look like:

Quoted 9 lines

	/* Read VRM & OVT Config only once */
	if (w83627thf = data->type || w83637hf = data->type) {
		data->vrm_ovt = 
			w83627hf_read_value(client, W83627THF_REG_VRM_OVT_CFG);
		data->vrm = (data->vrm_ovt & 0x01) ? 90 : 82;
	} else {
		/* Convert VID to voltage based on default VRM */
		data->vrm = DEFAULT_VRM;
	}

I like that. Patch to follow...

Quoted 5 lines

(As a side note, I don't much see the interest of DEFAULT_VRM. Having the same default for all chips doesn't make much sense since older chips will most likely need VRM8 and newer chips will most likely need VRM9. So I'd propose to get rid of that define and let every driver pick whatever is relevant.)

Err, why don't we just read the CPU type out of /proc and set the VRM accordingly during 'sensors -s'... entirely in userspace?

Quoted 3 lines

Maybe it would also make sense to (physically) change this configuration bit whenever the user changes VRM versions? We certainly want VID pins and in0 reading to refer to the same VRM version.

Why? The interpretation of the VID pins has nothing to do with that of in0; I see the in0 calculation mode as just a driver-internal detail.

Also, to force them in sync would require that writing to VRM also updates the in0 min and max... not impossible of course, just not worth it.

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Mark M. Hoffman

author

2005-05-19 06:25 UTC #5 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Hi Greg:

This patch applies on top of the previous one in the thread (which applied to 2.6.7-rc2). Please apply.

* * * * *

Pick a VRM (for VID interpretation) based on the VRM & OVT config, if available. Props to Jean Delvare <[email protected]> for the idea & code fragment.

drivers/i2c/chips/w83627hf.c renamed from drivers/i2c/chips/w83627hf.c.1 +6 -3

@@ -1275,12 +1275,15 @@
 	}
 
 	/* Read VRM & OVT Config only once */
-	if (w83627thf = data->type || w83637hf = data->type)
+	if (w83627thf = data->type || w83637hf = data->type) {
 		data->vrm_ovt = 
 			w83627hf_read_value(client, W83627THF_REG_VRM_OVT_CFG);
+		data->vrm = (data->vrm_ovt & 0x01) ? 90 : 82;
+	} else {
+		/* Convert VID to voltage based on default VRM */
+		data->vrm = DEFAULT_VRM;
+	}
 
-	/* Convert VID to voltage based on default VRM */
-	data->vrm = DEFAULT_VRM;
 	if (type != w83697hf)
 		vid = vid_from_reg(vid, data->vrm);

Signature · 2 lines

Mark M. Hoffman
[email protected]

Jean Delvare

2005-05-19 06:25 UTC #6 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Quoted 9 lines

Quoted 6 lines
(As a side note, I don't much see the interest of DEFAULT_VRM. Having the same default for all chips doesn't make much sense since older chips will most likely need VRM8 and newer chips will most likely need VRM9. So I'd propose to get rid of that define and let every driver pick whatever is relevant.)
Err, why don't we just read the CPU type out of /proc and set the VRM accordingly during 'sensors -s'... entirely in userspace?

You seem to suggest that each CPU type can only work with one VRM version. Are you sure?

The CPU type information is also available in kernel-space, maybe even more easily (no parsing required), and I believe that setting the VRM from the CPU type belongs to the driver (once, at init time). I don't see how you are going to do that in "sensors".

Quoted 6 lines

Quoted 3 lines
Maybe it would also make sense to (physically) change this configuration bit whenever the user changes VRM versions? We certainly want VID pins and in0 reading to refer to the same VRM version.
Why? The interpretation of the VID pins has nothing to do with that of in0; I see the in0 calculation mode as just a driver-internal detail.

I admit these are two different things, but both are supposed to depend on which VRM version is used. The fact that in0 calculation can be changed is admittedly specific to these chips, but I doubt they called the register "VRM config" without a reason. Thus my suggestion, mainly based of the (wrong) idea that motherboard manufacturers would be logical folks ;) Something we cannot rely upon, so you can just forget about what I said.

Quoted 2 lines

Also, to force them in sync would require that writing to VRM also updates the in0 min and max... not impossible of course, just not worth it.

All in all, this configuration bit is no different from fan clock dividers. It's a range vs. resolution tradeoff, not strongly related to VRM. It just happens that VRM9 voltages will necessarily fit in the shortest range, while VRM8 ones don't necessarily do.

Jean Delvare

Mark M. Hoffman

author

2005-05-19 06:25 UTC #7 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Quoted 11 lines · Jean Delvare

Quoted 8 lines
Quoted 5 lines
(As a side note, I don't much see the interest of DEFAULT_VRM. Having the same default for all chips doesn't make much sense since older chips will most likely need VRM8 and newer chips will most likely need VRM9. So I'd propose to get rid of that define and let every driver pick whatever is relevant.)
Err, why don't we just read the CPU type out of /proc and set the VRM accordingly during 'sensors -s'... entirely in userspace?
You seem to suggest that each CPU type can only work with one VRM version. Are you sure?

Well, I wasn't sure at first. But later, I looked at some Intel docs which say that the VID signals come right from the processor package itself (1). So now I'm sure.

Quoted 4 lines

The CPU type information is also available in kernel-space, maybe even more easily (no parsing required), and I believe that setting the VRM from the CPU type belongs to the driver (once, at init time). I don't see how you are going to do that in "sensors".

Actually, "sensors -s". It wouldn't be hard - I mean, it already sets the VRM based on /etc/sensors.conf. But you're probably right that it belongs in the driver; perhaps another shared function of the i2c-sensors module.

Does anyone know how to read CPU id info from kernel space?

(1) http://www.intel.com/design/Pentium4/guides/24920504.pdf (page 8)

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Jean Delvare

2005-05-19 06:25 UTC #8 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Quoted 6 lines

Quoted 2 lines
You seem to suggest that each CPU type can only work with one VRM version. Are you sure?
Well, I wasn't sure at first. But later, I looked at some Intel docs which say that the VID signals come right from the processor package itself (1). So now I'm sure.

A CPU vs VRM version table would be welcome then.

Quoted 7 lines

Quoted 4 lines
The CPU type information is also available in kernel-space, maybe even more easily (no parsing required), and I believe that setting the VRM from the CPU type belongs to the driver (once, at init time). I don't see how you are going to do that in "sensors".
Actually, "sensors -s". It wouldn't be hard - I mean, it already sets the VRM based on /etc/sensors.conf.

My point isn't about how to do it, I can imagine that you'll parse /proc/cpuinfo and issue the correct libsensors action.

My point is about when you will do it. So far, only "set" lines of sensors.conf do trigger a write action, and in a very generic way. You would have to do something very specific for vrm, possibly conflicting with sensors.conf set lines. This is why I'm skeptical about your proposal.

Quoted 2 lines

But you're probably right that it belongs in the driver; perhaps another shared function of the i2c-sensors module.

It would even belong to i2c-vid.h (in which case we may need to create i2c-vid.c and build the i2c_sensor module from i2c-vid.c and i2c_sensor.c).

Quoted 1 line

Does anyone know how to read CPU id info from kernel space?

I have no clue and not much time to check right now. Grepping the sources for "cpuid" could help. Or you may try to locate where /proc/cpuinfo gets it from.

One may wonder if the change would allow us to get rid of the vrm sysfs file entirely. This would be great, but I'm not 100% sure. There are a few corner cases I can think of that may require our attention.

1* Evaluation boards (or more generaly, cases where the chip doesn't belong to the monitoring system). This is uncommon and people doing this are expected to know their stuff. VID readings are for comfort, you can live without them, especially in experimental context.

2* Multi-cpu systems. I'd expect such systems to use a single family of chips, and so a single VRM version for all CPUs.

All in all I'm quite confident that we could get rid of the additional interface (if and only if you hypothesis that we can know the VRM version from the CPUID is correct). But comments are welcome.

Thanks.

Signature · 2 lines

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

Greg KH

Applied

2005-05-19 06:25 UTC #9 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

Quoted 10 lines · Mark M. Hoffman

Hi Greg:
This patch applies on top of the previous one in the thread (which applied to 2.6.7-rc2). Please apply.
* * * * *
Pick a VRM (for VID interpretation) based on the VRM & OVT config, if available. Props to Jean Delvare <[email protected]> for the idea & code fragment.

Applied, thanks.

Oh, can you add the Signed-off-by: line to your patches from now on?

thanks,

greg k-h

Mark Studebaker

2005-05-19 06:25 UTC #10 lore raw

PATCH 1/1 add alternate VCORE calculations for w83627thf and

In reply to Mark M. Hoffman

agreed this is a good idea, and that CPU type absolutely determines VRM type. The long-term goal should be to do it in-kernel and remove /proc or /sys/.../vrm files for the various drivers.

Definitive CPU/VRM table surely a challenge but can be built up over time or just take good guesses.

mds

Quoted 64 lines · Jean Delvare

Quoted 6 lines
Quoted 2 lines
You seem to suggest that each CPU type can only work with one VRM version. Are you sure?
Well, I wasn't sure at first. But later, I looked at some Intel docs which say that the VID signals come right from the processor package itself (1). So now I'm sure.
A CPU vs VRM version table would be welcome then.
Quoted 7 lines
Quoted 4 lines
The CPU type information is also available in kernel-space, maybe even more easily (no parsing required), and I believe that setting the VRM from the CPU type belongs to the driver (once, at init time). I don't see how you are going to do that in "sensors".
Actually, "sensors -s". It wouldn't be hard - I mean, it already sets the VRM based on /etc/sensors.conf.
My point isn't about how to do it, I can imagine that you'll parse /proc/cpuinfo and issue the correct libsensors action.
My point is about when you will do it. So far, only "set" lines of sensors.conf do trigger a write action, and in a very generic way. You would have to do something very specific for vrm, possibly conflicting with sensors.conf set lines. This is why I'm skeptical about your proposal.
Quoted 2 lines
But you're probably right that it belongs in the driver; perhaps another shared function of the i2c-sensors module.
It would even belong to i2c-vid.h (in which case we may need to create i2c-vid.c and build the i2c_sensor module from i2c-vid.c and i2c_sensor.c).
Quoted 1 line
Does anyone know how to read CPU id info from kernel space?
I have no clue and not much time to check right now. Grepping the sources for "cpuid" could help. Or you may try to locate where /proc/cpuinfo gets it from.
One may wonder if the change would allow us to get rid of the vrm sysfs file entirely. This would be great, but I'm not 100% sure. There are a few corner cases I can think of that may require our attention.
1* Evaluation boards (or more generaly, cases where the chip doesn't belong to the monitoring system). This is uncommon and people doing this are expected to know their stuff. VID readings are for comfort, you can live without them, especially in experimental context.
2* Multi-cpu systems. I'd expect such systems to use a single family of chips, and so a single VRM version for all CPUs.
All in all I'm quite confident that we could get rid of the additional interface (if and only if you hypothesis that we can know the VRM version from the CPUID is correct). But comments are welcome.
Thanks.