From c04e0ec5a8b977f01590725ebc000cc30ed0a923 Mon Sep 17 00:00:00 2001 From: Simone Date: Mon, 27 Jul 2026 01:26:07 +0200 Subject: [PATCH] i2c-imc-skylake: implement Receive Byte and Send Byte The engine encodes an address-only transaction in command word bit 18, which is what SMBus Receive Byte and Send Byte need and what was missing for ee1004 to bind. The bit is not documented for the PCU function. It was located by structure rather than by sweeping: SMBCMD on the iMC channel function (datasheet ref. 614073 section 3.1.8) places PNTR_SEL immediately above WORD_ACCESS, and word access here is bit 17. Bit 17 had itself been unknown until it turned out to be word access, and an earlier sweep of this register covered bits 31:20 only, so 18 was untried rather than already ruled out. Validated against the device rather than against the register. An EEPROM auto-increments its pointer after a read, so priming the pointer and repeating the read with the bit set has a predicted answer: the next byte, not the one named. Eight consecutive reads returned SPD bytes 0x01 through 0x08 in order, matching an ordinary dump. Send Byte was confirmed on the DDR4 page-select addresses, which hold a latch and store no data, so the page state is itself the readout. Result: ee1004 binds on all four SPD addresses and decode-dimms decodes all four modules with both CRCs valid on each. The part number comes off page 1, so the page select runs end to end. The four module serial numbers are unchanged from the baseline recorded before this series. SMBus Quick stays unimplemented. Beyond having no obvious encoding, a write into the 0x30-0x37 range is how an EE1004 SPD is write-protected, permanently on many modules, so an adapter on the memory bus should not advertise a probe that writes. i2cdetect scans in read mode. Also fix test-smoke.sh resolving the module through modinfo, which silently tested the installed DKMS build instead of the one just compiled. It now prefers the local .ko and prints which it loaded. Smoke suite: 19 passed, 0 failed. checkpatch --strict 0/0/0, in-tree W=1 and Sparse clean. --- docs/submission/cover-letter.txt | 60 +++++--- docs/submission/i2c-imc-skylake.rst | 51 +++++-- i2c-imc-skylake.c | 62 ++++++-- test-smoke.sh | 78 +++++++++- tools/README.md | 69 +++++++++ tools/probe-pntr-sel.sh | 221 ++++++++++++++++++++++++++++ tools/probe-send-byte.sh | 200 +++++++++++++++++++++++++ 7 files changed, 696 insertions(+), 45 deletions(-) create mode 100755 tools/probe-pntr-sel.sh create mode 100755 tools/probe-send-byte.sh diff --git a/docs/submission/cover-letter.txt b/docs/submission/cover-letter.txt index 3391b70..77dfa55 100644 --- a/docs/submission/cover-letter.txt +++ b/docs/submission/cover-letter.txt @@ -1,4 +1,4 @@ -Subject: [PATCH v4] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter +Subject: [PATCH v5] i2c: imc-skylake: add Intel Skylake-X iMC SMBus adapter The integrated memory controller of Intel Skylake-X and Cascade Lake-X processors contains an SMBus engine wired to the DDR4 DIMMs. This driver @@ -10,7 +10,7 @@ v2 was posted as i2c-imc-x299 and received no replies: https://lore.kernel.org/linux-i2c/20260620144131.415559-1-simone.chifari@gmail.com/ -v3 was never posted. +v3 and v4 were never posted. Correction to v2 ~~~~~~~~~~~~~~~~ @@ -104,16 +104,30 @@ 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 and devices are addressed directly. +It also implements SMBus BYTE through a pointer-select bit, without saying +which bit. That bit is 18 here. The datasheet gave it away: SMBCMD on the +iMC channel functions places PNTR_SEL immediately above WORD_ACCESS, and +word access on the PCU function is bit 17. -That omission costs more than the scan. Both functionality combinations -ee1004 accepts include I2C_FUNC_SMBUS_BYTE, so ee1004 does not bind to this -adapter and decode-dimms does not work through it; SPD bytes are readable -with i2cget, and jc42 binds fine where a sensor is populated. Implementing -Receive Byte is the most useful thing still missing, and I would rather add -it in a follow-up, validated, than advertise it now. +Bit 18 is the only part of this driver that rests on an undocumented +encoding, so it is worth saying exactly what was measured rather than +asserting it works. With the bit set on a read, eight consecutive +transfers to an SPD EEPROM returned bytes 0x01 through 0x08 in order, +matching an ordinary dump of the same range. That is the auto-increment +of a Receive Byte; a transfer that still named a register would have +returned the same byte eight times. With the bit set on a write, a Send +Byte to the DDR4 page-select addresses moves the 256-byte window: byte +0x00 stops reading as the JEDEC 0x23 after selecting page 1 and returns +to it after selecting page 0. + +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. + +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 +whose bus is the memory bus should not advertise a probe that writes. Changes since v2 ~~~~~~~~~~~~~~~~ @@ -139,7 +153,9 @@ Changes since v2 - Drop the ACPI dependency. - Refuse to bind when the iMC reports TSOD polling enabled. - Correct the claim that BYTE_DATA and WORD_DATA cover ee1004: they do - not, ee1004 also needs Receive Byte. + not, ee1004 also needs Receive Byte and Send Byte. + - Implement both, on the pointer-select bit, so ee1004 binds and + decode-dimms works. Validation ~~~~~~~~~~ @@ -147,12 +163,18 @@ Validation diagnostics. - checkpatch.pl --strict: clean. - Sparse with __CHECK_ENDIAN__: clean. - - Read-only hardware test on one Intel Core i9-10900X (Cascade Lake-X) - 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: 15 - checks, 0 failures, no kernel warning or oops, no interference - reported, every command word confirmed in the register. + - Hardware test on one Intel Core i9-10900X (Cascade Lake-X) system with + four DDR4 DIMMs: both adapters, four SPD EEPROMs, BYTE_DATA, WORD_DATA + byte order, Receive Byte tracking the EEPROM pointer, Send Byte moving + the SPD page and putting it back, ENXIO for an absent device, 100 + 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. - 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. @@ -182,7 +204,7 @@ Validation after. Still missing: an independent Tested-by, ideally on Skylake-X rather than -Cascade Lake-X, and SMBus Receive Byte so that ee1004 binds. +Cascade Lake-X. One note on the register interface, since a reader with the datasheet will ask. That document describes an SMBus interface on the iMC channel diff --git a/docs/submission/i2c-imc-skylake.rst b/docs/submission/i2c-imc-skylake.rst index ccc7027..a5b0b86 100644 --- a/docs/submission/i2c-imc-skylake.rst +++ b/docs/submission/i2c-imc-skylake.rst @@ -90,21 +90,34 @@ function and are reached with the ordinary ``pci_read_config_dword()`` and Supported transactions ---------------------- -The driver implements SMBus Read/Write Byte Data and Read/Write Word Data. +The driver implements SMBus Receive Byte and Send Byte, Read/Write Byte Data +and Read/Write Word Data. -That is enough for ``jc42`` (TSOD), which asks for exactly those two. It is -**not** enough for ``ee1004``: both of its accepted combinations include -``I2C_FUNC_SMBUS_BYTE``, which this adapter does not implement, so the driver -probe fails with ``-EPFNOSUPPORT``. SPD bytes are still readable directly with -``i2cget``. Implementing SMBus Receive Byte would make ``ee1004`` and -``decode-dimms`` work; it is the most useful thing still missing. +That covers ``jc42`` (TSOD) and ``ee1004``, so ``decode-dimms`` reads the whole +512-byte DDR4 SPD, including the manufacturer and part-number fields that live +on page 1. -SMBus Quick and Receive Byte are not implemented, so ``i2cdetect`` cannot scan -these buses. Address a device directly instead, for example SPD byte 2 of the -EEPROM at 0x50:: +Receive Byte and Send Byte need a command word that puts no register byte on +the bus. The engine encodes that in bit 18, which is not documented for this +function. It was found by noting that the interface Intel does document for +the iMC channel functions (SMBCMD, datasheet reference 614073, section 3.1.8) +places ``PNTR_SEL`` immediately above ``WORD_ACCESS``, and that word access +here is bit 17. With the bit set, consecutive reads of an SPD EEPROM return +consecutive bytes, which is the auto-increment behaviour of a Receive Byte and +cannot be produced by a transfer that names a register. - $ i2cget -y 6 0x50 0x02 - 0x0c +SMBus Quick is not implemented. ``i2cdetect`` therefore has to be run in +read mode:: + + $ i2cdetect -y -r 6 + 0 1 2 3 4 5 6 7 8 9 a b c d e f + 20: -- -- -- -- -- -- -- 27 -- -- -- -- -- -- -- -- + 30: 30 31 -- -- 34 35 36 -- -- -- -- -- -- -- -- -- + 50: 50 -- 52 -- -- -- -- -- -- -- -- -- -- -- -- -- + +Not advertising Quick is deliberate as well as accurate. A write to the +0x30-0x37 range is how an EE1004 SPD is write-protected, permanently on many +modules, so an adapter on a memory bus should not offer probing that writes. Block transfers are not implemented. The engine has block primitives, but no device on this bus needs them. @@ -126,8 +139,18 @@ description of these buses and the driver does not scan. Whether one is populated is visible in ``SMBCNTL.TSOD_PRESENT``; on the development system that mask is empty and no ``jc42`` binds. -``ee1004`` will not bind for the reason given above. Read SPD bytes directly -instead. +``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 + +Note that 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`` +selects a page when it needs one and leaves it selected, so a raw ``i2cget`` +issued afterwards may be looking at page 1. Select page 0 explicitly before +reading SPD by hand:: + + # i2cset -y 6 0x36 0x00 c Module parameters ----------------- diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index c4df50e..43f7d50 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -95,6 +95,21 @@ #define GO_BIT BIT(19) /* start transaction */ #define TSOD_ACTIVE_BIT BIT(20) #define WORD_BIT BIT(17) /* 16-bit word transfer (vs 8-bit byte) */ +/* + * Address-only transfer: the register byte in bits[7:0] is not put on the bus. + * A read then returns the byte at the device's own pointer (SMBus Receive + * Byte) and a write sends the data byte alone (SMBus Send Byte). + * + * The bit is not documented for this function. It was found by noting that + * the interface Intel does document for the iMC channel functions (SMBCMD, + * datasheet ref. 614073 section 3.1.8) places PNTR_SEL immediately above + * WORD_ACCESS, and that word access here is bit 17. Confirmed on hardware: + * with the bit set, eight consecutive reads of an SPD EEPROM returned bytes + * 0x01 through 0x08 in order, matching an ordinary dump of the same range, + * which is the auto-increment behaviour of a Receive Byte and cannot be + * produced by a transfer that names a register. + */ +#define PNTR_SEL_BIT BIT(18) #define WRITE_OPERATION BIT(15) #define COMMAND_PREFIX (COMMAND_TOGGLE | GO_BIT) #define COMMAND_KEEP_MASK (~TSOD_ACTIVE_BIT) @@ -104,7 +119,7 @@ * transfer means interference. */ #define COMMAND_OUR_BITS (COMMAND_TOGGLE | GO_BIT | WORD_BIT | \ - GENMASK(15, 0)) + PNTR_SEL_BIT | GENMASK(15, 0)) #define STAT_BUSY BIT(0) /* low bit set while transaction in flight */ #define STAT_ERROR BIT(1) #define STAT_READ_DONE BIT(2) @@ -387,6 +402,8 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr, command = COMMAND_PREFIX | ((u32)addr << 8) | reg; if (size == I2C_SMBUS_WORD_DATA) command |= WORD_BIT; + else if (size == I2C_SMBUS_BYTE) + command |= PNTR_SEL_BIT; if (read_write == I2C_SMBUS_WRITE) { raw = size == I2C_SMBUS_WORD_DATA ? swab16(*value) : *value; @@ -433,7 +450,7 @@ restore_command: /* * Standard SMBus transfer callback. The per-channel imc_chan is stashed in the * adapter's algo_data. The command word carries the 7-bit address, so - * SPD EEPROMs (0x50-0x57) are reachable on each channel. BYTE_DATA and + * SPD EEPROMs (0x50-0x57) are reachable on each channel. BYTE, BYTE_DATA and * WORD_DATA are supported; larger transfers would need the engine's block * primitives, which are not used by the devices on this bus. */ @@ -452,39 +469,61 @@ static s32 imc_smbus_xfer(struct i2c_adapter *adap, u16 addr, if (addr > 0x7f) return -EINVAL; - if (size != I2C_SMBUS_BYTE_DATA && size != I2C_SMBUS_WORD_DATA) + if (size != I2C_SMBUS_BYTE && size != I2C_SMBUS_BYTE_DATA && + size != I2C_SMBUS_WORD_DATA) return -EOPNOTSUPP; if (read_write != I2C_SMBUS_READ && read_write != I2C_SMBUS_WRITE) return -EINVAL; - reg = command; + /* + * BYTE puts no register on the bus. For a write the command parameter + * carries the data byte instead, and data is NULL. + */ + reg = size == I2C_SMBUS_BYTE ? 0 : command; mutex_lock(&s->lock); s->io_err = 0; if (read_write == I2C_SMBUS_WRITE) { - if (size == I2C_SMBUS_WORD_DATA) { - dev_dbg(s->dev, "ch%d W addr=%02x reg=%02x val=%04x\n", - c->idx, addr, reg, data->word); + switch (size) { + case I2C_SMBUS_BYTE: + val = command; + dev_dbg(s->dev, "ch%d W addr=%02x val=%02x (send byte)\n", + c->idx, addr, val); + break; + case I2C_SMBUS_WORD_DATA: val = data->word; - } else { + dev_dbg(s->dev, "ch%d W addr=%02x reg=%02x val=%04x\n", + c->idx, addr, reg, val); + break; + default: val = data->byte; dev_dbg(s->dev, "ch%d W addr=%02x reg=%02x val=%02x\n", c->idx, addr, reg, val); + break; } ret = imc_access(s, c, addr, reg, read_write, size, &val); } else { ret = imc_access(s, c, addr, reg, read_write, size, &val); - if (size == I2C_SMBUS_WORD_DATA) { + switch (size) { + case I2C_SMBUS_BYTE: + if (!ret) + data->byte = val; + dev_dbg(s->dev, "ch%d R addr=%02x -> %02x (receive byte, ret %d)\n", + c->idx, addr, val, ret); + break; + case I2C_SMBUS_WORD_DATA: if (!ret) data->word = val; dev_dbg(s->dev, "ch%d R addr=%02x reg=%02x -> %04x (ret %d)\n", c->idx, addr, reg, val, ret); - } else { + break; + default: if (!ret) data->byte = val; dev_dbg(s->dev, "ch%d R addr=%02x reg=%02x -> %02x (ret %d)\n", c->idx, addr, reg, val, ret); + break; } } @@ -504,7 +543,8 @@ static s32 imc_smbus_xfer(struct i2c_adapter *adap, u16 addr, static u32 imc_func(struct i2c_adapter *adap) { - return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA; + return I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA | + I2C_FUNC_SMBUS_WORD_DATA; } static const struct i2c_algorithm imc_algo = { diff --git a/test-smoke.sh b/test-smoke.sh index 228c3b8..1a8edd5 100755 --- a/test-smoke.sh +++ b/test-smoke.sh @@ -19,7 +19,23 @@ else fi fi -MODULE_PATH=${MODULE_PATH:-$(modinfo -n i2c-imc-skylake)} +# Prefer the module built in this directory. Falling straight through to +# modinfo would silently test whichever build is installed, which on a machine +# with a DKMS package is not the one just compiled. +_default_module() +{ + local local_build + + local_build=$(dirname "$(readlink -f "$0")")/i2c-imc-skylake.ko + if [[ -r "$local_build" ]]; then + printf '%s\n' "$local_build" + else + modinfo -n i2c-imc-skylake + fi +} + +MODULE_PATH=${MODULE_PATH:-$(_default_module)} +echo "testing module: $MODULE_PATH" if [[ ! -r "$MODULE_PATH" ]]; then echo "Module not found: $MODULE_PATH" >&2 exit 2 @@ -118,6 +134,46 @@ _word_test() ((word == expected)) } +# SMBus Receive Byte reads from the device's own pointer, so on an EEPROM a +# sequence of them walks forward through the array. Prime the pointer with an +# ordinary read, then check the next reads reproduce an ordinary dump of the +# following bytes. A transaction that still named a register would return the +# same byte every time. +_receive_byte_test() +{ + local bus=$1 address=$2 + local offset expected got + + "${SUDO[@]}" i2cget -y "$bus" "$address" 0x00 >/dev/null 2>&1 || return 1 + for offset in 0x01 0x02 0x03 0x04; do + got=$("${SUDO[@]}" i2cget -y "$bus" "$address" 2>/dev/null) || return 1 + expected=$("${SUDO[@]}" i2cget -y "$bus" "$address" "$offset" 2>/dev/null) || return 1 + ((got == expected)) || return 1 + # The explicit read above moved the pointer to $offset + 1, which + # is where the next Receive Byte should pick up. + done +} + +# SMBus Send Byte against the DDR4 page-select addresses. Selecting page 1 puts +# the upper half of the SPD in the 256-byte window, so byte 0x00 stops being +# the JEDEC 0x23; selecting page 0 brings it back. The page state is the +# readout, so the test verifies itself, and it ends on page 0 either way. +_send_byte_test() +{ + local bus=$1 address=$2 + local base flipped restored + + base=$("${SUDO[@]}" i2cget -y "$bus" "$address" 0x00 2>/dev/null) || return 1 + + "${SUDO[@]}" i2cset -y "$bus" 0x37 0x00 c 2>/dev/null + flipped=$("${SUDO[@]}" i2cget -y "$bus" "$address" 0x00 2>/dev/null) + + "${SUDO[@]}" i2cset -y "$bus" 0x36 0x00 c 2>/dev/null + restored=$("${SUDO[@]}" i2cget -y "$bus" "$address" 0x00 2>/dev/null) + + [[ "$flipped" != "$base" && "$restored" == "$base" ]] +} + _nack_test() { local bus=$1 @@ -169,6 +225,26 @@ if [[ -n "$CH1_ADDR" ]]; then _word_test "$CH1" "$CH1_ADDR" && _ok "WORD_DATA byte order on ch1" || _fail "WORD_DATA test on ch1" 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" +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" +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" +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" +fi if [[ -n "$CH0" ]]; then _nack_test "$CH0" && _ok "absent-device ENXIO on ch0" || _fail "expected ENXIO at address 0x77 on ch0" fi diff --git a/tools/README.md b/tools/README.md index d75e28a..637d1ff 100644 --- a/tools/README.md +++ b/tools/README.md @@ -64,3 +64,72 @@ Two results matter: Requirements: `msr-tools` (`rdmsr`), `pciutils` (`setpci`, `lspci`), and the `msr` module (`modprobe msr`). + +## 4. `probe-cf8-transaction.sh` — does CF8/CFC start a transfer? (WRITES TO HARDWARE) + +Follow-up to §3. `probe-cf8-write.sh` shows whether the register accepts a +value; this shows whether the engine acts on it, by setting the GO bit for a +read of an unpopulated address and watching STATUS. + +```bash +sudo ./tools/probe-cf8-transaction.sh --i-understand +``` + +It does start one. That result retired the plan to reach these registers +through ECAM with a PCI-core change, and the driver uses the ordinary config +accessors. + +## 5. `probe-imc-documented.sh` — the interface Intel documents (WRITES TO HARDWARE) + +Issues a read through `SMB_STAT`/`SMBCMD`/`SMBCNTL` on the iMC channel +functions (`8086:2040`), the interface the datasheet describes, rather than the +PCU registers this driver uses. + +```bash +sudo ./tools/probe-imc-documented.sh --i-understand +``` + +On the development board it reports `RDO=1, SBE=0, RDATA=0x00` identically for +a populated 0x50, a populated 0x52 and an absent 0x57: it completes but cannot +tell a populated slot from an empty one, so it is not reaching this bus. + +## 6. `probe-pntr-sel.sh` — find the address-only bit (read-only on the bus) + +Finds the command-word bit that suppresses the register byte, which is what +SMBus Receive Byte and Send Byte need. + +```bash +sudo ./tools/probe-pntr-sel.sh --i-understand --channel 1 +``` + +Not a blind sweep. An EEPROM auto-increments its pointer after a read, so +priming the pointer with an ordinary read and then repeating it with the +candidate bit set has a predicted answer: the *next* byte, not the one named. +The script checks that prediction at two different offsets and then walks eight +consecutive reads, which an ordinary dump must reproduce exactly. + +The answer on this engine is bit 18, matching the position of `PNTR_SEL` +relative to `WORD_ACCESS` in the documented `SMBCMD` layout. Reads only, and +never to the page-select addresses, so the DDR4 page latch does not move. + +## 7. `probe-send-byte.sh` — the write half (WRITES TO HARDWARE) + +Confirms the same bit produces a Send Byte, against the page-select addresses +`0x36`/`0x37`. Those hold a latch and store no data, so the only thing a write +there can do is choose which half of the SPD is visible — and that choice is +itself the readout, since byte 0x00 is the JEDEC `0x23` on page 0 and something +else on page 1. Nothing is written to `0x50`-`0x57`. + +```bash +sudo ./tools/probe-send-byte.sh --i-understand --channel 0 --restore +``` + +`--restore` is the safe first run: it targets a channel already stuck on page 1 +and selects page 0, so a working Send Byte repairs that channel and a broken one +leaves it exactly as found. There is no outcome worse than the starting state. +`--flip` does the full round trip on a healthy channel, after `--restore` has +shown the encoding works. + +A channel found on page 1 is not a fault of this driver: `ee1004`, and +OpenRGB's `DDR4DirectAccessor::set_page()`, both select a page and leave it +selected. diff --git a/tools/probe-pntr-sel.sh b/tools/probe-pntr-sel.sh new file mode 100755 index 0000000..3061526 --- /dev/null +++ b/tools/probe-pntr-sel.sh @@ -0,0 +1,221 @@ +#!/usr/bin/env bash +# +# READ-ONLY ON THE BUS. Tests whether the unexplored bits of the PCU command +# word encode an address-only ("pointer select") transaction, which is what +# SMBus Receive Byte and Send Byte need and what this driver cannot express +# today. +# +# Why bit 18 in particular. The command word bits the driver knows are: +# +# b29 engine enable b20 TSOD active b19 GO b17 word access +# b15 write b14:8 address b7:0 register +# +# The interface Intel does document for the iMC channel functions (SMBCMD, +# datasheet ref. 614073 section 3.1.8) lays its control bits out as: +# +# b31 CMD_TRIGGER b30 PNTR_SEL b29 WORD_ACCESS b28:27 rw +# +# There PNTR_SEL sits immediately above WORD_ACCESS. In the PCU word, word +# access is bit 17, so the same adjacency puts a pointer-select bit at 18. +# Bit 17 was itself unexplored until it turned out to be word access, so the +# guess has precedent on this engine. +# +# An earlier sweep of this register (2026-06-12, furyrs docs/linux-rgb-protocol.md) +# covered bits 31:20 only, so 16 and 18 have never been tried. Both are swept +# here. +# +# The test is a discriminator, not a bit flip in the dark. SPD 0x50 answers +# with known values, and an EEPROM auto-increments its internal pointer after +# a read. So: +# +# 1. ordinary read of register 0x00 -> 0x23, pointer left at 0x01 +# 2. same read with the candidate bit set +# - returns something other than 0x23 -> the register byte was not +# sent, the engine read from the pointer: address-only transaction +# - returns 0x23 again -> the bit changed nothing +# +# Repeated at register 0x02 (-> 0x0c) so a single coincidence cannot pass. +# +# Nothing is written to the bus: reads only, against an EEPROM, and never to +# the page-select addresses 0x36/0x37, so the global DDR4 page latch does not +# move. DATA is restored with GO masked on every exit path. +# +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +COMMAND_TOGGLE=$((1 << 29)) +GO_BIT=$((1 << 19)) +TSOD_ACTIVE_BIT=$((1 << 20)) +STAT_BUSY=1 +STAT_ERROR=2 + +ADDR=0x50 +CHANNEL=0 +CANDIDATES=(16 18) +WALK=8 +CONFIRMED=false + +while (($#)); do + case $1 in + --i-understand) CONFIRMED=true; shift ;; + --addr) ADDR=$2; shift 2 ;; + --channel) CHANNEL=$2; shift 2 ;; + --bits) IFS=, read -r -a CANDIDATES <<<"$2"; shift 2 ;; + -h|--help) + sed -n '3,40p' "$0" + echo "usage: $0 --i-understand [--addr 0xNN] [--channel 0|1] [--bits 16,18]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +$CONFIRMED || { + echo "Issues SMBus read transactions from userspace. Re-run with --i-understand." >&2 + exit 2 +} + +require_root +require_cmd lspci setpci awk + +case $CHANNEL in +0) REG_DATA=$REG_CH0_DATA; REG_STAT=$REG_CH0_STAT; REG_CTRL=$REG_CH0_CTRL ;; +1) REG_DATA=$REG_CH1_DATA; REG_STAT=$REG_CH1_STAT; REG_CTRL=$REG_CH1_CTRL ;; +*) die "channel must be 0 or 1" ;; +esac + +driver_is_loaded && die "unload i2c-imc-skylake first" + +BDF=$(find_pcu) +original=$(read_reg "$BDF" "$REG_DATA") +status0=$(read_reg "$BDF" "$REG_STAT") + +((original & GO_BIT)) && die "GO already set, refusing" +((original & TSOD_ACTIVE_BIT)) && die "TSOD_ACTIVE set, refusing" +((status0 & STAT_BUSY)) && die "engine BUSY, refusing" + +restore() +{ + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$((original & ~GO_BIT))")" || true +} +trap restore EXIT + +# read_once -> echoes " "; returns 1 on NACK. +read_once() +{ + local command=$1 i data status ctrl + + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$command")" + + for ((i = 0; i < 60; i++)); do + data=$(read_reg "$BDF" "$REG_DATA") + ((data & GO_BIT)) || break + sleep 0.005 + done + + status=$(read_reg "$BDF" "$REG_STAT") + ctrl=$(read_reg "$BDF" "$REG_CTRL") + printf '%s %s 0x%02x\n' "$status" "$ctrl" "$((ctrl & 0xff))" + ! ((status & STAT_ERROR)) +} + +plain() +{ + printf '%d' $((COMMAND_TOGGLE | GO_BIT | (ADDR << 8) | $1)) +} + +echo "device $BDF channel $CHANNEL, target $ADDR" +echo "DATA before: $original STATUS before: $status0" +echo + +echo "=== baseline: ordinary reads, no candidate bit ===" +declare -A baseline +for reg in 0x00 0x02; do + if out=$(read_once "$(plain "$reg")"); then + baseline[$reg]=${out##* } + printf ' reg %s -> %s (status %s ctrl %s)\n' \ + "$reg" "${baseline[$reg]}" "${out%% *}" "$(awk '{print $2}' <<<"$out")" + else + printf ' reg %s -> NACK (status %s)\n' "$reg" "${out%% *}" + baseline[$reg]=NACK + fi +done +echo + +verdict=none +for bit in "${CANDIDATES[@]}"; do + candidate=$((1 << bit)) + printf '=== candidate BIT(%d) = 0x%08x ===\n' "$bit" "$candidate" + differed=0 + for reg in 0x00 0x02; do + # Prime the EEPROM pointer with an ordinary read, then repeat it + # with the candidate bit set. A pointer-select transaction skips + # the register byte and answers from the auto-incremented pointer. + read_once "$(plain "$reg")" >/dev/null || true + if out=$(read_once "$(( $(plain "$reg") | candidate ))"); then + byte=${out##* } + printf ' after reg %s -> %s (baseline %s, status %s)\n' \ + "$reg" "$byte" "${baseline[$reg]}" "${out%% *}" + [[ "$byte" != "${baseline[$reg]}" ]] && differed=$((differed + 1)) + else + printf ' after reg %s -> NACK (status %s)\n' "$reg" "${out%% *}" + differed=$((differed + 1)) + fi + done + + if ((differed == 2)); then + echo " -> both reads changed: this bit alters the transaction." + verdict=$bit + elif ((differed == 1)); then + echo " -> one read changed. Not conclusive; could be an SPD byte" + echo " that happens to equal its neighbour. Re-run to confirm." + else + echo " -> identical to baseline: the bit is inert on reads." + fi + echo +done + +# A bit that merely perturbs the answer is not proof of a pointer-select. The +# proof is that consecutive transactions walk the EEPROM: prime the pointer with +# an ordinary read, then issue the candidate repeatedly and check the answers +# follow the device's own byte sequence. +if [[ "$verdict" != none ]]; then + candidate=$((1 << verdict)) + printf '=== walk: %d consecutive BIT(%d) reads after priming at 0x00 ===\n' \ + "$WALK" "$verdict" + read_once "$(plain 0x00)" >/dev/null || true + walked=() + for ((i = 0; i < WALK; i++)); do + if out=$(read_once "$(( $(plain 0x00) | candidate ))"); then + walked+=("${out##* }") + else + walked+=(NACK) + fi + done + printf ' bytes 0x01.. : %s\n' "${walked[*]}" + echo " compare against an ordinary dump of the same range: if they match," + echo " the engine is issuing a genuine address-only read." + echo +fi + +echo "=== verdict ===" +if [[ "$verdict" == none ]]; then + cat <<-EOF + NO ADDRESS-ONLY ENCODING FOUND in bits ${CANDIDATES[*]}. + Every candidate left the transaction unchanged, so the engine still + sends a register byte. SMBus Receive Byte and Send Byte cannot be + implemented honestly on this command word, and ee1004 (and therefore + decode-dimms) stays out of reach through this interface. + EOF +else + cat <<-EOF + BIT($verdict) CHANGES THE TRANSACTION. + Both probe reads returned something other than the register they named, + which is what an address-only transaction looks like. Next step is to + confirm the returned byte tracks the EEPROM pointer rather than being + another deterministic transform of the command word: read a known + sequence of registers and check the answers follow it. + EOF +fi diff --git a/tools/probe-send-byte.sh b/tools/probe-send-byte.sh new file mode 100755 index 0000000..5ddc203 --- /dev/null +++ b/tools/probe-send-byte.sh @@ -0,0 +1,200 @@ +#!/usr/bin/env bash +# +# WRITES TO THE BUS. Tests whether the pointer-select bit found by +# tools/probe-pntr-sel.sh also produces an SMBus Send Byte, the write half of +# I2C_FUNC_SMBUS_BYTE, which ee1004 needs to select a DDR4 SPD page. +# +# The target is the page-select address 0x36 or 0x37, never an EEPROM. Those +# addresses hold a single latch and store no data: the only effect a write can +# have there is to select page 0 or page 1. Nothing is written to 0x50-0x57. +# +# The test verifies itself. A DDR4 SPD is 512 bytes reached through a 256-byte +# window, so which page is latched is visible in the data: on page 0 byte 0x00 +# is 0x23 (the JEDEC "512 byte SPD" code), on page 1 the same offset shows +# byte 0x100 instead, which is not 0x23. Issue the Send Byte, read byte 0x00 +# back, and the page state is the answer. +# +# --restore is the safe first run. It targets a channel already latched on page +# 1 and selects page 0, so a working Send Byte repairs that channel and a +# broken one leaves it exactly as it was found. There is no outcome worse than +# the starting state. +# +# --flip runs the full round trip on a channel already on page 0: select page 1, +# confirm the data moved, select page 0 again, confirm it came back. Use it only +# after --restore has shown the encoding works. +# +set -euo pipefail + +# shellcheck source=tools/common.sh +source "$(dirname "$(readlink -f "$0")")/common.sh" + +COMMAND_TOGGLE=$((1 << 29)) +GO_BIT=$((1 << 19)) +PNTR_SEL_BIT=$((1 << 18)) +TSOD_ACTIVE_BIT=$((1 << 20)) +WRITE_OPERATION=$((1 << 15)) +STAT_BUSY=1 +STAT_ERROR=2 + +SPD_ADDR=0x50 +PAGE0_ADDR=0x36 +PAGE1_ADDR=0x37 +SPD_PAGE0_BYTE0=0x23 + +CHANNEL=0 +MODE=restore +CONFIRMED=false + +while (($#)); do + case $1 in + --i-understand) CONFIRMED=true; shift ;; + --channel) CHANNEL=$2; shift 2 ;; + --restore) MODE=restore; shift ;; + --flip) MODE=flip; shift ;; + -h|--help) + sed -n '3,24p' "$0" + echo "usage: $0 --i-understand --channel 0|1 [--restore|--flip]" + exit 0 + ;; + *) die "unknown argument: $1" ;; + esac +done + +$CONFIRMED || { + echo "This writes to the SPD page-select address. Re-run with --i-understand." >&2 + exit 2 +} + +require_root +require_cmd lspci setpci awk + +case $CHANNEL in +0) REG_DATA=$REG_CH0_DATA; REG_STAT=$REG_CH0_STAT; REG_CTRL=$REG_CH0_CTRL ;; +1) REG_DATA=$REG_CH1_DATA; REG_STAT=$REG_CH1_STAT; REG_CTRL=$REG_CH1_CTRL ;; +*) die "channel must be 0 or 1" ;; +esac + +driver_is_loaded && die "unload i2c-imc-skylake first" + +BDF=$(find_pcu) +original_data=$(read_reg "$BDF" "$REG_DATA") +original_ctrl=$(read_reg "$BDF" "$REG_CTRL") +status0=$(read_reg "$BDF" "$REG_STAT") + +((original_data & GO_BIT)) && die "GO already set, refusing" +((original_data & TSOD_ACTIVE_BIT)) && die "TSOD_ACTIVE set, refusing" +((status0 & STAT_BUSY)) && die "engine BUSY, refusing" + +restore() +{ + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$((original_data & ~GO_BIT))")" || true + setpci -s "$BDF" "$REG_CTRL.L=$(printf '%08x' "$original_ctrl")" || true +} +trap restore EXIT + +# wait_idle -> echoes the final status +wait_idle() +{ + local i data + + for ((i = 0; i < 60; i++)); do + data=$(read_reg "$BDF" "$REG_DATA") + ((data & GO_BIT)) || break + sleep 0.005 + done + read_reg "$BDF" "$REG_STAT" +} + +# spd_byte0 -> echoes the byte at SPD offset 0x00 as 0x%02x +spd_byte0() +{ + local ctrl + + setpci -s "$BDF" \ + "$REG_DATA.L=$(printf '%08x' "$((COMMAND_TOGGLE | GO_BIT | (SPD_ADDR << 8)))")" + wait_idle >/dev/null + ctrl=$(read_reg "$BDF" "$REG_CTRL") + printf '0x%02x\n' "$((ctrl & 0xff))" +} + +# send_byte -> echoes the status; returns 1 if the device NACKed +send_byte() +{ + local addr=$1 command status + + command=$((COMMAND_TOGGLE | GO_BIT | PNTR_SEL_BIT | WRITE_OPERATION | (addr << 8))) + # Data byte in the same place a BYTE_DATA write puts it. The page-select + # devices ignore it; it is set to zero so nothing meaningful is sent. + setpci -s "$BDF" "$REG_CTRL.L=00000000" + printf ' Send Byte to %s: command 0x%08x\n' "$addr" "$command" + setpci -s "$BDF" "$REG_DATA.L=$(printf '%08x' "$command")" + status=$(wait_idle) + printf ' status 0x%s\n' "${status#0x}" + # A NACK here is expected and harmless: some modules latch the page but + # do not acknowledge, which is why ee1004 rechecks instead of giving up. + ! ((status & STAT_ERROR)) +} + +echo "device $BDF channel $CHANNEL, mode $MODE" +echo "DATA before: $original_data CTRL before: $original_ctrl STATUS: $status0" +echo + +before=$(spd_byte0) +printf 'SPD 0x50 byte 0x00 before: %s\n' "$before" + +case $MODE in +restore) + if [[ "$before" == "$SPD_PAGE0_BYTE0" ]]; then + die "channel $CHANNEL is already on page 0, nothing to restore (use --flip)" + fi + echo "channel is not on page 0. Selecting page 0." + send_byte "$PAGE0_ADDR" || echo " (device did not ack; checking anyway)" + after=$(spd_byte0) + printf 'SPD 0x50 byte 0x00 after: %s\n\n' "$after" + + if [[ "$after" == "$SPD_PAGE0_BYTE0" ]]; then + cat <<-EOF + === SEND BYTE WORKS === + The page-select address accepted an address-only write: byte 0x00 went + from $before to $after, the JEDEC page-0 value. Bit 18 encodes SMBus + Send Byte as well as Receive Byte, so I2C_FUNC_SMBUS_BYTE can be + implemented honestly and ee1004 can bind. + + Channel $CHANNEL is also back on page 0, which is where it should have + been. + EOF + else + cat <<-EOF + === NO PAGE CHANGE === + Byte 0x00 is still $after. Either the write did not reach the bus in + Send Byte form, or this channel is not on page 1 for the reason + assumed. The channel is exactly as it was found; nothing was made + worse. Do not implement Send Byte on this encoding. + EOF + fi + ;; +flip) + [[ "$before" == "$SPD_PAGE0_BYTE0" ]] || + die "channel $CHANNEL is not on page 0 to begin with (byte 0x00 = $before)" + + echo "selecting page 1" + send_byte "$PAGE1_ADDR" || echo " (no ack; checking anyway)" + flipped=$(spd_byte0) + printf 'byte 0x00 on page 1: %s\n\n' "$flipped" + + echo "selecting page 0 again" + send_byte "$PAGE0_ADDR" || echo " (no ack; checking anyway)" + back=$(spd_byte0) + printf 'byte 0x00 restored: %s\n\n' "$back" + + echo "=== result ===" + if [[ "$flipped" != "$before" && "$back" == "$before" ]]; then + echo "Round trip clean: the page moved and came back. Send Byte confirmed." + elif [[ "$back" != "$before" ]]; then + echo "WARNING: the channel did not return to page 0 (byte 0x00 = $back)." + echo "Re-run with --restore to put it back." + else + echo "The page never moved. Send Byte is not encoded this way." + fi + ;; +esac