[PATCH v3 0/4] iMC SMBUS, TSOD hwmon devices, and eeprom modalias

[PATCH v3 0/4] iMC SMBUS, TSOD hwmon devices, and eeprom modalias

[Andy Lutomirski]

Intel LGA2011 machines have dedicated SMBUS controllers for DIMM sockets.  Because they're dedicated, they can be safely and accurately probed, since all devices on them are known to be attached to DIMMs.  The devices found are:
 - SPD EEPROMs
 - TSODs (Temperature Sensor on DIMMs), a JEDEC standard device
 - Other interesting things, with drivers hopefully to follow...

This patch series adds a simple generic layer for probing for DIMMs over
SMBUS, an i2c bus driver for the iMC controller found on Intel LGA2011
chips, and a modalias for the eeprom driver so it can be automatically
loaded.

I've tested this on a Core i7 Extreme and on a Xeon E5 server.
With this series applied, sensors shows (on the Xeon E5):

TSOD-i2c-1-18
Adapter: iMC socket 0 channel 0
DIMM Temperature:  +35.2°C  

TSOD-i2c-1-1c
Adapter: iMC socket 0 channel 0
DIMM Temperature:  +33.0°C  

TSOD-i2c-3-18
Adapter: iMC socket 1 channel 0
DIMM Temperature:  +34.0°C  

TSOD-i2c-3-1c
Adapter: iMC socket 1 channel 0
DIMM Temperature:  +27.2°C 

with no need for any userspace probing or manual module loading.
decode-dimms works on both machines, again without any manual module
loading.

The whole series is available on git here (against v3.9.7):
https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=i2c_imc/patch_v3

Changes from earlier versions:
 - Dropped changes to core I2C_CLASS_SPD probing
 - Cleanup up i2c_imc driver
 - Added dimm-bus, tsod driver, and eeprom modalias

Andy Lutomirski (4):
  i2c: Add DIMM bus code
  i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
  tsod: New hwmon driver for Temperature Sensors on DIMM
  eeprom: Add a MODULE_DEVICE_TABLE

 drivers/hwmon/Kconfig         |  10 +
 drivers/hwmon/Makefile        |   1 +
 drivers/hwmon/tsod.c          | 195 +++++++++++++++
 drivers/i2c/busses/Kconfig    |  19 ++
 drivers/i2c/busses/Makefile   |   5 +
 drivers/i2c/busses/dimm-bus.c |  84 +++++++
 drivers/i2c/busses/i2c-imc.c  | 548 ++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/eeprom/eeprom.c  |   1 +
 include/linux/i2c/dimm-bus.h  |  24 ++
 9 files changed, 887 insertions(+)
 create mode 100644 drivers/hwmon/tsod.c
 create mode 100644 drivers/i2c/busses/dimm-bus.c
 create mode 100644 drivers/i2c/busses/i2c-imc.c
 create mode 100644 include/linux/i2c/dimm-bus.h

-- 
1.8.1.4

[PATCH v3 1/4] i2c: Add DIMM bus code

[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.

Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
 drivers/i2c/busses/Kconfig    |  4 +++
 drivers/i2c/busses/Makefile   |  4 +++
 drivers/i2c/busses/dimm-bus.c | 84 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/dimm-bus.h  | 24 +++++++++++++
 4 files changed, 116 insertions(+)
 create mode 100644 drivers/i2c/busses/dimm-bus.c
 create mode 100644 include/linux/i2c/dimm-bus.h

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index adfee98..e837f0e 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -133,6 +133,10 @@ config I2C_ISMT
 	  This driver can also be built as a module.  If so, the module will be
 	  called i2c-ismt.
 
+config I2C_DIMM_BUS
+       tristate
+       default n
+
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 8f4fc23..226bb2e 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -24,6 +24,10 @@ obj-$(CONFIG_I2C_SIS96X)	+= i2c-sis96x.o
 obj-$(CONFIG_I2C_VIA)		+= i2c-via.o
 obj-$(CONFIG_I2C_VIAPRO)	+= i2c-viapro.o
 
+# DIMM busses
+obj-$(CONFIG_I2C_DIMM_BUS)	+= dimm-bus.o
+obj-$(CONFIG_I2C_IMC)		+= i2c-imc.o
+
 # Mac SMBus host controller drivers
 obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
diff --git a/drivers/i2c/busses/dimm-bus.c b/drivers/i2c/busses/dimm-bus.c
new file mode 100644
index 0000000..9012638
--- /dev/null
+++ b/drivers/i2c/busses/dimm-bus.c
@@ -0,0 +1,84 @@
+/*
+ * Copyright (c) 2013 Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/i2c.h>
+#include <linux/bug.h>
+#include <linux/module.h>
+#include <linux/i2c/dimm-bus.h>
+
+static bool probe_addr(struct i2c_adapter *adapter, int addr)
+{
+	/*
+	 * So far, all known devices that live on DIMMs can be safely
+	 * and reliably detected by trying to read a byte at address
+	 * zero.
+	 */
+	union i2c_smbus_data dummy;
+	return i2c_smbus_xfer(adapter, addr, 0, I2C_SMBUS_READ, 0,
+			      I2C_SMBUS_BYTE_DATA, &dummy) >= 0;
+}
+
+/**
+ * i2c_scan_dimm_bus() - Scans an SMBUS segment known to contain DIMMs
+ * @adapter: The SMBUS adapter to scan
+ *
+ * This function tells the DIMM-bus code that the adapter is known to
+ * contain DIMMs.  i2c_scan_dimm_bus will probe for devices known to
+ * live on DIMMs.
+ *
+ * Do NOT call this function on general-purpose system SMBUS segments
+ * unless you know that the only things on the bus are DIMMs.
+ * Otherwise is it very likely to mis-identify other things on the
+ * bus.
+ *
+ * Callers are advised not to set adapter->class = I2C_CLASS_SPD.
+ */
+void i2c_scan_dimm_bus(struct i2c_adapter *adapter)
+{
+	struct i2c_board_info info = {};
+	int slot;
+
+	/*
+	 * We probe with "read byte data".  If any DIMM SMBUS driver can't
+	 * support that access type, this function should be updated.
+	 */
+	if (WARN_ON(!i2c_check_functionality(adapter,
+					  I2C_FUNC_SMBUS_READ_BYTE_DATA)))
+		return;
+
+	for (slot = 0; slot < 8; slot++) {
+		/* If there's no SPD, then assume there's no DIMM here. */
+		if (!probe_addr(adapter, 0x50 | slot))
+			continue;
+
+		strcpy(info.type, "eeprom");
+		info.addr = 0x50 | slot;
+		i2c_new_device(adapter, &info);
+
+		if (probe_addr(adapter, 0x18 | slot)) {
+			/* This is a temperature sensor. */
+			strcpy(info.type, "tsod");
+			info.addr = 0x18 | slot;
+			i2c_new_device(adapter, &info);
+		}
+	}
+}
+EXPORT_SYMBOL(i2c_scan_dimm_bus);
+
+MODULE_AUTHOR("Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>");
+MODULE_DESCRIPTION("i2c DIMM bus support");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c/dimm-bus.h b/include/linux/i2c/dimm-bus.h
new file mode 100644
index 0000000..3d44df4
--- /dev/null
+++ b/include/linux/i2c/dimm-bus.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2013 Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _I2C_DIMM_BUS
+#define _I2C_DIMM_BUS
+
+struct i2c_adapter;
+void i2c_scan_dimm_bus(struct i2c_adapter *adapter);
+
+#endif /* _I2C_DIMM_BUS */
-- 
1.8.1.4

Re: [PATCH v3 1/4] i2c: Add DIMM bus code

[Guenter Roeck]

On Wed, Jul 17, 2013 at 01:53:05PM -0700, Andy Lutomirski wrote:
> 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.

Guenter

Re: [PATCH v3 1/4] i2c: Add DIMM bus code

[Andy Lutomirski]

On Wed, Jul 17, 2013 at 3:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On Wed, Jul 17, 2013 at 01:53:05PM -0700, Andy Lutomirski wrote:
>> 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.

--Andy

Re: [PATCH v3 1/4] i2c: Add DIMM bus code

[Andy Lutomirski]

Some thoughts on an alternative solution below:

On Wed, Jul 17, 2013 at 4:04 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Wed, Jul 17, 2013 at 3:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> On Wed, Jul 17, 2013 at 01:53:05PM -0700, Andy Lutomirski wrote:
>>> 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.

Perhaps i2c_adapter could have a new field for this purpose.  For DIMM
busses it could store topology information.  Then any clients
instantiated on the bus could read it to figure out where they are.

>
>  - 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.

What if the core code got a little smarter than just matching
arbitrary bits in the class?  For example, if the adapter is
I2C_CLASS_SPD, then the core could look for eeproms (and optionally
instantiate them itself instead of relying on the eeprom driver being
loaded).  Or, more generally, adapters could have a list of named
classes, and there could be a module or something for each class that
knows how to probe and instantiate devices of that class.  So the
class "nvdimm" could instantiate whatever it needs to instantiate, the
class "tsod" could instantiate jc42 instances, etc.  (I'm pretty sure
I'll eventually want nvdimm to be autoloaded, it may want to depend on
the eeprom driver somehow in case I need to read the SPD data -- I'm
not sure yet.)

I guess the logic of "probe 0x50, then, if there's an eeprom there,
probe 0x18 for jc42" isn't perfect.  For example, lm90 can live at
0x18 or 0x19, and if there are DIMMs on the same segment as an lm90,
then it's possible for a jc42 and an lm90 to collide.  So if my bus
were I2C_CLASS_SPD | I2C_CLASS_DIMM, then perhaps the jc42 (and core)
code could probe more aggressively.

--Andy

[PATCH v3 2/4] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips

[Andy Lutomirski]

Sandy Bridge Xeon and Extreme chips have integrated memory controllers
with (rather limited) onboard SMBUS masters.  This driver gives access
to the bus.

Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
 drivers/i2c/busses/Kconfig   |  15 ++
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-imc.c | 548 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 564 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-imc.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e837f0e..7636782 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -137,6 +137,21 @@ config I2C_DIMM_BUS
        tristate
        default n
 
+config I2C_IMC
+	tristate "Intel iMC (LGA 2011) SMBus Controller"
+	depends on PCI && X86
+	select I2C_DIMM_BUS
+	help
+	  If you say yes to this option, support will be included for the Intel
+	  Integrated Memory Controller SMBus host controller interface.  This
+	  controller is found on LGA 2011 Xeons and Core i7 Extremes.
+
+	  It is possibly, although unlikely, that the use of this driver will
+	  interfere with your platform's RAM thermal management.
+
+	  This driver can also be built as a module.  If so, the module will be
+	  called i2c-imc.
+
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 226bb2e..790b63d 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
 obj-$(CONFIG_I2C_I801)		+= i2c-i801.o
 obj-$(CONFIG_I2C_ISCH)		+= i2c-isch.o
 obj-$(CONFIG_I2C_ISMT)		+= i2c-ismt.o
+obj-$(CONFIG_I2C_IMC)		+= i2c-imc.o
 obj-$(CONFIG_I2C_NFORCE2)	+= i2c-nforce2.o
 obj-$(CONFIG_I2C_NFORCE2_S4985)	+= i2c-nforce2-s4985.o
 obj-$(CONFIG_I2C_PIIX4)		+= i2c-piix4.o
diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c
new file mode 100644
index 0000000..9643aeb
--- /dev/null
+++ b/drivers/i2c/busses/i2c-imc.c
@@ -0,0 +1,548 @@
+/*
+ * Copyright (c) 2013 Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/i2c/dimm-bus.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/pm_qos.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/i2c.h>
+
+/*
+ * The datasheet can be found here, for example:
+ * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf
+ *
+ * There seem to be quite a few bugs or spec errors, though:
+ *
+ *  - A successful transaction sets WOD and RDO.
+ *
+ *  - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim)
+ *
+ *  - Erratum BT109, which says:
+ *
+ *      The processor may not complete SMBus (System Management Bus)
+ *      transactions targeting the TSOD (Temperature Sensor On DIMM)
+ *      when Package C-States are enabled. Due to this erratum, if the
+ *      processor transitions into a Package C-State while an SMBus
+ *      transaction with the TSOD is in process, the processor will
+ *      suspend receipt of the transaction. The transaction completes
+ *      while the processor is in a Package C-State.  Upon exiting
+ *      Package C-State, the processor will attempt to resume the
+ *      SMBus transaction, detect a protocol violation, and log an
+ *      error.
+ *
+ *   That description seems to be wrong.  If the package enters a
+ *   C-State while a software-initiated SMBUS transaction is running,
+ *   then the result of the transaction seems to be wrong, but no
+ *   error is signaled.  It does not seem to matter whether the
+ *   transaction is targeting a TSOD.
+ *
+ *   For added fun, the relevant bits in MSR_PKG_CST_CONFIG_CONTROL
+ *   are lockable, so we can't just disable package C-States.
+ *
+ *   The upshot is that we need to use a hack to keep a package awake
+ *   while we're using its SMBUS master.  pm_qos is the easiest, so we
+ *   use it for now.  It has the unfortunate side-effect that each
+ *   claim and each release will result in an IPI to all CPUs and
+ *   that, while any SMBUS transaction is running, the whole system
+ *   will be forced to a high-power state.
+ */
+
+/* Register offsets (in PCI configuration space) */
+#define SMBSTAT(i)			(0x180 + 0x10*i)
+#define SMBCMD(i)			(0x184 + 0x10*i)
+#define SMBCNTL(i)			(0x188 + 0x10*i)
+#define SMB_TSOD_POLL_RATE_CNTR(i)	(0x18C + 0x10*i)
+#define SMB_TSOD_POLL_RATE		(0x1A8)
+
+/* SMBSTAT fields */
+#define SMBSTAT_RDO		(1U << 31)	/* Read Data Valid */
+#define SMBSTAT_WOD		(1U << 30)	/* Write Operation Done */
+#define SMBSTAT_SBE		(1U << 29)	/* SMBus Error */
+#define SMBSTAT_SMB_BUSY	(1U << 28)	/* SMBus Busy State */
+/* 26:24 is the last automatically polled TSOD address */
+#define SMBSTAT_RDATA_MASK	0xffff		/* result of a read */
+
+/* SMBCMD fields */
+#define SMBCMD_TRIGGER		(1U << 31)	/* CMD Trigger */
+#define SMBCMD_PNTR_SEL		(1U << 30)	/* HW polls TSOD with pointer */
+#define SMBCMD_WORD_ACCESS	(1U << 29)	/* word (vs byte) access */
+#define SMBCMD_TYPE_MASK	(3U << 27)	/* Mask for access type */
+#define  SMBCMD_TYPE_READ	(0U << 27)	/* Read */
+#define  SMBCMD_TYPE_WRITE	(1U << 27)	/* Write */
+#define  SMBCMD_TYPE_PNTR_WRITE	(3U << 27)	/* Write to pointer */
+#define SMBCMD_SA_MASK		(7U << 24)	/* Slave Address high bits */
+#define SMBCMD_SA_SHIFT		24
+#define SMBCMD_BA_MASK		0xff0000	/* Bus Txn address */
+#define SMBCMD_BA_SHIFT		16
+#define SMBCMD_WDATA_MASK	0xffff		/* data to write */
+
+/* SMBCNTL fields */
+#define SMBCNTL_DTI_MASK	0xf0000000	/* Slave Address low bits */
+#define SMBCNTL_DTI_SHIFT	28		/* Slave Address low bits */
+#define SMBCNTL_CKOVRD		(1U << 27)	/* # Clock Override */
+#define SMBCNTL_DIS_WRT		(1U << 26)	/* Disable Write (sadly) */
+#define SMBCNTL_SOFT_RST	(1U << 10)	/* Soft Reset */
+#define SMBCNTL_TSOD_POLL_EN	(1U << 8)	/* TSOD Polling Enable */
+/* Bits 0-3 and 4-6 indicate TSOD presence in various slots */
+
+/* System Address Controller, PCI dev 13 fn 6, 8086.3cf5 */
+#define SAD_CONTROL 0xf4
+
+/*
+ * The clock is around 100kHz, and transactions are nine cycles per byte
+ * plus a few start/stop cycles, plus whatever clock streching is involved.
+ * This is a guess at the polling interval.
+ */
+
+#define TXN_LEN_US (20 * 10)
+
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_BR          0x3cf5  /* 13.6 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA      0x3ca8  /* 15.0 */
+
+struct imc_channel {
+	struct i2c_adapter adapter;
+	struct mutex mutex;
+	bool can_write, suspended;
+};
+
+struct imc_priv {
+	struct pci_dev *pci_dev;
+	struct imc_channel channels[2];
+};
+
+/*
+ * There's some cost to just having a pm_qos request installed, so we
+ * just use one globally instead of one per device.
+ */
+static struct pm_qos_request imc_pm_qos;
+static int imc_pm_qos_count;
+static DEFINE_MUTEX(imc_pm_qos_mutex);
+
+static void imc_pmqos_get(void)
+{
+	mutex_lock(&imc_pm_qos_mutex);
+	if (++imc_pm_qos_count == 1)
+		pm_qos_update_request(&imc_pm_qos, 5);
+	mutex_unlock(&imc_pm_qos_mutex);
+}
+
+static void imc_pmqos_put(void)
+{
+	mutex_lock(&imc_pm_qos_mutex);
+	if (--imc_pm_qos_count == 0)
+		pm_qos_update_request(&imc_pm_qos, PM_QOS_DEFAULT_VALUE);
+	mutex_unlock(&imc_pm_qos_mutex);
+}
+
+static void imc_channel_release(struct imc_priv *priv, int chan)
+{
+	/* Return to HW control. */
+	u32 cntl;
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
+	cntl |= SMBCNTL_TSOD_POLL_EN;
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl);
+	imc_pmqos_put();
+}
+
+static int imc_channel_claim(struct imc_priv *priv, int chan)
+{
+	/*
+	 * The docs are a bit confused here.  We're supposed to disable TSOD
+	 * polling, then wait for busy to be cleared, then set
+	 * SMBCNTL_TSOD_POLL_EN to zero to switch to software control.  But
+	 * SMBCNTL_TSOD_POLL_EN is the only documented way to turn off polling.
+	 */
+
+	u32 cntl, stat;
+	int i;
+
+	if (priv->channels[chan].suspended)
+		return -EIO;
+
+	imc_pmqos_get();
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
+	cntl &= ~SMBCNTL_TSOD_POLL_EN;
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl);
+
+	for (i = 0; i < 20; i++) {
+		pci_read_config_dword(priv->pci_dev, SMBSTAT(chan), &stat);
+		if (!(stat & SMBSTAT_SMB_BUSY))
+			return 0;  /* The channel is ours. */
+		usleep_range(TXN_LEN_US, 3*TXN_LEN_US);
+	}
+
+	/* We failed to take control of the channel.  Return to HW control. */
+	imc_channel_release(priv, chan);
+	imc_pmqos_put();
+	return -EBUSY;
+}
+
+/*
+ * The iMC supports five access types.  The terminology is rather
+ * inconsistent.  These are the types:
+ *
+ * "Write to pointer register SMBus": I2C_SMBUS_WRITE, I2C_SMBUS_BYTE
+ *
+ * Read byte/word: I2C_SMBUS_READ, I2C_SMBUS_{BYTE|WORD}_DATA
+ *
+ * Write byte/word: I2C_SMBUS_WRITE, I2C_SMBUS_{BYTE|WORD}_DATA
+ *
+ * The pointer write operations is AFAICT completely useless for
+ * software control, for two reasons.  First, HW periodically polls any
+ * TSODs on the bus, so it will corrupt the pointer in between SW
+ * transactions.  More importantly, the matching "read byte"/"receive
+ * byte" (the address-less single-byte read) is not available for SW
+ * control.  Therefore, this driver doesn't implement pointer writes
+ *
+ * There is no PEC support.
+ */
+
+static u32 imc_func(struct i2c_adapter *adapter)
+{
+	int chan;
+	struct imc_channel *ch;
+	struct imc_priv *priv = i2c_get_adapdata(adapter);
+
+	chan = (adapter == &priv->channels[0].adapter ? 0 : 1);
+	ch = &priv->channels[chan];
+
+	if (ch->can_write)
+		return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA;
+	else
+		return I2C_FUNC_SMBUS_READ_BYTE_DATA |
+			I2C_FUNC_SMBUS_READ_WORD_DATA;
+}
+
+static s32 imc_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+			  unsigned short flags, char read_write, u8 command,
+			  int size, union i2c_smbus_data *data)
+{
+	int i, ret, chan;
+	u32 tmp, cmdbits = 0, cntlbits = 0, stat;
+	struct imc_channel *ch;
+	struct imc_priv *priv = i2c_get_adapdata(adap);
+
+	chan = (adap == &priv->channels[0].adapter ? 0 : 1);
+	ch = &priv->channels[chan];
+
+	if (addr > 0x7f)
+		return -EOPNOTSUPP;  /* No large address support */
+	if (flags)
+		return -EOPNOTSUPP;  /* No PEC */
+
+	cmdbits  |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;
+	cntlbits |= ((u32)addr >> 3) << SMBCNTL_DTI_SHIFT;
+
+	switch (size) {
+	case I2C_SMBUS_BYTE_DATA:
+		cmdbits |= ((u32)command) << SMBCMD_BA_SHIFT;
+		if (read_write == I2C_SMBUS_READ)
+			cmdbits |= SMBCMD_TYPE_READ;
+		else
+			cmdbits |= SMBCMD_TYPE_WRITE | data->byte;
+		break;
+	case I2C_SMBUS_WORD_DATA:
+		cmdbits |= ((u32)command) << SMBCMD_BA_SHIFT;
+		cmdbits |= SMBCMD_WORD_ACCESS;
+		if (read_write == I2C_SMBUS_READ)
+			cmdbits |= SMBCMD_TYPE_READ;
+		else
+			cmdbits |= SMBCMD_TYPE_WRITE | swab16(data->word);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	mutex_lock(&ch->mutex);
+
+	ret = imc_channel_claim(priv, chan);
+	if (ret)
+		goto out_unlock;
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &tmp);
+	tmp &= ~SMBCNTL_DTI_MASK;
+	tmp |= cntlbits;
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), tmp);
+
+	/*
+	 * This clears SMBCMD_PNTR_SEL.  We leave it cleared so that we don't
+	 * need to think about keeping the TSOD pointer state consistent with
+	 * the hardware's expectation.  This probably has some miniscule
+	 * power cost, as TSOD polls will take 9 extra cycles.
+	 */
+	cmdbits |= SMBCMD_TRIGGER;
+	pci_write_config_dword(priv->pci_dev, SMBCMD(chan), cmdbits);
+
+	for (i = 0; ; i++) {
+		pci_read_config_dword(priv->pci_dev, SMBSTAT(chan), &stat);
+		if (!(stat & SMBSTAT_SMB_BUSY))
+			break;
+		if (i < 50) {
+			usleep_range(TXN_LEN_US, 3*TXN_LEN_US);
+			continue;
+		}
+
+		/* Timeout.  TODO: Reset the controller? */
+		ret = -EIO;
+		dev_err(&priv->pci_dev->dev, "controller is wedged\n");
+		goto out_release;
+	}
+
+	if (stat & SMBSTAT_SBE) {
+		/*
+		 * Clear the error to re-enable TSOD polling.  The docs say
+		 * that, as long as SBE is set, TSOD polling won't happen.
+		 * The docs also say that writing zero to this bit (which is
+		 * the only writable bit in the whole register) will clear
+		 * the error.  Empirically, writing 0 does not clear SBE, but
+		 * it's probably still good to do the write in compliance with
+		 * the spec.  (TSOD polling still happens and seems to
+		 * clear SBE on its own.)
+		 */
+		pci_write_config_dword(priv->pci_dev, SMBSTAT(chan), 0);
+		ret = -ENXIO;
+		goto out_release;
+	}
+
+	if (read_write == I2C_SMBUS_READ) {
+		if (stat & SMBSTAT_RDO) {
+			/*
+			 * The iMC SMBUS controller thinks of SMBUS
+			 * words as being big-endian (MSB first).
+			 * Linux treats them as little-endian, we need
+			 * to swap them.
+			 *
+			 * Note: the controller will often (always?) set
+			 * WOD here.  This is probably a bug.
+			 */
+			if (size == I2C_SMBUS_WORD_DATA)
+				data->word = swab16(stat & SMBSTAT_RDATA_MASK);
+			else
+				data->byte = stat & 0xFF;
+			ret = 0;
+		} else {
+			dev_err(&priv->pci_dev->dev,
+				"Unexpected read status 0x%08X\n", stat);
+			ret = -EIO;
+		}
+	} else {
+		if (stat & SMBSTAT_WOD) {
+			/* Same bug (?) as in the read case. */
+			ret = 0;
+		} else {
+			dev_err(&priv->pci_dev->dev,
+				"Unexpected write status 0x%08X\n", stat);
+			ret = -EIO;
+		}
+	}
+
+out_release:
+	imc_channel_release(priv, chan);
+
+out_unlock:
+	mutex_unlock(&ch->mutex);
+
+	return ret;
+}
+
+static const struct i2c_algorithm imc_smbus_algorithm = {
+	.smbus_xfer	= imc_smbus_xfer,
+	.functionality	= imc_func,
+};
+
+static int imc_init_channel(struct imc_priv *priv, int i, int socket)
+{
+	int err;
+	u32 val;
+	struct imc_channel *ch = &priv->channels[i];
+
+	i2c_set_adapdata(&ch->adapter, priv);
+	ch->adapter.owner = THIS_MODULE;
+	ch->adapter.algo = &imc_smbus_algorithm;
+	ch->adapter.dev.parent = &priv->pci_dev->dev;
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(i), &val);
+	ch->can_write = !(val & SMBCNTL_DIS_WRT);
+
+	/*
+	 * i2c_add_addapter can call imc_smbus_xfer, so we need to be
+	 * ready immediately.
+	 */
+	mutex_init(&ch->mutex);
+
+	snprintf(ch->adapter.name, sizeof(ch->adapter.name),
+		 "iMC socket %d channel %d", socket, i);
+	err = i2c_add_adapter(&ch->adapter);
+	if (err) {
+		mutex_destroy(&ch->mutex);
+		return err;
+	}
+
+	i2c_scan_dimm_bus(&ch->adapter);
+
+	return 0;
+}
+
+static void imc_free_channel(struct imc_priv *priv, int i)
+{
+	struct imc_channel *ch = &priv->channels[i];
+
+	mutex_lock(&ch->mutex);
+	i2c_del_adapter(&ch->adapter);
+	mutex_unlock(&ch->mutex);
+	mutex_destroy(&ch->mutex);
+}
+
+static int imc_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	int i, err;
+	struct imc_priv *priv;
+	struct pci_dev *sad;  /* System Address Decoder */
+	u32 sad_control;
+
+	/* Paranoia: the datasheet says this is always at 15.0 */
+	if (dev->devfn != PCI_DEVFN(15, 0))
+		return -ENODEV;
+
+	/*
+	 * The socket number is hidden away on a different PCI device.
+	 * There's another copy at devfn 11.0 offset 0x40, and an even
+	 * less convincing copy at 5.0 0x140.  The actual APICID register
+	 * is "not used ... and is still implemented in hardware because
+	 * of FUD".
+	 *
+	 * In principle we could double-check that the socket matches
+	 * the numa_node from SRAT, but this is probably not worth it.
+	 */
+	sad = pci_get_slot(dev->bus, PCI_DEVFN(13, 6));
+	if (!sad)
+		return -ENODEV;
+	if (sad->vendor != PCI_VENDOR_ID_INTEL ||
+	    sad->device != PCI_DEVICE_ID_INTEL_SBRIDGE_BR) {
+		pci_dev_put(sad);
+		return -ENODEV;
+	}
+	pci_read_config_dword(sad, SAD_CONTROL, &sad_control);
+	pci_dev_put(sad);
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->pci_dev = dev;
+
+	pci_set_drvdata(dev, priv);
+
+	for (i = 0; i < 2; i++) {
+		int j;
+		err = imc_init_channel(priv, i, sad_control & 0x7);
+		if (err) {
+			for (j = 0; j < i; j++)
+				imc_free_channel(priv, j);
+			goto exit_free;
+		}
+	}
+
+	return 0;
+
+exit_free:
+	kfree(priv);
+	return err;
+}
+
+static void imc_remove(struct pci_dev *dev)
+{
+	int i;
+	struct imc_priv *priv = pci_get_drvdata(dev);
+
+	for (i = 0; i < 2; i++)
+		imc_free_channel(priv, i);
+
+	kfree(priv);
+}
+
+static int imc_suspend(struct pci_dev *dev, pm_message_t mesg)
+{
+	int i;
+	struct imc_priv *priv = pci_get_drvdata(dev);
+
+	/* BIOS is in charge.  We should finish any pending transaction */
+	for (i = 0; i < 2; i++) {
+		mutex_lock(&priv->channels[i].mutex);
+		priv->channels[i].suspended = true;
+		mutex_unlock(&priv->channels[i].mutex);
+	}
+
+	return 0;
+}
+
+static int imc_resume(struct pci_dev *dev)
+{
+	int i;
+	struct imc_priv *priv = pci_get_drvdata(dev);
+
+	for (i = 0; i < 2; i++) {
+		mutex_lock(&priv->channels[i].mutex);
+		priv->channels[i].suspended = false;
+		mutex_unlock(&priv->channels[i].mutex);
+	}
+
+	return 0;
+}
+
+static DEFINE_PCI_DEVICE_TABLE(imc_ids) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA) },
+	{ 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, imc_ids);
+
+static struct pci_driver imc_pci_driver = {
+	.name		= "imc_smbus",
+	.id_table	= imc_ids,
+	.probe		= imc_probe,
+	.remove		= imc_remove,
+	.suspend	= imc_suspend,
+	.resume		= imc_resume,
+};
+
+static int __init imc_init(void)
+{
+	int err;
+
+	pm_qos_add_request(&imc_pm_qos,
+			   PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
+	err = pci_register_driver(&imc_pci_driver);
+	if (err)
+		pm_qos_remove_request(&imc_pm_qos);
+	return err;
+}
+
+static void __exit imc_exit(void)
+{
+	pci_unregister_driver(&imc_pci_driver);
+	pm_qos_remove_request(&imc_pm_qos);
+}
+
+module_init(imc_init);
+module_exit(imc_exit);
+
+MODULE_AUTHOR("Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>");
+MODULE_DESCRIPTION("iMC SMBus driver");
+MODULE_LICENSE("GPL");
-- 
1.8.1.4

[PATCH v3 3/4] tsod: New hwmon driver for Temperature Sensors on DIMM

[Andy Lutomirski]

Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
 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

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 89ac1cb..96edb87 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8d6d97e..439ef1f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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/
 
diff --git a/drivers/hwmon/tsod.c b/drivers/hwmon/tsod.c
new file mode 100644
index 0000000..f7bb070
--- /dev/null
+++ b/drivers/hwmon/tsod.c
@@ -0,0 +1,195 @@
+/*
+ * drivers/hwmon/tsod.c - Temperaure Sensor On DIMM
+ *
+ * Copyright (C) 2013 Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
+ *
+ * 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 <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>");
+MODULE_DESCRIPTION("Temperaure Sensor On DIMM");
+MODULE_LICENSE("GPL");
-- 
1.8.1.4

Re: [PATCH v3 3/4] tsod: New hwmon driver for Temperature Sensors on DIMM

[Guenter Roeck]

On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote:
> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> ---

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

Guenter

>  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
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 89ac1cb..96edb87 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -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"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8d6d97e..439ef1f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -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/
>  
> diff --git a/drivers/hwmon/tsod.c b/drivers/hwmon/tsod.c
> new file mode 100644
> index 0000000..f7bb070
> --- /dev/null
> +++ b/drivers/hwmon/tsod.c
> @@ -0,0 +1,195 @@
> +/*
> + * drivers/hwmon/tsod.c - Temperaure Sensor On DIMM
> + *
> + * Copyright (C) 2013 Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> + *
> + * 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 <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>");
> +MODULE_DESCRIPTION("Temperaure Sensor On DIMM");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.1.4
> 
> 

Re: [PATCH v3 3/4] tsod: New hwmon driver for Temperature Sensors on DIMM

[Andy Lutomirski]

On Wed, Jul 17, 2013 at 3:19 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote:
>> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>> ---
>
> 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?

In any case, am I doing the right thing by not setting I2C_CLASS_SPD on my bus?

--Andy

Re: [PATCH v3 3/4] tsod: New hwmon driver for Temperature Sensors on DIMM

[Guenter Roeck]

On Wed, Jul 17, 2013 at 03:49:24PM -0700, Andy Lutomirski wrote:
> On Wed, Jul 17, 2013 at 3:19 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> > On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote:
> >> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> >> ---
> >
> > 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.)
> 
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.

> 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.

> 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.

Thanks,
Guenter

Re: [PATCH v3 3/4] tsod: New hwmon driver for Temperature Sensors on DIMM

[Andy Lutomirski]

On Wed, Jul 17, 2013 at 4:09 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On Wed, Jul 17, 2013 at 03:49:24PM -0700, Andy Lutomirski wrote:
>> On Wed, Jul 17, 2013 at 3:19 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> > On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote:
>> >> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>> >> ---
>> >
>> > 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.)
>>
> 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.

Fair enough.

>
>> 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 ?

I didn't set I2C_CLASS_HWMON (and I won't -- jc42 is the only hwmon
driver that should ever probe this bus).  The adapter only supports
read byte/word data, write byte/word data, and write byte.  (I'm
pretty sure that the hardware is capable of initiating a read byte
transaction, but not under software control.  I have no idea why write
byte is there without read byte being available, so I didn't implement
it.  I think it's so that you can reset the pointer register after
reading something else, but this is pointless because you don't know
what to reset it to.  This hardware is not exactly a thing of beauty,
but I'm stuck with it.)

>
> 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.

See my other post about dimm_bus.

>
>> 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.
>

OK, I'm convinced.  I'll scrap my tsod driver.

--Andy

[PATCH v3 4/4] eeprom: Add a MODULE_DEVICE_TABLE

[Andy Lutomirski]

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

Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
 drivers/misc/eeprom/eeprom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/eeprom/eeprom.c b/drivers/misc/eeprom/eeprom.c
index c169e07..1ec85b8 100644
--- a/drivers/misc/eeprom/eeprom.c
+++ b/drivers/misc/eeprom/eeprom.c
@@ -215,6 +215,7 @@ static const struct i2c_device_id eeprom_id[] = {
 	{ "eeprom", 0 },
 	{ }
 };
+MODULE_DEVICE_TABLE(i2c, eeprom_id);
 
 static struct i2c_driver eeprom_driver = {
 	.driver = {
-- 
1.8.1.4

Re: [PATCH v3 4/4] eeprom: Add a MODULE_DEVICE_TABLE

[Jean Delvare]

Hi Andy,

On Wed, 17 Jul 2013 13:53:08 -0700, Andy Lutomirski wrote:
> The new DIMM-bus code can detect eeproms, so give the eeprom driver
> an appropriate modalias.
> 
> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> ---
>  drivers/misc/eeprom/eeprom.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/eeprom/eeprom.c b/drivers/misc/eeprom/eeprom.c
> index c169e07..1ec85b8 100644
> --- a/drivers/misc/eeprom/eeprom.c
> +++ b/drivers/misc/eeprom/eeprom.c
> @@ -215,6 +215,7 @@ static const struct i2c_device_id eeprom_id[] = {
>  	{ "eeprom", 0 },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(i2c, eeprom_id);
>  
>  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

Re: [PATCH v3 4/4] eeprom: Add a MODULE_DEVICE_TABLE

[Andy Lutomirski]

On Thu, Jul 18, 2013 at 12:11 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Andy,
>
> On Wed, 17 Jul 2013 13:53:08 -0700, Andy Lutomirski wrote:
>> The new DIMM-bus code can detect eeproms, so give the eeprom driver
>> an appropriate modalias.
>>
>> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>> ---
>>  drivers/misc/eeprom/eeprom.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/misc/eeprom/eeprom.c b/drivers/misc/eeprom/eeprom.c
>> index c169e07..1ec85b8 100644
>> --- a/drivers/misc/eeprom/eeprom.c
>> +++ b/drivers/misc/eeprom/eeprom.c
>> @@ -215,6 +215,7 @@ static const struct i2c_device_id eeprom_id[] = {
>>       { "eeprom", 0 },
>>       { }
>>  };
>> +MODULE_DEVICE_TABLE(i2c, eeprom_id);
>>
>>  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.

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.)

--Andy

>
> Thanks,
> --
> Jean Delvare



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH v3 4/4] eeprom: Add a MODULE_DEVICE_TABLE

[Jean Delvare]

Hi Andy,

On Thu, 18 Jul 2013 09:15:47 -0700, Andy Lutomirski wrote:
> 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.

> 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.)

> (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.

-- 
Jean Delvare

Re: [PATCH v3 4/4] eeprom: Add a MODULE_DEVICE_TABLE

[Andy Lutomirski]

On Thu, Jul 18, 2013 at 1:31 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Thu, 18 Jul 2013 09:15:47 -0700, Andy Lutomirski wrote:
>> 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.
>
>> 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.)
>

Can you take a look at patch v4 1/2 and let me know if you think it's
any good?  It does more or less that, albeit using i2c_new_device and
a bit fancier logic.  It's also factored out to make it (in theory)
reusable by any other bus with similar properties, but I can easily
move it somewhere else.

http://news.gmane.org/find-root.php?message_id=%3c0087fdcb080b40f5eaf3abbfd35ee107ca713d52.1374171757.git.luto%40amacapital.net%3e

(And apologies for my newbishness in poking around the i2c code.)

>> (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.

Intel almost did it.  They give status bits indicating whether they
(where "they" means either BIOS or the chipset -- I'm not sure) think
that a given slot contains a TSOD.  I think that those bits actually
control the periodic hardware TSOD polling.  On my i7 Extreme, they're
all zero.

I suppose that one could query the EDAC infrastructure to see which
slots are mapped to which RAM addresses, but that would be a giant
mess.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC