[PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Wolfram Sang]

Hello RCar Fans!

So, here is V3 of this series. After a debugging session with Laurent, we
finally fixed his issue for good. It was not board dependent as we thought, but
toolchain dependent! Hidden by a macro, the driver used a compound assignemt
with a function call as the rvalue. After patch 6, this function also changed the
flags which were to be changed by the compound assignment. Basically (after macro):

	priv->flags |= i_change_priv_flags(priv);

Which is undefined behaviour, I guess. However, after my refactoring, the
called functions always returned 0, so we can simply do:

	i_change_priv_flags(priv);

Nasty one, but finally issue gone, for all toolchains and optimization settings.
Furthermore, patch 11 has been added because HW engineers wanted it.

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/rcar-i2c-rework-v3

Please test, test, test :)

   Wolfram


Changes since V2:
* patch 6/11 was cleaned up to not use the compund assignment
* patch 11/11 introduced as requested by HW engineers

Changes since V1:
* new patch 1/10 to ensure clock is always on
* rebased patch 2/10 to the new patch
* some patch descriptions slightly reworded


Here is the description of the V1 series for those who missed it:

Two issues people have seen with the i2c-rcar driver was:

a) immediately restarted messages after NACK from client
b) duplicated data bytes in messages

Some people already worked on those and had a tough time because it was hard to
reproduce these issues on non-customer setup. Luckily, I somewhen had a state
where the first transfer after boot would always show the above issues on a
plain Renesas Lager board. When measuring, I found a third issue thanks to my
new tool 'i2ctransfer' (and thanks to projects like sigrok and OpenLogicSniffer,
of course. Thank you very much!):

c) after read message, no repeated start was sent, but stop + start.

Due to some unlucky design choices in the IP core, it has some race windows
which can cause problems if interrupts get delayed. Also, for every new message
in one transfer, context switches between interrupt and process were needed.

So I refactored the driver to setup new messages in interrupt context, too.
This avoids the race for b) because we are now setting up the new message
before we release the i2c bus clock (before we released the clock and set up
the message in process context). c) is also fixed, this was not a race but a
bug in the state handling. a) however is not fixed 100% :( We have the race
window as small as possible now when utilizing interrupts, so it is an
improvement and worked for my test cases well. There were experiments by me and
Renesas engineers to use polling to prevent the issue but this caused other
side effects, sadly. So, let's improve the situation now and let's see where we
get.

I did quite some lab testing here and also verified that slave support does not
suffer from these changes. However, I'd really appreciate if people could give
this real-world-testing which is always different.

Please have a look, a test, etc...

Thanks,

   Wolfram


Wolfram Sang (11):
  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
  i2c: rcar: don't issue stop when HW does it automatically
  i2c: rcar: check master irqs before slave irqs
  i2c: rcar: revoke START request early
  i2c: rcar: clean up after refactoring
  i2c: rcar: handle difference in setting up non-first message

 drivers/i2c/busses/i2c-rcar.c | 249 ++++++++++++++++++------------------------
 1 file changed, 106 insertions(+), 143 deletions(-)

-- 
2.1.4

[PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Wolfram Sang]

Hello RCar Fans!

So, here is V3 of this series. After a debugging session with Laurent, we
finally fixed his issue for good. It was not board dependent as we thought, but
toolchain dependent! Hidden by a macro, the driver used a compound assignemt
with a function call as the rvalue. After patch 6, this function also changed the
flags which were to be changed by the compound assignment. Basically (after macro):

	priv->flags |= i_change_priv_flags(priv);

Which is undefined behaviour, I guess. However, after my refactoring, the
called functions always returned 0, so we can simply do:

	i_change_priv_flags(priv);

Nasty one, but finally issue gone, for all toolchains and optimization settings.
Furthermore, patch 11 has been added because HW engineers wanted it.

The branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/rcar-i2c-rework-v3

Please test, test, test :)

   Wolfram


Changes since V2:
* patch 6/11 was cleaned up to not use the compund assignment
* patch 11/11 introduced as requested by HW engineers

Changes since V1:
* new patch 1/10 to ensure clock is always on
* rebased patch 2/10 to the new patch
* some patch descriptions slightly reworded


Here is the description of the V1 series for those who missed it:

Two issues people have seen with the i2c-rcar driver was:

a) immediately restarted messages after NACK from client
b) duplicated data bytes in messages

Some people already worked on those and had a tough time because it was hard to
reproduce these issues on non-customer setup. Luckily, I somewhen had a state
where the first transfer after boot would always show the above issues on a
plain Renesas Lager board. When measuring, I found a third issue thanks to my
new tool 'i2ctransfer' (and thanks to projects like sigrok and OpenLogicSniffer,
of course. Thank you very much!):

c) after read message, no repeated start was sent, but stop + start.

Due to some unlucky design choices in the IP core, it has some race windows
which can cause problems if interrupts get delayed. Also, for every new message
in one transfer, context switches between interrupt and process were needed.

So I refactored the driver to setup new messages in interrupt context, too.
This avoids the race for b) because we are now setting up the new message
before we release the i2c bus clock (before we released the clock and set up
the message in process context). c) is also fixed, this was not a race but a
bug in the state handling. a) however is not fixed 100% :( We have the race
window as small as possible now when utilizing interrupts, so it is an
improvement and worked for my test cases well. There were experiments by me and
Renesas engineers to use polling to prevent the issue but this caused other
side effects, sadly. So, let's improve the situation now and let's see where we
get.

I did quite some lab testing here and also verified that slave support does not
suffer from these changes. However, I'd really appreciate if people could give
this real-world-testing which is always different.

Please have a look, a test, etc...

Thanks,

   Wolfram


Wolfram Sang (11):
  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
  i2c: rcar: don't issue stop when HW does it automatically
  i2c: rcar: check master irqs before slave irqs
  i2c: rcar: revoke START request early
  i2c: rcar: clean up after refactoring
  i2c: rcar: handle difference in setting up non-first message

 drivers/i2c/busses/i2c-rcar.c | 249 ++++++++++++++++++------------------------
 1 file changed, 106 insertions(+), 143 deletions(-)

-- 
2.1.4

[PATCH v3 01/11] i2c: rcar: make sure clocks are on when doing clock calculation

[Wolfram Sang]

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

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>
---
 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 b0ae560b38c308..dac0f5d1945341 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.1.4

[PATCH v3 01/11] i2c: rcar: make sure clocks are on when doing clock calculation

[Wolfram Sang]

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

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>
---
 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 b0ae560b38c308..dac0f5d1945341 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.1.4

[PATCH v3 02/11] i2c: rcar: rework hw init

[Wolfram Sang]

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

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>
---
 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 dac0f5d1945341..efc8de6cc8a2f9 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.1.4

[PATCH v3 02/11] i2c: rcar: rework hw init

[Wolfram Sang]

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

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>
---
 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 dac0f5d1945341..efc8de6cc8a2f9 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.1.4

[PATCH v3 03/11] i2c: rcar: remove unused IOERROR state

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 efc8de6cc8a2f9..746406923a5825 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.1.4

[PATCH v3 03/11] i2c: rcar: remove unused IOERROR state

[Wolfram Sang]

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

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 efc8de6cc8a2f9..746406923a5825 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.1.4

[PATCH v3 04/11] i2c: rcar: remove spinlock

[Wolfram Sang]

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

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>
---
 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 746406923a5825..0f2dc74ab8bcc1 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.1.4

[PATCH v3 04/11] i2c: rcar: remove spinlock

[Wolfram Sang]

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

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>
---
 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 746406923a5825..0f2dc74ab8bcc1 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.1.4

[PATCH v3 05/11] i2c: rcar: refactor setup of a msg

[Wolfram Sang]

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

We want to reuse this function later.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 0f2dc74ab8bcc1..4bd3099865b485 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.1.4

[PATCH v3 05/11] i2c: rcar: refactor setup of a msg

[Wolfram Sang]

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

We want to reuse this function later.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 0f2dc74ab8bcc1..4bd3099865b485 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.1.4

[PATCH v3 06/11] i2c: rcar: init new messages in irq

[Wolfram Sang]

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

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>
---
 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 4bd3099865b485..d91acc11631554 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.1.4

[PATCH v3 06/11] i2c: rcar: init new messages in irq

[Wolfram Sang]

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

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>
---
 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 4bd3099865b485..d91acc11631554 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.1.4

[PATCH v3 07/11] i2c: rcar: don't issue stop when HW does it automatically

[Wolfram Sang]

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

The manual says (55.4.8.6) that HW does automatically send STOP after
NACK was received. My measuerments confirm that.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d91acc11631554..87fccf20fc4c34 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -455,8 +455,8 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 
 	/* Nack */
 	if (msr & MNR) {
-		/* go to stop phase */
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
+		/* HW automatically sends STOP after received NACK */
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
 		rcar_i2c_flags_set(priv, ID_NACK);
 		goto out;
-- 
2.1.4

[PATCH v3 07/11] i2c: rcar: don't issue stop when HW does it automatically

[Wolfram Sang]

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

The manual says (55.4.8.6) that HW does automatically send STOP after
NACK was received. My measuerments confirm that.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d91acc11631554..87fccf20fc4c34 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -455,8 +455,8 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 
 	/* Nack */
 	if (msr & MNR) {
-		/* go to stop phase */
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
+		/* HW automatically sends STOP after received NACK */
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
 		rcar_i2c_flags_set(priv, ID_NACK);
 		goto out;
-- 
2.1.4

[PATCH v3 08/11] i2c: rcar: check master irqs before slave irqs

[Wolfram Sang]

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

Due to the HW design, master IRQs are timing critical, so give them
precedence over slave IRQ.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 87fccf20fc4c34..f237b4fc5b5e55 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -432,19 +432,17 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 {
 	struct rcar_i2c_priv *priv = ptr;
-	irqreturn_t result = IRQ_HANDLED;
 	u32 msr;
 
-	if (rcar_i2c_slave_irq(priv))
-		goto exit;
-
 	msr = rcar_i2c_read(priv, ICMSR);
 
 	/* Only handle interrupts that are currently enabled */
 	msr &= rcar_i2c_read(priv, ICMIER);
 	if (!msr) {
-		result = IRQ_NONE;
-		goto exit;
+		if (rcar_i2c_slave_irq(priv))
+			return IRQ_HANDLED;
+
+		return IRQ_NONE;
 	}
 
 	/* Arbitration lost */
@@ -481,8 +479,7 @@ out:
 		wake_up(&priv->wait);
 	}
 
-exit:
-	return result;
+	return IRQ_HANDLED;
 }
 
 static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
-- 
2.1.4

[PATCH v3 08/11] i2c: rcar: check master irqs before slave irqs

[Wolfram Sang]

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

Due to the HW design, master IRQs are timing critical, so give them
precedence over slave IRQ.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 87fccf20fc4c34..f237b4fc5b5e55 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -432,19 +432,17 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 {
 	struct rcar_i2c_priv *priv = ptr;
-	irqreturn_t result = IRQ_HANDLED;
 	u32 msr;
 
-	if (rcar_i2c_slave_irq(priv))
-		goto exit;
-
 	msr = rcar_i2c_read(priv, ICMSR);
 
 	/* Only handle interrupts that are currently enabled */
 	msr &= rcar_i2c_read(priv, ICMIER);
 	if (!msr) {
-		result = IRQ_NONE;
-		goto exit;
+		if (rcar_i2c_slave_irq(priv))
+			return IRQ_HANDLED;
+
+		return IRQ_NONE;
 	}
 
 	/* Arbitration lost */
@@ -481,8 +479,7 @@ out:
 		wake_up(&priv->wait);
 	}
 
-exit:
-	return result;
+	return IRQ_HANDLED;
 }
 
 static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
-- 
2.1.4

[PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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

If we don't clear START generation as soon as possible, it may cause
another message to be generated, e.g. when receiving NACK in address
phase. To keep the race window as small as possible, we clear it right
at the beginning of the interrupt. We don't need any checks since we
always want to stop START and STOP generation on the next occasion after
we started it.

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.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f237b4fc5b5e55..40979130200935 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -83,6 +83,7 @@
 
 #define RCAR_BUS_PHASE_START	(MDBS | MIE | ESG)
 #define RCAR_BUS_PHASE_DATA	(MDBS | MIE)
+#define RCAR_BUS_MASK_DATA	(~(ESG | FSB) & 0xFF)
 #define RCAR_BUS_PHASE_STOP	(MDBS | MIE | FSB)
 
 #define RCAR_IRQ_SEND	(MNR | MAL | MST | MAT | MDE)
@@ -289,13 +290,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 	if (!(msr & MDE))
 		return;
 
-	/*
-	 * If address transfer phase finished,
-	 * goto data phase.
-	 */
-	if (msr & MAT)
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
-
 	if (priv->pos < msg->len) {
 		/*
 		 * Prepare next data to ICRXTX register.
@@ -345,11 +339,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		return;
 
 	if (msr & MAT) {
-		/*
-		 * Address transfer phase finished,
-		 * but, there is no data at this point.
-		 * Do nothing.
-		 */
+		/* Address transfer phase finished, but no data at this point. */
 	} else if (priv->pos < msg->len) {
 		/*
 		 * get received data
@@ -365,8 +355,6 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	 */
 	if (priv->pos + 1 >= msg->len)
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-	else
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 
 	if (priv->pos = msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
@@ -432,7 +420,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 {
 	struct rcar_i2c_priv *priv = ptr;
-	u32 msr;
+	u32 msr, val;
+
+	/* Clear START or STOP as soon as we can */
+	val = rcar_i2c_read(priv, ICMCR);
+	rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA);
 
 	msr = rcar_i2c_read(priv, ICMSR);
 
@@ -454,7 +446,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	/* Nack */
 	if (msr & MNR) {
 		/* HW automatically sends STOP after received NACK */
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
 		rcar_i2c_flags_set(priv, ID_NACK);
 		goto out;
-- 
2.1.4

[PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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

If we don't clear START generation as soon as possible, it may cause
another message to be generated, e.g. when receiving NACK in address
phase. To keep the race window as small as possible, we clear it right
at the beginning of the interrupt. We don't need any checks since we
always want to stop START and STOP generation on the next occasion after
we started it.

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.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f237b4fc5b5e55..40979130200935 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -83,6 +83,7 @@
 
 #define RCAR_BUS_PHASE_START	(MDBS | MIE | ESG)
 #define RCAR_BUS_PHASE_DATA	(MDBS | MIE)
+#define RCAR_BUS_MASK_DATA	(~(ESG | FSB) & 0xFF)
 #define RCAR_BUS_PHASE_STOP	(MDBS | MIE | FSB)
 
 #define RCAR_IRQ_SEND	(MNR | MAL | MST | MAT | MDE)
@@ -289,13 +290,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 	if (!(msr & MDE))
 		return;
 
-	/*
-	 * If address transfer phase finished,
-	 * goto data phase.
-	 */
-	if (msr & MAT)
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
-
 	if (priv->pos < msg->len) {
 		/*
 		 * Prepare next data to ICRXTX register.
@@ -345,11 +339,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		return;
 
 	if (msr & MAT) {
-		/*
-		 * Address transfer phase finished,
-		 * but, there is no data at this point.
-		 * Do nothing.
-		 */
+		/* Address transfer phase finished, but no data at this point. */
 	} else if (priv->pos < msg->len) {
 		/*
 		 * get received data
@@ -365,8 +355,6 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 	 */
 	if (priv->pos + 1 >= msg->len)
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-	else
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 
 	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
@@ -432,7 +420,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 {
 	struct rcar_i2c_priv *priv = ptr;
-	u32 msr;
+	u32 msr, val;
+
+	/* Clear START or STOP as soon as we can */
+	val = rcar_i2c_read(priv, ICMCR);
+	rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA);
 
 	msr = rcar_i2c_read(priv, ICMSR);
 
@@ -454,7 +446,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	/* Nack */
 	if (msr & MNR) {
 		/* HW automatically sends STOP after received NACK */
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
 		rcar_i2c_flags_set(priv, ID_NACK);
 		goto out;
-- 
2.1.4

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

Hello.

On 11/19/2015 06:56 PM, Wolfram Sang wrote:

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> If we don't clear START generation as soon as possible, it may cause
> another message to be generated, e.g. when receiving NACK in address
> phase. To keep the race window as small as possible, we clear it right
> at the beginning of the interrupt. We don't need any checks since we
> always want to stop START and STOP generation on the next occasion after
> we started it.
>
> 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.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

    Thanks for a great work, Wolfram!
    We need this patch in -stable kernels. The R-Car audio just doesn't work 
without it...

> ---
>   drivers/i2c/busses/i2c-rcar.c | 23 +++++++----------------
>   1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index f237b4fc5b5e55..40979130200935 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -83,6 +83,7 @@
>
>   #define RCAR_BUS_PHASE_START	(MDBS | MIE | ESG)
>   #define RCAR_BUS_PHASE_DATA	(MDBS | MIE)
> +#define RCAR_BUS_MASK_DATA	(~(ESG | FSB) & 0xFF)
>   #define RCAR_BUS_PHASE_STOP	(MDBS | MIE | FSB)
>
>   #define RCAR_IRQ_SEND	(MNR | MAL | MST | MAT | MDE)
> @@ -289,13 +290,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
>   	if (!(msr & MDE))
>   		return;
>
> -	/*
> -	 * If address transfer phase finished,
> -	 * goto data phase.
> -	 */
> -	if (msr & MAT)
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
> -
>   	if (priv->pos < msg->len) {
>   		/*
>   		 * Prepare next data to ICRXTX register.
> @@ -345,11 +339,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>   		return;
>
>   	if (msr & MAT) {
> -		/*
> -		 * Address transfer phase finished,
> -		 * but, there is no data at this point.
> -		 * Do nothing.
> -		 */
> +		/* Address transfer phase finished, but no data at this point. */
>   	} else if (priv->pos < msg->len) {
>   		/*
>   		 * get received data
> @@ -365,8 +355,6 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>   	 */
>   	if (priv->pos + 1 >= msg->len)
>   		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> -	else
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
>
>   	if (priv->pos = msg->len && !(priv->flags & ID_LAST_MSG))
>   		rcar_i2c_next_msg(priv);
> @@ -432,7 +420,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
>   static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
>   {
>   	struct rcar_i2c_priv *priv = ptr;
> -	u32 msr;
> +	u32 msr, val;
> +
> +	/* Clear START or STOP as soon as we can */
> +	val = rcar_i2c_read(priv, ICMCR);
> +	rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA);
>
>   	msr = rcar_i2c_read(priv, ICMSR);
>
> @@ -454,7 +446,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
>   	/* Nack */
>   	if (msr & MNR) {
>   		/* HW automatically sends STOP after received NACK */
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
>   		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
>   		rcar_i2c_flags_set(priv, ID_NACK);
>   		goto out;

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

Hello.

On 11/19/2015 06:56 PM, Wolfram Sang wrote:

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> If we don't clear START generation as soon as possible, it may cause
> another message to be generated, e.g. when receiving NACK in address
> phase. To keep the race window as small as possible, we clear it right
> at the beginning of the interrupt. We don't need any checks since we
> always want to stop START and STOP generation on the next occasion after
> we started it.
>
> 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.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

    Thanks for a great work, Wolfram!
    We need this patch in -stable kernels. The R-Car audio just doesn't work 
without it...

> ---
>   drivers/i2c/busses/i2c-rcar.c | 23 +++++++----------------
>   1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index f237b4fc5b5e55..40979130200935 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -83,6 +83,7 @@
>
>   #define RCAR_BUS_PHASE_START	(MDBS | MIE | ESG)
>   #define RCAR_BUS_PHASE_DATA	(MDBS | MIE)
> +#define RCAR_BUS_MASK_DATA	(~(ESG | FSB) & 0xFF)
>   #define RCAR_BUS_PHASE_STOP	(MDBS | MIE | FSB)
>
>   #define RCAR_IRQ_SEND	(MNR | MAL | MST | MAT | MDE)
> @@ -289,13 +290,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
>   	if (!(msr & MDE))
>   		return;
>
> -	/*
> -	 * If address transfer phase finished,
> -	 * goto data phase.
> -	 */
> -	if (msr & MAT)
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
> -
>   	if (priv->pos < msg->len) {
>   		/*
>   		 * Prepare next data to ICRXTX register.
> @@ -345,11 +339,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>   		return;
>
>   	if (msr & MAT) {
> -		/*
> -		 * Address transfer phase finished,
> -		 * but, there is no data at this point.
> -		 * Do nothing.
> -		 */
> +		/* Address transfer phase finished, but no data at this point. */
>   	} else if (priv->pos < msg->len) {
>   		/*
>   		 * get received data
> @@ -365,8 +355,6 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>   	 */
>   	if (priv->pos + 1 >= msg->len)
>   		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> -	else
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
>
>   	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
>   		rcar_i2c_next_msg(priv);
> @@ -432,7 +420,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
>   static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
>   {
>   	struct rcar_i2c_priv *priv = ptr;
> -	u32 msr;
> +	u32 msr, val;
> +
> +	/* Clear START or STOP as soon as we can */
> +	val = rcar_i2c_read(priv, ICMCR);
> +	rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA);
>
>   	msr = rcar_i2c_read(priv, ICMSR);
>
> @@ -454,7 +446,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
>   	/* Nack */
>   	if (msr & MNR) {
>   		/* HW automatically sends STOP after received NACK */
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
>   		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
>   		rcar_i2c_flags_set(priv, ID_NACK);
>   		goto out;

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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

On Fri, Apr 01, 2016 at 12:02:56AM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 11/19/2015 06:56 PM, Wolfram Sang wrote:
> 
> >From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> >If we don't clear START generation as soon as possible, it may cause
> >another message to be generated, e.g. when receiving NACK in address
> >phase. To keep the race window as small as possible, we clear it right
> >at the beginning of the interrupt. We don't need any checks since we
> >always want to stop START and STOP generation on the next occasion after
> >we started it.
> >
> >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.
> >
> >Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
>    Thanks for a great work, Wolfram!
>    We need this patch in -stable kernels. The R-Car audio just doesn't work
> without it...

Really only this patch? IIRC my tests showed that if you don't remove
the spinlocks (patch 4), the interrupt latency will already be too high
again. In any case, you'd need to do some careful backporting to rip
this out of the whole refactoring series. But maybe you did that already
and have good experiences?


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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

On Fri, Apr 01, 2016 at 12:02:56AM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 11/19/2015 06:56 PM, Wolfram Sang wrote:
> 
> >From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> >If we don't clear START generation as soon as possible, it may cause
> >another message to be generated, e.g. when receiving NACK in address
> >phase. To keep the race window as small as possible, we clear it right
> >at the beginning of the interrupt. We don't need any checks since we
> >always want to stop START and STOP generation on the next occasion after
> >we started it.
> >
> >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.
> >
> >Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
>    Thanks for a great work, Wolfram!
>    We need this patch in -stable kernels. The R-Car audio just doesn't work
> without it...

Really only this patch? IIRC my tests showed that if you don't remove
the spinlocks (patch 4), the interrupt latency will already be too high
again. In any case, you'd need to do some careful backporting to rip
this out of the whole refactoring series. But maybe you did that already
and have good experiences?


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

Hello.

On 04/01/2016 01:48 AM, Wolfram Sang wrote:

>>> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>
>>> If we don't clear START generation as soon as possible, it may cause
>>> another message to be generated, e.g. when receiving NACK in address
>>> phase. To keep the race window as small as possible, we clear it right
>>> at the beginning of the interrupt. We don't need any checks since we
>>> always want to stop START and STOP generation on the next occasion after
>>> we started it.
>>>
>>> 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.
>>>
>>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>>     Thanks for a great work, Wolfram!
>>     We need this patch in -stable kernels. The R-Car audio just doesn't work
>> without it...

> Really only this patch?

    Well, my "reverse" bisection pointed at it. :-)

> IIRC my tests showed that if you don't remove
> the spinlocks (patch 4), the interrupt latency will already be too high
> again.

    Thank you for the valuable info!

> In any case, you'd need to do some careful backporting to rip
> this out of the whole refactoring series.

    Yes, I've already figured that.

> But maybe you did that already
> and have good experiences?

    Not yet, I will report back after more backporting/testing.

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

Hello.

On 04/01/2016 01:48 AM, Wolfram Sang wrote:

>>> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>
>>> If we don't clear START generation as soon as possible, it may cause
>>> another message to be generated, e.g. when receiving NACK in address
>>> phase. To keep the race window as small as possible, we clear it right
>>> at the beginning of the interrupt. We don't need any checks since we
>>> always want to stop START and STOP generation on the next occasion after
>>> we started it.
>>>
>>> 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.
>>>
>>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>>     Thanks for a great work, Wolfram!
>>     We need this patch in -stable kernels. The R-Car audio just doesn't work
>> without it...

> Really only this patch?

    Well, my "reverse" bisection pointed at it. :-)

> IIRC my tests showed that if you don't remove
> the spinlocks (patch 4), the interrupt latency will already be too high
> again.

    Thank you for the valuable info!

> In any case, you'd need to do some careful backporting to rip
> this out of the whole refactoring series.

    Yes, I've already figured that.

> But maybe you did that already
> and have good experiences?

    Not yet, I will report back after more backporting/testing.

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


> >IIRC my tests showed that if you don't remove
> >the spinlocks (patch 4), the interrupt latency will already be too high
> >again.
> 
>    Thank you for the valuable info!

Oh, and add patch 6 to it. The context switches are a major problem in
this.


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


> >IIRC my tests showed that if you don't remove
> >the spinlocks (patch 4), the interrupt latency will already be too high
> >again.
> 
>    Thank you for the valuable info!

Oh, and add patch 6 to it. The context switches are a major problem in
this.


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

On 04/01/2016 11:14 PM, Wolfram Sang wrote:
>
>>> IIRC my tests showed that if you don't remove
>>> the spinlocks (patch 4), the interrupt latency will already be too high
>>> again.
>>
>>     Thank you for the valuable info!
>
> Oh, and add patch 6 to it. The context switches are a major problem in
> this.

    Thanks again! I ended up backporting all the patches up to #9. Not sure 
it'd be acceptable for the -stable kernels since there's a lot of refactors 
involved...

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

On 04/01/2016 11:14 PM, Wolfram Sang wrote:
>
>>> IIRC my tests showed that if you don't remove
>>> the spinlocks (patch 4), the interrupt latency will already be too high
>>> again.
>>
>>     Thank you for the valuable info!
>
> Oh, and add patch 6 to it. The context switches are a major problem in
> this.

    Thanks again! I ended up backporting all the patches up to #9. Not sure 
it'd be acceptable for the -stable kernels since there's a lot of refactors 
involved...

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


>    Thanks again! I ended up backporting all the patches up to #9. Not sure
> it'd be acceptable for the -stable kernels since there's a lot of refactors
> involved...

That's what I thought, too.


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


>    Thanks again! I ended up backporting all the patches up to #9. Not sure
> it'd be acceptable for the -stable kernels since there's a lot of refactors
> involved...

That's what I thought, too.


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

On 4/2/2016 10:27 AM, Wolfram Sang wrote:

>>     Thanks again! I ended up backporting all the patches up to #9. Not sure
>> it'd be acceptable for the -stable kernels since there's a lot of refactors
>> involved...
>
> That's what I thought, too.

    And I still don't feel good about the patch removing the spinlock -- I'm 
not sure it's SMP-safe...

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

On 4/2/2016 10:27 AM, Wolfram Sang wrote:

>>     Thanks again! I ended up backporting all the patches up to #9. Not sure
>> it'd be acceptable for the -stable kernels since there's a lot of refactors
>> involved...
>
> That's what I thought, too.

    And I still don't feel good about the patch removing the spinlock -- I'm 
not sure it's SMP-safe...

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


>    And I still don't feel good about the patch removing the spinlock -- I'm
> not sure it's SMP-safe...

In what scenario?


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


>    And I still don't feel good about the patch removing the spinlock -- I'm
> not sure it's SMP-safe...

In what scenario?


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

On 04/03/2016 11:25 AM, Wolfram Sang wrote:

>>     And I still don't feel good about the patch removing the spinlock -- I'm
>> not sure it's SMP-safe...
>
> In what scenario?

    I've reviewed the patches once again and it looked like they are safe.

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Sergei Shtylyov]

On 04/03/2016 11:25 AM, Wolfram Sang wrote:

>>     And I still don't feel good about the patch removing the spinlock -- I'm
>> not sure it's SMP-safe...
>
> In what scenario?

    I've reviewed the patches once again and it looked like they are safe.

MBR, Sergei

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


> >>    And I still don't feel good about the patch removing the spinlock -- I'm
> >>not sure it's SMP-safe...
> >
> >In what scenario?
> 
>    I've reviewed the patches once again and it looked like they are safe.

Thanks.


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

Re: [PATCH v3 09/11] i2c: rcar: revoke START request early

[Wolfram Sang]

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


> >>    And I still don't feel good about the patch removing the spinlock -- I'm
> >>not sure it's SMP-safe...
> >
> >In what scenario?
> 
>    I've reviewed the patches once again and it looked like they are safe.

Thanks.


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

[PATCH v3 10/11] i2c: rcar: clean up after refactoring

[Wolfram Sang]

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

Update the comments to match current behaviour. Shorten some comments.
Update copyrights.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 40979130200935..e71fd4090247aa 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1,7 +1,8 @@
 /*
  * Driver for the Renesas RCar I2C unit
  *
- * Copyright (C) 2014 Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright (C) 2014-15 Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright (C) 2011-2015 Renesas Electronics Corporation
  *
  * Copyright (C) 2012-14 Renesas Solutions Corp.
  * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
@@ -9,9 +10,6 @@
  * This file is based on the drivers/i2c/busses/i2c-sh7760.c
  * (c) 2005-2008 MSC Vertriebsges.m.b.H, Manuel Lauss <mlau@msc-ge.com>
  *
- * This file used out-of-tree driver i2c-rcar.c
- * Copyright (C) 2011-2012 Renesas Electronics Corporation
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
@@ -245,9 +243,7 @@ scgd_find:
 	dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
 		scl, bus_speed, clk_get_rate(priv->clk), round, cdf, scgd);
 
-	/*
-	 * keep icccr value
-	 */
+	/* keep icccr value */
 	priv->icccr = scgd << cdf_width | cdf;
 
 	return 0;
@@ -282,11 +278,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
 
-	/*
-	 * FIXME
-	 * sometimes, unknown interrupt happened.
-	 * Do nothing
-	 */
+	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDE))
 		return;
 
@@ -330,28 +322,22 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
 
-	/*
-	 * FIXME
-	 * sometimes, unknown interrupt happened.
-	 * Do nothing
-	 */
+	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
 		return;
 
 	if (msr & MAT) {
 		/* Address transfer phase finished, but no data at this point. */
 	} else if (priv->pos < msg->len) {
-		/*
-		 * get received data
-		 */
+		/* get received data */
 		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
 		priv->pos++;
 	}
 
 	/*
-	 * If next received data is the _LAST_,
-	 * go to STOP phase,
-	 * otherwise, go to DATA phase.
+	 * If next received data is the _LAST_, go to STOP phase. Might be
+	 * overwritten by REP START when setting up a new msg. Not elegant
+	 * but the only stable sequence for REP START I have found so far.
 	 */
 	if (priv->pos + 1 >= msg->len)
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-- 
2.1.4

[PATCH v3 10/11] i2c: rcar: clean up after refactoring

[Wolfram Sang]

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

Update the comments to match current behaviour. Shorten some comments.
Update copyrights.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 40979130200935..e71fd4090247aa 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1,7 +1,8 @@
 /*
  * Driver for the Renesas RCar I2C unit
  *
- * Copyright (C) 2014 Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright (C) 2014-15 Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright (C) 2011-2015 Renesas Electronics Corporation
  *
  * Copyright (C) 2012-14 Renesas Solutions Corp.
  * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
@@ -9,9 +10,6 @@
  * This file is based on the drivers/i2c/busses/i2c-sh7760.c
  * (c) 2005-2008 MSC Vertriebsges.m.b.H, Manuel Lauss <mlau@msc-ge.com>
  *
- * This file used out-of-tree driver i2c-rcar.c
- * Copyright (C) 2011-2012 Renesas Electronics Corporation
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
@@ -245,9 +243,7 @@ scgd_find:
 	dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
 		scl, bus_speed, clk_get_rate(priv->clk), round, cdf, scgd);
 
-	/*
-	 * keep icccr value
-	 */
+	/* keep icccr value */
 	priv->icccr = scgd << cdf_width | cdf;
 
 	return 0;
@@ -282,11 +278,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
 
-	/*
-	 * FIXME
-	 * sometimes, unknown interrupt happened.
-	 * Do nothing
-	 */
+	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDE))
 		return;
 
@@ -330,28 +322,22 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
 
-	/*
-	 * FIXME
-	 * sometimes, unknown interrupt happened.
-	 * Do nothing
-	 */
+	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
 		return;
 
 	if (msr & MAT) {
 		/* Address transfer phase finished, but no data at this point. */
 	} else if (priv->pos < msg->len) {
-		/*
-		 * get received data
-		 */
+		/* get received data */
 		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
 		priv->pos++;
 	}
 
 	/*
-	 * If next received data is the _LAST_,
-	 * go to STOP phase,
-	 * otherwise, go to DATA phase.
+	 * If next received data is the _LAST_, go to STOP phase. Might be
+	 * overwritten by REP START when setting up a new msg. Not elegant
+	 * but the only stable sequence for REP START I have found so far.
 	 */
 	if (priv->pos + 1 >= msg->len)
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-- 
2.1.4

[PATCH v3 11/11] i2c: rcar: handle difference in setting up non-first message

[Wolfram Sang]

Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-rcar.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e71fd4090247aa..3ed1f0aa5eeb16 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -92,6 +92,7 @@
 #define RCAR_IRQ_ACK_RECV	(~(MAT | MDR) & 0xFF)
 
 #define ID_LAST_MSG	(1 << 0)
+#define ID_FIRST_MSG	(1 << 1)
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
@@ -254,13 +255,22 @@ 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);
+	/*
+	 * We don't have a testcase but the HW engineers say that the write order
+	 * of ICMSR and ICMCR depends on whether we issue START or REP_START. Since
+	 * it didn't cause a drawback for me, let's rather be safe than sorry.
+	 */
+	if (rcar_i2c_flags_has(priv, ID_FIRST_MSG)) {
+		rcar_i2c_write(priv, ICMSR, 0);
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+	} else {
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+		rcar_i2c_write(priv, ICMSR, 0);
+	}
 	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 }
 
@@ -268,6 +278,7 @@ static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv)
 {
 	priv->msg++;
 	priv->msgs_left--;
+	priv->flags = 0;
 	rcar_i2c_prepare_msg(priv);
 }
 
@@ -482,10 +493,10 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		}
 	}
 
-	/* init data */
+	/* init first message */
 	priv->msg = msgs;
 	priv->msgs_left = num;
-
+	priv->flags = ID_FIRST_MSG;
 	rcar_i2c_prepare_msg(priv);
 
 	time_left = wait_event_timeout(priv->wait,
-- 
2.1.4

[PATCH v3 11/11] i2c: rcar: handle difference in setting up non-first message

[Wolfram Sang]

Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-rcar.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e71fd4090247aa..3ed1f0aa5eeb16 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -92,6 +92,7 @@
 #define RCAR_IRQ_ACK_RECV	(~(MAT | MDR) & 0xFF)
 
 #define ID_LAST_MSG	(1 << 0)
+#define ID_FIRST_MSG	(1 << 1)
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
@@ -254,13 +255,22 @@ 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);
+	/*
+	 * We don't have a testcase but the HW engineers say that the write order
+	 * of ICMSR and ICMCR depends on whether we issue START or REP_START. Since
+	 * it didn't cause a drawback for me, let's rather be safe than sorry.
+	 */
+	if (rcar_i2c_flags_has(priv, ID_FIRST_MSG)) {
+		rcar_i2c_write(priv, ICMSR, 0);
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+	} else {
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+		rcar_i2c_write(priv, ICMSR, 0);
+	}
 	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 }
 
@@ -268,6 +278,7 @@ static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv)
 {
 	priv->msg++;
 	priv->msgs_left--;
+	priv->flags = 0;
 	rcar_i2c_prepare_msg(priv);
 }
 
@@ -482,10 +493,10 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		}
 	}
 
-	/* init data */
+	/* init first message */
 	priv->msg = msgs;
 	priv->msgs_left = num;
-
+	priv->flags = ID_FIRST_MSG;
 	rcar_i2c_prepare_msg(priv);
 
 	time_left = wait_event_timeout(priv->wait,
-- 
2.1.4

Re: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patches.

For the whole series,

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Thursday 19 November 2015 16:56:40 Wolfram Sang wrote:
> Hello RCar Fans!
> 
> So, here is V3 of this series. After a debugging session with Laurent, we
> finally fixed his issue for good. It was not board dependent as we thought,
> but toolchain dependent! Hidden by a macro, the driver used a compound
> assignemt with a function call as the rvalue. After patch 6, this function
> also changed the flags which were to be changed by the compound assignment.
> Basically (after macro):
> 
> 	priv->flags |= i_change_priv_flags(priv);
> 
> Which is undefined behaviour, I guess. However, after my refactoring, the
> called functions always returned 0, so we can simply do:
> 
> 	i_change_priv_flags(priv);
> 
> Nasty one, but finally issue gone, for all toolchains and optimization
> settings. Furthermore, patch 11 has been added because HW engineers wanted
> it.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> renesas/rcar-i2c-rework-v3
> 
> Please test, test, test :)
> 
>    Wolfram
> 
> 
> Changes since V2:
> * patch 6/11 was cleaned up to not use the compund assignment
> * patch 11/11 introduced as requested by HW engineers
> 
> Changes since V1:
> * new patch 1/10 to ensure clock is always on
> * rebased patch 2/10 to the new patch
> * some patch descriptions slightly reworded
> 
> 
> Here is the description of the V1 series for those who missed it:
> 
> Two issues people have seen with the i2c-rcar driver was:
> 
> a) immediately restarted messages after NACK from client
> b) duplicated data bytes in messages
> 
> Some people already worked on those and had a tough time because it was hard
> to reproduce these issues on non-customer setup. Luckily, I somewhen had a
> state where the first transfer after boot would always show the above
> issues on a plain Renesas Lager board. When measuring, I found a third
> issue thanks to my new tool 'i2ctransfer' (and thanks to projects like
> sigrok and OpenLogicSniffer, of course. Thank you very much!):
> 
> c) after read message, no repeated start was sent, but stop + start.
> 
> Due to some unlucky design choices in the IP core, it has some race windows
> which can cause problems if interrupts get delayed. Also, for every new
> message in one transfer, context switches between interrupt and process
> were needed.
> 
> So I refactored the driver to setup new messages in interrupt context, too.
> This avoids the race for b) because we are now setting up the new message
> before we release the i2c bus clock (before we released the clock and set up
> the message in process context). c) is also fixed, this was not a race but
> a bug in the state handling. a) however is not fixed 100% :( We have the
> race window as small as possible now when utilizing interrupts, so it is an
> improvement and worked for my test cases well. There were experiments by me
> and Renesas engineers to use polling to prevent the issue but this caused
> other side effects, sadly. So, let's improve the situation now and let's
> see where we get.
> 
> I did quite some lab testing here and also verified that slave support does
> not suffer from these changes. However, I'd really appreciate if people
> could give this real-world-testing which is always different.
> 
> Please have a look, a test, etc...
> 
> Thanks,
> 
>    Wolfram
> 
> 
> Wolfram Sang (11):
>   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
>   i2c: rcar: don't issue stop when HW does it automatically
>   i2c: rcar: check master irqs before slave irqs
>   i2c: rcar: revoke START request early
>   i2c: rcar: clean up after refactoring
>   i2c: rcar: handle difference in setting up non-first message
> 
>  drivers/i2c/busses/i2c-rcar.c | 249 +++++++++++++++++----------------------
>  1 file changed, 106 insertions(+), 143 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patches.

For the whole series,

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Thursday 19 November 2015 16:56:40 Wolfram Sang wrote:
> Hello RCar Fans!
> 
> So, here is V3 of this series. After a debugging session with Laurent, we
> finally fixed his issue for good. It was not board dependent as we thought,
> but toolchain dependent! Hidden by a macro, the driver used a compound
> assignemt with a function call as the rvalue. After patch 6, this function
> also changed the flags which were to be changed by the compound assignment.
> Basically (after macro):
> 
> 	priv->flags |= i_change_priv_flags(priv);
> 
> Which is undefined behaviour, I guess. However, after my refactoring, the
> called functions always returned 0, so we can simply do:
> 
> 	i_change_priv_flags(priv);
> 
> Nasty one, but finally issue gone, for all toolchains and optimization
> settings. Furthermore, patch 11 has been added because HW engineers wanted
> it.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> renesas/rcar-i2c-rework-v3
> 
> Please test, test, test :)
> 
>    Wolfram
> 
> 
> Changes since V2:
> * patch 6/11 was cleaned up to not use the compund assignment
> * patch 11/11 introduced as requested by HW engineers
> 
> Changes since V1:
> * new patch 1/10 to ensure clock is always on
> * rebased patch 2/10 to the new patch
> * some patch descriptions slightly reworded
> 
> 
> Here is the description of the V1 series for those who missed it:
> 
> Two issues people have seen with the i2c-rcar driver was:
> 
> a) immediately restarted messages after NACK from client
> b) duplicated data bytes in messages
> 
> Some people already worked on those and had a tough time because it was hard
> to reproduce these issues on non-customer setup. Luckily, I somewhen had a
> state where the first transfer after boot would always show the above
> issues on a plain Renesas Lager board. When measuring, I found a third
> issue thanks to my new tool 'i2ctransfer' (and thanks to projects like
> sigrok and OpenLogicSniffer, of course. Thank you very much!):
> 
> c) after read message, no repeated start was sent, but stop + start.
> 
> Due to some unlucky design choices in the IP core, it has some race windows
> which can cause problems if interrupts get delayed. Also, for every new
> message in one transfer, context switches between interrupt and process
> were needed.
> 
> So I refactored the driver to setup new messages in interrupt context, too.
> This avoids the race for b) because we are now setting up the new message
> before we release the i2c bus clock (before we released the clock and set up
> the message in process context). c) is also fixed, this was not a race but
> a bug in the state handling. a) however is not fixed 100% :( We have the
> race window as small as possible now when utilizing interrupts, so it is an
> improvement and worked for my test cases well. There were experiments by me
> and Renesas engineers to use polling to prevent the issue but this caused
> other side effects, sadly. So, let's improve the situation now and let's
> see where we get.
> 
> I did quite some lab testing here and also verified that slave support does
> not suffer from these changes. However, I'd really appreciate if people
> could give this real-world-testing which is always different.
> 
> Please have a look, a test, etc...
> 
> Thanks,
> 
>    Wolfram
> 
> 
> Wolfram Sang (11):
>   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
>   i2c: rcar: don't issue stop when HW does it automatically
>   i2c: rcar: check master irqs before slave irqs
>   i2c: rcar: revoke START request early
>   i2c: rcar: clean up after refactoring
>   i2c: rcar: handle difference in setting up non-first message
> 
>  drivers/i2c/busses/i2c-rcar.c | 249 +++++++++++++++++----------------------
>  1 file changed, 106 insertions(+), 143 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Wolfram Sang]

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

On Thu, Nov 19, 2015 at 04:56:40PM +0100, Wolfram Sang wrote:
> Hello RCar Fans!
> 
> So, here is V3 of this series. After a debugging session with Laurent, we
> finally fixed his issue for good. It was not board dependent as we thought, but
> toolchain dependent! Hidden by a macro, the driver used a compound assignemt
> with a function call as the rvalue. After patch 6, this function also changed the
> flags which were to be changed by the compound assignment. Basically (after macro):
> 
> 	priv->flags |= i_change_priv_flags(priv);
> 
> Which is undefined behaviour, I guess. However, after my refactoring, the
> called functions always returned 0, so we can simply do:
> 
> 	i_change_priv_flags(priv);
> 
> Nasty one, but finally issue gone, for all toolchains and optimization settings.
> Furthermore, patch 11 has been added because HW engineers wanted it.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/rcar-i2c-rework-v3
> 
> Please test, test, test :)
> 
>    Wolfram

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 00/11] i2c: rcar: tackle race conditions in the driver

[Wolfram Sang]

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

On Thu, Nov 19, 2015 at 04:56:40PM +0100, Wolfram Sang wrote:
> Hello RCar Fans!
> 
> So, here is V3 of this series. After a debugging session with Laurent, we
> finally fixed his issue for good. It was not board dependent as we thought, but
> toolchain dependent! Hidden by a macro, the driver used a compound assignemt
> with a function call as the rvalue. After patch 6, this function also changed the
> flags which were to be changed by the compound assignment. Basically (after macro):
> 
> 	priv->flags |= i_change_priv_flags(priv);
> 
> Which is undefined behaviour, I guess. However, after my refactoring, the
> called functions always returned 0, so we can simply do:
> 
> 	i_change_priv_flags(priv);
> 
> Nasty one, but finally issue gone, for all toolchains and optimization settings.
> Furthermore, patch 11 has been added because HW engineers wanted it.
> 
> The branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/rcar-i2c-rework-v3
> 
> Please test, test, test :)
> 
>    Wolfram

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]