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

[Andy Shevchenko]

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.

...

> >> 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.

...

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

> >> +}

...

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

> >> +	 * 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");

-- 
With Best Regards,
Andy Shevchenko