Re: [PATCH v2 1/4] i2c: imx: only poll for bus busy in multi master mode

Hi Fabio, Oleksij,

On Wed, Aug 21, 2024 at 04:39:39PM +0200, Oleksij Rempel wrote:

Hi Fabio, Stefan,

On Wed, Aug 21, 2024 at 08:01:20AM -0300, Fabio Estevam wrote:

quoted

Hi Stefan,

On Mon, Aug 19, 2024 at 4:20 AM Stefan Eichenberger [off-list ref] wrote:

quoted

From: Stefan Eichenberger <redacted>

According to the i.MX8M Mini reference manual chapter "16.1.4.2
Generation of Start" it is only necessary to poll for bus busy and
arbitration lost in multi master mode. This helps to avoid rescheduling
while the i2c bus is busy and avoids SMBus devices to timeout.

Signed-off-by: Stefan Eichenberger <redacted>
Reviewed-by: Frank Li <Frank.Li@nxp.com>

This fixes a pca953x probe error on an imx8mp board running linux-stable 6.6:

[    1.893260] pca953x 2-0020: failed writing register
[    1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11

Could you please add a Fixes tag and Cc stable so that this can reach
the stable kernels?

Tested-by: Fabio Estevam <redacted>

Thanks a lot for testing. Are the other patches required as well or did
only introducing the master mode flag solve the issue?

quoted

Thanks a lot,

It looks like with this patch, the I2SR_IAL interrupt is not cleared.
I would expect some kind of interrupt storm. Can you confirm it?

This is a good question. i2c_imx_trx_complete was never called in the
interrupt handler. So that would mean the storm would already be there
before just for a shorter time. We only clear the IFF flag in the isr.

This causes a processor interrupt request (if the interrupt enable is
asserted [IIEN = 1]). The interrupt is set when one of the following
occurs:
- One byte transfer is completed (the interrupt is set at the falling
  edge of the ninth clock).
- An address is received that matches its own specific address in
  Slave Receive mode.
- Arbitration is lost.

Unfortunately, I don't have a device that uses multi master mode and we
would only see it on such a device. However, also from the reference
manual:

IAL must be cleared by software by writing a "0" to it at the start of
the interrupt service routine

So most likely it was wrong the whole the time we just didn't see it
before, could that be? I think a fix would be relatively easy we have to
clear it at the beginning of the isr but after we read the status. I
could add this to the series if you agree.

Regards,
Stefan