iMC SMBUS, TSOD hwmon devices, and eeprom modalias

Add i2c_scan_dimm_bus to declare that a particular i2c_adapter contains DIMMs. This will probe (and autoload modules!) for useful SMBUS devices that live on DIMMs.

As more SMBUS-addressable DIMM components become supported, this code can be extended to probe for them.

Quoted 7 lines · Andy Lutomirski

Add i2c_scan_dimm_bus to declare that a particular i2c_adapter contains DIMMs. This will probe (and autoload modules!) for useful SMBUS devices that live on DIMMs.

As more SMBUS-addressable DIMM components become supported, this code can be extended to probe for them.

I don't think this code is necessary. The temperature sensor would be auto-detected if you mark the bus driver as I2C_CLASS_HWMON, and DIM eeprom auto-detection should not be needed.

Quoted 11 lines · Guenter Roeck

Quoted 7 lines · Andy Lutomirski

Add i2c_scan_dimm_bus to declare that a particular i2c_adapter contains DIMMs. This will probe (and autoload modules!) for useful SMBUS devices that live on DIMMs.

As more SMBUS-addressable DIMM components become supported, this code can be extended to probe for them.

I don't think this code is necessary. The temperature sensor would be auto-detected if you mark the bus driver as I2C_CLASS_HWMON, and DIM eeprom auto-detection should not be needed.

I'm not at all sure I want to mark this thing I2C_CLASS_HWMON. I don't want random hwmon drivers probing the bus. (Although I wouldn't need to -- jc42 is looking for I2C_CLASS_SPD.)

Also, my code is IMO better than the i2c core probing code for several reasons:

- i2c_imc is incompatible with the i2c core class-based detection
code because it can't do a read byte operation. I believe that, because of exactly this limitation, devices soldered onto DIMMs are unlikely to ever require this operation. (LGA2011 is the premier platform for interesting DIMM-like devices right now.) So I'd have to modify the i2c core with more nasty hard-coded lists of what can be safely probed how, and ISTM that kind of thing is better handled in code that runs on busses that are really known to contain DIMMs (and not, say, i2c-gpio).

- The eventual goal is to write a driver for NVDIMMs, which will also
appear on this bus, and I want them to pull in the right drivers via modalias and udev, which the i2c core code can't do. To do this well, I'll want the SMBUS driver to pass in (via platform-data) information on which DIMM channel is which, and I don't know any way to do anything like that without using i2c_new_device.

- Busses like i2c_i801 may or may not contain DIMMs. They do on some
machines I have. But on my Core i7 Extreme box, there are no DIMMs on that bus, and there are no SPDs on that bus (i.e. addresses 0x50 through 0x57 don't answer), but there is an unknown device at address 0x19. I don't really want to think about whether it's safe to probe by, say, read_word_data(0). When NVDIMMs show up, even more bizarre addresses may be populated. My code can probe *carefully* by looking for SPDs first, whereas the i2c-core code is not nearly as careful.

One option would be to move code like this into the core and replace I2C_CLASS_SPD with something like it. That way everything could get the benefit of DIMM-specific probing.

Signed-off-by Andy Lutomirski <[email protected]>

---

 drivers/hwmon/Kconfig  |  10 +++
 drivers/hwmon/Makefile |   1 +
 drivers/hwmon/tsod.c   | 195 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/hwmon/tsod.c

drivers/hwmon/Kconfig +10 -0

index 89ac1cb..96edb87 100644
@@ -1462,6 +1462,16 @@ config SENSORS_MC13783_ADC
         help
           Support for the A/D converter on MC13783 and MC13892 PMIC.
 
+config SENSORS_TSOD
+	tristate "Temperature Sensor On DIMM (TSOD)"
+	depends on I2C
+	help
+	  If you say yes here you get support for the integrated temperature
+	  sensors on newer DIMMs that comply with JESD21-C.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called tsod.
+
 if ACPI
 
 comment "ACPI drivers"

drivers/hwmon/Makefile +1 -0

index 8d6d97e..439ef1f 100644
@@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
 obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
+obj-$(CONFIG_SENSORS_TSOD)	+= tsod.o
 
 obj-$(CONFIG_PMBUS)		+= pmbus/
 

drivers/hwmon/tsod.c new file +195 -0

new file mode 100644
index 0000000..f7bb070
@@ -0,0 +1,195 @@
+/*
+ * drivers/hwmon/tsod.c - Temperaure Sensor On DIMM
+ *
+ * Copyright (C) 2013 Andrew Lutomirski <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License v2 as published by the
+ * Free Software Foundation.
+ *
+ * This program 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 program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ *
+ * The official reference for these devices is JEDEC Standard No. 21-C,
+ * which is available for free from www.jedec.org.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/slab.h>
+
+/* Registers */
+#define TSOD_CURRENT_TEMP	5
+#define TSOD_VENDOR		6
+#define TSOD_DEVICE		7
+
+/*
+ * This driver does not program the trip points, etc. -- this is done by
+ * firmware, and the memory controller probably wants the defaults preserved.
+ */
+
+struct tsod_priv {
+	struct i2c_client *client;
+	struct device *hwmondev;
+};
+
+static ssize_t show_name(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "TSOD\n");
+}
+
+static ssize_t show_label(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "DIMM Temperature\n");
+}
+
+static ssize_t show_temperature(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct tsod_priv *priv = dev_get_drvdata(dev->parent);
+	int temp, raw;
+
+	raw = i2c_smbus_read_word_swapped(priv->client, TSOD_CURRENT_TEMP);
+	if (raw < 0)
+		return raw;
+
+	/*
+	 * The three high bits are undefined and the rest is twos-complement.
+	 * Use a sign-extending right shift to propagate the sign bit.
+	 */
+	temp = ((s16)((s16)raw << 3) >> 3);
+
+	/*
+	 * The value is in units of 0.0625 degrees, but we want it in
+	 * units of 0.001 degrees.
+	 */
+	return sprintf(buf, "%d\n", DIV_ROUND_CLOSEST(temp * 625, 10));
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temperature, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, 0);
+
+static struct attribute *tsod_hwmon_attributes[] = {
+	&dev_attr_name.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+
+	NULL,
+};
+
+static const struct attribute_group tsod_hwmon_attr_group = {
+	.attrs	= tsod_hwmon_attributes,
+};
+
+static int tsod_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_WORD_DATA))
+		return -ENODEV;
+
+	strlcpy(info->type, "tsod", I2C_NAME_SIZE);
+	return 0;
+}
+
+static int tsod_probe(struct i2c_client *client,
+		      const struct i2c_device_id *id)
+{
+	int ret;
+	struct tsod_priv *priv;
+
+	/* Sanity check the address */
+	if ((client->addr & 0x78) != 0x18)
+		return -ENODEV;
+
+	/* Sanity check: make sure we can read the temperature. */
+	ret = i2c_smbus_read_word_swapped(client, TSOD_CURRENT_TEMP);
+	if (ret < 0)
+		return -ENODEV;
+
+	priv = kzalloc(sizeof(struct tsod_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+
+	priv->hwmondev = hwmon_device_register(&client->dev);
+	if (IS_ERR(priv->hwmondev)) {
+		ret = PTR_ERR(priv->hwmondev);
+		goto err_free;
+	}
+
+	i2c_set_clientdata(client, priv);
+
+	ret = sysfs_create_group(&priv->hwmondev->kobj, &tsod_hwmon_attr_group);
+	if (ret)
+		goto err_unreg;
+
+	return 0;
+
+err_unreg:
+	hwmon_device_unregister(&client->dev);
+
+err_free:
+	kfree(priv);
+	i2c_set_clientdata(client, 0);
+	return ret;
+}
+
+static int tsod_remove(struct i2c_client *client)
+{
+	struct tsod_priv *priv = i2c_get_clientdata(client);
+
+	sysfs_remove_group(&priv->hwmondev->kobj, &tsod_hwmon_attr_group);
+	hwmon_device_unregister(priv->hwmondev);
+	kfree(priv);
+	return 0;
+}
+
+static const unsigned short tsod_addresses[] = {
+	0x18, 0x19, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, I2C_CLIENT_END
+};
+
+static const struct i2c_device_id tsod_id[] = {
+	{ "tsod", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tsod_id);
+
+static struct i2c_driver tsod_driver = {
+	.driver = {
+		.name	= "tsod",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= tsod_probe,
+	.remove		= tsod_remove,
+	.id_table	= tsod_id,
+
+	/*
+	 * We do not claim I2C_CLASS_SPD -- there are other devices
+	 * on, e.g., the i2c_i801 bus that have these addresses.
+	 * Instead we let the dimm-bus code instantiate us.
+	 */
+
+	.detect		= tsod_detect,
+	.address_list	= tsod_addresses,
+};
+
+module_i2c_driver(tsod_driver);
+
+MODULE_AUTHOR("Andrew Lutomirski <[email protected]>");
+MODULE_DESCRIPTION("Temperaure Sensor On DIMM");
+MODULE_LICENSE("GPL");

Signature · 1 line

1.8.1.4

Quoted 2 lines · Andy Lutomirski

Signed-off-by Andy Lutomirski <[email protected]>

---

Why don't you just use the existing jc42 driver ?

Quoted 5 lines · Guenter Roeck

Quoted 2 lines · Andy Lutomirski

Signed-off-by Andy Lutomirski <[email protected]>

---

Why don't you just use the existing jc42 driver ?

Failure to notice it. *sigh*. I assumed that any driver for this device would have "tsod" or "TSOD" somewhere greppable. I'll at least send a patch to improve the description.

That being said, the jc42 driver *writes*. The iMC controller (AFAICS) uses the data from the TSOD for things like dynamically adjusting the DRAM refresh interval and IMO the kernel has no business at all writing to any registers on the TSOD on this platform. (FWIW, I tried fiddling with the critical threshold and my box didn't blow up or report thermal trip events. But still, this scares me a bit.)

If I modify the i2c_imc driver to add .class = I2C_CLASS_SPD and get rid of the TSOD probing and load the jc42 module, then it can't actually find my TSODs (because the i2c core isn't capable of probing on the iMC bus). If I manually add the device, it works.

But I'd like to keep the enumeration code I wrote around, since the main point of this exercise is to enumerate things that aren't actually hwmon devices. But I don't want random sensors scripts reprogramming the hardware. Is the some magic I can work (or should add) with i2c_board_info to force the jc42 driver into a read-only mode?

Quoted 11 lines · Andy Lutomirski

Quoted 5 lines · Guenter Roeck

Quoted 2 lines · Andy Lutomirski

Signed-off-by Andy Lutomirski <[email protected]>

---

Why don't you just use the existing jc42 driver ?

Failure to notice it. *sigh*. I assumed that any driver for this device would have "tsod" or "TSOD" somewhere greppable. I'll at least send a patch to improve the description.

Quoted 7 lines

That being said, the jc42 driver *writes*. The iMC controller (AFAICS) uses the data from the TSOD for things like dynamically adjusting the DRAM refresh interval and IMO the kernel has no business at all writing to any registers on the TSOD on this platform. (FWIW, I tried fiddling with the critical threshold and my box didn't blow up or report thermal trip events. But still, this scares me a bit.)

I don't see a problem with that. If the i2c controller supports writes, one can perform the write from user space with i2cset, so what is the problem ? Besides, jc42 compliant temperature sensors expect to be written to in order to set the temperature limits. If you don't like that, you don't have to do it.

Quoted 6 lines

If I modify the i2c_imc driver to add .class = I2C_CLASS_SPD and get rid of the TSOD probing and load the jc42 module, then it can't actually find my TSODs (because the i2c core isn't capable of probing on the iMC bus). If I manually add the device, it works.

You mean it doesn't support the SMBus quick command ? Did you set I2C_CLASS_HWMON as well, or just I2C_CLASS_SPD ?

If you have the ability to use i2c_board_info, the failure to auto-detect the devices should not really matter. In that case you can just use one of the other methods to instantiate the i2c devices.

Quoted 8 lines

But I'd like to keep the enumeration code I wrote around, since the main point of this exercise is to enumerate things that aren't actually hwmon devices. But I don't want random sensors scripts reprogramming the hardware. Is the some magic I can work (or should add) with i2c_board_info to force the jc42 driver into a read-only mode?

Again, that is user space code doing the write, which you can control without reverting to a "private" driver. That the driver permits the write doesn't mean you have to use it. The only write it always performs is to enable the sensor if it is disabled .. which presumably should be ok even for you, since otherwise loading the driver doesn't make any sense in the first place.

The new DIMM-bus code can detect eeproms, so give the eeprom driver an appropriate modalias.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/misc/eeprom/eeprom.c | 1 +
 1 file changed, 1 insertion(+)

drivers/misc/eeprom/eeprom.c +0 -0

index c169e07..1ec85b8 100644

Hi Andy,

Quoted 18 lines · Andy Lutomirski

The new DIMM-bus code can detect eeproms, so give the eeprom driver an appropriate modalias.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/misc/eeprom/eeprom.c | 1 +
 1 file changed, 1 insertion(+)

drivers/misc/eeprom/eeprom.c +1 -0

index c169e07..1ec85b8 100644
@@ -215,6 +215,7 @@ static const struct i2c_device_id eeprom_id[] = {
      { "eeprom", 0 },
      { }
 };
+MODULE_DEVICE_TABLE(i2c, eeprom_id);
 

drivers/misc/eeprom/eeprom.c static const struct i2c_device_id eeprom_id[] = { · line 221

Quoted 2 lines

drivers/misc/eeprom/eeprom.c +0 -0

 static struct i2c_driver eeprom_driver = {
      .driver = {

Nack. The eeprom driver will eventually die, I don't want to see anything like that added to it. Please use the at24 driver for anything new.

Thanks,
--
Jean Delvare

at24 doesn't have .class=I2C_CLASS_SPD (and I still am not really a fan of the i2c class mechanism).

Shall I play with ways to get known
DIMM-only busses to probe these things directly and instantiate with i2c_new_device(..., .type="spd")? Do you have other plans for how probing should work?

(For background in case you haven't been following the rest of the
thread: the Intel iMC (integrated memory controller) bus appears to have only DIMMs attached, so everything on the bus is either the host or is something attached to a DIMM. The latter devices have well-defined addresses: four bits of type and three bits of slot number. This makes it easy to probe accurately. I'm not sure that the i2c core class mechanism is well-suited to this, it's easy to do manually.)

Quoted 2 lines · Andy Lutomirski

at24 doesn't have .class=I2C_CLASS_SPD (and I still am not really a fan of the i2c class mechanism).

The class-based auto-detection has its merits but it's not the solution to all problems.

.class=I2C_CLASS_SPD could be added to the at24 driver. If we ever want to kill the legacy eeprom driver, we will have to do exactly that.

That being said this won't help in your case, as your bus driver does not support the SMBus transaction (Receive Byte) used to probe for EEPROM chips in address range 0x50-0x57.

Quoted 4 lines

Shall I play with ways to get known
DIMM-only busses to probe these things directly and instantiate with i2c_new_device(..., .type="spd")? Do you have other plans for how probing should work?

You could indeed call i2c_new_probed_device() on addresses 0x50-0x57 at the end of your new i2c bus driver's probe function, in order to instantiate spd devices. This function takes a probe function as a parameter so you could use a transaction type supported (Read Byte.)

Quoted 8 lines

(For background in case you haven't been following the rest of the
thread: the Intel iMC (integrated memory controller) bus appears to have only DIMMs attached, so everything on the bus is either the host or is something attached to a DIMM. The latter devices have well-defined addresses: four bits of type and three bits of slot number. This makes it easy to probe accurately. I'm not sure that the i2c core class mechanism is well-suited to this, it's easy to do manually.)

Ideally the BIOS/firmware/whatever would tell the OS which memory slots are used so that the kernel can instantiate all I2C/SMBus slave devices without even probing for them. I really wonder how many decades it will take to Intel and co to come up with a complete device typology description on the PC/x86_64 platfor, so that probing is finally history.