Skip to contentSkip to conversation

i2c: designware: Fix failure on baytrail

PATCH In review Ben Gardner 2 participants 6 messages Feb 8, 2018 – Feb 14, 2018

Subsystems (3)
I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver
lore
Share & tools
lore

Ben Gardner

author
#0 lore raw

PATCH 1/1 i2c: designware: Fix failure on baytrail

The I2C driver for my Atom E3845 board has been broken since 4.9.

These kernel logs show up whenever am I2C transaction is attempted. i2c-designware-pci 0000:00:18.3: timeout in disabling adapter i2c-designware-pci 0000:00:18.3: timeout waiting for bus ready

The root issue is that the I2C port takes a while to enable and somewhere along the way, the 'enable-and-wait' approach to enabling the adapter was changed to 'enable'.
That caused the driver and hardware to get out of sync and fail.

I have tested this patch on 4.14 and 4.15.

Signed-off-by Ben Gardner <[email protected]>
---
 drivers/i2c/busses/i2c-designware-master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-master.c +1 -1
index ae69188..55926ef 100644
@@ -209,7 +209,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	i2c_dw_disable_int(dev);
 
 	/* Enable the adapter */
-	__i2c_dw_enable(dev, true);
+	__i2c_dw_enable_and_wait(dev, true);
 
 	/* Clear and enable interrupts */
 	dw_readl(dev, DW_IC_CLR_INTR);
Signature · 1 line
2.7.4
#1 lore raw

Hi

Quoted 23 lines · Ben Gardner

The I2C driver for my Atom E3845 board has been broken since 4.9.

These kernel logs show up whenever am I2C transaction is attempted. i2c-designware-pci 0000:00:18.3: timeout in disabling adapter i2c-designware-pci 0000:00:18.3: timeout waiting for bus ready

The root issue is that the I2C port takes a while to enable and somewhere along the way, the 'enable-and-wait' approach to enabling the adapter was changed to 'enable'.
That caused the driver and hardware to get out of sync and fail.

I have tested this patch on 4.14 and 4.15.

Signed-off-by Ben Gardner <[email protected]>
---
  drivers/i2c/busses/i2c-designware-master.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-master.c +0 -0
index ae69188..55926ef 100644

drivers/i2c/busses/i2c-designware-master.c static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) · line 215

Quoted 8 lines
drivers/i2c/busses/i2c-designware-master.c +1 -1
@@ -209,7 +209,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
  	i2c_dw_disable_int(dev);
 
  	/* Enable the adapter */
-	__i2c_dw_enable(dev, true);
+	__i2c_dw_enable_and_wait(dev, true);
 
  	/* Clear and enable interrupts */
  	dw_readl(dev, DW_IC_CLR_INTR);

It seems commit 2702ea7dbec5 ("i2c: designware: wait for disable/enable only if necessary") is most likely reason for regression. Are you able to test some version between v4.9 and v4.12 and revert that commit does it fix the issue for you? Can you also test your fix on the same kernel version but apply to drivers/i2c/busses/i2c-designware-core.c?

i2c-designware-master.c was renamed from i2c-designware-core.c in v4.13 and thus we need to have the separate fix for kernels v4.9-v4.12.

Signature · 1 line
Jarkko
author
#2 lore raw

Hi Jarkko,

I'm out of the office until Monday and I don't have access to my notes, so this is from memory.

My board uses 3 of the i2c buses. Two have only 1 device, the problematic one has two devices.
All devices are declared via ACPI, so they all load their drivers right away. The first i2c transaction always worked. I didn't see any issues with the two i2c buses that only have 1 device.
The second i2c transaction, which immediately followed, would fail. The bus no longer worked after that.

I bisected the kernel to try to find where this broke, but the answer I kept on getting didn't make any sense.
I think t was this commit:

4d6d5f1d08d2138dc43b28966eb6200e3db2e623 i2c: designware: fix rx fifo depth tracking

Of course, reverting that one commit didn't fix anything. So I added a log to the dw_readl() and dw_writel() functions in both a working and broken kernel and compared.

In the working kernel, the enable sequence wrote 1 to enable, read back 0, write 1 again, read back 1.

The non-working kernel just wrote the 1 to then enable register and then went on.
I assume it ignored some subsequent register writes and ended up with a broken bus.
I'd see weird stuff like an abort interrupt. I do find it odd that the bus never recovered.

I suspect that if there was a delay between the two transactions, it would not have failed.

Quoted 29 lines
Quoted 29 lines

The I2C driver for my Atom E3845 board has been broken since 4.9.

These kernel logs show up whenever am I2C transaction is attempted. i2c-designware-pci 0000:00:18.3: timeout in disabling adapter i2c-designware-pci 0000:00:18.3: timeout waiting for bus ready

The root issue is that the I2C port takes a while to enable and somewhere along the way, the 'enable-and-wait' approach to enabling the adapter was changed to 'enable'.
That caused the driver and hardware to get out of sync and fail.

I have tested this patch on 4.14 and 4.15.

Signed-off-by Ben Gardner <[email protected]>
---
  drivers/i2c/busses/i2c-designware-master.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-master.c +1 -1
index ae69188..55926ef 100644
@@ -209,7 +209,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
        i2c_dw_disable_int(dev);
        /* Enable the adapter */
-       __i2c_dw_enable(dev, true);
+       __i2c_dw_enable_and_wait(dev, true);
        /* Clear and enable interrupts */
Quoted 8 lines

drivers/i2c/busses/i2c-designware-master.c static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) · line 215

Quoted 1 line
drivers/i2c/busses/i2c-designware-master.c +0 -0
        dw_readl(dev, DW_IC_CLR_INTR);

It seems commit 2702ea7dbec5 ("i2c: designware: wait for disable/enable only if necessary") is most likely reason for regression. Are you able to test some version between v4.9 and v4.12 and revert that commit does it fix the issue for you? Can you also test your fix on the same kernel version but apply to drivers/i2c/busses/i2c-designware-core.c?

I will test a similar change on 4.9 on Monday.

Quoted 2 lines

i2c-designware-master.c was renamed from i2c-designware-core.c in v4.13 and thus we need to have the separate fix for kernels v4.9-v4.12.

I will also create a similar patch for v4.9.

Ben

#3 lore raw

Hi

Quoted 10 lines · Ben Gardner

I bisected the kernel to try to find where this broke, but the answer I kept on getting didn't make any sense.
I think t was this commit:

4d6d5f1d08d2138dc43b28966eb6200e3db2e623 i2c: designware: fix rx fifo depth tracking

Quoted 1 line

Of course, reverting that one commit didn't fix anything.

So I added a log to the dw_readl() and dw_writel() functions in both a working and broken kernel and compared.

Yeah, it's not unusual that bisect diverts into wrong commit especially with issues that don't reproduce easily or if some unrelated thing is causing also failure at certain step leading to a wrong good/bad guess.

Can you test does reverting my guessed commit 2702ea7dbec5 ("i2c: designware: wait for disable/enable only if necessary") fix it?

For linux-stable it is good info to know exactly the commit causing regression and mark that in your changelog. It allows linux-stable folks to apply fix to earlier kernels and know versions this fix will still apply if cannot apply where regression was introduced. Fox example:

Fixes commit 2702ea7dbec5 ("i2c: designware: wait for disable/enable only if necessary")
Cc linux-stable <[email protected]> # 4.13+
Signature · 1 line
Jarkko
author
#4 lore raw
Quoted 2 lines

Can you test does reverting my guessed commit 2702ea7dbec5 ("i2c:
designware: wait for disable/enable only if necessary") fix it?

I verified that reverting 2702ea7dbec5 ("i2c: designware: wait for disable/enable only if necessary") also fixes the issue. Not waiting when disabling the adapter seems perfectly fine. Not waiting when enabling is the problem.

Quoted 8 lines

For linux-stable it is good info to know exactly the commit causing regression and mark that in your changelog. It allows linux-stable folks to apply fix to earlier kernels and know versions this fix will still apply if cannot apply where regression was introduced. Fox example:

Fixes commit 2702ea7dbec5 ("i2c: designware: wait for disable/enable only

if necessary")
Cc: linux-stable <[email protected]> # 4.13+

Do you want me to resubmit this patch with the above lines (Fixes and Cc) added or are you willing to add the appropriate lines and take care of it?
I suppose the commit message could use some rewording, now that the issue seems a bit clearer.

Thanks,
Ben

#5 lore raw

+ José

Quoted 8 lines · Ben Gardner
Quoted 2 lines

Can you test does reverting my guessed commit 2702ea7dbec5 ("i2c:
designware: wait for disable/enable only if necessary") fix it?

I verified that reverting 2702ea7dbec5 ("i2c: designware: wait for disable/enable only if necessary") also fixes the issue. Not waiting when disabling the adapter seems perfectly fine. Not waiting when enabling is the problem.

José: There is a regression with above commit and Ben has found a fix that brings back waiting when enabling the adapter in transfer start:

drivers/i2c/busses/i2c-designware-master.c +1 -1
@@ -209,7 +209,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	i2c_dw_disable_int(dev);
 
 	/* Enable the adapter */
-	__i2c_dw_enable(dev, true);
+	__i2c_dw_enable_and_wait(dev, true);

I guess this was just forgotten conversion rather than intentional? At least commit log is only about skipping waiting at the end of transaction.

I'd like to avoid full revert first because of Ben's fix I guess still keeps the benefit of commit 2702ea7dbec5 and second because of code changes revert is also a little bit more labor.

Quoted 9 lines
Quoted 8 lines

For linux-stable it is good info to know exactly the commit causing regression and mark that in your changelog. It allows linux-stable folks to apply fix to earlier kernels and know versions this fix will still apply if cannot apply where regression was introduced. Fox example:

Fixes commit 2702ea7dbec5 ("i2c: designware: wait for disable/enable only

if necessary")
Cc: linux-stable <[email protected]> # 4.13+

Quoted 6 lines

Do you want me to resubmit this patch with the above lines (Fixes and Cc) added or are you willing to add the appropriate lines and take care of it?
I suppose the commit message could use some rewording, now that the issue seems a bit clearer.

Yes please. It always good idea to make busy maintainers' life a bit more easier :-)

Signature · 1 line
Jarkko