Incorrect handling of flags in the Linux I2C driver for LS2088?

[met]

Hi!

We are in the process of debugging an issue in a multi-master I2C application, where the bus occasionally gets locked up.

Both masters are LS2088s running Linux, and we are aware of errata A-010124..

But while looking into the driver code (drivers/i2c/busses/i2c-imx.c), I believe I came across one or more bugs.

According to the reference manual, the IBAL and IBIF flags in IBSR should be cleared by writing a 1 to them.

However, the function i2c_imx_bus_busy(...) attempts to mask out the flag instead:

if (temp & I2SR_IAL) {
   temp &= ~I2SR_IAL;
   imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
   return -EAGAIN;
}

So if IBAL has become set, this code will never clear it. Instead, the I2C driver will return -EAGAIN forever.

This is actually exactly what we see in our application.

Investigating further, I found some struct definitions which provide hardware version-specific functions for, e.g., clearing flags.

But these are clearly not used consistently throughout the driver.

Can you please look into this?

Best regards,

    Martin Etnestad

[Pavel]

The following code is used for clearing the IBAL flag in the i2,_mx_bus_busy procedure:

                while (1) {

                               temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);

                               /* check for arbitration lost */

                               if (temp & I2SR_IAL) {

                                               temp &= ~I2SR_IAL;

                                               imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);

                                               return -EAGAIN;

                               }

This code is correct for i.MX I2c. This code is incorrect for the LS2088a I2C.

Correct code for the LS2088a is like to the following:

                while (1) {

                               temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);

                               /* check for arbitration lost */

                               if (temp & I2SR_IAL) {

                                               temp |=  I2SR_IAL;

                                               imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);

                                               return -EAGAIN;

                               }

The LS2088a I2C is similar to the QorIQ I2C. Therefore the i2c-mpc.c file contains correct code.

Have a great day,
Pavel Chubakov

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[Pavel]

The following code is used for clearing the IBAL flag in the i2,_mx_bus_busy procedure:

                while (1) {

                               temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);

                               /* check for arbitration lost */

                               if (temp & I2SR_IAL) {

                                               temp &= ~I2SR_IAL;

                                               imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);

                                               return -EAGAIN;

                               }

It is correct code.

Have a great day,
Pavel Chubakov

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[met]

Hi Pavel,

Are you absolutely sure?

The LS2088A reference manual I have (from 10/2016) says explicitly about both IBAL and IBIF:

"This bit must be cleared by software, by writing a one to it. A write of zero has no effect."

Both bits are flagged with "w1c" in the "W"-row of the register diagram.

Since the current code writes a zero to IBAL, I think either the code or the reference manual must be wrong.

Note that I am assuming the purpose is to clear the IBAL flag.

And as mentioned, there are these structs defined which provide different implementations for operations like clearing IBSR:

static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
   .devtype = IMX1_I2C,
   .regshift = IMX_I2C_REGSHIFT,
   .clk_div = imx_i2c_clk_div,
   .ndivs = ARRAY_SIZE(imx_i2c_clk_div),
   .i2sr_clr_opcode = I2SR_CLR_OPCODE_W0C,
   .i2cr_ien_opcode = I2CR_IEN_OPCODE_1,
};

static const struct imx_i2c_hwdata imx21_i2c_hwdata = {
   .devtype = IMX21_I2C,
   .regshift = IMX_I2C_REGSHIFT,
   .clk_div = imx_i2c_clk_div,
   .ndivs = ARRAY_SIZE(imx_i2c_clk_div),
   .i2sr_clr_opcode = I2SR_CLR_OPCODE_W0C,
   .i2cr_ien_opcode = I2CR_IEN_OPCODE_1,
};

static struct imx_i2c_hwdata vf610_i2c_hwdata = {
   .devtype = VF610_I2C,
   .regshift = VF610_I2C_REGSHIFT,
   .clk_div = vf610_i2c_clk_div,
   .ndivs = ARRAY_SIZE(vf610_i2c_clk_div),
   .i2sr_clr_opcode = I2SR_CLR_OPCODE_W1C,
   .i2cr_ien_opcode = I2CR_IEN_OPCODE_0,
};

Note especially that the last one has I2SR_CLR_OPCODE_W1C, named same as the flag from the register diagram.

Should i2c_mx_bus_busy() not use .i2sr_clr_opcode to clear flags from IBSR, instead of the current approach?

[edit]

Also, I just noticed this at the end of i2c_mx_bus_busy():

if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
   u8 status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);

   dev_dbg(&i2c_imx->adapter.dev,
      "<%s> I2C bus is busy\n", __func__);
   if ((status & (I2SR_ICF | I2SR_IBB | I2CR_TXAK)) != 0) {
      imx_i2c_write_reg(status & ~I2SR_IAL, i2c_imx,
            IMX_I2C_I2CR);
      imx_i2c_fixup(i2c_imx);
   }
   return -ETIMEDOUT;
}

Note that it reads flags from the status register, then it writes the same flags to the control register.

Is that also correct? Is not the purpose of this code to clear status flags?

If the TCF (I2SR_ICF) flag is set in status, this code will set the MDIS bit in control.

And that will actually disable the I2C module, according to our reference manual...

[/edit]

Best regards,

    Martin Etnestad