[PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Fabrizio Castro]

Hello Greg,

this series fixes an issue with the I2C driver of the Renesas R-Car and
RZ/G1 family of chips. The issue is clearly visible with the CIP kernel
(4.4) running on a iwg20d board from iWave due to the way the bq32000
driver/device is interacting with the I2C driver/controller.
In the stable kernel (4.4) there is no support for the iwg20d, I tried
to replicate the same problem on a Koelsch board with no success, but
the problem is there.

Do you think this series is suitable for (4.4) stable considering I
can't reproduce the problem on the Koelsch board?

This patches apply on top of 4.4.133.

Thanks,
Fab

Wolfram Sang (6):
  i2c: rcar: make sure clocks are on when doing clock calculation
  i2c: rcar: rework hw init
  i2c: rcar: remove unused IOERROR state
  i2c: rcar: remove spinlock
  i2c: rcar: refactor setup of a msg
  i2c: rcar: init new messages in irq

 drivers/i2c/busses/i2c-rcar.c | 166 ++++++++++++++++++------------------------
 1 file changed, 72 insertions(+), 94 deletions(-)

-- 
2.7.4

[PATCH 1/6] i2c: rcar: make sure clocks are on when doing clock calculation

[Fabrizio Castro]

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

commit e43e0df13f8528ca55ed79f469c4b2af897fa796 upstream.

When calculating the bus speed, the clock should be on, of course. Most
bootloaders left them on, so this went unnoticed so far.

Move the ioremapping out of this clock-enabled-block and prepare for
adding hw initialization there, too.

Reported-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
[fabrizio: cherry-picked to 4.4]
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
---
 drivers/i2c/busses/i2c-rcar.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 599c0d7..fef53c1 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -650,19 +650,23 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->clk);
 	}
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->io = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->io))
+		return PTR_ERR(priv->io);
+
 	bus_speed = 100000; /* default 100 kHz */
 	of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed);
 
 	priv->devtype = (enum rcar_i2c_type)of_match_device(rcar_i2c_dt_ids, dev)->data;
 
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 	ret = rcar_i2c_clock_calculate(priv, bus_speed, dev);
 	if (ret < 0)
-		return ret;
+		goto out_pm_put;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	priv->io = devm_ioremap_resource(dev, res);
-	if (IS_ERR(priv->io))
-		return PTR_ERR(priv->io);
+	pm_runtime_put(dev);
 
 	irq = platform_get_irq(pdev, 0);
 	init_waitqueue_head(&priv->wait);
@@ -682,22 +686,26 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 			       dev_name(dev), priv);
 	if (ret < 0) {
 		dev_err(dev, "cannot get irq %d\n", irq);
-		return ret;
+		goto out_pm_disable;
 	}
 
-	pm_runtime_enable(dev);
 	platform_set_drvdata(pdev, priv);
 
 	ret = i2c_add_numbered_adapter(adap);
 	if (ret < 0) {
 		dev_err(dev, "reg adap failed: %d\n", ret);
-		pm_runtime_disable(dev);
-		return ret;
+		goto out_pm_disable;
 	}
 
 	dev_info(dev, "probed\n");
 
 	return 0;
+
+ out_pm_put:
+	pm_runtime_put(dev);
+ out_pm_disable:
+	pm_runtime_disable(dev);
+	return ret;
 }
 
 static int rcar_i2c_remove(struct platform_device *pdev)
-- 
2.7.4

[PATCH 2/6] i2c: rcar: rework hw init

[Fabrizio Castro]

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

commit 2c78cdc1c06308a59d6ed4145cdba73fdeff8c0d upstream.

We don't need to init HW before every transfer since we know the HW
state then. HW init at probe time is enough. While here, add setting the
clock register which belongs to init HW. Also, set MDBS bit since not
setting it is prohibited according to the manual.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
[fabrizio: cherry-pick to 4.4]
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
---
 drivers/i2c/busses/i2c-rcar.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index fef53c1..b8ff5f2 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -144,9 +144,10 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 {
 	/* reset master mode */
 	rcar_i2c_write(priv, ICMIER, 0);
-	rcar_i2c_write(priv, ICMCR, 0);
+	rcar_i2c_write(priv, ICMCR, MDBS);
 	rcar_i2c_write(priv, ICMSR, 0);
-	rcar_i2c_write(priv, ICMAR, 0);
+	/* start clock */
+	rcar_i2c_write(priv, ICCCR, priv->icccr);
 }
 
 static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
@@ -496,16 +497,6 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 	pm_runtime_get_sync(dev);
 
-	/*-------------- spin lock -----------------*/
-	spin_lock_irqsave(&priv->lock, flags);
-
-	rcar_i2c_init(priv);
-	/* start clock */
-	rcar_i2c_write(priv, ICCCR, priv->icccr);
-
-	spin_unlock_irqrestore(&priv->lock, flags);
-	/*-------------- spin unlock -----------------*/
-
 	ret = rcar_i2c_bus_barrier(priv);
 	if (ret < 0)
 		goto out;
@@ -666,6 +657,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto out_pm_put;
 
+	rcar_i2c_init(priv);
 	pm_runtime_put(dev);
 
 	irq = platform_get_irq(pdev, 0);
-- 
2.7.4

[PATCH 3/6] i2c: rcar: remove unused IOERROR state

[Fabrizio Castro]

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

commit 90f779e565bdc18dd4f79d81cf11f43a7266010b upstream.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
[fabrizio: cherry-pick to 4.4]
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
---
 drivers/i2c/busses/i2c-rcar.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index b8ff5f2..7510733 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -94,7 +94,6 @@
 #define RCAR_IRQ_ACK_RECV	(~(MAT | MDR) & 0xFF)
 
 #define ID_LAST_MSG	(1 << 0)
-#define ID_IOERROR	(1 << 1)
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
@@ -541,11 +540,6 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 			break;
 		}
 
-		if (rcar_i2c_flags_has(priv, ID_IOERROR)) {
-			ret = -EIO;
-			break;
-		}
-
 		ret = i + 1; /* The number of transfer */
 	}
 out:
-- 
2.7.4

[PATCH 4/6] i2c: rcar: remove spinlock

[Fabrizio Castro]

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

commit ff2316b87a336bff940939cd9fc56287ed48e578 upstream.

After making sure to reinit the HW and clear interrupts in the timeout
case, we know that interrupts are always disabled in the sections
protected by the spinlock. Thus, we can simply remove it which is a
preparation for further refactoring. While here, rename the timeout
variable to time_left which is way more readable.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
[fabrizio: cherry-pick to 4.4]
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
---
 drivers/i2c/busses/i2c-rcar.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 7510733..46b6a5f 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -33,7 +33,6 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
-#include <linux/spinlock.h>
 
 /* register offsets */
 #define ICSCR	0x00	/* slave ctrl */
@@ -110,7 +109,6 @@ struct rcar_i2c_priv {
 	struct i2c_msg	*msg;
 	struct clk *clk;
 
-	spinlock_t lock;
 	wait_queue_head_t wait;
 
 	int pos;
@@ -429,9 +427,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	irqreturn_t result = IRQ_HANDLED;
 	u32 msr;
 
-	/*-------------- spin lock -----------------*/
-	spin_lock(&priv->lock);
-
 	if (rcar_i2c_slave_irq(priv))
 		goto exit;
 
@@ -478,9 +473,6 @@ out:
 	}
 
 exit:
-	spin_unlock(&priv->lock);
-	/*-------------- spin unlock -----------------*/
-
 	return result;
 }
 
@@ -490,9 +482,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 {
 	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
-	unsigned long flags;
 	int i, ret;
-	long timeout;
+	long time_left;
 
 	pm_runtime_get_sync(dev);
 
@@ -507,9 +498,6 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 			break;
 		}
 
-		/*-------------- spin lock -----------------*/
-		spin_lock_irqsave(&priv->lock, flags);
-
 		/* init each data */
 		priv->msg	= &msgs[i];
 		priv->pos	= 0;
@@ -519,13 +507,11 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 		rcar_i2c_prepare_msg(priv);
 
-		spin_unlock_irqrestore(&priv->lock, flags);
-		/*-------------- spin unlock -----------------*/
-
-		timeout = wait_event_timeout(priv->wait,
+		time_left = wait_event_timeout(priv->wait,
 					     rcar_i2c_flags_has(priv, ID_DONE),
 					     adap->timeout);
-		if (!timeout) {
+		if (!time_left) {
+			rcar_i2c_init(priv);
 			ret = -ETIMEDOUT;
 			break;
 		}
@@ -656,7 +642,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	init_waitqueue_head(&priv->wait);
-	spin_lock_init(&priv->lock);
 
 	adap = &priv->adap;
 	adap->nr = pdev->id;
-- 
2.7.4

[PATCH 5/6] i2c: rcar: refactor setup of a msg

[Fabrizio Castro]

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

commit b9d0684c79c4b9d30ce0d47d3270493dd0e76e59 upstream.

We want to reuse this function later.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
[fabrizio: cherry-pick to 4.4]
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
---
 drivers/i2c/busses/i2c-rcar.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 46b6a5f..7a4d8d2 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -106,7 +106,8 @@ enum rcar_i2c_type {
 struct rcar_i2c_priv {
 	void __iomem *io;
 	struct i2c_adapter adap;
-	struct i2c_msg	*msg;
+	struct i2c_msg *msg;
+	int msgs_left;
 	struct clk *clk;
 
 	wait_queue_head_t wait;
@@ -255,6 +256,11 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 {
 	int read = !!rcar_i2c_is_recv(priv);
 
+	priv->pos = 0;
+	priv->flags = 0;
+	if (priv->msgs_left == 1)
+		rcar_i2c_flags_set(priv, ID_LAST_MSG);
+
 	rcar_i2c_write(priv, ICMAR, (priv->msg->addr << 1) | read);
 	rcar_i2c_write(priv, ICMSR, 0);
 	rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
@@ -499,11 +505,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		}
 
 		/* init each data */
-		priv->msg	= &msgs[i];
-		priv->pos	= 0;
-		priv->flags	= 0;
-		if (i == num - 1)
-			rcar_i2c_flags_set(priv, ID_LAST_MSG);
+		priv->msg = &msgs[i];
+		priv->msgs_left = num - i;
 
 		rcar_i2c_prepare_msg(priv);
 
-- 
2.7.4

[PATCH 6/6] i2c: rcar: init new messages in irq

[Fabrizio Castro]

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

commit cc21d0b4b62e41e5013d763adade5ea4462c33a4 upstream.

Setting up new messages was done in process context while handling a
message was in interrupt context. Because of the HW design, this IP core
is sensitive to timing, so the context switches were too expensive. Move
this setup to interrupt context as well.

In my test setup, this fixed the occasional 'data byte sent twice' issue
which a number of people have seen. It also fixes to send REP_START
after a read message which was wrongly send as a STOP + START sequence
before.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
[fabrizio: cherry-pick to 4.4]
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
---
 drivers/i2c/busses/i2c-rcar.c | 90 +++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 7a4d8d2..44662e2 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -267,10 +267,17 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 }
 
+static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv)
+{
+	priv->msg++;
+	priv->msgs_left--;
+	rcar_i2c_prepare_msg(priv);
+}
+
 /*
  *		interrupt functions
  */
-static int rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
+static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
 
@@ -280,7 +287,7 @@ static int rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 	 * Do nothing
 	 */
 	if (!(msr & MDE))
-		return 0;
+		return;
 
 	/*
 	 * If address transfer phase finished,
@@ -309,29 +316,23 @@ static int rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 		 * [ICRXTX] -> [SHIFT] -> [I2C bus]
 		 */
 
-		if (priv->flags & ID_LAST_MSG)
+		if (priv->flags & ID_LAST_MSG) {
 			/*
 			 * If current msg is the _LAST_ msg,
 			 * prepare stop condition here.
 			 * ID_DONE will be set on STOP irq.
 			 */
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-		else
-			/*
-			 * If current msg is _NOT_ last msg,
-			 * it doesn't call stop phase.
-			 * thus, there is no STOP irq.
-			 * return ID_DONE here.
-			 */
-			return ID_DONE;
+		} else {
+			rcar_i2c_next_msg(priv);
+			return;
+		}
 	}
 
 	rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_SEND);
-
-	return 0;
 }
 
-static int rcar_i2c_irq_recv(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;
 
@@ -341,7 +342,7 @@ static int rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	 * Do nothing
 	 */
 	if (!(msr & MDR))
-		return 0;
+		return;
 
 	if (msr & MAT) {
 		/*
@@ -367,9 +368,10 @@ static int rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	else
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 
-	rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV);
-
-	return 0;
+	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
+		rcar_i2c_next_msg(priv);
+	else
+		rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV);
 }
 
 static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
@@ -462,14 +464,15 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 
 	/* Stop */
 	if (msr & MST) {
+		priv->msgs_left--; /* The last message also made it */
 		rcar_i2c_flags_set(priv, ID_DONE);
 		goto out;
 	}
 
 	if (rcar_i2c_is_recv(priv))
-		rcar_i2c_flags_set(priv, rcar_i2c_irq_recv(priv, msr));
+		rcar_i2c_irq_recv(priv, msr);
 	else
-		rcar_i2c_flags_set(priv, rcar_i2c_irq_send(priv, msr));
+		rcar_i2c_irq_send(priv, msr);
 
 out:
 	if (rcar_i2c_flags_has(priv, ID_DONE)) {
@@ -501,35 +504,28 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		/* This HW can't send STOP after address phase */
 		if (msgs[i].len == 0) {
 			ret = -EOPNOTSUPP;
-			break;
-		}
-
-		/* init each data */
-		priv->msg = &msgs[i];
-		priv->msgs_left = num - i;
-
-		rcar_i2c_prepare_msg(priv);
-
-		time_left = wait_event_timeout(priv->wait,
-					     rcar_i2c_flags_has(priv, ID_DONE),
-					     adap->timeout);
-		if (!time_left) {
-			rcar_i2c_init(priv);
-			ret = -ETIMEDOUT;
-			break;
-		}
-
-		if (rcar_i2c_flags_has(priv, ID_NACK)) {
-			ret = -ENXIO;
-			break;
-		}
-
-		if (rcar_i2c_flags_has(priv, ID_ARBLOST)) {
-			ret = -EAGAIN;
-			break;
+			goto out;
 		}
+	}
 
-		ret = i + 1; /* The number of transfer */
+	/* init data */
+	priv->msg = msgs;
+	priv->msgs_left = num;
+
+	rcar_i2c_prepare_msg(priv);
+
+	time_left = wait_event_timeout(priv->wait,
+				     rcar_i2c_flags_has(priv, ID_DONE),
+				     num * adap->timeout);
+	if (!time_left) {
+		rcar_i2c_init(priv);
+		ret = -ETIMEDOUT;
+	} else if (rcar_i2c_flags_has(priv, ID_NACK)) {
+		ret = -ENXIO;
+	} else if (rcar_i2c_flags_has(priv, ID_ARBLOST)) {
+		ret = -EAGAIN;
+	} else {
+		ret = num - priv->msgs_left; /* The number of transfer */
 	}
 out:
 	pm_runtime_put(dev);
-- 
2.7.4

Re: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Wolfram Sang]

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


> this series fixes an issue with the I2C driver of the Renesas R-Car and
> RZ/G1 family of chips. The issue is clearly visible with the CIP kernel
> (4.4) running on a iwg20d board from iWave due to the way the bq32000
> driver/device is interacting with the I2C driver/controller.
> In the stable kernel (4.4) there is no support for the iwg20d, I tried
> to replicate the same problem on a Koelsch board with no success, but
> the problem is there.

For the record, this patchset was developed on a Lager board (R-Car H2)
but the issue was known to be present on other Gen2 SoCs, too. This was
a nasty race condition. IIRC I was able to reproduce the issue only with
the first transfer after boot (for whatever reason). And fixing it,
together with another issue (double address byte), needed all this
refactoring.

I might try to recap how this double data byte problem was triggered.
This is somewhere on my todo-list anyhow. Just to check if the same
problem is still present with Gen3 SoCs. No promises, but I'll see what
I can do.


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

Re: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Wolfram Sang]

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


> the first transfer after boot (for whatever reason). And fixing it,
> together with another issue (double address byte), needed all this
> refactoring.

If you want this double address problem fixed, too, you need more
patches of this series up to commit 52df445f29b7 ("i2c: rcar: revoke
START request early").


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

RE: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Fabrizio Castro]

Hello Wolfram,

Thank you for your feedback.

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: 29 May 2018 18:26
> To: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-i2c@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>;
> Biju Das <biju.das@bp.renesas.com>; stable@vger.kernel.org; Ben Hutchings <ben.hutchings@codethink.co.uk>
> Subject: Re: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue
>
>
> > the first transfer after boot (for whatever reason). And fixing it,
> > together with another issue (double address byte), needed all this
> > refactoring.
>
> If you want this double address problem fixed, too, you need more
> patches of this series up to commit 52df445f29b7 ("i2c: rcar: revoke
> START request early").

I'll send a new series to fix the double address byte issue.

With respect to the changelog for commit 52df445f29b7 ("i2c: rcar: revoke
START request early"), in particular:
"This patch improves the situation but sadly does not completely fix it.
It is still to be researched if we can do better given this HW design."
This is still the best we can do so far, isn't it?

Thanks,
Fab





Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Wolfram Sang]

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


> With respect to the changelog for commit 52df445f29b7 ("i2c: rcar: revoke
> START request early"), in particular:
> "This patch improves the situation but sadly does not completely fix it.
> It is still to be researched if we can do better given this HW design."
> This is still the best we can do so far, isn't it?

Yes, I think so. Gen3 seems to have this fixed in HW.


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

RE: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Fabrizio Castro]

Hello Wolfram,

Thank you for your feedback!

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: 29 May 2018 18:20
> To: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-i2c@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>;
> Biju Das <biju.das@bp.renesas.com>; stable@vger.kernel.org; Ben Hutchings <ben.hutchings@codethink.co.uk>
> Subject: Re: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue
>
>
> > this series fixes an issue with the I2C driver of the Renesas R-Car and
> > RZ/G1 family of chips. The issue is clearly visible with the CIP kernel
> > (4.4) running on a iwg20d board from iWave due to the way the bq32000
> > driver/device is interacting with the I2C driver/controller.
> > In the stable kernel (4.4) there is no support for the iwg20d, I tried
> > to replicate the same problem on a Koelsch board with no success, but
> > the problem is there.
>
> For the record, this patchset was developed on a Lager board (R-Car H2)
> but the issue was known to be present on other Gen2 SoCs, too. This was
> a nasty race condition. IIRC I was able to reproduce the issue only with
> the first transfer after boot (for whatever reason). And fixing it,
> together with another issue (double address byte), needed all this
> refactoring.

Thank you for the information, I may give it a try.

>
> I might try to recap how this double data byte problem was triggered.
> This is somewhere on my todo-list anyhow. Just to check if the same
> problem is still present with Gen3 SoCs. No promises, but I'll see what
> I can do.

Thanks,
Fab




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH 0/6] Fix R-Car I2C data byte sent twice issue

[Greg Kroah-Hartman]

On Tue, May 29, 2018 at 02:33:10PM +0100, Fabrizio Castro wrote:
> Hello Greg,
> 
> this series fixes an issue with the I2C driver of the Renesas R-Car and
> RZ/G1 family of chips. The issue is clearly visible with the CIP kernel
> (4.4) running on a iwg20d board from iWave due to the way the bq32000
> driver/device is interacting with the I2C driver/controller.
> In the stable kernel (4.4) there is no support for the iwg20d, I tried
> to replicate the same problem on a Koelsch board with no success, but
> the problem is there.
> 
> Do you think this series is suitable for (4.4) stable considering I
> can't reproduce the problem on the Koelsch board?

It looks good, all now queued up.

greg k-h