* [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
@ 2023-09-04 9:00 ` Jonas Gorski
0 siblings, 0 replies; 9+ messages in thread
From: Jonas Gorski @ 2023-09-04 9:00 UTC (permalink / raw)
To: Andi Shyti, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Wolfram Sang,
Kevin Cernekee
Cc: linux-i2c, linux-arm-kernel, linux-kernel
If a transaction times out, the START_BUSY signal may have gotten stuck,
and subsequent transactaction attempts will fail as the bus is still
considered busy.
To work around this, check if the START_BUSY bit is still asserted, and
reset the controller in case it is.
This is also done by the alternative, non-upstream iproc-smbus driver
implementation [1].
Works around situations like:
bcm-iproc-2c 1803b000.i2c: transaction timed out
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
...
where the bus never recovers after a timeout.
[1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
---
The iproc-smbus driver does some additional checks/mitigations, but
since my I2C understanding is only very rudimentary, I didn't add them,
also the reset was enough to fix the issue I was seeing.
I was a bit conflicted about the Fixes tag, but since it fixes/work
around misbehaviour seen I decided to add one.
The issue was happening only in production, and only once per boot (so
far), but with 100% probability within a few hours.
drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 05c80680dff4..69f9c199fa3b 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
if (!time_left && !iproc_i2c->xfer_is_done) {
dev_err(iproc_i2c->device, "transaction timed out\n");
+ /* check if START_BUSY did not clear */
+ if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
+ BIT(M_CMD_START_BUSY_SHIFT))) {
+ /* re-initialize i2c for recovery */
+ bcm_iproc_i2c_enable_disable(iproc_i2c, false);
+ bcm_iproc_i2c_init(iproc_i2c);
+ bcm_iproc_i2c_enable_disable(iproc_i2c, true);
+ }
+
/* flush both TX/RX FIFOs */
val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
--
2.42.0
--
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany
Phone:
+49-30-6108-1-6100
Managing Directors:
Dr.-Ing. Hagen Woesner, Andreas
Köpsel
Commercial register:
Amtsgericht Berlin-Charlottenburg HRB 141569
B
VAT ID No: DE283257294
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
@ 2023-09-04 9:00 ` Jonas Gorski
0 siblings, 0 replies; 9+ messages in thread
From: Jonas Gorski @ 2023-09-04 9:00 UTC (permalink / raw)
To: Andi Shyti, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Wolfram Sang,
Kevin Cernekee
Cc: linux-i2c, linux-arm-kernel, linux-kernel
If a transaction times out, the START_BUSY signal may have gotten stuck,
and subsequent transactaction attempts will fail as the bus is still
considered busy.
To work around this, check if the START_BUSY bit is still asserted, and
reset the controller in case it is.
This is also done by the alternative, non-upstream iproc-smbus driver
implementation [1].
Works around situations like:
bcm-iproc-2c 1803b000.i2c: transaction timed out
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
bcm-iproc-2c 1803b000.i2c: bus is busy
...
where the bus never recovers after a timeout.
[1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
---
The iproc-smbus driver does some additional checks/mitigations, but
since my I2C understanding is only very rudimentary, I didn't add them,
also the reset was enough to fix the issue I was seeing.
I was a bit conflicted about the Fixes tag, but since it fixes/work
around misbehaviour seen I decided to add one.
The issue was happening only in production, and only once per boot (so
far), but with 100% probability within a few hours.
drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 05c80680dff4..69f9c199fa3b 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
if (!time_left && !iproc_i2c->xfer_is_done) {
dev_err(iproc_i2c->device, "transaction timed out\n");
+ /* check if START_BUSY did not clear */
+ if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
+ BIT(M_CMD_START_BUSY_SHIFT))) {
+ /* re-initialize i2c for recovery */
+ bcm_iproc_i2c_enable_disable(iproc_i2c, false);
+ bcm_iproc_i2c_init(iproc_i2c);
+ bcm_iproc_i2c_enable_disable(iproc_i2c, true);
+ }
+
/* flush both TX/RX FIFOs */
val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
--
2.42.0
--
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany
Phone:
+49-30-6108-1-6100
Managing Directors:
Dr.-Ing. Hagen Woesner, Andreas
Köpsel
Commercial register:
Amtsgericht Berlin-Charlottenburg HRB 141569
B
VAT ID No: DE283257294
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
2023-09-04 9:00 ` Jonas Gorski
(?)
@ 2023-09-05 16:26 ` Ray Jui
-1 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2023-09-05 16:26 UTC (permalink / raw)
To: Jonas Gorski, Andi Shyti, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Wolfram Sang,
Kevin Cernekee
Cc: linux-i2c, linux-arm-kernel, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2762 bytes --]
Hi Jonas,
On 9/4/2023 2:00 AM, Jonas Gorski wrote:
> If a transaction times out, the START_BUSY signal may have gotten stuck,
> and subsequent transactaction attempts will fail as the bus is still
> considered busy.
>
> To work around this, check if the START_BUSY bit is still asserted, and
> reset the controller in case it is.
>
> This is also done by the alternative, non-upstream iproc-smbus driver
> implementation [1].
>
> Works around situations like:
>
> bcm-iproc-2c 1803b000.i2c: transaction timed out
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> ...
>
> where the bus never recovers after a timeout.
>
> [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
>
> Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
> ---
> The iproc-smbus driver does some additional checks/mitigations, but
> since my I2C understanding is only very rudimentary, I didn't add them,
> also the reset was enough to fix the issue I was seeing.
>
> I was a bit conflicted about the Fixes tag, but since it fixes/work
> around misbehaviour seen I decided to add one.
>
> The issue was happening only in production, and only once per boot (so
> far), but with 100% probability within a few hours.
>
> drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 05c80680dff4..69f9c199fa3b 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> if (!time_left && !iproc_i2c->xfer_is_done) {
> dev_err(iproc_i2c->device, "transaction timed out\n");
>
> + /* check if START_BUSY did not clear */
Can you please rephrase the comment to make it more clear?
For example, something like this:
/*
* If START_BUSY is still not clear, it means the controller may have
* been stuck. In this case, reset the controller to recover.
*/
> + if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
> + BIT(M_CMD_START_BUSY_SHIFT))) {
> + /* re-initialize i2c for recovery */
> + bcm_iproc_i2c_enable_disable(iproc_i2c, false);
> + bcm_iproc_i2c_init(iproc_i2c);
> + bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> + }
> +
> /* flush both TX/RX FIFOs */
> val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
> iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
@ 2023-09-05 16:26 ` Ray Jui
0 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2023-09-05 16:26 UTC (permalink / raw)
To: Jonas Gorski, Andi Shyti, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Wolfram Sang,
Kevin Cernekee
Cc: linux-i2c, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]
Hi Jonas,
On 9/4/2023 2:00 AM, Jonas Gorski wrote:
> If a transaction times out, the START_BUSY signal may have gotten stuck,
> and subsequent transactaction attempts will fail as the bus is still
> considered busy.
>
> To work around this, check if the START_BUSY bit is still asserted, and
> reset the controller in case it is.
>
> This is also done by the alternative, non-upstream iproc-smbus driver
> implementation [1].
>
> Works around situations like:
>
> bcm-iproc-2c 1803b000.i2c: transaction timed out
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> ...
>
> where the bus never recovers after a timeout.
>
> [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
>
> Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
> ---
> The iproc-smbus driver does some additional checks/mitigations, but
> since my I2C understanding is only very rudimentary, I didn't add them,
> also the reset was enough to fix the issue I was seeing.
>
> I was a bit conflicted about the Fixes tag, but since it fixes/work
> around misbehaviour seen I decided to add one.
>
> The issue was happening only in production, and only once per boot (so
> far), but with 100% probability within a few hours.
>
> drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 05c80680dff4..69f9c199fa3b 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> if (!time_left && !iproc_i2c->xfer_is_done) {
> dev_err(iproc_i2c->device, "transaction timed out\n");
>
> + /* check if START_BUSY did not clear */
Can you please rephrase the comment to make it more clear?
For example, something like this:
/*
* If START_BUSY is still not clear, it means the controller may have
* been stuck. In this case, reset the controller to recover.
*/
> + if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
> + BIT(M_CMD_START_BUSY_SHIFT))) {
> + /* re-initialize i2c for recovery */
> + bcm_iproc_i2c_enable_disable(iproc_i2c, false);
> + bcm_iproc_i2c_init(iproc_i2c);
> + bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> + }
> +
> /* flush both TX/RX FIFOs */
> val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
> iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
@ 2023-09-05 16:26 ` Ray Jui
0 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2023-09-05 16:26 UTC (permalink / raw)
To: Jonas Gorski, Andi Shyti, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Wolfram Sang,
Kevin Cernekee
Cc: linux-i2c, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]
Hi Jonas,
On 9/4/2023 2:00 AM, Jonas Gorski wrote:
> If a transaction times out, the START_BUSY signal may have gotten stuck,
> and subsequent transactaction attempts will fail as the bus is still
> considered busy.
>
> To work around this, check if the START_BUSY bit is still asserted, and
> reset the controller in case it is.
>
> This is also done by the alternative, non-upstream iproc-smbus driver
> implementation [1].
>
> Works around situations like:
>
> bcm-iproc-2c 1803b000.i2c: transaction timed out
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> ...
>
> where the bus never recovers after a timeout.
>
> [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
>
> Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
> ---
> The iproc-smbus driver does some additional checks/mitigations, but
> since my I2C understanding is only very rudimentary, I didn't add them,
> also the reset was enough to fix the issue I was seeing.
>
> I was a bit conflicted about the Fixes tag, but since it fixes/work
> around misbehaviour seen I decided to add one.
>
> The issue was happening only in production, and only once per boot (so
> far), but with 100% probability within a few hours.
>
> drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 05c80680dff4..69f9c199fa3b 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> if (!time_left && !iproc_i2c->xfer_is_done) {
> dev_err(iproc_i2c->device, "transaction timed out\n");
>
> + /* check if START_BUSY did not clear */
Can you please rephrase the comment to make it more clear?
For example, something like this:
/*
* If START_BUSY is still not clear, it means the controller may have
* been stuck. In this case, reset the controller to recover.
*/
> + if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
> + BIT(M_CMD_START_BUSY_SHIFT))) {
> + /* re-initialize i2c for recovery */
> + bcm_iproc_i2c_enable_disable(iproc_i2c, false);
> + bcm_iproc_i2c_init(iproc_i2c);
> + bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> + }
> +
> /* flush both TX/RX FIFOs */
> val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
> iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
2023-09-04 9:00 ` Jonas Gorski
@ 2023-09-05 22:53 ` Andi Shyti
-1 siblings, 0 replies; 9+ messages in thread
From: Andi Shyti @ 2023-09-05 22:53 UTC (permalink / raw)
To: Jonas Gorski
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Wolfram Sang, Kevin Cernekee, linux-i2c, linux-arm-kernel,
linux-kernel
Hi Jonas,
On Mon, Sep 04, 2023 at 11:00:04AM +0200, Jonas Gorski wrote:
> If a transaction times out, the START_BUSY signal may have gotten stuck,
> and subsequent transactaction attempts will fail as the bus is still
> considered busy.
>
> To work around this, check if the START_BUSY bit is still asserted, and
> reset the controller in case it is.
>
> This is also done by the alternative, non-upstream iproc-smbus driver
> implementation [1].
>
> Works around situations like:
>
> bcm-iproc-2c 1803b000.i2c: transaction timed out
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> ...
>
> where the bus never recovers after a timeout.
>
> [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
>
> Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
I think the right Fixes tag should be:
Fixes: 3f98ad45e585 ("i2c: iproc: add polling support")
Cc: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Cc: <stable@vger.kernel.org> # v5.2+
> ---
> The iproc-smbus driver does some additional checks/mitigations, but
> since my I2C understanding is only very rudimentary, I didn't add them,
> also the reset was enough to fix the issue I was seeing.
>
> I was a bit conflicted about the Fixes tag, but since it fixes/work
> around misbehaviour seen I decided to add one.
>
> The issue was happening only in production, and only once per boot (so
> far), but with 100% probability within a few hours.
>
> drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 05c80680dff4..69f9c199fa3b 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> if (!time_left && !iproc_i2c->xfer_is_done) {
> dev_err(iproc_i2c->device, "transaction timed out\n");
>
> + /* check if START_BUSY did not clear */
as Ray asked, can you please expand this comment?
Thanks,
Andi
> + if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
> + BIT(M_CMD_START_BUSY_SHIFT))) {
> + /* re-initialize i2c for recovery */
> + bcm_iproc_i2c_enable_disable(iproc_i2c, false);
> + bcm_iproc_i2c_init(iproc_i2c);
> + bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> + }
> +
> /* flush both TX/RX FIFOs */
> val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
> iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
> --
> 2.42.0
>
>
> --
> BISDN GmbH
> Körnerstraße 7-10
> 10785 Berlin
> Germany
>
>
> Phone:
> +49-30-6108-1-6100
>
>
> Managing Directors:
> Dr.-Ing. Hagen Woesner, Andreas
> Köpsel
>
>
> Commercial register:
> Amtsgericht Berlin-Charlottenburg HRB 141569
> B
> VAT ID No: DE283257294
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
@ 2023-09-05 22:53 ` Andi Shyti
0 siblings, 0 replies; 9+ messages in thread
From: Andi Shyti @ 2023-09-05 22:53 UTC (permalink / raw)
To: Jonas Gorski
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Wolfram Sang, Kevin Cernekee, linux-i2c, linux-arm-kernel,
linux-kernel
Hi Jonas,
On Mon, Sep 04, 2023 at 11:00:04AM +0200, Jonas Gorski wrote:
> If a transaction times out, the START_BUSY signal may have gotten stuck,
> and subsequent transactaction attempts will fail as the bus is still
> considered busy.
>
> To work around this, check if the START_BUSY bit is still asserted, and
> reset the controller in case it is.
>
> This is also done by the alternative, non-upstream iproc-smbus driver
> implementation [1].
>
> Works around situations like:
>
> bcm-iproc-2c 1803b000.i2c: transaction timed out
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> bcm-iproc-2c 1803b000.i2c: bus is busy
> ...
>
> where the bus never recovers after a timeout.
>
> [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
>
> Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
I think the right Fixes tag should be:
Fixes: 3f98ad45e585 ("i2c: iproc: add polling support")
Cc: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Cc: <stable@vger.kernel.org> # v5.2+
> ---
> The iproc-smbus driver does some additional checks/mitigations, but
> since my I2C understanding is only very rudimentary, I didn't add them,
> also the reset was enough to fix the issue I was seeing.
>
> I was a bit conflicted about the Fixes tag, but since it fixes/work
> around misbehaviour seen I decided to add one.
>
> The issue was happening only in production, and only once per boot (so
> far), but with 100% probability within a few hours.
>
> drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 05c80680dff4..69f9c199fa3b 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> if (!time_left && !iproc_i2c->xfer_is_done) {
> dev_err(iproc_i2c->device, "transaction timed out\n");
>
> + /* check if START_BUSY did not clear */
as Ray asked, can you please expand this comment?
Thanks,
Andi
> + if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
> + BIT(M_CMD_START_BUSY_SHIFT))) {
> + /* re-initialize i2c for recovery */
> + bcm_iproc_i2c_enable_disable(iproc_i2c, false);
> + bcm_iproc_i2c_init(iproc_i2c);
> + bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> + }
> +
> /* flush both TX/RX FIFOs */
> val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
> iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
> --
> 2.42.0
>
>
> --
> BISDN GmbH
> Körnerstraße 7-10
> 10785 Berlin
> Germany
>
>
> Phone:
> +49-30-6108-1-6100
>
>
> Managing Directors:
> Dr.-Ing. Hagen Woesner, Andreas
> Köpsel
>
>
> Commercial register:
> Amtsgericht Berlin-Charlottenburg HRB 141569
> B
> VAT ID No: DE283257294
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
2023-09-05 22:53 ` Andi Shyti
@ 2023-09-08 14:03 ` Jonas Gorski
-1 siblings, 0 replies; 9+ messages in thread
From: Jonas Gorski @ 2023-09-08 14:03 UTC (permalink / raw)
To: Andi Shyti
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Wolfram Sang, Kevin Cernekee, linux-i2c, linux-arm-kernel,
linux-kernel
Hi,
Am Mi., 6. Sept. 2023 um 00:53 Uhr schrieb Andi Shyti <andi.shyti@kernel.org>:
>
> Hi Jonas,
>
> On Mon, Sep 04, 2023 at 11:00:04AM +0200, Jonas Gorski wrote:
> > If a transaction times out, the START_BUSY signal may have gotten stuck,
> > and subsequent transactaction attempts will fail as the bus is still
> > considered busy.
> >
> > To work around this, check if the START_BUSY bit is still asserted, and
> > reset the controller in case it is.
> >
> > This is also done by the alternative, non-upstream iproc-smbus driver
> > implementation [1].
> >
> > Works around situations like:
> >
> > bcm-iproc-2c 1803b000.i2c: transaction timed out
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > ...
> >
> > where the bus never recovers after a timeout.
> >
> > [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
> >
> > Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> > Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
>
> I think the right Fixes tag should be:
>
> Fixes: 3f98ad45e585 ("i2c: iproc: add polling support")
That was the last change that part of the code, but the "issue" was
not introduced there. The code before that already did a timeout check
and flush in that case, without the reset.
Obviously the fix wouldn't apply without changes to a version without
that commit, but the version would be nevertheless affected by the
issue. That's why I chose the commit introducing the timeout handling.
> Cc: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> Cc: <stable@vger.kernel.org> # v5.2+
>
> > ---
> > The iproc-smbus driver does some additional checks/mitigations, but
> > since my I2C understanding is only very rudimentary, I didn't add them,
> > also the reset was enough to fix the issue I was seeing.
> >
> > I was a bit conflicted about the Fixes tag, but since it fixes/work
> > around misbehaviour seen I decided to add one.
> >
> > The issue was happening only in production, and only once per boot (so
> > far), but with 100% probability within a few hours.
> >
> > drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> > index 05c80680dff4..69f9c199fa3b 100644
> > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> > @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> > if (!time_left && !iproc_i2c->xfer_is_done) {
> > dev_err(iproc_i2c->device, "transaction timed out\n");
> >
> > + /* check if START_BUSY did not clear */
>
> as Ray asked, can you please expand this comment?
Will do, thanks for the reviews!
Best Regards,
Jonas
--
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany
Phone:
+49-30-6108-1-6100
Managing Directors:
Dr.-Ing. Hagen Woesner, Andreas
Köpsel
Commercial register:
Amtsgericht Berlin-Charlottenburg HRB 141569
B
VAT ID No: DE283257294
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck
@ 2023-09-08 14:03 ` Jonas Gorski
0 siblings, 0 replies; 9+ messages in thread
From: Jonas Gorski @ 2023-09-08 14:03 UTC (permalink / raw)
To: Andi Shyti
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Wolfram Sang, Kevin Cernekee, linux-i2c, linux-arm-kernel,
linux-kernel
Hi,
Am Mi., 6. Sept. 2023 um 00:53 Uhr schrieb Andi Shyti <andi.shyti@kernel.org>:
>
> Hi Jonas,
>
> On Mon, Sep 04, 2023 at 11:00:04AM +0200, Jonas Gorski wrote:
> > If a transaction times out, the START_BUSY signal may have gotten stuck,
> > and subsequent transactaction attempts will fail as the bus is still
> > considered busy.
> >
> > To work around this, check if the START_BUSY bit is still asserted, and
> > reset the controller in case it is.
> >
> > This is also done by the alternative, non-upstream iproc-smbus driver
> > implementation [1].
> >
> > Works around situations like:
> >
> > bcm-iproc-2c 1803b000.i2c: transaction timed out
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > bcm-iproc-2c 1803b000.i2c: bus is busy
> > ...
> >
> > where the bus never recovers after a timeout.
> >
> > [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
> >
> > Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> > Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
>
> I think the right Fixes tag should be:
>
> Fixes: 3f98ad45e585 ("i2c: iproc: add polling support")
That was the last change that part of the code, but the "issue" was
not introduced there. The code before that already did a timeout check
and flush in that case, without the reset.
Obviously the fix wouldn't apply without changes to a version without
that commit, but the version would be nevertheless affected by the
issue. That's why I chose the commit introducing the timeout handling.
> Cc: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> Cc: <stable@vger.kernel.org> # v5.2+
>
> > ---
> > The iproc-smbus driver does some additional checks/mitigations, but
> > since my I2C understanding is only very rudimentary, I didn't add them,
> > also the reset was enough to fix the issue I was seeing.
> >
> > I was a bit conflicted about the Fixes tag, but since it fixes/work
> > around misbehaviour seen I decided to add one.
> >
> > The issue was happening only in production, and only once per boot (so
> > far), but with 100% probability within a few hours.
> >
> > drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> > index 05c80680dff4..69f9c199fa3b 100644
> > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> > @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> > if (!time_left && !iproc_i2c->xfer_is_done) {
> > dev_err(iproc_i2c->device, "transaction timed out\n");
> >
> > + /* check if START_BUSY did not clear */
>
> as Ray asked, can you please expand this comment?
Will do, thanks for the reviews!
Best Regards,
Jonas
--
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany
Phone:
+49-30-6108-1-6100
Managing Directors:
Dr.-Ing. Hagen Woesner, Andreas
Köpsel
Commercial register:
Amtsgericht Berlin-Charlottenburg HRB 141569
B
VAT ID No: DE283257294
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-09-08 14:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-04 9:00 [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck Jonas Gorski
2023-09-04 9:00 ` Jonas Gorski
2023-09-05 16:26 ` Ray Jui
2023-09-05 16:26 ` Ray Jui
2023-09-05 16:26 ` Ray Jui
2023-09-05 22:53 ` Andi Shyti
2023-09-05 22:53 ` Andi Shyti
2023-09-08 14:03 ` Jonas Gorski
2023-09-08 14:03 ` Jonas Gorski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.