Re: [Qemu-devel] [PATCH 2/4] bcm2835_aux: add emulation of BCM2835 AUX (aka UART1) block

[Peter Maydell <peter.maydell@linaro.org>]

On 27 February 2016 at 00:16, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:

This bit of the commit message is a good place to list the
"not yet implemented" parts of the device.

> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> +/*
> + * BCM2835 (Raspberry Pi / Pi 2) Aux block (mini UART and SPI).
> + * Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann
> + * Based on pl011.c, copyright terms below:
> + *
> + * Arm PrimeCell PL011 UART
> + *
> + * Copyright (c) 2006 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the GPL.
> + */
>

I'm looking at the documentation at
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2835/BCM2835-ARM-Peripherals.pdf
for my review...

You should say in a comment that only the UART parts are
currently implemented.

> +#include "qemu/osdep.h"
> +#include "hw/char/bcm2835_aux.h"
> +
> +#define AUX_ENABLES     0x4
> +#define AUX_MU_IO_REG   0x40
> +#define AUX_MU_IER_REG  0x44
> +#define AUX_MU_IIR_REG  0x48
> +#define AUX_MU_LSR_REG  0x54
> +#define AUX_MU_STAT_REG 0x64
> +
> +static void bcm2835_aux_update(BCM2835AuxState *s)
> +{
> +    bool status = (s->rx_int_enable && s->read_count != 0) || s->tx_int_enable;
> +    qemu_set_irq(s->irq, status);

Could use a comment somewhere to the effect that since we model the
tx fifo as instantly-draining the 'tx fifo register empty' interrupt
is always asserted.

> +}
> +
> +static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    BCM2835AuxState *s = opaque;
> +    uint32_t c, res;
> +
> +    switch (offset) {

No AUX_IRQ implementation? (You need the current interrupt
status anyway for one of the other register bits.)

> +    case AUX_ENABLES:
> +        return 1; /* mini UART enabled */
> +
> +    case AUX_MU_IO_REG:
> +        c = s->read_fifo[s->read_pos];

The datasheet says this is a byte UART but you've implemented
the read_fifo and c as 32-bit.

> +        if (s->read_count > 0) {
> +            s->read_count--;
> +            if (++s->read_pos == 8) {
> +                s->read_pos = 0;
> +            }
> +        }
> +        if (s->chr) {
> +            qemu_chr_accept_input(s->chr);
> +        }
> +        bcm2835_aux_update(s);

This doesn't implement the "if line control DLAB bit is set, then read/write
the LS 8 bits of the baudrate register" behaviour described in the datasheet.

> +        return c;
> +
> +    case AUX_MU_IER_REG:
> +        res = 0;
> +        if (s->rx_int_enable) {
> +            res |= 0x2;
> +        }
> +        if (s->tx_int_enable) {
> +            res |= 0x1;
> +        }

I suspect you'll find the code is clearer generally if you model this
register with a uint8_t ier in the state structure rather than two bools
(for instance "is interrupt asserted" is generally "enables | status".)

Doesn't implement "DLAB bit set means access MS 8 bits of the baudrate
register".

> +        return res;
> +
> +    case AUX_MU_IIR_REG:
> +        res = 0xc0;
> +        if (s->tx_int_enable) {
> +            res |= 0x1;
> +        } else if (s->rx_int_enable && s->read_count != 0) {
> +            res |= 0x2;
> +        }

This is kind of repeating the logic in bcm2835_aux_update(), would
be nice to factor it out.

The data sheet says that the bit allocation here is different:
bit 0 is zero when an interrupt is pending, and bits 1 and 2 are
the read and write information.

The data sheet also says that 0b11 for bits [2:1] is not possible, though
it doesn't say why. It's not clear what the hardware reads as if
the transmit holding register is empty *and* the receiver has a
valid byte...

> +        return res;
> +
> +    case AUX_MU_LSR_REG:
> +        res = 0x60; /* tx idle, empty */
> +        if (s->read_count != 0) {
> +            res |= 0x1;
> +        }
> +        return res;
> +
> +    case AUX_MU_STAT_REG:
> +        res = 0x302; /* space in the output buffer, empty tx fifo */

Shouldn't we set the "transmitter idle" bit too?

> +        if (s->read_count > 0) {
> +            res |= 0x1; /* data in input buffer */
> +            assert(s->read_count < 8);
> +            res |= ((uint32_t)s->read_count) << 16; /* rx fifo fill level */
> +        }
> +        return res;
> +

No AUX_MU_LCR_REG ? No AUX_MU_MCR_REG ? No AUX_MU_MSR_REG ?
AUX_MU_SCRATCH ? AUX_MU_CNTL_REG ? AUX_MU_BAUD ?

At least implementing them to log unimplemented would be nice.

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %"HWADDR_PRIx"\n",
> +                      __func__, offset);
> +        return 0;
> +    }
> +}
> +
> +static void bcm2835_aux_write(void *opaque, hwaddr offset, uint64_t value,
> +                              unsigned size)
> +{
> +    BCM2835AuxState *s = opaque;
> +    unsigned char ch;
> +
> +    switch (offset) {
> +    case AUX_ENABLES:
> +        if (value != 1) {
> +            qemu_log_mask(LOG_UNIMP, "%s: unsupported attempt to enable SPI "
> +                          "or disable UART\n", __func__);
> +        }
> +        break;
> +
> +    case AUX_MU_IO_REG:
> +        ch = value;
> +        if (s->chr) {
> +            qemu_chr_fe_write(s->chr, &ch, 1);
> +        }
> +        break;
> +
> +    case AUX_MU_IER_REG:
> +        s->rx_int_enable = (value & 0x2) != 0;
> +        s->tx_int_enable = (value & 0x1) != 0;
> +        break;
> +
> +    case AUX_MU_IIR_REG:
> +        if (value & 0x1) {
> +            s->read_count = 0;
> +        }

Datasheet says bit 0 is read only, bit 1 clears the rx fifo.

> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %"HWADDR_PRIx"\n",
> +                      __func__, offset);
> +    }
> +
> +    bcm2835_aux_update(s);
> +}
> +
> +static int bcm2835_aux_can_receive(void *opaque)
> +{
> +    BCM2835AuxState *s = opaque;
> +
> +    return s->read_count < 8;
> +}
> +
> +static void bcm2835_aux_put_fifo(void *opaque, uint32_t value)
> +{
> +    BCM2835AuxState *s = opaque;
> +    int slot;
> +
> +    slot = s->read_pos + s->read_count;
> +    if (slot >= 8) {
> +        slot -= 8;
> +    }
> +    s->read_fifo[slot] = value;
> +    s->read_count++;
> +    if (s->read_count == 8) {
> +        /* buffer full */
> +    }
> +    bcm2835_aux_update(s);
> +}
> +
> +static void bcm2835_aux_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +    bcm2835_aux_put_fifo(opaque, *buf);
> +}
> +
> +static void bcm2835_aux_event(void *opaque, int event)
> +{
> +    if (event == CHR_EVENT_BREAK) {
> +        bcm2835_aux_put_fifo(opaque, 0x400);
> +    }

Data sheet says the UART does not have break detection.

> +}
> +
> +static const MemoryRegionOps bcm2835_aux_ops = {
> +    .read = bcm2835_aux_read,
> +    .write = bcm2835_aux_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +};
> +
> +static const VMStateDescription vmstate_bcm2835_aux = {
> +    .name = TYPE_BCM2835_AUX,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(read_fifo, BCM2835AuxState, 8),
> +        VMSTATE_UINT8(read_pos, BCM2835AuxState),
> +        VMSTATE_UINT8(read_count, BCM2835AuxState),
> +        VMSTATE_BOOL(rx_int_enable, BCM2835AuxState),
> +        VMSTATE_BOOL(tx_int_enable, BCM2835AuxState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void bcm2835_aux_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    BCM2835AuxState *s = BCM2835_AUX(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_aux_ops, s,
> +                          TYPE_BCM2835_AUX, 0x100);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static void bcm2835_aux_realize(DeviceState *dev, Error **errp)
> +{
> +    BCM2835AuxState *s = BCM2835_AUX(dev);
> +    /* FIXME use a qdev chardev prop instead of qemu_char_get_next_serial() */

Good idea :-)  imx_serial.c has an example, you use
DEFINE_PROP_CHR and then wire it up in the board construction with
qdev_prop_set_chr(). (You probably want to create an alias property on
the SoC object as we now do for spi etc; see the xilinx soc for
an example.)

> +    s->chr = qemu_char_get_next_serial();

thanks
-- PMM