i2c-imc-skylake/docs/submission/cover-letter.txt

Subject: [RFC PATCH v4 0/2] i2c: imc-skylake: Intel Skylake-X iMC SMBus adapter

This series exposes the two SMBus channels of the integrated memory
controller of Intel Skylake-X and Cascade Lake-X processors 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.

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

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

Patch 1 is a PCI/x86 change and is the reason this is still an RFC.
Patch 2 is the driver and depends on patch 1.

  1/2  PCI: allow config access forced through the memory-mapped path
  2/2  i2c: imc-skylake: add driver for the Intel Skylake-X iMC SMBus engine

Why patch 1 exists
~~~~~~~~~~~~~~~~~~
The engine's registers are at config space offsets 0x9C-0xB8, inside the
first 256 bytes. On x86, raw_pci_read()/raw_pci_write() dispatch that range
to raw_pci_ops, which is CF8/CFC whenever configuration type 1 is usable
(arch/x86/pci/common.c). The extended path, raw_pci_ext_ops, is only
consulted from offset 0x100 upwards.

On the tested board a dword written to the command register through
CF8/CFC does not take effect: the register reads back unchanged, the GO bit
is never consumed and no SMBus transaction is issued. The same write through
the ECAM window works, and that is how the register layout was confirmed.

This is not only a problem for this driver. If CF8/CFC writes to that
function are dropped, then setpci, sysfs and anything else the kernel does
to its low config space is equally unreliable. So rather than have a leaf
driver map the ECAM page itself, patch 1 adds

  int pci_mmcfg_read_config(struct pci_dev *, int where, int size, u32 *val);
  int pci_mmcfg_write_config(struct pci_dev *, int where, int size, u32 val);

in drivers/pci/access.c, taking pci_lock and honouring
pci_cfg_access_lock(), on top of a weak arch_pci_mmcfg_access() hook that
x86 implements over the already-validated pci_mmcfg_list. No MCFG re-parsing
and no second mapping of a window the architecture has already mapped.

Questions for the PCI and x86 maintainers:

  - is an explicit per-access opt-in the right shape, or should a platform
    that filters CF8/CFC for a function be handled by a quirk that redirects
    all of its config accesses?
  - if a helper is right, is drivers/pci/access.c plus a weak arch hook the
    right place, or should this live entirely in arch/x86?

I do not consider patch 1 settled and will rework it in whatever direction
you point.

What causes the dropped write is not known. Earlier revisions of this
driver attributed it to System Management Mode; the only evidence for that
was the boot log line naming the access method, which says nothing about
who filters the write, so the attribution has been removed everywhere. The
observation stands on its own: the write does not reach the register.

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

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;
  - requires a positive sign that the engine acted on the command, because
    the done bits are latched: without that, a command that is never
    accepted is indistinguishable from a completed one and the caller would
    receive the previous transaction's data.

What is missing: no equivalent of Broadwell's TSODCNTL has been identified
on this part, so a system with CLTT enabled is expected to trip the
interference check rather than be arbitrated against. Finding that register
is the main open item. I would rather ship a driver that detects the
condition and refuses than one that quietly races.

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),
the register layout (0x9C-0xB8 vs 0x180-0x188) and the working access
method all differ. A separate file is cleaner than a hw_data table with
mutually exclusive access paths.

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
~~~~~~~~~~~~~~~~
  - Split into a PCI/x86 patch and the driver; the driver no longer walks
    the MCFG table or maps the ECAM page itself.
  - 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.
  - Require a positive sign that a command was accepted.
  - 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().
  - Add Documentation/i2c/busses/i2c-imc-skylake.rst.
  - Drop the ACPI dependency; the Kconfig entry now depends on PCI_MMCONFIG.
  - Remove the unsupported attribution of the dropped write to SMM.

Validation
~~~~~~~~~~
  - In-tree build of both patches, W=1, x86_64, GCC: no diagnostics.
  - arm64 COMPILE_TEST with Clang: no diagnostics.
  - checkpatch.pl --strict: clean on both patches.
  - Sparse with __CHECK_ENDIAN__: clean.
  - Read-only hardware test on one Intel X299 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, with no kernel warning or oops, no
    interference reported and every command accepted.

Still required before this can lose the RFC prefix: an independent
Tested-by, ideally on Cascade Lake-X; suspend/resume with active clients;
a measurement of MSR_SMI_COUNT across a failing CF8/CFC write, to say
something about the mechanism rather than only about the effect; and a
direction from the PCI and x86 maintainers on patch 1.

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 of patch 2, the
    kerneldoc and .rst documentation, the commit messages and this cover
    letter. The register decoding, the ECAM finding and every hardware
    measurement are 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 the check now in
    imc_engine_responded().
  - Testing: every revision was built with W=1, checked with
    checkpatch --strict and sparse, and the hardware behaviour was tested
    on the X299 system by me.

I have reviewed all of it, I understand it, and I take responsibility for
it. The Assisted-by trailer is on patch 2, which is where the assistance
went; patch 1 was written by hand from the existing arch/x86 code.

Simone Chifari (2):
  PCI: allow config access forced through the memory-mapped path
  i2c: imc-skylake: add driver for the Intel Skylake-X iMC SMBus engine