[PATCH v6 0/4] iMC SMBUS driver and DIMM bus probing

[PATCH v6 0/4] iMC SMBUS driver and DIMM bus probing

[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)
 - Other interesting things, with drivers hopefully to follow...

This patch series adds a simple generic layer for probing for DIMMs over
SMBUS and an i2c bus driver for the iMC controller found on Intel
LGA2011 chips.

It now uses only modern infrastructure -- new-style I2C probing and the
at24 (instead of eeprom) driver.

I've tested this on a Core i7 Extreme and on a Xeon E5 server.  I haven't
tested it on an LGA2011 Ivy Bridge box, since I don't have one.

Patches 1-3 are useful even without patch 4.  I'm still hoping for feedback
on patch 4.

Changes from v5:
 - Rebase to a6ddeee32dad (i.e. -linus from today) and re-test

Changes from v4:
 - Added the sb_edac changes -- i2c_imc and sb_edac can now coexist
 - Added some paranoid race detection.
 - The driver now confirms its ability to claim software control of the SMBUS
   master.  This prevents unpleasant problems on systems that enable CLTT
   (closed loop thermal throttling).
 - Reordered the patches so that the DIMM bus code is last.

Changes from v3:
 - Dropped redundant "tsod" driver
 - Dropped eeprom modalias
 - Switched to probing for the "eeprom" and "jc42"

Andy Lutomirski (4):
  Move Intel SNB device ids from sb_edac to pci_ids.h
  sb_edac: Claim a different PCI device
  i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
  i2c, i2c_imc: Add DIMM bus code

 drivers/edac/sb_edac.c        |  32 +--
 drivers/i2c/busses/Kconfig    |  19 ++
 drivers/i2c/busses/Makefile   |   5 +
 drivers/i2c/busses/dimm-bus.c |  97 ++++++++
 drivers/i2c/busses/i2c-imc.c  | 562 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/dimm-bus.h  |  24 ++
 include/linux/pci_ids.h       |  15 ++
 7 files changed, 723 insertions(+), 31 deletions(-)
 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.3.1

[PATCH v6 1/4] Move Intel SNB device ids from sb_edac to pci_ids.h

[Andy Lutomirski]

The i2c_imc driver will use two of them, and moving only part of
the list seems messier.

Cc: Mauro Carvalho Chehab <m.chehab-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Rui Wang <ruiv.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
 drivers/edac/sb_edac.c  | 30 ------------------------------
 include/linux/pci_ids.h | 15 +++++++++++++++
 2 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index d7f1b57..06426f6 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -52,36 +52,6 @@ static int probed;
 #define GET_BITFIELD(v, lo, hi)	\
 	(((v) & GENMASK_ULL(hi, lo)) >> (lo))
 
-/*
- * sbridge Memory Controller Registers
- */
-
-/*
- * FIXME: For now, let's order by device function, as it makes
- * easier for driver's development process. This table should be
- * moved to pci_id.h when submitted upstream
- */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_SAD0	0x3cf4	/* 12.6 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_SAD1	0x3cf6	/* 12.7 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_BR		0x3cf5	/* 13.6 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_HA0	0x3ca0	/* 14.0 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA	0x3ca8	/* 15.0 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_RAS	0x3c71	/* 15.1 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD0	0x3caa	/* 15.2 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD1	0x3cab	/* 15.3 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD2	0x3cac	/* 15.4 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD3	0x3cad	/* 15.5 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_DDRIO	0x3cb8	/* 17.0 */
-
-	/*
-	 * Currently, unused, but will be needed in the future
-	 * implementations, as they hold the error counters
-	 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR0	0x3c72	/* 16.2 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR1	0x3c73	/* 16.3 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR2	0x3c76	/* 16.6 */
-#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR3	0x3c77	/* 16.7 */
-
 /* Devices 12 Function 6, Offsets 0x80 to 0xcc */
 static const u32 sbridge_dram_rule[] = {
 	0x80, 0x88, 0x90, 0x98, 0xa0,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 97fbecd..6dc4b18 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2812,7 +2812,22 @@
 #define PCI_DEVICE_ID_INTEL_UNC_R2PCIE	0x3c43
 #define PCI_DEVICE_ID_INTEL_UNC_R3QPI0	0x3c44
 #define PCI_DEVICE_ID_INTEL_UNC_R3QPI1	0x3c45
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_RAS	0x3c71	/* 15.1 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR0	0x3c72	/* 16.2 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR1	0x3c73	/* 16.3 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR2	0x3c76	/* 16.6 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_ERR3	0x3c77	/* 16.7 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_HA0	0x3ca0	/* 14.0 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA	0x3ca8	/* 15.0 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD0	0x3caa	/* 15.2 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD1	0x3cab	/* 15.3 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD2	0x3cac	/* 15.4 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD3	0x3cad	/* 15.5 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_DDRIO	0x3cb8	/* 17.0 */
 #define PCI_DEVICE_ID_INTEL_JAKETOWN_UBOX	0x3ce0
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_SAD0	0x3cf4	/* 12.6 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_BR		0x3cf5	/* 13.6 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_SAD1	0x3cf6	/* 12.7 */
 #define PCI_DEVICE_ID_INTEL_IOAT_SNB	0x402f
 #define PCI_DEVICE_ID_INTEL_5100_16	0x65f0
 #define PCI_DEVICE_ID_INTEL_5100_19	0x65f3
-- 
1.8.3.1

[PATCH v6 2/4] sb_edac: Claim a different PCI device

[Andy Lutomirski]

sb_edac controls a large number of different PCI functions.  Rather
than registering as a normal PCI driver for all of them, it
registers for just one so that it gets probed and, at probe time, it
looks for all the others.

Coincidentally, the device it registers for also contains the SMBUS
registers, so the PCI core will refuse to probe both sb_edac and an
iMC SMBUS driver.  The drivers don't actually conflict, so just
change sb_edac's device table to probe a different device.

An alternative fix would be to merge the two drivers, but sb_edac
will also refuse to load on non-ECC systems, whereas the i2c_imc
is still useful without ECC.

The only user-visible change should be that sb_edac appears to bind
a different device.

Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: Rui Wang <ruiv.wang@gmail.com>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/edac/sb_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 06426f6..26e8ea9 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -432,7 +432,7 @@ static const struct pci_id_table pci_dev_descr_ibridge_table[] = {
  *	pci_device_id	table for which devices we are looking for
  */
 static DEFINE_PCI_DEVICE_TABLE(sbridge_pci_tbl) = {
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA)},
+	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_HA0)},
 	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA0_TA)},
 	{0,}			/* 0 terminated list. */
 };
-- 
1.8.3.1

[PATCH v6 3/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@amacapital.net>
---
 drivers/i2c/busses/Kconfig   |  15 ++
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-imc.c | 559 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 575 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-imc.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 3b26129..0fe76cc 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -135,6 +135,21 @@ config I2C_ISMT
 	  This driver can also be built as a module.  If so, the module will be
 	  called i2c-ismt.
 
+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 c73eb0e..ea2a6a5 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..c846077
--- /dev/null
+++ b/drivers/i2c/busses/i2c-imc.c
@@ -0,0 +1,559 @@
+/*
+ * Copyright (c) 2013 Andrew Lutomirski <luto@amacapital.net>
+ *
+ * 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/module.h>
+#include <linux/kernel.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.
+ *
+ *   The description notwithstanding, I've seen difficult-to-reproduce
+ *   issues when the system goes completely idle (so package C-states can
+ *   be entered) while software-initiated SMBUS transactions are in
+ *   progress.
+ */
+
+/* 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 */
+
+/* Bits that might randomly change if we race with something. */
+#define SMBCMD_OUR_BITS		(~(u32)SMBCMD_TRIGGER)
+#define SMBCNTL_OUR_BITS	(SMBCNTL_DTI_MASK | SMBCNTL_TSOD_POLL_EN)
+
+/* System Address Controller, PCI dev 13 fn 6, 8086.3cf5 */
+#define SAD_CONTROL 0xf4
+
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_BR          0x3cf5  /* 13.6 */
+#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA      0x3ca8  /* 15.0 */
+
+static atomic_t imc_raced;  /* Set permanently to 1 if we screw up. */
+
+struct imc_channel {
+	struct i2c_adapter adapter;
+	struct mutex mutex;
+	bool can_write, suspended;
+	bool prev_tsod_poll;
+};
+
+struct imc_priv {
+	struct pci_dev *pci_dev;
+	struct imc_channel channels[2];
+};
+
+static int imc_wait_not_busy(struct imc_priv *priv, int chan, u32 *stat)
+{
+	/*
+	 * 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 means that polling every 70us
+	 * or so will give decent performance.
+	 *
+	 * Ideally we would calculate a good estimate for the
+	 * transaction time and sleep, but busy-waiting is an effective
+	 * workaround for an apparent Sandy Bridge bug that causes bogus
+	 * output if the system enters a package C-state.  (NB: these
+	 * states are systemwide -- we don't need be running on the
+	 * right package for this to work.)
+	 */
+
+	int i;
+	for (i = 0; i < 50; i++) {
+		pci_read_config_dword(priv->pci_dev, SMBSTAT(chan), stat);
+		if (!(*stat & SMBSTAT_SMB_BUSY))
+			return 0;
+		udelay(70);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static void imc_channel_release(struct imc_priv *priv, int chan)
+{
+	/* Return to HW control. */
+	if (priv->channels[chan].prev_tsod_poll) {
+		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);
+	}
+}
+
+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;
+
+	if (priv->channels[chan].suspended)
+		return -EIO;
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
+	priv->channels[chan].prev_tsod_poll = !!(cntl & SMBCNTL_TSOD_POLL_EN);
+	cntl &= ~SMBCNTL_TSOD_POLL_EN;
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl);
+
+	/* Sometimes the hardware won't let go. */
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
+	if (cntl & SMBCNTL_TSOD_POLL_EN)
+		return -EBUSY;
+
+	if (imc_wait_not_busy(priv, chan, &stat) != 0) {
+		imc_channel_release(priv, chan);
+		return -EBUSY;
+	}
+
+	return 0;  /* The channel is ours. */
+}
+
+static bool imc_channel_can_claim(struct imc_priv *priv, int chan)
+{
+	u32 orig_cntl, cntl;
+
+	/* See if we can turn off TSOD_POLL_EN. */
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &orig_cntl);
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan),
+			       orig_cntl & ~SMBCNTL_TSOD_POLL_EN);
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
+	if (cntl & SMBCNTL_TSOD_POLL_EN)
+		return false;  /* Failed. */
+
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), orig_cntl);
+	return true;
+}
+
+/*
+ * 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 ret, chan;
+	u32 cmd = 0, cntl, final_cmd, final_cntl, stat;
+	struct imc_channel *ch;
+	struct imc_priv *priv = i2c_get_adapdata(adap);
+
+	if (atomic_read(&imc_raced))
+		return -EIO;  /* Minimize damage. */
+
+	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 */
+	if (size != I2C_SMBUS_BYTE_DATA && size != I2C_SMBUS_WORD_DATA)
+		return -EOPNOTSUPP;
+
+	/* Encode CMD part of addresses and access size */
+	cmd  |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;
+	cmd |= ((u32)command) << SMBCMD_BA_SHIFT;
+	if (size == I2C_SMBUS_WORD_DATA)
+		cmd |= SMBCMD_WORD_ACCESS;
+
+	/* Encode read/write and data to write */
+	if (read_write == I2C_SMBUS_READ) {
+		cmd |= SMBCMD_TYPE_READ;
+	} else {
+		cmd |= SMBCMD_TYPE_WRITE;
+		cmd |= (size == I2C_SMBUS_WORD_DATA
+			    ? swab16(data->word)
+			    : data->byte);
+	}
+
+	mutex_lock(&ch->mutex);
+
+	ret = imc_channel_claim(priv, chan);
+	if (ret)
+		goto out_unlock;
+
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
+	cntl &= ~SMBCNTL_DTI_MASK;
+	cntl |= ((u32)addr >> 3) << SMBCNTL_DTI_SHIFT;
+	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl);
+
+	/*
+	 * 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.
+	 */
+	cmd |= SMBCMD_TRIGGER;
+	pci_write_config_dword(priv->pci_dev, SMBCMD(chan), cmd);
+
+	if (imc_wait_not_busy(priv, chan, &stat) != 0) {
+		/* Timeout.  TODO: Reset the controller? */
+		ret = -EIO;
+		dev_err(&priv->pci_dev->dev, "controller is wedged\n");
+		goto out_release;
+	}
+
+	/*
+	 * Be paranoid: try to detect races.  This will only detect races
+	 * against BIOS, not against hardware.  (I've never seen this happen.)
+	 */
+	pci_read_config_dword(priv->pci_dev, SMBCMD(chan), &final_cmd);
+	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &final_cntl);
+	if (((cmd ^ final_cmd) & SMBCMD_OUR_BITS) ||
+	    ((cntl ^ final_cntl) & SMBCNTL_OUR_BITS)) {
+		WARN(1, "iMC SMBUS raced against firmware");
+		dev_emerg(&priv->pci_dev->dev,
+			  "Access to channel %d raced: cmd 0x%08X->0x%08X, cntl 0x%08X->0x%08X\n",
+			  chan, cmd, final_cmd, cntl, final_cntl);
+		atomic_set(&imc_raced, 1);
+		ret = -EIO;
+		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, so 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];
+
+	/*
+	 * With CLTT enabled, the hardware won't let us turn
+	 * off TSOD polling.  The device is completely useless
+	 * when this happens (at least without help from Intel),
+	 * but we can at least minimize confusion.
+	 */
+	if (!imc_channel_can_claim(priv, i)) {
+		dev_warn(&priv->pci_dev->dev,
+			 "iMC channel %d: we cannot control the HW.  Is CLTT on?\n",
+			 i);
+		return -EBUSY;
+	}
+
+	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;
+	}
+
+	return 0;
+}
+
+static void imc_free_channel(struct imc_priv *priv, int i)
+{
+	struct imc_channel *ch = &priv->channels[i];
+
+	/* This can recurse into imc_smbus_xfer. */
+	i2c_del_adapter(&ch->adapter);
+
+	mutex_destroy(&ch->mutex);
+}
+
+static struct pci_dev *imc_get_related_device(struct pci_bus *bus, unsigned int devfn, u16 devid)
+{
+	struct pci_dev *ret = pci_get_slot(bus, devfn);
+	if (!ret)
+		return NULL;
+	if (ret->vendor != PCI_VENDOR_ID_INTEL || ret->device != devid) {
+		pci_dev_put(ret);
+		return NULL;
+	}
+	return ret;
+}
+
+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 = imc_get_related_device(dev->bus, PCI_DEVFN(13, 6),
+				     PCI_DEVICE_ID_INTEL_SBRIDGE_BR);
+	if (!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,
+};
+module_pci_driver(imc_pci_driver);
+
+MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>");
+MODULE_DESCRIPTION("iMC SMBus driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

Re: [PATCH v6 3/4] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips

[Wolfram Sang]

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


> +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 sounds scary and thus needs some more detail :) How does that show?
What can happen? Anything to take care of?

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

This is not perfectly sorted.

>  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..c846077
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-imc.c
> @@ -0,0 +1,559 @@
> +/*
> + * 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.

Please skip the address. And run checkpatch.pl --strict! It would have
warned you about this and other stuff:

total: 1 errors, 3 warnings, 2 checks, 587 lines checked


> +/*
> + * 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:

Kudos for the useful comments!

> +/* 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)

Spaces around operators!

> +	int i;
> +	for (i = 0; i < 50; i++) {
> +		pci_read_config_dword(priv->pci_dev, SMBSTAT(chan), stat);
> +		if (!(*stat & SMBSTAT_SMB_BUSY))
> +			return 0;
> +		udelay(70);

usleep_range?

> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +

...

> +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 ret, chan;
> +	u32 cmd = 0, cntl, final_cmd, final_cntl, stat;
> +	struct imc_channel *ch;
> +	struct imc_priv *priv = i2c_get_adapdata(adap);
> +
> +	if (atomic_read(&imc_raced))
> +		return -EIO;  /* Minimize damage. */
> +
> +	chan = (adap == &priv->channels[0].adapter ? 0 : 1);
> +	ch = &priv->channels[chan];
> +
> +	if (addr > 0x7f)
> +		return -EOPNOTSUPP;  /* No large address support */

Unneeded. The core checks for that...

> +	if (flags)
> +		return -EOPNOTSUPP;  /* No PEC */

... and your code would bail out if I2C_M_TEN was set.

> +	if (size != I2C_SMBUS_BYTE_DATA && size != I2C_SMBUS_WORD_DATA)
> +		return -EOPNOTSUPP;
> +
> +	/* Encode CMD part of addresses and access size */
> +	cmd  |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;
> +	cmd |= ((u32)command) << SMBCMD_BA_SHIFT;
> +	if (size == I2C_SMBUS_WORD_DATA)
> +		cmd |= SMBCMD_WORD_ACCESS;
> +
> +	/* Encode read/write and data to write */
> +	if (read_write == I2C_SMBUS_READ) {
> +		cmd |= SMBCMD_TYPE_READ;
> +	} else {
> +		cmd |= SMBCMD_TYPE_WRITE;
> +		cmd |= (size == I2C_SMBUS_WORD_DATA
> +			    ? swab16(data->word)
> +			    : data->byte);
> +	}
> +
> +	mutex_lock(&ch->mutex);

Why is the per-adapter lock not sufficient?

> +
> +	ret = imc_channel_claim(priv, chan);
> +	if (ret)
> +		goto out_unlock;
> +
> +	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
> +	cntl &= ~SMBCNTL_DTI_MASK;
> +	cntl |= ((u32)addr >> 3) << SMBCNTL_DTI_SHIFT;
> +	pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl);
> +
> +	/*
> +	 * 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.
> +	 */
> +	cmd |= SMBCMD_TRIGGER;
> +	pci_write_config_dword(priv->pci_dev, SMBCMD(chan), cmd);
> +
> +	if (imc_wait_not_busy(priv, chan, &stat) != 0) {
> +		/* Timeout.  TODO: Reset the controller? */
> +		ret = -EIO;
> +		dev_err(&priv->pci_dev->dev, "controller is wedged\n");
> +		goto out_release;
> +	}
> +
> +	/*
> +	 * Be paranoid: try to detect races.  This will only detect races
> +	 * against BIOS, not against hardware.  (I've never seen this happen.)
> +	 */
> +	pci_read_config_dword(priv->pci_dev, SMBCMD(chan), &final_cmd);
> +	pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &final_cntl);
> +	if (((cmd ^ final_cmd) & SMBCMD_OUR_BITS) ||
> +	    ((cntl ^ final_cntl) & SMBCNTL_OUR_BITS)) {
> +		WARN(1, "iMC SMBUS raced against firmware");
> +		dev_emerg(&priv->pci_dev->dev,
> +			  "Access to channel %d raced: cmd 0x%08X->0x%08X, cntl 0x%08X->0x%08X\n",
> +			  chan, cmd, final_cmd, cntl, final_cntl);

Ehrm, do we need WARN and dev_emerg? And the error message should be
updated. It should tell what the user should do next to handle the
emergency.


...

> +	if (!imc_channel_can_claim(priv, i)) {
> +		dev_warn(&priv->pci_dev->dev,
> +			 "iMC channel %d: we cannot control the HW.  Is CLTT on?\n",
> +			 i);

That should go on the previous line IMO. The 80 char limit may be broken
for readability reasons.

> +MODULE_AUTHOR("Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>");
> +MODULE_DESCRIPTION("iMC SMBus driver");
> +MODULE_LICENSE("GPL");

GPL v2 according to header.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 3/4] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips

[Andy Lutomirski]

On Wed, Feb 19, 2014 at 7:38 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> +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 sounds scary and thus needs some more detail :) How does that show?
> What can happen? Anything to take care of?
>

Here's how this works, as far as I can figure it out:

The Sandy Bridge (and presumably Ivy Bridge) platforms make some
effort to avoid overheating system DRAM, and they can dynamically
adjust the refresh interval depending on DIMM temperature.  This
happens in one of a few modes:

CLTT (closed loop thermal throttling):  the memory controller
(presumably via the magic, barely documented "P-code") will
periodically poll the TSOD (Temperature Sensor on DIMM) over i2c.  In
this mode the SMBUS registers are, IMO sensibly, locked*, and the
worse that can happen is that the driver will confuse itself.  For
sanity, the driver will just refuse to load in this case.

OLTT (open loop thermal throttling):  the memory controller will
estimate recent heat output based on access patterns and will adjust
accordingly.  There are lots of registers related to this in the
public docs, but the overall algorithm and purpose is not described
anywhere that I've looked.  In this mode, something (SMI? P-code?) can
still poll the TSOD, but it respects the POLL_EN bit.

Thermal throttling off: the memory controller pays no attention.  I
suspect that we can't cause any real harm in this case even if we
tried.

The interesting case is OLTT.  If we fail to twiddle the POLL_EN bit
correctly, then, in principle, we could cause a problem.  I don't know
what that problem would be -- the memory controller's P-code could
malfunction with unknown results.  I've abused a test system quite a
bit, and I've been completely unable to cause a problem, though.

There may be other modes, too, but, if so, I can't find them.  Maybe
some day there will be CLTT with i2c access.  If so, presumably the
driver will need updating.

In some sense, this is no worse than other i2c drivers -- there are
plenty of ways that a badly behaving i2c driver can cause something to
blow up.

* This is not documented at all AFAIK.  I figured it out by trial and error.

>> @@ -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
>
> This is not perfectly sorted.
>
>>  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..c846077
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-imc.c
>> @@ -0,0 +1,559 @@
>> +/*
>> + * Copyright (c) 2013 Andrew Lutomirski <luto@amacapital.net>
>> + *
>> + * 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.
>
> Please skip the address. And run checkpatch.pl --strict! It would have
> warned you about this and other stuff:

Will do.  That's the first time I've heard of --strict :)


>
>> +/* 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)
>
> Spaces around operators!

Will do.

>
>> +     int i;
>> +     for (i = 0; i < 50; i++) {
>> +             pci_read_config_dword(priv->pci_dev, SMBSTAT(chan), stat);
>> +             if (!(*stat & SMBSTAT_SMB_BUSY))
>> +                     return 0;
>> +             udelay(70);
>
> usleep_range?

No -- see the comment just above this excerpt.  There's an erratum
(which I can trigger on occasion but not reliably) that causes the i2c
hardware to return bogus results if the system enters a sufficiently
deep sleep while a transaction is running.  udelay prevents that.
It's a bit ugly, but 70us isn't very long, and pm_qos doesn't seem to
be a good alternative (there's no way to tell pm_qos to keep at least
one logical thread out of C2 and lower).  I'll change the code to
udelay(70); /* don't sleep -- see above */ to make it more obvious.
Better ideas welcome.

>
>> +     }
>> +
>> +     return -ETIMEDOUT;
>> +}
>> +
>
> ...
>
>> +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 ret, chan;
>> +     u32 cmd = 0, cntl, final_cmd, final_cntl, stat;
>> +     struct imc_channel *ch;
>> +     struct imc_priv *priv = i2c_get_adapdata(adap);
>> +
>> +     if (atomic_read(&imc_raced))
>> +             return -EIO;  /* Minimize damage. */
>> +
>> +     chan = (adap == &priv->channels[0].adapter ? 0 : 1);
>> +     ch = &priv->channels[chan];
>> +
>> +     if (addr > 0x7f)
>> +             return -EOPNOTSUPP;  /* No large address support */
>
> Unneeded. The core checks for that...
>
>> +     if (flags)
>> +             return -EOPNOTSUPP;  /* No PEC */
>
> ... and your code would bail out if I2C_M_TEN was set.

I'll take another look at the core code and try to get this more correct :)

>
>> +     if (size != I2C_SMBUS_BYTE_DATA && size != I2C_SMBUS_WORD_DATA)
>> +             return -EOPNOTSUPP;
>> +
>> +     /* Encode CMD part of addresses and access size */
>> +     cmd  |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;
>> +     cmd |= ((u32)command) << SMBCMD_BA_SHIFT;
>> +     if (size == I2C_SMBUS_WORD_DATA)
>> +             cmd |= SMBCMD_WORD_ACCESS;
>> +
>> +     /* Encode read/write and data to write */
>> +     if (read_write == I2C_SMBUS_READ) {
>> +             cmd |= SMBCMD_TYPE_READ;
>> +     } else {
>> +             cmd |= SMBCMD_TYPE_WRITE;
>> +             cmd |= (size == I2C_SMBUS_WORD_DATA
>> +                         ? swab16(data->word)
>> +                         : data->byte);
>> +     }
>> +
>> +     mutex_lock(&ch->mutex);
>
> Why is the per-adapter lock not sufficient?

Is there a per-adapter lock in the core that I didn't notice?
i2c_lock_adapter seems to be optional, and I don't want to blow up if
two threads try to hit the same channel at the same time.

There's nothing wrong with accessing both channels on an adapter at
once, though -- AFAICT they are completely independent.

>
>> +
>> +     ret = imc_channel_claim(priv, chan);
>> +     if (ret)
>> +             goto out_unlock;
>> +
>> +     pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl);
>> +     cntl &= ~SMBCNTL_DTI_MASK;
>> +     cntl |= ((u32)addr >> 3) << SMBCNTL_DTI_SHIFT;
>> +     pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl);
>> +
>> +     /*
>> +      * 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.
>> +      */
>> +     cmd |= SMBCMD_TRIGGER;
>> +     pci_write_config_dword(priv->pci_dev, SMBCMD(chan), cmd);
>> +
>> +     if (imc_wait_not_busy(priv, chan, &stat) != 0) {
>> +             /* Timeout.  TODO: Reset the controller? */
>> +             ret = -EIO;
>> +             dev_err(&priv->pci_dev->dev, "controller is wedged\n");
>> +             goto out_release;
>> +     }
>> +
>> +     /*
>> +      * Be paranoid: try to detect races.  This will only detect races
>> +      * against BIOS, not against hardware.  (I've never seen this happen.)
>> +      */
>> +     pci_read_config_dword(priv->pci_dev, SMBCMD(chan), &final_cmd);
>> +     pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &final_cntl);
>> +     if (((cmd ^ final_cmd) & SMBCMD_OUR_BITS) ||
>> +         ((cntl ^ final_cntl) & SMBCNTL_OUR_BITS)) {
>> +             WARN(1, "iMC SMBUS raced against firmware");
>> +             dev_emerg(&priv->pci_dev->dev,
>> +                       "Access to channel %d raced: cmd 0x%08X->0x%08X, cntl 0x%08X->0x%08X\n",
>> +                       chan, cmd, final_cmd, cntl, final_cntl);
>
> Ehrm, do we need WARN and dev_emerg? And the error message should be
> updated. It should tell what the user should do next to handle the
> emergency.

I tried for a while, and I can't trigger this race.  The code is here
out of paranoia -- it should, in theory, detect a case where BIOS or
P-code fails to respect the TSOD_EN bit and conflicts with us.  If
this happens, the driver shuts itself down.

The outcome I want is that anyone who manages to trigger this will
email me and the linux-i2c list so that we can fix it.  The WARN will
cause abrt to notice, and the dev_emerg (which could be downgraded,
given the WARN) will supply useful information.  I could add an extra
line saying "Please contact luto@amacapital.net and
linux-i2c@vger.kernel.org".

Suggestions welcome :)

It would be awesome if an Intel engineer with access to better docs or
architectural insight could chime in -- I'm pretty sure that Intel is
funding NV-DIMM work, so they need this driver, and the old NDAed imc
smbus driver is almost certainly far less safe, less correct, and less
comprehensible than my driver. :)

TBH, the fact that, in CLTT mode, the SMBUS registers are all locked
makes me a lot more comfortable with this driver.  That's the mode in
which it would be easy to cause problems.

--Andy

[PATCH v6 4/4] i2c, i2c_imc: 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.  i2c_imc calls it.

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

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/i2c/busses/Kconfig    |  4 ++
 drivers/i2c/busses/Makefile   |  4 ++
 drivers/i2c/busses/dimm-bus.c | 97 +++++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-imc.c  |  3 ++
 include/linux/i2c/dimm-bus.h  | 24 +++++++++++
 5 files changed, 132 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 0fe76cc..8b46ad8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -135,6 +135,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_IMC
 	tristate "Intel iMC (LGA 2011) SMBus Controller"
 	depends on PCI && X86
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ea2a6a5..91827cb 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -25,6 +25,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..0968428
--- /dev/null
+++ b/drivers/i2c/busses/dimm-bus.c
@@ -0,0 +1,97 @@
+/*
+ * Copyright (c) 2013 Andrew Lutomirski <luto@amacapital.net>
+ *
+ * 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.  (The exception is the SPD write protection control,
+	 * which can't be probed and requires special hardware and/or
+	 * quick writes to access, and has no driver.)
+	 */
+	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;
+
+	/*
+	 * Addresses on DIMMs use the three low bits to identify the slot
+	 * and the four high bits to identify the device type.  Known
+	 * devices are:
+	 *
+	 *  - 0x50 - 0x57: SPD (Serial Presence Detect) EEPROM
+	 *  - 0x30 - 0x37: SPD WP control -- not worth trying to probe
+	 *  - 0x18 - 0x1f: TSOD (Temperature Sensor on DIMM)
+	 *
+	 * There may be more some day.
+	 */
+	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, "spd");
+		info.addr = 0x50 | slot;
+		i2c_new_device(adapter, &info);
+
+		if (probe_addr(adapter, 0x18 | slot)) {
+			/* This is a temperature sensor. */
+			strcpy(info.type, "jc42");
+			info.addr = 0x18 | slot;
+			i2c_new_device(adapter, &info);
+		}
+	}
+}
+EXPORT_SYMBOL(i2c_scan_dimm_bus);
+
+MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>");
+MODULE_DESCRIPTION("i2c DIMM bus support");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c
index c846077..437d8d8 100644
--- a/drivers/i2c/busses/i2c-imc.c
+++ b/drivers/i2c/busses/i2c-imc.c
@@ -15,6 +15,7 @@
  * 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/delay.h>
@@ -421,6 +422,8 @@ static int imc_init_channel(struct imc_priv *priv, int i, int socket)
 		return err;
 	}
 
+	i2c_scan_dimm_bus(&ch->adapter);
+
 	return 0;
 }
 
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@amacapital.net>
+ *
+ * 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.3.1

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Wolfram Sang]

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

On Fri, Dec 20, 2013 at 05:45:13PM -0800, 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.  i2c_imc calls it.

Hmm, after thinking about it for a while and a couple of times, I get
the impression that the functionality of i2c_scan_dimm_bus() should
better be in userspace, i.e. a udev helper. I could imagine introducing
a new functionality macro I2C_FUNC_DIMM_BUS which can be detected in
userspace via i2c-dev. Based on that, the helper can do whatever
necessary. What do you think?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Andy Lutomirski]

On Wed, Feb 19, 2014 at 7:16 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Fri, Dec 20, 2013 at 05:45:13PM -0800, 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.  i2c_imc calls it.
>
> Hmm, after thinking about it for a while and a couple of times, I get
> the impression that the functionality of i2c_scan_dimm_bus() should
> better be in userspace, i.e. a udev helper. I could imagine introducing
> a new functionality macro I2C_FUNC_DIMM_BUS which can be detected in
> userspace via i2c-dev. Based on that, the helper can do whatever
> necessary. What do you think?
>

Hmm.  So there would be udev rules that detect an I2C_FUNC_DIMM_BUS
driver and probe it appropriately.

I'm not sure I like it.  It would mean that any future kernel code
that wants to use things hanging off the dimm bus would need to stay
in sync with the udev rules, and it would make things like sticking
platform_data into the probed devices complicated or impossible.

This seems to be a somewhat common problem with i2c code.  For
example, lots of graphics drivers provide i2c busses, and those busses
often contain eeproms, and, in theory, things should know that the
eeprom is associated with a particular graphics port, for example.
Unfortunately, the i2c core does not know that, things like
decode-dimms will try to decode it, sensors-detect will scan graphics
ports for motherboard sensors, etc.

It would be great if there was a generic way to tag an i2c bus as
serving a particular purpose.  Code that knew the purpose could probe
appropriately, and code that wants to find things like eeproms on a
particular port could look for the i2c bus that's tagged as belonging
to the port and read its eeprom.

Is this something that could be done using something like kobject
symlinks?  For example, suppose there were a new class device
"i2c_port".  A driver could instantiate this class and tell the i2c
core (perhaps as part of i2c_register_adapter or in struct
i2c_adapter?) that a given i2c_adapter that a certain range of
addresses on the i2c_adapter belong to the port.  The i2c core could
create symlinks between the i2c_port and the i2c_adapter.

For extra fun, there could be drivers for different types of i2c_port.
 One of them could be the "DIMM bus" driver, which would know how to
probe the i2c adapter associated with a DIMM port.  Another could be
the graphics port driver, which (maybe with some extra configuration
hints from the graphics driver) could look for EDID-related things.

This could result in (after a bunch of extra code gets written) an
actual struct device for a DIMM slot, which would have a class device
for its i2c_port that claims all the the DIMM addresses with the slot
number bits set appropriately for that port.  In the even more distant
future, maybe the EDAC devices would also hang off of that DIMM slot.

I wonder if this would fit in well with the device tree stuff, too --
DT has ways to say "this node links to that one", right?

*sigh*.  The driver model wasn't really designed for a world where the
logical device topology is completely unrelated to the physical bus
topology.

OK, enough rambling for now. :)

--Andy

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Mauro Carvalho Chehab]

(I'm c/c Tony here, as he also shared the same concern that I had on a 
previous feedback about using I2C to talk with the DIMM).

Em Wed, 19 Feb 2014 09:30:46 -0800
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> escreveu:

> On Wed, Feb 19, 2014 at 7:16 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> > On Fri, Dec 20, 2013 at 05:45:13PM -0800, 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.  i2c_imc calls it.
> >
> > Hmm, after thinking about it for a while and a couple of times, I get
> > the impression that the functionality of i2c_scan_dimm_bus() should
> > better be in userspace, i.e. a udev helper. I could imagine introducing
> > a new functionality macro I2C_FUNC_DIMM_BUS which can be detected in
> > userspace via i2c-dev. Based on that, the helper can do whatever
> > necessary. What do you think?
> >
> 
> Hmm.  So there would be udev rules that detect an I2C_FUNC_DIMM_BUS
> driver and probe it appropriately.
> 
> I'm not sure I like it.  It would mean that any future kernel code
> that wants to use things hanging off the dimm bus would need to stay
> in sync with the udev rules, and it would make things like sticking
> platform_data into the probed devices complicated or impossible.

As I said already a few times, my main concern with such patches is that it
could eventually cause really bad things, especially since we're using
experimental data collected on a system (or maybe on more than one, but
still a limited set of systems), to infere that the same behavior will
be valid for all.

Even assuming that you covered all existing systems, couldn't a BIOS 
or microcode upgrade do some changes that could increase the chances 
that the above-described problems to happen more likely? I'm afraid so.

Let me also cut-and-paste a few relevant parts taken from patch 4/4:

> >> +       It is possibly, although unlikely, that the use of this driver will
> >> +       interfere with your platform's RAM thermal management.
> >
> > This sounds scary and thus needs some more detail :) How does that show?
> > What can happen? Anything to take care of?
> >
> 
> Here's how this works, as far as I can figure it out:
> 
> The Sandy Bridge (and presumably Ivy Bridge) platforms make some
> effort to avoid overheating system DRAM, and they can dynamically
> adjust the refresh interval depending on DIMM temperature.  This
> happens in one of a few modes:

...

> OLTT (open loop thermal throttling):  the memory controller will
> estimate recent heat output based on access patterns and will adjust
> accordingly.  There are lots of registers related to this in the
> public docs, but the overall algorithm and purpose is not described
> anywhere that I've looked.  In this mode, something (SMI? P-code?) can
> still poll the TSOD, but it respects the POLL_EN bit.

...

> The interesting case is OLTT.  If we fail to twiddle the POLL_EN bit
> correctly, then, in principle, we could cause a problem.  I don't know
> what that problem would be -- the memory controller's P-code could
> malfunction with unknown results.  I've abused a test system quite a
> bit, and I've been completely unable to cause a problem, though.
>
> There may be other modes, too, but, if so, I can't find them.  Maybe
> some day there will be CLTT with i2c access.  If so, presumably the
> driver will need updating.

...

> * This is not documented at all AFAIK.  I figured it out by trial and error.

...

> >> +             udelay(70);
> >
> > usleep_range?
> 
> No -- see the comment just above this excerpt.  There's an erratum
> (which I can trigger on occasion but not reliably) that causes the i2c
> hardware to return bogus results if the system enters a sufficiently
> deep sleep while a transaction is running.  udelay prevents that.

Well, I've seen already very bad things happening on I2C.

For example, some I2C eeproms miss-understands 0-sized I2C bus read 
messages, even sent to other I2C chips. On such eeproms, sometimes,
instead of doing an eeprom read, they actually do an I2C write.

There are enough known reported cases of such troubles with TV boards
that we even added a contrib perl code at v4l2-utils to allow one to
try to restore the board's EEPROM content, in order to allow fixing an 
accidental card brick, due to an EEPROM content loss caused by this bug.

Of course, the user should have made a copy first of the content of the
eeprom, or to find, at the net, a valid content for his board. This is
easier said than done.

Knowing nothing about how the DIMMs are manufactured, I can think on an
hypothetical scenario where a manufacturer would test the DIMM timings
and other DIMM quality parameters on their production line, and fill an
I2C EEPROM at the DIMMS after those tests, in order to determine the safe
zone for a DIMM.

So, if such scenario is possible (someone with more experience with
DIMM production could help here?), then, in thesis, a badly timed read
(or an I2C read that would be interrupted by a SMI call that would also
touch at the same I2C bus) could damage the EEPROM content, bricking
the DIMM.

Well, what I'm trying to say is that, before implementing any solution
that reads data directly from the DIMM I2C bus (doesn't matter if in
userspace or Kernelspace), we should know for sure that this won't cause 
DIMM loses, either by overheating them or by causing a corruption on the
information that stores the DIMM size and timings on each DIMM slot.

So, I suggest to not commit any patches here, without a careful
review from an Intel engineer that would then be sure that:
- the BIOS will respect POLL_EN;
- there will be no way to accidentally write data at DIMM control eeproms;
- that current and newer microcode/firmware updates will keep such driver
  reliable in the long term.

That's said, as there are critical timings at the driver (so udelay is
needed), and a precise time can't be warranted in userspace - as the
userspace process may be interrupted during an userspace usleep() call - I
would very much prefer to have this implemented at Kernelspace level.

Regards,
Mauro

RE: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Luck, Tony]

> (I'm c/c Tony here, as he also shared the same concern that I had on a 
> previous feedback about using I2C to talk with the DIMM).

Correct - I've heard the same issues that reads on I2C can be misinterpreted
as writes ... and oops, you have a brick.

What is the larger context/  What problem are we trying to solve?

-Tony

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Andy Lutomirski]

On Wed, Feb 19, 2014 at 11:03 AM, Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> (I'm c/c Tony here, as he also shared the same concern that I had on a
>> previous feedback about using I2C to talk with the DIMM).
>
> Correct - I've heard the same issues that reads on I2C can be misinterpreted
> as writes ... and oops, you have a brick.

Is this true on DDR3 DIMMs, i.e. anything that's compatible with
LGA2011?  If you plug a DIMM into an LGA2011 board's memory slot,
then, one way or another, it's very likely that there will be TSOD
traffic, if for no other purpose than to determine that there is no
TSOD present.  TSOD traffic consists of reads and writes, both with
and without register numbers.  (Sorry, I can never remember the smbus
terminology here -- the relevant transactions are two-byte reads and
two-byte writes, both with a command specified and without one.  One
of the bits in the iMC SMBUS registers tells the controller which kind
of read to use to probe the thermometer.)

>
> What is the larger context/  What problem are we trying to solve?

NV-DIMM control registers are exposed via i2c, presumably because
trying to access them through the memory pins would be a giant mess.
So, one way or another, something needs to be able to initiate
transactions to access those registers.  BIOS will do some initial
setup, but the OS will need to poke at these registers, too.  (The
actual docs are covered by NDA.  I suspect that this will change if
the manufacturers ever want these things to be widely used, though,
since these things really want a full-featured kernel driver so that
things like pmfs will work cleanly.)

As a secondary benefit, having access to the TSOD and SPD is nice,
albeit far from critical.

AFAICT Intel actively working on NV-DIMM-related things, so maybe
Intel will contribute an engineer who help :)

--Andy

>
> -Tony



-- 
Andy Lutomirski
AMA Capital Management, LLC

RE: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Luck, Tony]

> NV-DIMM control registers are exposed via i2c, presumably because
> trying to access them through the memory pins would be a giant mess.
> So, one way or another, something needs to be able to initiate
> transactions to access those registers.  BIOS will do some initial
> setup, but the OS will need to poke at these registers, too.  (The
> actual docs are covered by NDA.  I suspect that this will change if
> the manufacturers ever want these things to be widely used, though,
> since these things really want a full-featured kernel driver so that
> things like pmfs will work cleanly.)
>
> As a secondary benefit, having access to the TSOD and SPD is nice,
> albeit far from critical.
>
> AFAICT Intel actively working on NV-DIMM-related things, so maybe
> Intel will contribute an engineer who help :)

Yes - we have people looking at pmfs and NV-DIMMs.  I don't know
the internal details ... to keep these accesses safe may require letting
the platform BIOS code perform them (via some ACPI thingy) ... messy
and slow - but probably workable if these registers are only required
for some maintenance/configuration usage patterns.  Not so good if
they are in the high frequency read/write path (but I2C in the critical
path sounds like a recipe for failure).

-Tony

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Andy Lutomirski]

On Thu, Feb 20, 2014 at 8:42 AM, Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> NV-DIMM control registers are exposed via i2c, presumably because
>> trying to access them through the memory pins would be a giant mess.
>> So, one way or another, something needs to be able to initiate
>> transactions to access those registers.  BIOS will do some initial
>> setup, but the OS will need to poke at these registers, too.  (The
>> actual docs are covered by NDA.  I suspect that this will change if
>> the manufacturers ever want these things to be widely used, though,
>> since these things really want a full-featured kernel driver so that
>> things like pmfs will work cleanly.)
>>
>> As a secondary benefit, having access to the TSOD and SPD is nice,
>> albeit far from critical.
>>
>> AFAICT Intel actively working on NV-DIMM-related things, so maybe
>> Intel will contribute an engineer who help :)
>
> Yes - we have people looking at pmfs and NV-DIMMs.  I don't know
> the internal details ... to keep these accesses safe may require letting
> the platform BIOS code perform them (via some ACPI thingy) ... messy
> and slow - but probably workable if these registers are only required
> for some maintenance/configuration usage patterns.  Not so good if
> they are in the high frequency read/write path (but I2C in the critical
> path sounds like a recipe for failure).

As far as I know, they're not involved in reads and writes, but they
are useful periodically to keep everything happy.

I suppose that *shudder* calling an ACPI method to initiate an i2c
transaction aimed at a DIMM slot isn't *that* bad.  On the other hand,
if we're supposed to issue an ACPI call to ask "is it okay to start
using this device now", then won't we need to reflash firmware every
time we switch NV-DIMM vendors?  Ugh!

In any case, let's hold off on merging this until Intel and/or the
ACPI people figure out what's going on.  This driver is IMO not worth
it just to access SPD and TSOD.  (Actually, I think I could write a
separate TSOD driver that just asks the memory controller to report
cached temperature values.  That should be almost entirely non-scary.)

--Andy

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Mauro Carvalho Chehab]

Em Wed, 19 Feb 2014 17:39:07 -0800
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> escreveu:

> On Wed, Feb 19, 2014 at 11:03 AM, Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> >> (I'm c/c Tony here, as he also shared the same concern that I had on a
> >> previous feedback about using I2C to talk with the DIMM).
> >
> > Correct - I've heard the same issues that reads on I2C can be misinterpreted
> > as writes ... and oops, you have a brick.
> 
> Is this true on DDR3 DIMMs, i.e. anything that's compatible with
> LGA2011?  If you plug a DIMM into an LGA2011 board's memory slot,
> then, one way or another, it's very likely that there will be TSOD
> traffic, if for no other purpose than to determine that there is no
> TSOD present.  TSOD traffic consists of reads and writes, both with
> and without register numbers.  (Sorry, I can never remember the smbus
> terminology here -- the relevant transactions are two-byte reads and
> two-byte writes, both with a command specified and without one.  One
> of the bits in the iMC SMBUS registers tells the controller which kind
> of read to use to probe the thermometer.)

An update on that: I double-checked with a DIMM manufacturer.
I was told that some DIMM models have write protect circuits but
others don't.

So, yeah, accessing it could eventually brick the DIMM, depending 
on the DIMM model, if such driver won't block I2C write ops or
if the BIOS is also trying to access the bus at the same time.

Regards,
Mauro

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Andy Lutomirski]

On Feb 28, 2014 11:15 AM, "Mauro Carvalho Chehab" <m.chehab@samsung.com> wrote:
>
> Em Wed, 19 Feb 2014 17:39:07 -0800
> Andy Lutomirski <luto@amacapital.net> escreveu:
>
> > On Wed, Feb 19, 2014 at 11:03 AM, Luck, Tony <tony.luck@intel.com> wrote:
> > >> (I'm c/c Tony here, as he also shared the same concern that I had on a
> > >> previous feedback about using I2C to talk with the DIMM).
> > >
> > > Correct - I've heard the same issues that reads on I2C can be misinterpreted
> > > as writes ... and oops, you have a brick.
> >
> > Is this true on DDR3 DIMMs, i.e. anything that's compatible with
> > LGA2011?  If you plug a DIMM into an LGA2011 board's memory slot,
> > then, one way or another, it's very likely that there will be TSOD
> > traffic, if for no other purpose than to determine that there is no
> > TSOD present.  TSOD traffic consists of reads and writes, both with
> > and without register numbers.  (Sorry, I can never remember the smbus
> > terminology here -- the relevant transactions are two-byte reads and
> > two-byte writes, both with a command specified and without one.  One
> > of the bits in the iMC SMBUS registers tells the controller which kind
> > of read to use to probe the thermometer.)
>
> An update on that: I double-checked with a DIMM manufacturer.
> I was told that some DIMM models have write protect circuits but
> others don't.
>
> So, yeah, accessing it could eventually brick the DIMM, depending
> on the DIMM model, if such driver won't block I2C write ops or
> if the BIOS is also trying to access the bus at the same time.

I'm not sure I buy the argument about blocking writes -- as far as I
know, i2c-i801 on desktop platforms has exactly the same issue.  Or
maybe I'm misunderstanding you.

I'd be okay with adding code to block writes to SPD addresses, but I'm
not sure I see the point.  If root wants to brick a DIMM, I'm sure
root can find a way.

On the other hand, racing with BIOS is a real problem.  I'd like to
know how Intel plans to handle this.  I suspect that POLL_EN is the
right bit to use, but the docs are vague, and there could be strange
BIOSes out there.

(Grr.  Everything would be saner if SMM didn't exist.)

--Andy

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[One Thousand Gnomes]

> example, lots of graphics drivers provide i2c busses, and those busses
> often contain eeproms, and, in theory, things should know that the
> eeprom is associated with a particular graphics port, for example.
> Unfortunately, the i2c core does not know that, things like
> decode-dimms will try to decode it, sensors-detect will scan graphics
> ports for motherboard sensors, etc.

ACPI does now try to describe what is on an i²c bus. We perhaps don't use
that information well but on a modern PC class box at least for onboard
devices most of the info is there for the munching.

> For extra fun, there could be drivers for different types of i2c_port.
>  One of them could be the "DIMM bus" driver, which would know how to
> probe the i2c adapter associated with a DIMM port.  Another could be
> the graphics port driver, which (maybe with some extra configuration
> hints from the graphics driver) could look for EDID-related things.

Busses are not necessarily that tidily organised. There isn't anything
saying you can't sneak multiple things on the same bus. In the graphics
case its unlikely but I wouldn't rule even that out for a display panel.
Once you get onto phones and tablets it seems anything goes 8)

> I wonder if this would fit in well with the device tree stuff, too --
> DT has ways to say "this node links to that one", right?

ACPI basically tries to describe the heirarchy of devices on the bus, but
as you say the "real" device can be a rambling mix of GPIO, I²C, SPI,
PCI (quite probably fake PCI at that) and other resources.

If there is a legitimate use case for poking around with memory dimm i2c
on these boards then really there needs to be a proper defined interface
for doing so that covers firmware and OS users.

Alan

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Andy Lutomirski]

On Wed, Feb 19, 2014 at 11:26 AM, One Thousand Gnomes
<gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:
>> example, lots of graphics drivers provide i2c busses, and those busses
>> often contain eeproms, and, in theory, things should know that the
>> eeprom is associated with a particular graphics port, for example.
>> Unfortunately, the i2c core does not know that, things like
>> decode-dimms will try to decode it, sensors-detect will scan graphics
>> ports for motherboard sensors, etc.
>
> ACPI does now try to describe what is on an i²c bus. We perhaps don't use
> that information well but on a modern PC class box at least for onboard
> devices most of the info is there for the munching.

I'm discussing the in-kernel representation, not where the information
comes from.  Presumably there should be something in the kernel that
works for ACPI, DT, and directly-enumerated busses (see below).

>
>> For extra fun, there could be drivers for different types of i2c_port.
>>  One of them could be the "DIMM bus" driver, which would know how to
>> probe the i2c adapter associated with a DIMM port.  Another could be
>> the graphics port driver, which (maybe with some extra configuration
>> hints from the graphics driver) could look for EDID-related things.
>
> Busses are not necessarily that tidily organised. There isn't anything
> saying you can't sneak multiple things on the same bus. In the graphics
> case its unlikely but I wouldn't rule even that out for a display panel.
> Once you get onto phones and tablets it seems anything goes 8)
>

I'm suggesting identifying a range of addresses on a bus with a "port"
(or whatever it should be called).  Multiple ports could claim
non-overlapping ranges on the same bus.

>> I wonder if this would fit in well with the device tree stuff, too --
>> DT has ways to say "this node links to that one", right?
>
> ACPI basically tries to describe the heirarchy of devices on the bus, but
> as you say the "real" device can be a rambling mix of GPIO, I²C, SPI,
> PCI (quite probably fake PCI at that) and other resources.
>
> If there is a legitimate use case for poking around with memory dimm i2c
> on these boards then really there needs to be a proper defined interface
> for doing so that covers firmware and OS users.

The legitimate use case is NV-DIMMs.  They have control registers that
are accessed via smbus.  BIOS is likely to poke at those registers
early in boot, but the kernel and/or userspace will need them, too.
It would be nice to know which DIMM slot they map to.

In this particular case (Intel LGA2011 systems), there's only one sane
way to wire up the busses, since the memory controller *is* the smbus
master.  According to the JEDEC spec, each DIMM slot is has three pins
that indicate which slot it is on its channel, and the onboard smbus
hardware uses those pins to select its addresses.  (This maps directly
to three of the seven address bits.)  Each memory controller supports
two RAM channels and two i2c channels, and, if you wire them backwards
(or mix them up between controllers), then I would be amazed if the
system works.  So this particular driver really does know the topology
a priori.

On the other hand, nothing rules out having NV-DIMMs in non-LGA2011
systems where the smbus/i2c topology is less certain.  But I think it
would be silly for an eventual NVDIMM driver to specifically depend on
ACPI, since ACPI isn't needed for existing LGA2011 NVDIMM systems.

(Knowing which physical memory addresses map to which DIMM is a
different story.  I think that the sb-edac driver knows the mapping,
though.)

--Andy

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[One Thousand Gnomes]

> I'm suggesting identifying a range of addresses on a bus with a "port"
> (or whatever it should be called).  Multiple ports could claim
> non-overlapping ranges on the same bus.

Which is fine until you meant a mux or a device that can be moved about
by writing to it, or has a wide range of addresses determined by
strapping.

> In this particular case (Intel LGA2011 systems), there's only one sane
> way to wire up the busses, since the memory controller *is* the smbus
> master.  According to the JEDEC spec, each DIMM slot is has three pins

Ok that helps a lot for the specific case, and you have at least in
theory got a flag between the OS and BIOS to avoid things like SMM
throwing parties on the smbus while you are using it.

Re: [PATCH v6 4/4] i2c, i2c_imc: Add DIMM bus code

[Andy Lutomirski]

On Fri, Feb 21, 2014 at 7:32 AM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
>> I'm suggesting identifying a range of addresses on a bus with a "port"
>> (or whatever it should be called).  Multiple ports could claim
>> non-overlapping ranges on the same bus.
>
> Which is fine until you meant a mux or a device that can be moved about
> by writing to it, or has a wide range of addresses determined by
> strapping.

For muxes: isn't there already a system in place for dealing with them?

For things with addresses determined by strapping: my vague proposal
is explicitly intended to support them.  (DIMMs are such a device.)
Whoever instantiates the i2c "ports" is supposed to *know* the
topology, including which straps are set where.  In some cases, that
data could come from ACPI or DT.  In other cases, it's enumerated
directly.  The idea would be to allow the code that figures out the
topology (both physical connectivity and straps) to be separated from
the code that talks to the i2c endpoints.

Just specifying the address ranges might be insufficient for things
that are more flexible than DIMMs, though -- it might be necessary to
say "this port has these addresses and this kind of device lives at
this address".

--Andy