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

[Liu Kimriver/刘金河]

HI andi

Due to a 12 time difference，I had been off work.
I am very sorry that I can't reply email in time. I will reply to your email  immediately after going to work tomorrow.

发自我的 iPhone

>From: Andi Shyti <andi.shyti@xxxxxxxxxx>
>To: Kimriver Liu <kimriver.liu@xxxxxxxxxxxx>
>Cc: jarkko.nikula@xxxxxxxxxxxxxxx, andriy.shevchenko@xxxxxxxxxxxxxxx,
	 mika.westerberg@xxxxxxxxxxxxxxx, jsd@xxxxxxxxxxxx,
	linux-i2c@xxxxxxxxxxxxxxx,  linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v7] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled
>Date: Mon, 9 Sep 2024 15:08:25 +0200	[thread overview]
>Message-ID: <traj5uhqbny5yro3hf72k2qpga7ez7cuqv2mesvl73fku2b5xq@uw5dqjdc6mms> (raw)
>In-Reply-To: <20240909015646.2285-1-kimriver.liu@xxxxxxxxxxxx>

>Hi Kimriver,

>On Mon, Sep 09, 2024 at 09:56:46AM GMT, Kimriver Liu wrote:
>> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when
>> IC_ENABLE is already disabled.
>> 
>> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is
>> 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.
>> 
>> Signed-off-by: Kimriver Liu <kimriver.liu@xxxxxxxxxxxx>

>You forgot:

>Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
  
  Thank you for telling me
  
>> ---
>> V6->V7:
>> 	1. add Subject versioning [PATCH v7]
>> 	2. change fsleep(25) to usleep_range(25, 250)
>> 	3. Add macro definition DW_iC_ENABLE_ENABLE to fix compile errors
>> 	4. base: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=master

>Thanks a lot for following up! :-)

>> V5->V6: restore i2c_dw_is_master_idling() function checking
>> V4->V5: delete master idling checking
>> V3->V4:
>> 	1. update commit messages and add patch version and changelog
>> 	2. move print the error message in i2c_dw_xfer
>> V2->V3: change (!enable) to (!(enable & DW_IC_ENABLE_ENABLE))
>> V1->V2: used standard words in function names and addressed review comments
>> 
>> link to V1:
>> https://lore.kernel.org/lkml/20240904064224.2394-1-kimriver.liu@xxxxxxxxxxxx/
>> ---

...

>> --- a/drivers/i2c/busses/i2c-designware-common.c
>> +++ b/drivers/i2c/busses/i2c-designware-common.c
>> @@ -453,6 +453,18 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
>>  
>>  	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);
>> +			enable |= DW_IC_ENABLE_ENABLE;
>> +			/*
>> +			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
>> +			 * is already set. Wait 10 times the signaling period of the highest
>> +			 * I2C transfer supported by the driver(for 400KHz this is 25us)
>> +			 * as described in the DesignWare I2C databook.
>> +			 */
>> +			usleep_range(25, 250);

>I think there is a misunderstanding here. Andy asked you to use
>flseep and improve the calculation: "Please, calculate this delay
>based on the actual speed in use (or about to be in use)."[*]

>Andy can you please clarify with Kimriver here?

 if we use 400kHz ,need setting flseep(25);
 if we use 100kHz ,need setting flseep(100);
 Overall, take the maximum value:flseep(100);

> --
> 2.17.1