| Message ID | 1327597395-9527-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 26, 2012 at 17:03, Fabien Chouteau <chouteau@adacore.com> wrote: > This patch implements the RX channel of GRLIB UART with a FIFO to > improve data rate. > > Signed-off-by: Fabien Chouteau <chouteau@adacore.com> > --- > hw/grlib_apbuart.c | 106 +++++++++++++++++++++++++++++++++++++++++++-------- > trace-events | 1 + > 2 files changed, 90 insertions(+), 17 deletions(-) > > diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c > index f8a64e1..51406c6 100644 > --- a/hw/grlib_apbuart.c > +++ b/hw/grlib_apbuart.c > @@ -24,7 +24,6 @@ > > #include "sysbus.h" > #include "qemu-char.h" > -#include "ptimer.h" > > #include "trace.h" > > @@ -66,6 +65,8 @@ > #define SCALER_OFFSET 0x0C /* not supported */ > #define FIFO_DEBUG_OFFSET 0x10 /* not supported */ > > +#define FIFO_LENGTH 1024 > + > typedef struct UART { > SysBusDevice busdev; > MemoryRegion iomem; > @@ -77,21 +78,67 @@ typedef struct UART { > uint32_t receive; > uint32_t status; > uint32_t control; > + > + /* FIFO */ > + char buffer[FIFO_LENGTH]; > + int len; > + int current; > } UART; > > +static int uart_data_to_read(UART *uart) > +{ > + return uart->current < uart->len; > +} > + > +static char uart_pop(UART *uart) > +{ > + char ret; > + > + if (uart->len == 0) { > + uart->status &= ~UART_DATA_READY; > + return 0; > + } > + > + ret = uart->buffer[uart->current++]; > + > + if (uart->current >= uart->len) { > + /* Flush */ > + uart->len = 0; > + uart->current = 0; > + } > + > + if (!uart_data_to_read(uart)) { > + uart->status &= ~UART_DATA_READY; > + } > + > + return ret; > +} > + > +static void uart_add_to_fifo(UART *uart, > + const uint8_t *buffer, > + int length) > +{ > + if (uart->len + length > FIFO_LENGTH) { > + abort(); A guest could trigger this abort(), which is not OK. I think you can just return. > + } > + memcpy(uart->buffer + uart->len, buffer, length); > + uart->len += length; > +} > + > static int grlib_apbuart_can_receive(void *opaque) > { > UART *uart = opaque; > > - return !!(uart->status & UART_DATA_READY); > + return FIFO_LENGTH - uart->len; > } > > static void grlib_apbuart_receive(void *opaque, const uint8_t *buf, int size) > { > UART *uart = opaque; > > - uart->receive = *buf; > - uart->status |= UART_DATA_READY; > + uart_add_to_fifo(uart, buf, size); > + > + uart->status |= UART_DATA_READY; > > if (uart->control & UART_RECEIVE_INTERRUPT) { > qemu_irq_pulse(uart->irq); > @@ -103,9 +150,39 @@ static void grlib_apbuart_event(void *opaque, int event) > trace_grlib_apbuart_event(event); > } > > -static void > -grlib_apbuart_write(void *opaque, target_phys_addr_t addr, > - uint64_t value, unsigned size) > + > +static uint64_t grlib_apbuart_read(void *opaque, target_phys_addr_t addr, > + unsigned size) > +{ > + UART *uart = opaque; > + > + addr &= 0xff; > + > + /* Unit registers */ > + switch (addr) { > + case DATA_OFFSET: > + case DATA_OFFSET + 3: /* when only one byte read */ > + return uart_pop(uart); > + > + case STATUS_OFFSET: > + /* Read Only */ > + return uart->status; > + > + case CONTROL_OFFSET: > + return uart->control; > + > + case SCALER_OFFSET: > + /* Not supported */ > + return 0; > + > + default: > + trace_grlib_apbuart_readl_unknown(addr); > + return 0; > + } > +} > + > +static void grlib_apbuart_write(void *opaque, target_phys_addr_t addr, > + uint64_t value, unsigned size) > { > UART *uart = opaque; > unsigned char c = 0; > @@ -115,6 +192,7 @@ grlib_apbuart_write(void *opaque, target_phys_addr_t addr, > /* Unit registers */ > switch (addr) { > case DATA_OFFSET: > + case DATA_OFFSET + 3: /* When only one byte write */ > c = value & 0xFF; > qemu_chr_fe_write(uart->chr, &c, 1); > return; > @@ -124,7 +202,7 @@ grlib_apbuart_write(void *opaque, target_phys_addr_t addr, > return; > > case CONTROL_OFFSET: > - /* Not supported */ > + uart->control = value; > return; > > case SCALER_OFFSET: > @@ -138,21 +216,15 @@ grlib_apbuart_write(void *opaque, target_phys_addr_t addr, > trace_grlib_apbuart_writel_unknown(addr, value); > } > > -static bool grlib_apbuart_accepts(void *opaque, target_phys_addr_t addr, > - unsigned size, bool is_write) > -{ > - return is_write && size == 4; > -} > - > static const MemoryRegionOps grlib_apbuart_ops = { > - .write = grlib_apbuart_write, > - .valid.accepts = grlib_apbuart_accepts, > + .write = grlib_apbuart_write, > + .read = grlib_apbuart_read, > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > static int grlib_apbuart_init(SysBusDevice *dev) > { > - UART *uart = FROM_SYSBUS(typeof(*uart), dev); > + UART *uart = FROM_SYSBUS(typeof(*uart), dev); > > qemu_chr_add_handlers(uart->chr, > grlib_apbuart_can_receive, > diff --git a/trace-events b/trace-events > index d2b0c61..5bb8bfb 100644 > --- a/trace-events > +++ b/trace-events > @@ -340,6 +340,7 @@ grlib_irqmp_writel_unknown(uint64_t addr, uint32_t value) "addr 0x%"PRIx64" valu > # hw/grlib_apbuart.c > grlib_apbuart_event(int event) "event:%d" > grlib_apbuart_writel_unknown(uint64_t addr, uint32_t value) "addr 0x%"PRIx64" value 0x%x" > +grlib_apbuart_readl_unknown(uint64_t addr) "addr 0x%"PRIx64"" > > # hw/leon3.c > leon3_set_irq(int intno) "Set CPU IRQ %d" > -- > 1.7.4.1 >
On 28/01/2012 13:20, Blue Swirl wrote: > On Thu, Jan 26, 2012 at 17:03, Fabien Chouteau <chouteau@adacore.com> wrote: >> This patch implements the RX channel of GRLIB UART with a FIFO to >> improve data rate. >> >> Signed-off-by: Fabien Chouteau <chouteau@adacore.com> >> --- >> hw/grlib_apbuart.c | 106 +++++++++++++++++++++++++++++++++++++++++++-------- >> trace-events | 1 + >> 2 files changed, 90 insertions(+), 17 deletions(-) >> >> diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c >> index f8a64e1..51406c6 100644 >> --- a/hw/grlib_apbuart.c >> +++ b/hw/grlib_apbuart.c >> @@ -24,7 +24,6 @@ >> >> #include "sysbus.h" >> #include "qemu-char.h" >> -#include "ptimer.h" >> >> #include "trace.h" >> >> @@ -66,6 +65,8 @@ >> #define SCALER_OFFSET 0x0C /* not supported */ >> #define FIFO_DEBUG_OFFSET 0x10 /* not supported */ >> >> +#define FIFO_LENGTH 1024 >> + >> typedef struct UART { >> SysBusDevice busdev; >> MemoryRegion iomem; >> @@ -77,21 +78,67 @@ typedef struct UART { >> uint32_t receive; >> uint32_t status; >> uint32_t control; >> + >> + /* FIFO */ >> + char buffer[FIFO_LENGTH]; >> + int len; >> + int current; >> } UART; >> >> +static int uart_data_to_read(UART *uart) >> +{ >> + return uart->current < uart->len; >> +} >> + >> +static char uart_pop(UART *uart) >> +{ >> + char ret; >> + >> + if (uart->len == 0) { >> + uart->status &= ~UART_DATA_READY; >> + return 0; >> + } >> + >> + ret = uart->buffer[uart->current++]; >> + >> + if (uart->current >= uart->len) { >> + /* Flush */ >> + uart->len = 0; >> + uart->current = 0; >> + } >> + >> + if (!uart_data_to_read(uart)) { >> + uart->status &= ~UART_DATA_READY; >> + } >> + >> + return ret; >> +} >> + >> +static void uart_add_to_fifo(UART *uart, >> + const uint8_t *buffer, >> + int length) >> +{ >> + if (uart->len + length > FIFO_LENGTH) { >> + abort(); > > A guest could trigger this abort(), which is not OK. I think you can > just return. > This will abort if Qemu sends more bytes than the number requested in grlib_apbuart_can_receive, so this would be a failure from Qemu not the guest. Regards,
On Mon, Jan 30, 2012 at 09:22, Fabien Chouteau <chouteau@adacore.com> wrote: > On 28/01/2012 13:20, Blue Swirl wrote: >> On Thu, Jan 26, 2012 at 17:03, Fabien Chouteau <chouteau@adacore.com> wrote: >>> This patch implements the RX channel of GRLIB UART with a FIFO to >>> improve data rate. >>> >>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com> >>> --- >>> hw/grlib_apbuart.c | 106 +++++++++++++++++++++++++++++++++++++++++++-------- >>> trace-events | 1 + >>> 2 files changed, 90 insertions(+), 17 deletions(-) >>> >>> diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c >>> index f8a64e1..51406c6 100644 >>> --- a/hw/grlib_apbuart.c >>> +++ b/hw/grlib_apbuart.c >>> @@ -24,7 +24,6 @@ >>> >>> #include "sysbus.h" >>> #include "qemu-char.h" >>> -#include "ptimer.h" >>> >>> #include "trace.h" >>> >>> @@ -66,6 +65,8 @@ >>> #define SCALER_OFFSET 0x0C /* not supported */ >>> #define FIFO_DEBUG_OFFSET 0x10 /* not supported */ >>> >>> +#define FIFO_LENGTH 1024 >>> + >>> typedef struct UART { >>> SysBusDevice busdev; >>> MemoryRegion iomem; >>> @@ -77,21 +78,67 @@ typedef struct UART { >>> uint32_t receive; >>> uint32_t status; >>> uint32_t control; >>> + >>> + /* FIFO */ >>> + char buffer[FIFO_LENGTH]; >>> + int len; >>> + int current; >>> } UART; >>> >>> +static int uart_data_to_read(UART *uart) >>> +{ >>> + return uart->current < uart->len; >>> +} >>> + >>> +static char uart_pop(UART *uart) >>> +{ >>> + char ret; >>> + >>> + if (uart->len == 0) { >>> + uart->status &= ~UART_DATA_READY; >>> + return 0; >>> + } >>> + >>> + ret = uart->buffer[uart->current++]; >>> + >>> + if (uart->current >= uart->len) { >>> + /* Flush */ >>> + uart->len = 0; >>> + uart->current = 0; >>> + } >>> + >>> + if (!uart_data_to_read(uart)) { >>> + uart->status &= ~UART_DATA_READY; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +static void uart_add_to_fifo(UART *uart, >>> + const uint8_t *buffer, >>> + int length) >>> +{ >>> + if (uart->len + length > FIFO_LENGTH) { >>> + abort(); >> >> A guest could trigger this abort(), which is not OK. I think you can >> just return. >> > > This will abort if Qemu sends more bytes than the number requested in > grlib_apbuart_can_receive, so this would be a failure from Qemu not the > guest. OK. Thanks, applied. > Regards, > > -- > Fabien Chouteau
diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c index f8a64e1..51406c6 100644 --- a/hw/grlib_apbuart.c +++ b/hw/grlib_apbuart.c @@ -24,7 +24,6 @@ #include "sysbus.h" #include "qemu-char.h" -#include "ptimer.h" #include "trace.h" @@ -66,6 +65,8 @@ #define SCALER_OFFSET 0x0C /* not supported */ #define FIFO_DEBUG_OFFSET 0x10 /* not supported */ +#define FIFO_LENGTH 1024 + typedef struct UART { SysBusDevice busdev; MemoryRegion iomem; @@ -77,21 +78,67 @@ typedef struct UART { uint32_t receive; uint32_t status; uint32_t control; + + /* FIFO */ + char buffer[FIFO_LENGTH]; + int len; + int current; } UART; +static int uart_data_to_read(UART *uart) +{ + return uart->current < uart->len; +} + +static char uart_pop(UART *uart) +{ + char ret; + + if (uart->len == 0) { + uart->status &= ~UART_DATA_READY; + return 0; + } + + ret = uart->buffer[uart->current++]; + + if (uart->current >= uart->len) { + /* Flush */ + uart->len = 0; + uart->current = 0; + } + + if (!uart_data_to_read(uart)) { + uart->status &= ~UART_DATA_READY; + } + + return ret; +} + +static void uart_add_to_fifo(UART *uart, + const uint8_t *buffer, + int length) +{ + if (uart->len + length > FIFO_LENGTH) { + abort(); + } + memcpy(uart->buffer + uart->len, buffer, length); + uart->len += length; +} + static int grlib_apbuart_can_receive(void *opaque) { UART *uart = opaque; - return !!(uart->status & UART_DATA_READY); + return FIFO_LENGTH - uart->len; } static void grlib_apbuart_receive(void *opaque, const uint8_t *buf, int size) { UART *uart = opaque; - uart->receive = *buf; - uart->status |= UART_DATA_READY; + uart_add_to_fifo(uart, buf, size); + + uart->status |= UART_DATA_READY; if (uart->control & UART_RECEIVE_INTERRUPT) { qemu_irq_pulse(uart->irq); @@ -103,9 +150,39 @@ static void grlib_apbuart_event(void *opaque, int event) trace_grlib_apbuart_event(event); } -static void -grlib_apbuart_write(void *opaque, target_phys_addr_t addr, - uint64_t value, unsigned size) + +static uint64_t grlib_apbuart_read(void *opaque, target_phys_addr_t addr, + unsigned size) +{ + UART *uart = opaque; + + addr &= 0xff; + + /* Unit registers */ + switch (addr) { + case DATA_OFFSET: + case DATA_OFFSET + 3: /* when only one byte read */ + return uart_pop(uart); + + case STATUS_OFFSET: + /* Read Only */ + return uart->status; + + case CONTROL_OFFSET: + return uart->control; + + case SCALER_OFFSET: + /* Not supported */ + return 0; + + default: + trace_grlib_apbuart_readl_unknown(addr); + return 0; + } +} + +static void grlib_apbuart_write(void *opaque, target_phys_addr_t addr, + uint64_t value, unsigned size) { UART *uart = opaque; unsigned char c = 0; @@ -115,6 +192,7 @@ grlib_apbuart_write(void *opaque, target_phys_addr_t addr, /* Unit registers */ switch (addr) { case DATA_OFFSET: + case DATA_OFFSET + 3: /* When only one byte write */ c = value & 0xFF; qemu_chr_fe_write(uart->chr, &c, 1); return; @@ -124,7 +202,7 @@ grlib_apbuart_write(void *opaque, target_phys_addr_t addr, return; case CONTROL_OFFSET: - /* Not supported */ + uart->control = value; return; case SCALER_OFFSET: @@ -138,21 +216,15 @@ grlib_apbuart_write(void *opaque, target_phys_addr_t addr, trace_grlib_apbuart_writel_unknown(addr, value); } -static bool grlib_apbuart_accepts(void *opaque, target_phys_addr_t addr, - unsigned size, bool is_write) -{ - return is_write && size == 4; -} - static const MemoryRegionOps grlib_apbuart_ops = { - .write = grlib_apbuart_write, - .valid.accepts = grlib_apbuart_accepts, + .write = grlib_apbuart_write, + .read = grlib_apbuart_read, .endianness = DEVICE_NATIVE_ENDIAN, }; static int grlib_apbuart_init(SysBusDevice *dev) { - UART *uart = FROM_SYSBUS(typeof(*uart), dev); + UART *uart = FROM_SYSBUS(typeof(*uart), dev); qemu_chr_add_handlers(uart->chr, grlib_apbuart_can_receive, diff --git a/trace-events b/trace-events index d2b0c61..5bb8bfb 100644 --- a/trace-events +++ b/trace-events @@ -340,6 +340,7 @@ grlib_irqmp_writel_unknown(uint64_t addr, uint32_t value) "addr 0x%"PRIx64" valu # hw/grlib_apbuart.c grlib_apbuart_event(int event) "event:%d" grlib_apbuart_writel_unknown(uint64_t addr, uint32_t value) "addr 0x%"PRIx64" value 0x%x" +grlib_apbuart_readl_unknown(uint64_t addr) "addr 0x%"PRIx64"" # hw/leon3.c leon3_set_irq(int intno) "Set CPU IRQ %d"
This patch implements the RX channel of GRLIB UART with a FIFO to improve data rate. Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- hw/grlib_apbuart.c | 106 +++++++++++++++++++++++++++++++++++++++++++-------- trace-events | 1 + 2 files changed, 90 insertions(+), 17 deletions(-)