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