On 15:04-20250228, Andreas Kemnade wrote:
> On the GTA04A5 writing a reset command to the gyroscope causes IRQ
> storms because NACK IRQs are enabled and therefore triggered but not
> acked.
>
> Sending a reset command to the gyroscope by
> i2cset 1 0x69 0x14 0xb6
> with an additional debug print in the ISR (not the thread) itself
> causes
>
> [ 363.353515] i2c i2c-1: ioctl, cmd=0x720, arg=0xbe801b00
> [ 363.359039] omap_i2c 48072000.i2c: addr: 0x0069, len: 2, flags: 0x0, stop: 1
> [ 363.366180] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x1110)
> [ 363.371673] omap_i2c 48072000.i2c: IRQ (ISR = 0x0010)
> [ 363.376892] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x0102)
> [ 363.382263] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x0102)
> [ 363.387664] omap_i2c 48072000.i2c: IRQ LL (ISR = 0x0102)
> repeating till infinity
> [...]
> (0x2 = NACK, 0x100 = Bus free, which is not enabled)
> Apparently no other IRQ bit gets set, so this stalls.
>
> Do not ignore enabled interrupts and make sure they are acked.
> If the NACK IRQ is not needed, it should simply not enabled, but
> according to the above log, caring about it is necessary unless
> the Bus free IRQ is enabled and handled. The assumption that is
> will always come with a ARDY IRQ, which was the idea behind
> ignoring it, proves wrong.
> It is true for simple reads from an unused address.
>
> To still avoid the i2cdetect trouble which is the reason for
> commit c770657bd261 ("i2c: omap: Fix standard mode false ACK readings"),
> avoid doing much about NACK in omap_i2c_xfer_data() which is used
> by both IRQ mode and polling mode, so also the false detection fix
> is extended to polling usage and IRQ storms are avoided.
>
> By changing this, the hardirq handler is not needed anymore to filter
> stuff.
>
> The mentioned gyro reset now just causes a -ETIMEDOUT instead of
> hanging the system.
>
> Fixes: c770657bd261 ("i2c: omap: Fix standard mode false ACK readings").
> CC: <stable@xxxxxxxxxx>
> Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
> ---
> This needs at least to be tested on systems where false acks were
> detected.
At least on BeaglePlay, I have not been able to reproduce the original
bug which was the trigger for commit c770657bd261
I also ran basic boot tests on other K3 platforms and none seem to show
regressions at the very least.
Tested-by: Nishanth Menon <nm@xxxxxx>
>
> drivers/i2c/busses/i2c-omap.c | 26 +++++++-------------------
> 1 file changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 92faf03d64cf..f18c3e74b076 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1048,23 +1048,6 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *omap, u8 num_bytes,
> return 0;
> }
>
> -static irqreturn_t
> -omap_i2c_isr(int irq, void *dev_id)
> -{
> - struct omap_i2c_dev *omap = dev_id;
> - irqreturn_t ret = IRQ_HANDLED;
> - u16 mask;
> - u16 stat;
> -
> - stat = omap_i2c_read_reg(omap, OMAP_I2C_STAT_REG);
> - mask = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG) & ~OMAP_I2C_STAT_NACK;
> -
> - if (stat & mask)
> - ret = IRQ_WAKE_THREAD;
> -
> - return ret;
> -}
> -
> static int omap_i2c_xfer_data(struct omap_i2c_dev *omap)
> {
> u16 bits;
> @@ -1095,8 +1078,13 @@ static int omap_i2c_xfer_data(struct omap_i2c_dev *omap)
> }
>
> if (stat & OMAP_I2C_STAT_NACK) {
> - err |= OMAP_I2C_STAT_NACK;
> + omap->cmd_err |= OMAP_I2C_STAT_NACK;
> omap_i2c_ack_stat(omap, OMAP_I2C_STAT_NACK);
> +
> + if (!(stat & ~OMAP_I2C_STAT_NACK)) {
> + err = -EAGAIN;
> + break;
> + }
> }
>
> if (stat & OMAP_I2C_STAT_AL) {
> @@ -1472,7 +1460,7 @@ omap_i2c_probe(struct platform_device *pdev)
> IRQF_NO_SUSPEND, pdev->name, omap);
> else
> r = devm_request_threaded_irq(&pdev->dev, omap->irq,
> - omap_i2c_isr, omap_i2c_isr_thread,
> + NULL, omap_i2c_isr_thread,
> IRQF_NO_SUSPEND | IRQF_ONESHOT,
> pdev->name, omap);
>
> --
> 2.39.5
>
>
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
