RE: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

["Liu Kimriver/刘金河" <kimriver.liu@siengine.com>]

Hi Andy,

>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
>Sent: 2024年9月10日 17:03
>To: Liu Kimriver/刘金河 <kimriver.liu@siengine.com>
>Cc: jarkko.nikula@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; andi.shyti@kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

>On Tue, Sep 10, 2024 at 02:13:09PM +0800, Kimriver Liu wrote:
>> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when
>
>"...observed that issuing..."
>...bit (..."


>> IC_ENABLE is already disabled.
>> 
>> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is

>"...bit (..."
>master --> controller

 Update it in V9

>> holding SCL low. If ENABLE bit is disabled, the software need
>> enable it before trying to issue ABORT bit. otherwise,
>> the controller ignores any write to ABORT bit.

>Fixes tag?

 Patch rebase:  on Linux v6.11.0-rc6 (89f5e14d05b)

>...

>>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>>  	if (abort_needed) {
>> +		if (!(enable & DW_IC_ENABLE_ENABLE)) {
>
>> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
>
>This call might also need a one line comment.
  Last Wednesday
>> +			enable |= DW_IC_ENABLE_ENABLE;

>More natural is to put this after the fsleep() call. The rationale is that it
>will be easier to see what exactly is going to be written back to the
>register.
 Ok 

>> +			/*
>> +			 * Wait 10 times the signaling period of the highest I2C
> >+			 * transfer supported by the driver (for 400KHz this is
> >+			 * 25us) to ensure the I2C ENABLE bit is already set
>> +			 * as described in the DesignWare I2C databook.
>> +			 */
> >+			fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
>
>...somewhere here...
>
	Ok
>> +		}
> +
>>  		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);

...

>> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)

>Sorry if I made a mistake, but again, looking at the usage you have again
>negation here and there...

>	i2c_dw_is_controller_active

> (note new terminology, dunno if it makes sense start using it in function
> names, as we have more of them following old style)

 Last week , You suggested that I used this i2c_dw_is_master_idling(dev)

>> +{
>> +	u32 status;
>> +
>> +	regmap_read(dev->map, DW_IC_STATUS, &status);
>> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> +		return true;

		return false;

.,,

>> +	return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> +			1100, 20000);

>...and drop !.


 We reproduce this issue in RTL simulation(About(~1:500) in our soc). It is necessary
 to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling before disabling I2C when 
 I2C transfer completed.  as described in the DesignWare
 I2C databook(Flowchart for DW_apb_i2c Controller)

>> +}

...

>> +	/*
>> +	 * This happens rarely and is hard to reproduce. Debug trace

>Rarely how? Perhaps put a ration in the parentheses, like

>"...rarely (~1:100)..."
 About(~1:500) in our soc

>> +	 * showed that IC_STATUS had value of 0x23 when STOP_DET occurred,
>> +	 * if disable IC_ENABLE.ENABLE immediately that can result in
>> +	 * IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.
>> +	 */
>> +	if (!i2c_dw_is_master_idling(dev))

>...and here

>	if (i2c_dw_is_controller_active(dev))

>But please double check that I haven't made any mistakes in all this logic.

 Last week , You suggested that I used this i2c_dw_is_master_idling(dev)
 keep using i2c_dw_is_master_idling(dev) , Ok?

>> +		dev_err(dev->dev, "I2C master not idling\n");


------------------------------------------
Best Regards
Kimriver Liu