[lm-sensors] [Patch] Adding threshold support to coretemp

[lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

[-- Attachment #1: Type: text/plain, Size: 8231 bytes --]

Hi Jean/Fenghua,

I am submitting a patch to enable core thermal threshold
Support to coretemp.c. There are two core thermal thresholds
available through sysfs interfaces temp1_core_thresh[0/1].

The expectation is that thresh0 is lesser than the current temperature
and thresh1 is higher than the current temperature. Whenever the current
temperature crosses these limits, an interrupt is generated.
This interrupt is handles by the user space to do power
Management via CPU throttling, etc..

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Tue, 14 Dec 2010 05:10:27 +0530
Subject: [PATCH] Adding_threshold_support_to_coretemp

This patch adds the core thermal threshold support to coretemp.c.
These thresholds can be read/written using the sysfs interface
temp1_core_thresh[0/1]. These can be used to generate interrupts,
to do dynamic power management.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/msr-index.h |   12 ++++
 drivers/hwmon/coretemp.c         |  133 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..31cefad 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_OFFSET_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_OFFSET_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 42de98d..4c221b8 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -42,6 +42,9 @@
 typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
 		SHOW_NAME } SHOW;
 
+/* C indicates core thermal thresholds */
+enum thresholds { C_TTHRESH0, C_TTHRESH1} THRESH;
+
 /*
  * Functions declaration
  */
@@ -59,9 +62,13 @@ struct coretemp_data {
 	int temp;
 	int tjmax;
 	int ttarget;
+	int c_tthresh0;
+	int c_tthresh1;
 	u8 alarm;
 };
 
+static int set_core_threshold(struct coretemp_data *data, int val,
+						enum thresholds thresh);
 /*
  * Sysfs stuff
  */
@@ -104,6 +111,41 @@ static ssize_t show_temp(struct device *dev,
 	return err;
 }
 
+static ssize_t show_threshold(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+
+	if (!data->valid)
+		return -EINVAL;
+
+	switch (attr->index) {
+	case C_TTHRESH0:
+		return sprintf(buf, "%d\n", data->c_tthresh0);
+	case C_TTHRESH1:
+		return sprintf(buf, "%d\n", data->c_tthresh1);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t set_threshold(struct device *dev,
+		struct device_attribute *devattr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+	unsigned long val;
+	int err;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	err = set_core_threshold(data, val, attr->index);
+
+	return (err) ? -EINVAL : count;
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
 			  SHOW_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
@@ -114,12 +156,19 @@ static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
 static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
 static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
 
+static SENSOR_DEVICE_ATTR(temp1_core_thresh0, S_IWUSR | S_IRUGO,
+				show_threshold,	set_threshold, C_TTHRESH0);
+static SENSOR_DEVICE_ATTR(temp1_core_thresh1, S_IWUSR | S_IRUGO,
+				show_threshold, set_threshold, C_TTHRESH1);
+
 static struct attribute *coretemp_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_temp1_label.dev_attr.attr,
 	&dev_attr_temp1_crit_alarm.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh0.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh1.dev_attr.attr,
 	NULL
 };
 
@@ -298,6 +347,77 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
 	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
 }
 
+static void configure_apic(void *info)
+{
+	u32 l;
+	int *flag = (int *)info;
+
+	l = apic_read(APIC_LVTTHMR);
+
+	if (*flag)	/* Non-Zero flag Masks the APIC */
+		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
+	else		/* Zero flag UnMasks the APIC */
+		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
+}
+
+static int set_core_threshold(struct coretemp_data *data, int temp,
+						enum thresholds thresh)
+{
+	u32 eax, edx;
+	int diff;
+	int flag = 1;
+
+	if (temp > data->tjmax)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	diff = (data->tjmax - temp)/1000;
+
+	/* Mask the APIC */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	if (thresh == C_TTHRESH0) {
+		eax = (eax & ~THERM_MASK_THRESHOLD0) |
+					(diff << THERM_OFFSET_THRESHOLD0);
+		data->c_tthresh0 = temp;
+	} else {
+		eax = (eax & ~THERM_MASK_THRESHOLD1) |
+					(diff << THERM_OFFSET_THRESHOLD1);
+		data->c_tthresh1 = temp;
+	}
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	/* Unmask the APIC */
+	flag = 0;
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	mutex_unlock(&data->update_lock);
+	return 0;
+}
+
+static int __devinit enable_thresh_support(struct coretemp_data *data)
+{
+	u32 eax, edx;
+	int flag = 1; /* Non-Zero Flag masks the apic */
+
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	flag = 0; /*Flag should be zero to unmask the apic */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	return 0;
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -353,6 +473,15 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
 	platform_set_drvdata(pdev, data);
 
+	/* Enable threshold support */
+	enable_thresh_support(data);
+
+	/* Set Initial Core thresholds.
+	 * The lower and upper threshold values here are assumed
+	 */
+	set_core_threshold(data, 0, C_TTHRESH0);
+	set_core_threshold(data, 90000, C_TTHRESH1);
+
 	/*
 	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
 	 * on older CPUs but not in this register,
@@ -405,6 +534,10 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
 	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh0.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh1.dev_attr);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 	return 0;
-- 
1.6.5.2



[-- Attachment #2: 0001-Adding_threshold_support_to_coretemp.patch --]
[-- Type: application/octet-stream, Size: 7328 bytes --]

From: Durgadoss R <durgadoss.r@intel.com>
Date: Tue, 14 Dec 2010 05:10:27 +0530
Subject: [PATCH] Adding_threshold_support_to_coretemp

This patch adds the core thermal threshold support to coretemp.c.
These thresholds can be read/written using the sysfs interface
temp1_core_thresh[0/1]. These can be used to generate interrupts,
to do dynamic power management.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 arch/x86/include/asm/msr-index.h |   12 ++++
 drivers/hwmon/coretemp.c         |  133 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..31cefad 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_OFFSET_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_OFFSET_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 42de98d..4c221b8 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -42,6 +42,9 @@
 typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
 		SHOW_NAME } SHOW;
 
+/* C indicates core thermal thresholds */
+enum thresholds { C_TTHRESH0, C_TTHRESH1} THRESH;
+
 /*
  * Functions declaration
  */
@@ -59,9 +62,13 @@ struct coretemp_data {
 	int temp;
 	int tjmax;
 	int ttarget;
+	int c_tthresh0;
+	int c_tthresh1;
 	u8 alarm;
 };
 
+static int set_core_threshold(struct coretemp_data *data, int val,
+						enum thresholds thresh);
 /*
  * Sysfs stuff
  */
@@ -104,6 +111,41 @@ static ssize_t show_temp(struct device *dev,
 	return err;
 }
 
+static ssize_t show_threshold(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+
+	if (!data->valid)
+		return -EINVAL;
+
+	switch (attr->index) {
+	case C_TTHRESH0:
+		return sprintf(buf, "%d\n", data->c_tthresh0);
+	case C_TTHRESH1:
+		return sprintf(buf, "%d\n", data->c_tthresh1);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t set_threshold(struct device *dev,
+		struct device_attribute *devattr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+	unsigned long val;
+	int err;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	err = set_core_threshold(data, val, attr->index);
+
+	return (err) ? -EINVAL : count;
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
 			  SHOW_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
@@ -114,12 +156,19 @@ static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
 static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
 static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
 
+static SENSOR_DEVICE_ATTR(temp1_core_thresh0, S_IWUSR | S_IRUGO,
+				show_threshold,	set_threshold, C_TTHRESH0);
+static SENSOR_DEVICE_ATTR(temp1_core_thresh1, S_IWUSR | S_IRUGO,
+				show_threshold, set_threshold, C_TTHRESH1);
+
 static struct attribute *coretemp_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_temp1_label.dev_attr.attr,
 	&dev_attr_temp1_crit_alarm.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh0.dev_attr.attr,
+	&sensor_dev_attr_temp1_core_thresh1.dev_attr.attr,
 	NULL
 };
 
@@ -298,6 +347,77 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
 	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
 }
 
+static void configure_apic(void *info)
+{
+	u32 l;
+	int *flag = (int *)info;
+
+	l = apic_read(APIC_LVTTHMR);
+
+	if (*flag)	/* Non-Zero flag Masks the APIC */
+		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
+	else		/* Zero flag UnMasks the APIC */
+		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
+}
+
+static int set_core_threshold(struct coretemp_data *data, int temp,
+						enum thresholds thresh)
+{
+	u32 eax, edx;
+	int diff;
+	int flag = 1;
+
+	if (temp > data->tjmax)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	diff = (data->tjmax - temp)/1000;
+
+	/* Mask the APIC */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	if (thresh == C_TTHRESH0) {
+		eax = (eax & ~THERM_MASK_THRESHOLD0) |
+					(diff << THERM_OFFSET_THRESHOLD0);
+		data->c_tthresh0 = temp;
+	} else {
+		eax = (eax & ~THERM_MASK_THRESHOLD1) |
+					(diff << THERM_OFFSET_THRESHOLD1);
+		data->c_tthresh1 = temp;
+	}
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	/* Unmask the APIC */
+	flag = 0;
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	mutex_unlock(&data->update_lock);
+	return 0;
+}
+
+static int __devinit enable_thresh_support(struct coretemp_data *data)
+{
+	u32 eax, edx;
+	int flag = 1; /* Non-Zero Flag masks the apic */
+
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	flag = 0; /*Flag should be zero to unmask the apic */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	return 0;
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -353,6 +473,15 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
 	platform_set_drvdata(pdev, data);
 
+	/* Enable threshold support */
+	enable_thresh_support(data);
+
+	/* Set Initial Core thresholds.
+	 * The lower and upper threshold values here are assumed
+	 */
+	set_core_threshold(data, 0, C_TTHRESH0);
+	set_core_threshold(data, 90000, C_TTHRESH1);
+
 	/*
 	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
 	 * on older CPUs but not in this register,
@@ -405,6 +534,10 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
 	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh0.dev_attr);
+	device_remove_file(&pdev->dev,
+				&sensor_dev_attr_temp1_core_thresh1.dev_attr);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 	return 0;
-- 
1.6.5.2


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Tue, Dec 14, 2010 at 06:55:35AM -0500, R, Durgadoss wrote:
> Hi Jean/Fenghua,
> 
> I am submitting a patch to enable core thermal threshold
> Support to coretemp.c. There are two core thermal thresholds
> available through sysfs interfaces temp1_core_thresh[0/1].
> 
> The expectation is that thresh0 is lesser than the current temperature
> and thresh1 is higher than the current temperature. Whenever the current
> temperature crosses these limits, an interrupt is generated.
> This interrupt is handles by the user space to do power
> Management via CPU throttling, etc..
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> Kindly review and merge.
> ------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Tue, 14 Dec 2010 05:10:27 +0530
> Subject: [PATCH] Adding_threshold_support_to_coretemp
> 
> This patch adds the core thermal threshold support to coretemp.c.
> These thresholds can be read/written using the sysfs interface
> temp1_core_thresh[0/1]. These can be used to generate interrupts,
> to do dynamic power management.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
I am not inclined to look further into this patch until the ABI questions are resolved.
As written, the patch gets my NACK simply because it introduces random driver specific
new ABI attributes.

Guenter

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Tue, Dec 14, 2010 at 06:55:35AM -0500, R, Durgadoss wrote:
> Hi Jean/Fenghua,
> 
> I am submitting a patch to enable core thermal threshold
> Support to coretemp.c. There are two core thermal thresholds
> available through sysfs interfaces temp1_core_thresh[0/1].
> 
> The expectation is that thresh0 is lesser than the current temperature
> and thresh1 is higher than the current temperature. Whenever the current
> temperature crosses these limits, an interrupt is generated.
> This interrupt is handles by the user space to do power
> Management via CPU throttling, etc..
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> Kindly review and merge.
> ------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Tue, 14 Dec 2010 05:10:27 +0530
> Subject: [PATCH] Adding_threshold_support_to_coretemp
> 
> This patch adds the core thermal threshold support to coretemp.c.
> These thresholds can be read/written using the sysfs interface
> temp1_core_thresh[0/1]. These can be used to generate interrupts,
> to do dynamic power management.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
I am not inclined to look further into this patch until the ABI questions are resolved.
As written, the patch gets my NACK simply because it introduces random driver specific
new ABI attributes.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

RE: [lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

Hi Guenter,

> > ------------------------------------------------------------------
> > From: Durgadoss R <durgadoss.r@intel.com>
> >
> > Date: Tue, 14 Dec 2010 05:10:27 +0530
> > Subject: [PATCH] Adding_threshold_support_to_coretemp
> >
> > This patch adds the core thermal threshold support to coretemp.c.
> > These thresholds can be read/written using the sysfs interface
> > temp1_core_thresh[0/1]. These can be used to generate interrupts,
> > to do dynamic power management.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >
> I am not inclined to look further into this patch until the ABI questions are
> resolved.
> As written, the patch gets my NACK simply because it introduces random driver
> specific
> new ABI attributes.
> 

Since the Hardware supports the programming of thresholds, I added this support
in the driver. I had to add new attributes, since there are no existing attributes,
that support these thresholds.
Could you please suggest any better way(s) of enabling this support ?
I can add documentation for these attributes in Doc/hwmon/sysfs-interface.

Thanks,
Durga

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

Hi Guenter,

> > ------------------------------------------------------------------
> > From: Durgadoss R <durgadoss.r@intel.com>
> >
> > Date: Tue, 14 Dec 2010 05:10:27 +0530
> > Subject: [PATCH] Adding_threshold_support_to_coretemp
> >
> > This patch adds the core thermal threshold support to coretemp.c.
> > These thresholds can be read/written using the sysfs interface
> > temp1_core_thresh[0/1]. These can be used to generate interrupts,
> > to do dynamic power management.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >
> I am not inclined to look further into this patch until the ABI questions are
> resolved.
> As written, the patch gets my NACK simply because it introduces random driver
> specific
> new ABI attributes.
> 

Since the Hardware supports the programming of thresholds, I added this support
in the driver. I had to add new attributes, since there are no existing attributes,
that support these thresholds.
Could you please suggest any better way(s) of enabling this support ?
I can add documentation for these attributes in Doc/hwmon/sysfs-interface.

Thanks,
Durga

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Wed, Dec 15, 2010 at 07:29:15AM -0500, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > ------------------------------------------------------------------
> > > From: Durgadoss R <durgadoss.r@intel.com>
> > >
> > > Date: Tue, 14 Dec 2010 05:10:27 +0530
> > > Subject: [PATCH] Adding_threshold_support_to_coretemp
> > >
> > > This patch adds the core thermal threshold support to coretemp.c.
> > > These thresholds can be read/written using the sysfs interface
> > > temp1_core_thresh[0/1]. These can be used to generate interrupts,
> > > to do dynamic power management.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > >
> > I am not inclined to look further into this patch until the ABI questions are
> > resolved.
> > As written, the patch gets my NACK simply because it introduces random driver
> > specific
> > new ABI attributes.
> > 
> 
> Since the Hardware supports the programming of thresholds, I added this support
> in the driver. I had to add new attributes, since there are no existing attributes,
> that support these thresholds.
> Could you please suggest any better way(s) of enabling this support ?
> I can add documentation for these attributes in Doc/hwmon/sysfs-interface.
> 
At least two.

1) Use existing ABI attributes. The coretemp driver doesn't use all available attributes
for low/high temperatures, so you could pick unused ones (and if necessary redefine the ones 
already used). We have
	lcrit
	min
	max
	crit
	emergency

All those attributes are ultimately thresholds; no reason not to use them.

Furthermore, the two thresholds are really upper/lower temperature targets which
result in requesting a specific action. As such, it is really just one (upper) threshold
with an associated hysteresis value to undo the action caused by the upper threshold.
So you could use
	tempX_max
	tempX_max_hyst
or
	tempX_crit
	tempX_crit_hyst
or
	tempX_emergency
	tempX_emergency_hyst
instead to reflect this meaning.

I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each other.
Maybe you could simply use tempX_max as upper threshold (where it exists),
with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use tempX_max_hyst
to calculate the lower threshold.

2) Define new attribute names which can be used as generic ABI, such as
	tempX_threshold
	tempX_threshold_hyst

Right now I don't really see the need for additional attributes, since there are spare ones
available, so 2) is less likely to be accepted. You would have to make a really good case
for it, and not just to me but convince Jean as well.

Thanks,
Guenter

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Wed, Dec 15, 2010 at 07:29:15AM -0500, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > ------------------------------------------------------------------
> > > From: Durgadoss R <durgadoss.r@intel.com>
> > >
> > > Date: Tue, 14 Dec 2010 05:10:27 +0530
> > > Subject: [PATCH] Adding_threshold_support_to_coretemp
> > >
> > > This patch adds the core thermal threshold support to coretemp.c.
> > > These thresholds can be read/written using the sysfs interface
> > > temp1_core_thresh[0/1]. These can be used to generate interrupts,
> > > to do dynamic power management.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > >
> > I am not inclined to look further into this patch until the ABI questions are
> > resolved.
> > As written, the patch gets my NACK simply because it introduces random driver
> > specific
> > new ABI attributes.
> > 
> 
> Since the Hardware supports the programming of thresholds, I added this support
> in the driver. I had to add new attributes, since there are no existing attributes,
> that support these thresholds.
> Could you please suggest any better way(s) of enabling this support ?
> I can add documentation for these attributes in Doc/hwmon/sysfs-interface.
> 
At least two.

1) Use existing ABI attributes. The coretemp driver doesn't use all available attributes
for low/high temperatures, so you could pick unused ones (and if necessary redefine the ones 
already used). We have
	lcrit
	min
	max
	crit
	emergency

All those attributes are ultimately thresholds; no reason not to use them.

Furthermore, the two thresholds are really upper/lower temperature targets which
result in requesting a specific action. As such, it is really just one (upper) threshold
with an associated hysteresis value to undo the action caused by the upper threshold.
So you could use
	tempX_max
	tempX_max_hyst
or
	tempX_crit
	tempX_crit_hyst
or
	tempX_emergency
	tempX_emergency_hyst
instead to reflect this meaning.

I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each other.
Maybe you could simply use tempX_max as upper threshold (where it exists),
with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use tempX_max_hyst
to calculate the lower threshold.

2) Define new attribute names which can be used as generic ABI, such as
	tempX_threshold
	tempX_threshold_hyst

Right now I don't really see the need for additional attributes, since there are spare ones
available, so 2) is less likely to be accepted. You would have to make a really good case
for it, and not just to me but convince Jean as well.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

RE: [lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

Hi Guenter,

Thanks for the suggestions.
I need clarification on some of the things you mentioned here.
Kindly help.

> > Since the Hardware supports the programming of thresholds, I added this
> support
> > in the driver. I had to add new attributes, since there are no existing
> attributes,
> > that support these thresholds.
> > Could you please suggest any better way(s) of enabling this support ?
> > I can add documentation for these attributes in Doc/hwmon/sysfs-interface.
> >
> At least two.
> 
> 1) Use existing ABI attributes. The coretemp driver doesn't use all available
> attributes
> for low/high temperatures, so you could pick unused ones (and if necessary
> redefine the ones
> already used). We have
> 	lcrit
> 	min
> 	max
> 	crit
> 	emergency
> 
> All those attributes are ultimately thresholds; no reason not to use them.

These are thresholds only. But their corresponding documentation in sysfs-interface
reflects completely different meaning from what these thresholds do. When we program
these thresholds, we get an interrupt, if the current temperature goes below the
lower threshold (thresh0) or above the upper threshold (thresh1). As per the documentation,
none of these existing attributes generate interrupt like this. In fact, I had an
idea to use _max and _min. Then the meaning will be contradictory. Hence I refrained,
from using that.

> 
> Furthermore, the two thresholds are really upper/lower temperature targets
> which
> result in requesting a specific action. As such, it is really just one (upper)
> threshold
> with an associated hysteresis value to undo the action caused by the upper
> threshold.
> So you could use
> 	tempX_max
> 	tempX_max_hyst
> or
> 	tempX_crit
> 	tempX_crit_hyst
> or
> 	tempX_emergency
> 	tempX_emergency_hyst
> instead to reflect this meaning.
> 

Here also, the _max and _crit have already been used. Even if we redefine,
it will not adhere to the Documentation. Since the thresh0 and thresh1 both,
are lesser than _crit, I feel it's not really nice to use _emergency.
(But if that's the only way and we all are Ok with that, I can use it,
with a bit of change in Documentation..)

> I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each
> other.
> Maybe you could simply use tempX_max as upper threshold (where it exists),
> with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use
> tempX_max_hyst
> to calculate the lower threshold.

The _thresh value is lower than _max, which in turn is lower than _crit.
The newer generation of CPU's don't support the IA32_TEMP_TARGET register,
and hence the _max is almost never used. But since it has already been used,
as "ttarget", I can't use it for this upper threshold. Otherwise, I do not
have any problem in using _min and _max. Though, we have to modify the
documentation. I am proposing this idea, since you said "redefine" old ones if
necessary. Please let me know your opinion here.

> 
> 2) Define new attribute names which can be used as generic ABI, such as
> 	tempX_threshold
> 	tempX_threshold_hyst
> 
> Right now I don't really see the need for additional attributes, since there
> are spare ones
> available, so 2) is less likely to be accepted. You would have to make a really
> good case
> for it, and not just to me but convince Jean as well.

I understand there are available spare attributes; but seems like they have been
defined with some other meaning. I also wanted to refrain from introducing
new attributes as much as possible. But, because of the above-mentioned issues,
I had to arrive at this solution.

Thanks,
Durga

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[R, Durgadoss]

Hi Guenter,

Thanks for the suggestions.
I need clarification on some of the things you mentioned here.
Kindly help.

> > Since the Hardware supports the programming of thresholds, I added this
> support
> > in the driver. I had to add new attributes, since there are no existing
> attributes,
> > that support these thresholds.
> > Could you please suggest any better way(s) of enabling this support ?
> > I can add documentation for these attributes in Doc/hwmon/sysfs-interface.
> >
> At least two.
> 
> 1) Use existing ABI attributes. The coretemp driver doesn't use all available
> attributes
> for low/high temperatures, so you could pick unused ones (and if necessary
> redefine the ones
> already used). We have
> 	lcrit
> 	min
> 	max
> 	crit
> 	emergency
> 
> All those attributes are ultimately thresholds; no reason not to use them.

These are thresholds only. But their corresponding documentation in sysfs-interface
reflects completely different meaning from what these thresholds do. When we program
these thresholds, we get an interrupt, if the current temperature goes below the
lower threshold (thresh0) or above the upper threshold (thresh1). As per the documentation,
none of these existing attributes generate interrupt like this. In fact, I had an
idea to use _max and _min. Then the meaning will be contradictory. Hence I refrained,
from using that.

> 
> Furthermore, the two thresholds are really upper/lower temperature targets
> which
> result in requesting a specific action. As such, it is really just one (upper)
> threshold
> with an associated hysteresis value to undo the action caused by the upper
> threshold.
> So you could use
> 	tempX_max
> 	tempX_max_hyst
> or
> 	tempX_crit
> 	tempX_crit_hyst
> or
> 	tempX_emergency
> 	tempX_emergency_hyst
> instead to reflect this meaning.
> 

Here also, the _max and _crit have already been used. Even if we redefine,
it will not adhere to the Documentation. Since the thresh0 and thresh1 both,
are lesser than _crit, I feel it's not really nice to use _emergency.
(But if that's the only way and we all are Ok with that, I can use it,
with a bit of change in Documentation..)

> I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each
> other.
> Maybe you could simply use tempX_max as upper threshold (where it exists),
> with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use
> tempX_max_hyst
> to calculate the lower threshold.

The _thresh value is lower than _max, which in turn is lower than _crit.
The newer generation of CPU's don't support the IA32_TEMP_TARGET register,
and hence the _max is almost never used. But since it has already been used,
as "ttarget", I can't use it for this upper threshold. Otherwise, I do not
have any problem in using _min and _max. Though, we have to modify the
documentation. I am proposing this idea, since you said "redefine" old ones if
necessary. Please let me know your opinion here.

> 
> 2) Define new attribute names which can be used as generic ABI, such as
> 	tempX_threshold
> 	tempX_threshold_hyst
> 
> Right now I don't really see the need for additional attributes, since there
> are spare ones
> available, so 2) is less likely to be accepted. You would have to make a really
> good case
> for it, and not just to me but convince Jean as well.

I understand there are available spare attributes; but seems like they have been
defined with some other meaning. I also wanted to refrain from introducing
new attributes as much as possible. But, because of the above-mentioned issues,
I had to arrive at this solution.

Thanks,
Durga

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Thu, Dec 16, 2010 at 05:48:58AM -0500, R, Durgadoss wrote:
[ ... ]
> 
> These are thresholds only. But their corresponding documentation in sysfs-interface
> reflects completely different meaning from what these thresholds do. When we program
> these thresholds, we get an interrupt, if the current temperature goes below the
> lower threshold (thresh0) or above the upper threshold (thresh1). As per the documentation,
> none of these existing attributes generate interrupt like this. In fact, I had an
> idea to use _max and _min. Then the meaning will be contradictory. Hence I refrained,
> from using that.
> 
temp[1-*]_min   Temperature min value.
temp[1-*]_max   Temperature max value.
temp[1-*]_max_hyst
                Temperature hysteresis value for max limit.

I don't see a limitation in use here. It only says min/max/hysteresis value, nor what
that means or implies, nor if there can an interrupt be associated with it.
I don't see anything contradictory.

> Here also, the _max and _crit have already been used. Even if we redefine,
> it will not adhere to the Documentation. Since the thresh0 and thresh1 both,

Again, why ? Where would it not adhere to the documentation ?

> are lesser than _crit, I feel it's not really nice to use _emergency.
> (But if that's the only way and we all are Ok with that, I can use it,
> with a bit of change in Documentation..)
> 
Not really.

> > I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each
> > other.
> > Maybe you could simply use tempX_max as upper threshold (where it exists),
> > with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use
> > tempX_max_hyst
> > to calculate the lower threshold.
> 
> The _thresh value is lower than _max, which in turn is lower than _crit.
> The newer generation of CPU's don't support the IA32_TEMP_TARGET register,
> and hence the _max is almost never used. But since it has already been used,
> as "ttarget", I can't use it for this upper threshold. Otherwise, I do not
> have any problem in using _min and _max. Though, we have to modify the
> documentation. I am proposing this idea, since you said "redefine" old ones if
> necessary. Please let me know your opinion here.
> 
As you state yourself, IA32_TEMP_TARGET is no longer supported, and it is only used
to set the value of _max (ie to instantiate ttarget). What is done with it subsequently
is another question. And if the CPUs supporting IA32_TEMP_TARGET don't support the
threshold registers, the question does not even arise.

> I understand there are available spare attributes; but seems like they have been
> defined with some other meaning. I also wanted to refrain from introducing
> new attributes as much as possible. But, because of the above-mentioned issues,
> I had to arrive at this solution.
> 
My recommendation would be to use _max as upper threshold, instantiate it with IA32_TEMP_TARGET
if supported, and make it writable. Then use _max_threshold for the lower threshold.
I don't see any problem with that. Then explain in Documentation/sysfs/coretemp what 
exactly happens if the limits are reached.

Guenter

Re: [lm-sensors] [Patch] Adding threshold support to coretemp

[Guenter Roeck]

On Thu, Dec 16, 2010 at 05:48:58AM -0500, R, Durgadoss wrote:
[ ... ]
> 
> These are thresholds only. But their corresponding documentation in sysfs-interface
> reflects completely different meaning from what these thresholds do. When we program
> these thresholds, we get an interrupt, if the current temperature goes below the
> lower threshold (thresh0) or above the upper threshold (thresh1). As per the documentation,
> none of these existing attributes generate interrupt like this. In fact, I had an
> idea to use _max and _min. Then the meaning will be contradictory. Hence I refrained,
> from using that.
> 
temp[1-*]_min   Temperature min value.
temp[1-*]_max   Temperature max value.
temp[1-*]_max_hyst
                Temperature hysteresis value for max limit.

I don't see a limitation in use here. It only says min/max/hysteresis value, nor what
that means or implies, nor if there can an interrupt be associated with it.
I don't see anything contradictory.

> Here also, the _max and _crit have already been used. Even if we redefine,
> it will not adhere to the Documentation. Since the thresh0 and thresh1 both,

Again, why ? Where would it not adhere to the documentation ?

> are lesser than _crit, I feel it's not really nice to use _emergency.
> (But if that's the only way and we all are Ok with that, I can use it,
> with a bit of change in Documentation..)
> 
Not really.

> > I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each
> > other.
> > Maybe you could simply use tempX_max as upper threshold (where it exists),
> > with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use
> > tempX_max_hyst
> > to calculate the lower threshold.
> 
> The _thresh value is lower than _max, which in turn is lower than _crit.
> The newer generation of CPU's don't support the IA32_TEMP_TARGET register,
> and hence the _max is almost never used. But since it has already been used,
> as "ttarget", I can't use it for this upper threshold. Otherwise, I do not
> have any problem in using _min and _max. Though, we have to modify the
> documentation. I am proposing this idea, since you said "redefine" old ones if
> necessary. Please let me know your opinion here.
> 
As you state yourself, IA32_TEMP_TARGET is no longer supported, and it is only used
to set the value of _max (ie to instantiate ttarget). What is done with it subsequently
is another question. And if the CPUs supporting IA32_TEMP_TARGET don't support the
threshold registers, the question does not even arise.

> I understand there are available spare attributes; but seems like they have been
> defined with some other meaning. I also wanted to refrain from introducing
> new attributes as much as possible. But, because of the above-mentioned issues,
> I had to arrive at this solution.
> 
My recommendation would be to use _max as upper threshold, instantiate it with IA32_TEMP_TARGET
if supported, and make it writable. Then use _max_threshold for the lower threshold.
I don't see any problem with that. Then explain in Documentation/sysfs/coretemp what 
exactly happens if the limits are reached.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[R, Durgadoss]

[-- Attachment #1: Type: text/plain, Size: 9211 bytes --]

Hi,

I am submitting a patch to enable core thermal threshold
Support to coretemp.c. There are two core thermal thresholds
available through sysfs interfaces temp1_max and temp1_min.

The expectation is that _min is lesser than the current temperature
and _max is higher than the current temperature. Whenever the current
temperature crosses these limits, an interrupt is generated.

This patch is generated against stable Linux-2.6 kernel.

As per Guenter's earlier comments the ABI names are changed to
_max and _min.

Kindly review and merge.
------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Thu, 16 Dec 2010 23:09:54 +0530
Subject: [PATCH] Adding_threshold_support_to_coretemp

This patch adds the core thermal threshold support to coretemp.c.
These thresholds can be read/written using the sysfs interface
temp1_max and temp1_min. These can be used to generate interrupts,
to do dynamic power management.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 Documentation/hwmon/coretemp     |    6 ++
 arch/x86/include/asm/msr-index.h |   12 ++++
 drivers/hwmon/coretemp.c         |  118 +++++++++++++++++++++++++++++++++++---
 3 files changed, 127 insertions(+), 9 deletions(-)

diff --git a/Documentation/hwmon/coretemp b/Documentation/hwmon/coretemp
index 25568f8..096cd8b 100644
--- a/Documentation/hwmon/coretemp
+++ b/Documentation/hwmon/coretemp
@@ -29,6 +29,12 @@ the Out-Of-Spec bit. Following table summarizes the exported sysfs files:
 
 temp1_input	 - Core temperature (in millidegrees Celsius).
 temp1_max	 - All cooling devices should be turned on (on Core2).
+		   If the IA32_TEMPERATURE_TARGET is not supported, this
+		   value indicates the higher core threshold. When the CPU
+		   temperature crosses this temperature, an interrupt is
+		   generated.
+temp1_min	 - Indicates the lower threshold of the core. An interrupt is
+		   generated when CPU temperature crosses this threshold.
 temp1_crit	 - Maximum junction temperature (in millidegrees Celsius).
 temp1_crit_alarm - Set when Out-of-spec bit is set, never clears.
 		   Correct CPU operation is no longer guaranteed.
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..31cefad 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_OFFSET_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_OFFSET_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 42de98d..30d873e 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -39,7 +39,7 @@
 
 #define DRVNAME	"coretemp"
 
-typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
+typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_TMIN, SHOW_LABEL,
 		SHOW_NAME } SHOW;
 
 /*
@@ -59,9 +59,11 @@ struct coretemp_data {
 	int temp;
 	int tjmax;
 	int ttarget;
+	int tmin;
 	u8 alarm;
 };
 
+static int set_core_threshold(struct coretemp_data *data, int val, int indx);
 /*
  * Sysfs stuff
  */
@@ -99,17 +101,37 @@ static ssize_t show_temp(struct device *dev,
 		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
 	else if (attr->index == SHOW_TJMAX)
 		err = sprintf(buf, "%d\n", data->tjmax);
-	else
+	else if (attr->index == SHOW_TTARGET)
 		err = sprintf(buf, "%d\n", data->ttarget);
+	else
+		err = sprintf(buf, "%d\n", data->tmin);
 	return err;
 }
 
+static ssize_t store_temp(struct device *dev,
+		struct device_attribute *devattr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+	unsigned long val;
+	int err;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	err = set_core_threshold(data, val, attr->index);
+
+	return (err) ? -EINVAL : count;
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
 			  SHOW_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
 			  SHOW_TJMAX);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
-			  SHOW_TTARGET);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp,
+						store_temp, SHOW_TTARGET);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp,
+						store_temp, SHOW_TMIN);
 static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
 static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
 static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
@@ -120,6 +142,8 @@ static struct attribute *coretemp_attributes[] = {
 	&dev_attr_temp1_crit_alarm.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
 	NULL
 };
 
@@ -298,6 +322,76 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
 	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
 }
 
+static void configure_apic(void *info)
+{
+	u32 l;
+	int *flag = (int *)info;
+
+	l = apic_read(APIC_LVTTHMR);
+
+	if (*flag)	/* Non-Zero flag Masks the APIC */
+		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
+	else		/* Zero flag UnMasks the APIC */
+		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
+}
+
+static int set_core_threshold(struct coretemp_data *data, int temp, int thres)
+{
+	u32 eax, edx;
+	int diff;
+	int flag = 1;
+
+	if (temp > data->tjmax)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	diff = (data->tjmax - temp)/1000;
+
+	/* Mask the APIC */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	if (thres == SHOW_TMIN) {
+		eax = (eax & ~THERM_MASK_THRESHOLD0) |
+					(diff << THERM_OFFSET_THRESHOLD0);
+		data->tmin = temp;
+	} else {
+		eax = (eax & ~THERM_MASK_THRESHOLD1) |
+					(diff << THERM_OFFSET_THRESHOLD1);
+		data->ttarget = temp;
+	}
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	/* Unmask the APIC */
+	flag = 0;
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	mutex_unlock(&data->update_lock);
+	return 0;
+}
+
+static int __devinit enable_thresh_support(struct coretemp_data *data)
+{
+	u32 eax, edx;
+	int flag = 1; /* Non-Zero Flag masks the apic */
+
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	flag = 0; /*Flag should be zero to unmask the apic */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	return 0;
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -353,10 +447,21 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
 	platform_set_drvdata(pdev, data);
 
+	/* Enable threshold support */
+	enable_thresh_support(data);
+
+	/* Set Initial Core thresholds.
+	 * The lower and upper threshold values here are assumed
+	 */
+	set_core_threshold(data, 0, SHOW_TMIN);
+	set_core_threshold(data, 90000, SHOW_TTARGET);
+
 	/*
 	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
 	 * on older CPUs but not in this register,
 	 * Atoms don't have it either.
+	 * If this register is not supported, then ttarget has the value
+	 * of upper core threshold, set by set_core_threshold;
 	 */
 
 	if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
@@ -368,10 +473,6 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 		} else {
 			data->ttarget = data->tjmax -
 					(((eax >> 8) & 0xff) * 1000);
-			err = device_create_file(&pdev->dev,
-					&sensor_dev_attr_temp1_max.dev_attr);
-			if (err)
-				goto exit_free;
 		}
 	}
 
@@ -404,7 +505,6 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
-	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 	return 0;
-- 
1.6.5.2


[-- Attachment #2: 0001-Adding_threshold_support_to_coretemp.patch --]
[-- Type: application/octet-stream, Size: 8325 bytes --]

From: Durgadoss R <durgadoss.r@intel.com>
Date: Thu, 16 Dec 2010 23:09:54 +0530
Subject: [PATCH] Adding_threshold_support_to_coretemp

This patch adds the core thermal threshold support to coretemp.c.
These thresholds can be read/written using the sysfs interface
temp1_max and temp1_min. These can be used to generate interrupts,
to do dynamic power management.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 Documentation/hwmon/coretemp     |    6 ++
 arch/x86/include/asm/msr-index.h |   12 ++++
 drivers/hwmon/coretemp.c         |  118 +++++++++++++++++++++++++++++++++++---
 3 files changed, 127 insertions(+), 9 deletions(-)

diff --git a/Documentation/hwmon/coretemp b/Documentation/hwmon/coretemp
index 25568f8..096cd8b 100644
--- a/Documentation/hwmon/coretemp
+++ b/Documentation/hwmon/coretemp
@@ -29,6 +29,12 @@ the Out-Of-Spec bit. Following table summarizes the exported sysfs files:
 
 temp1_input	 - Core temperature (in millidegrees Celsius).
 temp1_max	 - All cooling devices should be turned on (on Core2).
+		   If the IA32_TEMPERATURE_TARGET is not supported, this
+		   value indicates the higher core threshold. When the CPU
+		   temperature crosses this temperature, an interrupt is
+		   generated.
+temp1_min	 - Indicates the lower threshold of the core. An interrupt is
+		   generated when CPU temperature crosses this threshold.
 temp1_crit	 - Maximum junction temperature (in millidegrees Celsius).
 temp1_crit_alarm - Set when Out-of-spec bit is set, never clears.
 		   Correct CPU operation is no longer guaranteed.
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..31cefad 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_OFFSET_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_OFFSET_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 42de98d..30d873e 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -39,7 +39,7 @@
 
 #define DRVNAME	"coretemp"
 
-typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
+typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_TMIN, SHOW_LABEL,
 		SHOW_NAME } SHOW;
 
 /*
@@ -59,9 +59,11 @@ struct coretemp_data {
 	int temp;
 	int tjmax;
 	int ttarget;
+	int tmin;
 	u8 alarm;
 };
 
+static int set_core_threshold(struct coretemp_data *data, int val, int indx);
 /*
  * Sysfs stuff
  */
@@ -99,17 +101,37 @@ static ssize_t show_temp(struct device *dev,
 		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
 	else if (attr->index == SHOW_TJMAX)
 		err = sprintf(buf, "%d\n", data->tjmax);
-	else
+	else if (attr->index == SHOW_TTARGET)
 		err = sprintf(buf, "%d\n", data->ttarget);
+	else
+		err = sprintf(buf, "%d\n", data->tmin);
 	return err;
 }
 
+static ssize_t store_temp(struct device *dev,
+		struct device_attribute *devattr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct coretemp_data *data = coretemp_update_device(dev);
+	unsigned long val;
+	int err;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	err = set_core_threshold(data, val, attr->index);
+
+	return (err) ? -EINVAL : count;
+}
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
 			  SHOW_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
 			  SHOW_TJMAX);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
-			  SHOW_TTARGET);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp,
+						store_temp, SHOW_TTARGET);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp,
+						store_temp, SHOW_TMIN);
 static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
 static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
 static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
@@ -120,6 +142,8 @@ static struct attribute *coretemp_attributes[] = {
 	&dev_attr_temp1_crit_alarm.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
 	NULL
 };
 
@@ -298,6 +322,76 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
 	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
 }
 
+static void configure_apic(void *info)
+{
+	u32 l;
+	int *flag = (int *)info;
+
+	l = apic_read(APIC_LVTTHMR);
+
+	if (*flag)	/* Non-Zero flag Masks the APIC */
+		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
+	else		/* Zero flag UnMasks the APIC */
+		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
+}
+
+static int set_core_threshold(struct coretemp_data *data, int temp, int thres)
+{
+	u32 eax, edx;
+	int diff;
+	int flag = 1;
+
+	if (temp > data->tjmax)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	diff = (data->tjmax - temp)/1000;
+
+	/* Mask the APIC */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	if (thres == SHOW_TMIN) {
+		eax = (eax & ~THERM_MASK_THRESHOLD0) |
+					(diff << THERM_OFFSET_THRESHOLD0);
+		data->tmin = temp;
+	} else {
+		eax = (eax & ~THERM_MASK_THRESHOLD1) |
+					(diff << THERM_OFFSET_THRESHOLD1);
+		data->ttarget = temp;
+	}
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	/* Unmask the APIC */
+	flag = 0;
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	mutex_unlock(&data->update_lock);
+	return 0;
+}
+
+static int __devinit enable_thresh_support(struct coretemp_data *data)
+{
+	u32 eax, edx;
+	int flag = 1; /* Non-Zero Flag masks the apic */
+
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
+
+	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
+
+	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
+
+	flag = 0; /*Flag should be zero to unmask the apic */
+	smp_call_function_single(data->id, &configure_apic, &flag, 1);
+
+	return 0;
+}
+
 static int __devinit coretemp_probe(struct platform_device *pdev)
 {
 	struct coretemp_data *data;
@@ -353,10 +447,21 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
 	platform_set_drvdata(pdev, data);
 
+	/* Enable threshold support */
+	enable_thresh_support(data);
+
+	/* Set Initial Core thresholds.
+	 * The lower and upper threshold values here are assumed
+	 */
+	set_core_threshold(data, 0, SHOW_TMIN);
+	set_core_threshold(data, 90000, SHOW_TTARGET);
+
 	/*
 	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
 	 * on older CPUs but not in this register,
 	 * Atoms don't have it either.
+	 * If this register is not supported, then ttarget has the value
+	 * of upper core threshold, set by set_core_threshold;
 	 */
 
 	if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
@@ -368,10 +473,6 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
 		} else {
 			data->ttarget = data->tjmax -
 					(((eax >> 8) & 0xff) * 1000);
-			err = device_create_file(&pdev->dev,
-					&sensor_dev_attr_temp1_max.dev_attr);
-			if (err)
-				goto exit_free;
 		}
 	}
 
@@ -404,7 +505,6 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
-	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 	return 0;
-- 
1.6.5.2


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Guenter Roeck]

On Fri, Dec 17, 2010 at 12:56:15AM -0500, R, Durgadoss wrote:
> Hi,
> 
> I am submitting a patch to enable core thermal threshold
> Support to coretemp.c. There are two core thermal thresholds
> available through sysfs interfaces temp1_max and temp1_min.
> 
> The expectation is that _min is lesser than the current temperature
> and _max is higher than the current temperature. Whenever the current
> temperature crosses these limits, an interrupt is generated.
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> As per Guenter's earlier comments the ABI names are changed to
> _max and _min.
> 
> Kindly review and merge.
> ------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Thu, 16 Dec 2010 23:09:54 +0530
> Subject: [PATCH] Adding_threshold_support_to_coretemp
> 
> This patch adds the core thermal threshold support to coretemp.c.
> These thresholds can be read/written using the sysfs interface
> temp1_max and temp1_min. These can be used to generate interrupts,
> to do dynamic power management.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
> ---
Ok, trying to go to the merits.

Couple of generic comments.

First, it is customary to add comments here, after the --- line.
This ensures that the comments are not committed.

Second, it is customary to add a version log here. This helps reviewers to track
changes between versions.

>  Documentation/hwmon/coretemp     |    6 ++
>  arch/x86/include/asm/msr-index.h |   12 ++++
>  drivers/hwmon/coretemp.c         |  118 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 127 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/hwmon/coretemp b/Documentation/hwmon/coretemp
> index 25568f8..096cd8b 100644
> --- a/Documentation/hwmon/coretemp
> +++ b/Documentation/hwmon/coretemp
> @@ -29,6 +29,12 @@ the Out-Of-Spec bit. Following table summarizes the exported sysfs files:
>  
>  temp1_input	 - Core temperature (in millidegrees Celsius).
>  temp1_max	 - All cooling devices should be turned on (on Core2).
> +		   If the IA32_TEMPERATURE_TARGET is not supported, this
> +		   value indicates the higher core threshold. When the CPU
> +		   temperature crosses this temperature, an interrupt is
> +		   generated.
> +temp1_min	 - Indicates the lower threshold of the core. An interrupt is
> +		   generated when CPU temperature crosses this threshold.

What is supposed to happen if this threshold is reached ? Does it mean "it is too cold",
or does it mean "it is ok to turn off some of the cooling devices which were turned on earlier" ?

If it means "it is too cold", question is what the system is supposed to do about it.
I am not sure if such a use would be of much value.

If it means "it is no longer too hot", the atribute name should really be temp1_max_threshold.

>  temp1_crit	 - Maximum junction temperature (in millidegrees Celsius).
>  temp1_crit_alarm - Set when Out-of-spec bit is set, never clears.
>  		   Correct CPU operation is no longer guaranteed.
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 3ea3dc4..31cefad 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -253,6 +253,18 @@
>  #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
>  #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
>  
> +/* Thermal Thresholds Support */
> +#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
> +#define THERM_OFFSET_THRESHOLD0        8
> +#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
> +#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
> +#define THERM_OFFSET_THRESHOLD1        16
> +#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)

OFFSET should be named SHIFT, since that is what it is.

> +#define THERM_STATUS_THRESHOLD0        (1 << 6)
> +#define THERM_LOG_THRESHOLD0           (1 << 7)
> +#define THERM_STATUS_THRESHOLD1        (1 << 8)
> +#define THERM_LOG_THRESHOLD1           (1 << 9)
> +

You might want to merge those defines with the already existing defines 
in msr-index.h. Also, the THERM_STATUS_xxx and THERM_LOG_xxx defines are
not used anywhere. If you don't plan to use them, please remove.

On the other side, you might be able to use them for supporting temp1_max_alarm.
Please consider adding that attribute if it can be supported.

>  /* MISC_ENABLE bits: architectural */
>  #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
>  #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 42de98d..30d873e 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -39,7 +39,7 @@
>  
>  #define DRVNAME	"coretemp"
>  
> -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
> +typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_TMIN, SHOW_LABEL,
>  		SHOW_NAME } SHOW;
>  
Please remove the "typedef" here; it causes a checkpatch warning.
Also, "SHOW_xxx" is no longer appropriate since at least some of the attributes
can be written. Maybe use CORE_xxx or something else that doesn't imply "SHOW".

>  /*
> @@ -59,9 +59,11 @@ struct coretemp_data {
>  	int temp;
>  	int tjmax;
>  	int ttarget;
> +	int tmin;
>  	u8 alarm;
>  };
>  
> +static int set_core_threshold(struct coretemp_data *data, int val, int indx);

I dislike forward declarations, but there is already another one ... oh well.

>  /*
>   * Sysfs stuff
>   */
> @@ -99,17 +101,37 @@ static ssize_t show_temp(struct device *dev,
>  		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
>  	else if (attr->index = SHOW_TJMAX)
>  		err = sprintf(buf, "%d\n", data->tjmax);
> -	else
> +	else if (attr->index = SHOW_TTARGET)
>  		err = sprintf(buf, "%d\n", data->ttarget);
> +	else
> +		err = sprintf(buf, "%d\n", data->tmin);
>  	return err;
>  }
>  
> +static ssize_t store_temp(struct device *dev,
> +		struct device_attribute *devattr, const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct coretemp_data *data = coretemp_update_device(dev);
> +	unsigned long val;
> +	int err;
> +
> +	if (strict_strtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	err = set_core_threshold(data, val, attr->index);
> +
> +	return (err) ? -EINVAL : count;

Better
	if (err)
		return err;
	return count;

or 
	return err ? err : count;

No reason to drop the error value returned from set_core_threshold().

> +}
> +
>  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
>  			  SHOW_TEMP);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
>  			  SHOW_TJMAX);
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
> -			  SHOW_TTARGET);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp,
> +						store_temp, SHOW_TTARGET);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp,
> +						store_temp, SHOW_TMIN);
>  static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
>  static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
>  static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> @@ -120,6 +142,8 @@ static struct attribute *coretemp_attributes[] = {
>  	&dev_attr_temp1_crit_alarm.attr,
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
>  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
>  	NULL
>  };
>  
> @@ -298,6 +322,76 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
>  	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
>  }
>  
> +static void configure_apic(void *info)
> +{
> +	u32 l;
> +	int *flag = (int *)info;
> +
> +	l = apic_read(APIC_LVTTHMR);
> +
> +	if (*flag)	/* Non-Zero flag Masks the APIC */
> +		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
> +	else		/* Zero flag UnMasks the APIC */
> +		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
> +}
> +
> +static int set_core_threshold(struct coretemp_data *data, int temp, int thres)
> +{
> +	u32 eax, edx;
> +	int diff;
> +	int flag = 1;
> +
> +	if (temp > data->tjmax)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	diff = (data->tjmax - temp)/1000;
> +
> +	/* Mask the APIC */
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
> +
> +	if (thres = SHOW_TMIN) {
> +		eax = (eax & ~THERM_MASK_THRESHOLD0) |
> +					(diff << THERM_OFFSET_THRESHOLD0);
> +		data->tmin = temp;
> +	} else {
> +		eax = (eax & ~THERM_MASK_THRESHOLD1) |
> +					(diff << THERM_OFFSET_THRESHOLD1);
> +		data->ttarget = temp;
> +	}
> +
> +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> +
> +	/* Unmask the APIC */
> +	flag = 0;
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	mutex_unlock(&data->update_lock);
> +	return 0;
> +}
> +
> +static int __devinit enable_thresh_support(struct coretemp_data *data)
> +{
> +	u32 eax, edx;
> +	int flag = 1; /* Non-Zero Flag masks the apic */
> +
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
> +
> +	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
> +
> +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> +
If you enable the interrupt here, but there is no interrupt service routine registered for it,
what exactly will happen ?

> +	flag = 0; /*Flag should be zero to unmask the apic */
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	return 0;
> +}
> +
>  static int __devinit coretemp_probe(struct platform_device *pdev)
>  {
>  	struct coretemp_data *data;
> @@ -353,10 +447,21 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
>  	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
>  	platform_set_drvdata(pdev, data);
>  
> +	/* Enable threshold support */
> +	enable_thresh_support(data);
> +
Does this work for all Intel CPUs ? If not, you will need to check
for the CPU revision and enable the functionality only if supported.

You should only call this function _after_ the thresholds are initialized.

> +	/* Set Initial Core thresholds.
> +	 * The lower and upper threshold values here are assumed
> +	 */
> +	set_core_threshold(data, 0, SHOW_TMIN);
> +	set_core_threshold(data, 90000, SHOW_TTARGET);
> +
Those functions should only be called after the initial value of ttarget 
is known, and that value should be used to set the upper threshold.
Otherwise, the threshold is set to one value but reading temp1_max 
may return something completely different. Also, please make sure that the
upper threshold does not exceed tjmax.

>  	/*
>  	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
>  	 * on older CPUs but not in this register,
>  	 * Atoms don't have it either.
> +	 * If this register is not supported, then ttarget has the value
> +	 * of upper core threshold, set by set_core_threshold;
>  	 */
>  
>  	if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
> @@ -368,10 +473,6 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
>  		} else {
>  			data->ttarget = data->tjmax -
>  					(((eax >> 8) & 0xff) * 1000);
> -			err = device_create_file(&pdev->dev,
> -					&sensor_dev_attr_temp1_max.dev_attr);
> -			if (err)
> -				goto exit_free;
>  		}
You probably want to set ttarget to some other known value if it can not be read
from the CPU, such as to tjmax.

The error case here does not disable the interrupt. Please add.

>  	}
>  
> @@ -404,7 +505,6 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
>  
>  	hwmon_device_unregister(data->hwmon_dev);
>  	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> -	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
>  	platform_set_drvdata(pdev, NULL);
>  	kfree(data);

You should also disable the interrupt here.

>  	return 0;
> -- 
> 1.6.5.2
> 



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[R, Durgadoss]

Hi Guenter,
Thanks for the comments.
Shall fix them and re-submit the patch.

> What is supposed to happen if this threshold is reached ? Does it mean "it is
> too cold",
> or does it mean "it is ok to turn off some of the cooling devices which were
> turned on earlier" ?
> 
> If it means "it is too cold", question is what the system is supposed to do
> about it.
> I am not sure if such a use would be of much value.
> 
> If it means "it is no longer too hot", the atribute name should really be
> temp1_max_threshold.

It means "it is no longer too hot". So, shall change the name to _max_threshold.

> > +#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
> 
> OFFSET should be named SHIFT, since that is what it is.

Ok.

> You might want to merge those defines with the already existing defines
> in msr-index.h. Also, the THERM_STATUS_xxx and THERM_LOG_xxx defines are
> not used anywhere. If you don't plan to use them, please remove.
> 
> On the other side, you might be able to use them for supporting
> temp1_max_alarm.
> Please consider adding that attribute if it can be supported.

Yes. Shall check with the spec and do.

> >  #define DRVNAME	"coretemp"
> >
> > -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
> > +typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_TMIN, SHOW_LABEL,
> >  		SHOW_NAME } SHOW;
> >
> Please remove the "typedef" here; it causes a checkpatch warning.
> Also, "SHOW_xxx" is no longer appropriate since at least some of the attributes
> can be written. Maybe use CORE_xxx or something else that doesn't imply "SHOW".

The typedef was already there. So I did not remove.
No problems...I can change it in the next submission.

> > +
> > +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> > +
> If you enable the interrupt here, but there is no interrupt service routine
> registered for it,
> what exactly will happen ?

No harm...it will be just be dropped uncaught...
But in the second patch that I am sending I had a check,
that only if the interrupt handler is defined..interrupt is
Enabled...

> > +	/* Enable threshold support */
> > +	enable_thresh_support(data);
> > +
> Does this work for all Intel CPUs ? If not, you will need to check
> for the CPU revision and enable the functionality only if supported.

Yes.

> > +
> Those functions should only be called after the initial value of ttarget
> is known, and that value should be used to set the upper threshold.
> Otherwise, the threshold is set to one value but reading temp1_max
> may return something completely different. Also, please make sure that the
> upper threshold does not exceed tjmax.

Understood. I shall change it accordingly.

> > -			err = device_create_file(&pdev->dev,
> > -					&sensor_dev_attr_temp1_max.dev_attr);
> > -			if (err)
> > -				goto exit_free;
> >  		}
> You probably want to set ttarget to some other known value if it can not be
> read
> from the CPU, such as to tjmax.
> 
> The error case here does not disable the interrupt. Please add.

Got it. Thanks Guenter. I should have caught this...

Thanks,
Durga

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Fenghua Yu]

On Fri, Dec 17, 2010 at 09:15:46AM -0800, Guenter Roeck wrote:
> On Fri, Dec 17, 2010 at 12:56:15AM -0500, R, Durgadoss wrote:
> > Hi,
> > 
> > I am submitting a patch to enable core thermal threshold
> > Support to coretemp.c. There are two core thermal thresholds
> > available through sysfs interfaces temp1_max and temp1_min.
> > 
> > The expectation is that _min is lesser than the current temperature
> > and _max is higher than the current temperature. Whenever the current
> > temperature crosses these limits, an interrupt is generated.
> > 
> > This patch is generated against stable Linux-2.6 kernel.
> > 
> > As per Guenter's earlier comments the ABI names are changed to
> > _max and _min.
> > 
> > Kindly review and merge.
> > ------------------------------------------------------------------
> > From: Durgadoss R <durgadoss.r@intel.com>
> > 
> > Date: Thu, 16 Dec 2010 23:09:54 +0530
> > Subject: [PATCH] Adding_threshold_support_to_coretemp
> > 
> > This patch adds the core thermal threshold support to coretemp.c.
> > These thresholds can be read/written using the sysfs interface
> > temp1_max and temp1_min. These can be used to generate interrupts,
> > to do dynamic power management.
> > 
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > 
> > ---
> Ok, trying to go to the merits.
> 
> Couple of generic comments.
> 
> First, it is customary to add comments here, after the --- line.
> This ensures that the comments are not committed.
> 
> Second, it is customary to add a version log here. This helps reviewers to track
> changes between versions.
> 
> >  Documentation/hwmon/coretemp     |    6 ++
> >  arch/x86/include/asm/msr-index.h |   12 ++++
> >  drivers/hwmon/coretemp.c         |  118 +++++++++++++++++++++++++++++++++++---
> >  3 files changed, 127 insertions(+), 9 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/coretemp b/Documentation/hwmon/coretemp
> > index 25568f8..096cd8b 100644
> > --- a/Documentation/hwmon/coretemp
> > +++ b/Documentation/hwmon/coretemp
> > @@ -29,6 +29,12 @@ the Out-Of-Spec bit. Following table summarizes the exported sysfs files:
> >  
> >  temp1_input	 - Core temperature (in millidegrees Celsius).
> >  temp1_max	 - All cooling devices should be turned on (on Core2).
> > +		   If the IA32_TEMPERATURE_TARGET is not supported, this
> > +		   value indicates the higher core threshold. When the CPU
> > +		   temperature crosses this temperature, an interrupt is
> > +		   generated.
The temp1_max explanation is confusing. This is not what your code is doing. 
If both IA32_TEMPERATURE_TARGET and threshold are supported (most likely in
new processors), this explanation goes nowhere.

> > +temp1_min	 - Indicates the lower threshold of the core. An interrupt is
> > +		   generated when CPU temperature crosses this threshold.

Guenter and Durgadoss, I don't want to discuss this back and forth. But can we
use temp1_max_alarm and temp1_min_alarm as the threshold interfaces? Seems they
are existing interfaces and won't chang legacy temp1_max meaning in coretemp?

> 
> What is supposed to happen if this threshold is reached ? Does it mean "it is too cold",
> or does it mean "it is ok to turn off some of the cooling devices which were turned on earlier" ?
> 
> If it means "it is too cold", question is what the system is supposed to do about it.
> I am not sure if such a use would be of much value.
> 
> If it means "it is no longer too hot", the atribute name should really be temp1_max_threshold.
> 
> >  temp1_crit	 - Maximum junction temperature (in millidegrees Celsius).
> >  temp1_crit_alarm - Set when Out-of-spec bit is set, never clears.
> >  		   Correct CPU operation is no longer guaranteed.
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index 3ea3dc4..31cefad 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -253,6 +253,18 @@
> >  #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
> >  #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
> >  
> > +/* Thermal Thresholds Support */
> > +#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
> > +#define THERM_OFFSET_THRESHOLD0        8
> > +#define THERM_MASK_THRESHOLD0          (0x7f << THERM_OFFSET_THRESHOLD0)
> > +#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
> > +#define THERM_OFFSET_THRESHOLD1        16
> > +#define THERM_MASK_THRESHOLD1          (0x7f << THERM_OFFSET_THRESHOLD1)
> 
> OFFSET should be named SHIFT, since that is what it is.
> 
> > +#define THERM_STATUS_THRESHOLD0        (1 << 6)
> > +#define THERM_LOG_THRESHOLD0           (1 << 7)
> > +#define THERM_STATUS_THRESHOLD1        (1 << 8)
> > +#define THERM_LOG_THRESHOLD1           (1 << 9)
> > +
> 
> You might want to merge those defines with the already existing defines 
> in msr-index.h. Also, the THERM_STATUS_xxx and THERM_LOG_xxx defines are
> not used anywhere. If you don't plan to use them, please remove.
> 
> On the other side, you might be able to use them for supporting temp1_max_alarm.
> Please consider adding that attribute if it can be supported.
> 
> >  /* MISC_ENABLE bits: architectural */
> >  #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
> >  #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
> > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > index 42de98d..30d873e 100644
> > --- a/drivers/hwmon/coretemp.c
> > +++ b/drivers/hwmon/coretemp.c
> > @@ -39,7 +39,7 @@
> >  
> >  #define DRVNAME	"coretemp"
> >  
> > -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
> > +typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_TMIN, SHOW_LABEL,
> >  		SHOW_NAME } SHOW;
> >  
> Please remove the "typedef" here; it causes a checkpatch warning.
> Also, "SHOW_xxx" is no longer appropriate since at least some of the attributes
> can be written. Maybe use CORE_xxx or something else that doesn't imply "SHOW".
> 
> >  /*
> > @@ -59,9 +59,11 @@ struct coretemp_data {
> >  	int temp;
> >  	int tjmax;
> >  	int ttarget;
> > +	int tmin;
> >  	u8 alarm;
> >  };
> >  
> > +static int set_core_threshold(struct coretemp_data *data, int val, int indx);
> 
> I dislike forward declarations, but there is already another one ... oh well.
> 
> >  /*
> >   * Sysfs stuff
> >   */
> > @@ -99,17 +101,37 @@ static ssize_t show_temp(struct device *dev,
> >  		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
> >  	else if (attr->index = SHOW_TJMAX)
> >  		err = sprintf(buf, "%d\n", data->tjmax);
> > -	else
> > +	else if (attr->index = SHOW_TTARGET)
> >  		err = sprintf(buf, "%d\n", data->ttarget);
> > +	else
> > +		err = sprintf(buf, "%d\n", data->tmin);
> >  	return err;
> >  }
> >  
> > +static ssize_t store_temp(struct device *dev,
> > +		struct device_attribute *devattr, const char *buf, size_t count)
> > +{
> > +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +	struct coretemp_data *data = coretemp_update_device(dev);
> > +	unsigned long val;
> > +	int err;
> > +
> > +	if (strict_strtoul(buf, 10, &val))
> > +		return -EINVAL;
> > +
> > +	err = set_core_threshold(data, val, attr->index);
> > +
> > +	return (err) ? -EINVAL : count;
> 
> Better
> 	if (err)
> 		return err;
> 	return count;
> 
> or 
> 	return err ? err : count;
> 
> No reason to drop the error value returned from set_core_threshold().
> 
> > +}
> > +
> >  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
> >  			  SHOW_TEMP);
> >  static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
> >  			  SHOW_TJMAX);
> > -static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
> > -			  SHOW_TTARGET);
> > +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp,
> > +						store_temp, SHOW_TTARGET);
> > +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp,
> > +						store_temp, SHOW_TMIN);
> >  static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
> >  static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
> >  static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> > @@ -120,6 +142,8 @@ static struct attribute *coretemp_attributes[] = {
> >  	&dev_attr_temp1_crit_alarm.attr,
> >  	&sensor_dev_attr_temp1_input.dev_attr.attr,
> >  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> > +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> > +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> >  	NULL
> >  };
> >  
> > @@ -298,6 +322,76 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
> >  	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
> >  }
> >  
> > +static void configure_apic(void *info)
> > +{
> > +	u32 l;
> > +	int *flag = (int *)info;
> > +
> > +	l = apic_read(APIC_LVTTHMR);
> > +
> > +	if (*flag)	/* Non-Zero flag Masks the APIC */
> > +		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
> > +	else		/* Zero flag UnMasks the APIC */
> > +		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
> > +}
> > +
> > +static int set_core_threshold(struct coretemp_data *data, int temp, int thres)
> > +{
> > +	u32 eax, edx;
> > +	int diff;
> > +	int flag = 1;
> > +
> > +	if (temp > data->tjmax)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&data->update_lock);
> > +
> > +	diff = (data->tjmax - temp)/1000;
> > +
> > +	/* Mask the APIC */
> > +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> > +
> > +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
> > +
> > +	if (thres = SHOW_TMIN) {
> > +		eax = (eax & ~THERM_MASK_THRESHOLD0) |
> > +					(diff << THERM_OFFSET_THRESHOLD0);
> > +		data->tmin = temp;
> > +	} else {
> > +		eax = (eax & ~THERM_MASK_THRESHOLD1) |
> > +					(diff << THERM_OFFSET_THRESHOLD1);
> > +		data->ttarget = temp;
> > +	}
> > +
> > +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> > +
> > +	/* Unmask the APIC */
> > +	flag = 0;
> > +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> > +
> > +	mutex_unlock(&data->update_lock);
> > +	return 0;
> > +}
> > +
> > +static int __devinit enable_thresh_support(struct coretemp_data *data)
> > +{
> > +	u32 eax, edx;
> > +	int flag = 1; /* Non-Zero Flag masks the apic */
> > +
> > +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> > +
> > +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
> > +
> > +	eax |= (THERM_INT_THRESHOLD0_ENABLE | THERM_INT_THRESHOLD1_ENABLE);
> > +
> > +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> > +
> If you enable the interrupt here, but there is no interrupt service routine registered for it,
> what exactly will happen ?
> 
> > +	flag = 0; /*Flag should be zero to unmask the apic */
> > +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> > +
> > +	return 0;
> > +}
> > +
> >  static int __devinit coretemp_probe(struct platform_device *pdev)
> >  {
> >  	struct coretemp_data *data;
> > @@ -353,10 +447,21 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
> >  	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
> >  	platform_set_drvdata(pdev, data);
> >  
> > +	/* Enable threshold support */
> > +	enable_thresh_support(data);
> > +
> Does this work for all Intel CPUs ? If not, you will need to check
> for the CPU revision and enable the functionality only if supported.
> 
> You should only call this function _after_ the thresholds are initialized.
> 
> > +	/* Set Initial Core thresholds.
> > +	 * The lower and upper threshold values here are assumed
> > +	 */
> > +	set_core_threshold(data, 0, SHOW_TMIN);
> > +	set_core_threshold(data, 90000, SHOW_TTARGET);
> > +
> Those functions should only be called after the initial value of ttarget 
> is known, and that value should be used to set the upper threshold.
> Otherwise, the threshold is set to one value but reading temp1_max 
> may return something completely different. Also, please make sure that the
> upper threshold does not exceed tjmax.
> 
> >  	/*
> >  	 * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
> >  	 * on older CPUs but not in this register,
> >  	 * Atoms don't have it either.
> > +	 * If this register is not supported, then ttarget has the value
> > +	 * of upper core threshold, set by set_core_threshold;
> >  	 */
> >  
> >  	if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
> > @@ -368,10 +473,6 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
> >  		} else {
> >  			data->ttarget = data->tjmax -
> >  					(((eax >> 8) & 0xff) * 1000);
> > -			err = device_create_file(&pdev->dev,
> > -					&sensor_dev_attr_temp1_max.dev_attr);
> > -			if (err)
> > -				goto exit_free;
> >  		}
> You probably want to set ttarget to some other known value if it can not be read
> from the CPU, such as to tjmax.
> 
> The error case here does not disable the interrupt. Please add.
> 
> >  	}
> >  
> > @@ -404,7 +505,6 @@ static int __devexit coretemp_remove(struct platform_device *pdev)
> >  
> >  	hwmon_device_unregister(data->hwmon_dev);
> >  	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> > -	device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
> >  	platform_set_drvdata(pdev, NULL);
> >  	kfree(data);
> 
> You should also disable the interrupt here.

Maybe it does make sense to have the interrupt handling in coretemp. Put it in
therm_throt.c isn't good because this is not throttling. And seems the
threshold interrupt handling is only used for this coretemp which handles r/w
the threshold values already. The interrupt handler has close dependency on
coretemp. If coretemp is not running, the interrupt handler shouldn't run. I'm not in a position to make this judgement either. 

> 
> >  	return 0;
> > -- 
> > 1.6.5.2
> > 
> 
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Guenter Roeck]

On Fri, Dec 17, 2010 at 12:56:15AM -0500, R, Durgadoss wrote:
> Hi,
> 
> I am submitting a patch to enable core thermal threshold
> Support to coretemp.c. There are two core thermal thresholds
> available through sysfs interfaces temp1_max and temp1_min.
> 
> The expectation is that _min is lesser than the current temperature
> and _max is higher than the current temperature. Whenever the current
> temperature crosses these limits, an interrupt is generated.
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> As per Guenter's earlier comments the ABI names are changed to
> _max and _min.
> 
> Kindly review and merge.
> ------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Thu, 16 Dec 2010 23:09:54 +0530
> Subject: [PATCH] Adding_threshold_support_to_coretemp
> 
> This patch adds the core thermal threshold support to coretemp.c.
> These thresholds can be read/written using the sysfs interface
> temp1_max and temp1_min. These can be used to generate interrupts,
> to do dynamic power management.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
Another comment ... I still have concerns about the pproposed changes to
therm_throt.c and the proposed interaction with term_throt.c in your second
patch. 

When you re-submit the patches, please submit both patches together and make
sure to include the x86 maintainers and the x86 mailing list. After all, 
it doesn't make sense to integrate one part of the patch into hwmon and not
get the second part into term_throt.c.

Also, please make sure the patches compile as modules before you resubmit.

Thanks,
Guenter 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Guenter Roeck]

On Fri, Dec 17, 2010 at 01:17:09PM -0500, Fenghua Yu wrote:
[ ... ]
> > >  temp1_max   - All cooling devices should be turned on (on Core2).
> > > +              If the IA32_TEMPERATURE_TARGET is not supported, this
> > > +              value indicates the higher core threshold. When the CPU
> > > +              temperature crosses this temperature, an interrupt is
> > > +              generated.
> The temp1_max explanation is confusing. This is not what your code is doing.
> If both IA32_TEMPERATURE_TARGET and threshold are supported (most likely in
> new processors), this explanation goes nowhere.
> 
Then find a better text.

> > > +temp1_min   - Indicates the lower threshold of the core. An interrupt is
> > > +              generated when CPU temperature crosses this threshold.
> 
> Guenter and Durgadoss, I don't want to discuss this back and forth. But can we
> use temp1_max_alarm and temp1_min_alarm as the threshold interfaces? Seems they
> are existing interfaces and won't chang legacy temp1_max meaning in coretemp?
> 
Those are _alarms_, ie flags, not values.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Jean Delvare]

On Fri, 17 Dec 2010 23:10:17 +0530, R, Durgadoss wrote:
> > What is supposed to happen if this threshold is reached ? Does it mean "it is
> > too cold",
> > or does it mean "it is ok to turn off some of the cooling devices which were
> > turned on earlier" ?
> > 
> > If it means "it is too cold", question is what the system is supposed to do
> > about it.
> > I am not sure if such a use would be of much value.
> > 
> > If it means "it is no longer too hot", the atribute name should really be
> > temp1_max_threshold.
> 
> It means "it is no longer too hot". So, shall change the name to _max_threshold.

I'm fairly certain Guenter actually meant temp1_max_hyst, per
Documentation/hwmon/sysfs-interface.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Guenter Roeck]

On Fri, Dec 17, 2010 at 02:59:34PM -0500, Guenter Roeck wrote:
> On Fri, Dec 17, 2010 at 01:17:09PM -0500, Fenghua Yu wrote:
> [ ... ]
> > > >  temp1_max   - All cooling devices should be turned on (on Core2).
> > > > +              If the IA32_TEMPERATURE_TARGET is not supported, this
> > > > +              value indicates the higher core threshold. When the CPU
> > > > +              temperature crosses this temperature, an interrupt is
> > > > +              generated.
> > The temp1_max explanation is confusing. This is not what your code is doing.
> > If both IA32_TEMPERATURE_TARGET and threshold are supported (most likely in
> > new processors), this explanation goes nowhere.
> > 
> Then find a better text.
> 
... such as:

temp1_max	Temperature at which all cooling devices should be turned on.
		Initialized with IA32_TEMPERATURE_TARGET if supported, otherwise
		initialized with (tjmax - <pick something>).
		When the CPU temperature reaches this temperature, an interrupt is
		generated and temp1_max_alarm is set.

temp1_max_hyst	If the CPU temperature falls below than this temperature, an interrupt
		is generated and temp1_max_alarm is reset.

temp1_max_alarm	Set if the temperature reaches or exceeds temp1_max. Reset if the temperature 
		drops to or below temp1_max_hyst.

This would extend the semantics of temp1_max, but not redefine it. Also, temp1_max_alarm
could conveniently be implemented such that it supports polling, and be used by an application 
with poll() or select().

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch]Adding_threshold_support_to_coretemp

[Guenter Roeck]

On Fri, Dec 17, 2010 at 04:56:39PM -0500, Jean Delvare wrote:
> On Fri, 17 Dec 2010 23:10:17 +0530, R, Durgadoss wrote:
> > > What is supposed to happen if this threshold is reached ? Does it mean "it is
> > > too cold",
> > > or does it mean "it is ok to turn off some of the cooling devices which were
> > > turned on earlier" ?
> > > 
> > > If it means "it is too cold", question is what the system is supposed to do
> > > about it.
> > > I am not sure if such a use would be of much value.
> > > 
> > > If it means "it is no longer too hot", the atribute name should really be
> > > temp1_max_threshold.
> > 
> > It means "it is no longer too hot". So, shall change the name to _max_threshold.
> 
> I'm fairly certain Guenter actually meant temp1_max_hyst, per
> Documentation/hwmon/sysfs-interface.
> 
Definitely yes ... I must have been low on coffee or something :(.
Thanks for correcting me.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors