[PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.

Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.

Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
Signed-off-by: Vikram Pandita<vikram.pandita@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			break;
 		}
 
+		err = 0;
+complete:
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 
-		err = 0;
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
 							"data to send\n");
 					break;
 				}
+
+				/*
+				 * OMAP3430 Errata 1.153: When an XRDY/XDR
+				 * is hit, wait for XUDF before writing data
+				 * to DATA_REG. Otherwise some data bytes can
+				 * be lost while transferring them from the
+				 * memory to the I2C interface.
+				 */
+
+				if (cpu_is_omap34xx()) {
+						while (!(stat & OMAP_I2C_STAT_XUDF)) {
+							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
+								err |= OMAP_I2C_STAT_XUDF;
+								goto complete;
+							}
+							cpu_relax();
+							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+						}
+				}
+
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
 			omap_i2c_ack_stat(dev,
-- 
1.5.6.3

Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Nishanth Menon]

Sonasath, Moiz wrote:
> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> Otherwise some data bytes can be lost while transferring them from the
> memory to the I2C interface.
> 
> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
> if there is NACK | AL, set the appropriate error flags, ack the pending
> interrupts and return from the ISR.
> 
> Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
> Signed-off-by: Vikram Pandita<vikram.pandita@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 05b5e4c..8deaf87 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  			break;
>  		}
>  
> +		err = 0;
> +complete:
cant we rename this label?
>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
>  
> -		err = 0;
>  		if (stat & OMAP_I2C_STAT_NACK) {
>  			err |= OMAP_I2C_STAT_NACK;
>  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  							"data to send\n");
>  					break;
>  				}
> +
> +				/*
> +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> +				 * is hit, wait for XUDF before writing data
> +				 * to DATA_REG. Otherwise some data bytes can
> +				 * be lost while transferring them from the
> +				 * memory to the I2C interface.
> +				 */
> +
> +				if (cpu_is_omap34xx()) {
> +						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> +							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> +								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
generic comment - code nesting is getting overwhelming - we may like to 
refactor the isr function at a later date I think..
> +								err |= OMAP_I2C_STAT_XUDF;
why set the err if we wantedly force this to happen?
> +								goto complete;

> +							}
> +							cpu_relax();
> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +						}
this is an infinite while loop + it tries to handle error cases - 
essentially do another isr routine inside itself.


How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
                 }
                 if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
                         u8 num_bytes = 1;
+                       if (cpu_is_omap34xx() &&
+                               !(stat & OMAP_I2C_STAT_XUDF)){
+                               cpu_relax();
+                               continue;
+                       }
+
                         if (dev->fifo_size) {
                                 if (stat & OMAP_I2C_STAT_XRDY)
                                         num_bytes = dev->fifo_size;


Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Hello Nishant,

Comments inlined:

Regards
Moiz Sonasath
Linux Baseport Team, Dallas
214-567-5514


-----Original Message-----
From: Menon, Nishanth 
Sent: Tuesday, July 14, 2009 6:23 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

Sonasath, Moiz wrote:
> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> Otherwise some data bytes can be lost while transferring them from the
> memory to the I2C interface.
> 
> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
> if there is NACK | AL, set the appropriate error flags, ack the pending
> interrupts and return from the ISR.
> 
> Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
> Signed-off-by: Vikram Pandita<vikram.pandita@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 05b5e4c..8deaf87 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  			break;
>  		}
>  
> +		err = 0;
> +complete:
cant we rename this label?
[Moiz]
This path actually completes the IRQ service routine and therefore the name. 

>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
>  
> -		err = 0;
>  		if (stat & OMAP_I2C_STAT_NACK) {
>  			err |= OMAP_I2C_STAT_NACK;
>  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  							"data to send\n");
>  					break;
>  				}
> +
> +				/*
> +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> +				 * is hit, wait for XUDF before writing data
> +				 * to DATA_REG. Otherwise some data bytes can
> +				 * be lost while transferring them from the
> +				 * memory to the I2C interface.
> +				 */
> +
> +				if (cpu_is_omap34xx()) {
> +						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> +							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> +								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
generic comment - code nesting is getting overwhelming - we may like to 
refactor the isr function at a later date I think..
> +								err |= OMAP_I2C_STAT_XUDF;
why set the err if we wantedly force this to happen?
[Moiz]
The idea here is to let the upper application to know that something went wrong while waiting for the XUDF bit to be set. This is just for debugging purpose.
> +								goto complete;

> +							}
> +							cpu_relax();
> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +						}
this is an infinite while loop + it tries to handle error cases - 
essentially do another isr routine inside itself.
[Moiz]
Yes, it is a busy wait loop. But AFAIK while we come to this part of the code the only thing that can go wrong in the transfer is either the device would go off suddenly (creating a NACK) or there is an arbitration lost(AL). This loop takes care of these two error conditions. Apart from these, if the hardware is functional, the XUDF bit should be set as soon as the FIFO gets empty. 

Correct me if I am missing something.

How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
                 }
                 if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
                         u8 num_bytes = 1;
+                       if (cpu_is_omap34xx() &&
+                               !(stat & OMAP_I2C_STAT_XUDF)){
+                               cpu_relax();
+                               continue;
+                       }
+
                         if (dev->fifo_size) {
                                 if (stat & OMAP_I2C_STAT_XRDY)
                                         num_bytes = dev->fifo_size;


Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/

Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Nishanth Menon]

Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
>> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
>> Otherwise some data bytes can be lost while transferring them from the
>> memory to the I2C interface.
>>
>> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
>> if there is NACK | AL, set the appropriate error flags, ack the pending
>> +							}
>> +							cpu_relax();
>> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>> +						}
> this is an infinite while loop + it tries to handle error cases - 
> essentially do another isr routine inside itself.
> [Moiz]
> Yes, it is a busy wait loop. But AFAIK while we come to this part of
 > the code the only thing that can go wrong in the transfer is either
 > the device would go off suddenly (creating a NACK) or there is an
 > arbitration lost(AL). This loop takes care of these two error conditions.
 > Apart from these, if the hardware is functional, the XUDF bit should
 > be set as soon as the FIFO gets empty.
> 
> Correct me if I am missing something.
That is exactly what I was complaining about -> why not use the existing 
logic above in the code to take care of it as I indicated below? do you 
see a problem with the logic I send? it is lesser code and should IMHO 
take care of the same issue without the additional 3rd nested while loop
> 
> How about:
> Apply [1] and the following?
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ad8d201..e3f21af 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
>                  }
>                  if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
>                          u8 num_bytes = 1;
> +                       if (cpu_is_omap34xx() &&
> +                               !(stat & OMAP_I2C_STAT_XUDF)){
> +                               cpu_relax();
> +                               continue;
> +                       }
> +
>                          if (dev->fifo_size) {
>                                  if (stat & OMAP_I2C_STAT_XRDY)
>                                          num_bytes = dev->fifo_size;
> 
> 
> Regards,
> Nishanth Menon
> Ref:
> [1] http://patchwork.kernel.org/patch/32332/

-- 
Regards,
Nishanth Menon

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Hello Nishant,

Comments inlined,

Regards
Moiz Sonasath


-----Original Message-----
From: Menon, Nishanth 
Sent: Wednesday, July 15, 2009 4:43 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
>> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
>> Otherwise some data bytes can be lost while transferring them from the
>> memory to the I2C interface.
>>
>> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
>> if there is NACK | AL, set the appropriate error flags, ack the pending
>> +							}
>> +							cpu_relax();
>> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>> +						}
> this is an infinite while loop + it tries to handle error cases - 
> essentially do another isr routine inside itself.
> [Moiz]
> Yes, it is a busy wait loop. But AFAIK while we come to this part of
 > the code the only thing that can go wrong in the transfer is either
 > the device would go off suddenly (creating a NACK) or there is an
 > arbitration lost(AL). This loop takes care of these two error conditions.
 > Apart from these, if the hardware is functional, the XUDF bit should
 > be set as soon as the FIFO gets empty.
> 
> Correct me if I am missing something.
That is exactly what I was complaining about -> why not use the existing 
logic above in the code to take care of it as I indicated below? do you 
see a problem with the logic I send? it is lesser code and should IMHO 
take care of the same issue without the additional 3rd nested while loop
> 
> How about:
> Apply [1] and the following?
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ad8d201..e3f21af 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
>                  }
>                  if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
>                          u8 num_bytes = 1;
> +                       if (cpu_is_omap34xx() &&
> +                               !(stat & OMAP_I2C_STAT_XUDF)){
> +                               cpu_relax();
> +                               continue;
> +                       }
> +

[Moiz]

In this alternate implementation if a NACK|AL is produced while waiting on the XUDF, it returns from the ISR (as per my patch [2/3]) without ACKING the XRDY/XDR interrupts which would make it return to the ISR again and again which looks like a problem. To accommodate this implementation, we will have to ACK XRDY/XDR before returning from the ISR.
		

>                          if (dev->fifo_size) {
>                                  if (stat & OMAP_I2C_STAT_XRDY)
>                                          num_bytes = dev->fifo_size;
> 
> 
> Regards,
> Nishanth Menon
> Ref:
> [1] http://patchwork.kernel.org/patch/32332/

-- 
Regards,
Nishanth Menon

Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Nishanth Menon]

Sonasath, Moiz had written, on 07/15/2009 05:23 PM, the following:
> Hello Nishant,
> 
> Comments inlined,
> 
> Regards
> Moiz Sonasath
> 
> 
> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Wednesday, July 15, 2009 4:43 PM
> To: Sonasath, Moiz
> Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
> Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> 
> Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
>>> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
>>> Otherwise some data bytes can be lost while transferring them from the
>>> memory to the I2C interface.
>>>
>>> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
>>> if there is NACK | AL, set the appropriate error flags, ack the pending
>>> +							}
>>> +							cpu_relax();
>>> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>>> +						}
>> this is an infinite while loop + it tries to handle error cases - 
>> essentially do another isr routine inside itself.
>> [Moiz]
>> Yes, it is a busy wait loop. But AFAIK while we come to this part of
>  > the code the only thing that can go wrong in the transfer is either
>  > the device would go off suddenly (creating a NACK) or there is an
>  > arbitration lost(AL). This loop takes care of these two error conditions.
>  > Apart from these, if the hardware is functional, the XUDF bit should
>  > be set as soon as the FIFO gets empty.
>> Correct me if I am missing something.
> That is exactly what I was complaining about -> why not use the existing 
> logic above in the code to take care of it as I indicated below? do you 
> see a problem with the logic I send? it is lesser code and should IMHO 
> take care of the same issue without the additional 3rd nested while loop
>> How about:
>> Apply [1] and the following?
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index ad8d201..e3f21af 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
>>                  }
>>                  if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
>>                          u8 num_bytes = 1;
>> +                       if (cpu_is_omap34xx() &&
>> +                               !(stat & OMAP_I2C_STAT_XUDF)){
>> +                               cpu_relax();
>> +                               continue;
>> +                       }
>> +
> 
> [Moiz]
> 
> In this alternate implementation if a NACK|AL is produced while waiting
 > on the XUDF, it returns from the ISR (as per my patch [2/3])
 > without ACKING the XRDY/XDR interrupts which would make it return
 > to the ISR again and again which looks like a problem. To accommodate
 >this implementation, we will have to ACK XRDY/XDR before returning 
from the ISR.
ok, this is due to my [1] patch which should have handled this condition 
in the first place. I will rework my original [1] patch and resend it.

Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Hello Nishant!

I am also not sure, if the count=100; value will be enough time for the XUDF to be set. If not then it will keep running into timeout errors.

Regards
Moiz Sonasath


-----Original Message-----
From: Menon, Nishanth 
Sent: Wednesday, July 15, 2009 5:27 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

Sonasath, Moiz had written, on 07/15/2009 05:23 PM, the following:
> Hello Nishant,
> 
> Comments inlined,
> 
> Regards
> Moiz Sonasath
> 
> 
> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Wednesday, July 15, 2009 4:43 PM
> To: Sonasath, Moiz
> Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
> Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> 
> Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
>>> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
>>> Otherwise some data bytes can be lost while transferring them from the
>>> memory to the I2C interface.
>>>
>>> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
>>> if there is NACK | AL, set the appropriate error flags, ack the pending
>>> +							}
>>> +							cpu_relax();
>>> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>>> +						}
>> this is an infinite while loop + it tries to handle error cases - 
>> essentially do another isr routine inside itself.
>> [Moiz]
>> Yes, it is a busy wait loop. But AFAIK while we come to this part of
>  > the code the only thing that can go wrong in the transfer is either
>  > the device would go off suddenly (creating a NACK) or there is an
>  > arbitration lost(AL). This loop takes care of these two error conditions.
>  > Apart from these, if the hardware is functional, the XUDF bit should
>  > be set as soon as the FIFO gets empty.
>> Correct me if I am missing something.
> That is exactly what I was complaining about -> why not use the existing 
> logic above in the code to take care of it as I indicated below? do you 
> see a problem with the logic I send? it is lesser code and should IMHO 
> take care of the same issue without the additional 3rd nested while loop
>> How about:
>> Apply [1] and the following?
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index ad8d201..e3f21af 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
>>                  }
>>                  if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
>>                          u8 num_bytes = 1;
>> +                       if (cpu_is_omap34xx() &&
>> +                               !(stat & OMAP_I2C_STAT_XUDF)){
>> +                               cpu_relax();
>> +                               continue;
>> +                       }
>> +
> 
> [Moiz]
> 
> In this alternate implementation if a NACK|AL is produced while waiting
 > on the XUDF, it returns from the ISR (as per my patch [2/3])
 > without ACKING the XRDY/XDR interrupts which would make it return
 > to the ISR again and again which looks like a problem. To accommodate
 >this implementation, we will have to ACK XRDY/XDR before returning 
from the ISR.
ok, this is due to my [1] patch which should have handled this condition 
in the first place. I will rework my original [1] patch and resend it.

Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/

Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Nishanth Menon]

Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following:
> 
> I am also not sure, if the count=100; value will be enough time for the XUDF
 > to be set. If not then it will keep running into timeout errors.
> 
Do you mean we need a delay for checking again? that should be easy to 
incorporate - what kind of delay are we speaking of here? do you have a 
count requirement for handling this? it is essentially the time b/w XRDY 
to XUNDF.. this should be deterministic rt?

Regards,
Nishanth Menon

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Nishant,

AFAIK, the time between XRDY/XDR and setting of XUDF bit is not deterministic, it depends on the I2C bus speed but I am not sure if we can translate the speed into a fixed count number which we can use as a timeout limit. In that case we need to make a balanced assumption of the count value so that we don't fall in the timeout case under normal operation.

May be someone can give a pointer here.

Regards
Moiz Sonasath


-----Original Message-----
From: Menon, Nishanth 
Sent: Wednesday, July 15, 2009 5:32 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following:
> 
> I am also not sure, if the count=100; value will be enough time for the XUDF
 > to be set. If not then it will keep running into timeout errors.
> 
Do you mean we need a delay for checking again? that should be easy to 
incorporate - what kind of delay are we speaking of here? do you have a 
count requirement for handling this? it is essentially the time b/w XRDY 
to XUNDF.. this should be deterministic rt?

Regards,
Nishanth Menon

Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Nishanth Menon]

Sonasath, Moiz had written, on 07/15/2009 05:37 PM, the following:
 >
 > -----Original Message-----
 > From: Menon, Nishanth
 > Sent: Wednesday, July 15, 2009 5:32 PM

please stop top posting..

 >> Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following:
 >> I am also not sure, if the count=100; value will be enough time for 
the XUDF
 >>  > to be set. If not then it will keep running into timeout errors.
 >> Do you mean we need a delay for checking again? that should be easy to
 >> incorporate - what kind of delay are we speaking of here? do you 
have a
 >> count requirement for handling this? it is essentially the time b/w 
XRDY
 >> to XUNDF.. this should be deterministic rt?
 >>
 >
 > AFAIK, the time between XRDY/XDR and setting of XUDF bit is not
 >deterministic, it depends on the I2C bus speed but I am not sure if we
 >can translate the speed into a fixed count number which we can use as a
 > timeout limit. In that case we need to make a balanced assumption of
 >the count value so that we don't fall in the timeout case under normal
 >operation.
 >
 > May be someone can give a pointer here.
 >
Here is my attempt at this:
XRDY -> in this case the FIFO is completely empty - fill it up for XTRSH
XDR -> FIFO is not completely empty, fill as per TXSTAT reg.
if you look at the sequences of events that should happen ->
a) no previous data transmitted: XRDY, XUNDF
b) previous data available, XDR, XRDY, XUNDF
The variables are:
i) bus speed
ii) num bytes to empty (if XRDY then 0, else TXSTAT)

Time = (num_bytes+c) * (1/bus_speed)
where c = some constant time interval for XRDY->XUNDF.. I think..

One possible strategy:
a)Worst case is XTRSH, so, Compute time prior to entry into transmit
loop. Set the count as this+ add a constant for additional events
b) Add a constant delay when you decide to continue back - not sure if
cpu_relax is predictable delay..

--
Regards,
Nishanth Menon

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Hello Nishant,

Comments inlined,

Regards
Moiz Sonasath


-----Original Message-----
From: Menon, Nishanth 
Sent: Wednesday, July 15, 2009 6:03 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

Sonasath, Moiz had written, on 07/15/2009 05:37 PM, the following:
 >
 > -----Original Message-----
 > From: Menon, Nishanth
 > Sent: Wednesday, July 15, 2009 5:32 PM

please stop top posting..

 >> Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following:
 >> I am also not sure, if the count=100; value will be enough time for 
the XUDF
 >>  > to be set. If not then it will keep running into timeout errors.
 >> Do you mean we need a delay for checking again? that should be easy to
 >> incorporate - what kind of delay are we speaking of here? do you 
have a
 >> count requirement for handling this? it is essentially the time b/w 
XRDY
 >> to XUNDF.. this should be deterministic rt?
 >>
 >
 > AFAIK, the time between XRDY/XDR and setting of XUDF bit is not
 >deterministic, it depends on the I2C bus speed but I am not sure if we
 >can translate the speed into a fixed count number which we can use as a
 > timeout limit. In that case we need to make a balanced assumption of
 >the count value so that we don't fall in the timeout case under normal
 >operation.
 >
 > May be someone can give a pointer here.
 >
Here is my attempt at this:
XRDY -> in this case the FIFO is completely empty - fill it up for XTRSH
XDR -> FIFO is not completely empty, fill as per TXSTAT reg.
if you look at the sequences of events that should happen ->
a) no previous data transmitted: XRDY, XUNDF
b) previous data available, XDR, XRDY, XUNDF
The variables are:
i) bus speed
ii) num bytes to empty (if XRDY then 0, else TXSTAT)

Time = (num_bytes+c) * (1/bus_speed)
where c = some constant time interval for XRDY->XUNDF.. I think..

One possible strategy:
a)Worst case is XTRSH, so, Compute time prior to entry into transmit
loop. Set the count as this+ add a constant for additional events
b) Add a constant delay when you decide to continue back - not sure if
cpu_relax is predictable delay..


[Moiz]

Let me just rephrase what you said and from what I see in the TRM,

XRDY-> when TX FIFO level is equal/below XTRSH and TXSTAT is equal/greater than XTRSH, LH will write number of data bytes specified by XTRSH
XDR-> when TX FIFO level is equal/below the XTRSH and TXSTAT is less than XTRSH, LH will write number of data bytes specified by TXSTAT
XUDF-> when shift register and the TX FIFO are empty and there are still some bytes to transmit (DCOUNT not 0)

Here we are looking at the wait time between XRDY/XDR and XUDF. Considering worst case scenarios, 

Wait_time = (time to transmit out XTRSH bytes from the FIFO at the slowest bus speed) + (safeguard delay)

The slowest bus speed appears in standard mode (100K bits/sec). Therefore,

Wait_time = [(XTRSH*8)*(1/100K)] seconds + constant;

Does this look correct?

 

--
Regards,
Nishanth Menon

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

> Sonasath, Moiz wrote:
> > When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> > Otherwise some data bytes can be lost while transferring them from the
> > memory to the I2C interface.
> >
> > Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
> > if there is NACK | AL, set the appropriate error flags, ack the pending
> > interrupts and return from the ISR.
> >
> > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
> > Signed-off-by: Vikram Pandita<vikram.pandita@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
> >  1 files changed, 23 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
> omap.c
> > index 05b5e4c..8deaf87 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >  			break;
> >  		}
> >
> > +		err = 0;
> > +complete:
> cant we rename this label?
> [Moiz]
> This path actually completes the IRQ service routine and therefore the
> name.
> 
> >  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
> >
> > -		err = 0;
> >  		if (stat & OMAP_I2C_STAT_NACK) {
> >  			err |= OMAP_I2C_STAT_NACK;
> >  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> > @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >  							"data to send\n");
> >  					break;
> >  				}
> > +
> > +				/*
> > +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> > +				 * is hit, wait for XUDF before writing data
> > +				 * to DATA_REG. Otherwise some data bytes can
> > +				 * be lost while transferring them from the
> > +				 * memory to the I2C interface.
> > +				 */
> > +
> > +				if (cpu_is_omap34xx()) {
> > +						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> > +							if (stat & (OMAP_I2C_STAT_NACK |
> OMAP_I2C_STAT_AL)) {
> > +								omap_i2c_ack_stat(dev,
> stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> generic comment - code nesting is getting overwhelming - we may like to
> refactor the isr function at a later date I think..
> > +								err |= OMAP_I2C_STAT_XUDF;
> why set the err if we wantedly force this to happen?
> [Moiz]
> The idea here is to let the upper application to know that something went
> wrong while waiting for the XUDF bit to be set. This is just for debugging
> purpose.
> > +								goto complete;
> 
> > +							}
> > +							cpu_relax();
> > +							stat = omap_i2c_read_reg(dev,
> OMAP_I2C_STAT_REG);
> > +						}
> this is an infinite while loop + it tries to handle error cases -
> essentially do another isr routine inside itself.
> [Moiz]
> Yes, it is a busy wait loop. But AFAIK while we come to this part of the
> code the only thing that can go wrong in the transfer is either the device
> would go off suddenly (creating a NACK) or there is an arbitration
> lost(AL). This loop takes care of these two error conditions. Apart from
> these, if the hardware is functional, the XUDF bit should be set as soon
> as the FIFO gets empty.
> 
> Correct me if I am missing something.

As of now I am posting my patch without a timeout, later as per the need I will optimize it with a timeout approach.

> 
> How about:
> Apply [1] and the following?
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ad8d201..e3f21af 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
>                  }
>                  if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
>                          u8 num_bytes = 1;
> +                       if (cpu_is_omap34xx() &&
> +                               !(stat & OMAP_I2C_STAT_XUDF)){
> +                               cpu_relax();
> +                               continue;
> +                       }
> +
>                          if (dev->fifo_size) {
>                                  if (stat & OMAP_I2C_STAT_XRDY)
>                                          num_bytes = dev->fifo_size;
> 
> 
> Regards,
> Nishanth Menon
> Ref:
> [1] http://patchwork.kernel.org/patch/32332/
> --
> 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

[PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.

Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.

Signed-off-by: Moiz Sonasath<m-sonasath-l0cyMroinI0@public.gmane.org>
Signed-off-by: Vikram pandita<vikram.pandita-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			break;
 		}
 
+		err = 0;
+complete:
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 
-		err = 0;
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
 							"data to send\n");
 					break;
 				}
+
+				/*
+				 * OMAP3430 Errata 1.153: When an XRDY/XDR
+				 * is hit, wait for XUDF before writing data
+				 * to DATA_REG. Otherwise some data bytes can
+				 * be lost while transferring them from the
+				 * memory to the I2C interface.
+				 */
+
+				if (cpu_is_omap34xx()) {
+						while (!(stat & OMAP_I2C_STAT_XUDF)) {
+							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
+								err |= OMAP_I2C_STAT_XUDF;
+								goto complete;
+							}
+							cpu_relax();
+							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+						}
+				}
+
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
 			omap_i2c_ack_stat(dev,
-- 
1.5.6.3

Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Paul Walmsley]

Hello Moiz,

A few remaining comments, most of these from an earlier message.

On Tue, 21 Jul 2009, Sonasath, Moiz wrote:

> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> Otherwise some data bytes can be lost while transferring them from the
> memory to the I2C interface.
> 
> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
> if there is NACK | AL, set the appropriate error flags, ack the pending
> interrupts and return from the ISR.
> 
> Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
> Signed-off-by: Vikram pandita<vikram.pandita@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 05b5e4c..8deaf87 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  			break;
>  		}
>  
> +		err = 0;
> +complete:
>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
>  
> -		err = 0;
>  		if (stat & OMAP_I2C_STAT_NACK) {
>  			err |= OMAP_I2C_STAT_NACK;
>  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  							"data to send\n");
>  					break;
>  				}
> +
> +				/*
> +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> +				 * is hit, wait for XUDF before writing data
> +				 * to DATA_REG. Otherwise some data bytes can
> +				 * be lost while transferring them from the
> +				 * memory to the I2C interface.
> +				 */

Based on this description, shouldn't this patch also zero the transmit 
FIFO threshold?  Consider what the transmit path becomes after this patch:

1. Fill transmit FIFO
2. Leave ISR & wait for interrupt
3. Interrupt happens due to XDR/XRDY (transmit FIFO low-water-mark 
   reached)
4. Busy-wait until transmit FIFO & shift register completely empty
5. If more data to send, go to step #1

i2c-omap.c currently sets the transmit FIFO threshold to 1/2 of the total 
FIFO size[1].  This means that, in the worst case, I2C3, the I2C ISR will 
busy-wait in step 4 for the time it takes 32 bytes to be transmitted.  
This is time that the MPU spends doing nothing but spinning, wasting 
power.  This seems unnecessary and wasteful.  The time the driver spends 
busy-waiting in the ISR should be reduced to the lowest possible duration.

To do this, what I suggest that you additionally do in the patch is to 
reduce the transit FIFO threshold/low-water-mark, controlled by 
I2C_BUF.XTRSH, to the lowest possible value.  This should maximize the 
time spent between steps 2 and 3 and minimize the time spent between steps 
3 and 5.

Is there a reason why this can't be done?

> +
> +				if (cpu_is_omap34xx()) {

Does this erratum apply to the I2C IP block on OMAP2430?  It also has FIFO 
transmit capability.  It would be ideal if you can find out from the I2C 
IP block designers.  If you cannot, please consider adding a comment that 
this may also apply to the I2C block on OMAP2430.

In general it is best to enable these workarounds based on the I2C IP 
block's own revision register contents, not the OMAP CPU type.  The goal 
is to remove all these OMAP-specific "cpu_is_omapxxxx()" macros from 
device drivers.  For example, what if a future DaVinci part uses the same 
I2C IP block?

> +						while (!(stat & OMAP_I2C_STAT_XUDF)) {

Is there a reason why you can't just reuse the main while() loop in the 
ISR, and add a state variable to handle any special casing needed in this 
context?  That will avoid this separate while() loop.

> +							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> +								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> +								err |= OMAP_I2C_STAT_XUDF;
> +								goto complete;
> +							}
> +							cpu_relax();
> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +						}
> +				}
> +
>  				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
>  			}
>  			omap_i2c_ack_stat(dev,

For those following along in the archives, this is an extension of 
comments from

   http://www.mail-archive.com/linux-omap@vger.kernel.org/msg13846.html


- Paul


1. Eventually this is likely to change, based on power management 
   constraints.

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Monday, August 03, 2009 2:36 AM
> To: Sonasath, Moiz
> Cc: linux-i2c@vger.kernel.org; linux-omap@vger.kernel.org; Menon,
> Nishanth; Pandita, Vikram
> Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> 
> Hello Moiz,
> 
> A few remaining comments, most of these from an earlier message.
> 
> On Tue, 21 Jul 2009, Sonasath, Moiz wrote:
> 
> > When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> > Otherwise some data bytes can be lost while transferring them from the
> > memory to the I2C interface.
> >
> > Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
> > if there is NACK | AL, set the appropriate error flags, ack the pending
> > interrupts and return from the ISR.
> >
> > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
> > Signed-off-by: Vikram pandita<vikram.pandita@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
> >  1 files changed, 23 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
> omap.c
> > index 05b5e4c..8deaf87 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >  			break;
> >  		}
> >
> > +		err = 0;
> > +complete:
> >  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
> >
> > -		err = 0;
> >  		if (stat & OMAP_I2C_STAT_NACK) {
> >  			err |= OMAP_I2C_STAT_NACK;
> >  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> > @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >  							"data to send\n");
> >  					break;
> >  				}
> > +
> > +				/*
> > +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> > +				 * is hit, wait for XUDF before writing data
> > +				 * to DATA_REG. Otherwise some data bytes can
> > +				 * be lost while transferring them from the
> > +				 * memory to the I2C interface.
> > +				 */
> 
> Based on this description, shouldn't this patch also zero the transmit
> FIFO threshold?  Consider what the transmit path becomes after this patch:
> 
> 1. Fill transmit FIFO
> 2. Leave ISR & wait for interrupt
> 3. Interrupt happens due to XDR/XRDY (transmit FIFO low-water-mark
>    reached)
> 4. Busy-wait until transmit FIFO & shift register completely empty
> 5. If more data to send, go to step #1
> 
> i2c-omap.c currently sets the transmit FIFO threshold to 1/2 of the total
> FIFO size[1].  This means that, in the worst case, I2C3, the I2C ISR will
> busy-wait in step 4 for the time it takes 32 bytes to be transmitted.
> This is time that the MPU spends doing nothing but spinning, wasting
> power.  This seems unnecessary and wasteful.  The time the driver spends
> busy-waiting in the ISR should be reduced to the lowest possible duration.
> 
> To do this, what I suggest that you additionally do in the patch is to
> reduce the transit FIFO threshold/low-water-mark, controlled by
> I2C_BUF.XTRSH, to the lowest possible value.  This should maximize the
> time spent between steps 2 and 3 and minimize the time spent between steps
> 3 and 5.
> 
> Is there a reason why this can't be done?

Yes, this is actually lined up in my list of actions. I will be working on this to test the functionality and stability of I2C code with the threshold set to zero.

> 
> > +
> > +				if (cpu_is_omap34xx()) {
> 
> Does this erratum apply to the I2C IP block on OMAP2430?  It also has FIFO
> transmit capability.  It would be ideal if you can find out from the I2C
> IP block designers.  If you cannot, please consider adding a comment that
> this may also apply to the I2C block on OMAP2430.
> 
> In general it is best to enable these workarounds based on the I2C IP
> block's own revision register contents, not the OMAP CPU type.  The goal
> is to remove all these OMAP-specific "cpu_is_omapxxxx()" macros from
> device drivers.  For example, what if a future DaVinci part uses the same
> I2C IP block?

Yes this is the right way.
I am checking with the IP team and will get back on this action item.

> 
> > +						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> 
> Is there a reason why you can't just reuse the main while() loop in the
> ISR, and add a state variable to handle any special casing needed in this
> context?  That will avoid this separate while() loop.
> 

The problem with using the main while() loop is the counter 'count' associated with it as I am not sure if the count value of 100 is enough wait time for allowing the XUDF bit to set and if we can come up with an accurate wait count to be used there. 

The idea is that if the hardware is functional, XUDF bit will be set once the FIFO and shift registers are empty and the only thing that can go wrong at that point is a sudden NACK or AL. These errors are taken care in my while loop thereby eliminating the need of a timeout on XUDF wait.



> > +							if (stat & (OMAP_I2C_STAT_NACK |
> OMAP_I2C_STAT_AL)) {
> > +								omap_i2c_ack_stat(dev,
> stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> > +								err |= OMAP_I2C_STAT_XUDF;
> > +								goto complete;
> > +							}
> > +							cpu_relax();
> > +							stat = omap_i2c_read_reg(dev,
> OMAP_I2C_STAT_REG);
> > +						}
> > +				}
> > +
> >  				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> >  			}
> >  			omap_i2c_ack_stat(dev,
> 
> For those following along in the archives, this is an extension of
> comments from
> 
>    http://www.mail-archive.com/linux-omap@vger.kernel.org/msg13846.html
> 
> 
> - Paul
> 
> 
> 1. Eventually this is likely to change, based on power management
>    constraints.

Regards
Moiz Sonasath

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

> -----Original Message-----
> From: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-i2c-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Sonasath, Moiz
> Sent: Monday, August 03, 2009 3:20 PM
> To: Paul Walmsley
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Menon,
> Nishanth; Pandita, Vikram
> Subject: RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> 
> 
> > -----Original Message-----
> > From: Paul Walmsley [mailto:paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org]
> > Sent: Monday, August 03, 2009 2:36 AM
> > To: Sonasath, Moiz
> > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Menon,
> > Nishanth; Pandita, Vikram
> > Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> >
> > Hello Moiz,
> >
> > A few remaining comments, most of these from an earlier message.
> >
> > On Tue, 21 Jul 2009, Sonasath, Moiz wrote:
> >
> > > When an XRDY/XDR is hit, wait for XUDF before writing data to
> DATA_REG.
> > > Otherwise some data bytes can be lost while transferring them from the
> > > memory to the I2C interface.
> > >
> > > Do a Busy-wait for XUDF, before writing data to DATA_REG. While
> waiting
> > > if there is NACK | AL, set the appropriate error flags, ack the
> pending
> > > interrupts and return from the ISR.
> > >
> > > Signed-off-by: Moiz Sonasath<m-sonasath-l0cyMroinI0@public.gmane.org>
> > > Signed-off-by: Vikram pandita<vikram.pandita-l0cyMroinI0@public.gmane.org>
> > > ---
> > >  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
> > >  1 files changed, 23 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
> > omap.c
> > > index 05b5e4c..8deaf87 100644
> > > --- a/drivers/i2c/busses/i2c-omap.c
> > > +++ b/drivers/i2c/busses/i2c-omap.c
> > > @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > >  			break;
> > >  		}
> > >
> > > +		err = 0;
> > > +complete:
> > >  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
> > >
> > > -		err = 0;
> > >  		if (stat & OMAP_I2C_STAT_NACK) {
> > >  			err |= OMAP_I2C_STAT_NACK;
> > >  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> > > @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > >  							"data to send\n");
> > >  					break;
> > >  				}
> > > +
> > > +				/*
> > > +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> > > +				 * is hit, wait for XUDF before writing data
> > > +				 * to DATA_REG. Otherwise some data bytes can
> > > +				 * be lost while transferring them from the
> > > +				 * memory to the I2C interface.
> > > +				 */
> >
> > Based on this description, shouldn't this patch also zero the transmit
> > FIFO threshold?  Consider what the transmit path becomes after this
> patch:
> >
> > 1. Fill transmit FIFO
> > 2. Leave ISR & wait for interrupt
> > 3. Interrupt happens due to XDR/XRDY (transmit FIFO low-water-mark
> >    reached)
> > 4. Busy-wait until transmit FIFO & shift register completely empty
> > 5. If more data to send, go to step #1
> >
> > i2c-omap.c currently sets the transmit FIFO threshold to 1/2 of the
> total
> > FIFO size[1].  This means that, in the worst case, I2C3, the I2C ISR
> will
> > busy-wait in step 4 for the time it takes 32 bytes to be transmitted.
> > This is time that the MPU spends doing nothing but spinning, wasting
> > power.  This seems unnecessary and wasteful.  The time the driver spends
> > busy-waiting in the ISR should be reduced to the lowest possible
> duration.
> >
> > To do this, what I suggest that you additionally do in the patch is to
> > reduce the transit FIFO threshold/low-water-mark, controlled by
> > I2C_BUF.XTRSH, to the lowest possible value.  This should maximize the
> > time spent between steps 2 and 3 and minimize the time spent between
> steps
> > 3 and 5.
> >
> > Is there a reason why this can't be done?
> 
> Yes, this is actually lined up in my list of actions. I will be working on
> this to test the functionality and stability of I2C code with the
> threshold set to zero.
> 

I did some analysis and testing of the code with threshold set to zero, we cannot make the threshold zero with the present code in place, as this would hamper the functionality of the draining feature because in this case the XDR interrupt will not be triggered.

XDR-> when TX FIFO level equal/below the XTRSH AND TXSTAT is less than XTRSH

XRDY-> when TX FIFO level equal/below the XTRSH AND TXSTAT is equal/greater than XTRSH 

This in turn causes XRDY to be triggered always (even when there are only last few bytes left to be transmitted) and therefore the code tries to transmit data when the upper application is out of data.

> >
> > > +
> > > +				if (cpu_is_omap34xx()) {
> >
> > Does this erratum apply to the I2C IP block on OMAP2430?  It also has
> FIFO
> > transmit capability.  It would be ideal if you can find out from the I2C
> > IP block designers.  If you cannot, please consider adding a comment
> that
> > this may also apply to the I2C block on OMAP2430.
> >
> > In general it is best to enable these workarounds based on the I2C IP
> > block's own revision register contents, not the OMAP CPU type.  The goal
> > is to remove all these OMAP-specific "cpu_is_omapxxxx()" macros from
> > device drivers.  For example, what if a future DaVinci part uses the
> same
> > I2C IP block?
> 
> Yes this is the right way.
> I am checking with the IP team and will get back on this action item.

I checked with the I2C IP team, yes the errata applies to the I2C block on OMAP2430 and OMAP2420. I will send out a patch to include OMAP24XX for this erratum.

> 
> >
> > > +						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> >
> > Is there a reason why you can't just reuse the main while() loop in the
> > ISR, and add a state variable to handle any special casing needed in
> this
> > context?  That will avoid this separate while() loop.
> >
> 
> The problem with using the main while() loop is the counter 'count'
> associated with it as I am not sure if the count value of 100 is enough
> wait time for allowing the XUDF bit to set and if we can come up with an
> accurate wait count to be used there.
> 
> The idea is that if the hardware is functional, XUDF bit will be set once
> the FIFO and shift registers are empty and the only thing that can go
> wrong at that point is a sudden NACK or AL. These errors are taken care in
> my while loop thereby eliminating the need of a timeout on XUDF wait.
> 
> 
> 
> > > +							if (stat & (OMAP_I2C_STAT_NACK |
> > OMAP_I2C_STAT_AL)) {
> > > +								omap_i2c_ack_stat(dev,
> > stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> > > +								err |= OMAP_I2C_STAT_XUDF;
> > > +								goto complete;
> > > +							}
> > > +							cpu_relax();
> > > +							stat = omap_i2c_read_reg(dev,
> > OMAP_I2C_STAT_REG);
> > > +						}
> > > +				}
> > > +
> > >  				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> > >  			}
> > >  			omap_i2c_ack_stat(dev,
> >
> > For those following along in the archives, this is an extension of
> > comments from
> >
> >    http://www.mail-archive.com/linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg13846.html
> >
> >
> > - Paul
> >
> >
> > 1. Eventually this is likely to change, based on power management
> >    constraints.
> 
> Regards
> Moiz Sonasath
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards
Moiz Sonasath

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Paul Walmsley]

Hello Moiz,

On Mon, 10 Aug 2009, Sonasath, Moiz wrote:

> I did some analysis and testing of the code with threshold set to zero, 
> we cannot make the threshold zero with the present code in place, as 
> this would hamper the functionality of the draining feature because in 
> this case the XDR interrupt will not be triggered.
> 
> XDR-> when TX FIFO level equal/below the XTRSH AND TXSTAT is less than XTRSH
> 
> XRDY-> when TX FIFO level equal/below the XTRSH AND TXSTAT is 
> equal/greater than XTRSH
> 
> This in turn causes XRDY to be triggered always (even when there are 
> only last few bytes left to be transmitted) and therefore the code tries 
> to transmit data when the upper application is out of data.

Thanks for looking into this.  How about just changing

	if (dev->fifo_size) {
		if (stat & OMAP_I2C_STAT_XRDY)
			num_bytes = dev->fifo_size;
		else    /* read TXSTAT on XDR interrupt */
			num_bytes = omap_i2c_read_reg(dev,
					    OMAP_I2C_BUFSTAT_REG)
					    & 0x3F;
	}

to something like:

	if (dev->fifo_size) {   /* 2430 and beyond */
		if (stat & OMAP_I2C_STAT_XRDY)
			num_bytes = clamp(dev->buf_len, 1, dev->fifo_size);
	        else
		        num_bytes = omap_i2c_read_reg(dev,							
                                         OMAP_I2C_BUFSTAT_REG)
		 			 & 0x3F;
        }

in the ISR?

The transmit and receive FIFO thresholds should also be split, so the 
short transmit FIFO threshold doesn't affect the receive FIFO threshold.

> I checked with the I2C IP team, yes the errata applies to the I2C block 
> on OMAP2430 and OMAP2420. I will send out a patch to include OMAP24XX 
> for this erratum.

Great, thanks for checking this.  Might as well combine it with the same 
patch and make it a revision test based on the I2C controller revision 
reg.


- Paul

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Hello Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Monday, August 10, 2009 11:30 AM
> To: Sonasath, Moiz
> Cc: linux-i2c@vger.kernel.org; linux-omap@vger.kernel.org; Menon,
> Nishanth; Pandita, Vikram
> Subject: RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> 
> Hello Moiz,
> 
> On Mon, 10 Aug 2009, Sonasath, Moiz wrote:
> 
> > I did some analysis and testing of the code with threshold set to zero,
> > we cannot make the threshold zero with the present code in place, as
> > this would hamper the functionality of the draining feature because in
> > this case the XDR interrupt will not be triggered.
> >
> > XDR-> when TX FIFO level equal/below the XTRSH AND TXSTAT is less than
> XTRSH
> >
> > XRDY-> when TX FIFO level equal/below the XTRSH AND TXSTAT is
> > equal/greater than XTRSH
> >
> > This in turn causes XRDY to be triggered always (even when there are
> > only last few bytes left to be transmitted) and therefore the code tries
> > to transmit data when the upper application is out of data.
> 
> Thanks for looking into this.  How about just changing
> 
> 	if (dev->fifo_size) {
> 		if (stat & OMAP_I2C_STAT_XRDY)
> 			num_bytes = dev->fifo_size;
> 		else    /* read TXSTAT on XDR interrupt */
> 			num_bytes = omap_i2c_read_reg(dev,
> 					    OMAP_I2C_BUFSTAT_REG)
> 					    & 0x3F;
> 	}
> 
> to something like:
> 
> 	if (dev->fifo_size) {   /* 2430 and beyond */
> 		if (stat & OMAP_I2C_STAT_XRDY)
> 			num_bytes = clamp(dev->buf_len, 1, dev->fifo_size);
> 	        else
> 		        num_bytes = omap_i2c_read_reg(dev,
> 
>                                          OMAP_I2C_BUFSTAT_REG)
> 		 			 & 0x3F;
>         }
> 
> in the ISR?
> 
> The transmit and receive FIFO thresholds should also be split, so the
> short transmit FIFO threshold doesn't affect the receive FIFO threshold.
> 

Thanks Paul for suggesting this, it definitely works. But there are a few things that I would like to discuss:

1. From the code we have in place now, we are writing 
num_bytes=dev->fifosize(=XTRSH+1) bytes in case of XRDY OR
num_bytes=TXSTAT (last few bytes left) in case of XDR (draining feature)

Without the errata we were writing these num_bytes in a while loop one by one into I2C_DATA reg (TXFIFO).

With the errata we are writing num_bytes in a while loop, but now we write one byte wait for XUDF bit to get set and then write another byte. Thereby, we write a byte, wait for it to get out of the TXFIFO and only then we write the second byte and so on. 

	While(num_bytes)
	{
		Write one byte to I2C_DATA Reg
		Wait for XUDF to set
	}
	Ack the interrupt

So irrespective of the XTRSH value, the wait time is actually the same.

2. Now if we see it from the perspective of interrupts, we are generating an interrupt after every chunk of 4 bytes (with XTRSH+1=4) written to the TXFIFO because we ACK the interrupt after writing a chunk of 4 bytes (one byte at a time waiting for the XUDF bit to be set in between each byte). 

>From the TRM Figure-18-31, in the XRDY path:
Write I2Ci.I2C_DATA register for (XTRSH+1) times and then clear XRDY interrupt

Thus, XTRSH is actually driving how many chunks of data you write before generating a next interrupt. So from this point of view it will be more desirable to make the XTRSH=7 and write chunk of 8 bytes before generating a new interrupt. But we end up staying a longer time in the ISR.

Essentially we are looking at a tradeoff between:
1. Lower XTRSH value: More number of interrupts, less time in an ISR  
2. Higher XTRSH value: Less number of interrupts, more time in an ISR 

Please correct me if I am wrong.

> > I checked with the I2C IP team, yes the errata applies to the I2C block
> > on OMAP2430 and OMAP2420. I will send out a patch to include OMAP24XX
> > for this erratum.
> 
> Great, thanks for checking this.  Might as well combine it with the same
> patch and make it a revision test based on the I2C controller revision
> reg.
> 
> 
> - Paul

Regards
Moiz Sonasath

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Paul Walmsley]

Hi Moiz,

On Tue, 11 Aug 2009, Sonasath, Moiz wrote:

> 1. From the code we have in place now, we are writing 
> num_bytes=dev->fifosize(=XTRSH+1) bytes in case of XRDY OR 
> num_bytes=TXSTAT (last few bytes left) in case of XDR (draining feature)
> 
> Without the errata we were writing these num_bytes in a while loop one 
> by one into I2C_DATA reg (TXFIFO).
> 
> With the errata we are writing num_bytes in a while loop, but now we 
> write one byte wait for XUDF bit to get set and then write another byte. 
> Thereby, we write a byte, wait for it to get out of the TXFIFO and only 
> then we write the second byte and so on.
> 
> 	While(num_bytes)
> 	{
> 		Write one byte to I2C_DATA Reg
> 		Wait for XUDF to set
> 	}
> 	Ack the interrupt

Doesn't your patch do:

 	While(num_bytes)
 	{
 		Wait for XUDF to set
 		Write one byte to I2C_DATA Reg
 	}
 	Ack the interrupt

?

> So irrespective of the XTRSH value, the wait time is actually the same.
> 
> 2. Now if we see it from the perspective of interrupts, we are 
> generating an interrupt after every chunk of 4 bytes (with XTRSH+1=4) 
> written to the TXFIFO because we ACK the interrupt after writing a chunk 
> of 4 bytes (one byte at a time waiting for the XUDF bit to be set in 
> between each byte).
> 
> >From the TRM Figure-18-31, in the XRDY path:
> Write I2Ci.I2C_DATA register for (XTRSH+1) times and then clear XRDY interrupt
> 
> Thus, XTRSH is actually driving how many chunks of data you write before 
> generating a next interrupt. So from this point of view it will be more 
> desirable to make the XTRSH=7 and write chunk of 8 bytes before 
> generating a new interrupt. But we end up staying a longer time in the 
> ISR.
> 
> Essentially we are looking at a tradeoff between:
> 1. Lower XTRSH value: More number of interrupts, less time in an ISR  
> 2. Higher XTRSH value: Less number of interrupts, more time in an ISR 
> 
> Please correct me if I am wrong.

Your analysis looks correct.  I suppose my point is dependent on when the 
XRDY interrupt occurs if XTRSH = 0.

If it occurs one I2C byte transmission time before the XUDF condition 
occurs, then we should just use your busy-waiting patch.  I don't think we 
can do better than that.

But if the XRDY interrupt occurs at the same time as the shift register 
empties, or if there is an undocumented XUDF interrupt that we can use, 
then please consider:

For slower I2C bus speeds, e.g., for a 400KHz I2C bus, emptying a byte out 
of the transmit FIFO should take about 20 microseconds [1].  Another way 
of expressing this is that the duration from when we write a byte to the 
I2C FIFO, to when the controller raises the XRDY/XDR interrupt, should be 
about 20 microseconds when XTRSH = 0.

We can either spend this time busy-looping in the ISR (the tradeoff #2 
that you mention above), or trying to reach WFI and hopefully entering it 
(the tradeoff #1 above).  If possible, tradeoff #1 seems better.  If the 
MPU can reach WFI, it will waste less active power, but it will also 
trigger the PRCM to shut down any inactive power domains, which might not 
need to wake back up when the next system wakeup event occurs.

This should improve over time, from a power efficiency perspective, 
as the amount of time the MPU spends trying to reach WFI should decrease 
[2].

What do you think?


- Paul


1. (8 I2C-cycles/byte / 400000 I2C-cycles/second) = 20 microseconds

2. With currently-available OMAP3 chips, the CPU cycle time can be as 
  short as 1.67 ns (1/600000000 cycles/second).  Future chips will 
  presumably reach at least a 1 ns cycle time, if 
  http://en.wikipedia.org/wiki/Texas_Instruments_OMAP is to be believed. 
  Multiple instructions can be executed per clock cycle.

RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Sonasath, Moiz]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org]
> Sent: Sunday, August 16, 2009 12:39 PM
> To: Sonasath, Moiz
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Menon,
> Nishanth; Pandita, Vikram
> Subject: RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
> 
> Hi Moiz,
> 
> On Tue, 11 Aug 2009, Sonasath, Moiz wrote:
> 
> > 1. From the code we have in place now, we are writing
> > num_bytes=dev->fifosize(=XTRSH+1) bytes in case of XRDY OR
> > num_bytes=TXSTAT (last few bytes left) in case of XDR (draining feature)
> >
> > Without the errata we were writing these num_bytes in a while loop one
> > by one into I2C_DATA reg (TXFIFO).
> >
> > With the errata we are writing num_bytes in a while loop, but now we
> > write one byte wait for XUDF bit to get set and then write another byte.
> > Thereby, we write a byte, wait for it to get out of the TXFIFO and only
> > then we write the second byte and so on.
> >
> > 	While(num_bytes)
> > 	{
> > 		Write one byte to I2C_DATA Reg
> > 		Wait for XUDF to set
> > 	}
> > 	Ack the interrupt
> 
> Doesn't your patch do:
> 
>  	While(num_bytes)
>  	{
>  		Wait for XUDF to set
>  		Write one byte to I2C_DATA Reg
>  	}
>  	Ack the interrupt
> 
> ?
> 


Yes I think I just disordered the sequence there, but yes this is exactly what the patch does.

> > So irrespective of the XTRSH value, the wait time is actually the same.
> >
> > 2. Now if we see it from the perspective of interrupts, we are
> > generating an interrupt after every chunk of 4 bytes (with XTRSH+1=4)
> > written to the TXFIFO because we ACK the interrupt after writing a chunk
> > of 4 bytes (one byte at a time waiting for the XUDF bit to be set in
> > between each byte).
> >
> > >From the TRM Figure-18-31, in the XRDY path:
> > Write I2Ci.I2C_DATA register for (XTRSH+1) times and then clear XRDY
> interrupt
> >
> > Thus, XTRSH is actually driving how many chunks of data you write before
> > generating a next interrupt. So from this point of view it will be more
> > desirable to make the XTRSH=7 and write chunk of 8 bytes before
> > generating a new interrupt. But we end up staying a longer time in the
> > ISR.
> >
> > Essentially we are looking at a tradeoff between:
> > 1. Lower XTRSH value: More number of interrupts, less time in an ISR
> > 2. Higher XTRSH value: Less number of interrupts, more time in an ISR
> >
> > Please correct me if I am wrong.
> 
> Your analysis looks correct.  I suppose my point is dependent on when the
> XRDY interrupt occurs if XTRSH = 0.
> 
> If it occurs one I2C byte transmission time before the XUDF condition
> occurs, then we should just use your busy-waiting patch.  I don't think we
> can do better than that.
> 
> But if the XRDY interrupt occurs at the same time as the shift register
> empties, or if there is an undocumented XUDF interrupt that we can use,
> then please consider:
> 
> For slower I2C bus speeds, e.g., for a 400KHz I2C bus, emptying a byte out
> of the transmit FIFO should take about 20 microseconds [1].  Another way
> of expressing this is that the duration from when we write a byte to the
> I2C FIFO, to when the controller raises the XRDY/XDR interrupt, should be
> about 20 microseconds when XTRSH = 0.
> 
> We can either spend this time busy-looping in the ISR (the tradeoff #2
> that you mention above), or trying to reach WFI and hopefully entering it
> (the tradeoff #1 above).  If possible, tradeoff #1 seems better.  If the
> MPU can reach WFI, it will waste less active power, but it will also
> trigger the PRCM to shut down any inactive power domains, which might not
> need to wake back up when the next system wakeup event occurs.
> 
> This should improve over time, from a power efficiency perspective,
> as the amount of time the MPU spends trying to reach WFI should decrease
> [2].
> 
> What do you think?
> 

On having a closer look at the code, I realized that there is a struct i2c_msg msg[ ] passed to omap_i2c_xfer func in i2c-omap.c driver from the upper application and this relates to the following assignments:

dev->buf = msg->buf;
dev->buf_len = msg->len; 

The code returns from the ISR context only in the following conditions:
-after completing the transfer of dev->len amount of data (ARDY interrupt)
-In case of an error (NACK|AL) 
-when the counter is 100 (too much data to send)

So actually, for a given amount of data we spend more or less the same time in the ISR irrespective of the XTRSH value. I missed this point in my prior analysis.

With the code in place and for the draining feature to work correctly with the present code, we want XTRSH=dev->fiffo_size (presently 4). 

Now if we make XTRSH=dev->fiffo_size=8 (use the full FIFO size) we can have slightly faster performance. We have the following code in place:

omap_i2c_isr
{
	While(any interrupt)
	{
		If (NACK|AL|ARDY)
			Complete_cmd and return IRQ_HANDLED
		If (RRDY|RDR)
			RX implemenetation
		If (XRDY|XDR)
			If (XRDY)
				Num_bytes=dev->fifo_size (8 bytes)
			Else
				Num_bytes=TXSTAT (amount of data left)
			While(Num_bytes)
			{
				Wait for XUDF to set          
 				Write one byte to I2C_DATA Reg
  			}
  			Ack the interrupt
			Continue
}	}
	
Interrupts:
XRDY: triggered when TX FIFO level equal/below XTRSH AND TXSTAT is equal/greater than XTRSH
XDR: triggered when TX FIFO level equal/below XTRSH AND TXSTAT is less than XTRSH

For:
dev->buf_len = 18 bytes and with XTRSH=dev->fiffo_size=8
- XRDY: write 8 bytes in the inner while loop, back to main while loop
- XRDY: write 8 bytes in inner while loop, go back to main while loop
- XDR: write remaining 4 bytes in inner while loop, go back to main while loop
- ARDY: complete command and return IRQ_HANDLED

Opposed to:

dev->buf_len = 18 bytes and with XTRSH=dev->fiffo_size=4
- XRDY: write 4 bytes in inner while loop, go back to main while loop 
- XRDY: write 4 bytes in inner while loop, go back to main while loop
- XRDY: write 4 bytes in inner while loop, go back to main while loop
- XRDY: write 4 bytes in inner while loop, go back to main while loop
- XDR: write remaining 2 bytes in inner while loop, go back to main while loop
- ARDY: complete command and return IRQ_HANDLED

The performance will be worse with XTRSH=dev->fiffo_size=1.

I will try to verify this using oprofile.

Regaards
Moiz