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
