Message ID | 1439567447-8139-3-git-send-email-sifan.naeem@imgtec.com |
---|---|
State | Changes Requested |
Headers | show |
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? > 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. > > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com> > --- > 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); > } >
Hi James, > -----Original Message----- > From: James Hogan > Sent: 03 September 2015 09:54 > To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org; 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@imgtec.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) #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); }
Now that we are using the transaction halt interrupt to safely control 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. Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com> --- drivers/i2c/busses/i2c-img-scb.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)