From patchwork Wed Jul 15 15:40:39 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Sonasath, Moiz" X-Patchwork-Id: 35717 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n6FFeq1l032353 for ; Wed, 15 Jul 2009 15:40:53 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754869AbZGOPkv (ORCPT ); Wed, 15 Jul 2009 11:40:51 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755198AbZGOPkv (ORCPT ); Wed, 15 Jul 2009 11:40:51 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:33004 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754869AbZGOPku convert rfc822-to-8bit (ORCPT ); Wed, 15 Jul 2009 11:40:50 -0400 Received: from dlep34.itg.ti.com ([157.170.170.115]) by arroyo.ext.ti.com (8.13.7/8.13.7) with ESMTP id n6FFeiC0011828; Wed, 15 Jul 2009 10:40:49 -0500 Received: from dlep20.itg.ti.com (localhost [127.0.0.1]) by dlep34.itg.ti.com (8.13.7/8.13.7) with ESMTP id n6FFeiMF007713; Wed, 15 Jul 2009 10:40:44 -0500 (CDT) Received: from dlee75.ent.ti.com (localhost [127.0.0.1]) by dlep20.itg.ti.com (8.12.11/8.12.11) with ESMTP id n6FFehT5020589; Wed, 15 Jul 2009 10:40:43 -0500 (CDT) Received: from dlee06.ent.ti.com ([157.170.170.11]) by dlee75.ent.ti.com ([157.170.170.72]) with mapi; Wed, 15 Jul 2009 10:40:43 -0500 From: "Sonasath, Moiz" To: "Menon, Nishanth" CC: "linux-omap@vger.kernel.org" , "Kamat, Nishant" , Paul Walmsley Date: Wed, 15 Jul 2009 10:40:39 -0500 Subject: RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Thread-Topic: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Thread-Index: AcoE2hYE+7DsVwQiRQ+19EFsYQmKBQAhn40g Message-ID: References: <4A5D136D.8030701@ti.com> In-Reply-To: <4A5D136D.8030701@ti.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: acceptlanguage: en-US MIME-Version: 1.0 Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org 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 > Signed-off-by: Vikram Pandita > --- > 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? 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 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;