Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
sensors. Such sensors are typically found on DDR5 memory modules.
The first patch of the series adds SPD5118 devicetree bindings. The second
patch adds support for SPD5118 temperature sensors. The third patch adds
support for configuring PEC (Packet Error Checking). This patch depends
on [1] and is untested.
Note: The driver introduced with this patch series does not currently
support accessing the SPD5118 EEPROM, or accessing SPD5118 compatible chips
in I3C mode.
[1] https://patchwork.kernel.org/project/linux-hwmon/list/?series=857097
----------------------------------------------------------------
Guenter Roeck (3):
dt-bindings: hwmon: jedec,spd5118: Add bindings
hwmon: Add support for SPD5118 compliant temperature sensors
hwmon: (spd5118) Add PEC support
.../devicetree/bindings/hwmon/jedec,spd5118.yaml | 52 +++
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/spd5118.rst | 60 +++
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/spd5118.c | 499 +++++++++++++++++++++
6 files changed, 625 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/jedec,spd5118.yaml
create mode 100644 Documentation/hwmon/spd5118.rst
create mode 100644 drivers/hwmon/spd5118.c
Add device tree bindings for the SPD hub present in DDR5 modules.
Signed-off-by: Guenter Roeck <[email protected]>
---
I am not sure about the "pec-enable" property since it is not really
a chip property but a SMBus property. It is also not chip specific;
there are other chips with the capability to enable or disable PEC
support. There are no generic SMBus bindings, though, so I don't know
a better place (or name - maybe it should be 'smbus,pec-enable', but
there are no other similar bindings, or at least I didn't find any).
.../bindings/hwmon/jedec,spd5118.yaml | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/jedec,spd5118.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/jedec,spd5118.yaml b/Documentation/devicetree/bindings/hwmon/jedec,spd5118.yaml
new file mode 100644
index 000000000000..1717f75129da
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/jedec,spd5118.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/jedec,spd5118.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: JEDEC JESD300-5B (SPD5118) compatible DDR5 SPD hub
+
+maintainers:
+ - Guenter Roeck <[email protected]>
+
+description: |
+ JEDEC JESD300-5B.01 SPD5118 Hub and Serial Presence Detect
+ https://www.jedec.org/standards-documents/docs/jesd300-5b01
+
+select:
+ properties:
+ compatible:
+ const: jedec,spd5118
+
+ required:
+ - compatible
+ - reg
+
+properties:
+ compatible:
+ const: jedec,spd5118
+
+ reg:
+ maxItems: 1
+
+ pec-enable:
+ description: Set to enable PEC (Packet Error Checking).
+ type: boolean
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ temp-sensor@51 {
+ compatible = "jedec,spd5118";
+ reg = <0x51>;
+ };
+ };
--
2.39.2
Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
sensors. Such sensors are typically found on DDR5 memory modules.
Cc: René Rebe <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
(Corsair Venegance DDR5).
René: I included you as MODULE_AUTHOR since the patch is derived from
your driver. Please let me know if you prefer not to be listed as
author.
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/spd5118.rst | 60 ++++
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
5 files changed, 556 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..67e990551a8a
--- /dev/null
+++ b/Documentation/hwmon/spd5118.rst
@@ -0,0 +1,60 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver spd5118
+=====================
+
+Supported chips:
+
+ * SPD5118 (JEDEC JESD300-5B.01) 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-5B.01) 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.
+
+
+PEC configuration
+-----------------
+
+If the I2C/SMBus controller supports PEC, it can be enabled or disabled using
+the 'pec' sysfs attribute attached to the i2c device.
+
+
+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
+======================= ===========================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7f384a2494c9..111d05718b89 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2182,6 +2182,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-5B)
+ 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..07c593fae9a3 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_SPD51118) += 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..440503d09d13
--- /dev/null
+++ b/drivers/hwmon/spd5118.c
@@ -0,0 +1,482 @@
+// 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, ®val);
+ 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, u32 attr, long *val)
+{
+ u32 regval;
+ int err;
+
+ err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, ®val);
+ 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, attr, 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, u32 attr, 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, attr, 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;
+
+ 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;
+
+ if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
+ 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, ®val);
+ 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
Add support for configuring PEC (Packet Error Checking).
Signed-off-by: Guenter Roeck <[email protected]>
---
This patch depends on the patch series at
https://patchwork.kernel.org/project/linux-hwmon/list/?series=857097
drivers/hwmon/spd5118.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 440503d09d13..eb21bf3dffd7 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -33,6 +33,7 @@ static const unsigned short normal_i2c[] = {
#define SPD5118_REG_VENDOR 0x03 /* MR3:MR4 */
#define SPD5118_REG_CAPABILITY 0x05 /* MR5 */
#define SPD5118_REG_I2C_LEGACY_MODE 0x0B /* MR11 */
+#define SPD5118_REG_DEV_CONFIG 0x12 /* MR18 */
#define SPD5118_REG_TEMP_CLR 0x13 /* MR19 */
#define SPD5118_REG_ERROR_CLR 0x14 /* MR20 */
#define SPD5118_REG_TEMP_CONFIG 0x1A /* MR26 */
@@ -50,6 +51,8 @@ static const unsigned short normal_i2c[] = {
#define SPD5118_CAP_TS_SUPPORT BIT(1) /* temperature sensor support */
+#define SPD5118_PEC_ENABLE BIT(7) /* PEC enable */
+
#define SPD5118_TS_DISABLE BIT(0) /* temperature sensor disable */
/* Temperature unit in millicelsius */
@@ -217,6 +220,18 @@ static int spd5118_write_enable(struct regmap *regmap, u32 attr, long val)
val ? 0 : SPD5118_TS_DISABLE);
}
+static int spd5118_chip_write(struct regmap *regmap, u32 attr, long val)
+{
+ switch (attr) {
+ case hwmon_chip_pec:
+ return regmap_update_bits(regmap, SPD5118_REG_DEV_CONFIG,
+ SPD5118_PEC_ENABLE,
+ val ? SPD5118_PEC_ENABLE : 0);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static int spd5118_temp_write(struct regmap *regmap, u32 attr, long val)
{
switch (attr) {
@@ -238,6 +253,8 @@ static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
struct regmap *regmap = dev_get_drvdata(dev);
switch (type) {
+ case hwmon_chip:
+ return spd5118_chip_write(regmap, attr, val);
case hwmon_temp:
return spd5118_temp_write(regmap, attr, val);
default:
@@ -338,7 +355,7 @@ static int spd5118_detect(struct i2c_client *client, struct i2c_board_info *info
static const struct hwmon_channel_info *spd5118_info[] = {
HWMON_CHANNEL_INFO(chip,
- HWMON_C_REGISTER_TZ),
+ HWMON_C_REGISTER_TZ | HWMON_C_PEC),
HWMON_CHANNEL_INFO(temp,
HWMON_T_INPUT |
HWMON_T_LCRIT | HWMON_T_LCRIT_ALARM |
--
2.39.2
Am 29.05.24 um 22:52 schrieb Guenter Roeck:
> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.
>
> Cc: René Rebe <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
> (Corsair Venegance DDR5).
>
> René: I included you as MODULE_AUTHOR since the patch is derived from
> your driver. Please let me know if you prefer not to be listed as
> author.
>
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/spd5118.rst | 60 ++++
> drivers/hwmon/Kconfig | 12 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
> 5 files changed, 556 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..67e990551a8a
> --- /dev/null
> +++ b/Documentation/hwmon/spd5118.rst
> @@ -0,0 +1,60 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver spd5118
> +=====================
> +
> +Supported chips:
> +
> + * SPD5118 (JEDEC JESD300-5B.01) 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-5B.01) 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.
> +
> +
> +PEC configuration
> +-----------------
> +
> +If the I2C/SMBus controller supports PEC, it can be enabled or disabled using
> +the 'pec' sysfs attribute attached to the i2c device.
> +
> +
> +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
> +======================= ===========================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7f384a2494c9..111d05718b89 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2182,6 +2182,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-5B)
> + 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..07c593fae9a3 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_SPD51118) += spd5118.o
Hi,
thank you for working on this, i am currently testing the driver on my machine.
I already noticed the kconfig option is wrong, the correct one would be CONFIG_SENSORS_SPD5118.
Thanks,
Armin Wolf
> 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..440503d09d13
> --- /dev/null
> +++ b/drivers/hwmon/spd5118.c
> @@ -0,0 +1,482 @@
> +// 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, ®val);
> + 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, u32 attr, long *val)
> +{
> + u32 regval;
> + int err;
> +
> + err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, ®val);
> + 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, attr, 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, u32 attr, 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, attr, 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;
> +
> + 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;
> +
> + if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
> + 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, ®val);
> + 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");
On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.
>
> Cc: René Rebe <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
<snip>
> +/* 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;
> +
> + 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;
> +
> + if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
> + return -ENODEV;
This breaks automatic detection for me.
I think the test should after the read of SPD5118_REG_CAPABILITY and
test that register, similar on how it is done in _probe().
> +
> + 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;
> +}
<snip>
On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.
>
> Cc: René Rebe <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
> (Corsair Venegance DDR5).
>
> René: I included you as MODULE_AUTHOR since the patch is derived from
> your driver. Please let me know if you prefer not to be listed as
> author.
>
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/spd5118.rst | 60 ++++
> drivers/hwmon/Kconfig | 12 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
> 5 files changed, 556 insertions(+)
> create mode 100644 Documentation/hwmon/spd5118.rst
> create mode 100644 drivers/hwmon/spd5118.c
With the Makefile and detect callback fixed:
Reviewed-by: Thomas Weißschuh <[email protected]>
Tested-by: Thomas Weißschuh <[email protected]>
On 5/30/24 01:08, Armin Wolf wrote:
[ ... ]
>> +obj-$(CONFIG_SENSORS_SPD51118) += spd5118.o
>
> Hi,
>
> thank you for working on this, i am currently testing the driver on my machine.
> I already noticed the kconfig option is wrong, the correct one would be CONFIG_SENSORS_SPD5118.
>
Oops. Thanks for noticing!
Guenter
On 5/30/24 02:08, Thomas Weißschuh wrote:
> On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
>> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
>> sensors. Such sensors are typically found on DDR5 memory modules.
>>
>> Cc: René Rebe <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>
> <snip>
>
>> +/* 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;
>> +
>> + 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;
>> +
>> + if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
>> + return -ENODEV;
>
> This breaks automatic detection for me.
>
> I think the test should after the read of SPD5118_REG_CAPABILITY and
> test that register, similar on how it is done in _probe().
>
Yes, that got messed up when I added reading SPD5118_REG_TEMP_CLR and
SPD5118_REG_ERROR_CLR in the last minute. Thanks!
Thanks,
Guenter
On 5/30/24 06:23, Guenter Roeck wrote:
> On 5/30/24 01:08, Armin Wolf wrote:
> [ ... ]
>>> +obj-$(CONFIG_SENSORS_SPD51118) += spd5118.o
>>
>> Hi,
>>
>> thank you for working on this, i am currently testing the driver on my machine.
>> I already noticed the kconfig option is wrong, the correct one would be CONFIG_SENSORS_SPD5118.
>>
>
> Oops. Thanks for noticing!
>
I fixed this up. I'll send v2 in a couple of days, probably early next week.
If it is not too much trouble, could you send me a register dump ?
Thanks,
Guenter
On 5/30/24 03:51, Thomas Weißschuh wrote:
> On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
>> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
>> sensors. Such sensors are typically found on DDR5 memory modules.
>>
>> Cc: René Rebe <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
>> (Corsair Venegance DDR5).
>>
>> René: I included you as MODULE_AUTHOR since the patch is derived from
>> your driver. Please let me know if you prefer not to be listed as
>> author.
>>
>> Documentation/hwmon/index.rst | 1 +
>> Documentation/hwmon/spd5118.rst | 60 ++++
>> drivers/hwmon/Kconfig | 12 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
>> 5 files changed, 556 insertions(+)
>> create mode 100644 Documentation/hwmon/spd5118.rst
>> create mode 100644 drivers/hwmon/spd5118.c
>
> With the Makefile and detect callback fixed:
>
> Reviewed-by: Thomas Weißschuh <[email protected]>
> Tested-by: Thomas Weißschuh <[email protected]>
Thanks a lot for the feedback!
If it is not too much trouble, could you send me a register dump ?
The one I have is from Montage Technology M88SPD5118, and I'd like to get
a few more to improve my module test script.
Thanks,
Guenter
On 5/30/24 06:57, Thomas Weißschuh wrote:
> On 2024-05-30 06:47:17+0000, Guenter Roeck wrote:
>> On 5/30/24 03:51, Thomas Weißschuh wrote:
>>> On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
>>>> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
>>>> sensors. Such sensors are typically found on DDR5 memory modules.
>>>>
>>>> Cc: René Rebe <[email protected]>
>>>> Signed-off-by: Guenter Roeck <[email protected]>
>>>> ---
>>>> Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
>>>> (Corsair Venegance DDR5).
>>>>
>>>> René: I included you as MODULE_AUTHOR since the patch is derived from
>>>> your driver. Please let me know if you prefer not to be listed as
>>>> author.
>>>>
>>>> Documentation/hwmon/index.rst | 1 +
>>>> Documentation/hwmon/spd5118.rst | 60 ++++
>>>> drivers/hwmon/Kconfig | 12 +
>>>> drivers/hwmon/Makefile | 1 +
>>>> drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
>>>> 5 files changed, 556 insertions(+)
>>>> create mode 100644 Documentation/hwmon/spd5118.rst
>>>> create mode 100644 drivers/hwmon/spd5118.c
>>>
>>> With the Makefile and detect callback fixed:
>>>
>>> Reviewed-by: Thomas Weißschuh <[email protected]>
>>> Tested-by: Thomas Weißschuh <[email protected]>
>>
>> Thanks a lot for the feedback!
>>
>> If it is not too much trouble, could you send me a register dump ?
>> The one I have is from Montage Technology M88SPD5118, and I'd like to get
>> a few more to improve my module test script.
>
>>From a Kingston KF556S40-32:
>
> # i2cdump 20 0x50
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> 00: 51 18 0a 86 32 03 32 00 00 00 00 07 ff 3c 00 00 Q???2?2....?.<..
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
> 20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
> 30: 00 f0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
> 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
This is the same SPD hub chip (Montage Technology M88SPD5118)
as used on my Corsair DDRs. Interesting.
Thanks!
Guenter
Am 30.05.24 um 15:39 schrieb Guenter Roeck:
> On 5/30/24 06:23, Guenter Roeck wrote:
>> On 5/30/24 01:08, Armin Wolf wrote:
>> [ ... ]
>>>> +obj-$(CONFIG_SENSORS_SPD51118) += spd5118.o
>>>
>>> Hi,
>>>
>>> thank you for working on this, i am currently testing the driver on
>>> my machine.
>>> I already noticed the kconfig option is wrong, the correct one would
>>> be CONFIG_SENSORS_SPD5118.
>>>
>>
>> Oops. Thanks for noticing!
>>
>
> I fixed this up. I'll send v2 in a couple of days, probably early next
> week.
>
> If it is not too much trouble, could you send me a register dump ?
>
> Thanks,
> Guenter
>
>
# i2cdump 1 0x51
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 51 18 0a 86 32 03 32 00 00 00 00 00 ff 3c 00 00 Q???2?2......<..
10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
30: 00 c0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
80: 30 10 12 02 04 00 40 42 00 00 00 00 b2 12 0d 00 0????.@B....???.
90: 00 00 00 00 a0 01 f2 03 7a 0d 00 00 00 00 80 3e ....????z?....?>
a0: 80 3e 80 3e 00 7d 80 bb 30 75 27 01 a0 00 82 00 ?>?>.}??0u'??.?.
b0: 00 00 00 00 00 00 d4 00 00 00 d4 00 00 00 d4 00 ......?...?...?.
c0: 00 00 d4 00 00 00 88 13 08 88 13 08 20 4e 20 10 ..?...?????? N ?
d0: 27 10 1a 41 28 10 27 10 c4 09 04 4c 1d 0c 00 00 '??A(?'????L??..
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
# i2cdump 1 0x53
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 51 18 0a 86 32 03 32 00 00 00 00 00 ff 3c 00 00 Q???2?2......<..
10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
30: 00 cc 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
80: 30 10 12 02 04 00 40 42 00 00 00 00 b2 12 0d 00 0????.@B....???.
90: 00 00 00 00 a0 01 f2 03 7a 0d 00 00 00 00 80 3e ....????z?....?>
a0: 80 3e 80 3e 00 7d 80 bb 30 75 27 01 a0 00 82 00 ?>?>.}??0u'??.?.
b0: 00 00 00 00 00 00 d4 00 00 00 d4 00 00 00 d4 00 ......?...?...?.
c0: 00 00 d4 00 00 00 88 13 08 88 13 08 20 4e 20 10 ..?...?????? N ?
d0: 27 10 1a 41 28 10 27 10 c4 09 04 4c 1d 0c 00 00 '??A(?'????L??..
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Hi Armin,
On 5/30/24 09:45, Armin Wolf wrote:
[ ... ]
>>
> # i2cdump 1 0x51
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> 00: 51 18 0a 86 32 03 32 00 00 00 00 00 ff 3c 00 00 Q???2?2......<..
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
> 20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
> 30: 00 c0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
> 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 80: 30 10 12 02 04 00 40 42 00 00 00 00 b2 12 0d 00 0????.@B....???.
> 90: 00 00 00 00 a0 01 f2 03 7a 0d 00 00 00 00 80 3e ....????z?....?>
> a0: 80 3e 80 3e 00 7d 80 bb 30 75 27 01 a0 00 82 00 ?>?>.}??0u'??.?.
> b0: 00 00 00 00 00 00 d4 00 00 00 d4 00 00 00 d4 00 ......?...?...?.
> c0: 00 00 d4 00 00 00 88 13 08 88 13 08 20 4e 20 10 ..?...?????? N ?
> d0: 27 10 1a 41 28 10 27 10 c4 09 04 4c 1d 0c 00 00 '??A(?'????L??..
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>
Thanks a lot. This is again Montage Technology's M88SPD5118.
What is your DDR module vendor ?
Thanks,
Guenter
Am 29.05.24 um 22:52 schrieb Guenter Roeck:
> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.
>
> Cc: René Rebe <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
> (Corsair Venegance DDR5).
>
> René: I included you as MODULE_AUTHOR since the patch is derived from
> your driver. Please let me know if you prefer not to be listed as
> author.
>
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/spd5118.rst | 60 ++++
> drivers/hwmon/Kconfig | 12 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
> 5 files changed, 556 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..67e990551a8a
> --- /dev/null
> +++ b/Documentation/hwmon/spd5118.rst
> @@ -0,0 +1,60 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver spd5118
> +=====================
> +
> +Supported chips:
> +
> + * SPD5118 (JEDEC JESD300-5B.01) 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-5B.01) 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.
> +
> +
> +PEC configuration
> +-----------------
> +
> +If the I2C/SMBus controller supports PEC, it can be enabled or disabled using
> +the 'pec' sysfs attribute attached to the i2c device.
> +
Hi,
the spd5118 only supports PEC when in i3c basic mode, so afaik we cannot use PEC here.
> +
> +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
Maybe it would be a good idea to tell users that the alarm attributes are sticky.
> +======================= ===========================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7f384a2494c9..111d05718b89 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2182,6 +2182,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-5B)
> + 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..07c593fae9a3 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_SPD51118) += 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..440503d09d13
> --- /dev/null
> +++ b/drivers/hwmon/spd5118.c
> @@ -0,0 +1,482 @@
> +// 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, ®val);
> + 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, u32 attr, long *val)
> +{
> + u32 regval;
> + int err;
> +
> + err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, ®val);
> + 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, attr, 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, u32 attr, 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);
The spd5118 spec says that we have to wait 10ms after enabling the sensors before
we start reading temperature values, maybe we need a delay + locking here?
> +}
> +
> +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, attr, 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;
> +
> + 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;
> +
> + if (!(regval & SPD5118_CAP_TS_SUPPORT) || (regval & 0xfc))
> + 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, ®val);
> + 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,
The driver is missing suspend support, without it hibernation/S4 sleep will cause the
limit and config registers to be out of sync with the regmap cache.
Thanks,
Armin Wolf
> + },
> + .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");
Am 30.05.24 um 18:51 schrieb Guenter Roeck:
> Hi Armin,
>
> On 5/30/24 09:45, Armin Wolf wrote:
> [ ... ]
>>>
>> # i2cdump 1 0x51
>> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
>> 00: 51 18 0a 86 32 03 32 00 00 00 00 00 ff 3c 00 00 Q???2?2......<..
>> 10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
>> 20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
>> 30: 00 c0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
>> 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 80: 30 10 12 02 04 00 40 42 00 00 00 00 b2 12 0d 00 0????.@B....???.
>> 90: 00 00 00 00 a0 01 f2 03 7a 0d 00 00 00 00 80 3e ....????z?....?>
>> a0: 80 3e 80 3e 00 7d 80 bb 30 75 27 01 a0 00 82 00 ?>?>.}??0u'??.?.
>> b0: 00 00 00 00 00 00 d4 00 00 00 d4 00 00 00 d4 00 ......?...?...?.
>> c0: 00 00 d4 00 00 00 88 13 08 88 13 08 20 4e 20 10 ..?...?????? N ?
>> d0: 27 10 1a 41 28 10 27 10 c4 09 04 4c 1d 0c 00 00 '??A(?'????L??..
>> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>
>
> Thanks a lot. This is again Montage Technology's M88SPD5118.
> What is your DDR module vendor ?
>
> Thanks,
> Guenter
>
Kingston Fury KF552C40BBK2-16, a 2x8GB DDR5 5200 kit.
Thanks,
Armin Wolf
On 5/30/24 10:03, Armin Wolf wrote:
> Am 29.05.24 um 22:52 schrieb Guenter Roeck:
>
[ ... ]
>> +
>> +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
>
> Maybe it would be a good idea to tell users that the alarm attributes are sticky.
>
The driver auto-clears them after read, so they are only sticky in the sense
that they will remain active until read. This is quite common for hardware
monitoring devices. However, sure, I'll add a note.
[ ... ]
>> +static int spd5118_write_enable(struct regmap *regmap, u32 attr, 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);
>
> The spd5118 spec says that we have to wait 10ms after enabling the sensors before
> we start reading temperature values, maybe we need a delay + locking here?
>
I don't think that would add much if any value but a lot of complexity
for little gain. I find it acceptable that the sensor returns 0 for a few ms
after enabling it. Pretty much all chips have the same problem, so I am
really not concerned about it.
>> +
>> +static struct i2c_driver spd5118_driver = {
>> + .class = I2C_CLASS_HWMON,
>> + .driver = {
>> + .name = "spd5118",
>> + .of_match_table = spd5118_of_ids,
>
> The driver is missing suspend support, without it hibernation/S4 sleep will cause the
> limit and config registers to be out of sync with the regmap cache.
>
Good point. Do you have a means to test this if I add suspend support ?
I have not been able to figure out how to put my system into suspend.
Thanks,
Guenter
On 5/30/24 10:03, Armin Wolf wrote:
[ ... ]
>> +
>> +
>> +PEC configuration
>> +-----------------
>> +
>> +If the I2C/SMBus controller supports PEC, it can be enabled or disabled using
>> +the 'pec' sysfs attribute attached to the i2c device.
>> +
>
> Hi,
>
> the spd5118 only supports PEC when in i3c basic mode, so afaik we cannot use PEC here.
>
Thanks for noticing. I misread the spec and associated the note with PAR_DIS,
not PEC_EN. I'll drop this and the PEC patch. Good that I kept it separate.
Thanks,
Guenter
Am 30.05.24 um 19:33 schrieb Guenter Roeck:
> On 5/30/24 10:03, Armin Wolf wrote:
>> Am 29.05.24 um 22:52 schrieb Guenter Roeck:
>>
> [ ... ]
>
>>> +
>>> +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
>>
>> Maybe it would be a good idea to tell users that the alarm attributes
>> are sticky.
>>
>
> The driver auto-clears them after read, so they are only sticky in the
> sense
> that they will remain active until read. This is quite common for
> hardware
> monitoring devices. However, sure, I'll add a note.
>
> [ ... ]
>
>>> +static int spd5118_write_enable(struct regmap *regmap, u32 attr,
>>> 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);
>>
>> The spd5118 spec says that we have to wait 10ms after enabling the
>> sensors before
>> we start reading temperature values, maybe we need a delay + locking
>> here?
>>
>
> I don't think that would add much if any value but a lot of complexity
> for little gain. I find it acceptable that the sensor returns 0 for a
> few ms
> after enabling it. Pretty much all chips have the same problem, so I am
> really not concerned about it.
>
>>> +
>>> +static struct i2c_driver spd5118_driver = {
>>> + .class = I2C_CLASS_HWMON,
>>> + .driver = {
>>> + .name = "spd5118",
>>> + .of_match_table = spd5118_of_ids,
>>
>> The driver is missing suspend support, without it hibernation/S4
>> sleep will cause the
>> limit and config registers to be out of sync with the regmap cache.
>>
>
> Good point. Do you have a means to test this if I add suspend support ?
> I have not been able to figure out how to put my system into suspend.
>
> Thanks,
> Guenter
>
I think so, at least i can verify if S3 sleep works, but S4 sleep should work fine too.
Thanks,
Armin Wolf
On 5/30/24 13:20, Thomas Weißschuh wrote:
> On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
>> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
>> sensors. Such sensors are typically found on DDR5 memory modules.
>
> I can get the module to automatically probe with this change:
>
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 97f338b123b1..8d9218f755d7 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -382,6 +386,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",
>
> (Credits go to Paul Menzel [0])
>
> Maybe you can add that to your series.
>
That is specifically for SPD (eeprom) support, which I didn't provide
in the driver. It does not register the equivalent jc42.4 temperature
sensor either. Given that, using the code to register a temperature
sensor seems inappropriate.
I didn't include accessing the SPD eeprom to the driver because I don't
have a use case. I don't mind adding it, though, if others think that it is
important.
> To also work with my PIIX4 I2C bus, I also need:
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index fe6e8a1bb607..ff66e883b348 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -195,6 +195,7 @@ config I2C_ISMT
> config I2C_PIIX4
> tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
> depends on PCI && HAS_IOPORT
> + select I2C_SMBUS
> help
> If you say yes to this option, support will be included for the Intel
> PIIX4 family of mainboard I2C interfaces. Specifically, the following
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 6a0392172b2f..f8d81f8c0cb3 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -29,6 +29,7 @@
> #include <linux/stddef.h>
> #include <linux/ioport.h>
> #include <linux/i2c.h>
> +#include <linux/i2c-smbus.h>
> #include <linux/slab.h>
> #include <linux/dmi.h>
> #include <linux/acpi.h>
> @@ -982,6 +983,8 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> return retval;
> }
>
> + i2c_register_spd(adap);
> +
> *padap = adap;
> return 0;
> }
>
> Though I guess it's not the right place to call i2c_register_sdp(),
> I'll look at it some more and then submit it.
>
Hmm, I didn't find a better place though.
Please copy me when you submit a patch; I can test it on an AMD system with
DDR4.
Thanks,
Guenter
On 5/30/24 14:02, Thomas Weißschuh wrote:
> On 2024-05-30 13:46:48+0000, Guenter Roeck wrote:
>> On 5/30/24 13:20, Thomas Weißschuh wrote:
>>> On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
>>>> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
>>>> sensors. Such sensors are typically found on DDR5 memory modules.
>>>
>>> I can get the module to automatically probe with this change:
>>>
>>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>>> index 97f338b123b1..8d9218f755d7 100644
>>> --- a/drivers/i2c/i2c-smbus.c
>>> +++ b/drivers/i2c/i2c-smbus.c
>>> @@ -382,6 +386,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",
>>>
>>> (Credits go to Paul Menzel [0])
>>>
>>> Maybe you can add that to your series.
>>>
>>
>> That is specifically for SPD (eeprom) support, which I didn't provide
>> in the driver. It does not register the equivalent jc42.4 temperature
>> sensor either. Given that, using the code to register a temperature
>> sensor seems inappropriate.
>
> I see, I wasn't aware about the specifics of SPD.
>
> It felt like a nice way to get automatic probing.
> (I was wondering about that today before)
>
>> I didn't include accessing the SPD eeprom to the driver because I don't
>> have a use case. I don't mind adding it, though, if others think that it is
>> important.
>
> Wolfgang seems to think it's important:
> https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
>
Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
think this is needed ? Note that I am not opposed to adding spd
eeprom support, but I'd like to know why I am doing it before
I spend time on it.
Auto detection would be nice, though, because with that we could
drop support for the _detect function (which is kind of risky
on the i2c address range normally used for eeproms).
Thanks,
Guenter
On 2024-05-30 13:46:48+0000, Guenter Roeck wrote:
> On 5/30/24 13:20, Thomas Weißschuh wrote:
> > On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
> > > Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> > > sensors. Such sensors are typically found on DDR5 memory modules.
> >
> > I can get the module to automatically probe with this change:
> >
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index 97f338b123b1..8d9218f755d7 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -382,6 +386,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",
> >
> > (Credits go to Paul Menzel [0])
> >
> > Maybe you can add that to your series.
> >
>
> That is specifically for SPD (eeprom) support, which I didn't provide
> in the driver. It does not register the equivalent jc42.4 temperature
> sensor either. Given that, using the code to register a temperature
> sensor seems inappropriate.
I see, I wasn't aware about the specifics of SPD.
It felt like a nice way to get automatic probing.
(I was wondering about that today before)
> I didn't include accessing the SPD eeprom to the driver because I don't
> have a use case. I don't mind adding it, though, if others think that it is
> important.
Wolfgang seems to think it's important:
https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
> > To also work with my PIIX4 I2C bus, I also need:
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index fe6e8a1bb607..ff66e883b348 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -195,6 +195,7 @@ config I2C_ISMT
> > config I2C_PIIX4
> > tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
> > depends on PCI && HAS_IOPORT
> > + select I2C_SMBUS
> > help
> > If you say yes to this option, support will be included for the Intel
> > PIIX4 family of mainboard I2C interfaces. Specifically, the following
> > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> > index 6a0392172b2f..f8d81f8c0cb3 100644
> > --- a/drivers/i2c/busses/i2c-piix4.c
> > +++ b/drivers/i2c/busses/i2c-piix4.c
> > @@ -29,6 +29,7 @@
> > #include <linux/stddef.h>
> > #include <linux/ioport.h>
> > #include <linux/i2c.h>
> > +#include <linux/i2c-smbus.h>
> > #include <linux/slab.h>
> > #include <linux/dmi.h>
> > #include <linux/acpi.h>
> > @@ -982,6 +983,8 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> > return retval;
> > }
> >
> > + i2c_register_spd(adap);
> > +
> > *padap = adap;
> > return 0;
> > }
> >
> > Though I guess it's not the right place to call i2c_register_sdp(),
> > I'll look at it some more and then submit it.
> >
>
> Hmm, I didn't find a better place though.
>
> Please copy me when you submit a patch; I can test it on an AMD system with
> DDR4.
Will do.
Thomas
On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
> Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> sensors. Such sensors are typically found on DDR5 memory modules.
I can get the module to automatically probe with this change:
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 97f338b123b1..8d9218f755d7 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -382,6 +386,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",
(Credits go to Paul Menzel [0])
Maybe you can add that to your series.
To also work with my PIIX4 I2C bus, I also need:
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fe6e8a1bb607..ff66e883b348 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -195,6 +195,7 @@ config I2C_ISMT
config I2C_PIIX4
tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
depends on PCI && HAS_IOPORT
+ select I2C_SMBUS
help
If you say yes to this option, support will be included for the Intel
PIIX4 family of mainboard I2C interfaces. Specifically, the following
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 6a0392172b2f..f8d81f8c0cb3 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -29,6 +29,7 @@
#include <linux/stddef.h>
#include <linux/ioport.h>
#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
#include <linux/slab.h>
#include <linux/dmi.h>
#include <linux/acpi.h>
@@ -982,6 +983,8 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
return retval;
}
+ i2c_register_spd(adap);
+
*padap = adap;
return 0;
}
Though I guess it's not the right place to call i2c_register_sdp(),
I'll look at it some more and then submit it.
[0] https://lore.kernel.org/lkml/[email protected]/
Hi all,
> > Wolfgang seems to think it's important:
Wolfram, please.
> > https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
> >
>
> Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
> think this is needed ? Note that I am not opposed to adding spd
> eeprom support, but I'd like to know why I am doing it before
> I spend time on it.
A working eeprom driver is needed to get 'decode-dimms' from the
i2c-tools package working. Jean reported that EEPROM access for DDR5 is
different from DDR4, so it needs a separate driver. And
i2c_register_spd() then needs to be updated to use the new driver for
DDR5.
Happy hacking,
Wolfram
On 2024-05-30 06:47:17+0000, Guenter Roeck wrote:
> On 5/30/24 03:51, Thomas Weißschuh wrote:
> > On 2024-05-29 13:52:03+0000, Guenter Roeck wrote:
> > > Add support for SPD5118 (Jedec JESD300-5B.01) compliant temperature
> > > sensors. Such sensors are typically found on DDR5 memory modules.
> > >
> > > Cc: René Rebe <[email protected]>
> > > Signed-off-by: Guenter Roeck <[email protected]>
> > > ---
> > > Tested on MAG B650 TOMAHAWK WIFI with CMH32GX5M2B6000Z30
> > > (Corsair Venegance DDR5).
> > >
> > > René: I included you as MODULE_AUTHOR since the patch is derived from
> > > your driver. Please let me know if you prefer not to be listed as
> > > author.
> > >
> > > Documentation/hwmon/index.rst | 1 +
> > > Documentation/hwmon/spd5118.rst | 60 ++++
> > > drivers/hwmon/Kconfig | 12 +
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/spd5118.c | 482 ++++++++++++++++++++++++++++++++
> > > 5 files changed, 556 insertions(+)
> > > create mode 100644 Documentation/hwmon/spd5118.rst
> > > create mode 100644 drivers/hwmon/spd5118.c
> >
> > With the Makefile and detect callback fixed:
> >
> > Reviewed-by: Thomas Weißschuh <[email protected]>
> > Tested-by: Thomas Weißschuh <[email protected]>
>
> Thanks a lot for the feedback!
>
> If it is not too much trouble, could you send me a register dump ?
> The one I have is from Montage Technology M88SPD5118, and I'd like to get
> a few more to improve my module test script.
From a Kingston KF556S40-32:
# i2cdump 20 0x50
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 51 18 0a 86 32 03 32 00 00 00 00 07 ff 3c 00 00 Q???2?2....?.<..
10: 00 00 00 00 00 00 00 00 00 00 00 00 70 03 00 00 ............p?..
20: 50 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 P?..............
30: 00 f0 01 00 00 00 00 00 00 00 00 00 00 00 00 00 .??.............
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Hi,
On May 31, 2024, at 11:31, Wolfram Sang <[email protected]> wrote:
>
> Hi all,
>
>>> Wolfgang seems to think it's important:
>
> Wolfram, please.
>
>>> https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
>>>
>>
>> Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
>> think this is needed ? Note that I am not opposed to adding spd
>> eeprom support, but I'd like to know why I am doing it before
>> I spend time on it.
>
> A working eeprom driver is needed to get 'decode-dimms' from the
> i2c-tools package working. Jean reported that EEPROM access for DDR5 is
> different from DDR4, so it needs a separate driver. And
> i2c_register_spd() then needs to be updated to use the new driver for
> DDR5.
Well my original downstream driver already had eeprom access:
https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch
Note there are some surrounding -2, and parity patches around this patch.
Thanks,
René
--
ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
http://exactcode.com | http://exactscan.com | http://ocrkit.com
Hi René,
On 5/31/24 03:01, René Rebe wrote:
> Hi,
>
> On May 31, 2024, at 11:31, Wolfram Sang <[email protected]> wrote:
>>
>> Hi all,
>>
>>>> Wolfgang seems to think it's important:
>>
>> Wolfram, please.
>>
>>>> https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
>>>>
>>>
>>> Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
>>> think this is needed ? Note that I am not opposed to adding spd
>>> eeprom support, but I'd like to know why I am doing it before
>>> I spend time on it.
>>
>> A working eeprom driver is needed to get 'decode-dimms' from the
>> i2c-tools package working. Jean reported that EEPROM access for DDR5 is
>> different from DDR4, so it needs a separate driver. And
>> i2c_register_spd() then needs to be updated to use the new driver for
>> DDR5.
>
> Well my original downstream driver already had eeprom access:
>
> https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch
>
Yes, but you didn't send it upstream, so I took it, fixed a couple of bugs,
dropped eeprom support since that is secondary for my use case as well as the
out-of-tree parity code, and submitted it. I'd be more than happy to let you
take over if you like.
Thanks,
Guenter
On 5/31/24 06:20, René Rebe wrote:
> Hi,
>
>> On May 31, 2024, at 15:14, Guenter Roeck <[email protected]> wrote:
>>
>> Hi René,
>>
>> On 5/31/24 03:01, René Rebe wrote:
>>> Hi,
>>> On May 31, 2024, at 11:31, Wolfram Sang <[email protected]> wrote:
>>>>
>>>> Hi all,
>>>>
>>>>>> Wolfgang seems to think it's important:
>>>>
>>>> Wolfram, please.
>>>>
>>>>>> https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
>>>>>>
>>>>>
>>>>> Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
>>>>> think this is needed ? Note that I am not opposed to adding spd
>>>>> eeprom support, but I'd like to know why I am doing it before
>>>>> I spend time on it.
>>>>
>>>> A working eeprom driver is needed to get 'decode-dimms' from the
>>>> i2c-tools package working. Jean reported that EEPROM access for DDR5 is
>>>> different from DDR4, so it needs a separate driver. And
>>>> i2c_register_spd() then needs to be updated to use the new driver for
>>>> DDR5.
>>> Well my original downstream driver already had eeprom access:
>>> https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch
>>
>> Yes, but you didn't send it upstream, so I took it, fixed a couple of bugs,
>
> And I appreciate that!
>
>> dropped eeprom support since that is secondary for my use case as well as the
>
> I only said the original code had this implemented if someone wants to re-add it
> to save them some time not having to re-write it from scratch ;-)
>
That is for sure what I had planned to do.
>> out-of-tree parity code, and submitted it. I'd be more than happy to let you
>> take over if you like.
>
> I’m mostly out of time, so I appreciate you starting the upstream process.
>
Ok, I'll keep going.
Thanks,
Guenter
On 2024-05-31 12:01:35+0000, René Rebe wrote:
> On May 31, 2024, at 11:31, Wolfram Sang <[email protected]> wrote:
> >>> Wolfgang seems to think it's important:
> >
> > Wolfram, please.
Sorry, Wolfram.
> >
> >>> https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
> >>>
> >>
> >> Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
> >> think this is needed ? Note that I am not opposed to adding spd
> >> eeprom support, but I'd like to know why I am doing it before
> >> I spend time on it.
> >
> > A working eeprom driver is needed to get 'decode-dimms' from the
> > i2c-tools package working. Jean reported that EEPROM access for DDR5 is
> > different from DDR4, so it needs a separate driver. And
> > i2c_register_spd() then needs to be updated to use the new driver for
> > DDR5.
>
> Well my original downstream driver already had eeprom access:
>
> https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch
>
> Note there are some surrounding -2, and parity patches around this patch.
That would need to be rewritten to use the nvmem APIs though, I guess.
If nobody wants to do it, I'm volunteering.
Thomas
Hi,
> On May 31, 2024, at 15:14, Guenter Roeck <[email protected]> wrote:
>
> Hi René,
>
> On 5/31/24 03:01, René Rebe wrote:
>> Hi,
>> On May 31, 2024, at 11:31, Wolfram Sang <[email protected]> wrote:
>>>
>>> Hi all,
>>>
>>>>> Wolfgang seems to think it's important:
>>>
>>> Wolfram, please.
>>>
>>>>> https://lore.kernel.org/lkml/tdia472d4pow2osabef24y2ujkkquplfajxmmtk5pnxllsdxsz@wxzynz7llasr/
>>>>>
>>>>
>>>> Ok, but that doesn't explain the reason. Wolfram, Paul, why do you
>>>> think this is needed ? Note that I am not opposed to adding spd
>>>> eeprom support, but I'd like to know why I am doing it before
>>>> I spend time on it.
>>>
>>> A working eeprom driver is needed to get 'decode-dimms' from the
>>> i2c-tools package working. Jean reported that EEPROM access for DDR5 is
>>> different from DDR4, so it needs a separate driver. And
>>> i2c_register_spd() then needs to be updated to use the new driver for
>>> DDR5.
>> Well my original downstream driver already had eeprom access:
>> https://svn.exactcode.de/t2/trunk/package/kernel/linux/spd-5118.patch
>
> Yes, but you didn't send it upstream, so I took it, fixed a couple of bugs,
And I appreciate that!
> dropped eeprom support since that is secondary for my use case as well as the
I only said the original code had this implemented if someone wants to re-add it
to save them some time not having to re-write it from scratch ;-)
> out-of-tree parity code, and submitted it. I'd be more than happy to let you
> take over if you like.
I’m mostly out of time, so I appreciate you starting the upstream process.
Thank you so much,
René
--
ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
http://exactcode.com | http://exactscan.com | http://ocrkit.com