linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] i2c-sh_mobile non-urgent changes
@ 2012-10-24 10:55 Shinya Kuribayashi
  2012-10-24 10:57 ` [PATCH 2/5] i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed Shinya Kuribayashi
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-10-24 10:55 UTC (permalink / raw)
  To: w.sang, ben-linux, magnus.damm; +Cc: linux-i2c, linux-arm-kernel, linux-sh

Hello,

This is the first batch to fix the i2c-sh_mobile driver.  As a first
step, this comprises of the SCL optimization work and a simple fix to
annoying spurious WAIT interrupt issue; in other words, this does not
change tx/rx data handling logic.

This driver has an architectural problem with send/receive procedures
and needs a fundamental overhaul.  I've been working on splitting into
logical chunks, but not yet completed.

Patches are prepared against the vanilla v3.6.  I don't have a chance
to give it a try with v3.6+ kernels myself, but tend to be optimistic
about that.  I have been cooking these patches over a year, and they
work perfectly fine with older kernels ..v3.4 so far.


Shinya Kuribayashi (5):
      i2c: i2c-sh_mobile: calculate clock parameters at driver probing time
      i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed
      i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec
      i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock
      i2c: i2c-sh_mobile: fix spurious transfer request timed out

 drivers/i2c/busses/i2c-sh_mobile.c | 150 ++++++++++++++++++++++++-------------
 include/linux/i2c/i2c-sh_mobile.h  |   1 +
 2 files changed, 98 insertions(+), 53 deletions(-)

--
Shinya Kuribayashi
Renesas Electronics

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] i2c: i2c-sh_mobile: calculate clock parameters at driver probing time
       [not found] ` <5087C93F.6080601-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
@ 2012-10-24 10:56   ` Shinya Kuribayashi
  2012-11-16  8:07   ` [PATCH 0/5] i2c-sh_mobile non-urgent changes Wolfram Sang
  1 sibling, 0 replies; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-10-24 10:56 UTC (permalink / raw)
  To: w.sang-bIcnvbaLZ9MEGnE8C9+IrQ, ben-linux-elnMNo+KYs3YtjvyW6yDsg,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w
  Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sh-u79uwXL29TY76Z2rM5mHXA

Currently SCL clock parameters (ICCH/ICCL) are calculated in
activate_ch(), which gets called every time sh_mobile_i2c_xfer() is
processed, while each I2C bus speed is system-defined and in general
those parameters do not have to be updated over I2C transactions.

The only reason I could see having it transaction-time is to adjust
ICCH/ICCL values according to the operating frequency of the I2C
hardware block, in the face of DFS (Dynamic Frequency Scaling).

However, this won't be necessary.

The operating frequency of the I2C hardware block can change _even_
in the middle of I2C transactions.  There is no way to prevent it
from happening, and I2C hardware block can work with such dynamic
frequency change, of course.

Another is that ICCH/ICCL clock parameters optimized for the faster
operating frequency, can also be applied to the slower operating
frequency, as long as slave devices work.  However, the converse is
not true.  It would violate SCL timing specs of the I2C standard.

What we can do now is to calculate the ICCH/ICCL clock parameters
according to the fastest operating clock of the I2C hardware block.
And if that's the case, that calculation should be done just once
at driver-module-init time.

This patch moves ICCH/ICCL calculating part from activate_ch() into
sh_mobile_i2c_init(), and call it from sh_mobile_i2c_probe().

Note that sh_mobile_i2c_init() just prepares clock parameters using
the clock rate and platform data provided, but does _not_ make any
hardware I/O accesses.  We don't have to care about run-time PM
maintenance here.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 8110ca4..309d0d5 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -187,18 +187,15 @@ static void iic_set_clr(struct sh_mobile_i2c_data *pd, int offs,
 	iic_wr(pd, offs, (iic_rd(pd, offs) | set) & ~clr);
 }
 
-static void activate_ch(struct sh_mobile_i2c_data *pd)
+static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 {
 	unsigned long i2c_clk;
 	u_int32_t num;
 	u_int32_t denom;
 	u_int32_t tmp;
 
-	/* Wake up device and enable clock */
-	pm_runtime_get_sync(pd->dev);
-	clk_enable(pd->clk);
-
 	/* Get clock rate after clock is enabled */
+	clk_enable(pd->clk);
 	i2c_clk = clk_get_rate(pd->clk);
 
 	/* Calculate the value for iccl. From the data sheet:
@@ -239,6 +236,15 @@ static void activate_ch(struct sh_mobile_i2c_data *pd)
 			pd->icic &= ~ICIC_ICCHB8;
 	}
 
+	clk_disable(pd->clk);
+}
+
+static void activate_ch(struct sh_mobile_i2c_data *pd)
+{
+	/* Wake up device and enable clock */
+	pm_runtime_get_sync(pd->dev);
+	clk_enable(pd->clk);
+
 	/* Enable channel and configure rx ack */
 	iic_set_clr(pd, ICCR, ICCR_ICE, 0);
 
@@ -632,6 +638,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	if (size > 0x17)
 		pd->flags |= IIC_FLAG_HAS_ICIC67;
 
+	sh_mobile_i2c_init(pd);
+
 	/* Enable Runtime PM for this device.
 	 *
 	 * Also tell the Runtime PM core to ignore children
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/5] i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed
  2012-10-24 10:55 [PATCH 0/5] i2c-sh_mobile non-urgent changes Shinya Kuribayashi
@ 2012-10-24 10:57 ` Shinya Kuribayashi
  2012-10-24 10:57 ` [PATCH 3/5] i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec Shinya Kuribayashi
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-10-24 10:57 UTC (permalink / raw)
  To: w.sang, ben-linux, magnus.damm; +Cc: linux-i2c, linux-arm-kernel, linux-sh

ICCH/ICCL values is supposed to be calculated/optimized to strictly meet
the timing specs required by the I2C standard.  The resulting I2C bus
speed does not matter at all, if it's less than 100 or 400 kHz.

Also fix a typo in the comment, print icch/iccl values at probe, etc.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 121 ++++++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 49 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 309d0d5..1ad80c9 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -122,9 +122,9 @@ struct sh_mobile_i2c_data {
 	unsigned long bus_speed;
 	struct clk *clk;
 	u_int8_t icic;
-	u_int8_t iccl;
-	u_int8_t icch;
 	u_int8_t flags;
+	u_int16_t iccl;
+	u_int16_t icch;
 
 	spinlock_t lock;
 	wait_queue_head_t wait;
@@ -135,7 +135,8 @@ struct sh_mobile_i2c_data {
 
 #define IIC_FLAG_HAS_ICIC67	(1 << 0)
 
-#define NORMAL_SPEED		100000 /* FAST_SPEED 400000 */
+#define STANDARD_MODE		100000
+#define FAST_MODE		400000
 
 /* Register offsets */
 #define ICDR			0x00
@@ -187,55 +188,76 @@ static void iic_set_clr(struct sh_mobile_i2c_data *pd, int offs,
 	iic_wr(pd, offs, (iic_rd(pd, offs) | set) & ~clr);
 }
 
+static u32 sh_mobile_i2c_iccl(unsigned long count_khz, u32 tLOW, u32 tf, int offset)
+{
+	/*
+	 * Conditional expression:
+	 *   ICCL >= COUNT_CLK * (tLOW + tf)
+	 *
+	 * SH-Mobile IIC hardware starts counting the LOW period of
+	 * the SCL signal (tLOW) as soon as it pulls the SCL line.
+	 * In order to meet the tLOW timing spec, we need to take into
+	 * account the fall time of SCL signal (tf).  Default tf value
+	 * should be 0.3 us, for safety.
+	 */
+	return (((count_khz * (tLOW + tf)) + 5000) / 10000) + offset;
+}
+
+static u32 sh_mobile_i2c_icch(unsigned long count_khz, u32 tHIGH, int offset)
+{
+	/*
+	 * Conditional expression:
+	 *   ICCH >= COUNT_CLK * tHIGH
+	 *
+	 * SH-Mobile IIC hardware is aware of SCL transition period 'tr',
+	 * and can ignore it.  SH-Mobile IIC controller starts counting
+	 * the HIGH period of the SCL signal (tHIGH) after the SCL input
+	 * voltage increases at VIH.
+	 */
+	return ((count_khz * tHIGH) + 5000) / 10000 + offset;
+}
+
 static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 {
-	unsigned long i2c_clk;
-	u_int32_t num;
-	u_int32_t denom;
-	u_int32_t tmp;
+	unsigned long i2c_clk_khz;
+	u32 tHIGH, tLOW, tf;
+	int offset;
 
 	/* Get clock rate after clock is enabled */
 	clk_enable(pd->clk);
-	i2c_clk = clk_get_rate(pd->clk);
-
-	/* Calculate the value for iccl. From the data sheet:
-	 * iccl = (p clock / transfer rate) * (L / (L + H))
-	 * where L and H are the SCL low/high ratio (5/4 in this case).
-	 * We also round off the result.
-	 */
-	num = i2c_clk * 5;
-	denom = pd->bus_speed * 9;
-	tmp = num * 10 / denom;
-	if (tmp % 10 >= 5)
-		pd->iccl = (u_int8_t)((num/denom) + 1);
-	else
-		pd->iccl = (u_int8_t)(num/denom);
-
-	/* one more bit of ICCL in ICIC */
-	if (pd->flags & IIC_FLAG_HAS_ICIC67) {
-		if ((num/denom) > 0xff)
-			pd->icic |= ICIC_ICCLB8;
-		else
-			pd->icic &= ~ICIC_ICCLB8;
+	i2c_clk_khz = clk_get_rate(pd->clk) / 1000;
+
+	if (pd->bus_speed == STANDARD_MODE) {
+		tLOW	= 47;	/* tLOW = 4.7 us */
+		tHIGH	= 40;	/* tHD;STA = tHIGH = 4.0 us */
+		tf	= 3;	/* tf = 0.3 us */
+		offset	= 0;	/* No offset */
+	} else if (pd->bus_speed == FAST_MODE) {
+		tLOW	= 13;	/* tLOW = 1.3 us */
+		tHIGH	= 6;	/* tHD;STA = tHIGH = 0.6 us */
+		tf	= 3;	/* tf = 0.3 us */
+		offset	= 0;	/* No offset */
+	} else {
+		dev_err(pd->dev, "unrecognized bus speed %lu Hz\n",
+			pd->bus_speed);
+		goto out;
 	}
 
-	/* Calculate the value for icch. From the data sheet:
-	   icch = (p clock / transfer rate) * (H / (L + H)) */
-	num = i2c_clk * 4;
-	tmp = num * 10 / denom;
-	if (tmp % 10 >= 5)
-		pd->icch = (u_int8_t)((num/denom) + 1);
+	pd->iccl = sh_mobile_i2c_iccl(i2c_clk_khz, tLOW, tf, offset);
+	/* one more bit of ICCL in ICIC */
+	if ((pd->iccl > 0xff) && (pd->flags & IIC_FLAG_HAS_ICIC67))
+		pd->icic |= ICIC_ICCLB8;
 	else
-		pd->icch = (u_int8_t)(num/denom);
+		pd->icic &= ~ICIC_ICCLB8;
 
+	pd->icch = sh_mobile_i2c_icch(i2c_clk_khz, tHIGH, offset);
 	/* one more bit of ICCH in ICIC */
-	if (pd->flags & IIC_FLAG_HAS_ICIC67) {
-		if ((num/denom) > 0xff)
-			pd->icic |= ICIC_ICCHB8;
-		else
-			pd->icic &= ~ICIC_ICCHB8;
-	}
+	if ((pd->icch > 0xff) && (pd->flags & IIC_FLAG_HAS_ICIC67))
+		pd->icic |= ICIC_ICCHB8;
+	else
+		pd->icic &= ~ICIC_ICCHB8;
 
+out:
 	clk_disable(pd->clk);
 }
 
@@ -252,8 +274,8 @@ static void activate_ch(struct sh_mobile_i2c_data *pd)
 	iic_wr(pd, ICIC, 0);
 
 	/* Set the clock */
-	iic_wr(pd, ICCL, pd->iccl);
-	iic_wr(pd, ICCH, pd->icch);
+	iic_wr(pd, ICCL, pd->iccl & 0xff);
+	iic_wr(pd, ICCH, pd->icch & 0xff);
 }
 
 static void deactivate_ch(struct sh_mobile_i2c_data *pd)
@@ -457,8 +479,8 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg)
 	iic_set_clr(pd, ICCR, ICCR_ICE, 0);
 
 	/* Set the clock */
-	iic_wr(pd, ICCL, pd->iccl);
-	iic_wr(pd, ICCH, pd->icch);
+	iic_wr(pd, ICCL, pd->iccl & 0xff);
+	iic_wr(pd, ICCH, pd->icch & 0xff);
 
 	pd->msg = usr_msg;
 	pd->pos = -1;
@@ -627,8 +649,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 		goto err_irq;
 	}
 
-	/* Use platformd data bus speed or NORMAL_SPEED */
-	pd->bus_speed = NORMAL_SPEED;
+	/* Use platform data bus speed or STANDARD_MODE */
+	pd->bus_speed = STANDARD_MODE;
 	if (pdata && pdata->bus_speed)
 		pd->bus_speed = pdata->bus_speed;
 
@@ -675,8 +697,9 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 		goto err_all;
 	}
 
-	dev_info(&dev->dev, "I2C adapter %d with bus speed %lu Hz\n",
-		 adap->nr, pd->bus_speed);
+	dev_info(&dev->dev,
+		 "I2C adapter %d with bus speed %lu Hz (L/H=%x/%x)\n",
+		 adap->nr, pd->bus_speed, pd->iccl, pd->icch);
 
 	of_i2c_register_devices(adap);
 	return 0;
-- 
1.7.12.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/5] i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec
  2012-10-24 10:55 [PATCH 0/5] i2c-sh_mobile non-urgent changes Shinya Kuribayashi
  2012-10-24 10:57 ` [PATCH 2/5] i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed Shinya Kuribayashi
@ 2012-10-24 10:57 ` Shinya Kuribayashi
  2012-10-24 10:58 ` [PATCH 4/5] i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock Shinya Kuribayashi
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-10-24 10:57 UTC (permalink / raw)
  To: w.sang, ben-linux, magnus.damm; +Cc: linux-i2c, linux-arm-kernel, linux-sh

The optimized ICCH/ICCL values in the previous commit afterward turned
out to violate tHD;STA timing spec.  We had to take into account the
fall time of SDA signal (tf) at START condition.  Fix it accordingly.

With this change, sh_mobile_i2c_icch() is virtually identical to
sh_mobile_i2c_iccl(), but they're providing good descriptions of
SH-/R-Mobile I2C hardware spec, and I'd leave them as separated.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 1ad80c9..4dc0cc3 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -203,18 +203,23 @@ static u32 sh_mobile_i2c_iccl(unsigned long count_khz, u32 tLOW, u32 tf, int off
 	return (((count_khz * (tLOW + tf)) + 5000) / 10000) + offset;
 }
 
-static u32 sh_mobile_i2c_icch(unsigned long count_khz, u32 tHIGH, int offset)
+static u32 sh_mobile_i2c_icch(unsigned long count_khz, u32 tHIGH, u32 tf, int offset)
 {
 	/*
 	 * Conditional expression:
-	 *   ICCH >= COUNT_CLK * tHIGH
+	 *   ICCH >= COUNT_CLK * (tHIGH + tf)
 	 *
 	 * SH-Mobile IIC hardware is aware of SCL transition period 'tr',
 	 * and can ignore it.  SH-Mobile IIC controller starts counting
 	 * the HIGH period of the SCL signal (tHIGH) after the SCL input
 	 * voltage increases at VIH.
+	 *
+	 * Afterward it turned out calculating ICCH using only tHIGH spec
+	 * will result in violation of the tHD;STA timing spec.  We need
+	 * to take into account the fall time of SDA signal (tf) at START
+	 * condition, in order to meet both tHIGH and tHD;STA specs.
 	 */
-	return ((count_khz * tHIGH) + 5000) / 10000 + offset;
+	return (((count_khz * (tHIGH + tf)) + 5000) / 10000) + offset;
 }
 
 static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
@@ -250,7 +255,7 @@ static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 	else
 		pd->icic &= ~ICIC_ICCLB8;
 
-	pd->icch = sh_mobile_i2c_icch(i2c_clk_khz, tHIGH, offset);
+	pd->icch = sh_mobile_i2c_icch(i2c_clk_khz, tHIGH, tf, offset);
 	/* one more bit of ICCH in ICIC */
 	if ((pd->icch > 0xff) && (pd->flags & IIC_FLAG_HAS_ICIC67))
 		pd->icic |= ICIC_ICCHB8;
-- 
1.7.12.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/5] i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock
  2012-10-24 10:55 [PATCH 0/5] i2c-sh_mobile non-urgent changes Shinya Kuribayashi
  2012-10-24 10:57 ` [PATCH 2/5] i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed Shinya Kuribayashi
  2012-10-24 10:57 ` [PATCH 3/5] i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec Shinya Kuribayashi
@ 2012-10-24 10:58 ` Shinya Kuribayashi
  2012-10-24 10:58 ` [PATCH 5/5] i2c: i2c-sh_mobile: fix spurious transfer request timed out Shinya Kuribayashi
       [not found] ` <5087C93F.6080601-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
  4 siblings, 0 replies; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-10-24 10:58 UTC (permalink / raw)
  To: w.sang, ben-linux, magnus.damm; +Cc: linux-i2c, linux-arm-kernel, linux-sh

On newer SH-/R-Mobile SoCs, a clock supply to the I2C hardware block,
which is used to generate the SCL clock output, is getting faster than
before, while on the other hand, the SCL clock control registers, ICCH
and ICCL, stay unchanged in 9-bit-wide (8+1).

On such silicons, the internal SCL clock counter gets incremented every
2 clocks of the operating clock.

This patch makes it configurable through platform data.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 5 +++++
 include/linux/i2c/i2c-sh_mobile.h  | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 4dc0cc3..4c28358 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -120,6 +120,7 @@ struct sh_mobile_i2c_data {
 	void __iomem *reg;
 	struct i2c_adapter adap;
 	unsigned long bus_speed;
+	unsigned int clks_per_count;
 	struct clk *clk;
 	u_int8_t icic;
 	u_int8_t flags;
@@ -231,6 +232,7 @@ static void sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 	/* Get clock rate after clock is enabled */
 	clk_enable(pd->clk);
 	i2c_clk_khz = clk_get_rate(pd->clk) / 1000;
+	i2c_clk_khz /= pd->clks_per_count;
 
 	if (pd->bus_speed == STANDARD_MODE) {
 		tLOW	= 47;	/* tLOW = 4.7 us */
@@ -658,6 +660,9 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	pd->bus_speed = STANDARD_MODE;
 	if (pdata && pdata->bus_speed)
 		pd->bus_speed = pdata->bus_speed;
+	pd->clks_per_count = 1;
+	if (pdata && pdata->clks_per_count)
+		pd->clks_per_count = pdata->clks_per_count;
 
 	/* The IIC blocks on SH-Mobile ARM processors
 	 * come with two new bits in ICIC.
diff --git a/include/linux/i2c/i2c-sh_mobile.h b/include/linux/i2c/i2c-sh_mobile.h
index beda708..06e3089 100644
--- a/include/linux/i2c/i2c-sh_mobile.h
+++ b/include/linux/i2c/i2c-sh_mobile.h
@@ -5,6 +5,7 @@
 
 struct i2c_sh_mobile_platform_data {
 	unsigned long bus_speed;
+	unsigned int clks_per_count;
 };
 
 #endif /* __I2C_SH_MOBILE_H__ */
-- 
1.7.12.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/5] i2c: i2c-sh_mobile: fix spurious transfer request timed out
  2012-10-24 10:55 [PATCH 0/5] i2c-sh_mobile non-urgent changes Shinya Kuribayashi
                   ` (2 preceding siblings ...)
  2012-10-24 10:58 ` [PATCH 4/5] i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock Shinya Kuribayashi
@ 2012-10-24 10:58 ` Shinya Kuribayashi
       [not found] ` <5087C93F.6080601-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
  4 siblings, 0 replies; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-10-24 10:58 UTC (permalink / raw)
  To: w.sang, ben-linux, magnus.damm; +Cc: linux-i2c, linux-arm-kernel, linux-sh

Ensure that any of preceding register write operations to the I2C
hardware block reached the module, and the write data is reflected
in the registers, before leaving the interrupt handler.

Otherwise, we'll suffer from spurious WAIT interrupts that lead to
'Transfer request timed out' message, and the transaction failed.

Tracked-down-by: Teppei Kamijou <teppei.kamijou.yb@renesas.com>
Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 4c28358..9411c1b 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -469,6 +469,9 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 		wake_up(&pd->wait);
 	}
 
+	/* defeat write posting to avoid spurious WAIT interrupts */
+	iic_rd(pd, ICSR);
+
 	return IRQ_HANDLED;
 }
 
-- 
1.7.12.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/5] i2c-sh_mobile non-urgent changes
       [not found] ` <5087C93F.6080601-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
  2012-10-24 10:56   ` [PATCH 1/5] i2c: i2c-sh_mobile: calculate clock parameters at driver probing time Shinya Kuribayashi
@ 2012-11-16  8:07   ` Wolfram Sang
  2012-11-16  8:53     ` Shinya Kuribayashi
  1 sibling, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2012-11-16  8:07 UTC (permalink / raw)
  To: Shinya Kuribayashi
  Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sh-u79uwXL29TY76Z2rM5mHXA

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

On Wed, Oct 24, 2012 at 07:55:59PM +0900, Shinya Kuribayashi wrote:
> Hello,
> 
> This is the first batch to fix the i2c-sh_mobile driver.  As a first
> step, this comprises of the SCL optimization work and a simple fix to
> annoying spurious WAIT interrupt issue; in other words, this does not
> change tx/rx data handling logic.
> 
> This driver has an architectural problem with send/receive procedures
> and needs a fundamental overhaul.  I've been working on splitting into
> logical chunks, but not yet completed.
> 
> Patches are prepared against the vanilla v3.6.  I don't have a chance
> to give it a try with v3.6+ kernels myself, but tend to be optimistic
> about that.  I have been cooking these patches over a year, and they
> work perfectly fine with older kernels ..v3.4 so far.

Looks good to me, like the extensive patch descriptions. I am going to
apply it to for-next with patches 2+3 squashed, because after patch 2 we
would have a buggy state otherwise. Let me know if you are not okay with
that.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/5] i2c-sh_mobile non-urgent changes
  2012-11-16  8:07   ` [PATCH 0/5] i2c-sh_mobile non-urgent changes Wolfram Sang
@ 2012-11-16  8:53     ` Shinya Kuribayashi
  2012-11-17 23:27       ` Guennadi Liakhovetski
  0 siblings, 1 reply; 9+ messages in thread
From: Shinya Kuribayashi @ 2012-11-16  8:53 UTC (permalink / raw)
  To: w.sang; +Cc: ben-linux, magnus.damm, linux-i2c, linux-arm-kernel, linux-sh

On 11/16/2012 5:07 PM, Wolfram Sang wrote:
> Looks good to me, like the extensive patch descriptions. I am going to
> apply it to for-next with patches 2+3 squashed, because after patch 2 we
> would have a buggy state otherwise. Let me know if you are not okay with
> that.

It's not that buggy as you/we think, in most cases it work without
problem.  What's more important for me is to record the issue and
how to solve it.  So I'd like to have patch 2 and 3 separately.

Thank you for your offer anyway,
--
Shinya Kuribayashi
Renesas Electronics

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/5] i2c-sh_mobile non-urgent changes
  2012-11-16  8:53     ` Shinya Kuribayashi
@ 2012-11-17 23:27       ` Guennadi Liakhovetski
  0 siblings, 0 replies; 9+ messages in thread
From: Guennadi Liakhovetski @ 2012-11-17 23:27 UTC (permalink / raw)
  To: Shinya Kuribayashi
  Cc: w.sang, linux-arm-kernel, magnus.damm, linux-i2c, ben-linux,
	linux-sh

On Fri, 16 Nov 2012, Shinya Kuribayashi wrote:

> On 11/16/2012 5:07 PM, Wolfram Sang wrote:
> > Looks good to me, like the extensive patch descriptions. I am going to
> > apply it to for-next with patches 2+3 squashed, because after patch 2 we
> > would have a buggy state otherwise. Let me know if you are not okay with
> > that.
> 
> It's not that buggy as you/we think, in most cases it work without
> problem.  What's more important for me is to record the issue and
> how to solve it.  So I'd like to have patch 2 and 3 separately.

I agree with Wolfram. For developers it can be important to know their own 
development process, that's true. But for upstream it is an absolute 
preference to avoid breakages. So, we should never commit patches, that 
are known to contain problems. In this case it means, that patches 2 and 3 
should be merged. It is good, that the problem has been detected and fixed 
during development or testing, this gives us a chance to avoid breaking 
the mainline, which we should certainly use.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-11-17 23:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-24 10:55 [PATCH 0/5] i2c-sh_mobile non-urgent changes Shinya Kuribayashi
2012-10-24 10:57 ` [PATCH 2/5] i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed Shinya Kuribayashi
2012-10-24 10:57 ` [PATCH 3/5] i2c: i2c-sh_mobile: fix ICCH to avoid violation of the tHD;STA timing spec Shinya Kuribayashi
2012-10-24 10:58 ` [PATCH 4/5] i2c: i2c-sh_mobile: support I2C hardware block with a faster operating clock Shinya Kuribayashi
2012-10-24 10:58 ` [PATCH 5/5] i2c: i2c-sh_mobile: fix spurious transfer request timed out Shinya Kuribayashi
     [not found] ` <5087C93F.6080601-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
2012-10-24 10:56   ` [PATCH 1/5] i2c: i2c-sh_mobile: calculate clock parameters at driver probing time Shinya Kuribayashi
2012-11-16  8:07   ` [PATCH 0/5] i2c-sh_mobile non-urgent changes Wolfram Sang
2012-11-16  8:53     ` Shinya Kuribayashi
2012-11-17 23:27       ` Guennadi Liakhovetski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).