[PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Albrecht Dreß]

Improve the recovery of the MPC5200B's I2C bus from errors like bus
hangs.

Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>

---

This patch introduces several improvements to the MPC5200B's I2C driver
as to improve the recovery from error conditions I encountered when
testing a custom board with several I2C devices attached (eeprom, io
expander, rtc, sensors).  The error conditions included cases where the
bus if logic of one slave apparently went south, blocking the bus
completely.

My fixes include:
1. make the bus timeout configurable in fsl_i2c_probe(); the default of
   one second is *way* too long for my use case;
2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
   if *any* of the CF, BB and RXAK flags in the MSR is 1.  I actually
   saw different combinations with hangs, not only all three set;
3. improve the fixup procedure by calculating the timing needed from the
   real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
   Furthermore, I issue 9 instead of one cycle, as I experienced cases
   where the single one is not enough (found this tip in a forum).  As a
   side effect, the new scheme needs only 81us @375kHz bus clock instead
   of 150us.  I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, all
   looking fine, which I can provide if anyone is interested.

Open questions:
- is the approach correct at all, in particular the interpretation of
  the flags (#2)?
- could this code also be used on non-5200 processors?

--- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c	2009-12-03 04:51:21.0000=
00000 +0100
+++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c	2010-01-22 16:05:13.000000000=
 +0100
@@ -59,6 +59,7 @@ struct mpc_i2c {
 	wait_queue_head_t queue;
 	struct i2c_adapter adap;
 	int irq;
+	u32 real_clk;
 };
=20
 struct mpc_i2c_divider {
@@ -97,16 +98,32 @@ static irqreturn_t mpc_i2c_isr(int irq,=20
  */
 static void mpc_i2c_fixup(struct mpc_i2c *i2c)
 {
-	writeccr(i2c, 0);
-	udelay(30);
-	writeccr(i2c, CCR_MEN);
-	udelay(30);
-	writeccr(i2c, CCR_MSTA | CCR_MTX);
-	udelay(30);
-	writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
-	udelay(30);
-	writeccr(i2c, CCR_MEN);
-	udelay(30);
+	if (i2c->real_clk =3D=3D 0) {
+		writeccr(i2c, 0);
+		udelay(30);
+		writeccr(i2c, CCR_MEN);
+		udelay(30);
+		writeccr(i2c, CCR_MSTA | CCR_MTX);
+		udelay(30);
+		writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
+		udelay(30);
+		writeccr(i2c, CCR_MEN);
+		udelay(30);
+	} else {
+		int k;
+		u32 delay_val =3D 1000000 / i2c->real_clk + 1;
+
+		if (delay_val < 2)
+			delay_val =3D 2;
+
+		for (k =3D 9; k; k--) {
+			writeccr(i2c, 0);
+			writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
+			udelay(delay_val);
+			writeccr(i2c, CCR_MEN);
+			udelay(delay_val << 1);
+		}
+	}
 }
=20
 static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
@@ -186,15 +203,18 @@ static const struct mpc_i2c_divider mpc_
 	{10240, 0x9d}, {12288, 0x9e}, {15360, 0x9f}
 };
=20
-int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int prescale=
r)
+int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int prescale=
r,
+			 u32 *real_clk)
 {
 	const struct mpc_i2c_divider *div =3D NULL;
 	unsigned int pvr =3D mfspr(SPRN_PVR);
 	u32 divider;
 	int i;
=20
-	if (!clock)
+	if (!clock) {
+		*real_clk =3D 0;
 		return -EINVAL;
+	}
=20
 	/* Determine divider value */
 	divider =3D mpc5xxx_get_bus_frequency(node) / clock;
@@ -212,7 +232,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
 			break;
 	}
=20
-	return div ? (int)div->fdr : -EINVAL;
+	*real_clk =3D mpc5xxx_get_bus_frequency(node) / div->divider;
+	return (int)div->fdr;
 }
=20
 static void mpc_i2c_setclock_52xx(struct device_node *node,
@@ -221,13 +242,14 @@ static void mpc_i2c_setclock_52xx(struct
 {
 	int ret, fdr;
=20
-	ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler);
+	ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler, &i2c->real_clk);
 	fdr =3D (ret >=3D 0) ? ret : 0x3f; /* backward compatibility */
=20
 	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
=20
 	if (ret >=3D 0)
-		dev_info(i2c->dev, "clock %d Hz (fdr=3D%d)\n", clock, fdr);
+		dev_info(i2c->dev, "clock %u Hz (fdr=3D%d)\n", i2c->real_clk,
+			 fdr);
 }
 #else /* !CONFIG_PPC_MPC52xx */
 static void mpc_i2c_setclock_52xx(struct device_node *node,
@@ -446,10 +468,14 @@ static int mpc_xfer(struct i2c_adapter *
 			return -EINTR;
 		}
 		if (time_after(jiffies, orig_jiffies + HZ)) {
+			u8 status =3D readb(i2c->base + MPC_I2C_SR);
+
 			dev_dbg(i2c->dev, "timeout\n");
-			if (readb(i2c->base + MPC_I2C_SR) =3D=3D
-			    (CSR_MCF | CSR_MBB | CSR_RXAK))
+			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) !=3D 0) {
+				writeb(status & ~CSR_MAL,
+				       i2c->base + MPC_I2C_SR);
 				mpc_i2c_fixup(i2c);
+			}
 			return -EIO;
 		}
 		schedule();
@@ -540,6 +566,14 @@ static int __devinit fsl_i2c_probe(struc
 		}
 	}
=20
+	prop =3D of_get_property(op->node, "timeout", &plen);
+	if (prop && plen =3D=3D sizeof(u32)) {
+		mpc_ops.timeout =3D *prop * HZ / 1000000;
+		if (mpc_ops.timeout < 5)
+			mpc_ops.timeout =3D 5;
+	}
+	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
+
 	dev_set_drvdata(&op->dev, i2c);
=20
 	i2c->adap =3D mpc_ops;

Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Ben Dooks]

On Fri, Jan 22, 2010 at 09:17:55PM +0100, Albrecht Dreß wrote:
> Improve the recovery of the MPC5200B's I2C bus from errors like bus
> hangs.

This is very sparse comapred to the large comment below the --- line,
maybe some more description should be living up here.

Is thios a candidate for an -rc or should it be left to merge window?
 
> Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>
> 
> ---
> 
> This patch introduces several improvements to the MPC5200B's I2C driver
> as to improve the recovery from error conditions I encountered when
> testing a custom board with several I2C devices attached (eeprom, io
> expander, rtc, sensors).  The error conditions included cases where the
> bus if logic of one slave apparently went south, blocking the bus
> completely.
> 
> My fixes include:
> 1. make the bus timeout configurable in fsl_i2c_probe(); the default of
>    one second is *way* too long for my use case;
> 2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
>    if *any* of the CF, BB and RXAK flags in the MSR is 1.  I actually
>    saw different combinations with hangs, not only all three set;
> 3. improve the fixup procedure by calculating the timing needed from the
>    real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
>    Furthermore, I issue 9 instead of one cycle, as I experienced cases
>    where the single one is not enough (found this tip in a forum).  As a
>    side effect, the new scheme needs only 81us @375kHz bus clock instead
>    of 150us.  I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, all
>    looking fine, which I can provide if anyone is interested.
> 
> Open questions:
> - is the approach correct at all, in particular the interpretation of
>   the flags (#2)?
> - could this code also be used on non-5200 processors?
> 
> --- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c	2009-12-03 04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c	2010-01-22 16:05:13.000000000 +0100
> @@ -59,6 +59,7 @@ struct mpc_i2c {
>  	wait_queue_head_t queue;
>  	struct i2c_adapter adap;
>  	int irq;
> +	u32 real_clk;
>  };
>  
>  struct mpc_i2c_divider {
> @@ -97,16 +98,32 @@ static irqreturn_t mpc_i2c_isr(int irq, 
>   */
>  static void mpc_i2c_fixup(struct mpc_i2c *i2c)
>  {
> -	writeccr(i2c, 0);
> -	udelay(30);
> -	writeccr(i2c, CCR_MEN);
> -	udelay(30);
> -	writeccr(i2c, CCR_MSTA | CCR_MTX);
> -	udelay(30);
> -	writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> -	udelay(30);
> -	writeccr(i2c, CCR_MEN);
> -	udelay(30);
> +	if (i2c->real_clk == 0) {
> +		writeccr(i2c, 0);
> +		udelay(30);
> +		writeccr(i2c, CCR_MEN);
> +		udelay(30);
> +		writeccr(i2c, CCR_MSTA | CCR_MTX);
> +		udelay(30);
> +		writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> +		udelay(30);
> +		writeccr(i2c, CCR_MEN);
> +		udelay(30);
> +	} else {
> +		int k;
> +		u32 delay_val = 1000000 / i2c->real_clk + 1;
> +
> +		if (delay_val < 2)
> +			delay_val = 2;
> +
> +		for (k = 9; k; k--) {
> +			writeccr(i2c, 0);
> +			writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> +			udelay(delay_val);
> +			writeccr(i2c, CCR_MEN);
> +			udelay(delay_val << 1);
> +		}
> +	}
>  }
>  
>  static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> @@ -186,15 +203,18 @@ static const struct mpc_i2c_divider mpc_
>  	{10240, 0x9d}, {12288, 0x9e}, {15360, 0x9f}
>  };
>  
> -int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int prescaler)
> +int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int prescaler,
> +			 u32 *real_clk)
>  {
>  	const struct mpc_i2c_divider *div = NULL;
>  	unsigned int pvr = mfspr(SPRN_PVR);
>  	u32 divider;
>  	int i;
>  
> -	if (!clock)
> +	if (!clock) {
> +		*real_clk = 0;
>  		return -EINVAL;
> +	}
>  
>  	/* Determine divider value */
>  	divider = mpc5xxx_get_bus_frequency(node) / clock;
> @@ -212,7 +232,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
>  			break;
>  	}
>  
> -	return div ? (int)div->fdr : -EINVAL;
> +	*real_clk = mpc5xxx_get_bus_frequency(node) / div->divider;
> +	return (int)div->fdr;
>  }
>  
>  static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -221,13 +242,14 @@ static void mpc_i2c_setclock_52xx(struct
>  {
>  	int ret, fdr;
>  
> -	ret = mpc_i2c_get_fdr_52xx(node, clock, prescaler);
> +	ret = mpc_i2c_get_fdr_52xx(node, clock, prescaler, &i2c->real_clk);
>  	fdr = (ret >= 0) ? ret : 0x3f; /* backward compatibility */
>  
>  	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
>  
>  	if (ret >= 0)
> -		dev_info(i2c->dev, "clock %d Hz (fdr=%d)\n", clock, fdr);
> +		dev_info(i2c->dev, "clock %u Hz (fdr=%d)\n", i2c->real_clk,
> +			 fdr);
>  }
>  #else /* !CONFIG_PPC_MPC52xx */
>  static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -446,10 +468,14 @@ static int mpc_xfer(struct i2c_adapter *
>  			return -EINTR;
>  		}
>  		if (time_after(jiffies, orig_jiffies + HZ)) {
> +			u8 status = readb(i2c->base + MPC_I2C_SR);
> +
>  			dev_dbg(i2c->dev, "timeout\n");
> -			if (readb(i2c->base + MPC_I2C_SR) ==
> -			    (CSR_MCF | CSR_MBB | CSR_RXAK))
> +			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
> +				writeb(status & ~CSR_MAL,
> +				       i2c->base + MPC_I2C_SR);
>  				mpc_i2c_fixup(i2c);
> +			}
>  			return -EIO;
>  		}
>  		schedule();
> @@ -540,6 +566,14 @@ static int __devinit fsl_i2c_probe(struc
>  		}
>  	}
>  
> +	prop = of_get_property(op->node, "timeout", &plen);
> +	if (prop && plen == sizeof(u32)) {
> +		mpc_ops.timeout = *prop * HZ / 1000000;
> +		if (mpc_ops.timeout < 5)
> +			mpc_ops.timeout = 5;
> +	}
> +	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
> +
>  	dev_set_drvdata(&op->dev, i2c);
>  
>  	i2c->adap = mpc_ops;

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Albrecht Dreß]

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

Am 25.01.10 05:06 schrieb(en) Ben Dooks:
> On Fri, Jan 22, 2010 at 09:17:55PM +0100, Albrecht Dreß wrote:
>> Improve the recovery of the MPC5200B's I2C bus from errors like bus hangs.
> 
> This is very sparse comapred to the large comment below the --- line, maybe some more description should be living up here.

Hmm, that was my interpretation of #15 in Documentation/SubmittingPatches... ;-)  Maybe it should read

<snip>
Improve the recovery of the MPC5200B's I2C bus from errors like bus hangs.  This includes making the bus timeout configurable, a better detection of cases where the bus has to be "fixed" after a timeout, and a more thorough fixup sequence.
</snip>

> Is thios a candidate for an -rc or should it be left to merge window?

Well, basically it was a rfc.  I apparently need it on my 5200B board, but I hoped to get some more insight from the Freescale/I2C gurus (see "open questions" in the post).  Thus merge window, IMHO...

Thanks, Albrecht.

> 
> > Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>
> >
> > ---
> >
> > This patch introduces several improvements to the MPC5200B's I2C driver
> > as to improve the recovery from error conditions I encountered when
> > testing a custom board with several I2C devices attached (eeprom, io
> > expander, rtc, sensors).  The error conditions included cases where the
> > bus if logic of one slave apparently went south, blocking the bus
> > completely.
> >
> > My fixes include:
> > 1. make the bus timeout configurable in fsl_i2c_probe(); the default of
> >    one second is *way* too long for my use case;
> > 2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
> >    if *any* of the CF, BB and RXAK flags in the MSR is 1.  I actually
> >    saw different combinations with hangs, not only all three set;
> > 3. improve the fixup procedure by calculating the timing needed from the
> >    real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
> >    Furthermore, I issue 9 instead of one cycle, as I experienced cases
> >    where the single one is not enough (found this tip in a forum).  As a
> >    side effect, the new scheme needs only 81us @375kHz bus clock instead
> >    of 150us.  I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, all
> >    looking fine, which I can provide if anyone is interested.
> >
> > Open questions:
> > - is the approach correct at all, in particular the interpretation of
> >   the flags (#2)?
> > - could this code also be used on non-5200 processors?
> >
> > --- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c	2009-12-03 04:51:21.000000000 +0100
> > +++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c	2010-01-22 16:05:13.000000000 +0100
[snip]

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Grant Likely]

Hi Albrecht,

Comments below.

On Fri, Jan 22, 2010 at 1:17 PM, Albrecht Dre=DF <albrecht.dress@arcor.de> =
wrote:
> Improve the recovery of the MPC5200B's I2C bus from errors like bus
> hangs.
>
> Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>
>
> ---
>
> This patch introduces several improvements to the MPC5200B's I2C driver
> as to improve the recovery from error conditions I encountered when
> testing a custom board with several I2C devices attached (eeprom, io
> expander, rtc, sensors). =A0The error conditions included cases where the
> bus if logic of one slave apparently went south, blocking the bus
> completely.
>
> My fixes include:
> 1. make the bus timeout configurable in fsl_i2c_probe(); the default of
> =A0 one second is *way* too long for my use case;
> 2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
> =A0 if *any* of the CF, BB and RXAK flags in the MSR is 1. =A0I actually
> =A0 saw different combinations with hangs, not only all three set;
> 3. improve the fixup procedure by calculating the timing needed from the
> =A0 real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
> =A0 Furthermore, I issue 9 instead of one cycle, as I experienced cases
> =A0 where the single one is not enough (found this tip in a forum). =A0As=
 a
> =A0 side effect, the new scheme needs only 81us @375kHz bus clock instead
> =A0 of 150us. =A0I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, al=
l
> =A0 looking fine, which I can provide if anyone is interested.

These are three separate fixes.  Ideally you should submit them in
separate patches to make it easy on poor old reviewers like me.  And,
as Ben mentions, this descriptions should be above the '---' line so
it appears in the commit text.

>
> Open questions:
> - is the approach correct at all, in particular the interpretation of
> =A0the flags (#2)?
> - could this code also be used on non-5200 processors?
>
> --- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c =A0 =A0 =A02009-12-03 =
04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c =A0 2010-01-22 16:05:13.000=
000000 +0100
> @@ -59,6 +59,7 @@ struct mpc_i2c {
> =A0 =A0 =A0 =A0wait_queue_head_t queue;
> =A0 =A0 =A0 =A0struct i2c_adapter adap;
> =A0 =A0 =A0 =A0int irq;
> + =A0 =A0 =A0 u32 real_clk;
> =A0};
>
> =A0struct mpc_i2c_divider {
> @@ -97,16 +98,32 @@ static irqreturn_t mpc_i2c_isr(int irq,
> =A0*/
> =A0static void mpc_i2c_fixup(struct mpc_i2c *i2c)
> =A0{
> - =A0 =A0 =A0 writeccr(i2c, 0);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> - =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 if (i2c->real_clk =3D=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN)=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int k;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 delay_val =3D 1000000 / i2c->real_clk +=
 1;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (delay_val < 2)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 delay_val =3D 2;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (k =3D 9; k; k--) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CC=
R_MTX | CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val << 1);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }

This doesn't look right.  Why is the old code being preserved?  Isn't
it not as reliable?  It looks to me that the new block should be the
only path, with delay_val getting hard set to a sane value if real_clk
=3D=3D 0.  This approach looks to add complexity to the driver without a
reason other than fear it *might* breaking something.

If the new code is better, then be strong, stand tall, and say in a
loud voice, "this old code is crap.  The new stuff is much better."

g.

> =A0}
>
> =A0static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing=
)
> @@ -186,15 +203,18 @@ static const struct mpc_i2c_divider mpc_
> =A0 =A0 =A0 =A0{10240, 0x9d}, {12288, 0x9e}, {15360, 0x9f}
> =A0};
>
> -int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int presca=
ler)
> +int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int presca=
ler,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 *real_clk)
> =A0{
> =A0 =A0 =A0 =A0const struct mpc_i2c_divider *div =3D NULL;
> =A0 =A0 =A0 =A0unsigned int pvr =3D mfspr(SPRN_PVR);
> =A0 =A0 =A0 =A0u32 divider;
> =A0 =A0 =A0 =A0int i;
>
> - =A0 =A0 =A0 if (!clock)
> + =A0 =A0 =A0 if (!clock) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *real_clk =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> + =A0 =A0 =A0 }
>
> =A0 =A0 =A0 =A0/* Determine divider value */
> =A0 =A0 =A0 =A0divider =3D mpc5xxx_get_bus_frequency(node) / clock;
> @@ -212,7 +232,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 return div ? (int)div->fdr : -EINVAL;
> + =A0 =A0 =A0 *real_clk =3D mpc5xxx_get_bus_frequency(node) / div->divide=
r;
> + =A0 =A0 =A0 return (int)div->fdr;
> =A0}
>
> =A0static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -221,13 +242,14 @@ static void mpc_i2c_setclock_52xx(struct
> =A0{
> =A0 =A0 =A0 =A0int ret, fdr;
>
> - =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler);
> + =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler, &i2c->=
real_clk);
> =A0 =A0 =A0 =A0fdr =3D (ret >=3D 0) ? ret : 0x3f; /* backward compatibili=
ty */
>
> =A0 =A0 =A0 =A0writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
>
> =A0 =A0 =A0 =A0if (ret >=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(i2c->dev, "clock %d Hz (fdr=3D%d)\=
n", clock, fdr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(i2c->dev, "clock %u Hz (fdr=3D%d)\=
n", i2c->real_clk,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fdr);
> =A0}
> =A0#else /* !CONFIG_PPC_MPC52xx */
> =A0static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -446,10 +468,14 @@ static int mpc_xfer(struct i2c_adapter *
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINTR;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (time_after(jiffies, orig_jiffies + HZ)=
) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 status =3D readb(i2c->ba=
se + MPC_I2C_SR);
> +
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_dbg(i2c->dev, "timeout=
\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (readb(i2c->base + MPC_I=
2C_SR) =3D=3D
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (CSR_MCF | CSR_MBB =
| CSR_RXAK))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((status & (CSR_MCF | CS=
R_MBB | CSR_RXAK)) !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeb(stat=
us & ~CSR_MAL,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0i2c->base + MPC_I2C_SR);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mpc_i2c_fi=
xup(i2c);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EIO;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0schedule();
> @@ -540,6 +566,14 @@ static int __devinit fsl_i2c_probe(struc
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 prop =3D of_get_property(op->node, "timeout", &plen);
> + =A0 =A0 =A0 if (prop && plen =3D=3D sizeof(u32)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_ops.timeout =3D *prop * HZ / 1000000;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (mpc_ops.timeout < 5)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_ops.timeout =3D 5;
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 100=
0000 / HZ);
> +
> =A0 =A0 =A0 =A0dev_set_drvdata(&op->dev, i2c);
>
> =A0 =A0 =A0 =A0i2c->adap =3D mpc_ops;
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Ira W. Snyder]

On Fri, Jan 22, 2010 at 09:17:55PM +0100, Albrecht Dreß wrote:
> Improve the recovery of the MPC5200B's I2C bus from errors like bus
> hangs.
> 
> Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>
> 
> ---
> 
> This patch introduces several improvements to the MPC5200B's I2C driver
> as to improve the recovery from error conditions I encountered when
> testing a custom board with several I2C devices attached (eeprom, io
> expander, rtc, sensors).  The error conditions included cases where the
> bus if logic of one slave apparently went south, blocking the bus
> completely.
> 
> My fixes include:
> 1. make the bus timeout configurable in fsl_i2c_probe(); the default of
>    one second is *way* too long for my use case;
> 2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
>    if *any* of the CF, BB and RXAK flags in the MSR is 1.  I actually
>    saw different combinations with hangs, not only all three set;

Hello Albrecht,

I see this exact hang on a MPC8349EA board. I poll my i2c sensors every
500ms, and it takes around 12 hours to produce a hang. The usual hang
has (CF | BB) set, however I have seen a hang with just BB (only once so
far in about 2 weeks).

I think the fixup should be run on 8349 as well, if not all processors.
I'm happy to test patches. I have a way to reliably trigger a lockup,
using another master on the i2c bus.

> 3. improve the fixup procedure by calculating the timing needed from the
>    real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
>    Furthermore, I issue 9 instead of one cycle, as I experienced cases
>    where the single one is not enough (found this tip in a forum).  As a
>    side effect, the new scheme needs only 81us @375kHz bus clock instead
>    of 150us.  I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, all
>    looking fine, which I can provide if anyone is interested.
> 
> Open questions:
> - is the approach correct at all, in particular the interpretation of
>   the flags (#2)?
> - could this code also be used on non-5200 processors?
> 
> --- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c	2009-12-03 04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c	2010-01-22 16:05:13.000000000 +0100
> @@ -59,6 +59,7 @@ struct mpc_i2c {
>  	wait_queue_head_t queue;
>  	struct i2c_adapter adap;
>  	int irq;
> +	u32 real_clk;
>  };
>  
>  struct mpc_i2c_divider {
> @@ -97,16 +98,32 @@ static irqreturn_t mpc_i2c_isr(int irq, 
>   */
>  static void mpc_i2c_fixup(struct mpc_i2c *i2c)
>  {
> -	writeccr(i2c, 0);
> -	udelay(30);
> -	writeccr(i2c, CCR_MEN);
> -	udelay(30);
> -	writeccr(i2c, CCR_MSTA | CCR_MTX);
> -	udelay(30);
> -	writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> -	udelay(30);
> -	writeccr(i2c, CCR_MEN);
> -	udelay(30);
> +	if (i2c->real_clk == 0) {
> +		writeccr(i2c, 0);
> +		udelay(30);
> +		writeccr(i2c, CCR_MEN);
> +		udelay(30);
> +		writeccr(i2c, CCR_MSTA | CCR_MTX);
> +		udelay(30);
> +		writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> +		udelay(30);
> +		writeccr(i2c, CCR_MEN);
> +		udelay(30);
> +	} else {
> +		int k;
> +		u32 delay_val = 1000000 / i2c->real_clk + 1;
> +
> +		if (delay_val < 2)
> +			delay_val = 2;
> +
> +		for (k = 9; k; k--) {
> +			writeccr(i2c, 0);
> +			writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> +			udelay(delay_val);
> +			writeccr(i2c, CCR_MEN);
> +			udelay(delay_val << 1);
> +		}
> +	}
>  }
>  

The old sequence has always un-hung the bus for me. Yours might be
better. I'll try it next time the bus wedges up on me.

>  static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> @@ -186,15 +203,18 @@ static const struct mpc_i2c_divider mpc_
>  	{10240, 0x9d}, {12288, 0x9e}, {15360, 0x9f}
>  };
>  
> -int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int prescaler)
> +int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int prescaler,
> +			 u32 *real_clk)
>  {
>  	const struct mpc_i2c_divider *div = NULL;
>  	unsigned int pvr = mfspr(SPRN_PVR);
>  	u32 divider;
>  	int i;
>  
> -	if (!clock)
> +	if (!clock) {
> +		*real_clk = 0;
>  		return -EINVAL;
> +	}
>  
>  	/* Determine divider value */
>  	divider = mpc5xxx_get_bus_frequency(node) / clock;
> @@ -212,7 +232,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
>  			break;
>  	}
>  
> -	return div ? (int)div->fdr : -EINVAL;
> +	*real_clk = mpc5xxx_get_bus_frequency(node) / div->divider;
> +	return (int)div->fdr;
>  }
>  
>  static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -221,13 +242,14 @@ static void mpc_i2c_setclock_52xx(struct
>  {
>  	int ret, fdr;
>  
> -	ret = mpc_i2c_get_fdr_52xx(node, clock, prescaler);
> +	ret = mpc_i2c_get_fdr_52xx(node, clock, prescaler, &i2c->real_clk);
>  	fdr = (ret >= 0) ? ret : 0x3f; /* backward compatibility */
>  
>  	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
>  
>  	if (ret >= 0)
> -		dev_info(i2c->dev, "clock %d Hz (fdr=%d)\n", clock, fdr);
> +		dev_info(i2c->dev, "clock %u Hz (fdr=%d)\n", i2c->real_clk,
> +			 fdr);
>  }
>  #else /* !CONFIG_PPC_MPC52xx */
>  static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -446,10 +468,14 @@ static int mpc_xfer(struct i2c_adapter *
>  			return -EINTR;
>  		}
>  		if (time_after(jiffies, orig_jiffies + HZ)) {
> +			u8 status = readb(i2c->base + MPC_I2C_SR);
> +
>  			dev_dbg(i2c->dev, "timeout\n");
> -			if (readb(i2c->base + MPC_I2C_SR) ==
> -			    (CSR_MCF | CSR_MBB | CSR_RXAK))
> +			if ((status & (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0) {
> +				writeb(status & ~CSR_MAL,
> +				       i2c->base + MPC_I2C_SR);
>  				mpc_i2c_fixup(i2c);
> +			}
>  			return -EIO;
>  		}
>  		schedule();

This hunk looks good to me. It is basically what I did, except I didn't
clear the MAL bit. Judging by the manual, it should be cleared.

> @@ -540,6 +566,14 @@ static int __devinit fsl_i2c_probe(struc
>  		}
>  	}
>  
> +	prop = of_get_property(op->node, "timeout", &plen);
> +	if (prop && plen == sizeof(u32)) {
> +		mpc_ops.timeout = *prop * HZ / 1000000;
> +		if (mpc_ops.timeout < 5)
> +			mpc_ops.timeout = 5;
> +	}
> +	dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1000000 / HZ);
> +
>  	dev_set_drvdata(&op->dev, i2c);
>  
>  	i2c->adap = mpc_ops;
> 

Ira

Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Albrecht Dreß]

Hi Grant & Ira:

Thanks a lot for reviewing the patch, and for the encouraging comments!  I =
will re-submit a new version according according to them, hopefully tomorro=
w or on Thursday.

Best, Albrecht.

----- Original Nachricht ----
Von:     Grant Likely <grant.likely@secretlab.ca>
An:      Albrecht Dre=DF <albrecht.dress@arcor.de>
Datum:   16.02.2010 20:31
Betreff: Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

> Hi Albrecht,
>=20
> Comments below.
>=20
> On Fri, Jan 22, 2010 at 1:17 PM, Albrecht Dre=DF <albrecht.dress@arcor.de=
>
> wrote:
> > Improve the recovery of the MPC5200B's I2C bus from errors like bus
> > hangs.
> >
> > Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>
> >
> > ---
> >
> > This patch introduces several improvements to the MPC5200B's I2C driver
> > as to improve the recovery from error conditions I encountered when
> > testing a custom board with several I2C devices attached (eeprom, io
> > expander, rtc, sensors). =A0The error conditions included cases where t=
he
> > bus if logic of one slave apparently went south, blocking the bus
> > completely.
> >
> > My fixes include:
> > 1. make the bus timeout configurable in fsl_i2c_probe(); the default of
> > =A0 one second is *way* too long for my use case;
> > 2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
> > =A0 if *any* of the CF, BB and RXAK flags in the MSR is 1. =A0I actuall=
y
> > =A0 saw different combinations with hangs, not only all three set;
> > 3. improve the fixup procedure by calculating the timing needed from th=
e
> > =A0 real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
> > =A0 Furthermore, I issue 9 instead of one cycle, as I experienced cases
> > =A0 where the single one is not enough (found this tip in a forum). =A0=
As a
> > =A0 side effect, the new scheme needs only 81us @375kHz bus clock inste=
ad
> > =A0 of 150us. =A0I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, =
all
> > =A0 looking fine, which I can provide if anyone is interested.
>=20
> These are three separate fixes.  Ideally you should submit them in
> separate patches to make it easy on poor old reviewers like me.  And,
> as Ben mentions, this descriptions should be above the '---' line so
> it appears in the commit text.

OK, will do.

>=20
> >
> > Open questions:
> > - is the approach correct at all, in particular the interpretation of
> > =A0the flags (#2)?
> > - could this code also be used on non-5200 processors?
> >
> > --- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c =A0 =A0 =A02009-12-0=
3
> 04:51:21.000000000 +0100
> > +++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c =A0 2010-01-22
> 16:05:13.000000000 +0100
> > @@ -59,6 +59,7 @@ struct mpc_i2c {
> > =A0 =A0 =A0 =A0wait_queue_head_t queue;
> > =A0 =A0 =A0 =A0struct i2c_adapter adap;
> > =A0 =A0 =A0 =A0int irq;
> > + =A0 =A0 =A0 u32 real_clk;
> > =A0};
> >
> > =A0struct mpc_i2c_divider {
> > @@ -97,16 +98,32 @@ static irqreturn_t mpc_i2c_isr(int irq,
> > =A0*/
> > =A0static void mpc_i2c_fixup(struct mpc_i2c *i2c)
> > =A0{
> > - =A0 =A0 =A0 writeccr(i2c, 0);
> > - =A0 =A0 =A0 udelay(30);
> > - =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> > - =A0 =A0 =A0 udelay(30);
> > - =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX);
> > - =A0 =A0 =A0 udelay(30);
> > - =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> > - =A0 =A0 =A0 udelay(30);
> > - =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> > - =A0 =A0 =A0 udelay(30);
> > + =A0 =A0 =A0 if (i2c->real_clk =3D=3D 0) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_ME=
N);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> > + =A0 =A0 =A0 } else {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int k;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 delay_val =3D 1000000 / i2c->real_clk=
 + 1;
> > +
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (delay_val < 2)
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 delay_val =3D 2;
> > +
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (k =3D 9; k; k--) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | =
CCR_MTX | CCR_MEN);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val << 1);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> > + =A0 =A0 =A0 }
>=20
> This doesn't look right.  Why is the old code being preserved?  Isn't
> it not as reliable?  It looks to me that the new block should be the
> only path, with delay_val getting hard set to a sane value if real_clk
> =3D=3D 0.  This approach looks to add complexity to the driver without a
> reason other than fear it *might* breaking something.
>=20
> If the new code is better, then be strong, stand tall, and say in a
> loud voice, "this old code is crap.  The new stuff is much better."

:-)  Ok...

>=20
> g.
>=20
> > =A0}
> >
> > =A0static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writi=
ng)
> > @@ -186,15 +203,18 @@ static const struct mpc_i2c_divider mpc_
> > =A0 =A0 =A0 =A0{10240, 0x9d}, {12288, 0x9e}, {15360, 0x9f}
> > =A0};
> >
> > -int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int
> prescaler)
> > +int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int
> prescaler,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 *real_clk)
> > =A0{
> > =A0 =A0 =A0 =A0const struct mpc_i2c_divider *div =3D NULL;
> > =A0 =A0 =A0 =A0unsigned int pvr =3D mfspr(SPRN_PVR);
> > =A0 =A0 =A0 =A0u32 divider;
> > =A0 =A0 =A0 =A0int i;
> >
> > - =A0 =A0 =A0 if (!clock)
> > + =A0 =A0 =A0 if (!clock) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *real_clk =3D 0;
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> > + =A0 =A0 =A0 }
> >
> > =A0 =A0 =A0 =A0/* Determine divider value */
> > =A0 =A0 =A0 =A0divider =3D mpc5xxx_get_bus_frequency(node) / clock;
> > @@ -212,7 +232,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> > =A0 =A0 =A0 =A0}
> >
> > - =A0 =A0 =A0 return div ? (int)div->fdr : -EINVAL;
> > + =A0 =A0 =A0 *real_clk =3D mpc5xxx_get_bus_frequency(node) / div->divi=
der;
> > + =A0 =A0 =A0 return (int)div->fdr;
> > =A0}
> >
> > =A0static void mpc_i2c_setclock_52xx(struct device_node *node,
> > @@ -221,13 +242,14 @@ static void mpc_i2c_setclock_52xx(struct
> > =A0{
> > =A0 =A0 =A0 =A0int ret, fdr;
> >
> > - =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler);
> > + =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler,
> &i2c->real_clk);
> > =A0 =A0 =A0 =A0fdr =3D (ret >=3D 0) ? ret : 0x3f; /* backward compatibi=
lity */
> >
> > =A0 =A0 =A0 =A0writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
> >
> > =A0 =A0 =A0 =A0if (ret >=3D 0)
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(i2c->dev, "clock %d Hz (fdr=3D%d=
)\n", clock, fdr);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(i2c->dev, "clock %u Hz (fdr=3D%d=
)\n",
> i2c->real_clk,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fdr);
> > =A0}
> > =A0#else /* !CONFIG_PPC_MPC52xx */
> > =A0static void mpc_i2c_setclock_52xx(struct device_node *node,
> > @@ -446,10 +468,14 @@ static int mpc_xfer(struct i2c_adapter *
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINTR;
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (time_after(jiffies, orig_jiffies + H=
Z)) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 status =3D readb(i2c->=
base + MPC_I2C_SR);
> > +
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_dbg(i2c->dev, "timeo=
ut\n");
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (readb(i2c->base + MPC=
_I2C_SR) =3D=3D
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (CSR_MCF | CSR_MB=
B | CSR_RXAK))
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((status & (CSR_MCF | =
CSR_MBB | CSR_RXAK)) !=3D
> 0) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeb(st=
atus & ~CSR_MAL,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0i2c->base + MPC_I2C_SR);
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mpc_i2c_=
fixup(i2c);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EIO;
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0schedule();
> > @@ -540,6 +566,14 @@ static int __devinit fsl_i2c_probe(struc
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> > =A0 =A0 =A0 =A0}
> >
> > + =A0 =A0 =A0 prop =3D of_get_property(op->node, "timeout", &plen);
> > + =A0 =A0 =A0 if (prop && plen =3D=3D sizeof(u32)) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_ops.timeout =3D *prop * HZ / 1000000;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (mpc_ops.timeout < 5)
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_ops.timeout =3D 5;
> > + =A0 =A0 =A0 }
> > + =A0 =A0 =A0 dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 1=
000000 /
> HZ);
> > +
> > =A0 =A0 =A0 =A0dev_set_drvdata(&op->dev, i2c);
> >
> > =A0 =A0 =A0 =A0i2c->adap =3D mpc_ops;
> >
> > _______________________________________________
> > devicetree-discuss mailing list
> > devicetree-discuss@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/devicetree-discuss
> >
>=20
>=20
>=20
> --=20
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>=20

Immer auf dem Laufenden! Sport, Auto, Reise, Politik und Promis. Von uns f=
=FCr Sie: der neue Arcor.de-Newsletter!
Jetzt anmelden und einfach alles wissen: http://www.arcor.de/rd/footer.news=
letter

Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery

[Albrecht Dreß]

Hi Ira:

[snip]
> I see this exact hang on a MPC8349EA board. I poll my i2c sensors every
> 500ms, and it takes around 12 hours to produce a hang. The usual hang
> has (CF | BB) set, however I have seen a hang with just BB (only once so
> far in about 2 weeks).
>=20
> I think the fixup should be run on 8349 as well, if not all processors.
> I'm happy to test patches. I have a way to reliably trigger a lockup,
> using another master on the i2c bus.

See my other post - hopefully a new patch is ready tomorrow/Thursday...

[snip]
> >  static void mpc_i2c_fixup(struct mpc_i2c *i2c)
> >  {
> > -=09writeccr(i2c, 0);
> > -=09udelay(30);
> > -=09writeccr(i2c, CCR_MEN);
> > -=09udelay(30);
> > -=09writeccr(i2c, CCR_MSTA | CCR_MTX);
> > -=09udelay(30);
> > -=09writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> > -=09udelay(30);
> > -=09writeccr(i2c, CCR_MEN);
> > -=09udelay(30);
> > +=09if (i2c->real_clk =3D=3D 0) {
> > +=09=09writeccr(i2c, 0);
> > +=09=09udelay(30);
> > +=09=09writeccr(i2c, CCR_MEN);
> > +=09=09udelay(30);
> > +=09=09writeccr(i2c, CCR_MSTA | CCR_MTX);
> > +=09=09udelay(30);
> > +=09=09writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> > +=09=09udelay(30);
> > +=09=09writeccr(i2c, CCR_MEN);
> > +=09=09udelay(30);
> > +=09} else {
> > +=09=09int k;
> > +=09=09u32 delay_val =3D 1000000 / i2c->real_clk + 1;
> > +
> > +=09=09if (delay_val < 2)
> > +=09=09=09delay_val =3D 2;
> > +
> > +=09=09for (k =3D 9; k; k--) {
> > +=09=09=09writeccr(i2c, 0);
> > +=09=09=09writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> > +=09=09=09udelay(delay_val);
> > +=09=09=09writeccr(i2c, CCR_MEN);
> > +=09=09=09udelay(delay_val << 1);
> > +=09=09}
> > +=09}
> >  }
> > =20
>=20
> The old sequence has always un-hung the bus for me. Yours might be
> better. I'll try it next time the bus wedges up on me.

The easiest way to verify the /waveforms/ is to simply call the function so=
mewhere else (e.g. in a i2c write call), and then record SCK and SDA with a=
 scope.  I saw *huge* unnecessary delays in the original code, and by some =
creative interpretation of the (not very clear) 5200 data sheet I came to t=
he write's and delays above.  If you have a scope, you might want to repeat=
 the test with your '8349.

Best, Albrecht.

Immer auf dem Laufenden! Sport, Auto, Reise, Politik und Promis. Von uns f=
=FCr Sie: der neue Arcor.de-Newsletter!
Jetzt anmelden und einfach alles wissen: http://www.arcor.de/rd/footer.news=
letter