From 93701d3b84ac5f3ea07259d4ced405c53d757985 Mon Sep 17 00:00:00 2001 From: Tommy Huang Date: Wed, 11 Sep 2024 17:39:51 +0800 Subject: [PATCH 1/5] i2c: aspeed: Update the stop sw state when the bus recovery occurs When the i2c bus recovery occurs, driver will send i2c stop command in the scl low condition. In this case the sw state will still keep original situation. Under multi-master usage, i2c bus recovery will be called when i2c transfer timeout occurs. Update the stop command calling with aspeed_i2c_do_stop function to update master_state. Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") Cc: stable@vger.kernel.org # v4.13+ Signed-off-by: Tommy Huang Signed-off-by: Andi Shyti --- drivers/i2c/busses/i2c-aspeed.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index ce8c4846b7fa..2a03a221e2dd 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -170,6 +170,13 @@ struct aspeed_i2c_bus { static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus); +/* precondition: bus.lock has been acquired. */ +static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus) +{ + bus->master_state = ASPEED_I2C_MASTER_STOP; + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG); +} + static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus) { unsigned long time_left, flags; @@ -187,7 +194,7 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus) command); reinit_completion(&bus->cmd_complete); - writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG); + aspeed_i2c_do_stop(bus); spin_unlock_irqrestore(&bus->lock, flags); time_left = wait_for_completion_timeout( @@ -390,13 +397,6 @@ static void aspeed_i2c_do_start(struct aspeed_i2c_bus *bus) writel(command, bus->base + ASPEED_I2C_CMD_REG); } -/* precondition: bus.lock has been acquired. */ -static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus) -{ - bus->master_state = ASPEED_I2C_MASTER_STOP; - writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG); -} - /* precondition: bus.lock has been acquired. */ static void aspeed_i2c_next_msg_or_stop(struct aspeed_i2c_bus *bus) { From 521da1e9225450bd323db5fa5bca942b1dc485b7 Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Tue, 21 Nov 2023 18:11:16 +0000 Subject: [PATCH 2/5] i2c: xiic: Wait for TX empty to avoid missed TX NAKs Frequently an I2C write will be followed by a read, such as a register address write followed by a read of the register value. In this driver, when the TX FIFO half empty interrupt was raised and it was determined that there was enough space in the TX FIFO to send the following read command, it would do so without waiting for the TX FIFO to actually empty. Unfortunately it appears that in some cases this can result in a NAK that was raised by the target device on the write, such as due to an unsupported register address, being ignored and the subsequent read being done anyway. This can potentially put the I2C bus into an invalid state and/or result in invalid read data being processed. To avoid this, once a message has been fully written to the TX FIFO, wait for the TX FIFO empty interrupt before moving on to the next message, to ensure NAKs are handled properly. Fixes: e1d5b6598cdc ("i2c: Add support for Xilinx XPS IIC Bus Interface") Signed-off-by: Robert Hancock Cc: # v2.6.34+ Reviewed-by: Manikanta Guntupalli Acked-by: Michal Simek Signed-off-by: Andi Shyti --- drivers/i2c/busses/i2c-xiic.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 19468565120e..bd6e107472b7 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -772,14 +772,17 @@ static irqreturn_t xiic_process(int irq, void *dev_id) goto out; } - xiic_fill_tx_fifo(i2c); - - /* current message sent and there is space in the fifo */ - if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >= 2) { + if (xiic_tx_space(i2c)) { + xiic_fill_tx_fifo(i2c); + } else { + /* current message fully written */ dev_dbg(i2c->adap.dev.parent, "%s end of message sent, nmsgs: %d\n", __func__, i2c->nmsgs); - if (i2c->nmsgs > 1) { + /* Don't move onto the next message until the TX FIFO empties, + * to ensure that a NAK is not missed. + */ + if (i2c->nmsgs > 1 && (pend & XIIC_INTR_TX_EMPTY_MASK)) { i2c->nmsgs--; i2c->tx_msg++; xfer_more = 1; @@ -790,11 +793,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id) "%s Got TX IRQ but no more to do...\n", __func__); } - } else if (!xiic_tx_space(i2c) && (i2c->nmsgs == 1)) - /* current frame is sent and is last, - * make sure to disable tx half - */ - xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK); + } } if (pend & XIIC_INTR_BNB_MASK) { From 1d4a1adbed2582444aaf97671858b7d12915bd05 Mon Sep 17 00:00:00 2001 From: Robert Hancock Date: Wed, 11 Sep 2024 22:16:53 +0200 Subject: [PATCH 3/5] i2c: xiic: Try re-initialization on bus busy timeout In the event that the I2C bus was powered down when the I2C controller driver loads, or some spurious pulses occur on the I2C bus, it's possible that the controller detects a spurious I2C "start" condition. In this situation it may continue to report the bus is busy indefinitely and block the controller from working. The "single-master" DT flag can be specified to disable bus busy checks entirely, but this may not be safe to use in situations where other I2C masters may potentially exist. In the event that the controller reports "bus busy" for too long when starting a transaction, we can try reinitializing the controller to see if the busy condition clears. This allows recovering from this scenario. Fixes: e1d5b6598cdc ("i2c: Add support for Xilinx XPS IIC Bus Interface") Signed-off-by: Robert Hancock Cc: # v2.6.34+ Reviewed-by: Manikanta Guntupalli Acked-by: Michal Simek Signed-off-by: Andi Shyti --- drivers/i2c/busses/i2c-xiic.c | 41 ++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index bd6e107472b7..4c89aad02451 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -843,23 +843,11 @@ static int xiic_bus_busy(struct xiic_i2c *i2c) return (sr & XIIC_SR_BUS_BUSY_MASK) ? -EBUSY : 0; } -static int xiic_busy(struct xiic_i2c *i2c) +static int xiic_wait_not_busy(struct xiic_i2c *i2c) { int tries = 3; int err; - if (i2c->tx_msg || i2c->rx_msg) - return -EBUSY; - - /* 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. - */ - if (i2c->singlemaster) { - return 0; - } - /* 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 @@ -1103,13 +1091,36 @@ static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num) mutex_lock(&i2c->lock); - ret = xiic_busy(i2c); - if (ret) { + if (i2c->tx_msg || i2c->rx_msg) { dev_err(i2c->adap.dev.parent, "cannot start a transfer while busy\n"); + ret = -EBUSY; goto out; } + /* 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. + */ + if (!i2c->singlemaster) { + ret = xiic_wait_not_busy(i2c); + if (ret) { + /* If the bus is stuck in a busy state, such as due to spurious low + * pulses on the bus causing a false start condition to be detected, + * then try to recover by re-initializing the controller and check + * again if the bus is still busy. + */ + dev_warn(i2c->adap.dev.parent, "I2C bus busy timeout, reinitializing\n"); + ret = xiic_reinit(i2c); + if (ret) + goto out; + ret = xiic_wait_not_busy(i2c); + if (ret) + goto out; + } + } + i2c->tx_msg = msgs; i2c->rx_msg = NULL; i2c->nmsgs = num; From 1db4da55070d6a2754efeb3743f5312fc32f5961 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Wed, 11 Sep 2024 18:39:14 +0300 Subject: [PATCH 4/5] i2c: isch: Add missed 'else' In accordance with the existing comment and code analysis it is quite likely that there is a missed 'else' when adapter times out. Add it. Fixes: 5bc1200852c3 ("i2c: Add Intel SCH SMBus support") Signed-off-by: Andy Shevchenko Cc: # v2.6.27+ Signed-off-by: Andi Shyti --- drivers/i2c/busses/i2c-isch.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c index 33dbc19d3848..f59158489ad9 100644 --- a/drivers/i2c/busses/i2c-isch.c +++ b/drivers/i2c/busses/i2c-isch.c @@ -99,8 +99,7 @@ static int sch_transaction(void) if (retries > MAX_RETRIES) { dev_err(&sch_adapter.dev, "SMBus Timeout!\n"); result = -ETIMEDOUT; - } - if (temp & 0x04) { + } else if (temp & 0x04) { result = -EIO; dev_dbg(&sch_adapter.dev, "Bus collision! SMBus may be " "locked until next hard reset. (sorry!)\n"); From e2c85d85a05f16af2223fcc0195ff50a7938b372 Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Thu, 12 Sep 2024 11:34:59 +0800 Subject: [PATCH 5/5] i2c: qcom-geni: Use IRQF_NO_AUTOEN flag in request_irq() disable_irq() after request_irq() still has a time gap in which interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will disable IRQ auto-enable when request IRQ. Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller") Signed-off-by: Jinjie Ruan Cc: # v4.19+ Acked-by: Mukesh Kumar Savaliya Reviewed-by: Vladimir Zapolskiy Signed-off-by: Andi Shyti --- drivers/i2c/busses/i2c-qcom-geni.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 06e836e3e877..4c9050a4d58e 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -818,15 +818,13 @@ static int geni_i2c_probe(struct platform_device *pdev) init_completion(&gi2c->done); spin_lock_init(&gi2c->lock); platform_set_drvdata(pdev, gi2c); - ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, 0, + ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN, dev_name(dev), gi2c); if (ret) { dev_err(dev, "Request_irq failed:%d: err:%d\n", gi2c->irq, ret); return ret; } - /* Disable the interrupt so that the system can enter low-power mode */ - disable_irq(gi2c->irq); i2c_set_adapdata(&gi2c->adap, gi2c); gi2c->adap.dev.parent = dev; gi2c->adap.dev.of_node = dev->of_node;