Re: [PATCH dev-5.2 2/2] i2c: aspeed: add slave inactive timeout support

[Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>]

Hi Tao,

On 9/5/2019 7:35 PM, Tao Ren wrote:
> Hi Jae,
> 
> On 9/5/19 6:56 PM, Tao Ren wrote:
>> On 9/5/19 6:16 PM, Tao Ren wrote:
>>> Hi Jae,
>>>
>>> On 9/5/19 4:35 PM, Jae Hyun Yoo wrote:
>>>> On 9/5/2019 4:19 PM, Tao Ren wrote:
>>>>> On 9/5/19 3:48 PM, Jae Hyun Yoo wrote:
>>>>>> Hi Tao,
>>>>>>
>>>>>> On 9/5/2019 3:28 PM, Tao Ren wrote:
>>>>>>> Hi Jae,
>>>>>>>
>>>>>>> On 9/4/19 1:07 PM, Jae Hyun Yoo wrote:
>>>>>>>> In case of multi-master environment, if a peer master incorrectly handles
>>>>>>>> a bus in the middle of a transaction, I2C hardware hangs in slave state
>>>>>>>> and it can't escape from the slave state, so this commit adds slave
>>>>>>>> inactive timeout support to recover the bus in the case.
>>>>>>>>
>>>>>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>>>>>
>>>>>>> I have a generic question on the patch: is it good enough to just enable slave_inactive_timeout and acknowledge the interrupt when it happens? Or do we need to reset the i2c controller to bring it out of slave state?
>>>>>>>
>>>>>>> I'm asking this because I hit an issue on my Minipack BMC where the slave_inactive_timeout interrupt was generated once every ~0.1 seconds (I set aspeed,hw-timeout-ms to 1000), and a few seconds later my BMC was rebooted (I guess watchdog timed out). Given the issue happened only once, I don't have chance to collect more information (such as why the repetitive interrupt was generated, why BMC rebooted, and etc.). Will share more if I met the problem again.
>>>>>>
>>>>>> Basic concept of this implementation is setting the slave state of
>>>>>> driver to ASPEED_I2C_SLAVE_INACTIVE to avoid calling of
>>>>>> aspeed_i2c_reset() directly from interrupt context. Instead, when a
>>>>>> master xfer happens after that, it will try bus recovery
>>>>>> through aspeed_i2c_recover_bus() and it will call aspeed_i2c_reset()
>>>>>> if needed.
>>>>>>
>>>>>> If this patch doesn't work in your case, test it again after adding
>>>>>> one line code into this driver. See below.
>>>>>
>>>>> If bus_reset is really needed in this case, then I'd prefer to do it immediately or in a threaded_irq_handler if it's bad idea to call aspeed_i2c_reset() in hardware interrupt context. The reasons being:
>>>>>
>>>>> 1) we don't know when userspace starts next master transfer.
>>>>> 2) aspeed_i2c_recover_bus() is not triggered in aspeed_i2c_master_xfer() in my environment because bus is "idle" (I2CD14[16] == 0).
>>>>>
>>>>
>>>> Oh, so to support the case as well, probably we need to add a flag for
>>>> indicating recovery needs when a master xfer comes then it could
>>>> forcibly recover and reset the bus even if the bus is idle. Can you
>>>> please test that with making code changes? Unfortunately, I can't
>>>> reproduce the case in my system.
>>>
>>> Not sure if I understand it correctly, but given we already reset the bus in interrupt handler, the extra flag should not be needed?
>>
>> One of my colleagues reminded me to enable bus auto release (I2CD00[17]) which could avoid explicit bus reset in interrupt handler.
>>
>> Let me try and will update back soon.
> 
> Let me summarize my testing result on Minipack BMC:
> 
> BMC i2c controller cannot exit slave state by enabling slave_inactive_timeout interrupt only: in this case, i2c controller stays in SRXD state and slave_inactive_timeout interrupt will be generated again and again until bus is explicitly reset.
> 
> I've tried below 2 approaches and both works on my platform:
>    - calling aspeed_i2c_reset() in interrupt handler when slave_inacive_timeout interrupt is delivered.
>    - enabling bus auto release feature (I2CD00[17]) in probe function.

Thanks a lot for your testing result and suggestions. It seems that the
latter way - enabling bus auto release would be better for this
implementation.

I'll submit a next spin today.

Cheers,
Jae