[Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

This patch improves the recovery of the MPC's I2C bus from errors like bus
hangs resulting in timeouts:
1. make the bus timeout configurable, as it depends on the bus clock and
    the attached slave chip(s); default is still 1 second;
2. detect any of the cases indicated by the CF, BB and RXAK MSR flags if a
    timeout occurs, and add a missing (required) MAL reset;
3. use a more reliable method to fixup the bus if a hang has been detected.
    The sequence is sent 9 times which seems to be necessary if a slave
    "misses" more than one clock cycle.  For 400 kHz bus speed, the fixup i=
s
    also ~70us (81us vs. 150us) faster.

Tested on a custom Lite5200b derived board, with a Dallas RTC, AD sensors
and NXP IO expander chips attached to the i2c.

Changes vs. v1:
- use improved bus fixup sequence for all chips (not only the 5200)
- calculate real clock from defaults if no clock is given in the device tre=
e
- better description (I hope) of the changes.

I didn't split the changes in this file into three parts as recommended by
Grant, as they actually belong together (i.e. they address one single
problem, just in three places of one single source file).

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

---

Note about the reset sequence: I verified the waveforms for 18.4kHz, 85.9kH=
z
and 375kHz (drop me a note if you want to see scope screen shots).  Not
verified on other mpc chips yet.
@Ira: thanks in advance for giving it a try on your box!


--- 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-02-17 16:23:11.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 {
@@ -93,20 +94,23 @@ static irqreturn_t mpc_i2c_isr(int irq,
  /* Sometimes 9th clock pulse isn't generated, and slave doesn't release
   * the bus, because it wants to send ACK.
   * Following sequence of enabling/disabling and sending start/stop genera=
tes
- * the pulse, so it's all OK.
+ * the 9 pulses, so it's all OK.
   */
  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);
+	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);
+	}
  }

  static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
@@ -186,15 +190,19 @@ 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 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;

-	if (!clock)
+	if (!clock) {
+		/* see below - default fdr =3D 0x3f -> div =3D 2048 */
+		*real_clk =3D mpc5xxx_get_bus_frequency(node) / 2048;
  		return -EINVAL;
+	}

  	/* Determine divider value */
  	divider =3D mpc5xxx_get_bus_frequency(node) / clock;
@@ -212,7 +220,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
  			break;
  	}

-	return div ? (int)div->fdr : -EINVAL;
+	*real_clk =3D mpc5xxx_get_bus_frequency(node) / div->divider;
+	return (int)div->fdr;
  }

  static void mpc_i2c_setclock_52xx(struct device_node *node,
@@ -221,13 +230,14 @@ static void mpc_i2c_setclock_52xx(struct
  {
  	int ret, fdr;

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

  	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);

  	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,
@@ -287,14 +297,18 @@ u32 mpc_i2c_get_sec_cfg_8xxx(void)
  	return val;
  }

-int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, u32 prescale=
r)
+int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, u32 prescale=
r,
+			 u32 *real_clk)
  {
  	const struct mpc_i2c_divider *div =3D NULL;
  	u32 divider;
  	int i;

-	if (!clock)
+	if (!clock) {
+		/* see below - default fdr =3D 0x1031 -> div =3D 16 * 3072 */
+		*real_clk =3D fsl_get_sys_freq() / prescaler / (16 * 3072);
  		return -EINVAL;
+	}

  	/* Determine proper divider value */
  	if (of_device_is_compatible(node, "fsl,mpc8544-i2c"))
@@ -317,6 +331,7 @@ int mpc_i2c_get_fdr_8xxx(struct device_n
  			break;
  	}

+	*real_clk =3D fsl_get_sys_freq() / prescaler / div->divider;
  	return div ? (int)div->fdr : -EINVAL;
  }

@@ -326,7 +341,7 @@ static void mpc_i2c_setclock_8xxx(struct
  {
  	int ret, fdr;

-	ret =3D mpc_i2c_get_fdr_8xxx(node, clock, prescaler);
+	ret =3D mpc_i2c_get_fdr_8xxx(node, clock, prescaler, &i2c->real_clk);
  	fdr =3D (ret >=3D 0) ? ret : 0x1031; /* backward compatibility */

  	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
@@ -334,7 +349,7 @@ static void mpc_i2c_setclock_8xxx(struct

  	if (ret >=3D 0)
  		dev_info(i2c->dev, "clock %d Hz (dfsrr=3D%d fdr=3D%d)\n",
-			 clock, fdr >> 8, fdr & 0xff);
+			 i2c->real_clk, fdr >> 8, fdr & 0xff);
  }

  #else /* !CONFIG_FSL_SOC */
@@ -446,10 +461,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 +559,14 @@ static int __devinit fsl_i2c_probe(struc
  		}
  	}

+	prop =3D of_get_property(op->node, "fsl,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);

  	i2c->adap =3D mpc_ops;

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Grant Likely]

On Wed, Feb 17, 2010 at 11:59 AM, Albrecht Dre=DF <albrecht.dress@arcor.de>=
 wrote:
> This patch improves the recovery of the MPC's I2C bus from errors like bu=
s
> hangs resulting in timeouts:
> 1. make the bus timeout configurable, as it depends on the bus clock and
> =A0 the attached slave chip(s); default is still 1 second;
> 2. detect any of the cases indicated by the CF, BB and RXAK MSR flags if =
a
> =A0 timeout occurs, and add a missing (required) MAL reset;
> 3. use a more reliable method to fixup the bus if a hang has been detecte=
d.
> =A0 The sequence is sent 9 times which seems to be necessary if a slave
> =A0 "misses" more than one clock cycle. =A0For 400 kHz bus speed, the fix=
up is
> =A0 also ~70us (81us vs. 150us) faster.
>
> Tested on a custom Lite5200b derived board, with a Dallas RTC, AD sensors
> and NXP IO expander chips attached to the i2c.
>
> Changes vs. v1:
> - use improved bus fixup sequence for all chips (not only the 5200)
> - calculate real clock from defaults if no clock is given in the device t=
ree
> - better description (I hope) of the changes.
>
> I didn't split the changes in this file into three parts as recommended b=
y
> Grant, as they actually belong together (i.e. they address one single
> problem, just in three places of one single source file).
>
> Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

>
> ---
>
> Note about the reset sequence: I verified the waveforms for 18.4kHz, 85.9=
kHz
> and 375kHz (drop me a note if you want to see scope screen shots). =A0Not
> verified on other mpc chips yet.
> @Ira: thanks in advance for giving it a try on your box!
>
>
> --- 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-02-17
> 16:23:11.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 {
> @@ -93,20 +94,23 @@ static irqreturn_t mpc_i2c_isr(int irq,
> =A0/* Sometimes 9th clock pulse isn't generated, and slave doesn't releas=
e
> =A0* the bus, because it wants to send ACK.
> =A0* Following sequence of enabling/disabling and sending start/stop gene=
rates
> - * the pulse, so it's all OK.
> + * the 9 pulses, so it's all OK.
> =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 int k;
> + =A0 =A0 =A0 u32 delay_val =3D 1000000 / i2c->real_clk + 1;
> +
> + =A0 =A0 =A0 if (delay_val < 2)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 delay_val =3D 2;
> +
> + =A0 =A0 =A0 for (k =3D 9; k; k--) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN)=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val << 1);
> + =A0 =A0 =A0 }
> =A0}
>
> =A0static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing=
)
> @@ -186,15 +190,19 @@ 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 /* see below - default fdr =3D 0x3f -> div =
=3D 2048 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *real_clk =3D mpc5xxx_get_bus_frequency(nod=
e) / 2048;
> =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 +220,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 +230,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,
> @@ -287,14 +297,18 @@ u32 mpc_i2c_get_sec_cfg_8xxx(void)
> =A0 =A0 =A0 =A0return val;
> =A0}
>
> -int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, u32
> prescaler)
> +int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, u32
> 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 =A0u32 divider;
> =A0 =A0 =A0 =A0int i;
>
> - =A0 =A0 =A0 if (!clock)
> + =A0 =A0 =A0 if (!clock) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* see below - default fdr =3D 0x1031 -> di=
v =3D 16 * 3072 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *real_clk =3D fsl_get_sys_freq() / prescale=
r / (16 * 3072);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> + =A0 =A0 =A0 }
>
> =A0 =A0 =A0 =A0/* Determine proper divider value */
> =A0 =A0 =A0 =A0if (of_device_is_compatible(node, "fsl,mpc8544-i2c"))
> @@ -317,6 +331,7 @@ int mpc_i2c_get_fdr_8xxx(struct device_n
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 *real_clk =3D fsl_get_sys_freq() / prescaler / div->divider=
;
> =A0 =A0 =A0 =A0return div ? (int)div->fdr : -EINVAL;
> =A0}
>
> @@ -326,7 +341,7 @@ static void mpc_i2c_setclock_8xxx(struct
> =A0{
> =A0 =A0 =A0 =A0int ret, fdr;
>
> - =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_8xxx(node, clock, prescaler);
> + =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_8xxx(node, clock, prescaler, &i2c->=
real_clk);
> =A0 =A0 =A0 =A0fdr =3D (ret >=3D 0) ? ret : 0x1031; /* backward compatibi=
lity */
>
> =A0 =A0 =A0 =A0writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
> @@ -334,7 +349,7 @@ static void mpc_i2c_setclock_8xxx(struct
>
> =A0 =A0 =A0 =A0if (ret >=3D 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_info(i2c->dev, "clock %d Hz (dfsrr=3D%=
d fdr=3D%d)\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0clock, fdr >> 8, fdr & 0=
xff);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0i2c->real_clk, fdr >> 8,=
 fdr & 0xff);
> =A0}
>
> =A0#else /* !CONFIG_FSL_SOC */
> @@ -446,10 +461,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 +559,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, "fsl,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;
>



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

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Joakim Tjernlund]

>
> This patch improves the recovery of the MPC's I2C bus from errors lik=
e bus
> hangs resulting in timeouts:
> 1. make the bus timeout configurable, as it depends on the bus clock =
and
>     the attached slave chip(s); default is still 1 second;
> 2. detect any of the cases indicated by the CF, BB and RXAK MSR flags=
 if a
>     timeout occurs, and add a missing (required) MAL reset;
> 3. use a more reliable method to fixup the bus if a hang has been det=
ected.
>     The sequence is sent 9 times which seems to be necessary if a sla=
ve
>     "misses" more than one clock cycle.  For 400 kHz bus speed, the f=
ixup is
>     also ~70us (81us vs. 150us) faster.
>
> Tested on a custom Lite5200b derived board, with a Dallas RTC, AD sen=
sors
> and NXP IO expander chips attached to the i2c.
>
> Changes vs. v1:
> - use improved bus fixup sequence for all chips (not only the 5200)
> - calculate real clock from defaults if no clock is given in the devi=
ce tree
> - better description (I hope) of the changes.
>
> I didn't split the changes in this file into three parts as recommend=
ed by
> Grant, as they actually belong together (i.e. they address one single=

> problem, just in three places of one single source file).
>
> Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>
>
> ---
>
> Note about the reset sequence: I verified the waveforms for 18.4kHz, =
85.9kHz
> and 375kHz (drop me a note if you want to see scope screen shots).  N=
ot
> verified on other mpc chips yet.
> @Ira: thanks in advance for giving it a try on your box!

Does this reset sequence also send a START condition for every clock?
The ideal I2C reset sequence should look like this:

	for(j =3D 0; j < 9; j++) {
		if(I2C_READ)
			send_start();
		I2C_SCL(0);
		I2C_DELAY;
		I2C_TRISTATE;
		I2C_SDA(1);
		I2C_DELAY;
		I2C_SCL(1);
		I2C_DELAY;
		I2C_DELAY;
	}
	send_stop();

static void send_start(void)
{
	I2C_DELAY;
	I2C_TRISTATE;
	I2C_SDA(1);
	I2C_DELAY;
	I2C_SCL(1);
	I2C_DELAY;
	I2C_SDA(0);
	I2C_ACTIVE;
	I2C_DELAY;
}

static void send_stop(void)
{
	I2C_SCL(0);
	I2C_DELAY;
	I2C_SDA(0);
	I2C_ACTIVE;
	I2C_DELAY;
	I2C_SCL(1);
	I2C_DELAY;
	I2C_TRISTATE;
	I2C_SDA(1);
	I2C_DELAY;
}

 Jocke=

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

[-- Attachment #1.1: Type: text/plain, Size: 897 bytes --]

Hi Joakim:

> Does this reset sequence also send a START condition for every clock?

Please see the attached scan from a scope output, showing the first two out of the 9 sequences at 375 kHz (that's what the 5200's divider makes from 400 kHz requested).  Resolution is 2us/div and 1V/div for both signals.  The waveform itself for each of the 9 sequences is exactly the same we had before with the old solution, just the timing is faster and adjusted to the ii2c clock, i.e. the /relative/ waveforms look identical for slower clocks.

Any insight if this is *really* correct would be great, as I'm not an i2c expert.  I can only say it reliably fixes the bus hangs I saw!

Thanks,
Albrecht.

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

[-- Attachment #2: i2c-fixup.png --]
[-- Type: image/png, Size: 17960 bytes --]

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Joakim Tjernlund]

"Albrecht Dre=DF" <albrecht.dress@arcor.de> wrote on 2010/02/18 10:09:2=
3:
>
> Hi Joakim:
>
> > Does this reset sequence also send a START condition for every cloc=
k?
>
> Please see the attached scan from a scope output, showing the first t=
wo out of
> the 9 sequences at 375 kHz (that's what the 5200's divider makes from=
 400 kHz
> requested).  Resolution is 2us/div and 1V/div for both signals.  The =
waveform
> itself for each of the 9 sequences is exactly the same we had before =
with the
> old solution, just the timing is faster and adjusted to the ii2c cloc=
k, i.e.
> the /relative/ waveforms look identical for slower clocks.
>
> Any insight if this is *really* correct would be great, as I'm not an=
 i2c
> expert.  I can only say it reliably fixes the bus hangs I saw!

Looks like you do a STOP then START each time SCL is high so yes you
do a START each SCL and a STOP too. Don't think the STOP will hurt thou=
gh.

Timing is OK for FAST-MODE(400kHz), cannot say for STANDARD-MODE though=

need a 100Khz scope img for that.

The times to look for are:
tHD;STA, tSU;STA, tSU;STO and tBUF
at least that is what I have identified.

   Jocke=

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Joakim Tjernlund]

> @@ -93,20 +94,23 @@ static irqreturn_t mpc_i2c_isr(int irq,
>   /* Sometimes 9th clock pulse isn't generated, and slave doesn't release
>    * the bus, because it wants to send ACK.
>    * Following sequence of enabling/disabling and sending start/stop generates
> - * the pulse, so it's all OK.
> + * the 9 pulses, so it's all OK.
>    */
>   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);
> +   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);
> +   }
>   }

I am curious, didn't old method work with by just wrapping
a for(k=9; k; k--) around it? How did the wave form look?

   Jocke

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Ira W. Snyder]

On Wed, Feb 17, 2010 at 07:59:14PM +0100, Albrecht Dreß wrote:
> This patch improves the recovery of the MPC's I2C bus from errors like bus
> hangs resulting in timeouts:
> 1. make the bus timeout configurable, as it depends on the bus clock and
>     the attached slave chip(s); default is still 1 second;
> 2. detect any of the cases indicated by the CF, BB and RXAK MSR flags if a
>     timeout occurs, and add a missing (required) MAL reset;
> 3. use a more reliable method to fixup the bus if a hang has been detected.
>     The sequence is sent 9 times which seems to be necessary if a slave
>     "misses" more than one clock cycle.  For 400 kHz bus speed, the fixup is
>     also ~70us (81us vs. 150us) faster.
> 
> Tested on a custom Lite5200b derived board, with a Dallas RTC, AD sensors
> and NXP IO expander chips attached to the i2c.
> 
> Changes vs. v1:
> - use improved bus fixup sequence for all chips (not only the 5200)
> - calculate real clock from defaults if no clock is given in the device tree
> - better description (I hope) of the changes.
> 
> I didn't split the changes in this file into three parts as recommended by
> Grant, as they actually belong together (i.e. they address one single
> problem, just in three places of one single source file).
> 
> Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>
> 
> ---
> 
> Note about the reset sequence: I verified the waveforms for 18.4kHz, 85.9kHz
> and 375kHz (drop me a note if you want to see scope screen shots).  Not
> verified on other mpc chips yet.
> @Ira: thanks in advance for giving it a try on your box!
> 

Did this series get forgotten about? I don't see it in bjdook's next-i2c
branch or in benh's next branch.

I've pulled these into my 2.6.31.13 kernel, and they seem to work fine.
You've got my Tested-by if you didn't get one from me already.

Ira

> --- 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-02-17 16:23:11.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 {
> @@ -93,20 +94,23 @@ static irqreturn_t mpc_i2c_isr(int irq,
>   /* Sometimes 9th clock pulse isn't generated, and slave doesn't release
>    * the bus, because it wants to send ACK.
>    * Following sequence of enabling/disabling and sending start/stop generates
> - * the pulse, so it's all OK.
> + * the 9 pulses, so it's all OK.
>    */
>   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);
> +	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 +190,19 @@ 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) {
> +		/* see below - default fdr = 0x3f -> div = 2048 */
> +		*real_clk = mpc5xxx_get_bus_frequency(node) / 2048;
>   		return -EINVAL;
> +	}
> 
>   	/* Determine divider value */
>   	divider = mpc5xxx_get_bus_frequency(node) / clock;
> @@ -212,7 +220,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 +230,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,
> @@ -287,14 +297,18 @@ u32 mpc_i2c_get_sec_cfg_8xxx(void)
>   	return val;
>   }
> 
> -int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, u32 prescaler)
> +int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, u32 prescaler,
> +			 u32 *real_clk)
>   {
>   	const struct mpc_i2c_divider *div = NULL;
>   	u32 divider;
>   	int i;
> 
> -	if (!clock)
> +	if (!clock) {
> +		/* see below - default fdr = 0x1031 -> div = 16 * 3072 */
> +		*real_clk = fsl_get_sys_freq() / prescaler / (16 * 3072);
>   		return -EINVAL;
> +	}
> 
>   	/* Determine proper divider value */
>   	if (of_device_is_compatible(node, "fsl,mpc8544-i2c"))
> @@ -317,6 +331,7 @@ int mpc_i2c_get_fdr_8xxx(struct device_n
>   			break;
>   	}
> 
> +	*real_clk = fsl_get_sys_freq() / prescaler / div->divider;
>   	return div ? (int)div->fdr : -EINVAL;
>   }
> 
> @@ -326,7 +341,7 @@ static void mpc_i2c_setclock_8xxx(struct
>   {
>   	int ret, fdr;
> 
> -	ret = mpc_i2c_get_fdr_8xxx(node, clock, prescaler);
> +	ret = mpc_i2c_get_fdr_8xxx(node, clock, prescaler, &i2c->real_clk);
>   	fdr = (ret >= 0) ? ret : 0x1031; /* backward compatibility */
> 
>   	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
> @@ -334,7 +349,7 @@ static void mpc_i2c_setclock_8xxx(struct
> 
>   	if (ret >= 0)
>   		dev_info(i2c->dev, "clock %d Hz (dfsrr=%d fdr=%d)\n",
> -			 clock, fdr >> 8, fdr & 0xff);
> +			 i2c->real_clk, fdr >> 8, fdr & 0xff);
>   }
> 
>   #else /* !CONFIG_FSL_SOC */
> @@ -446,10 +461,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 +559,14 @@ static int __devinit fsl_i2c_probe(struc
>   		}
>   	}
> 
> +	prop = of_get_property(op->node, "fsl,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;
> 

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

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

Am 06.05.10 00:09 schrieb(en) Ira W. Snyder:
> Did this series get forgotten about? I don't see it in bjdook's next-i2c branch or in benh's next branch.

It's not in Grant's 5xxx tree either - as it's specific for those processors, I think he might be the responsible person.  The patch is still in a "new" state in patchwork, btw.

> I've pulled these into my 2.6.31.13 kernel, and they seem to work fine. You've got my Tested-by if you didn't get one from me already.

Tanks a lot!

I think, though, the whole stuff has been discussed in depth in February, so I do not understand why it's still pending as "new".  Grant, did we miss something here?

Thanks, Albrecht.

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

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Grant Likely]

On Thu, May 6, 2010 at 7:54 PM, Albrecht Dre=DF <albrecht.dress@arcor.de> w=
rote:
> Am 06.05.10 00:09 schrieb(en) Ira W. Snyder:
>>
>> Did this series get forgotten about? I don't see it in bjdook's next-i2c
>> branch or in benh's next branch.
>
> It's not in Grant's 5xxx tree either - as it's specific for those
> processors, I think he might be the responsible person. =A0The patch is s=
till
> in a "new" state in patchwork, btw.
>
>> I've pulled these into my 2.6.31.13 kernel, and they seem to work fine.
>> You've got my Tested-by if you didn't get one from me already.
>
> Tanks a lot!
>
> I think, though, the whole stuff has been discussed in depth in February,=
 so
> I do not understand why it's still pending as "new". =A0Grant, did we mis=
s
> something here?

I generally let subsystem maintainers pick up the device driver
patches for embedded platforms I'm responsible for unless I'm asked to
do otherwise.  I think ben has asked me to take these through my tree.

g.

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

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

Am 06.05.10 20:06 schrieb(en) Grant Likely:
> > I think, though, the whole stuff has been discussed in depth in February, so
> > I do not understand why it's still pending as "new".  Grant, did we miss
> > something here?
> 
> I generally let subsystem maintainers pick up the device driver
> patches for embedded platforms I'm responsible for unless I'm asked to
> do otherwise.  I think ben has asked me to take these through my tree.

That's <http://git.secretlab.ca/?p=linux-2.6.git;a=shortlog>, isn't it?  Hmmm, didn't find it there... :-/

Best, Albrecht.

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

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Grant Likely]

On Sun, May 16, 2010 at 11:47 AM, Albrecht Dre=DF <albrecht.dress@arcor.de>=
 wrote:
> Am 06.05.10 20:06 schrieb(en) Grant Likely:
>>
>> > I think, though, the whole stuff has been discussed in depth in
>> > February, so
>> > I do not understand why it's still pending as "new". =A0Grant, did we =
miss
>> > something here?
>>
>> I generally let subsystem maintainers pick up the device driver
>> patches for embedded platforms I'm responsible for unless I'm asked to
>> do otherwise. =A0I think ben has asked me to take these through my tree.
>
> That's <http://git.secretlab.ca/?p=3Dlinux-2.6.git;a=3Dshortlog>, isn't i=
t?
> =A0Hmmm, didn't find it there... :-/

Ugh... Stupid typing too fast.  I meant to say, "I *don't* think ben
has asked me to take..."

Well this leaves a bit of a mess.  I'll make sure it gets in one way or ano=
ther.

g.

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

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

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

Am 19.05.10 18:02 schrieb(en) Grant Likely:
> > That's <http://git.secretlab.ca/?p=linux-2.6.git;a=shortlog>, isn't it?
> >  Hmmm, didn't find it there... :-/
> 
> Ugh... Stupid typing too fast.  I meant to say, "I *don't* think ben
> has asked me to take..."
> 
> Well this leaves a bit of a mess.  I'll make sure it gets in one way or another.

*ping*  Any news about this issue?

Cheers, Albrecht.

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

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[panpan2523]

Ash Shoes are one of the leading brands in the modern footwear industry. Lots
of people now prefer to wear shoes from this particular brand because they
become accustom to their high quality and superb fit. Ash has revolutionized
the footwear industry by combining it with the fashion industry.  Check out
this
<http://onlyashshoes.com/goods-109-Virgin-Bis-Sneaker-Blue-Denim-312023.html> 
These shoes will surely add a new dimension to your wardrobe. The shoes as
well as boots from Ash are manufactured using the most advanced shoe making
technology in combination with high quality materials and expert
craftsmanship. This helps in fulfilling the various requirements of
different people. You will get shoes that are ideal for both formal and
casual occasions. Innovative collections are introduced each season to
satisfy the consumers. Many of the shoes in their collection are both
practical and stylish, making them an excellent choice for day to day
use.<br /><br /><br />



--
View this message in context: http://linuxppc.10917.n7.nabble.com/Patch-v2-1-2-5200-mpc-improve-i2c-bus-error-recovery-tp9637p69233.html
Sent from the linuxppc-dev mailing list archive at Nabble.com.

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

Hi Joakim:

[snip]
> >   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);
> > +   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
> I am curious, didn't old method work with by just wrapping
> a for(k=3D9; k; k--) around it? How did the wave form look?

Sure does that work!  The waveform was somewhat "streched", mainly due to t=
he delays between some of the writeccr() calls which don't change the sda/s=
cl lines.  Unfortunately I didn't take shots from the scope.

However, for *one* cycle, the old code needed (only counting the udelay's) =
150 us.  For 9 cycles, it's 1.35 ms, which isn't really nice ;-).  At 375 k=
Hz real clock rate, delay_val is 3, i.e. each cycle consumes 9 us, or 81 us=
 for the whole fixup procedure.  If the clock is slower, the gain is of cou=
rse a lot smaller, and at 20.5 kHz each cycle again needs 150 us...

My feeling is that the delays used in the old code are just "some" values w=
hich work for sure, to if you like, my change is basically optimisation...

BTW, related to your earlier question, I checked the timings recorded with =
the scope at 100 and at 20 kHz against the nxp's "I2C bus specification and=
 user manual", rev. 03 - everything seems to be fine.

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

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Joakim Tjernlund]

"Albrecht Dre=DF" <albrecht.dress@arcor.de> wrote on 2010/02/18 16:04:1=
6:
>
> Hi Joakim:
>
> [snip]
> > >   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);
> > > +   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);
> > > +   }
> > >   }
> >
> > I am curious, didn't old method work with by just wrapping
> > a for(k=3D9; k; k--) around it? How did the wave form look?
>
> Sure does that work!  The waveform was somewhat "streched", mainly du=
e to the
> delays between some of the writeccr() calls which don't change the sd=
a/scl
> lines.  Unfortunately I didn't take shots from the scope.

Yeah, the long delays has to go. So the wave form was the same but more=
 stretched
in time? I ask because I don't understand the writeccr(i2c, CCR_MSTA | =
CCR_MTX);
is supposed to do.

>
> However, for *one* cycle, the old code needed (only counting the udel=
ay's) 150
> us.  For 9 cycles, it's 1.35 ms, which isn't really nice ;-).  At 375=
 kHz real
> clock rate, delay_val is 3, i.e. each cycle consumes 9 us, or 81 us f=
or the
> whole fixup procedure.  If the clock is slower, the gain is of course=
 a lot
> smaller, and at 20.5 kHz each cycle again needs 150 us...
>
> My feeling is that the delays used in the old code are just "some" va=
lues
> which work for sure, to if you like, my change is basically optimisat=
ion...

The old code only works when the device is stuck at the last bit. To co=
pe
with any bit (worst case is the first bit) you need 9 cycles, 8 bits + =
ack =3D 9

Just toggling the clock 9 cycles should unlock any slave stuck in a rea=
d operation.
To unlock slaves stuck in a write operation you also need to generate a=
 START in
every cycle too.

As far as I can tell your patch does all of the above so

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

>
> BTW, related to your earlier question, I checked the timings recorded=
 with the
> scope at 100 and at 20 kHz against the nxp's "I2C bus specification a=
nd user
> manual", rev. 03 - everything seems to be fine.

Good, thanks.

 Jocke=

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Grant Likely]

On Thu, Feb 18, 2010 at 10:14 AM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> "Albrecht Dre=DF" <albrecht.dress@arcor.de> wrote on 2010/02/18 16:04:16:
> As far as I can tell your patch does all of the above so
>
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

Thanks Joakim.  On a point of process; "Signed-off-by" means that
you've actually handled the patch and forwarded it on, either by
reposting or putting it in a git tree for someone to pull.  The
Signed-off-by trail collects a history of all the people who have
touched a patch during its development and merging.

Reviewers can use Acked-by: or Reviewed-by: to indicate their consent.
 Ben can add your annotation when he merges the patch into his i2c
tree.

Cheers,
g.

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Joakim Tjernlund]

glikely@secretlab.ca wrote on 2010/02/18 18:41:40:
>
> On Thu, Feb 18, 2010 at 10:14 AM, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> > "Albrecht Dre=DF" <albrecht.dress@arcor.de> wrote on 2010/02/18 16:=
04:16:
> > As far as I can tell your patch does all of the above so
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>
> Thanks Joakim.  On a point of process; "Signed-off-by" means that
> you've actually handled the patch and forwarded it on, either by
> reposting or putting it in a git tree for someone to pull.  The
> Signed-off-by trail collects a history of all the people who have
> touched a patch during its development and merging.
>
> Reviewers can use Acked-by: or Reviewed-by: to indicate their consent=
.
>  Ben can add your annotation when he merges the patch into his i2c
> tree.

I see, then please change that to
Acked-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>


Thanks,
        Jocke=

Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery

[Albrecht Dreß]

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

Hi Joakim:

Am 18.02.10 18:14 schrieb(en) Joakim Tjernlund:
> > [snip]
> > > >   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);
> > > > +   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);
> > > > +   }
> > > >   }
> > >
>>> I am curious, didn't old method work with by just wrapping a for(k=9; k; k--) around it? How did the wave form look?
> >
>> Sure does that work!  The waveform was somewhat "streched", mainly due to the delays between some of the writeccr() calls which don't change the sda/scl lines.  Unfortunately I didn't take shots from the scope.
> 
> Yeah, the long delays has to go. So the wave form was the same but more stretched in time? I ask because I don't understand the writeccr(i2c, CCR_MSTA | CCR_MTX); is supposed to do.

Afaict, this is really a no-op.  The '5200 user's manual says about MEN

<snip>
* 0 module is reset and disabled. This is the Power-ON reset. When low the interface is held in reset, but registers can still be accessed.
* 1 I2C module is enabled. Bit must be set before other CR bits have any effect.
</snip>

The change in the MSTA is needed -with the proper delays- as to generate the START and STOP conditions.

Unfortunately, the data sheet is not very clear (or my English too bad), but reading it *after* seeing the signals on the scope, I can at least guess what they mean :-/

Thus, the old code will probably produce SDA and SCL held high for ~90us, then a SDA/SCL low for ~30us (plus/minus the delays the hw adds internally according to the clock setting), and then a ~30us SDA/SCL high.  It is not possible to get the necessary delays from the data sheet, but as I said I empirically verified some cases to be safe.

> The old code only works when the device is stuck at the last bit. To cope with any bit (worst case is the first bit) you need 9 cycles, 8 bits + ack = 9
> 
> Just toggling the clock 9 cycles should unlock any slave stuck in a read operation. To unlock slaves stuck in a write operation you also need to generate a START in every cycle too.

Yes, of course...  this was the initial motivation of the patch, as I *have* a slave which sometimes needs more than one cycle!

> As far as I can tell your patch does all of the above so
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

Thanks a lot again for your time, and your helpful comments!

Best, Albrecht.

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