Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
> > > > > > > >
> > > > > > > > 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
> > > > > > > > ---
> > > > > > > > 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: 000 ffaa
> console: poweroff
> console: 010
> console: *
> console: 100
>
> 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: 000
> console: *
> console: 100
> C.
Right.
This is because the i2c-echo device is scheduling the bottom half
initially on its own. What happens is that the bottom half gets queued
up in the pending masters list instead of being scheduling directly. And
since the i2c controller is idle, the bottom half is never scheduled.
Fixing i2c_bus_acquire() to schedulue it directly if the bus is free
seems the proper way to do it. I'll include that in v2.
While it is not directly invalid, the
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
> > > > > > > >
> > > > > > > > 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
> > > > > > > > ---
> > > > > > > > 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: 000 ffaa
> console: poweroff
> console: 010
> console: *
> console: 100
>
> 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: 000
> console: *
> console: 100
> C.
Interesting,
I'll check it out.
signature.asc
Description: PGP signature
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
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
---
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: 000 ffaa
console: poweroff
console: 010
console: *
console: 100
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: 000
console: *
console: 100
C.
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
> > > > > >
> > > > > > 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
> > > > > > ---
> > > > > > 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 ;)
signature.asc
Description: PGP signature
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
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
---
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.
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
> > > >
> > > > 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
> > > > ---
> > > >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().
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.
signature.asc
Description: PGP signature
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
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
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
---
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.
C.
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
On Nov 16 16:58, Cédric Le Goater wrote:
> On 11/16/22 09:43, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > 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
> > ---
> > 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.
signature.asc
Description: PGP signature
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
On 11/16/22 09:43, Klaus Jensen wrote:
From: Klaus Jensen
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
---
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() ?
Thanks,
C.
}
if (aspeed_i2c_bus_pkt_mode_en(bus)) {
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d4ba8146bffb..bed594fe599b 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool
is_recv)
void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
{
+I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+node->bh = bh;
+
+QSIMPLEQ_INSERT_TAIL(>pending_masters, node, entry);
+}
+
+void i2c_schedule_pending_master(I2CBus *bus)
+{
+I2CPendingMaster *node;
+
if (i2c_bus_busy(bus)) {
-I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
-node->bh = bh;
-
-QSIMPLEQ_INSERT_TAIL(>pending_masters, node, entry);
+/* someone is already controlling the bus; wait for it to release it */
+return;
+}
+if (QSIMPLEQ_EMPTY(>pending_masters)) {
return;
}
-bus->bh = bh;
+node = QSIMPLEQ_FIRST(>pending_masters);
+bus->bh = node->bh;
+
+QSIMPLEQ_REMOVE_HEAD(>pending_masters, entry);
+g_free(node);
+
qemu_bh_schedule(bus->bh);
}
void i2c_bus_release(I2CBus *bus)
{
bus->bh = NULL;
+
+i2c_schedule_pending_master(bus);
}
int i2c_start_recv(I2CBus *bus, uint8_t address)
@@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
g_free(node);
}
bus->broadcast = false;
-
-if (!QSIMPLEQ_EMPTY(>pending_masters)) {
-I2CPendingMaster *node = QSIMPLEQ_FIRST(>pending_masters);
-bus->bh = node->bh;
-
-QSIMPLEQ_REMOVE_HEAD(>pending_masters, entry);
-g_free(node);
-
-qemu_bh_schedule(bus->bh);
-}
}
int i2c_send(I2CBus *bus, uint8_t data)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b9581d23097..2a3abacd1ba6 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
*/
int i2c_start_send_async(I2CBus *bus, uint8_t address);
+void i2c_schedule_pending_master(I2CBus *bus);
+
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
void i2c_ack(I2CBus *bus);
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
On Wed, Nov 16, 2022 at 09:43:10AM +0100, Klaus Jensen wrote:
> From: Klaus Jensen
>
> 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.
>
Yes, I think this is correct.
Acked-by: Corey Minyard
Is there a reason you are thinking this is needed for 7.2? There's no
code in qemu proper that uses this yet. I had assumed that was coming
soon after the patch.
-corey
> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> Signed-off-by: Klaus Jensen
> ---
> 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);
> }
>
> if (aspeed_i2c_bus_pkt_mode_en(bus)) {
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index d4ba8146bffb..bed594fe599b 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address,
> bool is_recv)
>
> void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
> {
> +I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
> +node->bh = bh;
> +
> +QSIMPLEQ_INSERT_TAIL(>pending_masters, node, entry);
> +}
> +
> +void i2c_schedule_pending_master(I2CBus *bus)
> +{
> +I2CPendingMaster *node;
> +
> if (i2c_bus_busy(bus)) {
> -I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
> -node->bh = bh;
> -
> -QSIMPLEQ_INSERT_TAIL(>pending_masters, node, entry);
> +/* someone is already controlling the bus; wait for it to release it
> */
> +return;
> +}
>
> +if (QSIMPLEQ_EMPTY(>pending_masters)) {
> return;
> }
>
> -bus->bh = bh;
> +node = QSIMPLEQ_FIRST(>pending_masters);
> +bus->bh = node->bh;
> +
> +QSIMPLEQ_REMOVE_HEAD(>pending_masters, entry);
> +g_free(node);
> +
> qemu_bh_schedule(bus->bh);
> }
>
> void i2c_bus_release(I2CBus *bus)
> {
> bus->bh = NULL;
> +
> +i2c_schedule_pending_master(bus);
> }
>
> int i2c_start_recv(I2CBus *bus, uint8_t address)
> @@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
> g_free(node);
> }
> bus->broadcast = false;
> -
> -if (!QSIMPLEQ_EMPTY(>pending_masters)) {
> -I2CPendingMaster *node = QSIMPLEQ_FIRST(>pending_masters);
> -bus->bh = node->bh;
> -
> -QSIMPLEQ_REMOVE_HEAD(>pending_masters, entry);
> -g_free(node);
> -
> -qemu_bh_schedule(bus->bh);
> -}
> }
>
> int i2c_send(I2CBus *bus, uint8_t data)
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 9b9581d23097..2a3abacd1ba6 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
> */
> int i2c_start_send_async(I2CBus *bus, uint8_t address);
>
> +void i2c_schedule_pending_master(I2CBus *bus);
> +
> void i2c_end_transfer(I2CBus *bus);
> void i2c_nack(I2CBus *bus);
> void i2c_ack(I2CBus *bus);
> --
> 2.38.1
>
>
Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
On Wed, 16 Nov 2022 at 08:43, Klaus Jensen wrote:
>
> From: Klaus Jensen
>
> 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
If this is a bug fix we should consider for 7.2, you should
send it as a separate patch with a commit message that
describes the consequences of the bug (e.g. whether it
affects common workloads or if it's just an odd corner case).
As one patch in an otherwise RFC series it's going to get lost
otherwise.
thanks
-- PMM
[PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
From: Klaus Jensen
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
---
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);
}
if (aspeed_i2c_bus_pkt_mode_en(bus)) {
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d4ba8146bffb..bed594fe599b 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool
is_recv)
void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
{
+I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+node->bh = bh;
+
+QSIMPLEQ_INSERT_TAIL(>pending_masters, node, entry);
+}
+
+void i2c_schedule_pending_master(I2CBus *bus)
+{
+I2CPendingMaster *node;
+
if (i2c_bus_busy(bus)) {
-I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
-node->bh = bh;
-
-QSIMPLEQ_INSERT_TAIL(>pending_masters, node, entry);
+/* someone is already controlling the bus; wait for it to release it */
+return;
+}
+if (QSIMPLEQ_EMPTY(>pending_masters)) {
return;
}
-bus->bh = bh;
+node = QSIMPLEQ_FIRST(>pending_masters);
+bus->bh = node->bh;
+
+QSIMPLEQ_REMOVE_HEAD(>pending_masters, entry);
+g_free(node);
+
qemu_bh_schedule(bus->bh);
}
void i2c_bus_release(I2CBus *bus)
{
bus->bh = NULL;
+
+i2c_schedule_pending_master(bus);
}
int i2c_start_recv(I2CBus *bus, uint8_t address)
@@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
g_free(node);
}
bus->broadcast = false;
-
-if (!QSIMPLEQ_EMPTY(>pending_masters)) {
-I2CPendingMaster *node = QSIMPLEQ_FIRST(>pending_masters);
-bus->bh = node->bh;
-
-QSIMPLEQ_REMOVE_HEAD(>pending_masters, entry);
-g_free(node);
-
-qemu_bh_schedule(bus->bh);
-}
}
int i2c_send(I2CBus *bus, uint8_t data)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b9581d23097..2a3abacd1ba6 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
*/
int i2c_start_send_async(I2CBus *bus, uint8_t address);
+void i2c_schedule_pending_master(I2CBus *bus);
+
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
void i2c_ack(I2CBus *bus);
--
2.38.1
