hwmon: Add driver for AMD family 15h processor

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

This driver provides

* power1_cap (ProcessorPwrWatts)

* power1_input (CurrPwrWatts)

Changes from v1::
- removed deprecated code line,
- fixed comment,
- report power only once per socket (Power information is provided for the entire package. On multi-node processors it should only be reported on internal node 0.)

Signed-off-by Andreas Herrmann <[email protected]>

---
 drivers/hwmon/Kconfig      |   10 ++
 drivers/hwmon/Makefile     |    1 +
 drivers/hwmon/f15h_power.c |  216 ++++++++++++++++++++++++++++++++++++++++++++

 3 files changed, 227 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/f15h_power.c

drivers/hwmon/Kconfig +0 -0

index 8e84b31..0268623 100644
@@ -249,6 +249,16 @@ config SENSORS_K10TEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called k10temp.

drivers/hwmon/Kconfig +1 -0

+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on X86 && I2C && EXPERIMENTAL

drivers/hwmon/Makefile +1 -0

index dde02d9..e554b88 100644
@@ -62,6 +62,7 @@ obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
 obj-$(CONFIG_SENSORS_JZ4740)	+= jz4740-hwmon.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
+obj-$(CONFIG_SENSORS_F15H_POWER) += f15h_power.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o

drivers/hwmon/f15h_power.c new file +50 -0

new file mode 100644
index 0000000..40e3cbd
@@ -0,0 +1,216 @@
+/*
+ * f15h_power.c - AMD Family 15h processor power monitoring
+ *
+ * Copyright (c) 2011 Advanced Micro Devices, Inc.
+ * Author: Andreas Herrmann <[email protected]>
+ *
+ *
+ * This driver is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This driver is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this driver; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/bitops.h>
+#include <asm/processor.h>
+
+MODULE_DESCRIPTION("AMD Family 15h CPU processor power monitor");
+MODULE_AUTHOR("Andreas Herrmann <[email protected]>");
+MODULE_LICENSE("GPL");
+
+/* D18F3 */
+#define REG_NORTHBRIDGE_CAP		0xe8
+
+/* D18F4 */
+#define REG_PROCESSOR_TDP		0x1b8
+
+/* D18F5 */
+#define REG_TDP_RUNNING_AVERAGE		0xe0
+#define REG_TDP_LIMIT3			0xe8
+
+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, btdp, tdpl, tdp2w, arange;
+	s32 acap;
+	u64 ctdp;
+	struct pci_dev *f4 = to_pci_dev(dev);

drivers/hwmon/f15h_power.c new file +13 -0

+
+	pci_read_config_dword(f5, REG_TDP_RUNNING_AVERAGE, &val);
+	acap = (val >> 4) & 0x3fffff;
+	acap = sign_extend32(acap, 22);
+	arange = val & 0xf;
+
+	pci_read_config_dword(f5, REG_TDP_LIMIT3, &val);
+	pci_dev_put(f5);
+
+	tdpl = (val >> 16) & 0x1fff;
+	tdp2w = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	ctdp = tdpl - (s32)(acap >> (arange + 1)) + btdp;
+	ctdp *= tdp2w;

drivers/hwmon/f15h_power.c new file +10 -0

+}
+static DEVICE_ATTR(power1_input, S_IRUGO, show_power, NULL);
+
+static ssize_t show_power_cap(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	u32 val, tdp2w;
+	u64 ptdp;
+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct pci_dev *f5;

drivers/hwmon/f15h_power.c new file +9 -0

+}
+static DEVICE_ATTR(power1_cap, S_IRUGO, show_power_cap, NULL);
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "f15h_power\n");
+}
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);

drivers/hwmon/f15h_power.c new file +48 -0

+	}
+
+	pci_read_config_dword(f3, REG_NORTHBRIDGE_CAP, &val);
+	pci_dev_put(f3);
+
+	if ((val & BIT(29)) && ((val >> 30) & 3))
+		return 0;
+
+	return 1;
+}
+
+static int __devinit f15h_power_probe(struct pci_dev *pdev,
+				      const struct pci_device_id *id)
+{
+	struct device *hwmon_dev;
+	int err = -ENODEV;
+
+	if (!f15h_power_is_internal_node0(pdev))
+		goto exit;
+
+	err = device_create_file(&pdev->dev, &dev_attr_power1_input);
+	if (err)
+		goto exit;
+	err = device_create_file(&pdev->dev, &dev_attr_power1_cap);
+	if (err)
+		goto exit_remove;
+
+	err = device_create_file(&pdev->dev, &dev_attr_name);
+	if (err)
+		goto exit_remove;
+
+	hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(hwmon_dev)) {
+		err = PTR_ERR(hwmon_dev);
+		goto exit_remove;
+	}
+	dev_set_drvdata(&pdev->dev, hwmon_dev);
+
+	return 0;
+
+exit_remove:
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	device_remove_file(&pdev->dev, &dev_attr_power1_input);
+	device_remove_file(&pdev->dev, &dev_attr_power1_cap);
+exit:
+	return err;
+}
+

drivers/hwmon/f15h_power.c new file +24 -0

+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_LINK) },
+	{}
+};
+MODULE_DEVICE_TABLE(pci, f15h_power_id_table);
+
+static struct pci_driver f15h_power_driver = {
+	.name = "f15h_power",
+	.id_table = f15h_power_id_table,
+	.probe = f15h_power_probe,
+	.remove = __devexit_p(f15h_power_remove),
+};
+
+static int __init f15h_power_init(void)
+{
+	return pci_register_driver(&f15h_power_driver);
+}
+
+static void __exit f15h_power_exit(void)
+{
+	pci_unregister_driver(&f15h_power_driver);
+}
+
+module_init(f15h_power_init)
+module_exit(f15h_power_exit)

Signature · 1 line

1.7.3.1

drivers/hwmon/f15h_power.c line 134

Quoted 9 lines · Andreas Herrmann

drivers/hwmon/f15h_power.c new file +9 -0

+static int __devinit f15h_power_is_internal_node0(struct pci_dev *f4)
+{
+	u32 val;
+	struct pci_dev *f3;
+
+	f3 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3));
+	if (!f3) {
+		dev_err(&f4->dev, "no function 3 available on this slot\n");
+		return 0;

It is a common practice to return a negative value on errors. Why not here ?

Also, is this really an error which asks for an error message, or just a CPU or system which does not support the attribute ?

drivers/hwmon/f15h_power.c line 138

Quoted 2 lines

drivers/hwmon/f15h_power.c new file +2 -0

+	pci_read_config_dword(f3, REG_NORTHBRIDGE_CAP, &val);
+	pci_dev_put(f3);

Quoted 12 lines · Guenter Roeck

drivers/hwmon/f15h_power.c line 134

Quoted 9 lines · Andreas Herrmann

drivers/hwmon/f15h_power.c new file +9 -0

+static int __devinit f15h_power_is_internal_node0(struct pci_dev *f4)
+{
+	u32 val;
+	struct pci_dev *f3;
+
+	f3 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3));
+	if (!f3) {
+		dev_err(&f4->dev, "no function 3 available on this slot\n");
+		return 0;

It is a common practice to return a negative value on errors. Why not here ?

Apparently, this function returns a boolean value. Using "bool"/"true"/ "false" would have made this more obvious.

Quoted 2 lines

Also, is this really an error which asks for an error message, or just a CPU or system which does not support the attribute ?

AFAICT all F15h CPUs are _known_ to have all these PCI functions; the error should never occur in practice.

What I do in the k10temp driver in this situation is to trust the CPU to be there, omitting the pci_get_slot/pci_dev_put and just replacing this:

Quoted 2 lines

drivers/hwmon/f15h_power.c line 138

Quoted 2 lines

drivers/hwmon/f15h_power.c new file +2 -0

+	pci_read_config_dword(f3, REG_NORTHBRIDGE_CAP, &val);
+	pci_dev_put(f3);

with the equivalent of:

pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3),

                            REG_NORTHBRIDGE_CAP, &val);

BTW: The family 15h CPUs have the same temperature sensor registers (D18F3xA4 and D18F3x64) as the earlier families, haven't they?

Quoted 10 lines · Andreas Herrmann

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

Quoted 3 lines

This driver provides

* power1_cap (ProcessorPwrWatts)

Hi Andreas,

What does the CPU do if ProcessorPwrWatts is reached ? Does it start to limit power consumption, ie does it enforce the limit ?

If not you might want to use power1_max instead.

Quoted 9 lines

* power1_input (CurrPwrWatts)

Changes from v1::
- removed deprecated code line,
- fixed comment,
- report power only once per socket (Power information is provided for the entire package. On multi-node processors it should only be reported on internal node 0.)

Quoted 5 lines

Signed-off-by Andreas Herrmann <[email protected]>

---
 drivers/hwmon/Kconfig      |   10 ++
 drivers/hwmon/Makefile     |    1 +
 drivers/hwmon/f15h_power.c |  216 ++++++++++++++++++++++++++++++++++++++++++++

Please also add Documentation/hwmon/f15h_power.

Quoted 11 lines

 3 files changed, 227 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/f15h_power.c

drivers/hwmon/Kconfig +0 -0

index 8e84b31..0268623 100644
@@ -249,6 +249,16 @@ config SENSORS_K10TEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called k10temp.
 

drivers/hwmon/Kconfig config SENSORS_K10TEMP · line 260

Quoted 9 lines

drivers/hwmon/Kconfig +9 -0

+config SENSORS_F15H_POWER
+	tristate "AMD Family 15h processor power"
+	depends on X86 && PCI
+	help
+	  If you say yes here you get support for processor power
+	  information of your AMD family 15h CPU.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called f15hpower.

	f15h_power

Quoted 72 lines

drivers/hwmon/Kconfig +1 -0

+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on X86 && I2C && EXPERIMENTAL

drivers/hwmon/Makefile +1 -0

index dde02d9..e554b88 100644
@@ -62,6 +62,7 @@ obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
 obj-$(CONFIG_SENSORS_JZ4740)	+= jz4740-hwmon.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
+obj-$(CONFIG_SENSORS_F15H_POWER) += f15h_power.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o

drivers/hwmon/f15h_power.c new file +50 -0

new file mode 100644
index 0000000..40e3cbd
@@ -0,0 +1,216 @@
+/*
+ * f15h_power.c - AMD Family 15h processor power monitoring
+ *
+ * Copyright (c) 2011 Advanced Micro Devices, Inc.
+ * Author: Andreas Herrmann <[email protected]>
+ *
+ *
+ * This driver is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This driver is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this driver; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/bitops.h>
+#include <asm/processor.h>
+
+MODULE_DESCRIPTION("AMD Family 15h CPU processor power monitor");
+MODULE_AUTHOR("Andreas Herrmann <[email protected]>");
+MODULE_LICENSE("GPL");
+
+/* D18F3 */
+#define REG_NORTHBRIDGE_CAP		0xe8
+
+/* D18F4 */
+#define REG_PROCESSOR_TDP		0x1b8
+
+/* D18F5 */
+#define REG_TDP_RUNNING_AVERAGE		0xe0
+#define REG_TDP_LIMIT3			0xe8
+
+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, btdp, tdpl, tdp2w, arange;
+	s32 acap;
+	u64 ctdp;
+	struct pci_dev *f4 = to_pci_dev(dev);

drivers/hwmon/f15h_power.c line 60

Quoted 10 lines

drivers/hwmon/f15h_power.c new file +10 -0

+	struct pci_dev *f5;
+
+	pci_read_config_dword(f4, REG_PROCESSOR_TDP, &val);
+	btdp = (val >> 16) & 0xffff;
+
+	f5 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5));
+	if (!f5) {
+		dev_err(dev, "no function 5 available on this slot\n");
+		return 0;
+	}

Unless this is a temporary error condition, you should check if this function exists in the probe function, and not create a sysfs entry if it doesn't.

If it is a temporary error which needs a runtime check, you should return a negative value to indicate that there was an error.

Quoted 16 lines

drivers/hwmon/f15h_power.c new file +16 -0

+	pci_read_config_dword(f5, REG_TDP_RUNNING_AVERAGE, &val);
+	acap = (val >> 4) & 0x3fffff;
+	acap = sign_extend32(acap, 22);
+	arange = val & 0xf;
+
+	pci_read_config_dword(f5, REG_TDP_LIMIT3, &val);
+	pci_dev_put(f5);
+
+	tdpl = (val >> 16) & 0x1fff;
+	tdp2w = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	ctdp = tdpl - (s32)(acap >> (arange + 1)) + btdp;
+	ctdp *= tdp2w;
+
+	/*
+	 * Convert to microWatt
+	 *

drivers/hwmon/f15h_power.c line 83

Quoted 6 lines

drivers/hwmon/f15h_power.c new file +6 -0

+	 * power is in Watt provided as fixed point integer with
+	 * scaling factor 1/(2^16).  For conversion we use
+	 * (10^6)/(2^16) = 15625/(2^10)
+	 */
+	ctdp = (ctdp * 15625) >> 10;
+	return sprintf(buf, "%d\n", (u32) ctdp);

	%u

Quoted 10 lines

drivers/hwmon/f15h_power.c new file +10 -0

+}
+static DEVICE_ATTR(power1_input, S_IRUGO, show_power, NULL);
+
+static ssize_t show_power_cap(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	u32 val, tdp2w;
+	u64 ptdp;
+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct pci_dev *f5;

drivers/hwmon/f15h_power.c line 103

Quoted 10 lines

drivers/hwmon/f15h_power.c new file +10 -0

+
+	pci_read_config_dword(f4, REG_PROCESSOR_TDP, &val);
+	ptdp = val & 0xffff;
+
+	f5 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5));
+	if (!f5) {
+		dev_err(dev, "no function 5 available on this slot\n");
+		return 0;
+	}
+

Same as above - should be checked in probe function.

If it is a temporary error which needs a runtime check, you should return a negative value to indicate that there was an error.

drivers/hwmon/f15h_power.c line 112

Quoted 9 lines

drivers/hwmon/f15h_power.c new file +9 -0

+	pci_read_config_dword(f5, REG_TDP_LIMIT3, &val);
+	pci_dev_put(f5);
+
+	tdp2w = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	ptdp *= tdp2w;
+
+	/* result not allowed to be >= 256W */
+	WARN_ON(ptdp>>16 >= 256);
+

Does this really ask for such drastic measures, or would dev_warn() be sufficient ?

drivers/hwmon/f15h_power.c line 115

Quoted 3 lines

drivers/hwmon/f15h_power.c new file +3 -0

+	/* convert to microWatt */
+	ptdp = (ptdp * 15625) >> 10;
+	return sprintf(buf, "%d\n", (u32) ptdp);

	%u

Quoted 16 lines

drivers/hwmon/f15h_power.c new file +16 -0

+}
+static DEVICE_ATTR(power1_cap, S_IRUGO, show_power_cap, NULL);
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "f15h_power\n");
+}
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static int __devinit f15h_power_is_internal_node0(struct pci_dev *f4)
+{
+	u32 val;
+	struct pci_dev *f3;
+
+	f3 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3));

drivers/hwmon/f15h_power.c line 134

Quoted 3 lines

drivers/hwmon/f15h_power.c new file +3 -0

+	if (!f3) {
+		dev_err(&f4->dev, "no function 3 available on this slot\n");
+		return 0;

It is a common practice to return a negative value on errors. Why not here ? Also, is this really an error which asks for an error message, or just a CPU or system which does not support the attribute ? In the latter case, you should not display an error message.

Quoted 48 lines

drivers/hwmon/f15h_power.c new file +48 -0

+	}
+
+	pci_read_config_dword(f3, REG_NORTHBRIDGE_CAP, &val);
+	pci_dev_put(f3);
+
+	if ((val & BIT(29)) && ((val >> 30) & 3))
+		return 0;
+
+	return 1;
+}
+
+static int __devinit f15h_power_probe(struct pci_dev *pdev,
+				      const struct pci_device_id *id)
+{
+	struct device *hwmon_dev;
+	int err = -ENODEV;
+
+	if (!f15h_power_is_internal_node0(pdev))
+		goto exit;
+
+	err = device_create_file(&pdev->dev, &dev_attr_power1_input);
+	if (err)
+		goto exit;
+	err = device_create_file(&pdev->dev, &dev_attr_power1_cap);
+	if (err)
+		goto exit_remove;
+
+	err = device_create_file(&pdev->dev, &dev_attr_name);
+	if (err)
+		goto exit_remove;
+
+	hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(hwmon_dev)) {
+		err = PTR_ERR(hwmon_dev);
+		goto exit_remove;
+	}
+	dev_set_drvdata(&pdev->dev, hwmon_dev);
+
+	return 0;
+
+exit_remove:
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	device_remove_file(&pdev->dev, &dev_attr_power1_input);
+	device_remove_file(&pdev->dev, &dev_attr_power1_cap);
+exit:
+	return err;
+}
+

drivers/hwmon/f15h_power.c line 192

Quoted 10 lines

drivers/hwmon/f15h_power.c new file +10 -0

+static void __devexit f15h_power_remove(struct pci_dev *pdev)
+{
+	hwmon_device_unregister(dev_get_drvdata(&pdev->dev));
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	device_remove_file(&pdev->dev, &dev_attr_power1_input);
+	device_remove_file(&pdev->dev, &dev_attr_power1_cap);
+	dev_set_drvdata(&pdev->dev, NULL);
+}
+
+static const struct pci_device_id f15h_power_id_table[] = {

checkpatch says:

WARNING: Use DEFINE_PCI_DEVICE_TABLE for struct pci_device_id #326: FILE: drivers/hwmon/f15h_power.c:192:

Please fix.

Quoted 16 lines

drivers/hwmon/f15h_power.c new file +16 -0

+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_LINK) },
+	{}
+};
+MODULE_DEVICE_TABLE(pci, f15h_power_id_table);
+
+static struct pci_driver f15h_power_driver = {
+	.name = "f15h_power",
+	.id_table = f15h_power_id_table,
+	.probe = f15h_power_probe,
+	.remove = __devexit_p(f15h_power_remove),
+};
+
+static int __init f15h_power_init(void)
+{
+	return pci_register_driver(&f15h_power_driver);
+}

drivers/hwmon/f15h_power.c line 216

Quoted 8 lines

drivers/hwmon/f15h_power.c new file +8 -0

+
+static void __exit f15h_power_exit(void)
+{
+	pci_unregister_driver(&f15h_power_driver);
+}
+
+module_init(f15h_power_init)
+module_exit(f15h_power_exit)

Documentation/hwmon/f15h_power new file +3 -0

@@ -0,0 +1,37 @@
+Kernel driver f15h_power
+============
+

Hi Andreas,

Quoted 10 lines · Andreas Herrmann

Documentation/hwmon/f15h_power new file +7 -0

@@ -0,0 +1,37 @@
+Kernel driver f15h_power
+============
+
+Supported chips:
+* AMD Family 15h Processors
+
+  Prefix: 'f15h_power'

Documentation/hwmon/f15h_power line 13

Quoted 6 lines

Documentation/hwmon/f15h_power new file +6 -0

+  Addresses scanned: PCI space
+  Datasheets:
+  BIOS and Kernel Developer's Guide (BKDG) For AMD Family 15h Processors
+    (not yet published)
+
+Author: Andreas Herrmann <[email protected]>

BTW, please consider adding an entry in MAINTAINERS.

Documentation/hwmon/f15h_power line 22

Quoted 9 lines

Documentation/hwmon/f15h_power new file +9 -0

+
+Description
+-----------
+
+This driver permits reading of registers providing power information
+of AMD Family 15h processors.
+
+For AMD Family 15h processors the following power values can be
+calculated using different processor northbridge function registers

A trailing ":" would be nice.

Documentation/hwmon/f15h_power line 32

Quoted 10 lines

Documentation/hwmon/f15h_power new file +10 -0

+
+* BasePwrWatts: Specifies in watts the maximum amount of power
+  consumed by the processor for NB and logic external to the core.
+* ProcessorPwrWatts: Specifies in watts the maximum amount of power
+  the processor can support.
+* CurrPwrWatts: Specifies in watts the current amount of power being
+  consumed by the processor.
+
+This driver provides ProcessorPwrWatts and CurrPwrWatts:
+* power1_max (ProcessorPwrWatts)

I see you changed this name once already, but... What is expected to happen if the power consumed exceeds this limit? If you expect the CPU to get damaged, then power1_crit would be more appropriate.

Another way to decide if _max or _crit is more appropriate is by answering the question: if a second limit was added in the future, would it more likely be below or above this one?

Documentation/hwmon/f15h_power

Quoted 8 lines

Documentation/hwmon/f15h_power new file +6 -0

+* power1_input (CurrPwrWatts)
+
+On multi-node processors the calculated value is for the entire
+package and not for a single node. Thus the driver creates sysfs
+attributes only for internal node0 of a multi-node processor.
 
(...)
+	return sprintf(buf, "%u\n", (u32) ptdp);

Maybe I'm just nitpicking, but I still don't think it's correct. %u wants unsigned int, not u32. It might be the same but that's by luck.

Quoted 6 lines

drivers/hwmon/k10temp.c +0 -0

@@ -205,7 +205,7 @@ static void __devexit k10temp_remove(struct pci_dev *pdev)
 	dev_set_drvdata(&pdev->dev, NULL);
 }
 

drivers/hwmon/k10temp.c static void __devexit k10temp_remove(struct pci_dev *pdev) · line 211

Quoted 5 lines

drivers/hwmon/k10temp.c +1 -1

-static const struct pci_device_id k10temp_id_table[] = {
+static DEFINE_PCI_DEVICE_TABLE(k10temp_id_table) = {
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_11H_NB_MISC) },
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_CNB17H_F3) },

Such cleanups should go to separate patches, as they don't have anything to do with your initial effort. And that way you can fix k8temp i5k_amb too.

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

This driver provides

* power1_max (ProcessorPwrWatts)

* power1_input (CurrPwrWatts)

Changes from v2:
- fix format strings
- removed paranoid checks for existense of functions 3 and 5
- changed return type of function f15h_power_is_internal_node0
- use power1_max instead of power1_cap
- use dev_warn instead of WARN_ON
- rebased against 2.6.39-rc2
- added Documentation/hwmon/f15h_power

Changes from v3:
- read static power information only once (during driver initialization)
- made use of attribute groups
- renamed f15h_power to fam15h_power

Signed-off-by Andreas Herrmann <[email protected]>

---
 Documentation/hwmon/fam15h_power |   37 ++++++
 drivers/hwmon/Kconfig            |   10 ++
 drivers/hwmon/Makefile           |    1 +
 drivers/hwmon/fam15h_power.c     |  229 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 277 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/hwmon/fam15h_power
 create mode 100644 drivers/hwmon/fam15h_power.c

At this stage I consider the _max attribute as the right one for reporting ProcessorPwrWatts.

Hopefully I've addressed all your coments.
Please apply.

Documentation/hwmon/fam15h_power new file +37 -0

new file mode 100644
index 0000000..2f4d291
@@ -0,0 +1,37 @@
+Kernel driver fam15h_power
+=============
+
+Supported chips:
+* AMD Family 15h Processors
+
+  Prefix: 'fam15h_power'
+  Addresses scanned: PCI space
+  Datasheets:
+  BIOS and Kernel Developer's Guide (BKDG) For AMD Family 15h Processors
+    (not yet published)
+
+Author: Andreas Herrmann <[email protected]>
+
+Description
+-----------
+
+This driver permits reading of registers providing power information
+of AMD Family 15h processors.
+
+For AMD Family 15h processors the following power values can be
+calculated using different processor northbridge function registers:
+
+* BasePwrWatts: Specifies in watts the maximum amount of power
+  consumed by the processor for NB and logic external to the core.
+* ProcessorPwrWatts: Specifies in watts the maximum amount of power
+  the processor can support.
+* CurrPwrWatts: Specifies in watts the current amount of power being
+  consumed by the processor.
+
+This driver provides ProcessorPwrWatts and CurrPwrWatts:
+* power1_max (ProcessorPwrWatts)
+* power1_input (CurrPwrWatts)
+
+On multi-node processors the calculated value is for the entire
+package and not for a single node. Thus the driver creates sysfs
+attributes only for internal node0 of a multi-node processor.

drivers/hwmon/Kconfig +10 -0

index 060ef63..fb3e334 100644
@@ -249,6 +249,16 @@ config SENSORS_K10TEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called k10temp.
 
+config SENSORS_FAM15H_POWER
+	tristate "AMD Family 15h processor power"
+	depends on X86 && PCI
+	help
+	  If you say yes here you get support for processor power
+	  information of your AMD family 15h CPU.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called fam15h_power.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on X86 && I2C && EXPERIMENTAL

drivers/hwmon/Makefile +1 -0

index 967d0ea..236d3f9 100644
@@ -49,6 +49,7 @@ obj-$(CONFIG_SENSORS_EMC2103)	+= emc2103.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
+obj-$(CONFIG_SENSORS_FAM15H_POWER) += fam15h_power.o
 obj-$(CONFIG_SENSORS_FSCHMD)	+= fschmd.o
 obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o

drivers/hwmon/fam15h_power.c new file +38 -0

new file mode 100644
index 0000000..cb6eb99
@@ -0,0 +1,229 @@
+/*
+ * fam15h_power.c - AMD Family 15h processor power monitoring
+ *
+ * Copyright (c) 2011 Advanced Micro Devices, Inc.
+ * Author: Andreas Herrmann <[email protected]>
+ *
+ *
+ * This driver is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This driver is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this driver; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/bitops.h>
+#include <asm/processor.h>
+
+MODULE_DESCRIPTION("AMD Family 15h CPU processor power monitor");
+MODULE_AUTHOR("Andreas Herrmann <[email protected]>");
+MODULE_LICENSE("GPL");
+
+/* D18F3 */
+#define REG_NORTHBRIDGE_CAP		0xe8
+
+/* D18F4 */
+#define REG_PROCESSOR_TDP		0x1b8

drivers/hwmon/fam15h_power.c new file +10 -0

+		(s32)(running_avg_capture >> (running_avg_range + 1));
+	curr_pwr_watts *= data->tdp_to_watt;
+
+	/*
+	 * Convert to microWatt
+	 *
+	 * power is in Watt provided as fixed point integer with
+	 * scaling factor 1/(2^16).  For conversion we use
+	 * (10^6)/(2^16) = 15625/(2^10)
+	 */

drivers/hwmon/fam15h_power.c new file +9 -0

+	return sprintf(buf, "%u\n", data->processor_pwr_watts);
+}
+static DEVICE_ATTR(power1_max, S_IRUGO, show_power_max, NULL);
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "fam15h_power\n");
+}

drivers/hwmon/fam15h_power.c new file +23 -0

+	.attrs	= fam15h_power_attrs,
+};
+
+static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
+{
+	u32 val;
+
+	pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3),
+				  REG_NORTHBRIDGE_CAP, &val);
+	if ((val & BIT(29)) && ((val >> 30) & 3))
+		return false;
+
+	return true;
+}
+
+static void __devinit fam15h_power_init_data(struct pci_dev *f4,
+					     struct fam15h_power_data *data)
+{
+	u32 val;
+	u64 tmp;
+
+	pci_read_config_dword(f4, REG_PROCESSOR_TDP, &val);
+	data->base_tdp = val >> 16;

drivers/hwmon/fam15h_power.c new file +28 -0

+		dev_warn(&f4->dev, "Bogus value for ProcessorPwrWatts "
+			 "(processor_pwr_watts>=%u)\n",
+			 (unsigned int) (tmp >> 16));
+
+	/* convert to microWatt */
+	data->processor_pwr_watts = (tmp * 15625) >> 10;
+}
+
+static int __devinit fam15h_power_probe(struct pci_dev *pdev,
+					const struct pci_device_id *id)
+{
+	struct fam15h_power_data *data;
+	struct device *dev;
+	int err;
+
+	if (!fam15h_power_is_internal_node0(pdev)) {
+		err = -ENODEV;
+		goto exit;
+	}
+
+	data = kzalloc(sizeof(struct fam15h_power_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	fam15h_power_init_data(pdev, data);
+
+	dev = &pdev->dev;

drivers/hwmon/fam15h_power.c new file +38 -0

+
+	return 0;
+
+exit_remove_group:
+	sysfs_remove_group(&dev->kobj, &fam15h_power_attr_group);
+exit_free_data:
+	kfree(data);
+exit:
+	return err;
+}
+
+static void __devexit fam15h_power_remove(struct pci_dev *pdev)
+{
+	struct device *dev;
+	struct fam15h_power_data *data = pci_get_drvdata(pdev);
+
+	dev = &pdev->dev;
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&dev->kobj, &fam15h_power_attr_group);
+	pci_set_drvdata(pdev, NULL);
+	kfree(data);
+}
+
+static DEFINE_PCI_DEVICE_TABLE(fam15h_power_id_table) = {
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+	{}
+};
+MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
+
+static struct pci_driver fam15h_power_driver = {
+	.name = "fam15h_power",
+	.id_table = fam15h_power_id_table,
+	.probe = fam15h_power_probe,
+	.remove = __devexit_p(fam15h_power_remove),
+};
+
+static int __init fam15h_power_init(void)
+{

Hi Andreas,

Quoted 10 lines · Andreas Herrmann

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

Quoted 3 lines

This driver provides

* power1_max (ProcessorPwrWatts)

Still questionable whether power1_crit would be more appropriate...

Quoted 26 lines

* power1_input (CurrPwrWatts)

Changes from v2:
- fix format strings
- removed paranoid checks for existense of functions 3 and 5
- changed return type of function f15h_power_is_internal_node0
- use power1_max instead of power1_cap
- use dev_warn instead of WARN_ON
- rebased against 2.6.39-rc2
- added Documentation/hwmon/f15h_power

Changes from v3:
- read static power information only once (during driver initialization)
- made use of attribute groups
- renamed f15h_power to fam15h_power

Signed-off-by Andreas Herrmann <[email protected]>

---
 Documentation/hwmon/fam15h_power |   37 ++++++
 drivers/hwmon/Kconfig            |   10 ++
 drivers/hwmon/Makefile           |    1 +
 drivers/hwmon/fam15h_power.c     |  229 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 277 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/hwmon/fam15h_power
 create mode 100644 drivers/hwmon/fam15h_power.c

Quoted 5 lines

At this stage I consider the _max attribute as the right one for reporting ProcessorPwrWatts.

Hopefully I've addressed all your coments.
Please apply.

It seems I have some more comments, but these are very small things you should have no problem addressing quickly:

Quoted 124 lines

Documentation/hwmon/fam15h_power new file +37 -0

new file mode 100644
index 0000000..2f4d291
@@ -0,0 +1,37 @@
+Kernel driver fam15h_power
+=============
+
+Supported chips:
+* AMD Family 15h Processors
+
+  Prefix: 'fam15h_power'
+  Addresses scanned: PCI space
+  Datasheets:
+  BIOS and Kernel Developer's Guide (BKDG) For AMD Family 15h Processors
+    (not yet published)
+
+Author: Andreas Herrmann <[email protected]>
+
+Description
+-----------
+
+This driver permits reading of registers providing power information
+of AMD Family 15h processors.
+
+For AMD Family 15h processors the following power values can be
+calculated using different processor northbridge function registers:
+
+* BasePwrWatts: Specifies in watts the maximum amount of power
+  consumed by the processor for NB and logic external to the core.
+* ProcessorPwrWatts: Specifies in watts the maximum amount of power
+  the processor can support.
+* CurrPwrWatts: Specifies in watts the current amount of power being
+  consumed by the processor.
+
+This driver provides ProcessorPwrWatts and CurrPwrWatts:
+* power1_max (ProcessorPwrWatts)
+* power1_input (CurrPwrWatts)
+
+On multi-node processors the calculated value is for the entire
+package and not for a single node. Thus the driver creates sysfs
+attributes only for internal node0 of a multi-node processor.

drivers/hwmon/Kconfig +10 -0

index 060ef63..fb3e334 100644
@@ -249,6 +249,16 @@ config SENSORS_K10TEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called k10temp.
 
+config SENSORS_FAM15H_POWER
+	tristate "AMD Family 15h processor power"
+	depends on X86 && PCI
+	help
+	  If you say yes here you get support for processor power
+	  information of your AMD family 15h CPU.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called fam15h_power.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on X86 && I2C && EXPERIMENTAL

drivers/hwmon/Makefile +1 -0

index 967d0ea..236d3f9 100644
@@ -49,6 +49,7 @@ obj-$(CONFIG_SENSORS_EMC2103)	+= emc2103.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
+obj-$(CONFIG_SENSORS_FAM15H_POWER) += fam15h_power.o
 obj-$(CONFIG_SENSORS_FSCHMD)	+= fschmd.o
 obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o

drivers/hwmon/fam15h_power.c new file +42 -0

new file mode 100644
index 0000000..cb6eb99
@@ -0,0 +1,229 @@
+/*
+ * fam15h_power.c - AMD Family 15h processor power monitoring
+ *
+ * Copyright (c) 2011 Advanced Micro Devices, Inc.
+ * Author: Andreas Herrmann <[email protected]>
+ *
+ *
+ * This driver is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This driver is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this driver; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/bitops.h>
+#include <asm/processor.h>
+
+MODULE_DESCRIPTION("AMD Family 15h CPU processor power monitor");
+MODULE_AUTHOR("Andreas Herrmann <[email protected]>");
+MODULE_LICENSE("GPL");
+
+/* D18F3 */
+#define REG_NORTHBRIDGE_CAP		0xe8
+
+/* D18F4 */
+#define REG_PROCESSOR_TDP		0x1b8
+
+/* D18F5 */
+#define REG_TDP_RUNNING_AVERAGE		0xe0
+#define REG_TDP_LIMIT3			0xe8

drivers/hwmon/fam15h_power.c line 48

Quoted 6 lines

drivers/hwmon/fam15h_power.c new file +6 -0

+
+struct fam15h_power_data {
+	struct device *hwmon_dev;
+	unsigned int tdp_to_watt;
+	unsigned int base_tdp;
+	unsigned int processor_pwr_watts;

watt vs. watts is inconsistent, isn't it?

drivers/hwmon/fam15h_power.c line 58

Quoted 10 lines

drivers/hwmon/fam15h_power.c new file +10 -0

+};
+
+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, tdp_limit, running_avg_range;
+	s32 running_avg_capture;
+	u64 curr_pwr_watts;
+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct fam15h_power_data *data = pci_get_drvdata(f4);

dev_get_drvdata(dev) would be more efficient.

drivers/hwmon/fam15h_power.c line 70

Quoted 12 lines

drivers/hwmon/fam15h_power.c new file +12 -0

+
+	pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
+				  REG_TDP_RUNNING_AVERAGE, &val);
+	running_avg_capture = (val >> 4) & 0x3fffff;
+	running_avg_capture = sign_extend32(running_avg_capture, 22);
+	running_avg_range = val & 0xf;
+
+	pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
+				  REG_TDP_LIMIT3, &val);
+
+	tdp_limit = val >> 16;
+	curr_pwr_watts = tdp_limit  + data->base_tdp -

Doubled space.

Quoted 10 lines

drivers/hwmon/fam15h_power.c new file +10 -0

+		(s32)(running_avg_capture >> (running_avg_range + 1));
+	curr_pwr_watts *= data->tdp_to_watt;
+
+	/*
+	 * Convert to microWatt
+	 *
+	 * power is in Watt provided as fixed point integer with
+	 * scaling factor 1/(2^16).  For conversion we use
+	 * (10^6)/(2^16) = 15625/(2^10)
+	 */

drivers/hwmon/fam15h_power.c line 90

Quoted 10 lines

drivers/hwmon/fam15h_power.c new file +10 -0

+	curr_pwr_watts = (curr_pwr_watts * 15625) >> 10;
+	return sprintf(buf, "%u\n", (unsigned int) curr_pwr_watts);
+}
+static DEVICE_ATTR(power1_input, S_IRUGO, show_power, NULL);
+
+static ssize_t show_power_max(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct fam15h_power_data *data = pci_get_drvdata(f4);

dev_get_drvdata(dev) would be more efficient (you don't even need f4 then.)

Quoted 13 lines

drivers/hwmon/fam15h_power.c new file +13 -0

+	return sprintf(buf, "%u\n", data->processor_pwr_watts);
+}
+static DEVICE_ATTR(power1_max, S_IRUGO, show_power_max, NULL);
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "fam15h_power\n");
+}
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static struct attribute *fam15h_power_attrs[] = {
+	&dev_attr_power1_input.attr,

drivers/hwmon/fam15h_power.c line 110

Quoted 6 lines

drivers/hwmon/fam15h_power.c new file +6 -0

+	&dev_attr_power1_max.attr,
+	&dev_attr_name.attr,
+	NULL
+};
+
+static struct attribute_group fam15h_power_attr_group = {

Can be made const.

Quoted 27 lines

drivers/hwmon/fam15h_power.c new file +27 -0

+	.attrs	= fam15h_power_attrs,
+};
+
+static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
+{
+	u32 val;
+
+	pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3),
+				  REG_NORTHBRIDGE_CAP, &val);
+	if ((val & BIT(29)) && ((val >> 30) & 3))
+		return false;
+
+	return true;
+}
+
+static void __devinit fam15h_power_init_data(struct pci_dev *f4,
+					     struct fam15h_power_data *data)
+{
+	u32 val;
+	u64 tmp;
+
+	pci_read_config_dword(f4, REG_PROCESSOR_TDP, &val);
+	data->base_tdp = val >> 16;
+	tmp = val & 0xffff;
+
+	pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
+				  REG_TDP_LIMIT3, &val);

drivers/hwmon/fam15h_power.c line 143

Quoted 6 lines

drivers/hwmon/fam15h_power.c new file +6 -0

+
+	data->tdp_to_watt = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	tmp *= data->tdp_to_watt;
+
+	/* result not allowed to be >= 256W */
+	if ((tmp>>16) >= 256)

Please add spaces around ">>" for consistency.

Quoted 33 lines

drivers/hwmon/fam15h_power.c new file +33 -0

+		dev_warn(&f4->dev, "Bogus value for ProcessorPwrWatts "
+			 "(processor_pwr_watts>=%u)\n",
+			 (unsigned int) (tmp >> 16));
+
+	/* convert to microWatt */
+	data->processor_pwr_watts = (tmp * 15625) >> 10;
+}
+
+static int __devinit fam15h_power_probe(struct pci_dev *pdev,
+					const struct pci_device_id *id)
+{
+	struct fam15h_power_data *data;
+	struct device *dev;
+	int err;
+
+	if (!fam15h_power_is_internal_node0(pdev)) {
+		err = -ENODEV;
+		goto exit;
+	}
+
+	data = kzalloc(sizeof(struct fam15h_power_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	fam15h_power_init_data(pdev, data);
+
+	dev = &pdev->dev;
+	err = sysfs_create_group(&dev->kobj, &fam15h_power_attr_group);
+	if (err)
+		goto exit_free_data;
+
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);

drivers/hwmon/fam15h_power.c line 181

Quoted 5 lines

drivers/hwmon/fam15h_power.c new file +5 -0

+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		goto exit_remove_group;
+	}
+	pci_set_drvdata(pdev, data);

This is racy. pci_set_drvdata() should be called before creating the sysfs attributes, because the sysfs callbacks need it.

drivers/hwmon/fam15h_power.c line 182

Quoted 2 lines

drivers/hwmon/fam15h_power.c new file +2 -0

+
+	return 0;

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

This driver provides

* power1_crit (ProcessorPwrWatts)
* power1_input (CurrPwrWatts)
(...)

From: Andreas Herrmann <[email protected]>

Signed-off-by Andreas Herrmann <[email protected]>

---
 MAINTAINERS |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

As suggested by Jean, here's a patch to add a maintainers entry for the new driver.

Regards,

Andreas

MAINTAINERS +0 -0

index 6b4b9cd..af8421d 100644

Hi Andreas,

Quoted 20 lines · Andreas Herrmann

From: Andreas Herrmann <[email protected]>

Signed-off-by Andreas Herrmann <[email protected]>

---
 MAINTAINERS |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

As suggested by Jean, here's a patch to add a maintainers entry for the new driver.

Regards,

Andreas

MAINTAINERS +0 -0

index 6b4b9cd..af8421d 100644

MAINTAINERS F: drivers/tty/serial/altera_jtaguart.c · line 486

Quoted 4 lines

MAINTAINERS +1 -0

@@ -483,6 +483,13 @@ F:	drivers/tty/serial/altera_jtaguart.c
 F:	include/linux/altera_uart.h
 F:	include/linux/altera_jtaguart.h
 
+AMD FAM15H PROCESSOR POWER HARDWARE MONITORING DRIVER

I'd remove "HARDWARE" as the target hardware is already described.

MAINTAINERS F: drivers/tty/serial/altera_jtaguart.c · line 495

Quoted 9 lines

MAINTAINERS +6 -0

+M:	Andreas Herrmann <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/hwmon/fam15h_power
+F:	drivers/hwmon/fam15h_power.c
+
 AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER
 M:	Thomas Dahlmann <[email protected]>
 L:	[email protected] (moderated for non-subscribers)

Other than this small detail, which I can fix myself if you agree with me, this looks good, thanks for stepping in!

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

This driver provides

* power1_cap (ProcessorPwrWatts)
* power1_input (CurrPwrWatts)

Changes from v1::
- removed deprecated code line,
- fixed comment,
- report power only once per socket (Power information is provided for the entire package. On multi-node processors it should only be reported on internal node 0.)

Signed-off-by Andreas Herrmann <[email protected]>

---
 drivers/hwmon/Kconfig      |   10 ++
 drivers/hwmon/Makefile     |    1 +
 drivers/hwmon/f15h_power.c |  216 ++++++++++++++++++++++++++++++++++++++++++++

 3 files changed, 227 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/f15h_power.c

drivers/hwmon/Kconfig +10 -0

index 8e84b31..0268623 100644
@@ -249,6 +249,16 @@ config SENSORS_K10TEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called k10temp.
 
+config SENSORS_F15H_POWER
+	tristate "AMD Family 15h processor power"
+	depends on X86 && PCI
+	help
+	  If you say yes here you get support for processor power
+	  information of your AMD family 15h CPU.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called f15hpower.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on X86 && I2C && EXPERIMENTAL

drivers/hwmon/Makefile +0 -0

index dde02d9..e554b88 100644

drivers/hwmon/Makefile +0 -0

 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o

drivers/hwmon/f15h_power.c new file +39 -0

new file mode 100644
index 0000000..40e3cbd
@@ -0,0 +1,216 @@
+/*
+ * f15h_power.c - AMD Family 15h processor power monitoring
+ *
+ * Copyright (c) 2011 Advanced Micro Devices, Inc.
+ * Author: Andreas Herrmann <[email protected]>
+ *
+ *
+ * This driver is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This driver is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this driver; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/bitops.h>
+#include <asm/processor.h>
+
+MODULE_DESCRIPTION("AMD Family 15h CPU processor power monitor");
+MODULE_AUTHOR("Andreas Herrmann <[email protected]>");
+MODULE_LICENSE("GPL");
+
+/* D18F3 */
+#define REG_NORTHBRIDGE_CAP		0xe8
+
+/* D18F4 */
+#define REG_PROCESSOR_TDP		0x1b8
+

drivers/hwmon/f15h_power.c new file +6 -0

+	pci_dev_put(f5);
+
+	tdpl = (val >> 16) & 0x1fff;
+	tdp2w = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	ctdp = tdpl - (s32)(acap >> (arange + 1)) + btdp;
+	ctdp *= tdp2w;

drivers/hwmon/f15h_power.c new file +11 -0

+}
+static DEVICE_ATTR(power1_input, S_IRUGO, show_power, NULL);
+
+static ssize_t show_power_cap(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	u32 val, tdp2w;
+	u64 ptdp;
+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct pci_dev *f5;
+

drivers/hwmon/f15h_power.c new file +8 -0

+}
+static DEVICE_ATTR(power1_cap, S_IRUGO, show_power_cap, NULL);
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "f15h_power\n");
+}

drivers/hwmon/f15h_power.c new file +7 -0

+		return 0;
+	}
+
+	pci_read_config_dword(f3, REG_NORTHBRIDGE_CAP, &val);
+	pci_dev_put(f3);
+
+	if ((val & BIT(29)) && ((val >> 30) & 3))

drivers/hwmon/f15h_power.c new file +4 -0

+
+	if (!f15h_power_is_internal_node0(pdev))
+		goto exit;
+

drivers/hwmon/f15h_power.c new file +8 -0

+
+	return 0;
+
+exit_remove:
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	device_remove_file(&pdev->dev, &dev_attr_power1_input);
+	device_remove_file(&pdev->dev, &dev_attr_power1_cap);
+exit:

drivers/hwmon/f15h_power.c new file +23 -0

+	{}
+};
+MODULE_DEVICE_TABLE(pci, f15h_power_id_table);
+
+static struct pci_driver f15h_power_driver = {
+	.name = "f15h_power",
+	.id_table = f15h_power_id_table,
+	.probe = f15h_power_probe,
+	.remove = __devexit_p(f15h_power_remove),
+};
+
+static int __init f15h_power_init(void)
+{
+	return pci_register_driver(&f15h_power_driver);
+}
+
+static void __exit f15h_power_exit(void)
+{
+	pci_unregister_driver(&f15h_power_driver);
+}
+
+module_init(f15h_power_init)
+module_exit(f15h_power_exit)

Quoted 10 lines · Andreas Herrmann

From: Andreas Herrmann <[email protected]>

This CPU family provides NB register values to gather following TDP information

* ProcessorPwrWatts: Specifies in Watts the maximum amount of power the processor can support.
* CurrPwrWatts: Specifies in Watts the current amount of power being consumed by the processor.

Quoted 4 lines

This driver provides

* power1_cap (ProcessorPwrWatts)
* power1_input (CurrPwrWatts)

Just a few random comments as I don't have the time for a complete review:

Quoted 8 lines

Changes from v1::
- removed deprecated code line,
- fixed comment,
- report power only once per socket (Power information is provided for the entire package. On multi-node processors it should only be reported on internal node 0.)

Quoted 5 lines

Signed-off-by Andreas Herrmann <[email protected]>

---
 drivers/hwmon/Kconfig      |   10 ++
 drivers/hwmon/Makefile     |    1 +
 drivers/hwmon/f15h_power.c |  216 ++++++++++++++++++++++++++++++++++++++++++++

I would prefer fam15h to f15h. The latter is a little too cryptic for me.

drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_JC42) += jc42.o · line 65

Quoted 7 lines

drivers/hwmon/Makefile +1 -0

@@ -62,6 +62,7 @@ obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
 obj-$(CONFIG_SENSORS_JZ4740)	+= jz4740-hwmon.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
+obj-$(CONFIG_SENSORS_F15H_POWER) += f15h_power.o

Alphabetic order please.

drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_JC42) += jc42.o

Quoted 6 lines

drivers/hwmon/Makefile +6 -0

+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, btdp, tdpl, tdp2w, arange;
+	s32 acap;
+	u64 ctdp;

These variable names aren't easy to understand.

drivers/hwmon/Makefile

Quoted 5 lines

drivers/hwmon/Makefile +5 -0

+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct pci_dev *f5;
+
+	pci_read_config_dword(f4, REG_PROCESSOR_TDP, &val);
+	btdp = (val >> 16) & 0xffff;

Useless masking.

drivers/hwmon/Makefile

Quoted 8 lines

drivers/hwmon/Makefile +8 -0

+
+	f5 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5));
+	if (!f5) {
+		dev_err(dev, "no function 5 available on this slot\n");
+		return 0;
+	}
+
+	pci_read_config_dword(f5, REG_TDP_RUNNING_AVERAGE, &val);

Does this value change over time?

drivers/hwmon/Makefile

Quoted 5 lines

drivers/hwmon/Makefile +5 -0

+	acap = (val >> 4) & 0x3fffff;
+	acap = sign_extend32(acap, 22);
+	arange = val & 0xf;
+
+	pci_read_config_dword(f5, REG_TDP_LIMIT3, &val);

Same question here. If these values don't change over time, they should be stored in a per-device structure to avoid repeated PCI configuration space access.

Quoted 10 lines

drivers/hwmon/Makefile +10 -0

+	pci_dev_put(f5);
+
+	tdpl = (val >> 16) & 0x1fff;
+	tdp2w = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	ctdp = tdpl - (s32)(acap >> (arange + 1)) + btdp;
+	ctdp *= tdp2w;
+
+	/*
+	 * Convert to microWatt
+	 *

drivers/hwmon/Makefile

Quoted 6 lines

drivers/hwmon/Makefile +6 -0

+	 * power is in Watt provided as fixed point integer with
+	 * scaling factor 1/(2^16).  For conversion we use
+	 * (10^6)/(2^16) = 15625/(2^10)
+	 */
+	ctdp = (ctdp * 15625) >> 10;
+	return sprintf(buf, "%d\n", (u32) ctdp);

%d and u32 aren't compatible. %lu and long unsigned int would be OK.

Quoted 15 lines

drivers/hwmon/Makefile +15 -0

+}
+static DEVICE_ATTR(power1_input, S_IRUGO, show_power, NULL);
+
+static ssize_t show_power_cap(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	u32 val, tdp2w;
+	u64 ptdp;
+	struct pci_dev *f4 = to_pci_dev(dev);
+	struct pci_dev *f5;
+
+	pci_read_config_dword(f4, REG_PROCESSOR_TDP, &val);
+	ptdp = val & 0xffff;
+
+	f5 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5));

drivers/hwmon/Makefile

Quoted 6 lines

drivers/hwmon/Makefile +6 -0

+	if (!f5) {
+		dev_err(dev, "no function 5 available on this slot\n");
+		return 0;
+	}
+
+	pci_read_config_dword(f5, REG_TDP_LIMIT3, &val);

Same question as above, can this value change over time?

Quoted 5 lines

drivers/hwmon/Makefile +5 -0

+	pci_dev_put(f5);
+
+	tdp2w = ((val & 0x3ff) << 6) | ((val >> 10) & 0x3f);
+	ptdp *= tdp2w;
+

drivers/hwmon/Makefile

Quoted 6 lines

drivers/hwmon/Makefile +6 -0

+	/* result not allowed to be >= 256W */
+	WARN_ON(ptdp>>16 >= 256);
+
+	/* convert to microWatt */
+	ptdp = (ptdp * 15625) >> 10;
+	return sprintf(buf, "%d\n", (u32) ptdp);

%d and u32 aren't compatible. %lu and long unsigned int would be OK.

Quoted 14 lines

drivers/hwmon/Makefile +14 -0

+}
+static DEVICE_ATTR(power1_cap, S_IRUGO, show_power_cap, NULL);
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "f15h_power\n");
+}
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static int __devinit f15h_power_is_internal_node0(struct pci_dev *f4)
+{
+	u32 val;
+	struct pci_dev *f3;

drivers/hwmon/Makefile

Quoted 4 lines

drivers/hwmon/Makefile +4 -0

+
+	f3 = pci_get_slot(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 3));
+	if (!f3) {
+		dev_err(&f4->dev, "no function 3 available on this slot\n");

Please use %d and pass the number as a parameter, and do the same for the 2 similar messages above. This will let the compiler reuse the string.

Quoted 12 lines

drivers/hwmon/Makefile +12 -0

+		return 0;
+	}
+
+	pci_read_config_dword(f3, REG_NORTHBRIDGE_CAP, &val);
+	pci_dev_put(f3);
+
+	if ((val & BIT(29)) && ((val >> 30) & 3))
+		return 0;
+
+	return 1;
+}
+

drivers/hwmon/Makefile

Quoted 5 lines

drivers/hwmon/Makefile +5 -0

+static int __devinit f15h_power_probe(struct pci_dev *pdev,
+				      const struct pci_device_id *id)
+{
+	struct device *hwmon_dev;
+	int err = -ENODEV;

You should move this initialization to the only case which needs it. Otherwise you're slowing down the success path with no good reason.

Quoted 9 lines

drivers/hwmon/Makefile +9 -0

+
+	if (!f15h_power_is_internal_node0(pdev))
+		goto exit;
+
+	err = device_create_file(&pdev->dev, &dev_attr_power1_input);
+	if (err)
+		goto exit;
+	err = device_create_file(&pdev->dev, &dev_attr_power1_cap);
+	if (err)

drivers/hwmon/Makefile

Quoted 5 lines

drivers/hwmon/Makefile +5 -0

+		goto exit_remove;
+
+	err = device_create_file(&pdev->dev, &dev_attr_name);
+	if (err)
+		goto exit_remove;

Please consider defining and using an attribute group, it makes the code more simple.

drivers/hwmon/Makefile

Quoted 7 lines

drivers/hwmon/Makefile +7 -0

+
+	hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(hwmon_dev)) {
+		err = PTR_ERR(hwmon_dev);
+		goto exit_remove;
+	}
+	dev_set_drvdata(&pdev->dev, hwmon_dev);

pci_set_drvdata()

Quoted 8 lines

drivers/hwmon/Makefile +8 -0

+
+	return 0;
+
+exit_remove:
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	device_remove_file(&pdev->dev, &dev_attr_power1_input);
+	device_remove_file(&pdev->dev, &dev_attr_power1_cap);
+exit:

drivers/hwmon/Makefile

Quoted 10 lines

drivers/hwmon/Makefile +10 -0

+	return err;
+}
+
+static void __devexit f15h_power_remove(struct pci_dev *pdev)
+{
+	hwmon_device_unregister(dev_get_drvdata(&pdev->dev));
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	device_remove_file(&pdev->dev, &dev_attr_power1_input);
+	device_remove_file(&pdev->dev, &dev_attr_power1_cap);
+	dev_set_drvdata(&pdev->dev, NULL);

pci_set_drvdata()

drivers/hwmon/Makefile

Quoted 4 lines

drivers/hwmon/Makefile +4 -0

+}
+
+static const struct pci_device_id f15h_power_id_table[] = {
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_LINK) },

Not defined anywhere, causing the driver to fail building:

drivers/hwmon/f15h_power.c:193:1: error: 'PCI_DEVICE_ID_AMD_15H_NB_LINK' undeclared here (not in a function) make[2]: *** [drivers/hwmon/f15h_power.o] Error 1
make[1]: *** [drivers/hwmon] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

Quoted 9 lines · Jean Delvare

drivers/hwmon/f15h_power.c line 49

Quoted 6 lines · Andreas Herrmann

drivers/hwmon/f15h_power.c new file +6 -0

+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, btdp, tdpl, tdp2w, arange;
+	s32 acap;
+	u64 ctdp;

These variable names aren't easy to understand.

Just random names which eventually map to the spec:

btdp - base_tdp
tdpl - tdp_limit
tdp2w - tdp_to_watt
acap - average_accumulator_capture (or even worse how about "processor_tdp_running_average_accumulator":( arange - average_range

I don't think that changing the names make it much easier to reconstruct the calculation but if you insist in changing it I'll adapt it.

Quoted 4 lines

drivers/hwmon/f15h_power.c line 171

Quoted 2 lines

(...)

drivers/hwmon/f15h_power.c new file +1 -0

+	dev_set_drvdata(&pdev->dev, hwmon_dev);

pci_set_drvdata()

Are you sure?

No single hwmon driver is using this wrapper around dev_set_drvdata() so far. (Or I just missed it.)

At the moment only dev_set_drvdata() is used by

drivers/hwmon/adcxx.c
drivers/hwmon/lm70.c
drivers/hwmon/ultra45_env.c
drivers/hwmon/ibmpex.c
drivers/hwmon/k10temp.c
drivers/hwmon/ibmaem.c
drivers/hwmon/k8temp.c

Quoted 11 lines · Andreas Herrmann

Quoted 9 lines · Jean Delvare

drivers/hwmon/f15h_power.c line 49

Quoted 6 lines · Andreas Herrmann

drivers/hwmon/f15h_power.c new file +6 -0

+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, btdp, tdpl, tdp2w, arange;
+	s32 acap;
+	u64 ctdp;

These variable names aren't easy to understand.

Quoted 7 lines

Just random names which eventually map to the spec:

btdp - base_tdp
tdpl - tdp_limit
tdp2w - tdp_to_watt
acap - average_accumulator_capture (or even worse how about "processor_tdp_running_average_accumulator":( arange - average_range

avg_cap and avg_range would do, respectively, for the last two.

Quoted 3 lines

I don't think that changing the names make it much easier to reconstruct the calculation but if you insist in changing it I'll adapt it.

I do prefer the "extended" names, really. Sure, this doesn't change the calculations, but it helps the reader understand what's going on. Which will be useful if one ever has to fix a bug in the code or extend it for a different CPU family.

But maybe this is just me. Guenter, do you have an opinion?

Quoted 31 lines · Jean Delvare

Quoted 11 lines · Andreas Herrmann

Quoted 9 lines · Jean Delvare

drivers/hwmon/f15h_power.c line 49

Quoted 6 lines · Andreas Herrmann

drivers/hwmon/f15h_power.c new file +6 -0

+static ssize_t show_power(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	u32 val, btdp, tdpl, tdp2w, arange;
+	s32 acap;
+	u64 ctdp;

These variable names aren't easy to understand.

Quoted 7 lines

Just random names which eventually map to the spec:

btdp - base_tdp
tdpl - tdp_limit
tdp2w - tdp_to_watt
acap - average_accumulator_capture (or even worse how about "processor_tdp_running_average_accumulator":( arange - average_range

avg_cap and avg_range would do, respectively, for the last two.

Quoted 3 lines

I don't think that changing the names make it much easier to reconstruct the calculation but if you insist in changing it I'll adapt it.

I do prefer the "extended" names, really. Sure, this doesn't change the calculations, but it helps the reader understand what's going on. Which will be useful if one ever has to fix a bug in the code or extend it for a different CPU family.

Quoted 2 lines

But maybe this is just me. Guenter, do you have an opinion?

I agree. base_tdp is definitely much better than btdp. Same for the others.

Thanks,
Guenter