diff mbox series

[v9,2/2] i2c: riic: Recover from arbitration loss

Message ID 20250430194647.332553-3-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series i2c: riic: Implement bus recovery | expand

Commit Message

Prabhakar April 30, 2025, 7:46 p.m. UTC
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add support for detecting arbitration loss in the RIIC controller. For
certain slave devices, it was observed that after I2C recovery, the
transmission triggered an arbitration loss. To handle this, initiate
the I2C recovery sequence and retry the transfer.

This ensures reliable communication in scenarios where arbitration
loss may occur after recovery.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/i2c/busses/i2c-riic.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Prabhakar April 30, 2025, 8:29 p.m. UTC | #1
Hi Wolfram,

Thank you for the quick review.

On Wed, Apr 30, 2025 at 9:17 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > Add support for detecting arbitration loss in the RIIC controller. For
> > certain slave devices, it was observed that after I2C recovery, the
> > transmission triggered an arbitration loss. To handle this, initiate
> > the I2C recovery sequence and retry the transfer.
>
> Does it maybe work even without triggering recovery again? A pure
> arbitration loss should not need a recovery procedure.
>
Do you mean that upon detecting an arbitration loss, we simply clear
the arbitration bit and retry?

However, when observing the SDA line after recovery, it goes LOW again
during the transfer. I've attached a screenshot of this case: we
recovered from a bus hang, the I2C recovery algorithm brought the bus
to a STOP state, and then a START condition was issued. But after
initiating the transfer, we can see the SDA line being held LOW again.

Cheers,
Prabhakar
Wolfram Sang May 1, 2025, 7:58 a.m. UTC | #2
> Do you mean that upon detecting an arbitration loss, we simply clear
> the arbitration bit and retry?

Yes, after the bus is considered free again.

> However, when observing the SDA line after recovery, it goes LOW again
> during the transfer. I've attached a screenshot of this case: we
> recovered from a bus hang, the I2C recovery algorithm brought the bus
> to a STOP state, and then a START condition was issued. But after
> initiating the transfer, we can see the SDA line being held LOW again.

That looks weird. Why are there two SDA transitions around 30us? Why is
SDA changed while SCL is high around 45us? Then, this small SCL spike
around 55us... What device is this?
Prabhakar May 1, 2025, 9:02 a.m. UTC | #3
Hi Wolfram,

On Thu, May 1, 2025 at 8:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > Do you mean that upon detecting an arbitration loss, we simply clear
> > the arbitration bit and retry?
>
> Yes, after the bus is considered free again.
>
I'll give that a try but in my case the SDA line has gone low.

> > However, when observing the SDA line after recovery, it goes LOW again
> > during the transfer. I've attached a screenshot of this case: we
> > recovered from a bus hang, the I2C recovery algorithm brought the bus
> > to a STOP state, and then a START condition was issued. But after
> > initiating the transfer, we can see the SDA line being held LOW again.
>
> That looks weird. Why are there two SDA transitions around 30us? Why is
> SDA changed while SCL is high around 45us? Then, this small SCL spike
> around 55us... What device is this?
>
Wolfram Sang May 1, 2025, 7:44 p.m. UTC | #4
> From 10µs to 50µs, the clock pulses are part of the recovery sequence.

Ahh, that explains. I thought this was all after the recovery.

> Around 55µs, the transfer function starts attempting to send data
> hence the clock pulse.

The short SCL spike around 55us is still strange. However, we might
violate t:buf time between STOP and START. Can you please try the
attached WIP patch?

> The slave device is versa clock geberator 5P35023 (exact part number
> on SMARC RZ/G2L 5P35023B-629NLGI)

Hmm, G3S has a versa clock generator as well. But I can't find a way to
wire GPIO lines to RIIC1 or I2C_PM.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7ad1ad5c8c3f..42058c789f3f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -278,6 +278,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		}
 	}
 
+	ndelay(RECOVERY_NDELAY / 2);
+
 	/* If we can't check bus status, assume recovery worked */
 	if (ret == -EOPNOTSUPP)
 		ret = 0;
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 740e53bdb2a9..86404d2df244 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -79,6 +79,7 @@ 
 #define ICIER_SPIE	BIT(3)
 
 #define ICSR2_NACKF	BIT(4)
+#define ICSR2_AL	BIT(1)
 
 #define ICBR_RESERVED	GENMASK(7, 5) /* Should be 1 on writes */
 
@@ -180,6 +181,7 @@  static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	reinit_completion(&riic->msg_done);
 
+retry:
 	riic_writeb(riic, 0, RIIC_ICSR2);
 
 	for (i = 0, start_bit = ICCR2_ST; i < num; i++) {
@@ -193,8 +195,14 @@  static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		riic_writeb(riic, start_bit, RIIC_ICCR2);
 
 		time_left = wait_for_completion_timeout(&riic->msg_done, riic->adapter.timeout);
-		if (time_left == 0)
+		if (time_left == 0) {
+			if (riic_readb(riic, RIIC_ICSR2) & ICSR2_AL) {
+				ret = i2c_recover_bus(&riic->adapter);
+				if (!ret)
+					goto retry;
+			}
 			riic->err = -ETIMEDOUT;
+		}
 
 		if (riic->err)
 			break;