[PATCH 0/5] i2c: sirf: a bundle of fixes for stability

[PATCH 0/5] i2c: sirf: a bundle of fixes for stability

[Barry Song]

here it is a bundle of fixes to sirfsoc i2c driver to make the driver stable
in random hardware and connect random i2c clients.

Zhiwu Song (5):
  i2c: sirf: reset i2c controller early after we get a noack
  i2c: sirf: reset i2c controller after resuming
  i2c: sirf: fix the typo for setting bitrate to less than 100k
  i2c: sirfsoc: support reverse direction of address
  i2c: sirf: retry 3 times as sometimes we get random noack and timeout

 drivers/i2c/busses/i2c-sirf.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

-- 
1.8.2.3

[PATCH 1/5] i2c: sirf: reset i2c controller early after we get a noack

[Barry Song]

From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>

Due to hardware ANOMALY, we need to reset I2C earlier after
we get NOACK while accessing non-existing clients, otherwise
we will get errors even we access existing clients later

Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/i2c/busses/i2c-sirf.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index a63c7d5..055210e 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -65,6 +65,8 @@
 #define SIRFSOC_I2C_START		BIT(7)
 
 #define SIRFSOC_I2C_DEFAULT_SPEED 100000
+#define SIRFSOC_I2C_ERR_NOACK      1
+#define SIRFSOC_I2C_ERR_TIMEOUT    2
 
 struct sirfsoc_i2c {
 	void __iomem *base;
@@ -143,14 +145,24 @@ static irqreturn_t i2c_sirfsoc_irq(int irq, void *dev_id)
 
 	if (i2c_stat & SIRFSOC_I2C_STAT_ERR) {
 		/* Error conditions */
-		siic->err_status = 1;
+		siic->err_status = SIRFSOC_I2C_ERR_NOACK;
 		writel(SIRFSOC_I2C_STAT_ERR, siic->base + SIRFSOC_I2C_STATUS);
 
 		if (i2c_stat & SIRFSOC_I2C_STAT_NACK)
-			dev_err(&siic->adapter.dev, "ACK not received\n");
+			dev_dbg(&siic->adapter.dev, "ACK not received\n");
 		else
 			dev_err(&siic->adapter.dev, "I2C error\n");
 
+		/*
+		 * Due to hardware ANOMALY, we need to reset I2C earlier after
+		 * we get NOACK while accessing non-existing clients, otherwise
+		 * we will get errors even we access existing clients later
+		 */
+		writel(readl(siic->base + SIRFSOC_I2C_CTRL) | SIRFSOC_I2C_RESET,
+				siic->base + SIRFSOC_I2C_CTRL);
+		while (readl(siic->base + SIRFSOC_I2C_CTRL) & SIRFSOC_I2C_RESET)
+			cpu_relax();
+
 		complete(&siic->done);
 	} else if (i2c_stat & SIRFSOC_I2C_STAT_CMD_DONE) {
 		/* CMD buffer execution complete */
@@ -191,7 +203,6 @@ static int i2c_sirfsoc_xfer_msg(struct sirfsoc_i2c *siic, struct i2c_msg *msg)
 	u32 regval = readl(siic->base + SIRFSOC_I2C_CTRL);
 	/* timeout waiting for the xfer to finish or fail */
 	int timeout = msecs_to_jiffies((msg->len + 1) * 50);
-	int ret = 0;
 
 	i2c_sirfsoc_set_address(siic, msg);
 
@@ -200,7 +211,7 @@ static int i2c_sirfsoc_xfer_msg(struct sirfsoc_i2c *siic, struct i2c_msg *msg)
 	i2c_sirfsoc_queue_cmd(siic);
 
 	if (wait_for_completion_timeout(&siic->done, timeout) == 0) {
-		siic->err_status = 1;
+		siic->err_status = SIRFSOC_I2C_ERR_TIMEOUT;
 		dev_err(&siic->adapter.dev, "Transfer timeout\n");
 	}
 
@@ -208,16 +219,14 @@ static int i2c_sirfsoc_xfer_msg(struct sirfsoc_i2c *siic, struct i2c_msg *msg)
 		siic->base + SIRFSOC_I2C_CTRL);
 	writel(0, siic->base + SIRFSOC_I2C_CMD_START);
 
-	if (siic->err_status) {
+	/* i2c control doesn't response, reset it */
+	if (siic->err_status == SIRFSOC_I2C_ERR_TIMEOUT) {
 		writel(readl(siic->base + SIRFSOC_I2C_CTRL) | SIRFSOC_I2C_RESET,
 			siic->base + SIRFSOC_I2C_CTRL);
 		while (readl(siic->base + SIRFSOC_I2C_CTRL) & SIRFSOC_I2C_RESET)
 			cpu_relax();
-
-		ret = -EIO;
 	}
-
-	return ret;
+	return siic->err_status ? -EIO : 0;
 }
 
 static u32 i2c_sirfsoc_func(struct i2c_adapter *adap)
-- 
1.8.2.3

Re: [PATCH 1/5] i2c: sirf: reset i2c controller early after we get a noack

[Wolfram Sang]

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


> +		while (readl(siic->base + SIRFSOC_I2C_CTRL) & SIRFSOC_I2C_RESET)
> +			cpu_relax();

Are you sure you want no timeout here? Same for patch 2/5.


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

Re: [PATCH 1/5] i2c: sirf: reset i2c controller early after we get a noack

[Barry Song]

2013/8/20 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
>
>> +             while (readl(siic->base + SIRFSOC_I2C_CTRL) & SIRFSOC_I2C_RESET)
>> +                     cpu_relax();
>
> Are you sure you want no timeout here? Same for patch 2/5.

the real situation is that timeout never happen for resetting the i2c
controller, in pressure test, this loop will always break.

>
-barry

[PATCH 2/5] i2c: sirf: we need to wait I2C_RESET status in resume

[Barry Song]

From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>

this fixes the issue that we lost to wait the i2c reset finished.

Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/i2c/busses/i2c-sirf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index 055210e..45a8881 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -425,6 +425,8 @@ static int i2c_sirfsoc_resume(struct device *dev)
 
 	clk_enable(siic->clk);
 	writel(SIRFSOC_I2C_RESET, siic->base + SIRFSOC_I2C_CTRL);
+	while (readl(siic->base + SIRFSOC_I2C_CTRL) & SIRFSOC_I2C_RESET)
+		cpu_relax();
 	writel(SIRFSOC_I2C_CORE_EN | SIRFSOC_I2C_MASTER_MODE,
 		siic->base + SIRFSOC_I2C_CTRL);
 	writel(siic->clk_div, siic->base + SIRFSOC_I2C_CLK_CTRL);
-- 
1.8.2.3

[PATCH 3/5] i2c: sirf: fix the typo for setting bitrate to less than 100k

[Barry Song]

From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>

there is a typo before, it makes the final bitrate wrong, this patch fixes
it.

Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/i2c/busses/i2c-sirf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index 45a8881..746388f 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -357,7 +357,7 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
 
 	if (bitrate < 100000)
 		regval =
-			(2 * ctrl_speed) / (2 * bitrate * 11);
+			(2 * ctrl_speed) / (bitrate * 11);
 	else
 		regval = ctrl_speed / (bitrate * 5);
 
-- 
1.8.2.3

[PATCH 4/5] i2c: sirfsoc: support reverse direction of address

[Barry Song]

From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>

if users set I2C_M_REV_DIR_ADDR, revert the direction of address.

Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/i2c/busses/i2c-sirf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index 746388f..d2b7913 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -195,6 +195,10 @@ static void i2c_sirfsoc_set_address(struct sirfsoc_i2c *siic,
 	if (msg->flags & I2C_M_RD)
 		addr |= 1;
 
+	/* Reverse direction bit */
+	if (msg->flags & I2C_M_REV_DIR_ADDR)
+		addr ^= 1;
+
 	writel(addr, siic->base + SIRFSOC_I2C_CMD(siic->cmd_ptr++));
 }
 
-- 
1.8.2.3

Re: [PATCH 4/5] i2c: sirfsoc: support reverse direction of address

[Wolfram Sang]

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

On Tue, Aug 13, 2013 at 05:11:30PM +0800, Barry Song wrote:
> From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
> 
> if users set I2C_M_REV_DIR_ADDR, revert the direction of address.
> 
> Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
> Signed-off-by: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
> Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>

Hmm, you are not advertising I2C_FUNC_PROTOCOL_MANGLING so users should
not use it. Does the master allow to implement the rest of mangling,
too?


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

Re: [PATCH 4/5] i2c: sirfsoc: support reverse direction of address

[Barry Song]

2013/8/20 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
> On Tue, Aug 13, 2013 at 05:11:30PM +0800, Barry Song wrote:
>> From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
>>
>> if users set I2C_M_REV_DIR_ADDR, revert the direction of address.
>>
>> Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
>> Signed-off-by: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
>> Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
>
> Hmm, you are not advertising I2C_FUNC_PROTOCOL_MANGLING so users should
> not use it.

Wolfram, good comments. the old codes did miss
I2C_FUNC_PROTOCOL_MANGLING which i will fix in v2.

> Does the master allow to implement the rest of mangling,
> too?
>

i think only I2C_M_REV_DIR_ADDR. i didn't hear I2C_M_NOSTART,
I2C_M_IGNORE_NAK from hardware guys.

-barry

Re: [PATCH 4/5] i2c: sirfsoc: support reverse direction of address

[Wolfram Sang]

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


> > Hmm, you are not advertising I2C_FUNC_PROTOCOL_MANGLING so users should
> > not use it.
> 
> Wolfram, good comments. the old codes did miss
> I2C_FUNC_PROTOCOL_MANGLING which i will fix in v2.
> 
> > Does the master allow to implement the rest of mangling,
> > too?
> >
> 
> i think only I2C_M_REV_DIR_ADDR. i didn't hear I2C_M_NOSTART,
> I2C_M_IGNORE_NAK from hardware guys.

Hmm, since the driver can't support all of MANGLING, it might be wise to
not advertise it. I am undecided how to handle this generally. Until
then, I'll accept this patch (and the series) as is.


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

Re: [PATCH 4/5] i2c: sirfsoc: support reverse direction of address

[Barry Song]

2013/8/28 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
>
>> > Hmm, you are not advertising I2C_FUNC_PROTOCOL_MANGLING so users should
>> > not use it.
>>
>> Wolfram, good comments. the old codes did miss
>> I2C_FUNC_PROTOCOL_MANGLING which i will fix in v2.
>>
>> > Does the master allow to implement the rest of mangling,
>> > too?
>> >
>>
>> i think only I2C_M_REV_DIR_ADDR. i didn't hear I2C_M_NOSTART,
>> I2C_M_IGNORE_NAK from hardware guys.
>
> Hmm, since the driver can't support all of MANGLING, it might be wise to
> not advertise it. I am undecided how to handle this generally. Until
> then, I'll accept this patch (and the series) as is.

ok, thanks Wolfram. then i will not have v2.
>

-barry

[PATCH 5/5] i2c: sirf: retry 3 times as sometimes we get random noack and timeout

[Barry Song]

From: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>

let i2c core retry 3 times as sometimes we get random noack and timeout
even when we access an existing i2c client.

Signed-off-by: Zhiwu Song <Zhiwu.Song-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/i2c/busses/i2c-sirf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index d2b7913..90adc2d 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -230,7 +230,7 @@ static int i2c_sirfsoc_xfer_msg(struct sirfsoc_i2c *siic, struct i2c_msg *msg)
 		while (readl(siic->base + SIRFSOC_I2C_CTRL) & SIRFSOC_I2C_RESET)
 			cpu_relax();
 	}
-	return siic->err_status ? -EIO : 0;
+	return siic->err_status ? -EAGAIN : 0;
 }
 
 static u32 i2c_sirfsoc_func(struct i2c_adapter *adap)
@@ -334,6 +334,7 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
 
 	adap->algo = &i2c_sirfsoc_algo;
 	adap->algo_data = siic;
+	adap->retries = 3;
 
 	adap->dev.of_node = pdev->dev.of_node;
 	adap->dev.parent = &pdev->dev;
-- 
1.8.2.3