| Message ID | 20200917122029.11121-2-ceggers@arri.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] i2c: imx: Fix reset of I2SR_IAL flag | expand |
Hello, On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote: > According to the "VFxxx Controller Reference Manual" (and the comment > block starting at line 97), Vybrid requires writing a one for clearing > an interrupt flag. Syncing with the method for clearing I2SR_IIF in > i2c_imx_isr(). > > Signed-off-by: Christian Eggers <ceggers@arri.de> > Cc: stable@vger.kernel.org > --- > drivers/i2c/busses/i2c-imx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 0ab5381aa012..d8b2e632dd10 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -425,6 +425,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a > /* check for arbitration lost */ > if (temp & I2SR_IAL) { > temp &= ~I2SR_IAL; > + temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL); > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR); > return -EAGAIN; This looks strange. First the flag is cleared and then it is (in some cases) set again. If I2SR_IIF is set in temp you ack this irq without handling it. (Which might happen if atomic is set and irqs are off?!) I see this idiom is used in a few more places in the driver already, I didn't check but these might have the same problem maybe? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Uwe, On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote: > Hello, > > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote: > ... > > /* check for arbitration lost */ > > if (temp & I2SR_IAL) { > > temp &= ~I2SR_IAL; > > + temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL); > > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR); > > return -EAGAIN; > ... > This looks strange. First the flag is cleared and then it is (in some > cases) set again. i.MX controllers require writing a 0 to clear these bits. Vybrid controllers need writing a 1 for the same. > If I2SR_IIF is set in temp you ack this irq without handling it. (Which > might happen if atomic is set and irqs are off?!) This patch is only about using the correct processor specific value for acknowledging an IRQ... But I think that returning EAGAIN (which aborts the transfer) should be handling enough. At the next transfer, the controller will be set back to master mode. > I see this idiom is used in a few more places in the driver already, I > didn't check but these might have the same problem maybe? Best regards Christian
On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote: > Hello Uwe, > > On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote: > > Hello, > > > > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote: > > ... > > > /* check for arbitration lost */ > > > if (temp & I2SR_IAL) { > > > temp &= ~I2SR_IAL; > > > + temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL); > > > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR); > > > return -EAGAIN; > > ... > > > This looks strange. First the flag is cleared and then it is (in some > > cases) set again. > i.MX controllers require writing a 0 to clear these bits. Vybrid controllers > need writing a 1 for the same. Yes, I understood that. > > If I2SR_IIF is set in temp you ack this irq without handling it. (Which > > might happen if atomic is set and irqs are off?!) > This patch is only about using the correct processor specific value for > acknowledging an IRQ... But I think that returning EAGAIN (which aborts the > transfer) should be handling enough. At the next transfer, the controller will > be set back to master mode. Either you didn't understand what I meant, or I don't understand why you consider your patch right anyhow. So I try to explain in other and more words. IMHO the intention here (and also what happens on i.MX) is that exactly the AL interrupt pending bit should be cleared and the IF irq is supposed to be untouched. Given there are only two irq flags in the I2SR register (which is called IBSR on Vybrid) the status quo (i.e. without your patch) is: On i.MX IAL is cleared On Vybrid IIF (which is called IBIF there) is cleared. With your patch we get: On i.MX IAL is cleared On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared. To get it right for both SoC types you have to do (e.g.): temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL; (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL because there currently IAL might be cleared by mistake on Vybrid). I considered creating a patch, but as I don't have a Vybrid on my desk and on i.MX there is no change, I let you do this. Best regards Uwe
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 0ab5381aa012..d8b2e632dd10 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -425,6 +425,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a /* check for arbitration lost */ if (temp & I2SR_IAL) { temp &= ~I2SR_IAL; + temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL); imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR); return -EAGAIN; }
According to the "VFxxx Controller Reference Manual" (and the comment block starting at line 97), Vybrid requires writing a one for clearing an interrupt flag. Syncing with the method for clearing I2SR_IIF in i2c_imx_isr(). Signed-off-by: Christian Eggers <ceggers@arri.de> Cc: stable@vger.kernel.org --- drivers/i2c/busses/i2c-imx.c | 1 + 1 file changed, 1 insertion(+)