Simone/i2c-imc-skylake
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

i2c-imc-skylake: retract the CF8/CFC claim, harden the state machine

Browse Source 
v2 said firmware filters CF8/CFC config writes to this function and that
the registers are reachable only through ECAM.  That was never tested; the
only evidence was the boot line "PCI: Using configuration type 1", which
names the accessor and says nothing about a write failing.

Measured on the board, driver unloaded:

  # setpci -s 16:1e.5 9c.L=20085002    # read SPD 0x50 register 0x02
  # setpci -s 16:1e.5 9c.L
  20005002                             # GO consumed by the engine
  # setpci -s 16:1e.5 a8.L
  0000500c                             # READ_DONE, no error
  # setpci -s 16:1e.5 b4.L
  0000000c                             # SPD byte 2, DDR4 type code

The command executes and returns correct data through the ordinary
accessors, and MSR_SMI_COUNT does not move across it.  So the driver now
uses pci_read_config_dword() and pci_write_config_dword(), the submission
is a single patch again, and nothing in it touches the PCI core.  The
cover letter retracts the claim explicitly rather than dropping it
quietly, because v2 was posted with it.

Correctness work in the same pass:

  - the completion bits are latched, so a command word that never reaches
    the register is indistinguishable from a completed transfer and the
    caller gets the previous transaction's data.  Read the command word
    back and require it to be the one just written; the engine only ever
    clears GO, which the measurement above shows directly;
  - compare the command registers after every transfer against what the
    driver left in them and fail with -EAGAIN on a mismatch.  The original
    iMC driver had this check and it is what makes an unarbitrated system
    detectable instead of silently wrong;
  - use the i2c core's suspended-adapter mechanism rather than a private
    flag and -EHOSTDOWN;
  - use devm_mutex_init(), drop the unnecessary pci_enable_device(), and
    return -ENODEV rather than -EPERM from the default-off gate, which was
    printing a driver-core error on every boot;
  - mask GO when restoring a saved command word.

An earlier version of the latched-DONE check required the GO bit to be
seen set or the status word to change.  That was wrong: repeated transfers
with the same outcome leave STATUS bit for bit identical, and GO is
consumed faster than a config read-back can observe it.  It produced false
positives on 3 of 15 smoke-test items before being replaced.

Add Documentation/i2c/busses/i2c-imc-skylake.rst, which every other PC
SMBus driver has and which was asked for on the 2020 iMC thread, and the
tools/ scripts for the hardware measurements: probe-cf8-write.sh and
probe-cf8-transaction.sh produced the result above, dump-pcu-config.sh is
the search for a TSOD polling control, watch-engine.sh is the firmware
activity measurement.

Smoke test on the X299 system with the current module: 15 checks, 0
failures, dmesg clean.
This commit is contained in:
Simone
2026-07-26 19:51:44 +02:00
parent 3cd1d6e0a6
commit 88a8b7eb27
13 changed files with 320 additions and 651 deletions
Download Patch File Download Diff File Expand all files Collapse all files
i2c-imc-skylake.c
+43 -101
View File
@@ -3,23 +3,12 @@
* Intel Skylake-X iMC SMBus I2C adapter.
*
* The integrated memory controller (iMC) on Intel Skylake-X / Cascade Lake-X
* processors exposes an SMBus engine used to reach the SPD EEPROMs and thermal
* sensors on DDR4 DIMMs. The engine is driven through the PCI configuration
* space of the Sky Lake-E PCU function (8086:2085). This driver presents that
* engine as two standard Linux I2C adapters - one per hardware SMBus channel -
* so that i2c-tools and lm-sensors can use it without bespoke sysfs hacks.
*
* Why the config accesses go through pci_mmcfg_{read,write}_config():
* The SMBus registers live at offsets 0x9C-0xB8, inside the first 256 config
* bytes. raw_pci_read()/raw_pci_write() dispatch that range to raw_pci_ops,
* which on this platform is CF8/CFC (the boot log reports "PCI: Using
* configuration type 1"). On the tested board a dword written that way to
* the command register does not take effect: the register reads back without
* the GO bit ever having been consumed and no SMBus transaction is issued.
* The same write through the ECAM window does work, and that is also how the
* register layout was confirmed on hardware. The mechanism behind the
* dropped write has not been identified; measurements of MSR_SMI_COUNT
* across a failing write show no SMI, so it is not attributed to SMM here.
* processors contains an SMBus engine wired to the DDR4 DIMMs. It reaches the
* SPD EEPROMs (0x50-0x57) and the thermal sensors (0x18-0x1f) of the installed
* modules. The engine is driven through the PCI configuration space of the
* Sky Lake-E PCU function (8086:2085). This driver presents it as two standard
* Linux I2C adapters - one per hardware SMBus channel - so that i2c-tools,
* decode-dimms and lm-sensors can use it without bespoke sysfs hacks.
*
* Per-channel register triple within the config space of the function:
* ch0 ch1
@@ -30,12 +19,11 @@
* SMBus command byte (DATA bits[15:8]) = (rw << 7) | addr7: the 7-bit slave
* address with bit7 = direction (1 write / 0 read). The register/offset goes
* in DATA[7:0]. So 0x50 reads SPD EEPROM 0x50. This was decoded and
* confirmed on hardware.
* confirmed on hardware, and cross-checked against an independent Windows
* implementation of the same engine.
*
* The two channels are independent SMBus buses (DIMMs 1,2 on ch0; DIMMs 3,4 on
* ch1), hence two i2c_adapter instances exposed as separate /dev/i2c-* nodes.
* Each channel carries the DIMM SPD EEPROMs (0x50-0x57) and thermal sensors -
* all reachable by 7-bit address.
*
* Bus arbitration / concurrency with firmware:
* The driver cannot coordinate with SMM, a BMC, or the iMC's own closed-loop
@@ -66,15 +54,10 @@
#include <linux/pci.h>
#include <linux/pci_ids.h>
/* expected PCI id of the Sky Lake-E PCU SMBus function */
/* PCI id of the Sky Lake-E PCU function carrying the SMBus engine */
#define PCU_DEVICE 0x2085
#define PCU_ID ((PCU_DEVICE << 16) | PCI_VENDOR_ID_INTEL)
/* function-global config registers */
#define CFG_VENDOR_DEV 0x00 /* cfg[0]: vendor/device id, for sanity check */
#define CFG_IMC_BUS 0xCC /* cfg[0xCC] bits[15:8]: iMC SMBus bus number */
/* per-channel register offsets */
/* per-channel register offsets in the function's config space */
#define CH0_CTRL 0xB4
#define CH0_DATA 0x9C
#define CH0_STAT 0xA8
@@ -103,8 +86,10 @@
#define WRITE_OPERATION BIT(15)
#define COMMAND_PREFIX (COMMAND_TOGGLE | GO_BIT)
#define COMMAND_KEEP_MASK (~TSOD_ACTIVE_BIT)
/* command bits this driver produces itself; everything else belongs to nobody
* we know about, so a change in them during a transfer means interference.
/*
* Command bits this driver produces itself. Everything outside this mask
* belongs to whoever else is driving the engine, so a change there during a
* transfer means interference.
*/
#define COMMAND_OUR_BITS (COMMAND_TOGGLE | GO_BIT | WORD_BIT | \
GENMASK(15, 0))
@@ -157,20 +142,19 @@ struct imc_xfer_state {
};
/*
* Config accessors. Every access goes through the ECAM path; see the comment
* at the top of the file. A failure is recorded in s->io_err and reported
* once, at the end of the transfer, so the transaction logic does not have to
* check a return value on every register touch. A failed read yields all-ones,
* which no polling loop mistakes for a completed transaction.
* Register accessors. A config-space failure is recorded in s->io_err and
* reported once, at the end of the transfer, so the transaction logic does not
* have to check a return value on every register touch. A failed read yields
* all-ones, which no polling loop mistakes for a completed transaction.
*/
static u32 imc_reg(struct imc_smbus *s, unsigned int off)
{
u32 val;
int ret;
ret = pci_mmcfg_read_config(s->pdev, off, 4, &val);
ret = pci_read_config_dword(s->pdev, off, &val);
if (ret) {
s->io_err = ret;
s->io_err = pcibios_err_to_errno(ret);
return ~0U;
}
@@ -181,9 +165,9 @@ static void imc_set(struct imc_smbus *s, unsigned int off, u32 val)
{
int ret;
ret = pci_mmcfg_write_config(s->pdev, off, 4, val);
ret = pci_write_config_dword(s->pdev, off, val);
if (ret)
s->io_err = ret;
s->io_err = pcibios_err_to_errno(ret);
}
static u32 imc_busy_mask(struct imc_smbus *s)
@@ -255,7 +239,6 @@ static int imc_wait_transfer(struct imc_smbus *s, const struct imc_chan *c,
command = imc_reg(s, c->data);
imc_set(s, c->data, command ^ COMMAND_TOGGLE);
imc_reg(s, c->data);
ret = imc_wait_done(s, c, status);
if (ret)
@@ -287,19 +270,21 @@ static int imc_check_status(struct imc_smbus *s, const struct imc_chan *c,
}
/*
* The done bits are latched and the driver has found no way to clear them, so
* a transaction that is never started at all would otherwise be indistinguish-
* able from one that completed: STATUS still shows the previous DONE and the
* caller would receive the previous transaction's data as valid.
* The done bits are latched and no way to clear them has been found, so a
* command that never reaches the register is indistinguishable from one that
* completed: STATUS still shows the previous DONE, the GO bit of the stale
* command word is already clear, and the caller would receive the previous
* transaction's data as valid.
*
* Require one of two positive signs that the engine acted on the command: the
* GO bit was observed set in the read-back that follows the command write, or
* STATUS changed during the transfer. Neither of them appearing means nothing
* happened.
* Read the command word back and require it to be the one just written. The
* engine only ever clears GO, so that bit is the single permitted difference;
* anything else means the write did not land. Checking the word rather than
* a status transition matters because two identical transfers in a row leave
* STATUS bit for bit the same.
*/
static bool imc_engine_responded(bool go_seen, u32 status_pre, u32 status)
static bool imc_command_landed(u32 command, u32 readback)
{
return go_seen || status != status_pre;
return !((command ^ readback) & ~GO_BIT);
}
/*
@@ -345,7 +330,6 @@ static void imc_restore_xfer(struct imc_smbus *s,
* transaction that was already in flight when we found it.
*/
imc_set(s, imc_chans[i].data, state->command[i] & ~GO_BIT);
imc_reg(s, imc_chans[i].data);
}
}
@@ -367,7 +351,6 @@ static int imc_prepare_xfer(struct imc_smbus *s, const struct imc_chan *c,
imc_set(s, imc_chans[i].data,
state->command[i] & COMMAND_KEEP_MASK);
imc_reg(s, imc_chans[i].data);
ret = imc_wait_go_clear(s, &imc_chans[i]);
if (ret) {
imc_restore_xfer(s, state);
@@ -382,8 +365,7 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr,
u8 reg, char read_write, int size, u16 *value)
{
struct imc_xfer_state state = {};
u32 command, expected, raw, status_pre, status = 0;
bool go_seen;
u32 command, expected, raw, readback, status = 0;
int ret;
ret = imc_prepare_xfer(s, c, &state);
@@ -397,27 +379,23 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr,
if (read_write == I2C_SMBUS_WRITE) {
raw = size == I2C_SMBUS_WORD_DATA ? swab16(*value) : *value;
imc_set(s, c->ctrl, raw << 16);
imc_reg(s, c->ctrl);
command |= WRITE_OPERATION;
}
status_pre = imc_reg(s, c->stat);
imc_set(s, c->data, command);
/* read back to flush the write and to see whether GO was accepted */
go_seen = imc_reg(s, c->data) & GO_BIT;
readback = imc_reg(s, c->data);
if (!imc_command_landed(command, readback)) {
dev_warn_ratelimited(s->dev,
"ch%d command 0x%08x did not reach the register (read back 0x%08x)\n",
c->idx, command, readback);
ret = -EIO;
goto restore_command;
}
ret = imc_wait_transfer(s, c, &status);
if (ret)
goto restore_command;
if (!imc_engine_responded(go_seen, status_pre, status)) {
dev_warn_ratelimited(s->dev,
"ch%d command not accepted for addr 0x%02x reg 0x%02x (stat 0x%08x unchanged, GO never seen)\n",
c->idx, addr, reg, status);
ret = -ETIMEDOUT;
goto restore_command;
}
expected = read_write == I2C_SMBUS_WRITE ?
STAT_WRITE_DONE : STAT_READ_DONE;
ret = imc_check_status(s, c, status, expected, addr, reg);
@@ -549,8 +527,6 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume);
static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct imc_smbus *s;
u8 imc_bus_hw;
u32 cfg0, cc;
int ret, i;
if (!allow_unsafe_access) {
@@ -575,40 +551,6 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (ret)
return ret;
ret = pci_mmcfg_read_config(pdev, CFG_VENDOR_DEV, 4, &cfg0);
if (ret) {
dev_err(&pdev->dev,
"no memory-mapped path to config space: %d\n", ret);
return ret;
}
if (cfg0 != PCU_ID) {
dev_err(&pdev->dev, "wrong device through ECAM (cfg[0]=0x%08x)\n",
cfg0);
return -ENODEV;
}
/*
* Cross-check the iMC bus number against the configuration register
* value: cfg[0xCC] bits[15:8] = the iMC SMBus bus number as seen by
* the PCU. On all known Skylake-X / Cascade Lake-X boards this matches
* the probed bus number.
* A mismatch means the accessor landed on the wrong function - warn but
* continue; the binding is already locked to 8086:2085.
*/
ret = pci_mmcfg_read_config(pdev, CFG_IMC_BUS, 4, &cc);
if (ret)
return ret;
imc_bus_hw = (cc >> 8) & 0xFF;
if (imc_bus_hw && imc_bus_hw != (u8)pdev->bus->number)
dev_warn(&pdev->dev,
"cfg[0xCC] reports iMC bus 0x%02x but probed bus=0x%02x\n",
imc_bus_hw, (u8)pdev->bus->number);
else
dev_dbg(&pdev->dev,
"cfg[0xCC]=0x%08x iMC bus 0x%02x confirmed\n",
cc, imc_bus_hw);
/*
* Lifetime safety: the I2C core guarantees that smbus_xfer callbacks
* are not invoked after i2c_del_adapter() returns. Since we use