[PATCH 0/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[PATCH 0/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Wolfram Sang]

As mentioned in the patch description, this patch works around a HW issue on
Renesas R-Car Gen3 SoCs. It had not been discovered yet because it only happens
when clearing DMA enable bits is too late because of latency. To reproduce the
problem reliably, the below patch was used [1].

To the best of my knowledge, all Gen3 SoCs are affected, so I didn't add any
soc_device_match kind of matching for now. If the situation changes, we can add
it. For now, priv->devtype is good.

The improvement can be shown by using the following command. Before the patch,
executed on a Salvator-XS / R-Car M3-N ES1.0:

# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00
# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00
0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00

Note the broken first byte (0xfe) after the first successful RXDMA transfer.
After the patch (tested with and without CONFIG_RESET_CONTROLLER):

# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00

Looks way better!

Patch was tested with current renesas-drivers/master, but should also apply cleanly
on i2c/for-next or v4.18-rc2. Please test and comment.

Kind regards,

   Wolfram


Wolfram Sang (1):
  i2c: rcar: handle RXDMA HW behaviour on Gen3

 drivers/i2c/busses/i2c-rcar.c | 54 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)


[1] Testcase patch:

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d445b5891280..e42cd0121862 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -353,6 +353,7 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
 	struct dma_chan *chan = priv->dma_direction == DMA_FROM_DEVICE
 		? priv->dma_rx : priv->dma_tx;
 
+	mdelay(10);
 	/* Disable DMA Master Received/Transmitted */
 	rcar_i2c_write(priv, ICDMAER, 0);
 
-- 
2.11.0

[PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Wolfram Sang]

On Gen3, we can only do RXDMA once per transfer reliably. For that, we
must reset the device, then we can have RXDMA once. This patch
implements this. When there is no reset controller or the reset fails,
RXDMA will be blocked completely. Otherwise, it will be disabled after
the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu
Yamasaki, yet completely refactored to handle multiple read messages
within one transfer.

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

Changes since RFC:

* poll reset status bit after reset (special case for I2C IP core).
* refactor I2C reset into seperate function
* improved comments
* use newer reset API (devm_reset_control_get_exclusive)

 drivers/i2c/busses/i2c-rcar.c | 54 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index c67c0b1a7fc9..b95900769a90 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -32,6 +32,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 
 /* register offsets */
@@ -111,8 +112,9 @@
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
 /* persistent flags */
+#define ID_P_NO_RXDMA	(1 << 30) /* HW forbids RXDMA sometimes */
 #define ID_P_PM_BLOCKED	(1 << 31)
-#define ID_P_MASK	ID_P_PM_BLOCKED
+#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NO_RXDMA)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -141,6 +143,8 @@ struct rcar_i2c_priv {
 	struct dma_chan *dma_rx;
 	struct scatterlist sg;
 	enum dma_data_direction dma_direction;
+
+	struct reset_control *rstc;
 };
 
 #define rcar_i2c_priv_to_dev(p)		((p)->adap.dev.parent)
@@ -371,6 +375,11 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
 	dma_unmap_single(chan->device->dev, sg_dma_address(&priv->sg),
 			 sg_dma_len(&priv->sg), priv->dma_direction);
 
+	/* Gen3 can only do one RXDMA per transfer and we just completed it */
+	if (priv->devtype == I2C_RCAR_GEN3 &&
+	    priv->dma_direction == DMA_FROM_DEVICE)
+		priv->flags |= ID_P_NO_RXDMA;
+
 	priv->dma_direction = DMA_NONE;
 }
 
@@ -408,8 +417,9 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	unsigned char *buf;
 	int len;
 
-	/* Do not use DMA if it's not available or for messages < 8 bytes */
-	if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE))
+	/* Do various checks to see if DMA is feasible at all */
+	if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE) ||
+	    (read && priv->flags & ID_P_NO_RXDMA))
 		return;
 
 	if (read) {
@@ -740,6 +750,25 @@ static void rcar_i2c_release_dma(struct rcar_i2c_priv *priv)
 	}
 }
 
+/* I2C is a special case, we need to poll the status of a reset */
+static int rcar_i2c_do_reset(struct rcar_i2c_priv *priv)
+{
+	int i, ret;
+
+	ret = reset_control_reset(priv->rstc);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < LOOP_TIMEOUT; i++) {
+		ret = reset_control_status(priv->rstc);
+		if (ret == 0)
+			return 0;
+		udelay(1);
+	}
+
+	return -ETIMEDOUT;
+}
+
 static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 				struct i2c_msg *msgs,
 				int num)
@@ -751,6 +780,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 	pm_runtime_get_sync(dev);
 
+	/* Gen3 needs a reset before allowing RXDMA once */
+	if (priv->devtype == I2C_RCAR_GEN3) {
+		priv->flags |= ID_P_NO_RXDMA;
+		if (!IS_ERR(priv->rstc)) {
+			ret = rcar_i2c_do_reset(priv);
+			if (ret == 0)
+				priv->flags &= ~ID_P_NO_RXDMA;
+		}
+	}
+
 	rcar_i2c_init(priv);
 
 	ret = rcar_i2c_bus_barrier(priv);
@@ -921,6 +960,15 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto out_pm_put;
 
+	if (priv->devtype == I2C_RCAR_GEN3) {
+		priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+		if (!IS_ERR(priv->rstc)) {
+			ret = reset_control_status(priv->rstc);
+			if (ret < 0)
+				priv->rstc = ERR_PTR(-ENOTSUPP);
+		}
+	}
+
 	/* Stay always active when multi-master to keep arbitration working */
 	if (of_property_read_bool(dev->of_node, "multi-master"))
 		priv->flags |= ID_P_PM_BLOCKED;
-- 
2.11.0

Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jun 28, 2018 at 10:45 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> On Gen3, we can only do RXDMA once per transfer reliably. For that, we
> must reset the device, then we can have RXDMA once. This patch
> implements this. When there is no reset controller or the reset fails,
> RXDMA will be blocked completely. Otherwise, it will be disabled after
> the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu
> Yamasaki, yet completely refactored to handle multiple read messages
> within one transfer.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

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

> @@ -751,6 +780,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>
>         pm_runtime_get_sync(dev);
>
> +       /* Gen3 needs a reset before allowing RXDMA once */
> +       if (priv->devtype == I2C_RCAR_GEN3) {

So on R-Car Gen3 the device is always reset, even if no reads will be done?
Or is that something you cannot check for at this point?

> +               priv->flags |= ID_P_NO_RXDMA;
> +               if (!IS_ERR(priv->rstc)) {
> +                       ret = rcar_i2c_do_reset(priv);
> +                       if (ret == 0)
> +                               priv->flags &= ~ID_P_NO_RXDMA;
> +               }
> +       }
> +
>         rcar_i2c_init(priv);
>
>         ret = rcar_i2c_bus_barrier(priv);
> @@ -921,6 +960,15 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto out_pm_put;
>
> +       if (priv->devtype == I2C_RCAR_GEN3) {
> +               priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +               if (!IS_ERR(priv->rstc)) {
> +                       ret = reset_control_status(priv->rstc);

Why this call and check? To check if .status() is implemented (it always is
on R-Car Gen3, if CONFIG_RESET_CONTROLLER is enabled), and to avoid the
timeout in rcar_i2c_do_reset() on every transfer?

> +                       if (ret < 0)
> +                               priv->rstc = ERR_PTR(-ENOTSUPP);
> +               }
> +       }

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

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Wolfram Sang]

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


> > +       /* Gen3 needs a reset before allowing RXDMA once */
> > +       if (priv->devtype == I2C_RCAR_GEN3) {
> 
> So on R-Car Gen3 the device is always reset, even if no reads will be done?
> Or is that something you cannot check for at this point?

You got a point. I copied the behaviour from the BSP to do this as early
as possible. Still, it might be worthwhile to merge this into
rcar_i2c_request_dma() and move the call to it to the front. Will check.

> > +       if (priv->devtype == I2C_RCAR_GEN3) {
> > +               priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > +               if (!IS_ERR(priv->rstc)) {
> > +                       ret = reset_control_status(priv->rstc);
> 
> Why this call and check? To check if .status() is implemented (it always is
> on R-Car Gen3, if CONFIG_RESET_CONTROLLER is enabled), and to avoid the
> timeout in rcar_i2c_do_reset() on every transfer?

Yes. We can save all the hazzle if it is not implemented. And I didn't
want to implicitly assume that it will be implemented anywhere forever.
That could be a very subtle bug later.

Thanks,

   Wolfram


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

Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Wolfram Sang]

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

On Fri, Jun 29, 2018 at 09:54:04AM +0200, Wolfram Sang wrote:
> 
> > > +       /* Gen3 needs a reset before allowing RXDMA once */
> > > +       if (priv->devtype == I2C_RCAR_GEN3) {
> > 
> > So on R-Car Gen3 the device is always reset, even if no reads will be done?
> > Or is that something you cannot check for at this point?
> 
> You got a point. I copied the behaviour from the BSP to do this as early
> as possible. Still, it might be worthwhile to merge this into
> rcar_i2c_request_dma() and move the call to it to the front. Will check.

I will apply this patch as is because it is easier to backport to stable
this way.

You are still right, we can do better and not issue a reset any time. To
get there, though, I want two kinds of refactoring beforehand. And I
think it is best to do this on top of this patch and refactor it along.
BSP backporters can then decide if they want the refactored version or
not.


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

Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Wolfram Sang]

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

On Thu, Jun 28, 2018 at 10:45:38PM +0200, Wolfram Sang wrote:
> On Gen3, we can only do RXDMA once per transfer reliably. For that, we
> must reset the device, then we can have RXDMA once. This patch
> implements this. When there is no reset controller or the reset fails,
> RXDMA will be blocked completely. Otherwise, it will be disabled after
> the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu
> Yamasaki, yet completely refactored to handle multiple read messages
> within one transfer.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3

[Wolfram Sang]

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

On Mon, Jul 23, 2018 at 07:55:22PM +0200, Wolfram Sang wrote:
> On Thu, Jun 28, 2018 at 10:45:38PM +0200, Wolfram Sang wrote:
> > On Gen3, we can only do RXDMA once per transfer reliably. For that, we
> > must reset the device, then we can have RXDMA once. This patch
> > implements this. When there is no reset controller or the reset fails,
> > RXDMA will be blocked completely. Otherwise, it will be disabled after
> > the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu
> > Yamasaki, yet completely refactored to handle multiple read messages
> > within one transfer.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Applied to for-next, thanks!

Reverted it from for-next, added to for-current and added stable.


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