cleanup of i2c-nforce2,

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

Hallo Hans-Frider,

Quoted 14 lines

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

Quoted 4 lines

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,

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.

Hans-Frieder,

Quoted 4 lines

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:

Review

Quoted 4 lines

(quoted diff) +0 -1

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

Quoted 4 lines

(quoted diff) +0 -0

@@ -174,24 +156,6 @@ static s32 nforce2_access(struct i2c_ada
 			protocol |= NVIDIA_SMB_PRTCL_BLOCK_DATA | pec;
 			break;
 

Review static s32 nforce2_access(struct i2c_ada · line 185

Quoted 9 lines

(quoted diff) +0 -9

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

Review static int __devinit nforce2_probe_smb ( · line 261

Quoted 8 lines

(quoted diff) +1 -1

@@ -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,

Quoted 4 lines

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

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -7

-		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);

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +0 -2

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

Quoted 5 lines

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

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -7

-		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);

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +0 -2

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

Quoted 7 lines

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?

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 2 lines

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 :-]

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -7

-		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);

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +0 -2

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

Quoted 7 lines

Quoted 3 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -7

-		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);

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +0 -2

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

Quoted 7 lines

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.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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?

Quoted 2 lines

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?

Quoted 15 lines

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 4 lines

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.

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.