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

Message ID 20171130143007.30258-4-a.hajda@samsung.com
State Changes Requested
Headers show
Series
  • i2c: exynos5: bus recovery implementation
Related show

Commit Message

Andrzej Hajda Nov. 30, 2017, 2:30 p.m.
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(-)

Comments

Wolfram Sang Jan. 15, 2018, 8:53 p.m. | #1
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?
Andrzej Hajda Jan. 16, 2018, 9:40 a.m. | #2
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
Wolfram Sang Jan. 17, 2018, 11:23 p.m. | #3
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?
Andrzej Hajda Jan. 26, 2018, 7:17 a.m. | #4
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
Wolfram Sang Jan. 26, 2018, 9:30 a.m. | #5
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

Patch

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) {