| Message ID | 20240618195348.1670547-3-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon: (spd5118) Various improvements | expand |
[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/
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
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
> 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 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
[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
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
[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
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
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 >
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
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
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
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");
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(-)