1. Patchew
  2. linux
  3. [PATCH 0/9] Add support for I2C bus recovery for riic driver
  4. View patch

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

Prabhakar posted 9 patches 1 year ago
There is a newer version of this series
[PATCH 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability
Posted by Prabhakar 1 year ago 
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>
---
 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 48812e7d9cef..919da1bdcce5 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) ||
+	    !(riic_readb(riic, RIIC_ICCR1) & 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 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability
Posted by Geert Uytterhoeven 1 year ago 
Hi Prabhakar,

On Fri, Dec 13, 2024 at 6:58 PM 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>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c

> @@ -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) ||
> +           !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI))

Surely you can read the register once, and check both bits?

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

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 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability
Posted by Lad, Prabhakar 1 year ago 
Hi Geert,

Thank you for the review.

On Mon, Dec 16, 2024 at 4:09 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Dec 13, 2024 at 6:58 PM 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>
>
> Thanks for your patch!
>
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
>
> > @@ -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) ||
> > +           !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI))
>
> Surely you can read the register once, and check both bits?
>
Agreed, I will add a helper func for this something like below, as
this can be re-used in patch 9/9

static inline bool riic_lines_ok(struct riic_dev *riic)
{
    u8 reg = riic_readb(riic, RIIC_ICCR1);

    if (!(reg & ICCR1_SDAI) || !(reg & ICCR1_SCLI))
        return false;

    return true;
}

Cheers,
Prabhakar
Re: [PATCH 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability
Posted by Geert Uytterhoeven 1 year ago 
Hi Prabhakar,

On Mon, Dec 16, 2024 at 7:19 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Mon, Dec 16, 2024 at 4:09 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Dec 13, 2024 at 6:58 PM 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>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/i2c/busses/i2c-riic.c
> > > +++ b/drivers/i2c/busses/i2c-riic.c
> >
> > > @@ -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) ||
> > > +           !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI))
> >
> > Surely you can read the register once, and check both bits?
> >
> Agreed, I will add a helper func for this something like below, as
> this can be re-used in patch 9/9
>
> static inline bool riic_lines_ok(struct riic_dev *riic)
> {
>     u8 reg = riic_readb(riic, RIIC_ICCR1);
>
>     if (!(reg & ICCR1_SDAI) || !(reg & ICCR1_SCLI))
>         return false;
>
>     return true;
> }

A helper might be overkill...

    if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
        ICCR1_SDAI | ICCR1_SCLI)
            ...

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 8/9] i2c: riic: Add `riic_bus_barrier()` to check bus availability
Posted by Lad, Prabhakar 1 year ago 
Hi Geert,

On Mon, Dec 16, 2024 at 7:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Dec 16, 2024 at 7:19 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Mon, Dec 16, 2024 at 4:09 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Dec 13, 2024 at 6:58 PM 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>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/i2c/busses/i2c-riic.c
> > > > +++ b/drivers/i2c/busses/i2c-riic.c
> > >
> > > > @@ -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) ||
> > > > +           !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI))
> > >
> > > Surely you can read the register once, and check both bits?
> > >
> > Agreed, I will add a helper func for this something like below, as
> > this can be re-used in patch 9/9
> >
> > static inline bool riic_lines_ok(struct riic_dev *riic)
> > {
> >     u8 reg = riic_readb(riic, RIIC_ICCR1);
> >
> >     if (!(reg & ICCR1_SDAI) || !(reg & ICCR1_SCLI))
> >         return false;
> >
> >     return true;
> > }
>
> A helper might be overkill...
>
>     if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
>         ICCR1_SDAI | ICCR1_SCLI)
>             ...
>
Ok, I'll update as suggested above.

Cheers,
Prabhakar

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.