From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Glauber Subject: Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Date: Tue, 8 Nov 2016 10:20:17 +0100 Message-ID: <20161108092017.GC5675@hardcore> References: <20161107200921.30284-1-paul.burton@imgtec.com> <20161107200921.30284-2-paul.burton@imgtec.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Content-Disposition: inline In-Reply-To: <20161107200921.30284-2-paul.burton@imgtec.com> Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Paul Burton Cc: linux-i2c@vger.kernel.org, linux-mips@linux-mips.org, David Daney , Peter Swain , Wolfram Sang List-Id: linux-i2c@vger.kernel.org On Mon, Nov 07, 2016 at 08:09:21PM +0000, Paul Burton wrote: > Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is > early") modified octeon_i2c_wait() & octeon_i2c_hlc_wait() to attempt to > check for a valid bit being clear & if not to sleep for a while then try > again before waiting on a waitqueue which may time out. However it does > so by sleeping within a function called as the condition provided to > wait_event_timeout() which seems to cause strange behaviour, with the > system hanging during boot with the condition being checked constantly & > the timeout not seeming to have any effect. > > Fix this by instead checking for the valid bit being clear in the > octeon_i2c(_hlc)_wait() functions & sleeping there if that condition is > not met, then calling the wait_event_timeout with a condition that does > not sleep. > > Tested on a Rhino Labs UTM-8 with Octeon CN7130. This patch breaks ipmi on ThunderX (CN88xx). We also have not seen the problems you mention, although I must admit that I don't like the complicated nested waits. --Jan > Signed-off-by: Paul Burton > Cc: David Daney > Cc: Jan Glauber > Cc: Peter Swain > Cc: Wolfram Sang > Cc: linux-i2c@vger.kernel.org > --- > > drivers/i2c/busses/i2c-octeon-core.c | 58 ++++++++++-------------------------- > 1 file changed, 15 insertions(+), 43 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c > index 419b54b..2e270a1 100644 > --- a/drivers/i2c/busses/i2c-octeon-core.c > +++ b/drivers/i2c/busses/i2c-octeon-core.c > @@ -36,24 +36,6 @@ static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c) > return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG); > } > > -static bool octeon_i2c_test_ready(struct octeon_i2c *i2c, bool *first) > -{ > - if (octeon_i2c_test_iflg(i2c)) > - return true; > - > - if (*first) { > - *first = false; > - return false; > - } > - > - /* > - * IRQ has signaled an event but IFLG hasn't changed. > - * Sleep and retry once. > - */ > - usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > - return octeon_i2c_test_iflg(i2c); > -} > - > /** > * octeon_i2c_wait - wait for the IFLG to be set > * @i2c: The struct octeon_i2c > @@ -80,8 +62,13 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) > } > > i2c->int_enable(i2c); > - time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_ready(i2c, &first), > - i2c->adap.timeout); > + time_left = i2c->adap.timeout; > + if (!octeon_i2c_test_iflg(i2c)) { > + usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > + time_left = wait_event_timeout(i2c->queue, > + octeon_i2c_test_iflg(i2c), > + time_left); > + } > i2c->int_disable(i2c); > > if (i2c->broken_irq_check && !time_left && > @@ -99,26 +86,8 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) > > static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c) > { > - return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0; > -} > - > -static bool octeon_i2c_hlc_test_ready(struct octeon_i2c *i2c, bool *first) > -{ > /* check if valid bit is cleared */ > - if (octeon_i2c_hlc_test_valid(i2c)) > - return true; > - > - if (*first) { > - *first = false; > - return false; > - } > - > - /* > - * IRQ has signaled an event but valid bit isn't cleared. > - * Sleep and retry once. > - */ > - usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > - return octeon_i2c_hlc_test_valid(i2c); > + return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0; > } > > static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c) > @@ -176,7 +145,6 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c) > */ > static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c) > { > - bool first = true; > int time_left; > > /* > @@ -194,9 +162,13 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c) > } > > i2c->hlc_int_enable(i2c); > - time_left = wait_event_timeout(i2c->queue, > - octeon_i2c_hlc_test_ready(i2c, &first), > - i2c->adap.timeout); > + time_left = i2c->adap.timeout; > + if (!octeon_i2c_hlc_test_valid(i2c)) { > + usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT); > + time_left = wait_event_timeout(i2c->queue, > + octeon_i2c_hlc_test_valid(i2c), > + time_left); > + } > i2c->hlc_int_disable(i2c); > if (!time_left) > octeon_i2c_hlc_int_clear(i2c); > -- > 2.10.2