Re: [Qemu-devel] [PATCH V2] GRLIB UART: Add RX channel

[Fabien Chouteau <chouteau@adacore.com>]

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,

-- 
Fabien Chouteau