[RFC 0/4] i2c: omap: patch series related to multimaster mode

[RFC 0/4] i2c: omap: patch series related to multimaster mode

[Alexander Kochetkov]

Here is the patch series intended to improve stability of
i2c-omap driver in the i2c multimaster environments.

Tested on Beagleboard XM C.
For now all fine. No controller timeouts, no data corruptions.

Also impelemented i2c bus fault detection during startup and
after reset.

So, instead of the message "controller timeout" blaming IP,
you get "timeout waiting for bus ready".

What do think about it?

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

[PATCH 1/4] i2c: omap: cleanup register definitions

[Alexander Kochetkov]

Delete STAT_AD0 mask as unrelated to current IP (omap1?).
Delete DEBUG conditional around SYSTEST masks group.
Add SYSTEST functional mode masks for SCL and SDA.
Add STAT_BF mask.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/i2c/busses/i2c-omap.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 9af7095..a021733 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -102,7 +102,7 @@ enum {
 #define OMAP_I2C_STAT_ROVR	(1 << 11)	/* Receive overrun */
 #define OMAP_I2C_STAT_XUDF	(1 << 10)	/* Transmit underflow */
 #define OMAP_I2C_STAT_AAS	(1 << 9)	/* Address as slave */
-#define OMAP_I2C_STAT_AD0	(1 << 8)	/* Address zero */
+#define OMAP_I2C_STAT_BF	(1 << 8)	/* Bus Free */
 #define OMAP_I2C_STAT_XRDY	(1 << 4)	/* Transmit data ready */
 #define OMAP_I2C_STAT_RRDY	(1 << 3)	/* Receive data ready */
 #define OMAP_I2C_STAT_ARDY	(1 << 2)	/* Register access ready */
@@ -150,16 +150,20 @@ enum {
 #define OMAP_I2C_SCLH_HSSCLH	8
 
 /* I2C System Test Register (OMAP_I2C_SYSTEST): */
-#ifdef DEBUG
 #define OMAP_I2C_SYSTEST_ST_EN		(1 << 15)	/* System test enable */
 #define OMAP_I2C_SYSTEST_FREE		(1 << 14)	/* Free running mode */
 #define OMAP_I2C_SYSTEST_TMODE_MASK	(3 << 12)	/* Test mode select */
 #define OMAP_I2C_SYSTEST_TMODE_SHIFT	(12)		/* Test mode select */
+/* Functional mode */
+#define OMAP_I2C_SYSTEST_SCL_I_FUNC	(1 << 8)	/* SCL line input value */
+#define OMAP_I2C_SYSTEST_SCL_O_FUNC	(1 << 7)	/* SCL line output value */
+#define OMAP_I2C_SYSTEST_SDA_I_FUNC	(1 << 6)	/* SDA line input value */
+#define OMAP_I2C_SYSTEST_SDA_O_FUNC	(1 << 5)	/* SDA line output value */
+/* SDA/SCL IO mode */
 #define OMAP_I2C_SYSTEST_SCL_I		(1 << 3)	/* SCL line sense in */
 #define OMAP_I2C_SYSTEST_SCL_O		(1 << 2)	/* SCL line drive out */
 #define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
 #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
-#endif
 
 /* OCP_SYSSTATUS bit definitions */
 #define SYSS_RESETDONE_MASK		(1 << 0)
-- 
1.7.9.5

Re: [PATCH 1/4] i2c: omap: cleanup register definitions

[Felipe Balbi]

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

On Fri, Nov 21, 2014 at 01:28:42AM +0400, Alexander Kochetkov wrote:
> Delete STAT_AD0 mask as unrelated to current IP (omap1?).
> Delete DEBUG conditional around SYSTEST masks group.
> Add SYSTEST functional mode masks for SCL and SDA.
> Add STAT_BF mask.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Tested on BBB and AM437x Starter Kit

Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

> ---
>  drivers/i2c/busses/i2c-omap.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 9af7095..a021733 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -102,7 +102,7 @@ enum {
>  #define OMAP_I2C_STAT_ROVR	(1 << 11)	/* Receive overrun */
>  #define OMAP_I2C_STAT_XUDF	(1 << 10)	/* Transmit underflow */
>  #define OMAP_I2C_STAT_AAS	(1 << 9)	/* Address as slave */
> -#define OMAP_I2C_STAT_AD0	(1 << 8)	/* Address zero */
> +#define OMAP_I2C_STAT_BF	(1 << 8)	/* Bus Free */
>  #define OMAP_I2C_STAT_XRDY	(1 << 4)	/* Transmit data ready */
>  #define OMAP_I2C_STAT_RRDY	(1 << 3)	/* Receive data ready */
>  #define OMAP_I2C_STAT_ARDY	(1 << 2)	/* Register access ready */
> @@ -150,16 +150,20 @@ enum {
>  #define OMAP_I2C_SCLH_HSSCLH	8
>  
>  /* I2C System Test Register (OMAP_I2C_SYSTEST): */
> -#ifdef DEBUG
>  #define OMAP_I2C_SYSTEST_ST_EN		(1 << 15)	/* System test enable */
>  #define OMAP_I2C_SYSTEST_FREE		(1 << 14)	/* Free running mode */
>  #define OMAP_I2C_SYSTEST_TMODE_MASK	(3 << 12)	/* Test mode select */
>  #define OMAP_I2C_SYSTEST_TMODE_SHIFT	(12)		/* Test mode select */
> +/* Functional mode */
> +#define OMAP_I2C_SYSTEST_SCL_I_FUNC	(1 << 8)	/* SCL line input value */
> +#define OMAP_I2C_SYSTEST_SCL_O_FUNC	(1 << 7)	/* SCL line output value */
> +#define OMAP_I2C_SYSTEST_SDA_I_FUNC	(1 << 6)	/* SDA line input value */
> +#define OMAP_I2C_SYSTEST_SDA_O_FUNC	(1 << 5)	/* SDA line output value */
> +/* SDA/SCL IO mode */
>  #define OMAP_I2C_SYSTEST_SCL_I		(1 << 3)	/* SCL line sense in */
>  #define OMAP_I2C_SYSTEST_SCL_O		(1 << 2)	/* SCL line drive out */
>  #define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
>  #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
> -#endif
>  
>  /* OCP_SYSSTATUS bit definitions */
>  #define SYSS_RESETDONE_MASK		(1 << 0)
> -- 
> 1.7.9.5
> 

-- 
balbi

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

[PATCH 3/4] i2c: omap: don't reset controller if Arbitration Lost detected

[Alexander Kochetkov]

Arbitration Lost is a expected situation in a multimaster environment.
IP correctly detect it.

The only reason for reseting IP in the AL case is to be sure to
avoid advisory 1.94 (omap3) and errata i595 (omap4):
"I2C: After an Arbitration is Lost the Module Incorrectly Starts
the Next Transfer" with workaround: "The MST and STT bits inside
I2C_CON should be set to 1 at the same moment (avoid setting the
MST bit to 1 while STT = 0)."

The driver never writes MST and STT bits separately and doesn't
create condition for errata. So the reset is not necessary.

Tested on Beagleboard XM C.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/i2c/busses/i2c-omap.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 3ffb9c0..47103e7 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -707,13 +707,17 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		return 0;
 
 	/* We have an error */
-	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
+	if (dev->cmd_err & (OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
 		omap_i2c_reset(dev);
 		__omap_i2c_init(dev);
 		return -EIO;
 	}
 
+	if (dev->cmd_err & OMAP_I2C_STAT_AL) {
+		return -EIO;
+	}
+
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
-- 
1.7.9.5

Re: [PATCH 3/4] i2c: omap: don't reset controller if Arbitration Lost detected

[Felipe Balbi]

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

On Fri, Nov 21, 2014 at 01:28:44AM +0400, Alexander Kochetkov wrote:
> Arbitration Lost is a expected situation in a multimaster environment.
> IP correctly detect it.
> 
> The only reason for reseting IP in the AL case is to be sure to
> avoid advisory 1.94 (omap3) and errata i595 (omap4):
> "I2C: After an Arbitration is Lost the Module Incorrectly Starts
> the Next Transfer" with workaround: "The MST and STT bits inside
> I2C_CON should be set to 1 at the same moment (avoid setting the
> MST bit to 1 while STT = 0)."
> 
> The driver never writes MST and STT bits separately and doesn't
> create condition for errata. So the reset is not necessary.
> 
> Tested on Beagleboard XM C.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>

Tested on BBB and AM437x Starter Kit

Tested-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/i2c/busses/i2c-omap.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3ffb9c0..47103e7 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -707,13 +707,17 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  		return 0;
>  
>  	/* We have an error */
> -	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> +	if (dev->cmd_err & (OMAP_I2C_STAT_ROVR |
>  			    OMAP_I2C_STAT_XUDF)) {
>  		omap_i2c_reset(dev);
>  		__omap_i2c_init(dev);
>  		return -EIO;
>  	}
>  
> +	if (dev->cmd_err & OMAP_I2C_STAT_AL) {
> +		return -EIO;
> +	}
> +
>  	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
>  		if (msg->flags & I2C_M_IGNORE_NAK)
>  			return 0;
> -- 
> 1.7.9.5
> 

-- 
balbi

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

Re: [PATCH 3/4] i2c: omap: don't reset controller if Arbitration Lost detected

[Wolfram Sang]

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

On Fri, Nov 21, 2014 at 01:28:44AM +0400, Alexander Kochetkov wrote:
> Arbitration Lost is a expected situation in a multimaster environment.
> IP correctly detect it.
> 
> The only reason for reseting IP in the AL case is to be sure to
> avoid advisory 1.94 (omap3) and errata i595 (omap4):
> "I2C: After an Arbitration is Lost the Module Incorrectly Starts
> the Next Transfer" with workaround: "The MST and STT bits inside
> I2C_CON should be set to 1 at the same moment (avoid setting the
> MST bit to 1 while STT = 0)."
> 
> The driver never writes MST and STT bits separately and doesn't
> create condition for errata. So the reset is not necessary.
> 
> Tested on Beagleboard XM C.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3ffb9c0..47103e7 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -707,13 +707,17 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  		return 0;
>  
>  	/* We have an error */
> -	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> +	if (dev->cmd_err & (OMAP_I2C_STAT_ROVR |
>  			    OMAP_I2C_STAT_XUDF)) {
>  		omap_i2c_reset(dev);
>  		__omap_i2c_init(dev);
>  		return -EIO;
>  	}
>  
> +	if (dev->cmd_err & OMAP_I2C_STAT_AL) {
> +		return -EIO;
> +	}

The errno for AL is -EAGAIN. Curly braces are not needed.

> +
>  	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
>  		if (msg->flags & I2C_M_IGNORE_NAK)
>  			return 0;
> -- 
> 1.7.9.5
> 

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

[PATCH v2 3/4] i2c: omap: don't reset controller if Arbitration Lost detected

[Alexander Kochetkov]

Arbitration Lost is an expected situation in a multimaster
environment. I2C controller (IP) correctly detect and report AL.

The only one visible reason for reseting IP in the AL case is
to avoid advisory 1.94 (omap3) and errata i595 (omap4): "I2C:
After an Arbitration is Lost the Module Incorrectly Starts
the Next Transfer".

Errata workaround states: "The MST and STT bits inside I2C_CON
should be set to 1 at the same moment (avoid setting the MST bit
to 1 while STT = 0)." The driver never set MST and STT bits
separately and doesn't create condition for errata. So the reset
is not necessary.

Also corrected return value for AL to -EAGAIN.

Tested on Beagleboard XM C.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

On 21.10.2014 21:11, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> The errno for AL is -EAGAIN. Curly braces are not needed.

Thank you, Wolfram, fixed.

 drivers/i2c/busses/i2c-omap.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 3ffb9c0..02da567 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -707,13 +707,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		return 0;
 
 	/* We have an error */
-	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
-			    OMAP_I2C_STAT_XUDF)) {
+	if (dev->cmd_err & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_XUDF)) {
 		omap_i2c_reset(dev);
 		__omap_i2c_init(dev);
 		return -EIO;
 	}
 
+	if (dev->cmd_err & OMAP_I2C_STAT_AL)
+		return -EAGAIN;
+
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
-- 
1.7.9.5

Re: [PATCH v2 3/4] i2c: omap: don't reset controller if Arbitration Lost detected

[Felipe Balbi]

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

On Sat, Nov 22, 2014 at 02:51:47AM +0400, Alexander Kochetkov wrote:
> Arbitration Lost is an expected situation in a multimaster
> environment. I2C controller (IP) correctly detect and report AL.
> 
> The only one visible reason for reseting IP in the AL case is
> to avoid advisory 1.94 (omap3) and errata i595 (omap4): "I2C:
> After an Arbitration is Lost the Module Incorrectly Starts
> the Next Transfer".
> 
> Errata workaround states: "The MST and STT bits inside I2C_CON
> should be set to 1 at the same moment (avoid setting the MST bit
> to 1 while STT = 0)." The driver never set MST and STT bits
> separately and doesn't create condition for errata. So the reset
> is not necessary.
> 
> Also corrected return value for AL to -EAGAIN.
> 
> Tested on Beagleboard XM C.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

you could have kept my tested-by and reviewed-by:

Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>


> On 21.10.2014 21:11, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> > The errno for AL is -EAGAIN. Curly braces are not needed.
> 
> Thank you, Wolfram, fixed.
> 
>  drivers/i2c/busses/i2c-omap.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3ffb9c0..02da567 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -707,13 +707,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  		return 0;
>  
>  	/* We have an error */
> -	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> -			    OMAP_I2C_STAT_XUDF)) {
> +	if (dev->cmd_err & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_XUDF)) {
>  		omap_i2c_reset(dev);
>  		__omap_i2c_init(dev);
>  		return -EIO;
>  	}
>  
> +	if (dev->cmd_err & OMAP_I2C_STAT_AL)
> +		return -EAGAIN;
> +
>  	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
>  		if (msg->flags & I2C_M_IGNORE_NAK)
>  			return 0;
> -- 
> 1.7.9.5
> 

-- 
balbi

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

[PATCH 4/4] i2c: omap: add notes related to i2c multimaster mode

[Alexander Kochetkov]

No functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/i2c/busses/i2c-omap.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 47103e7..4e3642c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -304,6 +304,12 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
 	/*
+	 * NOTE: right after setting CON_EN, STAT_BB could be 0 while the
+	 * bus is busy. It will be changed to 1 on the next IP FCLK clock.
+	 * udelay(1) will be enough to fix that.
+	 */
+
+	/*
 	 * Don't write to this register if the IE state is 0 as it can
 	 * cause deadlock.
 	 */
@@ -664,7 +670,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 
 	if (!dev->b_hw && stop)
 		w |= OMAP_I2C_CON_STP;
-
+	/*
+	 * NOTE: STAT_BB bit could became 1 here if another master occupy
+	 * the bus. IP successfully complete transfer when the bus will be
+	 * free again (BB reset to 0).
+	 */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
 
 	/*
-- 
1.7.9.5

Re: [PATCH 4/4] i2c: omap: add notes related to i2c multimaster mode

[Felipe Balbi]

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

On Fri, Nov 21, 2014 at 01:28:45AM +0400, Alexander Kochetkov wrote:
> No functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>

heh:

Tested on BBB and AM437x Starter Kit

Tested-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/i2c/busses/i2c-omap.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 47103e7..4e3642c 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -304,6 +304,12 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  
>  	/*
> +	 * NOTE: right after setting CON_EN, STAT_BB could be 0 while the
> +	 * bus is busy. It will be changed to 1 on the next IP FCLK clock.
> +	 * udelay(1) will be enough to fix that.
> +	 */
> +
> +	/*
>  	 * Don't write to this register if the IE state is 0 as it can
>  	 * cause deadlock.
>  	 */
> @@ -664,7 +670,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  
>  	if (!dev->b_hw && stop)
>  		w |= OMAP_I2C_CON_STP;
> -
> +	/*
> +	 * NOTE: STAT_BB bit could became 1 here if another master occupy
> +	 * the bus. IP successfully complete transfer when the bus will be
> +	 * free again (BB reset to 0).
> +	 */
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
>  
>  	/*
> -- 
> 1.7.9.5
> 

-- 
balbi

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

[PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

In a multimaster environment, after IP software reset, BB-bit value doesn't
correspond to the current bus state. It may happen what BB-bit will be 0,
while the bus is busy due to another I2C master activity.

Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
and results in controller timeout. More over, in some cases IP could
interrupt another master's transfer and corrupt data on wire.

The commit implement method allowing to prevent IP from entering into
"controller timeout" state and from "data corruption" state.

The one drawback is the need to wait for 10ms before the first transfer.

Tested on Beagleboard XM C.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |  103 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a021733..3ffb9c0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -58,6 +58,9 @@
 /* timeout for pm runtime autosuspend */
 #define OMAP_I2C_PM_TIMEOUT		1000	/* ms */
 
+/* timeout for making decision on bus free status */
+#define OMAP_I2C_BUS_FREE_TIMEOUT (msecs_to_jiffies(10))
+
 /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
 enum {
 	OMAP_I2C_REV_REG = 0,
@@ -210,6 +213,9 @@ struct omap_i2c_dev {
 						 */
 	u32			rev;
 	unsigned		b_hw:1;		/* bad h/w fixes */
+	unsigned		bb_valid:1;	/* true when BB-bit reflects
+						 * the I2C bus state
+						 */
 	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */
 	u16			pscstate;
@@ -336,7 +342,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
 		/* SYSC register is cleared by the reset; rewrite it */
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
 
+		/* Schedule I2C-bus monitoring on the next transfer */
+		dev->bb_valid = 0;
 	}
+
 	return 0;
 }
 
@@ -449,6 +458,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	dev->scllstate = scll;
 	dev->sclhstate = sclh;
 
+	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
+		/* Not implemented */
+		dev->bb_valid = 1;
+	}
+
 	__omap_i2c_init(dev);
 
 	return 0;
@@ -473,6 +487,91 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
 	return 0;
 }
 
+/*
+ * Wait while BB-bit doesn't reflect the I2C bus state
+ *
+ * In a multimaster environment, after IP software reset, BB-bit value doesn't
+ * correspond to the current bus state. It may happen what BB-bit will be 0,
+ * while the bus is busy due to another I2C master activity.
+ * Here are BB-bit values after reset:
+ *     SDA   SCL   BB   NOTES
+ *       0     0    0   1, 2
+ *       1     0    0   1, 2
+ *       0     1    1
+ *       1     1    0   3
+ * Later, if IP detect SDA=0 and SCL=1 (ACK) or SDA 1->0 while SCL=1 (START)
+ * combinations on the bus, it set BB-bit to 1.
+ * If IP detect SDA 0->1 while SCL=1 (STOP) combination on the bus,
+ * it set BB-bit to 0 and BF to 1.
+ * BB and BF bits correctly tracks the bus state while IP is suspended
+ * BB bit became valid on the next FCLK clock after CON_EN bit set
+ *
+ * NOTES:
+ * 1. Any transfer started when BB=0 and bus is busy wouldn't be
+ *    completed by IP and results in controller timeout.
+ * 2. Any transfer started when BB=0 and SCL=0 results in IP
+ *    starting to drive SDA low. In that case IP corrupt data
+ *    on the bus.
+ * 3. Any transfer started in the middle of another master's transfer
+ *    results in unpredictable results and data corruption
+ */
+static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev)
+{
+	unsigned long bus_free_timeout = 0;
+	unsigned long timeout;
+	int bus_free = 0;
+	u16 stat, systest;
+
+	if (dev->bb_valid)
+		return 0;
+
+	timeout = jiffies + OMAP_I2C_TIMEOUT;
+	while (1) {
+		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+		/*
+		 * We will see BB or BF event in a case IP had detected any
+		 * activity on the I2C bus. Now IP correctly tracks the bus
+		 * state. BB-bit value is valid.
+		 */
+		if (stat & (OMAP_I2C_STAT_BB | OMAP_I2C_STAT_BF))
+			break;
+
+		/*
+		 * Otherwise, we must look signals on the bus to make
+		 * the right decision.
+		 */
+		systest = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
+		if ((systest & OMAP_I2C_SYSTEST_SCL_I_FUNC) &&
+		    (systest & OMAP_I2C_SYSTEST_SDA_I_FUNC)) {
+			if (!bus_free) {
+				bus_free_timeout = jiffies +
+					OMAP_I2C_BUS_FREE_TIMEOUT;
+				bus_free = 1;
+			}
+
+			/*
+			 * SDA and SCL lines was high for 10 ms without bus
+			 * activity detected. The bus is free. Consider
+			 * BB-bit value is valid.
+			 */
+			if (time_after(jiffies, bus_free_timeout))
+				break;
+		} else {
+			bus_free = 0;
+		}
+
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev, "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+
+		msleep(1);
+	}
+
+	dev->bb_valid = 1;
+	return 0;
+}
+
 static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 {
 	u16		buf;
@@ -643,6 +742,10 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (IS_ERR_VALUE(r))
 		goto out;
 
+	r = omap_i2c_wait_for_bb_valid(dev);
+	if (r < 0)
+		goto out;
+
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
 		goto out;
-- 
1.7.9.5

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Felipe Balbi]

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

On Fri, Nov 21, 2014 at 01:28:43AM +0400, Alexander Kochetkov wrote:
> In a multimaster environment, after IP software reset, BB-bit value doesn't
> correspond to the current bus state. It may happen what BB-bit will be 0,
> while the bus is busy due to another I2C master activity.
> 
> Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> and results in controller timeout. More over, in some cases IP could
> interrupt another master's transfer and corrupt data on wire.
> 
> The commit implement method allowing to prevent IP from entering into
> "controller timeout" state and from "data corruption" state.
> 
> The one drawback is the need to wait for 10ms before the first transfer.
> 
> Tested on Beagleboard XM C.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>

Tested on BBB and AM437x Starter Kit

Tested-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/i2c/busses/i2c-omap.c |  103 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a021733..3ffb9c0 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -58,6 +58,9 @@
>  /* timeout for pm runtime autosuspend */
>  #define OMAP_I2C_PM_TIMEOUT		1000	/* ms */
>  
> +/* timeout for making decision on bus free status */
> +#define OMAP_I2C_BUS_FREE_TIMEOUT (msecs_to_jiffies(10))
> +
>  /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
>  enum {
>  	OMAP_I2C_REV_REG = 0,
> @@ -210,6 +213,9 @@ struct omap_i2c_dev {
>  						 */
>  	u32			rev;
>  	unsigned		b_hw:1;		/* bad h/w fixes */
> +	unsigned		bb_valid:1;	/* true when BB-bit reflects
> +						 * the I2C bus state
> +						 */
>  	unsigned		receiver:1;	/* true when we're in receiver mode */
>  	u16			iestate;	/* Saved interrupt register */
>  	u16			pscstate;
> @@ -336,7 +342,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>  		/* SYSC register is cleared by the reset; rewrite it */
>  		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
>  
> +		/* Schedule I2C-bus monitoring on the next transfer */
> +		dev->bb_valid = 0;
>  	}
> +
>  	return 0;
>  }
>  
> @@ -449,6 +458,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	dev->scllstate = scll;
>  	dev->sclhstate = sclh;
>  
> +	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
> +		/* Not implemented */
> +		dev->bb_valid = 1;
> +	}
> +
>  	__omap_i2c_init(dev);
>  
>  	return 0;
> @@ -473,6 +487,91 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
>  	return 0;
>  }
>  
> +/*
> + * Wait while BB-bit doesn't reflect the I2C bus state
> + *
> + * In a multimaster environment, after IP software reset, BB-bit value doesn't
> + * correspond to the current bus state. It may happen what BB-bit will be 0,
> + * while the bus is busy due to another I2C master activity.
> + * Here are BB-bit values after reset:
> + *     SDA   SCL   BB   NOTES
> + *       0     0    0   1, 2
> + *       1     0    0   1, 2
> + *       0     1    1
> + *       1     1    0   3
> + * Later, if IP detect SDA=0 and SCL=1 (ACK) or SDA 1->0 while SCL=1 (START)
> + * combinations on the bus, it set BB-bit to 1.
> + * If IP detect SDA 0->1 while SCL=1 (STOP) combination on the bus,
> + * it set BB-bit to 0 and BF to 1.
> + * BB and BF bits correctly tracks the bus state while IP is suspended
> + * BB bit became valid on the next FCLK clock after CON_EN bit set
> + *
> + * NOTES:
> + * 1. Any transfer started when BB=0 and bus is busy wouldn't be
> + *    completed by IP and results in controller timeout.
> + * 2. Any transfer started when BB=0 and SCL=0 results in IP
> + *    starting to drive SDA low. In that case IP corrupt data
> + *    on the bus.
> + * 3. Any transfer started in the middle of another master's transfer
> + *    results in unpredictable results and data corruption
> + */
> +static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev)
> +{
> +	unsigned long bus_free_timeout = 0;
> +	unsigned long timeout;
> +	int bus_free = 0;
> +	u16 stat, systest;
> +
> +	if (dev->bb_valid)
> +		return 0;
> +
> +	timeout = jiffies + OMAP_I2C_TIMEOUT;
> +	while (1) {
> +		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +		/*
> +		 * We will see BB or BF event in a case IP had detected any
> +		 * activity on the I2C bus. Now IP correctly tracks the bus
> +		 * state. BB-bit value is valid.
> +		 */
> +		if (stat & (OMAP_I2C_STAT_BB | OMAP_I2C_STAT_BF))
> +			break;
> +
> +		/*
> +		 * Otherwise, we must look signals on the bus to make
> +		 * the right decision.
> +		 */
> +		systest = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
> +		if ((systest & OMAP_I2C_SYSTEST_SCL_I_FUNC) &&
> +		    (systest & OMAP_I2C_SYSTEST_SDA_I_FUNC)) {
> +			if (!bus_free) {
> +				bus_free_timeout = jiffies +
> +					OMAP_I2C_BUS_FREE_TIMEOUT;
> +				bus_free = 1;
> +			}
> +
> +			/*
> +			 * SDA and SCL lines was high for 10 ms without bus
> +			 * activity detected. The bus is free. Consider
> +			 * BB-bit value is valid.
> +			 */
> +			if (time_after(jiffies, bus_free_timeout))
> +				break;
> +		} else {
> +			bus_free = 0;
> +		}
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(dev->dev, "timeout waiting for bus ready\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		msleep(1);
> +	}
> +
> +	dev->bb_valid = 1;
> +	return 0;
> +}
> +
>  static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
>  {
>  	u16		buf;
> @@ -643,6 +742,10 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	if (IS_ERR_VALUE(r))
>  		goto out;
>  
> +	r = omap_i2c_wait_for_bb_valid(dev);
> +	if (r < 0)
> +		goto out;
> +
>  	r = omap_i2c_wait_for_bb(dev);
>  	if (r < 0)
>  		goto out;
> -- 
> 1.7.9.5
> 

-- 
balbi

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

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Wolfram Sang]

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

On Fri, Nov 21, 2014 at 10:08:08AM -0600, Felipe Balbi wrote:
> On Fri, Nov 21, 2014 at 01:28:43AM +0400, Alexander Kochetkov wrote:
> > In a multimaster environment, after IP software reset, BB-bit value doesn't
> > correspond to the current bus state. It may happen what BB-bit will be 0,
> > while the bus is busy due to another I2C master activity.
> > 
> > Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> > and results in controller timeout. More over, in some cases IP could
> > interrupt another master's transfer and corrupt data on wire.
> > 
> > The commit implement method allowing to prevent IP from entering into
> > "controller timeout" state and from "data corruption" state.
> > 
> > The one drawback is the need to wait for 10ms before the first transfer.
> > 
> > Tested on Beagleboard XM C.
> > 
> > Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Tested on BBB and AM437x Starter Kit
> 
> Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

Huh, I can't apply this one? Which kernel version is this based on?


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

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

22 нояб. 2014 г., в 16:23, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> написал(а):

> Huh, I can't apply this one? Which kernel version is this based on?

v.3.13-rc8
It depends on PATCH 1/4

alexander@ubuntu:git.kernel.org.pub.scm.linux.kernel.git.stable.linux-stable$ git log --pretty=oneline --abbrev-commit -6
204c058 i2c: omap: add notes related to i2c multimaster mode
2b15417 i2c: omap: don't reset controller if Arbitration Lost detected
38ef30e i2c: omap: implement workaround for handling invalid BB-bit values
9b15135 i2c: omap: cleanup register definitions
a7483d8 Fixes: 1d7afc9 i2c: omap: ack IRQ in parts
7e22e91 Linux 3.13-rc8

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Wolfram Sang]

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

On Sat, Nov 22, 2014 at 05:06:10PM +0300, Alexander Kochetkov wrote:
> 
> 22 нояб. 2014 г., в 16:23, Wolfram Sang <wsa@the-dreams.de> написал(а):
> 
> > Huh, I can't apply this one? Which kernel version is this based on?
> 
> v.3.13-rc8

Wow, that's old. Can you please rebase the series on top of 3.18-rc4? Or
my i2c/for-next? Or at the very least 3.17?

@Felipe: With which kernel did you test the series? Also, 3.13?


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

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Felipe Balbi]

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

On Sat, Nov 22, 2014 at 07:02:22PM +0100, Wolfram Sang wrote:
> On Sat, Nov 22, 2014 at 05:06:10PM +0300, Alexander Kochetkov wrote:
> > 
> > 22 нояб. 2014 г., в 16:23, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> написал(а):
> > 
> > > Huh, I can't apply this one? Which kernel version is this based on?
> > 
> > v.3.13-rc8
> 
> Wow, that's old. Can you please rebase the series on top of 3.18-rc4? Or
> my i2c/for-next? Or at the very least 3.17?
> 
> @Felipe: With which kernel did you test the series? Also, 3.13?

maybe there was a typo? I tested on v3.18-rc3 :-)


-- 
balbi

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

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

23 нояб. 2014 г., в 7:43, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> написал(а):

> maybe there was a typo? I tested on v3.18-rc3 :-)

I do my tests on kernel from angstrom with almost all i2c-omap patches backported from linux/master.
Then I rebased them to wrong (old) kernel version and posted to the list.

Angstrom kernel is from meta-ti layer. It's the same kernel as for arago.
http://arago-project.org/wiki/index.php/Main_Page

I would really like to switch to the recent kernel, but I don't know how good codec engine (CE) is supported on it.
Initially all ti drivers for codec engine was done for 2.6.x kernels and later some of them was ported for 3.2.x.

Felipe, do you know how CE is supported on v3.18-rc3?

Regards,
Alexander.

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Felipe Balbi]

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

On Sun, Nov 23, 2014 at 04:18:40PM +0300, Alexander Kochetkov wrote:
> 
> 23 нояб. 2014 г., в 7:43, Felipe Balbi <balbi@ti.com> написал(а):
> 
> > maybe there was a typo? I tested on v3.18-rc3 :-)
> 
> I do my tests on kernel from angstrom with almost all i2c-omap patches
> backported from linux/master.
> Then I rebased them to wrong (old) kernel version and posted to the
> list.
> 
> Angstrom kernel is from meta-ti layer. It's the same kernel as for arago.
> http://arago-project.org/wiki/index.php/Main_Page
> 
> I would really like to switch to the recent kernel, but I don't know
> how good codec engine (CE) is supported on it.
> Initially all ti drivers for codec engine was done for 2.6.x kernels
> and later some of them was ported for 3.2.x.
> 
> Felipe, do you know how CE is supported on v3.18-rc3?

what's CE ?

-- 
balbi

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

Re: [PATCH 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Felipe Balbi]

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

Hi,

On Fri, Nov 21, 2014 at 01:28:43AM +0400, Alexander Kochetkov wrote:
> In a multimaster environment, after IP software reset, BB-bit value doesn't
> correspond to the current bus state. It may happen what BB-bit will be 0,
> while the bus is busy due to another I2C master activity.
> 
> Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> and results in controller timeout. More over, in some cases IP could
> interrupt another master's transfer and corrupt data on wire.
> 
> The commit implement method allowing to prevent IP from entering into
> "controller timeout" state and from "data corruption" state.
> 
> The one drawback is the need to wait for 10ms before the first transfer.
> 
> Tested on Beagleboard XM C.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>

we have a report from Kevin Hilman that this commit breaks OMAP3530, see
[1]

[1] http://storage.armcloud.us/kernel-ci/next/next-20141124/arm-omap2plus_defconfig/boot-omap3-overo-tobi.log

> ---
>  drivers/i2c/busses/i2c-omap.c |  103 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a021733..3ffb9c0 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -58,6 +58,9 @@
>  /* timeout for pm runtime autosuspend */
>  #define OMAP_I2C_PM_TIMEOUT		1000	/* ms */
>  
> +/* timeout for making decision on bus free status */
> +#define OMAP_I2C_BUS_FREE_TIMEOUT (msecs_to_jiffies(10))
> +
>  /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
>  enum {
>  	OMAP_I2C_REV_REG = 0,
> @@ -210,6 +213,9 @@ struct omap_i2c_dev {
>  						 */
>  	u32			rev;
>  	unsigned		b_hw:1;		/* bad h/w fixes */
> +	unsigned		bb_valid:1;	/* true when BB-bit reflects
> +						 * the I2C bus state
> +						 */
>  	unsigned		receiver:1;	/* true when we're in receiver mode */
>  	u16			iestate;	/* Saved interrupt register */
>  	u16			pscstate;
> @@ -336,7 +342,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>  		/* SYSC register is cleared by the reset; rewrite it */
>  		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
>  
> +		/* Schedule I2C-bus monitoring on the next transfer */
> +		dev->bb_valid = 0;
>  	}
> +
>  	return 0;
>  }
>  
> @@ -449,6 +458,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	dev->scllstate = scll;
>  	dev->sclhstate = sclh;
>  
> +	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
> +		/* Not implemented */
> +		dev->bb_valid = 1;
> +	}
> +
>  	__omap_i2c_init(dev);
>  
>  	return 0;
> @@ -473,6 +487,91 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
>  	return 0;
>  }
>  
> +/*
> + * Wait while BB-bit doesn't reflect the I2C bus state
> + *
> + * In a multimaster environment, after IP software reset, BB-bit value doesn't
> + * correspond to the current bus state. It may happen what BB-bit will be 0,
> + * while the bus is busy due to another I2C master activity.
> + * Here are BB-bit values after reset:
> + *     SDA   SCL   BB   NOTES
> + *       0     0    0   1, 2
> + *       1     0    0   1, 2
> + *       0     1    1
> + *       1     1    0   3
> + * Later, if IP detect SDA=0 and SCL=1 (ACK) or SDA 1->0 while SCL=1 (START)
> + * combinations on the bus, it set BB-bit to 1.
> + * If IP detect SDA 0->1 while SCL=1 (STOP) combination on the bus,
> + * it set BB-bit to 0 and BF to 1.
> + * BB and BF bits correctly tracks the bus state while IP is suspended
> + * BB bit became valid on the next FCLK clock after CON_EN bit set
> + *
> + * NOTES:
> + * 1. Any transfer started when BB=0 and bus is busy wouldn't be
> + *    completed by IP and results in controller timeout.
> + * 2. Any transfer started when BB=0 and SCL=0 results in IP
> + *    starting to drive SDA low. In that case IP corrupt data
> + *    on the bus.
> + * 3. Any transfer started in the middle of another master's transfer
> + *    results in unpredictable results and data corruption
> + */
> +static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev)
> +{
> +	unsigned long bus_free_timeout = 0;
> +	unsigned long timeout;
> +	int bus_free = 0;
> +	u16 stat, systest;
> +
> +	if (dev->bb_valid)
> +		return 0;
> +
> +	timeout = jiffies + OMAP_I2C_TIMEOUT;
> +	while (1) {
> +		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +		/*
> +		 * We will see BB or BF event in a case IP had detected any
> +		 * activity on the I2C bus. Now IP correctly tracks the bus
> +		 * state. BB-bit value is valid.
> +		 */
> +		if (stat & (OMAP_I2C_STAT_BB | OMAP_I2C_STAT_BF))
> +			break;
> +
> +		/*
> +		 * Otherwise, we must look signals on the bus to make
> +		 * the right decision.
> +		 */
> +		systest = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
> +		if ((systest & OMAP_I2C_SYSTEST_SCL_I_FUNC) &&
> +		    (systest & OMAP_I2C_SYSTEST_SDA_I_FUNC)) {
> +			if (!bus_free) {
> +				bus_free_timeout = jiffies +
> +					OMAP_I2C_BUS_FREE_TIMEOUT;
> +				bus_free = 1;
> +			}
> +
> +			/*
> +			 * SDA and SCL lines was high for 10 ms without bus
> +			 * activity detected. The bus is free. Consider
> +			 * BB-bit value is valid.
> +			 */
> +			if (time_after(jiffies, bus_free_timeout))
> +				break;
> +		} else {
> +			bus_free = 0;
> +		}
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(dev->dev, "timeout waiting for bus ready\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		msleep(1);
> +	}
> +
> +	dev->bb_valid = 1;
> +	return 0;
> +}
> +
>  static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
>  {
>  	u16		buf;
> @@ -643,6 +742,10 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	if (IS_ERR_VALUE(r))
>  		goto out;
>  
> +	r = omap_i2c_wait_for_bb_valid(dev);
> +	if (r < 0)
> +		goto out;
> +
>  	r = omap_i2c_wait_for_bb(dev);
>  	if (r < 0)
>  		goto out;
> -- 
> 1.7.9.5
> 

-- 
balbi

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

[PATCH v3 0/4] i2c: omap: patch series related to multimaster mode

[Alexander Kochetkov]

Here is the patch series intended to improve stability of
i2c-omap driver in the i2c multimaster environments.

Tested on Beagleboard XM C.
For now all fine. No controller timeouts, no data corruptions.

Also impelemented i2c bus fault detection during startup and
after reset.

So, instead of the message "controller timeout" blaming IP,
you get "timeout waiting for bus ready".

Rebased against i2c/for-next (eb61694b43dea0d43ea2574e7d48f476a6bdffb0)

Tested on the kernel from angstrom-v2013.06-yocto1.4
Kernel recepie linux-mainline_3.2.bb
git://github.com/Angstrom-distribution/meta-ti.git; branch angstrom-staging-yocto1.4
With all i2-omap.c patches backported from upstream linux/master

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Alexander Kochetkov (4):
  i2c: omap: cleanup register definitions
  i2c: omap: implement workaround for handling invalid BB-bit values
  i2c: omap: don't reset controller if Arbitration Lost detected
  i2c: omap: add notes related to i2c multimaster mode

 drivers/i2c/busses/i2c-omap.c |  131 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 125 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

In a multimaster environment, after IP software reset, BB-bit value doesn't
correspond to the current bus state. It may happen what BB-bit will be 0,
while the bus is busy due to another I2C master activity.

Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
and results in controller timeout. More over, in some cases IP could
interrupt another master's transfer and corrupt data on wire.

The commit implement method allowing to prevent IP from entering into
"controller timeout" state and from "data corruption" state.

The one drawback is the need to wait for 10ms before the first transfer.

Tested on Beagleboard XM C.
Tested on BBB and AM437x Starter Kit by Felipe Balbi.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Tested-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |  103 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index adfac9b..a238a1d 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -54,6 +54,9 @@
 /* timeout for pm runtime autosuspend */
 #define OMAP_I2C_PM_TIMEOUT		1000	/* ms */
 
+/* timeout for making decision on bus free status */
+#define OMAP_I2C_BUS_FREE_TIMEOUT (msecs_to_jiffies(10))
+
 /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
 enum {
 	OMAP_I2C_REV_REG = 0,
@@ -206,6 +209,9 @@ struct omap_i2c_dev {
 						 */
 	u32			rev;
 	unsigned		b_hw:1;		/* bad h/w fixes */
+	unsigned		bb_valid:1;	/* true when BB-bit reflects
+						 * the I2C bus state
+						 */
 	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */
 	u16			pscstate;
@@ -332,7 +338,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
 		/* SYSC register is cleared by the reset; rewrite it */
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
 
+		/* Schedule I2C-bus monitoring on the next transfer */
+		dev->bb_valid = 0;
 	}
+
 	return 0;
 }
 
@@ -445,6 +454,11 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	dev->scllstate = scll;
 	dev->sclhstate = sclh;
 
+	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
+		/* Not implemented */
+		dev->bb_valid = 1;
+	}
+
 	__omap_i2c_init(dev);
 
 	return 0;
@@ -469,6 +483,91 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
 	return 0;
 }
 
+/*
+ * Wait while BB-bit doesn't reflect the I2C bus state
+ *
+ * In a multimaster environment, after IP software reset, BB-bit value doesn't
+ * correspond to the current bus state. It may happen what BB-bit will be 0,
+ * while the bus is busy due to another I2C master activity.
+ * Here are BB-bit values after reset:
+ *     SDA   SCL   BB   NOTES
+ *       0     0    0   1, 2
+ *       1     0    0   1, 2
+ *       0     1    1
+ *       1     1    0   3
+ * Later, if IP detect SDA=0 and SCL=1 (ACK) or SDA 1->0 while SCL=1 (START)
+ * combinations on the bus, it set BB-bit to 1.
+ * If IP detect SDA 0->1 while SCL=1 (STOP) combination on the bus,
+ * it set BB-bit to 0 and BF to 1.
+ * BB and BF bits correctly tracks the bus state while IP is suspended
+ * BB bit became valid on the next FCLK clock after CON_EN bit set
+ *
+ * NOTES:
+ * 1. Any transfer started when BB=0 and bus is busy wouldn't be
+ *    completed by IP and results in controller timeout.
+ * 2. Any transfer started when BB=0 and SCL=0 results in IP
+ *    starting to drive SDA low. In that case IP corrupt data
+ *    on the bus.
+ * 3. Any transfer started in the middle of another master's transfer
+ *    results in unpredictable results and data corruption
+ */
+static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev)
+{
+	unsigned long bus_free_timeout = 0;
+	unsigned long timeout;
+	int bus_free = 0;
+	u16 stat, systest;
+
+	if (dev->bb_valid)
+		return 0;
+
+	timeout = jiffies + OMAP_I2C_TIMEOUT;
+	while (1) {
+		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+		/*
+		 * We will see BB or BF event in a case IP had detected any
+		 * activity on the I2C bus. Now IP correctly tracks the bus
+		 * state. BB-bit value is valid.
+		 */
+		if (stat & (OMAP_I2C_STAT_BB | OMAP_I2C_STAT_BF))
+			break;
+
+		/*
+		 * Otherwise, we must look signals on the bus to make
+		 * the right decision.
+		 */
+		systest = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
+		if ((systest & OMAP_I2C_SYSTEST_SCL_I_FUNC) &&
+		    (systest & OMAP_I2C_SYSTEST_SDA_I_FUNC)) {
+			if (!bus_free) {
+				bus_free_timeout = jiffies +
+					OMAP_I2C_BUS_FREE_TIMEOUT;
+				bus_free = 1;
+			}
+
+			/*
+			 * SDA and SCL lines was high for 10 ms without bus
+			 * activity detected. The bus is free. Consider
+			 * BB-bit value is valid.
+			 */
+			if (time_after(jiffies, bus_free_timeout))
+				break;
+		} else {
+			bus_free = 0;
+		}
+
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev, "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+
+		msleep(1);
+	}
+
+	dev->bb_valid = 1;
+	return 0;
+}
+
 static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 {
 	u16		buf;
@@ -639,6 +738,10 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (r < 0)
 		goto out;
 
+	r = omap_i2c_wait_for_bb_valid(dev);
+	if (r < 0)
+		goto out;
+
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
 		goto out;
-- 
1.7.9.5

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Kevin Hilman]

On Sat, Nov 22, 2014 at 11:47 AM, Alexander Kochetkov
<al.kochet@gmail.com> wrote:
> In a multimaster environment, after IP software reset, BB-bit value doesn't
> correspond to the current bus state. It may happen what BB-bit will be 0,
> while the bus is busy due to another I2C master activity.
>
> Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> and results in controller timeout. More over, in some cases IP could
> interrupt another master's transfer and corrupt data on wire.
>
> The commit implement method allowing to prevent IP from entering into
> "controller timeout" state and from "data corruption" state.
>
> The one drawback is the need to wait for 10ms before the first transfer.
>
> Tested on Beagleboard XM C.
> Tested on BBB and AM437x Starter Kit by Felipe Balbi.
>
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Tested-by: Felipe Balbi <balbi@ti.com>
> Reviewed-by: Felipe Balbi <balbi@ti.com>

This patch recently hit linux-next (as commit 903c3859f77f) and boot
breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards
was bisected down to this commit.

Kevin

[1] http://status.armcloud.us/boot/?next-20141124&omap

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Felipe Balbi]

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

On Mon, Nov 24, 2014 at 11:08:48AM -0800, Kevin Hilman wrote:
> On Sat, Nov 22, 2014 at 11:47 AM, Alexander Kochetkov
> <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > In a multimaster environment, after IP software reset, BB-bit value doesn't
> > correspond to the current bus state. It may happen what BB-bit will be 0,
> > while the bus is busy due to another I2C master activity.
> >
> > Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> > and results in controller timeout. More over, in some cases IP could
> > interrupt another master's transfer and corrupt data on wire.
> >
> > The commit implement method allowing to prevent IP from entering into
> > "controller timeout" state and from "data corruption" state.
> >
> > The one drawback is the need to wait for 10ms before the first transfer.
> >
> > Tested on Beagleboard XM C.
> > Tested on BBB and AM437x Starter Kit by Felipe Balbi.
> >
> > Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> 
> This patch recently hit linux-next (as commit 903c3859f77f) and boot
> breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards
> was bisected down to this commit.
> 
> Kevin
> 
> [1] http://status.armcloud.us/boot/?next-20141124&omap

btw, based on Kevin's boot test, only OMAP3530 is failing.

-- 
balbi

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

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Wolfram Sang]

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

On Mon, Nov 24, 2014 at 01:10:23PM -0600, Felipe Balbi wrote:
> On Mon, Nov 24, 2014 at 11:08:48AM -0800, Kevin Hilman wrote:
> > On Sat, Nov 22, 2014 at 11:47 AM, Alexander Kochetkov
> > <al.kochet@gmail.com> wrote:
> > > In a multimaster environment, after IP software reset, BB-bit value doesn't
> > > correspond to the current bus state. It may happen what BB-bit will be 0,
> > > while the bus is busy due to another I2C master activity.
> > >
> > > Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> > > and results in controller timeout. More over, in some cases IP could
> > > interrupt another master's transfer and corrupt data on wire.
> > >
> > > The commit implement method allowing to prevent IP from entering into
> > > "controller timeout" state and from "data corruption" state.
> > >
> > > The one drawback is the need to wait for 10ms before the first transfer.
> > >
> > > Tested on Beagleboard XM C.
> > > Tested on BBB and AM437x Starter Kit by Felipe Balbi.
> > >
> > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> > > Tested-by: Felipe Balbi <balbi@ti.com>
> > > Reviewed-by: Felipe Balbi <balbi@ti.com>
> > 
> > This patch recently hit linux-next (as commit 903c3859f77f) and boot
> > breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards
> > was bisected down to this commit.
> > 
> > Kevin
> > 
> > [1] http://status.armcloud.us/boot/?next-20141124&omap
> 
> btw, based on Kevin's boot test, only OMAP3530 is failing.

OK, giving Alexander some time for a fix. If it can't be found, I'll
revert this patch. Thanks!


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

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Tony Lindgren]

* Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> [141124 11:14]:
> On Mon, Nov 24, 2014 at 01:10:23PM -0600, Felipe Balbi wrote:
> > On Mon, Nov 24, 2014 at 11:08:48AM -0800, Kevin Hilman wrote:
> > > On Sat, Nov 22, 2014 at 11:47 AM, Alexander Kochetkov
> > > <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > In a multimaster environment, after IP software reset, BB-bit value doesn't
> > > > correspond to the current bus state. It may happen what BB-bit will be 0,
> > > > while the bus is busy due to another I2C master activity.
> > > >
> > > > Any transfer started when BB=0 and bus is busy wouldn't be completed by IP
> > > > and results in controller timeout. More over, in some cases IP could
> > > > interrupt another master's transfer and corrupt data on wire.
> > > >
> > > > The commit implement method allowing to prevent IP from entering into
> > > > "controller timeout" state and from "data corruption" state.
> > > >
> > > > The one drawback is the need to wait for 10ms before the first transfer.
> > > >
> > > > Tested on Beagleboard XM C.
> > > > Tested on BBB and AM437x Starter Kit by Felipe Balbi.
> > > >
> > > > Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > > > Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > > 
> > > This patch recently hit linux-next (as commit 903c3859f77f) and boot
> > > breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards
> > > was bisected down to this commit.
> > > 
> > > Kevin
> > > 
> > > [1] http://status.armcloud.us/boot/?next-20141124&omap
> > 
> > btw, based on Kevin's boot test, only OMAP3530 is failing.
> 
> OK, giving Alexander some time for a fix. If it can't be found, I'll
> revert this patch. Thanks!

I can confirm reverting it fixes things for me as well thanks.

Regards,

Tony

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

24 нояб. 2014 г., в 22:08, Kevin Hilman <khilman@kernel.org> написал(а):

> This patch recently hit linux-next (as commit 903c3859f77f) and boot
> breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards
> was bisected down to this commit.
> 
> Kevin
> 
> [1] http://status.armcloud.us/boot/?next-20141124&omap


Could someone advice with debugging on OMAP3530?

I'd like to apply one one more commit to see events during boot and
see actual signals on wire (during of timeout)?

Otherwise, commit must dropped (or rewritten such way it is disabled by default).


24 нояб. 2014 г., в 22:13, Wolfram Sang <wsa@the-dreams.de> написал(а):

> OK, giving Alexander some time for a fix. If it can't be found, I'll
> revert this patch. Thanks!

What deadline?

Alexander.



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Tony Lindgren]

* Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [141124 11:41]:
> 
> 24 нояб. 2014 г., в 22:08, Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> написал(а):
> 
> > This patch recently hit linux-next (as commit 903c3859f77f) and boot
> > breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards
> > was bisected down to this commit.
> > 
> > Kevin
> > 
> > [1] http://status.armcloud.us/boot/?next-20141124&omap
> 
> 
> Could someone advice with debugging on OMAP3530?

Just try to boot it with your patch? :)
 
> I'd like to apply one one more commit to see events during boot and
> see actual signals on wire (during of timeout)?

If you post a patch, I can test boot with it. Looks like the failing
ones have i2c rev3.3 on 34xx vs rev4.4 on 36xx.

But the test function should not loop forever in any case I take?

So I doubt we just want to change the test for for a higher rev number
for  OMAP_I2C_REV_ON_3430_3530.
 
> Otherwise, commit must dropped (or rewritten such way it is disabled by default).
> 
> 
> 24 нояб. 2014 г., в 22:13, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> написал(а):
> 
> > OK, giving Alexander some time for a fix. If it can't be found, I'll
> > revert this patch. Thanks!
> 
> What deadline?

I'd say by Tuesday as we have multiple automated test systems failing
as soon as people update their trees. And when they are failing, we
may miss other breakage happening in linux next.

Regards,

Tony

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

Guys, I really sorry for that :(

> Just try to boot it with your patch? :)

Yes, may be two-three times (max).
Something (u-boot, may be) leave the bus in the wrong state.
Really strange.

> But the test function should not loop forever in any case I take?

It doesn't loop forever. It finish with message:
"[    3.046691] omap_i2c 48070000.i2c: timeout waiting for bus ready"

> So I doubt we just want to change the test for for a higher rev number
> for  OMAP_I2C_REV_ON_3430_3530.


Yes, I'll do it within 15 minutes.
And later, if it possible, I'd try to see what happens.

Alexander.

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

24 нояб. 2014 г., в 23:05, Alexander Kochetkov <al.kochet@gmail.com> написал(а):

> Something (u-boot, may be) leave the bus in the wrong state.
> Really strange.

Actually something wrong with i2c-pullups on i2c.1 bus on fault boards.
May be these are boards without pull-ups?
All beagles doesn't have internal pull-ups on i2c.1 since u-boot 2011.x.

Here is the bug in the u-boot related to beagle:
http://git.denx.de/?p=u-boot.git;a=commit;h=04e2a13336f0e507ef416bbede3be92b79c46594

Yes, I made fix, but keep that in mind.

For example one of the boards (omap3-beagle):
http://status.armcloud.us/boot/omap3-beagle/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/
http://status.armcloud.us/boot/omap3-beagle,legacy/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/
http://status.armcloud.us/boot/omap3-beagle/job/next/kernel/next-20141124/defconfig/arm-multi_v7_defconfig/

has following warning message in the u-boot log:
> U-Boot 2014.07 (Aug 21 2014 - 11:03:05)
> 
> OMAP3530-GP ES3.0, CPU-OPP2, L3-165MHz, Max CPU Clock 600 MHz
> OMAP3 Beagle board + LPDDR/NAND
> ....
> Beagle Rev C1/C2/C3
> Timed out in wait_for_event: status=1000
> Check if pads/pull-ups of bus 1 are properly configured

Also beagle schematic has following log entry for A3:
4. Added optional pullup resistors on I2C2_SCL and I2C_SDA into the layout.

Is the fault beagle is pre A3 revision?

I can't tell anything about second one board (omap3-overo-tobi), because I could not get it schematic.

And how you have i2c.1 working without pull-ups I don't know.

Alexander.

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Tony Lindgren]

* Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [141124 16:11]:
> 
> 24 нояб. 2014 г., в 23:05, Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> написал(а):
> 
> > Something (u-boot, may be) leave the bus in the wrong state.
> > Really strange.
> 
> Actually something wrong with i2c-pullups on i2c.1 bus on fault boards.
> May be these are boards without pull-ups?

It could be. Boards that use external i2c pulls should have all the
internal pulls disabled as the pulls get connected in parallel and
the total resistance decreases. And then the i2c signals may not be
up to the spec.

Note that there are internal pulls in the i2c controller, and in the
i2c padconf registers.

> All beagles doesn't have internal pull-ups on i2c.1 since u-boot 2011.x.

The pulls should be enabled based on the board and possibly based
on the board revision.
 
> Here is the bug in the u-boot related to beagle:
> http://git.denx.de/?p=u-boot.git;a=commit;h=04e2a13336f0e507ef416bbede3be92b79c46594
> 
> Yes, I made fix, but keep that in mind.
> 
> For example one of the boards (omap3-beagle):
> http://status.armcloud.us/boot/omap3-beagle/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/
> http://status.armcloud.us/boot/omap3-beagle,legacy/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/
> http://status.armcloud.us/boot/omap3-beagle/job/next/kernel/next-20141124/defconfig/arm-multi_v7_defconfig/
> 
> has following warning message in the u-boot log:
> > U-Boot 2014.07 (Aug 21 2014 - 11:03:05)
> > 
> > OMAP3530-GP ES3.0, CPU-OPP2, L3-165MHz, Max CPU Clock 600 MHz
> > OMAP3 Beagle board + LPDDR/NAND
> > ....
> > Beagle Rev C1/C2/C3
> > Timed out in wait_for_event: status=1000
> > Check if pads/pull-ups of bus 1 are properly configured
> 
> Also beagle schematic has following log entry for A3:
> 4. Added optional pullup resistors on I2C2_SCL and I2C_SDA into the layout.
> 
> Is the fault beagle is pre A3 revision?

Seems to be C1/C2/C3 based on the above logs?
 
> I can't tell anything about second one board (omap3-overo-tobi), because I could not get it schematic.
> 
> And how you have i2c.1 working without pull-ups I don't know.

I checked on the LDP and it seems to have external pulls for i2c.
I also tested n900 without your fix, and it's failing too. On n900
there are external pulls and all the internal pulls should be
disabled.

Regards,

Tony

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

24 нояб. 2014 г., в 22:47, Tony Lindgren <tony@atomide.com> написал(а):

> I'd say by Tuesday as we have multiple automated test systems failing
> as soon as people update their trees. And when they are failing, we
> may miss other breakage happening in linux next.

Fix:
http://marc.info/?l=linux-i2c&m=141686120720069&w=2


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values

[Alexander Kochetkov]

Hello, Tony!

24.11.2014 22:47, Tony Lindgren <tony@atomide.com> *:

> If you post a patch, I can test boot with it. Looks like the failing
> ones have i2c rev3.3 on 34xx vs rev4.4 on 36xx.

> So I doubt we just want to change the test for for a higher rev number
> for  OMAP_I2C_REV_ON_3430_3530.

You 100% right here.

Functional mode bits are unimplemented in the SYSTEST register on omap3530.
"10:5 Reserved Write 0s for future compatibility. Read returns 0."
That was the reason.

My fault.
Thank you for giving right directions.
Thanks Kevin for running test, so I could debug the reason.

Regards,
Alexander.

[PATCH] omap: i2c: don't check bus state IP rev3.3 and earlier

[Alexander Kochetkov]

Commit 903c3859f77f9b0aace551da03267ef7a211dbc4 ("i2c: omap: implement
workaround for handling invalid BB-bit values") introduce the error result
in boot test fault on OMAP3530 boards

The patch fix the error (disable i2c bus test for OMAP3530).

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Fixes: 903c3859f77f9b0aace551da03267ef7a211dbc4
---
 drivers/i2c/busses/i2c-omap.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5d92d0e..4563200 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -344,8 +344,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
 		/* SYSC register is cleared by the reset; rewrite it */
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
 
-		/* Schedule I2C-bus monitoring on the next transfer */
-		dev->bb_valid = 0;
+		if (dev->rev > OMAP_I2C_REV_ON_3430_3530) {
+			/* Schedule I2C-bus monitoring on the next transfer */
+			dev->bb_valid = 0;
+		}
 	}
 
 	return 0;
@@ -460,7 +462,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	dev->scllstate = scll;
 	dev->sclhstate = sclh;
 
-	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
+	if (dev->rev <= OMAP_I2C_REV_ON_3430_3530) {
 		/* Not implemented */
 		dev->bb_valid = 1;
 	}
-- 
1.7.9.5

Re: [PATCH] omap: i2c: don't check bus state IP rev3.3 and earlier

[Tony Lindgren]

* Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [141124 12:35]:
> Commit 903c3859f77f9b0aace551da03267ef7a211dbc4 ("i2c: omap: implement
> workaround for handling invalid BB-bit values") introduce the error result
> in boot test fault on OMAP3530 boards
> 
> The patch fix the error (disable i2c bus test for OMAP3530).

Maybe add Reported-by: credit for Kevin Hilman here?
Other than that, this fixes the problem for me:

Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 903c3859f77f9b0aace551da03267ef7a211dbc4
> ---
>  drivers/i2c/busses/i2c-omap.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 5d92d0e..4563200 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -344,8 +344,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>  		/* SYSC register is cleared by the reset; rewrite it */
>  		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
>  
> -		/* Schedule I2C-bus monitoring on the next transfer */
> -		dev->bb_valid = 0;
> +		if (dev->rev > OMAP_I2C_REV_ON_3430_3530) {
> +			/* Schedule I2C-bus monitoring on the next transfer */
> +			dev->bb_valid = 0;
> +		}
>  	}
>  
>  	return 0;
> @@ -460,7 +462,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	dev->scllstate = scll;
>  	dev->sclhstate = sclh;
>  
> -	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
> +	if (dev->rev <= OMAP_I2C_REV_ON_3430_3530) {
>  		/* Not implemented */
>  		dev->bb_valid = 1;
>  	}
> -- 
> 1.7.9.5
> 

[PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Alexander Kochetkov]

Commit 903c3859f77f9b0aace551da03267ef7a211dbc4 ("i2c: omap: implement
workaround for handling invalid BB-bit values") introduce the error result
in boot test fault on OMAP3530 boards

The patch fix the error (disable i2c bus test for OMAP3530).

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Fixes: 903c3859f77f9b0aace551da03267ef7a211dbc4
Reported-by: Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

---
 drivers/i2c/busses/i2c-omap.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5d92d0e..4563200 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -344,8 +344,10 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
 		/* SYSC register is cleared by the reset; rewrite it */
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
 
-		/* Schedule I2C-bus monitoring on the next transfer */
-		dev->bb_valid = 0;
+		if (dev->rev > OMAP_I2C_REV_ON_3430_3530) {
+			/* Schedule I2C-bus monitoring on the next transfer */
+			dev->bb_valid = 0;
+		}
 	}
 
 	return 0;
@@ -460,7 +462,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	dev->scllstate = scll;
 	dev->sclhstate = sclh;
 
-	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
+	if (dev->rev <= OMAP_I2C_REV_ON_3430_3530) {
 		/* Not implemented */
 		dev->bb_valid = 1;
 	}
-- 
1.7.9.5

Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Wolfram Sang]

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

On Tue, Nov 25, 2014 at 02:20:55AM +0400, Alexander Kochetkov wrote:
> Commit 903c3859f77f9b0aace551da03267ef7a211dbc4 ("i2c: omap: implement
> workaround for handling invalid BB-bit values") introduce the error result
> in boot test fault on OMAP3530 boards
> 
> The patch fix the error (disable i2c bus test for OMAP3530).
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Fixes: 903c3859f77f9b0aace551da03267ef7a211dbc4
> Reported-by: Kevin Hilman <khilman@kernel.org>
> Tested-by: Tony Lindgren <tony@atomide.com>
> 

Applied to for-next, thanks!

I'll push out this evening to make the boot tests work again. If there
is more to be investigated, either hurry up and post v3 ;) or let me
know that you need more time.


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

Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Alexander Kochetkov]

25 нояб. 2014 г., в 17:19, Wolfram Sang <wsa@the-dreams.de> написал(а):

> I'll push out this evening to make the boot tests work again. If there
> is more to be investigated, either hurry up and post v3 ;) or let me
> know that you need more time.

Ok, thank you. Let the fix go to the kernel-next.
Maybe small fix to subject "omap: i2c:" to "i2c: omap:"

I still guessing what some boards have broken i2c pull-ups.
And real fix must go in the board file.
http://www.spinics.net/lists/linux-i2c/msg17750.html

I could create a patch to confirm this.
But I don't have omap3530 boards to run.
I'll be very appreciated if someone could run the patch.

Regards,
Alexander.

Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Kevin Hilman]

Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> 25 нояб. 2014 г., в 17:19, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> написал(а):
>
>> I'll push out this evening to make the boot tests work again. If there
>> is more to be investigated, either hurry up and post v3 ;) or let me
>> know that you need more time.
>
> Ok, thank you. Let the fix go to the kernel-next.
> Maybe small fix to subject "omap: i2c:" to "i2c: omap:"
>
> I still guessing what some boards have broken i2c pull-ups.
> And real fix must go in the board file.
> http://www.spinics.net/lists/linux-i2c/msg17750.html
>
> I could create a patch to confirm this.
> But I don't have omap3530 boards to run.
> I'll be very appreciated if someone could run the patch.

I'll test your patch on all my OMAP boards.  Put whatever debug output
you want, and I'll send you links to all the boot output.

Kevin

Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Alexander Kochetkov]

25 нояб. 2014 г., в 22:13, Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> написал(а):

> I'll test your patch on all my OMAP boards.  Put whatever debug output
> you want, and I'll send you links to all the boot output.

Hello, Kevin!

I've sent the patch[1]. Could you be so kind to run it on all your OMAP boards?
Thank you very much!
It is not urgent at all.

What is the preferred way for giving patches for you (for future)?
I have one more fixes for i2c-omap (I think final).
I don't want to break tests anymore.

And I found, that n900 boot test PASS, but in fact it doesn't[2].

Alexander.

[1] http://marc.info/?l=linux-i2c&m=141702877518332&w=2
[2] http://status.armcloud.us/boot/omap3-n900/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/

Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Kevin Hilman]

Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> 25 нояб. 2014 г., в 22:13, Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> написал(а):
>
>> I'll test your patch on all my OMAP boards.  Put whatever debug output
>> you want, and I'll send you links to all the boot output.
>
> Hello, Kevin!
>
> I've sent the patch[1]. Could you be so kind to run it on all your OMAP boards?
> Thank you very much!
> It is not urgent at all.

Done.  Built for omap2plus_defconfig, boot reports for all my OMAP
boards here: 
http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/

> What is the preferred way for giving patches for you (for future)?

Email is fine.  I have things fully automated for primary upstream trees
(mainline, linux-next, stable, etc.) but for stuff like this, I can
trigger one-off tests.

However, if Tony wants to have a branch (besides the one already goes to
linux-next) which I would add to the automation cycle, I'm willing to do that.

> I have one more fixes for i2c-omap (I think final).
> I don't want to break tests anymore.
>
> And I found, that n900 boot test PASS, but in fact it doesn't[2].
> [2] http://status.armcloud.us/boot/omap3-n900/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/

Right.  For these boot tests, PASS means it got to a userspace shell,
which it did.  The kernel got some warnings etc. during boot, but it
still booted up to a shell.

Kevin

Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier

[Kevin Hilman]

Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Commit 903c3859f77f9b0aace551da03267ef7a211dbc4 ("i2c: omap: implement
> workaround for handling invalid BB-bit values") introduce the error result
> in boot test fault on OMAP3530 boards
>
> The patch fix the error (disable i2c bus test for OMAP3530).
>
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 903c3859f77f9b0aace551da03267ef7a211dbc4
> Reported-by: Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

Tested-by: Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

I tested DT and legacy boot on 3430/n900, 3530/beagle and
3530/overo-tobi.  All boot fine.

Kevin

[RFC] i2c: omap: TEST: do IP reset during probe.

[Alexander Kochetkov]

NOT FOR UPSTREAM

The patch checks if IP reset during probe could bring I2C bus
to a free state on omap2430 - omap3530 boards.

I guess, IP hold one of I2C lines in a low state.
I guess, u-boot haven't sent a stop condition.

The patch is rebased on branch 'i2c/for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux
(6e79807443cba7397cd855ed29d6faba51d4c893)

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reported-by: Kevin Hilman <khilman@kernel.org>
Tested-by: Kevin Hilman <khilman@kernel.org>
---
 drivers/i2c/busses/i2c-omap.c |   41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 4563200..865c9a1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -282,6 +282,26 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 				(i2c_dev->regs[reg] << i2c_dev->reg_shift));
 }
 
+static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev);
+
+static inline void
+omap_i2c_dump_state(const char *func, int line, struct omap_i2c_dev *dev, const char *msg)
+{
+	u16 systest = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
+	dev_info(dev->dev, "%s: STAT=0x%04x; IE=0x%04x; CON=0x%04x; SYSTEST=0x%04x; SDA=%d%d [OI]; SCL=%d%d [OI] (%s:%d)\n",
+		msg,
+		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG),
+		omap_i2c_read_reg(dev, OMAP_I2C_IE_REG),
+		omap_i2c_read_reg(dev, OMAP_I2C_CON_REG),
+		systest,
+		(systest & OMAP_I2C_SYSTEST_SDA_O_FUNC) ? 1 : 0,
+		(systest & OMAP_I2C_SYSTEST_SDA_I_FUNC) ? 1 : 0,
+		(systest & OMAP_I2C_SYSTEST_SCL_O_FUNC) ? 1 : 0,
+		(systest & OMAP_I2C_SYSTEST_SCL_I_FUNC) ? 1 : 0,
+		func, line);
+}
+#define OMAP_I2C_DUMP_STATE(dev, msg) omap_i2c_dump_state(__func__, __LINE__, (dev), (msg))
+
 static void __omap_i2c_init(struct omap_i2c_dev *dev)
 {
 
@@ -469,6 +489,23 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 
 	__omap_i2c_init(dev);
 
+	msleep(1);
+	OMAP_I2C_DUMP_STATE(dev, "init0");
+
+	dev->bb_valid = 0;
+	omap_i2c_wait_for_bb_valid(dev);
+	OMAP_I2C_DUMP_STATE(dev, "init1");
+	dev_info(dev->dev, "init1: bb_valid=%d\n", dev->bb_valid);
+
+	omap_i2c_reset(dev);
+	__omap_i2c_init(dev);
+	dev->bb_valid = 0;
+	omap_i2c_wait_for_bb_valid(dev);
+	OMAP_I2C_DUMP_STATE(dev, "init2");
+	dev_info(dev->dev, "init2: bb_valid=%d\n", dev->bb_valid);
+
+	dev->bb_valid = 1;
+
 	return 0;
 }
 
@@ -1367,8 +1404,8 @@ omap_i2c_probe(struct platform_device *pdev)
 		goto err_unuse_clocks;
 	}
 
-	dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n", adap->nr,
-		 major, minor, dev->speed);
+	dev_info(dev->dev, "bus %d rev%d.%d at %d kHz (rev %08x)\n", adap->nr,
+		 major, minor, dev->speed, dev->rev);
 
 	pm_runtime_mark_last_busy(dev->dev);
 	pm_runtime_put_autosuspend(dev->dev);
-- 
1.7.9.5

Re: [RFC] i2c: omap: TEST: do IP reset during probe.

[Kevin Hilman]

Alexander Kochetkov <al.kochet@gmail.com> writes:

> NOT FOR UPSTREAM
>
> The patch checks if IP reset during probe could bring I2C bus
> to a free state on omap2430 - omap3530 boards.
>
> I guess, IP hold one of I2C lines in a low state.
> I guess, u-boot haven't sent a stop condition.
>
> The patch is rebased on branch 'i2c/for-next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux
> (6e79807443cba7397cd855ed29d6faba51d4c893)
>
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reported-by: Kevin Hilman <khilman@kernel.org>
> Tested-by: Kevin Hilman <khilman@kernel.org>

Built for omap2plus_defconfig and tested on all my OMAP boards.  Results
here:

http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/

Re: [RFC] i2c: omap: TEST: do IP reset during probe.

[Tony Lindgren]

* Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [141126 13:27]:
> Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
> > NOT FOR UPSTREAM
> >
> > The patch checks if IP reset during probe could bring I2C bus
> > to a free state on omap2430 - omap3530 boards.
> >
> > I guess, IP hold one of I2C lines in a low state.
> > I guess, u-boot haven't sent a stop condition.
> >
> > The patch is rebased on branch 'i2c/for-next' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux
> > (6e79807443cba7397cd855ed29d6faba51d4c893)
> >
> > Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Reported-by: Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Tested-by: Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Built for omap2plus_defconfig and tested on all my OMAP boards.  Results
> here:
> 
> http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/

And below is the output from 2430sdp with linux next plus Alexander's
test patch. Looks like for some time 2430 i2c has not been behaving
reliably unless I force compatible to ti,omap2420-i2c instead of
ti,omap2430-i2c.. The output below is with ti,omap2430-i2c.

Regards,

Tony

8< -------------
[    0.913574] omap_i2c 48070000.i2c: init0: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:493)
[    1.931365] omap_i2c 48070000.i2c: timeout waiting for bus ready
[    1.931457] omap_i2c 48070000.i2c: init1: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:497)
[    1.931518] omap_i2c 48070000.i2c: init1: bb_valid=0
[    2.949249] omap_i2c 48070000.i2c: timeout waiting for bus ready
[    2.949340] omap_i2c 48070000.i2c: init2: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:504)
[    2.949401] omap_i2c 48070000.i2c: init2: bb_valid=0
[    2.952941] omap_i2c 48070000.i2c: bus 0 rev3.3 at 100 kHz (rev 00000036)
[    2.969299] omap_i2c 48072000.i2c: init0: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:493)
[    3.987091] omap_i2c 48072000.i2c: timeout waiting for bus ready
[    3.987182] omap_i2c 48072000.i2c: init1: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:497)
[    3.987243] omap_i2c 48072000.i2c: init1: bb_valid=0
[    5.004974] omap_i2c 48072000.i2c: timeout waiting for bus ready
[    5.005096] omap_i2c 48072000.i2c: init2: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:504)
[    5.005126] omap_i2c 48072000.i2c: init2: bb_valid=0
[    5.017517] omap_i2c 48072000.i2c: bus 1 rev3.3 at 100 kHz (rev 00000036)
[    7.555419] twl4030_keypad 48072000.i2c:twl@48:keypad: OF: linux,keymap property not defined in /ocp/i2c@48072000/twl@48/keypad
[    7.567626] twl4030_keypad 48072000.i2c:twl@48:keypad: Failed to build keymap
[    7.575439] twl4030_keypad: probe of 48072000.i2c:twl@48:keypad failed with error -2
[    7.599639] input: twl4030_pwrbutton as /devices/platform/ocp/48072000.i2c/i2c-1/1-0048/48072000.i2c:twl@48:pwrbutton/input/input1
[    7.624450] twl_rtc 48072000.i2c:twl@48:rtc: Power up reset detected.
[    7.631988] twl_rtc 48072000.i2c:twl@48:rtc: Enabling TWL-RTC
[    7.655456] twl_rtc 48072000.i2c:twl@48:rtc: rtc core: registered 48072000.i2c:twl@48 as rtc0
[    7.923187] i2c /dev entries driver
[    8.246795] twl_rtc 48072000.i2c:twl@48:rtc: hctosys: unable to read the hardware clock
[    9.675994] omap_i2c 48072000.i2c: controller timed out
[   10.704010] omap_i2c 48072000.i2c: controller timed out
[   11.734069] omap_i2c 48072000.i2c: controller timed out
root@omap2430sdp:/# [   12.823638] omap_i2c 48072000.i2c: controller timed out
[   12.834747] twl4030: I2C error -110 reading PIH ISR

Re: [RFC] i2c: omap: TEST: do IP reset during probe.

[Alexander Kochetkov]

Hello, Tony!

I just want to know, is multimaster i2c feature is interesting for TI SOC,
so I could send another patches?

Or it's better to leave the thing without changes, as current single master version
well tested and work?

Also I have a draft version of mixed multimaster/slave version. But it could introduce new bugs.
Are we ready for that? Thats because IP behavior, sometimes, doesn't correspond to TRMs[4][5].
It's the one of strange IP I ever seen on TI SOC. And TRM not as detailed as DSP TRMs.

Yes, I test the patch. But  IP handling is really tricky.
So, I doubt.

Looks, like you haven't seen my response in another thread[1].
So, duplicate it here.

24.11.2014 22:47, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> *:

> If you post a patch, I can test boot with it. Looks like the failing
> ones have i2c rev3.3 on 34xx vs rev4.4 on 36xx.

> So I doubt we just want to change the test for for a higher rev number
> for  OMAP_I2C_REV_ON_3430_3530.

You 100% right here.
My fault.
Thank you for giving right directions.
Thanks Kevin for running test, so I could debug the reason.

Functional mode bits are unimplemented in the SYSTEST register on omap3530.
"10:5 Reserved Write 0s for future compatibility. Read returns 0."
That was the reason. SYSTEST always 0.

It is possible (and I could do it) to implement the bus check using SYSTEST SDA/SCL loopback mode.

One more problem, I found that BB-check from the patch[2] sometimes (very rarely) doesn't work
as expected. Sometimes the master start transfer while bus is in use by another master. 
It happens if another master continuously read from eeprom array of 0xff.

So, one of problems on the way of running IP in a multimaster mode, is BB-bit behavior.
I reported the problem to ti forum[3] and expect some response.

Regards,
Alexander.

[1] http://www.spinics.net/lists/linux-i2c/msg17797.html
[2] http://www.spinics.net/lists/linux-i2c/msg17678.html
[3] http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/537/t/383876.aspx
[4] omap3530 - TRM - spruf98y
[5] AM-DM37x Multimedia Device Silicon Revision 1.x  - sprugn4r

Re: [RFC] i2c: omap: TEST: do IP reset during probe.

[Tony Lindgren]

* Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [141128 15:27]:
> Hello, Tony!
> 
> I just want to know, is multimaster i2c feature is interesting for TI SOC,
> so I could send another patches?

Sure and thanks for looking into fixing things.
 
> Or it's better to leave the thing without changes, as current single master version
> well tested and work?

Well once the fixes are in, I don't see any reason to not add
multimaster support.
 
> Also I have a draft version of mixed multimaster/slave version. But it could introduce new bugs.
> Are we ready for that? Thats because IP behavior, sometimes, doesn't correspond to TRMs[4][5].
> It's the one of strange IP I ever seen on TI SOC. And TRM not as detailed as DSP TRMs.

I think we can pretty easily test the i2c support before things get
merged. I guess it should then be possible to loop two i2c controllers
and run automated tests on them :)
 
> Looks, like you haven't seen my response in another thread[1].
> So, duplicate it here.

Sorry I guess I forgot to reply, let me know if I still missed something.

I'll give your two fixes a try on Monday hopefully.

Regards,

Tony

Re: [RFC] i2c: omap: TEST: do IP reset during probe.

[Alexander Kochetkov]

29 нояб. 2014 г., в 1:13, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> написал(а):

> Looks like for some time 2430 i2c has not been behaving
> reliably

    commit dd74548ddece4b9d68e5528287a272fa552c81d0 ("i2c: omap:
    resize fifos before each message") dropped check for dev->buf_len.
    As result, data processing loop cause dev->buf overruns for
    devices with 16-bit data register (omap2420 only).
    Found by code review.

I have the patch for that. omap2420 actually broken at all.
But I'm not ready to send it right now.

Part of the patch:

-       while (num_bytes--) {
+       while (num_bytes) {
                w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
                *dev->buf++ = w;
                dev->buf_len--;
+               num_bytes--;
 
                /*
                 * Data reg in 2430, omap3 and
                 * omap4 is 8 bit wide
                 */
                if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
-                       *dev->buf++ = w >> 8;
-                       dev->buf_len--;
+                       if (num_bytes) {
+                               *dev->buf++ = w >> 8;
+                               dev->buf_len--;
+                               num_bytes--;
+                       }
                }
        }



-       while (num_bytes--) {
+       while (num_bytes) {
                if (dev->errata & I2C_OMAP_ERRATA_I462) {
                        int ret;
 
@@ -931,14 +947,18 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
 
                w = *dev->buf++;
                dev->buf_len--;
+               num_bytes--;
 
                /*
                 * Data reg in 2430, omap3 and
                 * omap4 is 8 bit wide
                 */
                if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
-                       w |= *dev->buf++ << 8;
-                       dev->buf_len--;
+                       if (num_bytes) {
+                               w |= *dev->buf++ << 8;
+                               dev->buf_len--;
+                               num_bytes--;
+                       }
                }

[PATCH v3 4/4] i2c: omap: add notes related to i2c multimaster mode

[Alexander Kochetkov]

No functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e67b7d0..f276c0c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -300,6 +300,12 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
 	/*
+	 * NOTE: right after setting CON_EN, STAT_BB could be 0 while the
+	 * bus is busy. It will be changed to 1 on the next IP FCLK clock.
+	 * udelay(1) will be enough to fix that.
+	 */
+
+	/*
 	 * Don't write to this register if the IE state is 0 as it can
 	 * cause deadlock.
 	 */
@@ -660,7 +666,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 
 	if (!dev->b_hw && stop)
 		w |= OMAP_I2C_CON_STP;
-
+	/*
+	 * NOTE: STAT_BB bit could became 1 here if another master occupy
+	 * the bus. IP successfully complete transfer when the bus will be
+	 * free again (BB reset to 0).
+	 */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
 
 	/*
-- 
1.7.9.5

[PATCH v3 1/4] i2c: omap: cleanup register definitions

[Alexander Kochetkov]

Delete STAT_AD0 mask as unrelated to current IP (omap1?).
Delete DEBUG conditional around SYSTEST masks group.
Add SYSTEST functional mode masks for SCL and SDA.
Add STAT_BF mask.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 32dc651..adfac9b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -98,7 +98,7 @@ enum {
 #define OMAP_I2C_STAT_ROVR	(1 << 11)	/* Receive overrun */
 #define OMAP_I2C_STAT_XUDF	(1 << 10)	/* Transmit underflow */
 #define OMAP_I2C_STAT_AAS	(1 << 9)	/* Address as slave */
-#define OMAP_I2C_STAT_AD0	(1 << 8)	/* Address zero */
+#define OMAP_I2C_STAT_BF	(1 << 8)	/* Bus Free */
 #define OMAP_I2C_STAT_XRDY	(1 << 4)	/* Transmit data ready */
 #define OMAP_I2C_STAT_RRDY	(1 << 3)	/* Receive data ready */
 #define OMAP_I2C_STAT_ARDY	(1 << 2)	/* Register access ready */
@@ -146,16 +146,20 @@ enum {
 #define OMAP_I2C_SCLH_HSSCLH	8
 
 /* I2C System Test Register (OMAP_I2C_SYSTEST): */
-#ifdef DEBUG
 #define OMAP_I2C_SYSTEST_ST_EN		(1 << 15)	/* System test enable */
 #define OMAP_I2C_SYSTEST_FREE		(1 << 14)	/* Free running mode */
 #define OMAP_I2C_SYSTEST_TMODE_MASK	(3 << 12)	/* Test mode select */
 #define OMAP_I2C_SYSTEST_TMODE_SHIFT	(12)		/* Test mode select */
+/* Functional mode */
+#define OMAP_I2C_SYSTEST_SCL_I_FUNC	(1 << 8)	/* SCL line input value */
+#define OMAP_I2C_SYSTEST_SCL_O_FUNC	(1 << 7)	/* SCL line output value */
+#define OMAP_I2C_SYSTEST_SDA_I_FUNC	(1 << 6)	/* SDA line input value */
+#define OMAP_I2C_SYSTEST_SDA_O_FUNC	(1 << 5)	/* SDA line output value */
+/* SDA/SCL IO mode */
 #define OMAP_I2C_SYSTEST_SCL_I		(1 << 3)	/* SCL line sense in */
 #define OMAP_I2C_SYSTEST_SCL_O		(1 << 2)	/* SCL line drive out */
 #define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
 #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
-#endif
 
 /* OCP_SYSSTATUS bit definitions */
 #define SYSS_RESETDONE_MASK		(1 << 0)
-- 
1.7.9.5

[PATCH v3 3/4] i2c: omap: don't reset controller if Arbitration Lost detected

[Alexander Kochetkov]

Arbitration Lost is an expected situation in a multimaster
environment. I2C controller (IP) correctly detect and report AL.

The only one visible reason for reseting IP in the AL case is
to avoid advisory 1.94 (omap3) and errata i595 (omap4): "I2C:
After an Arbitration is Lost the Module Incorrectly Starts
the Next Transfer".

Errata workaround states: "The MST and STT bits inside I2C_CON
should be set to 1 at the same moment (avoid setting the MST bit
to 1 while STT = 0)." The driver never set MST and STT bits
separately and doesn't create condition for errata. So the reset
is not necessary.

Also corrected return value for AL to -EAGAIN.

Tested on Beagleboard XM C.
Tested on BBB and AM437x Starter Kit by Felipe Balbi.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a238a1d..e67b7d0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -703,13 +703,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		return 0;
 
 	/* We have an error */
-	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
-			    OMAP_I2C_STAT_XUDF)) {
+	if (dev->cmd_err & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_XUDF)) {
 		omap_i2c_reset(dev);
 		__omap_i2c_init(dev);
 		return -EIO;
 	}
 
+	if (dev->cmd_err & OMAP_I2C_STAT_AL)
+		return -EAGAIN;
+
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
-- 
1.7.9.5

Re: [PATCH v3 0/4] i2c: omap: patch series related to multimaster mode

[Wolfram Sang]

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

On Sat, Nov 22, 2014 at 11:47:10PM +0400, Alexander Kochetkov wrote:
> Here is the patch series intended to improve stability of
> i2c-omap driver in the i2c multimaster environments.
> 
> Tested on Beagleboard XM C.
> For now all fine. No controller timeouts, no data corruptions.
> 
> Also impelemented i2c bus fault detection during startup and
> after reset.
> 
> So, instead of the message "controller timeout" blaming IP,
> you get "timeout waiting for bus ready".
> 
> Rebased against i2c/for-next (eb61694b43dea0d43ea2574e7d48f476a6bdffb0)
> 
> Tested on the kernel from angstrom-v2013.06-yocto1.4
> Kernel recepie linux-mainline_3.2.bb
> git://github.com/Angstrom-distribution/meta-ti.git; branch angstrom-staging-yocto1.4
> With all i2-omap.c patches backported from upstream linux/master
> 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Alexander Kochetkov (4):
>   i2c: omap: cleanup register definitions
>   i2c: omap: implement workaround for handling invalid BB-bit values
>   i2c: omap: don't reset controller if Arbitration Lost detected
>   i2c: omap: add notes related to i2c multimaster mode

All applied to for-next, thanks!


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