[PATCH] i2c: designware: Fix failure on baytrail

[PATCH] i2c: designware: Fix failure on baytrail

[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 <gardner.ben@gmail.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index ae69188..55926ef 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -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);
-- 
2.7.4

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Jarkko Nikula]

Hi

On 02/08/2018 08:12 PM, Ben Gardner wrote:
> 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 <gardner.ben@gmail.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index ae69188..55926ef 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -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.

-- 
Jarkko

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Ben Gardner]

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.

>> 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 <gardner.ben@gmail.com>
>> ---
>>   drivers/i2c/busses/i2c-designware-master.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-master.c
>> b/drivers/i2c/busses/i2c-designware-master.c
>> index ae69188..55926ef 100644
>> --- a/drivers/i2c/busses/i2c-designware-master.c
>> +++ b/drivers/i2c/busses/i2c-designware-master.c
>> @@ -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?

I will test a similar change on 4.9 on Monday.

> 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

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Jarkko Nikula]

Hi

On 02/09/2018 05:07 PM, Ben Gardner wrote:
> 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.
> 
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 <stable@vger.kernel.org> # 4.13+

-- 
Jarkko

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Ben Gardner]

> 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.

> 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 <stable@vger.kernel.org> # 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

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Jarkko Nikula]

+ José

On 02/13/2018 06:31 PM, Ben Gardner wrote:
>> 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:

--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -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.

>> 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 <stable@vger.kernel.org> # 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.
> 
Yes please. It always good idea to make busy maintainers' life a bit 
more easier :-)

-- 
Jarkko

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Jarkko Nikula]

On 02/12/2018 05:56 PM, Ben Gardner wrote:
> I didn't make it clear in the subject line, but this patch is intended
> for the 4.9 kernel.
> The earlier patch (Feb 9, 2018) was for 4.14, 4.15 and future.
> 
I think we need to handle this patch once earlier patch is applied by 
following the Option 3 rule in 
Documentation/process/stable-kernel-rules.rst.

-- 
Jarkko

Re: [PATCH] i2c: designware: Fix failure on baytrail

[Ben Gardner]

I didn't make it clear in the subject line, but this patch is intended
for the 4.9 kernel.
The earlier patch (Feb 9, 2018) was for 4.14, 4.15 and future.

On Mon, Feb 12, 2018 at 9:52 AM, Ben Gardner <gardner.ben@gmail.com> wrote:
> The I2C driver for my Atom E3845 board has been broken since 4.9.
> My board has two I2C devices on a bus that are declared via ACPI.
> At startup, both drivers attempt to access the devices at the same time,
> resulting in two back-to-back I2C transactions initiated by the kernel.
> The second transaction fails.
>
> 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.
>
> These kernel logs show up whenever an I2C transaction is attempted after
> this failure.
> i2c-designware-pci 0000:00:18.3: timeout in disabling adapter
> i2c-designware-pci 0000:00:18.3: timeout waiting for bus ready
>
> The driver is unable to recover the bus at this point, but that is a
> separate issue.
>
> This patch has been tested on 4.9.
>
> Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index b403fa5..980e5a1 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -509,7 +509,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);
> --
> 2.7.4
>

[PATCH] i2c: designware: Fix failure on baytrail

[Ben Gardner]

The I2C driver for my Atom E3845 board has been broken since 4.9.
My board has two I2C devices on a bus that are declared via ACPI.
At startup, both drivers attempt to access the devices at the same time,
resulting in two back-to-back I2C transactions initiated by the kernel.
The second transaction fails.

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.

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

The driver is unable to recover the bus at this point, but that is a
separate issue.

This patch has been tested on 4.9.

Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index b403fa5..980e5a1 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -509,7 +509,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);
-- 
2.7.4