Hi James,
> -----Original Message-----
> From: James Hogan
> Sent: 03 September 2015 09:54
> To: Sifan Naeem; Wolfram Sang; linux-i2c@xxxxxxxxxxxxxxx; Ezequiel Garcia
> Cc: Ionela Voinescu
> Subject: Re: [PATCH v2 2/5] i2c: img-scb: remove fifo EMPTYING interrupts
> handle
>
> On 14/08/15 16:50, Sifan Naeem wrote:
> > Now that we are using the transaction halt interrupt to safely control
>
> Is that referring to patch 4, which comes after this one?
Yes, sorry will update the commit message.
>
> > repeated start transfers, we no longer need to handle the fifo
> > emptying interrupts.
> >
> > Handling this interrupt along with Transaction Halt interrupt can
> > cause erratic behaviour.
>
> You said in response to my question before:
> > EMPTYING interrupt indicates that the transfer is in its last byte,
> > and in old ip versions it was safe to start a new transfer at this
> > point. The erratic behaviour I saw was due to how the latest IP
> > handles Master Halt. In this IP a transaction is halted after each
> > byte of a transfer. Having to halt the transfer after the last byte
> > means we can no longer service the EMPTYING interrupt, doing so may
> > cause repeated start transfers to fails.
>
> That doesn't look like its what the code did though. If emptying and not
> empty, it only wrote to fifo, but didn't start the next transaction.
>
The issue might be caused by the data being written to the fifo, hence i2c->msg.len = 0, but the transfer is actually still halted. So it will need the T_halt being lifted.
And as I saw this issue intermittently, it might be a case of the order interrupts are serviced.
Where as in the old IP, after data is written it was safe to return ISR_WAITSTOP.
Thanks,
Sifan
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem@xxxxxxxxxx>
> > ---
> > drivers/i2c/busses/i2c-img-scb.c | 16 +++-------------
> > 1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index ad1d1df943db..75a44e794d75 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -154,7 +154,6 @@
> > #define INT_TIMING BIT(18)
> >
> > #define INT_FIFO_FULL_FILLING (INT_FIFO_FULL |
> INT_FIFO_FILLING)
> > -#define INT_FIFO_EMPTY_EMPTYING (INT_FIFO_EMPTY |
> INT_FIFO_EMPTYING)
> >
> > /* Level interrupts need clearing after handling instead of before */
> > #define INT_LEVEL 0x01e00
> > @@ -176,8 +175,7 @@
> > INT_WRITE_ACK_ERR | \
> > INT_FIFO_FULL | \
> > INT_FIFO_FILLING | \
> > - INT_FIFO_EMPTY | \
> > - INT_FIFO_EMPTYING)
> > + INT_FIFO_EMPTY)
>
> img_i2c_write_fifo also clears INT_FIFO_EMPTYING from int_enable if
> nothing left to write. That would seem redundant after this change.
>
> Cheers
> James
>
> >
> > #define INT_ENABLE_MASK_WAITSTOP (INT_SLAVE_EVENT | \
> > INT_ADDR_ACK_ERR | \
> > @@ -882,16 +880,8 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> > return ISR_WAITSTOP;
> > }
> > } else {
> > - if (int_status & INT_FIFO_EMPTY_EMPTYING) {
> > - /*
> > - * The write fifo empty indicates that we're in the
> > - * last byte so it's safe to start a new write
> > - * transaction without losing any bytes from the
> > - * previous one.
> > - * see 2.3.7 Repeated Start Transactions.
> > - */
> > - if ((int_status & INT_FIFO_EMPTY) &&
> > - i2c->msg.len == 0)
> > + if (int_status & INT_FIFO_EMPTY) {
> > + if (i2c->msg.len == 0)
> > return ISR_WAITSTOP;
> > img_i2c_write_fifo(i2c);
> > }
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
