[PATCH v3 0/4] hwmon: Add support for SPD5118 compliant chips

[PATCH v3 0/4] hwmon: Add support for SPD5118 compliant chips

[Guenter Roeck]

Add support for SPD5118 (Jedec JESD300) compliant chips supporting
a temperature sensor and SPD NVRAM. Such devices are typically found on
DDR5 memory modules.

The first patch of the series adds SPD5118 devicetree bindings. The second
patch adds support for SPD5118 temperature sensors. The third patch adds
support for suspend/resume. The last patch adds support for reading the SPD
NVRAM.

Note: The driver introduced with this patch series does not currently
support accessing SPD5118 compliant chips in I3C mode.

v3: Drop explicit bindings document; add binding to trivial devices instead
    Add support for reading SPD NVRAM

v2: Drop PEC support; it only applies to I3C mode.
    Update documentation
    Add suspend/resume support 

----------------------------------------------------------------
Guenter Roeck (4):
      dt-bindings: trivial-devices: Add jedec,spd5118
      hwmon: Add support for SPD5118 compliant temperature sensors
      hwmon: (spd5118) Add suspend/resume support
      hwmon: (spd5118) Add support for reading SPD data

 .../devicetree/bindings/trivial-devices.yaml       |   2 +
 Documentation/hwmon/index.rst                      |   1 +
 Documentation/hwmon/spd5118.rst                    |  56 ++
 drivers/hwmon/Kconfig                              |  12 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/spd5118.c                            | 648 +++++++++++++++++++++
 6 files changed, 720 insertions(+)
 create mode 100644 Documentation/hwmon/spd5118.rst
 create mode 100644 drivers/hwmon/spd5118.c

[PATCH v3 1/4] dt-bindings: trivial-devices: Add jedec,spd5118

[Guenter Roeck]

Add bindings for the SPD hub present in DDR5 modules.
(https://www.jedec.org/standards-documents/docs/jesd300-5b01).

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: Drop explicit bindings file; add binding to trivial devices
    instead

 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 0a419453d183..1d19e67de2a1 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -168,6 +168,8 @@ properties:
           - isil,isl69269
             # Intersil ISL76682 Ambient Light Sensor
           - isil,isl76682
+            # JEDEC JESD300 (SPD5118) Hub and Serial Presence Detect
+          - jedec,spd5118
             # Linear Technology LTC2488
           - lineartechnology,ltc2488
             # 5 Bit Programmable, Pulse-Width Modulator
-- 
2.39.2

Re: [PATCH v3 1/4] dt-bindings: trivial-devices: Add jedec,spd5118

[Krzysztof Kozlowski]

On 01/06/2024 01:05, Guenter Roeck wrote:
> Add bindings for the SPD hub present in DDR5 modules.
> (https://www.jedec.org/standards-documents/docs/jesd300-5b01).
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: Drop explicit bindings file; add binding to trivial devices

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v3 2/4] hwmon: Add support for SPD5118 compliant temperature sensors

[Guenter Roeck]

Add support for SPD5118 (Jedec JESD300) compliant temperature
sensors. Such sensors are typically found on DDR5 memory modules.

Cc: René Rebe <rene@exactcode.de>
Cc: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
Tested-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: Shorten JESD300-5B.01 to JESD300; 5B.01 refers to the version
    of the standard
    Drop unnecessary 'attr' parameter from spd5118_{read,write}_enable()

v2: Drop PEC property documentation
    Add note indicating that alarm attributes are sticky until read
    to documentation
    Fix detect function
    Fix misspelling in Makefile (CONFIG_SENSORS_SPD5118->CONFIG_SENSORS_SPD5118)

 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/spd5118.rst |  55 ++++
 drivers/hwmon/Kconfig           |  12 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/spd5118.c         | 481 ++++++++++++++++++++++++++++++++
 5 files changed, 550 insertions(+)
 create mode 100644 Documentation/hwmon/spd5118.rst
 create mode 100644 drivers/hwmon/spd5118.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 03d313af469a..6e7b8726b60c 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -215,6 +215,7 @@ Hardware Monitoring Kernel Drivers
    smsc47m192
    smsc47m1
    sparx5-temp
+   spd5118
    stpddc60
    surface_fan
    sy7636a-hwmon
diff --git a/Documentation/hwmon/spd5118.rst b/Documentation/hwmon/spd5118.rst
new file mode 100644
index 000000000000..a15d75aa2066
--- /dev/null
+++ b/Documentation/hwmon/spd5118.rst
@@ -0,0 +1,55 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver spd5118
+=====================
+
+Supported chips:
+
+  * SPD5118 (JEDEC JESD300) compliant temperature sensor chips
+
+    JEDEC standard download:
+	https://www.jedec.org/standards-documents/docs/jesd300-5b01
+	(account required)
+
+
+    Prefix: 'spd5118'
+
+    Addresses scanned: I2C 0x50 - 0x57
+
+Author:
+	Guenter Roeck <linux@roeck-us.net>
+
+
+Description
+-----------
+
+This driver implements support for SPD5118 (JEDEC JESD300) compliant temperature
+sensors, which are used on many DDR5 memory modules. Some systems use the sensor
+to prevent memory overheating by automatically throttling the memory controller.
+
+The driver auto-detects SPD5118 compliant chips, but can also be instantiated
+using devicetree/firmware nodes.
+
+A SPD5118 compliant chip supports a single temperature sensor. Critical minimum,
+minimum, maximum, and critical temperature can be configured. There are alarms
+for low critical, low, high, and critical thresholds.
+
+
+Hardware monitoring sysfs entries
+---------------------------------
+
+======================= ==================================
+temp1_input		Temperature (RO)
+temp1_lcrit		Low critical high temperature (RW)
+temp1_min		Minimum temperature (RW)
+temp1_max		Maximum temperature (RW)
+temp1_crit		Critical high temperature (RW)
+
+temp1_lcrit_alarm	Temperature low critical alarm
+temp1_min_alarm		Temperature low alarm
+temp1_max_alarm		Temperature high alarm
+temp1_crit_alarm	Temperature critical alarm
+======================= ==================================
+
+Alarm attributes are sticky until read and will be cleared afterwards
+unless the alarm condition still applies.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..d0fb5fe1b2ac 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2181,6 +2181,18 @@ config SENSORS_INA3221
 	  This driver can also be built as a module. If so, the module
 	  will be called ina3221.
 
+config SENSORS_SPD5118
+	tristate "SPD5118 Compliant Temperature Sensors"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for SPD5118 (JEDEC JESD300)
+	  compliant temperature sensors. Such sensors are found on DDR5 memory
+	  modules.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called spd5118.
+
 config SENSORS_TC74
 	tristate "Microchip TC74"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e3f25475d1f0..6574ca67d761 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -207,6 +207,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
+obj-$(CONFIG_SENSORS_SPD5118)	+= spd5118.o
 obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
 obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
 obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
new file mode 100644
index 000000000000..d3fc0ae17743
--- /dev/null
+++ b/drivers/hwmon/spd5118.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Jedec 5118 compliant temperature sensors
+ *
+ * Derived from https://github.com/Steve-Tech/SPD5118-DKMS
+ * Originally from T/2 driver at https://t2sde.org/packages/linux
+ *	Copyright (c) 2023 René Rebe, ExactCODE GmbH; Germany.
+ *
+ * Copyright (c) 2024 Guenter Roeck
+ *
+ * Inspired by ee1004.c and jc42.c.
+ *
+ * SPD5118 compliant temperature sensors are typically used on DDR5
+ * memory modules.
+ */
+
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = {
+	0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, I2C_CLIENT_END };
+
+/* SPD5118 registers. */
+#define SPD5118_REG_TYPE		0x00	/* MR0:MR1 */
+#define SPD5118_REG_REVISION		0x02	/* MR2 */
+#define SPD5118_REG_VENDOR		0x03	/* MR3:MR4 */
+#define SPD5118_REG_CAPABILITY		0x05	/* MR5 */
+#define SPD5118_REG_I2C_LEGACY_MODE	0x0B	/* MR11 */
+#define SPD5118_REG_TEMP_CLR		0x13	/* MR19 */
+#define SPD5118_REG_ERROR_CLR		0x14	/* MR20 */
+#define SPD5118_REG_TEMP_CONFIG		0x1A	/* MR26 */
+#define SPD5118_REG_TEMP_MAX		0x1c	/* MR28:MR29 */
+#define SPD5118_REG_TEMP_MIN		0x1e	/* MR30:MR31 */
+#define SPD5118_REG_TEMP_CRIT		0x20	/* MR32:MR33 */
+#define SPD5118_REG_TEMP_LCRIT		0x22	/* MR34:MR35 */
+#define SPD5118_REG_TEMP		0x31	/* MR49:MR50 */
+#define SPD5118_REG_TEMP_STATUS		0x33	/* MR51 */
+
+#define SPD5118_TEMP_STATUS_HIGH	BIT(0)
+#define SPD5118_TEMP_STATUS_LOW		BIT(1)
+#define SPD5118_TEMP_STATUS_CRIT	BIT(2)
+#define SPD5118_TEMP_STATUS_LCRIT	BIT(3)
+
+#define SPD5118_CAP_TS_SUPPORT		BIT(1)	/* temperature sensor support */
+
+#define SPD5118_TS_DISABLE		BIT(0)	/* temperature sensor disable */
+
+/* Temperature unit in millicelsius */
+#define SPD5118_TEMP_UNIT		(MILLIDEGREE_PER_DEGREE / 4)
+/* Representable temperature range in millicelsius */
+#define SPD5118_TEMP_RANGE_MIN		-256000
+#define SPD5118_TEMP_RANGE_MAX		255750
+
+static int spd5118_temp_from_reg(u16 reg)
+{
+	int temp = sign_extend32(reg >> 2, 10);
+
+	return temp * SPD5118_TEMP_UNIT;
+}
+
+static u16 spd5118_temp_to_reg(long temp)
+{
+	temp = clamp_val(temp, SPD5118_TEMP_RANGE_MIN, SPD5118_TEMP_RANGE_MAX);
+	return (DIV_ROUND_CLOSEST(temp, SPD5118_TEMP_UNIT) & 0x7ff) << 2;
+}
+
+static int spd5118_read_temp(struct regmap *regmap, u32 attr, long *val)
+{
+	int reg, err;
+	u8 regval[2];
+	u16 temp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = SPD5118_REG_TEMP;
+		break;
+	case hwmon_temp_max:
+		reg = SPD5118_REG_TEMP_MAX;
+		break;
+	case hwmon_temp_min:
+		reg = SPD5118_REG_TEMP_MIN;
+		break;
+	case hwmon_temp_crit:
+		reg = SPD5118_REG_TEMP_CRIT;
+		break;
+	case hwmon_temp_lcrit:
+		reg = SPD5118_REG_TEMP_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_bulk_read(regmap, reg, regval, 2);
+	if (err)
+		return err;
+
+	temp = (regval[1] << 8) | regval[0];
+
+	*val = spd5118_temp_from_reg(temp);
+	return 0;
+}
+
+static int spd5118_read_alarm(struct regmap *regmap, u32 attr, long *val)
+{
+	unsigned int mask, regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_temp_max_alarm:
+		mask = SPD5118_TEMP_STATUS_HIGH;
+		break;
+	case hwmon_temp_min_alarm:
+		mask = SPD5118_TEMP_STATUS_LOW;
+		break;
+	case hwmon_temp_crit_alarm:
+		mask = SPD5118_TEMP_STATUS_CRIT;
+		break;
+	case hwmon_temp_lcrit_alarm:
+		mask = SPD5118_TEMP_STATUS_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(regmap, SPD5118_REG_TEMP_STATUS, &regval);
+	if (err < 0)
+		return err;
+	*val = !!(regval & mask);
+	if (*val)
+		return regmap_write(regmap, SPD5118_REG_TEMP_CLR, mask);
+	return 0;
+}
+
+static int spd5118_read_enable(struct regmap *regmap, long *val)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
+	if (err < 0)
+		return err;
+	*val = !(regval & SPD5118_TS_DISABLE);
+	return 0;
+}
+
+static int spd5118_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_lcrit:
+		return spd5118_read_temp(regmap, attr, val);
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_lcrit_alarm:
+		return spd5118_read_alarm(regmap, attr, val);
+	case hwmon_temp_enable:
+		return spd5118_read_enable(regmap, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int spd5118_write_temp(struct regmap *regmap, u32 attr, long val)
+{
+	u8 regval[2];
+	u16 temp;
+	int reg;
+
+	switch (attr) {
+	case hwmon_temp_max:
+		reg = SPD5118_REG_TEMP_MAX;
+		break;
+	case hwmon_temp_min:
+		reg = SPD5118_REG_TEMP_MIN;
+		break;
+	case hwmon_temp_crit:
+		reg = SPD5118_REG_TEMP_CRIT;
+		break;
+	case hwmon_temp_lcrit:
+		reg = SPD5118_REG_TEMP_LCRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	temp = spd5118_temp_to_reg(val);
+	regval[0] = temp & 0xff;
+	regval[1] = temp >> 8;
+
+	return regmap_bulk_write(regmap, reg, regval, 2);
+}
+
+static int spd5118_write_enable(struct regmap *regmap, long val)
+{
+	if (val && val != 1)
+		return -EINVAL;
+
+	return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
+				  SPD5118_TS_DISABLE,
+				  val ? 0 : SPD5118_TS_DISABLE);
+}
+
+static int spd5118_temp_write(struct regmap *regmap, u32 attr, long val)
+{
+	switch (attr) {
+	case hwmon_temp_max:
+	case hwmon_temp_min:
+	case hwmon_temp_crit:
+	case hwmon_temp_lcrit:
+		return spd5118_write_temp(regmap, attr, val);
+	case hwmon_temp_enable:
+		return spd5118_write_enable(regmap, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		return spd5118_temp_write(regmap, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	if (type != hwmon_temp)
+		return 0;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		return 0444;
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+	case hwmon_temp_lcrit:
+	case hwmon_temp_crit:
+	case hwmon_temp_enable:
+		return 0644;
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_lcrit_alarm:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static inline bool spd5118_parity8(u8 w)
+{
+	w ^= w >> 4;
+	return (0x6996 >> (w & 0xf)) & 1;
+}
+
+/*
+ * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
+ * Vendor IDs 0 and 0x7f are invalid.
+ * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs.
+ */
+static bool spd5118_vendor_valid(u8 bank, u8 id)
+{
+	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
+		return false;
+
+	id &= 0x7f;
+	return id && id != 0x7f;
+}
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int regval;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+	if (regval != 0x5118)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
+	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
+	if (regval < 0)
+		return -ENODEV;
+	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
+	if (regval)
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
+	if (regval)
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
+	if (regval < 0 || (regval & 0xc1))
+		return -ENODEV;
+
+	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
+	if (regval < 0)
+		return -ENODEV;
+	if (regval & ~SPD5118_TS_DISABLE)
+		return -ENODEV;
+
+	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
+	return 0;
+}
+
+static const struct hwmon_channel_info *spd5118_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT |
+			   HWMON_T_LCRIT | HWMON_T_LCRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+			   HWMON_T_ENABLE),
+	NULL
+};
+
+static const struct hwmon_ops spd5118_hwmon_ops = {
+	.is_visible = spd5118_is_visible,
+	.read = spd5118_read,
+	.write = spd5118_write,
+};
+
+static const struct hwmon_chip_info spd5118_chip_info = {
+	.ops = &spd5118_hwmon_ops,
+	.info = spd5118_info,
+};
+
+static bool spd5118_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SPD5118_REG_TEMP_CLR:
+	case SPD5118_REG_TEMP_CONFIG:
+	case SPD5118_REG_TEMP_MAX:
+	case SPD5118_REG_TEMP_MAX + 1:
+	case SPD5118_REG_TEMP_MIN:
+	case SPD5118_REG_TEMP_MIN + 1:
+	case SPD5118_REG_TEMP_CRIT:
+	case SPD5118_REG_TEMP_CRIT + 1:
+	case SPD5118_REG_TEMP_LCRIT:
+	case SPD5118_REG_TEMP_LCRIT + 1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SPD5118_REG_TEMP_CLR:
+	case SPD5118_REG_ERROR_CLR:
+	case SPD5118_REG_TEMP:
+	case SPD5118_REG_TEMP + 1:
+	case SPD5118_REG_TEMP_STATUS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config spd5118_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = SPD5118_REG_TEMP_STATUS,
+	.writeable_reg = spd5118_writeable_reg,
+	.volatile_reg = spd5118_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int spd5118_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	unsigned int regval, revision, vendor, bank;
+	struct device *hwmon_dev;
+	struct regmap *regmap;
+	int err;
+
+	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
+
+	err = regmap_read(regmap, SPD5118_REG_CAPABILITY, &regval);
+	if (err)
+		return err;
+	if (!(regval & SPD5118_CAP_TS_SUPPORT))
+		return -ENODEV;
+
+	err = regmap_read(regmap, SPD5118_REG_REVISION, &revision);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, SPD5118_REG_VENDOR, &bank);
+	if (err)
+		return err;
+	err = regmap_read(regmap, SPD5118_REG_VENDOR + 1, &vendor);
+	if (err)
+		return err;
+	if (!spd5118_vendor_valid(bank, vendor))
+		return -ENODEV;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
+							 regmap, &spd5118_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	/*
+	 * From JESD300-5B
+	 *   MR2 bits [5:4]: Major revision, 1..4
+	 *   MR2 bits [3:1]: Minor revision, 0..8? Probably a typo, assume 1..8
+	 */
+	dev_info(dev, "DDR5 temperature sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
+		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1, ((revision >> 1) & 0x07) + 1);
+
+	return 0;
+}
+
+static const struct i2c_device_id spd5118_id[] = {
+	{ "spd5118", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, spd5118_id);
+
+static const struct of_device_id spd5118_of_ids[] = {
+	{ .compatible = "jedec,spd5118", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, spd5118_of_ids);
+
+static struct i2c_driver spd5118_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "spd5118",
+		.of_match_table = spd5118_of_ids,
+	},
+	.probe		= spd5118_probe,
+	.id_table	= spd5118_id,
+	.detect		= spd5118_detect,
+	.address_list	= normal_i2c,
+};
+
+module_i2c_driver(spd5118_driver);
+
+MODULE_AUTHOR("René Rebe <rene@exactcode.de>");
+MODULE_AUTHOR("Guenter Roeck <linux@roeck-us.net>");
+MODULE_DESCRIPTION("SPD 5118 driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v3 2/4] hwmon: Add support for SPD5118 compliant temperature sensors

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]


> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	int regval;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
> +	if (regval != 0x5118)
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
> +	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
> +	if (regval < 0)
> +		return -ENODEV;
> +	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
> +	if (regval)
> +		return -ENODEV;
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
> +	if (regval)
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
> +	if (regval < 0 || (regval & 0xc1))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
> +	if (regval < 0)
> +		return -ENODEV;
> +	if (regval & ~SPD5118_TS_DISABLE)
> +		return -ENODEV;
> +
> +	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
> +	return 0;
> +}

What about adding DDR5 to i2c_register_spd() and dropping this function?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] hwmon: Add support for SPD5118 compliant temperature sensors

[Guenter Roeck]

On 5/31/24 18:28, Wolfram Sang wrote:
> 
>> +/* Return 0 if detection is successful, -ENODEV otherwise */
>> +static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
>> +{
>> +	struct i2c_adapter *adapter = client->adapter;
>> +	int regval;
>> +
>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>> +				     I2C_FUNC_SMBUS_WORD_DATA))
>> +		return -ENODEV;
>> +
>> +	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
>> +	if (regval != 0x5118)
>> +		return -ENODEV;
>> +
>> +	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
>> +	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
>> +		return -ENODEV;
>> +
>> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
>> +	if (regval < 0)
>> +		return -ENODEV;
>> +	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
>> +		return -ENODEV;
>> +
>> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
>> +	if (regval)
>> +		return -ENODEV;
>> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
>> +	if (regval)
>> +		return -ENODEV;
>> +
>> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
>> +	if (regval < 0 || (regval & 0xc1))
>> +		return -ENODEV;
>> +
>> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
>> +	if (regval < 0)
>> +		return -ENODEV;
>> +	if (regval & ~SPD5118_TS_DISABLE)
>> +		return -ENODEV;
>> +
>> +	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
>> +	return 0;
>> +}
> 
> What about adding DDR5 to i2c_register_spd() and dropping this function?
> 

Yes, that should be the next step. I didn't want to do that here because it
would introduce a cross-subsystem dependency. Of course, that depends a bit
on your position about such dependencies. If I do that as part of this series,
would you Ack it, or would you want to handle that through the i2c tree ?

Guenter

Re: [PATCH v3 2/4] hwmon: Add support for SPD5118 compliant temperature sensors

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]


> would introduce a cross-subsystem dependency. Of course, that depends a bit
> on your position about such dependencies. If I do that as part of this series,
> would you Ack it, or would you want to handle that through the i2c tree ?

I would ack it. If I'd have a conflicting commit in my tree (unlikely),
I'd ask you for an immutable branch to pull into my tree.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] hwmon: Add support for SPD5118 compliant temperature sensors

[Guenter Roeck]

On 6/2/24 13:18, Wolfram Sang wrote:
> 
>> would introduce a cross-subsystem dependency. Of course, that depends a bit
>> on your position about such dependencies. If I do that as part of this series,
>> would you Ack it, or would you want to handle that through the i2c tree ?
> 
> I would ack it. If I'd have a conflicting commit in my tree (unlikely),
> I'd ask you for an immutable branch to pull into my tree.
> 

Sounds good.

Turns out we can not entirely remove the detect code anyway.
i2c_register_spd() depends on DMI support, which is not a given
on all systems supporting DDR5, and it is limited to a maximum
of 8 memory modules. I'll try to make it conditional with a
configuration option.

Thanks,
Guenter

Re: [PATCH v3 2/4] hwmon: Add support for SPD5118 compliant temperature sensors

[Armin Wolf]

Am 01.06.24 um 01:05 schrieb Guenter Roeck:

> Add support for SPD5118 (Jedec JESD300) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.

Tested-by: Armin Wolf <W_Armin@gmx.de>
Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Cc: René Rebe <rene@exactcode.de>
> Cc: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: Shorten JESD300-5B.01 to JESD300; 5B.01 refers to the version
>      of the standard
>      Drop unnecessary 'attr' parameter from spd5118_{read,write}_enable()
>
> v2: Drop PEC property documentation
>      Add note indicating that alarm attributes are sticky until read
>      to documentation
>      Fix detect function
>      Fix misspelling in Makefile (CONFIG_SENSORS_SPD5118->CONFIG_SENSORS_SPD5118)
>
>   Documentation/hwmon/index.rst   |   1 +
>   Documentation/hwmon/spd5118.rst |  55 ++++
>   drivers/hwmon/Kconfig           |  12 +
>   drivers/hwmon/Makefile          |   1 +
>   drivers/hwmon/spd5118.c         | 481 ++++++++++++++++++++++++++++++++
>   5 files changed, 550 insertions(+)
>   create mode 100644 Documentation/hwmon/spd5118.rst
>   create mode 100644 drivers/hwmon/spd5118.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 03d313af469a..6e7b8726b60c 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -215,6 +215,7 @@ Hardware Monitoring Kernel Drivers
>      smsc47m192
>      smsc47m1
>      sparx5-temp
> +   spd5118
>      stpddc60
>      surface_fan
>      sy7636a-hwmon
> diff --git a/Documentation/hwmon/spd5118.rst b/Documentation/hwmon/spd5118.rst
> new file mode 100644
> index 000000000000..a15d75aa2066
> --- /dev/null
> +++ b/Documentation/hwmon/spd5118.rst
> @@ -0,0 +1,55 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver spd5118
> +=====================
> +
> +Supported chips:
> +
> +  * SPD5118 (JEDEC JESD300) compliant temperature sensor chips
> +
> +    JEDEC standard download:
> +	https://www.jedec.org/standards-documents/docs/jesd300-5b01
> +	(account required)
> +
> +
> +    Prefix: 'spd5118'
> +
> +    Addresses scanned: I2C 0x50 - 0x57
> +
> +Author:
> +	Guenter Roeck <linux@roeck-us.net>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for SPD5118 (JEDEC JESD300) compliant temperature
> +sensors, which are used on many DDR5 memory modules. Some systems use the sensor
> +to prevent memory overheating by automatically throttling the memory controller.
> +
> +The driver auto-detects SPD5118 compliant chips, but can also be instantiated
> +using devicetree/firmware nodes.
> +
> +A SPD5118 compliant chip supports a single temperature sensor. Critical minimum,
> +minimum, maximum, and critical temperature can be configured. There are alarms
> +for low critical, low, high, and critical thresholds.
> +
> +
> +Hardware monitoring sysfs entries
> +---------------------------------
> +
> +======================= ==================================
> +temp1_input		Temperature (RO)
> +temp1_lcrit		Low critical high temperature (RW)
> +temp1_min		Minimum temperature (RW)
> +temp1_max		Maximum temperature (RW)
> +temp1_crit		Critical high temperature (RW)
> +
> +temp1_lcrit_alarm	Temperature low critical alarm
> +temp1_min_alarm		Temperature low alarm
> +temp1_max_alarm		Temperature high alarm
> +temp1_crit_alarm	Temperature critical alarm
> +======================= ==================================
> +
> +Alarm attributes are sticky until read and will be cleared afterwards
> +unless the alarm condition still applies.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e14ae18a973b..d0fb5fe1b2ac 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2181,6 +2181,18 @@ config SENSORS_INA3221
>   	  This driver can also be built as a module. If so, the module
>   	  will be called ina3221.
>
> +config SENSORS_SPD5118
> +	tristate "SPD5118 Compliant Temperature Sensors"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for SPD5118 (JEDEC JESD300)
> +	  compliant temperature sensors. Such sensors are found on DDR5 memory
> +	  modules.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called spd5118.
> +
>   config SENSORS_TC74
>   	tristate "Microchip TC74"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e3f25475d1f0..6574ca67d761 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -207,6 +207,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>   obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>   obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>   obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
> +obj-$(CONFIG_SENSORS_SPD5118)	+= spd5118.o
>   obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
>   obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
>   obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> new file mode 100644
> index 000000000000..d3fc0ae17743
> --- /dev/null
> +++ b/drivers/hwmon/spd5118.c
> @@ -0,0 +1,481 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Jedec 5118 compliant temperature sensors
> + *
> + * Derived from https://github.com/Steve-Tech/SPD5118-DKMS
> + * Originally from T/2 driver at https://t2sde.org/packages/linux
> + *	Copyright (c) 2023 René Rebe, ExactCODE GmbH; Germany.
> + *
> + * Copyright (c) 2024 Guenter Roeck
> + *
> + * Inspired by ee1004.c and jc42.c.
> + *
> + * SPD5118 compliant temperature sensors are typically used on DDR5
> + * memory modules.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = {
> +	0x50, 0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, I2C_CLIENT_END };
> +
> +/* SPD5118 registers. */
> +#define SPD5118_REG_TYPE		0x00	/* MR0:MR1 */
> +#define SPD5118_REG_REVISION		0x02	/* MR2 */
> +#define SPD5118_REG_VENDOR		0x03	/* MR3:MR4 */
> +#define SPD5118_REG_CAPABILITY		0x05	/* MR5 */
> +#define SPD5118_REG_I2C_LEGACY_MODE	0x0B	/* MR11 */
> +#define SPD5118_REG_TEMP_CLR		0x13	/* MR19 */
> +#define SPD5118_REG_ERROR_CLR		0x14	/* MR20 */
> +#define SPD5118_REG_TEMP_CONFIG		0x1A	/* MR26 */
> +#define SPD5118_REG_TEMP_MAX		0x1c	/* MR28:MR29 */
> +#define SPD5118_REG_TEMP_MIN		0x1e	/* MR30:MR31 */
> +#define SPD5118_REG_TEMP_CRIT		0x20	/* MR32:MR33 */
> +#define SPD5118_REG_TEMP_LCRIT		0x22	/* MR34:MR35 */
> +#define SPD5118_REG_TEMP		0x31	/* MR49:MR50 */
> +#define SPD5118_REG_TEMP_STATUS		0x33	/* MR51 */
> +
> +#define SPD5118_TEMP_STATUS_HIGH	BIT(0)
> +#define SPD5118_TEMP_STATUS_LOW		BIT(1)
> +#define SPD5118_TEMP_STATUS_CRIT	BIT(2)
> +#define SPD5118_TEMP_STATUS_LCRIT	BIT(3)
> +
> +#define SPD5118_CAP_TS_SUPPORT		BIT(1)	/* temperature sensor support */
> +
> +#define SPD5118_TS_DISABLE		BIT(0)	/* temperature sensor disable */
> +
> +/* Temperature unit in millicelsius */
> +#define SPD5118_TEMP_UNIT		(MILLIDEGREE_PER_DEGREE / 4)
> +/* Representable temperature range in millicelsius */
> +#define SPD5118_TEMP_RANGE_MIN		-256000
> +#define SPD5118_TEMP_RANGE_MAX		255750
> +
> +static int spd5118_temp_from_reg(u16 reg)
> +{
> +	int temp = sign_extend32(reg >> 2, 10);
> +
> +	return temp * SPD5118_TEMP_UNIT;
> +}
> +
> +static u16 spd5118_temp_to_reg(long temp)
> +{
> +	temp = clamp_val(temp, SPD5118_TEMP_RANGE_MIN, SPD5118_TEMP_RANGE_MAX);
> +	return (DIV_ROUND_CLOSEST(temp, SPD5118_TEMP_UNIT) & 0x7ff) << 2;
> +}
> +
> +static int spd5118_read_temp(struct regmap *regmap, u32 attr, long *val)
> +{
> +	int reg, err;
> +	u8 regval[2];
> +	u16 temp;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = SPD5118_REG_TEMP;
> +		break;
> +	case hwmon_temp_max:
> +		reg = SPD5118_REG_TEMP_MAX;
> +		break;
> +	case hwmon_temp_min:
> +		reg = SPD5118_REG_TEMP_MIN;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = SPD5118_REG_TEMP_CRIT;
> +		break;
> +	case hwmon_temp_lcrit:
> +		reg = SPD5118_REG_TEMP_LCRIT;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = regmap_bulk_read(regmap, reg, regval, 2);
> +	if (err)
> +		return err;
> +
> +	temp = (regval[1] << 8) | regval[0];
> +
> +	*val = spd5118_temp_from_reg(temp);
> +	return 0;
> +}
> +
> +static int spd5118_read_alarm(struct regmap *regmap, u32 attr, long *val)
> +{
> +	unsigned int mask, regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_temp_max_alarm:
> +		mask = SPD5118_TEMP_STATUS_HIGH;
> +		break;
> +	case hwmon_temp_min_alarm:
> +		mask = SPD5118_TEMP_STATUS_LOW;
> +		break;
> +	case hwmon_temp_crit_alarm:
> +		mask = SPD5118_TEMP_STATUS_CRIT;
> +		break;
> +	case hwmon_temp_lcrit_alarm:
> +		mask = SPD5118_TEMP_STATUS_LCRIT;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = regmap_read(regmap, SPD5118_REG_TEMP_STATUS, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = !!(regval & mask);
> +	if (*val)
> +		return regmap_write(regmap, SPD5118_REG_TEMP_CLR, mask);
> +	return 0;
> +}
> +
> +static int spd5118_read_enable(struct regmap *regmap, long *val)
> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = !(regval & SPD5118_TS_DISABLE);
> +	return 0;
> +}
> +
> +static int spd5118_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_temp)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +	case hwmon_temp_max:
> +	case hwmon_temp_min:
> +	case hwmon_temp_crit:
> +	case hwmon_temp_lcrit:
> +		return spd5118_read_temp(regmap, attr, val);
> +	case hwmon_temp_max_alarm:
> +	case hwmon_temp_min_alarm:
> +	case hwmon_temp_crit_alarm:
> +	case hwmon_temp_lcrit_alarm:
> +		return spd5118_read_alarm(regmap, attr, val);
> +	case hwmon_temp_enable:
> +		return spd5118_read_enable(regmap, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int spd5118_write_temp(struct regmap *regmap, u32 attr, long val)
> +{
> +	u8 regval[2];
> +	u16 temp;
> +	int reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		reg = SPD5118_REG_TEMP_MAX;
> +		break;
> +	case hwmon_temp_min:
> +		reg = SPD5118_REG_TEMP_MIN;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = SPD5118_REG_TEMP_CRIT;
> +		break;
> +	case hwmon_temp_lcrit:
> +		reg = SPD5118_REG_TEMP_LCRIT;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	temp = spd5118_temp_to_reg(val);
> +	regval[0] = temp & 0xff;
> +	regval[1] = temp >> 8;
> +
> +	return regmap_bulk_write(regmap, reg, regval, 2);
> +}
> +
> +static int spd5118_write_enable(struct regmap *regmap, long val)
> +{
> +	if (val && val != 1)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
> +				  SPD5118_TS_DISABLE,
> +				  val ? 0 : SPD5118_TS_DISABLE);
> +}
> +
> +static int spd5118_temp_write(struct regmap *regmap, u32 attr, long val)
> +{
> +	switch (attr) {
> +	case hwmon_temp_max:
> +	case hwmon_temp_min:
> +	case hwmon_temp_crit:
> +	case hwmon_temp_lcrit:
> +		return spd5118_write_temp(regmap, attr, val);
> +	case hwmon_temp_enable:
> +		return spd5118_write_enable(regmap, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		return spd5118_temp_write(regmap, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	if (type != hwmon_temp)
> +		return 0;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		return 0444;
> +	case hwmon_temp_min:
> +	case hwmon_temp_max:
> +	case hwmon_temp_lcrit:
> +	case hwmon_temp_crit:
> +	case hwmon_temp_enable:
> +		return 0644;
> +	case hwmon_temp_min_alarm:
> +	case hwmon_temp_max_alarm:
> +	case hwmon_temp_crit_alarm:
> +	case hwmon_temp_lcrit_alarm:
> +		return 0444;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static inline bool spd5118_parity8(u8 w)
> +{
> +	w ^= w >> 4;
> +	return (0x6996 >> (w & 0xf)) & 1;
> +}
> +
> +/*
> + * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
> + * Vendor IDs 0 and 0x7f are invalid.
> + * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs.
> + */
> +static bool spd5118_vendor_valid(u8 bank, u8 id)
> +{
> +	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
> +		return false;
> +
> +	id &= 0x7f;
> +	return id && id != 0x7f;
> +}
> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	int regval;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
> +	if (regval != 0x5118)
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
> +	if (regval < 0 || !spd5118_vendor_valid(regval & 0xff, regval >> 8))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
> +	if (regval < 0)
> +		return -ENODEV;
> +	if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CLR);
> +	if (regval)
> +		return -ENODEV;
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_ERROR_CLR);
> +	if (regval)
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_REVISION);
> +	if (regval < 0 || (regval & 0xc1))
> +		return -ENODEV;
> +
> +	regval = i2c_smbus_read_byte_data(client, SPD5118_REG_TEMP_CONFIG);
> +	if (regval < 0)
> +		return -ENODEV;
> +	if (regval & ~SPD5118_TS_DISABLE)
> +		return -ENODEV;
> +
> +	strscpy(info->type, "spd5118", I2C_NAME_SIZE);
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *spd5118_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT |
> +			   HWMON_T_LCRIT | HWMON_T_LCRIT_ALARM |
> +			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
> +			   HWMON_T_MAX | HWMON_T_MAX_ALARM |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_ENABLE),
> +	NULL
> +};
> +
> +static const struct hwmon_ops spd5118_hwmon_ops = {
> +	.is_visible = spd5118_is_visible,
> +	.read = spd5118_read,
> +	.write = spd5118_write,
> +};
> +
> +static const struct hwmon_chip_info spd5118_chip_info = {
> +	.ops = &spd5118_hwmon_ops,
> +	.info = spd5118_info,
> +};
> +
> +static bool spd5118_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case SPD5118_REG_TEMP_CLR:
> +	case SPD5118_REG_TEMP_CONFIG:
> +	case SPD5118_REG_TEMP_MAX:
> +	case SPD5118_REG_TEMP_MAX + 1:
> +	case SPD5118_REG_TEMP_MIN:
> +	case SPD5118_REG_TEMP_MIN + 1:
> +	case SPD5118_REG_TEMP_CRIT:
> +	case SPD5118_REG_TEMP_CRIT + 1:
> +	case SPD5118_REG_TEMP_LCRIT:
> +	case SPD5118_REG_TEMP_LCRIT + 1:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case SPD5118_REG_TEMP_CLR:
> +	case SPD5118_REG_ERROR_CLR:
> +	case SPD5118_REG_TEMP:
> +	case SPD5118_REG_TEMP + 1:
> +	case SPD5118_REG_TEMP_STATUS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config spd5118_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = SPD5118_REG_TEMP_STATUS,
> +	.writeable_reg = spd5118_writeable_reg,
> +	.volatile_reg = spd5118_volatile_reg,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +static int spd5118_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	unsigned int regval, revision, vendor, bank;
> +	struct device *hwmon_dev;
> +	struct regmap *regmap;
> +	int err;
> +
> +	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
> +
> +	err = regmap_read(regmap, SPD5118_REG_CAPABILITY, &regval);
> +	if (err)
> +		return err;
> +	if (!(regval & SPD5118_CAP_TS_SUPPORT))
> +		return -ENODEV;
> +
> +	err = regmap_read(regmap, SPD5118_REG_REVISION, &revision);
> +	if (err)
> +		return err;
> +
> +	err = regmap_read(regmap, SPD5118_REG_VENDOR, &bank);
> +	if (err)
> +		return err;
> +	err = regmap_read(regmap, SPD5118_REG_VENDOR + 1, &vendor);
> +	if (err)
> +		return err;
> +	if (!spd5118_vendor_valid(bank, vendor))
> +		return -ENODEV;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
> +							 regmap, &spd5118_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	/*
> +	 * From JESD300-5B
> +	 *   MR2 bits [5:4]: Major revision, 1..4
> +	 *   MR2 bits [3:1]: Minor revision, 0..8? Probably a typo, assume 1..8
> +	 */
> +	dev_info(dev, "DDR5 temperature sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
> +		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1, ((revision >> 1) & 0x07) + 1);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id spd5118_id[] = {
> +	{ "spd5118", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, spd5118_id);
> +
> +static const struct of_device_id spd5118_of_ids[] = {
> +	{ .compatible = "jedec,spd5118", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, spd5118_of_ids);
> +
> +static struct i2c_driver spd5118_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "spd5118",
> +		.of_match_table = spd5118_of_ids,
> +	},
> +	.probe		= spd5118_probe,
> +	.id_table	= spd5118_id,
> +	.detect		= spd5118_detect,
> +	.address_list	= normal_i2c,
> +};
> +
> +module_i2c_driver(spd5118_driver);
> +
> +MODULE_AUTHOR("René Rebe <rene@exactcode.de>");
> +MODULE_AUTHOR("Guenter Roeck <linux@roeck-us.net>");
> +MODULE_DESCRIPTION("SPD 5118 driver");
> +MODULE_LICENSE("GPL");

[PATCH RFT v3 3/4] hwmon: (spd5118) Add suspend/resume support

[Guenter Roeck]

Add suspend/resume support to ensure that limit and configuration
registers are updated and synchronized after a suspend/resume cycle.

Cc: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: No change

v2: New patch

RFT: I tested the patch through a suspend/resume cycle, and it seems
     to work, but I am not sure if that had any effect because,
     after all, the memory is still active during suspend/resume.
     I was unable to test a hibernation cycle with my system.

 drivers/hwmon/spd5118.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index d3fc0ae17743..baa315172298 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/hwmon.h>
 #include <linux/module.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/units.h>
 
@@ -432,6 +433,8 @@ static int spd5118_probe(struct i2c_client *client)
 	if (!spd5118_vendor_valid(bank, vendor))
 		return -ENODEV;
 
+	dev_set_drvdata(dev, regmap);
+
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
 							 regmap, &spd5118_chip_info,
 							 NULL);
@@ -449,6 +452,31 @@ static int spd5118_probe(struct i2c_client *client)
 	return 0;
 }
 
+static int spd5118_suspend(struct device *dev)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	regcache_cache_bypass(regmap, true);
+	regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE,
+			   SPD5118_TS_DISABLE);
+	regcache_cache_bypass(regmap, false);
+
+	regcache_cache_only(regmap, true);
+	regcache_mark_dirty(regmap);
+
+	return 0;
+}
+
+static int spd5118_resume(struct device *dev)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	regcache_cache_only(regmap, false);
+	return regcache_sync(regmap);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(spd5118_pm_ops, spd5118_suspend, spd5118_resume);
+
 static const struct i2c_device_id spd5118_id[] = {
 	{ "spd5118", 0 },
 	{ }
@@ -466,6 +494,7 @@ static struct i2c_driver spd5118_driver = {
 	.driver = {
 		.name	= "spd5118",
 		.of_match_table = spd5118_of_ids,
+		.pm = pm_sleep_ptr(&spd5118_pm_ops),
 	},
 	.probe		= spd5118_probe,
 	.id_table	= spd5118_id,
-- 
2.39.2

Re: [PATCH RFT v3 3/4] hwmon: (spd5118) Add suspend/resume support

[Armin Wolf]

Am 01.06.24 um 01:05 schrieb Guenter Roeck:

> Add suspend/resume support to ensure that limit and configuration
> registers are updated and synchronized after a suspend/resume cycle.

Hi,

it seems that 6.10.0-rc1 cannot suspend/resume on my machine. Kernel 6.9.1 however
can successfully enter and leave S3 and S4, so i tested the driver on this kernel
version.

In both cases, the driver was successfully preserving the settings, so:

Tested-by: Armin Wolf <W_Armin@gmx.de>

> Cc: Armin Wolf <W_Armin@gmx.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: No change
>
> v2: New patch
>
> RFT: I tested the patch through a suspend/resume cycle, and it seems
>       to work, but I am not sure if that had any effect because,
>       after all, the memory is still active during suspend/resume.
>       I was unable to test a hibernation cycle with my system.
>
>   drivers/hwmon/spd5118.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index d3fc0ae17743..baa315172298 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -20,6 +20,7 @@
>   #include <linux/i2c.h>
>   #include <linux/hwmon.h>
>   #include <linux/module.h>
> +#include <linux/pm.h>
>   #include <linux/regmap.h>
>   #include <linux/units.h>
>
> @@ -432,6 +433,8 @@ static int spd5118_probe(struct i2c_client *client)
>   	if (!spd5118_vendor_valid(bank, vendor))
>   		return -ENODEV;
>
> +	dev_set_drvdata(dev, regmap);
> +
>   	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
>   							 regmap, &spd5118_chip_info,
>   							 NULL);
> @@ -449,6 +452,31 @@ static int spd5118_probe(struct i2c_client *client)
>   	return 0;
>   }
>
> +static int spd5118_suspend(struct device *dev)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +
> +	regcache_cache_bypass(regmap, true);
> +	regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE,
> +			   SPD5118_TS_DISABLE);
> +	regcache_cache_bypass(regmap, false);
> +
> +	regcache_cache_only(regmap, true);
> +	regcache_mark_dirty(regmap);
> +
> +	return 0;
> +}
> +
> +static int spd5118_resume(struct device *dev)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(regmap, false);
> +	return regcache_sync(regmap);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(spd5118_pm_ops, spd5118_suspend, spd5118_resume);
> +
>   static const struct i2c_device_id spd5118_id[] = {
>   	{ "spd5118", 0 },
>   	{ }
> @@ -466,6 +494,7 @@ static struct i2c_driver spd5118_driver = {
>   	.driver = {
>   		.name	= "spd5118",
>   		.of_match_table = spd5118_of_ids,
> +		.pm = pm_sleep_ptr(&spd5118_pm_ops),
>   	},
>   	.probe		= spd5118_probe,
>   	.id_table	= spd5118_id,

Re: [PATCH RFT v3 3/4] hwmon: (spd5118) Add suspend/resume support

[Stephen Horvath]

Hi,

On 1/6/24 09:05, Guenter Roeck wrote:
> Add suspend/resume support to ensure that limit and configuration
> registers are updated and synchronized after a suspend/resume cycle.
> 
> Cc: Armin Wolf <W_Armin@gmx.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: No change
> 
> v2: New patch
> 
> RFT: I tested the patch through a suspend/resume cycle, and it seems
>       to work, but I am not sure if that had any effect because,
>       after all, the memory is still active during suspend/resume.
>       I was unable to test a hibernation cycle with my system.
> 

For me, the driver just reports 0°C after suspend/resume, but works fine 
beforehand, tested on both my desktop and laptop, with kernel 6.9.3 and 
6.10.0-rc2 (on the laptop only).

Hibernation does seem to work fine though (at least on 6.9.3).

>   drivers/hwmon/spd5118.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index d3fc0ae17743..baa315172298 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -20,6 +20,7 @@
>   #include <linux/i2c.h>
>   #include <linux/hwmon.h>
>   #include <linux/module.h>
> +#include <linux/pm.h>
>   #include <linux/regmap.h>
>   #include <linux/units.h>
>   
> @@ -432,6 +433,8 @@ static int spd5118_probe(struct i2c_client *client)
>   	if (!spd5118_vendor_valid(bank, vendor))
>   		return -ENODEV;
>   
> +	dev_set_drvdata(dev, regmap);
> +
>   	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
>   							 regmap, &spd5118_chip_info,
>   							 NULL);
> @@ -449,6 +452,31 @@ static int spd5118_probe(struct i2c_client *client)
>   	return 0;
>   }
>   
> +static int spd5118_suspend(struct device *dev)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +
> +	regcache_cache_bypass(regmap, true);
> +	regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE,
> +			   SPD5118_TS_DISABLE);
> +	regcache_cache_bypass(regmap, false);
> +
> +	regcache_cache_only(regmap, true);
> +	regcache_mark_dirty(regmap);
> +
> +	return 0;
> +}
> +
> +static int spd5118_resume(struct device *dev)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(regmap, false);

Adding something like this fixes the readings after resume for me:

	regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE, 0);

But that was just the naive solution I thought of.

> +	return regcache_sync(regmap);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(spd5118_pm_ops, spd5118_suspend, spd5118_resume);
> +
>   static const struct i2c_device_id spd5118_id[] = {
>   	{ "spd5118", 0 },
>   	{ }
> @@ -466,6 +494,7 @@ static struct i2c_driver spd5118_driver = {
>   	.driver = {
>   		.name	= "spd5118",
>   		.of_match_table = spd5118_of_ids,
> +		.pm = pm_sleep_ptr(&spd5118_pm_ops),
>   	},
>   	.probe		= spd5118_probe,
>   	.id_table	= spd5118_id,


For reference: I believe both my devices also have Montage Technology 
M88SPD5118 chips; the desktop has Kingston KF560C36-32 RAM, the laptop 
has Crucial CT16G56C46S5.

Thanks,
Steve

Re: [PATCH RFT v3 3/4] hwmon: (spd5118) Add suspend/resume support

[Guenter Roeck]

On 6/3/24 05:31, Stephen Horvath wrote:
> Hi,
> 
> On 1/6/24 09:05, Guenter Roeck wrote:
>> Add suspend/resume support to ensure that limit and configuration
>> registers are updated and synchronized after a suspend/resume cycle.
>>
>> Cc: Armin Wolf <W_Armin@gmx.de>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v3: No change
>>
>> v2: New patch
>>
>> RFT: I tested the patch through a suspend/resume cycle, and it seems
>>       to work, but I am not sure if that had any effect because,
>>       after all, the memory is still active during suspend/resume.
>>       I was unable to test a hibernation cycle with my system.
>>
> 
> For me, the driver just reports 0°C after suspend/resume, but works fine beforehand, tested on both my desktop and laptop, with kernel 6.9.3 and 6.10.0-rc2 (on the laptop only).
> 
> Hibernation does seem to work fine though (at least on 6.9.3).
> 

Thanks a lot for the report. I can confirm that this happens if the enable
attribute was not touched before the suspend/resume cycle. I'll send another
version with the fix.

Guenter

[PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Guenter Roeck]

Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
compliant memory modules. NVRAM write operation is not supported.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: New patch

RFT: I'd like to get some more test coverage before moving forward
     with this patch. decode-dimms doesn't recognize the 'spd5118'
     driver.

 Documentation/hwmon/spd5118.rst |   8 ++
 drivers/hwmon/spd5118.c         | 146 +++++++++++++++++++++++++++++++-
 2 files changed, 150 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/spd5118.rst b/Documentation/hwmon/spd5118.rst
index a15d75aa2066..ef7338f46575 100644
--- a/Documentation/hwmon/spd5118.rst
+++ b/Documentation/hwmon/spd5118.rst
@@ -53,3 +53,11 @@ temp1_crit_alarm	Temperature critical alarm
 
 Alarm attributes are sticky until read and will be cleared afterwards
 unless the alarm condition still applies.
+
+
+SPD (Serial Presence Detect) support
+------------------------------------
+
+The driver also supports reading the SPD NVRAM on SPD5118 compatible chips.
+SPD data is available from the 'eeprom' binary attribute file attached to the
+chip's I2C device.
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index baa315172298..131002290ff3 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -20,6 +20,8 @@
 #include <linux/i2c.h>
 #include <linux/hwmon.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nvmem-provider.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/units.h>
@@ -53,12 +55,31 @@ static const unsigned short normal_i2c[] = {
 
 #define SPD5118_TS_DISABLE		BIT(0)	/* temperature sensor disable */
 
+#define SPD5118_LEGACY_MODE_ADDR	BIT(3)
+#define SPD5118_LEGACY_PAGE_MASK	GENMASK(2, 0)
+#define SPD5118_LEGACY_MODE_MASK	(SPD5118_LEGACY_MODE_ADDR | SPD5118_LEGACY_PAGE_MASK)
+
+
+#define SPD5118_NUM_PAGES		8
+#define SPD5118_PAGE_SIZE		128
+#define SPD5118_PAGE_SHIFT		7
+#define SPD5118_PAGE_MASK		GENMASK(6, 0)
+#define SPD5118_EEPROM_BASE		0x80
+#define SPD5118_EEPROM_SIZE		(SPD5118_PAGE_SIZE * SPD5118_NUM_PAGES)
+
 /* Temperature unit in millicelsius */
 #define SPD5118_TEMP_UNIT		(MILLIDEGREE_PER_DEGREE / 4)
 /* Representable temperature range in millicelsius */
 #define SPD5118_TEMP_RANGE_MIN		-256000
 #define SPD5118_TEMP_RANGE_MAX		255750
 
+struct spd5118_data {
+	struct regmap *regmap;
+	struct mutex nvmem_lock;
+};
+
+/* hwmon */
+
 static int spd5118_temp_from_reg(u16 reg)
 {
 	int temp = sign_extend32(reg >> 2, 10);
@@ -360,9 +381,111 @@ static const struct hwmon_chip_info spd5118_chip_info = {
 	.info = spd5118_info,
 };
 
+/* nvmem */
+
+static int spd5118_nvmem_set_page(struct regmap *regmap, int page)
+{
+	unsigned int old_page;
+	int err;
+
+	err = regmap_read(regmap, SPD5118_REG_I2C_LEGACY_MODE, &old_page);
+	if (err)
+		return err;
+
+	if (page != (old_page & SPD5118_LEGACY_MODE_MASK)) {
+		/* Update page and explicitly select 1-byte addressing */
+		err = regmap_update_bits(regmap, SPD5118_REG_I2C_LEGACY_MODE,
+					 SPD5118_LEGACY_MODE_MASK, page);
+		if (err)
+			return err;
+
+		/* Selected new NVMEM page, drop cached data */
+		regcache_drop_region(regmap, SPD5118_EEPROM_BASE, 0xff);
+	}
+
+	return 0;
+}
+
+static ssize_t spd5118_nvmem_read_page(struct regmap *regmap, char *buf,
+				       unsigned int offset, size_t count)
+{
+	int err;
+
+	err = spd5118_nvmem_set_page(regmap, offset >> SPD5118_PAGE_SHIFT);
+	if (err)
+		return err;
+
+	offset &= SPD5118_PAGE_MASK;
+
+	/* Can't cross page boundaries */
+	if (offset + count > SPD5118_PAGE_SIZE)
+		count = SPD5118_PAGE_SIZE - offset;
+
+	err = regmap_bulk_read(regmap, SPD5118_EEPROM_BASE + offset, buf, count);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static int spd5118_nvmem_read(void *priv, unsigned int off, void *val, size_t count)
+{
+	struct spd5118_data *data = priv;
+	char *buf = val;
+	int ret;
+
+	if (unlikely(!count))
+		return count;
+
+	if (off + count > SPD5118_EEPROM_SIZE)
+		return -EINVAL;
+
+	mutex_lock(&data->nvmem_lock);
+
+	while (count) {
+		ret = spd5118_nvmem_read_page(data->regmap, buf, off, count);
+		if (ret < 0) {
+			mutex_unlock(&data->nvmem_lock);
+			return ret;
+		}
+		buf += ret;
+		off += ret;
+		count -= ret;
+	}
+	mutex_unlock(&data->nvmem_lock);
+	return 0;
+}
+
+static int spd5118_nvmem_init(struct device *dev, struct spd5118_data *data)
+{
+	struct nvmem_config nvmem_config = {
+		.type = NVMEM_TYPE_EEPROM,
+		.name = dev_name(dev),
+		.id = NVMEM_DEVID_AUTO,
+		.dev = dev,
+		.base_dev = dev,
+		.read_only = true,
+		.root_only = false,
+		.owner = THIS_MODULE,
+		.compat = true,
+		.reg_read = spd5118_nvmem_read,
+		.priv = data,
+		.stride = 1,
+		.word_size = 1,
+		.size = SPD5118_EEPROM_SIZE,
+	};
+	struct nvmem_device *nvmem;
+
+	nvmem = devm_nvmem_register(dev, &nvmem_config);
+	return PTR_ERR_OR_ZERO(nvmem);
+}
+
+/* regmap */
+
 static bool spd5118_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
+	case SPD5118_REG_I2C_LEGACY_MODE:
 	case SPD5118_REG_TEMP_CLR:
 	case SPD5118_REG_TEMP_CONFIG:
 	case SPD5118_REG_TEMP_MAX:
@@ -396,7 +519,7 @@ static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
 static const struct regmap_config spd5118_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = SPD5118_REG_TEMP_STATUS,
+	.max_register = 0xff,
 	.writeable_reg = spd5118_writeable_reg,
 	.volatile_reg = spd5118_volatile_reg,
 	.cache_type = REGCACHE_MAPLE,
@@ -406,10 +529,15 @@ static int spd5118_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	unsigned int regval, revision, vendor, bank;
+	struct spd5118_data *data;
 	struct device *hwmon_dev;
 	struct regmap *regmap;
 	int err;
 
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
 	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
@@ -433,7 +561,15 @@ static int spd5118_probe(struct i2c_client *client)
 	if (!spd5118_vendor_valid(bank, vendor))
 		return -ENODEV;
 
-	dev_set_drvdata(dev, regmap);
+	data->regmap = regmap;
+	mutex_init(&data->nvmem_lock);
+	dev_set_drvdata(dev, data);
+
+	err = spd5118_nvmem_init(dev, data);
+	if (err) {
+		dev_err_probe(dev, err, "failed to register nvmem\n");
+		return err;
+	}
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
 							 regmap, &spd5118_chip_info,
@@ -454,7 +590,8 @@ static int spd5118_probe(struct i2c_client *client)
 
 static int spd5118_suspend(struct device *dev)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct spd5118_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 
 	regcache_cache_bypass(regmap, true);
 	regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE,
@@ -469,7 +606,8 @@ static int spd5118_suspend(struct device *dev)
 
 static int spd5118_resume(struct device *dev)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct spd5118_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 
 	regcache_cache_only(regmap, false);
 	return regcache_sync(regmap);
-- 
2.39.2

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Guenter Roeck]

On 5/31/24 16:05, Guenter Roeck wrote:
> Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
> compliant memory modules. NVRAM write operation is not supported.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: New patch
> 
> RFT: I'd like to get some more test coverage before moving forward
>       with this patch. decode-dimms doesn't recognize the 'spd5118'
>       driver.
> 

Looking for feedback:

[ ... ]

> +
> +	nvmem = devm_nvmem_register(dev, &nvmem_config);

This returns ERR_PTR(-EOPNOTSUPP) if CONFIG_NVRAM=n. We have two options:

- Ignore -EOPNOTSUPP and continue registering the hwmon device

or

- Add
	select NVRAM
	select NVRAM_SYSFS
   to the driver's Kconfig entry.

Any preferences ?
	
Thanks,
Guenter

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Thomas Weißschuh]

On 2024-05-31 22:42:24+0000, Guenter Roeck wrote:
> On 5/31/24 16:05, Guenter Roeck wrote:
> > Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
> > compliant memory modules. NVRAM write operation is not supported.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > v3: New patch
> > 
> > RFT: I'd like to get some more test coverage before moving forward
> >       with this patch. decode-dimms doesn't recognize the 'spd5118'
> >       driver.
> > 

Looks good to me.

Spot-checking against JSED400-5B and the embedded CRC are as expected.

> 
> Looking for feedback:
> 
> [ ... ]
> 
> > +
> > +	nvmem = devm_nvmem_register(dev, &nvmem_config);
> 
> This returns ERR_PTR(-EOPNOTSUPP) if CONFIG_NVRAM=n. We have two options:
> 
> - Ignore -EOPNOTSUPP and continue registering the hwmon device
> 
> or
> 
> - Add
> 	select NVRAM
> 	select NVRAM_SYSFS
>   to the driver's Kconfig entry.

s/NVRAM/NVMEM/g

> Any preferences ?

It seems reasonable to support the module without the eeprom logic.
When used in a fixed, embedded environment, the eeprom is of limited
value as it's known beforehand, while the hwmon functionality is still
useful.


EEPROM dump in case anyone wants it:

00000000: 3010 1203 0400 2062 0000 0000 b212 0d00  0..... b........
00000010: 0000 0000 6501 f203 7aaf 0000 0000 c837  ....e...z......7
00000020: c837 c837 906f 80bb 3075 2701 a000 8200  .7.7.o..0u'.....
00000030: 0000 0000 0000 d400 0000 d400 0000 d400  ................
00000040: 0000 d400 0000 8813 0888 1308 204e 2010  ............ N .
00000050: 2710 1534 2010 2710 c409 044c 1d0c 0000  '..4 .'....L....
00000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000070: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000080: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000090: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000000a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000000b0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000000c0: 1000 8632 8015 8a8c 8213 0000 0000 0000  ...2............
000000d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000000e0: 0000 0000 0000 0f11 0171 0822 0000 0000  .........q."....
000000f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000100: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000110: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000120: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000130: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000140: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000150: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000160: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000170: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000180: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000190: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000001a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000001b0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000001c0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000001d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000001e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000001f0: 0000 0000 0000 0000 0000 0000 0000 a14d  ...............M
00000200: 0198 0823 328e 0a9b e84b 4635 3536 5334  ...#2....KF556S4
00000210: 302d 3332 2020 2020 2020 2020 2020 2020  0-32            
00000220: 2020 2020 2020 2000 80ad 4100 0831 3030         ...A..100
00000230: 3139 3738 3700 0000 0000 0000 0000 0000  19787...........
00000240: 0000 4100 0000 0000 0001 0000 0000 0000  ..A.............
00000250: 0100 0000 0000 0000 0000 0000 0000 0000  ................
00000260: 0000 0001 0100 0000 0000 0000 0000 0088  ................
00000270: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000280: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000290: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000002a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000002b0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000002c0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000002d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000002e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000002f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000300: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000310: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000320: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000330: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000340: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000350: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000360: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000370: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000380: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000390: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000003a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000003b0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000003c0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000003d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000003e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
000003f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Guenter Roeck]

On 6/1/24 03:41, Thomas Weißschuh wrote:
> On 2024-05-31 22:42:24+0000, Guenter Roeck wrote:
>> On 5/31/24 16:05, Guenter Roeck wrote:
>>> Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
>>> compliant memory modules. NVRAM write operation is not supported.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> v3: New patch
>>>
>>> RFT: I'd like to get some more test coverage before moving forward
>>>        with this patch. decode-dimms doesn't recognize the 'spd5118'
>>>        driver.
>>>
> 
> Looks good to me.
> 
> Spot-checking against JSED400-5B and the embedded CRC are as expected.
> 
>>
>> Looking for feedback:
>>
>> [ ... ]
>>
>>> +
>>> +	nvmem = devm_nvmem_register(dev, &nvmem_config);
>>
>> This returns ERR_PTR(-EOPNOTSUPP) if CONFIG_NVRAM=n. We have two options:
>>
>> - Ignore -EOPNOTSUPP and continue registering the hwmon device
>>
>> or
>>
>> - Add
>> 	select NVRAM
>> 	select NVRAM_SYSFS
>>    to the driver's Kconfig entry.
> 
> s/NVRAM/NVMEM/g
> 
>> Any preferences ?
> 
> It seems reasonable to support the module without the eeprom logic.
> When used in a fixed, embedded environment, the eeprom is of limited
> value as it's known beforehand, while the hwmon functionality is still
> useful.
> 

Makes sense. Another question:

This:

+        struct nvmem_config nvmem_config = {
+               .type = NVMEM_TYPE_EEPROM,
+               .name = dev_name(dev),
+               .id = NVMEM_DEVID_AUTO,

results in:

$ ls /sys/bus/nvmem/devices
0-00501  0-00512  0-00523  0-00534  cmos_nvram0
^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^

which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
which results in

$ ls /sys/bus/nvmem/devices
0-0050	0-0051	0-0052	0-0053	cmos_nvram0

We could also used fixed strings, but "spd" results in "spd[1-4]" which
I think would be a bit misleading since the DDR3/4 SPD data format is
different, and "spd5118" would result in "spd5118[1-4]" which again would
look odd. Any suggestions ?

Thanks,
Guenter

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Thomas Weißschuh]

On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:

<snip>

> Makes sense. Another question:
> 
> This:
> 
> +        struct nvmem_config nvmem_config = {
> +               .type = NVMEM_TYPE_EEPROM,
> +               .name = dev_name(dev),
> +               .id = NVMEM_DEVID_AUTO,
> 
> results in:
> 
> $ ls /sys/bus/nvmem/devices
> 0-00501  0-00512  0-00523  0-00534  cmos_nvram0
> ^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^
> 
> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
> which results in
> 
> $ ls /sys/bus/nvmem/devices
> 0-0050	0-0051	0-0052	0-0053	cmos_nvram0
> 
> We could also used fixed strings, but "spd" results in "spd[1-4]" which
> I think would be a bit misleading since the DDR3/4 SPD data format is
> different, and "spd5118" would result in "spd5118[1-4]" which again would
> look odd. Any suggestions ?

In order of descending, personal preference:

* spd-ddr5-[0-3] (.id = client->address - 0x50)
* spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
* Same with only "ddr5-"
* spd5118-[0-3]
* Your proposal from above
* nvmem[0-3] (default handling)
* 0-0050-[0-3]

Also can't a user of the eeprom already figure out which kind of module
it is by looking at the eeprom contents?
The first few bytes used for that seem to be compatible between at least
DDR4 and DDR5.

So using plain spd[1-4] could be enough.

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Armin Wolf]

Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:

> On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
>
> <snip>
>
>> Makes sense. Another question:
>>
>> This:
>>
>> +        struct nvmem_config nvmem_config = {
>> +               .type = NVMEM_TYPE_EEPROM,
>> +               .name = dev_name(dev),
>> +               .id = NVMEM_DEVID_AUTO,
>>
>> results in:
>>
>> $ ls /sys/bus/nvmem/devices
>> 0-00501  0-00512  0-00523  0-00534  cmos_nvram0
>> ^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^
>>
>> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
>> which results in
>>
>> $ ls /sys/bus/nvmem/devices
>> 0-0050	0-0051	0-0052	0-0053	cmos_nvram0
>>
>> We could also used fixed strings, but "spd" results in "spd[1-4]" which
>> I think would be a bit misleading since the DDR3/4 SPD data format is
>> different, and "spd5118" would result in "spd5118[1-4]" which again would
>> look odd. Any suggestions ?
> In order of descending, personal preference:
>
> * spd-ddr5-[0-3] (.id = client->address - 0x50)

Hi,

this will break as soon as more than 8 DDR5 DIMMs are installed.

> * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
> * Same with only "ddr5-"
> * spd5118-[0-3]
> * Your proposal from above
> * nvmem[0-3] (default handling)
> * 0-0050-[0-3]
>
> Also can't a user of the eeprom already figure out which kind of module
> it is by looking at the eeprom contents?
> The first few bytes used for that seem to be compatible between at least
> DDR4 and DDR5.
>
> So using plain spd[1-4] could be enough.

This could cause problems when DDR6 arrives.
Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.

Thanks,
Armin Wolf

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Thomas Weißschuh]

On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
> 
> > On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
> > 
> > <snip>
> > 
> > > Makes sense. Another question:
> > > 
> > > This:
> > > 
> > > +        struct nvmem_config nvmem_config = {
> > > +               .type = NVMEM_TYPE_EEPROM,
> > > +               .name = dev_name(dev),
> > > +               .id = NVMEM_DEVID_AUTO,
> > > 
> > > results in:
> > > 
> > > $ ls /sys/bus/nvmem/devices
> > > 0-00501  0-00512  0-00523  0-00534  cmos_nvram0
> > > ^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^
> > > 
> > > which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
> > > which results in
> > > 
> > > $ ls /sys/bus/nvmem/devices
> > > 0-0050	0-0051	0-0052	0-0053	cmos_nvram0
> > > 
> > > We could also used fixed strings, but "spd" results in "spd[1-4]" which
> > > I think would be a bit misleading since the DDR3/4 SPD data format is
> > > different, and "spd5118" would result in "spd5118[1-4]" which again would
> > > look odd. Any suggestions ?
> > In order of descending, personal preference:
> > 
> > * spd-ddr5-[0-3] (.id = client->address - 0x50)
> 
> Hi,
> 
> this will break as soon as more than 8 DDR5 DIMMs are installed.

i2c_register_spd() only handles 8 DIMMs, too.
JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.

Outside of that range we could fall back to something else.

> > * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
> > * Same with only "ddr5-"
> > * spd5118-[0-3]
> > * Your proposal from above
> > * nvmem[0-3] (default handling)
> > * 0-0050-[0-3]
> > 
> > Also can't a user of the eeprom already figure out which kind of module
> > it is by looking at the eeprom contents?
> > The first few bytes used for that seem to be compatible between at least
> > DDR4 and DDR5.
> > 
> > So using plain spd[1-4] could be enough.
> 
> This could cause problems when DDR6 arrives.
> Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.

I have the impression that the eeprom layouts are designed to be
forward and backward compatible.

If a non-DDR5-aware parser reads the contents of a DDR5 eeprom it will
fail the CRC check, so there can be no accidental misinterpretation.
(Because the CRC'ed area is larger and the CRC is at another location)

On the other hand the first bytes of DDR4 and DDR5 are compatible, so
even an unaware parser can recognize that a SPD eeprom is being read and
which DIMM type and specification revision it is.

This seems intentional and therefore should also hold true for DDR5 to DDR6.


Thomas

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Guenter Roeck]

On 6/2/24 00:55, Thomas Weißschuh wrote:
> On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
>> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
>>
>>> On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
>>>
>>> <snip>
>>>
>>>> Makes sense. Another question:
>>>>
>>>> This:
>>>>
>>>> +        struct nvmem_config nvmem_config = {
>>>> +               .type = NVMEM_TYPE_EEPROM,
>>>> +               .name = dev_name(dev),
>>>> +               .id = NVMEM_DEVID_AUTO,
>>>>
>>>> results in:
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-00501  0-00512  0-00523  0-00534  cmos_nvram0
>>>> ^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^
>>>>
>>>> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
>>>> which results in
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-0050	0-0051	0-0052	0-0053	cmos_nvram0
>>>>
>>>> We could also used fixed strings, but "spd" results in "spd[1-4]" which
>>>> I think would be a bit misleading since the DDR3/4 SPD data format is
>>>> different, and "spd5118" would result in "spd5118[1-4]" which again would
>>>> look odd. Any suggestions ?
>>> In order of descending, personal preference:
>>>
>>> * spd-ddr5-[0-3] (.id = client->address - 0x50)
>>
>> Hi,
>>
>> this will break as soon as more than 8 DDR5 DIMMs are installed.
> 
> i2c_register_spd() only handles 8 DIMMs, too.
> JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.
> 
> Outside of that range we could fall back to something else.
> 
>>> * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
>>> * Same with only "ddr5-"
>>> * spd5118-[0-3]
>>> * Your proposal from above
>>> * nvmem[0-3] (default handling)
>>> * 0-0050-[0-3]
>>>
>>> Also can't a user of the eeprom already figure out which kind of module
>>> it is by looking at the eeprom contents?
>>> The first few bytes used for that seem to be compatible between at least
>>> DDR4 and DDR5.
>>>
>>> So using plain spd[1-4] could be enough.
>>
>> This could cause problems when DDR6 arrives.
>> Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.
> 
> I have the impression that the eeprom layouts are designed to be
> forward and backward compatible.
> 
> If a non-DDR5-aware parser reads the contents of a DDR5 eeprom it will
> fail the CRC check, so there can be no accidental misinterpretation.
> (Because the CRC'ed area is larger and the CRC is at another location)
> 
> On the other hand the first bytes of DDR4 and DDR5 are compatible, so
> even an unaware parser can recognize that a SPD eeprom is being read and
> which DIMM type and specification revision it is.
> 
> This seems intentional and therefore should also hold true for DDR5 to DDR6.
>

Looking into how this is handled by other drivers:

- at24 generates directories named {bus}-005{0-7}X, where X is from NVMEM_DEVID_AUTO.
   Alternatively, it uses the 'label' devicetree property. In that case, the name
   will be <label>X, with X again determined by NVMEM_DEVID_AUTO.
   It does that to prevent duplicate file names due to duplicate labels.

- ee1004 does not use the nvmem subsystem, and thus there will be no
   entries in /sys/bus/nvmem/devices/.

NVMEM_DEVID_AUTO counts up from 0, and affects every nvmem device. That means
the assigned ID is not fixed but simply reflects the n-th device using it,
in the order of registration. Effectively this means that any fixed name
plus NVMEM_DEVID_AUTO can not be associated with the originating device,
and there would be no guarantee that it was static (meaning it could change
from boot to boot). spd5118-X would not mean the Xth DIMM, it would be the
Xth device registering with the nvmem subsystem.

At the same time, something like spd5118-X, with X derived only from the
i2c address, would not work large systems because there could be DIMMs on
multiple I2C busses. X would have to derived from the bus number plus
the I2C address, such as '(bus << 3) | (address & 7)'. Even that would
not be static since the bus number could change from boot to boot
depending on the i2c bus instantiation order. It would also require extra
code since the name would have to be generated in the driver.

In the context of at24.c not really caring and ee1004.c not even using
the nvmem subsystem, I think it doesn't really matter how the nvmem device
subdirectories are named as long as they are unique. decode-dimms won't
use it, and I guess no one else really cares because, after all, ee1004
doesn't even support /sys/bus/nvmem/ in the first place and at24 uses the
odd/unusual form of {bus}-005{0-7}X.

Given all that, I'll just stick with the simple dev_name(dev) and
NVMEM_DEVID_NONE. That creates a clear association to i2c bus and address
without requiring extra code or a lengthy explanation how the index is
generated while at the same time guaranteeing uniqueness. I _could_ add
code to use the devicetree label if provided, but that would require using
the same mechanism as used by at24, i.e., we'd end up with {bus}-005{0-7}X
default names (after all, someone could provide duplicate labels or labels
such as "0-0050"). If that is deemed useful it could be added later.

Thanks,
Guenter

Re: [PATCH RFT v3 4/4] hwmon: (spd5118) Add support for reading SPD data

[Guenter Roeck]

On 6/2/24 00:55, Thomas Weißschuh wrote:
> On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
>> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
>>
>>> On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
>>>
>>> <snip>
>>>
>>>> Makes sense. Another question:
>>>>
>>>> This:
>>>>
>>>> +        struct nvmem_config nvmem_config = {
>>>> +               .type = NVMEM_TYPE_EEPROM,
>>>> +               .name = dev_name(dev),
>>>> +               .id = NVMEM_DEVID_AUTO,
>>>>
>>>> results in:
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-00501  0-00512  0-00523  0-00534  cmos_nvram0
>>>> ^^^^^^^  ^^^^^^^  ^^^^^^^  ^^^^^^^
>>>>
>>>> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
>>>> which results in
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-0050	0-0051	0-0052	0-0053	cmos_nvram0
>>>>
>>>> We could also used fixed strings, but "spd" results in "spd[1-4]" which
>>>> I think would be a bit misleading since the DDR3/4 SPD data format is
>>>> different, and "spd5118" would result in "spd5118[1-4]" which again would
>>>> look odd. Any suggestions ?
>>> In order of descending, personal preference:
>>>
>>> * spd-ddr5-[0-3] (.id = client->address - 0x50)
>>
>> Hi,
>>
>> this will break as soon as more than 8 DDR5 DIMMs are installed.
> 
> i2c_register_spd() only handles 8 DIMMs, too.
> JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.
> 

Agreed, but that doesn't mean that there must only be 8 DIMMs in the system.
It only means that there can only be up to 8 DIMMs on a single I2C bus.

i2c_register_spd() only exists if CONFIG_DMI is enabled. While that is supported
for multiple architectures, it is not supported on, for example, powerpc.
DDR5 support is not limited to systems supporting DMI, and/or to systems where
DMI is enabled. The ee1004 driver explicitly supports more than one i2c bus, and
I don't think it would be a good idea to limit DDR5 support to a single bus.

Ultimately that means that we can not rely on i2c_register_spd() for
auto-instantiation, even after adding DDR5 support to it.

Thanks,
Guenter

Re: [PATCH v3 0/4] hwmon: Add support for SPD5118 compliant chips

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

On Fri, May 31, 2024 at 04:05:52PM -0700, Guenter Roeck wrote:

Adding Paul to CC who is likely interested in this series.

> Add support for SPD5118 (Jedec JESD300) compliant chips supporting
> a temperature sensor and SPD NVRAM. Such devices are typically found on
> DDR5 memory modules.
> 
> The first patch of the series adds SPD5118 devicetree bindings. The second
> patch adds support for SPD5118 temperature sensors. The third patch adds
> support for suspend/resume. The last patch adds support for reading the SPD
> NVRAM.
> 
> Note: The driver introduced with this patch series does not currently
> support accessing SPD5118 compliant chips in I3C mode.
> 
> v3: Drop explicit bindings document; add binding to trivial devices instead
>     Add support for reading SPD NVRAM
> 
> v2: Drop PEC support; it only applies to I3C mode.
>     Update documentation
>     Add suspend/resume support 
> 
> ----------------------------------------------------------------
> Guenter Roeck (4):
>       dt-bindings: trivial-devices: Add jedec,spd5118
>       hwmon: Add support for SPD5118 compliant temperature sensors
>       hwmon: (spd5118) Add suspend/resume support
>       hwmon: (spd5118) Add support for reading SPD data
> 
>  .../devicetree/bindings/trivial-devices.yaml       |   2 +
>  Documentation/hwmon/index.rst                      |   1 +
>  Documentation/hwmon/spd5118.rst                    |  56 ++
>  drivers/hwmon/Kconfig                              |  12 +
>  drivers/hwmon/Makefile                             |   1 +
>  drivers/hwmon/spd5118.c                            | 648 +++++++++++++++++++++
>  6 files changed, 720 insertions(+)
>  create mode 100644 Documentation/hwmon/spd5118.rst
>  create mode 100644 drivers/hwmon/spd5118.c

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]