commit 6d9939f651419a63e091105663821f9c7d3fec37 (i2c: omap: split out [XR]DR
and [XR]RDY) changed the way how errata i207 (I2C: RDR Flag May Be Incorrectly
Set) get handled. 6d9939f6514 code doesn't correspond to workaround provided by
errata.
According to errata ISR must filter out spurious RDR before data read not after.
ISR must read RXSTAT to get number of bytes available to read. Because RDR
could be set while there could no data in the receive FIFO.
Restored pre 6d9939f6514 way of handling errata.
Found by code review. Real impact haven't seen.
Tested on Beagleboard XM C.
Signed-off-by: Alexander Kochetkov <[email protected]>
Fixes: 6d9939f651419a63e09110 i2c: omap: split out [XR]DR and [XR]RDY
---
drivers/i2c/busses/i2c-omap.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 90dcc2e..e7cbcb0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -958,11 +958,13 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
if (dev->fifo_size)
num_bytes = dev->buf_len;
- omap_i2c_receive_data(dev, num_bytes, true);
-
- if (dev->errata & I2C_OMAP_ERRATA_I207)
+ if (dev->errata & I2C_OMAP_ERRATA_I207) {
i2c_omap_errata_i207(dev, stat);
+ num_bytes = (omap_i2c_read_reg(dev,
+ OMAP_I2C_BUFSTAT_REG) >> 8) & 0x3F;
+ }
+ omap_i2c_receive_data(dev, num_bytes, true);
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
continue;
}
--
1.7.9.5
21 ????. 2014 ?., ? 3:16, Alexander Kochetkov <[email protected]> ???????(?):
> commit 6d9939f651419a63e091105663821f9c7d3fec37 (i2c: omap: split out [XR]DR
> and [XR]RDY) changed the way how errata i207 (I2C: RDR Flag May Be Incorrectly
> Set) get handled.
It's is seen if open 6d9939f651419a63e09^ and 6d9939f651419a63e09 side by side:
6d9939f651419a63e09^:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-omap.c?id=540a4790f7da0d3ca5ad9d726f198a5eb4db05ec#n800
6d9939f651419a63e09:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-omap.c?id=6d9939f651419a63e091105663821f9c7d3fec37#n800
Errata "2C: RDR Flag May Be Incorrectly Set"
http://www.ti.com/lit/er/sprz319f/sprz319f.pdf
21 ????. 2014 ?., ? 3:16, Alexander Kochetkov <[email protected]> ???????(?):
> Found by code review. Real impact haven't seen.
> Tested on Beagleboard XM C.
Does anybody know the "certain rare conditions" when RDR errata appears?
I tested without luck (Beagleboard XM C).
On Fri, Nov 21, 2014 at 04:16:51AM +0400, Alexander Kochetkov wrote:
> commit 6d9939f651419a63e091105663821f9c7d3fec37 (i2c: omap: split out [XR]DR
> and [XR]RDY) changed the way how errata i207 (I2C: RDR Flag May Be Incorrectly
> Set) get handled. 6d9939f6514 code doesn't correspond to workaround provided by
> errata.
>
> According to errata ISR must filter out spurious RDR before data read not after.
> ISR must read RXSTAT to get number of bytes available to read. Because RDR
> could be set while there could no data in the receive FIFO.
>
> Restored pre 6d9939f6514 way of handling errata.
>
> Found by code review. Real impact haven't seen.
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov <[email protected]>
> Fixes: 6d9939f651419a63e09110 i2c: omap: split out [XR]DR and [XR]RDY
Tested on BBB and AM437x Starter Kit
Tested-by: Felipe Balbi <[email protected]>
Reviewed-by: Felipe Balbi <[email protected]>
> ---
> drivers/i2c/busses/i2c-omap.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 90dcc2e..e7cbcb0 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -958,11 +958,13 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
> if (dev->fifo_size)
> num_bytes = dev->buf_len;
>
> - omap_i2c_receive_data(dev, num_bytes, true);
> -
> - if (dev->errata & I2C_OMAP_ERRATA_I207)
> + if (dev->errata & I2C_OMAP_ERRATA_I207) {
> i2c_omap_errata_i207(dev, stat);
> + num_bytes = (omap_i2c_read_reg(dev,
> + OMAP_I2C_BUFSTAT_REG) >> 8) & 0x3F;
> + }
>
> + omap_i2c_receive_data(dev, num_bytes, true);
> omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> continue;
> }
> --
> 1.7.9.5
>
--
balbi
21 ????. 2014 ?., ? 19:08, Felipe Balbi <[email protected]> ???????(?):
> Tested on BBB and AM437x Starter Kit
>
> Tested-by: Felipe Balbi <[email protected]>
> Reviewed-by: Felipe Balbi <[email protected]>
21 ????. 2014 ?., ? 0:10, Aaro Koskinen <[email protected]> ???????(?):
> I could not see any breakage or anything wrong on OMAP2 & OMAP3.
> On OMAP1 I don't have anything on the OMAP I2C bus, so cannot really
> test anything there.
>
> Tested-by: Aaro Koskinen <[email protected]>
21 ????. 2014 ?., ? 21:11, Wolfram Sang <[email protected]> ???????(?):
> The errno for AL is -EAGAIN. Curly braces are not needed.
Guys, I really appreciate you help.
So much testing and review.
I could not have done one.
Thank you!
Alexander.
21 ????. 2014 ?., ? 3:29, Alexander Kochetkov <[email protected]> ???????(?):
>>
>> Found by code review. Real impact haven't seen.
>> Tested on Beagleboard XM C.
>
> Does anybody know the "certain rare conditions" when RDR errata appears?
> I tested without luck (Beagleboard XM C).
Spent half a day trying to catch the errata without luck.
Tried to simulate noise on the bus in hope it may happen.
Tried to run with OMAP_I2C_FLAG_NO_FIFO flag.
What a mystery errata. Hiding.
Anyway, thanks!
Have a nice weekend!
On Fri, Nov 21, 2014 at 04:16:51AM +0400, Alexander Kochetkov wrote:
> commit 6d9939f651419a63e091105663821f9c7d3fec37 (i2c: omap: split out [XR]DR
> and [XR]RDY) changed the way how errata i207 (I2C: RDR Flag May Be Incorrectly
> Set) get handled. 6d9939f6514 code doesn't correspond to workaround provided by
> errata.
>
> According to errata ISR must filter out spurious RDR before data read not after.
> ISR must read RXSTAT to get number of bytes available to read. Because RDR
> could be set while there could no data in the receive FIFO.
>
> Restored pre 6d9939f6514 way of handling errata.
>
> Found by code review. Real impact haven't seen.
> Tested on Beagleboard XM C.
>
> Signed-off-by: Alexander Kochetkov <[email protected]>
> Fixes: 6d9939f651419a63e09110 i2c: omap: split out [XR]DR and [XR]RDY
Applied to for-current, thanks!
> Guys, I really appreciate you help.
> So much testing and review.
> I could not have done one.
> Thank you!
Well, thank YOU, for your hard work in verifying this controller at
low-level and in corner-cases. This is very much appreciated. Plus, you
have been very responsive to comments. Really good work! Hope you'll be
around for more :)