| Message ID | 20210924111528.2924251-1-megous@megous.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RESEND] i2c: rk3x: Handle a spurious start completion interrupt flag | expand |
On Fri, Sep 24, 2021 at 01:15:27PM +0200, Ondrej Jirman wrote: > In a typical read transfer, start completion flag is being set after > read finishes (notice ipd bit 4 being set): > > trasnfer poll=0 > i2c start > rk3x-i2c fdd40000.i2c: IRQ: state 1, ipd: 10 > i2c read > rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 1b > i2c stop > rk3x-i2c fdd40000.i2c: IRQ: state 4, ipd: 33 > > This causes I2C transfer being aborted in polled mode from a stop completion > handler: > > trasnfer poll=1 > i2c start > rk3x-i2c fdd40000.i2c: IRQ: state 1, ipd: 10 > i2c read > rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 0 > rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 1b > i2c stop > rk3x-i2c fdd40000.i2c: IRQ: state 4, ipd: 13 > i2c stop > rk3x-i2c fdd40000.i2c: unexpected irq in STOP: 0x10 > > Clearing the START flag after read fixes the issue without any obvious > side effects. > > This issue was dicovered on RK3566 when adding support for powering > off the RK817 PMIC. > > Signed-off-by: Ondrej Jirman <megous@megous.com> > --- I haven't seen the issue described here, so I can't test whether this fix works, but the explanation makes sense, so: Reviewed-by: John Keeping <john@metanate.com> > drivers/i2c/busses/i2c-rk3x.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c > index 819ab4ee517e..02ddb237f69a 100644 > --- a/drivers/i2c/busses/i2c-rk3x.c > +++ b/drivers/i2c/busses/i2c-rk3x.c > @@ -423,8 +423,8 @@ static void rk3x_i2c_handle_read(struct rk3x_i2c *i2c, unsigned int ipd) > if (!(ipd & REG_INT_MBRF)) > return; > > - /* ack interrupt */ > - i2c_writel(i2c, REG_INT_MBRF, REG_IPD); > + /* ack interrupt (read also produces a spurious START flag, clear it too) */ > + i2c_writel(i2c, REG_INT_MBRF | REG_INT_START, REG_IPD); > > /* Can only handle a maximum of 32 bytes at a time */ > if (len > 32)
> This causes I2C transfer being aborted in polled mode from a stop completion > handler: I wonder why this only happens in polling mode? The question behind that is: is it really a spurious irq from the HW or is it maybe a race in the driver? Because polling uses the same interrupt handler but just periodically polls it.
On Mon, Nov 29, 2021 at 10:37:43AM +0100, Wolfram Sang wrote: > > > This causes I2C transfer being aborted in polled mode from a stop completion > > handler: > > I wonder why this only happens in polling mode? The question behind that > is: is it really a spurious irq from the HW or is it maybe a race in the > driver? Because polling uses the same interrupt handler but just > periodically polls it. Spurious interupt happens in both modes. Abort only happens in polled mode, becase polling executes the irq handler before the stop condition interrupt fires. In that case rk3x_i2c_irq will execute rk3x_i2c_handle_stop, which will terminate the transfer, becasuse it's executed without STOP completion interrupt flag being set. In interrupt mode, the interrupt handler alwyas executes after the stop condition, so the spurious START interrupt is ignored in this case. kind regards, o.
> Spurious interupt happens in both modes.
Thanks for the detailed explanation!
On Fri, Sep 24, 2021 at 01:15:27PM +0200, Ondrej Jirman wrote: > In a typical read transfer, start completion flag is being set after > read finishes (notice ipd bit 4 being set): > > trasnfer poll=0 > i2c start > rk3x-i2c fdd40000.i2c: IRQ: state 1, ipd: 10 > i2c read > rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 1b > i2c stop > rk3x-i2c fdd40000.i2c: IRQ: state 4, ipd: 33 > > This causes I2C transfer being aborted in polled mode from a stop completion > handler: > > trasnfer poll=1 > i2c start > rk3x-i2c fdd40000.i2c: IRQ: state 1, ipd: 10 > i2c read > rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 0 > rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 1b > i2c stop > rk3x-i2c fdd40000.i2c: IRQ: state 4, ipd: 13 > i2c stop > rk3x-i2c fdd40000.i2c: unexpected irq in STOP: 0x10 > > Clearing the START flag after read fixes the issue without any obvious > side effects. > > This issue was dicovered on RK3566 when adding support for powering > off the RK817 PMIC. > > Signed-off-by: Ondrej Jirman <megous@megous.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 819ab4ee517e..02ddb237f69a 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -423,8 +423,8 @@ static void rk3x_i2c_handle_read(struct rk3x_i2c *i2c, unsigned int ipd) if (!(ipd & REG_INT_MBRF)) return; - /* ack interrupt */ - i2c_writel(i2c, REG_INT_MBRF, REG_IPD); + /* ack interrupt (read also produces a spurious START flag, clear it too) */ + i2c_writel(i2c, REG_INT_MBRF | REG_INT_START, REG_IPD); /* Can only handle a maximum of 32 bytes at a time */ if (len > 32)
In a typical read transfer, start completion flag is being set after read finishes (notice ipd bit 4 being set): trasnfer poll=0 i2c start rk3x-i2c fdd40000.i2c: IRQ: state 1, ipd: 10 i2c read rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 1b i2c stop rk3x-i2c fdd40000.i2c: IRQ: state 4, ipd: 33 This causes I2C transfer being aborted in polled mode from a stop completion handler: trasnfer poll=1 i2c start rk3x-i2c fdd40000.i2c: IRQ: state 1, ipd: 10 i2c read rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 0 rk3x-i2c fdd40000.i2c: IRQ: state 2, ipd: 1b i2c stop rk3x-i2c fdd40000.i2c: IRQ: state 4, ipd: 13 i2c stop rk3x-i2c fdd40000.i2c: unexpected irq in STOP: 0x10 Clearing the START flag after read fixes the issue without any obvious side effects. This issue was dicovered on RK3566 when adding support for powering off the RK817 PMIC. Signed-off-by: Ondrej Jirman <megous@megous.com> --- Re-sending, because originally, maintainers script did not pick up rockchip mailing list, to send this patch to. drivers/i2c/busses/i2c-rk3x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)