Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Julia Suvorova]

On 31.05.2018 12:42, Stefan Hajnoczi wrote:
> On Wed, May 30, 2018 at 01:03:37AM +0300, Julia Suvorova wrote:
>> The following features are not yet implemented:
>>      Control with SUSPEND/START*/STOP*
>
> This is probably worth implementing for completeness.  Just rx_enabled
> and tx_enabled boolean states will be sufficient.  SUSPEND flushes tx
> and stops rx, it doesn't require any extra state.
>
>>      CTS/NCTS flow control
>
> CTS flow control is probably not necessary since we don't have bitwise
> I/O and guest applications probably don't care either.
>
>>      Mapping registers to pins
>
> This is probably not necessary since the micro:bit only uses the UART in
> one pin configuration (back to the USB interface chip).
>
> Please make sure that every register is explicitly handle in the code
> and falls into these categories:
>
> 1. Implemented.
> 2. Unimplemented - can be observed via a trace event.  This will make
>     debugging easier in the future when an application doesn't work
>     with the UART.
> 3. Stubbed out - explicitly marked as ignored in the code.
>
> This way we can be confident about the completeness of this device
> model.

I've definitely missed handling some registers, so marking it seems a
good idea.

>> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
>> new file mode 100644
>> index 0000000000..2da97aa0c4
>> --- /dev/null
>> +++ b/hw/char/nrf51_uart.c
>> @@ -0,0 +1,232 @@
>> +/*
>> + * nRF51 SoC UART emulation
>
> Please add a reference to the hardware specfication here:
>
>    See nRF51 Series Reference Manual, "29 Universal Asynchronous
>    Receiver/Transmitter" for hardware specifications:
>    http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>

Sure, I'll add it.

>> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void 
*opaque)
>> +{
>> +    Nrf51UART *s = NRF51_UART(opaque);
>> +    int r;
>> +
>> +    s->watch_tag = 0;
>> +
>> +    r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);
>
> This does not work on big-endian hosts:
>
>    s->reg[A_TXD] = 'A';
>
> little-endian memory layout: 41 00 00 00
> big-endian memory layout:    00 00 00 41
> (uint8_t *) &s->reg[A_TXD] --^
>
> Instead please use a uint8_t local:
>
>    uint8_t ch = s->reg[A_TXD];
>
>    ...
>
>    r = qemu_chr_fe_write(&s->chr, &ch, 1);
>

A real bug! I'll fix this.

>> +    if (r <= 0) {
>> +        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
>> +                                             uart_transmit, s);
>> +        if (!s->watch_tag) {
>> +            goto buffer_drained;
>
> Please add a comment here:
>
>    /* The hardware has no transmit error reporting, so silently drop the byte 
*/
>

The QEMU code is inconsistent with comments. Some files are full of
comments, some have only code. Is there any policy how to comment
the files?

>> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
>> +{
>> +
>> +   Nrf51UART *s = NRF51_UART(opaque);
>> +
>> +   if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
>
> Indentation is off.  Please use 4 spaces.

Sorry, have no idea how it happened.

>> +        return;
>> +    }
>> +
>> +    s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
>> +    s->rx_fifo_len++;
>> +
>> +    s->reg[A_RXDRDY] = 1;
>> +    nrf51_uart_update_irq(s);
>> +}
>> +
>> +static int uart_can_receive(void *opaque)
>> +{
>> +    Nrf51UART *s = NRF51_UART(opaque);
>> +
>> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
>
> This is very subtle: this function returns the number of bytes that can
> be read.  This expression looks like a boolean return value but actually
> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>
> Please write it so it doesn't look like a boolean return value:
>
>    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>        return 0;
>    }
>
>    return 1 /* byte */;
>
> Alternatively, you could handle more than 1 byte:
>
>    return sizeof(s->rx_fifo) - s->rx_fifo_len;
>
> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
> data in a single call.

I will rewrite uart_receive function to accept many bytes at once.

>> +}
>> +
>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>> +{
>> +    Nrf51UART *s = NRF51_UART(dev);
>> +
>> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>> +                             NULL, NULL, s, NULL, true);
>> +}
>> +
>> +static void nrf51_uart_init(Object *obj)
>> +{
>> +    Nrf51UART *s = NRF51_UART(obj);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +
>> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
>> +                          "nrf51_soc.uart", 0x1000);
>
> s/0x1000/UART_SIZE/

Should I just add a new define or move the existing one from nrf51_soc.c to
some header (or any other way to pass the UART_SIZE)?

Best regards, Julia Suvorova.