diff --git a/Makefile b/Makefile index 2429bcf..26b47c9 100644 --- a/Makefile +++ b/Makefile @@ -2,6 +2,16 @@ obj-m := i2c-imc-skylake.o KVER ?= $(shell uname -r) KDIR ?= /lib/modules/$(KVER)/build + +# Out-of-tree only. The driver targets current mainline and calls +# i2c_register_spd_write_disable(); older kernels have the unsplit +# i2c_register_spd(). compat.h bridges that, force included here so +# i2c-imc-skylake.c is exactly the file sent upstream. Detected from the target +# kernel's header rather than from a version number, which keeps it right on +# backported and vendor trees. +SPD_SPLIT := $(shell grep -qs i2c_register_spd_write_disable \ + $(KDIR)/include/linux/i2c-smbus.h && echo -DHAVE_I2C_REGISTER_SPD_SPLIT) +ccflags-y += -include $(src)/compat.h $(SPD_SPLIT) INSTALL_MOD_PATH ?= $(DESTDIR) INSTALL_MOD_DIR ?= updates PWD := $(shell pwd) diff --git a/VERSION b/VERSION index 7dea76e..6d7de6e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.0.1 +1.0.2 diff --git a/compat.h b/compat.h new file mode 100644 index 0000000..4abcdc1 --- /dev/null +++ b/compat.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Out-of-tree build shim. NOT part of the upstream submission: the Makefile + * force includes it, i2c-imc-skylake.c never does, so the driver source stays + * free of build conditionals. + * + * i2c_register_spd() was split into i2c_register_spd_write_disable() and + * i2c_register_spd_write_enable() when DDR5 arrived and the caller had to say + * whether spd5118 should be instantiated at all. The driver targets current + * mainline and calls the split form; where only the old single function + * exists it has exactly the semantics this driver wants, since the split is + * about DDR5 and this part is DDR4 only. + * + * HAVE_I2C_REGISTER_SPD_SPLIT comes from the Makefile, which greps the target + * kernel's header for the symbol. Detecting the API beats guessing the version + * it appeared in, and it stays right on backported and vendor trees. + */ + +#ifndef I2C_IMC_SKYLAKE_COMPAT_H +#define I2C_IMC_SKYLAKE_COMPAT_H + +#include + +#ifndef HAVE_I2C_REGISTER_SPD_SPLIT +static inline void i2c_register_spd_write_disable(struct i2c_adapter *adap) +{ + i2c_register_spd(adap); +} + +static inline void i2c_register_spd_write_enable(struct i2c_adapter *adap) +{ + i2c_register_spd(adap); +} +#endif + +#endif /* I2C_IMC_SKYLAKE_COMPAT_H */ diff --git a/docs/submission/Kconfig.kernel b/docs/submission/Kconfig.kernel index 1d310ba..35af46f 100644 --- a/docs/submission/Kconfig.kernel +++ b/docs/submission/Kconfig.kernel @@ -1,6 +1,7 @@ config I2C_IMC_SKYLAKE tristate "Intel Skylake-X iMC SMBus adapter" depends on PCI && (X86_64 || COMPILE_TEST) + select I2C_SMBUS help Say Y here if you want kernel support for the integrated memory controller (iMC) SMBus engine found in Intel Skylake-X / Cascade @@ -10,6 +11,8 @@ config I2C_IMC_SKYLAKE Two I2C adapters are registered, one per hardware SMBus channel, allowing access to DDR4 DIMM SPD EEPROMs (0x50-0x57) and thermal sensors from userspace via standard i2c-tools and lm-sensors. + SPD EEPROMs are instantiated automatically from DMI, so decode-dimms + works without any manual step. Firmware, SMM, a BMC or CLTT may drive the same engine and no generic arbitration mechanism is known. The driver therefore diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index 77dfa55..dea6c97 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -124,6 +124,22 @@ ee1004 consequently binds, and decode-dimms reads all four DIMMs on this system with both SPD CRCs valid on each. jc42 binds where a sensor is populated. i2cdetect scans in read mode. +With Receive Byte in place the driver can also do what the other SMBus +host drivers do and call i2c_register_spd() per adapter, so the SPD +EEPROMs are instantiated at probe rather than by hand. The helper counts +the populated slots from DMI and probes upwards from 0x50, leaving alone +any address that does not answer; each channel carries half the DIMMs and +the scan is per adapter, so each finds its own. Nothing else is +instantiated, jc42 included: there is no firmware description of these +buses and this driver does not scan for anything but the SPDs. + +I used the write_disable variant. This part is DDR4 only, where the two +are equivalent - the distinction gates spd5118 on DDR5 - and between two +equivalent calls the one that does not read as "this driver enables SPD +writes" seemed the better thing to leave in a memory-bus driver. Happy +to switch to write_enable for consistency with i801 and piix4 if you +prefer the convention. + SMBus Quick is still not implemented, and not only because the engine has no obvious encoding for it. A write to the 0x30-0x37 range is how an EE1004 SPD is write-protected, permanently on many modules. An adapter @@ -156,6 +172,8 @@ Changes since v2 not, ee1004 also needs Receive Byte and Send Byte. - Implement both, on the pointer-select bit, so ee1004 binds and decode-dimms works. + - Instantiate the SPD EEPROMs with i2c_register_spd() instead of + leaving it to the user. Validation ~~~~~~~~~~ @@ -170,11 +188,13 @@ Validation concurrent reads per channel, the default-off gate, and 20 unload/reload cycles: 19 checks, 0 failures, no kernel warning or oops, no interference reported, every command word confirmed in the register. - - ee1004 bound on all four SPD addresses and decode-dimms decoded all - four modules, each with the CRC of bytes 0-125 and of bytes 128-253 - valid. The part number reads out of page 1, so the page select is - exercised end to end. The four module serial numbers were identical - before and after every experiment in this series. + - i2c_register_spd() instantiated ee1004 on all four populated SPD + addresses at probe, two per channel, and claimed none of the empty + ones. decode-dimms then decoded all four modules, each with the CRC + of bytes 0-125 and of bytes 128-253 valid. The part number reads out + of page 1, so the page select is exercised end to end. The four + module serial numbers were identical before and after every + experiment in this series. - Engine registers sampled every 2 s for one hour with the driver unloaded: 1791 samples, 0 changes. No firmware activity while the system is up. diff --git a/docs/submission/i2c-imc-skylake.rst b/docs/submission/i2c-imc-skylake.rst index c24224b..a1babbb 100644 --- a/docs/submission/i2c-imc-skylake.rst +++ b/docs/submission/i2c-imc-skylake.rst @@ -129,22 +129,27 @@ Word transfers use the standard SMBus byte order, so ``jc42`` reading through Instantiating clients --------------------- -The adapter does not create clients by itself. There is no firmware -description of these buses and the driver does not scan. +SPD EEPROMs are instantiated at probe with ``i2c_register_spd()``, like the +other SMBus host drivers. It counts the populated slots from DMI and probes +from 0x50 upwards, so an address that does not answer is left alone. Each +channel carries half the DIMMs and the scan runs per adapter, so each finds +its own:: -``jc42`` can be instantiated where a thermal sensor is populated:: + i2c i2c-6: Successfully instantiated SPD at 0x50 + ee1004 6-0050: 512 byte EE1004-compliant SPD EEPROM, read-only + +``decode-dimms`` therefore works with no manual step. + +Nothing else is instantiated: there is no firmware description of these buses +and the driver does not scan for anything but the SPDs. ``jc42`` has to be +added by hand where a thermal sensor is populated:: # echo jc42 0x18 > /sys/bus/i2c/devices/i2c-6/new_device Whether one is populated is visible in ``SMBCNTL.TSOD_PRESENT``; on the development system that mask is empty and no ``jc42`` binds. -``ee1004`` binds on each populated SPD address:: - - # echo ee1004 0x50 > /sys/bus/i2c/devices/i2c-6/new_device - ee1004 6-0050: 512 byte EE1004-compliant SPD EEPROM, read-only - -Two things are worth knowing before instantiating it on this bus. +Two things about the SPDs are worth knowing on this bus. A DDR4 SPD is a 512-byte array reached through a 256-byte window, and which half is visible is a property of the bus, not of the transfer. ``ee1004`` @@ -154,11 +159,14 @@ reading SPD by hand:: # i2cset -y 6 0x36 0x00 c -And a bound ``ee1004`` owns those addresses, so userspace can no longer reach -them through ``/dev/i2c-*``. That matters more here than on a mainboard SMBus, -because the tools that motivate this driver share the bus with the SPDs. Bind -``ee1004`` when you want ``decode-dimms``, and remove it when you want the -addresses back:: +And ``ee1004`` owns those addresses once bound, so userspace cannot reach them +through ``/dev/i2c-*`` any more. This is ordinary I2C behaviour, but it is +worth stating here because the userspace tools that motivate this driver share +the bus with the SPDs. Read the SPD through the driver instead:: + + $ cat /sys/bus/i2c/drivers/ee1004/6-0050/eeprom | hexdump -C + +or, if an address really is needed raw, release it:: # echo 0x50 > /sys/bus/i2c/devices/i2c-6/delete_device diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index 43f7d50..62ed943 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -47,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -680,6 +681,22 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) "i2c_add_adapter ch%d failed: %d\n", i, ret); return ret; } + + /* + * Instantiate the SPD EEPROMs the same way the other SMBus + * host drivers do. This counts the populated slots from DMI + * and probes 0x50 upwards, so no client is created for an + * address that does not answer and userspace keeps the ones + * that stay empty. Each channel carries half the DIMMs, and + * the scan is per adapter, so each finds its own. + * + * The write_disable variant: this part is DDR4 only, where the + * two differ in nothing (the distinction gates spd5118 on + * DDR5). Where they are equivalent, the one that does not + * suggest this driver enables SPD writes is the better name to + * leave in a memory-bus driver. + */ + i2c_register_spd_write_disable(a); } dev_dbg(&pdev->dev, "registered 2 SMBus channels\n"); diff --git a/test-smoke.sh b/test-smoke.sh index 1a8edd5..0d8dc94 100755 --- a/test-smoke.sh +++ b/test-smoke.sh @@ -4,6 +4,7 @@ set -euo pipefail PASS=0 FAIL=0 +SKIP=0 CH0="" CH1="" CH0_ADDR="" @@ -53,6 +54,16 @@ _fail() ((FAIL++)) || true } +# A check that could not run is not a check that passed. Several below are +# guarded on an SPD address having been found, and when that lookup failed they +# used to vanish from the run silently, leaving a shorter suite reporting +# "0 failed" — which is how a regression hides. +_skip() +{ + echo "SKIP: $*" + ((SKIP++)) || true +} + _module_loaded() { lsmod | awk '$1 == "i2c_imc_skylake" { found = 1 } END { exit !found }' @@ -200,6 +211,43 @@ _load_unsafe() "${SUDO[@]}" insmod "$MODULE_PATH" allow_unsafe_access=1 } +# The driver instantiates ee1004 on the populated SPD addresses at probe, and +# the checks below need those addresses raw. Unbind rather than delete: the +# sysfs delete_device only removes clients that sysfs new_device created, while +# i2c_register_spd() uses i2c_new_scanned_device(). Unbinding is enough anyway, +# because i2c-dev refuses an address only while a driver is *bound* to it +# (i2cdev_check: `return dev->driver ? -EBUSY : 0`). +SPD_UNBOUND=() + +_unbind_spd_clients() +{ + local dev name + + for dev in /sys/bus/i2c/drivers/ee1004/*-*; do + [[ -e "$dev" ]] || continue + name=${dev##*/} + [[ "$name" == "$CH0-"* || "$name" == "$CH1-"* ]] || continue + if printf '%s\n' "$name" | + "${SUDO[@]}" tee /sys/bus/i2c/drivers/ee1004/unbind >/dev/null 2>&1; then + SPD_UNBOUND+=("$name") + fi + done + echo "unbound ${#SPD_UNBOUND[@]} SPD client(s) for the raw transaction checks" +} + +# Put them back if the run ends early. On a clean run the reload cycles below +# have already recreated and rebound them, so these writes just fail harmlessly. +_rebind_spd_clients() +{ + local name + + for name in ${SPD_UNBOUND[@]+"${SPD_UNBOUND[@]}"}; do + printf '%s\n' "$name" | + "${SUDO[@]}" tee /sys/bus/i2c/drivers/ee1004/bind >/dev/null 2>&1 || true + done +} +trap _rebind_spd_clients EXIT + DMESG_START=$("${SUDO[@]}" dmesg | wc -l) "${SUDO[@]}" modprobe i2c-dev _module_loaded && "${SUDO[@]}" rmmod i2c-imc-skylake @@ -210,6 +258,15 @@ _refresh_buses [[ -n "$CH0" ]] && _ok "ch0 adapter at i2c-$CH0" || _fail "ch0 adapter not found" [[ -n "$CH1" ]] && _ok "ch1 adapter at i2c-$CH1" || _fail "ch1 adapter not found" +# The driver instantiates SPD clients at probe. Prove it did, then take the +# addresses back so the raw transaction checks can use them. +_spd_clients=$(ls -d /sys/bus/i2c/drivers/ee1004/"$CH0"-00* \ + /sys/bus/i2c/drivers/ee1004/"$CH1"-00* 2>/dev/null | wc -l) +((_spd_clients > 0)) && + _ok "driver instantiated $_spd_clients SPD EEPROM(s) at probe" || + _fail "no SPD EEPROM instantiated at probe" +_unbind_spd_clients + if [[ -n "$CH0" ]]; then _scan_spd "$CH0" CH0_ADDR && _ok "SPD device present on ch0" || _fail "no SPD device on ch0" fi @@ -220,30 +277,41 @@ fi echo "=== transfer semantics ===" if [[ -n "$CH0_ADDR" ]]; then _word_test "$CH0" "$CH0_ADDR" && _ok "WORD_DATA byte order on ch0" || _fail "WORD_DATA test on ch0" +else + _skip "WORD_DATA on ch0: no reachable SPD address" fi if [[ -n "$CH1_ADDR" ]]; then _word_test "$CH1" "$CH1_ADDR" && _ok "WORD_DATA byte order on ch1" || _fail "WORD_DATA test on ch1" - +else + _skip "WORD_DATA on ch1: no reachable SPD address" fi if [[ -n "$CH0_ADDR" ]]; then _receive_byte_test "$CH0" "$CH0_ADDR" && _ok "Receive Byte follows the EEPROM pointer on ch0" || _fail "Receive Byte test on ch0" +else + _skip "Receive Byte follows the EEPROM pointer on ch0: no reachable SPD address" fi if [[ -n "$CH1_ADDR" ]]; then _receive_byte_test "$CH1" "$CH1_ADDR" && _ok "Receive Byte follows the EEPROM pointer on ch1" || _fail "Receive Byte test on ch1" +else + _skip "Receive Byte follows the EEPROM pointer on ch1: no reachable SPD address" fi if [[ -n "$CH0_ADDR" ]]; then _send_byte_test "$CH0" "$CH0_ADDR" && _ok "Send Byte selects the SPD page on ch0" || _fail "Send Byte test on ch0" +else + _skip "Send Byte selects the SPD page on ch0: no reachable SPD address" fi if [[ -n "$CH1_ADDR" ]]; then _send_byte_test "$CH1" "$CH1_ADDR" && _ok "Send Byte selects the SPD page on ch1" || _fail "Send Byte test on ch1" +else + _skip "Send Byte selects the SPD page on ch1: no reachable SPD address" fi if [[ -n "$CH0" ]]; then _nack_test "$CH0" && _ok "absent-device ENXIO on ch0" || _fail "expected ENXIO at address 0x77 on ch0" @@ -325,5 +393,9 @@ else _ok "every command reached the register" fi -echo "=== result: $PASS passed, $FAIL failed ===" +if ((SKIP)); then + echo "=== result: $PASS passed, $FAIL failed, $SKIP skipped ===" +else + echo "=== result: $PASS passed, $FAIL failed ===" +fi ((FAIL == 0))