Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle

["Cédric Le Goater" <clg@kaod.org>]

On 11/17/22 08:37, Klaus Jensen wrote:
> On Nov 17 07:56, Cédric Le Goater wrote:
>> On 11/17/22 07:40, Klaus Jensen wrote:
>>> On Nov 16 16:58, Cédric Le Goater wrote:
>>>> On 11/16/22 09:43, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> It is not given that the current master will release the bus after a
>>>>> transfer ends. Only schedule a pending master if the bus is idle.
>>>>>
>>>>> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>>     hw/i2c/aspeed_i2c.c  |  2 ++
>>>>>     hw/i2c/core.c        | 37 ++++++++++++++++++++++---------------
>>>>>     include/hw/i2c/i2c.h |  2 ++
>>>>>     3 files changed, 26 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>>>> index c166fd20fa11..1f071a3811f7 100644
>>>>> --- a/hw/i2c/aspeed_i2c.c
>>>>> +++ b/hw/i2c/aspeed_i2c.c
>>>>> @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>>>             }
>>>>>             SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
>>>>>             aspeed_i2c_set_state(bus, I2CD_IDLE);
>>>>> +
>>>>> +        i2c_schedule_pending_master(bus->bus);
>>>>
>>>> Shouldn't it be i2c_bus_release() ?
>>>>
>>>
>>> The reason for having both i2c_bus_release() and
>>> i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
>>> with i2c_bus_master(). They either set or clear the bus->bh member.
>>>
>>> In the current design, the controller (in this case the Aspeed I2C) is
>>> an "implicit" master (it does not have a bottom half driving it), so
>>> there is no bus->bh to clear.
>>>
>>> I should (and will) write some documentation on the asynchronous API.
>>
>> I found the routine names confusing. Thanks for the clarification.
>>
>> Maybe we could do this rename  :
>>
>>    i2c_bus_release()             -> i2c_bus_release_and_clear()
>>    i2c_schedule_pending_master() -> i2c_bus_release()
>>
>> and keep i2c_schedule_pending_master() internal the I2C core subsystem.
>>
> 
> How about renaming i2c_bus_master to i2c_bus_acquire() such that it
> pairs with i2c_bus_release().

Looks good to me.

> And then add an i2c_bus_yield() to be used by the controller? I think we
> should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
> I'll take a closer look at that.

I am using your i2c-echo slave device to track regressions in the Aspeed
machines. May be we could merge it for tests ?

Thanks,

C.