From 745c35085c22c335136e973fb6ace801d46411a9 Mon Sep 17 00:00:00 2001 From: "huaming.jiang" Date: Fri, 10 May 2024 12:09:33 +0800 Subject: [PATCH] driver:dw_i2c:detect rx fifo over and do retry 1. dw_i2c for hw not enable IC_EMPTYFIFO_HOLD_MASTER_EN,RX FIFO may over if SW not read RX FIFO day timely.So enabel RX OVER INT,when receive RX OVER ,i2c_dw_xfer return -EAGAIN,to trigger retry. 2. for large i2c read, set DW_IC_RX_TL to rx_fifo_depth / 2, to make more efficient irq process. 3.skip isr once rx over by disable INT 4. merge fix from upstream commit 301c8f5c32c8fb79c67539bc23972dc3ef48024c Author: Jarkko Nikula Date: Tue Sep 27 16:56:44 2022 +0300 i2c: designware: Fix handling of real but unexpected device interrupts Commit c7b79a752871 ("mfd: intel-lpss: Add Intel Alder Lake PCH-S PCI IDs") caused a regression on certain Gigabyte motherboards for Intel Alder Lake-S where system crashes to NULL pointer dereference in i2c_dw_xfer_msg() when system resumes from S3 sleep state ("deep"). I was able to debug the issue on Gigabyte Z690 AORUS ELITE and made following notes: - Issue happens when resuming from S3 but not when resuming from "s2idle" - PCI device 00:15.0 == i2c_designware.0 is already in D0 state when system enters into pci_pm_resume_noirq() while all other i2c_designware PCI devices are in D3. Devices were runtime suspended and in D3 prior entering into suspend - Interrupt comes after pci_pm_resume_noirq() when device interrupts are re-enabled - According to register dump the interrupt really comes from the i2c_designware.0. Controller is enabled, I2C target address register points to a one detectable I2C device address 0x60 and the DW_IC_RAW_INTR_STAT register START_DET, STOP_DET, ACTIVITY and TX_EMPTY bits are set indicating completed I2C transaction. My guess is that the firmware uses this controller to communicate with an on-board I2C device during resume but does not disable the controller before giving control to an operating system. I was told the UEFI update fixes this but never the less it revealed the driver is not ready to handle TX_EMPTY (or RX_FULL) interrupt when device is supposed to be idle and state variables are not set (especially the dev->msgs pointer which may point to NULL or stale old data). Introduce a new software status flag STATUS_ACTIVE indicating when the controller is active in driver point of view. Now treat all interrupts that occur when is not set as unexpected and mask all interrupts from the controller. Change-Id: If81c92a03b9be8699742184b46c03df34f922a28 --- drivers/i2c/busses/i2c-designware-core.h | 7 ++- drivers/i2c/busses/i2c-designware-master.c | 51 +++++++++++++------ .../i2c/busses/i2c-designware-master_dma.c | 22 ++++++++ 3 files changed, 63 insertions(+), 17 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 604c3fd6e..44ee03b30 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -126,8 +126,9 @@ * status codes */ #define STATUS_IDLE 0x0 -#define STATUS_WRITE_IN_PROGRESS 0x1 -#define STATUS_READ_IN_PROGRESS 0x2 +#define STATUS_ACTIVE 0x1 +#define STATUS_WRITE_IN_PROGRESS 0x2 +#define STATUS_READ_IN_PROGRESS 0x4 /* * operation modes @@ -331,12 +332,14 @@ void i2c_dw_disable_int(struct dw_i2c_dev *dev); static inline void __i2c_dw_enable(struct dw_i2c_dev *dev) { + dev->status |= STATUS_ACTIVE; regmap_write(dev->map, DW_IC_ENABLE, 1); } static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev) { regmap_write(dev->map, DW_IC_ENABLE, 0); + dev->status &= ~STATUS_ACTIVE; } void __i2c_dw_disable(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index a35d56cd6..14ed88bca 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -261,9 +261,9 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) if (dev->dw_i2c_enable_dma) { i2c_dw_xfer_dma_init(dev); - regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK & (~DW_IC_INTR_TX_EMPTY)); + regmap_write(dev->map, DW_IC_INTR_MASK, (DW_IC_INTR_MASTER_MASK & (~DW_IC_INTR_TX_EMPTY))|DW_IC_INTR_RX_OVER); } else { - regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK); + regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK|DW_IC_INTR_RX_OVER); } } @@ -433,7 +433,6 @@ i2c_dw_read(struct dw_i2c_dev *dev) } regmap_read(dev->map, DW_IC_RXFLR, &rx_valid); - for (; len > 0 && rx_valid > 0; len--, rx_valid--) { u32 flags = msgs[dev->msg_read_idx].flags; @@ -546,12 +545,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) } if (dev->status || dev->tx_status) { - dev_err(dev->dev, "transfer terminated early - interrupt latency too high? sta 0x%x\n", dev->status); - - dev_err(dev->dev, "laststa 0x%x, laststatus 0x%x\n", dev->laststat, dev->laststatus); - dev_err(dev->dev, "dev->tx_status 0x%x\n", dev->tx_status); - dev_err(dev->dev, "dev->rx_outstanding %d\n", dev->rx_outstanding); - } + dev_err(dev->dev, "transfer terminated early - interrupt latency too high? sta 0x%x,tx_status 0x%x\n",dev->status,dev->tx_status); + dev_err(dev->dev, "laststa 0x%x, laststatus 0x%x,rx_outstanding %d\n", dev->laststat, dev->laststatus, dev->rx_outstanding); + if(dev->laststat&DW_IC_INTR_RX_OVER) + { + ret = -EAGAIN; + goto done; + } + } ret = -EIO; @@ -640,6 +641,19 @@ static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev) u32 stat, status; stat = i2c_dw_read_clear_intrbits(dev); + + if(!(dev->status & STATUS_ACTIVE)) { + /* + * Unexpected interrupt in driver point of view. State + * variables are either unset or stale so acknowledge and + * disable interrupts for suppressing further interrupts if + * interrupt really came from this HW (E.g. firmware has left + * the HW active). + */ + regmap_write(dev->map, DW_IC_INTR_MASK, 0); + return 0; + } + if (stat & DW_IC_INTR_TX_ABRT) { dev->cmd_err |= DW_IC_ERR_TX_ABRT; dev->status = STATUS_IDLE; @@ -653,8 +667,13 @@ static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev) } goto tx_aborted; } - - if (stat & DW_IC_INTR_RX_FULL) + if(stat & DW_IC_INTR_RX_OVER) + { + /* Anytime RX_OVER is set, Make sure to skip them.*/ + regmap_write(dev->map, DW_IC_INTR_MASK, 0); + goto tx_aborted; + } + if ((stat & DW_IC_INTR_RX_FULL) || (dev->rx_outstanding >0)) i2c_dw_read(dev); if (stat & DW_IC_INTR_TX_EMPTY) { @@ -669,13 +688,14 @@ static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev) tx_aborted: regmap_read(dev->map, DW_IC_STATUS, &status); - if ((stat & DW_IC_INTR_TX_ABRT) || dev->msg_err || + + if ((stat & (DW_IC_INTR_TX_ABRT|DW_IC_INTR_RX_OVER))|| dev->msg_err || ((status & DW_IC_STATUS_TFE) && (!(status & DW_IC_STATUS_RFNE)) && (!(status & DW_IC_STATUS_MASTER_ACTIVITY)))) { + dev->laststat = stat; + dev->laststatus = status; complete(&dev->cmd_complete); - dev->laststat = stat; - dev->laststatus = status; } else if (unlikely(dev->flags & ACCESS_INTR_MASK)) { /* Workaround to trigger pending interrupt */ regmap_read(dev->map, DW_IC_INTR_MASK, &stat); @@ -690,7 +710,8 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) { struct dw_i2c_dev *dev = dev_id; u32 stat, enabled; - + if (pm_runtime_suspended(dev->dev)) + return IRQ_NONE; regmap_read(dev->map, DW_IC_ENABLE, &enabled); regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat); dev_dbg(dev->dev, "enabled=%#x stat=%#x\n", enabled, stat); diff --git a/drivers/i2c/busses/i2c-designware-master_dma.c b/drivers/i2c/busses/i2c-designware-master_dma.c index 3b31461d1..734723714 100644 --- a/drivers/i2c/busses/i2c-designware-master_dma.c +++ b/drivers/i2c/busses/i2c-designware-master_dma.c @@ -267,6 +267,25 @@ error: return -1; } +static int i2x_dw_get_read_size(struct dw_i2c_dev *dev) +{ + struct i2c_msg *msgs = dev->msgs; + u32 addr = msgs[dev->msg_write_idx].addr; + int i = 0; + int len = 0; + __dev_vdgb(dev->dev, "%s, %d, enter\n", __func__, __LINE__); + for (i = dev->msg_write_idx; i < dev->msgs_num; i++) { + if (msgs[i].addr != addr) { + dev_err(dev->dev, "%s: invalid target address\n", __func__); + dev->msg_err = -EINVAL; + break; + } + if(msgs[i].flags & I2C_M_RD) + len += msgs[i].len; + } + __dev_vdgb(dev->dev, "%s, %d, exit\n", __func__, __LINE__); + return len; +} int i2c_dw_xfer_dma_init(struct dw_i2c_dev *dev) { struct i2c_dw_dma *dma = &dev->dma; @@ -275,6 +294,9 @@ int i2c_dw_xfer_dma_init(struct dw_i2c_dev *dev) // i2c dma set tx data level regmap_write(dev->map, DW_IC_DMA_TDLR, dev->tx_fifo_depth / 2); + if(i2x_dw_get_read_size(dev)>(dev->rx_fifo_depth / 2)) + regmap_write(dev->map, DW_IC_RX_TL, dev->rx_fifo_depth / 2); + // i2c dma tx enable regmap_write(dev->map, DW_IC_DMA_CR, DW_IC_DMA_CR_TXEN);