[PATCH v7] i2c: riic: Implement bus recovery

[PATCH v7] i2c: riic: Implement bus recovery

[Prabhakar]

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

Implement bus recovery by reinitializing the hardware to reset the bus
state and generating 9 clock cycles (and a stop condition) to release
the SDA line.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Resending this patch which was part of v6 [0] series.

[0] https://lore.kernel.org/lkml/Z4ZCJYPgvS0Ke39g@shikoro/T/

Hi Wolfram,

Ive replied to your comments on v2 here [1] as to why the generic
recovery algorithm was not used.

[1] https://lore.kernel.org/all/CA+V-a8s4-g9vxyfYMgnKMK=Oej9kDBwWsWehWLYTkxw-06w-2g@mail.gmail.com/

Cheers,
Prabhakar

v6->v7
- None

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

v1->v2
- Used single register read to check SDA/SCL lines
---
 drivers/i2c/busses/i2c-riic.c | 100 ++++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index d7dddd6c296a..888825423d94 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -51,6 +51,7 @@
 
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
+#define ICCR1_CLO	BIT(5)
 #define ICCR1_SOWP	BIT(4)
 #define ICCR1_SCLI	BIT(1)
 #define ICCR1_SDAI	BIT(0)
@@ -69,6 +70,7 @@
 #define ICMR3_ACKBT	BIT(3)
 
 #define ICFER_FMPE	BIT(7)
+#define ICFER_MALE	BIT(1)
 
 #define ICIER_TIE	BIT(7)
 #define ICIER_TEIE	BIT(6)
@@ -82,6 +84,8 @@
 
 #define RIIC_INIT_MSG	-1
 
+#define RIIC_RECOVERY_CLK_CNT	9
+
 enum riic_reg_list {
 	RIIC_ICCR1 = 0,
 	RIIC_ICCR2,
@@ -151,13 +155,16 @@ 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;
+		goto i2c_recover;
 
 	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
 	     (ICCR1_SDAI | ICCR1_SCLI))
-		return -EBUSY;
+		goto i2c_recover;
 
 	return 0;
+
+i2c_recover:
+	return i2c_recover_bus(&riic->adapter);
 }
 
 static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
@@ -332,7 +339,7 @@ static const struct i2c_algorithm riic_algo = {
 	.functionality = riic_func,
 };
 
-static int riic_init_hw(struct riic_dev *riic)
+static int riic_init_hw(struct riic_dev *riic, bool recover)
 {
 	int ret;
 	unsigned long rate;
@@ -414,9 +421,11 @@ static int riic_init_hw(struct riic_dev *riic)
 		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
 		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
 
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret)
-		return ret;
+	if (!recover) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret)
+			return ret;
+	}
 
 	/* Changing the order of accessing IICRST and ICE may break things! */
 	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
@@ -434,8 +443,74 @@ static int riic_init_hw(struct riic_dev *riic)
 
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
-	pm_runtime_mark_last_busy(dev);
-	pm_runtime_put_autosuspend(dev);
+	if (!recover) {
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
+	return 0;
+}
+
+static int riic_recover_bus(struct i2c_adapter *adap)
+{
+	struct riic_dev *riic = i2c_get_adapdata(adap);
+	struct device *dev = riic->adapter.dev.parent;
+	int ret;
+	u8 val;
+
+	ret = riic_init_hw(riic, true);
+	if (ret)
+		return ret;
+
+	/* output extra SCL clock cycles with master arbitration-lost detection disabled */
+	riic_clear_set_bit(riic, ICFER_MALE, 0, RIIC_ICFER);
+
+	for (unsigned int i = 0; i < RIIC_RECOVERY_CLK_CNT; i++) {
+		riic_clear_set_bit(riic, 0, ICCR1_CLO, RIIC_ICCR1);
+		ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR1], val,
+					 !(val & ICCR1_CLO), 0, 100);
+		if (ret) {
+			dev_err(dev, "SCL clock cycle timeout\n");
+			return ret;
+		}
+	}
+
+	/*
+	 * The last clock cycle may have driven the SDA line high, so add a
+	 * short delay to allow the line to stabilize before checking the status.
+	 */
+	udelay(5);
+
+	/*
+	 * If an incomplete byte write occurs, the SDA line may remain low
+	 * even after 9 clock pulses, indicating the bus is not released.
+	 * To resolve this, send an additional clock pulse to simulate a STOP
+	 * condition and ensure proper bus release.
+	 */
+	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
+	    (ICCR1_SDAI | ICCR1_SCLI)) {
+		riic_clear_set_bit(riic, 0, ICCR1_CLO, RIIC_ICCR1);
+		ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR1], val,
+					 !(val & ICCR1_CLO), 0, 100);
+		if (ret) {
+			dev_err(dev, "SCL clock cycle timeout occurred while issuing the STOP condition\n");
+			return ret;
+		}
+		/* delay to make sure SDA line goes back HIGH again */
+		udelay(5);
+	}
+
+	/* clear any flags set */
+	riic_writeb(riic, 0, RIIC_ICSR2);
+	/* read back register to confirm writes */
+	riic_readb(riic, RIIC_ICSR2);
+
+	/* restore back ICFER_MALE */
+	riic_clear_set_bit(riic, 0, ICFER_MALE, RIIC_ICFER);
+
+	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
+	    (ICCR1_SDAI | ICCR1_SCLI))
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -447,6 +522,10 @@ static const struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
 };
 
+static struct i2c_bus_recovery_info riic_bri = {
+	.recover_bus = riic_recover_bus,
+};
+
 static int riic_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -493,6 +572,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
 	strscpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
 	adap->owner = THIS_MODULE;
 	adap->algo = &riic_algo;
+	adap->bus_recovery_info = &riic_bri;
 	adap->dev.parent = dev;
 	adap->dev.of_node = dev->of_node;
 
@@ -505,7 +585,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
-	ret = riic_init_hw(riic);
+	ret = riic_init_hw(riic, false);
 	if (ret)
 		goto out;
 
@@ -613,7 +693,7 @@ static int riic_i2c_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = riic_init_hw(riic);
+	ret = riic_init_hw(riic, false);
 	if (ret) {
 		/*
 		 * In case this happens there is no way to recover from this
-- 
2.43.0

Re: [PATCH v7] i2c: riic: Implement bus recovery

[Lad, Prabhakar]

Hi Wolfram,

On Mon, Feb 3, 2025 at 2:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Implement bus recovery by reinitializing the hardware to reset the bus
> state and generating 9 clock cycles (and a stop condition) to release
> the SDA line.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> Resending this patch which was part of v6 [0] series.
>
> [0] https://lore.kernel.org/lkml/Z4ZCJYPgvS0Ke39g@shikoro/T/
>
> Hi Wolfram,
>
> Ive replied to your comments on v2 here [1] as to why the generic
> recovery algorithm was not used.
>
> [1] https://lore.kernel.org/all/CA+V-a8s4-g9vxyfYMgnKMK=Oej9kDBwWsWehWLYTkxw-06w-2g@mail.gmail.com/
>
> Cheers,
> Prabhakar
>
> v6->v7
> - None
>
> v2->v6
> - Included RB and TB from Claudiu.
>
> v1->v2
> - Used single register read to check SDA/SCL lines
> ---
>  drivers/i2c/busses/i2c-riic.c | 100 ++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 10 deletions(-)
>
Gentle ping.

Cheers,
Prabhakar

RE: [PATCH v7] i2c: riic: Implement bus recovery

[Fabrizio Castro]

> From: Prabhakar <prabhakar.csengg@gmail.com>
> Sent: 03 February 2025 14:35
> Subject: [PATCH v7] i2c: riic: Implement bus recovery
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement bus recovery by reinitializing the hardware to reset the bus
> state and generating 9 clock cycles (and a stop condition) to release
> the SDA line.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

> ---
> Resending this patch which was part of v6 [0] series.
> 
> [0] https://lore.kernel.org/lkml/Z4ZCJYPgvS0Ke39g@shikoro/T/
> 
> Hi Wolfram,
> 
> Ive replied to your comments on v2 here [1] as to why the generic
> recovery algorithm was not used.
> 
> [1] https://lore.kernel.org/all/CA+V-a8s4-g9vxyfYMgnKMK=Oej9kDBwWsWehWLYTkxw-06w-2g@mail.gmail.com/
> 
> Cheers,
> Prabhakar
> 
> v6->v7
> - None
> 
> v2->v6
> - Included RB and TB from Claudiu.
> 
> v1->v2
> - Used single register read to check SDA/SCL lines
> ---
>  drivers/i2c/busses/i2c-riic.c | 100 ++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index d7dddd6c296a..888825423d94 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -51,6 +51,7 @@
> 
>  #define ICCR1_ICE	BIT(7)
>  #define ICCR1_IICRST	BIT(6)
> +#define ICCR1_CLO	BIT(5)
>  #define ICCR1_SOWP	BIT(4)
>  #define ICCR1_SCLI	BIT(1)
>  #define ICCR1_SDAI	BIT(0)
> @@ -69,6 +70,7 @@
>  #define ICMR3_ACKBT	BIT(3)
> 
>  #define ICFER_FMPE	BIT(7)
> +#define ICFER_MALE	BIT(1)
> 
>  #define ICIER_TIE	BIT(7)
>  #define ICIER_TEIE	BIT(6)
> @@ -82,6 +84,8 @@
> 
>  #define RIIC_INIT_MSG	-1
> 
> +#define RIIC_RECOVERY_CLK_CNT	9
> +
>  enum riic_reg_list {
>  	RIIC_ICCR1 = 0,
>  	RIIC_ICCR2,
> @@ -151,13 +155,16 @@ 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;
> +		goto i2c_recover;
> 
>  	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
>  	     (ICCR1_SDAI | ICCR1_SCLI))
> -		return -EBUSY;
> +		goto i2c_recover;
> 
>  	return 0;
> +
> +i2c_recover:
> +	return i2c_recover_bus(&riic->adapter);
>  }
> 
>  static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> @@ -332,7 +339,7 @@ static const struct i2c_algorithm riic_algo = {
>  	.functionality = riic_func,
>  };
> 
> -static int riic_init_hw(struct riic_dev *riic)
> +static int riic_init_hw(struct riic_dev *riic, bool recover)
>  {
>  	int ret;
>  	unsigned long rate;
> @@ -414,9 +421,11 @@ static int riic_init_hw(struct riic_dev *riic)
>  		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
>  		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
> 
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		return ret;
> +	if (!recover) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;
> +	}
> 
>  	/* Changing the order of accessing IICRST and ICE may break things! */
>  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
> @@ -434,8 +443,74 @@ static int riic_init_hw(struct riic_dev *riic)
> 
>  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> 
> -	pm_runtime_mark_last_busy(dev);
> -	pm_runtime_put_autosuspend(dev);
> +	if (!recover) {
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +	}
> +	return 0;
> +}
> +
> +static int riic_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct riic_dev *riic = i2c_get_adapdata(adap);
> +	struct device *dev = riic->adapter.dev.parent;
> +	int ret;
> +	u8 val;
> +
> +	ret = riic_init_hw(riic, true);
> +	if (ret)
> +		return ret;
> +
> +	/* output extra SCL clock cycles with master arbitration-lost detection disabled */
> +	riic_clear_set_bit(riic, ICFER_MALE, 0, RIIC_ICFER);
> +
> +	for (unsigned int i = 0; i < RIIC_RECOVERY_CLK_CNT; i++) {
> +		riic_clear_set_bit(riic, 0, ICCR1_CLO, RIIC_ICCR1);
> +		ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR1], val,
> +					 !(val & ICCR1_CLO), 0, 100);
> +		if (ret) {
> +			dev_err(dev, "SCL clock cycle timeout\n");
> +			return ret;
> +		}
> +	}
> +
> +	/*
> +	 * The last clock cycle may have driven the SDA line high, so add a
> +	 * short delay to allow the line to stabilize before checking the status.
> +	 */
> +	udelay(5);
> +
> +	/*
> +	 * If an incomplete byte write occurs, the SDA line may remain low
> +	 * even after 9 clock pulses, indicating the bus is not released.
> +	 * To resolve this, send an additional clock pulse to simulate a STOP
> +	 * condition and ensure proper bus release.
> +	 */
> +	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
> +	    (ICCR1_SDAI | ICCR1_SCLI)) {
> +		riic_clear_set_bit(riic, 0, ICCR1_CLO, RIIC_ICCR1);
> +		ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR1], val,
> +					 !(val & ICCR1_CLO), 0, 100);
> +		if (ret) {
> +			dev_err(dev, "SCL clock cycle timeout occurred while issuing the STOP condition\n");
> +			return ret;
> +		}
> +		/* delay to make sure SDA line goes back HIGH again */
> +		udelay(5);
> +	}
> +
> +	/* clear any flags set */
> +	riic_writeb(riic, 0, RIIC_ICSR2);
> +	/* read back register to confirm writes */
> +	riic_readb(riic, RIIC_ICSR2);
> +
> +	/* restore back ICFER_MALE */
> +	riic_clear_set_bit(riic, 0, ICFER_MALE, RIIC_ICFER);
> +
> +	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
> +	    (ICCR1_SDAI | ICCR1_SCLI))
> +		return -EINVAL;
> +
>  	return 0;
>  }
> 
> @@ -447,6 +522,10 @@ static const struct riic_irq_desc riic_irqs[] = {
>  	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
>  };
> 
> +static struct i2c_bus_recovery_info riic_bri = {
> +	.recover_bus = riic_recover_bus,
> +};
> +
>  static int riic_i2c_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -493,6 +572,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  	strscpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
>  	adap->owner = THIS_MODULE;
>  	adap->algo = &riic_algo;
> +	adap->bus_recovery_info = &riic_bri;
>  	adap->dev.parent = dev;
>  	adap->dev.of_node = dev->of_node;
> 
> @@ -505,7 +585,7 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_enable(dev);
> 
> -	ret = riic_init_hw(riic);
> +	ret = riic_init_hw(riic, false);
>  	if (ret)
>  		goto out;
> 
> @@ -613,7 +693,7 @@ static int riic_i2c_resume(struct device *dev)
>  	if (ret)
>  		return ret;
> 
> -	ret = riic_init_hw(riic);
> +	ret = riic_init_hw(riic, false);
>  	if (ret) {
>  		/*
>  		 * In case this happens there is no way to recover from this
> --
> 2.43.0

Re: [PATCH v7] i2c: riic: Implement bus recovery

[Lad, Prabhakar]

Hi Wolfram

On Tue, Feb 18, 2025 at 9:07 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Wolfram,
>
> On Mon, Feb 3, 2025 at 2:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Implement bus recovery by reinitializing the hardware to reset the bus
> > state and generating 9 clock cycles (and a stop condition) to release
> > the SDA line.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> > Resending this patch which was part of v6 [0] series.
> >
> > [0] https://lore.kernel.org/lkml/Z4ZCJYPgvS0Ke39g@shikoro/T/
> >
> > Hi Wolfram,
> >
> > Ive replied to your comments on v2 here [1] as to why the generic
> > recovery algorithm was not used.
> >
> > [1] https://lore.kernel.org/all/CA+V-a8s4-g9vxyfYMgnKMK=Oej9kDBwWsWehWLYTkxw-06w-2g@mail.gmail.com/
> >
> > Cheers,
> > Prabhakar
> >
> > v6->v7
> > - None
> >
> > v2->v6
> > - Included RB and TB from Claudiu.
> >
> > v1->v2
> > - Used single register read to check SDA/SCL lines
> > ---
> >  drivers/i2c/busses/i2c-riic.c | 100 ++++++++++++++++++++++++++++++----
> >  1 file changed, 90 insertions(+), 10 deletions(-)
> >
Gentle ping.

Cheers,
Prabhakar

Re: [PATCH v7] i2c: riic: Implement bus recovery

[Andi Shyti]

Hi Lad,

On Mon, Feb 03, 2025 at 02:35:11PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement bus recovery by reinitializing the hardware to reset the bus
> state and generating 9 clock cycles (and a stop condition) to release
> the SDA line.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

I think you forgot here:

Reviewed-by: Andy Shevchenko <andy@kernel.org>

he sent his review in patch 0 on your v6.

Wolfram, if you don't have anymore concerns here, I can take this
in.

Andi

Re: [PATCH v7] i2c: riic: Implement bus recovery

[Wolfram Sang]

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

> Wolfram, if you don't have anymore concerns here, I can take this
> in.

Please wait for my still outstanding review. I am sorry for the delay.


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