[PATCH v2] OMAP3: I2C: Errata ID i207: Clear wrong RDR interrupt

[PATCH v2] OMAP3: I2C: Errata ID i207: Clear wrong RDR interrupt

[Manjunatha GK]

Under certain rare conditions, I2C_STAT[13].RDR bit may be set
and the corresponding interrupt fire, even there is no data in
the receive FIFO, or the I2C data transfer is still ongoing.
These spurious RDR events must be ignored by the software.

This patch handles and ignores RDR spurious interrupts.

Patch tested on OMAP zoom3 board.

Signed-off-by: Manjunatha GK <manjugk-l0cyMroinI0@public.gmane.org>
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
Cc: Kalliguddi, Hema <hemahk-l0cyMroinI0@public.gmane.org>
Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
---
Review comments for earlier post can be found at:
https://patchwork.kernel.org/patch/90122/

 drivers/i2c/busses/i2c-omap.c |   32 +++++++++++++++++++++++++++++++-
 1 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ae6f5c1..d4ec886 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -733,10 +733,40 @@ complete:
 		}
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
+
+			/*
+			 * I2C Errata(Errata Nos. OMAP2: 1.67, OMAP3: 1.8)
+			 * Not applicable for OMAP4.
+			 * Under certain rare conditions, RDR could be set again
+			 * when the bus is busy, then ignore the interrupt and
+			 * clear the interrupt.
+			 */
+			if ((stat & OMAP_I2C_STAT_RDR) && !cpu_is_omap44xx()) {
+				/* Step 1: If RDR is set, clear it */
+				omap_i2c_ack_stat(dev, stat & OMAP_I2C_STAT_RDR);
+
+				/* Step 2: */
+				if(!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG)
+							& OMAP_I2C_STAT_BB)) {
+					/* Step 3: */
+					while(omap_i2c_read_reg(dev,
+						OMAP_I2C_STAT_REG)
+							& OMAP_I2C_STAT_RDR) {
+						omap_i2c_ack_stat(dev, stat
+							& OMAP_I2C_STAT_RDR);
+						dev_err(dev->dev,
+						"I2C : RDR when the bus is busy.\n");
+						continue;
+					}
+
+				}
+				else
+					return IRQ_HANDLED;
+			}
 			if (dev->fifo_size) {
 				if (stat & OMAP_I2C_STAT_RRDY)
 					num_bytes = dev->fifo_size;
-				else    /* read RXSTAT on RDR interrupt */
+				else  /* Step4: read RXSTAT on RDR interrupt */
 					num_bytes = (omap_i2c_read_reg(dev,
 							OMAP_I2C_BUFSTAT_REG)
 							>> 8) & 0x3F;
-- 
1.6.0.4

Re: [PATCH v2] OMAP3: I2C: Errata ID i207: Clear wrong RDR interrupt

[Nishanth Menon]

On 04/13/2010 07:01 AM, G, Manjunath Kondaiah wrote:
> Under certain rare conditions, I2C_STAT[13].RDR bit may be set
> and the corresponding interrupt fire, even there is no data in
> the receive FIFO, or the I2C data transfer is still ongoing.
> These spurious RDR events must be ignored by the software.
>
> This patch handles and ignores RDR spurious interrupts.
>
> Patch tested on OMAP zoom3 board.
>
> Signed-off-by: Manjunatha GK<manjugk-l0cyMroinI0@public.gmane.org>
> Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> Cc: Kalliguddi, Hema<hemahk-l0cyMroinI0@public.gmane.org>
> Cc: Nishanth Menon<nm-l0cyMroinI0@public.gmane.org>
> ---
> Review comments for earlier post can be found at:
> https://patchwork.kernel.org/patch/90122/
Overall, the comments have not been implemented, so my NAK continues 
unfortunately. please review the comments again.

>
>   drivers/i2c/busses/i2c-omap.c |   32 +++++++++++++++++++++++++++++++-
>   1 files changed, 31 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ae6f5c1..d4ec886 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -733,10 +733,40 @@ complete:
>   		}
>   		if (stat&  (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
>   			u8 num_bytes = 1;
> +
> +			/*
> +			 * I2C Errata(Errata Nos. OMAP2: 1.67, OMAP3: 1.8)
> +			 * Not applicable for OMAP4.
> +			 * Under certain rare conditions, RDR could be set again
> +			 * when the bus is busy, then ignore the interrupt and
> +			 * clear the interrupt.
> +			 */
> +			if ((stat&  OMAP_I2C_STAT_RDR)&&  !cpu_is_omap44xx()) {
> +				/* Step 1: If RDR is set, clear it */
> +				omap_i2c_ack_stat(dev, stat&  OMAP_I2C_STAT_RDR);
> +
> +				/* Step 2: */
> +				if(!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG)
> +							&  OMAP_I2C_STAT_BB)) {
> +					/* Step 3: */
> +					while(omap_i2c_read_reg(dev,
> +						OMAP_I2C_STAT_REG)
> +							&  OMAP_I2C_STAT_RDR) {
> +						omap_i2c_ack_stat(dev, stat
> +							&  OMAP_I2C_STAT_RDR);
> +						dev_err(dev->dev,
> +						"I2C : RDR when the bus is busy.\n");
> +						continue;
continue in the inner while loop? NAK. should have been an if condition 
here.

> +					}
> +
> +				}
> +				else
> +					return IRQ_HANDLED;
using a continue is better as commented for patch v1.

> +			}
>   			if (dev->fifo_size) {
>   				if (stat&  OMAP_I2C_STAT_RRDY)
>   					num_bytes = dev->fifo_size;
> -				else    /* read RXSTAT on RDR interrupt */
> +				else  /* Step4: read RXSTAT on RDR interrupt */
dont really need this..
>   					num_bytes = (omap_i2c_read_reg(dev,
>   							OMAP_I2C_BUFSTAT_REG)
>   							>>  8)&  0x3F;

Regards,
Nishanth Menon

RE: [PATCH v2] OMAP3: I2C: Errata ID i207: Clear wrong RDR interrupt

[G, Manjunath Kondaiah]

 

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Tuesday, April 13, 2010 10:18 PM
> To: G, Manjunath Kondaiah
> Cc: linux-i2c@vger.kernel.org; linux-omap@vger.kernel.org; 
> ben-linux@fluff.org; Kalliguddi, Hema
> Subject: Re: [PATCH v2] OMAP3: I2C: Errata ID i207: Clear 
> wrong RDR interrupt
> 
> On 04/13/2010 07:01 AM, G, Manjunath Kondaiah wrote:
> > Under certain rare conditions, I2C_STAT[13].RDR bit may be set
> > and the corresponding interrupt fire, even there is no data in
> > the receive FIFO, or the I2C data transfer is still ongoing.
> > These spurious RDR events must be ignored by the software.
> >
> > This patch handles and ignores RDR spurious interrupts.
> >
> > Patch tested on OMAP zoom3 board.
> >
> > Signed-off-by: Manjunatha GK<manjugk@ti.com>
> > Cc: linux-omap@vger.kernel.org
> > Cc: linux-i2c@vger.kernel.org
> > Cc: ben-linux@fluff.org
> > Cc: Kalliguddi, Hema<hemahk@ti.com>
> > Cc: Nishanth Menon<nm@ti.com>
> > ---
> > Review comments for earlier post can be found at:
> > https://patchwork.kernel.org/patch/90122/
> Overall, the comments have not been implemented, so my NAK continues 
> unfortunately. please review the comments again.

This errata is applicable to OMAP2 and OMAP3 and not applicable to
OMAP4. I will update the same in patch description.

Rest of review comments will be taken care as explained below.

> 
> >
> >   drivers/i2c/busses/i2c-omap.c |   32 
> +++++++++++++++++++++++++++++++-
> >   1 files changed, 31 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c 
> b/drivers/i2c/busses/i2c-omap.c
> > index ae6f5c1..d4ec886 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -733,10 +733,40 @@ complete:
> >   		}
> >   		if (stat&  (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
> >   			u8 num_bytes = 1;
> > +
> > +			/*
> > +			 * I2C Errata(Errata Nos. OMAP2: 1.67, 
> OMAP3: 1.8)
> > +			 * Not applicable for OMAP4.
> > +			 * Under certain rare conditions, RDR 
> could be set again
> > +			 * when the bus is busy, then ignore 
> the interrupt and
> > +			 * clear the interrupt.
> > +			 */
> > +			if ((stat&  OMAP_I2C_STAT_RDR)&&  
> !cpu_is_omap44xx()) {
> > +				/* Step 1: If RDR is set, clear it */
> > +				omap_i2c_ack_stat(dev, stat&  
> OMAP_I2C_STAT_RDR);
> > +
> > +				/* Step 2: */
> > +				if(!(omap_i2c_read_reg(dev, 
> OMAP_I2C_STAT_REG)
> > +							&  
> OMAP_I2C_STAT_BB)) {
> > +					/* Step 3: */
> > +					while(omap_i2c_read_reg(dev,
> > +						OMAP_I2C_STAT_REG)
> > +							&  
> OMAP_I2C_STAT_RDR) {
> > +						
> omap_i2c_ack_stat(dev, stat
> > +							&  
> OMAP_I2C_STAT_RDR);
> > +						dev_err(dev->dev,
> > +						"I2C : RDR when 
> the bus is busy.\n");
> > +						continue;
> continue in the inner while loop? NAK. should have been an if 
> condition 
> here.
It's my bad. I will fix it.
> 
> > +					}
> > +
> > +				}
> > +				else
> > +					return IRQ_HANDLED;
> using a continue is better as commented for patch v1.
Agreed.
> 
> > +			}
> >   			if (dev->fifo_size) {
> >   				if (stat&  OMAP_I2C_STAT_RRDY)
> >   					num_bytes = dev->fifo_size;
> > -				else    /* read RXSTAT on RDR 
> interrupt */
> > +				else  /* Step4: read RXSTAT on 
> RDR interrupt */
> dont really need this..

I have changed only comment. I will revert if it is not required.

-Manjunath

Re: [PATCH v2] OMAP3: I2C: Errata ID i207: Clear wrong RDR interrupt

[Tony Lindgren]

* Manjunatha GK <manjugk@ti.com> [100413 06:32]:
> Under certain rare conditions, I2C_STAT[13].RDR bit may be set
> and the corresponding interrupt fire, even there is no data in
> the receive FIFO, or the I2C data transfer is still ongoing.
> These spurious RDR events must be ignored by the software.
> 
> This patch handles and ignores RDR spurious interrupts.
> 
> Patch tested on OMAP zoom3 board.
> 
> Signed-off-by: Manjunatha GK <manjugk@ti.com>
> Cc: linux-omap@vger.kernel.org
> Cc: linux-i2c@vger.kernel.org
> Cc: ben-linux@fluff.org
> Cc: Kalliguddi, Hema <hemahk@ti.com>
> Cc: Nishanth Menon <nm@ti.com>
> ---
> Review comments for earlier post can be found at:
> https://patchwork.kernel.org/patch/90122/
> 
>  drivers/i2c/busses/i2c-omap.c |   32 +++++++++++++++++++++++++++++++-
>  1 files changed, 31 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ae6f5c1..d4ec886 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -733,10 +733,40 @@ complete:
>  		}
>  		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
>  			u8 num_bytes = 1;
> +
> +			/*
> +			 * I2C Errata(Errata Nos. OMAP2: 1.67, OMAP3: 1.8)
> +			 * Not applicable for OMAP4.
> +			 * Under certain rare conditions, RDR could be set again
> +			 * when the bus is busy, then ignore the interrupt and
> +			 * clear the interrupt.
> +			 */
> +			if ((stat & OMAP_I2C_STAT_RDR) && !cpu_is_omap44xx()) {
> +				/* Step 1: If RDR is set, clear it */
> +				omap_i2c_ack_stat(dev, stat & OMAP_I2C_STAT_RDR);
> +
> +				/* Step 2: */
> +				if(!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG)
> +							& OMAP_I2C_STAT_BB)) {
> +					/* Step 3: */
> +					while(omap_i2c_read_reg(dev,
> +						OMAP_I2C_STAT_REG)
> +							& OMAP_I2C_STAT_RDR) {
> +						omap_i2c_ack_stat(dev, stat
> +							& OMAP_I2C_STAT_RDR);
> +						dev_err(dev->dev,
> +						"I2C : RDR when the bus is busy.\n");
> +						continue;
> +					}
> +
> +				}
> +				else
> +					return IRQ_HANDLED;
> +			}

Please move these hacks into a separate inline function that you initialize
during the init. Then in the function you do it if OMAP_I2C_QUIRK_1234 flag
has been set.

In general, it seems that a lot of TI omap4 patches just do:

if (!cpu_is_omap4430()) {
	indent_old_code_even_more
	...
} else {
	add_hacks_for_omap4
	...
}

And sprinkle that all over the place.

This is not a maintainable way of doing things. Please do something
like this instead:

#ifdef CONFIG_ARCH_OMAP4
static inline void omap_i2c_quirk_1234(struct device *dev)
{
	/* Do hacks here */	
}
#else
static inline void omap_i2c_quirk_1234(struct device *dev)
{
}
#endif

int omap_i2_some_function(struct device *dev)
{
	...
	if (dev->quirks & OMAP_I2C_QUIRK_1234)
		omap_i2c_quirk_1234(dev);
	...
}

And then in the driver init just set the quirks you need.

Regards,

Tony

RE: [PATCH v2] OMAP3: I2C: Errata ID i207: Clear wrong RDR interrupt

[G, Manjunath Kondaiah]

Tony, 

> -----Original Message-----
> From: Tony Lindgren [mailto:tony@atomide.com] 
> Sent: Thursday, April 22, 2010 3:13 AM
> To: G, Manjunath Kondaiah
> Cc: linux-i2c@vger.kernel.org; linux-omap@vger.kernel.org; 
> ben-linux@fluff.org; Kalliguddi, Hema; Menon, Nishanth
> Subject: Re: [PATCH v2] OMAP3: I2C: Errata ID i207: Clear 
> wrong RDR interrupt
> 
> * Manjunatha GK <manjugk@ti.com> [100413 06:32]:
> > Under certain rare conditions, I2C_STAT[13].RDR bit may be set
> > and the corresponding interrupt fire, even there is no data in
> > the receive FIFO, or the I2C data transfer is still ongoing.
> > These spurious RDR events must be ignored by the software.
> > 
> > This patch handles and ignores RDR spurious interrupts.
> > 
> > Patch tested on OMAP zoom3 board.
> > 
> > Signed-off-by: Manjunatha GK <manjugk@ti.com>
> > Cc: linux-omap@vger.kernel.org
> > Cc: linux-i2c@vger.kernel.org
> > Cc: ben-linux@fluff.org
> > Cc: Kalliguddi, Hema <hemahk@ti.com>
> > Cc: Nishanth Menon <nm@ti.com>
> > ---
> > Review comments for earlier post can be found at:
> > https://patchwork.kernel.org/patch/90122/
> > 
> >  drivers/i2c/busses/i2c-omap.c |   32 
> +++++++++++++++++++++++++++++++-
> >  1 files changed, 31 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-omap.c 
> b/drivers/i2c/busses/i2c-omap.c
> > index ae6f5c1..d4ec886 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -733,10 +733,40 @@ complete:
> >  		}
> >  		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
> >  			u8 num_bytes = 1;
> > +
> > +			/*
> > +			 * I2C Errata(Errata Nos. OMAP2: 1.67, 
> OMAP3: 1.8)
> > +			 * Not applicable for OMAP4.
> > +			 * Under certain rare conditions, RDR 
> could be set again
> > +			 * when the bus is busy, then ignore 
> the interrupt and
> > +			 * clear the interrupt.
> > +			 */
> > +			if ((stat & OMAP_I2C_STAT_RDR) && 
> !cpu_is_omap44xx()) {
> > +				/* Step 1: If RDR is set, clear it */
> > +				omap_i2c_ack_stat(dev, stat & 
> OMAP_I2C_STAT_RDR);
> > +
> > +				/* Step 2: */
> > +				if(!(omap_i2c_read_reg(dev, 
> OMAP_I2C_STAT_REG)
> > +							& 
> OMAP_I2C_STAT_BB)) {
> > +					/* Step 3: */
> > +					while(omap_i2c_read_reg(dev,
> > +						OMAP_I2C_STAT_REG)
> > +							& 
> OMAP_I2C_STAT_RDR) {
> > +						
> omap_i2c_ack_stat(dev, stat
> > +							& 
> OMAP_I2C_STAT_RDR);
> > +						dev_err(dev->dev,
> > +						"I2C : RDR when 
> the bus is busy.\n");
> > +						continue;
> > +					}
> > +
> > +				}
> > +				else
> > +					return IRQ_HANDLED;
> > +			}
> 
> Please move these hacks into a separate inline function that 
> you initialize
> during the init. Then in the function you do it if 
> OMAP_I2C_QUIRK_1234 flag
> has been set.
> 
> In general, it seems that a lot of TI omap4 patches just do:
> 
> if (!cpu_is_omap4430()) {
> 	indent_old_code_even_more
> 	...
> } else {
> 	add_hacks_for_omap4
> 	...
> }
> 
> And sprinkle that all over the place.
> 
> This is not a maintainable way of doing things. Please do something
> like this instead:
> 
> #ifdef CONFIG_ARCH_OMAP4
> static inline void omap_i2c_quirk_1234(struct device *dev)
> {
> 	/* Do hacks here */	
> }
> #else
> static inline void omap_i2c_quirk_1234(struct device *dev)
> {
> }
> #endif

This might create issue for multi omap build?

-Manjunath