On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@xxxxxxxxx>
>
> 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?
> + }
--
With Best Regards,
Andy Shevchenko
