| Message ID | 20230811124624.12792-1-yann@sionneau.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low | expand |
On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote: > From: Yann Sionneau <ysionneau@kalray.eu> > > The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN DesignWare > parameter. > In which case, if the TX FIFO gets empty and the last command didn't have "In this case when the..." > the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until "the controller will..." > a new command is pushed into the TX FIFO or the transfer is aborted. > > When the dw_apb_i2c is holding SCL low, it cannot be disabled. "When the controller..." > The transfer must first be aborted. > Also, the bus recover won't work because SCL is held low by the master. > > This patch checks if the master is holding SCL low in __i2c_dw_disable Grep for "This patch" in the Submitting Patches document and fix this accordingly. __i2c_dw_disable() > before trying to disable the IP. > If SCL is held low, an abort is initiated. > When the abort is done, the disabling can then proceed. > > This whole situation can happen for instance during SMBUS read data block > if the slave just responds with "byte count == 0". > This puts the master in an unrecoverable state, holding SCL low and the > current __i2c_dw_disable procedure is not working. In this situation __i2c_dw_disable() > only a Linux reboot can fix the i2c bus. If reboot helps, what magic does it do that Linux OS can't repeat in software? Please, elaborate more. ... > int timeout = 100; > u32 status; > + u32 raw_intr_stats; > + u32 enable; > + bool abort_needed; > + bool abort_done = false; Perhaps reversed xmas tree order? bool abort_done = false; bool abort_needed; u32 raw_intr_stats; int timeout = 100; u32 status; u32 enable; ... > + abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD; > + if (abort_needed) > + regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT); > > do { > + if (abort_needed && !abort_done) { > + regmap_read(dev->map, DW_IC_ENABLE, &enable); > + abort_done = !(enable & DW_IC_ENABLE_ABORT); > + continue; This will exhaust the timeout and below can be run at most once, is it a problem? Also it's a tight busyloop, are you sure it's what you need? > + }
Hi, Le 11/08/2023 à 15:59, Andy Shevchenko a écrit : > On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote: >> From: Yann Sionneau <ysionneau@kalray.eu> >> >> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN > DesignWare Ack! > >> parameter. >> In which case, if the TX FIFO gets empty and the last command didn't have > "In this case when the..." Ack! > >> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until > "the controller will..." Ack! > >> a new command is pushed into the TX FIFO or the transfer is aborted. >> >> When the dw_apb_i2c is holding SCL low, it cannot be disabled. > "When the controller..." Ack! > >> The transfer must first be aborted. >> Also, the bus recover won't work because SCL is held low by the master. >> >> This patch checks if the master is holding SCL low in __i2c_dw_disable > Grep for "This patch" in the Submitting Patches document and fix this > accordingly. Ok I didn't know, ack! > > __i2c_dw_disable() > >> before trying to disable the IP. >> If SCL is held low, an abort is initiated. >> When the abort is done, the disabling can then proceed. >> >> This whole situation can happen for instance during SMBUS read data block >> if the slave just responds with "byte count == 0". >> This puts the master in an unrecoverable state, holding SCL low and the >> current __i2c_dw_disable procedure is not working. In this situation > __i2c_dw_disable() > >> only a Linux reboot can fix the i2c bus. > If reboot helps, what magic does it do that Linux OS can't repeat in software? > Please, elaborate more. Sorry I was not very clear. In fact I meant a SoC reset, not a Linux reboot. It's just that on our SoC with boot-from-flash a reset will also reboot the Linux. But indeed what fixes the issue is the reset of the SoC. > > ... > >> int timeout = 100; >> u32 status; >> + u32 raw_intr_stats; >> + u32 enable; >> + bool abort_needed; >> + bool abort_done = false; > Perhaps reversed xmas tree order? Oh, I didn't know about this, thanks, ack! > > bool abort_done = false; > bool abort_needed; > u32 raw_intr_stats; > int timeout = 100; > u32 status; > u32 enable; > > ... > >> + abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD; >> + if (abort_needed) >> + regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT); >> >> do { >> + if (abort_needed && !abort_done) { >> + regmap_read(dev->map, DW_IC_ENABLE, &enable); >> + abort_done = !(enable & DW_IC_ENABLE_ABORT); >> + continue; > This will exhaust the timeout and below can be run at most once, > is it a problem? I was also wondering about this... I can propose to extract this in 2 loops. First loop to wait for the abort to finish, with its own timeout. Then the untouched second loop that waits for the disabling to finish. > > Also it's a tight busyloop, are you sure it's what you need? I don't know, I would expect that this would not take much time, I already have a V2 for this patch with all your remarks taken into account, including the splitting into 2 loops (previous comment). I am waiting before sending it to have the opportunity to test it on the real device, it will be done on the August 21st since I am in holidays for now. I will print the number of iterations it takes for the abort to finish. If the abort is quick, maybe there is no need for sleeping. I didn't see any info about the time it takes inside the IP documentation. Thanks for the review! I'll get back to you when I have done the testing of the V2 patch :) (or maybe you want it on the mailing list now as an RFC ?)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 9f8574320eb2..744927b0c5af 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -440,8 +440,25 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev) { int timeout = 100; u32 status; + u32 raw_intr_stats; + u32 enable; + bool abort_needed; + bool abort_done = false; + + 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; + if (abort_needed) + regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT); do { + if (abort_needed && !abort_done) { + regmap_read(dev->map, DW_IC_ENABLE, &enable); + abort_done = !(enable & DW_IC_ENABLE_ABORT); + continue; + } + __i2c_dw_disable_nowait(dev); /* * The enable status register may be unimplemented, but diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 19ae23575945..dcd9bd9ee00f 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -98,6 +98,7 @@ #define DW_IC_INTR_START_DET BIT(10) #define DW_IC_INTR_GEN_CALL BIT(11) #define DW_IC_INTR_RESTART_DET BIT(12) +#define DW_IC_INTR_MST_ON_HOLD BIT(13) #define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL | \ DW_IC_INTR_TX_ABRT | \ @@ -109,6 +110,8 @@ DW_IC_INTR_RX_UNDER | \ DW_IC_INTR_RD_REQ) +#define DW_IC_ENABLE_ABORT BIT(1) + #define DW_IC_STATUS_ACTIVITY BIT(0) #define DW_IC_STATUS_TFE BIT(2) #define DW_IC_STATUS_MASTER_ACTIVITY BIT(5)