* [PATCH 1/2] i2c: octeon: Fix register access
@ 2016-11-07 20:09 Paul Burton
2016-11-07 20:09 ` Paul Burton
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-07 20:09 UTC (permalink / raw)
To: linux-i2c; +Cc: linux-mips, Paul Burton, Jan Glauber, David Daney, Wolfram Sang
Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
retries") attempted to replace potentially infinite loops with ones
which will time out using readq_poll_timeout, but in doing so it
inverted the condition for exiting this loop.
Tested on a Rhino Labs UTM-8 with Octeon CN7130.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Jan Glauber <jglauber@cavium.com>
Cc: David Daney <david.daney@cavium.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-i2c@vger.kernel.org
---
drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 1db7c83..d980406 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
__raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI(i2c));
- readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp & SW_TWSI_V,
- I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
+ readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
+ !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
+ i2c->adap.timeout);
}
#define octeon_i2c_ctl_write(i2c, val) \
@@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
- tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
+ !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
i2c->adap.timeout);
if (error)
*error = ret;
--
2.10.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 1/2] i2c: octeon: Fix register access
2016-11-07 20:09 [PATCH 1/2] i2c: octeon: Fix register access Paul Burton
@ 2016-11-07 20:09 ` Paul Burton
2016-11-07 20:09 ` [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Paul Burton
2016-11-08 7:13 ` [PATCH 1/2] i2c: octeon: Fix register access Jan Glauber
2 siblings, 0 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-07 20:09 UTC (permalink / raw)
To: linux-i2c; +Cc: linux-mips, Paul Burton, Jan Glauber, David Daney, Wolfram Sang
Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
retries") attempted to replace potentially infinite loops with ones
which will time out using readq_poll_timeout, but in doing so it
inverted the condition for exiting this loop.
Tested on a Rhino Labs UTM-8 with Octeon CN7130.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Jan Glauber <jglauber@cavium.com>
Cc: David Daney <david.daney@cavium.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-i2c@vger.kernel.org
---
drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 1db7c83..d980406 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
__raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI(i2c));
- readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp & SW_TWSI_V,
- I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
+ readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
+ !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
+ i2c->adap.timeout);
}
#define octeon_i2c_ctl_write(i2c, val) \
@@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
- tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
+ !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
i2c->adap.timeout);
if (error)
*error = ret;
--
2.10.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-07 20:09 [PATCH 1/2] i2c: octeon: Fix register access Paul Burton
2016-11-07 20:09 ` Paul Burton
@ 2016-11-07 20:09 ` Paul Burton
2016-11-07 20:09 ` Paul Burton
` (2 more replies)
2016-11-08 7:13 ` [PATCH 1/2] i2c: octeon: Fix register access Jan Glauber
2 siblings, 3 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-07 20:09 UTC (permalink / raw)
To: linux-i2c
Cc: linux-mips, Paul Burton, David Daney, Jan Glauber, Peter Swain,
Wolfram Sang
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.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David Daney <david.daney@cavium.com>
Cc: Jan Glauber <jglauber@cavium.com>
Cc: Peter Swain <pswain@cavium.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-07 20:09 ` [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Paul Burton
@ 2016-11-07 20:09 ` Paul Burton
2016-11-08 9:20 ` Jan Glauber
2016-11-09 13:41 ` Jan Glauber
2 siblings, 0 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-07 20:09 UTC (permalink / raw)
To: linux-i2c
Cc: linux-mips, Paul Burton, David Daney, Jan Glauber, Peter Swain,
Wolfram Sang
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.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David Daney <david.daney@cavium.com>
Cc: Jan Glauber <jglauber@cavium.com>
Cc: Peter Swain <pswain@cavium.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-07 20:09 [PATCH 1/2] i2c: octeon: Fix register access Paul Burton
2016-11-07 20:09 ` Paul Burton
2016-11-07 20:09 ` [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Paul Burton
@ 2016-11-08 7:13 ` Jan Glauber
2016-11-08 7:13 ` Jan Glauber
` (2 more replies)
2 siblings, 3 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-08 7:13 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Wolfram Sang, stable
On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> retries") attempted to replace potentially infinite loops with ones
> which will time out using readq_poll_timeout, but in doing so it
> inverted the condition for exiting this loop.
>
> Tested on a Rhino Labs UTM-8 with Octeon CN7130.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-i2c@vger.kernel.org
>
> ---
Acked-by: Jan Glauber <jglauber@cavium.com>
Thanks for spotting this. I think this should go into stable too for
4.8, so adding Cc: stable@vger.kernel.org.
> drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
> index 1db7c83..d980406 100644
> --- a/drivers/i2c/busses/i2c-octeon-core.h
> +++ b/drivers/i2c/busses/i2c-octeon-core.h
> @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
>
> __raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI(i2c));
>
> - readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp & SW_TWSI_V,
> - I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
> + readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> + i2c->adap.timeout);
> }
>
> #define octeon_i2c_ctl_write(i2c, val) \
> @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
> __raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
>
> ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> - tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> i2c->adap.timeout);
> if (error)
> *error = ret;
> --
> 2.10.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-08 7:13 ` [PATCH 1/2] i2c: octeon: Fix register access Jan Glauber
@ 2016-11-08 7:13 ` Jan Glauber
2016-11-09 14:09 ` Paul Burton
2016-11-10 20:17 ` Wolfram Sang
2 siblings, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-08 7:13 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Wolfram Sang, stable
On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> retries") attempted to replace potentially infinite loops with ones
> which will time out using readq_poll_timeout, but in doing so it
> inverted the condition for exiting this loop.
>
> Tested on a Rhino Labs UTM-8 with Octeon CN7130.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-i2c@vger.kernel.org
>
> ---
Acked-by: Jan Glauber <jglauber@cavium.com>
Thanks for spotting this. I think this should go into stable too for
4.8, so adding Cc: stable@vger.kernel.org.
> drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
> index 1db7c83..d980406 100644
> --- a/drivers/i2c/busses/i2c-octeon-core.h
> +++ b/drivers/i2c/busses/i2c-octeon-core.h
> @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
>
> __raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI(i2c));
>
> - readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp & SW_TWSI_V,
> - I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
> + readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> + i2c->adap.timeout);
> }
>
> #define octeon_i2c_ctl_write(i2c, val) \
> @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
> __raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
>
> ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> - tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> i2c->adap.timeout);
> if (error)
> *error = ret;
> --
> 2.10.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-07 20:09 ` [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Paul Burton
2016-11-07 20:09 ` Paul Burton
@ 2016-11-08 9:20 ` Jan Glauber
2016-11-08 9:20 ` Jan Glauber
2016-11-09 13:41 ` Jan Glauber
2 siblings, 1 reply; 27+ messages in thread
From: Jan Glauber @ 2016-11-08 9:20 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
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 <paul.burton@imgtec.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: Peter Swain <pswain@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-08 9:20 ` Jan Glauber
@ 2016-11-08 9:20 ` Jan Glauber
0 siblings, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-08 9:20 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
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 <paul.burton@imgtec.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: Peter Swain <pswain@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-07 20:09 ` [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Paul Burton
2016-11-07 20:09 ` Paul Burton
2016-11-08 9:20 ` Jan Glauber
@ 2016-11-09 13:41 ` Jan Glauber
2016-11-09 13:41 ` Jan Glauber
2016-11-09 14:07 ` Paul Burton
2 siblings, 2 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-09 13:41 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
Hi Paul,
I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
register access retries". With debugging enabled I'm getting:
[ 78.871568] ipmi_ssif: Trying hotmod-specified SSIF interface at i2c address 0x12, adapter Cavium ThunderX i2c adapter at 0000:01:09.4, slave address 0x0
[ 78.886107] do not call blocking ops when !TASK_RUNNING; state=2 set at [<fffffc00080e0088>] prepare_to_wait_event+0x58/0x10c
[ 78.897436] ------------[ cut here ]------------
[ 78.902050] WARNING: CPU: 6 PID: 2235 at kernel/sched/core.c:7718 __might_sleep+0x80/0x88
[ 78.910218] Modules linked in: ipmi_ssif i2c_thunderx i2c_smbus nicvf nicpf thunder_bgx thunder_xcv mdio_thunder
[ 78.921916] CPU: 6 PID: 2235 Comm: bash Tainted: G W 4.9.0-rc3-jang+ #17
[ 78.929737] Hardware name: www.cavium.com ThunderX CRB-1S/ThunderX CRB-1S, BIOS 0.3 Aug 24 2016
[ 78.938426] task: fffffe1fdd554500 task.stack: fffffe1fe384c000
[ 78.944338] PC is at __might_sleep+0x80/0x88
[ 78.948601] LR is at __might_sleep+0x80/0x88
[ 78.952863] pc : [<fffffc00080c3aac>] lr : [<fffffc00080c3aac>] pstate: 80000145
[ 78.960250] sp : fffffe1fe384f600
[ 78.963557] x29: fffffe1fe384f600 x28: fffffe1fe384f860
[ 78.968875] x27: fffffe1fd07fa018 x26: fffffe1fe384f968
[ 78.974193] x25: fffffc0009a2b000 x24: 00000000ffff26d6
[ 78.979510] x23: fffffe1fe384f860 x22: fffffe1fe384f860
[ 78.984827] x21: 0000000000000000 x20: 00000000000000b1
[ 78.990144] x19: fffffc0000e330b8 x18: 0000000000005bb0
[ 78.995461] x17: fffffc0009669ca8 x16: 0000000000000000
[ 79.000779] x15: 0000000000000539 x14: 66663c5b20746120
[ 79.006097] x13: 74657320323d6574 x12: 617473203b474e49
[ 79.011415] x11: 4e4e55525f4b5341 x10: 5421206e65687720
[ 79.016732] x9 : 73706f20676e696b x8 : 0000000000000000
[ 79.022049] x7 : fffffc00080f5740 x6 : fffffc00080f5740
[ 79.027367] x5 : ffffffffffffff80 x4 : 0000000000000060
[ 79.032684] x3 : 0000000000000000 x2 : 0000000000000001
[ 79.038001] x1 : 0000000000000000 x0 : 0000000000000071
[ 79.044803] ---[ end trace d8af6005f683d444 ]---
[ 79.049413] Call trace:
[ 79.051853] Exception stack(0xfffffe1fe384f420 to 0xfffffe1fe384f550)
[ 79.058287] f420: fffffc0000e330b8 0000040000000000 fffffe1fe384f600 fffffc00080c3aac
[ 79.066109] f440: 0000000080000145 000000000000003d 0000000000000000 fffffc0008853920
[ 79.073931] f460: 0000040000000000 0000000100000001 fffffe1fe384f520 fffffc00080f60d8
[ 79.081752] f480: fffffc0000e330b8 00000000000000b1 0000000000000000 fffffe1fe384f860
[ 79.089574] f4a0: fffffe1fe384f860 00000000ffff26d6 fffffc0009a2b000 fffffe1fe384f968
[ 79.097396] f4c0: fffffe1fd07fa018 fffffe1fe384f860 0000000000000071 0000000000000000
[ 79.105218] f4e0: 0000000000000001 0000000000000000 0000000000000060 ffffffffffffff80
[ 79.113040] f500: fffffc00080f5740 fffffc00080f5740 0000000000000000 73706f20676e696b
[ 79.120861] f520: 5421206e65687720 4e4e55525f4b5341 617473203b474e49 74657320323d6574
[ 79.128683] f540: 66663c5b20746120 0000000000000539
[ 79.133553] [<fffffc00080c3aac>] __might_sleep+0x80/0x88
[ 79.138862] [<fffffc0000e30138>] octeon_i2c_test_iflg+0x4c/0xbc [i2c_thunderx]
[ 79.146077] [<fffffc0000e30958>] octeon_i2c_test_ready+0x18/0x70 [i2c_thunderx]
[ 79.153379] [<fffffc0000e30b04>] octeon_i2c_wait+0x154/0x1a4 [i2c_thunderx]
[ 79.160334] [<fffffc0000e310bc>] octeon_i2c_xfer+0xf4/0xf60 [i2c_thunderx]
This is not caused by the usleep inside the wait_event but by readq_poll_timeout().
Could you try if it works for you if you only revert this patch?
Thanks,
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-09 13:41 ` Jan Glauber
@ 2016-11-09 13:41 ` Jan Glauber
2016-11-09 14:07 ` Paul Burton
1 sibling, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-09 13:41 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
Hi Paul,
I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
register access retries". With debugging enabled I'm getting:
[ 78.871568] ipmi_ssif: Trying hotmod-specified SSIF interface at i2c address 0x12, adapter Cavium ThunderX i2c adapter at 0000:01:09.4, slave address 0x0
[ 78.886107] do not call blocking ops when !TASK_RUNNING; state=2 set at [<fffffc00080e0088>] prepare_to_wait_event+0x58/0x10c
[ 78.897436] ------------[ cut here ]------------
[ 78.902050] WARNING: CPU: 6 PID: 2235 at kernel/sched/core.c:7718 __might_sleep+0x80/0x88
[ 78.910218] Modules linked in: ipmi_ssif i2c_thunderx i2c_smbus nicvf nicpf thunder_bgx thunder_xcv mdio_thunder
[ 78.921916] CPU: 6 PID: 2235 Comm: bash Tainted: G W 4.9.0-rc3-jang+ #17
[ 78.929737] Hardware name: www.cavium.com ThunderX CRB-1S/ThunderX CRB-1S, BIOS 0.3 Aug 24 2016
[ 78.938426] task: fffffe1fdd554500 task.stack: fffffe1fe384c000
[ 78.944338] PC is at __might_sleep+0x80/0x88
[ 78.948601] LR is at __might_sleep+0x80/0x88
[ 78.952863] pc : [<fffffc00080c3aac>] lr : [<fffffc00080c3aac>] pstate: 80000145
[ 78.960250] sp : fffffe1fe384f600
[ 78.963557] x29: fffffe1fe384f600 x28: fffffe1fe384f860
[ 78.968875] x27: fffffe1fd07fa018 x26: fffffe1fe384f968
[ 78.974193] x25: fffffc0009a2b000 x24: 00000000ffff26d6
[ 78.979510] x23: fffffe1fe384f860 x22: fffffe1fe384f860
[ 78.984827] x21: 0000000000000000 x20: 00000000000000b1
[ 78.990144] x19: fffffc0000e330b8 x18: 0000000000005bb0
[ 78.995461] x17: fffffc0009669ca8 x16: 0000000000000000
[ 79.000779] x15: 0000000000000539 x14: 66663c5b20746120
[ 79.006097] x13: 74657320323d6574 x12: 617473203b474e49
[ 79.011415] x11: 4e4e55525f4b5341 x10: 5421206e65687720
[ 79.016732] x9 : 73706f20676e696b x8 : 0000000000000000
[ 79.022049] x7 : fffffc00080f5740 x6 : fffffc00080f5740
[ 79.027367] x5 : ffffffffffffff80 x4 : 0000000000000060
[ 79.032684] x3 : 0000000000000000 x2 : 0000000000000001
[ 79.038001] x1 : 0000000000000000 x0 : 0000000000000071
[ 79.044803] ---[ end trace d8af6005f683d444 ]---
[ 79.049413] Call trace:
[ 79.051853] Exception stack(0xfffffe1fe384f420 to 0xfffffe1fe384f550)
[ 79.058287] f420: fffffc0000e330b8 0000040000000000 fffffe1fe384f600 fffffc00080c3aac
[ 79.066109] f440: 0000000080000145 000000000000003d 0000000000000000 fffffc0008853920
[ 79.073931] f460: 0000040000000000 0000000100000001 fffffe1fe384f520 fffffc00080f60d8
[ 79.081752] f480: fffffc0000e330b8 00000000000000b1 0000000000000000 fffffe1fe384f860
[ 79.089574] f4a0: fffffe1fe384f860 00000000ffff26d6 fffffc0009a2b000 fffffe1fe384f968
[ 79.097396] f4c0: fffffe1fd07fa018 fffffe1fe384f860 0000000000000071 0000000000000000
[ 79.105218] f4e0: 0000000000000001 0000000000000000 0000000000000060 ffffffffffffff80
[ 79.113040] f500: fffffc00080f5740 fffffc00080f5740 0000000000000000 73706f20676e696b
[ 79.120861] f520: 5421206e65687720 4e4e55525f4b5341 617473203b474e49 74657320323d6574
[ 79.128683] f540: 66663c5b20746120 0000000000000539
[ 79.133553] [<fffffc00080c3aac>] __might_sleep+0x80/0x88
[ 79.138862] [<fffffc0000e30138>] octeon_i2c_test_iflg+0x4c/0xbc [i2c_thunderx]
[ 79.146077] [<fffffc0000e30958>] octeon_i2c_test_ready+0x18/0x70 [i2c_thunderx]
[ 79.153379] [<fffffc0000e30b04>] octeon_i2c_wait+0x154/0x1a4 [i2c_thunderx]
[ 79.160334] [<fffffc0000e310bc>] octeon_i2c_xfer+0xf4/0xf60 [i2c_thunderx]
This is not caused by the usleep inside the wait_event but by readq_poll_timeout().
Could you try if it works for you if you only revert this patch?
Thanks,
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-09 13:41 ` Jan Glauber
2016-11-09 13:41 ` Jan Glauber
@ 2016-11-09 14:07 ` Paul Burton
2016-11-09 14:07 ` Paul Burton
` (2 more replies)
1 sibling, 3 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-09 14:07 UTC (permalink / raw)
To: Jan Glauber; +Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]
On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote:
> Hi Paul,
>
> I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
> register access retries". With debugging enabled I'm getting:
>
> <snip>
>
> This is not caused by the usleep inside the wait_event but by
> readq_poll_timeout(). Could you try if it works for you if you only revert
> this patch?
>
> Thanks,
> Jan
Hi Jan,
If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx:
Limit register access retries") sadly it doesn't fix my system. A boot of a
cavium_octeon_defconfig kernel with initcall_debug ends with:
calling octeon_mgmt_mod_init+0x0/0x28 @ 1
initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs
calling ds1307_driver_init+0x0/0x10 @ 1
initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs
calling octeon_i2c_driver_init+0x0/0x10 @ 1
ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133
ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32)
ata1.00: configured for UDMA/133
scsi 0:0:0:0: Direct-Access ATA SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5
sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support
DPO or FUA
sda: sda1 sda2 sda3 sda4
sd 0:0:0:0: [sda] Attached SCSI disk
ata2: SATA link down (SStatus 0 SControl 300)
random: crng init done
As you can see octeon_i2c_driver_init never returns. Are you able to test on
one of your MIPS-based systems?
Thanks,
Paul
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-09 14:07 ` Paul Burton
@ 2016-11-09 14:07 ` Paul Burton
2016-11-09 14:38 ` Jan Glauber
2016-11-11 8:57 ` Jan Glauber
2 siblings, 0 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-09 14:07 UTC (permalink / raw)
To: Jan Glauber; +Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]
On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote:
> Hi Paul,
>
> I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
> register access retries". With debugging enabled I'm getting:
>
> <snip>
>
> This is not caused by the usleep inside the wait_event but by
> readq_poll_timeout(). Could you try if it works for you if you only revert
> this patch?
>
> Thanks,
> Jan
Hi Jan,
If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx:
Limit register access retries") sadly it doesn't fix my system. A boot of a
cavium_octeon_defconfig kernel with initcall_debug ends with:
calling octeon_mgmt_mod_init+0x0/0x28 @ 1
initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs
calling ds1307_driver_init+0x0/0x10 @ 1
initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs
calling octeon_i2c_driver_init+0x0/0x10 @ 1
ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133
ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32)
ata1.00: configured for UDMA/133
scsi 0:0:0:0: Direct-Access ATA SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5
sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support
DPO or FUA
sda: sda1 sda2 sda3 sda4
sd 0:0:0:0: [sda] Attached SCSI disk
ata2: SATA link down (SStatus 0 SControl 300)
random: crng init done
As you can see octeon_i2c_driver_init never returns. Are you able to test on
one of your MIPS-based systems?
Thanks,
Paul
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-08 7:13 ` [PATCH 1/2] i2c: octeon: Fix register access Jan Glauber
2016-11-08 7:13 ` Jan Glauber
@ 2016-11-09 14:09 ` Paul Burton
2016-11-09 14:09 ` Paul Burton
2016-11-09 14:43 ` Jan Glauber
2016-11-10 20:17 ` Wolfram Sang
2 siblings, 2 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-09 14:09 UTC (permalink / raw)
To: Jan Glauber; +Cc: linux-i2c, linux-mips, David Daney, Wolfram Sang, stable
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
On Tuesday, 8 November 2016 08:13:37 GMT Jan Glauber wrote:
> On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > retries") attempted to replace potentially infinite loops with ones
> > which will time out using readq_poll_timeout, but in doing so it
> > inverted the condition for exiting this loop.
> >
> > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> >
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Jan Glauber <jglauber@cavium.com>
> > Cc: David Daney <david.daney@cavium.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-i2c@vger.kernel.org
> >
> > ---
>
> Acked-by: Jan Glauber <jglauber@cavium.com>
>
> Thanks for spotting this. I think this should go into stable too for
> 4.8, so adding Cc: stable@vger.kernel.org.
Hi Jan,
...but the bad patch was only merged for v4.9-rc1?
Thanks,
Paul
>
> > drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-octeon-core.h
> > b/drivers/i2c/busses/i2c-octeon-core.h index 1db7c83..d980406 100644
> > --- a/drivers/i2c/busses/i2c-octeon-core.h
> > +++ b/drivers/i2c/busses/i2c-octeon-core.h
> > @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct
> > octeon_i2c *i2c, u64 eop_reg, u8>
> > __raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base +
SW_TWSI(i2c));
> >
> > - readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp &
SW_TWSI_V,
> > - I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
> > + readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > + i2c->adap.timeout);
> >
> > }
> >
> > #define octeon_i2c_ctl_write(i2c, val) \
> >
> > @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct
> > octeon_i2c *i2c, u64 eop_reg,>
> > __raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base +
> > SW_TWSI(i2c));
> >
> > ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> >
> > - tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> >
> > i2c->adap.timeout);
> >
> > if (error)
> >
> > *error = ret;
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-09 14:09 ` Paul Burton
@ 2016-11-09 14:09 ` Paul Burton
2016-11-09 14:43 ` Jan Glauber
1 sibling, 0 replies; 27+ messages in thread
From: Paul Burton @ 2016-11-09 14:09 UTC (permalink / raw)
To: Jan Glauber; +Cc: linux-i2c, linux-mips, David Daney, Wolfram Sang, stable
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
On Tuesday, 8 November 2016 08:13:37 GMT Jan Glauber wrote:
> On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > retries") attempted to replace potentially infinite loops with ones
> > which will time out using readq_poll_timeout, but in doing so it
> > inverted the condition for exiting this loop.
> >
> > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> >
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Jan Glauber <jglauber@cavium.com>
> > Cc: David Daney <david.daney@cavium.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-i2c@vger.kernel.org
> >
> > ---
>
> Acked-by: Jan Glauber <jglauber@cavium.com>
>
> Thanks for spotting this. I think this should go into stable too for
> 4.8, so adding Cc: stable@vger.kernel.org.
Hi Jan,
...but the bad patch was only merged for v4.9-rc1?
Thanks,
Paul
>
> > drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-octeon-core.h
> > b/drivers/i2c/busses/i2c-octeon-core.h index 1db7c83..d980406 100644
> > --- a/drivers/i2c/busses/i2c-octeon-core.h
> > +++ b/drivers/i2c/busses/i2c-octeon-core.h
> > @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct
> > octeon_i2c *i2c, u64 eop_reg, u8>
> > __raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base +
SW_TWSI(i2c));
> >
> > - readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp &
SW_TWSI_V,
> > - I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
> > + readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > + i2c->adap.timeout);
> >
> > }
> >
> > #define octeon_i2c_ctl_write(i2c, val) \
> >
> > @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct
> > octeon_i2c *i2c, u64 eop_reg,>
> > __raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base +
> > SW_TWSI(i2c));
> >
> > ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> >
> > - tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> >
> > i2c->adap.timeout);
> >
> > if (error)
> >
> > *error = ret;
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-09 14:07 ` Paul Burton
2016-11-09 14:07 ` Paul Burton
@ 2016-11-09 14:38 ` Jan Glauber
2016-11-09 14:38 ` Jan Glauber
2016-11-11 8:57 ` Jan Glauber
2 siblings, 1 reply; 27+ messages in thread
From: Jan Glauber @ 2016-11-09 14:38 UTC (permalink / raw)
To: Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang,
Steven J. Hill
On Wed, Nov 09, 2016 at 02:07:58PM +0000, Paul Burton wrote:
> On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote:
> > Hi Paul,
> >
> > I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
> > register access retries". With debugging enabled I'm getting:
> >
> > <snip>
> >
> > This is not caused by the usleep inside the wait_event but by
> > readq_poll_timeout(). Could you try if it works for you if you only revert
> > this patch?
> >
> > Thanks,
> > Jan
>
> Hi Jan,
>
> If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx:
> Limit register access retries") sadly it doesn't fix my system. A boot of a
> cavium_octeon_defconfig kernel with initcall_debug ends with:
>
> calling octeon_mgmt_mod_init+0x0/0x28 @ 1
> initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs
> calling ds1307_driver_init+0x0/0x10 @ 1
> initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs
> calling octeon_i2c_driver_init+0x0/0x10 @ 1
> ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133
> ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32)
> ata1.00: configured for UDMA/133
> scsi 0:0:0:0: Direct-Access ATA SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5
> sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support
> DPO or FUA
> sda: sda1 sda2 sda3 sda4
> sd 0:0:0:0: [sda] Attached SCSI disk
> ata2: SATA link down (SStatus 0 SControl 300)
> random: crng init done
>
> As you can see octeon_i2c_driver_init never returns. Are you able to test on
> one of your MIPS-based systems?
CC'ing Steven who might be able to test on MIPS.
> Thanks,
> Paul
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-09 14:38 ` Jan Glauber
@ 2016-11-09 14:38 ` Jan Glauber
0 siblings, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-09 14:38 UTC (permalink / raw)
To: Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang,
Steven J. Hill
On Wed, Nov 09, 2016 at 02:07:58PM +0000, Paul Burton wrote:
> On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote:
> > Hi Paul,
> >
> > I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
> > register access retries". With debugging enabled I'm getting:
> >
> > <snip>
> >
> > This is not caused by the usleep inside the wait_event but by
> > readq_poll_timeout(). Could you try if it works for you if you only revert
> > this patch?
> >
> > Thanks,
> > Jan
>
> Hi Jan,
>
> If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx:
> Limit register access retries") sadly it doesn't fix my system. A boot of a
> cavium_octeon_defconfig kernel with initcall_debug ends with:
>
> calling octeon_mgmt_mod_init+0x0/0x28 @ 1
> initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs
> calling ds1307_driver_init+0x0/0x10 @ 1
> initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs
> calling octeon_i2c_driver_init+0x0/0x10 @ 1
> ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133
> ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32)
> ata1.00: configured for UDMA/133
> scsi 0:0:0:0: Direct-Access ATA SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5
> sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support
> DPO or FUA
> sda: sda1 sda2 sda3 sda4
> sd 0:0:0:0: [sda] Attached SCSI disk
> ata2: SATA link down (SStatus 0 SControl 300)
> random: crng init done
>
> As you can see octeon_i2c_driver_init never returns. Are you able to test on
> one of your MIPS-based systems?
CC'ing Steven who might be able to test on MIPS.
> Thanks,
> Paul
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-09 14:09 ` Paul Burton
2016-11-09 14:09 ` Paul Burton
@ 2016-11-09 14:43 ` Jan Glauber
2016-11-09 14:43 ` Jan Glauber
1 sibling, 1 reply; 27+ messages in thread
From: Jan Glauber @ 2016-11-09 14:43 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Wolfram Sang, stable
On Wed, Nov 09, 2016 at 02:09:38PM +0000, Paul Burton wrote:
> On Tuesday, 8 November 2016 08:13:37 GMT Jan Glauber wrote:
> > On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > > retries") attempted to replace potentially infinite loops with ones
> > > which will time out using readq_poll_timeout, but in doing so it
> > > inverted the condition for exiting this loop.
> > >
> > > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > >
> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Jan Glauber <jglauber@cavium.com>
> > > Cc: David Daney <david.daney@cavium.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > Cc: linux-i2c@vger.kernel.org
> > >
> > > ---
> >
> > Acked-by: Jan Glauber <jglauber@cavium.com>
> >
> > Thanks for spotting this. I think this should go into stable too for
> > 4.8, so adding Cc: stable@vger.kernel.org.
>
> Hi Jan,
>
> ...but the bad patch was only merged for v4.9-rc1?
true, I've misread it.
> Thanks,
> Paul
>
> >
> > > drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-octeon-core.h
> > > b/drivers/i2c/busses/i2c-octeon-core.h index 1db7c83..d980406 100644
> > > --- a/drivers/i2c/busses/i2c-octeon-core.h
> > > +++ b/drivers/i2c/busses/i2c-octeon-core.h
> > > @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct
> > > octeon_i2c *i2c, u64 eop_reg, u8>
> > > __raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base +
> SW_TWSI(i2c));
> > >
> > > - readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp &
> SW_TWSI_V,
> > > - I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
> > > + readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > > + i2c->adap.timeout);
> > >
> > > }
> > >
> > > #define octeon_i2c_ctl_write(i2c, val) \
> > >
> > > @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct
> > > octeon_i2c *i2c, u64 eop_reg,>
> > > __raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base +
> > > SW_TWSI(i2c));
> > >
> > > ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > >
> > > - tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> > > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > >
> > > i2c->adap.timeout);
> > >
> > > if (error)
> > >
> > > *error = ret;
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-09 14:43 ` Jan Glauber
@ 2016-11-09 14:43 ` Jan Glauber
0 siblings, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-09 14:43 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-i2c, linux-mips, David Daney, Wolfram Sang, stable
On Wed, Nov 09, 2016 at 02:09:38PM +0000, Paul Burton wrote:
> On Tuesday, 8 November 2016 08:13:37 GMT Jan Glauber wrote:
> > On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > > retries") attempted to replace potentially infinite loops with ones
> > > which will time out using readq_poll_timeout, but in doing so it
> > > inverted the condition for exiting this loop.
> > >
> > > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > >
> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Jan Glauber <jglauber@cavium.com>
> > > Cc: David Daney <david.daney@cavium.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > Cc: linux-i2c@vger.kernel.org
> > >
> > > ---
> >
> > Acked-by: Jan Glauber <jglauber@cavium.com>
> >
> > Thanks for spotting this. I think this should go into stable too for
> > 4.8, so adding Cc: stable@vger.kernel.org.
>
> Hi Jan,
>
> ...but the bad patch was only merged for v4.9-rc1?
true, I've misread it.
> Thanks,
> Paul
>
> >
> > > drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-octeon-core.h
> > > b/drivers/i2c/busses/i2c-octeon-core.h index 1db7c83..d980406 100644
> > > --- a/drivers/i2c/busses/i2c-octeon-core.h
> > > +++ b/drivers/i2c/busses/i2c-octeon-core.h
> > > @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct
> > > octeon_i2c *i2c, u64 eop_reg, u8>
> > > __raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base +
> SW_TWSI(i2c));
> > >
> > > - readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp &
> SW_TWSI_V,
> > > - I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
> > > + readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > > + i2c->adap.timeout);
> > >
> > > }
> > >
> > > #define octeon_i2c_ctl_write(i2c, val) \
> > >
> > > @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct
> > > octeon_i2c *i2c, u64 eop_reg,>
> > > __raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base +
> > > SW_TWSI(i2c));
> > >
> > > ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > >
> > > - tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> > > + !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > >
> > > i2c->adap.timeout);
> > >
> > > if (error)
> > >
> > > *error = ret;
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-08 7:13 ` [PATCH 1/2] i2c: octeon: Fix register access Jan Glauber
2016-11-08 7:13 ` Jan Glauber
2016-11-09 14:09 ` Paul Burton
@ 2016-11-10 20:17 ` Wolfram Sang
2016-11-11 7:00 ` Jan Glauber
2 siblings, 1 reply; 27+ messages in thread
From: Wolfram Sang @ 2016-11-10 20:17 UTC (permalink / raw)
To: Jan Glauber; +Cc: Paul Burton, linux-i2c, linux-mips, David Daney, stable
[-- Attachment #1: Type: text/plain, Size: 980 bytes --]
On Tue, Nov 08, 2016 at 08:13:37AM +0100, Jan Glauber wrote:
> On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > retries") attempted to replace potentially infinite loops with ones
> > which will time out using readq_poll_timeout, but in doing so it
> > inverted the condition for exiting this loop.
> >
> > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> >
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Jan Glauber <jglauber@cavium.com>
> > Cc: David Daney <david.daney@cavium.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-i2c@vger.kernel.org
> >
> > ---
>
> Acked-by: Jan Glauber <jglauber@cavium.com>
>
> Thanks for spotting this. I think this should go into stable too for
> 4.8, so adding Cc: stable@vger.kernel.org.
Shall I still apply this one? Seems to me that 70121f7f3725 is still
under discussion of being reverted?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-10 20:17 ` Wolfram Sang
@ 2016-11-11 7:00 ` Jan Glauber
2016-11-11 7:00 ` Jan Glauber
0 siblings, 1 reply; 27+ messages in thread
From: Jan Glauber @ 2016-11-11 7:00 UTC (permalink / raw)
To: Wolfram Sang
Cc: Paul Burton, linux-i2c, linux-mips, David Daney, Steven J. Hill
On Thu, Nov 10, 2016 at 09:17:31PM +0100, Wolfram Sang wrote:
> On Tue, Nov 08, 2016 at 08:13:37AM +0100, Jan Glauber wrote:
> > On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > > retries") attempted to replace potentially infinite loops with ones
> > > which will time out using readq_poll_timeout, but in doing so it
> > > inverted the condition for exiting this loop.
> > >
> > > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > >
> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Jan Glauber <jglauber@cavium.com>
> > > Cc: David Daney <david.daney@cavium.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > Cc: linux-i2c@vger.kernel.org
> > >
> > > ---
> >
> > Acked-by: Jan Glauber <jglauber@cavium.com>
> >
> > Thanks for spotting this. I think this should go into stable too for
> > 4.8, so adding Cc: stable@vger.kernel.org.
>
> Shall I still apply this one? Seems to me that 70121f7f3725 is still
> under discussion of being reverted?
>
70121f7 needs to be reverted, but it looks like i2c devices are still
not working on Octeon after that. We are still debugging the issue,
so I would suggest to wait for some more days.
--Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] i2c: octeon: Fix register access
2016-11-11 7:00 ` Jan Glauber
@ 2016-11-11 7:00 ` Jan Glauber
0 siblings, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-11 7:00 UTC (permalink / raw)
To: Wolfram Sang
Cc: Paul Burton, linux-i2c, linux-mips, David Daney, Steven J. Hill
On Thu, Nov 10, 2016 at 09:17:31PM +0100, Wolfram Sang wrote:
> On Tue, Nov 08, 2016 at 08:13:37AM +0100, Jan Glauber wrote:
> > On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > > retries") attempted to replace potentially infinite loops with ones
> > > which will time out using readq_poll_timeout, but in doing so it
> > > inverted the condition for exiting this loop.
> > >
> > > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > >
> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Jan Glauber <jglauber@cavium.com>
> > > Cc: David Daney <david.daney@cavium.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > Cc: linux-i2c@vger.kernel.org
> > >
> > > ---
> >
> > Acked-by: Jan Glauber <jglauber@cavium.com>
> >
> > Thanks for spotting this. I think this should go into stable too for
> > 4.8, so adding Cc: stable@vger.kernel.org.
>
> Shall I still apply this one? Seems to me that 70121f7f3725 is still
> under discussion of being reverted?
>
70121f7 needs to be reverted, but it looks like i2c devices are still
not working on Octeon after that. We are still debugging the issue,
so I would suggest to wait for some more days.
--Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-09 14:07 ` Paul Burton
2016-11-09 14:07 ` Paul Burton
2016-11-09 14:38 ` Jan Glauber
@ 2016-11-11 8:57 ` Jan Glauber
2016-11-11 8:57 ` Jan Glauber
2016-11-11 20:51 ` Steven J. Hill
2 siblings, 2 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-11 8:57 UTC (permalink / raw)
To: Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang,
Steven J. Hill
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
> As you can see octeon_i2c_driver_init never returns. Are you able to test on
> one of your MIPS-based systems?
>
> Thanks,
> Paul
Hi Paul,
we can reproduce the problem on our side, but I don't have direct access
to a MIPS system so I'm still just guessing what happens. If you want
you can try the attached patches.
I'm trying to get rid of the polling around the interrupt altogether.
It works fine on Thunderx, sometimes I run into an interrupt timeout
after a lost-arbitration but recovery/retry is able to clean that up.
Please also revert 70121f7 as before. The last patch adds some debugging
output to see if we run into timeouts or recovery.
thanks,
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-11 8:57 ` Jan Glauber
@ 2016-11-11 8:57 ` Jan Glauber
2016-11-11 20:51 ` Steven J. Hill
1 sibling, 0 replies; 27+ messages in thread
From: Jan Glauber @ 2016-11-11 8:57 UTC (permalink / raw)
To: Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang,
Steven J. Hill
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
> As you can see octeon_i2c_driver_init never returns. Are you able to test on
> one of your MIPS-based systems?
>
> Thanks,
> Paul
Hi Paul,
we can reproduce the problem on our side, but I don't have direct access
to a MIPS system so I'm still just guessing what happens. If you want
you can try the attached patches.
I'm trying to get rid of the polling around the interrupt altogether.
It works fine on Thunderx, sometimes I run into an interrupt timeout
after a lost-arbitration but recovery/retry is able to clean that up.
Please also revert 70121f7 as before. The last patch adds some debugging
output to see if we run into timeouts or recovery.
thanks,
Jan
[-- Attachment #2: 0001-i2c-octeon-thunderx-TWSI-software-reset-in-recovery.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-11 8:57 ` Jan Glauber
2016-11-11 8:57 ` Jan Glauber
@ 2016-11-11 20:51 ` Steven J. Hill
2016-11-11 20:51 ` Steven J. Hill
2016-11-11 22:11 ` David Daney
1 sibling, 2 replies; 27+ messages in thread
From: Steven J. Hill @ 2016-11-11 20:51 UTC (permalink / raw)
To: Jan Glauber, Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
On 11/11/2016 02:57 AM, Jan Glauber wrote:
>
> we can reproduce the problem on our side, but I don't have direct access
> to a MIPS system so I'm still just guessing what happens. If you want
> you can try the attached patches.
>
> I'm trying to get rid of the polling around the interrupt altogether.
> It works fine on Thunderx, sometimes I run into an interrupt timeout
> after a lost-arbitration but recovery/retry is able to clean that up.
>
> Please also revert 70121f7 as before. The last patch adds some debugging
> output to see if we run into timeouts or recovery.
>
Using your three patches without reverting 70121f7 fails on both 71xx
and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
eliminates the i2c probe hang and both boards boot properly. Tested
on top of Linus' v4.9-rc4 tag.
Steve
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-11 20:51 ` Steven J. Hill
@ 2016-11-11 20:51 ` Steven J. Hill
2016-11-11 22:11 ` David Daney
1 sibling, 0 replies; 27+ messages in thread
From: Steven J. Hill @ 2016-11-11 20:51 UTC (permalink / raw)
To: Jan Glauber, Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
On 11/11/2016 02:57 AM, Jan Glauber wrote:
>
> we can reproduce the problem on our side, but I don't have direct access
> to a MIPS system so I'm still just guessing what happens. If you want
> you can try the attached patches.
>
> I'm trying to get rid of the polling around the interrupt altogether.
> It works fine on Thunderx, sometimes I run into an interrupt timeout
> after a lost-arbitration but recovery/retry is able to clean that up.
>
> Please also revert 70121f7 as before. The last patch adds some debugging
> output to see if we run into timeouts or recovery.
>
Using your three patches without reverting 70121f7 fails on both 71xx
and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
eliminates the i2c probe hang and both boards boot properly. Tested
on top of Linus' v4.9-rc4 tag.
Steve
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-11 20:51 ` Steven J. Hill
2016-11-11 20:51 ` Steven J. Hill
@ 2016-11-11 22:11 ` David Daney
2016-11-11 22:11 ` David Daney
1 sibling, 1 reply; 27+ messages in thread
From: David Daney @ 2016-11-11 22:11 UTC (permalink / raw)
To: Steven J. Hill
Cc: Jan Glauber, Paul Burton, linux-i2c, linux-mips, David Daney,
Peter Swain, Wolfram Sang
On 11/11/2016 12:51 PM, Steven J. Hill wrote:
> On 11/11/2016 02:57 AM, Jan Glauber wrote:
>>
>> we can reproduce the problem on our side, but I don't have direct access
>> to a MIPS system so I'm still just guessing what happens. If you want
>> you can try the attached patches.
>>
>> I'm trying to get rid of the polling around the interrupt altogether.
>> It works fine on Thunderx, sometimes I run into an interrupt timeout
>> after a lost-arbitration but recovery/retry is able to clean that up.
>>
>> Please also revert 70121f7 as before. The last patch adds some debugging
>> output to see if we run into timeouts or recovery.
>>
> Using your three patches without reverting 70121f7 fails on both 71xx
> and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
> eliminates the i2c probe hang and both boards boot properly. Tested
> on top of Linus' v4.9-rc4 tag.
>
We need a definitive set of patches to apply. If Paul's patches are not
sufficient, work with Jan to create and test a set (perhaps including
reversions) that will make it work on both OCTEON and Thunder, noting
any dependencies on Paul's patches. Then get that posted ASAP so that
I2C can work again
David.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
2016-11-11 22:11 ` David Daney
@ 2016-11-11 22:11 ` David Daney
0 siblings, 0 replies; 27+ messages in thread
From: David Daney @ 2016-11-11 22:11 UTC (permalink / raw)
To: Steven J. Hill
Cc: Jan Glauber, Paul Burton, linux-i2c, linux-mips, David Daney,
Peter Swain, Wolfram Sang
On 11/11/2016 12:51 PM, Steven J. Hill wrote:
> On 11/11/2016 02:57 AM, Jan Glauber wrote:
>>
>> we can reproduce the problem on our side, but I don't have direct access
>> to a MIPS system so I'm still just guessing what happens. If you want
>> you can try the attached patches.
>>
>> I'm trying to get rid of the polling around the interrupt altogether.
>> It works fine on Thunderx, sometimes I run into an interrupt timeout
>> after a lost-arbitration but recovery/retry is able to clean that up.
>>
>> Please also revert 70121f7 as before. The last patch adds some debugging
>> output to see if we run into timeouts or recovery.
>>
> Using your three patches without reverting 70121f7 fails on both 71xx
> and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
> eliminates the i2c probe hang and both boards boot properly. Tested
> on top of Linus' v4.9-rc4 tag.
>
We need a definitive set of patches to apply. If Paul's patches are not
sufficient, work with Jan to create and test a set (perhaps including
reversions) that will make it work on both OCTEON and Thunder, noting
any dependencies on Paul's patches. Then get that posted ASAP so that
I2C can work again
David.
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-11-11 22:11 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-07 20:09 [PATCH 1/2] i2c: octeon: Fix register access Paul Burton
2016-11-07 20:09 ` Paul Burton
2016-11-07 20:09 ` [PATCH 2/2] i2c: octeon: Fix waiting for operation completion Paul Burton
2016-11-07 20:09 ` Paul Burton
2016-11-08 9:20 ` Jan Glauber
2016-11-08 9:20 ` Jan Glauber
2016-11-09 13:41 ` Jan Glauber
2016-11-09 13:41 ` Jan Glauber
2016-11-09 14:07 ` Paul Burton
2016-11-09 14:07 ` Paul Burton
2016-11-09 14:38 ` Jan Glauber
2016-11-09 14:38 ` Jan Glauber
2016-11-11 8:57 ` Jan Glauber
2016-11-11 8:57 ` Jan Glauber
2016-11-11 20:51 ` Steven J. Hill
2016-11-11 20:51 ` Steven J. Hill
2016-11-11 22:11 ` David Daney
2016-11-11 22:11 ` David Daney
2016-11-08 7:13 ` [PATCH 1/2] i2c: octeon: Fix register access Jan Glauber
2016-11-08 7:13 ` Jan Glauber
2016-11-09 14:09 ` Paul Burton
2016-11-09 14:09 ` Paul Burton
2016-11-09 14:43 ` Jan Glauber
2016-11-09 14:43 ` Jan Glauber
2016-11-10 20:17 ` Wolfram Sang
2016-11-11 7:00 ` Jan Glauber
2016-11-11 7:00 ` Jan Glauber
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).