[PATCH v4 0/1] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter

The integrated memory controller of Intel Skylake-X and Cascade Lake-X
processors contains an SMBus engine wired to the DDR4 DIMMs.  This driver
exposes its two channels as standard Linux I2C adapters, so that DDR4 SPD
EEPROMs and DIMM thermal sensors can be read with i2c-tools, decode-dimms
and lm-sensors.

v2 was posted as i2c-imc-x299 and received no replies:

  https://lore.kernel.org/linux-i2c/[email protected]/

v3:

  https://lore.kernel.org/linux-i2c/[email protected]/

Correction to v2
~~~~~~~~~~~~~~~~
v2 claimed that firmware filters CF8/CFC config writes to this function and
that the registers can only be reached through the ECAM window.  That claim
was wrong and I am retracting it.

It was never tested directly.  The only evidence behind it was the boot log
line "PCI: Using configuration type 1", which says which accessor the kernel
uses and nothing about a write failing.  Measured on the same board:

  # setpci -s 16:1e.5 9c.L=20085002    # read SPD 0x50, register 0x02
  # setpci -s 16:1e.5 9c.L
  20005002                             # GO consumed
  # 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 written through CF8/CFC is executed and returns correct data,
and MSR_SMI_COUNT does not move across it.  This driver therefore uses
pci_read_config_dword() and pci_write_config_dword() like any other driver.
There is no ECAM mapping, no MCFG parsing and no PCI-core change in this
series any more.

Firmware arbitration
~~~~~~~~~~~~~~~~~~~~
This is the reason no iMC SMBus driver has ever been merged, and this patch
does not solve it either.  It does make the condition detectable.

Andy Lutomirski's original i2c_imc put it plainly: a BMC, the memory
controller's power management, and possibly SMM all like to touch this bus,
and Intel defined no way to arbitrate between them.  That driver already had
a module parameter called allow_unsafe_access.  Stefan Schaeckeler's 2020
Broadwell rewrite went further and stopped the PCU's TSOD polling for the
duration of a transfer (write 0 to TSODCNTL, wait 10 ms for in-flight
transactions to drain, restore afterwards), because on Broadwell the
original driver's consistency check tripped about every 30 minutes.

  https://lwn.net/Articles/635898/
  https://lore.kernel.org/linux-i2c/[email protected]/

So this driver:

  - keeps the default-off allow_unsafe_access gate, deliberately the same
    name as the original;
  - serializes both channels under one mutex and waits for the whole engine
    to be idle before touching any register;
  - saves the command state of both channels, clears TSOD_ACTIVE for the
    duration of the transfer, and restores both afterwards;
  - after every transfer compares the command registers against what it
    left in them and fails the transfer with -EAGAIN if anything else
    changed.  This is the check the original driver had, and it is what
    makes an unarbitrated system detectable rather than silently wrong;
  - reads the command word back and requires it to be the one just
    written, because the done bits are latched.  With the driver unloaded
    and nothing running, STATUS on the test system reads 0x0000300c:
    READ_DONE and WRITE_DONE both still set from an earlier transaction.
    Without the check, a command word that never reaches the register is
    indistinguishable from a completed transfer and the caller receives the
    previous transaction's data.  The comparison covers the bits the
    driver produces, minus GO, which the engine clears itself;
  - documents the one case the read-back check cannot see: two
    consecutive, identical commands - the shape of an ee1004 Receive Byte
    stream - leave the same word in the register, so a command write
    dropped between them is invisible.  Bit 29 would be the natural
    per-command nonce, but the engine refuses it: measured on this
    system, a command with bit 29 clear consumes GO and completes with
    the error bit set, on reads that succeed with the bit set.  Every
    dropped write between commands that differ in address, register,
    direction or size is caught.

  - refuses to bind when the memory controller is polling the DIMM thermal
    sensors itself.  Intel documents SMBCNTL on the iMC channel function
    (device 10 and 12, function 0, offsets 0xe88 and 0xe98) in the Xeon
    Processor Scalable Family datasheet volume 2, reference 614073, section
    3.1.9.  Its SMB_TSOD_POLL_EN bit is documented as mutually exclusive
    with SPD command access, so it answers directly whether the engine is
    in use.  On the test system all four channels read 0xac000000: polling
    disabled, TSOD_PRESENT mask empty, which also explains why no jc42
    device appears at 0x18-0x1f.

This is deliberately weaker than what the Broadwell-E driver did.  That one
switched the polling off for the duration of a transfer.  Reading the bit
and declining is a smaller claim on hardware the driver does not own, and
it needs no write to a firmware-configured register.  It covers the memory
controller only; SMM and a BMC remain out of reach, which is what the
opt-in is for.

Relation to prior work
~~~~~~~~~~~~~~~~~~~~~~
There is no iMC I2C driver in mainline.  Lutomirski's i2c-imc was an RFC;
the 2020 resend and github.com/Linux4/i2c-imc are out of tree.  Neither can
be extended to cover this part: the device ID (0x2085 vs 0x3ca8/0x6fa8) and
the register layout (0x9C-0xB8 vs 0x180-0x188) differ.  A separate file is
cleaner than a hw_data table.

The register encoding was cross-checked against an independent Windows
implementation for the same engine, which predates this work:

  https://github.com/namazso/PawnIO.Modules/blob/c975ca1d1b8a9c7473a175fa1ca24d83b6cbc8f3/SmbusIntelSkylakeIMC.p

It also implements SMBus BYTE through a pointer-select bit, without saying
which bit.  That bit is 18 here.  The datasheet gave it away: SMBCMD on the
iMC channel functions places PNTR_SEL immediately above WORD_ACCESS, and
word access on the PCU function is bit 17.

Bit 18 is the only part of this driver that rests on an undocumented
encoding, so it is worth saying exactly what was measured rather than
asserting it works.  With the bit set on a read, eight consecutive
transfers to an SPD EEPROM returned bytes 0x01 through 0x08 in order,
matching an ordinary dump of the same range.  That is the auto-increment
of a Receive Byte; a transfer that still named a register would have
returned the same byte eight times.  With the bit set on a write, a Send
Byte to the DDR4 page-select addresses moves the 256-byte window: byte
0x00 stops reading as the JEDEC 0x23 after selecting page 1 and returns
to it after selecting page 0.

ee1004 consequently binds, and decode-dimms reads all four DIMMs on this
system with both SPD CRCs valid on each.  i2cdetect scans in read mode.
jc42 needs only BYTE_DATA and WORD_DATA reads, which are implemented and
validated against the SPDs; jc42 itself has not been exercised, because no
DIMM on the test system populates a thermal sensor (TSOD_PRESENT reads
0x00).

With Receive Byte in place the driver can also do what the other SMBus
host drivers do and call i2c_register_spd() per adapter, so the SPD
EEPROMs are instantiated at probe rather than by hand.  The helper counts
the populated slots from DMI and probes upwards from 0x50, leaving alone
any address that does not answer; each channel carries half the DIMMs and
the scan is per adapter, so each finds its own.  Nothing else is
instantiated, jc42 included: there is no firmware description of these
buses and this driver does not scan for anything but the SPDs.

I used the write_disable variant.  This part is DDR4 only, where the two
are equivalent - the distinction gates spd5118 on DDR5 - and between two
equivalent calls the one that does not read as "this driver enables SPD
writes" seemed the better thing to leave in a memory-bus driver.  Happy
to switch to write_enable for consistency with i801 and piix4 if you
prefer the convention.

SMBus Quick is still not implemented, and not only because the engine has
no obvious encoding for it.  A write to the 0x30-0x37 range is how an
EE1004 SPD is write-protected, permanently on many modules.  An adapter
whose bus is the memory bus should not advertise a probe that writes.

Changes since v3
~~~~~~~~~~~~~~~~
All of these came out of a self-review of v3; none changes what the driver
supports.

  - Self-review found the read-back check blind for two identical
    commands in a row - precisely the shape of an ee1004 Receive Byte
    stream.  The obvious fix, alternating bit 29 per command, was
    implemented and then reverted after measurement: the engine consumes
    GO but sets the error bit for every command with bit 29 clear, so
    the bit cannot serve as a nonce.  The limitation is now documented
    at the check instead of silently present.
  - Compare only the bits the driver produces in the read-back check, and
    use the post-write read-back, not the pre-transfer word, as the
    baseline for the interference check on the driven channel.  v3
    implicitly required every undocumented bit of the command register to
    read as zero, which held on the test system but is not something to
    assume across steppings.
  - Read the result before the interference check, not after, so the check
    also covers the window in which the data latch is read.
  - Re-run the TSOD-polling check on resume and keep the adapters
    suspended if firmware enabled it across suspend; the probe-time
    refusal alone cannot see that.  The S3 observation below is exactly
    the scenario that motivates this.
  - Stop before writing a saved command word back if the config read that
    fetched it had already failed.
  - Rename the register accessors: the command register is now CMD and
    the data latch DATA.  v3 called the command register DATA and the
    data latch CTRL, which read backwards.
  - State plainly that jc42 itself has not been run (no TSOD populated
    here), only the transactions it needs.
  - Note that PCI ID 8086:2085 also matches Skylake-SP/Cascade Lake-SP
    servers, where a BMC is far more likely to own the engine; the gate,
    the SMBCNTL refusal and the interference detection are the same, but
    the audience of the warning is different.
  - Correct a stale function name in the v3 cover letter:
    imc_engine_responded() is imc_command_landed() in the code as posted.
  - Cast ~COMMAND_OUR_BITS to u32 in the interference check; BIT() and
    GENMASK() are unsigned long, so the complement was computed in 64
    bits and truncated on assignment.  The truncated value was already
    correct; this silences a clang W=1 -Wconstant-conversion warning.
    Reported-by: kernel test robot <[email protected]>
    Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

Changes since v2
~~~~~~~~~~~~~~~~
  - Retract the CF8/CFC claim; use the ordinary config accessors.  The
    driver no longer maps anything.
  - Rename i2c-imc-x299 to i2c-imc-skylake; the engine is in the CPU, not
    the chipset.
  - Add the default-off allow_unsafe_access gate.
  - Wait for both channels to be idle before touching the data latch.
  - Preserve and restore command and TSOD state for both channels.
  - Validate READ_DONE, WRITE_DONE and the hardware error state.
  - Detect interference from another master and report it as -EAGAIN.
  - Read the command word back and require it to be the one written.
  - Add bounded command-toggle recovery for a stuck BUSY state.
  - Add WORD_DATA support with the hardware byte ordering.
  - Reject I2C_M_TEN explicitly and validate transfer direction.
  - Use the i2c core's suspended-adapter mechanism instead of a private
    flag and a non-standard errno.
  - Use devm_mutex_init() and devm-managed adapters; drop the unnecessary
    pci_enable_device(); return -ENODEV rather than -EPERM from the
    default-off gate, which was logging a driver-core error on every boot.
  - Add Documentation/i2c/busses/i2c-imc-skylake.rst.
  - Drop the ACPI dependency.
  - Refuse to bind when the iMC reports TSOD polling enabled.
  - Correct the claim that BYTE_DATA and WORD_DATA cover ee1004: they do
    not, ee1004 also needs Receive Byte and Send Byte.
  - Implement both, on the pointer-select bit, so ee1004 binds and
    decode-dimms works.
  - Instantiate the SPD EEPROMs with i2c_register_spd() instead of
    leaving it to the user.

Validation
~~~~~~~~~~
  - In-tree build, W=1, x86_64 GCC and arm64 Clang COMPILE_TEST: no
    diagnostics.
  - checkpatch.pl --strict: clean.
  - Sparse with __CHECK_ENDIAN__: clean.
  - Hardware test on one Intel Core i9-10900X (Cascade Lake-X) system with
    four DDR4 DIMMs: both adapters, four SPD EEPROMs, BYTE_DATA, WORD_DATA
    byte order, Receive Byte tracking the EEPROM pointer, Send Byte moving
    the SPD page and putting it back, ENXIO for an absent device, 100
    concurrent reads per channel, the default-off gate, and 20
    unload/reload cycles: 19 checks, 0 failures, no kernel warning or oops,
    no interference reported, every command word confirmed in the register.
  - i2c_register_spd() instantiated ee1004 on all four populated SPD
    addresses at probe, two per channel, and claimed none of the empty
    ones.  decode-dimms then decoded all four modules, each with the CRC
    of bytes 0-125 and of bytes 128-253 valid.  The part number reads out
    of page 1, so the page select is exercised end to end.  The four
    module serial numbers were identical before and after every
    experiment in this series.
  - Engine registers sampled every 2 s for one hour with the driver
    unloaded: 1791 samples, 0 changes.  No firmware activity while the
    system is up.
  - Suspend and resume, s2idle and S3, with continuous traffic on both
    adapters: transfers recover and nothing complains.  Across S3 the
    firmware demonstrably drives the engine.  Both channels, sampled
    immediately before suspend and immediately after resume:

      DATA   0x2000b600 -> 0x20005000
      STATUS 0x0000500c -> 0x00005004

    The command word came back addressing SPD 0x50 for a read, and the
    status went from both done bits latched to READ_DONE alone.  Nothing in
    Linux issued that: the adapter was marked suspended and userspace was
    frozen.  This is consistent with firmware reading SPD during memory
    training on resume.  It is the only direct observation of a second
    master on this engine I have, and it is the concrete reason the driver
    restores command state per transfer and compares it afterwards.
  - SMBCNTL on all four iMC channels reads 0xac000000: TSOD polling
    disabled, TSOD_PRESENT empty.
  - Writes validated end to end against a populated device: a 12-write
    frame to an LED controller at 0x27, across both channels, every write
    acknowledged, no NACK, and the device applied the result.  A byte
    write to an unpopulated address returns -ENXIO with the engine
    reporting the address NACK.  No SPD was written at any point, and the
    four SPD EEPROMs were verified present with valid CRCs before and
    after.

Still missing: an independent Tested-by, ideally on Skylake-X rather than
Cascade Lake-X.

One note on the register interface, since a reader with the datasheet will
ask.  That document describes an SMBus interface on the iMC channel
function at 0xe80-0xe9c, with a different encoding from the one this driver
uses on PCU function 5 at 0x9c-0xb8; it does not describe function 5 at all,
covering device 30 functions 0 and 2 only.  I tried the documented path on
this system: issuing SMBCMD with CMD_TRIGGER sets SMB_RDO and clears
SMB_BUSY, but returns 0x00 for a populated DIMM at 0x50, for a second one
at 0x52, and for an empty slot at 0x57 alike, with SMB_SBE never set.  An
interface that cannot tell a populated slot from an empty one is not
reaching the bus here, while the path this driver uses returns the correct
SPD contents and a NACK for the empty slot.  I may well be missing a setup
step; if someone knows what it is, I would rather use the documented
interface.

Tool assistance
~~~~~~~~~~~~~~~
Per Documentation/process/generated-content.rst: this work was developed
with an AI coding assistant, run interactively rather than autonomously.

  - What: Claude Code, model claude-opus-5.
  - Where: it contributed to the transaction state machine, the .rst
    documentation, the commit message and this cover letter.  The register
    decoding is mine.
  - How it was used: iteratively, one reviewed change at a time, in
    response to specific requests, not from a single generating prompt.  A
    representative request is "the done bits are latched; show me how a
    command that is never accepted can be mistaken for a completed one, and
    make that case detectable", which produced imc_command_landed().  It
    also designed the measurement that retracted the CF8/CFC claim above,
    after asking why that claim had never been tested directly.
  - Testing: every revision was built with W=1, checked with
    checkpatch --strict and sparse, and tested on the hardware by me.

I have reviewed all of it, I understand it, and I take responsibility for
it.

Simone Chifari (1):
  i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter

 Documentation/i2c/busses/i2c-imc-skylake.rst | 196 +++++
 Documentation/i2c/busses/index.rst           |   1 +
 MAINTAINERS                                  |   7 +
 drivers/i2c/busses/Kconfig                   |  30 +
 drivers/i2c/busses/Makefile                  |   1 +
 drivers/i2c/busses/i2c-imc-skylake.c         | 780 +++++++++++++++++++
 6 files changed, 1015 insertions(+)
 create mode 100644 Documentation/i2c/busses/i2c-imc-skylake.rst
 create mode 100644 drivers/i2c/busses/i2c-imc-skylake.c


base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
-- 
2.43.0