[PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Sifan Naeem]

Following patches are required to fix the existing driver to
support i2c on pistachio.

Tested on Pistachio bub using an Adafruit I2C Non-Volatile FRAM Breakout
(256Kbit / 32KByte) eeprom.

Used i2c buildroot tools to test the eeprom and the other i2c blocks.
Also used dd commands to copy data to and then to dump data from the
eeprom. i2ctransfer was used to test repeated starts and verified
using a scope.

Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

Sifan Naeem (8):
  i2c: img-scb: enable fencing for all versions of the ip
  i2c: img-scb: do dummy writes before fifo access
  i2c: img-scb: use DIV_ROUND_UP to round divisor values
  i2c: img-scb: fix LOW and HIGH period values for the SCL clock
  i2c: img-scb: reset interrupts in img_i2c_soft_reset
  i2c: img-scb: remove start bit detected status after handling
  i2c: img-scb: improve transaction complete handle
  i2c: img-scb: verify support for requested bit rate

 drivers/i2c/busses/i2c-img-scb.c |  101 ++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 43 deletions(-)

-- 
1.7.9.5

[PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Sifan Naeem]

Following patches are required to fix the existing driver to
support i2c on pistachio.

Tested on Pistachio bub using an Adafruit I2C Non-Volatile FRAM Breakout
(256Kbit / 32KByte) eeprom.

Used i2c buildroot tools to test the eeprom and the other i2c blocks.
Also used dd commands to copy data to and then to dump data from the
eeprom. i2ctransfer was used to test repeated starts and verified
using a scope.

Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

Sifan Naeem (8):
  i2c: img-scb: enable fencing for all versions of the ip
  i2c: img-scb: do dummy writes before fifo access
  i2c: img-scb: use DIV_ROUND_UP to round divisor values
  i2c: img-scb: fix LOW and HIGH period values for the SCL clock
  i2c: img-scb: reset interrupts in img_i2c_soft_reset
  i2c: img-scb: remove start bit detected status after handling
  i2c: img-scb: improve transaction complete handle
  i2c: img-scb: verify support for requested bit rate

 drivers/i2c/busses/i2c-img-scb.c |  101 ++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 43 deletions(-)

-- 
1.7.9.5

[PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[Sifan Naeem]

The code to read from the master read fifo, and write to the master
write fifo, checks a bit in an SCB register before every byte to
ensure that the fifo is not full (write fifo) or empty (read fifo).
Due to clock domain crossing inside the SCB block the updated value
of this bit is only visible after 2 cycles.

The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
revision register), and it's called before reading from or writing to the
fifos to ensure that subsequent reads of the fifo status bits do not read
stale values.

As the 2 dummy writes are required in all versions of the ip, the version
check is dropped.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 00ffd66..5c3c615 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -278,8 +278,6 @@
 #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M & (err)))
 #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
 
-#define REL_SOC_IP_SCB_2_2_1	0x00020201
-
 enum img_i2c_mode {
 	MODE_INACTIVE,
 	MODE_RAW,
@@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
 		return -EINVAL;
 	}
 
-	if (rev == REL_SOC_IP_SCB_2_2_1) {
-		i2c->need_wr_rd_fence = true;
-		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
-	}
+	/* Fencing enabled by default. */
+	i2c->need_wr_rd_fence = true;
 
 	bitrate_khz = i2c->bitrate / 1000;
 	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
-- 
1.7.9.5

[PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[Sifan Naeem]

The code to read from the master read fifo, and write to the master
write fifo, checks a bit in an SCB register before every byte to
ensure that the fifo is not full (write fifo) or empty (read fifo).
Due to clock domain crossing inside the SCB block the updated value
of this bit is only visible after 2 cycles.

The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
revision register), and it's called before reading from or writing to the
fifos to ensure that subsequent reads of the fifo status bits do not read
stale values.

As the 2 dummy writes are required in all versions of the ip, the version
check is dropped.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 00ffd66..5c3c615 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -278,8 +278,6 @@
 #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M & (err)))
 #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
 
-#define REL_SOC_IP_SCB_2_2_1	0x00020201
-
 enum img_i2c_mode {
 	MODE_INACTIVE,
 	MODE_RAW,
@@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
 		return -EINVAL;
 	}
 
-	if (rev == REL_SOC_IP_SCB_2_2_1) {
-		i2c->need_wr_rd_fence = true;
-		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
-	}
+	/* Fencing enabled by default. */
+	i2c->need_wr_rd_fence = true;
 
 	bitrate_khz = i2c->bitrate / 1000;
 	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
-- 
1.7.9.5

[PATCH 2/8] i2c: img-scb: do dummy writes before fifo access

[Sifan Naeem]

Move scb_wr_rd_fence to before reading from fifo and writing to
fifo to make sure the the first read/write is done after the required
number of cycles.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Acked-by: James Hogan <james.hogan@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 5c3c615..0368d91 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -534,6 +534,7 @@ static void img_i2c_read_fifo(struct img_i2c *i2c)
 		u32 fifo_status;
 		u8 data;
 
+		img_i2c_wr_rd_fence(i2c);
 		fifo_status = img_i2c_readl(i2c, SCB_FIFO_STATUS_REG);
 		if (fifo_status & FIFO_READ_EMPTY)
 			break;
@@ -542,7 +543,6 @@ static void img_i2c_read_fifo(struct img_i2c *i2c)
 		*i2c->msg.buf = data;
 
 		img_i2c_writel(i2c, SCB_READ_FIFO_REG, 0xff);
-		img_i2c_wr_rd_fence(i2c);
 		i2c->msg.len--;
 		i2c->msg.buf++;
 	}
@@ -554,12 +554,12 @@ static void img_i2c_write_fifo(struct img_i2c *i2c)
 	while (i2c->msg.len) {
 		u32 fifo_status;
 
+		img_i2c_wr_rd_fence(i2c);
 		fifo_status = img_i2c_readl(i2c, SCB_FIFO_STATUS_REG);
 		if (fifo_status & FIFO_WRITE_FULL)
 			break;
 
 		img_i2c_writel(i2c, SCB_WRITE_DATA_REG, *i2c->msg.buf);
-		img_i2c_wr_rd_fence(i2c);
 		i2c->msg.len--;
 		i2c->msg.buf++;
 	}
-- 
1.7.9.5

[PATCH 2/8] i2c: img-scb: do dummy writes before fifo access

[Sifan Naeem]

Move scb_wr_rd_fence to before reading from fifo and writing to
fifo to make sure the the first read/write is done after the required
number of cycles.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Acked-by: James Hogan <james.hogan@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 5c3c615..0368d91 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -534,6 +534,7 @@ static void img_i2c_read_fifo(struct img_i2c *i2c)
 		u32 fifo_status;
 		u8 data;
 
+		img_i2c_wr_rd_fence(i2c);
 		fifo_status = img_i2c_readl(i2c, SCB_FIFO_STATUS_REG);
 		if (fifo_status & FIFO_READ_EMPTY)
 			break;
@@ -542,7 +543,6 @@ static void img_i2c_read_fifo(struct img_i2c *i2c)
 		*i2c->msg.buf = data;
 
 		img_i2c_writel(i2c, SCB_READ_FIFO_REG, 0xff);
-		img_i2c_wr_rd_fence(i2c);
 		i2c->msg.len--;
 		i2c->msg.buf++;
 	}
@@ -554,12 +554,12 @@ static void img_i2c_write_fifo(struct img_i2c *i2c)
 	while (i2c->msg.len) {
 		u32 fifo_status;
 
+		img_i2c_wr_rd_fence(i2c);
 		fifo_status = img_i2c_readl(i2c, SCB_FIFO_STATUS_REG);
 		if (fifo_status & FIFO_WRITE_FULL)
 			break;
 
 		img_i2c_writel(i2c, SCB_WRITE_DATA_REG, *i2c->msg.buf);
-		img_i2c_wr_rd_fence(i2c);
 		i2c->msg.len--;
 		i2c->msg.buf++;
 	}
-- 
1.7.9.5

[PATCH 3/8] i2c: img-scb: use DIV_ROUND_UP to round divisor values

[Sifan Naeem]

Using % can be slow depending on the architecture.

Using DIV_ROUND_UP is nicer and more efficient way to do it.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 0368d91..b4f59e1 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1179,9 +1179,7 @@ static int img_i2c_init(struct img_i2c *i2c)
 		int_bitrate++;
 
 	/* Setup TCKH value */
-	tckh = timing.tckh / clk_period;
-	if (timing.tckh % clk_period)
-		tckh++;
+	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
 
 	if (tckh > 0)
 		data = tckh - 1;
@@ -1201,9 +1199,7 @@ static int img_i2c_init(struct img_i2c *i2c)
 	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
 
 	/* Setup TSDH value */
-	tsdh = timing.tsdh / clk_period;
-	if (timing.tsdh % clk_period)
-		tsdh++;
+	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
 
 	if (tsdh > 1)
 		data = tsdh - 1;
-- 
1.7.9.5

[PATCH 3/8] i2c: img-scb: use DIV_ROUND_UP to round divisor values

[Sifan Naeem]

Using % can be slow depending on the architecture.

Using DIV_ROUND_UP is nicer and more efficient way to do it.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 0368d91..b4f59e1 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1179,9 +1179,7 @@ static int img_i2c_init(struct img_i2c *i2c)
 		int_bitrate++;
 
 	/* Setup TCKH value */
-	tckh = timing.tckh / clk_period;
-	if (timing.tckh % clk_period)
-		tckh++;
+	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
 
 	if (tckh > 0)
 		data = tckh - 1;
@@ -1201,9 +1199,7 @@ static int img_i2c_init(struct img_i2c *i2c)
 	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
 
 	/* Setup TSDH value */
-	tsdh = timing.tsdh / clk_period;
-	if (timing.tsdh % clk_period)
-		tsdh++;
+	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
 
 	if (tsdh > 1)
 		data = tsdh - 1;
-- 
1.7.9.5

[PATCH 4/8] i2c: img-scb: fix LOW and HIGH period values for the SCL clock

[Sifan Naeem]

After determining the minimum value for the High period (TCKH) the
remainder of the internal clock pulses is set as the Low period (TCKL).
This causes the i2c clock duty cycle to be much less than 50%.

The fix suggested here, start with TCKH and TCKL at 50% of the internal
clock pulses and adjusts the TCKH and TCKL values from there if the
minimum value for TCKL is not met. This will make sure the i2c clock
duty cycle is at 50% or close 50% whenever possible.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index b4f59e1..51a5be8 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1178,25 +1178,29 @@ static int img_i2c_init(struct img_i2c *i2c)
 	    ((bitrate_khz * clk_period) / 2))
 		int_bitrate++;
 
-	/* Setup TCKH value */
-	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
+	/*
+	 * Setup clock duty cycle, start with 50% and adjust TCKH and TCKL
+	 * values from there if they don't meet minimum timing requirements
+	 */
+	 tckh = int_bitrate / 2;
+	 tckl = int_bitrate - tckh;
 
-	if (tckh > 0)
-		data = tckh - 1;
-	else
-		data = 0;
+	/* Adjust TCKH and TCKL values */
+	data = DIV_ROUND_UP(timing.tckl, clk_period);
 
-	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, data);
+	if (tckl < data) {
+		tckl = data;
+		tckh = int_bitrate - tckl;
+	}
 
-	/* Setup TCKL value */
-	tckl = int_bitrate - tckh;
+	if (tckh > 0)
+		--tckh;
 
 	if (tckl > 0)
-		data = tckl - 1;
-	else
-		data = 0;
+		--tckl;
 
-	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
+	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, tckh);
+	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, tckl);
 
 	/* Setup TSDH value */
 	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
-- 
1.7.9.5

[PATCH 4/8] i2c: img-scb: fix LOW and HIGH period values for the SCL clock

[Sifan Naeem]

After determining the minimum value for the High period (TCKH) the
remainder of the internal clock pulses is set as the Low period (TCKL).
This causes the i2c clock duty cycle to be much less than 50%.

The fix suggested here, start with TCKH and TCKL at 50% of the internal
clock pulses and adjusts the TCKH and TCKL values from there if the
minimum value for TCKL is not met. This will make sure the i2c clock
duty cycle is at 50% or close 50% whenever possible.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index b4f59e1..51a5be8 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1178,25 +1178,29 @@ static int img_i2c_init(struct img_i2c *i2c)
 	    ((bitrate_khz * clk_period) / 2))
 		int_bitrate++;
 
-	/* Setup TCKH value */
-	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
+	/*
+	 * Setup clock duty cycle, start with 50% and adjust TCKH and TCKL
+	 * values from there if they don't meet minimum timing requirements
+	 */
+	 tckh = int_bitrate / 2;
+	 tckl = int_bitrate - tckh;
 
-	if (tckh > 0)
-		data = tckh - 1;
-	else
-		data = 0;
+	/* Adjust TCKH and TCKL values */
+	data = DIV_ROUND_UP(timing.tckl, clk_period);
 
-	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, data);
+	if (tckl < data) {
+		tckl = data;
+		tckh = int_bitrate - tckl;
+	}
 
-	/* Setup TCKL value */
-	tckl = int_bitrate - tckh;
+	if (tckh > 0)
+		--tckh;
 
 	if (tckl > 0)
-		data = tckl - 1;
-	else
-		data = 0;
+		--tckl;
 
-	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
+	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, tckh);
+	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, tckl);
 
 	/* Setup TSDH value */
 	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
-- 
1.7.9.5

[PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[Sifan Naeem]

Reset interrupt enable register and clear any generated interrupts
to make sure of a clean slate after a soft reset. Not doing so might
leave unhandle line status or generated interrupts which can cause
issues when handling new transfers.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 51a5be8..653f9bd 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
 {
 	i2c->t_halt = false;
 	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
+
+	/* Disable all interrupts */
+	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
+
+	/* Clear all interrupts */
+	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
+
+	/* Clear the scb_line_status events */
+	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
+
 	img_i2c_writel(i2c, SCB_CONTROL_REG,
 		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
+
+	/* Enable interrupts */
+	img_i2c_switch_mode(i2c, MODE_INACTIVE);
+	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
 }
 
 /* enable or release transaction halt for control of repeated starts */
@@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
 	/* Take module out of soft reset and enable clocks */
 	img_i2c_soft_reset(i2c);
 
-	/* Disable all interrupts */
-	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
-
-	/* Clear all interrupts */
-	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
-
-	/* Clear the scb_line_status events */
-	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
-
-	/* Enable interrupts */
-	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
-
 	/* Perform a synchronous sequence to reset the bus */
 	ret = img_i2c_reset_bus(i2c);
 
-- 
1.7.9.5

[PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[Sifan Naeem]

Reset interrupt enable register and clear any generated interrupts
to make sure of a clean slate after a soft reset. Not doing so might
leave unhandle line status or generated interrupts which can cause
issues when handling new transfers.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 51a5be8..653f9bd 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
 {
 	i2c->t_halt = false;
 	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
+
+	/* Disable all interrupts */
+	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
+
+	/* Clear all interrupts */
+	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
+
+	/* Clear the scb_line_status events */
+	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
+
 	img_i2c_writel(i2c, SCB_CONTROL_REG,
 		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
+
+	/* Enable interrupts */
+	img_i2c_switch_mode(i2c, MODE_INACTIVE);
+	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
 }
 
 /* enable or release transaction halt for control of repeated starts */
@@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
 	/* Take module out of soft reset and enable clocks */
 	img_i2c_soft_reset(i2c);
 
-	/* Disable all interrupts */
-	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
-
-	/* Clear all interrupts */
-	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
-
-	/* Clear the scb_line_status events */
-	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
-
-	/* Enable interrupts */
-	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
-
 	/* Perform a synchronous sequence to reset the bus */
 	ret = img_i2c_reset_bus(i2c);
 
-- 
1.7.9.5

[PATCH 6/8] i2c: img-scb: remove start bit detected status after handling

[Sifan Naeem]

Remove start bit detected status after it is handled,
doing so will prevent this condition being hit for
every interrupt on a particular transfer.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 653f9bd..341130e 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -871,10 +871,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 	}
 
 	/* Enable transaction halt on start bit */
-	if (!i2c->last_msg && i2c->line_status & LINESTAT_START_BIT_DET) {
-		img_i2c_transaction_halt(i2c, true);
-		/* we're no longer interested in the slave event */
-		i2c->int_enable &= ~INT_SLAVE_EVENT;
+	if (i2c->line_status & LINESTAT_START_BIT_DET) {
+		if (!i2c->last_msg) {
+			img_i2c_transaction_halt(i2c, true);
+			/* we're no longer interested in the slave event */
+			i2c->int_enable &= ~INT_SLAVE_EVENT;
+		}
+		/*
+		 * Remove start bit detected status after it is handled,
+		 * doing so will prevent this condition being hit for
+		 * every interrupt on a particular transfer.
+		 */
+		i2c->line_status &= ~LINESTAT_START_BIT_DET;
 	}
 
 	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
-- 
1.7.9.5

[PATCH 6/8] i2c: img-scb: remove start bit detected status after handling

[Sifan Naeem]

Remove start bit detected status after it is handled,
doing so will prevent this condition being hit for
every interrupt on a particular transfer.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 653f9bd..341130e 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -871,10 +871,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 	}
 
 	/* Enable transaction halt on start bit */
-	if (!i2c->last_msg && i2c->line_status & LINESTAT_START_BIT_DET) {
-		img_i2c_transaction_halt(i2c, true);
-		/* we're no longer interested in the slave event */
-		i2c->int_enable &= ~INT_SLAVE_EVENT;
+	if (i2c->line_status & LINESTAT_START_BIT_DET) {
+		if (!i2c->last_msg) {
+			img_i2c_transaction_halt(i2c, true);
+			/* we're no longer interested in the slave event */
+			i2c->int_enable &= ~INT_SLAVE_EVENT;
+		}
+		/*
+		 * Remove start bit detected status after it is handled,
+		 * doing so will prevent this condition being hit for
+		 * every interrupt on a particular transfer.
+		 */
+		i2c->line_status &= ~LINESTAT_START_BIT_DET;
 	}
 
 	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
-- 
1.7.9.5

[PATCH 7/8] i2c: img-scb: improve transaction complete handle

[Sifan Naeem]

Clear line status and all interrupts when transaction is complete,
as not doing so might leave unserviced interrupts that might be
handled in the context of a new transfer. Soft reset if the the
transfer failed to bring back the i2c block to a reset state.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 341130e..bbfee33 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -626,7 +626,10 @@ static void img_i2c_complete_transaction(struct img_i2c *i2c, int status)
 	img_i2c_switch_mode(i2c, MODE_INACTIVE);
 	if (status) {
 		i2c->msg_status = status;
-		img_i2c_transaction_halt(i2c, false);
+		img_i2c_soft_reset(i2c);
+	} else {
+		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
+		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
 	}
 	complete(&i2c->msg_complete);
 }
-- 
1.7.9.5

[PATCH 7/8] i2c: img-scb: improve transaction complete handle

[Sifan Naeem]

Clear line status and all interrupts when transaction is complete,
as not doing so might leave unserviced interrupts that might be
handled in the context of a new transfer. Soft reset if the the
transfer failed to bring back the i2c block to a reset state.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 341130e..bbfee33 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -626,7 +626,10 @@ static void img_i2c_complete_transaction(struct img_i2c *i2c, int status)
 	img_i2c_switch_mode(i2c, MODE_INACTIVE);
 	if (status) {
 		i2c->msg_status = status;
-		img_i2c_transaction_halt(i2c, false);
+		img_i2c_soft_reset(i2c);
+	} else {
+		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
+		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
 	}
 	complete(&i2c->msg_complete);
 }
-- 
1.7.9.5

[PATCH 8/8] i2c: img-scb: verify support for requested bit rate

[Sifan Naeem]

The requested bit rate can be outside the range supported by the driver.
The maximum bit rate this driver supports at the moment is 400Khz.

Return -EINVAL if the bit rate is larger than 400khz.

Maximum speed supported by the driver can be increased to 1Mhz by
adding support for "fast plus mode" in the future.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index bbfee33..07a039c 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1157,6 +1157,12 @@ static int img_i2c_init(struct img_i2c *i2c)
 			break;
 		}
 	}
+	if (i2c->bitrate > timing.max_bitrate) {
+		dev_err(i2c->adap.dev.parent,
+			"requested bitrate (%d) is higher than the max bitrate supported (%d)\n",
+			 i2c->bitrate, timing.max_bitrate);
+		return -EINVAL;
+	}
 
 	/* Find the prescale that would give us that inc (approx delay = 0) */
 	prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
-- 
1.7.9.5

[PATCH 8/8] i2c: img-scb: verify support for requested bit rate

[Sifan Naeem]

The requested bit rate can be outside the range supported by the driver.
The maximum bit rate this driver supports at the moment is 400Khz.

Return -EINVAL if the bit rate is larger than 400khz.

Maximum speed supported by the driver can be increased to 1Mhz by
adding support for "fast plus mode" in the future.

Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index bbfee33..07a039c 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1157,6 +1157,12 @@ static int img_i2c_init(struct img_i2c *i2c)
 			break;
 		}
 	}
+	if (i2c->bitrate > timing.max_bitrate) {
+		dev_err(i2c->adap.dev.parent,
+			"requested bitrate (%d) is higher than the max bitrate supported (%d)\n",
+			 i2c->bitrate, timing.max_bitrate);
+		return -EINVAL;
+	}
 
 	/* Find the prescale that would give us that inc (approx delay = 0) */
 	prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
-- 
1.7.9.5

Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[James Hogan]

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

Hi Sifan,

On Mon, Jul 27, 2015 at 12:47:14PM +0100, Sifan Naeem wrote:
> The code to read from the master read fifo, and write to the master
> write fifo, checks a bit in an SCB register before every byte to
> ensure that the fifo is not full (write fifo) or empty (read fifo).
> Due to clock domain crossing inside the SCB block the updated value
> of this bit is only visible after 2 cycles.
> 
> The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
> revision register), and it's called before reading from or writing to the
> fifos to ensure that subsequent reads of the fifo status bits do not read
> stale values.
> 
> As the 2 dummy writes are required in all versions of the ip, the version
> check is dropped.

Is it anticipated that a future version of the hardware will probably
resolve the clock domain crossing issue? If so fine, but if not its
probably worth removing need_wr_rd_fence.

> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")

I believe 12 digits of SHA1 is recommended now, to avoid collisions. I
suggest doing this:
$ git config --global core.abbrev 12

> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

That's a fairly non-conventional way to specify stable versions. The
recommended way to Cc stable according to
Documentation/stable_kernel_rules.txt is more like this:
Cc: <stable@vger.kernel.org> # 3.19.x-

Patch looks fine though

Acked-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

> ---
>  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 00ffd66..5c3c615 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -278,8 +278,6 @@
>  #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M & (err)))
>  #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
>  
> -#define REL_SOC_IP_SCB_2_2_1	0x00020201
> -
>  enum img_i2c_mode {
>  	MODE_INACTIVE,
>  	MODE_RAW,
> @@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
>  		return -EINVAL;
>  	}
>  
> -	if (rev == REL_SOC_IP_SCB_2_2_1) {
> -		i2c->need_wr_rd_fence = true;
> -		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
> -	}
> +	/* Fencing enabled by default. */
> +	i2c->need_wr_rd_fence = true;
>  
>  	bitrate_khz = i2c->bitrate / 1000;
>  	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> -- 
> 1.7.9.5
> 

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

Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[James Hogan]

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

Hi Sifan,

On Mon, Jul 27, 2015 at 12:47:14PM +0100, Sifan Naeem wrote:
> The code to read from the master read fifo, and write to the master
> write fifo, checks a bit in an SCB register before every byte to
> ensure that the fifo is not full (write fifo) or empty (read fifo).
> Due to clock domain crossing inside the SCB block the updated value
> of this bit is only visible after 2 cycles.
> 
> The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
> revision register), and it's called before reading from or writing to the
> fifos to ensure that subsequent reads of the fifo status bits do not read
> stale values.
> 
> As the 2 dummy writes are required in all versions of the ip, the version
> check is dropped.

Is it anticipated that a future version of the hardware will probably
resolve the clock domain crossing issue? If so fine, but if not its
probably worth removing need_wr_rd_fence.

> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")

I believe 12 digits of SHA1 is recommended now, to avoid collisions. I
suggest doing this:
$ git config --global core.abbrev 12

> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

That's a fairly non-conventional way to specify stable versions. The
recommended way to Cc stable according to
Documentation/stable_kernel_rules.txt is more like this:
Cc: <stable@vger.kernel.org> # 3.19.x-

Patch looks fine though

Acked-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

> ---
>  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 00ffd66..5c3c615 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -278,8 +278,6 @@
>  #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M & (err)))
>  #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
>  
> -#define REL_SOC_IP_SCB_2_2_1	0x00020201
> -
>  enum img_i2c_mode {
>  	MODE_INACTIVE,
>  	MODE_RAW,
> @@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
>  		return -EINVAL;
>  	}
>  
> -	if (rev == REL_SOC_IP_SCB_2_2_1) {
> -		i2c->need_wr_rd_fence = true;
> -		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
> -	}
> +	/* Fencing enabled by default. */
> +	i2c->need_wr_rd_fence = true;
>  
>  	bitrate_khz = i2c->bitrate / 1000;
>  	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> -- 
> 1.7.9.5
> 

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

RE: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 27 July 2015 21:21
> To: Sifan Naeem
> Cc: Wolfram Sang; linux-i2c@vger.kernel.org; Stable kernel (v3.19+)
> Subject: Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip
> 
> Hi Sifan,
> 
> On Mon, Jul 27, 2015 at 12:47:14PM +0100, Sifan Naeem wrote:
> > The code to read from the master read fifo, and write to the master
> > write fifo, checks a bit in an SCB register before every byte to
> > ensure that the fifo is not full (write fifo) or empty (read fifo).
> > Due to clock domain crossing inside the SCB block the updated value of
> > this bit is only visible after 2 cycles.
> >
> > The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
> > revision register), and it's called before reading from or writing to
> > the fifos to ensure that subsequent reads of the fifo status bits do
> > not read stale values.
> >
> > As the 2 dummy writes are required in all versions of the ip, the
> > version check is dropped.
> 
> Is it anticipated that a future version of the hardware will probably resolve
> the clock domain crossing issue? If so fine, but if not its probably worth
> removing need_wr_rd_fence.
> 
Yes, it's expected to be fixed in the future, albeit not in the near future.

> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> 
> I believe 12 digits of SHA1 is recommended now, to avoid collisions. I suggest
> doing this:
> $ git config --global core.abbrev 12
> 
Should I send a new patch with 12 digit SHA1?

> > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> > Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> 
> That's a fairly non-conventional way to specify stable versions. The
> recommended way to Cc stable according to
> Documentation/stable_kernel_rules.txt is more like this:
> Cc: <stable@vger.kernel.org> # 3.19.x-
> 

I go this error when doing that way:

(body) Adding cc: Sifan Naeem <sifan.naeem@imgtec.com> from line 'Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>'
(body) Adding cc: <stable@vger.kernel.org> # 4.1 from line 'Cc: <stable@vger.kernel.org> # 4.1'
Use of uninitialized value $cc in string eq at /usr/lib/git-core/git-send-email line 983.
Use of uninitialized value $cc in quotemeta at /usr/lib/git-core/git-send-email line 983.
W: unable to extract a valid address from: <stable@vger.kernel.org> # 4.1
W: unable to extract a valid address from: <stable@vger.kernel.org> # 4.1

> Patch looks fine though
> 
> Acked-by: James Hogan <james.hogan@imgtec.com>
> 
Thanks,
Sifan

> Thanks
> James
> 
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 00ffd66..5c3c615 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -278,8 +278,6 @@
> >  #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M &
> (err)))
> >  #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
> >
> > -#define REL_SOC_IP_SCB_2_2_1	0x00020201
> > -
> >  enum img_i2c_mode {
> >  	MODE_INACTIVE,
> >  	MODE_RAW,
> > @@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> >  		return -EINVAL;
> >  	}
> >
> > -	if (rev == REL_SOC_IP_SCB_2_2_1) {
> > -		i2c->need_wr_rd_fence = true;
> > -		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
> > -	}
> > +	/* Fencing enabled by default. */
> > +	i2c->need_wr_rd_fence = true;
> >
> >  	bitrate_khz = i2c->bitrate / 1000;
> >  	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> > --
> > 1.7.9.5
> >

Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip

[James Hogan]

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

On 28/07/15 10:26, Sifan Naeem wrote:
> Hi James,
> 
>> -----Original Message-----
>> From: James Hogan
>> Sent: 27 July 2015 21:21
>> To: Sifan Naeem
>> Cc: Wolfram Sang; linux-i2c@vger.kernel.org; Stable kernel (v3.19+)
>> Subject: Re: [PATCH 1/8] i2c: img-scb: enable fencing for all versions of the ip
>>
>> Hi Sifan,
>>
>> On Mon, Jul 27, 2015 at 12:47:14PM +0100, Sifan Naeem wrote:
>>> The code to read from the master read fifo, and write to the master
>>> write fifo, checks a bit in an SCB register before every byte to
>>> ensure that the fifo is not full (write fifo) or empty (read fifo).
>>> Due to clock domain crossing inside the SCB block the updated value of
>>> this bit is only visible after 2 cycles.
>>>
>>> The scb_wr_rd_fence() function does 2 dummy writes (to the read-only
>>> revision register), and it's called before reading from or writing to
>>> the fifos to ensure that subsequent reads of the fifo status bits do
>>> not read stale values.
>>>
>>> As the 2 dummy writes are required in all versions of the ip, the
>>> version check is dropped.
>>
>> Is it anticipated that a future version of the hardware will probably resolve
>> the clock domain crossing issue? If so fine, but if not its probably worth
>> removing need_wr_rd_fence.
>>
> Yes, it's expected to be fixed in the future, albeit not in the near future.

Okay, no problem then.

> 
>>>
>>> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
>>> driver")
>>
>> I believe 12 digits of SHA1 is recommended now, to avoid collisions. I suggest
>> doing this:
>> $ git config --global core.abbrev 12
>>
> Should I send a new patch with 12 digit SHA1?

I need to review the other patches anyway (sorry i've been slow to look
through them properly).

> 
>>> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
>>> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
>>
>> That's a fairly non-conventional way to specify stable versions. The
>> recommended way to Cc stable according to
>> Documentation/stable_kernel_rules.txt is more like this:
>> Cc: <stable@vger.kernel.org> # 3.19.x-
>>
> 
> I go this error when doing that way:
> 
> (body) Adding cc: Sifan Naeem <sifan.naeem@imgtec.com> from line 'Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>'
> (body) Adding cc: <stable@vger.kernel.org> # 4.1 from line 'Cc: <stable@vger.kernel.org> # 4.1'
> Use of uninitialized value $cc in string eq at /usr/lib/git-core/git-send-email line 983.
> Use of uninitialized value $cc in quotemeta at /usr/lib/git-core/git-send-email line 983.
> W: unable to extract a valid address from: <stable@vger.kernel.org> # 4.1
> W: unable to extract a valid address from: <stable@vger.kernel.org> # 4.1

Sounds like you're using an old version of git. Something similar is
described here:
http://comments.gmane.org/gmane.comp.version-control.git/210042

If the warning can be ignored, you can always add stable back to cc list
with --cc=stable@vger.kernel.org.

Cheers
James

> 
>> Patch looks fine though
>>
>> Acked-by: James Hogan <james.hogan@imgtec.com>
>>
> Thanks,
> Sifan
> 
>> Thanks
>> James
>>
>>> ---
>>>  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-img-scb.c
>>> b/drivers/i2c/busses/i2c-img-scb.c
>>> index 00ffd66..5c3c615 100644
>>> --- a/drivers/i2c/busses/i2c-img-scb.c
>>> +++ b/drivers/i2c/busses/i2c-img-scb.c
>>> @@ -278,8 +278,6 @@
>>>  #define ISR_COMPLETE(err)	(ISR_COMPLETE_M | (ISR_STATUS_M &
>> (err)))
>>>  #define ISR_FATAL(err)		(ISR_COMPLETE(err) | ISR_FATAL_M)
>>>
>>> -#define REL_SOC_IP_SCB_2_2_1	0x00020201
>>> -
>>>  enum img_i2c_mode {
>>>  	MODE_INACTIVE,
>>>  	MODE_RAW,
>>> @@ -1120,10 +1118,8 @@ static int img_i2c_init(struct img_i2c *i2c)
>>>  		return -EINVAL;
>>>  	}
>>>
>>> -	if (rev == REL_SOC_IP_SCB_2_2_1) {
>>> -		i2c->need_wr_rd_fence = true;
>>> -		dev_info(i2c->adap.dev.parent, "fence quirk enabled");
>>> -	}
>>> +	/* Fencing enabled by default. */
>>> +	i2c->need_wr_rd_fence = true;
>>>
>>>  	bitrate_khz = i2c->bitrate / 1000;
>>>  	clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
>>> --
>>> 1.7.9.5
>>>


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

Re: [PATCH 3/8] i2c: img-scb: use DIV_ROUND_UP to round divisor values

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> Using % can be slow depending on the architecture.
> 
> Using DIV_ROUND_UP is nicer and more efficient way to do it.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

Acked-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Cheers
James

> ---
>  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 0368d91..b4f59e1 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1179,9 +1179,7 @@ static int img_i2c_init(struct img_i2c *i2c)
>  		int_bitrate++;
>  
>  	/* Setup TCKH value */
> -	tckh = timing.tckh / clk_period;
> -	if (timing.tckh % clk_period)
> -		tckh++;
> +	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
>  
>  	if (tckh > 0)
>  		data = tckh - 1;
> @@ -1201,9 +1199,7 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
>  
>  	/* Setup TSDH value */
> -	tsdh = timing.tsdh / clk_period;
> -	if (timing.tsdh % clk_period)
> -		tsdh++;
> +	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
>  
>  	if (tsdh > 1)
>  		data = tsdh - 1;
> 


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

Re: [PATCH 3/8] i2c: img-scb: use DIV_ROUND_UP to round divisor values

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> Using % can be slow depending on the architecture.
> 
> Using DIV_ROUND_UP is nicer and more efficient way to do it.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

Acked-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> ---
>  drivers/i2c/busses/i2c-img-scb.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 0368d91..b4f59e1 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1179,9 +1179,7 @@ static int img_i2c_init(struct img_i2c *i2c)
>  		int_bitrate++;
>  
>  	/* Setup TCKH value */
> -	tckh = timing.tckh / clk_period;
> -	if (timing.tckh % clk_period)
> -		tckh++;
> +	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
>  
>  	if (tckh > 0)
>  		data = tckh - 1;
> @@ -1201,9 +1199,7 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
>  
>  	/* Setup TSDH value */
> -	tsdh = timing.tsdh / clk_period;
> -	if (timing.tsdh % clk_period)
> -		tsdh++;
> +	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
>  
>  	if (tsdh > 1)
>  		data = tsdh - 1;
> 


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

Re: [PATCH 4/8] i2c: img-scb: fix LOW and HIGH period values for the SCL clock

[James Hogan]

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

Hi Sifan,

On 27/07/15 12:47, Sifan Naeem wrote:
> After determining the minimum value for the High period (TCKH) the
> remainder of the internal clock pulses is set as the Low period (TCKL).
> This causes the i2c clock duty cycle to be much less than 50%.
> 
> The fix suggested here, start with TCKH and TCKL at 50% of the internal
> clock pulses and adjusts the TCKH and TCKL values from there if the
> minimum value for TCKL is not met. This will make sure the i2c clock
> duty cycle is at 50% or close 50% whenever possible.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index b4f59e1..51a5be8 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1178,25 +1178,29 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	    ((bitrate_khz * clk_period) / 2))
>  		int_bitrate++;
>  
> -	/* Setup TCKH value */
> -	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
> +	/*
> +	 * Setup clock duty cycle, start with 50% and adjust TCKH and TCKL
> +	 * values from there if they don't meet minimum timing requirements
> +	 */
> +	 tckh = int_bitrate / 2;
> +	 tckl = int_bitrate - tckh;

too much indentation here.

Otherwise looks good to me.

Acked-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

>  
> -	if (tckh > 0)
> -		data = tckh - 1;
> -	else
> -		data = 0;
> +	/* Adjust TCKH and TCKL values */
> +	data = DIV_ROUND_UP(timing.tckl, clk_period);
>  
> -	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, data);
> +	if (tckl < data) {
> +		tckl = data;
> +		tckh = int_bitrate - tckl;
> +	}
>  
> -	/* Setup TCKL value */
> -	tckl = int_bitrate - tckh;
> +	if (tckh > 0)
> +		--tckh;
>  
>  	if (tckl > 0)
> -		data = tckl - 1;
> -	else
> -		data = 0;
> +		--tckl;
>  
> -	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
> +	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, tckh);
> +	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, tckl);
>  
>  	/* Setup TSDH value */
>  	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
> 


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

Re: [PATCH 4/8] i2c: img-scb: fix LOW and HIGH period values for the SCL clock

[James Hogan]

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

Hi Sifan,

On 27/07/15 12:47, Sifan Naeem wrote:
> After determining the minimum value for the High period (TCKH) the
> remainder of the internal clock pulses is set as the Low period (TCKL).
> This causes the i2c clock duty cycle to be much less than 50%.
> 
> The fix suggested here, start with TCKH and TCKL at 50% of the internal
> clock pulses and adjusts the TCKH and TCKL values from there if the
> minimum value for TCKL is not met. This will make sure the i2c clock
> duty cycle is at 50% or close 50% whenever possible.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index b4f59e1..51a5be8 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1178,25 +1178,29 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	    ((bitrate_khz * clk_period) / 2))
>  		int_bitrate++;
>  
> -	/* Setup TCKH value */
> -	tckh = DIV_ROUND_UP(timing.tckh, clk_period);
> +	/*
> +	 * Setup clock duty cycle, start with 50% and adjust TCKH and TCKL
> +	 * values from there if they don't meet minimum timing requirements
> +	 */
> +	 tckh = int_bitrate / 2;
> +	 tckl = int_bitrate - tckh;

too much indentation here.

Otherwise looks good to me.

Acked-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

>  
> -	if (tckh > 0)
> -		data = tckh - 1;
> -	else
> -		data = 0;
> +	/* Adjust TCKH and TCKL values */
> +	data = DIV_ROUND_UP(timing.tckl, clk_period);
>  
> -	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, data);
> +	if (tckl < data) {
> +		tckl = data;
> +		tckh = int_bitrate - tckl;
> +	}
>  
> -	/* Setup TCKL value */
> -	tckl = int_bitrate - tckh;
> +	if (tckh > 0)
> +		--tckh;
>  
>  	if (tckl > 0)
> -		data = tckl - 1;
> -	else
> -		data = 0;
> +		--tckl;
>  
> -	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, data);
> +	img_i2c_writel(i2c, SCB_TIME_TCKH_REG, tckh);
> +	img_i2c_writel(i2c, SCB_TIME_TCKL_REG, tckl);
>  
>  	/* Setup TSDH value */
>  	tsdh = DIV_ROUND_UP(timing.tsdh, clk_period);
> 


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

Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> Reset interrupt enable register and clear any generated interrupts
> to make sure of a clean slate after a soft reset. Not doing so might
> leave unhandle line status or generated interrupts which can cause
> issues when handling new transfers.

That already happens after the call to img_i2c_soft_reset, you've just
moved it to while the clock is off and the block is in soft reset. So
are you saying its important for interrupts and the various event bits
to be clear before taking it out of reset and enabling the clock? TBH
I'm confused how they could do any harm.

Cheers
James

> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 51a5be8..653f9bd 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
>  {
>  	i2c->t_halt = false;
>  	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
> +
> +	/* Disable all interrupts */
> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
> +
> +	/* Clear all interrupts */
> +	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> +
> +	/* Clear the scb_line_status events */
> +	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> +
>  	img_i2c_writel(i2c, SCB_CONTROL_REG,
>  		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
> +
> +	/* Enable interrupts */
> +	img_i2c_switch_mode(i2c, MODE_INACTIVE);
> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>  }
>  
>  /* enable or release transaction halt for control of repeated starts */
> @@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	/* Take module out of soft reset and enable clocks */
>  	img_i2c_soft_reset(i2c);
>  
> -	/* Disable all interrupts */
> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
> -
> -	/* Clear all interrupts */
> -	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> -
> -	/* Clear the scb_line_status events */
> -	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> -
> -	/* Enable interrupts */
> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
> -
>  	/* Perform a synchronous sequence to reset the bus */
>  	ret = img_i2c_reset_bus(i2c);
>  
> 


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

Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> Reset interrupt enable register and clear any generated interrupts
> to make sure of a clean slate after a soft reset. Not doing so might
> leave unhandle line status or generated interrupts which can cause
> issues when handling new transfers.

That already happens after the call to img_i2c_soft_reset, you've just
moved it to while the clock is off and the block is in soft reset. So
are you saying its important for interrupts and the various event bits
to be clear before taking it out of reset and enabling the clock? TBH
I'm confused how they could do any harm.

Cheers
James

> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 51a5be8..653f9bd 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
>  {
>  	i2c->t_halt = false;
>  	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
> +
> +	/* Disable all interrupts */
> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
> +
> +	/* Clear all interrupts */
> +	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> +
> +	/* Clear the scb_line_status events */
> +	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> +
>  	img_i2c_writel(i2c, SCB_CONTROL_REG,
>  		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
> +
> +	/* Enable interrupts */
> +	img_i2c_switch_mode(i2c, MODE_INACTIVE);
> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>  }
>  
>  /* enable or release transaction halt for control of repeated starts */
> @@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	/* Take module out of soft reset and enable clocks */
>  	img_i2c_soft_reset(i2c);
>  
> -	/* Disable all interrupts */
> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
> -
> -	/* Clear all interrupts */
> -	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> -
> -	/* Clear the scb_line_status events */
> -	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> -
> -	/* Enable interrupts */
> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
> -
>  	/* Perform a synchronous sequence to reset the bus */
>  	ret = img_i2c_reset_bus(i2c);
>  
> 


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

RE: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 28 July 2015 12:36
> To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org
> Cc: Stable kernel (v3.19+)
> Subject: Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset
> 
> On 27/07/15 12:47, Sifan Naeem wrote:
> > Reset interrupt enable register and clear any generated interrupts to
> > make sure of a clean slate after a soft reset. Not doing so might
> > leave unhandle line status or generated interrupts which can cause
> > issues when handling new transfers.
> 
> That already happens after the call to img_i2c_soft_reset, you've just moved
> it to while the clock is off and the block is in soft reset. So are you saying its
> important for interrupts and the various event bits to be clear before taking it
> out of reset and enabling the clock? TBH I'm confused how they could do any
> harm.
> 
In " [PATCH 7/8] i2c: img-scb: improve transaction complete handle " img_i2c_soft_reset is added to img_i2c_complete_transaction, this will be useful in this scenario, not necessarily for when initialising the block.

Thanks
Sifan
> Cheers
> James
> 
> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> > Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 51a5be8..653f9bd 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c
> > *i2c)  {
> >  	i2c->t_halt = false;
> >  	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
> > +
> > +	/* Disable all interrupts */
> > +	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
> > +
> > +	/* Clear all interrupts */
> > +	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> > +
> > +	/* Clear the scb_line_status events */
> > +	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> > +
> >  	img_i2c_writel(i2c, SCB_CONTROL_REG,
> >  		       SCB_CONTROL_CLK_ENABLE |
> SCB_CONTROL_SOFT_RESET);
> > +
> > +	/* Enable interrupts */
> > +	img_i2c_switch_mode(i2c, MODE_INACTIVE);
> > +	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
> >  }
> >
> >  /* enable or release transaction halt for control of repeated starts
> > */ @@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
> >  	/* Take module out of soft reset and enable clocks */
> >  	img_i2c_soft_reset(i2c);
> >
> > -	/* Disable all interrupts */
> > -	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
> > -
> > -	/* Clear all interrupts */
> > -	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> > -
> > -	/* Clear the scb_line_status events */
> > -	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> > -
> > -	/* Enable interrupts */
> > -	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
> > -
> >  	/* Perform a synchronous sequence to reset the bus */
> >  	ret = img_i2c_reset_bus(i2c);
> >
> >

Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[James Hogan]

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

On 28/07/15 12:46, Sifan Naeem wrote:
> Hi James,
> 
>> -----Original Message-----
>> From: James Hogan
>> Sent: 28 July 2015 12:36
>> To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org
>> Cc: Stable kernel (v3.19+)
>> Subject: Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset
>>
>> On 27/07/15 12:47, Sifan Naeem wrote:
>>> Reset interrupt enable register and clear any generated interrupts to
>>> make sure of a clean slate after a soft reset. Not doing so might
>>> leave unhandle line status or generated interrupts which can cause
>>> issues when handling new transfers.
>>
>> That already happens after the call to img_i2c_soft_reset, you've just moved
>> it to while the clock is off and the block is in soft reset. So are you saying its
>> important for interrupts and the various event bits to be clear before taking it
>> out of reset and enabling the clock? TBH I'm confused how they could do any
>> harm.
>>
> In " [PATCH 7/8] i2c: img-scb: improve transaction complete handle "
> img_i2c_soft_reset is added to img_i2c_complete_transaction, this will
> be useful in this scenario, not necessarily for when initialising the
> block.

Okay, in that case you should clarify in the commit message that it is
in preparation for later patches, rather than fixing an existing problem.

Cheers
James

> 
> Thanks
> Sifan
>> Cheers
>> James
>>
>>>
>>> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
>>> driver")
>>> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
>>> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
>>> ---
>>>  drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
>>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-img-scb.c
>>> b/drivers/i2c/busses/i2c-img-scb.c
>>> index 51a5be8..653f9bd 100644
>>> --- a/drivers/i2c/busses/i2c-img-scb.c
>>> +++ b/drivers/i2c/busses/i2c-img-scb.c
>>> @@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c
>>> *i2c)  {
>>>  	i2c->t_halt = false;
>>>  	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
>>> +
>>> +	/* Disable all interrupts */
>>> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
>>> +
>>> +	/* Clear all interrupts */
>>> +	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
>>> +
>>> +	/* Clear the scb_line_status events */
>>> +	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>>> +
>>>  	img_i2c_writel(i2c, SCB_CONTROL_REG,
>>>  		       SCB_CONTROL_CLK_ENABLE |
>> SCB_CONTROL_SOFT_RESET);
>>> +
>>> +	/* Enable interrupts */
>>> +	img_i2c_switch_mode(i2c, MODE_INACTIVE);
>>> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>>>  }
>>>
>>>  /* enable or release transaction halt for control of repeated starts
>>> */ @@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
>>>  	/* Take module out of soft reset and enable clocks */
>>>  	img_i2c_soft_reset(i2c);
>>>
>>> -	/* Disable all interrupts */
>>> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
>>> -
>>> -	/* Clear all interrupts */
>>> -	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
>>> -
>>> -	/* Clear the scb_line_status events */
>>> -	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>>> -
>>> -	/* Enable interrupts */
>>> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>>> -
>>>  	/* Perform a synchronous sequence to reset the bus */
>>>  	ret = img_i2c_reset_bus(i2c);
>>>
>>>
> 


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

Re: [PATCH 6/8] i2c: img-scb: remove start bit detected status after handling

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> Remove start bit detected status after it is handled,
> doing so will prevent this condition being hit for
> every interrupt on a particular transfer.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 653f9bd..341130e 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -871,10 +871,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	}
>  
>  	/* Enable transaction halt on start bit */
> -	if (!i2c->last_msg && i2c->line_status & LINESTAT_START_BIT_DET) {

i2c->line_status accumulates the line status bits that have been seen
with each interrupt. If we're only interested in that bit from the
current interrupt, should it just be referring to line_status (the
argument to img_i2c_auto) instead of i2c->line_status?

Cheers
James

> -		img_i2c_transaction_halt(i2c, true);
> -		/* we're no longer interested in the slave event */
> -		i2c->int_enable &= ~INT_SLAVE_EVENT;
> +	if (i2c->line_status & LINESTAT_START_BIT_DET) {
> +		if (!i2c->last_msg) {
> +			img_i2c_transaction_halt(i2c, true);
> +			/* we're no longer interested in the slave event */
> +			i2c->int_enable &= ~INT_SLAVE_EVENT;
> +		}
> +		/*
> +		 * Remove start bit detected status after it is handled,
> +		 * doing so will prevent this condition being hit for
> +		 * every interrupt on a particular transfer.
> +		 */
> +		i2c->line_status &= ~LINESTAT_START_BIT_DET;
>  	}
>  
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> 


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

Re: [PATCH 6/8] i2c: img-scb: remove start bit detected status after handling

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> Remove start bit detected status after it is handled,
> doing so will prevent this condition being hit for
> every interrupt on a particular transfer.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 653f9bd..341130e 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -871,10 +871,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	}
>  
>  	/* Enable transaction halt on start bit */
> -	if (!i2c->last_msg && i2c->line_status & LINESTAT_START_BIT_DET) {

i2c->line_status accumulates the line status bits that have been seen
with each interrupt. If we're only interested in that bit from the
current interrupt, should it just be referring to line_status (the
argument to img_i2c_auto) instead of i2c->line_status?

Cheers
James

> -		img_i2c_transaction_halt(i2c, true);
> -		/* we're no longer interested in the slave event */
> -		i2c->int_enable &= ~INT_SLAVE_EVENT;
> +	if (i2c->line_status & LINESTAT_START_BIT_DET) {
> +		if (!i2c->last_msg) {
> +			img_i2c_transaction_halt(i2c, true);
> +			/* we're no longer interested in the slave event */
> +			i2c->int_enable &= ~INT_SLAVE_EVENT;
> +		}
> +		/*
> +		 * Remove start bit detected status after it is handled,
> +		 * doing so will prevent this condition being hit for
> +		 * every interrupt on a particular transfer.
> +		 */
> +		i2c->line_status &= ~LINESTAT_START_BIT_DET;
>  	}
>  
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> 


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

Re: [PATCH 8/8] i2c: img-scb: verify support for requested bit rate

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> The requested bit rate can be outside the range supported by the driver.
> The maximum bit rate this driver supports at the moment is 400Khz.
> 
> Return -EINVAL if the bit rate is larger than 400khz.
> 
> Maximum speed supported by the driver can be increased to 1Mhz by
> adding support for "fast plus mode" in the future.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index bbfee33..07a039c 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1157,6 +1157,12 @@ static int img_i2c_init(struct img_i2c *i2c)
>  			break;
>  		}
>  	}
> +	if (i2c->bitrate > timing.max_bitrate) {
> +		dev_err(i2c->adap.dev.parent,
> +			"requested bitrate (%d) is higher than the max bitrate supported (%d)\n",
> +			 i2c->bitrate, timing.max_bitrate);
> +		return -EINVAL;
> +	}

The timing is only chosen if i2c->bitrate <= timing.max_bitrate, so
you'd only hit this case if none of the timings were valid, in which
case timing == timings[0], so when you print timing.max_bitrate it won't
be the max bitrate supported, it'll be the max bitrate of the first
timing in the array.

Anyway, I think the original intent of the DT provided clock-frequency
was more as a maximum bitrate. This was used with TZ1090 as a way to
limit the bitrate of the bus if some devices on the bus don't support
the full speed, e.g. we had an HDMI chip that would get confused at 400khz.

So would it be acceptable to change it to just clamp the bitrate to the
maximum rate supported, before the bitrate_khz calculation, such that if
you specified 1MHz in DT, it could safely fall back to 400KHz until the
driver supports the faster mode?

Cheers
James

>  
>  	/* Find the prescale that would give us that inc (approx delay = 0) */
>  	prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
> 


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

Re: [PATCH 8/8] i2c: img-scb: verify support for requested bit rate

[James Hogan]

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

On 27/07/15 12:47, Sifan Naeem wrote:
> The requested bit rate can be outside the range supported by the driver.
> The maximum bit rate this driver supports at the moment is 400Khz.
> 
> Return -EINVAL if the bit rate is larger than 400khz.
> 
> Maximum speed supported by the driver can be increased to 1Mhz by
> adding support for "fast plus mode" in the future.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index bbfee33..07a039c 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1157,6 +1157,12 @@ static int img_i2c_init(struct img_i2c *i2c)
>  			break;
>  		}
>  	}
> +	if (i2c->bitrate > timing.max_bitrate) {
> +		dev_err(i2c->adap.dev.parent,
> +			"requested bitrate (%d) is higher than the max bitrate supported (%d)\n",
> +			 i2c->bitrate, timing.max_bitrate);
> +		return -EINVAL;
> +	}

The timing is only chosen if i2c->bitrate <= timing.max_bitrate, so
you'd only hit this case if none of the timings were valid, in which
case timing == timings[0], so when you print timing.max_bitrate it won't
be the max bitrate supported, it'll be the max bitrate of the first
timing in the array.

Anyway, I think the original intent of the DT provided clock-frequency
was more as a maximum bitrate. This was used with TZ1090 as a way to
limit the bitrate of the bus if some devices on the bus don't support
the full speed, e.g. we had an HDMI chip that would get confused at 400khz.

So would it be acceptable to change it to just clamp the bitrate to the
maximum rate supported, before the bitrate_khz calculation, such that if
you specified 1MHz in DT, it could safely fall back to 400KHz until the
driver supports the faster mode?

Cheers
James

>  
>  	/* Find the prescale that would give us that inc (approx delay = 0) */
>  	prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
> 


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

Re: [PATCH 7/8] i2c: img-scb: improve transaction complete handle

[James Hogan]

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

Hi Sifan,

On 27/07/15 12:47, Sifan Naeem wrote:
> Clear line status and all interrupts when transaction is complete,
> as not doing so might leave unserviced interrupts that might be

Do you have a specific example of when this might happen, and whether it
could occur after img_i2c_complete_transaction()?

I'm just wondering if it would be better to do this before starting
every new message instead of after handling the last irq that is of
interest (maybe somewhere in img_i2c_xfer).

> handled in the context of a new transfer. Soft reset if the the
> transfer failed to bring back the i2c block to a reset state.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 341130e..bbfee33 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -626,7 +626,10 @@ static void img_i2c_complete_transaction(struct img_i2c *i2c, int status)
>  	img_i2c_switch_mode(i2c, MODE_INACTIVE);
>  	if (status) {
>  		i2c->msg_status = status;
> -		img_i2c_transaction_halt(i2c, false);
> +		img_i2c_soft_reset(i2c);

This seems like overkill. This will only happen in a couple of cases:
(1) an automatic mode ack error, which is completely recoverable.
(2) a fatal error (clock low timeout), which switches mode to MODE_FATAL
anyway, preventing further transactions.

Cheers
James

> +	} else {
> +		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> +		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>  	}
>  	complete(&i2c->msg_complete);
>  }
> 


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

Re: [PATCH 7/8] i2c: img-scb: improve transaction complete handle

[James Hogan]

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

Hi Sifan,

On 27/07/15 12:47, Sifan Naeem wrote:
> Clear line status and all interrupts when transaction is complete,
> as not doing so might leave unserviced interrupts that might be

Do you have a specific example of when this might happen, and whether it
could occur after img_i2c_complete_transaction()?

I'm just wondering if it would be better to do this before starting
every new message instead of after handling the last irq that is of
interest (maybe somewhere in img_i2c_xfer).

> handled in the context of a new transfer. Soft reset if the the
> transfer failed to bring back the i2c block to a reset state.
> 
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 341130e..bbfee33 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -626,7 +626,10 @@ static void img_i2c_complete_transaction(struct img_i2c *i2c, int status)
>  	img_i2c_switch_mode(i2c, MODE_INACTIVE);
>  	if (status) {
>  		i2c->msg_status = status;
> -		img_i2c_transaction_halt(i2c, false);
> +		img_i2c_soft_reset(i2c);

This seems like overkill. This will only happen in a couple of cases:
(1) an automatic mode ack error, which is completely recoverable.
(2) a fatal error (clock low timeout), which switches mode to MODE_FATAL
anyway, preventing further transactions.

Cheers
James

> +	} else {
> +		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> +		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>  	}
>  	complete(&i2c->msg_complete);
>  }
> 


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

RE: [PATCH 6/8] i2c: img-scb: remove start bit detected status after handling

[Sifan Naeem]

> -----Original Message-----
> From: James Hogan
> Sent: 28 July 2015 14:53
> To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org
> Cc: Stable kernel (v3.19+)
> Subject: Re: [PATCH 6/8] i2c: img-scb: remove start bit detected status after
> handling
> 
> On 27/07/15 12:47, Sifan Naeem wrote:
> > Remove start bit detected status after it is handled, doing so will
> > prevent this condition being hit for every interrupt on a particular
> > transfer.
> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> > Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |   16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 653f9bd..341130e 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -871,10 +871,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  	}
> >
> >  	/* Enable transaction halt on start bit */
> > -	if (!i2c->last_msg && i2c->line_status & LINESTAT_START_BIT_DET) {
> 
> i2c->line_status accumulates the line status bits that have been seen
> with each interrupt. If we're only interested in that bit from the current
> interrupt, should it just be referring to line_status (the argument to
> img_i2c_auto) instead of i2c->line_status?
> 
Yes, I can't think of why we cannot use line_status from the argument.

Thanks,
Sifan

> Cheers
> James
> 
> > -		img_i2c_transaction_halt(i2c, true);
> > -		/* we're no longer interested in the slave event */
> > -		i2c->int_enable &= ~INT_SLAVE_EVENT;
> > +	if (i2c->line_status & LINESTAT_START_BIT_DET) {
> > +		if (!i2c->last_msg) {
> > +			img_i2c_transaction_halt(i2c, true);
> > +			/* we're no longer interested in the slave event */
> > +			i2c->int_enable &= ~INT_SLAVE_EVENT;
> > +		}
> > +		/*
> > +		 * Remove start bit detected status after it is handled,
> > +		 * doing so will prevent this condition being hit for
> > +		 * every interrupt on a particular transfer.
> > +		 */
> > +		i2c->line_status &= ~LINESTAT_START_BIT_DET;
> >  	}
> >
> >  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> >

RE: [PATCH 8/8] i2c: img-scb: verify support for requested bit rate

[Sifan Naeem]

Hi James,

> On 27/07/15 12:47, Sifan Naeem wrote:
> > The requested bit rate can be outside the range supported by the driver.
> > The maximum bit rate this driver supports at the moment is 400Khz.
> >
> > Return -EINVAL if the bit rate is larger than 400khz.
> >
> > Maximum speed supported by the driver can be increased to 1Mhz by
> > adding support for "fast plus mode" in the future.
> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> > Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index bbfee33..07a039c 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -1157,6 +1157,12 @@ static int img_i2c_init(struct img_i2c *i2c)
> >  			break;
> >  		}
> >  	}
> > +	if (i2c->bitrate > timing.max_bitrate) {
> > +		dev_err(i2c->adap.dev.parent,
> > +			"requested bitrate (%d) is higher than the max
> bitrate supported (%d)\n",
> > +			 i2c->bitrate, timing.max_bitrate);
> > +		return -EINVAL;
> > +	}
> 
> The timing is only chosen if i2c->bitrate <= timing.max_bitrate, so you'd only
> hit this case if none of the timings were valid, in which case timing ==
> timings[0], so when you print timing.max_bitrate it won't be the max bitrate
> supported, it'll be the max bitrate of the first timing in the array.
> 
> Anyway, I think the original intent of the DT provided clock-frequency was
> more as a maximum bitrate. This was used with TZ1090 as a way to limit the
> bitrate of the bus if some devices on the bus don't support the full speed,
> e.g. we had an HDMI chip that would get confused at 400khz.
> 
> So would it be acceptable to change it to just clamp the bitrate to the
> maximum rate supported, before the bitrate_khz calculation, such that if you
> specified 1MHz in DT, it could safely fall back to 400KHz until the driver
> supports the faster mode?
> 
I'll rework this, when the requested bit rate is larger than the maximum supported, set the bitrate down to the maximum supported before bitrate_khz is calculated. 

Thanks,
Sifan
> Cheers
> James
> 
> >
> >  	/* Find the prescale that would give us that inc (approx delay = 0) */
> >  	prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
> >

RE: [PATCH 7/8] i2c: img-scb: improve transaction complete handle

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 29 July 2015 13:22
> To: Sifan Naeem; Wolfram Sang; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Stable kernel (v3.19+)
> Subject: Re: [PATCH 7/8] i2c: img-scb: improve transaction complete handle
> 
> Hi Sifan,
> 
> On 27/07/15 12:47, Sifan Naeem wrote:
> > Clear line status and all interrupts when transaction is complete, as
> > not doing so might leave unserviced interrupts that might be
> 
> Do you have a specific example of when this might happen, and whether it
> could occur after img_i2c_complete_transaction()?
> 
> I'm just wondering if it would be better to do this before starting every new
> message instead of after handling the last irq that is of interest (maybe
> somewhere in img_i2c_xfer).
> 
Moved to happen before each transfer.

> > handled in the context of a new transfer. Soft reset if the the
> > transfer failed to bring back the i2c block to a reset state.
> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 341130e..bbfee33 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -626,7 +626,10 @@ static void img_i2c_complete_transaction(struct
> img_i2c *i2c, int status)
> >  	img_i2c_switch_mode(i2c, MODE_INACTIVE);
> >  	if (status) {
> >  		i2c->msg_status = status;
> > -		img_i2c_transaction_halt(i2c, false);
> > +		img_i2c_soft_reset(i2c);
> 
> This seems like overkill. This will only happen in a couple of cases:
> (1) an automatic mode ack error, which is completely recoverable.
> (2) a fatal error (clock low timeout), which switches mode to MODE_FATAL
> anyway, preventing further transactions.
> 
Removed. 

Thanks,
Sifan
> Cheers
> James
> 
> > +	} else {
> > +		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> > +		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> >  	}
> >  	complete(&i2c->msg_complete);
> >  }
> >

RE: [PATCH 7/8] i2c: img-scb: improve transaction complete handle

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 29 July 2015 13:22
> To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org
> Cc: Stable kernel (v3.19+)
> Subject: Re: [PATCH 7/8] i2c: img-scb: improve transaction complete handle
> 
> Hi Sifan,
> 
> On 27/07/15 12:47, Sifan Naeem wrote:
> > Clear line status and all interrupts when transaction is complete, as
> > not doing so might leave unserviced interrupts that might be
> 
> Do you have a specific example of when this might happen, and whether it
> could occur after img_i2c_complete_transaction()?
> 
> I'm just wondering if it would be better to do this before starting every new
> message instead of after handling the last irq that is of interest (maybe
> somewhere in img_i2c_xfer).
> 
Moved to happen before each transfer.

> > handled in the context of a new transfer. Soft reset if the the
> > transfer failed to bring back the i2c block to a reset state.
> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> > Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 341130e..bbfee33 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -626,7 +626,10 @@ static void img_i2c_complete_transaction(struct
> img_i2c *i2c, int status)
> >  	img_i2c_switch_mode(i2c, MODE_INACTIVE);
> >  	if (status) {
> >  		i2c->msg_status = status;
> > -		img_i2c_transaction_halt(i2c, false);
> > +		img_i2c_soft_reset(i2c);
> 
> This seems like overkill. This will only happen in a couple of cases:
> (1) an automatic mode ack error, which is completely recoverable.
> (2) a fatal error (clock low timeout), which switches mode to MODE_FATAL
> anyway, preventing further transactions.
> 
Removed. 

Thanks,
Sifan
> Cheers
> James
> 
> > +	} else {
> > +		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
> > +		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
> >  	}
> >  	complete(&i2c->msg_complete);
> >  }
> >

Re: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Wolfram Sang]

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

On Mon, Jul 27, 2015 at 12:47:13PM +0100, Sifan Naeem wrote:
> Following patches are required to fix the existing driver to
> support i2c on pistachio.
> 
> Tested on Pistachio bub using an Adafruit I2C Non-Volatile FRAM Breakout
> (256Kbit / 32KByte) eeprom.
> 
> Used i2c buildroot tools to test the eeprom and the other i2c blocks.
> Also used dd commands to copy data to and then to dump data from the
> eeprom. i2ctransfer was used to test repeated starts and verified
> using a scope.

That sounds like good testing \o/ If you are happy with i2ctransfer,
then please consider adding Tested-by tags to the i2ctransfer patches,
so Jean gets a more cosy feeling to include them to i2ctools.

> 
> Cc: Stable kernel (v3.19+) <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

I am happy for indications like "please consider adding this line for
stable: ...", but not CC stable yourself. These patches need changes,
so they are noise for the stable lists. The ones I ultimately apply are
the proper ones.

That being said, I'll mark all your patches "changes reqeuested" since you
need to resend based on current review from James (thanks a lot!). Even
if some patches are good and got acked, but it is easier for me to mark
them all the same :)

> 
> Sifan Naeem (8):
>   i2c: img-scb: enable fencing for all versions of the ip
>   i2c: img-scb: do dummy writes before fifo access
>   i2c: img-scb: use DIV_ROUND_UP to round divisor values
>   i2c: img-scb: fix LOW and HIGH period values for the SCL clock
>   i2c: img-scb: reset interrupts in img_i2c_soft_reset
>   i2c: img-scb: remove start bit detected status after handling
>   i2c: img-scb: improve transaction complete handle
>   i2c: img-scb: verify support for requested bit rate
> 
>  drivers/i2c/busses/i2c-img-scb.c |  101 ++++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 43 deletions(-)
> 
> -- 
> 1.7.9.5
> 

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

Re: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Wolfram Sang]

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

On Mon, Jul 27, 2015 at 12:47:13PM +0100, Sifan Naeem wrote:
> Following patches are required to fix the existing driver to
> support i2c on pistachio.
> 
> Tested on Pistachio bub using an Adafruit I2C Non-Volatile FRAM Breakout
> (256Kbit / 32KByte) eeprom.
> 
> Used i2c buildroot tools to test the eeprom and the other i2c blocks.
> Also used dd commands to copy data to and then to dump data from the
> eeprom. i2ctransfer was used to test repeated starts and verified
> using a scope.

That sounds like good testing \o/ If you are happy with i2ctransfer,
then please consider adding Tested-by tags to the i2ctransfer patches,
so Jean gets a more cosy feeling to include them to i2ctools.

> 
> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>

I am happy for indications like "please consider adding this line for
stable: ...", but not CC stable yourself. These patches need changes,
so they are noise for the stable lists. The ones I ultimately apply are
the proper ones.

That being said, I'll mark all your patches "changes reqeuested" since you
need to resend based on current review from James (thanks a lot!). Even
if some patches are good and got acked, but it is easier for me to mark
them all the same :)

> 
> Sifan Naeem (8):
>   i2c: img-scb: enable fencing for all versions of the ip
>   i2c: img-scb: do dummy writes before fifo access
>   i2c: img-scb: use DIV_ROUND_UP to round divisor values
>   i2c: img-scb: fix LOW and HIGH period values for the SCL clock
>   i2c: img-scb: reset interrupts in img_i2c_soft_reset
>   i2c: img-scb: remove start bit detected status after handling
>   i2c: img-scb: improve transaction complete handle
>   i2c: img-scb: verify support for requested bit rate
> 
>  drivers/i2c/busses/i2c-img-scb.c |  101 ++++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 43 deletions(-)
> 
> -- 
> 1.7.9.5
> 

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

Re: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Ezequiel Garcia]

Wolfram, Jean:

On 31 July 2015 at 08:12, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Mon, Jul 27, 2015 at 12:47:13PM +0100, Sifan Naeem wrote:
>> Following patches are required to fix the existing driver to
>> support i2c on pistachio.
>>
>> Tested on Pistachio bub using an Adafruit I2C Non-Volatile FRAM Breakout
>> (256Kbit / 32KByte) eeprom.
>>
>> Used i2c buildroot tools to test the eeprom and the other i2c blocks.
>> Also used dd commands to copy data to and then to dump data from the
>> eeprom. i2ctransfer was used to test repeated starts and verified
>> using a scope.
>
> That sounds like good testing \o/ If you are happy with i2ctransfer,
> then please consider adding Tested-by tags to the i2ctransfer patches,
> so Jean gets a more cosy feeling to include them to i2ctools.
>
>>

Definitely! i2ctransfer was a HUGE help while developing this driver,
specially when preparing involved transfers to test repeated starts.

Any chance it gets merged in its current state? I think it'll be useful, even
if the usage cli API is still a moving target.

We could print some warning "i2ctransfer is experimental"
or something like that.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Wolfram Sang]

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


> > That sounds like good testing \o/ If you are happy with i2ctransfer,
> > then please consider adding Tested-by tags to the i2ctransfer patches,
> > so Jean gets a more cosy feeling to include them to i2ctools.
> >
> 
> Definitely! i2ctransfer was a HUGE help while developing this driver,
> specially when preparing involved transfers to test repeated starts.

Glad to hear that! I also discovered and fixed a corner case in repeated
start handling recently in the i2c-rcar driver thanks to i2ctransfer.

> Any chance it gets merged in its current state? I think it'll be useful, even
> if the usage cli API is still a moving target.

I don't think it is anymore TBH. Not more than any other tool.

Waiving to Jean \o_ :)


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

Re: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio

[Ezequiel Garcia]

On 7 September 2015 at 10:50, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> > That sounds like good testing \o/ If you are happy with i2ctransfer,
>> > then please consider adding Tested-by tags to the i2ctransfer patches,
>> > so Jean gets a more cosy feeling to include them to i2ctools.
>> >
>>
>> Definitely! i2ctransfer was a HUGE help while developing this driver,
>> specially when preparing involved transfers to test repeated starts.
>
> Glad to hear that! I also discovered and fixed a corner case in repeated
> start handling recently in the i2c-rcar driver thanks to i2ctransfer.
>
>> Any chance it gets merged in its current state? I think it'll be useful, even
>> if the usage cli API is still a moving target.
>
> I don't think it is anymore TBH. Not more than any other tool.
>
> Waiving to Jean \o_ :)
>

OK, let me wave as well \\O//

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: i2ctransfer (Was: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio)

[Jean Delvare]

On Mon, 7 Sep 2015 11:22:23 -0300, Ezequiel Garcia wrote:
> On 7 September 2015 at 10:50, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> >
> >> > That sounds like good testing \o/ If you are happy with i2ctransfer,
> >> > then please consider adding Tested-by tags to the i2ctransfer patches,
> >> > so Jean gets a more cosy feeling to include them to i2ctools.
> >> >
> >>
> >> Definitely! i2ctransfer was a HUGE help while developing this driver,
> >> specially when preparing involved transfers to test repeated starts.
> >
> > Glad to hear that! I also discovered and fixed a corner case in repeated
> > start handling recently in the i2c-rcar driver thanks to i2ctransfer.
> >
> >> Any chance it gets merged in its current state? I think it'll be useful, even
> >> if the usage cli API is still a moving target.
> >
> > I don't think it is anymore TBH. Not more than any other tool.
> >
> > Waiving to Jean \o_ :)
> 
> OK, let me wave as well \\O//

Review in progress, will continue tomorrow.

-- 
Jean Delvare
SUSE L3 Support