i2c: designware: Fix failure on baytrail

Message ID 1518113569-19991-1-git-send-email-gardner.ben@gmail.com
State Superseded
Headers show
Series
  • i2c: designware: Fix failure on baytrail
Related show

Commit Message

Ben Gardner Feb. 8, 2018, 6:12 p.m.
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(-)

Comments

Jarkko Nikula Feb. 9, 2018, 9:25 a.m. | #1
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.
Ben Gardner Feb. 9, 2018, 3:07 p.m. | #2
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
Jarkko Nikula Feb. 13, 2018, 2:35 p.m. | #3
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+
Ben Gardner Feb. 13, 2018, 4:31 p.m. | #4
> 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
Jarkko Nikula Feb. 14, 2018, 3:06 p.m. | #5
+ 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 :-)
Souza, Jose Feb. 21, 2018, 1:11 a.m. | #6
On Wed, 2018-02-14 at 17:06 +0200, Jarkko Nikula wrote:
> + 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.


It was intentional as the databook describe the procedure(read
IC_ENABLE_STATUS) to disable but there is no mention of something for
enable however I agree that wait makes sense when enabling too.

> 

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

>

Patch

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