[PATCH] i2c: rcar: add SMBus block read support

[PATCH] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

The smbus block read is not currently supported for rcar i2c devices.
This patchset adds the support to rcar i2c bus so that blocks of data
can be read using SMbus block reads.(using i2c_smbus_read_block_data()
function from the i2c-core-smbus.c).

Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")

This patch (adapted) was tested with v4.14, but due to lack of real
hardware with SMBus block read operations support, using "simulation",
that is manual analysis of data, read from plain I2C devices with
SMBus block read request.

Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/i2c/busses/i2c-rcar.c | 45 +++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index bff9913c37b8..a9fc2b3b6392 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -105,6 +105,7 @@
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
+#define ID_EPROTO	(1 << 5)
 /* persistent flags */
 #define ID_P_HOST_NOTIFY	BIT(28)
 #define ID_P_REP_AFTER_RD	BIT(29)
@@ -412,6 +413,7 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
 	struct i2c_msg *msg = priv->msg;
 	bool read = msg->flags & I2C_M_RD;
+	bool block_data = msg->flags & I2C_M_RECV_LEN;
 	enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 	struct dma_chan *chan = read ? priv->dma_rx : priv->dma_tx;
 	struct dma_async_tx_descriptor *txdesc;
@@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
 		/*
 		 * The last two bytes needs to be fetched using PIO in
 		 * order for the STOP phase to work.
+		 *
+		 * For SMBus block read the first byte was received using PIO.
 		 */
-		buf = priv->msg->buf;
-		len = priv->msg->len - 2;
+		if (block_data) {
+			buf = priv->msg->buf + 1;
+			len = priv->msg->len - 3;
+		} else {
+			buf = priv->msg->buf;
+			len = priv->msg->len - 2;
+		}
 	} else {
 		/*
 		 * First byte in message was sent using PIO.
@@ -530,6 +539,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	bool block_data = msg->flags & I2C_M_RECV_LEN;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
@@ -538,8 +548,29 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	if (msr & MAT) {
 		/*
 		 * Address transfer phase finished, but no data at this point.
-		 * Try to use DMA to receive data.
+		 * Try to use DMA to receive data if it is not SMBus block
+		 * data read.
 		 */
+		if (block_data)
+			goto next_txn;
+
+		rcar_i2c_dma(priv);
+	} else if (priv->pos == 0 && block_data) {
+		/*
+		 * First byte is the length of remaining packet
+		 * in the SMBus block data read. Add it to
+		 * msg->len.
+		 */
+		u8 data = rcar_i2c_read(priv, ICRXTX);
+
+		if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) {
+			priv->flags |= ID_DONE | ID_EPROTO;
+			return;
+		}
+		msg->len += data;
+		msg->buf[priv->pos] = data;
+		priv->pos++;
+		/* Still try to use DMA to receive the rest of data */
 		rcar_i2c_dma(priv);
 	} else if (priv->pos < msg->len) {
 		/* get received data */
@@ -557,6 +588,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		}
 	}
 
+next_txn:
 	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
 	else
@@ -855,6 +887,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -917,6 +951,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -983,7 +1019,8 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
 	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
 	 */
 	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
-		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
+		   I2C_FUNC_SMBUS_READ_BLOCK_DATA;
 
 	if (priv->flags & ID_P_HOST_NOTIFY)
 		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
-- 
2.21.0

Re: [PATCH] i2c: rcar: add SMBus block read support

[Geert Uytterhoeven]

Hi Andrew,

On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov
<andrew_gabbasov@mentor.com> wrote:
> The smbus block read is not currently supported for rcar i2c devices.
> This patchset adds the support to rcar i2c bus so that blocks of data
> can be read using SMbus block reads.(using i2c_smbus_read_block_data()
> function from the i2c-core-smbus.c).
>
> Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")
>
> This patch (adapted) was tested with v4.14, but due to lack of real
> hardware with SMBus block read operations support, using "simulation",
> that is manual analysis of data, read from plain I2C devices with
> SMBus block read request.
>
> Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
>                 /*
>                  * The last two bytes needs to be fetched using PIO in
>                  * order for the STOP phase to work.
> +                *
> +                * For SMBus block read the first byte was received using PIO.

So it might be easier to read, and more maintainable, to keep the
old assignments:

    buf = priv->msg->buf;
    len = priv->msg->len - 2;

and adjust them for SMBus afterwards:

    if (block_data) {
            /* For SMBus block read the first byte was received using PIO */
            buf++;
            len--;
    }

?

>                  */
> -               buf = priv->msg->buf;
> -               len = priv->msg->len - 2;
> +               if (block_data) {
> +                       buf = priv->msg->buf + 1;
> +                       len = priv->msg->len - 3;
> +               } else {
> +                       buf = priv->msg->buf;
> +                       len = priv->msg->len - 2;
> +               }
>         } else {
>                 /*
>                  * First byte in message was sent using PIO.

And below we have another case handling buf and len :-(

So perhaps:

    buf = priv->msg->buf;
    len = priv->msg->len;

    if (read) {
            /*
             * The last two bytes needs to be fetched using PIO in
             * order for the STOP phase to work.
             */
            len -= 2;
    }
    if (!read || block_data) {
            /* First byte in message was sent using PIO *
            buf++;
            len--;
    }

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] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

Hi Geert,

Thank you for your review!

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, October 05, 2021 4:32 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> Subject: Re: [PATCH] i2c: rcar: add SMBus block read support
> 
> Hi Andrew,
> 
> On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov
> <andrew_gabbasov@mentor.com> wrote:
> > The smbus block read is not currently supported for rcar i2c devices.
> > This patchset adds the support to rcar i2c bus so that blocks of data
> > can be read using SMbus block reads.(using i2c_smbus_read_block_data()
> > function from the i2c-core-smbus.c).
> >
> > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")
> >
> > This patch (adapted) was tested with v4.14, but due to lack of real
> > hardware with SMBus block read operations support, using "simulation",
> > that is manual analysis of data, read from plain I2C devices with
> > SMBus block read request.
> >
> > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
> > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
> >                 /*
> >                  * The last two bytes needs to be fetched using PIO in
> >                  * order for the STOP phase to work.
> > +                *
> > +                * For SMBus block read the first byte was received using PIO.
> 
> So it might be easier to read, and more maintainable, to keep the
> old assignments:
> 
>     buf = priv->msg->buf;
>     len = priv->msg->len - 2;
> 
> and adjust them for SMBus afterwards:
> 
>     if (block_data) {
>             /* For SMBus block read the first byte was received using PIO */
>             buf++;
>             len--;
>     }
> 
> ?
> 
> >                  */
> > -               buf = priv->msg->buf;
> > -               len = priv->msg->len - 2;
> > +               if (block_data) {
> > +                       buf = priv->msg->buf + 1;
> > +                       len = priv->msg->len - 3;
> > +               } else {
> > +                       buf = priv->msg->buf;
> > +                       len = priv->msg->len - 2;
> > +               }
> >         } else {
> >                 /*
> >                  * First byte in message was sent using PIO.
> 
> And below we have another case handling buf and len :-(
> 
> So perhaps:
> 
>     buf = priv->msg->buf;
>     len = priv->msg->len;
> 
>     if (read) {
>             /*
>              * The last two bytes needs to be fetched using PIO in
>              * order for the STOP phase to work.
>              */
>             len -= 2;
>     }
>     if (!read || block_data) {
>             /* First byte in message was sent using PIO *
>             buf++;
>             len--;
>     }

Probably I was trying to minimize the changes ;-)

However, I agree with you that the whole code fragment can be simplified
and your variant indeed looks more clean and understandable.
Thank you for your suggestion, I'll submit version 2 of the patch
with this fragment changed.

Thanks!

Best regards,
Andrew

[PATCH v2] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

The smbus block read is not currently supported for rcar i2c devices.
This patchset adds the support to rcar i2c bus so that blocks of data
can be read using SMbus block reads.(using i2c_smbus_read_block_data()
function from the i2c-core-smbus.c).

Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")

This patch (adapted) was tested with v4.14, but due to lack of real
hardware with SMBus block read operations support, using "simulation",
that is manual analysis of data, read from plain I2C devices with
SMBus block read request.

Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/i2c/busses/i2c-rcar.c | 49 +++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index bff9913c37b8..e4b603f425fa 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -105,6 +105,7 @@
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
+#define ID_EPROTO	(1 << 5)
 /* persistent flags */
 #define ID_P_HOST_NOTIFY	BIT(28)
 #define ID_P_REP_AFTER_RD	BIT(29)
@@ -412,6 +413,7 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
 	struct i2c_msg *msg = priv->msg;
 	bool read = msg->flags & I2C_M_RD;
+	bool block_data = msg->flags & I2C_M_RECV_LEN;
 	enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 	struct dma_chan *chan = read ? priv->dma_rx : priv->dma_tx;
 	struct dma_async_tx_descriptor *txdesc;
@@ -425,19 +427,22 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	    !(msg->flags & I2C_M_DMA_SAFE) || (read && priv->flags & ID_P_NO_RXDMA))
 		return false;
 
+	buf = priv->msg->buf;
+	len = priv->msg->len;
+
 	if (read) {
 		/*
 		 * The last two bytes needs to be fetched using PIO in
 		 * order for the STOP phase to work.
 		 */
-		buf = priv->msg->buf;
-		len = priv->msg->len - 2;
-	} else {
+		len -= 2;
+	}
+	if (!read || block_data) {
 		/*
-		 * First byte in message was sent using PIO.
+		 * First byte in message was sent or received using PIO.
 		 */
-		buf = priv->msg->buf + 1;
-		len = priv->msg->len - 1;
+		buf++;
+		len--;
 	}
 
 	dma_addr = dma_map_single(chan->device->dev, buf, len, dir);
@@ -530,6 +535,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	bool block_data = msg->flags & I2C_M_RECV_LEN;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
@@ -538,8 +544,29 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	if (msr & MAT) {
 		/*
 		 * Address transfer phase finished, but no data at this point.
-		 * Try to use DMA to receive data.
+		 * Try to use DMA to receive data if it is not SMBus block
+		 * data read.
 		 */
+		if (block_data)
+			goto next_txn;
+
+		rcar_i2c_dma(priv);
+	} else if (priv->pos == 0 && block_data) {
+		/*
+		 * First byte is the length of remaining packet
+		 * in the SMBus block data read. Add it to
+		 * msg->len.
+		 */
+		u8 data = rcar_i2c_read(priv, ICRXTX);
+
+		if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) {
+			priv->flags |= ID_DONE | ID_EPROTO;
+			return;
+		}
+		msg->len += data;
+		msg->buf[priv->pos] = data;
+		priv->pos++;
+		/* Still try to use DMA to receive the rest of data */
 		rcar_i2c_dma(priv);
 	} else if (priv->pos < msg->len) {
 		/* get received data */
@@ -557,6 +584,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		}
 	}
 
+next_txn:
 	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
 	else
@@ -855,6 +883,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -917,6 +947,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -983,7 +1015,8 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
 	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
 	 */
 	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
-		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
+		   I2C_FUNC_SMBUS_READ_BLOCK_DATA;
 
 	if (priv->flags & ID_P_HOST_NOTIFY)
 		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
-- 
2.21.0

RE: [PATCH] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

Hello Geert, Wolfram,

Do you have any feedback on version 2 of this patch, that was submitted
after your review comments below?

https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/

Thanks!

Best regards,
Andrew

> -----Original Message-----
> From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Sent: Wednesday, October 06, 2021 9:12 PM
> To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> 
> Hi Geert,
> 
> Thank you for your review!
> 
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Tuesday, October 05, 2021 4:32 PM
> > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support
> >
> > Hi Andrew,
> >
> > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov
> > <andrew_gabbasov@mentor.com> wrote:
> > > The smbus block read is not currently supported for rcar i2c devices.
> > > This patchset adds the support to rcar i2c bus so that blocks of data
> > > can be read using SMbus block reads.(using i2c_smbus_read_block_data()
> > > function from the i2c-core-smbus.c).
> > >
> > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")
> > >
> > > This patch (adapted) was tested with v4.14, but due to lack of real
> > > hardware with SMBus block read operations support, using "simulation",
> > > that is manual analysis of data, read from plain I2C devices with
> > > SMBus block read request.
> > >
> > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/i2c/busses/i2c-rcar.c
> > > +++ b/drivers/i2c/busses/i2c-rcar.c
> > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
> > >                 /*
> > >                  * The last two bytes needs to be fetched using PIO in
> > >                  * order for the STOP phase to work.
> > > +                *
> > > +                * For SMBus block read the first byte was received using PIO.
> >
> > So it might be easier to read, and more maintainable, to keep the
> > old assignments:
> >
> >     buf = priv->msg->buf;
> >     len = priv->msg->len - 2;
> >
> > and adjust them for SMBus afterwards:
> >
> >     if (block_data) {
> >             /* For SMBus block read the first byte was received using PIO */
> >             buf++;
> >             len--;
> >     }
> >
> > ?
> >
> > >                  */
> > > -               buf = priv->msg->buf;
> > > -               len = priv->msg->len - 2;
> > > +               if (block_data) {
> > > +                       buf = priv->msg->buf + 1;
> > > +                       len = priv->msg->len - 3;
> > > +               } else {
> > > +                       buf = priv->msg->buf;
> > > +                       len = priv->msg->len - 2;
> > > +               }
> > >         } else {
> > >                 /*
> > >                  * First byte in message was sent using PIO.
> >
> > And below we have another case handling buf and len :-(
> >
> > So perhaps:
> >
> >     buf = priv->msg->buf;
> >     len = priv->msg->len;
> >
> >     if (read) {
> >             /*
> >              * The last two bytes needs to be fetched using PIO in
> >              * order for the STOP phase to work.
> >              */
> >             len -= 2;
> >     }
> >     if (!read || block_data) {
> >             /* First byte in message was sent using PIO *
> >             buf++;
> >             len--;
> >     }
> 
> Probably I was trying to minimize the changes ;-)
> 
> However, I agree with you that the whole code fragment can be simplified
> and your variant indeed looks more clean and understandable.
> Thank you for your suggestion, I'll submit version 2 of the patch
> with this fragment changed.
> 
> Thanks!
> 
> Best regards,
> Andrew

RE: [PATCH] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

Hello Geert, Wolfram,

Could you please let me know your opinion on version 2 of this patch,
that addressed your earlier review comments?

https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/

Does it still need any further modifications or are you going to promote it further upstream?

Thanks.

Best regards,
Andrew

> -----Original Message-----
> From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Sent: Thursday, November 18, 2021 1:35 PM
> To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel
> Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari,
> Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> 
> Hello Geert, Wolfram,
> 
> Do you have any feedback on version 2 of this patch, that was submitted
> after your review comments below?
> 
> https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/
> 
> Thanks!
> 
> Best regards,
> Andrew
> 
> > -----Original Message-----
> > From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > Sent: Wednesday, October 06, 2021 9:12 PM
> > To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> >
> > Hi Geert,
> >
> > Thank you for your review!
> >
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Sent: Tuesday, October 05, 2021 4:32 PM
> > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support
> > >
> > > Hi Andrew,
> > >
> > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov
> > > <andrew_gabbasov@mentor.com> wrote:
> > > > The smbus block read is not currently supported for rcar i2c devices.
> > > > This patchset adds the support to rcar i2c bus so that blocks of data
> > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data()
> > > > function from the i2c-core-smbus.c).
> > > >
> > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")
> > > >
> > > > This patch (adapted) was tested with v4.14, but due to lack of real
> > > > hardware with SMBus block read operations support, using "simulation",
> > > > that is manual analysis of data, read from plain I2C devices with
> > > > SMBus block read request.
> > > >
> > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/i2c/busses/i2c-rcar.c
> > > > +++ b/drivers/i2c/busses/i2c-rcar.c
> > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
> > > >                 /*
> > > >                  * The last two bytes needs to be fetched using PIO in
> > > >                  * order for the STOP phase to work.
> > > > +                *
> > > > +                * For SMBus block read the first byte was received using PIO.
> > >
> > > So it might be easier to read, and more maintainable, to keep the
> > > old assignments:
> > >
> > >     buf = priv->msg->buf;
> > >     len = priv->msg->len - 2;
> > >
> > > and adjust them for SMBus afterwards:
> > >
> > >     if (block_data) {
> > >             /* For SMBus block read the first byte was received using PIO */
> > >             buf++;
> > >             len--;
> > >     }
> > >
> > > ?
> > >
> > > >                  */
> > > > -               buf = priv->msg->buf;
> > > > -               len = priv->msg->len - 2;
> > > > +               if (block_data) {
> > > > +                       buf = priv->msg->buf + 1;
> > > > +                       len = priv->msg->len - 3;
> > > > +               } else {
> > > > +                       buf = priv->msg->buf;
> > > > +                       len = priv->msg->len - 2;
> > > > +               }
> > > >         } else {
> > > >                 /*
> > > >                  * First byte in message was sent using PIO.
> > >
> > > And below we have another case handling buf and len :-(
> > >
> > > So perhaps:
> > >
> > >     buf = priv->msg->buf;
> > >     len = priv->msg->len;
> > >
> > >     if (read) {
> > >             /*
> > >              * The last two bytes needs to be fetched using PIO in
> > >              * order for the STOP phase to work.
> > >              */
> > >             len -= 2;
> > >     }
> > >     if (!read || block_data) {
> > >             /* First byte in message was sent using PIO *
> > >             buf++;
> > >             len--;
> > >     }
> >
> > Probably I was trying to minimize the changes ;-)
> >
> > However, I agree with you that the whole code fragment can be simplified
> > and your variant indeed looks more clean and understandable.
> > Thank you for your suggestion, I'll submit version 2 of the patch
> > with this fragment changed.
> >
> > Thanks!
> >
> > Best regards,
> > Andrew

RE: [PATCH] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

Hello Geert, Wolfram,

Any feedback on the patch, please?

Thanks.

Best regards,
Andrew

> -----Original Message-----
> From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Sent: Sunday, January 09, 2022 10:20 PM
> To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel
> Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari,
> Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> 
> Hello Geert, Wolfram,
> 
> Could you please let me know your opinion on version 2 of this patch,
> that addressed your earlier review comments?
> 
> https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/
> 
> Does it still need any further modifications or are you going to promote it further upstream?
> 
> Thanks.
> 
> Best regards,
> Andrew
> 
> > -----Original Message-----
> > From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > Sent: Thursday, November 18, 2021 1:35 PM
> > To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel
> > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari,
> > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> >
> > Hello Geert, Wolfram,
> >
> > Do you have any feedback on version 2 of this patch, that was submitted
> > after your review comments below?
> >
> > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/
> >
> > Thanks!
> >
> > Best regards,
> > Andrew
> >
> > > -----Original Message-----
> > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > Sent: Wednesday, October 06, 2021 9:12 PM
> > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> > >
> > > Hi Geert,
> > >
> > > Thank you for your review!
> > >
> > > > -----Original Message-----
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Sent: Tuesday, October 05, 2021 4:32 PM
> > > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support
> > > >
> > > > Hi Andrew,
> > > >
> > > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov
> > > > <andrew_gabbasov@mentor.com> wrote:
> > > > > The smbus block read is not currently supported for rcar i2c devices.
> > > > > This patchset adds the support to rcar i2c bus so that blocks of data
> > > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data()
> > > > > function from the i2c-core-smbus.c).
> > > > >
> > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")
> > > > >
> > > > > This patch (adapted) was tested with v4.14, but due to lack of real
> > > > > hardware with SMBus block read operations support, using "simulation",
> > > > > that is manual analysis of data, read from plain I2C devices with
> > > > > SMBus block read request.
> > > > >
> > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/i2c/busses/i2c-rcar.c
> > > > > +++ b/drivers/i2c/busses/i2c-rcar.c
> > > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
> > > > >                 /*
> > > > >                  * The last two bytes needs to be fetched using PIO in
> > > > >                  * order for the STOP phase to work.
> > > > > +                *
> > > > > +                * For SMBus block read the first byte was received using PIO.
> > > >
> > > > So it might be easier to read, and more maintainable, to keep the
> > > > old assignments:
> > > >
> > > >     buf = priv->msg->buf;
> > > >     len = priv->msg->len - 2;
> > > >
> > > > and adjust them for SMBus afterwards:
> > > >
> > > >     if (block_data) {
> > > >             /* For SMBus block read the first byte was received using PIO */
> > > >             buf++;
> > > >             len--;
> > > >     }
> > > >
> > > > ?
> > > >
> > > > >                  */
> > > > > -               buf = priv->msg->buf;
> > > > > -               len = priv->msg->len - 2;
> > > > > +               if (block_data) {
> > > > > +                       buf = priv->msg->buf + 1;
> > > > > +                       len = priv->msg->len - 3;
> > > > > +               } else {
> > > > > +                       buf = priv->msg->buf;
> > > > > +                       len = priv->msg->len - 2;
> > > > > +               }
> > > > >         } else {
> > > > >                 /*
> > > > >                  * First byte in message was sent using PIO.
> > > >
> > > > And below we have another case handling buf and len :-(
> > > >
> > > > So perhaps:
> > > >
> > > >     buf = priv->msg->buf;
> > > >     len = priv->msg->len;
> > > >
> > > >     if (read) {
> > > >             /*
> > > >              * The last two bytes needs to be fetched using PIO in
> > > >              * order for the STOP phase to work.
> > > >              */
> > > >             len -= 2;
> > > >     }
> > > >     if (!read || block_data) {
> > > >             /* First byte in message was sent using PIO *
> > > >             buf++;
> > > >             len--;
> > > >     }
> > >
> > > Probably I was trying to minimize the changes ;-)
> > >
> > > However, I agree with you that the whole code fragment can be simplified
> > > and your variant indeed looks more clean and understandable.
> > > Thank you for your suggestion, I'll submit version 2 of the patch
> > > with this fragment changed.
> > >
> > > Thanks!
> > >
> > > Best regards,
> > > Andrew

RE: [PATCH] i2c: rcar: add SMBus block read support

[Andrew Gabbasov]

Hello Geert, Wolfram,

Could you please let us know your opinion on this patch
and further requirements, if any.

Thanks!

Best regards,
Andrew

> -----Original Message-----
> From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Sent: Tuesday, January 25, 2022 9:46 AM
> To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel
> Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari,
> Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> 
> Hello Geert, Wolfram,
> 
> Any feedback on the patch, please?
> 
> Thanks.
> 
> Best regards,
> Andrew
> 
> > -----Original Message-----
> > From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > Sent: Sunday, January 09, 2022 10:20 PM
> > To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel
> > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari,
> > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> >
> > Hello Geert, Wolfram,
> >
> > Could you please let me know your opinion on version 2 of this patch,
> > that addressed your earlier review comments?
> >
> > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/
> >
> > Does it still need any further modifications or are you going to promote it further upstream?
> >
> > Thanks.
> >
> > Best regards,
> > Andrew
> >
> > > -----Original Message-----
> > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > Sent: Thursday, November 18, 2021 1:35 PM
> > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> > > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux
> Kernel
> > > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari,
> > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> > >
> > > Hello Geert, Wolfram,
> > >
> > > Do you have any feedback on version 2 of this patch, that was submitted
> > > after your review comments below?
> > >
> > > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/
> > >
> > > Thanks!
> > >
> > > Best regards,
> > > Andrew
> > >
> > > > -----Original Message-----
> > > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > > Sent: Wednesday, October 06, 2021 9:12 PM
> > > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org>
> > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel
> > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support
> > > >
> > > > Hi Geert,
> > > >
> > > > Thank you for your review!
> > > >
> > > > > -----Original Message-----
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > Sent: Tuesday, October 05, 2021 4:32 PM
> > > > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> > > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux
> Kernel
> > > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari,
> > > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> > > > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support
> > > > >
> > > > > Hi Andrew,
> > > > >
> > > > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov
> > > > > <andrew_gabbasov@mentor.com> wrote:
> > > > > > The smbus block read is not currently supported for rcar i2c devices.
> > > > > > This patchset adds the support to rcar i2c bus so that blocks of data
> > > > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data()
> > > > > > function from the i2c-core-smbus.c).
> > > > > >
> > > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support")
> > > > > >
> > > > > > This patch (adapted) was tested with v4.14, but due to lack of real
> > > > > > hardware with SMBus block read operations support, using "simulation",
> > > > > > that is manual analysis of data, read from plain I2C devices with
> > > > > > SMBus block read request.
> > > > > >
> > > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> > > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > --- a/drivers/i2c/busses/i2c-rcar.c
> > > > > > +++ b/drivers/i2c/busses/i2c-rcar.c
> > > > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
> > > > > >                 /*
> > > > > >                  * The last two bytes needs to be fetched using PIO in
> > > > > >                  * order for the STOP phase to work.
> > > > > > +                *
> > > > > > +                * For SMBus block read the first byte was received using PIO.
> > > > >
> > > > > So it might be easier to read, and more maintainable, to keep the
> > > > > old assignments:
> > > > >
> > > > >     buf = priv->msg->buf;
> > > > >     len = priv->msg->len - 2;
> > > > >
> > > > > and adjust them for SMBus afterwards:
> > > > >
> > > > >     if (block_data) {
> > > > >             /* For SMBus block read the first byte was received using PIO */
> > > > >             buf++;
> > > > >             len--;
> > > > >     }
> > > > >
> > > > > ?
> > > > >
> > > > > >                  */
> > > > > > -               buf = priv->msg->buf;
> > > > > > -               len = priv->msg->len - 2;
> > > > > > +               if (block_data) {
> > > > > > +                       buf = priv->msg->buf + 1;
> > > > > > +                       len = priv->msg->len - 3;
> > > > > > +               } else {
> > > > > > +                       buf = priv->msg->buf;
> > > > > > +                       len = priv->msg->len - 2;
> > > > > > +               }
> > > > > >         } else {
> > > > > >                 /*
> > > > > >                  * First byte in message was sent using PIO.
> > > > >
> > > > > And below we have another case handling buf and len :-(
> > > > >
> > > > > So perhaps:
> > > > >
> > > > >     buf = priv->msg->buf;
> > > > >     len = priv->msg->len;
> > > > >
> > > > >     if (read) {
> > > > >             /*
> > > > >              * The last two bytes needs to be fetched using PIO in
> > > > >              * order for the STOP phase to work.
> > > > >              */
> > > > >             len -= 2;
> > > > >     }
> > > > >     if (!read || block_data) {
> > > > >             /* First byte in message was sent using PIO *
> > > > >             buf++;
> > > > >             len--;
> > > > >     }
> > > >
> > > > Probably I was trying to minimize the changes ;-)
> > > >
> > > > However, I agree with you that the whole code fragment can be simplified
> > > > and your variant indeed looks more clean and understandable.
> > > > Thank you for your suggestion, I'll submit version 2 of the patch
> > > > with this fragment changed.
> > > >
> > > > Thanks!
> > > >
> > > > Best regards,
> > > > Andrew

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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

Hi Andrew,

first sorry that it took so long. The reason here is that my original
plan was to add 256-byte support to RECV_LEN in the I2C core and enable
it on R-Car afterwards. Sadly, I never found the time to drive this
forward. So, all RECV_LEN things got stuck for a while :(

> This patch (adapted) was tested with v4.14, but due to lack of real
> hardware with SMBus block read operations support, using "simulation",
> that is manual analysis of data, read from plain I2C devices with
> SMBus block read request.

You could wire up two R-Car I2C instances, set up one as an I2C slave
handled by the I2C testunit and then use the other instance with
SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
Documentation/i2c/slave-testunit-backend.rst for details.

I wonder a bit about the complexity of your patch. In my WIP-branch for
256-byte transfers, I have the following patch. It is only missing the
range check for the received byte, but that it easy to add. Do you see
anything else missing? If not, I prefer this simpler version because it
is less intrusive and the state machine is a bit fragile (due to HW
issues with old HW).

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: Sun, 2 Aug 2020 00:24:52 +0200
Subject: [PATCH] i2c: rcar: add support for I2C_M_RECV_LEN

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 217def2d7cb4..e473f5c0a708 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -528,6 +528,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
@@ -542,11 +543,13 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	} else if (priv->pos < msg->len) {
 		/* get received data */
 		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
+		if (recv_len_init)
+			msg->len += msg->buf[0];
 		priv->pos++;
 	}
 
 	/* If next received data is the _LAST_, go to new phase. */
-	if (priv->pos + 1 == msg->len) {
+	if (priv->pos + 1 == msg->len && !recv_len_init) {
 		if (priv->flags & ID_LAST_MSG) {
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
 		} else {
@@ -889,7 +892,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
 	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
 	 */
 	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
-		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
 
 	if (priv->flags & ID_P_HOST_NOTIFY)
 		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;

Happy hacking,

   Wolfram


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

RE: [PATCH v2] i2c: rcar: add SMBus block read support

[Gabbasov, Andrew]

Hi Wolfram!

Thank you for your feedback!
See my responses below.

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: Thursday, February 17, 2022 10:45 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Geert
> Uytterhoeven <geert+renesas@glider.be>; Surachari, Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
> Subject: Re: [PATCH v2] i2c: rcar: add SMBus block read support
> 
[skipped]
> 
> > This patch (adapted) was tested with v4.14, but due to lack of real
> > hardware with SMBus block read operations support, using "simulation",
> > that is manual analysis of data, read from plain I2C devices with
> > SMBus block read request.
> 
> You could wire up two R-Car I2C instances, set up one as an I2C slave
> handled by the I2C testunit and then use the other instance with
> SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
> Documentation/i2c/slave-testunit-backend.rst for details.

You mean physical connection of two R-Car boards via I2C bus,
or physical connection of I2C bus wires on the single board, right?
It looks like all the boards, that I have access to, do not have
I2C bus wires exposed to some connectors, so both variants would
require hardware re-wiring modification of the boards, which is
not an option for me. Or do I understand you incorrectly and you
mean something different?

> I wonder a bit about the complexity of your patch. In my WIP-branch for
> 256-byte transfers, I have the following patch. It is only missing the
> range check for the received byte, but that it easy to add. Do you see
> anything else missing? If not, I prefer this simpler version because it
> is less intrusive and the state machine is a bit fragile (due to HW
> issues with old HW).

Most of complexity in my patch is related to DMA transfers support,
that I'm trying to retain for SMBus block data transfers too (for the rest
of bytes after the first "length" byte). Your simple patch makes
the driver perform all M_RECV_LEN transfers in PIO mode only (with no DMA at all),
which is probably not quite good (it's a pity to loose existing HW capability,
already supported by the driver).

Also, see a couple of comments below.

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date: Sun, 2 Aug 2020 00:24:52 +0200
> Subject: [PATCH] i2c: rcar: add support for I2C_M_RECV_LEN
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 217def2d7cb4..e473f5c0a708 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -528,6 +528,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
>  static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  {
>  	struct i2c_msg *msg = priv->msg;
> +	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
> 
>  	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
>  	if (!(msr & MDR))
> @@ -542,11 +543,13 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  	} else if (priv->pos < msg->len) {
>  		/* get received data */
>  		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
> +		if (recv_len_init)
> +			msg->len += msg->buf[0];
>  		priv->pos++;
>  	}
> 
>  	/* If next received data is the _LAST_, go to new phase. */
> -	if (priv->pos + 1 == msg->len) {
> +	if (priv->pos + 1 == msg->len && !recv_len_init) {

If a message contains a single byte after the length byte,
when we come here after processing the length (in the same function call),
"pos" is 1, "len" is 2, and we indeed are going to process the last byte.
However, "recv_len_init" is still "true", and we skip these corresponding
register writes, which is probably incorrect.
The flag in this case should be re-set back to "false" after length
processing and "pos" moving, but I think the variant in my patch
(leaving this "if" unchanged, but skipping it on the first pass with "goto")
may be even simpler.

>  		if (priv->flags & ID_LAST_MSG) {
>  			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
>  		} else {
> @@ -889,7 +892,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
>  	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
>  	 */
>  	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> -		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);

This flags setting adds also I2C_FUNC_SMBUS_BLOCK_PROC_CALL flag,
which is missed in my patch. My patch should probably be updated
to include it too (if you'll agree to take my variant ;-) ).

> 
>  	if (priv->flags & ID_P_HOST_NOTIFY)
>  		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
> 
> Happy hacking,
> 
>    Wolfram

Thanks!

Best regards,
Andrew

RE: [PATCH v2] i2c: rcar: add SMBus block read support

[Surachari, Bhuvanesh]

Hi Wolfram,

     Could you please provide feedback to our response at,

https://lore.kernel.org/all/0a07902900bc4ecc84bd93a6b85a2e0c@svr-ies-mbx-02.mgc.mentorg.com/

Thank you,
Regards,
Bhuvanesh
-----Original Message-----
From: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> 
Sent: 18 February 2022 16:33
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Surachari, Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
Subject: RE: [PATCH v2] i2c: rcar: add SMBus block read support

Hi Wolfram!

Thank you for your feedback!
See my responses below.

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: Thursday, February 17, 2022 10:45 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; 
> linux-kernel@vger.kernel.org; Geert Uytterhoeven 
> <geert+renesas@glider.be>; Surachari, Bhuvanesh 
> <Bhuvanesh_Surachari@mentor.com>
> Subject: Re: [PATCH v2] i2c: rcar: add SMBus block read support
> 
[skipped]
> 
> > This patch (adapted) was tested with v4.14, but due to lack of real 
> > hardware with SMBus block read operations support, using 
> > "simulation", that is manual analysis of data, read from plain I2C 
> > devices with SMBus block read request.
> 
> You could wire up two R-Car I2C instances, set up one as an I2C slave 
> handled by the I2C testunit and then use the other instance with 
> SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check 
> Documentation/i2c/slave-testunit-backend.rst for details.

You mean physical connection of two R-Car boards via I2C bus, or physical connection of I2C bus wires on the single board, right?
It looks like all the boards, that I have access to, do not have I2C bus wires exposed to some connectors, so both variants would require hardware re-wiring modification of the boards, which is not an option for me. Or do I understand you incorrectly and you mean something different?

> I wonder a bit about the complexity of your patch. In my WIP-branch 
> for 256-byte transfers, I have the following patch. It is only missing 
> the range check for the received byte, but that it easy to add. Do you 
> see anything else missing? If not, I prefer this simpler version 
> because it is less intrusive and the state machine is a bit fragile 
> (due to HW issues with old HW).

Most of complexity in my patch is related to DMA transfers support, that I'm trying to retain for SMBus block data transfers too (for the rest of bytes after the first "length" byte). Your simple patch makes the driver perform all M_RECV_LEN transfers in PIO mode only (with no DMA at all), which is probably not quite good (it's a pity to loose existing HW capability, already supported by the driver).

Also, see a couple of comments below.

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date: Sun, 2 Aug 2020 00:24:52 +0200
> Subject: [PATCH] i2c: rcar: add support for I2C_M_RECV_LEN
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c 
> b/drivers/i2c/busses/i2c-rcar.c index 217def2d7cb4..e473f5c0a708 
> 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -528,6 +528,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv 
> *priv, u32 msr)  static void rcar_i2c_irq_recv(struct rcar_i2c_priv 
> *priv, u32 msr)  {
>  	struct i2c_msg *msg = priv->msg;
> +	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
> 
>  	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
>  	if (!(msr & MDR))
> @@ -542,11 +543,13 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  	} else if (priv->pos < msg->len) {
>  		/* get received data */
>  		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
> +		if (recv_len_init)
> +			msg->len += msg->buf[0];
>  		priv->pos++;
>  	}
> 
>  	/* If next received data is the _LAST_, go to new phase. */
> -	if (priv->pos + 1 == msg->len) {
> +	if (priv->pos + 1 == msg->len && !recv_len_init) {

If a message contains a single byte after the length byte, when we come here after processing the length (in the same function call), "pos" is 1, "len" is 2, and we indeed are going to process the last byte.
However, "recv_len_init" is still "true", and we skip these corresponding register writes, which is probably incorrect.
The flag in this case should be re-set back to "false" after length processing and "pos" moving, but I think the variant in my patch (leaving this "if" unchanged, but skipping it on the first pass with "goto") may be even simpler.

>  		if (priv->flags & ID_LAST_MSG) {
>  			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
>  		} else {
> @@ -889,7 +892,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
>  	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
>  	 */
>  	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> -		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);

This flags setting adds also I2C_FUNC_SMBUS_BLOCK_PROC_CALL flag, which is missed in my patch. My patch should probably be updated to include it too (if you'll agree to take my variant ;-) ).

> 
>  	if (priv->flags & ID_P_HOST_NOTIFY)
>  		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
> 
> Happy hacking,
> 
>    Wolfram

Thanks!

Best regards,
Andrew

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Eugeniu Rosca]

Dear Wolfram,
Dear Andrew,
Dear Geert,

A couple of questions and test results below.

On Thu, Feb 17, 2022 at 08:44:51PM +0100, Wolfram Sang wrote:
> Hi Andrew,
> 
> first sorry that it took so long. The reason here is that my original
> plan was to add 256-byte support to RECV_LEN in the I2C core and enable
> it on R-Car afterwards. Sadly, I never found the time to drive this
> forward. So, all RECV_LEN things got stuck for a while :(
> 
> > This patch (adapted) was tested with v4.14, but due to lack of real
> > hardware with SMBus block read operations support, using "simulation",
> > that is manual analysis of data, read from plain I2C devices with
> > SMBus block read request.
> 
> You could wire up two R-Car I2C instances, set up one as an I2C slave
> handled by the I2C testunit and then use the other instance with
> SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
> Documentation/i2c/slave-testunit-backend.rst for details.

I am obviously not an SMBus expert, but I wonder if simply testing the
PCA9654 I/O Expander with SMBus support on the H3-Salvator-X target
could be acceptable as a test procedure? See some test results below.

> 
> I wonder a bit about the complexity of your patch. In my WIP-branch for
> 256-byte transfers, I have the following patch. It is only missing the
> range check for the received byte, but that it easy to add. Do you see
> anything else missing? If not, I prefer this simpler version because it
> is less intrusive and the state machine is a bit fragile (due to HW
> issues with old HW).
> 
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date: Sun, 2 Aug 2020 00:24:52 +0200
> Subject: [PATCH] i2c: rcar: add support for I2C_M_RECV_LEN
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 217def2d7cb4..e473f5c0a708 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -528,6 +528,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
>  static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  {
>  	struct i2c_msg *msg = priv->msg;
> +	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
>  
>  	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
>  	if (!(msr & MDR))
> @@ -542,11 +543,13 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  	} else if (priv->pos < msg->len) {
>  		/* get received data */
>  		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
> +		if (recv_len_init)
> +			msg->len += msg->buf[0];
>  		priv->pos++;
>  	}
>  
>  	/* If next received data is the _LAST_, go to new phase. */
> -	if (priv->pos + 1 == msg->len) {
> +	if (priv->pos + 1 == msg->len && !recv_len_init) {
>  		if (priv->flags & ID_LAST_MSG) {
>  			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
>  		} else {
> @@ -889,7 +892,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
>  	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
>  	 */
>  	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> -		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
>  
>  	if (priv->flags & ID_P_HOST_NOTIFY)
>  		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
> 

############################################################
################# PATCH-INDEPENDENT OUTPUT #################
############################################################

## .config: https://gist.github.com/erosca/690c3e6065b55546e511f9ef8ba59625
## i2c-tools: https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/commit/?id=cf3541b8a7

root@rcar-gen3:# uname -r
5.17.0+

root@rcar-gen3:# cat /sys/firmware/devicetree/base/model 
Renesas Salvator-X board based on r8a77951

root@rcar-gen3:# i2cdetect -l     
i2c-7   i2c             e60b0000.i2c                            I2C adapter
i2c-2   i2c             e6510000.i2c                            I2C adapter
i2c-4   i2c             e66d8000.i2c                            I2C adapter
    ^
     ` i2c-4 is the PCA9654 I/O Expander with SMBus protocol support

root@rcar-gen3:# i2cdetect -y -r 4
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                         -- -- -- -- -- -- -- -- 
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
20: UU -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60: UU UU UU UU UU UU -- -- 68 -- UU -- -- -- -- -- 
70: UU UU UU UU UU UU -- --      

############################################################
###################### VANILLA v5.17 #######################
############################################################

root@rcar-gen3:# i2cdetect -F 4
Functionalities implemented by /dev/i2c-4:
I2C                              yes
SMBus Quick Command              no
SMBus Send Byte                  yes
SMBus Receive Byte               yes
SMBus Write Byte                 yes
SMBus Read Byte                  yes
SMBus Write Word                 yes
SMBus Read Word                  yes
SMBus Process Call               yes
SMBus Block Write                yes
SMBus Block Read                 no	<<<--- We aim to enable this
SMBus Block Process Call         no
SMBus PEC                        yes
I2C Block Write                  yes
I2C Block Read                   yes

root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08

root@rcar-gen3:# i2cget -y 4 0x68 0 s
Error: Adapter does not have SMBus block read capability

############################################################
#################### ANDREW'S V2 PATCH #####################
############################################################

root@rcar-gen3:# i2cdetect -F 4
Functionalities implemented by /dev/i2c-4:
I2C                              yes
SMBus Quick Command              no
SMBus Send Byte                  yes
SMBus Receive Byte               yes
SMBus Write Byte                 yes
SMBus Read Byte                  yes
SMBus Write Word                 yes
SMBus Read Word                  yes
SMBus Process Call               yes
SMBus Block Write                yes
SMBus Block Read                 yes 	<<<--- Enabled (tested below)
SMBus Block Process Call         no
SMBus PEC                        yes
I2C Block Write                  yes
I2C Block Read                   yes

root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08

root@rcar-gen3:# i2cget -y 4 0x68 0 s
0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08

############################################################
##################### WOLFRAM'S PATCH ######################
############################################################

root@rcar-gen3:# i2cdetect -F 4
Functionalities implemented by /dev/i2c-4:
I2C                              yes
SMBus Quick Command              no
SMBus Send Byte                  yes
SMBus Receive Byte               yes
SMBus Write Byte                 yes
SMBus Read Byte                  yes
SMBus Write Word                 yes
SMBus Read Word                  yes
SMBus Process Call               yes
SMBus Block Write                yes
SMBus Block Read                 yes	<<<--- Enabled (tested)
SMBus Block Process Call         yes	<<<--- Enabled (not tested)
SMBus PEC                        yes
I2C Block Write                  yes
I2C Block Read                   yes

root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08

root@rcar-gen3:# i2cget -y 4 0x68 0 s
0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08

############################################################

Any comments?

Best regards,
Eugeniu

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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

Hi Eugeniu,

coming back to this topic, thanks for your patience everyone.

> > 
> > You could wire up two R-Car I2C instances, set up one as an I2C slave
> > handled by the I2C testunit and then use the other instance with
> > SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
> > Documentation/i2c/slave-testunit-backend.rst for details.
> 
> I am obviously not an SMBus expert, but I wonder if simply testing the
> PCA9654 I/O Expander with SMBus support on the H3-Salvator-X target
> could be acceptable as a test procedure? See some test results below.

As long as the first read value is 8 (or lower than 32), it will work.
But it is testing only this one value while my method above is more
flexible and allows for arbitrary test patterns. However, your tests
already showed that Andrew's patch seems to be not correct.

> ############################################################
> #################### ANDREW'S V2 PATCH #####################
> ############################################################
> root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
> 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08
> 
> root@rcar-gen3:# i2cget -y 4 0x68 0 s
> 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08

This is wrong. The first byte is the length byte and should not be seen
here. Check the i2c_smbus_read_block_data() implementation in i2c-tools.

> ############################################################
> ##################### WOLFRAM'S PATCH ######################
> ############################################################
> root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
> 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08
> 
> root@rcar-gen3:# i2cget -y 4 0x68 0 s
> 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08

This is how it should look like IMO.

Happy hacking,

   Wolfram


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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

Hi Andrew,

thanks for your patience, I can finally work on this issue.

> > You could wire up two R-Car I2C instances, set up one as an I2C slave
> > handled by the I2C testunit and then use the other instance with
> > SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
> > Documentation/i2c/slave-testunit-backend.rst for details.
> 
> You mean physical connection of two R-Car boards via I2C bus,
> or physical connection of I2C bus wires on the single board, right?

I have two instances on the same board wired.

> It looks like all the boards, that I have access to, do not have
> I2C bus wires exposed to some connectors, so both variants would
> require hardware re-wiring modification of the boards, which is
> not an option for me. Or do I understand you incorrectly and you
> mean something different?

Probably you understood correctly. Which boards do you have access to?
I use the EXIO connectors on a Salvator-X(S).

> Most of complexity in my patch is related to DMA transfers support,
> that I'm trying to retain for SMBus block data transfers too (for the rest
> of bytes after the first "length" byte). Your simple patch makes
> the driver perform all M_RECV_LEN transfers in PIO mode only (with no DMA at all),
> which is probably not quite good (it's a pity to loose existing HW capability,
> already supported by the driver).

I will have a look into RECV_LEN and DMA. I already started looking into
it but will need to dive in some more. Stay tuned, I hope to have the
next response ready this week.

> >  	/* If next received data is the _LAST_, go to new phase. */
> > -	if (priv->pos + 1 == msg->len) {
> > +	if (priv->pos + 1 == msg->len && !recv_len_init) {
> 
> If a message contains a single byte after the length byte,
> when we come here after processing the length (in the same function call),
> "pos" is 1, "len" is 2, and we indeed are going to process the last byte.
> However, "recv_len_init" is still "true", and we skip these corresponding
> register writes, which is probably incorrect.
> The flag in this case should be re-set back to "false" after length
> processing and "pos" moving, but I think the variant in my patch
> (leaving this "if" unchanged, but skipping it on the first pass with "goto")
> may be even simpler.

I also need to look into this but thank you already for the detailed
explanation!

> >  	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> > -		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> > +		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
> 
> This flags setting adds also I2C_FUNC_SMBUS_BLOCK_PROC_CALL flag,
> which is missed in my patch. My patch should probably be updated
> to include it too (if you'll agree to take my variant ;-) ).

Yes, the final version, whatever it will be, should use this new macro.

Until soon,

   Wolfram


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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


BTW...

> > ############################################################
> > ##################### WOLFRAM'S PATCH ######################
> > ############################################################
> > root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
> > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08

... for further tests I think the last parameter should be "i 9" here...

> > 
> > root@rcar-gen3:# i2cget -y 4 0x68 0 s
> > 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08

... to see if these 8 bytes match the last 8 bytes from above. The first
byte from above is returned internally as the length. It is not a data
byte.


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Eugeniu Rosca]

Hello Wolfram,

Thanks for your feedback!

On Wed, Mar 30, 2022 at 01:09:17PM +0200, Wolfram Sang wrote:
> 
> BTW...
> 
> > > ############################################################
> > > ##################### WOLFRAM'S PATCH ######################
> > > ############################################################
> > > root@rcar-gen3:# i2cget -y 4 0x68 0 i 8
> > > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08
> 
> ... for further tests I think the last parameter should be "i 9" here...

Your patch re-tested on your request (we'll use -i 9 in the future):

root@rcar-gen3:# i2cget -y 4 0x68 0 s
0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08
root@rcar-gen3:# i2cget -y 4 0x68 0 i 9
0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08
root@rcar-gen3:# i2cget -y 4 0x68 0 i
0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 0xff 0xff 0x7e 0x99 0x3e 0x00 0x00 0xfb 0xff 0xff 0x87 0x27 0xff 0x04 0xff 0x30 0x03 0xfd 0xff 0xff 0xff 0xff 0xff
 
> > > 
> > > root@rcar-gen3:# i2cget -y 4 0x68 0 s
> > > 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08
> 
> ... to see if these 8 bytes match the last 8 bytes from above. The first
> byte from above is returned internally as the length. It is not a data
> byte.
> 

BTW, thanks to Bhuvanesh, we've got another patch [*] which tries
to combine the best of both worlds:

* DMA support in the v1/v2 patches from Andrew/Bhuvanesh
* Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/

Unfortunately, this patch has a dependency to the rcar_i2c_is_pio()
in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 
(which should be resolvable by extracting the function).

Do you think we are on the right track with this new approach or do
you feel the implementation is still overly complicated?

Appreciate your time/feedback.

Best regards,
Eugeniu

[*] v3

From: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
Date: Wed, 26 May 2021 11:36:36 +0530
Subject: [PATCH v3] i2c: rcar: add SMBus block read support

The SMBus block read is currently not supported for rcar i2c devices.

Add appropriate support to R-Car i2c driver, so that blocks of data
can be read using SMbus block reads (using i2c_smbus_read_block_data()
function from i2c-core-smbus.c).

Inspired from:
 - commit 8e8782c71595a5 ("i2c: imx: add SMBus block read support")
 - https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/
   (proposal/suggestion from Wolfram Sang)

Suggested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/i2c/busses/i2c-rcar.c | 39 ++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f71c730f9838..f4f36689464c 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -105,6 +105,7 @@
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
+#define ID_EPROTO	(1 << 5)
 /* persistent flags */
 #define ID_P_HOST_NOTIFY	BIT(28)
 #define ID_P_REP_AFTER_RD	BIT(29)
@@ -522,6 +523,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
@@ -535,12 +537,37 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		rcar_i2c_dma(priv);
 	} else if (priv->pos < msg->len) {
 		/* get received data */
-		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
+		u8 data = rcar_i2c_read(priv, ICRXTX);
+
+		if (recv_len_init) {
+			/*
+			 * First byte is the length of remaining packet
+			 * in the SMBus block data read. Add it to
+			 * msg->len.
+			 */
+			if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) {
+				priv->flags |= ID_DONE | ID_EPROTO;
+				return;
+			}
+			msg->len += data;
+
+			if (!rcar_i2c_is_pio(priv)) {
+				/*
+				 * Still try to use DMA to receive the rest of
+				 * data
+				 */
+				rcar_i2c_dma(priv);
+				goto next_txn;
+			} else {
+				recv_len_init = false;
+			}
+		}
+		msg->buf[priv->pos] = data;
 		priv->pos++;
 	}
 
 	/* If next received data is the _LAST_, go to new phase. */
-	if (priv->pos + 1 == msg->len) {
+	if (priv->pos + 1 == msg->len && !recv_len_init) {
 		if (priv->flags & ID_LAST_MSG) {
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
 		} else {
@@ -549,6 +576,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		}
 	}
 
+next_txn:
 	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
 	else
@@ -847,6 +875,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -909,6 +939,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -975,7 +1007,8 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
 	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
 	 */
 	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
-		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
+		   I2C_FUNC_SMBUS_READ_BLOCK_DATA;
 
 	if (priv->flags & ID_P_HOST_NOTIFY)
 		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
-- 
2.35.1

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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


> I use the EXIO connectors on a Salvator-X(S).

Sorry, I confused things. The EXIO connectors on Salvator-X(S) are not
so helpful. I use the EXIO connectors on a Lager board (R-Car H2) for
testing.


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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

> >  	/* If next received data is the _LAST_, go to new phase. */
> > -	if (priv->pos + 1 == msg->len) {
> > +	if (priv->pos + 1 == msg->len && !recv_len_init) {
> 
> If a message contains a single byte after the length byte,
> when we come here after processing the length (in the same function call),
> "pos" is 1, "len" is 2, and we indeed are going to process the last byte.
> However, "recv_len_init" is still "true", and we skip these corresponding
> register writes, which is probably incorrect.
> The flag in this case should be re-set back to "false" after length
> processing and "pos" moving, but I think the variant in my patch

Confirmed. Tests fail with only one extra byte and clearing
'recv_len_init' fixes the issue. I don't think this is the proper
solution, though. I think it will create more readable code if we update
the checks. So people will understand what we are aiming for. The
current code is already implicit enough.


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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

Hi Eugeniu,

> BTW, thanks to Bhuvanesh, we've got another patch [*] which tries
> to combine the best of both worlds:
> 
> * DMA support in the v1/v2 patches from Andrew/Bhuvanesh
> * Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/

This was nice to see. But where does it come from? I don't see it on
this list and I also couldn't find it in the regular BSP?

> Unfortunately, this patch has a dependency to the rcar_i2c_is_pio()
> in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 
> (which should be resolvable by extracting the function).

This patch is obsolete since March 2019. It has been properly fixed with
94e290b0e9a6 ("i2c: rcar: wait for data empty before starting DMA"). I
am still trying to feed this information back.

> Do you think we are on the right track with this new approach or do
> you feel the implementation is still overly complicated?

The approach is much better but there are still things I don't like. The
use of 'goto next_txn' is bad. I hope it could be done better with
refactoring the code, so DMA will be tried at one place (with two
conditions then). Not sure yet, I am still working on refactoring the
one-byte transfer which is broken with my patch. What we surely can use
from this patch is the -EPROTO handling because I have given up on
converting the max read block size first. We can still remove it from
this driver if that gets implemented somewhen.

> +			if (!rcar_i2c_is_pio(priv)) {
> +				/*
> +				 * Still try to use DMA to receive the rest of
> +				 * data
> +				 */
> +				rcar_i2c_dma(priv);
> +				goto next_txn;
> +			} else {
> +				recv_len_init = false;
> +			}

So, I'd like to get rid of this block with refactoring.

>  	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> -		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
> +		   I2C_FUNC_SMBUS_READ_BLOCK_DATA;

Still not using the new macro to include PROC_CALL, but that is easy to
change.

All the best,

   Wolfram


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Eugeniu Rosca]

Hi Wolfram,

On Fri, Apr 01, 2022 at 06:38:56PM +0200, Wolfram Sang wrote:
> > BTW, thanks to Bhuvanesh, we've got another patch [*] which tries
> > to combine the best of both worlds:
> > 
> > * DMA support in the v1/v2 patches from Andrew/Bhuvanesh
> > * Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/
> 
> This was nice to see. But where does it come from? I don't see it on
> this list and I also couldn't find it in the regular BSP?

The patch was worked on and tested collaboratively w/o submission.
The idea was to push it to LKML, once/after you are happy with it.

> > Unfortunately, this patch has a dependency to the rcar_i2c_is_pio()
> > in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 
> > (which should be resolvable by extracting the function).
> 
> This patch is obsolete since March 2019. It has been properly fixed with
> 94e290b0e9a6 ("i2c: rcar: wait for data empty before starting DMA"). I
> am still trying to feed this information back.

Thanks for the precious feedback. We've requested Renesas to revert the
obsolete BSP commit, based on your recommendation.

In general, the Renesas kernel always carries a set of patches with
non-mainlined changes, Fortunately, for i2c specifically (as opposed
to other subsystems), it is narrow enough to not raise major concerns:

$ git log --oneline v5.10.41..rcar-5.1.2 -- drivers/i2c/busses/i2c-rcar.c
6745303b2bfa i2c: rcar: Add support for r8a77961 (R-Car M3-W+)
3422d3131700 i2c: rcar: Support the suspend/resume
5680e77f2427 i2c: rcar: Tidy up the register order for hardware specification ver1.00.
41394ab7420f i2c: rcar: Fix I2C DMA transmission by setting sequence

> 
> > Do you think we are on the right track with this new approach or do
> > you feel the implementation is still overly complicated?
> 
> The approach is much better but there are still things I don't like. The
> use of 'goto next_txn' is bad. I hope it could be done better with
> refactoring the code, so DMA will be tried at one place (with two
> conditions then). Not sure yet, I am still working on refactoring the
> one-byte transfer which is broken with my patch. What we surely can use
> from this patch is the -EPROTO handling because I have given up on
> converting the max read block size first. We can still remove it from
> this driver if that gets implemented somewhen.

Thank you for the review comments. We are still working on a cleaner
solution. In case it comes from you first, we are very much keen to
give it a try on the target and report the results.

Best regards,
Eugeniu

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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

Hi Eugeniu,

> The idea was to push it to LKML, once/after you are happy with it.

I see.

> Thanks for the precious feedback. We've requested Renesas to revert the
> obsolete BSP commit, based on your recommendation.

Thank you!

> In general, the Renesas kernel always carries a set of patches with
> non-mainlined changes, Fortunately, for i2c specifically (as opposed
> to other subsystems), it is narrow enough to not raise major concerns:

Well, yes, that shows that I am mostly successful with reporting back to
the BSP team :D But some are still missing, as you can see.

> $ git log --oneline v5.10.41..rcar-5.1.2 -- drivers/i2c/busses/i2c-rcar.c
> 6745303b2bfa i2c: rcar: Add support for r8a77961 (R-Car M3-W+)

Can be dropped: "Driver matches against family-specific compatible
value, which has always been present in upstream DTS"

> 3422d3131700 i2c: rcar: Support the suspend/resume

Already upstream: "18569fa89a4db9ed6b5181624788a1574a9b6ed7 # i2c: rcar:
add suspend/resume support"

> 5680e77f2427 i2c: rcar: Tidy up the register order for hardware specification ver1.00.

Relevant parts upstream: "e7f4264821a4ee07775f3775f8530cfa9a6d4b5d #
i2c: rcar: enable interrupts before starting transfer"

Other parts can be dropped.

> 41394ab7420f i2c: rcar: Fix I2C DMA transmission by setting sequence

Upstream: "94e290b0e9a6c360a5660c480c1ba996d892c650 # i2c: rcar: wait
for data empty before starting DMA"

> Thank you for the review comments. We are still working on a cleaner
> solution. In case it comes from you first, we are very much keen to
> give it a try on the target and report the results.

I have a cleaner solution quite ready. Give me another hour for testing
before I send it out.

All the best,

   Wolfram


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

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Eugeniu Rosca]

Hi Wolfram,

On Tue, Apr 05, 2022 at 11:43:29AM +0200, Wolfram Sang wrote:
> > In general, the Renesas kernel always carries a set of patches with
> > non-mainlined changes, Fortunately, for i2c specifically (as opposed
> > to other subsystems), it is narrow enough to not raise major concerns:
> 
> Well, yes, that shows that I am mostly successful with reporting back to
> the BSP team :D

100%! I remember the early days of Renesas R-Car Gen3 kernels, with
literally thousands of patches on top of v4.2/v4.4/v4.6 vanilla tags.

It felt like mission impossible to upstream those. But here we are. Kudos!

Best regards,
Eugeniu

Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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


> > Well, yes, that shows that I am mostly successful with reporting back to
> > the BSP team :D
> 
> 100%! I remember the early days of Renesas R-Car Gen3 kernels, with
> literally thousands of patches on top of v4.2/v4.4/v4.6 vanilla tags.
> 
> It felt like mission impossible to upstream those. But here we are. Kudos!

Wow, thanks! I am glad you appreciate our work!

All the best,

   Wolfram


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