[PATCH v2 0/9] Add support for I2C bus recovery for riic driver

[PATCH v2 0/9] Add support for I2C bus recovery for riic driver

[Prabhakar]

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

Hi All,

This patch series introduces support for I2C bus recovery in the RIIC
driver, which is utilized in RZ series SoCs. The addition of bus recovery
functionality enhances the reliability of the I2C interface by allowing it
to recover from error conditions that might leave the bus in an unusable
state.

Alongside the bus recovery implementation, the series includes several
cleanup and improvement patches that simplify and modernize the driver
code. These include replacing `dev_err` calls with `dev_err_probe`,
consistent usage of the `BIT` and `GENMASK` macros, leveraging devres
helpers for reset management, and improving code readability by marking
static data as `const`.

v1->v2
- Fixed review comments and collected RB tags from Geert

v1:
https://lore.kernel.org/all/20241213175828.909987-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (9):
  i2c: riic: Replace dev_err with dev_err_probe in probe function
  i2c: riic: Use local `dev` pointer in `dev_err_probe()`
  i2c: riic: Use BIT macro consistently
  i2c: riic: Use GENMASK() macro for bitmask definitions
  i2c: riic: Make use of devres helper to request deasserted reset line
  i2c: riic: Mark riic_irqs array as const
  i2c: riic: Use predefined macro and simplify clock tick calculation
  i2c: riic: Add `riic_bus_barrier()` to check bus availability
  i2c: riic: Implement bus recovery

 drivers/i2c/busses/i2c-riic.c | 222 ++++++++++++++++++++++++----------
 1 file changed, 155 insertions(+), 67 deletions(-)

-- 
2.43.0

[PATCH v2 1/9] i2c: riic: Replace dev_err with dev_err_probe in probe function

[Prabhakar]

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

Refactor error handling in the riic_i2c_probe() function by replacing
multiple dev_err() calls with dev_err_probe().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 9264adc97ca9..bfaa2d728a76 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -445,10 +445,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(riic->base);
 
 	riic->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(riic->clk)) {
-		dev_err(dev, "missing controller clock");
-		return PTR_ERR(riic->clk);
-	}
+	if (IS_ERR(riic->clk))
+		return dev_err_probe(dev, PTR_ERR(riic->clk),
+				     "missing controller clock");
 
 	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
 	if (IS_ERR(riic->rstc))
@@ -470,10 +469,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
 				       0, riic_irqs[i].name, riic);
-		if (ret) {
-			dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request irq %s\n",
+					     riic_irqs[i].name);
 	}
 
 	riic->info = of_device_get_match_data(dev);
-- 
2.43.0

Re: [PATCH v2 1/9] i2c: riic: Replace dev_err with dev_err_probe in probe function

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:10AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Refactor error handling in the riic_i2c_probe() function by replacing
> multiple dev_err() calls with dev_err_probe().
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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


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

[PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Prabhakar]

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

Update the `riic_init_hw()` function to use the local `dev` pointer in
calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
`riic->adapter.dev` in error reporting. Since this function is invoked
during the probe phase, the I2C adapter is not yet initialized, leading to
`(null) ...` being printed in error messages. This patch fixes the issue by
consistently using the local `dev` pointer, which points to
`riic->adapter.dev.parent`.

Additionally, replace `dev_err()` with `dev_err_probe()` throughout
`riic_init_hw()` for consistency.

Fixes: d982d66514192 ("i2c: riic: remove clock and frequency restrictions")
Fixes: 71dacb2565ed (i2c: riic: Simplify unsupported bus speed handling")
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index bfaa2d728a76..01195ffd4c07 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
 				      : I2C_MAX_FAST_MODE_FREQ;
 
 	if (t->bus_freq_hz > max_freq)
-		return dev_err_probe(&riic->adapter.dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "unsupported bus speed %uHz (%u max)\n",
 				     t->bus_freq_hz, max_freq);
 
@@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
 		rate /= 2;
 	}
 
-	if (brl > (0x1F + 3)) {
-		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
-			(unsigned long)t->bus_freq_hz);
-		return -EINVAL;
-	}
+	if (brl > (0x1F + 3))
+		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
+				     (unsigned long)t->bus_freq_hz);
 
 	brh = total_ticks - brl;
 
-- 
2.43.0

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Andi Shyti]

Hi Prabhakar,

> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index bfaa2d728a76..01195ffd4c07 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
>  				      : I2C_MAX_FAST_MODE_FREQ;
>  
>  	if (t->bus_freq_hz > max_freq)
> -		return dev_err_probe(&riic->adapter.dev, -EINVAL,
> +		return dev_err_probe(dev, -EINVAL,
>  				     "unsupported bus speed %uHz (%u max)\n",
>  				     t->bus_freq_hz, max_freq);

Which branch are you on? This change has already been introduced
in commit 71dacb2565ed ("i2c: riic: Simplify unsupported bus
speed handling")

>  
> @@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
>  		rate /= 2;
>  	}
>  
> -	if (brl > (0x1F + 3)) {
> -		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
> -			(unsigned long)t->bus_freq_hz);
> -		return -EINVAL;
> -	}
> +	if (brl > (0x1F + 3))
> +		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> +				     (unsigned long)t->bus_freq_hz);

This is OK

Thanks,
Andi

>  
>  	brh = total_ticks - brl;
>  
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Lad, Prabhakar]

Hi Andi,

On Thu, Dec 19, 2024 at 12:21 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Prabhakar,
>
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > index bfaa2d728a76..01195ffd4c07 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
> >                                     : I2C_MAX_FAST_MODE_FREQ;
> >
> >       if (t->bus_freq_hz > max_freq)
> > -             return dev_err_probe(&riic->adapter.dev, -EINVAL,
> > +             return dev_err_probe(dev, -EINVAL,
> >                                    "unsupported bus speed %uHz (%u max)\n",
> >                                    t->bus_freq_hz, max_freq);
>
> Which branch are you on? This change has already been introduced
> in commit 71dacb2565ed ("i2c: riic: Simplify unsupported bus
> speed handling")
>
I'm on v6.13-rc3, the above change just replaces the first parameter
in dev_err_probe(). The change introduced in commit 71dacb2565ed
("i2c: riic: Simplify unsupported bus speed handling") does not update
the first parameter in dev_err_probe() which this patch addresses.
Actually I have fixes tag for commit 71dacb2565ed in the current
patch.

Cheers,
Prabhakar

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Andi Shyti]

Hi Prabhakar,

On Thu, Dec 19, 2024 at 12:44:50PM +0000, Lad, Prabhakar wrote:
> Hi Andi,
> 
> On Thu, Dec 19, 2024 at 12:21 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> >
> > Hi Prabhakar,
> >
> > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > > index bfaa2d728a76..01195ffd4c07 100644
> > > --- a/drivers/i2c/busses/i2c-riic.c
> > > +++ b/drivers/i2c/busses/i2c-riic.c
> > > @@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
> > >                                     : I2C_MAX_FAST_MODE_FREQ;
> > >
> > >       if (t->bus_freq_hz > max_freq)
> > > -             return dev_err_probe(&riic->adapter.dev, -EINVAL,
> > > +             return dev_err_probe(dev, -EINVAL,
> > >                                    "unsupported bus speed %uHz (%u max)\n",
> > >                                    t->bus_freq_hz, max_freq);
> >
> > Which branch are you on? This change has already been introduced
> > in commit 71dacb2565ed ("i2c: riic: Simplify unsupported bus
> > speed handling")
> >
> I'm on v6.13-rc3, the above change just replaces the first parameter
> in dev_err_probe(). The change introduced in commit 71dacb2565ed
> ("i2c: riic: Simplify unsupported bus speed handling") does not update
> the first parameter in dev_err_probe() which this patch addresses.
> Actually I have fixes tag for commit 71dacb2565ed in the current
> patch.

oh, yes, sorry, ignore that :-)

Andi

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:11AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Update the `riic_init_hw()` function to use the local `dev` pointer in
> calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
> `riic->adapter.dev` in error reporting. Since this function is invoked
> during the probe phase, the I2C adapter is not yet initialized, leading to
> `(null) ...` being printed in error messages. This patch fixes the issue by
> consistently using the local `dev` pointer, which points to
> `riic->adapter.dev.parent`.
> 
> Additionally, replace `dev_err()` with `dev_err_probe()` throughout
> `riic_init_hw()` for consistency.
> 
> Fixes: d982d66514192 ("i2c: riic: remove clock and frequency restrictions")
> Fixes: 71dacb2565ed (i2c: riic: Simplify unsupported bus speed handling")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Maybe patches 1+2 can be squashed, but I leave this to Andi.

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


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

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Andi Shyti]

Hi Prabhakar,

On Wed, Dec 18, 2024 at 12:16:11AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Update the `riic_init_hw()` function to use the local `dev` pointer in
> calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
> `riic->adapter.dev` in error reporting. Since this function is invoked
> during the probe phase, the I2C adapter is not yet initialized, leading to
> `(null) ...` being printed in error messages. This patch fixes the issue by
> consistently using the local `dev` pointer, which points to
> `riic->adapter.dev.parent`.
> 
> Additionally, replace `dev_err()` with `dev_err_probe()` throughout
> `riic_init_hw()` for consistency.
> 
> Fixes: d982d66514192 ("i2c: riic: remove clock and frequency restrictions")
> Fixes: 71dacb2565ed (i2c: riic: Simplify unsupported bus speed handling")

I'm not sure the Fixes are really necessary here, as it's not
really leading to a bug, but I can live with it. But, then, ...

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v1->v2
> - Collected RB tag from Geert
> ---
>  drivers/i2c/busses/i2c-riic.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index bfaa2d728a76..01195ffd4c07 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
>  				      : I2C_MAX_FAST_MODE_FREQ;
>  
>  	if (t->bus_freq_hz > max_freq)
> -		return dev_err_probe(&riic->adapter.dev, -EINVAL,
> +		return dev_err_probe(dev, -EINVAL,
>  				     "unsupported bus speed %uHz (%u max)\n",
>  				     t->bus_freq_hz, max_freq);
>  
> @@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
>  		rate /= 2;
>  	}
>  
> -	if (brl > (0x1F + 3)) {
> -		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
> -			(unsigned long)t->bus_freq_hz);
> -		return -EINVAL;
> -	}
> +	if (brl > (0x1F + 3))
> +		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> +				     (unsigned long)t->bus_freq_hz);

... I'm not happy with the splitting here: mixing a bug fix with
a cosmetic is wrong for two reasons:

 - they are conceptually different;
 - fixes take are applied to the -fixes branch and sent to the
   weekly pull request.

I will appreciate if this second chunk is squashed with patch 1
and the first part has a patch on its own.

Thanks,
Andi

>  
>  	brh = total_ticks - brl;
>  
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Lad, Prabhakar]

Hi Andi,

On Thu, Dec 26, 2024 at 1:19 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, Dec 18, 2024 at 12:16:11AM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Update the `riic_init_hw()` function to use the local `dev` pointer in
> > calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
> > `riic->adapter.dev` in error reporting. Since this function is invoked
> > during the probe phase, the I2C adapter is not yet initialized, leading to
> > `(null) ...` being printed in error messages. This patch fixes the issue by
> > consistently using the local `dev` pointer, which points to
> > `riic->adapter.dev.parent`.
> >
> > Additionally, replace `dev_err()` with `dev_err_probe()` throughout
> > `riic_init_hw()` for consistency.
> >
> > Fixes: d982d66514192 ("i2c: riic: remove clock and frequency restrictions")
> > Fixes: 71dacb2565ed (i2c: riic: Simplify unsupported bus speed handling")
>
> I'm not sure the Fixes are really necessary here, as it's not
> really leading to a bug, but I can live with it. But, then, ...
>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v1->v2
> > - Collected RB tag from Geert
> > ---
> >  drivers/i2c/busses/i2c-riic.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > index bfaa2d728a76..01195ffd4c07 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
> >                                     : I2C_MAX_FAST_MODE_FREQ;
> >
> >       if (t->bus_freq_hz > max_freq)
> > -             return dev_err_probe(&riic->adapter.dev, -EINVAL,
> > +             return dev_err_probe(dev, -EINVAL,
> >                                    "unsupported bus speed %uHz (%u max)\n",
> >                                    t->bus_freq_hz, max_freq);
> >
> > @@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
> >               rate /= 2;
> >       }
> >
> > -     if (brl > (0x1F + 3)) {
> > -             dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
> > -                     (unsigned long)t->bus_freq_hz);
> > -             return -EINVAL;
> > -     }
> > +     if (brl > (0x1F + 3))
> > +             return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> > +                                  (unsigned long)t->bus_freq_hz);
>
> ... I'm not happy with the splitting here: mixing a bug fix with
> a cosmetic is wrong for two reasons:
>
>  - they are conceptually different;
>  - fixes take are applied to the -fixes branch and sent to the
>    weekly pull request.
>
> I will appreciate if this second chunk is squashed with patch 1
> and the first part has a patch on its own.
>
OK, I think the best approach here would be to promote this to patch
#1 ie just replacing `&riic->adapter.dev` with `dev` (as second chunk
also includes the fix along with cosmetic change) and then make patch
#2 as replacing `dev_err` with `dev_err_probe`.

Please let me know if this is OK with you.

Cheers,
Prabhakar

Re: [PATCH v2 2/9] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Andi Shyti]

Hi,

On Thu, Dec 26, 2024 at 07:39:33AM +0000, Lad, Prabhakar wrote:
> On Thu, Dec 26, 2024 at 1:19 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > On Wed, Dec 18, 2024 at 12:16:11AM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Update the `riic_init_hw()` function to use the local `dev` pointer in
> > > calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
> > > `riic->adapter.dev` in error reporting. Since this function is invoked
> > > during the probe phase, the I2C adapter is not yet initialized, leading to
> > > `(null) ...` being printed in error messages. This patch fixes the issue by
> > > consistently using the local `dev` pointer, which points to
> > > `riic->adapter.dev.parent`.
> > >
> > > Additionally, replace `dev_err()` with `dev_err_probe()` throughout
> > > `riic_init_hw()` for consistency.
> > >
> > > Fixes: d982d66514192 ("i2c: riic: remove clock and frequency restrictions")
> > > Fixes: 71dacb2565ed (i2c: riic: Simplify unsupported bus speed handling")
> >
> > I'm not sure the Fixes are really necessary here, as it's not
> > really leading to a bug, but I can live with it. But, then, ...

On a second thought, I think this Fixes tag is not needed: there
is no bug as the missing reference is already handled.

> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > v1->v2
> > > - Collected RB tag from Geert
> > > ---
> > >  drivers/i2c/busses/i2c-riic.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > > index bfaa2d728a76..01195ffd4c07 100644
> > > --- a/drivers/i2c/busses/i2c-riic.c
> > > +++ b/drivers/i2c/busses/i2c-riic.c
> > > @@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
> > >                                     : I2C_MAX_FAST_MODE_FREQ;
> > >
> > >       if (t->bus_freq_hz > max_freq)
> > > -             return dev_err_probe(&riic->adapter.dev, -EINVAL,
> > > +             return dev_err_probe(dev, -EINVAL,
> > >                                    "unsupported bus speed %uHz (%u max)\n",
> > >                                    t->bus_freq_hz, max_freq);
> > >
> > > @@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
> > >               rate /= 2;
> > >       }
> > >
> > > -     if (brl > (0x1F + 3)) {
> > > -             dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
> > > -                     (unsigned long)t->bus_freq_hz);
> > > -             return -EINVAL;
> > > -     }
> > > +     if (brl > (0x1F + 3))
> > > +             return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> > > +                                  (unsigned long)t->bus_freq_hz);
> >
> > ... I'm not happy with the splitting here: mixing a bug fix with
> > a cosmetic is wrong for two reasons:
> >
> >  - they are conceptually different;
> >  - fixes take are applied to the -fixes branch and sent to the
> >    weekly pull request.
> >
> > I will appreciate if this second chunk is squashed with patch 1
> > and the first part has a patch on its own.
> >
> OK, I think the best approach here would be to promote this to patch
> #1 ie just replacing `&riic->adapter.dev` with `dev` (as second chunk
> also includes the fix along with cosmetic change) and then make patch
> #2 as replacing `dev_err` with `dev_err_probe`.
> 
> Please let me know if this is OK with you.

As Wolfram suggested, the dev_err_probe chunk can be squashed in
the patch 1 and make a single patch that is about
/dev_err/dev_err_probe. Further splitting based on internal
functions is a bit too much.

While the first chunk belongs to a different change.

Please, keep all the tags in your next version: I don't believe
anyone would object.

Thanks,
Andi

[PATCH v2 3/9] i2c: riic: Use BIT macro consistently

[Prabhakar]

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

Easier to read and ensures proper types.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 36 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 01195ffd4c07..954e066d61a8 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -46,32 +46,32 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
-#define ICCR1_ICE	0x80
-#define ICCR1_IICRST	0x40
-#define ICCR1_SOWP	0x10
+#define ICCR1_ICE	BIT(7)
+#define ICCR1_IICRST	BIT(6)
+#define ICCR1_SOWP	BIT(4)
 
-#define ICCR2_BBSY	0x80
-#define ICCR2_SP	0x08
-#define ICCR2_RS	0x04
-#define ICCR2_ST	0x02
+#define ICCR2_BBSY	BIT(7)
+#define ICCR2_SP	BIT(3)
+#define ICCR2_RS	BIT(2)
+#define ICCR2_ST	BIT(1)
 
 #define ICMR1_CKS_MASK	0x70
-#define ICMR1_BCWP	0x08
+#define ICMR1_BCWP	BIT(3)
 #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
 
-#define ICMR3_RDRFS	0x20
-#define ICMR3_ACKWP	0x10
-#define ICMR3_ACKBT	0x08
+#define ICMR3_RDRFS	BIT(5)
+#define ICMR3_ACKWP	BIT(4)
+#define ICMR3_ACKBT	BIT(3)
 
-#define ICFER_FMPE	0x80
+#define ICFER_FMPE	BIT(7)
 
-#define ICIER_TIE	0x80
-#define ICIER_TEIE	0x40
-#define ICIER_RIE	0x20
-#define ICIER_NAKIE	0x10
-#define ICIER_SPIE	0x08
+#define ICIER_TIE	BIT(7)
+#define ICIER_TEIE	BIT(6)
+#define ICIER_RIE	BIT(5)
+#define ICIER_NAKIE	BIT(4)
+#define ICIER_SPIE	BIT(3)
 
-#define ICSR2_NACKF	0x10
+#define ICSR2_NACKF	BIT(4)
 
 #define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
 
-- 
2.43.0

Re: [PATCH v2 3/9] i2c: riic: Use BIT macro consistently

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:12AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Easier to read and ensures proper types.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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


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

[PATCH v2 4/9] i2c: riic: Use GENMASK() macro for bitmask definitions

[Prabhakar]

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

Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
driver to improve readability and maintain consistency.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 954e066d61a8..ddae4b74a86b 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -55,7 +55,7 @@
 #define ICCR2_RS	BIT(2)
 #define ICCR2_ST	BIT(1)
 
-#define ICMR1_CKS_MASK	0x70
+#define ICMR1_CKS_MASK	GENMASK(6, 4)
 #define ICMR1_BCWP	BIT(3)
 #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
 
@@ -73,7 +73,7 @@
 
 #define ICSR2_NACKF	BIT(4)
 
-#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
+#define ICBR_RESERVED	GENMASK(7, 5) /* Should be 1 on writes */
 
 #define RIIC_INIT_MSG	-1
 
-- 
2.43.0

Re: [PATCH v2 4/9] i2c: riic: Use GENMASK() macro for bitmask definitions

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:13AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> driver to improve readability and maintain consistency.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Again, I think patches 3+4 could be squashed, but this is minor.

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


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

Re: [PATCH v2 4/9] i2c: riic: Use GENMASK() macro for bitmask definitions

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:13AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> driver to improve readability and maintain consistency.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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


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

[PATCH v2 5/9] i2c: riic: Make use of devres helper to request deasserted reset line

[Prabhakar]

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

Simplify the `riic_i2c_probe()` function by using the
`devm_reset_control_get_optional_exclusive_deasserted()` API to request a
deasserted reset line. This eliminates the need to manually deassert the
reset control and the additional cleanup.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Updated error message
---
 drivers/i2c/busses/i2c-riic.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index ddae4b74a86b..edf2212e96ea 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -422,11 +422,6 @@ static struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
 };
 
-static void riic_reset_control_assert(void *data)
-{
-	reset_control_assert(data);
-}
-
 static int riic_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -447,18 +442,10 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(riic->clk),
 				     "missing controller clock");
 
-	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
+	riic->rstc = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
 	if (IS_ERR(riic->rstc))
 		return dev_err_probe(dev, PTR_ERR(riic->rstc),
-				     "Error: missing reset ctrl\n");
-
-	ret = reset_control_deassert(riic->rstc);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, riic_reset_control_assert, riic->rstc);
-	if (ret)
-		return ret;
+				     "failed to acquire deasserted reset\n");
 
 	for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
 		ret = platform_get_irq(pdev, riic_irqs[i].res_num);
-- 
2.43.0

Re: [PATCH v2 5/9] i2c: riic: Make use of devres helper to request deasserted reset line

[Geert Uytterhoeven]

On Wed, Dec 18, 2024 at 1:16 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Simplify the `riic_i2c_probe()` function by using the
> `devm_reset_control_get_optional_exclusive_deasserted()` API to request a
> deasserted reset line. This eliminates the need to manually deassert the
> reset control and the additional cleanup.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> - Updated error message

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 6/9] i2c: riic: Mark riic_irqs array as const

[Prabhakar]

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

The riic_irqs array describes the supported IRQs by the RIIC driver and
does not change at runtime.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index edf2212e96ea..378887b133a5 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -414,7 +414,7 @@ static int riic_init_hw(struct riic_dev *riic)
 	return 0;
 }
 
-static struct riic_irq_desc riic_irqs[] = {
+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" },
 	{ .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
-- 
2.43.0

[PATCH v2 7/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Prabhakar]

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

Replace the hardcoded `1000000000` with the predefined `NANO` macro for
clarity. Simplify the code by introducing a `ns_per_tick` variable to
store `NANO / rate`, reducing redundancy and improving readability.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 378887b133a5..a2d0cde5ac54 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -45,6 +45,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
+#include <linux/units.h>
 
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
@@ -312,6 +313,7 @@ static int riic_init_hw(struct riic_dev *riic)
 {
 	int ret;
 	unsigned long rate;
+	unsigned long ns_per_tick;
 	int total_ticks, cks, brl, brh;
 	struct i2c_timings *t = &riic->i2c_t;
 	struct device *dev = riic->adapter.dev.parent;
@@ -375,8 +377,9 @@ static int riic_init_hw(struct riic_dev *riic)
 	 * Remove clock ticks for rise and fall times. Convert ns to clock
 	 * ticks.
 	 */
-	brl -= t->scl_fall_ns / (1000000000 / rate);
-	brh -= t->scl_rise_ns / (1000000000 / rate);
+	ns_per_tick = NANO / rate;
+	brl -= t->scl_fall_ns / ns_per_tick;
+	brh -= t->scl_rise_ns / ns_per_tick;
 
 	/* Adjust for min register values for when SCLE=1 and NFE=1 */
 	if (brl < 1)
@@ -386,8 +389,7 @@ static int riic_init_hw(struct riic_dev *riic)
 
 	pr_debug("i2c-riic: freq=%lu, duty=%d, fall=%lu, rise=%lu, cks=%d, brl=%d, brh=%d\n",
 		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
-		 t->scl_fall_ns / (1000000000 / rate),
-		 t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
+		 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)
-- 
2.43.0

Re: [PATCH v2 7/9] i2c: riic: Use predefined macro and simplify clock tick calculation

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:16AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Replace the hardcoded `1000000000` with the predefined `NANO` macro for
> clarity. Simplify the code by introducing a `ns_per_tick` variable to
> store `NANO / rate`, reducing redundancy and improving readability.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Yay, way better readable!

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


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

[PATCH v2 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Prabhakar]

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

Introduce a new `riic_bus_barrier()` function to verify bus availability
before initiating an I2C transfer. This function enhances the bus
arbitration check by ensuring that the SDA and SCL lines are not held low,
in addition to checking the BBSY flag using `readb_poll_timeout()`.

Previously, only the BBSY flag was checked to determine bus availability.
However, it is possible for the SDA line to remain low even when BBSY = 0.
This new implementation performs an additional check on the SDA and SCL
lines to avoid potential bus contention issues.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Used single register read to check SDA/SCL lines
---
 drivers/i2c/busses/i2c-riic.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index a2d0cde5ac54..586092454bb2 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -40,6 +40,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -50,6 +51,8 @@
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
 #define ICCR1_SOWP	BIT(4)
+#define ICCR1_SCLI	BIT(1)
+#define ICCR1_SDAI	BIT(0)
 
 #define ICCR2_BBSY	BIT(7)
 #define ICCR2_SP	BIT(3)
@@ -135,6 +138,27 @@ static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u
 	riic_writeb(riic, (riic_readb(riic, reg) & ~clear) | set, reg);
 }
 
+static int riic_bus_barrier(struct riic_dev *riic)
+{
+	int ret;
+	u8 val;
+
+	/*
+	 * The SDA line can still be low even when BBSY = 0. Therefore, after checking
+	 * the BBSY flag, also verify that the SDA and SCL lines are not being held low.
+	 */
+	ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
+				 !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
+	if (ret)
+		return -EBUSY;
+
+	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
+	     (ICCR1_SDAI | ICCR1_SCLI))
+		return -EBUSY;
+
+	return 0;
+}
+
 static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct riic_dev *riic = i2c_get_adapdata(adap);
@@ -147,10 +171,9 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (ret)
 		return ret;
 
-	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
-		riic->err = -EBUSY;
+	riic->err = riic_bus_barrier(riic);
+	if (riic->err)
 		goto out;
-	}
 
 	reinit_completion(&riic->msg_done);
 	riic->err = 0;
-- 
2.43.0

Re: [PATCH v2 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Geert Uytterhoeven]

On Wed, Dec 18, 2024 at 1:16 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Introduce a new `riic_bus_barrier()` function to verify bus availability
> before initiating an I2C transfer. This function enhances the bus
> arbitration check by ensuring that the SDA and SCL lines are not held low,
> in addition to checking the BBSY flag using `readb_poll_timeout()`.
>
> Previously, only the BBSY flag was checked to determine bus availability.
> However, it is possible for the SDA line to remain low even when BBSY = 0.
> This new implementation performs an additional check on the SDA and SCL
> lines to avoid potential bus contention issues.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> - Used single register read to check SDA/SCL lines

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:17AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Introduce a new `riic_bus_barrier()` function to verify bus availability
> before initiating an I2C transfer. This function enhances the bus
> arbitration check by ensuring that the SDA and SCL lines are not held low,
> in addition to checking the BBSY flag using `readb_poll_timeout()`.
> 
> Previously, only the BBSY flag was checked to determine bus availability.
> However, it is possible for the SDA line to remain low even when BBSY = 0.
> This new implementation performs an additional check on the SDA and SCL
> lines to avoid potential bus contention issues.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

...

> -	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
> -		riic->err = -EBUSY;
> +	riic->err = riic_bus_barrier(riic);
> +	if (riic->err)
>  		goto out;
> -	}
>  
>  	reinit_completion(&riic->msg_done);
>  	riic->err = 0;

This initialization can go now. err is 0 already.

With that fixed:

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


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

Re: [PATCH v2 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the review.

On Fri, Dec 20, 2024 at 9:27 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> On Wed, Dec 18, 2024 at 12:16:17AM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Introduce a new `riic_bus_barrier()` function to verify bus availability
> > before initiating an I2C transfer. This function enhances the bus
> > arbitration check by ensuring that the SDA and SCL lines are not held low,
> > in addition to checking the BBSY flag using `readb_poll_timeout()`.
> >
> > Previously, only the BBSY flag was checked to determine bus availability.
> > However, it is possible for the SDA line to remain low even when BBSY = 0.
> > This new implementation performs an additional check on the SDA and SCL
> > lines to avoid potential bus contention issues.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> ...
>
> > -     if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
> > -             riic->err = -EBUSY;
> > +     riic->err = riic_bus_barrier(riic);
> > +     if (riic->err)
> >               goto out;
> > -     }
> >
> >       reinit_completion(&riic->msg_done);
> >       riic->err = 0;
>
> This initialization can go now. err is 0 already.
>
Agreed, I will fix that in the v3.

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

Cheers,
Prabhakar

[PATCH v2 9/9] 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>
---
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 586092454bb2..d93c371a22de 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -50,6 +50,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)
@@ -68,6 +69,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)
@@ -81,6 +83,8 @@
 
 #define RIIC_INIT_MSG	-1
 
+#define RIIC_RECOVERY_CLK_CNT	9
+
 enum riic_reg_list {
 	RIIC_ICCR1 = 0,
 	RIIC_ICCR2,
@@ -150,13 +154,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 -EBUSY;
+		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 -EINVAL;
+
+	/* 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;
@@ -491,6 +570,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;
 
@@ -503,7 +583,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;
 
@@ -611,7 +691,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 v2 9/9] i2c: riic: Implement bus recovery

[Wolfram Sang]

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

On Wed, Dec 18, 2024 at 12:16:18AM +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>

I need to ask a high level question first: why don't you use the general
scl_recovery algorithm? We have stuff like get/set_scl/sda as well as
(un)prepare_recovery. Won't that do?


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

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Lad, Prabhakar]

Hi Wolfram,

On Fri, Dec 20, 2024 at 9:16 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> On Wed, Dec 18, 2024 at 12:16:18AM +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>
>
> I need to ask a high level question first: why don't you use the general
> scl_recovery algorithm? We have stuff like get/set_scl/sda as well as
> (un)prepare_recovery. Won't that do?
>
On the RZ/G2L and RZ/G3S there is a restriction for forcing the SDA/SCL states:

● Write:
0: Changes the RIICnSCL/RIICnSDA pin output to a low level.
1: Changes the RIICnSCL/RIICnSDA pin in a high-impedance state.
(High level output is achieved through an external pull-up resistor.)

So using the generic algorithm may be platform dependent as it would
only work on platforms which have external pull-up resistor on SDA/SCL
pins. So to overcome this and make recovery possible on the platforms
I choose the RIIC feature to output clock cycles as required.

Cheers,
Prabhakar

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Wolfram Sang]

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


> On the RZ/G2L and RZ/G3S there is a restriction for forcing the SDA/SCL states:
> 
> ● Write:
> 0: Changes the RIICnSCL/RIICnSDA pin output to a low level.
> 1: Changes the RIICnSCL/RIICnSDA pin in a high-impedance state.
> (High level output is achieved through an external pull-up resistor.)
> 
> So using the generic algorithm may be platform dependent as it would
> only work on platforms which have external pull-up resistor on SDA/SCL
> pins. So to overcome this and make recovery possible on the platforms
> I choose the RIIC feature to output clock cycles as required.

I would be super-surprised if this is really a restriction and not a
very precise documentation. In other words, I am quite sure that there
is no difference between the bit forcing SCL high (via high-impedance)
and the internal RIIC handling when it needs SCL high. Most I2C busses
are open-drain anyhow.

Or is it confirmed by hardware engineers that RIIC is able to support
push/pull-busses but only this bit cannot?


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

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Lad, Prabhakar]

Hi Wolfram,

On Sun, Dec 22, 2024 at 12:44 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > On the RZ/G2L and RZ/G3S there is a restriction for forcing the SDA/SCL states:
> >
> > ● Write:
> > 0: Changes the RIICnSCL/RIICnSDA pin output to a low level.
> > 1: Changes the RIICnSCL/RIICnSDA pin in a high-impedance state.
> > (High level output is achieved through an external pull-up resistor.)
> >
> > So using the generic algorithm may be platform dependent as it would
> > only work on platforms which have external pull-up resistor on SDA/SCL
> > pins. So to overcome this and make recovery possible on the platforms
> > I choose the RIIC feature to output clock cycles as required.
>
> I would be super-surprised if this is really a restriction and not a
> very precise documentation. In other words, I am quite sure that there
> is no difference between the bit forcing SCL high (via high-impedance)
> and the internal RIIC handling when it needs SCL high. Most I2C busses
> are open-drain anyhow.
>
I had asked this previously to the HW engineers about the requirement
(as this restriction is not mentioned in the RZ/V2H(P) SoC, Ive seen
it for RZ/A series RZ/G2L family and RZ/G3S only) before the start of
the I2C recovery work but haven't got a response yet. Ive pinged them
again and I'll wait for their feedback.

Cheers,
Prabhakar

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Andi Shyti]

Hi,

On Mon, Dec 23, 2024 at 06:35:28AM +0000, Lad, Prabhakar wrote:
> On Sun, Dec 22, 2024 at 12:44 PM Wolfram Sang
> > > ● Write:
> > > 0: Changes the RIICnSCL/RIICnSDA pin output to a low level.
> > > 1: Changes the RIICnSCL/RIICnSDA pin in a high-impedance state.
> > > (High level output is achieved through an external pull-up resistor.)
> > >
> > > So using the generic algorithm may be platform dependent as it would
> > > only work on platforms which have external pull-up resistor on SDA/SCL
> > > pins. So to overcome this and make recovery possible on the platforms
> > > I choose the RIIC feature to output clock cycles as required.
> >
> > I would be super-surprised if this is really a restriction and not a
> > very precise documentation. In other words, I am quite sure that there
> > is no difference between the bit forcing SCL high (via high-impedance)
> > and the internal RIIC handling when it needs SCL high. Most I2C busses
> > are open-drain anyhow.
> >
> I had asked this previously to the HW engineers about the requirement
> (as this restriction is not mentioned in the RZ/V2H(P) SoC, Ive seen
> it for RZ/A series RZ/G2L family and RZ/G3S only) before the start of
> the I2C recovery work but haven't got a response yet. Ive pinged them
> again and I'll wait for their feedback.

Wolfram has commented on a very valid point, on a standard i2c
specification.

I'd like to merge this once all the hardware questions are
answered.

Please, do follow up on this.

Thanks,
Andi

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Wolfram Sang]

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


> I'd like to merge this once all the hardware questions are
> answered.

Maybe we can apply patches 1-8 already, so they don't reappear on the
list? IMHO they make sense independently of the bus recovery patch.


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

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Andi Shyti]

Hi Wolfram,

On Fri, Dec 27, 2024 at 12:27:38PM +0100, Wolfram Sang wrote:
> > I'd like to merge this once all the hardware questions are
> > answered.
> 
> Maybe we can apply patches 1-8 already, so they don't reappear on the
> list? IMHO they make sense independently of the bus recovery patch.

yes, this is what I already suggested as well in 0/8. Indeed
Prabhakar sent already patch 1-8 that I just checked and applied
(locally, still).

Thanks for your reviews and test here and thanks Prabhakar for
following on the reviews.

Andi

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Lad, Prabhakar]

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

Hi Wolfram,

On Sun, Dec 22, 2024 at 12:44 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > On the RZ/G2L and RZ/G3S there is a restriction for forcing the SDA/SCL states:
> >
> > ● Write:
> > 0: Changes the RIICnSCL/RIICnSDA pin output to a low level.
> > 1: Changes the RIICnSCL/RIICnSDA pin in a high-impedance state.
> > (High level output is achieved through an external pull-up resistor.)
> >
> > So using the generic algorithm may be platform dependent as it would
> > only work on platforms which have external pull-up resistor on SDA/SCL
> > pins. So to overcome this and make recovery possible on the platforms
> > I choose the RIIC feature to output clock cycles as required.
>
> I would be super-surprised if this is really a restriction and not a
> very precise documentation. In other words, I am quite sure that there
> is no difference between the bit forcing SCL high (via high-impedance)
> and the internal RIIC handling when it needs SCL high. Most I2C busses
> are open-drain anyhow.
>
> Or is it confirmed by hardware engineers that RIIC is able to support
> push/pull-busses but only this bit cannot?
>
Based on the feedback from the HW engineer the restriction is valid
see attached image (i2c-pullup.png). The SCL and SDA are Schmitt
input/open-drain output pins for both master and slave operations.
Because the output is open drain, an external pull-up resistor is
required.

Assuming there is an external pull-up resistor for all the platforms I
implemented the I2C bus recovery using the generic recovery algorithm
and I'm seeing issues, as the required number of clock pulses are not
being triggered (Note, the i2c clock frequency is 400000Hz where the
below tests are run).

I'm not sure if the below restriction is causing an issue:
--------------------
The SDAO and SCLO bits directly control the SDAn and SCLn signals
output from the RIIC. When writing to these bits, also write 0b to the
SOWP bit. Setting these bits results in input to the RIIC by the input
buffer. When slave mode is selected, a start condition might be
detected and the bus might be released, depending on the bit settings.
Do not rewrite these bits during a start condition, stop condition,
restart condition, or during transmission or reception. Operation
after rewriting under the specified conditions is not guaranteed. When
reading these bits, the state of signals output from the RIIC can be
read.

Ive run the below sequence and attached the images where the recovery
is failing, during recovery I see there aren't reliable clock pulses
see the attached images i2c-error-*.png

$ echo 0x68 > incomplete_address_phase;i2cget -y -f 3 0x68 8
See images  i2c-error-0.png and i2c-error-1.png ---> In here we can
see the i2c error is injected and the SDA line goes low and for
recovery we can see only 6 clock pulses were forced

$ i2cget -y -f 3 0x68 8 #trigger recovery again by reading
See images  i2c-error-2.png --->In here we can see only 3 clock pulses
were forced

$ i2cget -y -f 3 0x68 8  #trigger recovery again by reading
See images  i2c-error-3.png ---> This is where the i2c has recovered
successfully and we can see proper 9 clock pulses

Below is the I2C recovery code using the generic algorithm:
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 850f640d8fd4..3b9cb26ede3d 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -54,6 +54,8 @@
 #define ICCR1_IICRST   BIT(6)
 #define ICCR1_CLO      BIT(5)
 #define ICCR1_SOWP     BIT(4)
+#define ICCR1_SCLO     BIT(3)
+#define ICCR1_SDAO     BIT(2)
 #define ICCR1_SCLI     BIT(1)
 #define ICCR1_SDAI     BIT(0)

@@ -181,7 +183,7 @@ static int riic_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[], int num)
                return ret;

        riic->err = riic_bus_barrier(riic);
-       if (riic->err)
+       if (riic->err < 0)
                goto out;

        reinit_completion(&riic->msg_done);
@@ -515,6 +517,51 @@ static int riic_recover_bus(struct i2c_adapter *adap)
        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 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 int riic_get_bus_free(struct i2c_adapter *adap)
+{
+       struct riic_dev *riic = i2c_get_adapdata(adap);
+
+       udelay(5);
+
+       /* Check if the bus is busy or SDA is not high */
+       if ((riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) ||
+           !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SDAI))
+               return -EBUSY;
+
+       return 1;
+}
+
 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" },
@@ -524,7 +571,11 @@ static const struct riic_irq_desc riic_irqs[] = {
 };

 static struct i2c_bus_recovery_info riic_bri = {
-       .recover_bus = riic_recover_bus,
+       .get_scl = riic_get_scl,
+       .set_scl = riic_set_scl,
+       .set_sda = riic_set_sda,
+       .get_bus_free = riic_get_bus_free,
+       .recover_bus = i2c_generic_scl_recovery,
 };


 static int riic_i2c_probe(struct platform_device *pdev)

[-- Attachment #2: i2c-error-0.png --]
[-- Type: image/png, Size: 166713 bytes --]

[-- Attachment #3: i2c-error-3.png --]
[-- Type: image/png, Size: 173627 bytes --]

[-- Attachment #4: i2c-pullup.png --]
[-- Type: image/png, Size: 76738 bytes --]

[-- Attachment #5: i2c-error-1.png --]
[-- Type: image/png, Size: 161098 bytes --]

[-- Attachment #6: i2c-error-2.png --]
[-- Type: image/png, Size: 166765 bytes --]

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Wolfram Sang]

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

Hi Prabhakar,

finally some time for this!

> Based on the feedback from the HW engineer the restriction is valid
> see attached image (i2c-pullup.png). The SCL and SDA are Schmitt
> input/open-drain output pins for both master and slave operations.
> Because the output is open drain, an external pull-up resistor is
> required.

That confirms what I was saying. It is required. There is no difference
between setting the bit manually and the IP core doing it internally.

> Assuming there is an external pull-up resistor for all the platforms I
> implemented the I2C bus recovery using the generic recovery algorithm
> and I'm seeing issues, as the required number of clock pulses are not
> being triggered (Note, the i2c clock frequency is 400000Hz where the
> below tests are run).

So, my take is to check further why this is the case. The code looks
mostly good, except for bus_free:

> +static int riic_get_bus_free(struct i2c_adapter *adap)
> +{
> +       struct riic_dev *riic = i2c_get_adapdata(adap);
> +
> +       udelay(5);

I wonder about the udelay here. Both, why this is necessary and where
the value comes from.

> +
> +       /* Check if the bus is busy or SDA is not high */
> +       if ((riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) ||
> +           !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SDAI))

And maybe if we can't skip reading SDAI here?

> +               return -EBUSY;
> +
> +       return 1;
> +}

Have you already played with these options? If you didn't and don't have
time to do so, I can also check it. I luckily got a G3S meanwhile.

Happy hacking,

   Wolfram


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

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Wolfram Sang]

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


> Have you already played with these options? If you didn't and don't have
> time to do so, I can also check it. I luckily got a G3S meanwhile.

If the BBSY bit does not work the way we need it, maybe it is simpler
and better to just implement get_sda() and drop bus_free()?


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

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Claudiu Beznea]

On 18.12.2024 02:16, 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>
> ---
> 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 586092454bb2..d93c371a22de 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -50,6 +50,7 @@
>  

[ ... ]

> +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 -EINVAL;

Any reason for not returning directly the ret? It is true the
riic_init_hw() can, ATM, return only -EINVAL in case of failure.

Thank you,
Claudiu

Re: [PATCH v2 9/9] i2c: riic: Implement bus recovery

[Lad, Prabhakar]

On Sat, Dec 21, 2024 at 9:13 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
>
>
> On 18.12.2024 02:16, 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>
> > ---
> > 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 586092454bb2..d93c371a22de 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -50,6 +50,7 @@
> >
>
> [ ... ]
>
> > +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 -EINVAL;
>
> Any reason for not returning directly the ret? It is true the
> riic_init_hw() can, ATM, return only -EINVAL in case of failure.
>
No reason, I will fix that to return ret directly on failure.

Cheers,
Prabhakar

Re: [PATCH v2 0/9] Add support for I2C bus recovery for riic driver

[Wolfram Sang]

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


> This patch series introduces support for I2C bus recovery in the RIIC
> driver, which is utilized in RZ series SoCs. The addition of bus recovery
> functionality enhances the reliability of the I2C interface by allowing it
> to recover from error conditions that might leave the bus in an unusable
> state.
> 
> Alongside the bus recovery implementation, the series includes several
> cleanup and improvement patches that simplify and modernize the driver
> code. These include replacing `dev_err` calls with `dev_err_probe`,
> consistent usage of the `BIT` and `GENMASK` macros, leveraging devres
> helpers for reset management, and improving code readability by marking
> static data as `const`.

I am planning to review and test this series later today.


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

Re: [PATCH v2 0/9] Add support for I2C bus recovery for riic driver

[Wolfram Sang]

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

On Fri, Dec 20, 2024 at 01:02:51PM +0100, Wolfram Sang wrote:
> 
> > This patch series introduces support for I2C bus recovery in the RIIC
> > driver, which is utilized in RZ series SoCs. The addition of bus recovery
> > functionality enhances the reliability of the I2C interface by allowing it
> > to recover from error conditions that might leave the bus in an unusable
> > state.
> > 
> > Alongside the bus recovery implementation, the series includes several
> > cleanup and improvement patches that simplify and modernize the driver
> > code. These include replacing `dev_err` calls with `dev_err_probe`,
> > consistent usage of the `BIT` and `GENMASK` macros, leveraging devres
> > helpers for reset management, and improving code readability by marking
> > static data as `const`.
> 
> I am planning to review and test this series later today.

Thanks for this series! Regarding the cleanups, rhe driver is indeed in
a better shape afterwards. Good work. Patch 9 still needs discussion but
for patches 1-8:

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

On a RZ/G3S, doing bus scans and some transfers with checksumming.


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

Re: [PATCH v2 0/9] Add support for I2C bus recovery for riic driver

[Claudiu Beznea]

Hi, Prabhakar,

On 18.12.2024 02:16, Prabhakar wrote:
> Lad Prabhakar (9):
>   i2c: riic: Replace dev_err with dev_err_probe in probe function
>   i2c: riic: Use local `dev` pointer in `dev_err_probe()`
>   i2c: riic: Use BIT macro consistently
>   i2c: riic: Use GENMASK() macro for bitmask definitions
>   i2c: riic: Make use of devres helper to request deasserted reset line
>   i2c: riic: Mark riic_irqs array as const
>   i2c: riic: Use predefined macro and simplify clock tick calculation
>   i2c: riic: Add `riic_bus_barrier()` to check bus availability
>   i2c: riic: Implement bus recovery

I've tested this series on RZ/G3S checking the already enabled
functionalities and suspend to RAM. All good. Please add:

Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thank you,
Claudiu

Re: [PATCH v2 0/9] Add support for I2C bus recovery for riic driver

[Andi Shyti]

Hi,

> Lad Prabhakar (9):
>   i2c: riic: Replace dev_err with dev_err_probe in probe function
>   i2c: riic: Use local `dev` pointer in `dev_err_probe()`

just a change re-ordering here.

>   i2c: riic: Use BIT macro consistently
>   i2c: riic: Use GENMASK() macro for bitmask definitions

I'm OK with these two patches to be separate.

>   i2c: riic: Make use of devres helper to request deasserted reset line
>   i2c: riic: Mark riic_irqs array as const
>   i2c: riic: Use predefined macro and simplify clock tick calculation
>   i2c: riic: Add `riic_bus_barrier()` to check bus availability
>   i2c: riic: Implement bus recovery

There were a few comments on this patch '9' that are waiting for
a follow up.

If you want you can split the series and send patches 1-8 and I
can merge them in the meantime. Patch '9' can be taken once we
are sure about the change from a hardware perspective.

Thanks,
Andi