| Message ID | 20170616092317.584-1-sr@denx.de |
|---|---|
| State | Rejected |
| Headers | show |
>>>>> "Stefan" == Stefan Roese <sr@denx.de> writes: > I've noticed that this driver adds a timeout pause of 1 second after > each xfer. This is due to the wait_event_timeout() call in ocores_xfer() > using a "HZ" timeout value and a missing call to wake_up() in > ocores_process() called by the interrupt handler when the state changes > to STATE_DONE at the end of the frame. > This patch adds the missing call resulting in the removal of this 1 > second timeout delay after each xfer. > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Wolfram Sang <wsa@the-dreams.de> > --- > drivers/i2c/busses/i2c-ocores.c | 1 + > 1 file changed, 1 insertion(+) > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index 34f1889a4073..5f8395ea0106 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -191,6 +191,7 @@ static void ocores_process(struct ocores_i2c *i2c) > } else { > i2c-> state = STATE_DONE; > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); > + wake_up(&i2c->wait); > return; It is close to 10 years ago since I last had access to any boards with the ocores controller, but the logic in ocores_process() indicates that the controller would generate another interrupt once the stop condition has been sent: if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) { /* stop has been sent */ oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); wake_up(&i2c->wait); return; } Do you not see this interrupt?
Hi Peter, On 16.06.2017 14:52, Peter Korsgaard wrote: >>>>>> "Stefan" == Stefan Roese <sr@denx.de> writes: > > > I've noticed that this driver adds a timeout pause of 1 second after > > each xfer. This is due to the wait_event_timeout() call in ocores_xfer() > > using a "HZ" timeout value and a missing call to wake_up() in > > ocores_process() called by the interrupt handler when the state changes > > to STATE_DONE at the end of the frame. > > > This patch adds the missing call resulting in the removal of this 1 > > second timeout delay after each xfer. > > > Signed-off-by: Stefan Roese <sr@denx.de> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > --- > > drivers/i2c/busses/i2c-ocores.c | 1 + > > 1 file changed, 1 insertion(+) > > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > > index 34f1889a4073..5f8395ea0106 100644 > > --- a/drivers/i2c/busses/i2c-ocores.c > > +++ b/drivers/i2c/busses/i2c-ocores.c > > @@ -191,6 +191,7 @@ static void ocores_process(struct ocores_i2c *i2c) > > } else { >> i2c-> state = STATE_DONE; > > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); > > + wake_up(&i2c->wait); > > return; > > It is close to 10 years ago since I last had access to any boards with > the ocores controller, but the logic in ocores_process() indicates that > the controller would generate another interrupt once the stop condition > has been sent: > > if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) { > /* stop has been sent */ > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); > wake_up(&i2c->wait); > return; > } > > Do you not see this interrupt? No. It took me quite some time last week, to notice this misbehavior in this I2C driver. As the client (Goodix I2C touchscreen) always returned only after more than 1 second from reading one I2C frame. Thanks, Stefan -- 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
>>>>> "Stefan" == Stefan Roese <sr@denx.de> writes: Hi, >> It is close to 10 years ago since I last had access to any boards with >> the ocores controller, but the logic in ocores_process() indicates that >> the controller would generate another interrupt once the stop condition >> has been sent: >> >> if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) { >> /* stop has been sent */ >> oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); >> wake_up(&i2c->wait); >> return; >> } >> >> Do you not see this interrupt? > No. It took me quite some time last week, to notice this misbehavior > in this I2C driver. As the client (Goodix I2C touchscreen) always > returned only after more than 1 second from reading one I2C frame. Funky. On what kind of platform / what VHDL source version is this? The driver has now existed for more than 10 years and has had contributions from a number of people, but this is the first time I hear about this missing interrupt.
Hi Peter, first sorry for the delay. On 16.06.2017 15:26, Peter Korsgaard wrote: >>>>>> "Stefan" == Stefan Roese <sr@denx.de> writes: > > Hi, > > >> It is close to 10 years ago since I last had access to any boards with > >> the ocores controller, but the logic in ocores_process() indicates that > >> the controller would generate another interrupt once the stop condition > >> has been sent: > >> > >> if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) { > >> /* stop has been sent */ > >> oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); > >> wake_up(&i2c->wait); > >> return; > >> } > >> > >> Do you not see this interrupt? > > > No. It took me quite some time last week, to notice this misbehavior > > in this I2C driver. As the client (Goodix I2C touchscreen) always > > returned only after more than 1 second from reading one I2C frame. > > Funky. On what kind of platform / what VHDL source version is this? Thanks for looking into this. It seems that we had a quite old version of the OpenCores I2C core implemented on our Altera / Intel Arria V board: Rev 1.2 (2001/11/10) > The > driver has now existed for more than 10 years and has had contributions > from a number of people, but this is the first time I hear about this > missing interrupt. After updating to the latest version from OpenCores, the issue with the missing interrupt seems to be resolved. So this patch can be dropped. Thanks, Stefan
> After updating to the latest version from OpenCores, the issue > with the missing interrupt seems to be resolved. So this patch > can be dropped. Thanks for the heads up and thanks to Peter for looking into this issue!
>>>>> "Wolfram" == Wolfram Sang <wsa@the-dreams.de> writes: >> After updating to the latest version from OpenCores, the issue >> with the missing interrupt seems to be resolved. So this patch >> can be dropped. > Thanks for the heads up and thanks to Peter for looking into this issue! Great, thanks both!
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c index 34f1889a4073..5f8395ea0106 100644 --- a/drivers/i2c/busses/i2c-ocores.c +++ b/drivers/i2c/busses/i2c-ocores.c @@ -191,6 +191,7 @@ static void ocores_process(struct ocores_i2c *i2c) } else { i2c->state = STATE_DONE; oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP); + wake_up(&i2c->wait); return; } }
I've noticed that this driver adds a timeout pause of 1 second after each xfer. This is due to the wait_event_timeout() call in ocores_xfer() using a "HZ" timeout value and a missing call to wake_up() in ocores_process() called by the interrupt handler when the state changes to STATE_DONE at the end of the frame. This patch adds the missing call resulting in the removal of this 1 second timeout delay after each xfer. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Wolfram Sang <wsa@the-dreams.de> --- drivers/i2c/busses/i2c-ocores.c | 1 + 1 file changed, 1 insertion(+)