Hi Andy
>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>Sent: 2024年9月10日 19:59
>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 11:43:34AM +0000, Liu Kimriver/刘金河 wrote:
>> >-----Original Message-----
>> >From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> >Sent: 2024年9月10日 18:45
>> >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
>> >> >at 02:13:09PM +0800, Kimriver Liu wrote:
>
>...
>
>> > >> +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
>>
Change above text as a comment:
/*
* This functions waits controller idling before disabling I2C
* When the controller is not in the IDLE state,
* MST_ACTIVITY bit (IC_STATUS[5]) is set:
* 0x1 (ACTIVE): Controller not idle
* 0x0 (IDLE): Controller is idle
* The function is called after returning the end of the current transfer
* Returns:
* Return 0 as controller IDLE,
* Return a negative errno as controller ACTIVE
*/
>> 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 0;
>>
>> return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> 1100, 20000);
>> }
>Yes, thank you. This is pure readability wise, you may actually leave the above text as a comment on top of that helper. It will add a value of understanding what's behind the scenes.
> >> >> +}
...
>> 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.
>No problem. Just keep your time, proof-read and test the v9 before sending and I believe it will be the last iteration. Thank you for your patience and energy to push this change forward!
After the testing and validation are completed, I will resend v9 version.
Thank you!
>
>...
>
>> Thanks you for your suggestion!
>You are welcome!
------------------------------------------
Best Regards
Kimriver Liu
