From e9a90d63ac2717e2ebcc16fc3280a151ecb83e1e Mon Sep 17 00:00:00 2001 From: Simone Date: Thu, 2 Jul 2026 12:48:30 +0200 Subject: [PATCH] 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 --- .gitignore | 1 + docs/submission/Kconfig.kernel | 2 +- docs/submission/cover-letter.txt | 27 ++++++++++++++++++--------- i2c-imc-skylake.c | 30 ++++++++++-------------------- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/.gitignore b/.gitignore index 161f28e..6d0a15f 100644 --- a/.gitignore +++ b/.gitignore @@ -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/ diff --git a/docs/submission/Kconfig.kernel b/docs/submission/Kconfig.kernel index 462d7bf..b106389 100644 --- a/docs/submission/Kconfig.kernel +++ b/docs/submission/Kconfig.kernel @@ -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 diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index 8cd786a..c4d1c92 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -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 (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(+) diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index deb8879..e1df8ab 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -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 @@ -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; }