LinuxLists.cc - [PATCH v4 0/6] hwmon: Add support for SPD5118 compliant chips

[permalink] [raw]

Subject: [PATCH v4 1/6] dt-bindings: trivial-devices: Add jedec,spd5118

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

Acked-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v4: Add Krzysztof's Acked-by:

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

2024-06-04 04:03:34

[permalink] [raw]

Subject: [PATCH v4 3/6] hwmon: (spd5118) Add suspend/resume support

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

Cc: Armin Wolf <[email protected]>
Cc: Stephen Horvath <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v4: Fix bug seen if the enable attribute was never read prior
to a suspend/resume cycle.

v3: No change

v2: New patch

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

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index d3fc0ae17743..d55c073ff5fd 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,41 @@ static int spd5118_probe(struct i2c_client *client)
return 0;
}

+static int spd5118_suspend(struct device *dev)
+{
+ struct regmap *regmap = dev_get_drvdata(dev);
+ u32 regval;
+ int err;
+
+ /*
+ * Make sure the configuration register in the regmap cache is current
+ * before bypassing it.
+ */
+ err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
+ if (err < 0)
+ return err;
+
+ 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 +504,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

2024-06-04 04:03:35

[permalink] [raw]

Subject: [PATCH v4 2/6] hwmon: Add support for SPD5118 compliant temperature sensors

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

Cc: René Rebe <[email protected]>
Cc: Thomas Weißschuh <[email protected]>
Reviewed-by: Thomas Weißschuh <[email protected]>
Tested-by: Thomas Weißschuh <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v4: No change

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 <[email protected]>
+
+
+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..7a84e7637b51 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 <[email protected]>");
+MODULE_AUTHOR("Guenter Roeck <[email protected]>");
+MODULE_DESCRIPTION("SPD 5118 driver");
+MODULE_LICENSE("GPL");
--
2.39.2

2024-06-04 04:03:52

[permalink] [raw]

Subject: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

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

NVMEM support is optional. If CONFIG_NVMEM is disabled, the driver will
still instantiate but not provide NVMEM attribute files.

Signed-off-by: Guenter Roeck <[email protected]>
---
v4: Use NVMEM_DEVID_NONE instead of NVMEM_DEVID_AUTO
Ignore nvmem registration failure if nvmem support is disabled

v3: New patch

Documentation/hwmon/spd5118.rst | 8 ++
drivers/hwmon/spd5118.c | 147 +++++++++++++++++++++++++++++++-
2 files changed, 151 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 d55c073ff5fd..5cb5e52c0a38 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_NONE,
+ .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,16 @@ 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);
+ /* Ignore if NVMEM support is disabled */
+ if (err && err != -EOPNOTSUPP) {
+ 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 +591,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;
u32 regval;
int err;

@@ -479,7 +617,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

2024-06-04 04:04:08

[permalink] [raw]

Subject: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

Detect DDR5 memory and instantiate the SPD5118 driver automatically.

Suggested-by: Thomas Weißschuh <[email protected]>
Cc: Wolfram Sang <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v5: New patch

drivers/i2c/i2c-smbus.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 97f338b123b1..8a0dab835761 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -382,6 +382,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
case 0x1E: /* LPDDR4 */
name = "ee1004";
break;
+ case 0x22: /* DDR5 */
+ case 0x23: /* LPDDR5 */
+ name = "spd5118";
+ break;
default:
dev_info(&adap->dev,
"Memory type 0x%02x not supported yet, not instantiating SPD\n",
--
2.39.2

2024-06-04 04:06:38

[permalink] [raw]

Subject: [PATCH v4 6/6] hwmon: (spd5118) Add configuration option for auto-detection

With SPD5118 chip detection for the most part handled by the i2c-smbus
core using DMI information, the spd5118 driver no longer needs to
auto-detect spd5118 compliant chips.

Auto-detection by the driver is still needed on systems with no DMI support
or on systems with more than eight DIMMs and can not be removed entirely.
However, it affects boot time and introduces the risk of mis-identifying
chips. Add configuration option to be able to disable it on systems where
chip detection is handled outside the driver.

Cc: Wolfram Sang <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v4: New patch

drivers/hwmon/Kconfig | 18 ++++++++++++++++++
drivers/hwmon/spd5118.c | 4 +++-
2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7a84e7637b51..0bb1bdee3e43 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2185,6 +2185,7 @@ config SENSORS_SPD5118
tristate "SPD5118 Compliant Temperature Sensors"
depends on I2C
select REGMAP_I2C
+ select SENSORS_SPD5118_DETECT if !DMI
help
If you say yes here you get support for SPD5118 (JEDEC JESD300)
compliant temperature sensors. Such sensors are found on DDR5 memory
@@ -2193,6 +2194,23 @@ config SENSORS_SPD5118
This driver can also be built as a module. If so, the module
will be called spd5118.

+config SENSORS_SPD5118_DETECT
+ bool "Enable detect function"
+ depends on SENSORS_SPD5118
+ default y
+ help
+ If enabled, the driver auto-detects if a chip in the SPD address
+ range is compliant to the SPD51888 standard and auto-instantiates
+ if that is the case. If disabled, SPD5118 compliant devices have
+ to be instantiated by other means. On systems with DMI support
+ this will typically be done from DMI DDR detection code in the
+ I2C SMBus subsystem.
+ Disabling the detect function will speed up boot time and reduce
+ the risk of mis-detecting SPD5118 compliant devices. In general
+ it should only be enabled if necessary.
+
+ If unsure, say Y.
+
config SENSORS_TC74
tristate "Microchip TC74"
depends on I2C
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 5cb5e52c0a38..19d203283a21 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -313,7 +313,7 @@ static bool spd5118_vendor_valid(u8 bank, u8 id)
}

/* Return 0 if detection is successful, -ENODEV otherwise */
-static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+static int __maybe_unused spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
{
struct i2c_adapter *adapter = client->adapter;
int regval;
@@ -647,7 +647,9 @@ static struct i2c_driver spd5118_driver = {
},
.probe = spd5118_probe,
.id_table = spd5118_id,
+#ifdef CONFIG_SENSORS_SPD5118_DETECT
.detect = spd5118_detect,
+#endif
.address_list = normal_i2c,
};

--
2.39.2

2024-06-04 04:39:08

by Thomas Weißschuh

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] hwmon: (spd5118) Add configuration option for auto-detection

On 2024-06-03 21:02:37+0000, Guenter Roeck wrote:
> With SPD5118 chip detection for the most part handled by the i2c-smbus
> core using DMI information, the spd5118 driver no longer needs to
> auto-detect spd5118 compliant chips.
>
> Auto-detection by the driver is still needed on systems with no DMI support
> or on systems with more than eight DIMMs and can not be removed entirely.
> However, it affects boot time and introduces the risk of mis-identifying
> chips. Add configuration option to be able to disable it on systems where
> chip detection is handled outside the driver.
>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v4: New patch
>
> drivers/hwmon/Kconfig | 18 ++++++++++++++++++
> drivers/hwmon/spd5118.c | 4 +++-
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7a84e7637b51..0bb1bdee3e43 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2185,6 +2185,7 @@ config SENSORS_SPD5118
> tristate "SPD5118 Compliant Temperature Sensors"
> depends on I2C
> select REGMAP_I2C
> + select SENSORS_SPD5118_DETECT if !DMI
> help
> If you say yes here you get support for SPD5118 (JEDEC JESD300)
> compliant temperature sensors. Such sensors are found on DDR5 memory
> @@ -2193,6 +2194,23 @@ config SENSORS_SPD5118
> This driver can also be built as a module. If so, the module
> will be called spd5118.
>
> +config SENSORS_SPD5118_DETECT
> + bool "Enable detect function"
> + depends on SENSORS_SPD5118
> + default y
> + help
> + If enabled, the driver auto-detects if a chip in the SPD address
> + range is compliant to the SPD51888 standard and auto-instantiates
> + if that is the case. If disabled, SPD5118 compliant devices have
> + to be instantiated by other means. On systems with DMI support
> + this will typically be done from DMI DDR detection code in the
> + I2C SMBus subsystem.
> + Disabling the detect function will speed up boot time and reduce
> + the risk of mis-detecting SPD5118 compliant devices. In general
> + it should only be enabled if necessary.
> +
> + If unsure, say Y.

The combination of

"In general it should only be enabled if necessary."

and

"default y" / "If unsure, say Y."

looks weird.

Also right now it is not possible to disable detection on non-DMI
configurations. But when using OF, custom kernel code or userspace
instantiation then neither DMI nor CONFIG_DETECT are necessary.

The following would support those usecases, too:

config SENSORS_SPD5118_DETECT
bool "Enable detect function"
depends on SENSORS_SPD5118
default !DMI

(And no "select SENSORS_SPD5118_DETECT if !DMI")

> +
> config SENSORS_TC74
> tristate "Microchip TC74"
> depends on I2C
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 5cb5e52c0a38..19d203283a21 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -313,7 +313,7 @@ static bool spd5118_vendor_valid(u8 bank, u8 id)
> }
>
> /* Return 0 if detection is successful, -ENODEV otherwise */
> -static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> +static int __maybe_unused spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> {
> struct i2c_adapter *adapter = client->adapter;
> int regval;
> @@ -647,7 +647,9 @@ static struct i2c_driver spd5118_driver = {
> },
> .probe = spd5118_probe,
> .id_table = spd5118_id,
> +#ifdef CONFIG_SENSORS_SPD5118_DETECT
> .detect = spd5118_detect,
> +#endif
> .address_list = normal_i2c,

.address_list is also only needed with CONFIG_SENSORS_SPD5118_DETECT.

If you use

.detect = IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? spd5118_detect : NULL,
.address_list = IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? normal_i2c : NULL,

then the need for __maybe_unused goes away and type checking is a tiny
bit better.

2024-06-04 07:41:33

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

On Mon, Jun 03, 2024 at 09:02:36PM -0700, Guenter Roeck wrote:
> Detect DDR5 memory and instantiate the SPD5118 driver automatically.
>
> Suggested-by: Thomas Weißschuh <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>

Acked-by: Wolfram Sang <[email protected]>

Attachments:

(No filename) (369.00 B)
signature.asc (849.00 B)
Download all attachments

2024-06-04 07:45:20

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] hwmon: (spd5118) Add configuration option for auto-detection

> +#ifdef CONFIG_SENSORS_SPD5118_DETECT
> .detect = spd5118_detect,
> +#endif
> .address_list = normal_i2c,
> };

So, I think we have too many Kconfig symbols already. What about using
!DMI here?

Attachments:

(No filename) (212.00 B)
signature.asc (849.00 B)
Download all attachments

2024-06-04 08:50:03

by Stephen Horvath

[permalink] [raw]

Subject: Re: [PATCH v4 2/6] hwmon: Add support for SPD5118 compliant temperature sensors

Hi,

On 4/6/24 14:02, Guenter Roeck wrote:
> Add support for SPD5118 (Jedec JESD300) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.
>
> Cc: René Rebe <[email protected]>
> Cc: Thomas Weißschuh <[email protected]>
> Reviewed-by: Thomas Weißschuh <[email protected]>
> Tested-by: Thomas Weißschuh <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v4: No change
>
> 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
>

It seems to report correct temperatures for my sticks, so I guess:

Tested-by: Stephen Horvath <[email protected]>

Thanks,
Steve

2024-06-04 09:04:34

by Stephen Horvath

[permalink] [raw]

Subject: Re: [PATCH v4 3/6] hwmon: (spd5118) Add suspend/resume support

Hi,

On 4/6/24 14:02, 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 <[email protected]>
> Cc: Stephen Horvath <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v4: Fix bug seen if the enable attribute was never read prior
> to a suspend/resume cycle.

I can confirm this works for my devices, so:

Tested-by: Stephen Horvath <[email protected]>

Thanks,
Steve

2024-06-04 11:59:34

[permalink] [raw]

Subject: Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

Am 04.06.24 um 06:02 schrieb Guenter Roeck:

> Add support for reading SPD NVMEM data from SPD5118 (Jedec JESD300)
> compliant memory modules. NVMEM write operation is not supported.
>
> NVMEM support is optional. If CONFIG_NVMEM is disabled, the driver will
> still instantiate but not provide NVMEM attribute files.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v4: Use NVMEM_DEVID_NONE instead of NVMEM_DEVID_AUTO
> Ignore nvmem registration failure if nvmem support is disabled
>
> v3: New patch
>
> Documentation/hwmon/spd5118.rst | 8 ++
> drivers/hwmon/spd5118.c | 147 +++++++++++++++++++++++++++++++-
> 2 files changed, 151 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 d55c073ff5fd..5cb5e52c0a38 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_NONE,
> + .dev = dev,
> + .base_dev = dev,
> + .read_only = true,
> + .root_only = false,
> + .owner = THIS_MODULE,
> + .compat = true,

Hi,

do we really need this setting here?

> + .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,16 @@ 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);
> + /* Ignore if NVMEM support is disabled */
> + if (err && err != -EOPNOTSUPP) {

Maybe we can use IS_REACHABLE(CONFIG_NVMEM) here?

Thanks,
Armin Wolf

> + 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 +591,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;
> u32 regval;
> int err;
>
> @@ -479,7 +617,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);

2024-06-04 14:04:24

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] hwmon: (spd5118) Add configuration option for auto-detection

On 6/3/24 21:37, Thomas Weißschuh wrote:
> On 2024-06-03 21:02:37+0000, Guenter Roeck wrote:
>> With SPD5118 chip detection for the most part handled by the i2c-smbus
>> core using DMI information, the spd5118 driver no longer needs to
>> auto-detect spd5118 compliant chips.
>>
>> Auto-detection by the driver is still needed on systems with no DMI support
>> or on systems with more than eight DIMMs and can not be removed entirely.
>> However, it affects boot time and introduces the risk of mis-identifying
>> chips. Add configuration option to be able to disable it on systems where
>> chip detection is handled outside the driver.
>>
>> Cc: Wolfram Sang <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> v4: New patch
>>
>> drivers/hwmon/Kconfig | 18 ++++++++++++++++++
>> drivers/hwmon/spd5118.c | 4 +++-
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 7a84e7637b51..0bb1bdee3e43 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2185,6 +2185,7 @@ config SENSORS_SPD5118
>> tristate "SPD5118 Compliant Temperature Sensors"
>> depends on I2C
>> select REGMAP_I2C
>> + select SENSORS_SPD5118_DETECT if !DMI
>> help
>> If you say yes here you get support for SPD5118 (JEDEC JESD300)
>> compliant temperature sensors. Such sensors are found on DDR5 memory
>> @@ -2193,6 +2194,23 @@ config SENSORS_SPD5118
>> This driver can also be built as a module. If so, the module
>> will be called spd5118.
>>
>> +config SENSORS_SPD5118_DETECT
>> + bool "Enable detect function"
>> + depends on SENSORS_SPD5118
>> + default y
>> + help
>> + If enabled, the driver auto-detects if a chip in the SPD address
>> + range is compliant to the SPD51888 standard and auto-instantiates
>> + if that is the case. If disabled, SPD5118 compliant devices have
>> + to be instantiated by other means. On systems with DMI support
>> + this will typically be done from DMI DDR detection code in the
>> + I2C SMBus subsystem.
>> + Disabling the detect function will speed up boot time and reduce
>> + the risk of mis-detecting SPD5118 compliant devices. In general
>> + it should only be enabled if necessary.
>> +
>> + If unsure, say Y.
>
> The combination of
>
> "In general it should only be enabled if necessary."
>
> and
>
> "default y" / "If unsure, say Y."
>
> looks weird.
>

Yes, I know. I just couldn't find a better wording.

>
> Also right now it is not possible to disable detection on non-DMI
> configurations. But when using OF, custom kernel code or userspace
> instantiation then neither DMI nor CONFIG_DETECT are necessary.
>

That is a good point.

> The following would support those usecases, too:
>
> config SENSORS_SPD5118_DETECT
> bool "Enable detect function"
> depends on SENSORS_SPD5118
> default !DMI
>
> (And no "select SENSORS_SPD5118_DETECT if !DMI")
>

I don't think "default !DMI" would be a good idea because DMI
is supported by multiple architectures, but only two X86 specific
controllers call i2c_register_spd().

"default !DMI || !X86" might work, though. I think I'll use that.

I'll rephrase the text and drop the "select".

Thanks,
Guenter

2024-06-04 14:04:45

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] hwmon: (spd5118) Add configuration option for auto-detection

On 6/4/24 00:44, Wolfram Sang wrote:
>> +#ifdef CONFIG_SENSORS_SPD5118_DETECT
>> .detect = spd5118_detect,
>> +#endif
>> .address_list = normal_i2c,
>> };
>
> So, I think we have too many Kconfig symbols already. What about using
> !DMI here?
>

That would be nice, but i2c_register_spd() is only called from i2c-i801.c
and, with the recent patch, from i2c-piix4.c. DMI is also supported on
arm, arm64, loongarch, and mips. i2c_register_spd() will not be called
on those systems or, more generally, if the adapter used to connect to
the DIMMs is not one of those two.

On top of that, it may be desirable to disable it for a system with DMI
support if that system boots from devicetree. I don't know if such a
system exists, but I would not want to bet that it doesn't.

Thanks,
Guenter

2024-06-04 14:52:59

[permalink] [raw]

Subject: Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

On 6/4/24 04:58, Armin Wolf wrote:
> Am 04.06.24 um 06:02 schrieb Guenter Roeck:
>
>> Add support for reading SPD NVMEM data from SPD5118 (Jedec JESD300)
>> compliant memory modules. NVMEM write operation is not supported.
>>
>> NVMEM support is optional. If CONFIG_NVMEM is disabled, the driver will
>> still instantiate but not provide NVMEM attribute files.
>>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> v4: Use NVMEM_DEVID_NONE instead of NVMEM_DEVID_AUTO
>>      Ignore nvmem registration failure if nvmem support is disabled
>>
>> v3: New patch
>>
>> Documentation/hwmon/spd5118.rst |   8 ++
>> drivers/hwmon/spd5118.c         | 147 +++++++++++++++++++++++++++++++-

[ ... ]

>> +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_NONE,
>> +        .dev = dev,
>> +        .base_dev = dev,
>> +        .read_only = true,
>> +        .root_only = false,
>> +        .owner = THIS_MODULE,
>> +        .compat = true,
>
> Hi,
>
> do we really need this setting here?
>

The "eeprom" file is only created if both "base_dev" and "compat" are set.
decode-dimms depends on it. While decode-dimms has to be updated anyway,
I did not want to make that more complicated than necessary.

Another option would be not to use the nvmem subsystem in the first place,
similar to the ee1004 driver, but my understanding is that the use of the
nvmem subsystem is preferred.

[ ... ]

>> +
>> +    err = spd5118_nvmem_init(dev, data);
>> +    /* Ignore if NVMEM support is disabled */
>> +    if (err && err != -EOPNOTSUPP) {
>
> Maybe we can use IS_REACHABLE(CONFIG_NVMEM) here?
>

We could, but I prefer to avoid conditionals in the code if possible,
the dummy devm_nvmem_register() is there specifically to cover that
situation, and no other driver does that. Also, since the underlying
functions are dummy, the compiler should optimize it all away if
CONFIG_NVMEM=n.

Thanks,
Guenter

2024-06-04 14:54:16

[permalink] [raw]

Subject: Re: [PATCH v4 3/6] hwmon: (spd5118) Add suspend/resume support

On 6/4/24 01:45, Stephen Horvath wrote:
> Hi,
>
> On 4/6/24 14:02, 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 <[email protected]>
>> Cc: Stephen Horvath <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> v4: Fix bug seen if the enable attribute was never read prior
>> to a suspend/resume cycle.
>
> I can confirm this works for my devices, so:
>
> Tested-by: Stephen Horvath <[email protected]>
>

Thanks a lot for testing!

Guenter

2024-06-04 14:54:48

[permalink] [raw]

Subject: Re: [PATCH v4 2/6] hwmon: Add support for SPD5118 compliant temperature sensors

On 6/4/24 01:48, Stephen Horvath wrote:
> Hi,
>
> On 4/6/24 14:02, Guenter Roeck wrote:
>> Add support for SPD5118 (Jedec JESD300) compliant temperature
>> sensors. Such sensors are typically found on DDR5 memory modules.
>>
>> Cc: René Rebe <[email protected]>
>> Cc: Thomas Weißschuh <[email protected]>
>> Reviewed-by: Thomas Weißschuh <[email protected]>
>> Tested-by: Thomas Weißschuh <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> v4: No change
>>
>> 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
>>
>
> It seems to report correct temperatures for my sticks, so I guess:
>
> Tested-by: Stephen Horvath <[email protected]>
>

Thanks!

Guenter

2024-06-05 02:19:38

[permalink] [raw]

Subject: [PATCH v4a 6/6] hwmon: (spd5118) Add configuration option for auto-detection

With SPD5118 chip detection for the most part handled by the i2c-smbus
core using DMI information, the spd5118 driver no longer needs to
auto-detect spd5118 compliant chips.

Auto-detection by the driver is still needed on systems with no DMI support
or on systems with more than eight DIMMs and can not be removed entirely.
However, it affects boot time and introduces the risk of mis-identifying
chips. Add configuration option to be able to disable it on systems where
chip detection is handled outside the driver.

Cc: Wolfram Sang <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
Sent as v4a to avoid resending the entire series.

v4a:
Do not auto-select SENSORS_SPD5118_DETECT if DMI is disabled
Modify help text of SENSORS_SPD5118_DETECT
Default SENSORS_SPD5118_DETECT to y if (!DMI || !X86)

v4: New patch

drivers/hwmon/Kconfig | 19 +++++++++++++++++++
drivers/hwmon/spd5118.c | 4 +++-
2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7a84e7637b51..d5eced417fc3 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2193,6 +2193,25 @@ config SENSORS_SPD5118
This driver can also be built as a module. If so, the module
will be called spd5118.

+config SENSORS_SPD5118_DETECT
+ bool "Enable detect function"
+ depends on SENSORS_SPD5118
+ default (!DMI || !X86)
+ help
+ If enabled, the driver auto-detects if a chip in the SPD address
+ range is compliant to the SPD51888 standard and auto-instantiates
+ if that is the case. If disabled, SPD5118 compliant devices have
+ to be instantiated by other means. On X86 systems with DMI support
+ this will typically be done from DMI DDR detection code in the
+ I2C SMBus subsystem. Devicetree based systems will instantiate
+ attached devices if the DIMMs are listed in the devicetree file.
+
+ Disabling the detect function will speed up boot time and reduce
+ the risk of mis-detecting SPD5118 compliant devices. However, it
+ may result in missed DIMMs under some circumstances.
+
+ If unsure, say Y.
+
config SENSORS_TC74
tristate "Microchip TC74"
depends on I2C
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 5cb5e52c0a38..19d203283a21 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -313,7 +313,7 @@ static bool spd5118_vendor_valid(u8 bank, u8 id)
}

/* Return 0 if detection is successful, -ENODEV otherwise */
-static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
+static int __maybe_unused spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
{
struct i2c_adapter *adapter = client->adapter;
int regval;
@@ -647,7 +647,9 @@ static struct i2c_driver spd5118_driver = {
},
.probe = spd5118_probe,
.id_table = spd5118_id,
+#ifdef CONFIG_SENSORS_SPD5118_DETECT
.detect = spd5118_detect,
+#endif
.address_list = normal_i2c,
};

--
2.39.2

2024-06-05 09:24:12

by Thomas Weißschuh

[permalink] [raw]

Subject: Re: [PATCH v4a 6/6] hwmon: (spd5118) Add configuration option for auto-detection

On 2024-06-04 19:19:07+0000, Guenter Roeck wrote:
> With SPD5118 chip detection for the most part handled by the i2c-smbus
> core using DMI information, the spd5118 driver no longer needs to
> auto-detect spd5118 compliant chips.
>
> Auto-detection by the driver is still needed on systems with no DMI support
> or on systems with more than eight DIMMs and can not be removed entirely.
> However, it affects boot time and introduces the risk of mis-identifying
> chips. Add configuration option to be able to disable it on systems where
> chip detection is handled outside the driver.
>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Sent as v4a to avoid resending the entire series.
>
> v4a:
> Do not auto-select SENSORS_SPD5118_DETECT if DMI is disabled
> Modify help text of SENSORS_SPD5118_DETECT
> Default SENSORS_SPD5118_DETECT to y if (!DMI || !X86)
>
> v4: New patch
>
> drivers/hwmon/Kconfig | 19 +++++++++++++++++++
> drivers/hwmon/spd5118.c | 4 +++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7a84e7637b51..d5eced417fc3 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2193,6 +2193,25 @@ config SENSORS_SPD5118
> This driver can also be built as a module. If so, the module
> will be called spd5118.
>
> +config SENSORS_SPD5118_DETECT
> + bool "Enable detect function"
> + depends on SENSORS_SPD5118
> + default (!DMI || !X86)
> + help
> + If enabled, the driver auto-detects if a chip in the SPD address
> + range is compliant to the SPD51888 standard and auto-instantiates
> + if that is the case. If disabled, SPD5118 compliant devices have
> + to be instantiated by other means. On X86 systems with DMI support
> + this will typically be done from DMI DDR detection code in the
> + I2C SMBus subsystem. Devicetree based systems will instantiate
> + attached devices if the DIMMs are listed in the devicetree file.
> +
> + Disabling the detect function will speed up boot time and reduce
> + the risk of mis-detecting SPD5118 compliant devices. However, it
> + may result in missed DIMMs under some circumstances.
> +
> + If unsure, say Y.
> +
> config SENSORS_TC74
> tristate "Microchip TC74"
> depends on I2C
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 5cb5e52c0a38..19d203283a21 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -313,7 +313,7 @@ static bool spd5118_vendor_valid(u8 bank, u8 id)
> }
>
> /* Return 0 if detection is successful, -ENODEV otherwise */
> -static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> +static int __maybe_unused spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> {
> struct i2c_adapter *adapter = client->adapter;
> int regval;
> @@ -647,7 +647,9 @@ static struct i2c_driver spd5118_driver = {
> },
> .probe = spd5118_probe,
> .id_table = spd5118_id,
> +#ifdef CONFIG_SENSORS_SPD5118_DETECT
> .detect = spd5118_detect,
> +#endif
> .address_list = normal_i2c,
> };

For the if-deffery I proposed something during the last review,
I'm not sure if you saw it. If you did, please ignore this comment:

.address_list is also only needed with CONFIG_SENSORS_SPD5118_DETECT.

If you use

.detect = IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? spd5118_detect : NULL,
.address_list = IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? normal_i2c : NULL,

then the need for __maybe_unused goes away and type checking is a tiny
bit better.

2024-06-05 13:19:06

by Paul Menzel

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

Dear Guenter,

Thank you so much for taking this on.

Am 04.06.24 um 06:02 schrieb Guenter Roeck:
> Detect DDR5 memory and instantiate the SPD5118 driver automatically.
>
> Suggested-by: Thomas Weißschuh <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v5: New patch
>
> drivers/i2c/i2c-smbus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 97f338b123b1..8a0dab835761 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -382,6 +382,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
> case 0x1E: /* LPDDR4 */
> name = "ee1004";
> break;
> + case 0x22: /* DDR5 */
> + case 0x23: /* LPDDR5 */
> + name = "spd5118";
> + break;
> default:
> dev_info(&adap->dev,
> "Memory type 0x%02x not supported yet, not instantiating SPD\n",

Testing this on top of 6.10-rc2+ on a Supermicro X13SAE, Linux logs:

$ dmesg | grep -e "DMI:" -e "Linux version" -e i2c-0
[ 0.000000] Linux version
6.10.0-rc2.mx64.461-00036-g151dfab265df
([email protected]) (gcc (GCC) 12.3.0, GNU ld (GNU
Binutils) 2.41) #74 SMP PREEMPT_DYNAMIC Wed Jun 5 08:24:59 CEST 2024
[ 0.000000] DMI: Supermicro Super Server/X13SAE, BIOS 2.0 10/17/2022
[ 0.000000] DMI: Memory slots populated: 4/4
[ 5.434488] i2c i2c-0: Successfully instantiated SPD at 0x50
[ 5.443848] i2c i2c-0: Successfully instantiated SPD at 0x51
[ 5.450033] i2c i2c-0: Successfully instantiated SPD at 0x52
[ 5.459378] i2c i2c-0: Successfully instantiated SPD at 0x53

Then with `sudo modprobe at24` and `sudo modprobe ee1004`,
`decode-dimms` shows:

$ sudo ./eeprom/decode-dimms
# decode-dimms version 4.3

Memory Serial Presence Detect Decoder
By Philip Edelbrock, Christian Zuckschwerdt, Burkart Lingner,
Jean Delvare, Trent Piepho and others

Number of SDRAM DIMMs detected and decoded: 0

This might be expected, and `decode-dimms` also needs to be updated.

Kind regards and thank you again for these patches,

Paul

2024-06-05 13:56:44

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

Hi Paul,

On Wed, Jun 05, 2024 at 02:21:50PM +0200, Paul Menzel wrote:
> Dear Guenter,
>
>
> Thank you so much for taking this on.
>
> Am 04.06.24 um 06:02 schrieb Guenter Roeck:
> > Detect DDR5 memory and instantiate the SPD5118 driver automatically.
> >
> > Suggested-by: Thomas Wei?schuh <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > v5: New patch
> >
> > drivers/i2c/i2c-smbus.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index 97f338b123b1..8a0dab835761 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -382,6 +382,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
> > case 0x1E: /* LPDDR4 */
> > name = "ee1004";
> > break;
> > + case 0x22: /* DDR5 */
> > + case 0x23: /* LPDDR5 */
> > + name = "spd5118";
> > + break;
> > default:
> > dev_info(&adap->dev,
> > "Memory type 0x%02x not supported yet, not instantiating SPD\n",
>
> Testing this on top of 6.10-rc2+ on a Supermicro X13SAE, Linux logs:
>
> $ dmesg | grep -e "DMI:" -e "Linux version" -e i2c-0
> [ 0.000000] Linux version 6.10.0-rc2.mx64.461-00036-g151dfab265df
> ([email protected]) (gcc (GCC) 12.3.0, GNU ld (GNU
> Binutils) 2.41) #74 SMP PREEMPT_DYNAMIC Wed Jun 5 08:24:59 CEST 2024
> [ 0.000000] DMI: Supermicro Super Server/X13SAE, BIOS 2.0 10/17/2022
> [ 0.000000] DMI: Memory slots populated: 4/4
> [ 5.434488] i2c i2c-0: Successfully instantiated SPD at 0x50
> [ 5.443848] i2c i2c-0: Successfully instantiated SPD at 0x51
> [ 5.450033] i2c i2c-0: Successfully instantiated SPD at 0x52
> [ 5.459378] i2c i2c-0: Successfully instantiated SPD at 0x53
>
> Then with `sudo modprobe at24` and `sudo modprobe ee1004`, `decode-dimms`
> shows:
>
You'd actually have to load the spd5118 driver.

> $ sudo ./eeprom/decode-dimms
> # decode-dimms version 4.3
>
> Memory Serial Presence Detect Decoder
> By Philip Edelbrock, Christian Zuckschwerdt, Burkart Lingner,
> Jean Delvare, Trent Piepho and others
>
>
> Number of SDRAM DIMMs detected and decoded: 0
>
> This might be expected, and `decode-dimms` also needs to be updated.
>

Correct. The hack below makes it detect the DIMMs, but the data format
is all different, so it is only really useful to validate reading
the EEPROM (i.e., the checksum over the first 512 bytes of the eeprom
is correct). With that patch applied, I get

Decoding EEPROM: /sys/bus/i2c/drivers/spd5118/0-0050
Guessing DIMM is in bank 1
Kernel driver used spd5118

---=== SPD EEPROM Information ===---
EEPROM CRC of bytes 0-509 48 1 OK (0x47A2)
# of bytes written to SDRAM EEPROM 1024
Total number of bytes in EEPROM 1024
Fundamental Memory type DDR5 SDRAM

---=== Manufacturing Information ===---
Manufacturer Invalid
Custom Manufacturer Data 00 00 00 00 00 88 13 ("???????")
Manufacturing Location Code 0x08
Part Number Undefined
Revision Code 0x4C1D
Manufacturing Date 0x0C00

and so on.

Thanks,
Guenter

---
diff --git a/eeprom/decode-dimms b/eeprom/decode-dimms
index 787b6f5..64b6e81 100755
--- a/eeprom/decode-dimms
+++ b/eeprom/decode-dimms
@@ -2407,7 +2407,12 @@ sub spd_sizes($)
my $bytes = shift;
my $type = $bytes->[2];

- if ($type == 12 || $type == 14 || $type == 16 || $type == 17) {
+ if ($type == 18 || $type == 19 || $type == 20 || $type == 21) {
+ # DDR5
+ my $spd_len = 256 * ((($bytes->[0] >> 4) & 7) + 1);
+ my $used = $spd_len;
+ return ($spd_len, $used);
+ } elsif ($type == 12 || $type == 14 || $type == 16 || $type == 17) {
# DDR4
my $spd_len = 256 * (($bytes->[0] >> 4) & 7);
my $used = 128 * ($bytes->[0] & 15);
@@ -2516,11 +2521,17 @@ sub calculate_crc($$$)
sub check_crc($)
{
my $bytes = shift;
+ my $is_ddr5 = ($bytes->[0] & 0x70) == 0x30;
my $crc_cover = $bytes->[0] & 0x80 ? 116 : 125;
+ my $crc_start = 126;
+ if ($is_ddr5) {
+ $crc_cover = 509;
+ $crc_start = 510;
+ }
my $crc = calculate_crc($bytes, 0, $crc_cover + 1);

- my $dimm_crc = ($bytes->[127] << 8) | $bytes->[126];
- return ("EEPROM CRC of bytes 0-$crc_cover",
+ my $dimm_crc = ($bytes->[$crc_start + 1] << 8) | $bytes->[$crc_start];
+ return ("EEPROM CRC of bytes 0-$crc_cover $bytes->[0] $is_ddr5",
($dimm_crc == $crc) ? 1 : 0,
sprintf("0x%04X", $dimm_crc),
sprintf("0x%04X", $crc));
@@ -2622,6 +2633,7 @@ sub get_dimm_list
if ($use_sysfs) {
@drivers = ('eeprom',
'at24',
+ 'spd5118',
'ee1004'); # DDR4
} else {
@drivers = ('eeprom');
@@ -2640,14 +2652,13 @@ sub get_dimm_list
# We look for I2C devices like 0-0050 or 2-0051
next unless $file =~ /^\d+-[\da-f]+$/i;
next unless -d "$dir/$file";
-
# Device name must be eeprom (driver eeprom)
# spd (driver at24) or ee1004 (driver ee1004)
my $attr = sysfs_device_attribute("$dir/$file", "name");
next unless defined $attr &&
($attr eq "eeprom" ||
$attr eq "spd" ||
- $attr eq "ee1004"); # DDR4
+ $attr eq "spd5118" || $attr eq "ee1004"); # DDR4
} else {
next unless $file =~ /^eeprom-/;
}
@@ -2681,7 +2692,7 @@ sub get_dimm_list
@dimm = get_dimm_list() unless $use_hexdump;

for my $i (0 .. $#dimm) {
- my @bytes = readspd(0, 128, $dimm[$i]->{file});
+ my @bytes = readspd(0, 512, $dimm[$i]->{file});
$dimm[$i]->{bytes} = \@bytes;
$dimm[$i]->{is_rambus} = $bytes[0] < 4; # Simple heuristic
if ($dimm[$i]->{is_rambus} || $bytes[2] < 9) {

2024-06-05 14:47:20

[permalink] [raw]

Subject: Re: [PATCH v4a 6/6] hwmon: (spd5118) Add configuration option for auto-detection

On 6/5/24 02:22, Thomas Weißschuh wrote:
> On 2024-06-04 19:19:07+0000, Guenter Roeck wrote:
>> With SPD5118 chip detection for the most part handled by the i2c-smbus
>> core using DMI information, the spd5118 driver no longer needs to
>> auto-detect spd5118 compliant chips.
>>
>> Auto-detection by the driver is still needed on systems with no DMI support
>> or on systems with more than eight DIMMs and can not be removed entirely.
>> However, it affects boot time and introduces the risk of mis-identifying
>> chips. Add configuration option to be able to disable it on systems where
>> chip detection is handled outside the driver.
>>
>> Cc: Wolfram Sang <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> Sent as v4a to avoid resending the entire series.
>>
>> v4a:
>> Do not auto-select SENSORS_SPD5118_DETECT if DMI is disabled
>> Modify help text of SENSORS_SPD5118_DETECT
>> Default SENSORS_SPD5118_DETECT to y if (!DMI || !X86)
>>
>> v4: New patch
>>
>> drivers/hwmon/Kconfig | 19 +++++++++++++++++++
>> drivers/hwmon/spd5118.c | 4 +++-
>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 7a84e7637b51..d5eced417fc3 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2193,6 +2193,25 @@ config SENSORS_SPD5118
>> This driver can also be built as a module. If so, the module
>> will be called spd5118.
>>
>> +config SENSORS_SPD5118_DETECT
>> + bool "Enable detect function"
>> + depends on SENSORS_SPD5118
>> + default (!DMI || !X86)
>> + help
>> + If enabled, the driver auto-detects if a chip in the SPD address
>> + range is compliant to the SPD51888 standard and auto-instantiates
>> + if that is the case. If disabled, SPD5118 compliant devices have
>> + to be instantiated by other means. On X86 systems with DMI support
>> + this will typically be done from DMI DDR detection code in the
>> + I2C SMBus subsystem. Devicetree based systems will instantiate
>> + attached devices if the DIMMs are listed in the devicetree file.
>> +
>> + Disabling the detect function will speed up boot time and reduce
>> + the risk of mis-detecting SPD5118 compliant devices. However, it
>> + may result in missed DIMMs under some circumstances.
>> +
>> + If unsure, say Y.
>> +
>> config SENSORS_TC74
>> tristate "Microchip TC74"
>> depends on I2C
>> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
>> index 5cb5e52c0a38..19d203283a21 100644
>> --- a/drivers/hwmon/spd5118.c
>> +++ b/drivers/hwmon/spd5118.c
>> @@ -313,7 +313,7 @@ static bool spd5118_vendor_valid(u8 bank, u8 id)
>> }
>>
>> /* Return 0 if detection is successful, -ENODEV otherwise */
>> -static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
>> +static int __maybe_unused spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
>> {
>> struct i2c_adapter *adapter = client->adapter;
>> int regval;
>> @@ -647,7 +647,9 @@ static struct i2c_driver spd5118_driver = {
>> },
>> .probe = spd5118_probe,
>> .id_table = spd5118_id,
>> +#ifdef CONFIG_SENSORS_SPD5118_DETECT
>> .detect = spd5118_detect,
>> +#endif
>> .address_list = normal_i2c,
>> };
>
> For the if-deffery I proposed something during the last review,
> I'm not sure if you saw it. If you did, please ignore this comment:
>

I missed that, sorry (and I hope I didn't miss anything else).

>
> .address_list is also only needed with CONFIG_SENSORS_SPD5118_DETECT.
>
> If you use
>
> .detect = IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? spd5118_detect : NULL,
> .address_list = IS_ENABLED(CONFIG_SENSORS_SPD5118_DETECT) ? normal_i2c : NULL,
>
> then the need for __maybe_unused goes away and type checking is a tiny
> bit better.

It does let me drop the #ifdef, so I agree it is a bit better.
I made that change, but I'll wait for a couple of days before sending
the updated version to give others time for additional feedback.

Thanks,
Guenter

2024-06-07 15:56:19

[permalink] [raw]

Subject: Re: [PATCH v4 2/6] hwmon: Add support for SPD5118 compliant temperature sensors

Am 04.06.24 um 06:02 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 <[email protected]>

> Cc: René Rebe <[email protected]>
> Cc: Thomas Weißschuh <[email protected]>
> Reviewed-by: Thomas Weißschuh <[email protected]>
> Tested-by: Thomas Weißschuh <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v4: No change
>
> 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 <[email protected]>
> +
> +
> +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..7a84e7637b51 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 <[email protected]>");
> +MODULE_AUTHOR("Guenter Roeck <[email protected]>");
> +MODULE_DESCRIPTION("SPD 5118 driver");
> +MODULE_LICENSE("GPL");

2024-06-07 15:58:50

[permalink] [raw]

Subject: Re: [PATCH v4 3/6] hwmon: (spd5118) Add suspend/resume support

Am 04.06.24 um 06:02 schrieb Guenter Roeck:

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

Tested-by: Armin Wolf <[email protected]>

>
> Cc: Armin Wolf <[email protected]>
> Cc: Stephen Horvath <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v4: Fix bug seen if the enable attribute was never read prior
> to a suspend/resume cycle.
>
> v3: No change
>
> v2: New patch
>
> drivers/hwmon/spd5118.c | 39 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index d3fc0ae17743..d55c073ff5fd 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,41 @@ static int spd5118_probe(struct i2c_client *client)
> return 0;
> }
>
> +static int spd5118_suspend(struct device *dev)
> +{
> + struct regmap *regmap = dev_get_drvdata(dev);
> + u32 regval;
> + int err;
> +
> + /*
> + * Make sure the configuration register in the regmap cache is current
> + * before bypassing it.
> + */
> + err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
> + if (err < 0)
> + return err;
> +
> + 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 +504,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,

2024-06-07 16:05:58

[permalink] [raw]

Subject: Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

Am 04.06.24 um 16:30 schrieb Guenter Roeck:

> On 6/4/24 04:58, Armin Wolf wrote:
>> Am 04.06.24 um 06:02 schrieb Guenter Roeck:
>>
>>> Add support for reading SPD NVMEM data from SPD5118 (Jedec JESD300)
>>> compliant memory modules. NVMEM write operation is not supported.
>>>
>>> NVMEM support is optional. If CONFIG_NVMEM is disabled, the driver will
>>> still instantiate but not provide NVMEM attribute files.
>>>
>>> Signed-off-by: Guenter Roeck <[email protected]>
>>> ---
>>> v4: Use NVMEM_DEVID_NONE instead of NVMEM_DEVID_AUTO
>>>      Ignore nvmem registration failure if nvmem support is disabled
>>>
>>> v3: New patch
>>>
>>> Documentation/hwmon/spd5118.rst |   8 ++
>>> drivers/hwmon/spd5118.c         | 147
>>> +++++++++++++++++++++++++++++++-
>
>
> [ ... ]
>
>>> +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_NONE,
>>> +        .dev = dev,
>>> +        .base_dev = dev,
>>> +        .read_only = true,
>>> +        .root_only = false,
>>> +        .owner = THIS_MODULE,
>>> +        .compat = true,
>>
>> Hi,
>>
>> do we really need this setting here?
>>
>
> The "eeprom" file is only created if both "base_dev" and "compat" are
> set.
> decode-dimms depends on it. While decode-dimms has to be updated anyway,
> I did not want to make that more complicated than necessary.
>
I understand.

> Another option would be not to use the nvmem subsystem in the first
> place,
> similar to the ee1004 driver, but my understanding is that the use of the
> nvmem subsystem is preferred.
>
> [ ... ]
>
>>> +
>>> +    err = spd5118_nvmem_init(dev, data);
>>> +    /* Ignore if NVMEM support is disabled */
>>> +    if (err && err != -EOPNOTSUPP) {
>>
>> Maybe we can use IS_REACHABLE(CONFIG_NVMEM) here?
>>
>
> We could, but I prefer to avoid conditionals in the code if possible,
> the dummy devm_nvmem_register() is there specifically to cover that
> situation, and no other driver does that. Also, since the underlying
> functions are dummy, the compiler should optimize it all away if
> CONFIG_NVMEM=n.
>
> Thanks,
> Guenter
>
Ok, then i am ok with with this patch.

Tested-by: Armin Wolf <[email protected]>

2024-06-07 16:18:15

[permalink] [raw]

Subject: Re: [PATCH v4a 6/6] hwmon: (spd5118) Add configuration option for auto-detection

Am 05.06.24 um 04:19 schrieb Guenter Roeck:

> With SPD5118 chip detection for the most part handled by the i2c-smbus
> core using DMI information, the spd5118 driver no longer needs to
> auto-detect spd5118 compliant chips.
>
> Auto-detection by the driver is still needed on systems with no DMI support
> or on systems with more than eight DIMMs and can not be removed entirely.
> However, it affects boot time and introduces the risk of mis-identifying
> chips. Add configuration option to be able to disable it on systems where
> chip detection is handled outside the driver.

Tested-by: Armin Wolf <[email protected]>

> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Sent as v4a to avoid resending the entire series.
>
> v4a:
> Do not auto-select SENSORS_SPD5118_DETECT if DMI is disabled
> Modify help text of SENSORS_SPD5118_DETECT
> Default SENSORS_SPD5118_DETECT to y if (!DMI || !X86)
>
> v4: New patch
>
> drivers/hwmon/Kconfig | 19 +++++++++++++++++++
> drivers/hwmon/spd5118.c | 4 +++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7a84e7637b51..d5eced417fc3 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2193,6 +2193,25 @@ config SENSORS_SPD5118
> This driver can also be built as a module. If so, the module
> will be called spd5118.
>
> +config SENSORS_SPD5118_DETECT
> + bool "Enable detect function"
> + depends on SENSORS_SPD5118
> + default (!DMI || !X86)
> + help
> + If enabled, the driver auto-detects if a chip in the SPD address
> + range is compliant to the SPD51888 standard and auto-instantiates
> + if that is the case. If disabled, SPD5118 compliant devices have
> + to be instantiated by other means. On X86 systems with DMI support
> + this will typically be done from DMI DDR detection code in the
> + I2C SMBus subsystem. Devicetree based systems will instantiate
> + attached devices if the DIMMs are listed in the devicetree file.
> +
> + Disabling the detect function will speed up boot time and reduce
> + the risk of mis-detecting SPD5118 compliant devices. However, it
> + may result in missed DIMMs under some circumstances.
> +
> + If unsure, say Y.
> +
> config SENSORS_TC74
> tristate "Microchip TC74"
> depends on I2C
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 5cb5e52c0a38..19d203283a21 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -313,7 +313,7 @@ static bool spd5118_vendor_valid(u8 bank, u8 id)
> }
>
> /* Return 0 if detection is successful, -ENODEV otherwise */
> -static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> +static int __maybe_unused spd5118_detect(struct i2c_client *client, struct i2c_board_info *info)
> {
> struct i2c_adapter *adapter = client->adapter;
> int regval;
> @@ -647,7 +647,9 @@ static struct i2c_driver spd5118_driver = {
> },
> .probe = spd5118_probe,
> .id_table = spd5118_id,
> +#ifdef CONFIG_SENSORS_SPD5118_DETECT
> .detect = spd5118_detect,
> +#endif
> .address_list = normal_i2c,
> };
>

2024-06-07 16:18:47

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

Am 04.06.24 um 06:02 schrieb Guenter Roeck:

> Detect DDR5 memory and instantiate the SPD5118 driver automatically.

Hi,

the text "Only works for DDR, DDR2, DDR3 and DDR4 for now" should be updated too.
With this fixed:

Reviewed-by: Armin Wolf <[email protected]>

>
> Suggested-by: Thomas Weißschuh <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v5: New patch
>
> drivers/i2c/i2c-smbus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 97f338b123b1..8a0dab835761 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -382,6 +382,10 @@ void i2c_register_spd(struct i2c_adapter *adap)
> case 0x1E: /* LPDDR4 */
> name = "ee1004";
> break;
> + case 0x22: /* DDR5 */
> + case 0x23: /* LPDDR5 */
> + name = "spd5118";
> + break;
> default:
> dev_info(&adap->dev,
> "Memory type 0x%02x not supported yet, not instantiating SPD\n",

2024-06-07 18:00:40

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

> the text "Only works for DDR, DDR2, DDR3 and DDR4 for now" should be updated too.
> With this fixed:

Yes, maybe this could be simplified to "(LP)DDR memory types"

Attachments:

(No filename) (174.00 B)
signature.asc (849.00 B)
Download all attachments

2024-06-10 13:59:02

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

On 6/7/24 11:00, Wolfram Sang wrote:
>
>> the text "Only works for DDR, DDR2, DDR3 and DDR4 for now" should be updated too.
>> With this fixed:
>
> Yes, maybe this could be simplified to "(LP)DDR memory types"
>

I rephrased it to "Only works for (LP)DDR memory types up to DDR5".

How about "Only works on systems with 1 to 8 memory slots" ?
That isn't really correct anymore either, unless I misunderstand
the changes made in commit 8821c8376993 ("i2c: smbus: Prepare
i2c_register_spd for usage on muxed segments").

Thanks,
Guenter

2024-06-10 14:52:24

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

> > Yes, maybe this could be simplified to "(LP)DDR memory types"
> >
>
> I rephrased it to "Only works for (LP)DDR memory types up to DDR5".

Thanks!

> How about "Only works on systems with 1 to 8 memory slots" ?

This is a question for Heiner. I'd think it is is still correct, but I
don't know exactly.

Attachments:

(No filename) (324.00 B)
signature.asc (849.00 B)
Download all attachments

2024-06-10 16:28:16

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 SPD EEPROMs

On 6/10/24 07:52, Wolfram Sang wrote:
>
>>> Yes, maybe this could be simplified to "(LP)DDR memory types"
>>>
>>
>> I rephrased it to "Only works for (LP)DDR memory types up to DDR5".
>
> Thanks!
>
>> How about "Only works on systems with 1 to 8 memory slots" ?
>
> This is a question for Heiner. I'd think it is is still correct, but I
> don't know exactly.
>

My interpretation was that it should work if the DIMMs are connected to
multiplexed I2C busses, but probably not if they are connected to
different adapters. The error message in that case is a bit misleading,
though, because it claims that "More than 8 memory slots on a single bus",
which isn't necessarily the case. For example, it should be perfectly valid
to have up to 24 DIMMs in this system.

i2c-0/name:SMBus PIIX4 adapter port 0 at 0b00
i2c-1/name:SMBus PIIX4 adapter port 2 at 0b00
i2c-2/name:SMBus PIIX4 adapter port 1 at 0b20

... but I guess that is a question for someone with such a system to answer.

Ultimately the handling of systems with more than 8 memory slots will need
to be updated at some point. On my systems, with 'i2c: piix4: Register SPDs'
applied, I see

i2c i2c-0: 4/4 memory slots populated (from DMI)
[my system is running 6.6.y which still generates that message]
i2c i2c-0: Successfully instantiated SPD at 0x50
i2c i2c-0: Successfully instantiated SPD at 0x51
i2c i2c-0: Successfully instantiated SPD at 0x52
i2c i2c-0: Successfully instantiated SPD at 0x53
i2c i2c-1: 4/4 memory slots populated (from DMI)
i2c i2c-2: 4/4 memory slots populated (from DMI)

meaning the function is called for each adapter (which makes sense).
However, the code counting the DIMMs doesn't really take the adapter
into account, meaning adapters 1 and 2 are still probed even though
all DIMMs were already instantiated from adapter 0.

On a system with more than 8 DIMMs connected to different piix4 adapters
(without mux) we'd probably see something like

i2c i2c-0: More than 8 memory slots on a single bus, contact i801 maintainer ...
i2c i2c-1: More than 8 memory slots on a single bus, contact i801 maintainer ...
i2c i2c-2: More than 8 memory slots on a single bus, contact i801 maintainer ...

which wouldn't be very helpful. I think the main problem may be that
the i801 driver implements sub-adapters as muxes, but the piix4 driver
doesn't do (or need) that. The message is also i801 centric which doesn't
apply anymore after 'i2c: piix4: Register SPDs' is applied.

However, I would not want to even try changing that code without access
to a system using piix4 and supporting more than 8 memory slots.

Thanks,
Guenter

2024-06-12 16:28:15