Message ID | 81b9e406e0b0292ccd3168385ab6a73a78a0ada5.1344218410.git.peter.crosthwaite@petalogix.com |
---|---|
State | New |
Headers | show |
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 */ >>> >>> >
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), \ +} + +#endif /* FIFO_H */
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