On 20. 08. 20 12:02, Jaakko Laine wrote:
> I2C master operating in multimaster mode can get stuck
> indefinitely if I2C start is detected on bus, but no master
> has a transaction going.
>
> This is a weakness in I2C standard, which defines no way
> to recover, since all masters are indefinitely disallowed
> from interrupting the currently operating master. A start
> condition can be created for example by an electromagnetic
> discharge applied near physical I2C lines. Or a already
> operating master could get reset immediately after sending
> a start.
>
> If it is known during device tree creation that only a single
> I2C master will be present on the bus, this deadlock of the
> I2C bus could be avoided in the driver by ignoring the
> bus_is_busy register of the xiic, since bus can never be
> reserved by any other master.
>
> This patch adds this support for detecting single-master flag
> in device tree and when provided, improves I2C reliability by
> ignoring the therefore unnecessary xiic bus_is_busy register.
>
> Error can be reproduced by pulling I2C SDA -line temporarily low
> by shorting it to ground, while linux I2C master is operating on
> it using the xiic driver. The application using the bus will
> start receiving linux error code 16: "Device or resource busy"
> indefinitely:
>
> kernel: pca953x 0-0020: failed writing register
> app: Error writing file, error: 16
>
> With multi-master disabled device will instead receive error
> code 5: "I/O error" while SDA is grounded, but recover normal
> operation once short is removed.
>
> kernel: pca953x 0-0020: failed reading register
> app: Error reading file, error: 5
>
> Signed-off-by: Jaakko Laine <ext-jaakko.laine@xxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-xiic.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 10380531d45c..5d06e6cc5d5c 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -58,6 +58,7 @@ enum xiic_endian {
> * @rx_msg: Current RX message
> * @rx_pos: Position within current RX message
> * @endianness: big/little-endian byte order
> + * @singlemaster: Indicates bus is single master
> * @clk: Pointer to AXI4-lite input clock
> */
> struct xiic_i2c {
> @@ -73,6 +74,7 @@ struct xiic_i2c {
> struct i2c_msg *rx_msg;
> int rx_pos;
> enum xiic_endian endianness;
> + bool singlemaster;
I would understand if this is placed above rx_msg to fill that 4bytes
hole in the structure. But this location doesn't make any sense.
The best would be to move state to the end and add this bool behind it.
To have nicely packed structure like this
on 64bit system.
struct xiic_i2c {
struct device * dev; /* 0 8 */
void * base; /* 8 8 */
wait_queue_head_t wait; /* 16 24 */
struct i2c_adapter adap; /* 40 1032 */
/* --- cacheline 16 boundary (1024 bytes) was 48 bytes ago --- */
struct i2c_msg * tx_msg; /* 1072 8 */
struct mutex lock; /* 1080 32 */
/* --- cacheline 17 boundary (1088 bytes) was 24 bytes ago --- */
unsigned int tx_pos; /* 1112 4 */
unsigned int nmsgs; /* 1116 4 */
struct i2c_msg * rx_msg; /* 1120 8 */
int rx_pos; /* 1128 4 */
enum xiic_endian endianness; /* 1132 4 */
struct clk * clk; /* 1136 8 */
enum xilinx_i2c_state state; /* 1144 4 */
bool singlemaster; /* 1148 1 */
/* size: 1152, cachelines: 18, members: 14 */
/* padding: 3 */
};
on 32bit system.
struct xiic_i2c {
struct device * dev; /* 0 4 */
void * base; /* 4 4 */
wait_queue_head_t wait; /* 8 24 */
struct i2c_adapter adap; /* 32 508 */
/* --- cacheline 8 boundary (512 bytes) was 28 bytes ago --- */
struct i2c_msg * tx_msg; /* 540 4 */
struct mutex lock; /* 544 28 */
unsigned int tx_pos; /* 572 4 */
/* --- cacheline 9 boundary (576 bytes) --- */
unsigned int nmsgs; /* 576 4 */
struct i2c_msg * rx_msg; /* 580 4 */
int rx_pos; /* 584 4 */
enum xiic_endian endianness; /* 588 4 */
struct clk * clk; /* 592 4 */
enum xilinx_i2c_state state; /* 596 4 */
bool singlemaster; /* 600 1 */
/* size: 604, cachelines: 10, members: 14 */
/* padding: 3 */
/* last cacheline: 28 bytes */
};
> struct clk *clk;
> };
>
> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
> static int xiic_busy(struct xiic_i2c *i2c)
> {
> int tries = 3;
> - int err;
> + int err = 0;
>
> if (i2c->tx_msg)
> return -EBUSY;
>
> - /* for instance if previous transfer was terminated due to TX error
> - * it might be that the bus is on it's way to become available
> - * give it at most 3 ms to wake
> + /* In single master mode bus can only be busy, when in use by this
> + * driver. If the register indicates bus being busy for some reason we
> + * should ignore it, since bus will never be released and i2c will be
> + * stuck forever.
> */
> - err = xiic_bus_busy(i2c);
> - while (err && tries--) {
> - msleep(1);
> + if (!i2c->singlemaster) {
> + /* for instance if previous transfer was terminated due to TX
> + * error it might be that the bus is on it's way to become
> + * available give it at most 3 ms to wake
> + */
> err = xiic_bus_busy(i2c);
> + while (err && tries--) {
> + msleep(1);
> + err = xiic_bus_busy(i2c);
> + }
> }
I would prefer to write this differently.
if (i2c->singlemaster)
return 0;
Followed by origin code. Patch will be smaller and you don't need to add
one more level of indentation.
>
> return err;
> @@ -811,6 +820,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> goto err_clk_dis;
> }
>
> + i2c->singlemaster =
> + of_property_read_bool(pdev->dev.of_node, "single-master");
> +
> /*
> * Detect endianness
> * Try to reset the TX FIFO. Then check the EMPTY flag. If it is not
>
Thanks,
Michal
