[PATCH 0/3] i2c: exynos5: bus recovery implementation

[PATCH 0/3] i2c: exynos5: bus recovery implementation

[Andrzej Hajda]

Hi,

This patchset adds bus recovery functionality to Exynos HSI2C, it is used in
case of transmission timeouts.
Internal tests shows that it significantly improves I2C transmissions.

Andi, could you take a look at it/test, feedback welcome.

Later, I will post also patches to MHL chip which uses this functionality.
MHL chip has known bug of being in incorrect state after hard reset,
I2C bus recovery cures it.

Regards
Andrzej


Andrzej Hajda (3):
  i2c: exynos5: change internal transmission timeout to 100ms
  i2c: exynos5: implement bus recovery functionality
  i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

 drivers/i2c/busses/i2c-exynos5.c | 52 +++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 8 deletions(-)

-- 
2.15.0

[PATCH 1/3] i2c: exynos5: change internal transmission timeout to 100ms

[Andrzej Hajda]

Exynos-I2C uses default timeout of 1 second for the whole transaction,
including re-transmissions due to arbitration lost errors (-EAGAIN).
To allow re-transmissions driver's internal timeout should be significantly
lower, 100ms seems to be good candidate.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 3855e0b11877..b02428498f6d 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -170,7 +170,7 @@
 #define HSI2C_HS_TX_CLOCK	1000000
 #define HSI2C_FS_TX_CLOCK	100000
 
-#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(1000))
+#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(100))
 
 #define HSI2C_EXYNOS7	BIT(0)
 
-- 
2.15.0

Re: [1/3] i2c: exynos5: change internal transmission timeout to 100ms

[Wolfram Sang]

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

On Thu, Nov 30, 2017 at 03:30:05PM +0100, Andrzej Hajda wrote:
> Exynos-I2C uses default timeout of 1 second for the whole transaction,
> including re-transmissions due to arbitration lost errors (-EAGAIN).
> To allow re-transmissions driver's internal timeout should be significantly
> lower, 100ms seems to be good candidate.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Applied to for-next, thanks!


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

[PATCH 2/3] i2c: exynos5: implement bus recovery functionality

[Andrzej Hajda]

In some circumstances I2C clients can be in abnormal state, to allow
recover them from it I2C master can pulse SCL line until SDA line
goes up and then generate STOP condition. Exynos driver does not have
possibility to manipulate I2C lines directly, but it can provide similar
functionality using manual commands. Replacing I2C adapter reset with
bus recovery significantly incereases I2C bus robustness in case of timeouts.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 44 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index b02428498f6d..4ca43980e2ed 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -128,6 +128,10 @@
 #define HSI2C_TIMEOUT_EN			(1u << 31)
 #define HSI2C_TIMEOUT_MASK			0xff
 
+/* I2C_MANUAL_CMD register bits */
+#define HSI2C_CMD_READ_DATA			(1u << 4)
+#define HSI2C_CMD_SEND_STOP			(1u << 2)
+
 /* I2C_TRANS_STATUS register bits */
 #define HSI2C_MASTER_BUSY			(1u << 17)
 #define HSI2C_SLAVE_BUSY			(1u << 16)
@@ -613,6 +617,26 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop)
 	spin_unlock_irqrestore(&i2c->lock, flags);
 }
 
+void exynos5_i2c_recovery(struct exynos5_i2c *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->regs + HSI2C_CTL) | HSI2C_RXCHON;
+	writel(val, i2c->regs + HSI2C_CTL);
+	val = readl(i2c->regs + HSI2C_CONF) & ~HSI2C_AUTO_MODE;
+	writel(val, i2c->regs + HSI2C_CONF);
+
+	writel(HSI2C_CMD_READ_DATA, i2c->regs + HSI2C_MANUAL_CMD);
+	exynos5_i2c_wait_bus_idle(i2c);
+	writel(HSI2C_CMD_SEND_STOP, i2c->regs + HSI2C_MANUAL_CMD);
+	exynos5_i2c_wait_bus_idle(i2c);
+
+	val = readl(i2c->regs + HSI2C_CTL) & ~HSI2C_RXCHON;
+	writel(val, i2c->regs + HSI2C_CTL);
+	val = readl(i2c->regs + HSI2C_CONF) | HSI2C_AUTO_MODE;
+	writel(val, i2c->regs + HSI2C_CONF);
+}
+
 static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
 			      struct i2c_msg *msgs, int stop)
 {
@@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
 		ret = exynos5_i2c_wait_bus_idle(i2c);
 
 	if (ret < 0) {
-		exynos5_i2c_reset(i2c);
+		exynos5_i2c_recovery(i2c);
 		if (ret == -ETIMEDOUT)
 			dev_warn(i2c->dev, "%s timeout\n",
 				 (msgs->flags & I2C_M_RD) ? "rx" : "tx");
@@ -703,6 +727,23 @@ static const struct i2c_algorithm exynos5_i2c_algorithm = {
 	.functionality		= exynos5_i2c_func,
 };
 
+int exynos5_i2c_recover_bus(struct i2c_adapter *adap)
+{
+	struct exynos5_i2c *i2c = adap->algo_data;
+
+	i2c_lock_adapter(adap);
+	clk_enable(i2c->clk);
+	exynos5_i2c_recovery(i2c);
+	clk_disable(i2c->clk);
+	i2c_unlock_adapter(adap);
+
+	return 0;
+}
+
+static struct i2c_bus_recovery_info exynos5_i2c_recovery_info = {
+	.recover_bus = exynos5_i2c_recover_bus
+};
+
 static int exynos5_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -773,6 +814,7 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
 		goto err_clk;
 
 	exynos5_i2c_reset(i2c);
+	i2c->adap.bus_recovery_info = &exynos5_i2c_recovery_info;
 
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret < 0)
-- 
2.15.0

Re: [2/3] i2c: exynos5: implement bus recovery functionality

[Wolfram Sang]

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

Hi,

On Thu, Nov 30, 2017 at 03:30:06PM +0100, Andrzej Hajda wrote:
> In some circumstances I2C clients can be in abnormal state, to allow
> recover them from it I2C master can pulse SCL line until SDA line
> goes up and then generate STOP condition. Exynos driver does not have
> possibility to manipulate I2C lines directly, but it can provide similar
> functionality using manual commands. Replacing I2C adapter reset with

Really? READ_DATA looks like 8 clock pulses, but you need 9.

> bus recovery significantly incereases I2C bus robustness in case of timeouts.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>



> @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
>  		ret = exynos5_i2c_wait_bus_idle(i2c);
>  
>  	if (ret < 0) {
> -		exynos5_i2c_reset(i2c);
> +		exynos5_i2c_recovery(i2c);

Bus recovery is only defined for stalled busses, i.e. when SDA is stuck
low. It is not a suitable method to deal with general timeouts.


> +int exynos5_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct exynos5_i2c *i2c = adap->algo_data;
> +
> +	i2c_lock_adapter(adap);
> +	clk_enable(i2c->clk);
> +	exynos5_i2c_recovery(i2c);
> +	clk_disable(i2c->clk);
> +	i2c_unlock_adapter(adap);

Why do you need to lock? Aren't you protected by the call to master_xfer
which then detected a problem?

Regards,

   Wolfram


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

Re: [2/3] i2c: exynos5: implement bus recovery functionality

[Andrzej Hajda]

On 15.01.2018 21:52, Wolfram Sang wrote:
> Hi,
>
> On Thu, Nov 30, 2017 at 03:30:06PM +0100, Andrzej Hajda wrote:
>> In some circumstances I2C clients can be in abnormal state, to allow
>> recover them from it I2C master can pulse SCL line until SDA line
>> goes up and then generate STOP condition. Exynos driver does not have
>> possibility to manipulate I2C lines directly, but it can provide similar
>> functionality using manual commands. Replacing I2C adapter reset with
> Really? READ_DATA looks like 8 clock pulses, but you need 9.

This is why I have used word 'similar' :) Unfortunately chip does not
provide method to send exactly 9 pulses.
But according to [1] it is the worst case scenario, usually less than 9
pulses is enough.
Another solution I see is to READ_DATA twice, this way we will have 16
pulses.

[1]:
http://www.analog.com/media/en/technical-documentation/application-notes/54305147357414AN686_0.pdf
>
>> bus recovery significantly incereases I2C bus robustness in case of timeouts.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>
>
>> @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
>>  		ret = exynos5_i2c_wait_bus_idle(i2c);
>>  
>>  	if (ret < 0) {
>> -		exynos5_i2c_reset(i2c);
>> +		exynos5_i2c_recovery(i2c);
> Bus recovery is only defined for stalled busses, i.e. when SDA is stuck
> low. It is not a suitable method to deal with general timeouts.

You are right, I though I do not have precise info that SDA is stuck
low, but I guess status HSI2C_MASTER_ST_LOSE indicates is, I will check it.

>
>
>> +int exynos5_i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> +	struct exynos5_i2c *i2c = adap->algo_data;
>> +
>> +	i2c_lock_adapter(adap);
>> +	clk_enable(i2c->clk);
>> +	exynos5_i2c_recovery(i2c);
>> +	clk_disable(i2c->clk);
>> +	i2c_unlock_adapter(adap);
> Why do you need to lock? Aren't you protected by the call to master_xfer
> which then detected a problem?

If the problem is detected in master_xfer driver uses internal function
exynos5_i2c_recovery,
but function exynos5_i2c_recover_bus is called by i2c_recover_bus, and
the latter can be called from any context,
this is why I have added locking.

Regards
Andrzej
>
> Regards,
>
>    Wolfram
>

Re: [2/3] i2c: exynos5: implement bus recovery functionality

[Wolfram Sang]

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


> > Really? READ_DATA looks like 8 clock pulses, but you need 9.
> 
> This is why I have used word 'similar' :) Unfortunately chip does not
> provide method to send exactly 9 pulses.

Pity :(

> But according to [1] it is the worst case scenario, usually less than 9
> pulses is enough.

The I2C specification says "should send nine clock pulses". A recovery
mechanism which "usually works" doesn't sound very attractive.

> Another solution I see is to READ_DATA twice, this way we will have 16
> pulses.

Can't you clock a single pulse when requesting an ACK? Or does READ_DATE
clock an ACK so we might end up with 9 pulses after all?

> > Bus recovery is only defined for stalled busses, i.e. when SDA is stuck
> > low. It is not a suitable method to deal with general timeouts.
> 
> You are right, I though I do not have precise info that SDA is stuck
> low, but I guess status HSI2C_MASTER_ST_LOSE indicates is, I will check it.

Thank you!

> >> +	i2c_lock_adapter(adap);
> >> +	clk_enable(i2c->clk);
> >> +	exynos5_i2c_recovery(i2c);
> >> +	clk_disable(i2c->clk);
> >> +	i2c_unlock_adapter(adap);
> > Why do you need to lock? Aren't you protected by the call to master_xfer
> > which then detected a problem?
> 
> If the problem is detected in master_xfer driver uses internal function
> exynos5_i2c_recovery,
> but function exynos5_i2c_recover_bus is called by i2c_recover_bus, and
> the latter can be called from any context,
> this is why I have added locking.

But the wrong one? The adapter lock is for serializing access to the
adapter using the I2C API. If you need to protect register access within
your driver, then you need the lock within struct exynos5_i2c.


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

[PATCH 3/3] i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

[Andrzej Hajda]

HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
show that hardware is usually able to recover from this state without
interrupting the transfer. On the other side enforcing transfer repetition
in such case does not help in many situations, especially on busy systems
and causes -EAGAIN and -ETIMEOUT errors. Moreover documentation says that
such state can be caused by slave clock stretching, and should not be treated
as an error.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 4ca43980e2ed..d579b45b3092 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -445,12 +445,6 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 			i2c->state = -ETIMEDOUT;
 			goto stop;
 		}
-
-		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
-		if ((trans_status & HSI2C_MASTER_ST_MASK) == HSI2C_MASTER_ST_LOSE) {
-			i2c->state = -EAGAIN;
-			goto stop;
-		}
 	} else if (int_status & HSI2C_INT_I2C) {
 		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
 		if (trans_status & HSI2C_NO_DEV_ACK) {
-- 
2.15.0

Re: [3/3] i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

[Wolfram Sang]

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

On Thu, Nov 30, 2017 at 03:30:07PM +0100, Andrzej Hajda wrote:
> HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
> show that hardware is usually able to recover from this state without
> interrupting the transfer. On the other side enforcing transfer repetition
> in such case does not help in many situations, especially on busy systems
> and causes -EAGAIN and -ETIMEOUT errors. Moreover documentation says that
> such state can be caused by slave clock stretching, and should not be treated
> as an error.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Can this be applied independently of my comments to patch 2?


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

Re: [3/3] i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

[Andrzej Hajda]

On 15.01.2018 21:53, Wolfram Sang wrote:
> On Thu, Nov 30, 2017 at 03:30:07PM +0100, Andrzej Hajda wrote:
>> HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
>> show that hardware is usually able to recover from this state without
>> interrupting the transfer. On the other side enforcing transfer repetition
>> in such case does not help in many situations, especially on busy systems
>> and causes -EAGAIN and -ETIMEOUT errors. Moreover documentation says that
>> such state can be caused by slave clock stretching, and should not be treated
>> as an error.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Can this be applied independently of my comments to patch 2?
>
Yes, please apply it alone. I will continue work on patch 2.

--
Regards
Andrzej

Re: [3/3] i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

[Wolfram Sang]

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

On Tue, Jan 16, 2018 at 10:40:36AM +0100, Andrzej Hajda wrote:
> On 15.01.2018 21:53, Wolfram Sang wrote:
> > On Thu, Nov 30, 2017 at 03:30:07PM +0100, Andrzej Hajda wrote:
> >> HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
> >> show that hardware is usually able to recover from this state without
> >> interrupting the transfer. On the other side enforcing transfer repetition
> >> in such case does not help in many situations, especially on busy systems
> >> and causes -EAGAIN and -ETIMEOUT errors. Moreover documentation says that
> >> such state can be caused by slave clock stretching, and should not be treated
> >> as an error.
> >>
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > Can this be applied independently of my comments to patch 2?
> >
> Yes, please apply it alone. I will continue work on patch 2.

I just thought it might be nice to have a comment where you removed the
code summarizing your findings. So we will remember about this in the
future. Makes sense?


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

Re: [3/3] i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

[Andrzej Hajda]

On 18.01.2018 00:23, Wolfram Sang wrote:
> On Tue, Jan 16, 2018 at 10:40:36AM +0100, Andrzej Hajda wrote:
>> On 15.01.2018 21:53, Wolfram Sang wrote:
>>> On Thu, Nov 30, 2017 at 03:30:07PM +0100, Andrzej Hajda wrote:
>>>> HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
>>>> show that hardware is usually able to recover from this state without
>>>> interrupting the transfer. On the other side enforcing transfer repetition
>>>> in such case does not help in many situations, especially on busy systems
>>>> and causes -EAGAIN and -ETIMEOUT errors. Moreover documentation says that
>>>> such state can be caused by slave clock stretching, and should not be treated
>>>> as an error.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> Can this be applied independently of my comments to patch 2?
>>>
>> Yes, please apply it alone. I will continue work on patch 2.
> I just thought it might be nice to have a comment where you removed the
> code summarizing your findings. So we will remember about this in the
> future. Makes sense?
>
Forgive me delayed response - holiday.

The code removed was something extra, so I am not sure if it is
necessary to add comment to the code, git log should be enough.

Anyway I will post patches dealing with HSI2C_MASTER_ST_LOSE on
transaction start very soon, so I can add relevant comment there.


Regards

Andrzej

Re: [3/3] i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

[Wolfram Sang]

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

Hi Andrzej,

> Forgive me delayed response - holiday.

No worries!

> The code removed was something extra, so I am not sure if it is
> necessary to add comment to the code, git log should be enough.

If somebody later discovers this "new bit" and implements support for
it, I am not sure this person will check git log to see if there has
ever been support added and later removed. Also, it helps me reviewing
when gory details are explicitly mentioned in the code.

> Anyway I will post patches dealing with HSI2C_MASTER_ST_LOSE on
> transaction start very soon, so I can add relevant comment there.

Please do.

Kind regards,

   Wolfram


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

Re: [PATCH 0/3] i2c: exynos5: bus recovery implementation

[Andi Shyti]

Hi Andrzej,

> This patchset adds bus recovery functionality to Exynos HSI2C, it is used in
> case of transmission timeouts.
> Internal tests shows that it significantly improves I2C transmissions.
> 
> Andi, could you take a look at it/test, feedback welcome.
> 
> Later, I will post also patches to MHL chip which uses this functionality.
> MHL chip has known bug of being in incorrect state after hard reset,
> I2C bus recovery cures it.
> 
> Regards
> Andrzej
> 
> 
> Andrzej Hajda (3):
>   i2c: exynos5: change internal transmission timeout to 100ms
>   i2c: exynos5: implement bus recovery functionality
>   i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

Thanks for the patches, they look good and I tested them.

You can add for all three

Reviewed-by: Andi Shyti <andi.shyti@samsung.com>
Tested-by: Andi Shyti <andi.shyti@samsung.com>

Thanks,
Andi