| Message ID | 20221116084312.35808-2-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/{i2c, nvme}: mctp endpoint, nvme management interface model | expand |
On Wed, 16 Nov 2022 at 08:43, Klaus Jensen <its@irrelevant.dk> 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> 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
On Wed, Nov 16, 2022 at 09:43:10AM +0100, 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. > Yes, I think this is correct. Acked-by: Corey Minyard <cminyard@mvista.com> 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 <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); > } > > 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(&bus->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(&bus->pending_masters, node, entry); > + /* someone is already controlling the bus; wait for it to release it */ > + return; > + } > > + if (QSIMPLEQ_EMPTY(&bus->pending_masters)) { > return; > } > > - bus->bh = bh; > + node = QSIMPLEQ_FIRST(&bus->pending_masters); > + bus->bh = node->bh; > + > + QSIMPLEQ_REMOVE_HEAD(&bus->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(&bus->pending_masters)) { > - I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters); > - bus->bh = node->bh; > - > - QSIMPLEQ_REMOVE_HEAD(&bus->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 > >
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() ? 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(&bus->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(&bus->pending_masters, node, entry); > + /* someone is already controlling the bus; wait for it to release it */ > + return; > + } > > + if (QSIMPLEQ_EMPTY(&bus->pending_masters)) { > return; > } > > - bus->bh = bh; > + node = QSIMPLEQ_FIRST(&bus->pending_masters); > + bus->bh = node->bh; > + > + QSIMPLEQ_REMOVE_HEAD(&bus->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(&bus->pending_masters)) { > - I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters); > - bus->bh = node->bh; > - > - QSIMPLEQ_REMOVE_HEAD(&bus->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);
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.
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. C.
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(). 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.
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.
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 ;)
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.
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.
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. 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 echo device should be fixed to better align with the api, like so: --- hw/misc/i2c-echo.c.orig 2022-11-22 09:35:00.478173652 +0100 +++ hw/misc/i2c-echo.c 2022-11-22 09:34:31.428174379 +0100 @@ -9,7 +9,6 @@ enum i2c_echo_state { I2C_ECHO_STATE_IDLE, - I2C_ECHO_STATE_REQUEST_MASTER, I2C_ECHO_STATE_START_SEND, I2C_ECHO_STATE_ACK, }; @@ -34,11 +33,6 @@ case I2C_ECHO_STATE_IDLE: return; - case I2C_ECHO_STATE_REQUEST_MASTER: - i2c_bus_acquire(state->bus, state->bh); - state->state = I2C_ECHO_STATE_START_SEND; - return; - case I2C_ECHO_STATE_START_SEND: if (i2c_start_send_async(state->bus, state->data[0])) { goto release_bus; @@ -85,8 +79,8 @@ case I2C_FINISH: state->pos = 0; - state->state = I2C_ECHO_STATE_REQUEST_MASTER; - qemu_bh_schedule(state->bh); + state->state = I2C_ECHO_STATE_START_SEND; + i2c_bus_acquire(state->bus, state->bh); break;
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(&bus->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(&bus->pending_masters, node, entry); + /* someone is already controlling the bus; wait for it to release it */ + return; + } + if (QSIMPLEQ_EMPTY(&bus->pending_masters)) { return; } - bus->bh = bh; + node = QSIMPLEQ_FIRST(&bus->pending_masters); + bus->bh = node->bh; + + QSIMPLEQ_REMOVE_HEAD(&bus->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(&bus->pending_masters)) { - I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters); - bus->bh = node->bh; - - QSIMPLEQ_REMOVE_HEAD(&bus->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);