Add support for SPD5118 (Jedec JESD300) compliant chips supporting
a temperature sensor and SPD NVRAM. Such devices are typically found on
DDR5 memory modules.
The first patch of the series adds SPD5118 devicetree bindings. The second
patch adds support for SPD5118 temperature sensors. The third patch adds
support for suspend/resume. The last patch adds support for reading the SPD
NVRAM.
Note: The driver introduced with this patch series does not currently
support accessing SPD5118 compliant chips in I3C mode.
v3: Drop explicit bindings document; add binding to trivial devices instead
Add support for reading SPD NVRAM
v2: Drop PEC support; it only applies to I3C mode.
Update documentation
Add suspend/resume support
----------------------------------------------------------------
Guenter Roeck (4):
dt-bindings: trivial-devices: Add jedec,spd5118
hwmon: Add support for SPD5118 compliant temperature sensors
hwmon: (spd5118) Add suspend/resume support
hwmon: (spd5118) Add support for reading SPD data
.../devicetree/bindings/trivial-devices.yaml | 2 +
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/spd5118.rst | 56 ++
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/spd5118.c | 648 +++++++++++++++++++++
6 files changed, 720 insertions(+)
create mode 100644 Documentation/hwmon/spd5118.rst
create mode 100644 drivers/hwmon/spd5118.c
Add bindings for the SPD hub present in DDR5 modules.
(https://www.jedec.org/standards-documents/docs/jesd300-5b01).
Signed-off-by: Guenter Roeck <[email protected]>
---
v3: Drop explicit bindings file; add binding to trivial devices
instead
Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 0a419453d183..1d19e67de2a1 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -168,6 +168,8 @@ properties:
- isil,isl69269
# Intersil ISL76682 Ambient Light Sensor
- isil,isl76682
+ # JEDEC JESD300 (SPD5118) Hub and Serial Presence Detect
+ - jedec,spd5118
# Linear Technology LTC2488
- lineartechnology,ltc2488
# 5 Bit Programmable, Pulse-Width Modulator
--
2.39.2
Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
compliant memory modules. NVRAM write operation is not supported.
Signed-off-by: Guenter Roeck <[email protected]>
---
v3: New patch
RFT: I'd like to get some more test coverage before moving forward
with this patch. decode-dimms doesn't recognize the 'spd5118'
driver.
Documentation/hwmon/spd5118.rst | 8 ++
drivers/hwmon/spd5118.c | 146 +++++++++++++++++++++++++++++++-
2 files changed, 150 insertions(+), 4 deletions(-)
diff --git a/Documentation/hwmon/spd5118.rst b/Documentation/hwmon/spd5118.rst
index a15d75aa2066..ef7338f46575 100644
--- a/Documentation/hwmon/spd5118.rst
+++ b/Documentation/hwmon/spd5118.rst
@@ -53,3 +53,11 @@ temp1_crit_alarm Temperature critical alarm
Alarm attributes are sticky until read and will be cleared afterwards
unless the alarm condition still applies.
+
+
+SPD (Serial Presence Detect) support
+------------------------------------
+
+The driver also supports reading the SPD NVRAM on SPD5118 compatible chips.
+SPD data is available from the 'eeprom' binary attribute file attached to the
+chip's I2C device.
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index baa315172298..131002290ff3 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -20,6 +20,8 @@
#include <linux/i2c.h>
#include <linux/hwmon.h>
#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nvmem-provider.h>
#include <linux/pm.h>
#include <linux/regmap.h>
#include <linux/units.h>
@@ -53,12 +55,31 @@ static const unsigned short normal_i2c[] = {
#define SPD5118_TS_DISABLE BIT(0) /* temperature sensor disable */
+#define SPD5118_LEGACY_MODE_ADDR BIT(3)
+#define SPD5118_LEGACY_PAGE_MASK GENMASK(2, 0)
+#define SPD5118_LEGACY_MODE_MASK (SPD5118_LEGACY_MODE_ADDR | SPD5118_LEGACY_PAGE_MASK)
+
+
+#define SPD5118_NUM_PAGES 8
+#define SPD5118_PAGE_SIZE 128
+#define SPD5118_PAGE_SHIFT 7
+#define SPD5118_PAGE_MASK GENMASK(6, 0)
+#define SPD5118_EEPROM_BASE 0x80
+#define SPD5118_EEPROM_SIZE (SPD5118_PAGE_SIZE * SPD5118_NUM_PAGES)
+
/* Temperature unit in millicelsius */
#define SPD5118_TEMP_UNIT (MILLIDEGREE_PER_DEGREE / 4)
/* Representable temperature range in millicelsius */
#define SPD5118_TEMP_RANGE_MIN -256000
#define SPD5118_TEMP_RANGE_MAX 255750
+struct spd5118_data {
+ struct regmap *regmap;
+ struct mutex nvmem_lock;
+};
+
+/* hwmon */
+
static int spd5118_temp_from_reg(u16 reg)
{
int temp = sign_extend32(reg >> 2, 10);
@@ -360,9 +381,111 @@ static const struct hwmon_chip_info spd5118_chip_info = {
.info = spd5118_info,
};
+/* nvmem */
+
+static int spd5118_nvmem_set_page(struct regmap *regmap, int page)
+{
+ unsigned int old_page;
+ int err;
+
+ err = regmap_read(regmap, SPD5118_REG_I2C_LEGACY_MODE, &old_page);
+ if (err)
+ return err;
+
+ if (page != (old_page & SPD5118_LEGACY_MODE_MASK)) {
+ /* Update page and explicitly select 1-byte addressing */
+ err = regmap_update_bits(regmap, SPD5118_REG_I2C_LEGACY_MODE,
+ SPD5118_LEGACY_MODE_MASK, page);
+ if (err)
+ return err;
+
+ /* Selected new NVMEM page, drop cached data */
+ regcache_drop_region(regmap, SPD5118_EEPROM_BASE, 0xff);
+ }
+
+ return 0;
+}
+
+static ssize_t spd5118_nvmem_read_page(struct regmap *regmap, char *buf,
+ unsigned int offset, size_t count)
+{
+ int err;
+
+ err = spd5118_nvmem_set_page(regmap, offset >> SPD5118_PAGE_SHIFT);
+ if (err)
+ return err;
+
+ offset &= SPD5118_PAGE_MASK;
+
+ /* Can't cross page boundaries */
+ if (offset + count > SPD5118_PAGE_SIZE)
+ count = SPD5118_PAGE_SIZE - offset;
+
+ err = regmap_bulk_read(regmap, SPD5118_EEPROM_BASE + offset, buf, count);
+ if (err)
+ return err;
+
+ return count;
+}
+
+static int spd5118_nvmem_read(void *priv, unsigned int off, void *val, size_t count)
+{
+ struct spd5118_data *data = priv;
+ char *buf = val;
+ int ret;
+
+ if (unlikely(!count))
+ return count;
+
+ if (off + count > SPD5118_EEPROM_SIZE)
+ return -EINVAL;
+
+ mutex_lock(&data->nvmem_lock);
+
+ while (count) {
+ ret = spd5118_nvmem_read_page(data->regmap, buf, off, count);
+ if (ret < 0) {
+ mutex_unlock(&data->nvmem_lock);
+ return ret;
+ }
+ buf += ret;
+ off += ret;
+ count -= ret;
+ }
+ mutex_unlock(&data->nvmem_lock);
+ return 0;
+}
+
+static int spd5118_nvmem_init(struct device *dev, struct spd5118_data *data)
+{
+ struct nvmem_config nvmem_config = {
+ .type = NVMEM_TYPE_EEPROM,
+ .name = dev_name(dev),
+ .id = NVMEM_DEVID_AUTO,
+ .dev = dev,
+ .base_dev = dev,
+ .read_only = true,
+ .root_only = false,
+ .owner = THIS_MODULE,
+ .compat = true,
+ .reg_read = spd5118_nvmem_read,
+ .priv = data,
+ .stride = 1,
+ .word_size = 1,
+ .size = SPD5118_EEPROM_SIZE,
+ };
+ struct nvmem_device *nvmem;
+
+ nvmem = devm_nvmem_register(dev, &nvmem_config);
+ return PTR_ERR_OR_ZERO(nvmem);
+}
+
+/* regmap */
+
static bool spd5118_writeable_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
+ case SPD5118_REG_I2C_LEGACY_MODE:
case SPD5118_REG_TEMP_CLR:
case SPD5118_REG_TEMP_CONFIG:
case SPD5118_REG_TEMP_MAX:
@@ -396,7 +519,7 @@ static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
static const struct regmap_config spd5118_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
- .max_register = SPD5118_REG_TEMP_STATUS,
+ .max_register = 0xff,
.writeable_reg = spd5118_writeable_reg,
.volatile_reg = spd5118_volatile_reg,
.cache_type = REGCACHE_MAPLE,
@@ -406,10 +529,15 @@ static int spd5118_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
unsigned int regval, revision, vendor, bank;
+ struct spd5118_data *data;
struct device *hwmon_dev;
struct regmap *regmap;
int err;
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
if (IS_ERR(regmap))
return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
@@ -433,7 +561,15 @@ static int spd5118_probe(struct i2c_client *client)
if (!spd5118_vendor_valid(bank, vendor))
return -ENODEV;
- dev_set_drvdata(dev, regmap);
+ data->regmap = regmap;
+ mutex_init(&data->nvmem_lock);
+ dev_set_drvdata(dev, data);
+
+ err = spd5118_nvmem_init(dev, data);
+ if (err) {
+ dev_err_probe(dev, err, "failed to register nvmem\n");
+ return err;
+ }
hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
regmap, &spd5118_chip_info,
@@ -454,7 +590,8 @@ static int spd5118_probe(struct i2c_client *client)
static int spd5118_suspend(struct device *dev)
{
- struct regmap *regmap = dev_get_drvdata(dev);
+ struct spd5118_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
regcache_cache_bypass(regmap, true);
regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE,
@@ -469,7 +606,8 @@ static int spd5118_suspend(struct device *dev)
static int spd5118_resume(struct device *dev)
{
- struct regmap *regmap = dev_get_drvdata(dev);
+ struct spd5118_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
regcache_cache_only(regmap, false);
return regcache_sync(regmap);
--
2.39.2
On Fri, May 31, 2024 at 04:05:52PM -0700, Guenter Roeck wrote:
Adding Paul to CC who is likely interested in this series.
> Add support for SPD5118 (Jedec JESD300) compliant chips supporting
> a temperature sensor and SPD NVRAM. Such devices are typically found on
> DDR5 memory modules.
>
> The first patch of the series adds SPD5118 devicetree bindings. The second
> patch adds support for SPD5118 temperature sensors. The third patch adds
> support for suspend/resume. The last patch adds support for reading the SPD
> NVRAM.
>
> Note: The driver introduced with this patch series does not currently
> support accessing SPD5118 compliant chips in I3C mode.
>
> v3: Drop explicit bindings document; add binding to trivial devices instead
> Add support for reading SPD NVRAM
>
> v2: Drop PEC support; it only applies to I3C mode.
> Update documentation
> Add suspend/resume support
>
> ----------------------------------------------------------------
> Guenter Roeck (4):
> dt-bindings: trivial-devices: Add jedec,spd5118
> hwmon: Add support for SPD5118 compliant temperature sensors
> hwmon: (spd5118) Add suspend/resume support
> hwmon: (spd5118) Add support for reading SPD data
>
> .../devicetree/bindings/trivial-devices.yaml | 2 +
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/spd5118.rst | 56 ++
> drivers/hwmon/Kconfig | 12 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/spd5118.c | 648 +++++++++++++++++++++
> 6 files changed, 720 insertions(+)
> create mode 100644 Documentation/hwmon/spd5118.rst
> create mode 100644 drivers/hwmon/spd5118.c
On 5/31/24 16:05, Guenter Roeck wrote:
> Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
> compliant memory modules. NVRAM write operation is not supported.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v3: New patch
>
> RFT: I'd like to get some more test coverage before moving forward
> with this patch. decode-dimms doesn't recognize the 'spd5118'
> driver.
>
Looking for feedback:
[ ... ]
> +
> + nvmem = devm_nvmem_register(dev, &nvmem_config);
This returns ERR_PTR(-EOPNOTSUPP) if CONFIG_NVRAM=n. We have two options:
- Ignore -EOPNOTSUPP and continue registering the hwmon device
or
- Add
select NVRAM
select NVRAM_SYSFS
to the driver's Kconfig entry.
Any preferences ?
Thanks,
Guenter
On 2024-05-31 22:42:24+0000, Guenter Roeck wrote:
> On 5/31/24 16:05, Guenter Roeck wrote:
> > Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
> > compliant memory modules. NVRAM write operation is not supported.
> >
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > v3: New patch
> >
> > RFT: I'd like to get some more test coverage before moving forward
> > with this patch. decode-dimms doesn't recognize the 'spd5118'
> > driver.
> >
Looks good to me.
Spot-checking against JSED400-5B and the embedded CRC are as expected.
>
> Looking for feedback:
>
> [ ... ]
>
> > +
> > + nvmem = devm_nvmem_register(dev, &nvmem_config);
>
> This returns ERR_PTR(-EOPNOTSUPP) if CONFIG_NVRAM=n. We have two options:
>
> - Ignore -EOPNOTSUPP and continue registering the hwmon device
>
> or
>
> - Add
> select NVRAM
> select NVRAM_SYSFS
> to the driver's Kconfig entry.
s/NVRAM/NVMEM/g
> Any preferences ?
It seems reasonable to support the module without the eeprom logic.
When used in a fixed, embedded environment, the eeprom is of limited
value as it's known beforehand, while the hwmon functionality is still
useful.
EEPROM dump in case anyone wants it:
00000000: 3010 1203 0400 2062 0000 0000 b212 0d00 0..... b........
00000010: 0000 0000 6501 f203 7aaf 0000 0000 c837 ....e...z......7
00000020: c837 c837 906f 80bb 3075 2701 a000 8200 .7.7.o..0u'.....
00000030: 0000 0000 0000 d400 0000 d400 0000 d400 ................
00000040: 0000 d400 0000 8813 0888 1308 204e 2010 ............ N .
00000050: 2710 1534 2010 2710 c409 044c 1d0c 0000 '..4 .'....L....
00000060: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000070: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000080: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000090: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000000a0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000000b0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000000c0: 1000 8632 8015 8a8c 8213 0000 0000 0000 ...2............
000000d0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000000e0: 0000 0000 0000 0f11 0171 0822 0000 0000 .........q."....
000000f0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000100: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000110: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000120: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000130: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000140: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000150: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000160: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000170: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000180: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000190: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000001a0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000001b0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000001c0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000001d0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000001e0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000001f0: 0000 0000 0000 0000 0000 0000 0000 a14d ...............M
00000200: 0198 0823 328e 0a9b e84b 4635 3536 5334 ...#2....KF556S4
00000210: 302d 3332 2020 2020 2020 2020 2020 2020 0-32
00000220: 2020 2020 2020 2000 80ad 4100 0831 3030 ...A..100
00000230: 3139 3738 3700 0000 0000 0000 0000 0000 19787...........
00000240: 0000 4100 0000 0000 0001 0000 0000 0000 ..A.............
00000250: 0100 0000 0000 0000 0000 0000 0000 0000 ................
00000260: 0000 0001 0100 0000 0000 0000 0000 0088 ................
00000270: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000280: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000290: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000002a0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000002b0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000002c0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000002d0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000002e0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000002f0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000300: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000310: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000320: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000330: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000340: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000350: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000360: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000370: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000380: 0000 0000 0000 0000 0000 0000 0000 0000 ................
00000390: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000003a0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000003b0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000003c0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000003d0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000003e0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
000003f0: 0000 0000 0000 0000 0000 0000 0000 0000 ................
On 6/1/24 03:41, Thomas Weißschuh wrote:
> On 2024-05-31 22:42:24+0000, Guenter Roeck wrote:
>> On 5/31/24 16:05, Guenter Roeck wrote:
>>> Add support for reading SPD NVRAM data from SPD5118 (Jedec JESD300)
>>> compliant memory modules. NVRAM write operation is not supported.
>>>
>>> Signed-off-by: Guenter Roeck <[email protected]>
>>> ---
>>> v3: New patch
>>>
>>> RFT: I'd like to get some more test coverage before moving forward
>>> with this patch. decode-dimms doesn't recognize the 'spd5118'
>>> driver.
>>>
>
> Looks good to me.
>
> Spot-checking against JSED400-5B and the embedded CRC are as expected.
>
>>
>> Looking for feedback:
>>
>> [ ... ]
>>
>>> +
>>> + nvmem = devm_nvmem_register(dev, &nvmem_config);
>>
>> This returns ERR_PTR(-EOPNOTSUPP) if CONFIG_NVRAM=n. We have two options:
>>
>> - Ignore -EOPNOTSUPP and continue registering the hwmon device
>>
>> or
>>
>> - Add
>> select NVRAM
>> select NVRAM_SYSFS
>> to the driver's Kconfig entry.
>
> s/NVRAM/NVMEM/g
>
>> Any preferences ?
>
> It seems reasonable to support the module without the eeprom logic.
> When used in a fixed, embedded environment, the eeprom is of limited
> value as it's known beforehand, while the hwmon functionality is still
> useful.
>
Makes sense. Another question:
This:
+ struct nvmem_config nvmem_config = {
+ .type = NVMEM_TYPE_EEPROM,
+ .name = dev_name(dev),
+ .id = NVMEM_DEVID_AUTO,
results in:
$ ls /sys/bus/nvmem/devices
0-00501 0-00512 0-00523 0-00534 cmos_nvram0
^^^^^^^ ^^^^^^^ ^^^^^^^ ^^^^^^^
which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
which results in
$ ls /sys/bus/nvmem/devices
0-0050 0-0051 0-0052 0-0053 cmos_nvram0
We could also used fixed strings, but "spd" results in "spd[1-4]" which
I think would be a bit misleading since the DDR3/4 SPD data format is
different, and "spd5118" would result in "spd5118[1-4]" which again would
look odd. Any suggestions ?
Thanks,
Guenter
On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
<snip>
> Makes sense. Another question:
>
> This:
>
> + struct nvmem_config nvmem_config = {
> + .type = NVMEM_TYPE_EEPROM,
> + .name = dev_name(dev),
> + .id = NVMEM_DEVID_AUTO,
>
> results in:
>
> $ ls /sys/bus/nvmem/devices
> 0-00501 0-00512 0-00523 0-00534 cmos_nvram0
> ^^^^^^^ ^^^^^^^ ^^^^^^^ ^^^^^^^
>
> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
> which results in
>
> $ ls /sys/bus/nvmem/devices
> 0-0050 0-0051 0-0052 0-0053 cmos_nvram0
>
> We could also used fixed strings, but "spd" results in "spd[1-4]" which
> I think would be a bit misleading since the DDR3/4 SPD data format is
> different, and "spd5118" would result in "spd5118[1-4]" which again would
> look odd. Any suggestions ?
In order of descending, personal preference:
* spd-ddr5-[0-3] (.id = client->address - 0x50)
* spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
* Same with only "ddr5-"
* spd5118-[0-3]
* Your proposal from above
* nvmem[0-3] (default handling)
* 0-0050-[0-3]
Also can't a user of the eeprom already figure out which kind of module
it is by looking at the eeprom contents?
The first few bytes used for that seem to be compatible between at least
DDR4 and DDR5.
So using plain spd[1-4] could be enough.
On 01/06/2024 01:05, Guenter Roeck wrote:
> Add bindings for the SPD hub present in DDR5 modules.
> (https://www.jedec.org/standards-documents/docs/jesd300-5b01).
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> v3: Drop explicit bindings file; add binding to trivial devices
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
> On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
>
> <snip>
>
>> Makes sense. Another question:
>>
>> This:
>>
>> + struct nvmem_config nvmem_config = {
>> + .type = NVMEM_TYPE_EEPROM,
>> + .name = dev_name(dev),
>> + .id = NVMEM_DEVID_AUTO,
>>
>> results in:
>>
>> $ ls /sys/bus/nvmem/devices
>> 0-00501 0-00512 0-00523 0-00534 cmos_nvram0
>> ^^^^^^^ ^^^^^^^ ^^^^^^^ ^^^^^^^
>>
>> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
>> which results in
>>
>> $ ls /sys/bus/nvmem/devices
>> 0-0050 0-0051 0-0052 0-0053 cmos_nvram0
>>
>> We could also used fixed strings, but "spd" results in "spd[1-4]" which
>> I think would be a bit misleading since the DDR3/4 SPD data format is
>> different, and "spd5118" would result in "spd5118[1-4]" which again would
>> look odd. Any suggestions ?
> In order of descending, personal preference:
>
> * spd-ddr5-[0-3] (.id = client->address - 0x50)
Hi,
this will break as soon as more than 8 DDR5 DIMMs are installed.
> * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
> * Same with only "ddr5-"
> * spd5118-[0-3]
> * Your proposal from above
> * nvmem[0-3] (default handling)
> * 0-0050-[0-3]
>
> Also can't a user of the eeprom already figure out which kind of module
> it is by looking at the eeprom contents?
> The first few bytes used for that seem to be compatible between at least
> DDR4 and DDR5.
>
> So using plain spd[1-4] could be enough.
This could cause problems when DDR6 arrives.
Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.
Thanks,
Armin Wolf
On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
>
> > On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
> >
> > <snip>
> >
> > > Makes sense. Another question:
> > >
> > > This:
> > >
> > > + struct nvmem_config nvmem_config = {
> > > + .type = NVMEM_TYPE_EEPROM,
> > > + .name = dev_name(dev),
> > > + .id = NVMEM_DEVID_AUTO,
> > >
> > > results in:
> > >
> > > $ ls /sys/bus/nvmem/devices
> > > 0-00501 0-00512 0-00523 0-00534 cmos_nvram0
> > > ^^^^^^^ ^^^^^^^ ^^^^^^^ ^^^^^^^
> > >
> > > which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
> > > which results in
> > >
> > > $ ls /sys/bus/nvmem/devices
> > > 0-0050 0-0051 0-0052 0-0053 cmos_nvram0
> > >
> > > We could also used fixed strings, but "spd" results in "spd[1-4]" which
> > > I think would be a bit misleading since the DDR3/4 SPD data format is
> > > different, and "spd5118" would result in "spd5118[1-4]" which again would
> > > look odd. Any suggestions ?
> > In order of descending, personal preference:
> >
> > * spd-ddr5-[0-3] (.id = client->address - 0x50)
>
> Hi,
>
> this will break as soon as more than 8 DDR5 DIMMs are installed.
i2c_register_spd() only handles 8 DIMMs, too.
JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.
Outside of that range we could fall back to something else.
> > * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
> > * Same with only "ddr5-"
> > * spd5118-[0-3]
> > * Your proposal from above
> > * nvmem[0-3] (default handling)
> > * 0-0050-[0-3]
> >
> > Also can't a user of the eeprom already figure out which kind of module
> > it is by looking at the eeprom contents?
> > The first few bytes used for that seem to be compatible between at least
> > DDR4 and DDR5.
> >
> > So using plain spd[1-4] could be enough.
>
> This could cause problems when DDR6 arrives.
> Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.
I have the impression that the eeprom layouts are designed to be
forward and backward compatible.
If a non-DDR5-aware parser reads the contents of a DDR5 eeprom it will
fail the CRC check, so there can be no accidental misinterpretation.
(Because the CRC'ed area is larger and the CRC is at another location)
On the other hand the first bytes of DDR4 and DDR5 are compatible, so
even an unaware parser can recognize that a SPD eeprom is being read and
which DIMM type and specification revision it is.
This seems intentional and therefore should also hold true for DDR5 to DDR6.
Thomas
On 6/2/24 00:55, Thomas Weißschuh wrote:
> On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
>> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
>>
>>> On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
>>>
>>> <snip>
>>>
>>>> Makes sense. Another question:
>>>>
>>>> This:
>>>>
>>>> + struct nvmem_config nvmem_config = {
>>>> + .type = NVMEM_TYPE_EEPROM,
>>>> + .name = dev_name(dev),
>>>> + .id = NVMEM_DEVID_AUTO,
>>>>
>>>> results in:
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-00501 0-00512 0-00523 0-00534 cmos_nvram0
>>>> ^^^^^^^ ^^^^^^^ ^^^^^^^ ^^^^^^^
>>>>
>>>> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
>>>> which results in
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-0050 0-0051 0-0052 0-0053 cmos_nvram0
>>>>
>>>> We could also used fixed strings, but "spd" results in "spd[1-4]" which
>>>> I think would be a bit misleading since the DDR3/4 SPD data format is
>>>> different, and "spd5118" would result in "spd5118[1-4]" which again would
>>>> look odd. Any suggestions ?
>>> In order of descending, personal preference:
>>>
>>> * spd-ddr5-[0-3] (.id = client->address - 0x50)
>>
>> Hi,
>>
>> this will break as soon as more than 8 DDR5 DIMMs are installed.
>
> i2c_register_spd() only handles 8 DIMMs, too.
> JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.
>
> Outside of that range we could fall back to something else.
>
>>> * spd-ddr5-[0-3] (NVMEM_DEVID_AUTO)
>>> * Same with only "ddr5-"
>>> * spd5118-[0-3]
>>> * Your proposal from above
>>> * nvmem[0-3] (default handling)
>>> * 0-0050-[0-3]
>>>
>>> Also can't a user of the eeprom already figure out which kind of module
>>> it is by looking at the eeprom contents?
>>> The first few bytes used for that seem to be compatible between at least
>>> DDR4 and DDR5.
>>>
>>> So using plain spd[1-4] could be enough.
>>
>> This could cause problems when DDR6 arrives.
>> Personally i would prefer the spd5118-X (NVMEM_DEVID_AUTO) format.
>
> I have the impression that the eeprom layouts are designed to be
> forward and backward compatible.
>
> If a non-DDR5-aware parser reads the contents of a DDR5 eeprom it will
> fail the CRC check, so there can be no accidental misinterpretation.
> (Because the CRC'ed area is larger and the CRC is at another location)
>
> On the other hand the first bytes of DDR4 and DDR5 are compatible, so
> even an unaware parser can recognize that a SPD eeprom is being read and
> which DIMM type and specification revision it is.
>
> This seems intentional and therefore should also hold true for DDR5 to DDR6.
>
Looking into how this is handled by other drivers:
- at24 generates directories named {bus}-005{0-7}X, where X is from NVMEM_DEVID_AUTO.
Alternatively, it uses the 'label' devicetree property. In that case, the name
will be <label>X, with X again determined by NVMEM_DEVID_AUTO.
It does that to prevent duplicate file names due to duplicate labels.
- ee1004 does not use the nvmem subsystem, and thus there will be no
entries in /sys/bus/nvmem/devices/.
NVMEM_DEVID_AUTO counts up from 0, and affects every nvmem device. That means
the assigned ID is not fixed but simply reflects the n-th device using it,
in the order of registration. Effectively this means that any fixed name
plus NVMEM_DEVID_AUTO can not be associated with the originating device,
and there would be no guarantee that it was static (meaning it could change
from boot to boot). spd5118-X would not mean the Xth DIMM, it would be the
Xth device registering with the nvmem subsystem.
At the same time, something like spd5118-X, with X derived only from the
i2c address, would not work large systems because there could be DIMMs on
multiple I2C busses. X would have to derived from the bus number plus
the I2C address, such as '(bus << 3) | (address & 7)'. Even that would
not be static since the bus number could change from boot to boot
depending on the i2c bus instantiation order. It would also require extra
code since the name would have to be generated in the driver.
In the context of at24.c not really caring and ee1004.c not even using
the nvmem subsystem, I think it doesn't really matter how the nvmem device
subdirectories are named as long as they are unique. decode-dimms won't
use it, and I guess no one else really cares because, after all, ee1004
doesn't even support /sys/bus/nvmem/ in the first place and at24 uses the
odd/unusual form of {bus}-005{0-7}X.
Given all that, I'll just stick with the simple dev_name(dev) and
NVMEM_DEVID_NONE. That creates a clear association to i2c bus and address
without requiring extra code or a lengthy explanation how the index is
generated while at the same time guaranteeing uniqueness. I _could_ add
code to use the devicetree label if provided, but that would require using
the same mechanism as used by at24, i.e., we'd end up with {bus}-005{0-7}X
default names (after all, someone could provide duplicate labels or labels
such as "0-0050"). If that is deemed useful it could be added later.
Thanks,
Guenter
On 6/2/24 00:55, Thomas Weißschuh wrote:
> On 2024-06-01 21:23:24+0000, Armin Wolf wrote:
>> Am 01.06.24 um 16:08 schrieb Thomas Weißschuh:
>>
>>> On 2024-06-01 06:48:29+0000, Guenter Roeck wrote:
>>>
>>> <snip>
>>>
>>>> Makes sense. Another question:
>>>>
>>>> This:
>>>>
>>>> + struct nvmem_config nvmem_config = {
>>>> + .type = NVMEM_TYPE_EEPROM,
>>>> + .name = dev_name(dev),
>>>> + .id = NVMEM_DEVID_AUTO,
>>>>
>>>> results in:
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-00501 0-00512 0-00523 0-00534 cmos_nvram0
>>>> ^^^^^^^ ^^^^^^^ ^^^^^^^ ^^^^^^^
>>>>
>>>> which really doesn't look good. My current plan is to go with NVMEM_DEVID_NONE,
>>>> which results in
>>>>
>>>> $ ls /sys/bus/nvmem/devices
>>>> 0-0050 0-0051 0-0052 0-0053 cmos_nvram0
>>>>
>>>> We could also used fixed strings, but "spd" results in "spd[1-4]" which
>>>> I think would be a bit misleading since the DDR3/4 SPD data format is
>>>> different, and "spd5118" would result in "spd5118[1-4]" which again would
>>>> look odd. Any suggestions ?
>>> In order of descending, personal preference:
>>>
>>> * spd-ddr5-[0-3] (.id = client->address - 0x50)
>>
>> Hi,
>>
>> this will break as soon as more than 8 DDR5 DIMMs are installed.
>
> i2c_register_spd() only handles 8 DIMMs, too.
> JESD 300-5B.01 (section 2.6.5) also defines i2c addresses for 8 DIMMS only.
>
Agreed, but that doesn't mean that there must only be 8 DIMMs in the system.
It only means that there can only be up to 8 DIMMs on a single I2C bus.
i2c_register_spd() only exists if CONFIG_DMI is enabled. While that is supported
for multiple architectures, it is not supported on, for example, powerpc.
DDR5 support is not limited to systems supporting DMI, and/or to systems where
DMI is enabled. The ee1004 driver explicitly supports more than one i2c bus, and
I don't think it would be a good idea to limit DDR5 support to a single bus.
Ultimately that means that we can not rely on i2c_register_spd() for
auto-instantiation, even after adding DDR5 support to it.
Thanks,
Guenter