[PATCH 0/2] i2c_imc: New driver, at long last

[PATCH 0/2] i2c_imc: New driver, at long last

[Andy Lutomirski]

This adds i2c_imc, a driver for the SMBUS lines on DIMM slots on modern
Intel server chips.  Conceptually, I like it a lot -- it's a driver for
a bus for which we know the exact topology a priori.  That means that we
can actually enumerate the things on the bus reasonably cleanly.

This driver is weird, but I don't think that merging it will cause
problems, and I believe that there are Real Users (tm) of this driver.

It has two caveats.

Big caveat: Lots of things like to touch this bus, such as a BMC, the
memory controller power management stuff, and maybe even SMM code.
Intel forgot to define a way to arbitrate between them, nor are the
SMBUS master regs designed to be poked by multiple things at once.  The
upshot is that loading this driver is generally unsafe.  The driver
takes some measures to detect contention for the registers and shut
itself down, but no one should rely on that.

My understanding is that there's work in ACPI land to define an
arbitration mechanism.  Once this happens, we can support it in this
driver.  In the mean time, this driver has actual users, and there are
motherboards specifically designed to be used with a driver like this.
(I have such a board, and you can even buy them on regular online
shopping sutes.)

Therefore, I added a module parameter called allow_unsafe_access.  If
unset, the driver will refuse to load with an informative message.  If
set, the driver will pr_warn and load.

Little caveat: This submission only supposts Sandy Bridge.  I'd rather
get it merged without Ivy Bridge and Haswell support and add them later
(should be easy) rather than trying to make the driver support all
possible hardware before merging it.

Changes: This is an updated resubmission after about a year of thumb
twiddling.

Andy Lutomirski (2):
  i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
  i2c, i2c_imc: Add DIMM bus code

 drivers/i2c/busses/Kconfig    |  22 ++
 drivers/i2c/busses/Makefile   |   5 +
 drivers/i2c/busses/dimm-bus.c |  97 +++++++
 drivers/i2c/busses/i2c-imc.c  | 586 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/dimm-bus.h  |  24 ++
 5 files changed, 734 insertions(+)
 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

-- 
2.1.0

[PATCH 1/2] 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.

There are various groups working on standardizing a way to arbitrate
access to the bus between the OS, SMM firmware, a BMC, hardware
thermal control, etc.  In the mean time, running this driver is
unsafe except under special circumstances.  Nonetheless, this driver
has real users.

As a compromise, the driver will refuse to load unless
i2c_imc.allow_unsafe_access=Y.  When safe access becomes available,
we can leave this option as a way for legacy users to run the
driver, and we'll allow the driver to load by default if safe bus
access is available.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/i2c/busses/Kconfig   |  18 ++
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-imc.c | 583 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 602 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-imc.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index ab838d9e28b6..d6b9ce164fbf 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -149,6 +149,24 @@ 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.
+
+	  There are currently no systems on which the kernel knows that it can
+	  safely enable this driver.  For now, you need to pass this driver a
+	  scary module parameter, and you should only pass that parameter if you
+	  have a special motherboard and know exactly what you are doing.
+	  Special motherboards include the Supermicro X9DRH-iF-NV.
+
+	  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 56388f658d2f..4287c891e782 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 000000000000..2dbf171114c6
--- /dev/null
+++ b/drivers/i2c/busses/i2c-imc.c
@@ -0,0 +1,583 @@
+/*
+ * 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.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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. */
+
+static bool allow_unsafe_access;
+
+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,
+};
+
+static int __init i2c_imc_init(void)
+{
+	if (!allow_unsafe_access) {
+		pr_info("disabled because we cannot safely arbitrate with firmware and hardware\n");
+		return -EBUSY;
+	}
+
+	pr_warn("using this driver is dangerous unless your firmware is specifically designed for it; use at your own risk\n");
+	return pci_register_driver(&imc_pci_driver);
+}
+module_init(i2c_imc_init);
+
+static void __exit i2c_imc_exit(void)
+{
+	pci_unregister_driver(&imc_pci_driver);
+}
+module_exit(i2c_imc_exit);
+
+module_param(allow_unsafe_access, bool, 0400);
+MODULE_PARM_DESC(allow_unsafe_access, "enable i2c_imc despite potential races against BIOS/hardware bus access");
+
+MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>");
+MODULE_DESCRIPTION("iMC SMBus driver");
+MODULE_LICENSE("GPL");
-- 
2.1.0

[PATCH 2/2] 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 d6b9ce164fbf..2ea6648492eb 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -149,6 +149,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 4287c891e782..a01bdcc0e239 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 000000000000..096842811199
--- /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 2dbf171114c6..f9d535a99035 100644
--- a/drivers/i2c/busses/i2c-imc.c
+++ b/drivers/i2c/busses/i2c-imc.c
@@ -22,6 +22,7 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/i2c.h>
+#include <linux/i2c/dimm-bus.h>
 
 /*
  * The datasheet can be found here, for example:
@@ -425,6 +426,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 000000000000..3d44df43cf30
--- /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 */
-- 
2.1.0

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

[Paul Bolle]

Just two nits.

Andy Lutomirski schreef op vr 06-03-2015 om 18:50 [-0800]:

> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -149,6 +149,24 @@ 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

The pedant in me can't help but notice that I2C_DIMM_BUS itself is added
in patch 2/2. And so is the call of i2c_scan_dimm_bus() in i2c-imc.c
that apparently requires this select. So that select isn't really needed
in this patch but in 2/2.

> +	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.
> +
> +	  There are currently no systems on which the kernel knows that it can
> +	  safely enable this driver.  For now, you need to pass this driver a
> +	  scary module parameter, and you should only pass that parameter if you
> +	  have a special motherboard and know exactly what you are doing.
> +	  Special motherboards include the Supermicro X9DRH-iF-NV.
> +
> +	  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

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

This states the license is GPL v2.

> +MODULE_LICENSE("GPL");

So you probably want to use
    MODULE_LICENSE("GPL v2");

here.


Paul Bolle

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

[Paul Bolle]

Just a license nit, I'm afraid.

Andy Lutomirski schreef op vr 06-03-2015 om 18:50 [-0800]:
> --- /dev/null
> +++ b/drivers/i2c/busses/dimm-bus.c
> @@ -0,0 +1,97 @@
> +/*
> + * 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.
> + */

This states the license is GPL v2.

> +MODULE_LICENSE("GPL");

So you probably want
    MODULE_LICENSE("GPL v2");

here too.


Paul Bolle

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

[Guenter Roeck]

On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
> Sandy Bridge Xeon and Extreme chips have integrated memory
> controllers with (rather limited) onboard SMBUS masters.  This
> driver gives access to the bus.
>
> There are various groups working on standardizing a way to arbitrate
> access to the bus between the OS, SMM firmware, a BMC, hardware
> thermal control, etc.  In the mean time, running this driver is
> unsafe except under special circumstances.  Nonetheless, this driver
> has real users.
>
> As a compromise, the driver will refuse to load unless
> i2c_imc.allow_unsafe_access=Y.  When safe access becomes available,
> we can leave this option as a way for legacy users to run the
> driver, and we'll allow the driver to load by default if safe bus
> access is available.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---

Please consider running your patch through checkpatch --strict, or at least checkpatch.
[ I won't comment on the checkpatch problems below ]

>   drivers/i2c/busses/Kconfig   |  18 ++
>   drivers/i2c/busses/Makefile  |   1 +
>   drivers/i2c/busses/i2c-imc.c | 583 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 602 insertions(+)
>   create mode 100644 drivers/i2c/busses/i2c-imc.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index ab838d9e28b6..d6b9ce164fbf 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -149,6 +149,24 @@ 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.
> +
> +	  There are currently no systems on which the kernel knows that it can
> +	  safely enable this driver.  For now, you need to pass this driver a
> +	  scary module parameter, and you should only pass that parameter if you
> +	  have a special motherboard and know exactly what you are doing.
> +	  Special motherboards include the Supermicro X9DRH-iF-NV.
> +
> +	  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 56388f658d2f..4287c891e782 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 000000000000..2dbf171114c6
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-imc.c
> @@ -0,0 +1,583 @@
> +/*
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#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)

You might want to use (i) to avoid undesirable side effects if i is an expression.

> +#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. */
> +
> +static bool allow_unsafe_access;
> +
> +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);
> +	}

70 * 50 = 3500uS or 3.5ms is a long time. Can you use usleep_range ?
Not sure if it would buy much, just asking.

> +
> +	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;
> +

It is quite uncommon to have those checks in the code. Usually one trusts
the infrastructure to not request unsupported functionality.
Is this really necessary ?

> +	/* Encode CMD part of addresses and access size */
> +	cmd  |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;

Double space

> +	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;

timeout -> -ETIMEDOUT ?

> +		dev_err(&priv->pci_dev->dev, "controller is wedged\n");

If this happens, it will presumably happen all the time and the message will
pollute the log. Is the message really necessary ?

> +		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,

Is a stack trace and dev_emerg really warranted here ?

> +			  "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;

ret is already guaranteed to be 0 here.

> +		} 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;

ret is already 0, so only the else case is really needed.

> +		} 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.
> +	 */

Seems to be a comment with no value. Every adapter needs to be ready
by the time it registers.

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

So ?

> +	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);

ret is a bit unusual as variable name for a pointer. dev, maybe ?

> +	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);

devm_kzalloc() ?

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

			if (i)
				imc_free_channel(priv, 0);

would be a bit simpler and accomplish the same.

> +				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 i2c_imc_init(void)
> +{
> +	if (!allow_unsafe_access) {
> +		pr_info("disabled because we cannot safely arbitrate with firmware and hardware\n");
> +		return -EBUSY;

Why -EBUSY and not -ENODEV ?
Also not sure if the message is really warranted.

> +	}
> +
> +	pr_warn("using this driver is dangerous unless your firmware is specifically designed for it; use at your own risk\n");

Seems to me this is a bit noisy. User should already know.

> +	return pci_register_driver(&imc_pci_driver);
> +}
> +module_init(i2c_imc_init);
> +
> +static void __exit i2c_imc_exit(void)
> +{
> +	pci_unregister_driver(&imc_pci_driver);
> +}
> +module_exit(i2c_imc_exit);
> +
> +module_param(allow_unsafe_access, bool, 0400);
> +MODULE_PARM_DESC(allow_unsafe_access, "enable i2c_imc despite potential races against BIOS/hardware bus access");
> +
> +MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>");
> +MODULE_DESCRIPTION("iMC SMBus driver");
> +MODULE_LICENSE("GPL");
>

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

[Guenter Roeck]

On 03/06/2015 06:50 PM, 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.
>
> 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>

Similar to the other patch, I would suggest to run it through checkpatch.

> ---
>   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 d6b9ce164fbf..2ea6648492eb 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -149,6 +149,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 4287c891e782..a01bdcc0e239 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 000000000000..096842811199
> --- /dev/null
> +++ b/drivers/i2c/busses/dimm-bus.c
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright (c) 2013 Andrew Lutomirski <luto@amacapital.net>

2013 ?

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

That leads to the question if the controller supports quick commands,
and if it does, if would make sense to add support for it,
just for the purpose of reducing risk here.


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

Why not ?

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

Not worth, or just "not probed" ?

> +	 *  - 0x18 - 0x1f: TSOD (Temperature Sensor on DIMM)
> +	 *
> +	 * There may be more some day.

That is always a possibility. Does that really warrant this comment ?

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

How are those devices removed ? Might warrant an explanation.

> +		}
> +	}
> +}
> +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 2dbf171114c6..f9d535a99035 100644
> --- a/drivers/i2c/busses/i2c-imc.c
> +++ b/drivers/i2c/busses/i2c-imc.c
> @@ -22,6 +22,7 @@
>   #include <linux/delay.h>
>   #include <linux/pci.h>
>   #include <linux/i2c.h>
> +#include <linux/i2c/dimm-bus.h>
>
>   /*
>    * The datasheet can be found here, for example:
> @@ -425,6 +426,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 000000000000..3d44df43cf30
> --- /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 */
>

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

[Andy Lutomirski]

On Mar 7, 2015 6:39 AM, "Guenter Roeck" <linux@roeck-us.net> wrote:
>
> On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
>>
>> Sandy Bridge Xeon and Extreme chips have integrated memory
>> controllers with (rather limited) onboard SMBUS masters.  This
>> driver gives access to the bus.
>>
>> There are various groups working on standardizing a way to arbitrate
>> access to the bus between the OS, SMM firmware, a BMC, hardware
>> thermal control, etc.  In the mean time, running this driver is
>> unsafe except under special circumstances.  Nonetheless, this driver
>> has real users.
>>
>> As a compromise, the driver will refuse to load unless
>> i2c_imc.allow_unsafe_access=Y.  When safe access becomes available,
>> we can leave this option as a way for legacy users to run the
>> driver, and we'll allow the driver to load by default if safe bus
>> access is available.
>>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>
>
> Please consider running your patch through checkpatch --strict, or at least checkpatch.
> [ I won't comment on the checkpatch problems below ]
>
>
>>   drivers/i2c/busses/Kconfig   |  18 ++
>>   drivers/i2c/busses/Makefile  |   1 +
>>   drivers/i2c/busses/i2c-imc.c | 583 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 602 insertions(+)
>>   create mode 100644 drivers/i2c/busses/i2c-imc.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index ab838d9e28b6..d6b9ce164fbf 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -149,6 +149,24 @@ 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.
>> +
>> +         There are currently no systems on which the kernel knows that it can
>> +         safely enable this driver.  For now, you need to pass this driver a
>> +         scary module parameter, and you should only pass that parameter if you
>> +         have a special motherboard and know exactly what you are doing.
>> +         Special motherboards include the Supermicro X9DRH-iF-NV.
>> +
>> +         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 56388f658d2f..4287c891e782 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 000000000000..2dbf171114c6
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-imc.c
>> @@ -0,0 +1,583 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#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)
>
>
> You might want to use (i) to avoid undesirable side effects if i is an expression.
>

Will do.

>
>> +#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. */
>> +
>> +static bool allow_unsafe_access;
>> +
>> +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);
>> +       }
>
>
> 70 * 50 = 3500uS or 3.5ms is a long time. Can you use usleep_range ?
> Not sure if it would buy much, just asking.

In general, it should be much closer to 70 us.  Also, at least on
Sandy Bridge, we don't want to go idle here -- there's an erratum.
Not sure about IVB/Haswell.

>
>
>> +
>> +       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;
>> +
>
>
> It is quite uncommon to have those checks in the code. Usually one trusts
> the infrastructure to not request unsupported functionality.
> Is this really necessary ?

No, I can remove it.

>
>
>> +       /* Encode CMD part of addresses and access size */
>> +       cmd  |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;
>
>
> Double space

Fixed.

>
>
>> +       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;
>
>
> timeout -> -ETIMEDOUT ?

OK

>
>
>> +               dev_err(&priv->pci_dev->dev, "controller is wedged\n");
>
>
> If this happens, it will presumably happen all the time and the message will
> pollute the log. Is the message really necessary ?

I'd rather log something to help diagnose.  Would rate-limiting it be okay?

>
>
>> +               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,
>
>
> Is a stack trace and dev_emerg really warranted here ?
>

If this happens, something's very wrong and the user should stop using
the driver.  We could potentially write the wrong address, and, if we
manage to screw up thermal management, we could potentially corrupt
data for to an inappropriate refresh interval.

IOW, I want to hear about it if this happens.

>
>> +                         "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;
>
>
> ret is already guaranteed to be 0 here.
>
>
>> +               } 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;
>
>
> ret is already 0, so only the else case is really needed.
>

I wanted to keep the success and failure paths in the same order in
both the read and write cases.  I'll remove the unnecessary
assignment, though.

>
>> +               } 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.
>> +        */
>
>
> Seems to be a comment with no value. Every adapter needs to be ready
> by the time it registers.

It surprised me :)  I'll remove it.

>
>
>> +       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. */
>
>
> So ?

It needs to happen before mutex_destroy.  I improved the comment.

>
>
>> +       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);
>
>
> ret is a bit unusual as variable name for a pointer. dev, maybe ?
>
>
>> +       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);
>
>
> devm_kzalloc() ?
>

Done.

>
>> +       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++)
>
>
>                         if (i)
>                                 imc_free_channel(priv, 0);
>
> would be a bit simpler and accomplish the same.

I want to be ready for future hardware that might support more than
two channels.

>
>
>> +                               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 i2c_imc_init(void)
>> +{
>> +       if (!allow_unsafe_access) {
>> +               pr_info("disabled because we cannot safely arbitrate with firmware and hardware\n");
>> +               return -EBUSY;
>
>
> Why -EBUSY and not -ENODEV ?

No real reason.   Changed.

> Also not sure if the message is really warranted.
>

Me neither.  It could be helpful, and it's only dev_info.  I'll remove
it, though -- it'll just be noise for the vast majority of users.

>
>> +       }
>> +
>> +       pr_warn("using this driver is dangerous unless your firmware is specifically designed for it; use at your own risk\n");
>
>
> Seems to me this is a bit noisy. User should already know.

I think I'm willing to mildly annoy the smallish number of legitimate
allow_unsafe_access users to help scare away all the people who like
shiny decode-dimms toys and enable this because some forum told them
to.  I could be convinced otherwise, though.

One other question: from my reading of the spec, it should be possible to
augment this driver to expose a temporate sensor subdevice that shows
recent cached temperatures from HW DIMM measurements.  They would be
redundant with the jc42 outputs, but it would be safe to use them even on
systems without safe SMBUS arbitration.  Should I do that as a followup
later on?

--Andy

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

[Andy Lutomirski]

On Sat, Mar 7, 2015 at 8:03 AM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 03/06/2015 06:50 PM, 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.
>>
>> 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>
>
>
> Similar to the other patch, I would suggest to run it through checkpatch.
>

Done for both patches.

>
>> ---
>>   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 d6b9ce164fbf..2ea6648492eb 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -149,6 +149,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 4287c891e782..a01bdcc0e239 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 000000000000..096842811199
>> --- /dev/null
>> +++ b/drivers/i2c/busses/dimm-bus.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * Copyright (c) 2013 Andrew Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>
>
> 2013 ?
>

Fixed.  It's been a long time since I wrote most of this code...

>
>> + *
>> + * 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.)
>> +        */
>
>
> That leads to the question if the controller supports quick commands,
> and if it does, if would make sense to add support for it,
> just for the purpose of reducing risk here.

I don't think it supports them, and even if it did, I don't know how
to test them since I don't have an obvious i2c slave device to poke at
with them.  I have a friend with logic analyzers, though...

>
>
>> +       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.
>
>
> Why not ?
>

It could make the legacy eeprom driver try to bind to the bus if it's
loaded.  I improved the comment.

>> + */
>> +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
>
>
> Not worth, or just "not probed" ?

Not obviously safe to probe, and not really useful even if we could do
it.  Comment updated.

>
>> +        *  - 0x18 - 0x1f: TSOD (Temperature Sensor on DIMM)
>> +        *
>> +        * There may be more some day.
>
>
> That is always a possibility. Does that really warrant this comment ?

No.  I removed that.

>
>> +        */
>> +       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);
>
>
> How are those devices removed ? Might warrant an explanation.
>

The core handles it.  I added a comment.

--Andy

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

[Guenter Roeck]

On 03/08/2015 07:03 AM, Andy Lutomirski wrote:
> On Mar 7, 2015 6:39 AM, "Guenter Roeck" <linux@roeck-us.net> wrote:
>>
>> On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
>>>
>>> Sandy Bridge Xeon and Extreme chips have integrated memory
>>> controllers with (rather limited) onboard SMBUS masters.  This
>>> driver gives access to the bus.
>>>
>>> There are various groups working on standardizing a way to arbitrate
>>> access to the bus between the OS, SMM firmware, a BMC, hardware
>>> thermal control, etc.  In the mean time, running this driver is
>>> unsafe except under special circumstances.  Nonetheless, this driver
>>> has real users.
>>>
>>> As a compromise, the driver will refuse to load unless
>>> i2c_imc.allow_unsafe_access=Y.  When safe access becomes available,
>>> we can leave this option as a way for legacy users to run the
>>> driver, and we'll allow the driver to load by default if safe bus
>>> access is available.
>>>
>>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>>> ---
[ ... ]

>>> +
>>> +       if (imc_wait_not_busy(priv, chan, &stat) != 0) {
>>> +               /* Timeout.  TODO: Reset the controller? */
>>> +               ret = -EIO;
>>
>>
>> timeout -> -ETIMEDOUT ?
>
> OK
>
Actually, I just realized that imc_wait_not_busy returns a valid error code.
Given that, some static analysis checkers (and now me) will ask you
why you don't just use the error code from imc_wait_not_busy.
This applies to other calls to the same function as well.

>>
>>
>>> +               dev_err(&priv->pci_dev->dev, "controller is wedged\n");
>>
>>
>> If this happens, it will presumably happen all the time and the message will
>> pollute the log. Is the message really necessary ?
>
> I'd rather log something to help diagnose.  Would rate-limiting it be okay?
>
It would still pollute the log because it doesn't happen that often.
A message once a second still fills the log.

If it is for diagnose/debugging, why not dev_dbg ?

>>
>>
>>> +               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,
>>
>>
>> Is a stack trace and dev_emerg really warranted here ?
>>
>
> If this happens, something's very wrong and the user should stop using
> the driver.  We could potentially write the wrong address, and, if we
> manage to screw up thermal management, we could potentially corrupt
> data for to an inappropriate refresh interval.
>
> IOW, I want to hear about it if this happens.
>
Ok, that explains the WARN. Still not an "emergency", though.

>>
>>> +                         "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;
>>
>>
>> ret is already guaranteed to be 0 here.
>>
>>
>>> +               } 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;
>>
>>
>> ret is already 0, so only the else case is really needed.
>>
>
> I wanted to keep the success and failure paths in the same order in
> both the read and write cases.  I'll remove the unnecessary
> assignment, though.
>
Coding style suggests

	if (!(stat & SMBSTAT_RDO)) {
		dev_err();
		ret - -EIO;
		goto out_release;
	}

and

	if (!(stat & SMBSTAT_WOD)) {
		dev_err();
		ret = -EIO;
		goto out_release;
	}

which would improve readability here a lot since it would reduce
indentation level by one.

On a side note, I am a bit confused about the note "same bug as in the read case".
Do you want to say that RDO is sometimes/often set in the write case ?
If so, it might make more sense to just say it.

[ ... ]

>>> +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. */
>>
>>
>> So ?
>
> It needs to happen before mutex_destroy.  I improved the comment.
>
Seems to me obvious that a mutex would be destroyed last in cleanup.

>>
>>
>>> +       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);
>>
>>
>> ret is a bit unusual as variable name for a pointer. dev, maybe ?
>>
>>
>>> +       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);
>>
>>
>> devm_kzalloc() ?
>>
>
> Done.
>
>>
>>> +       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++)
>>
>>
>>                          if (i)
>>                                  imc_free_channel(priv, 0);
>>
>> would be a bit simpler and accomplish the same.
>
> I want to be ready for future hardware that might support more than
> two channels.
>
Not my call to make, but I am a bit wary of future hardware support which may
never materialize. I prefer writing code liks this for the current use case.
The time to optimize the code for the future hardware is if and when the future
hardware materializes.

In general, I am also in favor of the guidance in the coding style document,
which suggests to have a single error exit and handle any necessary cleanup there.
In this case, it could be

		if (err)
			goto exit_cleanup;
...
exit_cleanup:
	for (i--; i >= 0; i--)
		imc_free_channel(priv, i);
exit_free:

...

>>> +       }
>>> +
>>> +       pr_warn("using this driver is dangerous unless your firmware is specifically designed for it; use at your own risk\n");
>>
>>
>> Seems to me this is a bit noisy. User should already know.
>
> I think I'm willing to mildly annoy the smallish number of legitimate
> allow_unsafe_access users to help scare away all the people who like
> shiny decode-dimms toys and enable this because some forum told them
> to.  I could be convinced otherwise, though.
>
Not my call to make ... you'll have to convince the maintainer.

Anyway, I wonder if it would make sense to use acpi_check_resource_conflict()
to check if there is a resource conflict with the BIOS instead of all these
warnings, and if that would answer the concerns about unsynchronized access.
 From looking into the datasheet, I don't really see the difference to the
i2c-i801 driver and other drivers where chip access might conflict with
BIOS / ACPI access. I may have missed some discussion, though, so maybe that
has been discussed already and doesn't work in this case.

> One other question: from my reading of the spec, it should be possible to
> augment this driver to expose a temporate sensor subdevice that shows
> recent cached temperatures from HW DIMM measurements.  They would be
> redundant with the jc42 outputs, but it would be safe to use them even on
> systems without safe SMBUS arbitration.  Should I do that as a followup
> later on?
>

Without thinking too much about it, this should be a separate driver,
and I think it might actually be more valuable (since less risky)
than this entire patch set.

Thanks,
Guenter

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

[Andy Lutomirski]

On Sun, Mar 8, 2015 at 8:39 AM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 03/08/2015 07:03 AM, Andy Lutomirski wrote:
>>
>> On Mar 7, 2015 6:39 AM, "Guenter Roeck" <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>>
>>>
>>> On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
>>>>
>>>>
>>>> Sandy Bridge Xeon and Extreme chips have integrated memory
>>>> controllers with (rather limited) onboard SMBUS masters.  This
>>>> driver gives access to the bus.
>>>>
>>>> There are various groups working on standardizing a way to arbitrate
>>>> access to the bus between the OS, SMM firmware, a BMC, hardware
>>>> thermal control, etc.  In the mean time, running this driver is
>>>> unsafe except under special circumstances.  Nonetheless, this driver
>>>> has real users.
>>>>
>>>> As a compromise, the driver will refuse to load unless
>>>> i2c_imc.allow_unsafe_access=Y.  When safe access becomes available,
>>>> we can leave this option as a way for legacy users to run the
>>>> driver, and we'll allow the driver to load by default if safe bus
>>>> access is available.
>>>>
>>>> Signed-off-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>>>> ---
>
> [ ... ]
>
>>>> +
>>>> +       if (imc_wait_not_busy(priv, chan, &stat) != 0) {
>>>> +               /* Timeout.  TODO: Reset the controller? */
>>>> +               ret = -EIO;
>>>
>>>
>>>
>>> timeout -> -ETIMEDOUT ?
>>
>>
>> OK
>>
> Actually, I just realized that imc_wait_not_busy returns a valid error code.
> Given that, some static analysis checkers (and now me) will ask you
> why you don't just use the error code from imc_wait_not_busy.
> This applies to other calls to the same function as well.

I changed it the other way.  One if the imc_wait_not_busy callers is
trying to get access to the bus in the first place.  If that fails,
then I think that -EBUSY is appropriate -- we're failing because the
thing is in use, not because our transaction timed out.  If the other
caller of imc_wait_not_busy fails, then our transaction timed out.  So
I'll make imc_wait_not_busy return bool.  This ends up simplifying the
code a bit, too.

>
>>>
>>>
>>>> +               dev_err(&priv->pci_dev->dev, "controller is wedged\n");
>>>
>>>
>>>
>>> If this happens, it will presumably happen all the time and the message
>>> will
>>> pollute the log. Is the message really necessary ?
>>
>>
>> I'd rather log something to help diagnose.  Would rate-limiting it be
>> okay?
>>
> It would still pollute the log because it doesn't happen that often.
> A message once a second still fills the log.
>
> If it is for diagnose/debugging, why not dev_dbg ?
>

OK.  I demoted some other log lines, too.

>>>
>>>
>>>> +               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,
>>>
>>>
>>>
>>> Is a stack trace and dev_emerg really warranted here ?
>>>
>>
>> If this happens, something's very wrong and the user should stop using
>> the driver.  We could potentially write the wrong address, and, if we
>> manage to screw up thermal management, we could potentially corrupt
>> data for to an inappropriate refresh interval.
>>
>> IOW, I want to hear about it if this happens.
>>
> Ok, that explains the WARN. Still not an "emergency", though.
>

It's dev_err now.

> Coding style suggests
>
>         if (!(stat & SMBSTAT_RDO)) {
>                 dev_err();
>                 ret - -EIO;
>                 goto out_release;
>         }
>
> and
>
>         if (!(stat & SMBSTAT_WOD)) {
>                 dev_err();
>                 ret = -EIO;
>                 goto out_release;
>         }

Done.  dev_dbg, too -- I think that either all of these errors should
be dev_dbg or none should be.

> On a side note, I am a bit confused about the note "same bug as in the read
> case".
> Do you want to say that RDO is sometimes/often set in the write case ?
> If so, it might make more sense to just say it.

Yes, fixed.

>
> [ ... ]
>
>>>> +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. */
>>>
>>>
>>>
>>> So ?
>>
>>
>> It needs to happen before mutex_destroy.  I improved the comment.
>>
> Seems to me obvious that a mutex would be destroyed last in cleanup.
>

It wasn't to me.  I'll remove the comment.


>> I want to be ready for future hardware that might support more than
>> two channels.
>>
> Not my call to make, but I am a bit wary of future hardware support which
> may
> never materialize. I prefer writing code liks this for the current use case.
> The time to optimize the code for the future hardware is if and when the
> future
> hardware materializes.
>
> In general, I am also in favor of the guidance in the coding style document,
> which suggests to have a single error exit and handle any necessary cleanup
> there.
> In this case, it could be
>
>                 if (err)
>                         goto exit_cleanup;
> ...
> exit_cleanup:
>         for (i--; i >= 0; i--)
>                 imc_free_channel(priv, i);
> exit_free:
>
> ...

Fair enough.  I did this.

>
>>>> +       }
>>>> +
>>>> +       pr_warn("using this driver is dangerous unless your firmware is
>>>> specifically designed for it; use at your own risk\n");
>>>
>>>
>>>
>>> Seems to me this is a bit noisy. User should already know.
>>
>>
>> I think I'm willing to mildly annoy the smallish number of legitimate
>> allow_unsafe_access users to help scare away all the people who like
>> shiny decode-dimms toys and enable this because some forum told them
>> to.  I could be convinced otherwise, though.
>>
> Not my call to make ... you'll have to convince the maintainer.
>
> Anyway, I wonder if it would make sense to use
> acpi_check_resource_conflict()
> to check if there is a resource conflict with the BIOS instead of all these
> warnings, and if that would answer the concerns about unsynchronized access.
> From looking into the datasheet, I don't really see the difference to the
> i2c-i801 driver and other drivers where chip access might conflict with
> BIOS / ACPI access. I may have missed some discussion, though, so maybe that
> has been discussed already and doesn't work in this case.

I don't think that BIOS will ever claim these resources.  It'll just
use them :(  Also, I don't think we can do this anyway -- the resource
we're accessing is a range of PCI configuration registers, not a BAR,
and I don't think that the resource system supports that.

In i2c-i801, I think that these issues are mostly mitigated by ACPI
code using OpRegions.

>
>> One other question: from my reading of the spec, it should be possible to
>> augment this driver to expose a temporate sensor subdevice that shows
>> recent cached temperatures from HW DIMM measurements.  They would be
>> redundant with the jc42 outputs, but it would be safe to use them even on
>> systems without safe SMBUS arbitration.  Should I do that as a followup
>> later on?
>>
>
> Without thinking too much about it, this should be a separate driver,
> and I think it might actually be more valuable (since less risky)
> than this entire patch set.

The main problem there is that they'll fight over the PCI ids.

--Andy

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

[Guenter Roeck]

On 03/08/2015 12:30 PM, Andy Lutomirski wrote:
[ ... ]

>>
>>> One other question: from my reading of the spec, it should be possible to
>>> augment this driver to expose a temporate sensor subdevice that shows
>>> recent cached temperatures from HW DIMM measurements.  They would be
>>> redundant with the jc42 outputs, but it would be safe to use them even on
>>> systems without safe SMBUS arbitration.  Should I do that as a followup
>>> later on?
>>>
>>
>> Without thinking too much about it, this should be a separate driver,
>> and I think it might actually be more valuable (since less risky)
>> than this entire patch set.
>
> The main problem there is that they'll fight over the PCI ids.
>
That is why we have mfd drivers. While that most likely won't be a
solution here (and I don't suggest it), you can use the same idea.
If can not be instantiated as pci driver, the alternative would be
to instantiate it as platform driver and let it get its pci
information from the parent device.

Guenter

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

[Andy Lutomirski]

On Sun, Mar 8, 2015 at 12:50 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 03/08/2015 12:30 PM, Andy Lutomirski wrote:
> [ ... ]
>
>>>
>>>> One other question: from my reading of the spec, it should be possible
>>>> to
>>>> augment this driver to expose a temporate sensor subdevice that shows
>>>> recent cached temperatures from HW DIMM measurements.  They would be
>>>> redundant with the jc42 outputs, but it would be safe to use them even
>>>> on
>>>> systems without safe SMBUS arbitration.  Should I do that as a followup
>>>> later on?
>>>>
>>>
>>> Without thinking too much about it, this should be a separate driver,
>>> and I think it might actually be more valuable (since less risky)
>>> than this entire patch set.
>>
>>
>> The main problem there is that they'll fight over the PCI ids.
>>
> That is why we have mfd drivers. While that most likely won't be a
> solution here (and I don't suggest it), you can use the same idea.
> If can not be instantiated as pci driver, the alternative would be
> to instantiate it as platform driver and let it get its pci
> information from the parent device.
>

Hmm.  It could make sense to have an Intel chipset driver that
instantiates sub-devices for thermal monitoring, smbus, and EDAC.  It
might be a bit of a cleanup for sb-edac, too.

Anyway, this isn't currently necessary.  Let's deal with it when we get there.

--Andy