Subject: [PATCH v4] 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/20260620144131.415559-1-simone.chifari@gmail.com/

v3 was never posted.

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/1582498270-50674-1-git-send-email-schaecsn@gmx.net/

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 engine only ever clears GO, so that
    is the single permitted difference between the two words.

  - 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.  That is not
advertised here because it has not been validated on the Linux system, so
i2cdetect cannot scan these buses; devices are addressed directly.

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.

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.
  - Read-only 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, ENXIO for an absent device, 100 concurrent reads
    per channel, the default-off gate, and 20 unload/reload cycles: 15
    checks, 0 failures, no kernel warning or oops, no interference
    reported, every command word confirmed in the register.
  - Engine registers sampled every 2 s for one hour with the driver
    unloaded: 1791 samples, 0 changes.  No firmware activity was observed
    on this system.
  - 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, and suspend/resume testing with active I2C clients.

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