[v5,09/15] hw: Added generic FIFO API.

Added a FIFO API that can be used to create and operate byte FIFOs.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/Makefile.objs |    1 +
 hw/fifo.c        |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/fifo.h        |   47 ++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100644 hw/fifo.c
 create mode 100644 hw/fifo.h

On 08/06/2012 06:16 AM, Peter A. G. Crosthwaite wrote:
> Added a FIFO API that can be used to create and operate byte FIFOs.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>   hw/Makefile.objs |    1 +
>   hw/fifo.c        |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/fifo.h        |   47 ++++++++++++++++++++++++++++++++
>   3 files changed, 127 insertions(+), 0 deletions(-)
>   create mode 100644 hw/fifo.c
>   create mode 100644 hw/fifo.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 8327e55..6ba570e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -15,6 +15,7 @@ hw-obj-$(CONFIG_ECC) += ecc.o
>   hw-obj-$(CONFIG_NAND) += nand.o
>   hw-obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o
>   hw-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
> +hw-obj-y += fifo.o
Perhaps it'd be better to make it common object and put it into root 
directory, like its done for bitops.c and bitmap.c
>   
>   hw-obj-$(CONFIG_M48T59) += m48t59.o
>   hw-obj-$(CONFIG_ESCC) += escc.o
> diff --git a/hw/fifo.c b/hw/fifo.c
> new file mode 100644
> index 0000000..5e14e1e
> --- /dev/null
> +++ b/hw/fifo.c
> @@ -0,0 +1,79 @@
> +/*
> + * Generic FIFO component, implemented as a circular buffer.
> + *
> + * Copyright (c) 2012 Peter A. G. Crosthwaite
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "fifo.h"
> +
> +void fifo8_create(Fifo8 *fifo, uint32_t capacity)
> +{
> +    fifo->data = g_new(uint8_t, capacity);
> +    fifo->capacity = capacity;
> +    fifo->head = 0;
> +    fifo->num = 0;
> +}
> +
> +void fifo8_destroy(Fifo8 *fifo)
> +{
> +    g_free(fifo->data);
> +}
> +
> +void fifo8_push(Fifo8 *fifo, uint8_t data)
> +{
> +    if (fifo->num == fifo->capacity) {
> +        abort();
> +    }
I think its too harsh to abort here (and in pop() too), fifo 
overrun/underrun condition is absolutely normal for most of the devices, 
usually it would just trigger an  interrupt. I suggest return a error 
code instead and let a caller decide what should happen in this situation.
> +    fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
> +    fifo->num++;
> +}
> +
> +uint8_t fifo8_pop(Fifo8 *fifo)
> +{
> +    uint8_t ret;
> +
> +    if (fifo->num == 0) {
> +        abort();
> +    }
> +    ret = fifo->data[fifo->head++];
> +    fifo->head %= fifo->capacity;
> +    fifo->num--;
> +    return ret;
> +}
> +
> +void fifo8_reset(Fifo8 *fifo)
> +{
> +    fifo->num = 0;
> +}
> +
> +bool fifo8_is_empty(Fifo8 *fifo)
> +{
> +    return (fifo->num == 0);
> +}
> +
> +bool fifo8_is_full(Fifo8 *fifo)
> +{
> +    return (fifo->num == fifo->capacity);
> +}
> +
> +const VMStateDescription vmstate_fifo8 = {
> +    .name = "SSISlave",
thats not a good name for a generic fifo)
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
too much spaces here
> +        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, 0, capacity),
> +        VMSTATE_UINT32(head, Fifo8),
> +        VMSTATE_UINT32(num, Fifo8),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> diff --git a/hw/fifo.h b/hw/fifo.h
> new file mode 100644
> index 0000000..3fb09ff
> --- /dev/null
> +++ b/hw/fifo.h
> @@ -0,0 +1,47 @@
> +#ifndef FIFO_H
> +#define FIFO_H
> +
> +#include "hw.h"
> +
> +typedef struct {
> +    /* All fields are private */
> +    uint8_t *data;
> +    uint32_t capacity;
> +    uint32_t head;
> +    uint32_t num;
> +} Fifo8;
> +
> +/* create a fifo of the specified size */
> +
> +void fifo8_create(Fifo8 *, uint32_t);
> +
> +/* cleanup a fifo */
> +
> +void fifo8_destroy(Fifo8 *);
> +
> +/* push a data byte to the fifo. Behaviour is undefined if the fifo is full */
> +
> +void fifo8_push(Fifo8 *, uint8_t);
> +
> +/* pop a data byte from the fifo. Behviour is undefined if the fifo is empty */
> +
> +uint8_t fifo8_pop(Fifo8 *);
> +
> +/* reset (empty) the fifo */
> +
> +void fifo8_reset(Fifo8 *);
> +
> +bool fifo8_is_empty(Fifo8 *);
> +bool fifo8_is_full(Fifo8 *);
> +
> +extern const VMStateDescription vmstate_fifo8;
> +
> +#define VMSTATE_FIFO8(_field, _state) {                              \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(Fifo8),                                     \
> +    .vmsd       = &vmstate_fifo8,                                    \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, Fifo8),       \
> +}
> +
> +#endif /* FIFO_H */

On 6 August 2012 03:16, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Added a FIFO API that can be used to create and operate byte FIFOs.

I'm not asking for actual conversions, but it would be nice to see a
list of some devices that could in principle be moved to using this FIFO,
as an indication of its general utility.

Would it make sense for the FIFO to be a QOM object, or is that a
silly idea?

-- PMM

On 08/06/2012 06:16 AM, Peter A. G. Crosthwaite wrote:
> Added a FIFO API that can be used to create and operate byte FIFOs.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>   hw/Makefile.objs |    1 +
>   hw/fifo.c        |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/fifo.h        |   47 ++++++++++++++++++++++++++++++++
>   3 files changed, 127 insertions(+), 0 deletions(-)
>   create mode 100644 hw/fifo.c
>   create mode 100644 hw/fifo.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 8327e55..6ba570e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -15,6 +15,7 @@ hw-obj-$(CONFIG_ECC) += ecc.o
>   hw-obj-$(CONFIG_NAND) += nand.o
>   hw-obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o
>   hw-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
> +hw-obj-y += fifo.o
>   
>   hw-obj-$(CONFIG_M48T59) += m48t59.o
>   hw-obj-$(CONFIG_ESCC) += escc.o
> diff --git a/hw/fifo.c b/hw/fifo.c
> new file mode 100644
> index 0000000..5e14e1e
> --- /dev/null
> +++ b/hw/fifo.c
> @@ -0,0 +1,79 @@
> +/*
> + * Generic FIFO component, implemented as a circular buffer.
> + *
> + * Copyright (c) 2012 Peter A. G. Crosthwaite
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "fifo.h"
> +
> +void fifo8_create(Fifo8 *fifo, uint32_t capacity)
> +{
> +    fifo->data = g_new(uint8_t, capacity);
> +    fifo->capacity = capacity;
> +    fifo->head = 0;
> +    fifo->num = 0;
> +}
> +
> +void fifo8_destroy(Fifo8 *fifo)
> +{
> +    g_free(fifo->data);
> +}
> +
> +void fifo8_push(Fifo8 *fifo, uint8_t data)
> +{
> +    if (fifo->num == fifo->capacity) {
> +        abort();
> +    }
> +    fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
> +    fifo->num++;
> +}
> +
> +uint8_t fifo8_pop(Fifo8 *fifo)
> +{
> +    uint8_t ret;
> +
> +    if (fifo->num == 0) {
> +        abort();
> +    }
> +    ret = fifo->data[fifo->head++];
> +    fifo->head %= fifo->capacity;
> +    fifo->num--;
> +    return ret;
> +}
> +
> +void fifo8_reset(Fifo8 *fifo)
> +{
> +    fifo->num = 0;
> +}
> +
> +bool fifo8_is_empty(Fifo8 *fifo)
> +{
> +    return (fifo->num == 0);
> +}
> +
> +bool fifo8_is_full(Fifo8 *fifo)
> +{
> +    return (fifo->num == fifo->capacity);
> +}
> +
> +const VMStateDescription vmstate_fifo8 = {
> +    .name = "SSISlave",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, 0, capacity),
> +        VMSTATE_UINT32(head, Fifo8),
> +        VMSTATE_UINT32(num, Fifo8),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> diff --git a/hw/fifo.h b/hw/fifo.h
> new file mode 100644
> index 0000000..3fb09ff
> --- /dev/null
> +++ b/hw/fifo.h
> @@ -0,0 +1,47 @@
> +#ifndef FIFO_H
> +#define FIFO_H
> +
> +#include "hw.h"
> +
> +typedef struct {
> +    /* All fields are private */
> +    uint8_t *data;
> +    uint32_t capacity;
> +    uint32_t head;
> +    uint32_t num;
> +} Fifo8;
> +
> +/* create a fifo of the specified size */
> +
> +void fifo8_create(Fifo8 *, uint32_t);
> +
> +/* cleanup a fifo */
> +
> +void fifo8_destroy(Fifo8 *);
> +
> +/* push a data byte to the fifo. Behaviour is undefined if the fifo is full */
> +
> +void fifo8_push(Fifo8 *, uint8_t);
> +
> +/* pop a data byte from the fifo. Behviour is undefined if the fifo is empty */
> +
> +uint8_t fifo8_pop(Fifo8 *);
> +
> +/* reset (empty) the fifo */
> +
> +void fifo8_reset(Fifo8 *);
> +
> +bool fifo8_is_empty(Fifo8 *);
> +bool fifo8_is_full(Fifo8 *);
> +
> +extern const VMStateDescription vmstate_fifo8;
> +
> +#define VMSTATE_FIFO8(_field, _state) {                              \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(Fifo8),                                     \
> +    .vmsd       = &vmstate_fifo8,                                    \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, Fifo8),       \
> +}

how about implementing this as a wrapper to VMSTATE_STRUCT_TEST() macro 
instead? And maybe this should go to vmstate.h header

> +
> +#endif /* FIFO_H */

On 08/06/2012 01:48 PM, Peter Maydell wrote:
> On 6 August 2012 03:16, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> Added a FIFO API that can be used to create and operate byte FIFOs.
> I'm not asking for actual conversions, but it would be nice to see a
> list of some devices that could in principle be moved to using this FIFO,
> as an indication of its general utility.
>
> Would it make sense for the FIFO to be a QOM object, or is that a
> silly idea?
>
> -- PMM
>
FIFO introspection capability could be useful I think, and we could 
implement device-specific fifo "mutants" then (for example, PL330 fifo 
could be a general FIFO object + "tag" variable).

On Mon, Aug 6, 2012 at 7:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 August 2012 03:16, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> Added a FIFO API that can be used to create and operate byte FIFOs.
>
> I'm not asking for actual conversions, but it would be nice to see a
> list of some devices that could in principle be moved to using this FIFO,
> as an indication of its general utility.
>

The two device models in this series. mcf_uart.c, strongarm.c
(StrongARMUARTState), xilinx_uartlite, cadence_uart.c, sh_serial.c,
tsc210x.c to name the ones Ive just had a brief look over. There are
more. grep -i fifo hw | grep uint8_t gives you a short list of files
to look for potential clients.

Dominated by uarts as they are the ones that need 8-bit fifos.
Usability is greatly expanded if the fifo can be an arbitrary data
type (uint32_t, uint16_t, struct foo), but that is a much harder
problem to solve that id like to say is out of scope of this series. I
think this is what Igor is getting at RE his PL330 comments on his
continuation of this thread. Can we get the ball rolling with uint8_t
then talk about generalisation strategy?

> Would it make sense for the FIFO to be a QOM object, or is that a
> silly idea?

There could be merit in making the data entries some form of QOM
object. It would hurt performance, but could solve the arbitrary data
problem.

Regards,
Peter

>
> -- PMM

>> +
>> +extern const VMStateDescription vmstate_fifo8;
>> +
>> +#define VMSTATE_FIFO8(_field, _state) {                              \
>> +    .name       = (stringify(_field)),                               \
>> +    .size       = sizeof(Fifo8),                                     \
>> +    .vmsd       = &vmstate_fifo8,                                    \
>> +    .flags      = VMS_STRUCT,                                        \
>> +    .offset     = vmstate_offset_value(_state, _field, Fifo8),       \
>> +}
>
>
> how about implementing this as a wrapper to VMSTATE_STRUCT_TEST() macro
> instead?

This has no existing precedent in QEMU so I am unsure of what you mean?

And maybe this should go to vmstate.h header

I disagree. All other clients of VMS_STRUCT are out in their repective
device specific headers (pci.h, i2c.h) etc. Unless this is new
established policy, I dont really want to change the current adopted
approach.

Regards,
Peter

>
>
>> +
>> +#endif /* FIFO_H */
>
>

On 08/07/2012 10:10 AM, Peter Crosthwaite wrote:
>>> +
>>> +extern const VMStateDescription vmstate_fifo8;
>>> +
>>> +#define VMSTATE_FIFO8(_field, _state) {                              \
>>> +    .name       = (stringify(_field)),                               \
>>> +    .size       = sizeof(Fifo8),                                     \
>>> +    .vmsd       = &vmstate_fifo8,                                    \
>>> +    .flags      = VMS_STRUCT,                                        \
>>> +    .offset     = vmstate_offset_value(_state, _field, Fifo8),       \
>>> +}
>>
>> how about implementing this as a wrapper to VMSTATE_STRUCT_TEST() macro
>> instead?
> This has no existing precedent in QEMU so I am unsure of what you mean?

I meant VMSTATE_TIMER_TEST() in vmstate.h as an example, which is a 
wrapper to VMSTATE_POINTER_TEST(). With this approach, fifo macro could be

#define VMSTATE_FIFO8(_field, _state) \
VMSTATE_STRUCT(_field, _state, 0, vmstate_fifo8, Fifo8)


>
> And maybe this should go to vmstate.h header
>
> I disagree. All other clients of VMS_STRUCT are out in their repective
> device specific headers (pci.h, i2c.h) etc. Unless this is new
> established policy, I dont really want to change the current adopted
> approach.

Yeah, looks like you're right.

> Regards,
> Peter
>
>>
>>> +
>>> +#endif /* FIFO_H */
>>

On Tue, Aug 7, 2012 at 4:28 PM, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 08/07/2012 10:10 AM, Peter Crosthwaite wrote:
>>>>
>>>> +
>>>> +extern const VMStateDescription vmstate_fifo8;
>>>> +
>>>> +#define VMSTATE_FIFO8(_field, _state) {                              \
>>>> +    .name       = (stringify(_field)),                               \
>>>> +    .size       = sizeof(Fifo8),                                     \
>>>> +    .vmsd       = &vmstate_fifo8,                                    \
>>>> +    .flags      = VMS_STRUCT,                                        \
>>>> +    .offset     = vmstate_offset_value(_state, _field, Fifo8),       \
>>>> +}
>>>
>>>
>>> how about implementing this as a wrapper to VMSTATE_STRUCT_TEST() macro
>>> instead?
>>
>> This has no existing precedent in QEMU so I am unsure of what you mean?
>
>
> I meant VMSTATE_TIMER_TEST() in vmstate.h as an example, which is a wrapper
> to VMSTATE_POINTER_TEST(). With this approach, fifo macro could be
>
> #define VMSTATE_FIFO8(_field, _state) \
> VMSTATE_STRUCT(_field, _state, 0, vmstate_fifo8, Fifo8)
>

Yeh just greppin around it looks like this is functionally equivalent.
VMSTATE_IDE_BUS looks like a reasonable precedent for that. Any
opinions one way or the other RE with my original approach (based on
I2C) vs Igors shortened version?

Regards,
Peter

>
>
>>
>> And maybe this should go to vmstate.h header
>>
>> I disagree. All other clients of VMS_STRUCT are out in their repective
>> device specific headers (pci.h, i2c.h) etc. Unless this is new
>> established policy, I dont really want to change the current adopted
>> approach.
>
>
> Yeah, looks like you're right.
>
>
>> Regards,
>> Peter
>>
>>>
>>>> +
>>>> +#endif /* FIFO_H */
>>>
>>>
>