[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Hans-Frieder Vogt]

Hello Jean,

I invested a little bit of time into the nforce2/3/4 SMBus driver and changed 
the following things:

- general cleanup:
	o removed code for the unsupported I2C block transfer mode
	o removed detail warnings about unsupported modes that are covered in a 
general warning (unsupported transaction...) anyway
	o removed necessity of a definition of struct i2c_adapter
	o moved definition of struct i2c_algorithm, making forward declarations of 
nforce2_access and nforce2_func unnecessary
- improvements:
	o support MCP51 and MCP55 southbridges
	o advertise  I2C_FUNC_SMBUS_BLOCK_DATA, so that it really can be used

the patch is relative to 2.6.15-rc7 with your patch 
i2c-nforce2-support-nforce4-mcp04.patch of 18 december (thanks) already 
applied. If preferred, I can of course also send a full patch relative to 
2.6.15-rc7 (which should be applicable to other kernel versions as well).

Signed-off-by: Hans-Frieder Vogt <hfvogt at arcor.de>

--- linux-2.6.15-rc7-jean/include/linux/pci_ids.h	2005-12-26 
13:15:06.582626423 +0100
+++ linux-2.6.15-rc7/include/linux/pci_ids.h	2005-12-26 13:15:34.625804868 
+0100
@@ -1091,9 +1091,11 @@
 #define PCI_DEVICE_ID_NVIDIA_QUADRO4_900XGL	0x0258
 #define PCI_DEVICE_ID_NVIDIA_QUADRO4_750XGL	0x0259
 #define PCI_DEVICE_ID_NVIDIA_QUADRO4_700XGL	0x025B
+#define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP51_SMBUS	0x0264
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP51_IDE	0x0265
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP51_SATA	0x0266
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP51_SATA2	0x0267
+#define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SMBUS	0x0368
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_IDE	0x036E
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SATA	0x037E
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SATA2	0x037F
--- linux-2.6.15-rc7-jean/Documentation/i2c/busses/i2c-nforce2	2005-12-26 
13:10:06.125382505 +0100
+++ linux-2.6.15-rc7/Documentation/i2c/busses/i2c-nforce2	2005-12-26 
13:11:48.400743251 +0100
@@ -7,6 +7,8 @@
   * nForce3 250Gb MCP          10de:00E4 
   * nForce4 MCP	       	       10de:0052
   * nForce4 MCP-04             10de:0034
+  * nForce  MCP-51             10de:0264
+  * nForce  MCP-55             10de:0368
 
 Datasheet: not publically available, but seems to be similar to the
            AMD-8111 SMBus 2.0 adapter.
--- linux-2.6.15-rc7-jean/drivers/i2c/busses/i2c-nforce2.c	2005-12-26 
13:12:48.669359700 +0100
+++ linux-2.6.15-rc7/drivers/i2c/busses/i2c-nforce2.c	2005-12-27 
00:21:24.719391220 +0100
@@ -31,6 +31,8 @@
     nForce3 250Gb MCP		00E4
     nForce4 MCP			0052
     nForce4 MCP-04		0034
+    nForce MCP-51		0264
+    nForce MCP-55		0368
 
     This driver supports the 2 SMBuses that are included in the MCP of the
     nForce2/3/4 chipsets.
@@ -100,24 +102,7 @@
 
 static struct pci_driver nforce2_driver;
 
-static s32 nforce2_access(struct i2c_adapter *adap, u16 addr,
-		       unsigned short flags, char read_write,
-		       u8 command, int size, union i2c_smbus_data *data);
-static u32 nforce2_func(struct i2c_adapter *adapter);
 
-
-static struct i2c_algorithm smbus_algorithm = {
-	.smbus_xfer = nforce2_access,
-	.functionality = nforce2_func,
-};
-
-static struct i2c_adapter nforce2_adapter = {
-	.owner          = THIS_MODULE,
-	.class          = I2C_CLASS_HWMON,
-	.algo           = &smbus_algorithm,
-};
-
-/* Return -1 on error. See smbus.h for more information */
 static s32 nforce2_access(struct i2c_adapter * adap, u16 addr,
 		unsigned short flags, char read_write,
 		u8 command, int size, union i2c_smbus_data * data)
@@ -171,24 +156,6 @@
 			protocol |= NVIDIA_SMB_PRTCL_BLOCK_DATA | pec;
 			break;
 
-		case I2C_SMBUS_I2C_BLOCK_DATA:
-			len = min_t(u8, data->block[0], 32);
-			outb_p(command, NVIDIA_SMB_CMD);
-			outb_p(len, NVIDIA_SMB_BCNT);
-			if (read_write = I2C_SMBUS_WRITE)
-				for (i = 0; i < len; i++)
-					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
-			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
-			break;
-
-		case I2C_SMBUS_PROC_CALL:
-			dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-			return -1;
-
-		case I2C_SMBUS_BLOCK_PROC_CALL:
-			dev_err(&adap->dev, "I2C_SMBUS_BLOCK_PROC_CALL not supported!\n");
-			return -1;
-
 		default:
 			dev_err(&adap->dev, "Unsupported transaction %d\n", size);
 			return -1;
@@ -224,15 +191,12 @@
 			break;
 
 		case I2C_SMBUS_WORD_DATA:
-		/* case I2C_SMBUS_PROC_CALL: not supported */
 			data->word = inb_p(NVIDIA_SMB_DATA) | (inb_p(NVIDIA_SMB_DATA+1) << 8);
 			break;
 
 		case I2C_SMBUS_BLOCK_DATA:
-		/* case I2C_SMBUS_BLOCK_PROC_CALL: not supported */
 			len = inb_p(NVIDIA_SMB_BCNT);
 			len = min_t(u8, len, 32);
-		case I2C_SMBUS_I2C_BLOCK_DATA:
 			for (i = 0; i < len; i++)
 				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
 			data->block[0] = len;
@@ -247,10 +211,15 @@
 {
 	/* other functionality might be possible, but is not tested */
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
-	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA /* |
-	    I2C_FUNC_SMBUS_BLOCK_DATA */;
+	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+	    I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
+static struct i2c_algorithm smbus_algorithm = {
+	.smbus_xfer = nforce2_access,
+	.functionality = nforce2_func,
+};
+
 
 static struct pci_device_id nforce2_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE2_SMBUS) },
@@ -259,6 +228,8 @@
 	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE3S_SMBUS) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE4_SMBUS) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 
PCI_DEVICE_ID_NVIDIA_NFORCE_MCP04_SMBUS) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 
PCI_DEVICE_ID_NVIDIA_NFORCE_MCP51_SMBUS) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 
PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SMBUS) },
 	{ 0 }
 };
 
@@ -278,14 +249,16 @@
 	}
 	smbus->dev  = dev;
 	smbus->base = iobase & 0xfffc;
-	smbus->size = 8;
+	smbus->size = 64;
 
 	if (!request_region(smbus->base, smbus->size, nforce2_driver.name)) {
 		dev_err(&smbus->adapter.dev, "Error requesting region %02x .. %02X for 
%s\n",
 			smbus->base, smbus->base+smbus->size-1, name);
 		return -1;
 	}
-	smbus->adapter = nforce2_adapter;
+	smbus->adapter.owner = THIS_MODULE;
+	smbus->adapter.class = I2C_CLASS_HWMON;
+	smbus->adapter.algo = &smbus_algorithm;
 	smbus->adapter.algo_data = smbus;
 	smbus->adapter.dev.parent = &dev->dev;
 	snprintf(smbus->adapter.name, I2C_NAME_SIZE,
@@ -318,6 +291,7 @@
 		dev_err(&dev->dev, "Error probing SMB1.\n");
 		smbuses[0].base = 0;	/* to have a check value */
 	}
+	/* SMBus adapter 2 */
 	res2 = nforce2_probe_smb (dev, NFORCE_PCI_SMB2, &smbuses[1], "SMB2");
 	if (res2 < 0) {
 		dev_err(&dev->dev, "Error probing SMB2.\n");


-- 
--
Hans-Frieder Vogt                 e-mail: hfvogt <at> arcor .dot. de
                                          hfvogt <at> gmx .dot. net

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Jean Delvare]

Hallo Hans-Frider,

> I invested a little bit of time into the nforce2/3/4 SMBus driver and changed 
> the following things:
> 
> - general cleanup:
> 	o removed code for the unsupported I2C block transfer mode
> 	o removed detail warnings about unsupported modes that are covered in a 
> general warning (unsupported transaction...) anyway
> 	o removed necessity of a definition of struct i2c_adapter
> 	o moved definition of struct i2c_algorithm, making forward declarations of 
> nforce2_access and nforce2_func unnecessary
> - improvements:
> 	o support MCP51 and MCP55 southbridges
> 	o advertise  I2C_FUNC_SMBUS_BLOCK_DATA, so that it really can be used
> 
> the patch is relative to 2.6.15-rc7 with your patch 
> i2c-nforce2-support-nforce4-mcp04.patch of 18 december (thanks) already 
> applied. If preferred, I can of course also send a full patch relative to 
> 2.6.15-rc7 (which should be applicable to other kernel versions as well).

I'm sorry for overlooking this patch when you sent it back in December
2005. Now we happen to have a part of these changes already merged in
the i2c-nforce2 driver (in particular support for the MPC51 and MPC55.)
Would you be kind enough to respin your cleanups on top the i2c-nforce2
driver as it exists in recent -mm tree? (Or on top of Linus' latest +
http://khali.linux-fr.org/devel/i2c/linux-2.6/i2c-nforce2-add-mcp51-mcp55-support.patch,
at your option.)

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Hans-Frieder Vogt]

Jean,

attached is my latest patch against kernel 2.6.17-rc5-mm3, now only containing 
the cleanup parts of my original patch. I hope we can get these changes 
quickly into the kernel. They mainly delete non-funtioning parts, so the 
patch should not have many side effects.

Signed-off-by: Hans-Frieder Vogt <hfvogt at arcor.de>

Am Montag, 5. Juni 2006 09:00 schrieb Jean Delvare:
> Hallo Hans-Frider,
>
> > I invested a little bit of time into the nforce2/3/4 SMBus driver and
> > changed the following things:
> >
> > - general cleanup:
> > 	o removed code for the unsupported I2C block transfer mode
> > 	o removed detail warnings about unsupported modes that are covered in a
> > general warning (unsupported transaction...) anyway
> > 	o removed necessity of a definition of struct i2c_adapter
> > 	o moved definition of struct i2c_algorithm, making forward declarations
> > of nforce2_access and nforce2_func unnecessary
> > - improvements:
> > 	o support MCP51 and MCP55 southbridges
> > 	o advertise  I2C_FUNC_SMBUS_BLOCK_DATA, so that it really can be used
> >
> > the patch is relative to 2.6.15-rc7 with your patch
> > i2c-nforce2-support-nforce4-mcp04.patch of 18 december (thanks) already
> > applied. If preferred, I can of course also send a full patch relative to
> > 2.6.15-rc7 (which should be applicable to other kernel versions as well).
>
> I'm sorry for overlooking this patch when you sent it back in December
> 2005. Now we happen to have a part of these changes already merged in
> the i2c-nforce2 driver (in particular support for the MPC51 and MPC55.)
> Would you be kind enough to respin your cleanups on top the i2c-nforce2
> driver as it exists in recent -mm tree? (Or on top of Linus' latest +
> http://khali.linux-fr.org/devel/i2c/linux-2.6/i2c-nforce2-add-mcp51-mcp55-s
>upport.patch, at your option.)
>
> Thanks,

-- 
--
Hans-Frieder Vogt                 e-mail: hfvogt <at> arcor .dot. de
                                          hfvogt <at> gmx .dot. net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-nforce2-2.6.17-rc5-mm3.patch
Type: text/x-diff
Size: 3558 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060608/60645b68/attachment.bin 

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Jean Delvare]

Hans-Frieder,

> attached is my latest patch against kernel 2.6.17-rc5-mm3, now only containing 
> the cleanup parts of my original patch. I hope we can get these changes 
> quickly into the kernel. They mainly delete non-funtioning parts, so the 
> patch should not have many side effects.

This statement isn't totally true, as the driver is now advertising
I2C_FUNC_SMBUS_BLOCK_DATA as being supported, while so far it didn't.
This may cause new code paths to be taken, which have never been
exercised and might have bugs.

Overall I'm happy with your patch, the cleanups are very welcome,
thanks for the update. I would have a couple objections though:

> -/* Return -1 on error. See smbus.h for more information */
>  static s32 nforce2_access(struct i2c_adapter * adap, u16 addr,
>  		unsigned short flags, char read_write,
>  		u8 command, int size, union i2c_smbus_data * data)

It does indeed return -1 on error, even though the second part of the
comment should be discarded, I agree.

> @@ -174,24 +156,6 @@ static s32 nforce2_access(struct i2c_ada
>  			protocol |= NVIDIA_SMB_PRTCL_BLOCK_DATA | pec;
>  			break;
>  
> -		case I2C_SMBUS_I2C_BLOCK_DATA:
> -			len = min_t(u8, data->block[0], 32);
> -			outb_p(command, NVIDIA_SMB_CMD);
> -			outb_p(len, NVIDIA_SMB_BCNT);
> -			if (read_write = I2C_SMBUS_WRITE)
> -				for (i = 0; i < len; i++)
> -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> -			break;

Are you sure this transaction type isn't supported? That's a useful one
to access EEPROMs, and given that NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA
exists, it would seem that the chip does support it. What makes you
think otherwise? Isn't it simply a matter of advertising it, as for the
SMBus block transactions?

> @@ -291,7 +256,7 @@ static int __devinit nforce2_probe_smb (
>  		}
>  
>  		smbus->base = iobase & PCI_BASE_ADDRESS_IO_MASK;
> -		smbus->size = 8;
> +		smbus->size = 64;
>  	}
>  	smbus->dev = dev;
>  

Your summary didn't mention this change. It looks like a bug fix to me,
rather than a simple cleanup. We were previously reserving only 8 I/O
addresses while the highest offset we access is 0x24 so we should have
been reserving at least 37 bytes. 64 sounds sane.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Hans-Frieder Vogt]

Jean,

> Hans-Frieder,
>
> > attached is my latest patch against kernel 2.6.17-rc5-mm3, now only
> > containing the cleanup parts of my original patch. I hope we can get
> > these changes quickly into the kernel. They mainly delete non-funtioning
> > parts, so the patch should not have many side effects.
>
> This statement isn't totally true, as the driver is now advertising
> I2C_FUNC_SMBUS_BLOCK_DATA as being supported, while so far it didn't.
> This may cause new code paths to be taken, which have never been
> exercised and might have bugs.
>
I am pretty sure now that this function is supported by the nforce2/3/4 SMBus 
controller, so I took the opportunity to not only have the function coded in 
the driver but also advertise it. It is now well tested though, and I would 
be happy to get response from users of the new mode (success or failure, both 
is welcome).

> Overall I'm happy with your patch, the cleanups are very welcome,
>
> thanks for the update. I would have a couple objections though:
> > -/* Return -1 on error. See smbus.h for more information */
> >  static s32 nforce2_access(struct i2c_adapter * adap, u16 addr,
> >  		unsigned short flags, char read_write,
> >  		u8 command, int size, union i2c_smbus_data * data)
>
> It does indeed return -1 on error, even though the second part of the
> comment should be discarded, I agree.
>
> > @@ -174,24 +156,6 @@ static s32 nforce2_access(struct i2c_ada
> >  			protocol |= NVIDIA_SMB_PRTCL_BLOCK_DATA | pec;
> >  			break;
> >
> > -		case I2C_SMBUS_I2C_BLOCK_DATA:
> > -			len = min_t(u8, data->block[0], 32);
> > -			outb_p(command, NVIDIA_SMB_CMD);
> > -			outb_p(len, NVIDIA_SMB_BCNT);
> > -			if (read_write = I2C_SMBUS_WRITE)
> > -				for (i = 0; i < len; i++)
> > -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> > -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> > -			break;
>
> Are you sure this transaction type isn't supported? That's a useful one
> to access EEPROMs, and given that NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA
> exists, it would seem that the chip does support it. What makes you
> think otherwise? Isn't it simply a matter of advertising it, as for the
> SMBus block transactions?
>

I have to admit, I do not know whether NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA is 
supported. However, I have only limited possibilities to check the 
functionality of the SMBus host controller, and the eeprom on my nforce4 
board did not react any more when I tried to read data via i2cdump in i2c 
block data mode. Therefore I consider this function as very experimental for 
the moment. Maybe I just tried the function on the wrong i2c chip or using 
the wrong tool.
Do you know or have other tools that may be useful in testing the capability 
of i2c bus hosts and chips?
As soon as I am able to confirm that i2c block data or other functions do 
really work I will of course add again the code (and advertise it as well).

> > @@ -291,7 +256,7 @@ static int __devinit nforce2_probe_smb (
> >  		}
> >
> >  		smbus->base = iobase & PCI_BASE_ADDRESS_IO_MASK;
> > -		smbus->size = 8;
> > +		smbus->size = 64;
> >  	}
> >  	smbus->dev = dev;
>
> Your summary didn't mention this change. It looks like a bug fix to me,
> rather than a simple cleanup. We were previously reserving only 8 I/O
> addresses while the highest offset we access is 0x24 so we should have
> been reserving at least 37 bytes. 64 sounds sane.
>
I confess, this is really a bug fix and I forgot to mention it.
I chose 64 for the I/O window size because
a) it is big enough to cover the currently addressed I/O range
b) on all nforce2/3/4 boards that I have checked the SMBus I/O addresses are 
64 apart, so it is sort of a "substantiated guess".

> Thanks,
Thanks as well for your constructive comments.

-- 
--
Hans-Frieder Vogt                 e-mail: hfvogt <at> arcor .dot. de
                                          hfvogt <at> gmx .dot. net

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Jean Delvare]

Hans-Frieder,

> > This statement isn't totally true, as the driver is now advertising
> > I2C_FUNC_SMBUS_BLOCK_DATA as being supported, while so far it didn't.
> > This may cause new code paths to be taken, which have never been
> > exercised and might have bugs.
>
> I am pretty sure now that this function is supported by the nforce2/3/4 SMBus 
> controller, so I took the opportunity to not only have the function coded in 
> the driver but also advertise it. It is now well tested though, and I would 
> be happy to get response from users of the new mode (success or failure, both 
> is welcome).

I guess you meant _not_ well tested?

I doubt you'll find many testers, as the SMBus block transactions are
rarely used. You need very specific devices to make use of them.

> > > -		case I2C_SMBUS_I2C_BLOCK_DATA:
> > > -			len = min_t(u8, data->block[0], 32);
> > > -			outb_p(command, NVIDIA_SMB_CMD);
> > > -			outb_p(len, NVIDIA_SMB_BCNT);
> > > -			if (read_write = I2C_SMBUS_WRITE)
> > > -				for (i = 0; i < len; i++)
> > > -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> > > -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> > > -			break;
> >
> > Are you sure this transaction type isn't supported? That's a useful one
> > to access EEPROMs, and given that NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA
> > exists, it would seem that the chip does support it. What makes you
> > think otherwise? Isn't it simply a matter of advertising it, as for the
> > SMBus block transactions?
> 
> I have to admit, I do not know whether NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA is 
> supported. However, I have only limited possibilities to check the 
> functionality of the SMBus host controller, and the eeprom on my nforce4 
> board did not react any more when I tried to read data via i2cdump in i2c 
> block data mode. Therefore I consider this function as very experimental for 
> the moment. Maybe I just tried the function on the wrong i2c chip or using 
> the wrong tool.

Be very careful with SPD EEPROMs, they are sometimes writable, and
corrupting them will make your memory module unusable as soon as you
reboot :(

Do you have any datasheet for the nforce2/3/4 chips?

> Do you know or have other tools that may be useful in testing the capability 
> of i2c bus hosts and chips?

i2cdetect can be used to test quick command and receive byte. i2cdump
can be used to test send byte, receive byte, read byte, read word and
i2c block read.

> As soon as I am able to confirm that i2c block data or other functions do 
> really work I will of course add again the code (and advertise it as well).

I'd prefer that we don't rip the code out if we will reintroduce it
shortly afterwards. Better test everything now, and come up with a
single patch doing the right thing.

I have a spare nforce2 board here, I can build a test system from it
and help you with the tests. However, it will have to wait for the next
week-end so that I have the time to assemble the system.

> > Your summary didn't mention this change. It looks like a bug fix to me,
> > rather than a simple cleanup. We were previously reserving only 8 I/O
> > addresses while the highest offset we access is 0x24 so we should have
> > been reserving at least 37 bytes. 64 sounds sane.
> >
> I confess, this is really a bug fix and I forgot to mention it.
> I chose 64 for the I/O window size because
> a) it is big enough to cover the currently addressed I/O range
> b) on all nforce2/3/4 boards that I have checked the SMBus I/O addresses are 
> 64 apart, so it is sort of a "substantiated guess".

A very reasonable guess, considering that for the nforce3/4 the I/O
addresses are in standard PCI BARs and they are indeed 64-byte large.
And I think the PCI I/O ranges have to be powers of two anyway. Let's go
with 64.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Hans-Frieder Vogt]

Jean,

> > > This statement isn't totally true, as the driver is now advertising
> > > I2C_FUNC_SMBUS_BLOCK_DATA as being supported, while so far it didn't.
> > > This may cause new code paths to be taken, which have never been
> > > exercised and might have bugs.
> >
> > I am pretty sure now that this function is supported by the nforce2/3/4
> > SMBus controller, so I took the opportunity to not only have the function
> > coded in the driver but also advertise it. It is now well tested though,
> > and I would be happy to get response from users of the new mode (success
> > or failure, both is welcome).
>
> I guess you meant _not_ well tested?

Of course you are right.

> I doubt you'll find many testers, as the SMBus block transactions are
> rarely used. You need very specific devices to make use of them.

at least the BIOS provides this function on nforce4 mainboards, although I 
don't know whether they also use it :-]

> > > > -		case I2C_SMBUS_I2C_BLOCK_DATA:
> > > > -			len = min_t(u8, data->block[0], 32);
> > > > -			outb_p(command, NVIDIA_SMB_CMD);
> > > > -			outb_p(len, NVIDIA_SMB_BCNT);
> > > > -			if (read_write = I2C_SMBUS_WRITE)
> > > > -				for (i = 0; i < len; i++)
> > > > -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> > > > -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> > > > -			break;
> > >
> > > Are you sure this transaction type isn't supported? That's a useful one
> > > to access EEPROMs, and given that NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA
> > > exists, it would seem that the chip does support it. What makes you
> > > think otherwise? Isn't it simply a matter of advertising it, as for the
> > > SMBus block transactions?
> >
> > I have to admit, I do not know whether NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA is
> > supported. However, I have only limited possibilities to check the
> > functionality of the SMBus host controller, and the eeprom on my nforce4
> > board did not react any more when I tried to read data via i2cdump in i2c
> > block data mode. Therefore I consider this function as very experimental
> > for the moment. Maybe I just tried the function on the wrong i2c chip or
> > using the wrong tool.
>
> Be very careful with SPD EEPROMs, they are sometimes writable, and
> corrupting them will make your memory module unusable as soon as you
> reboot :(

Thanks for this hint. Fortunately, I was lucky so far...

> Do you have any datasheet for the nforce2/3/4 chips?

Unfortunately I do not have a datasheet for the nforce chips.

>
> > Do you know or have other tools that may be useful in testing the
> > capability of i2c bus hosts and chips?
>
> i2cdetect can be used to test quick command and receive byte. i2cdump
> can be used to test send byte, receive byte, read byte, read word and
> i2c block read.
>
> > As soon as I am able to confirm that i2c block data or other functions do
> > really work I will of course add again the code (and advertise it as
> > well).
>
> I'd prefer that we don't rip the code out if we will reintroduce it
> shortly afterwards. Better test everything now, and come up with a
> single patch doing the right thing.

OK, I will do again some testing over a limited time, and will then either 
confirm the patch or come up with some better variant.

> I have a spare nforce2 board here, I can build a test system from it
> and help you with the tests. However, it will have to wait for the next
> week-end so that I have the time to assemble the system.

Thanks very much. It would help me a lot if you could confirm my findings with 
respect to the block data / I2C block data mode or provide me with some other 
experience.

> > > Your summary didn't mention this change. It looks like a bug fix to me,
> > > rather than a simple cleanup. We were previously reserving only 8 I/O
> > > addresses while the highest offset we access is 0x24 so we should have
> > > been reserving at least 37 bytes. 64 sounds sane.
> >
> > I confess, this is really a bug fix and I forgot to mention it.
> > I chose 64 for the I/O window size because
> > a) it is big enough to cover the currently addressed I/O range
> > b) on all nforce2/3/4 boards that I have checked the SMBus I/O addresses
> > are 64 apart, so it is sort of a "substantiated guess".
>
> A very reasonable guess, considering that for the nforce3/4 the I/O
> addresses are in standard PCI BARs and they are indeed 64-byte large.
> And I think the PCI I/O ranges have to be powers of two anyway. Let's go
> with 64.

Thanks again.

-- 
--
Hans-Frieder Vogt                 e-mail: hfvogt <at> arcor .dot. de
                                          hfvogt <at> gmx .dot. net

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Jean Delvare]

Hi Hans-Frieder,

> > This statement isn't totally true, as the driver is now advertising
> > I2C_FUNC_SMBUS_BLOCK_DATA as being supported, while so far it didn't.
> > This may cause new code paths to be taken, which have never been
> > exercised and might have bugs.
>
> I am pretty sure now that this function is supported by the nforce2/3/4
> SMBus controller, so I took the opportunity to not only have the function
> coded in the driver but also advertise it. It is not well tested though,
> and I would be happy to get response from users of the new mode (success
> or failure, both is welcome).

I don't have a chip which actually uses SMBus block transactions, so I
used an SPD EEPROM to test SMBus block read, carefully choosing the
offset so that the first byte would be a valid length. It seemed to
work well, except for length = 32, where I get a timeout, and the bus
is dead until next reboot. You may want to investigate on your end. At
any rate it would be convenient to have a way to reset the bus in this
case, so that a failed transaction doesn't kill the bus.

> > I doubt you'll find many testers, as the SMBus block transactions are
> > rarely used. You need very specific devices to make use of them.
> 
> at least the BIOS provides this function on nforce4 mainboards, although I 
> don't know whether they also use it :-]

Do they offer the I2C block transaction variant as well?

Maybe you can check if they do anything special for 32-byte long SMBus
block reads.

> > > -		case I2C_SMBUS_I2C_BLOCK_DATA:
> > > -			len = min_t(u8, data->block[0], 32);
> > > -			outb_p(command, NVIDIA_SMB_CMD);
> > > -			outb_p(len, NVIDIA_SMB_BCNT);
> > > -			if (read_write = I2C_SMBUS_WRITE)
> > > -				for (i = 0; i < len; i++)
> > > -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> > > -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> > > -			break;
> >
> > Are you sure this transaction type isn't supported? That's a useful one
> > to access EEPROMs, and given that NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA
> > exists, it would seem that the chip does support it. What makes you
> > think otherwise? Isn't it simply a matter of advertising it, as for the
> > SMBus block transactions?
>
> I have to admit, I do not know whether NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA is
> supported. However, I have only limited possibilities to check the
> functionality of the SMBus host controller, and the eeprom on my nforce4
> board did not react any more when I tried to read data via i2cdump in i2c
> block data mode. Therefore I consider this function as very experimental
> for the moment. Maybe I just tried the function on the wrong i2c chip or
> using the wrong tool.

I did some testing on my end (with an nforce2 chip) and it didn't work
either. I had to change the driver code to force the read length to 32
because that's the way we implement it in Linux, but even then I still
can't get anything but a timeout (status register value 0x19.) Forcing
the length to 16 instead didn't help. As we don't even know what the
status bits mean, there's not much more to try at this point.

Of course that could work better on newer chips, I only tested on
nforce2.

> > > As soon as I am able to confirm that i2c block data or other functions do
> > > really work I will of course add again the code (and advertise it as
> > > well).
> >
> > I'd prefer that we don't rip the code out if we will reintroduce it
> > shortly afterwards. Better test everything now, and come up with a
> > single patch doing the right thing.
> 
> OK, I will do again some testing over a limited time, and will then either 
> confirm the patch or come up with some better variant.

My own tests suggest that your original patch is the right thing to do
(i.e. keep SMBus block transactions and drop I2C block transactions)
although I'm a bit worried by the problem I observed for 32-byte long
SMBus block reads. It might not be a problem in practice though.

I am waiting for the results of your own tests, then we can decide what
we want to do.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Hans-Frieder Vogt]

Hi Jean,

thank you very much for your testing. Your results are extremely helpful for 
me!

Am Sonntag, 11. Juni 2006 13:28 schrieb Jean Delvare:
> > > This statement isn't totally true, as the driver is now advertising
> > > I2C_FUNC_SMBUS_BLOCK_DATA as being supported, while so far it didn't.
> > > This may cause new code paths to be taken, which have never been
> > > exercised and might have bugs.
> >
> > I am pretty sure now that this function is supported by the nforce2/3/4
> > SMBus controller, so I took the opportunity to not only have the function
> > coded in the driver but also advertise it. It is not well tested though,
> > and I would be happy to get response from users of the new mode (success
> > or failure, both is welcome).
>
> I don't have a chip which actually uses SMBus block transactions, so I
> used an SPD EEPROM to test SMBus block read, carefully choosing the
> offset so that the first byte would be a valid length. It seemed to
> work well, except for length = 32, where I get a timeout, and the bus
> is dead until next reboot. You may want to investigate on your end. At
> any rate it would be convenient to have a way to reset the bus in this
> case, so that a failed transaction doesn't kill the bus.

very valuable results! I only tried a length of 32 so far (also on SPD EEPROMs 
of two boards) and wondered, why it always failed!
There is also a strange thing in general with the length2: The ACPI 
documentation (I have checked in ACPIspec-2-0a.pdf and ACPIspec-2-0b.pdf) 
only allows 5 bits for the block count in the SMB_BCNT register. This is a 
bit contradictory to the explicitely mentioned maximum length of 32 bytes for 
a block transfer. Do you know how a length of 32 is coded in this? With 0?

> > > I doubt you'll find many testers, as the SMBus block transactions are
> > > rarely used. You need very specific devices to make use of them.
> >
> > at least the BIOS provides this function on nforce4 mainboards, although
> > I don't know whether they also use it :-]
>
> Do they offer the I2C block transaction variant as well?

I have found none.

>
> Maybe you can check if they do anything special for 32-byte long SMBus
> block reads.

I have not found anything so far, but I continue searching and trying. What 
they do is that they mask the output from the block count register with 0x1f 
to only use the allowed bits. Does that mean they only use transfers with 31 
bytes maximum?

> > > > -		case I2C_SMBUS_I2C_BLOCK_DATA:
> > > > -			len = min_t(u8, data->block[0], 32);
> > > > -			outb_p(command, NVIDIA_SMB_CMD);
> > > > -			outb_p(len, NVIDIA_SMB_BCNT);
> > > > -			if (read_write = I2C_SMBUS_WRITE)
> > > > -				for (i = 0; i < len; i++)
> > > > -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> > > > -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> > > > -			break;
> > >
> > > Are you sure this transaction type isn't supported? That's a useful one
> > > to access EEPROMs, and given that NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA
> > > exists, it would seem that the chip does support it. What makes you
> > > think otherwise? Isn't it simply a matter of advertising it, as for the
> > > SMBus block transactions?
> >
> > I have to admit, I do not know whether NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA is
> > supported. However, I have only limited possibilities to check the
> > functionality of the SMBus host controller, and the eeprom on my nforce4
> > board did not react any more when I tried to read data via i2cdump in i2c
> > block data mode. Therefore I consider this function as very experimental
> > for the moment. Maybe I just tried the function on the wrong i2c chip or
> > using the wrong tool.
>
> I did some testing on my end (with an nforce2 chip) and it didn't work
> either. I had to change the driver code to force the read length to 32
> because that's the way we implement it in Linux, but even then I still
> can't get anything but a timeout (status register value 0x19.) Forcing
> the length to 16 instead didn't help. As we don't even know what the
> status bits mean, there's not much more to try at this point.
>
> Of course that could work better on newer chips, I only tested on
> nforce2.

I can only confirm your findings. However, the value 0x19 means "SMBus Host 
unsupported protocol". Pretty clear, isn't it?

> > > > As soon as I am able to confirm that i2c block data or other
> > > > functions do really work I will of course add again the code (and
> > > > advertise it as well).
> > >
> > > I'd prefer that we don't rip the code out if we will reintroduce it
> > > shortly afterwards. Better test everything now, and come up with a
> > > single patch doing the right thing.
> >
> > OK, I will do again some testing over a limited time, and will then
> > either confirm the patch or come up with some better variant.
>
> My own tests suggest that your original patch is the right thing to do
> (i.e. keep SMBus block transactions and drop I2C block transactions)
> although I'm a bit worried by the problem I observed for 32-byte long
> SMBus block reads. It might not be a problem in practice though.

I am happy that you agree with my approach, and I will concentrate now on the 
32-byte long block read problem.

> I am waiting for the results of your own tests, then we can decide what
> we want to do.

Thanks again for your support. I will use the next weekend and will then 
summarise my findings.

-- 
--
Hans-Frieder Vogt                 e-mail: hfvogt <at> arcor .dot. de
                                          hfvogt <at> gmx .dot. net

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Jean Delvare]

Hallo Hans,

> > I don't have a chip which actually uses SMBus block transactions, so I
> > used an SPD EEPROM to test SMBus block read, carefully choosing the
> > offset so that the first byte would be a valid length. It seemed to
> > work well, except for length = 32, where I get a timeout, and the bus
> > is dead until next reboot. You may want to investigate on your end. At
> > any rate it would be convenient to have a way to reset the bus in this
> > case, so that a failed transaction doesn't kill the bus.
> 
> very valuable results! I only tried a length of 32 so far (also on SPD EEPROMs 
> of two boards) and wondered, why it always failed!
> There is also a strange thing in general with the length2: The ACPI 
> documentation (I have checked in ACPIspec-2-0a.pdf and ACPIspec-2-0b.pdf) 
> only allows 5 bits for the block count in the SMB_BCNT register. This is a 
> bit contradictory to the explicitely mentioned maximum length of 32 bytes for 
> a block transfer. Do you know how a length of 32 is coded in this? With 0?

This matches my observation that the higher 3 bits of the length byte
are ignored. Given that 0 is an invalid block length and 32 is valid,
it would be possible to actually code 32 as 0. I don't think it is
particularly clever, as it adds to confusion and will also require
additional code in several places, but we'll have to do with it as
this is in the ACPI specification.

Attached is a suggested patch for the i2c-nforce2 driver, applying on
top of your own cleanup patch. Other drivers (i2c-amd8111 and the not
yet merged i2c-acpi-ec) would need something similar. That being said,
I doubt this will fix the hangs I was observing, as they seem to be
caused by the hardware itself and not the i2c-nforce2 driver.

> > Maybe you can check if they do anything special for 32-byte long SMBus
> > block reads.
> 
> I have not found anything so far, but I continue searching and trying. What 
> they do is that they mask the output from the block count register with 0x1f 
> to only use the allowed bits.

This is what my patch does, actually.

>                                Does that mean they only use transfers with 31 
> bytes maximum?

The SMBus specification allows 32, and the ACPI specification mentions
a 32 byte buffer (SMB_DATA[i], i=0-31) so this would be odd. But maybe
this is a flaw in the nVidia implementation.

> > > I have to admit, I do not know whether NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA is
> > > supported. However, I have only limited possibilities to check the
> > > functionality of the SMBus host controller, and the eeprom on my nforce4
> > > board did not react any more when I tried to read data via i2cdump in i2c
> > > block data mode. Therefore I consider this function as very experimental
> > > for the moment. Maybe I just tried the function on the wrong i2c chip or
> > > using the wrong tool.
> >
> > I did some testing on my end (with an nforce2 chip) and it didn't work
> > either. I had to change the driver code to force the read length to 32
> > because that's the way we implement it in Linux, but even then I still
> > can't get anything but a timeout (status register value 0x19.) Forcing
> > the length to 16 instead didn't help. As we don't even know what the
> > status bits mean, there's not much more to try at this point.
> >
> > Of course that could work better on newer chips, I only tested on
> > nforce2.
> 
> I can only confirm your findings. However, the value 0x19 means "SMBus Host 
> unsupported protocol". Pretty clear, isn't it?

Ah, great, you just made me realize that the meaning of these status
bits were described in the ACPI specification. Thanks ;)

Indeed this is quite clear. If you didn't have more success with a more
recent chip, we really can wipe out that part of the code, including
the definition of NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA itself. It's not even
in the ACPI specification, even though i2c-amd8111 has it.

> > My own tests suggest that your original patch is the right thing to do
> > (i.e. keep SMBus block transactions and drop I2C block transactions)
> > although I'm a bit worried by the problem I observed for 32-byte long
> > SMBus block reads. It might not be a problem in practice though.
> 
> I am happy that you agree with my approach, and I will concentrate now on the 
> 32-byte long block read problem.

Good. I'll do some more tests next week-end as well, before I
disassemble my test system. Now that I have the help of the ACPI
specification, I might get a bit further, who knows. Do you have the
possibility to test the block write transactions? I don't.

I am worried that, if the 32-byte long block read hang is indeed a
hardware problem, we can't safely implement the feature in the driver.
We could intercept block writes and reject the ones which would hang the
controller, but we can't intercept reads, as the length is provided by
the slave chip and the hardware gets it before we do. So, unless we can
find a way to restore the host controller to a working state after the
problem occured, we better don't implement the block read functionality
in the driver :(

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH 2.6] cleanup of i2c-nforce2,

[Jean Delvare]

> Attached is a suggested patch for the i2c-nforce2 driver, applying on
> top of your own cleanup patch. Other drivers (i2c-amd8111 and the not
> yet merged i2c-acpi-ec) would need something similar. That being said,
> I doubt this will fix the hangs I was observing, as they seem to be
> caused by the hardware itself and not the i2c-nforce2 driver.

Hm, here it is:


The ACPI specification says that the length of SMBus block transactions
is stored on 5 bits. This suggests that the maximum value of 32 is encoded
as 0, so we must modify the i2c-nforce2 driver to handle this special case
properly.

Signed-off-by: Jean Delvare <khali at linux-fr.org>
---
 drivers/i2c/busses/i2c-nforce2.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

--- linux-2.6.17-rc6.orig/drivers/i2c/busses/i2c-nforce2.c	2006-06-15 09:16:16.000000000 +0200
+++ linux-2.6.17-rc6/drivers/i2c/busses/i2c-nforce2.c	2006-06-15 09:27:51.000000000 +0200
@@ -149,7 +149,9 @@
 			outb_p(command, NVIDIA_SMB_CMD);
 			if (read_write = I2C_SMBUS_WRITE) {
 				len = min_t(u8, data->block[0], 32);
-				outb_p(len, NVIDIA_SMB_BCNT);
+				/* Block length is stored on only 5 bits
+				   internally, 0 means 32 */
+				outb_p(len & 0x1f, NVIDIA_SMB_BCNT);
 				for (i = 0; i < len; i++)
 					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
 			}
@@ -195,8 +197,11 @@
 			break;
 
 		case I2C_SMBUS_BLOCK_DATA:
-			len = inb_p(NVIDIA_SMB_BCNT);
-			len = min_t(u8, len, 32);
+			/* Block length is stored on only 5 bits internally,
+			   0 means 32 */
+			len = inb_p(NVIDIA_SMB_BCNT) & 0x1f;
+			if (len = 0)
+				len = 32;
 			for (i = 0; i < len; i++)
 				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
 			data->block[0] = len;

-- 
Jean Delvare