| Message ID | 20231013141131.1531-10-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand |
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > In order to make the next commit easier to review, > introduce the transmit FIFO, but do not yet use it. might be worth mentioning the migration bits here as well. > > Uninline pl011_reset_tx_fifo(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/char/pl011.h | 2 ++ > hw/char/pl011.c | 35 +++++++++++++++++++++++++++++++++-- > 2 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h > index d853802132..20898f43a6 100644 > --- a/include/hw/char/pl011.h > +++ b/include/hw/char/pl011.h > @@ -18,6 +18,7 @@ > #include "hw/sysbus.h" > #include "chardev/char-fe.h" > #include "qom/object.h" > +#include "qemu/fifo8.h" > > #define TYPE_PL011 "pl011" > OBJECT_DECLARE_SIMPLE_TYPE(PL011State, PL011) > @@ -53,6 +54,7 @@ struct PL011State { > Clock *clk; > bool migrate_clk; > const unsigned char *id; > + Fifo8 xmit_fifo; > }; > > DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr); > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 727decd428..9d98bd8f9a 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -147,11 +147,13 @@ static inline void pl011_reset_rx_fifo(PL011State *s) > s->flags |= PL011_FLAG_RXFE; > } > > -static inline void pl011_reset_tx_fifo(PL011State *s) > +static void pl011_reset_tx_fifo(PL011State *s) > { > /* Reset FIFO flags */ > s->flags &= ~PL011_FLAG_TXFF; > s->flags |= PL011_FLAG_TXFE; > + > + fifo8_reset(&s->xmit_fifo); > } > > static void pl011_write_txdata(PL011State *s, uint8_t data) > @@ -436,6 +438,22 @@ static const VMStateDescription vmstate_pl011_clock = { > } > }; > > +static bool pl011_xmit_fifo_state_needed(void *opaque) > +{ > + return false; > +} > + > +static const VMStateDescription vmstate_pl011_xmit_fifo = { > + .name = "pl011/xmit_fifo", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = pl011_xmit_fifo_state_needed, > + .fields = (VMStateField[]) { > + VMSTATE_FIFO8(xmit_fifo, PL011State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static int pl011_post_load(void *opaque, int version_id) > { > PL011State* s = opaque; > @@ -487,7 +505,11 @@ static const VMStateDescription vmstate_pl011 = { > .subsections = (const VMStateDescription * []) { > &vmstate_pl011_clock, > NULL > - } > + }, > + .subsections = (const VMStateDescription * []) { > + &vmstate_pl011_xmit_fifo, > + NULL > + }, > }; Doesn't this necessitate the bumping of the migration version data or do we not worry about new -> old migrations? > > static Property pl011_properties[] = { > @@ -502,6 +524,7 @@ static void pl011_init(Object *obj) > PL011State *s = PL011(obj); > int i; > > + fifo8_create(&s->xmit_fifo, PL011_FIFO_DEPTH); > memory_region_init_io(&s->iomem, OBJECT(s), &pl011_ops, s, "pl011", 0x1000); > sysbus_init_mmio(sbd, &s->iomem); > for (i = 0; i < ARRAY_SIZE(s->irq); i++) { > @@ -514,6 +537,13 @@ static void pl011_init(Object *obj) > s->id = pl011_id_arm; > } > > +static void pl011_finalize(Object *obj) > +{ > + PL011State *s = PL011(obj); > + > + fifo8_destroy(&s->xmit_fifo); > +} > + > static void pl011_realize(DeviceState *dev, Error **errp) > { > PL011State *s = PL011(dev); > @@ -557,6 +587,7 @@ static const TypeInfo pl011_arm_info = { > .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(PL011State), > .instance_init = pl011_init, > + .instance_finalize = pl011_finalize, > .class_init = pl011_class_init, > }; Otherwise: Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On 10/13/23 10:05, Alex Bennée wrote: >> @@ -487,7 +505,11 @@ static const VMStateDescription vmstate_pl011 = { >> .subsections = (const VMStateDescription * []) { >> &vmstate_pl011_clock, >> NULL >> - } >> + }, >> + .subsections = (const VMStateDescription * []) { >> + &vmstate_pl011_xmit_fifo, >> + NULL >> + }, >> }; > > Doesn't this necessitate the bumping of the migration version data or > do we not worry about new -> old migrations? We usually don't care about new->old, however: If the fifo is empty, migration will still work because of the subsection. If the fifo is not empty... I think the subsection will be ignored, with the only consequence being that some characters will be dropped. r~
diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h index d853802132..20898f43a6 100644 --- a/include/hw/char/pl011.h +++ b/include/hw/char/pl011.h @@ -18,6 +18,7 @@ #include "hw/sysbus.h" #include "chardev/char-fe.h" #include "qom/object.h" +#include "qemu/fifo8.h" #define TYPE_PL011 "pl011" OBJECT_DECLARE_SIMPLE_TYPE(PL011State, PL011) @@ -53,6 +54,7 @@ struct PL011State { Clock *clk; bool migrate_clk; const unsigned char *id; + Fifo8 xmit_fifo; }; DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr); diff --git a/hw/char/pl011.c b/hw/char/pl011.c index 727decd428..9d98bd8f9a 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -147,11 +147,13 @@ static inline void pl011_reset_rx_fifo(PL011State *s) s->flags |= PL011_FLAG_RXFE; } -static inline void pl011_reset_tx_fifo(PL011State *s) +static void pl011_reset_tx_fifo(PL011State *s) { /* Reset FIFO flags */ s->flags &= ~PL011_FLAG_TXFF; s->flags |= PL011_FLAG_TXFE; + + fifo8_reset(&s->xmit_fifo); } static void pl011_write_txdata(PL011State *s, uint8_t data) @@ -436,6 +438,22 @@ static const VMStateDescription vmstate_pl011_clock = { } }; +static bool pl011_xmit_fifo_state_needed(void *opaque) +{ + return false; +} + +static const VMStateDescription vmstate_pl011_xmit_fifo = { + .name = "pl011/xmit_fifo", + .version_id = 1, + .minimum_version_id = 1, + .needed = pl011_xmit_fifo_state_needed, + .fields = (VMStateField[]) { + VMSTATE_FIFO8(xmit_fifo, PL011State), + VMSTATE_END_OF_LIST() + } +}; + static int pl011_post_load(void *opaque, int version_id) { PL011State* s = opaque; @@ -487,7 +505,11 @@ static const VMStateDescription vmstate_pl011 = { .subsections = (const VMStateDescription * []) { &vmstate_pl011_clock, NULL - } + }, + .subsections = (const VMStateDescription * []) { + &vmstate_pl011_xmit_fifo, + NULL + }, }; static Property pl011_properties[] = { @@ -502,6 +524,7 @@ static void pl011_init(Object *obj) PL011State *s = PL011(obj); int i; + fifo8_create(&s->xmit_fifo, PL011_FIFO_DEPTH); memory_region_init_io(&s->iomem, OBJECT(s), &pl011_ops, s, "pl011", 0x1000); sysbus_init_mmio(sbd, &s->iomem); for (i = 0; i < ARRAY_SIZE(s->irq); i++) { @@ -514,6 +537,13 @@ static void pl011_init(Object *obj) s->id = pl011_id_arm; } +static void pl011_finalize(Object *obj) +{ + PL011State *s = PL011(obj); + + fifo8_destroy(&s->xmit_fifo); +} + static void pl011_realize(DeviceState *dev, Error **errp) { PL011State *s = PL011(dev); @@ -557,6 +587,7 @@ static const TypeInfo pl011_arm_info = { .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(PL011State), .instance_init = pl011_init, + .instance_finalize = pl011_finalize, .class_init = pl011_class_init, };
In order to make the next commit easier to review, introduce the transmit FIFO, but do not yet use it. Uninline pl011_reset_tx_fifo(). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/char/pl011.h | 2 ++ hw/char/pl011.c | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-)