[pacth] I2C bug fixes for L-O and L-Z

[pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

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

Hi,

Recently one custom board was having some I2C issues. In looking at it a couple things stood out.  I've attached a patches for l-o and l-z for anyone to comment on which cares.

Bug Fixes:

        -1- OMAP_I2C_WE_STC_WE should never be enabled for wake up unless I+F clk is cut.  Having it set like code does can mess up I2C FSM.

        -2- STT/STP bits are best written together.  There was a 2430-es1'ish errata which might have existed.  This is not the case in 3430.  There should be no need to use b_hw = 1.
                -a- Any write to I2C_CON is bad before ARDY.  An untimely write can mess up I2C FSM for next write.

        -3- There is a possible issue which a double clear of ARDY status in irq handler can make sure doesn't happen.

Note: When using I2C with HS mode there is a target bus capacitance for proper operation.  Depending on your board the optimal pull up resister level might very.  On OMAP3 there seems to be a few options which can allow you to play with effective resistance with out a hardware mod (and potential get better quality).
        - CONTROL block has pull-up control for special internal pulls
        - Padconf has normal pull-ups
        - T2 has internal controllable pull-ups (default on)
        - Your board might have an external pull up.

Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>

Regards,
Richard W.

[-- Attachment #2: i2c_lo.diff --]
[-- Type: application/octet-stream, Size: 3954 bytes --]

diff --git a/arch/arm/mach-integrator/clock.h b/arch/arm/mach-integrator/clock.h
deleted file mode 100644
index e69de29..0000000
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c95368c..ce39e7f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -91,9 +91,13 @@
 #define OMAP_I2C_STAT_NACK	(1 << 1)	/* No ack interrupt enable */
 #define OMAP_I2C_STAT_AL	(1 << 0)	/* Arbitration lost int ena */
 
-/* I2C WE wakeup enable register */
+/* I2C WE wakeup enable register 
+ * Don't use WE_STC unless I+F clocks are OFF else bad FSM
+ */
 #define OMAP_I2C_WE_XDR_WE	(1 << 14)	/* TX drain wakup */
 #define OMAP_I2C_WE_RDR_WE	(1 << 13)	/* RX drain wakeup */
+#define OMAP_I2C_WE_ROVR_WE	(1 << 11)	/* RX overflow wakeup */
+#define OMAP_I2C_WE_XUDF_WE	(1 << 10)	/* TX underflow wakeup */
 #define OMAP_I2C_WE_AAS_WE	(1 << 9)	/* Address as slave wakeup*/
 #define OMAP_I2C_WE_BF_WE	(1 << 8)	/* Bus free wakeup */
 #define OMAP_I2C_WE_STC_WE	(1 << 6)	/* Start condition wakeup */
@@ -104,10 +108,11 @@
 #define OMAP_I2C_WE_AL_WE	(1 << 0)	/* Arbitration lost wakeup */
 
 #define OMAP_I2C_WE_ALL		(OMAP_I2C_WE_XDR_WE | OMAP_I2C_WE_RDR_WE | \
+				OMAP_I2C_WE_ROVR_WE | OMAP_I2C_WE_ROVR_WE | \
 				OMAP_I2C_WE_AAS_WE | OMAP_I2C_WE_BF_WE | \
-				OMAP_I2C_WE_STC_WE | OMAP_I2C_WE_GC_WE | \
-				OMAP_I2C_WE_DRDY_WE | OMAP_I2C_WE_ARDY_WE | \
-				OMAP_I2C_WE_NACK_WE | OMAP_I2C_WE_AL_WE)
+				OMAP_I2C_WE_GC_WE | OMAP_I2C_WE_DRDY_WE | \
+				OMAP_I2C_WE_ARDY_WE | OMAP_I2C_WE_NACK_WE | \
+				OMAP_I2C_WE_AL_WE)
 
 /* I2C Buffer Configuration Register (OMAP_I2C_BUF): */
 #define OMAP_I2C_BUF_RDMA_EN	(1 << 15)	/* RX DMA channel enable */
@@ -265,6 +270,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	unsigned long fclk_rate = 12000000;
 	unsigned long timeout;
 	unsigned long internal_clk = 0;
+	int delay_count = 100;
 
 	if (dev->rev >= OMAP_I2C_REV_2) {
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, SYSC_SOFTRESET_MASK);
@@ -279,7 +285,10 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 						"for controller reset\n");
 				return -ETIMEDOUT;
 			}
-			msleep(1);
+			if( --delay_count > 0 )
+				udelay(1);
+			else    
+				msleep(1);
 		}
 
 		/* SYSC register is cleared by the reset; rewrite it */
@@ -408,14 +417,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
 {
 	unsigned long timeout;
-
+	int delay_count = 100;
+	
 	timeout = jiffies + OMAP_I2C_TIMEOUT;
 	while (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) & OMAP_I2C_STAT_BB) {
 		if (time_after(jiffies, timeout)) {
 			dev_warn(dev->dev, "timeout waiting for bus ready\n");
 			return -ETIMEDOUT;
 		}
-		msleep(1);
+		if( --delay_count > 0 )
+			udelay(1);
+		else    
+			msleep(1);
 	}
 
 	return 0;
@@ -486,7 +499,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 			}
 			cpu_relax();
 		}
-
+		/* FIXME: should consider ARDY value before writing to I2C_CON */
 		w |= OMAP_I2C_CON_STP;
 		w &= ~OMAP_I2C_CON_STT;
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
@@ -673,8 +686,11 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			err |= OMAP_I2C_STAT_AL;
 		}
 		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
-					OMAP_I2C_STAT_AL))
-			omap_i2c_complete_cmd(dev, err);
+					OMAP_I2C_STAT_AL)){
+				/* errata: ARDY needs double clear for some hardware */
+				omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, OMAP_I2C_STAT_ARDY);	
+				omap_i2c_complete_cmd(dev, err);
+		}
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {
@@ -846,9 +862,11 @@ omap_i2c_probe(struct platform_device *pdev)
 		 * call back latencies.
 		 */
 		dev->fifo_size = (dev->fifo_size / 2);
-		dev->b_hw = 1; /* Enable hardware fixes */
 	}
 
+	if (cpu_is_omap2430())
+		dev->b_hw = 1; /* Enable hardware fix (might be es1 only) */
+
 	/* reset ASAP, clearing any IRQs */
 	omap_i2c_init(dev);
 

[-- Attachment #3: i2c_lz.diff --]
[-- Type: application/octet-stream, Size: 4044 bytes --]

diff --git a/arch/arm/mach-omap2/board-ldp.c b/arch/arm/mach-omap2/board-ldp.c
index 722d1bc..c799695 100755
--- a/arch/arm/mach-omap2/board-ldp.c
+++ b/arch/arm/mach-omap2/board-ldp.c
@@ -805,7 +805,7 @@ static struct i2c_board_info __initdata ldp_i2c_boardinfo_2[] = {
 
 static int __init omap_i2c_init(void)
 {
-	omap_register_i2c_bus(1, 400, ldp_i2c_boardinfo,
+	omap_register_i2c_bus(1, 2600, ldp_i2c_boardinfo,
 			ARRAY_SIZE(ldp_i2c_boardinfo));
 	omap_register_i2c_bus(2, 100, ldp_i2c_boardinfo_2,
 			ARRAY_SIZE(ldp_i2c_boardinfo_2));
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 9a05419..0bd8332 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -91,7 +91,9 @@
 #define OMAP_I2C_STAT_NACK	(1 << 1)	/* No ack interrupt enable */
 #define OMAP_I2C_STAT_AL	(1 << 0)	/* Arbitration lost int ena */
 
-/* I2C WE wakeup enable register */
+/* I2C WE wakeup enable register 
+ * Don't use WE_STC unless I+F clocks are OFF else bad FSM
+ */
 #define OMAP_I2C_WE_XDR_WE	(1 << 14)	/* TX drain wakup */
 #define OMAP_I2C_WE_RDR_WE	(1 << 13)	/* RX drain wakeup */
 #define OMAP_I2C_WE_ROVR_WE	(1 << 11)	/* RX overflow wakeup */
@@ -108,9 +110,9 @@
 #define OMAP_I2C_WE_ALL		(OMAP_I2C_WE_XDR_WE | OMAP_I2C_WE_RDR_WE | \
 				OMAP_I2C_WE_ROVR_WE | OMAP_I2C_WE_ROVR_WE | \
 				OMAP_I2C_WE_AAS_WE | OMAP_I2C_WE_BF_WE | \
-				OMAP_I2C_WE_STC_WE | OMAP_I2C_WE_GC_WE | \
-				OMAP_I2C_WE_DRDY_WE | OMAP_I2C_WE_ARDY_WE | \
-				OMAP_I2C_WE_NACK_WE | OMAP_I2C_WE_AL_WE)
+				OMAP_I2C_WE_GC_WE | OMAP_I2C_WE_DRDY_WE | \
+				OMAP_I2C_WE_ARDY_WE | OMAP_I2C_WE_NACK_WE | \
+				OMAP_I2C_WE_AL_WE)
 
 /* I2C Buffer Configuration Register (OMAP_I2C_BUF): */
 #define OMAP_I2C_BUF_RDMA_EN	(1 << 15)	/* RX DMA channel enable */
@@ -284,6 +286,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	unsigned long fclk_rate = 12000000;
 	unsigned long timeout;
 	unsigned long internal_clk = 0;
+	int delay_count = 100;
 
 	if (dev->rev >= OMAP_I2C_REV_2) {
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, SYSC_SOFTRESET_MASK);
@@ -298,7 +301,10 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 						"for controller reset\n");
 				return -ETIMEDOUT;
 			}
-			msleep(1);
+			if( --delay_count > 0 )
+				udelay(1);
+			else    
+				msleep(1);
 		}
 
 		/* SYSC register is cleared by the reset; rewrite it */
@@ -428,14 +434,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
 {
 	unsigned long timeout;
-
+	int delay_count = 100;
+	
 	timeout = jiffies + OMAP_I2C_TIMEOUT;
 	while (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) & OMAP_I2C_STAT_BB) {
 		if (time_after(jiffies, timeout)) {
 			dev_warn(dev->dev, "timeout waiting for bus ready\n");
 			return -ETIMEDOUT;
 		}
-		msleep(1);
+		if( --delay_count > 0 )
+			udelay(1);
+		else    
+			msleep(1);
 	}
 
 	return 0;
@@ -506,7 +516,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 			}
 			cpu_relax();
 		}
-
+		/* FIXME: should consider ARDY value before writing to I2C_CON */
 		w |= OMAP_I2C_CON_STP;
 		w &= ~OMAP_I2C_CON_STT;
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
@@ -693,8 +703,11 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			err |= OMAP_I2C_STAT_AL;
 		}
 		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
-					OMAP_I2C_STAT_AL))
-			omap_i2c_complete_cmd(dev, err);
+					OMAP_I2C_STAT_AL)){
+				/* errata: ARDY needs double clear for some hardware */
+				omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, OMAP_I2C_STAT_ARDY);	
+				omap_i2c_complete_cmd(dev, err);
+		}
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {
@@ -866,9 +879,11 @@ omap_i2c_probe(struct platform_device *pdev)
 		 * call back latencies.
 		 */
 		dev->fifo_size = (dev->fifo_size / 2);
-		dev->b_hw = 1; /* Enable hardware fixes */
 	}
 
+	if (cpu_is_omap2430())
+		dev->b_hw = 1; /* Enable hardware fix (might be es1 only) */
+
 	/* reset ASAP, clearing any IRQs */
 	omap_i2c_init(dev);
 

Re: [pacth] I2C bug fixes for L-O and L-Z

[Aaro Koskinen]

Hello,

Woodruff, Richard wrote:
> Recently one custom board was having some I2C issues. In looking at it a couple things stood out.
> I've attached a patches for l-o and l-z for anyone to comment on which cares.

I hope these fixes are not ignored. Maybe they should be resent as they 
are missing from patchwork and there's some formatting issues.

A.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Eero Nurkkala]

Could you please also address the question of the load on the SCL line?

In my understanding, many boards are using the experimental values
from TRM for SCLL, SCLH, HSSCLL and HSSCLH. I have notices that those
"default" values fail to provide a standard I2C clock for, at least,
boards that I have tested. Should those values be board specific and
taken off from the I2C driver? Or should a note be added there saying
"These values are board specific and they depend on the load on the SCL
line. Please measure and validate correct values for your board".

RE: [pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

Hi,

> Could you please also address the question of the load on the SCL line?

Are you talking about rise/fall time?

I can comment on bits I picked up reading and talking to others.

The current data sheets I looked at give a couple kinds of guidance for resistor usage. One type has you measure bus capacitance then gives optimized resistor values per speed range in a capacitance range. The other type gives rise/fall which you should measure on board then choose the correct value for your board.

You need to have a low enough value resistor such that it pulls voltage quickly enough. One tradeoff is the lower the value the more current flows per bit.

> In my understanding, many boards are using the experimental values
> from TRM for SCLL, SCLH, HSSCLL and HSSCLH. I have notices that those
> "default" values fail to provide a standard I2C clock for, at least,
> boards that I have tested. Should those values be board specific and
> taken off from the I2C driver? Or should a note be added there saying
> "These values are board specific and they depend on the load on the SCL
> line. Please measure and validate correct values for your board".

So yes, how fast your I2C bus can safely run is board dependent.  However, depending on resistor choice you might have full range.

The way the resistor/capacitive ranges are defined if you're good for HS I guess you should be good for fast and standard speed.

You should be engaging your board people if what you are measuring is not to your needs. If the board has only put in a FS value then HS operation might be limited.  In that case you might need tables as you say in question.

* The idea I had when looking at this is you might be able to make use of the 3 internal pull-ups which exist on the path (and possible external pullup) to make sort of a resistive network.  This might allow you to lower resistance when running at high speeds and make it higher at slower ones (to save power).  This should expand speed range even if your board designer only put a FS resistor value in.

Does that help?

Regards,
Richard W.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Eero Nurkkala]

On Mon, 2009-02-16 at 15:19 +0100, ext Woodruff, Richard wrote:
> Hi,
> 
> > Could you please also address the question of the load on the SCL line?
> 
> Are you talking about rise/fall time?
> 

Sorry for being unclear;

The I2C standard addresses also rise/fall times, but more interesting,
the tLow and tHigh (and a number of other parameters).

It seems with the open source drivers, that somehow they're after a
"balanced I2C clock" meaning tLow == tHigh, which is very, very
dangerous.

If you look at the I2C standard, you see that actually tLow is
about twice as large as tHigh! (that is closer to truth than
the balanced clock)

So I'm talking about the registers I2C_SCLL and I2C_SCLH. If they
have the TRM or "open source" values, then it's very likely 
the I2C clock is out of standard. The SCLL (tLow) is in danger
for being far too short.

The I2C chip manufacturers consider the I2C standard as basis for
any proper operation. They're not after "balanced" I2C clock.

So I wish I could get some comments on the SCLL and SCLH, TRM,
open source and obeying I2C standard.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

> From: Eero Nurkkala [mailto:ext-eero.nurkkala@nokia.com]

> On Mon, 2009-02-16 at 15:19 +0100, ext Woodruff, Richard wrote:
> > Hi,
> >
> > > Could you please also address the question of the load on the SCL line?
> >
> > Are you talking about rise/fall time?
> >
> Sorry for being unclear;
>
> The I2C standard addresses also rise/fall times, but more interesting,
> the tLow and tHigh (and a number of other parameters).
>
> It seems with the open source drivers, that somehow they're after a
> "balanced I2C clock" meaning tLow == tHigh, which is very, very
> dangerous.

Oh, I see. I didn't look at that angle. I can inquire/look.

For sure wave forms on O-Scope and LA showed variable data line times, sometimes longer then one would expect (for stop/start/ack).  But yes, clocks were pretty even iirc.

Regards,
Richard W.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

Eero,

> > From: Eero Nurkkala [mailto:ext-eero.nurkkala@nokia.com]
>
> > On Mon, 2009-02-16 at 15:19 +0100, ext Woodruff, Richard wrote:
> > > Hi,
> > >
> > > > Could you please also address the question of the load on the SCL line?
> > >
> > > Are you talking about rise/fall time?
> > >
> > Sorry for being unclear;
> >
> > The I2C standard addresses also rise/fall times, but more interesting,
> > the tLow and tHigh (and a number of other parameters).
> >
> > It seems with the open source drivers, that somehow they're after a
> > "balanced I2C clock" meaning tLow == tHigh, which is very, very
> > dangerous.
>
> Oh, I see. I didn't look at that angle. I can inquire/look.
>
> For sure wave forms on O-Scope and LA showed variable data line times,
> sometimes longer then one would expect (for stop/start/ack).  But yes, clocks
> were pretty even iirc.

I received the following comment from a HW Apps person whom has dealt with this at the board level.

<comment start>
Attached is the I2C spec that I have.  As I understand it, the I2C only specify the minimum tLow and tHigh (which is not "balanced").  However what is more important is that the appropriate setup and hold time are followed.

If the equal time still meets the setup/hold time then there should not be any issue from a compliance standpoint.
<comment end>

Regards,
Richard W.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Eero Nurkkala]

On Fri, 2009-02-20 at 21:59 +0100, ext Woodruff, Richard wrote:
> 
> I received the following comment from a HW Apps person whom has dealt with this at the board level.
> 
> <comment start>
> Attached is the I2C spec that I have.  As I understand it, the I2C only specify the minimum tLow and tHigh (which is not "balanced").  However what is more important is that the appropriate setup and hold time are followed.
> 
> If the equal time still meets the setup/hold time then there should not be any issue from a compliance standpoint.
> <comment end>
> 
> Regards,
> Richard W.

Hi,

With out board, the tLow and tHigh values are in line with I2C standard
with the patch:
http://marc.info/?l=linux-omap&m=122770723311340&w=2

Would that risk the setup and hold times? (If I remember correctly, the
values (setup, hold) were within the I2C standard even with the patch.)

I think it's a risk not to meet tLow and tHigh. I'm saying, with open
source values with our board, the tLow was not in standard. If using
TRM minimum values, things get even worse. Why? because it states, for
example, Fast Mode SCLL = 5 and SCLH = 7. This means that the low
period is smaller than the high! Shouldn't it be vice versa? (scope
verified).

- Eero