Re: I2c-ocore linux driver debug

Re: I2c-ocore linux driver debug

[Peter Korsgaard]

>>>>> "FM" == Fabien Marteau <lepingouin.tux@gmail.com> writes:

Hi,

Discussions about the Linux i2c drivers belongs on the
linux-i2c@vger.kernel.org list, let's move discussion there.

FM> I integrated the IP i2c ocore for my platform ARMadeus apf9328
FM> (http://www.armadeus.com/wiki).  ARMadeus platform is an embedded
FM> system with a processor i.MXL and a FPGA Spartan3.

FM> The interface between the processor and FPGA is a 16bits data bus,
FM> and it's wired to do only 16bits read/write. Thus, I had to write
FM> a wrapper to convert 8bits ocore interface in 16bits
FM> interface. This wrapper mutiplex data bus to keep data in low part
FM> of the 16bits bus (see attachement) and add an ID register to
FM> unsure driver that the component is configured correctly.

FM> I had two problems with the driver:
FM> * Registers addresses.
FM> * To send stop command, driver have to set stop bit *before*
FM> sending the last byte. In the driver, this stop bit is set at the
FM> end of command, and that doesn't work.

FM> I was really surprised of this bugs, because this IP is old and I
FM> feel I am the only one that have these problems. Maybe the problem
FM> come from wrapper but I don't know why.

FM> I modified the driver to fix these bugs (see diff in attachment), and that
FM> work well with two components (LIS3LV02DL accelerometer and DS28CZ04 EEPROM)
FM> and testbench provided in attachment.

FM> Addresses registers problem
FM> ===========================

FM> The original driver give these addresses:
FM> /* registers */
FM> #define OCI2C_PRELOW		0
FM> #define OCI2C_PREHIGH		1
FM> #define OCI2C_CONTROL		2
FM> #define OCI2C_DATA		3
FM> #define OCI2C_CMD		4 /* write only */
FM> #define OCI2C_STATUS		4 /* read only, same address as OCI2C_CMD */

That fits the specs (i2c_specs.pdf, by Richard Herveille)

FM> After lot of tests, I found that this addresses work better:
FM> /* registers */
FM> #define OCI2C_PRELOW		0
FM> #define OCI2C_PREHIGH		1
FM> #define OCI2C_CONTROL		2
FM> #define OCI2C_CMD			4
FM> #define OCI2C_DATA_TXR		5
FM> #define OCI2C_STATUS		6
FM> #define OCI2C_DATA_RXR		7

Strange, seems like something went wrong in your wrapper.

FM> Stop command
FM> ============

FM> The most important problem I had was the stop command. To send a command like
FM> write quick SMBus (see Documentation/i2c/smbus-protocol) :

FM> S Addr [Rd/Wr] P

FM> The stop bit must be set at the same time that start bit. To do
FM> that I modified
FM> the xfer function:
FM> 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
FM> by:
FM> 	if(i2c->msg->len == 0)
FM> 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START|OCI2C_CMD_STOP);
FM> 	else
FM> 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);

It could be that we have a problem with zero length transfers (don't
have access to any smbus devices) - Could you please send a proper
patch in diff -urpN format?

-- 
Bye, Peter Korsgaard

Re: [oc] I2c-ocore linux driver debug

[Fabien Marteau]

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

Hi,

> Discussions about the Linux i2c drivers belongs on the
> linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org list, let's move discussion there.
>

Ok, I wrote on opencore list because I made a little modification in
VHDL design (wrapper).

>
> It could be that we have a problem with zero length transfers (don't
> have access to any smbus devices) - Could you please send a proper
> patch in diff -urpN format?
>

Done

--
Bye, FabM

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: i2c-ocores.diff --]
[-- Type: text/x-diff; name=i2c-ocores.diff, Size: 5189 bytes --]

--- ../../../../../../buildroot/project_build_armv4t/apf9328/linux-2.6.27.2/drivers/i2c/busses/i2c-ocores.c	2008-10-10 00:13:53.000000000 +0200
+++ i2c-ocores.c	2009-01-14 17:11:03.000000000 +0100
@@ -36,26 +36,30 @@ struct ocores_i2c {
 #define OCI2C_PRELOW		0
 #define OCI2C_PREHIGH		1
 #define OCI2C_CONTROL		2
-#define OCI2C_DATA		3
-#define OCI2C_CMD		4 /* write only */
-#define OCI2C_STATUS		4 /* read only, same address as OCI2C_CMD */
-
-#define OCI2C_CTRL_IEN		0x40
-#define OCI2C_CTRL_EN		0x80
-
-#define OCI2C_CMD_START		0x91
-#define OCI2C_CMD_STOP		0x41
-#define OCI2C_CMD_READ		0x21
-#define OCI2C_CMD_WRITE		0x11
-#define OCI2C_CMD_READ_ACK	0x21
-#define OCI2C_CMD_READ_NACK	0x29
-#define OCI2C_CMD_IACK		0x01
-
-#define OCI2C_STAT_IF		0x01
-#define OCI2C_STAT_TIP		0x02
-#define OCI2C_STAT_ARBLOST	0x20
-#define OCI2C_STAT_BUSY		0x40
-#define OCI2C_STAT_NACK		0x80
+#define OCI2C_CMD			4 /* write only */
+#define OCI2C_DATA_TXR		5
+#define OCI2C_STATUS		6 /* read only, same address as OCI2C_CMD */
+#define OCI2C_DATA_RXR		7
+
+
+#define OCI2C_CTRL_IEN		(0x40) 
+#define OCI2C_CTRL_EN		(0x80)
+
+#define OCI2C_CMD_START		(0x91)
+#define OCI2C_CMD_STOP		(0x41)
+#define OCI2C_CMD_READ		(0x21)
+#define OCI2C_CMD_WRITE		(0x11)
+#define OCI2C_CMD_READ_ACK	(0x21)
+#define OCI2C_CMD_READ_NACK	(0x29)
+#define OCI2C_CMD_IACK		(0x01)
+#define OCI2C_CMD_NACK		(0x08)
+#define OCI2C_CMD_ACK		(0x00)
+
+#define OCI2C_STAT_IF		(0x01)
+#define OCI2C_STAT_TIP		(0x02)
+#define OCI2C_STAT_ARBLOST	(0x20)
+#define OCI2C_STAT_BUSY		(0x40)
+#define OCI2C_STAT_NACK		(0x80)
 
 #define STATE_DONE		0
 #define STATE_START		1
@@ -65,12 +69,12 @@ struct ocores_i2c {
 
 static inline void oc_setreg(struct ocores_i2c *i2c, int reg, u8 value)
 {
-	iowrite8(value, i2c->base + reg * i2c->regstep);
+	iowrite16((u16)value, i2c->base + reg * i2c->regstep);
 }
 
 static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
 {
-	return ioread8(i2c->base + reg * i2c->regstep);
+	return (u8)ioread16(i2c->base + reg * i2c->regstep);
 }
 
 static void ocores_process(struct ocores_i2c *i2c)
@@ -78,6 +82,15 @@ static void ocores_process(struct ocores
 	struct i2c_msg *msg = i2c->msg;
 	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
 
+	/* manage SMBus quick command */
+	if ((i2c->state == STATE_START)&&(i2c->msg->len==0)){
+		if(stat&OCI2C_STAT_NACK){
+			i2c->state = STATE_ERROR;
+		}else{
+			i2c->state = STATE_DONE;
+		}
+	}
+
 	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
 		/* stop has been sent */
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
@@ -88,7 +101,7 @@ static void ocores_process(struct ocores
 	/* error? */
 	if (stat & OCI2C_STAT_ARBLOST) {
 		i2c->state = STATE_ERROR;
-		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP|OCI2C_CMD_READ);
 		return;
 	}
 
@@ -98,11 +111,11 @@ static void ocores_process(struct ocores
 
 		if (stat & OCI2C_STAT_NACK) {
 			i2c->state = STATE_ERROR;
-			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP|OCI2C_CMD_READ);
 			return;
 		}
 	} else
-		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
+		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA_RXR);
 
 	/* end of msg? */
 	if (i2c->pos == msg->len) {
@@ -121,7 +134,7 @@ static void ocores_process(struct ocores
 
 				i2c->state = STATE_START;
 
-				oc_setreg(i2c, OCI2C_DATA, addr);
+				oc_setreg(i2c, OCI2C_DATA_TXR, addr);
 				oc_setreg(i2c, OCI2C_CMD,  OCI2C_CMD_START);
 				return;
 			} else
@@ -129,17 +142,21 @@ static void ocores_process(struct ocores
 					? STATE_READ : STATE_WRITE;
 		} else {
 			i2c->state = STATE_DONE;
-			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
+			wake_up(&i2c->wait);
 			return;
 		}
 	}
 
 	if (i2c->state == STATE_READ) {
-		oc_setreg(i2c, OCI2C_CMD, i2c->pos == (msg->len-1) ?
-			  OCI2C_CMD_READ_NACK : OCI2C_CMD_READ_ACK);
-	} else {
-		oc_setreg(i2c, OCI2C_DATA, msg->buf[i2c->pos++]);
-		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_WRITE);
+		oc_setreg(i2c, OCI2C_CMD, ((i2c->pos == (msg->len-1)) ?
+			  (OCI2C_CMD_READ_NACK|OCI2C_CMD_STOP) : OCI2C_CMD_READ_ACK));
+		return;
+	} else {/* STATE_WRITE */
+		oc_setreg(i2c, OCI2C_DATA_TXR, msg->buf[i2c->pos++]);
+		oc_setreg(i2c, OCI2C_CMD, ((i2c->pos == msg->len) ?
+			(OCI2C_CMD_WRITE|OCI2C_CMD_STOP):OCI2C_CMD_WRITE));
+		return;
 	}
 }
 
@@ -161,11 +178,15 @@ static int ocores_xfer(struct i2c_adapte
 	i2c->nmsgs = num;
 	i2c->state = STATE_START;
 
-	oc_setreg(i2c, OCI2C_DATA,
+	oc_setreg(i2c, OCI2C_DATA_TXR,
 			(i2c->msg->addr << 1) |
 			((i2c->msg->flags & I2C_M_RD) ? 1:0));
 
-	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
+	if(i2c->msg->len == 0)
+		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START|OCI2C_CMD_STOP);
+	else
+		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
+
 
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
 			       (i2c->state == STATE_DONE), HZ))
@@ -215,7 +236,7 @@ static int __devinit ocores_i2c_probe(st
 	struct ocores_i2c *i2c;
 	struct ocores_i2c_platform_data *pdata;
 	struct resource *res, *res2;
-	int ret;
+	int ret=0;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)