5dc45f7b91c6b08f9d5dfb0dbeb32b1b0ea0a0f2
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Simone
|
7bac49b4f6 |
i2c-imc-skylake: harden the checks for the v4 respin
All of this came out of a self-review of v3 plus one robot report; none of it changes what the driver supports. The read-back check required the whole command word to come back as written, and the interference check on the driven channel compared against the pre-transfer word. Both implicitly demanded that every undocumented bit of the command register reads as zero, which held on the test system but is not something to assume across steppings. Compare only the bits this driver produces, with the post-write read-back as the baseline for the driven channel; GO clearing remains the single permitted difference. 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 polling across suspend; the probe-time refusal cannot see that, and the S3 observation in the cover letter is exactly this scenario. 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 CMD and the data latch DATA. v3 called the command register DATA and the data latch CTRL, which read backwards. 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 only 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]/ The Kconfig help and the .rst now say that PCI ID 8086:2085 also matches Skylake-SP / Cascade Lake-SP servers, where a BMC is far more likely to own the engine, and the .rst states plainly that jc42 itself has not been run here, only the transactions it needs. |
||
|
Simone
|
694ed6a712 |
i2c-imc-skylake: instantiate the SPD EEPROMs at probe
Call i2c_register_spd() per adapter, like i801 and piix4, instead of
telling the user to echo into new_device. The helper counts populated
slots from DMI and probes upwards from 0x50 via i2c_new_scanned_device(),
so it claims no address that does not answer; each channel carries half
the DIMMs and the scan is per adapter, so each finds its own. Verified:
four clients on load, two per channel, none on the empty slots, and
decode-dimms works with no manual step.
This needed Receive Byte to be worth doing — without I2C_FUNC_SMBUS_BYTE
ee1004 would not have bound to what was instantiated.
The write_disable variant is deliberate. This part is DDR4 only, where
the two are equivalent (the split gates spd5118 on DDR5), and of two
equivalent calls the one that does not read as "this driver enables SPD
writes" is the better name to leave in a driver whose bus is the memory
bus. In-tree convention is write_enable; the cover letter offers to
switch.
Kconfig gains select I2C_SMBUS.
Out-of-tree, compat.h maps the call onto the older unsplit
i2c_register_spd() where that is what the target kernel has. It is force
included from the Makefile so i2c-imc-skylake.c stays byte-identical to
the submitted file, and the Makefile detects the API by grepping the
target kernel's header rather than testing a version number, which keeps
it correct on backported trees. Builds clean against both 6.8 and 7.0
headers; DKMS 1.0.2 installs on all four kernels here.
test-smoke.sh needed two fixes to keep testing what it claims:
- A bound driver owns its address, so the raw transaction checks lost
the SPDs the moment the driver started instantiating them. The script
now unbinds ee1004 for the duration rather than deleting the clients:
sysfs delete_device only removes clients that new_device created,
and unbinding is sufficient because i2c-dev refuses an address only
while a driver is bound (i2cdev_check: `return dev->driver ? -EBUSY
: 0`). Rebound from an EXIT trap.
- Those checks were guarded on an SPD address having been found, and
when the lookup failed they vanished from the run silently: the suite
reported "12 passed, 0 failed" while seven checks had not executed.
Skipped checks are now counted and printed, which is how the above
was noticed at all.
Smoke suite: 20 passed, 0 failed, 0 skipped. checkpatch --strict 0/0/0,
in-tree W=1 and Sparse clean.
|
||
|
Simone
|
88a8b7eb27 |
i2c-imc-skylake: retract the CF8/CFC claim, harden the state machine
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.
|
||
|
Simone
|
3cd1d6e0a6 |
i2c-imc-skylake: split the submission into a PCI patch and the driver
The driver reached the PCU function's config space by walking the ACPI MCFG
table and mapping the ECAM page itself. That is one leaf driver deciding
config-space policy, and no other driver in mainline does it, so it was the
single most likely reason for a NAK.
Move that decision where it belongs: the upstream series is now two patches,
a PCI/x86 accessor that forces the memory-mapped path with the core's
locking, and this driver on top of it. Reviewers on each side now get
something they can act on, and get_maintainer.pl selects the PCI and x86
people by itself.
Correctness work in the same pass:
- the completion bits are latched, so a command the engine never accepted
was indistinguishable from a completed one and returned the previous
transaction's data as valid. Require a positive sign that the engine
acted on the command;
- 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.
Stop attributing the dropped CF8/CFC write to System Management Mode. The
only evidence was a boot log line naming the access method, which says
nothing about who filters the write. tools/probe-cf8-write.sh is there to
settle it with an MSR_SMI_COUNT measurement.
Add Documentation/i2c/busses/i2c-imc-skylake.rst, which every other PC SMBus
driver has and which was explicitly asked for on the 2020 iMC thread, the
tools/ scripts for the hardware measurements the submission still owes, and
compat/pci-mmcfg-compat.h so the driver still builds out-of-tree against a
kernel without the new accessor while the source stays byte-identical to the
submitted patch.
|
||
|
Simone
|
e9a90d63ac |
i2c-imc-skylake: final v3 submission fixes
- Rewrite timeout comments with single-board test data (drop multi-board validation claim not backed by the test campaign) - Reword CLTT arbitration comment as verified observation, drop TODO - Drop CONFIG_ACPI fallback stub; Kconfig already depends on ACPI - Quiet probe: demote informational messages to dev_dbg() - Kconfig: allow COMPILE_TEST builds - Cover letter: v3 subject prefix, complete changes-since-v2 list, updated diffstat, drop stray Signed-off-by, fix GO-bit wording - Ignore local reviewer Q&A notes |
||
|
Simone
|
fee48bba23 |
i2c-imc-skylake: apply LKML review fixes
Apply senior-review feedback to the driver, validated on hardware
(Skylake-X / X299). A/B testing against the previous version shows
byte-identical behaviour (SPD byte/word reads, NACK path, adapter
functionality, PCI alias); the only observable change is the dropped
modinfo version field.
- Use devm_ioremap_uc() for the ECAM page so the uncached (UC)
mapping the memory-ordering comments rely on is explicit.
- Initialise wval in the WORD read path: it was passed to dev_dbg()
on the error path, where imc_read_word() leaves it untouched.
- Drop MODULE_VERSION() (discouraged for in-tree drivers).
- Iterate over ARRAY_SIZE(s->adap) instead of the literal 2.
- Name the config-space offsets (CFG_VENDOR_DEV, CFG_IMC_BUS).
- Restrict Kconfig to X86_64 (Skylake-X is 64-bit only).
- Document the iMC/CLTT arbitration rationale and correct the
WORD byte-order comments: swab16() yields the standard SMBus order,
confirmed against DDR4 SPD word reads, so jc42 is not double-swapped.
Signed-off-by: Simone Chifari <[email protected]>
|
||
|
Simone
|
4697ce3e3b |
i2c: imc-skylake: add driver for Intel Skylake-X iMC SMBus engine
Add a driver for the integrated memory controller (iMC) SMBus engine on Intel Skylake-X / Cascade Lake-X processors (socket LGA 2066, platform X299, PCU function 8086:2085). The engine provides two SMBus channels — one per pair of DIMM slots — over which SPD EEPROMs, DDR4 thermal sensors and third-party LED controllers are accessible. Exposing it as a pair of standard Linux I2C adapters lets existing tools (i2c-tools, lm-sensors) use it without bespoke sysfs hacks. Key design decisions: - ECAM MMIO access instead of CF8/CFC port I/O (SMM traps port writes) - Dynamic MCFG table parsing for mmcfg_base (no hardcoding) - Support for SMBus BYTE_DATA and WORD_DATA transfers - devm-managed resources with automatic cleanup - Global mutex to serialize transactions across both channels sharing the same ECAM mapping Signed-off-by: Simone Chifari <[email protected]> |