[PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Stefan Roese]

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;
 		}
 	}
-- 
2.13.1

Re: [PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Peter Korsgaard]

>>>>> "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?

-- 
Bye, Peter Korsgaard

Re: [PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Stefan Roese]

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

Re: [PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Peter Korsgaard]

>>>>> "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.

-- 
Bye, Peter Korsgaard

Re: [PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Stefan Roese]

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

Re: [PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]


> 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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: ocores: Add missing wake_up() call upon state change to STATE_DONE

[Peter Korsgaard]

>>>>> "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!

-- 
Bye, Peter Korsgaard