[PATCH v5] OMAP2/3: I2C: Errata ID i207: Clear wrong RDR interrupt

[PATCH v5] OMAP2/3: 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.

The below sequence is required in interrupt handler for
handling this errata:
1. If RDR is set to 1, clear RDR
2. Read I2C status register and check for BusBusy bit. If BusBusy
bit is set, skip remaining steps.
3. If BusBusy bit is not set, perform read operation on I2C status
register.
4. If RDR is set, clear the same. Check RDR again and clear if it sets
RDR bit again.
5. Perform I2C Data Read operation N number of times(where N is value
read from the register BUFSTAT-RXSTAT bit fields).

Note : This errata is applicable for OMAP2 and OMAP3 platforms only.
It is not applicable for OMAP4.

Signed-off-by: Manjunatha GK <manjugk-l0cyMroinI0@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>
Cc: Aaro Koskinen <Aaro.Koskinen-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ef73483..f232f96 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -199,6 +199,7 @@ struct omap_i2c_dev {
 	u16			bufstate;
 	u16			syscstate;
 	u16			westate;
+	bool			errata_i207;
 };
 
 const static u8 reg_map[] = {
@@ -498,6 +499,12 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
+	dev->errata_i207 = false;
+
+	if (cpu_is_omap2430() || cpu_is_omap34xx()) {
+		dev->errata_i207 = true;
+	}
+
 	/* Enable interrupts */
 	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
 			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
@@ -695,6 +702,34 @@ omap_i2c_ack_stat(struct omap_i2c_dev *dev, u16 stat)
 	omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 }
 
+static inline void i2c_omap_errata_i207(struct omap_i2c_dev *dev, u16 stat)
+{
+	/*
+	 * 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) {
+		/* Step 1: If RDR is set, clear it */
+		omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
+
+		/* Step 2: */
+		if(!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG)
+						& OMAP_I2C_STAT_BB)) {
+
+			/* Step 3: */
+			if(omap_i2c_read_reg(dev,OMAP_I2C_STAT_REG)
+						& OMAP_I2C_STAT_RDR) {
+				omap_i2c_ack_stat(dev,OMAP_I2C_STAT_RDR);
+				dev_dbg(dev->dev,"RDR when the bus is busy.\n");
+			}
+
+		}
+	}
+}
+
 /* rev1 devices are apparently only on some 15xx */
 #ifdef CONFIG_ARCH_OMAP15XX
 
@@ -826,6 +861,10 @@ complete:
 		}
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
+
+			if (dev->errata_i207)
+				i2c_omap_errata_i207(dev, stat);
+
 			if (dev->fifo_size) {
 				if (stat & OMAP_I2C_STAT_RRDY)
 					num_bytes = dev->fifo_size;
-- 
1.6.0.4

Re: [PATCH v5] OMAP2/3: I2C: Errata ID i207: Clear wrong RDR interrupt

[Tony Lindgren]

* Manjunatha GK <manjugk@ti.com> [100427 07:21]:
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -199,6 +199,7 @@ struct omap_i2c_dev {
>  	u16			bufstate;
>  	u16			syscstate;
>  	u16			westate;
> +	bool			errata_i207;
>  };

Thanks for updating this.

One more comment though to make this more generic for handling
the other errata for i2c-omap.

Can you please make errata_i207 to be just:

	u16			errata;

And then we can use it for handing all the i2c-omap errata.

You also need something like

#define I2C_OMAP_ERRATA_I207	(1 << 0)
  
>  const static u8 reg_map[] = {
> @@ -498,6 +499,12 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	/* Take the I2C module out of reset: */
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  
> +	dev->errata_i207 = false;
> +
> +	if (cpu_is_omap2430() || cpu_is_omap34xx()) {
> +		dev->errata_i207 = true;
> +	}
> +
>  	/* Enable interrupts */
>  	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
>  			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |

Then update this for for the bitmask too

	dev->errata = 0;

	if (cpu_is_omap2430() || cpu_is_omap34xx())
		dev->errata |= I2C_OMAP_ERRATA_I207;


> @@ -695,6 +702,34 @@ omap_i2c_ack_stat(struct omap_i2c_dev *dev, u16 stat)
>  	omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
>  }
>  
> +static inline void i2c_omap_errata_i207(struct omap_i2c_dev *dev, u16 stat)
> +{
> +	/*
> +	 * 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) {
> +		/* Step 1: If RDR is set, clear it */
> +		omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> +
> +		/* Step 2: */
> +		if(!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG)

Here you should have a space after if (

> +						& OMAP_I2C_STAT_BB)) {
> +
> +			/* Step 3: */
> +			if(omap_i2c_read_reg(dev,OMAP_I2C_STAT_REG)
> +						& OMAP_I2C_STAT_RDR) {
> +				omap_i2c_ack_stat(dev,OMAP_I2C_STAT_RDR);
> +				dev_dbg(dev->dev,"RDR when the bus is busy.\n");
> +			}
> +
> +		}
> +	}
> +}
> +
>  /* rev1 devices are apparently only on some 15xx */
>  #ifdef CONFIG_ARCH_OMAP15XX
>  
> @@ -826,6 +861,10 @@ complete:
>  		}
>  		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
>  			u8 num_bytes = 1;
> +
> +			if (dev->errata_i207)
> +				i2c_omap_errata_i207(dev, stat);
> +
>  			if (dev->fifo_size) {
>  				if (stat & OMAP_I2C_STAT_RRDY)
>  					num_bytes = dev->fifo_size;
> -- 

And here you would need to test for the bitmask instead.

Regards,

Tony

RE: [PATCH v5] OMAP2/3: I2C: Errata ID i207: Clear wrong RDR interrupt

[G, Manjunath Kondaiah]

Tony,
Thanks for the inputs and suggestions for handling i2c-omap errata's in
generic way.


> -----Original Message-----
> From: Tony Lindgren [mailto:tony@atomide.com] 
> Sent: Tuesday, April 27, 2010 8:31 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap@vger.kernel.org; linux-i2c@vger.kernel.org; 
> ben-linux@fluff.org; Kalliguddi, Hema; Menon, Nishanth; Aaro Koskinen
> Subject: Re: [PATCH v5] OMAP2/3: I2C: Errata ID i207: Clear 
> wrong RDR interrupt
> 
> * Manjunatha GK <manjugk@ti.com> [100427 07:21]:
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -199,6 +199,7 @@ struct omap_i2c_dev {
> >  	u16			bufstate;
> >  	u16			syscstate;
> >  	u16			westate;
> > +	bool			errata_i207;
> >  };
> 
> Thanks for updating this.
> 
> One more comment though to make this more generic for handling
> the other errata for i2c-omap.
> 
> Can you please make errata_i207 to be just:
> 
> 	u16			errata;
> 
> And then we can use it for handing all the i2c-omap errata.
> 
> You also need something like
> 
> #define I2C_OMAP_ERRATA_I207	(1 << 0)

Agreed. 

>   
> >  const static u8 reg_map[] = {
> > @@ -498,6 +499,12 @@ static int omap_i2c_init(struct 
> omap_i2c_dev *dev)
> >  	/* Take the I2C module out of reset: */
> >  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> >  
> > +	dev->errata_i207 = false;
> > +
> > +	if (cpu_is_omap2430() || cpu_is_omap34xx()) {
> > +		dev->errata_i207 = true;
> > +	}
> > +
> >  	/* Enable interrupts */
> >  	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
> >  			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> 
> Then update this for for the bitmask too
> 
> 	dev->errata = 0;
> 
> 	if (cpu_is_omap2430() || cpu_is_omap34xx())
> 		dev->errata |= I2C_OMAP_ERRATA_I207;
> 
Ok. I will incorporate these changes in next version.

-Manjunath

Re: [PATCH v5] OMAP2/3: I2C: Errata ID i207: Clear wrong RDR interrupt

[Nishanth Menon]

G, Manjunath Kondaiah had written, on 04/27/2010 09:47 AM, the following:
> 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.
> 
> The below sequence is required in interrupt handler for
> handling this errata:
> 1. If RDR is set to 1, clear RDR
> 2. Read I2C status register and check for BusBusy bit. If BusBusy
> bit is set, skip remaining steps.
> 3. If BusBusy bit is not set, perform read operation on I2C status
> register.
> 4. If RDR is set, clear the same. Check RDR again and clear if it sets
> RDR bit again.
> 5. Perform I2C Data Read operation N number of times(where N is value
> read from the register BUFSTAT-RXSTAT bit fields).
> 
> Note : This errata is applicable for OMAP2 and OMAP3 platforms only.
> It is not applicable for OMAP4.
> 
> Signed-off-by: Manjunatha GK <manjugk-l0cyMroinI0@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>
> Cc: Aaro Koskinen <Aaro.Koskinen-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ef73483..f232f96 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -199,6 +199,7 @@ struct omap_i2c_dev {
>  	u16			bufstate;
>  	u16			syscstate;
>  	u16			westate;
> +	bool			errata_i207;
Just my 2cents:

Could we use a u32 quirk? this will allow multiple similar quirks 
(already existing in the driver to use this mechanism)

and
#define OMAP_I2C_QUIRK_i207	0x1
/* we can add more quirks here and cleanup the i2c driver quiet a bit */

#define IS_OMAP_I2C_QUIRK(d,q) ((d)->q & quirk)

>  };
>  
>  const static u8 reg_map[] = {
> @@ -498,6 +499,12 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	/* Take the I2C module out of reset: */
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  
> +	dev->errata_i207 = false;
> +
> +	if (cpu_is_omap2430() || cpu_is_omap34xx()) {
> +		dev->errata_i207 = true;
> +	}
oneliner with {}? did you run checkpatch.pl?
> +
>  	/* Enable interrupts */
>  	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
>  			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> @@ -695,6 +702,34 @@ omap_i2c_ack_stat(struct omap_i2c_dev *dev, u16 stat)
>  	omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
>  }
>  
> +static inline void i2c_omap_errata_i207(struct omap_i2c_dev *dev, u16 stat)
> +{
> +	/*
> +	 * 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) {
> +		/* Step 1: If RDR is set, clear it */
> +		omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> +
> +		/* Step 2: */
> +		if(!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG)
> +						& OMAP_I2C_STAT_BB)) {
> +
> +			/* Step 3: */
> +			if(omap_i2c_read_reg(dev,OMAP_I2C_STAT_REG)
> +						& OMAP_I2C_STAT_RDR) {
> +				omap_i2c_ack_stat(dev,OMAP_I2C_STAT_RDR);
> +				dev_dbg(dev->dev,"RDR when the bus is busy.\n");
> +			}
> +
> +		}
> +	}
> +}
> +
>  /* rev1 devices are apparently only on some 15xx */
>  #ifdef CONFIG_ARCH_OMAP15XX
>  
> @@ -826,6 +861,10 @@ complete:
>  		}
>  		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
>  			u8 num_bytes = 1;
> +
> +			if (dev->errata_i207)
> +				i2c_omap_errata_i207(dev, stat);
> +
>  			if (dev->fifo_size) {
>  				if (stat & OMAP_I2C_STAT_RRDY)
>  					num_bytes = dev->fifo_size;
looks good overall.. thanks..

-- 
Regards,
Nishanth Menon