Devicetree
 help / color / mirror / Atom feed
* [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
@ 2010-02-17 18:59 Albrecht Dreß
  2010-02-17 20:10 ` Grant Likely
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Albrecht Dreß @ 2010-02-17 18:59 UTC (permalink / raw)
  To: Linux PPC Development, Devicetree Discussions, Grant Likely, Ben 
  Cc: Ira W. Snyder

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-KvP5wT2u2U0@public.gmane.org>

---

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!


--- 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;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-17 18:59 [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery Albrecht Dreß
@ 2010-02-17 20:10 ` Grant Likely
  2010-02-18  8:09 ` Joakim Tjernlund
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: Grant Likely @ 2010-02-17 20:10 UTC (permalink / raw)
  To: Albrecht Dreß
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder

On Wed, Feb 17, 2010 at 11:59 AM, Albrecht Dreß <albrecht.dress@arcor.de> 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>

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

>
> ---
>
> 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!
>
>
> --- 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;
>



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-17 18:59 [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery Albrecht Dreß
  2010-02-17 20:10 ` Grant Likely
@ 2010-02-18  8:09 ` Joakim Tjernlund
       [not found] ` <OF3D88A7A0.9A4F1639-ONC12576CE.002C0FB9-C12576CE.002CCBBB-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: Joakim Tjernlund @ 2010-02-18  8:09 UTC (permalink / raw)
  To: Albrecht Dreß
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder

>
> 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!

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

	for(j = 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
       [not found] ` <OF3D88A7A0.9A4F1639-ONC12576CE.002C0FB9-C12576CE.002CCBBB-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
@ 2010-02-18  9:09   ` Albrecht Dreß
  2010-02-18 12:33     ` Joakim Tjernlund
  0 siblings, 1 reply; 17+ messages in thread
From: Albrecht Dreß @ 2010-02-18  9:09 UTC (permalink / raw)
  To: joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w
  Cc: linuxppc-dev-mnsaURCQ41sdnm+yROfE0A,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	ben-linux-elnMNo+KYs3YtjvyW6yDsg, iws-lulEs6mt1IksTUYHLfqkUA


[-- 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 --]

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-18  9:09   ` Albrecht Dreß
@ 2010-02-18 12:33     ` Joakim Tjernlund
  0 siblings, 0 replies; 17+ messages in thread
From: Joakim Tjernlund @ 2010-02-18 12:33 UTC (permalink / raw)
  To: Albrecht Dreß; +Cc: linuxppc-dev, devicetree-discuss, ben-linux, iws

"Albrecht Dreß" <albrecht.dress@arcor.de> wrote on 2010/02/18 10:09:23:
>
> 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!

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 though.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-17 18:59 [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery Albrecht Dreß
                   ` (2 preceding siblings ...)
       [not found] ` <OF3D88A7A0.9A4F1639-ONC12576CE.002C0FB9-C12576CE.002CCBBB-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
@ 2010-02-18 13:23 ` Joakim Tjernlund
  2010-05-05 22:09 ` Ira W. Snyder
  4 siblings, 0 replies; 17+ messages in thread
From: Joakim Tjernlund @ 2010-02-18 13:23 UTC (permalink / raw)
  To: Albrecht Dreß
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder


> @@ -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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
@ 2010-02-18 15:04 Albrecht Dreß
  2010-02-18 17:14 ` Joakim Tjernlund
  0 siblings, 1 reply; 17+ messages in thread
From: Albrecht Dreß @ 2010-02-18 15:04 UTC (permalink / raw)
  To: joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w
  Cc: linuxppc-dev-mnsaURCQ41sdnm+yROfE0A,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	ben-linux-elnMNo+KYs3YtjvyW6yDsg, iws-lulEs6mt1IksTUYHLfqkUA

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 = 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.

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 kHz 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 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" values which 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ür Sie: der neue Arcor.de-Newsletter!
Jetzt anmelden und einfach alles wissen: http://www.arcor.de/rd/footer.newsletter

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-18 15:04 Albrecht Dreß
@ 2010-02-18 17:14 ` Joakim Tjernlund
  2010-02-18 17:41   ` Grant Likely
       [not found]   ` <OFC412BED7.5D5934A3-ONC12576CE.005D5F44-C12576CE.005EBE55-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
  0 siblings, 2 replies; 17+ messages in thread
From: Joakim Tjernlund @ 2010-02-18 17:14 UTC (permalink / raw)
  To: Albrecht Dreß; +Cc: linuxppc-dev, devicetree-discuss, ben-linux, iws

"Albrecht Dreß" <albrecht.dress@arcor.de> wrote on 2010/02/18 16:04:16:
>
> 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 = 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.

>
> 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 kHz 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 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" values
> which work for sure, to if you like, my change is basically optimisation...

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.

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 and user
> manual", rev. 03 - everything seems to be fine.

Good, thanks.

 Jocke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-18 17:14 ` Joakim Tjernlund
@ 2010-02-18 17:41   ` Grant Likely
  2010-02-18 18:07     ` Joakim Tjernlund
       [not found]   ` <OFC412BED7.5D5934A3-ONC12576CE.005D5F44-C12576CE.005EBE55-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
  1 sibling, 1 reply; 17+ messages in thread
From: Grant Likely @ 2010-02-18 17:41 UTC (permalink / raw)
  To: Joakim Tjernlund
  Cc: linuxppc-dev, Albrecht Dreß, devicetree-discuss, ben-linux,
	iws

On Thu, Feb 18, 2010 at 10:14 AM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> "Albrecht Dreß" <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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-18 17:41   ` Grant Likely
@ 2010-02-18 18:07     ` Joakim Tjernlund
  0 siblings, 0 replies; 17+ messages in thread
From: Joakim Tjernlund @ 2010-02-18 18:07 UTC (permalink / raw)
  To: Grant Likely
  Cc: glikely, iws, Albrecht Dreß, devicetree-discuss,
	linuxppc-dev, ben-linux

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ß" <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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
       [not found]   ` <OFC412BED7.5D5934A3-ONC12576CE.005D5F44-C12576CE.005EBE55-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
@ 2010-02-18 18:45     ` Albrecht Dreß
  0 siblings, 0 replies; 17+ messages in thread
From: Albrecht Dreß @ 2010-02-18 18:45 UTC (permalink / raw)
  To: Joakim Tjernlund
  Cc: linuxppc-dev-mnsaURCQ41sdnm+yROfE0A,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	ben-linux-elnMNo+KYs3YtjvyW6yDsg, iws-lulEs6mt1IksTUYHLfqkUA


[-- Attachment #1.1: Type: text/plain, Size: 3118 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-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>

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

Best, Albrecht.

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

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-02-17 18:59 [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery Albrecht Dreß
                   ` (3 preceding siblings ...)
  2010-02-18 13:23 ` Joakim Tjernlund
@ 2010-05-05 22:09 ` Ira W. Snyder
  2010-05-06 17:54   ` Albrecht Dreß
  4 siblings, 1 reply; 17+ messages in thread
From: Ira W. Snyder @ 2010-05-05 22:09 UTC (permalink / raw)
  To: Albrecht Dreß
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms)

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-KvP5wT2u2U0@public.gmane.org>
> 
> ---
> 
> 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;
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-05-05 22:09 ` Ira W. Snyder
@ 2010-05-06 17:54   ` Albrecht Dreß
  2010-05-06 18:06     ` Grant Likely
  0 siblings, 1 reply; 17+ messages in thread
From: Albrecht Dreß @ 2010-05-06 17:54 UTC (permalink / raw)
  To: Ira W. Snyder, Grant Likely
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms)


[-- Attachment #1.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 #1.2: Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-05-06 17:54   ` Albrecht Dreß
@ 2010-05-06 18:06     ` Grant Likely
  2010-05-16 17:47       ` Albrecht Dreß
  0 siblings, 1 reply; 17+ messages in thread
From: Grant Likely @ 2010-05-06 18:06 UTC (permalink / raw)
  To: Albrecht Dreß
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder

On Thu, May 6, 2010 at 7:54 PM, Albrecht Dreß <albrecht.dress@arcor.de> wrote:
> 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?

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-05-06 18:06     ` Grant Likely
@ 2010-05-16 17:47       ` Albrecht Dreß
  2010-05-19 16:02         ` Grant Likely
  0 siblings, 1 reply; 17+ messages in thread
From: Albrecht Dreß @ 2010-05-16 17:47 UTC (permalink / raw)
  To: Grant Likely
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder


[-- Attachment #1.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 #1.2: Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-05-16 17:47       ` Albrecht Dreß
@ 2010-05-19 16:02         ` Grant Likely
  2010-06-16 19:30           ` Albrecht Dreß
  0 siblings, 1 reply; 17+ messages in thread
From: Grant Likely @ 2010-05-19 16:02 UTC (permalink / raw)
  To: Albrecht Dreß
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder

On Sun, May 16, 2010 at 11:47 AM, Albrecht Dreß <albrecht.dress-KvP5wT2u2U0@public.gmane.org> 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".  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... :-/

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.

g.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
  2010-05-19 16:02         ` Grant Likely
@ 2010-06-16 19:30           ` Albrecht Dreß
  0 siblings, 0 replies; 17+ messages in thread
From: Albrecht Dreß @ 2010-06-16 19:30 UTC (permalink / raw)
  To: Grant Likely
  Cc: Linux PPC Development, Devicetree Discussions,
	Ben Dooks (embedded platforms), Ira W. Snyder


[-- Attachment #1.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 #1.2: Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2010-06-16 19:30 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-17 18:59 [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery Albrecht Dreß
2010-02-17 20:10 ` Grant Likely
2010-02-18  8:09 ` Joakim Tjernlund
     [not found] ` <OF3D88A7A0.9A4F1639-ONC12576CE.002C0FB9-C12576CE.002CCBBB-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
2010-02-18  9:09   ` Albrecht Dreß
2010-02-18 12:33     ` Joakim Tjernlund
2010-02-18 13:23 ` Joakim Tjernlund
2010-05-05 22:09 ` Ira W. Snyder
2010-05-06 17:54   ` Albrecht Dreß
2010-05-06 18:06     ` Grant Likely
2010-05-16 17:47       ` Albrecht Dreß
2010-05-19 16:02         ` Grant Likely
2010-06-16 19:30           ` Albrecht Dreß
  -- strict thread matches above, loose matches on Subject: below --
2010-02-18 15:04 Albrecht Dreß
2010-02-18 17:14 ` Joakim Tjernlund
2010-02-18 17:41   ` Grant Likely
2010-02-18 18:07     ` Joakim Tjernlund
     [not found]   ` <OFC412BED7.5D5934A3-ONC12576CE.005D5F44-C12576CE.005EBE55-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
2010-02-18 18:45     ` Albrecht Dreß

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox