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

[Klaus Jensen]

On Nov 17 14:40, Cédric Le Goater wrote:
> On 11/17/22 12:58, Klaus Jensen wrote:
> > On Nov 17 09:01, Cédric Le Goater wrote:
> > > 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 ?
> > > 
> > 
> > Oh, cool.
> > 
> > Sure, I'd be happy to help "maintain" it ;)
> 
> And so, I am seeing errors with the little POC you sent.
> 
> without:
>   console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: [    4.252431] i2c i2c-3: new_device: Instantiated device 
> slave-24c02 at 0x64
>   console: i2cset -y 3 0x42 0x64 0x00 0xaa i
>   /console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
>   console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
>   console: 0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
>   console: poweroff
>   console: 0000010 ffff ffff ffff ffff ffff ffff ffff ffff
>   console: *
>   console: 0000100
> 
> with:
>   console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: [    4.413210] i2c i2c-3: new_device: Instantiated device 
> slave-24c02 at 0x64
>   console: i2cset -y 3 0x42 0x64 0x00 0xaa i
>   console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
>   console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
>   console: 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
>   console: *
>   console: 0000100
> C.

Interesting,

I'll check it out.

signature.asc
Description: PGP signature