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

[Tao Ren]

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


Cheers,

Tao