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

i2c-imc-skylake: final v3 submission fixes

Browse Source 
- 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
This commit is contained in:
Simone
2026-07-02 12:48:30 +02:00
parent 3c86f6c866
commit e9a90d63ac
4 changed files with 30 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitignore
+1
View File
@@ -50,6 +50,7 @@ docs/mainline-plan.md
docs/archive-i2c-imc-driver-plan.md
docs/archive-v3-review-fixes.md
docs/submission/submission-instructions.md
docs/submission/review-answers.md
# Private Gitea CI (replaced by .github/workflows/ for public repo)
.gitea/
docs/submission/Kconfig.kernel
+1 -1
View File
@@ -1,6 +1,6 @@
config I2C_IMC_SKYLAKE
tristate "Intel Skylake-X iMC SMBus adapter"
depends on PCI && ACPI && X86_64
depends on PCI && ACPI && (X86_64 || COMPILE_TEST)
help
Say Y here if you want kernel support for the integrated memory
controller (iMC) SMBus engine found in Intel Skylake-X / Cascade
docs/submission/cover-letter.txt
+18 -9
View File
@@ -1,4 +1,4 @@
Subject: [PATCH 0/1] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter
Subject: [PATCH v3 0/1] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter
This is v3 of the i2c-imc-skylake driver. v2 was submitted under the name
i2c-imc-x299; the driver has been renamed to reflect that the iMC SMBus
@@ -23,7 +23,8 @@ config space access, as reported by boot-time dmesg:
System Management Mode traps writes to those ports for this device, so a
standard pci_write_config_dword() targeting the SMBus DATA register never
reaches the hardware — the transaction hangs at status bit 0x08 indefinitely.
reaches the hardware — the GO bit never clears and the transaction never
completes.
The Windows NTIOLib (used by Kingston FURY) reaches the same registers via the
ECAM (MMIO) window, which is not trapped. This driver follows the same path:
ioremap() of the ECAM page for the target function and driving the registers
@@ -71,6 +72,16 @@ Changes since v2
the X299 chipset
- Add I2C_SMBUS_WORD_DATA support (WORD_BIT bit 17) for TSOD reads
- Update adapter name: "iMC SMBus Skylake-X channel N"
- Use devm_ioremap_uc() for the ECAM mapping (uncached, strongly
ordered) instead of devm_ioremap()
- Use ARRAY_SIZE() for the adapter loop; initialise wval on the
word-read path
- Build FRAME from named bits (ENGINE_ENABLE | GO_BIT) instead of a
magic constant; document the CTRL/DATA/STATUS encoding in full
- Cross-check the iMC bus number against cfg[0xCC] at probe
- Kconfig: depends on PCI && ACPI && (X86_64 || COMPILE_TEST); drop
the CONFIG_ACPI fallback stub accordingly
- Quiet probe: demote informational probe messages to dev_dbg()
Testing
~~~~~~~
@@ -97,15 +108,13 @@ and bind successfully via BYTE_DATA reads.
udev autoloads the module on PCI add event (MODULE_DEVICE_TABLE present).
20 rmmod/modprobe cycles: no oops, no warnings, no resource leaks in dmesg.
Signed-off-by: Simone Chifari <simone.chifari@gmail.com>
---
Simone Chifari (1):
i2c: imc-skylake: add driver for Intel Skylake-X iMC SMBus engine
MAINTAINERS | 6 +
drivers/i2c/busses/Kconfig | 19 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-imc-skylake.c | 512 +++++++++++++++++++++++++++++++++++
4 files changed, 538 insertions(+)
MAINTAINERS | 6 +
drivers/i2c/busses/Kconfig | 19 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-imc-skylake.c | 551 +++++++++++++++++++++++++++++++++
4 files changed, 577 insertions(+)
i2c-imc-skylake.c
+10 -20
View File
@@ -44,10 +44,10 @@
* closed-loop thermal throttling (CLTT) TSOD polling. On the X299 HEDT
* platform there is no BMC and CLTT firmware polling is not active, so the
* quiesce handshake required on server parts (Sandy Bridge-EP, Broadwell-E)
* is not needed here. The global mutex still serialises the two channels
* is not needed here. On the tested hardware the engine is idle at probe
* and no firmware-initiated transaction has ever been observed between
* driver transactions. The global mutex still serialises the two channels
* against each other, as they share a single engine.
* TODO: read back the CLTT polling-interval register at probe to assert it
* is disabled, rather than relying on the platform assumption.
*/
#include <linux/acpi.h>
@@ -122,9 +122,8 @@ struct imc_smbus {
* ECAM base discovery from ACPI MCFG. acpi_table_parse() is not exported to
* modules, so map the MCFG table with acpi_get_table() (exported) and walk the
* allocation entries by hand. Uses pdev's PCI segment and bus number so no
* module parameter override is needed.
* module parameter override is needed. CONFIG_ACPI is guaranteed by Kconfig.
*/
#ifdef CONFIG_ACPI
static u64 imc_detect_mmcfg_base(struct pci_dev *pdev)
{
unsigned int seg = pci_domain_nr(pdev->bus);
@@ -156,12 +155,6 @@ static u64 imc_detect_mmcfg_base(struct pci_dev *pdev)
acpi_put_table(hdr);
return base;
}
#else
static u64 imc_detect_mmcfg_base(struct pci_dev *pdev)
{
return 0;
}
#endif
/*
* Wait until GO clears (transaction issued), then until the busy bit drops.
@@ -169,11 +162,9 @@ static u64 imc_detect_mmcfg_base(struct pci_dev *pdev)
* success *status (if non-NULL) gets the final status word. Process context
* only - it sleeps between polls.
*
* Timeout values validated empirically across 5 X299 motherboards with 16
* DIMM configurations (DDR4-2133 to DDR4-3200, single/dual rank, ECC/non-ECC):
* - GO clear: worst-case observed 87ms, timeout set to 200ms (2.3x margin)
* - BUSY clear: worst-case observed 12ms, timeout set to 50ms (4x margin)
* Margins account for SMM interference and clock stretching per SMBus spec.
* On the test system (Skylake-X, 4 DDR4 DIMMs) transactions complete in a few
* milliseconds; the 200ms (GO clear) and 50ms (BUSY clear) timeouts leave a
* generous margin for SMM interference and SMBus clock stretching.
*/
static int imc_wait(struct imc_smbus *s, const struct imc_chan *c, u32 *status)
{
@@ -197,8 +188,7 @@ static int imc_wait(struct imc_smbus *s, const struct imc_chan *c, u32 *status)
/*
* Poll the busy bit clear only (no GO check). The firmware polls STATUS after
* the CTRL (data-latch) write too, before issuing the DATA/GO word.
* Timeout: 50ms validated empirically across 5 X299 boards, 16 DIMM configs.
* Worst-case observed: 12ms, margin 4x for SMM interference.
* Timeout: 50ms, same rationale as in imc_wait().
*
* Note: devm_ioremap_uc() returns an uncached (UC) mapping, which enforces
* strong ordering. writel() includes a full mb() barrier, ensuring the write
@@ -501,7 +491,7 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
"cfg[0xCC]=0x%08x iMC bus 0x%02x confirmed\n",
cc, imc_bus_hw);
dev_info(&pdev->dev, "ECAM mapped at %pa\n", &phys);
dev_dbg(&pdev->dev, "ECAM mapped at %pa\n", &phys);
/*
* Lifetime safety: the I2C core guarantees that smbus_xfer callbacks
@@ -531,7 +521,7 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
}
}
dev_info(&pdev->dev, "registered 2 SMBus channels (use i2cdetect -l)\n");
dev_dbg(&pdev->dev, "registered 2 SMBus channels\n");
return 0;
}