Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

[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@xxxxxxxxx>
> >
> > 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