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@xxxxxxxxxxxxxxx; 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@xxxxxx>
> Signed-off-by: Vikram Pandita<vikram.pandita@xxxxxx>
> ---
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
