Re: [PATCH v3 9/10] hw/char/pl011: Add transmit FIFO to PL011State

["Alex Bennée" <alex.bennee@linaro.org>]

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>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro