| Message ID | 20250430194647.332553-3-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: riic: Implement bus recovery | expand |
> 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.
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
> 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?
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? >
> 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;
Hi Wolfram, On Thu, May 1, 2025 at 8:44 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > > 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? > I'm seeing the same behaviour as seen previously (attached is the capture). > > 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. > Yes, same here apart from soldering the pins there seems to be no option. Cheers, Prabhakar
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;