i2c-imc-skylake: close F2 with the register Intel documents

The open question was how to know whether the memory controller is polling
the DIMM thermal sensors, which is what killed the Broadwell-E attempt's
predecessor and what this driver had no answer for.

Intel documents it. The Xeon Processor Scalable Family datasheet volume 2
(reference 614073, section 3.1.9) puts SMBCNTL on the iMC channel function,
device 10 and 12 function 0, at offsets 0xe88 and 0xe98. SMB_TSOD_POLL_EN
is documented as mutually exclusive with SPD command access, so it answers
the question directly. The driver now reads it on every channel at probe
and refuses to bind if it is set.

This is deliberately weaker than the Broadwell-E driver, which switched the
polling off for the duration of a transfer. Reading and declining is a
smaller claim on hardware the driver does not own and needs no write to a
firmware-configured register. It covers the memory controller only; SMM and
a BMC are still out of reach, which is what the opt-in is for.

On the test system all four channels read 0xac000000: polling disabled,
TSOD_PRESENT empty, which also explains why no jc42 ever appeared at
0x18-0x1f on these DIMMs. Offset 0xe0, where the Broadwell driver has its
TSODCNTL, reads zero on all seven PCU functions here: that layout does not
transfer.

Two more measurements, both recorded in the cover letter:

  - the SMBus interface Intel documents, at 0xe80-0xe9c on the iMC, does not
    reach the bus on this platform. Issuing SMBCMD with CMD_TRIGGER sets
    SMB_RDO and clears SMB_BUSY, but returns 0x00 for a populated DIMM at
    0x50, a second one at 0x52 and an empty slot at 0x57 alike, with SMB_SBE
    never set. The path this driver uses returns the correct SPD contents
    and a NACK for the empty slot. The datasheet does not describe PCU
    function 5 at all, so the choice is now stated with data rather than
    left for a reviewer to ask about;
  - writes are issued on the bus: a byte write to an unpopulated address
    returns -ENXIO with the engine reporting the address NACK. This matters
    because SMB_DIS_WRT is set in the documented SMBCNTL and is SMM-writable
    only, so it evidently governs that interface and not this one.

Engine registers sampled every 2 s for an hour with the driver unloaded:
1791 samples, 0 changes.

Add tools/probe-imc-documented.sh, which produced the comparison above.
Smoke test with the new probe check: 15 checks, 0 failures.

docs/submission/cover-letter.txt

@@ -74,11 +74,22 @@ So this driver:
     previous transaction's data.  The engine only ever clears GO, so that
     is the single permitted difference between the two words.
 
-What is missing: no equivalent of Broadwell's TSODCNTL has been identified
+  - refuses to bind when the memory controller is polling the DIMM thermal
-on this part.  Offset 0xe0, where Broadwell has it, reads as zero here.  A
+    sensors itself.  Intel documents SMBCNTL on the iMC channel function
-system with CLTT enabled is therefore expected to trip the interference
+    (device 10 and 12, function 0, offsets 0xe88 and 0xe98) in the Xeon
-check rather than be arbitrated against.  I would rather ship a driver that
+    Processor Scalable Family datasheet volume 2, reference 614073, section
-detects the condition and refuses than one that quietly races.
+    3.1.9.  Its SMB_TSOD_POLL_EN bit is documented as mutually exclusive
+    with SPD command access, so it answers directly whether the engine is
+    in use.  On the test system all four channels read 0xac000000: polling
+    disabled, TSOD_PRESENT mask empty, which also explains why no jc42
+    device appears at 0x18-0x1f.
+
+This is deliberately weaker than what the Broadwell-E driver did.  That one
+switched the polling off for the duration of a transfer.  Reading the bit
+and declining is a smaller claim on hardware the driver does not own, and
+it needs no write to a firmware-configured register.  It covers the memory
+controller only; SMM and a BMC remain out of reach, which is what the
+opt-in is for.
 
 Relation to prior work
 ~~~~~~~~~~~~~~~~~~~~~~
@@ -119,6 +130,7 @@ Changes since v2
     default-off gate, which was logging a driver-core error on every boot.
   - Add Documentation/i2c/busses/i2c-imc-skylake.rst.
   - Drop the ACPI dependency.
+  - Refuse to bind when the iMC reports TSOD polling enabled.
 
 Validation
 ~~~~~~~~~~
@@ -132,12 +144,35 @@ Validation
     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.
-  - Config space of the PCU function sampled with the driver unloaded: no
+  - Engine registers sampled every 2 s for one hour with the driver
-    register changed on its own, and TSOD_ACTIVE was clear on both channels,
+    unloaded: 1791 samples, 0 changes.  No firmware activity was observed
-    so no firmware activity was observed on this system.
+    on this system.
+  - SMBCNTL on all four iMC channels reads 0xac000000: TSOD polling
+    disabled, TSOD_PRESENT empty.
+  - Write transactions reach the bus: a byte write to an unpopulated
+    address returns -ENXIO with the engine reporting the address NACK, so
+    the write path is issued rather than suppressed.  An end-to-end write
+    to a populated device is not part of this validation; the only
+    non-SPD device on the test system is an LED controller whose reads are
+    themselves unreliable, and SPD writes were out of scope.
 
 Still missing: an independent Tested-by, ideally on Skylake-X rather than
-Cascade Lake-X, and suspend/resume testing with active I2C clients.
+Cascade Lake-X, suspend/resume testing with active I2C clients, and an
+end-to-end write to a populated device.
+
+One note on the register interface, since a reader with the datasheet will
+ask.  That document describes an SMBus interface on the iMC channel
+function at 0xe80-0xe9c, with a different encoding from the one this driver
+uses on PCU function 5 at 0x9c-0xb8; it does not describe function 5 at all,
+covering device 30 functions 0 and 2 only.  I tried the documented path on
+this system: issuing SMBCMD with CMD_TRIGGER sets SMB_RDO and clears
+SMB_BUSY, but returns 0x00 for a populated DIMM at 0x50, for a second one
+at 0x52, and for an empty slot at 0x57 alike, with SMB_SBE never set.  An
+interface that cannot tell a populated slot from an empty one is not
+reaching the bus here, while the path this driver uses returns the correct
+SPD contents and a NACK for the empty slot.  I may well be missing a setup
+step; if someone knows what it is, I would rather use the documented
+interface.
 
 Tool assistance
 ~~~~~~~~~~~~~~~

docs/submission/i2c-imc-skylake.rst

@@ -48,7 +48,18 @@ Do this only on a system where concurrent firmware access has been excluded.
 Desktop and HEDT boards have no BMC, but CLTT may still be enabled by the
 firmware.
 
-The driver reduces, but cannot remove, the risk:
+The driver reduces, but cannot remove, the risk.
+
+At probe it reads SMBCNTL on every iMC channel function (device 10 and 12,
+function 0; offsets 0xe88 and 0xe98, documented in the Intel Xeon Processor
+Scalable Family datasheet volume 2, reference 614073, section 3.1.9).
+``SMB_TSOD_POLL_EN`` in that register says whether the memory controller is
+polling the DIMM thermal sensors on its own, which Intel documents as mutually
+exclusive with SPD command access.  If the bit is set on any channel the driver
+refuses to bind: the engine is not ours to drive.  This covers the memory
+controller, not SMM or a BMC, which remain the reason for the opt-in.
+
+Per transfer:
 
   * every transaction waits for both channels to report idle before it
     touches any register;

i2c-imc-skylake.c

@@ -57,6 +57,18 @@
 /* PCI id of the Sky Lake-E PCU function carrying the SMBus engine */
 #define PCU_DEVICE	0x2085
 
+/*
+ * The iMC channel function, device 10 and 12 function 0, carries the SMBus
+ * control register documented in the Intel Xeon Processor Scalable Family
+ * datasheet volume 2 (reference 614073) section 3.1.9.  Only SMBCNTL is read
+ * here, and only to find out whether the hardware is polling the DIMM thermal
+ * sensors on its own.
+ */
+#define IMC_CHANNEL_DEVICE	0x2040
+#define IMC_SMBCNTL(ch)		(0xE88 + (ch) * 0x10)
+#define SMBCNTL_TSOD_POLL_EN	BIT(8)
+#define SMBCNTL_TSOD_PRESENT	GENMASK(7, 0)
+
 /* per-channel register offsets in the function's config space */
 #define CH0_CTRL	0xB4
 #define CH0_DATA	0x9C
@@ -524,6 +536,51 @@ static int imc_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume);
 
+/*
+ * Refuse to share the engine with the memory controller's own thermal
+ * throttling.  SMBCNTL.TSOD_POLL_EN tells us directly whether the iMC is
+ * issuing SMBus transactions of its own; Intel documents that SPD command
+ * access and TSOD polling are mutually exclusive, so a set bit means the
+ * engine is not ours to drive.  This does not cover SMM or a BMC, which
+ * remain the reason for the allow_unsafe_access gate.
+ */
+static int imc_check_firmware_polling(struct pci_dev *pdev)
+{
+	struct pci_dev *imc = NULL;
+	unsigned int found = 0;
+	int i;
+
+	while ((imc = pci_get_device(PCI_VENDOR_ID_INTEL, IMC_CHANNEL_DEVICE,
+				     imc))) {
+		found++;
+		for (i = 0; i < ARRAY_SIZE(imc_chans); i++) {
+			u32 cntl;
+
+			if (pci_read_config_dword(imc, IMC_SMBCNTL(i), &cntl))
+				continue;
+
+			if (cntl & SMBCNTL_TSOD_POLL_EN) {
+				dev_err(&pdev->dev,
+					"%s ch%d has TSOD polling enabled (SMBCNTL 0x%08x); the memory controller is using the engine\n",
+					pci_name(imc), i, cntl);
+				pci_dev_put(imc);
+				return -EBUSY;
+			}
+
+			dev_dbg(&pdev->dev,
+				"%s ch%d SMBCNTL 0x%08x, TSOD present mask 0x%02lx\n",
+				pci_name(imc), i, cntl,
+				cntl & SMBCNTL_TSOD_PRESENT);
+		}
+	}
+
+	if (!found)
+		dev_warn(&pdev->dev,
+			 "no iMC channel function found; cannot verify that TSOD polling is disabled\n");
+
+	return 0;
+}
+
 static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct imc_smbus *s;
@@ -535,6 +592,10 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENODEV;
 	}
 
+	ret = imc_check_firmware_polling(pdev);
+	if (ret)
+		return ret;
+
 	s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
 	if (!s)
 		return -ENOMEM;

tools/probe-imc-documented.sh

@@ -0,0 +1,135 @@
+#!/usr/bin/env bash
+#
+# WRITES TO HARDWARE. Issues an SMBus read through the interface Intel
+# documents, rather than the one this driver currently uses.
+#
+# Intel Xeon Processor Scalable Family datasheet vol. 2 (ref. 614073) chapter 3
+# puts the iMC SMBus interface on device 10 and 12, function 0:
+#
+#   SMB_STAT_[0:1]   0xe80 0xe90   b31 RDO, b30 WOD, b29 SBE, b28 BUSY,
+#                                  b27:24 tsod_sa, b15:0 RDATA
+#   SMBCMD_[0:1]     0xe84 0xe94   b31 CMD_TRIGGER, b30 PNTR_SEL,
+#                                  b29 WORD_ACCESS, b28:27 rw, b26:24 SA,
+#                                  b23:16 BA, b15:0 WDATA
+#   SMBCNTL_[0:1]    0xe88 0xe98   b31:28 DTI (0xa EEPROM, 0x3 TSOD),
+#                                  b27 CKOVRD, b26 DIS_WRT, b10 SOFT_RST,
+#                                  b9 start_tsod_poll, b8 TSOD_POLL_EN,
+#                                  b7:0 TSOD_PRESENT
+#
+# The driver instead uses PCU function 5 at 0x9c-0xb8, which that document does
+# not describe (it covers device 30 functions 0 and 2 only).  This script reads
+# a known SPD byte through the documented path so the two can be compared.
+#
+# Read transaction only, against an EEPROM that is certainly present.  SMBCMD is
+# restored on every exit path.  SMBCNTL is not written at all.
+#
+set -euo pipefail
+
+# shellcheck source=tools/common.sh
+source "$(dirname "$(readlink -f "$0")")/common.sh"
+
+SMB_STAT=(0xe80 0xe90)
+SMB_CMD=(0xe84 0xe94)
+SMB_CNTL=(0xe88 0xe98)
+
+CMD_TRIGGER=$((1 << 31))
+STAT_RDO=$((1 << 31))
+STAT_WOD=$((1 << 30))
+STAT_SBE=$((1 << 29))
+STAT_BUSY=$((1 << 28))
+CNTL_TSOD_POLL_EN=$((1 << 8))
+
+SA=0		# slave address low bits; DTI 0xa | SA 0 selects 0x50
+BA=0x02		# SPD byte 2, DDR4 type code, expected 0x0c
+CONFIRMED=false
+
+while (($#)); do
+	case $1 in
+	--i-understand) CONFIRMED=true; shift ;;
+	--sa) SA=$2; shift 2 ;;
+	--ba) BA=$2; shift 2 ;;
+	-h|--help)
+		sed -n '3,26p' "$0"
+		echo "usage: $0 --i-understand [--sa N] [--ba 0xNN]"
+		exit 0
+		;;
+	*) die "unknown argument: $1" ;;
+	esac
+done
+
+$CONFIRMED || {
+	echo "Issues an SMBus read from userspace. Re-run with --i-understand." >&2
+	exit 2
+}
+
+require_root
+require_cmd lspci setpci awk
+driver_is_loaded && die "unload i2c-imc-skylake first"
+
+mapfile -t IMCS < <(lspci -Dn -d 8086:2040 | awk '{print $1}')
+((${#IMCS[@]})) || die "no iMC channel device (8086:2040) found"
+
+probe_one()
+{
+	local bdf=$1 ch=$2
+	local stat=${SMB_STAT[ch]} cmd=${SMB_CMD[ch]} cntl=${SMB_CNTL[ch]}
+	local cntl_val stat_val cmd_saved command i data
+
+	cntl_val=$(read_reg "$bdf" "$cntl")
+	stat_val=$(read_reg "$bdf" "$stat")
+	cmd_saved=$(read_reg "$bdf" "$cmd")
+
+	printf '  SMBCNTL=%s  SMB_STAT=%s  SMBCMD=%s\n' \
+		"$cntl_val" "$stat_val" "$cmd_saved"
+
+	if ((cntl_val & CNTL_TSOD_POLL_EN)); then
+		echo "  SKIP: TSOD_POLL_EN is set, firmware owns this channel"
+		return 0
+	fi
+	if ((stat_val & STAT_BUSY)); then
+		echo "  SKIP: SMB_BUSY is set"
+		return 0
+	fi
+
+	# byte read: TRIGGER, PNTR_SEL=0, WORD=0, rw=00, SA, BA
+	command=$((CMD_TRIGGER | (SA << 24) | (BA << 16)))
+	printf '  issuing SMBCMD=0x%08x (read SA=%d BA=%s)\n' "$command" "$SA" "$BA"
+	setpci -s "$bdf" "$cmd.L=$(printf '%08x' "$command")"
+
+	for i in $(seq 1 50); do
+		stat_val=$(read_reg "$bdf" "$stat")
+		((stat_val & STAT_BUSY)) || break
+		sleep 0.005
+	done
+
+	setpci -s "$bdf" "$cmd.L=$(printf '%08x' "$cmd_saved")"
+
+	data=$((stat_val & 0xff))
+	printf '  SMB_STAT=%s  RDO=%d WOD=%d SBE=%d BUSY=%d  RDATA=0x%02x\n' \
+		"$stat_val" \
+		"$(((stat_val & STAT_RDO) != 0))" \
+		"$(((stat_val & STAT_WOD) != 0))" \
+		"$(((stat_val & STAT_SBE) != 0))" \
+		"$(((stat_val & STAT_BUSY) != 0))" \
+		"$data"
+
+	if ((stat_val & STAT_SBE)); then
+		echo "  -> SBE: the addressed device did not answer on this channel"
+	elif ((stat_val & STAT_RDO)); then
+		printf '  -> READ OK, data 0x%02x\n' "$data"
+		if [[ "$BA" == "0x02" ]] && ((data == 0x0c)); then
+			echo "     matches the DDR4 type code, so the documented"
+			echo "     interface reaches the same devices."
+		fi
+	else
+		echo "  -> no completion: the interface did not act on the command"
+	fi
+}
+
+for bdf in "${IMCS[@]}"; do
+	for ch in 0 1; do
+		echo "=== $bdf channel $ch ==="
+		probe_one "$bdf" "$ch"
+		echo
+	done
+done