Messages in this thread | | | Date | Tue, 22 Aug 2023 11:04:41 +0200 | Subject | Re: [PATCH v2] i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low | From | Yann Sionneau <> |
| |
Hi,
On 8/21/23 17:32, Andy Shevchenko wrote: > On Mon, Aug 21, 2023 at 04:01:03PM +0200, Yann Sionneau wrote: >> From: Yann Sionneau <ysionneau@kalray.eu> >> >> The DesignWare IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN >> parameter. >> In this case, when the TX FIFO gets empty and the last command didn't have >> the STOP bit (IC_DATA_CMD[9]), the controller will hold SCL low until >> a new command is pushed into the TX FIFO or the transfer is aborted. >> >> When the controller is holding SCL low, it cannot be disabled. >> The transfer must first be aborted. >> Also, the bus recovery won't work because SCL is held low by the master. >> >> Check if the master is holding SCL low in __i2c_dw_disable() before trying >> to disable the controller. If SCL is held low, an abort is initiated. >> When the abort is done, then proceed with disabling the controller. >> >> This whole situation can happen for instance during SMBus read data block >> if the slave just responds with "byte count == 0". >> This puts the driver in an unrecoverable state, because the controller is >> holding SCL low and the current __i2c_dw_disable() procedure is not >> working. In this situation only a SoC reset can fix the i2c bus. > Thank you for an update! > My comments below. > > ... > >> void __i2c_dw_disable(struct dw_i2c_dev *dev) >> { >> - int timeout = 100; > I would leave this untouched to make patch less invasive and > easier to backport. Ack > >> + unsigned int raw_intr_stats; >> + bool abort_done = false; >> + int abort_timeout = 100; >> + int dis_timeout = 100; >> + unsigned int enable; >> + bool abort_needed; >> u32 status; >> >> + regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_intr_stats); >> + regmap_read(dev->map, DW_IC_ENABLE, &enable); >> + >> + abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD; >> + > This blank line is not needed. Ack > >> + if (abort_needed) { >> + regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT); >> + do { >> + regmap_read(dev->map, DW_IC_ENABLE, &enable); >> + abort_done = !(enable & DW_IC_ENABLE_ABORT); >> + usleep_range(10, 20); >> + } while (!abort_done && abort_timeout--); > Now as you split this loop, it may be replaced by regmap_read_poll_timeout() > call. Ah yes, good idea, thanks! >> + if (!abort_done) >> + dev_err(dev->dev, "timeout while trying to abort current transfer\n"); >> + } > ... > > Other than above, looks good to me. > Very cool, thanks for the review!
--
Yann
| |