Updated MPC I2C driver

This version should support MPC8540 as well as MPC8245 and MPC107. I don't have MPC85xx hardware, so I'd like feedback from people who do.

To use it on MPC107/824x requires the OCP patch that went by on the list on Tuesday. The support for MPC85xx is already in the kernel.

If nobody has any complaints I'm ready to send it to Greg K-H.

- Adrian Cox
Humboldt Solutions Ltd.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/07/01 17:53:05+01:00 [email protected]
# Add I2C driver for Motorola I2C
#

# drivers/i2c/busses/i2c-mpc.c
#   2004/07/01 17:52:55+01:00 [email protected] +410 -0
#
# drivers/i2c/busses/i2c-mpc.c
#   2004/07/01 17:52:55+01:00 [email protected] +0 -0
#   BitKeeper file /home/adrian/kernels/sa107-2.6/for-upstream/drivers/i2c/busses/i2c-mpc.c
#
# drivers/i2c/busses/Makefile
#   2004/07/01 17:52:55+01:00 [email protected] +1 -0
#   Add I2C driver for Motorola I2C
#
# drivers/i2c/busses/Kconfig
#   2004/07/01 17:52:54+01:00 [email protected] +11 -0
#   Add I2C driver for Motorola I2C
#
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig	Thu Jul  1 17:54:57 2004
+++ b/drivers/i2c/busses/Kconfig	Thu Jul  1 17:54:57 2004
@@ -396,4 +396,15 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-voodoo3.

+config I2C_MPC
+ tristate "MPC107/824x/85xx"
+ depends on I2C && FSL_OCP
+ help

+	  If you say yes to this option, support will be included for the
+	  built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245 and
+	  MPC85xx family processors.

+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mpc.
+
endmenu

diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
@@ -32,6 +32,7 @@
 obj-$(CONFIG_I2C_VOODOO3)	+= i2c-voodoo3.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
 obj-$(CONFIG_SCx200_I2C)	+= scx200_i2c.o
+obj-$(CONFIG_I2C_MPC)	+= i2c-mpc.o
 
 ifeq ($(CONFIG_I2C_DEBUG_BUS),y)
 EXTRA_CFLAGS += -DDEBUG

Hi Adrian,

The driver works well on both an mpc8540ads and a sandpoint here.
However, I just learned yesterday that i2c 32-bit read/writes on the 85xx will not always be guaranteed to work. So there does in fact need to be an abstraction there for 8-bit/32-bit read/writes. I wrote up a patch that does just that and it is attached, any suggestions or different ways to implement it I would be glad to take a go at it if your busy. I tested the patch on our mpc8540ads and sandpoint here, and it seems to work well. One note: the FS_I2C_32BIT needs to be set in the flags. Let me know what you think.

Regards,
Matthew

--
Matthew McClintock <[email protected]>

diff -purN linuxppc-2.5-i2c-bak/drivers/i2c/busses/i2c-mpc.c linuxppc-2.5-i2c/drivers/i2c/busses/i2c-mpc.c
@@ -72,18 +72,40 @@ struct mpc_i2c {
 	struct i2c_adapter adap;
 };
 
+static u32 mpc_i2c_read(struct mpc_i2c *i2c, char *addr)
+{
+	struct ocp_fs_i2c_data *i2c_data = i2c->ocpdef->additions;
+
+	if (i2c_data && (i2c_data->flags & FS_I2C_32BIT)) {
+		return readl(addr);
+	} else {
+		return readb(addr);
+	}
+}
+
+static void mpc_i2c_write(struct mpc_i2c *i2c, u32 val, char *addr)
+{
+	struct ocp_fs_i2c_data *i2c_data = i2c->ocpdef->additions;
+
+	if (i2c_data && (i2c_data->flags & FS_I2C_32BIT)) {
+		writel(val, addr);
+	} else {
+		writeb(val & 0xff, addr);
+	}
+}
+
 static __inline__ void writeccr(struct mpc_i2c *i2c, u32 x)
 {
-	writel(x, i2c->base + MPC_I2C_CR);
+	mpc_i2c_write(i2c, x, i2c->base + MPC_I2C_CR);
 }
 
 static irqreturn_t mpc_i2c_isr(int irq, void *dev_id, struct pt_regs * regs)
 {
 	struct mpc_i2c *i2c = dev_id;
-	if (readl(i2c->base + MPC_I2C_SR) & CSR_MIF) {
+	if (mpc_i2c_read(i2c, i2c->base + MPC_I2C_SR) & CSR_MIF) {
 		/* Read again to allow register to stabilise */
-		i2c->interrupt = readl(i2c->base + MPC_I2C_SR);
-		writel(0, i2c->base + MPC_I2C_SR);
+		i2c->interrupt = mpc_i2c_read(i2c, i2c->base + MPC_I2C_SR);
+		mpc_i2c_write(i2c, 0, i2c->base + MPC_I2C_SR);
 		wake_up_interruptible(&i2c->queue);
 	}
 	return IRQ_HANDLED;

@@ -97,7 +119,7 @@ static int i2c_wait(struct mpc_i2c *i2c,
 	int result = 0;
 
 	if (i2c->ocpdef->irq == OCP_IRQ_NA) {
-		while(! (readl(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
+		while(! (mpc_i2c_read(i2c, i2c->base + MPC_I2C_SR) & CSR_MIF)) {
 			schedule();
 			if (orig_jiffies + timeout < jiffies) {
 				DPRINTK("I2C: timeout\n");

@@ -105,8 +127,8 @@ static int i2c_wait(struct mpc_i2c *i2c,
 				break;
 			}
 		}
-		x = readl(i2c->base + MPC_I2C_SR);
-		writel(0, i2c->base + MPC_I2C_SR);
+		x = mpc_i2c_read(i2c, i2c->base + MPC_I2C_SR);
+		mpc_i2c_write(i2c, 0, i2c->base + MPC_I2C_SR);
 	} else {
 		add_wait_queue(&i2c->queue, &wait);
 		while ( ! (i2c->interrupt & CSR_MIF)) {

@@ -158,12 +180,12 @@ static void mpc_i2c_start(struct mpc_i2c
 	struct ocp_fs_i2c_data *i2c_data = i2c->ocpdef->additions;
         /* Set clock and filters */
 	if (i2c_data && (i2c_data->flags & FS_I2C_SEPARATE_DFSRR)) {
-		writel(0x31, i2c->base + MPC_I2C_FDR);
-		writel(0x10, i2c->base + MPC_I2C_DFSRR);
+		mpc_i2c_write(i2c, 0x31, i2c->base + MPC_I2C_FDR);
+		mpc_i2c_write(i2c, 0x10, i2c->base + MPC_I2C_DFSRR);
 	} else
-		writel(0x1031, i2c->base + MPC_I2C_FDR);
+		mpc_i2c_write(i2c, 0x1031, i2c->base + MPC_I2C_FDR);
         /* Clear arbitration */
-        writel(0, i2c->base + MPC_I2C_SR);
+        mpc_i2c_write(i2c, 0, i2c->base + MPC_I2C_SR);
        	/* Start with MEN */
 	writeccr(i2c, CCR_MEN);
 }

@@ -187,14 +209,14 @@ static int mpc_write(struct mpc_i2c *i2c
 	writeccr(i2c, CCR_MIEN | CCR_MEN |
 		      CCR_MSTA | CCR_MTX | flags);
 	/* Write target byte */
-	writel((target << 1), i2c->base + MPC_I2C_DR);
+	mpc_i2c_write(i2c, (target << 1), i2c->base + MPC_I2C_DR);
 
 	if (i2c_wait(i2c, timeout, 1) < 0)
 	    return -1;
 
         for(i = 0; i < length; i++) {
 	        /* Write data byte */
-	        writel(data[i], i2c->base + MPC_I2C_DR);
+	        mpc_i2c_write(i2c, data[i], i2c->base + MPC_I2C_DR);
 
 	        if (i2c_wait(i2c, timeout, 1) < 0)
 	            return -1;

@@ -217,7 +239,7 @@ static int mpc_read(struct mpc_i2c *i2c,
 	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA |
 		      CCR_MTX | flags);
 	/* Write target address byte - this time with the read flag set */
-	writel((target << 1) | 1, i2c->base + MPC_I2C_DR);
+	mpc_i2c_write(i2c, (target << 1) | 1, i2c->base + MPC_I2C_DR);
 
 	if (i2c_wait(i2c, timeout, 0) < 0)
 	    return -1;

@@ -228,7 +250,7 @@ static int mpc_read(struct mpc_i2c *i2c,
 	else
 	    writeccr(i2c, CCR_MIEN| CCR_MEN | CCR_MSTA);
     	/* Dummy read */
-	readl(i2c->base + MPC_I2C_DR);
+	mpc_i2c_read(i2c, i2c->base + MPC_I2C_DR);
 
 	for(i = 0; i < length; i++) {
 		if (i2c_wait(i2c, timeout, 0) < 0)

@@ -242,7 +264,7 @@ static int mpc_read(struct mpc_i2c *i2c,
 		if (i == length - 1)
 			writeccr(i2c, CCR_MIEN | CCR_MEN |
 				      CCR_TXAK);
-		data[i] = readl(i2c->base + MPC_I2C_DR);
+		data[i] = mpc_i2c_read(i2c, i2c->base + MPC_I2C_DR);
 	}
 
 	return length;

@@ -260,7 +282,7 @@ static int mpc_xfer(struct i2c_adapter *
         mpc_i2c_start(i2c);
 
 	/* Allow bus up to 1s to become not busy */
-	while(readl(i2c->base + MPC_I2C_SR) & CSR_MBB) {
+	while(mpc_i2c_read(i2c, i2c->base + MPC_I2C_SR) & CSR_MBB) {
                 if (signal_pending(current))
 		{
 			DPRINTK("I2C: Interrupted\n");

It seems that the MPC107 is happy with 8 bit accesses to the registers, so here's another version of the patch. This also changes the time calculations to use time_after().

This still leaves the interrupt handling register of the MPC5200 unsolved. I suggest introducing another flag for the MPC5200, and adding a small piece of extra setup code.

- Adrian Cox
Humboldt Solutions Ltd.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/07/01 22:10:01+01:00 [email protected]
# Switch to time_after for jiffies calculations
#

# drivers/i2c/busses/i2c-mpc.c
#   2004/07/01 22:09:50+01:00 [email protected] +3 -3
#   Switch to time_after for jiffies calculations
#
# ChangeSet
#   2004/07/01 22:04:45+01:00 [email protected]
#   Switch from readl/writel to readb/writeb for MPC85xx compatibility.
#
# drivers/i2c/busses/i2c-mpc.c
#   2004/07/01 22:04:32+01:00 [email protected] +17 -17
#   Switch from readl/writel to readb/writeb for MPC85xx compatibility.
#
# ChangeSet
#   2004/07/01 17:53:05+01:00 [email protected]
#   Add I2C driver for Motorola I2C
#
# drivers/i2c/busses/i2c-mpc.c
#   2004/07/01 17:52:55+01:00 [email protected] +410 -0
#
# drivers/i2c/busses/i2c-mpc.c
#   2004/07/01 17:52:55+01:00 [email protected] +0 -0
#   BitKeeper file /home/adrian/kernels/sa107-2.6/for-upstream/drivers/i2c/busses/i2c-mpc.c
#
# drivers/i2c/busses/Makefile
#   2004/07/01 17:52:55+01:00 [email protected] +1 -0
#   Add I2C driver for Motorola I2C
#
# drivers/i2c/busses/Kconfig
#   2004/07/01 17:52:54+01:00 [email protected] +11 -0
#   Add I2C driver for Motorola I2C
#
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig	Thu Jul  1 22:20:58 2004
+++ b/drivers/i2c/busses/Kconfig	Thu Jul  1 22:20:58 2004
@@ -396,4 +396,15 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-voodoo3.

+config I2C_MPC
+ tristate "MPC107/824x/85xx"
+ depends on I2C && FSL_OCP
+ help

+	  If you say yes to this option, support will be included for the
+	  built-in I2C interface on the MPC107/Tsi107/MPC8240/MPC8245 and
+	  MPC85xx family processors.

+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mpc.
+
endmenu

diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
@@ -32,6 +32,7 @@
 obj-$(CONFIG_I2C_VOODOO3)	+= i2c-voodoo3.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
 obj-$(CONFIG_SCx200_I2C)	+= scx200_i2c.o
+obj-$(CONFIG_I2C_MPC)	+= i2c-mpc.o
 
 ifeq ($(CONFIG_I2C_DEBUG_BUS),y)
 EXTRA_CFLAGS += -DDEBUG

The MPC5200 seems happy too ;) There is a eeprom on the board and it's detected and I can access it. If I find a soldering iron, I'll hook up another I2C device to test other devices.

Some gotchas though :

- The FDR computation is completly different. Here is the code I attached the code I used in my I2C driver. Basically, it takes as argument the desired I2C clock rate ( was a module parameter ) and the internal bus frequency and then compute the FDR.
- During the scan, the first bus is scanned without devices and the error when debug is active is I2C_MAL. From the moment the eeprom is detected, the error code become I2C_NORXACK for the address wihout device. Included dmesg

drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x57 - 1 of 1 messages drivers/i2c/busses/i2c-mpc.c: I2C: MAL
i2c_adapter i2c-1: found normal i2c_range entry for adapter 1, addr 0050 i2c_adapter i2c-1: master_xfer: with 1 msgs. drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x50 - 1 of 1 messages i2c_adapter i2c-1: master_xfer: with 1 msgs. drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x50 - 1 of 1 messages i2c_adapter i2c-1: client [eeprom] registered to adapter registering 1-0050
i2c_adapter i2c-1: found normal i2c_range entry for adapter 1, addr 0051 i2c_adapter i2c-1: master_xfer: with 1 msgs. drivers/i2c/busses/i2c-mpc.c: Doing write 0 bytes to 0x51 - 1 of 1 messages drivers/i2c/busses/i2c-mpc.c: I2C: No RXAK
i2c_adapter i2c-1: found normal i2c_range entry for adapter 1, addr 0052 i2c_adapter i2c-1: master_xfer: with 1 msgs.

- The interrupt bit. BTW, on what event should the interrupt be fired ? (I did my test without any interrupts )

Yes, something like FS_I2C_IS_MPC52xx

Another remark : The register setup are done at every _start. Couldn't they be done once for all during init ( just after the probe, call a init_hardware functions ).

Sylvain Munaut

static int
mpc52xx_i2c_get_best_fdr(int ipb_freq, int i2c_speed)

{
	/* Consts */
	const struct { int scl2tap; int tap2tap; } x2taps[] = {
		{   4,   1 },
		{   4,   2 },
		{   6,   4 },
		{   6,   8 },
		{  14,  16 },
		{  30,  32 },
		{  62,  64 },
		{ 126, 128 },
	};
	const int scl_taps[] = { 9, 10, 12, 15, 5, 6, 7, 8 };

	int best_i, best_j, i, j;
	int scl;
	int best_diff = 0x7fffffff, diff;
	int fdr;

	for ( i=7 ; i>=0 ; i-- ) {
		for ( j=0 ; j<8 ; j++ ) {
			scl = 2 * (x2taps[i].scl2tap + ((scl_taps[j] - 1) * x2taps[i].tap2tap) + 2);
			/* We only want frequency BELOW or EQUAL to */
			/* the target frequency */
			diff = i2c_speed * scl - ipb_freq;
			if ( (diff > 0) && (diff < best_diff) ) {
				best_diff = diff;
				best_i = i;
				best_j = j;
			}
		}
	}

	fdr = ((best_i << 2) | (best_j & 0x03) | ((best_j & 0x04) << 5)) & 0x3f;
	return fdr;
}

The 5200 seems to have a different FDR implementation. I could include your calculation code if you can tell me where the value of ipb_freq should come from.

I don't currently do a calculation on the 107, but pick a slow value for safety. I'd need more platform specific knowledge to do the calculation, as the 107/824x scale from the SDRAM clock, and the 85xx scales from the CCB clock.

Are you sure that it changes from MAL to NORXACK after seeing a device? It looks to me that it returns MAL only on the bus without devices, probably due to lack of pull-up resistors.

To use my driver, just set IE1 and IE2. The other flags are for a different approach using DMA to load the TX and RX registers. That would give a lower CPU overhead, but would also require a separate driver.

In fact, the reset value of this register is correct. The easiest fix may be for platform code to ensure that the register has the correct value, in case boot firmware changed it.

Yes, the frequency settings could be done once on startup. I might try that.

- Adrian Cox
Humboldt Solutions Ltd.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Hi

That would be nice.
The ipb_freq value is given by the boot loader or computed at run-time if the boot loader don't give it.

#include <asm/ppcboot.h>
extern bd_t __res;
u32 ipbfreq = __res.bi_ipbfreq;

But this field will only exists when :
- CONFIG_PPC_MPC52xx symbol is defined.
- When the MPC52xx patch is applied to the kernel

Also, there is no DFSRR register on the 5200.

Well yes, I guess that where the I2C modules takes it's clock from is dependent of each platform.

Yes, indeed, looking at the schematics indicates that it's not even I2C and it's disabled by the port multiplexing config. I've suppressed that entry in my ocp definition.

Are you sure ? If I don't set the BNBE (Bus Not Busy Enable) bit, I just get timeouts.

Yes sure, that's the easiest way. It's just that I'd like to avoid it. Especially when it's content is dependent on if the user has choosed to use irq or not.
But It's sure is a pity that the register is shared between the two I2C ... Because even with a flag, the driver should be passed the address of this register, and what bits to use.

Thanks.
Computing the FDR each time is a little overkill ;)

Sylvain Munaut

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

It looks to me that supporting the 5200 will require a lot of small changes.

Maybe we should define two more fields in the ocp_fs_i2c_data structure:
one for base clock, and one for i2c clock. Then platform code could fill in the clocks as necessary.

I noticed that. I don't think anybody ever used anything but the default value on the other chips.

interrupts, which the driver has no way to handle. Could you try enabling the interrupts, and see if this happens?

How about putting a function pointer for platform interrupt enabling and disabling into the ocp_fs_i2c_data?

- Adrian Cox
Humboldt Solutions Ltd.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Hi

Yes, that's a good idea. It would need a flag to tell which clock computation to take though.
Maybe instead of passing the baseclock, giving a pointer to a u32 that contains the clock would be more appropriate. Since all board have probably an int somewhere holding that value, you can put de definition in the ocp_def without any further modifications.

Yes. I've seen that the DFSRR is not as the same place. So maybe we would need a two bits flag
like
#define FS_I2C_NO_DFSRR 0x00
#define FS_I2C_PACKED_DFSRR 0x02
#define FS_I2C_SEPARATE_DFSRR 0x04

Well, you're right setting the BNBE is not right, it just hang because interrupts are fired and not handled.
Now, here is the interesting part :
If I set the ocp_def to use both I2C controller, defining the I2C1 before I2C2, it works fine. Now if I just invert the entry, it does not. Interrupts for I2C2 are never fired.
If I only put I2C1, it seems to work ( no timout). With only I2C2, interrupts are never fired ...

I have no clue why ! The best option is just to set IRQ to OCP_IRQ_NA, and it works fine, whatever the value of the shared interrupt registers is.

Well, for the interrupt yes that a quirk. The solution of dropping interrupt completly for it is the best option for now.
For the DFSRR register, the solution mentionned above is clean, the 3 chips requires different implementation anyway.
For the clock thing, I think it's good to be able to set the I2C clock.

Sylvain Munaut

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Interesting, it really looks like the MPC5200 I2C controller. Unfortunatly :
- On the MPC5200, all the status/controls bits are in the MSBs ( so the consts would need to be << 24 )
- And there is a interrupt control register I don't see in your code. It's shared by the two I2C controllers onboard.

I'll try to see if I can make it work on the lite5200 board.

Sylvain Munaut

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

The blocks look very similar, you might want to look at the follow up patch I just sent that abstracts the read/writes. It might be as simple as abstracting another read/write function for your particular platform.

Regards,
Matthew

--
Matthew McClintock <[email protected]>

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Yes
I'd say they are identical in fact. I kind of forgot that readl/writel does a endianness correction. So in fact this driver should work for the MPC5200.

My only concern is about interrupts.
MPC5200 has two I2C port:
* One is MBAR + 0x3D00
* Second is MBAR + 0x3D40

At MBAR + 0x3D20 there is an interrupt control register for the I2C ports ( shared, 4bits for the first port, 4bits for the second). It controls for what reason an I2C interrupt should be fired. I don't know if there is a similar register on the other, I'll look up the datasheets.

Sylvain Munaut

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Are you sure about that? The MPC107 registers are little-endian PCI registers, accessed with readl/writel, but it looks to me that the MPC5200 registers are big-endian.

That could easily be added as a special case.

Please do try. It would be good if one driver covered all these chips.

I'm going to try byte reads and writes on the MPC107 as my next experiment. I think that may avoid the need for separate access functions.

- Adrian Cox

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jul 01, 2004 at 07:19:15PM +0100, Adrian Cox wrote:

[snip]