| Message ID | 1518113569-19991-1-git-send-email-gardner.ben@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: designware: Fix failure on baytrail | expand |
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.
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
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+
> 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
+ 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 :-)
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 :-) >
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);
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(-)