| Message ID | 1370630768-4077-4-git-send-email-grygorii.strashko@ti.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi,
On Fri, Jun 07, 2013 at 09:46:06PM +0300, Grygorii Strashko wrote:
> ARDY|NACK and ARDY|AL are set together in OMAP_I2C_STAT_REG, which will be
Have you seen that happen ever ? AL is Arbitration Lost, we never put
OMAP in a multi-master environment before.
ARDY | NACK I also find it a bit hard for those two to happen together
since ARDY will be set when you can change controller's register
*again*, mening that a transfer has completed.
Also, we need to follow what the programming model says. And, I don't
have docs with me right now, but IIRC it tells us to bail out if any of
the error conditions are met.
Hi Felipe, On 06/07/2013 10:05 PM, Felipe Balbi wrote: > Hi, > > On Fri, Jun 07, 2013 at 09:46:06PM +0300, Grygorii Strashko wrote: >> ARDY|NACK and ARDY|AL are set together in OMAP_I2C_STAT_REG, which will be > Have you seen that happen ever ? AL is Arbitration Lost, we never put > OMAP in a multi-master environment before. This is an example from real life: [ 0.271942] omap_i2c omap_i2c.1: bus 1 rev2.4.0 at 400 kHz [ 1.283416] omap_i2c omap_i2c.1: timeout waiting for bus ready [ 1.300109] OMAP_I2C DEBUG: stat=1001 [ 1.300140] omap_i2c omap_i2c.1: Arbitration lost [ 1.300140] OMAP_I2C DEBUG: IE=601F [ 1.300140] OMAP_I2C DEBUG: STAT=1000 [ 1.300170] OMAP_I2C DEBUG: IV=636F [ 1.300170] OMAP_I2C DEBUG: WE=636F [ 1.300170] OMAP_I2C DEBUG: SYSS=1 [ 1.300170] OMAP_I2C DEBUG: BUF=707 [ 1.300201] OMAP_I2C DEBUG: CNT=1 [ 1.300201] OMAP_I2C DEBUG: DATA=1 [ 1.300201] OMAP_I2C DEBUG: SYSC=215 [ 1.300201] OMAP_I2C DEBUG: CON=8200 [ 1.300231] OMAP_I2C DEBUG: OA=0 [ 1.300231] OMAP_I2C DEBUG: SA=49 [ 1.300231] OMAP_I2C DEBUG: PSC=9 [ 1.300262] OMAP_I2C DEBUG: SCLL=9 [ 1.300262] OMAP_I2C DEBUG: SCLH=3 [ 1.300262] OMAP_I2C DEBUG: SYSTEST=1E0 [ 1.300262] OMAP_I2C DEBUG: BUFSTAT=4000 and my headache now :..( > > ARDY | NACK I also find it a bit hard for those two to happen together > since ARDY will be set when you can change controller's register > *again*, mening that a transfer has completed. There are examples: [ 3.544952] omap_i2c 48060000.i2c: IRQ (ISR = 0x0006) [ 25.574523] omap_i2c 48350000.i2c: IRQ (ISR = 0x0014) [ 25.579925] omap_i2c 48350000.i2c: IRQ (ISR = 0x0012) to see it - enable debug output in omap_i2c_isr_thread: dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); > > Also, we need to follow what the programming model says. And, I don't > have docs with me right now, but IIRC it tells us to bail out if any of > the error conditions are met. > yep, but first of all - all IRQs need to be acked before exit. Sorry, for delayed reply - I've had problems with my e-mail. - grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 19, 2013 at 09:43:04PM +0300, Grygorii Strashko wrote: > Hi Felipe, > On 06/07/2013 10:05 PM, Felipe Balbi wrote: > >Hi, > > > >On Fri, Jun 07, 2013 at 09:46:06PM +0300, Grygorii Strashko wrote: > >>ARDY|NACK and ARDY|AL are set together in OMAP_I2C_STAT_REG, which will be > >Have you seen that happen ever ? AL is Arbitration Lost, we never put > >OMAP in a multi-master environment before. > This is an example from real life: > [ 0.271942] omap_i2c omap_i2c.1: bus 1 rev2.4.0 at 400 kHz > [ 1.283416] omap_i2c omap_i2c.1: timeout waiting for bus ready > [ 1.300109] OMAP_I2C DEBUG: stat=1001 > [ 1.300140] omap_i2c omap_i2c.1: Arbitration lost > [ 1.300140] OMAP_I2C DEBUG: IE=601F > [ 1.300140] OMAP_I2C DEBUG: STAT=1000 > [ 1.300170] OMAP_I2C DEBUG: IV=636F > [ 1.300170] OMAP_I2C DEBUG: WE=636F > [ 1.300170] OMAP_I2C DEBUG: SYSS=1 > [ 1.300170] OMAP_I2C DEBUG: BUF=707 > [ 1.300201] OMAP_I2C DEBUG: CNT=1 > [ 1.300201] OMAP_I2C DEBUG: DATA=1 > [ 1.300201] OMAP_I2C DEBUG: SYSC=215 > [ 1.300201] OMAP_I2C DEBUG: CON=8200 > [ 1.300231] OMAP_I2C DEBUG: OA=0 > [ 1.300231] OMAP_I2C DEBUG: SA=49 > [ 1.300231] OMAP_I2C DEBUG: PSC=9 > [ 1.300262] OMAP_I2C DEBUG: SCLL=9 > [ 1.300262] OMAP_I2C DEBUG: SCLH=3 > [ 1.300262] OMAP_I2C DEBUG: SYSTEST=1E0 > [ 1.300262] OMAP_I2C DEBUG: BUFSTAT=4000 > > and my headache now :..( have you looked for erratas around that ? Maybe you just found a silicon issue. Why is AL bit being set ? Have you tried to reach the IP owner ? If there are no other I2C masters in the system, there will be no arbitration, hence we would never loose the arbitration. > >ARDY | NACK I also find it a bit hard for those two to happen together > >since ARDY will be set when you can change controller's register > >*again*, mening that a transfer has completed. > There are examples: > [ 3.544952] omap_i2c 48060000.i2c: IRQ (ISR = 0x0006) > > [ 25.574523] omap_i2c 48350000.i2c: IRQ (ISR = 0x0014) > [ 25.579925] omap_i2c 48350000.i2c: IRQ (ISR = 0x0012) > > to see it - enable debug output in omap_i2c_isr_thread: > dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); then you need to figure out why that's happening, right ? What do you do to trigger that particular condition, have you looked in the wire to see if you find a real NACK or is the OMAP I2C controller misbehaving ? > >Also, we need to follow what the programming model says. And, I don't > >have docs with me right now, but IIRC it tells us to bail out if any of > >the error conditions are met. > > > yep, but first of all - all IRQs need to be acked before exit. that's alright, then fix only that... OTOH, you don't want to ack a read while data is still sitting in the FIFO, data you haven't read out of the FIFO, I mean. Not sure if that could happen though.
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 2dac598..46fb8a5 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -938,17 +938,15 @@ omap_i2c_isr_thread(int this_irq, void *dev_id) break; } + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL); + if (stat & OMAP_I2C_STAT_NACK) { err |= OMAP_I2C_STAT_NACK; - omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK); - break; } if (stat & OMAP_I2C_STAT_AL) { dev_err(dev->dev, "Arbitration lost\n"); err |= OMAP_I2C_STAT_AL; - omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL); - break; } /*
ARDY|NACK and ARDY|AL are set together in OMAP_I2C_STAT_REG, which will be processed incorrectly now: iterration 1: - I2C irq triggered (ARDY|NACK) - omap_i2c_isr_thread() will ask NACK, fill dev->cmd_err = OMAP_I2C_STAT_NACK and trigger "cmd_complete" completion. - omap_i2c_xfer_msg() will be unblocked, hande NACK and finish its execution. - omap_i2c_xfer() will finish iteration 2: - I2C irq triggered (ARDY) - omap_i2c_isr_thread() will ask ARDY, trigger completion At this point dev->cmd_err == OMAP_I2C_STAT_NACK, "cmd_complete" is not initialized and omap_i2c_xfer() may be completed at all. So, I2C driver is not ready for iteration 2. Hence, fix it by asking NACK, AL and ARDY all togather in omap_i2c_isr_thread() before unblocking omap_i2c_xfer_msg() execution. This is the "old" I2C interrupt handler behavior which was changed by commit: 1d7afc95946487945cc7f5019b41255b72224b70 "i2c: omap: ack IRQ in parts". CC: Kevin Hilman <khilman@linaro.org> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/i2c/busses/i2c-omap.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)