All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
@ 2016-10-11 11:13 Jean Delvare
  2016-10-11 11:32 ` Jarkko Nikula
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jean Delvare @ 2016-10-11 11:13 UTC (permalink / raw)
  To: Linux I2C; +Cc: Jarkko Nikula, Mika Westerberg, Wolfram Sang

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 <jdelvare@suse.de>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
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(-)

--- linux-4.8.orig/drivers/i2c/busses/i2c-i801.c	2016-10-11 10:41:43.984120197 +0200
+++ linux-4.8/drivers/i2c/busses/i2c-i801.c	2016-10-11 10:42:46.648783059 +0200
@@ -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))


-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-11 11:13 [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later Jean Delvare
@ 2016-10-11 11:32 ` Jarkko Nikula
  2016-10-11 18:20   ` Jean Delvare
  2016-10-17 12:38 ` Jarkko Nikula
  2016-10-25 10:02 ` Wolfram Sang
  2 siblings, 1 reply; 8+ messages in thread
From: Jarkko Nikula @ 2016-10-11 11:32 UTC (permalink / raw)
  To: Jean Delvare, Linux I2C; +Cc: Mika Westerberg, Wolfram Sang

On 10/11/2016 02:13 PM, Jean Delvare wrote:
> 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 <jdelvare@suse.de>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> ---
> 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 :-(

-- 
Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-11 11:32 ` Jarkko Nikula
@ 2016-10-11 18:20   ` Jean Delvare
  2016-10-17 12:38     ` Jarkko Nikula
  0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2016-10-11 18:20 UTC (permalink / raw)
  To: Jarkko Nikula; +Cc: Linux I2C, Mika Westerberg, Wolfram Sang

Hi Jarkko,

On Tue, 11 Oct 2016 14:32:55 +0300, Jarkko Nikula wrote:
> On 10/11/2016 02:13 PM, Jean Delvare wrote:
> > 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 <jdelvare@suse.de>
> > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > ---
> > 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 :-(

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?

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-11 18:20   ` Jean Delvare
@ 2016-10-17 12:38     ` Jarkko Nikula
  0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Nikula @ 2016-10-17 12:38 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Linux I2C, Mika Westerberg, Wolfram Sang

Hi

On 10/11/2016 09:20 PM, Jean Delvare wrote:
>>> 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?
>
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.

-- 
Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-11 11:13 [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later Jean Delvare
  2016-10-11 11:32 ` Jarkko Nikula
@ 2016-10-17 12:38 ` Jarkko Nikula
  2016-10-25 10:02 ` Wolfram Sang
  2 siblings, 0 replies; 8+ messages in thread
From: Jarkko Nikula @ 2016-10-17 12:38 UTC (permalink / raw)
  To: Jean Delvare, Linux I2C; +Cc: Mika Westerberg, Wolfram Sang

On 10/11/2016 02:13 PM, Jean Delvare wrote:
>  drivers/i2c/busses/i2c-i801.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> --- linux-4.8.orig/drivers/i2c/busses/i2c-i801.c	2016-10-11 10:41:43.984120197 +0200
> +++ linux-4.8/drivers/i2c/busses/i2c-i801.c	2016-10-11 10:42:46.648783059 +0200
> @@ -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.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-11 11:13 [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later Jean Delvare
  2016-10-11 11:32 ` Jarkko Nikula
  2016-10-17 12:38 ` Jarkko Nikula
@ 2016-10-25 10:02 ` Wolfram Sang
  2016-10-26 12:15   ` Jean Delvare
  2 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2016-10-25 10:02 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Linux I2C, Jarkko Nikula, Mika Westerberg

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

On Tue, Oct 11, 2016 at 01:13:27PM +0200, Jean Delvare wrote:
> 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 <jdelvare@suse.de>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>

Fixed the BIT() issue mentioned by Jarkko and applied to for-current,
thanks! But please double check my commit once I pushed out.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-25 10:02 ` Wolfram Sang
@ 2016-10-26 12:15   ` Jean Delvare
  2016-10-26 12:51     ` Wolfram Sang
  0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2016-10-26 12:15 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Linux I2C, Jarkko Nikula, Mika Westerberg

Hi Wolfram,

On Tue, 25 Oct 2016 12:02:42 +0200, Wolfram Sang wrote:
> On Tue, Oct 11, 2016 at 01:13:27PM +0200, Jean Delvare wrote:
> > 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 <jdelvare@suse.de>
> > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> 
> 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,
-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later
  2016-10-26 12:15   ` Jean Delvare
@ 2016-10-26 12:51     ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2016-10-26 12:51 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Linux I2C, Jarkko Nikula, Mika Westerberg

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


> 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.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-10-26 12:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-11 11:13 [PATCH v2] i2c: i801: Fix I2C Block Read on 8-Series/C220 and later Jean Delvare
2016-10-11 11:32 ` Jarkko Nikula
2016-10-11 18:20   ` Jean Delvare
2016-10-17 12:38     ` Jarkko Nikula
2016-10-17 12:38 ` Jarkko Nikula
2016-10-25 10:02 ` Wolfram Sang
2016-10-26 12:15   ` Jean Delvare
2016-10-26 12:51     ` Wolfram Sang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.