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

[Blue Swirl]

On Mon, Jan 30, 2012 at 09:22, Fabien Chouteau <address@hidden> wrote:
> On 28/01/2012 13:20, Blue Swirl wrote:
>> On Thu, Jan 26, 2012 at 17:03, Fabien Chouteau <address@hidden> wrote:
>>> This patch implements the RX channel of GRLIB UART with a FIFO to
>>> improve data rate.
>>>
>>> Signed-off-by: Fabien Chouteau <address@hidden>
>>> ---
>>>  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