Hi Andy,
>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>Sent: 2024年9月10日 17:03
>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 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
