i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
PATCH v2 1/1 i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
Starting with the 8-Series/C220 PCH (Lynx Point), the SMBus controller includes a SPD EEPROM protection mechanism. Once the SPD Write Disable bit is set, only reads are allowed to slave addresses 0x50-0x57.
However the legacy implementation of I2C Block Read since the ICH5 looks like a write, and is therefore blocked by the SPD protection mechanism. This causes the eeprom and at24 drivers to fail.
So assume that I2C Block Read is implemented as an actual read on these chipsets. I tested it on my Q87 chipset and it seems to work just fine.
---
Changes since v1:
* Rebased on Linus' latest tree.
Jarkko, still no information about this from your Windows or hardware folks?
drivers/i2c/busses/i2c-i801.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-i801.c +13 -3
@@ -146,6 +146,7 @@#define SMBHSTCFG_HST_EN BIT(0)#define SMBHSTCFG_SMB_SMI_EN BIT(1)#define SMBHSTCFG_I2C_EN BIT(2) +#define SMBHSTCFG_SPD_WD BIT(4)/* TCO configuration bits for TCOCTL */ #define TCOCTL_EN BIT(8)
@@ -871,9 +872,16 @@ static s32 i801_access(struct i2c_adapte block =1 ;break ;case I2C_SMBUS_I2C_BLOCK_DATA : -/* NB: page 240 of ICH5 datasheet shows that the R/#W -* bit should be cleared here, even when reading */ -outb_p ((addr &0x7f ) <<1 ,SMBHSTADD (priv)); +/* +* NB: page 240 of ICH5 datasheet shows that the R/#W +* bit should be cleared here, even when reading. +* However if SPD Write Disable is set (Lynx Point and later), +* the read will fail if we don't set the R/#W bit. +*/ +outb_p (((addr &0x7f ) <<1 ) | + ((priv->original_hstcfg &SMBHSTCFG_SPD_WD ) ? + (read_write &0x01 ) :0 ), +SMBHSTADD (priv));if (read_write ==I2C_SMBUS_READ ) {/* NB: page 240 of ICH5 datasheet also shows * that DATA1 is the cmd field when reading */
@@ -1593,6 +1601,8 @@ static int i801_probe(struct pci_dev *de/* Disable SMBus interrupt feature if SMBus using SMI# */ priv->features &= ~FEATURE_IRQ ; } +if (temp &SMBHSTCFG_SPD_WD ) +dev_info (&dev->dev,"SPD Write Disable is set\n" );/* Clear special mode bits */ if (priv->features & (FEATURE_SMBUS_PEC |FEATURE_BLOCK_BUFFER ))
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 17 lines · Jean Delvare
Starting with the 8-Series/C220 PCH (Lynx Point), the SMBus controller includes a SPD EEPROM protection mechanism. Once the SPD Write Disable bit is set, only reads are allowed to slave addresses 0x50-0x57.
However the legacy implementation of I2C Block Read since the ICH5 looks like a write, and is therefore blocked by the SPD protection mechanism. This causes the eeprom and at24 drivers to fail.
So assume that I2C Block Read is implemented as an actual read on these chipsets. I tested it on my Q87 chipset and it seems to work just fine.
Signed-off-by Jean Delvare <[email protected]>Cc Jarkko Nikula <[email protected]>Cc Mika Westerberg <[email protected]>Cc Wolfram Sang <[email protected]>
Quoted 7 lines
---
Changes since v1:
* Rebased on Linus' latest tree.Jarkko, still no information about this from your Windows or hardware folks?
No update, in fact some of the contacts have left the company :-(
Signature · 1 line
Jarkko
Hi Jarkko,
Quoted 22 lines · Jarkko Nikula
Quoted 21 lines · Jean Delvare
Starting with the 8-Series/C220 PCH (Lynx Point), the SMBus controller includes a SPD EEPROM protection mechanism. Once the SPD Write Disable bit is set, only reads are allowed to slave addresses 0x50-0x57.
However the legacy implementation of I2C Block Read since the ICH5 looks like a write, and is therefore blocked by the SPD protection mechanism. This causes the eeprom and at24 drivers to fail.
So assume that I2C Block Read is implemented as an actual read on these chipsets. I tested it on my Q87 chipset and it seems to work just fine.
Signed-off-by Jean Delvare <[email protected]>Cc Jarkko Nikula <[email protected]>Cc Mika Westerberg <[email protected]>Cc Wolfram Sang <[email protected]>---
Changes since v1:
* Rebased on Linus' latest tree.
Quoted 4 lines
Quoted 2 lines
Jarkko, still no information about this from your Windows or hardware folks?
No update, in fact some of the contacts have left the company :-(
What do we do then? Without this patch, I2C Block Read doesn't work at all on recent chipsets. I don't think it can be worse with the patch. And it works fine on the few machines I tested on. So I would like this patch to go upstream.
My coverage is limited though. Did anyone at Intel test this patch on other (recent) systems?
Signature · 2 lines
Jean Delvare SUSE L3 Support
Hi
Quoted 5 lines · Jean Delvare
Quoted 4 lines
Quoted 2 lines
Jarkko, still no information about this from your Windows or hardware folks?
No update, in fact some of the contacts have left the company :-(
Quoted 8 lines
What do we do then? Without this patch, I2C Block Read doesn't work at all on recent chipsets. I don't think it can be worse with the patch. And it works fine on the few machines I tested on. So I would like this patch to go upstream.
My coverage is limited though. Did anyone at Intel test this patch on other (recent) systems?
Sorry the delay. I have only handful of machine that have on-board EEPROM connected to SMBUS in our lab and they are relatively new so only limited testing from my side.
In fact those had the SPD Write Disable set so attempt to use kernel's at24 driver didn't register the EEPROM at all without your patch since one-byte test read in drivers/misc/eeprom/at24.c: at24_probe() failed.
echo "24c02 0x56" >/sys/bus/i2c/devices/i2c-0/new_device
With your patch at24 probes and I can dump the 0xff content (I guess it's unprogrammed) with the hexdump:
hexdump -C /sys/bus/i2c/devices/i2c-0/0-0056/eeprom
I think we are better to have this patch as we know it makes things better. I'll add my tested-by to your patch with one comment.
Signature · 1 line
Jarkko
drivers/i2c/busses/i2c-i801.c line 150
Quoted 11 lines · Jean Delvare
drivers/i2c/busses/i2c-i801.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)drivers/i2c/busses/i2c-i801.c +1
-0@@ -146,6 +146,7 @@#define SMBHSTCFG_HST_EN BIT(0)#define SMBHSTCFG_SMB_SMI_EN BIT(1)#define SMBHSTCFG_I2C_EN BIT(2) +#define SMBHSTCFG_SPD_WD BIT(4)
Looks like you had some local BIT() cleanup patch before this and patch doesn't apply. I fixed it locally and you can add my tested-by to this patch.
Quoted 9 lines · Jean Delvare
Starting with the 8-Series/C220 PCH (Lynx Point), the SMBus controller includes a SPD EEPROM protection mechanism. Once the SPD Write Disable bit is set, only reads are allowed to slave addresses 0x50-0x57.
However the legacy implementation of I2C Block Read since the ICH5 looks like a write, and is therefore blocked by the SPD protection mechanism. This causes the eeprom and at24 drivers to fail.
Quoted 8 lines
So assume that I2C Block Read is implemented as an actual read on these chipsets. I tested it on my Q87 chipset and it seems to work just fine.
Signed-off-by Jean Delvare <[email protected]>Cc Jarkko Nikula <[email protected]>Cc Mika Westerberg <[email protected]>Cc Wolfram Sang <[email protected]>
Fixed the BIT() issue mentioned by Jarkko and applied to for-current, thanks! But please double check my commit once I pushed out.
Hi Wolfram,
Quoted 14 lines · Wolfram Sang
Quoted 13 lines · Jean Delvare
Starting with the 8-Series/C220 PCH (Lynx Point), the SMBus controller includes a SPD EEPROM protection mechanism. Once the SPD Write Disable bit is set, only reads are allowed to slave addresses 0x50-0x57.
However the legacy implementation of I2C Block Read since the ICH5 looks like a write, and is therefore blocked by the SPD protection mechanism. This causes the eeprom and at24 drivers to fail.
So assume that I2C Block Read is implemented as an actual read on these chipsets. I tested it on my Q87 chipset and it seems to work just fine.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Jean Delvare <[email protected]>Cc Jarkko Nikula <[email protected]>Cc Mika Westerberg <[email protected]>Cc Wolfram Sang <[email protected]>Fixed the
BIT()issue mentioned by Jarkko and applied to for-current, thanks! But please double check my commit once I pushed out.
The BIT() cleanup is a patch by Benjamin Tissoires ("i2c: i801: use BIT() macro for bits definition"), I thought you had applied it already so I rebased my patch on it, but it turns out I was wrong. You could just have used v1 of the patch ;-)
Your changes are obviously correct, but you'll have to solve the merge conflict again when applying Benjamin's patch, sorry.
Thanks,
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 3 lines
Your changes are obviously correct, but you'll have to solve the merge conflict again when applying Benjamin's patch, sorry.
I know and I am OK. Still, I wanted your patch to be in v4.9 while Benjamin's patches look like v4.10 material to me.