[RFT PATCH v2 0/3] hwmon: (spd5118) Various improvements

[RFT PATCH v2 0/3] hwmon: (spd5118) Various improvements

[Guenter Roeck]

The first patch of this series introduces multi-page support using the
regmap infrastructure. This simplifies the code and improves regmap caching.

The second patch introduces a spd5118-specific regmap bus to implement
SMBus accesses. This solves a problem seen with i801 I2C controllers
when writing to the chip. The I2C_FUNC_SMBUS_I2C_BLOCK support implemented
in those controllers does not work with spd5118 compatible chips, so
byte-by-byte access needs to be used explicitly.

The third patch adds support for spd5118 compatible chips which follow
the standard literally and block access to volatile registers if not
on page 0.

RFT: I was able to test the code on AMD systems using the piix4 I2C
controller. It needs testing with i801 controllers and with Renesas
chips.

v2: Added patches 1 and 2; simplified patch 3 to rely on regmap
    based paging.

----------------------------------------------------------------
Guenter Roeck (3):
      hwmon: (spd5118) Use regmap to implement paging
      hwmon: (spd5118) Use spd5118 specific read/write operations
      hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers

 drivers/hwmon/Kconfig   |   2 +-
 drivers/hwmon/spd5118.c | 131 +++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 102 insertions(+), 31 deletions(-)

[RFT PATCH v2 1/3] hwmon: (spd5118) Use regmap to implement paging

[Guenter Roeck]

Using regmap for paging significantly improves caching since the regmap
cache no longer needs to be cleared after changing the page, so let's
use it.

Suggested-by: Armin Wolf <W_Armin@gmx.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch

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

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index ac94a6779360..d405c5ef755d 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -382,45 +382,19 @@ static const struct hwmon_chip_info spd5118_chip_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 addr = (offset >> SPD5118_PAGE_SHIFT) * 0x100 + SPD5118_EEPROM_BASE;
 	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);
+	err = regmap_bulk_read(regmap, addr + offset, buf, count);
 	if (err)
 		return err;
 
@@ -515,13 +489,28 @@ static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static const struct regmap_range_cfg spd5118_regmap_range_cfg[] = {
+	{
+	.selector_reg   = SPD5118_REG_I2C_LEGACY_MODE,
+	.selector_mask  = SPD5118_LEGACY_PAGE_MASK,
+	.selector_shift = 0,
+	.window_start   = 0,
+	.window_len     = 0x100,
+	.range_min      = 0,
+	.range_max      = 0x7ff,
+	},
+};
+
 static const struct regmap_config spd5118_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
-	.max_register = 0xff,
+	.max_register = 0x7ff,
 	.writeable_reg = spd5118_writeable_reg,
 	.volatile_reg = spd5118_volatile_reg,
 	.cache_type = REGCACHE_MAPLE,
+
+	.ranges = spd5118_regmap_range_cfg,
+	.num_ranges = ARRAY_SIZE(spd5118_regmap_range_cfg),
 };
 
 static int spd5118_probe(struct i2c_client *client)
-- 
2.39.2

[RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Guenter Roeck]

regmap write operations on i801 controllers were observed to fail with
-ENXIO errors. It appears that the i801 controller and/or at least some
spd5118 compatible chips do not support the I2C_FUNC_SMBUS_I2C_BLOCK
operation used by the regmap-i2c core if I2C_FUNC_SMBUS_I2C_BLOCK is
supported by the I2C/SMBus controller.

Stop using the regmap-i2c core and always use basic SMBus operations
instead.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/linux-hwmon/33f369c1-1098-458e-9398-30037bd8c5aa@molgen.mpg.de/
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch

 drivers/hwmon/Kconfig   |  2 +-
 drivers/hwmon/spd5118.c | 28 +++++++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d5eced417fc3..fdfa778a965d 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2184,7 +2184,7 @@ config SENSORS_INA3221
 config SENSORS_SPD5118
 	tristate "SPD5118 Compliant Temperature Sensors"
 	depends on I2C
-	select REGMAP_I2C
+	select REGMAP
 	help
 	  If you say yes here you get support for SPD5118 (JEDEC JESD300)
 	  compliant temperature sensors. Such sensors are found on DDR5 memory
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index d405c5ef755d..995c45e2a997 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -489,6 +489,31 @@ static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static int spd5118_regmap_reg_read(void *context, unsigned int reg,
+				   unsigned int *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(context, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
+static int spd5118_regmap_reg_write(void *context, unsigned int reg,
+				    unsigned int val)
+{
+	return i2c_smbus_write_byte_data(context, reg, val);
+}
+
+static const struct regmap_bus spd5118_regmap_bus = {
+	.reg_write = spd5118_regmap_reg_write,
+	.reg_read = spd5118_regmap_reg_read,
+};
+
 static const struct regmap_range_cfg spd5118_regmap_range_cfg[] = {
 	{
 	.selector_reg   = SPD5118_REG_I2C_LEGACY_MODE,
@@ -526,7 +551,8 @@ static int spd5118_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
+	regmap = devm_regmap_init(dev, &spd5118_regmap_bus, client,
+				  &spd5118_regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
 
-- 
2.39.2

[RFT PATCH v2 3/3] hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers

[Guenter Roeck]

The SPD5118 specification says, in its documentation of the page bits
in the MR11 register:

"
This register only applies to non-volatile memory (1024) Bytes) access of
SPD5 Hub device.
For volatile memory access, this register must be programmed to '000'.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"

Renesas/ITD SPD5118 hub controllers take this literally and disable access
to volatile memory if the page selected in MR11 is != 0. Since the BIOS or
ROMMON will access the non-volatile memory and likely select a page != 0,
this means that the driver will not instantiate since it can not identify
the chip. Even if the driver instantiates, access to volatile registers
is blocked after a nvram read operation which selects a page other than 0.

To solve the problem, add initialization code to select page 0 during
probe. Before doing that, use basic validation to ensure that this is
really a SPD5118 device and not some random EEPROM.

Cc: Sasha Kozachuk <skozachuk@google.com>
Cc: John Hamrick <johnham@google.com>
Cc: Chris Sarra <chrissarra@google.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Simplified to use mechanisms introduced in the first two patches
    of the series.

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

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 995c45e2a997..80a02dba2ccc 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -538,6 +538,58 @@ static const struct regmap_config spd5118_regmap_config = {
 	.num_ranges = ARRAY_SIZE(spd5118_regmap_range_cfg),
 };
 
+static int spd5118_init(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int err, regval, mode;
+
+	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 < 0 || (regval && regval != 0x5118))
+		return -ENODEV;
+
+	/*
+	 * If the device type registers return 0, it is possible that the chip
+	 * has a non-zero page selected and takes the specification literally,
+	 * i.e. disables access to volatile registers besides the page register
+	 * if the page is not 0. Try to identify such chips.
+	 */
+	if (!regval) {
+		/* Vendor ID registers must also be 0 */
+		regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
+		if (regval)
+			return -ENODEV;
+
+		/* The selected page in MR11 must not be 0 */
+		mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
+		if (mode < 0 || (mode & ~SPD5118_LEGACY_MODE_MASK) ||
+		    !(mode & SPD5118_LEGACY_PAGE_MASK))
+			return -ENODEV;
+
+		err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE,
+						mode & SPD5118_LEGACY_MODE_ADDR);
+		if (err)
+			return -ENODEV;
+
+		/*
+		 * If the device type registers are still bad after selecting
+		 * page 0, this is not a SPD5118 device. Restore original
+		 * legacy mode register value and abort.
+		 */
+		regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+		if (regval != 0x5118) {
+			i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
+			return -ENODEV;
+		}
+	}
+
+	/* We are reasonably sure that this is really a SPD5118 hub controller */
+	return 0;
+}
+
 static int spd5118_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -547,6 +599,10 @@ static int spd5118_probe(struct i2c_client *client)
 	struct regmap *regmap;
 	int err;
 
+	err = spd5118_init(client);
+	if (err)
+		return err;
+
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.39.2

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Paul Menzel]

[continue tests from thread *Re: [PATCH v4 5/6] i2c: smbus: Support DDR5 
SPD EEPROMs* [1] here]

[Cc: +Jean Delvare]

Dear Guenter,


Am 18.06.24 um 21:53 schrieb Guenter Roeck:
> regmap write operations on i801 controllers were observed to fail with
> -ENXIO errors. It appears that the i801 controller and/or at least some
> spd5118 compatible chips do not support the I2C_FUNC_SMBUS_I2C_BLOCK
> operation used by the regmap-i2c core if I2C_FUNC_SMBUS_I2C_BLOCK is
> supported by the I2C/SMBus controller.
> 
> Stop using the regmap-i2c core and always use basic SMBus operations
> instead.
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/linux-hwmon/33f369c1-1098-458e-9398-30037bd8c5aa@molgen.mpg.de/
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Added patch
> 
>   drivers/hwmon/Kconfig   |  2 +-
>   drivers/hwmon/spd5118.c | 28 +++++++++++++++++++++++++++-
>   2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d5eced417fc3..fdfa778a965d 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2184,7 +2184,7 @@ config SENSORS_INA3221
>   config SENSORS_SPD5118
>   	tristate "SPD5118 Compliant Temperature Sensors"
>   	depends on I2C
> -	select REGMAP_I2C
> +	select REGMAP
>   	help
>   	  If you say yes here you get support for SPD5118 (JEDEC JESD300)
>   	  compliant temperature sensors. Such sensors are found on DDR5 memory
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index d405c5ef755d..995c45e2a997 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -489,6 +489,31 @@ static bool spd5118_volatile_reg(struct device *dev, unsigned int reg)
>   	}
>   }
>   
> +static int spd5118_regmap_reg_read(void *context, unsigned int reg,
> +				   unsigned int *val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(context, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int spd5118_regmap_reg_write(void *context, unsigned int reg,
> +				    unsigned int val)
> +{
> +	return i2c_smbus_write_byte_data(context, reg, val);
> +}
> +
> +static const struct regmap_bus spd5118_regmap_bus = {
> +	.reg_write = spd5118_regmap_reg_write,
> +	.reg_read = spd5118_regmap_reg_read,
> +};
> +
>   static const struct regmap_range_cfg spd5118_regmap_range_cfg[] = {
>   	{
>   	.selector_reg   = SPD5118_REG_I2C_LEGACY_MODE,
> @@ -526,7 +551,8 @@ static int spd5118_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> -	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
> +	regmap = devm_regmap_init(dev, &spd5118_regmap_bus, client,
> +				  &spd5118_regmap_config);
>   	if (IS_ERR(regmap))
>   		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
>   

Unfortunately, it still fails:

     $ git log --no-decorate --oneline -4
     7ddcff2d44ae3 hwmon: (spd5118) Add support for Renesas/ITD SPD5118 
hub controllers
     e89136743324f hwmon: (spd5118) Use spd5118 specific read/write 
operations
     0fcc7279f0cc4 hwmon: (spd5118) Use regmap to implement paging
     801b6aad6fa7a hwmon: (spd5118) Add configuration option for 
auto-detection

     $ uname -r
     6.10.0-rc4.mx64.461-00050-g7ddcff2d44ae
     $ sudo bash -c 'echo 56000 > /sys/class/hwmon/hwmon3/temp1_max'
     bash: line 1: echo: write error: No such device or address


Kind regards,

Paul


[1]: 
https://lore.kernel.org/all/342dae24-56c5-4b81-9591-dc23ddbb2806@roeck-us.net/

Re: [RFT PATCH v2 0/3] hwmon: (spd5118) Various improvements

[Armin Wolf]

Am 18.06.24 um 21:53 schrieb Guenter Roeck:

> The first patch of this series introduces multi-page support using the
> regmap infrastructure. This simplifies the code and improves regmap caching.
>
> The second patch introduces a spd5118-specific regmap bus to implement
> SMBus accesses. This solves a problem seen with i801 I2C controllers
> when writing to the chip. The I2C_FUNC_SMBUS_I2C_BLOCK support implemented
> in those controllers does not work with spd5118 compatible chips, so
> byte-by-byte access needs to be used explicitly.
>
> The third patch adds support for spd5118 compatible chips which follow
> the standard literally and block access to volatile registers if not
> on page 0.
>
> RFT: I was able to test the code on AMD systems using the piix4 I2C
> controller. It needs testing with i801 controllers and with Renesas
> chips.
>
> v2: Added patches 1 and 2; simplified patch 3 to rely on regmap
>      based paging.

Hi,

i also have an AMD system and DRAM chips without the Renesas controller, but still:

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

> ----------------------------------------------------------------
> Guenter Roeck (3):
>        hwmon: (spd5118) Use regmap to implement paging
>        hwmon: (spd5118) Use spd5118 specific read/write operations
>        hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers
>
>   drivers/hwmon/Kconfig   |   2 +-
>   drivers/hwmon/spd5118.c | 131 +++++++++++++++++++++++++++++++++++++-----------
>   2 files changed, 102 insertions(+), 31 deletions(-)

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Guenter Roeck]

Hi Paul,

On 6/18/24 13:37, Paul Menzel wrote:
[ ... ]
> Unfortunately, it still fails:
> 
>      $ git log --no-decorate --oneline -4
>      7ddcff2d44ae3 hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers
>      e89136743324f hwmon: (spd5118) Use spd5118 specific read/write operations
>      0fcc7279f0cc4 hwmon: (spd5118) Use regmap to implement paging
>      801b6aad6fa7a hwmon: (spd5118) Add configuration option for auto-detection
> 
>      $ uname -r
>      6.10.0-rc4.mx64.461-00050-g7ddcff2d44ae
>      $ sudo bash -c 'echo 56000 > /sys/class/hwmon/hwmon3/temp1_max'
>      bash: line 1: echo: write error: No such device or address
> 

Now I am really baffled. I don't think we could do anything simpler
than that.

Please try
	sudo i2cset -y -f 0 0x50 0x21 0x06

That should update the critical temperature from 85 degrees C
to 86 degrees C. If that doesn't work, we'll be really out of luck
with that controller (or at least I don't have an idea what else to try).

Thanks,
Guenter

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Paul Menzel]

Dear Guenter,


Am 18.06.24 um 23:08 schrieb Guenter Roeck:

> On 6/18/24 13:37, Paul Menzel wrote:
> [ ... ]
>> Unfortunately, it still fails:
>>
>>      $ git log --no-decorate --oneline -4
>>      7ddcff2d44ae3 hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers
>>      e89136743324f hwmon: (spd5118) Use spd5118 specific read/write operations
>>      0fcc7279f0cc4 hwmon: (spd5118) Use regmap to implement paging
>>      801b6aad6fa7a hwmon: (spd5118) Add configuration option for auto-detection
>>
>>      $ uname -r
>>      6.10.0-rc4.mx64.461-00050-g7ddcff2d44ae
>>      $ sudo bash -c 'echo 56000 > /sys/class/hwmon/hwmon3/temp1_max'
>>      bash: line 1: echo: write error: No such device or address
> 
> Now I am really baffled. I don't think we could do anything simpler
> than that.
> 
> Please try
>      sudo i2cset -y -f 0 0x50 0x21 0x06
> 
> That should update the critical temperature from 85 degrees C
> to 86 degrees C. If that doesn't work, we'll be really out of luck
> with that controller (or at least I don't have an idea what else to try).

Bad news:

     $ sudo LD_LIBRARY_PATH=~/src/i2c-tools/lib tools/i2cset -y -f 0 
0x50 0x21 0x06
     Error: Write failed


Kind regards,

Paul

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Wolfram Sang]

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


> to 86 degrees C. If that doesn't work, we'll be really out of luck
> with that controller (or at least I don't have an idea what else to try).

Try CCing Heiner Kallweit for ideas about the i801 controller.


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

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Guenter Roeck]

Hi Paul,

On 6/18/24 14:45, Paul Menzel wrote:
> Dear Guenter,
> 
> 
> Am 18.06.24 um 23:08 schrieb Guenter Roeck:
> 
>> On 6/18/24 13:37, Paul Menzel wrote:
>> [ ... ]
>>> Unfortunately, it still fails:
>>>
>>>      $ git log --no-decorate --oneline -4
>>>      7ddcff2d44ae3 hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers
>>>      e89136743324f hwmon: (spd5118) Use spd5118 specific read/write operations
>>>      0fcc7279f0cc4 hwmon: (spd5118) Use regmap to implement paging
>>>      801b6aad6fa7a hwmon: (spd5118) Add configuration option for auto-detection
>>>
>>>      $ uname -r
>>>      6.10.0-rc4.mx64.461-00050-g7ddcff2d44ae
>>>      $ sudo bash -c 'echo 56000 > /sys/class/hwmon/hwmon3/temp1_max'
>>>      bash: line 1: echo: write error: No such device or address
>>
>> Now I am really baffled. I don't think we could do anything simpler
>> than that.
>>
>> Please try
>>      sudo i2cset -y -f 0 0x50 0x21 0x06
>>
>> That should update the critical temperature from 85 degrees C
>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>> with that controller (or at least I don't have an idea what else to try).
> 
> Bad news:
> 
>      $ sudo LD_LIBRARY_PATH=~/src/i2c-tools/lib tools/i2cset -y -f 0 0x50 0x21 0x06
>      Error: Write failed
> 

I wonder if there is some write protect active in your system. I don't see anything
in the spd5118 specification, though, or in the i801 datasheet. I really don't know
what else we could try, sorry. Unfortunately I don't have a system with Intel CPU,
much less one with DDR5, so I won't be able to play with this myself and/or
determine if there is something special with your system, or if this is a generic
problem with i801 controllers.

Let's hope that we get some feedback from others.

Thanks,
Guenter

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Paul Menzel]

[Cc: +Heiner Kallweit]


Dear Guenter,


Thank you for your support and trying all these things without having 
such a system at hand.

Am 19.06.24 um 00:37 schrieb Guenter Roeck:

> On 6/18/24 14:45, Paul Menzel wrote:

>> Am 18.06.24 um 23:08 schrieb Guenter Roeck:
>>
>>> On 6/18/24 13:37, Paul Menzel wrote:
>>> [ ... ]
>>>> Unfortunately, it still fails:
>>>>
>>>>      $ git log --no-decorate --oneline -4
>>>>      7ddcff2d44ae3 hwmon: (spd5118) Add support for Renesas/ITD SPD5118 hub controllers
>>>>      e89136743324f hwmon: (spd5118) Use spd5118 specific read/write operations
>>>>      0fcc7279f0cc4 hwmon: (spd5118) Use regmap to implement paging
>>>>      801b6aad6fa7a hwmon: (spd5118) Add configuration option for auto-detection
>>>>
>>>>      $ uname -r
>>>>      6.10.0-rc4.mx64.461-00050-g7ddcff2d44ae
>>>>      $ sudo bash -c 'echo 56000 > /sys/class/hwmon/hwmon3/temp1_max'
>>>>      bash: line 1: echo: write error: No such device or address
>>>
>>> Now I am really baffled. I don't think we could do anything simpler
>>> than that.
>>>
>>> Please try
>>>      sudo i2cset -y -f 0 0x50 0x21 0x06
>>>
>>> That should update the critical temperature from 85 degrees C
>>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>>> with that controller (or at least I don't have an idea what else to 
>>> try).
>>
>> Bad news:
>>
>>      $ sudo LD_LIBRARY_PATH=~/src/i2c-tools/lib tools/i2cset -y -f 0 0x50 0x21 0x06
>>      Error: Write failed
> 
> I wonder if there is some write protect active in your system.

Not that I know of.

> I don't see anything in the spd5118 specification, though, or in the
> i801 datasheet. I really don't know what else we could try, sorry.
> Unfortunately I don't have a system with Intel CPU, much less one
> with DDR5, so I won't be able to play with this myself and/or 
> determine if there is something special with your system, or if this
> is a generic problem with i801 controllers.
> 
> Let's hope that we get some feedback from others.

Thank you and kind regards,

Paul

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Armin Wolf]

Am 19.06.24 um 00:28 schrieb Wolfram Sang:

>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>> with that controller (or at least I don't have an idea what else to try).
> Try CCing Heiner Kallweit for ideas about the i801 controller.
>
Hi,

i am not Heiner Kallweit, but i found something interesting in
commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").

Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit
which blocks all writes for slave addresses 0x50-0x57.

Does the i801 i2c controller driver print something like "SPD Write Disable is set" during
boot?

Thanks,
Armin Wolf

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Paul Menzel]

[Cc: +Heiner]


Dear Armin,


Am 19.06.24 um 01:28 schrieb Armin Wolf:
> Am 19.06.24 um 00:28 schrieb Wolfram Sang:
> 
>>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>>> with that controller (or at least I don't have an idea what else to 
>>> try).
>> 
>> Try CCing Heiner Kallweit for ideas about the i801 controller.

> i am not Heiner Kallweit, but i found something interesting in
> commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and 
> later").
> 
> Basically, it seems that the i802 i2c controller indeed features a SPD 
> write disable bit which blocks all writes for slave addresses 0x50-0x57.
> 
> Does the i801 i2c controller driver print something like "SPD Write 
> Disable is set" during boot?

Nice find. Yes, it does:

     [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
     [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt


Kind regards,

Paul

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Guenter Roeck]

On 6/18/24 16:39, Paul Menzel wrote:
> [Cc: +Heiner]
> 
> 
> Dear Armin,
> 
> 
> Am 19.06.24 um 01:28 schrieb Armin Wolf:
>> Am 19.06.24 um 00:28 schrieb Wolfram Sang:
>>
>>>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>>>> with that controller (or at least I don't have an idea what else to try).
>>>
>>> Try CCing Heiner Kallweit for ideas about the i801 controller.
> 
>> i am not Heiner Kallweit, but i found something interesting in
>> commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").
>>
>> Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit which blocks all writes for slave addresses 0x50-0x57.
>>
>> Does the i801 i2c controller driver print something like "SPD Write Disable is set" during boot?
> 
> Nice find. Yes, it does:
> 

Yes, definitely. I didn't have any recent datasheets, so I missed that flag.
Oh well :-(.

>      [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
>      [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
> 

Bummer. That explains the problem. It means that the BIOS effectively
blocks reading the eeprom on your system (because that would require writing
the page register), as well as changing temperature limits. That is really
annoying, but there is nothing we can do about it. Maybe the BIOS has a
configuration flag to enable or disable write protect, but I doubt it.

I'll drop this patch from the series.

Thanks,
Guenter

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Thomas Weißschuh]

On 2024-06-18 17:23:44+0000, Guenter Roeck wrote:
> On 6/18/24 16:39, Paul Menzel wrote:
> > [Cc: +Heiner]
> > 
> > 
> > Dear Armin,
> > 
> > 
> > Am 19.06.24 um 01:28 schrieb Armin Wolf:
> > > Am 19.06.24 um 00:28 schrieb Wolfram Sang:
> > > 
> > > > > to 86 degrees C. If that doesn't work, we'll be really out of luck
> > > > > with that controller (or at least I don't have an idea what else to try).
> > > > 
> > > > Try CCing Heiner Kallweit for ideas about the i801 controller.
> > 
> > > i am not Heiner Kallweit, but i found something interesting in
> > > commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").
> > > 
> > > Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit which blocks all writes for slave addresses 0x50-0x57.
> > > 
> > > Does the i801 i2c controller driver print something like "SPD Write Disable is set" during boot?
> > 
> > Nice find. Yes, it does:
> > 
> 
> Yes, definitely. I didn't have any recent datasheets, so I missed that flag.
> Oh well :-(.
> 
> >      [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
> >      [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
> > 
> 
> Bummer. That explains the problem. It means that the BIOS effectively
> blocks reading the eeprom on your system (because that would require writing
> the page register), as well as changing temperature limits. That is really
> annoying, but there is nothing we can do about it. Maybe the BIOS has a
> configuration flag to enable or disable write protect, but I doubt it.

What about using 16bit addressing mode?

    Alternatively, at initial power on, the host can set the Table 112, “MR11” [3] = ‘1’ to address the entire 1024 bytes of
    non-volatile memory with 2 bytes of address and hence not required to go through page selection to address entire
    non-volatile memory.

regmap-i2c allows 16bit addresses when I2C_FUNC_SMBUS_I2C_BLOCK is supported,
which to me looks like it should be the case on i801 for ICH5.

piix4 doesn't though.

> I'll drop this patch from the series.
> 
> Thanks,
> Guenter
> 

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Guenter Roeck]

On 6/18/24 17:50, Thomas Weißschuh wrote:
> On 2024-06-18 17:23:44+0000, Guenter Roeck wrote:
>> On 6/18/24 16:39, Paul Menzel wrote:
>>> [Cc: +Heiner]
>>>
>>>
>>> Dear Armin,
>>>
>>>
>>> Am 19.06.24 um 01:28 schrieb Armin Wolf:
>>>> Am 19.06.24 um 00:28 schrieb Wolfram Sang:
>>>>
>>>>>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>>>>>> with that controller (or at least I don't have an idea what else to try).
>>>>>
>>>>> Try CCing Heiner Kallweit for ideas about the i801 controller.
>>>
>>>> i am not Heiner Kallweit, but i found something interesting in
>>>> commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").
>>>>
>>>> Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit which blocks all writes for slave addresses 0x50-0x57.
>>>>
>>>> Does the i801 i2c controller driver print something like "SPD Write Disable is set" during boot?
>>>
>>> Nice find. Yes, it does:
>>>
>>
>> Yes, definitely. I didn't have any recent datasheets, so I missed that flag.
>> Oh well :-(.
>>
>>>       [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
>>>       [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
>>>
>>
>> Bummer. That explains the problem. It means that the BIOS effectively
>> blocks reading the eeprom on your system (because that would require writing
>> the page register), as well as changing temperature limits. That is really
>> annoying, but there is nothing we can do about it. Maybe the BIOS has a
>> configuration flag to enable or disable write protect, but I doubt it.
> 
> What about using 16bit addressing mode?
> 
>      Alternatively, at initial power on, the host can set the Table 112, “MR11” [3] = ‘1’ to address the entire 1024 bytes of
>      non-volatile memory with 2 bytes of address and hence not required to go through page selection to address entire
>      non-volatile memory.
> 
> regmap-i2c allows 16bit addresses when I2C_FUNC_SMBUS_I2C_BLOCK is supported,
> which to me looks like it should be the case on i801 for ICH5.
> 

Good idea, but it doesn't work. I can get write operations with
16-bit register addresses to work even on piix4, but read operations
require writing a 16-bit register address followed by byte reads (see
regmap_i2c_smbus_i2c_read_reg16). Unfortunately, spd5118 devices
don't auto-increment the address on byte read operations, meaning
each byte read returns data from address 0x00 (i.e., it returns
0x51). Try "i2cdump -y -f 0 0x50 c" and you'll see what I mean.
Maybe there is a way around it, but I have not found it.

On top of that, configuring 16-bit mode requires a write operation
into the page register, and that is blocked.

Thanks,
Guenter

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Thomas Weißschuh]

On 2024-06-18 18:02:51+0000, Guenter Roeck wrote:
> On 6/18/24 17:50, Thomas Weißschuh wrote:
> > On 2024-06-18 17:23:44+0000, Guenter Roeck wrote:
> > > On 6/18/24 16:39, Paul Menzel wrote:
> > > > [Cc: +Heiner]
> > > > 
> > > > 
> > > > Dear Armin,
> > > > 
> > > > 
> > > > Am 19.06.24 um 01:28 schrieb Armin Wolf:
> > > > > Am 19.06.24 um 00:28 schrieb Wolfram Sang:
> > > > > 
> > > > > > > to 86 degrees C. If that doesn't work, we'll be really out of luck
> > > > > > > with that controller (or at least I don't have an idea what else to try).
> > > > > > 
> > > > > > Try CCing Heiner Kallweit for ideas about the i801 controller.
> > > > 
> > > > > i am not Heiner Kallweit, but i found something interesting in
> > > > > commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").
> > > > > 
> > > > > Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit which blocks all writes for slave addresses 0x50-0x57.
> > > > > 
> > > > > Does the i801 i2c controller driver print something like "SPD Write Disable is set" during boot?
> > > > 
> > > > Nice find. Yes, it does:
> > > > 
> > > 
> > > Yes, definitely. I didn't have any recent datasheets, so I missed that flag.
> > > Oh well :-(.
> > > 
> > > >       [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
> > > >       [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
> > > > 
> > > 
> > > Bummer. That explains the problem. It means that the BIOS effectively
> > > blocks reading the eeprom on your system (because that would require writing
> > > the page register), as well as changing temperature limits. That is really
> > > annoying, but there is nothing we can do about it. Maybe the BIOS has a
> > > configuration flag to enable or disable write protect, but I doubt it.
> > 
> > What about using 16bit addressing mode?
> > 
> >      Alternatively, at initial power on, the host can set the Table 112, “MR11” [3] = ‘1’ to address the entire 1024 bytes of
> >      non-volatile memory with 2 bytes of address and hence not required to go through page selection to address entire
> >      non-volatile memory.
> > 
> > regmap-i2c allows 16bit addresses when I2C_FUNC_SMBUS_I2C_BLOCK is supported,
> > which to me looks like it should be the case on i801 for ICH5.
> > 
> 
> Good idea, but it doesn't work. I can get write operations with
> 16-bit register addresses to work even on piix4, but read operations
> require writing a 16-bit register address followed by byte reads (see
> regmap_i2c_smbus_i2c_read_reg16). Unfortunately, spd5118 devices
> don't auto-increment the address on byte read operations, meaning
> each byte read returns data from address 0x00 (i.e., it returns
> 0x51). Try "i2cdump -y -f 0 0x50 c" and you'll see what I mean.
> Maybe there is a way around it, but I have not found it.

Thanks for the pointer to regmap_i2c_smbus_i2c_read_reg16().
I'm not really familiar with I2C/SMBUS ...

Did you look into "2.6.8.3 Default Read Address Pointer Mode"?

I am failing to understand how that address pointer mode would ever make
sense without address auto-increment.

> On top of that, configuring 16-bit mode requires a write operation
> into the page register, and that is blocked.

... this one on the other hand is really obvious.


Thanks,
Thomas

Re: [RFT PATCH v2 2/3] hwmon: (spd5118) Use spd5118 specific read/write operations

[Guenter Roeck]

On 6/19/24 02:13, Thomas Weißschuh wrote:
> On 2024-06-18 18:02:51+0000, Guenter Roeck wrote:
>> On 6/18/24 17:50, Thomas Weißschuh wrote:
>>> On 2024-06-18 17:23:44+0000, Guenter Roeck wrote:
>>>> On 6/18/24 16:39, Paul Menzel wrote:
>>>>> [Cc: +Heiner]
>>>>>
>>>>>
>>>>> Dear Armin,
>>>>>
>>>>>
>>>>> Am 19.06.24 um 01:28 schrieb Armin Wolf:
>>>>>> Am 19.06.24 um 00:28 schrieb Wolfram Sang:
>>>>>>
>>>>>>>> to 86 degrees C. If that doesn't work, we'll be really out of luck
>>>>>>>> with that controller (or at least I don't have an idea what else to try).
>>>>>>>
>>>>>>> Try CCing Heiner Kallweit for ideas about the i801 controller.
>>>>>
>>>>>> i am not Heiner Kallweit, but i found something interesting in
>>>>>> commit ba9ad2af7019 ("i2c: i801: Fix I2C Block Read on 8-Series/C220 and later").
>>>>>>
>>>>>> Basically, it seems that the i802 i2c controller indeed features a SPD write disable bit which blocks all writes for slave addresses 0x50-0x57.
>>>>>>
>>>>>> Does the i801 i2c controller driver print something like "SPD Write Disable is set" during boot?
>>>>>
>>>>> Nice find. Yes, it does:
>>>>>
>>>>
>>>> Yes, definitely. I didn't have any recent datasheets, so I missed that flag.
>>>> Oh well :-(.
>>>>
>>>>>        [    5.462605] i801_smbus 0000:00:1f.4: SPD Write Disable is set
>>>>>        [    5.468399] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
>>>>>
>>>>
>>>> Bummer. That explains the problem. It means that the BIOS effectively
>>>> blocks reading the eeprom on your system (because that would require writing
>>>> the page register), as well as changing temperature limits. That is really
>>>> annoying, but there is nothing we can do about it. Maybe the BIOS has a
>>>> configuration flag to enable or disable write protect, but I doubt it.
>>>
>>> What about using 16bit addressing mode?
>>>
>>>       Alternatively, at initial power on, the host can set the Table 112, “MR11” [3] = ‘1’ to address the entire 1024 bytes of
>>>       non-volatile memory with 2 bytes of address and hence not required to go through page selection to address entire
>>>       non-volatile memory.
>>>
>>> regmap-i2c allows 16bit addresses when I2C_FUNC_SMBUS_I2C_BLOCK is supported,
>>> which to me looks like it should be the case on i801 for ICH5.
>>>
>>
>> Good idea, but it doesn't work. I can get write operations with
>> 16-bit register addresses to work even on piix4, but read operations
>> require writing a 16-bit register address followed by byte reads (see
>> regmap_i2c_smbus_i2c_read_reg16). Unfortunately, spd5118 devices
>> don't auto-increment the address on byte read operations, meaning
>> each byte read returns data from address 0x00 (i.e., it returns
>> 0x51). Try "i2cdump -y -f 0 0x50 c" and you'll see what I mean.
>> Maybe there is a way around it, but I have not found it.
> 
> Thanks for the pointer to regmap_i2c_smbus_i2c_read_reg16().
> I'm not really familiar with I2C/SMBUS ...
> 
> Did you look into "2.6.8.3 Default Read Address Pointer Mode"?
> 
Yes, I did, and, yes, setting that for each planned read operation
might do the trick, but even that would not work here since writes
are blocked. On top of that, it would be extremely expensive (one
would have to write the address into the default address registers
before starting a read). The reason to use 16-bit access mode would
be to simplify access, not to make it more expensive.

> I am failing to understand how that address pointer mode would ever make
> sense without address auto-increment.

Oh, it kind of does, as long as each access is a single i2c operation.
One would have to use a SMBus read word operation to read two bytes
with a single SMBus command. I guess one could also use something similar
to an i2c block operation - start a read with no command byte and just
keep going.

> 
>> On top of that, configuring 16-bit mode requires a write operation
>> into the page register, and that is blocked.
> 
> ... this one on the other hand is really obvious.
> 

There is actually another problem: When I tried enabling 16-bit mode
in my system, I initially had trouble clearing it. When I rebooted,
I got a BIOS error telling me that the configuration changed, and
it gave me the option to either enter setup or continue. A soft
reset did not clear the bit. Power cycle did, but I got the
"configuration changed" message again.

So even if we would get 16-bit mode to work, it would not be a good idea
because it would expose people to the "configuration changed" BIOS
message. Resetting the bit on shutdown and when unloading the driver
would not help because that would not happen when the system crashes.

So, in summary, 16-bit mode is just not usable. If some BIOS actually
enables it, we might have to disable it or figure out how to use it
without depending on "Default Read Address Pointer Mode".

Thanks,
Guenter