[PATCH v9 0/2] i2c: riic: Implement bus recovery

[PATCH v9 0/2] i2c: riic: Implement bus recovery

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi all,

This patch series implements bus recovery for the RIIC driver. The first
patch implements the bus recovery mechanism, while the second patch
handles arbitration loss.

v8>->v9:
- Dropped Tested-by and Reviewed-by tags from patch 1/2
- Updated commit message for patch 1/2 to include the new approach
- New patch 2/2 added to handle arbitration loss

v7->v8:
- Included Acks from Andy and Fabrizio.

v6->v7:
- https://lore.kernel.org/all/20250203143511.629140-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

v2->v6:
- Included RB and TB from Claudiu.

v1->v2:
- Used single register read to check SDA/SCL lines

Cheers,
Prabhakar

Lad Prabhakar (2):
  i2c: riic: Implement bus recovery
  i2c: riic: Recover from arbitration loss

 drivers/i2c/busses/i2c-riic.c | 63 +++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 3 deletions(-)

-- 
2.49.0

[PATCH v9 1/2] i2c: riic: Implement bus recovery

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Implement I2C bus recovery support for the RIIC controller by making use
of software-controlled SCL and SDA line manipulation. The controller allows
forcing SCL and SDA levels through control bits, which enables generation
of manual clock pulses and a stop condition to free a stuck bus.

This implementation wires up the bus recovery mechanism using
i2c_generic_scl_recovery and provides get/set operations for SCL and SDA.

This allows the RIIC driver to recover from bus hang scenarios where SDA
is held low by a slave.

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

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index d7dddd6c296a..740e53bdb2a9 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -53,6 +53,8 @@
 #define ICCR1_IICRST	BIT(6)
 #define ICCR1_SOWP	BIT(4)
 #define ICCR1_SCLI	BIT(1)
+#define ICCR1_SCLO     BIT(3)
+#define ICCR1_SDAO     BIT(2)
 #define ICCR1_SDAI	BIT(0)
 
 #define ICCR2_BBSY	BIT(7)
@@ -151,11 +153,11 @@ static int riic_bus_barrier(struct riic_dev *riic)
 	ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
 				 !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
 	if (ret)
-		return ret;
+		return i2c_recover_bus(&riic->adapter);
 
 	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
 	     (ICCR1_SDAI | ICCR1_SCLI))
-		return -EBUSY;
+		return i2c_recover_bus(&riic->adapter);
 
 	return 0;
 }
@@ -439,6 +441,52 @@ static int riic_init_hw(struct riic_dev *riic)
 	return 0;
 }
 
+static int riic_get_scl(struct i2c_adapter *adap)
+{
+	struct riic_dev *riic = i2c_get_adapdata(adap);
+
+	return !!(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI);
+}
+
+static int riic_get_sda(struct i2c_adapter *adap)
+{
+	struct riic_dev *riic = i2c_get_adapdata(adap);
+
+	return !!(riic_readb(riic, RIIC_ICCR1) & ICCR1_SDAI);
+}
+
+static void riic_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct riic_dev *riic = i2c_get_adapdata(adap);
+
+	if (val)
+		riic_clear_set_bit(riic, ICCR1_SOWP, ICCR1_SCLO, RIIC_ICCR1);
+	else
+		riic_clear_set_bit(riic, ICCR1_SOWP | ICCR1_SCLO, 0, RIIC_ICCR1);
+
+	riic_clear_set_bit(riic, 0, ICCR1_SOWP, RIIC_ICCR1);
+}
+
+static void riic_set_sda(struct i2c_adapter *adap, int val)
+{
+	struct riic_dev *riic = i2c_get_adapdata(adap);
+
+	if (val)
+		riic_clear_set_bit(riic, ICCR1_SOWP, ICCR1_SDAO, RIIC_ICCR1);
+	else
+		riic_clear_set_bit(riic, ICCR1_SOWP | ICCR1_SDAO, 0, RIIC_ICCR1);
+
+	riic_clear_set_bit(riic, 0, ICCR1_SOWP, RIIC_ICCR1);
+}
+
+static struct i2c_bus_recovery_info riic_bri = {
+	.recover_bus = i2c_generic_scl_recovery,
+	.get_scl = riic_get_scl,
+	.set_scl = riic_set_scl,
+	.get_sda = riic_get_sda,
+	.set_sda = riic_set_sda,
+};
+
 static const struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
 	{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
@@ -495,6 +543,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
 	adap->algo = &riic_algo;
 	adap->dev.parent = dev;
 	adap->dev.of_node = dev->of_node;
+	adap->bus_recovery_info = &riic_bri;
 
 	init_completion(&riic->msg_done);
 
-- 
2.49.0

Re: [PATCH v9 1/2] i2c: riic: Implement bus recovery

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On Wed, Apr 30, 2025 at 08:46:46PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement I2C bus recovery support for the RIIC controller by making use
> of software-controlled SCL and SDA line manipulation. The controller allows
> forcing SCL and SDA levels through control bits, which enables generation
> of manual clock pulses and a stop condition to free a stuck bus.
> 
> This implementation wires up the bus recovery mechanism using
> i2c_generic_scl_recovery and provides get/set operations for SCL and SDA.
> 
> This allows the RIIC driver to recover from bus hang scenarios where SDA
> is held low by a slave.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v9 1/2] i2c: riic: Implement bus recovery

[Fabrizio Castro]

> From: Prabhakar <prabhakar.csengg@gmail.com>
> Sent: 30 April 2025 20:47
> Subject: [PATCH v9 1/2] i2c: riic: Implement bus recovery
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement I2C bus recovery support for the RIIC controller by making use
> of software-controlled SCL and SDA line manipulation. The controller allows
> forcing SCL and SDA levels through control bits, which enables generation
> of manual clock pulses and a stop condition to free a stuck bus.
> 
> This implementation wires up the bus recovery mechanism using
> i2c_generic_scl_recovery and provides get/set operations for SCL and SDA.
> 
> This allows the RIIC driver to recover from bus hang scenarios where SDA
> is held low by a slave.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

> ---
>  drivers/i2c/busses/i2c-riic.c | 53 +++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index d7dddd6c296a..740e53bdb2a9 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -53,6 +53,8 @@
>  #define ICCR1_IICRST	BIT(6)
>  #define ICCR1_SOWP	BIT(4)
>  #define ICCR1_SCLI	BIT(1)
> +#define ICCR1_SCLO     BIT(3)
> +#define ICCR1_SDAO     BIT(2)
>  #define ICCR1_SDAI	BIT(0)
> 
>  #define ICCR2_BBSY	BIT(7)
> @@ -151,11 +153,11 @@ static int riic_bus_barrier(struct riic_dev *riic)
>  	ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
>  				 !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
>  	if (ret)
> -		return ret;
> +		return i2c_recover_bus(&riic->adapter);
> 
>  	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
>  	     (ICCR1_SDAI | ICCR1_SCLI))
> -		return -EBUSY;
> +		return i2c_recover_bus(&riic->adapter);
> 
>  	return 0;
>  }
> @@ -439,6 +441,52 @@ static int riic_init_hw(struct riic_dev *riic)
>  	return 0;
>  }
> 
> +static int riic_get_scl(struct i2c_adapter *adap)
> +{
> +	struct riic_dev *riic = i2c_get_adapdata(adap);
> +
> +	return !!(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI);
> +}
> +
> +static int riic_get_sda(struct i2c_adapter *adap)
> +{
> +	struct riic_dev *riic = i2c_get_adapdata(adap);
> +
> +	return !!(riic_readb(riic, RIIC_ICCR1) & ICCR1_SDAI);
> +}
> +
> +static void riic_set_scl(struct i2c_adapter *adap, int val)
> +{
> +	struct riic_dev *riic = i2c_get_adapdata(adap);
> +
> +	if (val)
> +		riic_clear_set_bit(riic, ICCR1_SOWP, ICCR1_SCLO, RIIC_ICCR1);
> +	else
> +		riic_clear_set_bit(riic, ICCR1_SOWP | ICCR1_SCLO, 0, RIIC_ICCR1);
> +
> +	riic_clear_set_bit(riic, 0, ICCR1_SOWP, RIIC_ICCR1);
> +}
> +
> +static void riic_set_sda(struct i2c_adapter *adap, int val)
> +{
> +	struct riic_dev *riic = i2c_get_adapdata(adap);
> +
> +	if (val)
> +		riic_clear_set_bit(riic, ICCR1_SOWP, ICCR1_SDAO, RIIC_ICCR1);
> +	else
> +		riic_clear_set_bit(riic, ICCR1_SOWP | ICCR1_SDAO, 0, RIIC_ICCR1);
> +
> +	riic_clear_set_bit(riic, 0, ICCR1_SOWP, RIIC_ICCR1);
> +}
> +
> +static struct i2c_bus_recovery_info riic_bri = {
> +	.recover_bus = i2c_generic_scl_recovery,
> +	.get_scl = riic_get_scl,
> +	.set_scl = riic_set_scl,
> +	.get_sda = riic_get_sda,
> +	.set_sda = riic_set_sda,
> +};
> +
>  static const struct riic_irq_desc riic_irqs[] = {
>  	{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
>  	{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
> @@ -495,6 +543,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  	adap->algo = &riic_algo;
>  	adap->dev.parent = dev;
>  	adap->dev.of_node = dev->of_node;
> +	adap->bus_recovery_info = &riic_bri;
> 
>  	init_completion(&riic->msg_done);
> 
> --
> 2.49.0

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

[Prabhakar]

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(-)

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;
-- 
2.49.0

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Lad, Prabhakar]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

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

[-- Attachment #2: arbitration.png --]
[-- Type: image/png, Size: 127067 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]


> 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Lad, Prabhakar]

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.
The SDA line is likely being toggled by the slave around 30µs, after
two clock pulses. At 45µs, we are still within the recovery algorithm
-- SCL is set to 1, followed by SDA. The recovery algorithm then
checks if SCL is high and whether the bus is free (i.e., SDA is also
high). At that point, i2c_generic_scl_recovery() returns, assuming the
bus has been successfully recovered.
Around 55µs, the transfer function starts attempting to send data
hence the clock pulse.

The slave device is versa clock geberator 5P35023 (exact part number
on SMARC RZ/G2L 5P35023B-629NLGI)
https://www.renesas.com/en/products/clocks-timing/clock-generation/programmable-clocks/5p35023-versaclock-3s-programmable-clock-generator?srsltid=AfmBOoqlLSt_ul3hLh7NHYlCShXsnH-QZf90uSdoxZXI_Pre5Qg7soD6#overview

Cheers,
Prabhaka

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

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 626 bytes --]


> 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.


[-- Attachment #1.2: buf --]
[-- Type: text/plain, Size: 410 bytes --]

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;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Lad, Prabhakar]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

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

[-- Attachment #2: Screenshot (379).png --]
[-- Type: image/png, Size: 124051 bytes --]