> -----Original Message----- > From: Menon, Nishanth > Sent: Tuesday, June 23, 2009 3:27 PM > To: Paul Walmsley; Hald, Ulrik Bech > Cc: Pakaravoor, Jagadeesh; linux-omap@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR > event > > Paul, Ulrik, > > Just adding my view to this discussion: > > -----Original Message----- > > From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap- > > owner@xxxxxxxxxxxxxxx] On Behalf Of Paul Walmsley > > Sent: Tuesday, June 23, 2009 9:05 PM > > To: Hald, Ulrik Bech > > Cc: Pakaravoor, Jagadeesh; linux-omap@xxxxxxxxxxxxxxx > > Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR > > event > > > > > > > > > > > + u8 stat2 = 0; > > > > > > > + stat2 = omap_i2c_read_reg(dev, > OMAP_I2C_STAT_REG); > > > > > > > + if (stat2 & OMAP_I2C_STAT_BB) > > > > > > > + return IRQ_HANDLED; > > > > > > > > > > > > Why use stat2? Why not just test stat again? > > > > > > > > > > Stat is read at the beginning of the ISR, what if BB bit gets > > > > > cleared/set a while later, not along with RDR, as a corner case? > > > > > > > > If that is possible, then the comment in this patch needs to be > > changed: > > > > > > > > > + /* 3430 I2C Errata 1.15 > > > > > + * RDR could be set when the bus is busy, > then > > > > > + * ignore the interrupt, and clear the bit. > > > > > + */ > > > > > > > > This implies that the state of the BB bit is important when the RDR > > bit is > > > > set. The closest sample we have for that is the contents of the > > 'stat' > > > > variable. > > > > > > > If I understand it correctly, you're concerned that the wording of the > > > comment lets one think, that the state of the bus is critical at the > > > moment the RDR interrupt is set? I guess you're right about that, > since > > > the wording could be a little misleading. > > > > My concern is that the comment and the code seem to conflict. > > > > > The point of the errata workaround is only to prevent handling of the > > > RDR interrupt, if the bus is busy at the time when the RDR is to be > > > handled. It doesn't matter if BB has been set/cleared before that. > > > > > > So maybe, the wording of the comment should be: > > > > > > /* 3430 I2C Errata 1.15, 2430 I2C Errata 1.67: > > > * RDR should not be handled when bus is busy */ > > > > > > ? > > > > Yes, that is better. > > > > > > > If we keep it this way, re-reading the register; what could be the > > > > > potential problem? > > > > > > > > It doesn't match the definition of the erratum as expressed in the > > > > comment. Is it possible for the RDR bit to be erroneously set when > BB > > = > > > > 0? > > > > > > Yes, the nature of the errata is that the RDR interrupt could be > > > spurious. Although, if the bus is not busy and the RDR is set > > > erroneously (there are no bytes in the FIFO to be drained), then the > > > normal isr code would just leave and do nothing, which is what we also > > > need in the errata work around. > > > > Okay. So it looks like there is a unfixable race here. Is it possible > > for BB to be 0 during the stat2 read, then for BB to go to 1 immediately > > afterwards? Then the rest of the RDR handler would be entered. If this > > is possible, what will the ISR do -- will it hang? > > > > If this assessment of the problem is accurate, the stat2 read shrinks > > the race window, and then indeed seems useful. > > > > > > a) why would bus be busy? - answer bus is busy before a transaction > starts - each transaction is an atomic operation. If someone goofs around > with signal while a master is sending/receiving data, there is > AL(Arbitration Lost).. not Bus busy. > But couldn't the BB bit be set by the I2C controller to signal a new receive transaction has started, while the ISR is still in the process of handling the RDR interrupt? Anyway, when I look at the code below where the patch is introduced: .. if (dev->fifo_size) { if (stat & OMAP_I2C_STAT_RRDY) num_bytes = dev->fifo_size; else num_bytes = omap_i2c_read_reg(dev, OMAP_I2C_BUFSTAT_REG); } while (num_bytes) { num_bytes--; w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); if (dev->buf_len) { *dev->buf++ = w; dev->buf_len--; /* Data reg from 2430 is 8 bit wide */ if (!cpu_is_omap2430() && !cpu_is_omap34xx()) { if (dev->buf_len) { *dev->buf++ = w >> 8; dev->buf_len--; } } } else { if (stat & OMAP_I2C_STAT_RRDY) dev_err(dev->dev, "RRDY IRQ while no data" " requested\n"); if (stat & OMAP_I2C_STAT_RDR) dev_err(dev->dev, "RDR IRQ while no data" " requested\n"); break; } } .. ... is there really a need for the patch in the first place? The data extraction from the FIFO is handled the same way for both RDR and RRDY interrupts. The only difference is what num_bytes is set to. Sure, RDR could be set spuriously, but if we're handling it the same way as RRDY what's the problem? If data is ready in the FIFO, we extract only what is there. If no data is ready, we simply leave. Please correct me if I am wrong.. > b) Look at the flow in TRM figure 18-13 I2C Master Reciever Mode, > interrupt mode -> where does BB get checked by the h/w? Not in the middle > of the transaction.. > > Apologies if I am wrong, so am not entirely following the discussion > here.. > > Regards, > Nishanth Menon /Ulrik -- 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
