Messages in this thread | | | Date | Thu, 17 Aug 2023 16:39:57 +0200 | Subject | Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low | From | Yann Sionneau <> |
| |
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 ?)
--
Yann
| |