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

[Liu Kimriver/刘金河]

HI, Andy


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

>On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
>> >From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> >Sent: 2024年9月10日 17:03
>> >To: Liu Kimriver/刘金河 <kimriver.liu@xxxxxxxxxxxx> On Tue, Sep 10, 2024 
>> >at 02:13:09PM +0800, Kimriver Liu wrote:

>...

>> >master --> controller
>> 
>>  Update it in V9

>Also in the Subject.
 OK, update it in [PATCH 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)

>No, this one is done by understanding where the problem appear first.
>What you mentioned above may be achieved by using --base option when format the patch.

 Fixes: 2409205acd3c ("i2c: designware: fix __i2c_dw_disable() in case master is holding SCL low")

>...

> >> +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)

>Yes, sorry about that. I did maybe not clearly get how it is going to look like.

>> >> +{
>> >> +	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)

>Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.

 as described in the DesignWare I2C databook:
 DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
 Controller FSM Activity Status. When the Controller Finite
 State Machine (FSM) is not in the IDLE state, this bit is set.
 Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of
 SLV_ACTIVITY and MST_ACTIVITY bits.
 Values:
 ■ 0x1 (ACTIVE): Controller not idle
 ■ 0x0 (IDLE): Controller is idle

We need waiting DW_IC_STATUS.MST_ACTIVITY idling,
If Controller not idle, Wait for a while.
Return value: 
  false(0): Controller is idle
  timeout(-110): Controller activity

Ok, change the function name i2c_dw_is_master_idling(dev) to i2c_dw_is_controller_active(dev)
it seems more reasonable

static int i2c_dw_is_controller_ active(struct dw_i2c_dev *dev)
{
	u32 status;

	regmap_read(dev->map, DW_IC_STATUS, &status);
	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
		return false;

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

>> >> +}

...

>> >> +	/*
>> >> +	 * 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

>Yes, what I showed was just an example, put the real numbers into the comment.

  * This happens rarely (~1:500) and is hard to reproduce. Debug trace

>> >> +	 * 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?

See above.

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

I will be off work,  If there are still emails that I have not been replied to, 
 I will reply to your email immediately after going to work tomorrow.
 
Thanks you for your suggestion!

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