Re: [Qemu-devel] [PATCH v6 1/4] cadence_uart: initial version of device model

[Peter Crosthwaite]

On Tue, Feb 21, 2012 at 4:58 AM, Peter Maydell <address@hidden> wrote:
> On 20 February 2012 01:45, Peter A. G. Crosthwaite
> <address@hidden> wrote:
>> Implemented cadence UART serial controller
>
> Is there any publicly available documentation for this? Google
> only found a minimum-info datasheet...

No, Silicon vendor still has it under NDA. We are pushing for the release.

>
>> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
>> Signed-off-by: John Linn <address@hidden>
>> ---
>> ---
>> changed from v4:
>> fixed FSF addess
>> changed device_init -> type_init
>> changes from v1:
>> converted register file to array
>> added vmsd state save/load support
>> removed read side effects from CISR register
>>
>>  Makefile.target   |    1 +
>>  hw/cadence_uart.c |  559 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 560 insertions(+), 0 deletions(-)
>>  create mode 100644 hw/cadence_uart.c
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index 651500e..868be15 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -343,6 +343,7 @@ endif
>>  obj-arm-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
>>  obj-arm-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o 
>> pl190.o
>>  obj-arm-y += versatile_pci.o
>> +obj-arm-y += cadence_uart.o
>>  obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
>>  obj-arm-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
>>  obj-arm-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
>> diff --git a/hw/cadence_uart.c b/hw/cadence_uart.c
>> new file mode 100644
>> index 0000000..a7e461f
>> --- /dev/null
>> +++ b/hw/cadence_uart.c
>> @@ -0,0 +1,559 @@
>> +/*
>> + * Device model for Cadence UART
>> + *
>> + * Copyright (c) 2010 Xilinx Inc.
>> + * Copyright (c) 2012 Peter A.G. Crosthwaite (address@hidden)
>> + * Copyright (c) 2012 PetaLogix Pty Ltd.
>> + * Written by Haibing Ma
>> + *            M.Habib
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "sysbus.h"
>> +#include "qemu-char.h"
>> +#include "qemu-timer.h"
>> +
>> +#ifdef CADENCE_UART_ERR_DEBUG
>> +#define qemu_debug(...) do { \
>> +    fprintf(stderr,  ": %s: ", __func__); \
>> +    fprintf(stderr, ## __VA_ARGS__); \
>> +    fflush(stderr); \
>> +    } while (0);
>> +#else
>> +    #define qemu_debug(...)
>> +#endif
>
> Don't use lowercase for macros; also qemu_debug() is a bad
> choice as it's liable to get used by generic code.

OK

> In fact since this is used exactly once in the whole file
> I think I'd just drop it completely.
>

Keeps it consistent with how I handle it in other device models

> (Ideally we'd support "debug register accesses to this device"
> by letting you interpose a MemoryRegion that traced addresses
> and values. Sadly not implemented yet ;-))
>
>> +#define UART_INTR_RTRIG     0x00000001
>> +#define UART_INTR_REMPTY    0x00000002
>> +#define UART_INTR_RFUL      0x00000004
>> +#define UART_INTR_TEMPTY    0x00000008
>> +#define UART_INTR_TFUL      0x00000010
>> +#define UART_INTR_ROVR      0x00000020
>> +#define UART_INTR_FRAME     0x00000040
>> +#define UART_INTR_PARE      0x00000080
>> +#define UART_INTR_TIMEOUT   0x00000100
>> +#define UART_INTR_DMSI      0x00000200
>> +#define UART_INTR_TTRIG     0x00000400
>> +#define UART_INTR_TNFUL     0x00000800
>> +#define UART_INTR_TOVR      0x00001000
>> +
>> +#define UART_CSR_RTRIG      0x00000001
>> +#define UART_CSR_REMPTY     0x00000002
>> +#define UART_CSR_RFUL       0x00000004
>> +#define UART_CSR_TEMPTY     0x00000008
>> +#define UART_CSR_TFUL       0x00000010
>> +#define UART_CSR_ROVR       0x00000020
>> +#define UART_CSR_FRAME      0x00000040
>> +#define UART_CSR_PARE       0x00000080
>> +#define UART_CSR_TIMEOUT    0x00000100
>> +#define UART_CSR_DMSI       0x00000200
>> +#define UART_CSR_RACTIVE    0x00000400
>> +#define UART_CSR_TACTIVE    0x00000800
>> +#define UART_CSR_FDELT      0x00001000
>> +#define UART_CSR_TTRIG      0x00002000
>> +#define UART_CSR_TNFUL      0x00004000
>> +
>> +#define UART_CR_STOPBRK     0x00000100
>> +#define UART_CR_STARTBRK    0x00000080
>> +#define UART_CR_RST_TO      0x00000040
>> +#define UART_CR_TX_DIS      0x00000020
>> +#define UART_CR_TX_EN       0x00000010
>> +#define UART_CR_RX_DIS      0x00000008
>> +#define UART_CR_RX_EN       0x00000004
>> +#define UART_CR_TXRST       0x00000002
>> +#define UART_CR_RXRST       0x00000001
>> +
>> +#define UART_MR_CLKS            0x00000001
>> +#define UART_MR_CHRL            0x00000006
>> +#define UART_MR_PAR             0x00000038
>> +#define UART_MR_NBSTOP          0x000000C0
>> +#define UART_MR_CHMODE          0x00000300
>> +#define UART_MR_UCLKEN          0x00000400
>> +#define UART_MR_IRMODE          0x00000800
>> +
>> +#define UART_PARITY_ODD        0x001
>> +#define UART_PARITY_EVEN       0x000
>> +#define UART_DATA_BITS_6       0x003
>> +#define UART_DATA_BITS_7       0x002
>> +#define UART_STOP_BITS_1       0x003
>> +#define UART_STOP_BITS_2       0x002
>> +#define RX_FIFO_SIZE           16
>> +#define TX_FIFO_SIZE           16
>> +#define UARK_INPUT_CLK         50000000
>> +
>> +#define NORMAL_MODE            0
>> +#define ECHO_MODE              1
>> +#define LOCAL_LOOPBACK         2
>> +#define REMOTE_LOOPBACK        3
>> +
>> +#define R_CR       (0x00/4)
>> +#define R_MR       (0x04/4)
>> +#define R_IER      (0x08/4)
>> +#define R_IDR      (0x0C/4)
>> +#define R_IMR      (0x10/4)
>> +#define R_CISR     (0x14/4)
>> +#define R_BRGR     (0x18/4)
>> +#define R_RTOR     (0x1C/4)
>> +#define R_RTRIG    (0x20/4)
>> +#define R_MCR      (0x24/4)
>> +#define R_MSR      (0x28/4)
>> +#define R_CSR      (0x2C/4)
>> +#define R_TX_RX    (0x30/4)
>> +#define R_BDIV     (0x34/4)
>> +#define R_FDEL     (0x38/4)
>> +#define R_PMIN     (0x3C/4)
>> +#define R_PWID     (0x40/4)
>> +#define R_TTRIG    (0x44/4)
>> +
>> +#define R_MAX (R_TTRIG + 1)
>> +
>> +typedef struct {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    uint32_t r[R_MAX];
>> +    uint8_t r_fifo[RX_FIFO_SIZE];
>> +    uint32_t rx_wpos;
>> +    uint32_t rx_count;
>> +    uint64_t char_tx_time;
>> +    CharDriverState *chr;
>> +    qemu_irq irq;
>> +    struct QEMUTimer *fifo_trigger_handle;
>> +    struct QEMUTimer *tx_time_handle;
>> +} UartState;
>> +
>> +static void uart_update_status(UartState *s)
>> +{
>> +    qemu_set_irq(s->irq, !!(s->r[R_IMR] & s->r[R_CISR]));
>> +}
>> +
>> +static void fifo_trigger_update(void *opaque)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +
>> +    s->r[R_CSR] |= UART_CSR_TIMEOUT;
>> +    s->r[R_CISR] |= UART_INTR_TIMEOUT;
>> +
>> +    uart_update_status(s);
>> +}
>> +
>> +static void uart_tx_redo(UartState *s)
>> +{
>> +    uint64_t new_tx_time = qemu_get_clock_ns(vm_clock);
>> +
>> +    qemu_mod_timer(s->tx_time_handle, new_tx_time + s->char_tx_time);
>> +
>> +    s->r[R_CSR] |= UART_CSR_TEMPTY;
>> +    s->r[R_CISR] |= UART_INTR_TEMPTY;
>> +
>> +    uart_update_status(s);
>> +}
>> +
>> +static void uart_tx_write(void *opaque)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +
>> +    uart_tx_redo(s);
>> +}
>> +
>> +static void uart_rx_reset(UartState *s)
>> +{
>> +    s->rx_wpos = 0;
>> +    s->rx_count = 0;
>> +
>> +    s->r[R_CSR] |= UART_CSR_REMPTY;
>> +    s->r[R_CSR] &= ~UART_CSR_RFUL;
>> +    s->r[R_CSR] &= ~UART_CSR_ROVR;
>> +    s->r[R_CSR] &= ~UART_CSR_TIMEOUT;
>> +
>> +    s->r[R_CISR] &= ~UART_INTR_REMPTY;
>> +    s->r[R_CISR] &= ~UART_INTR_RFUL;
>> +    s->r[R_CISR] &= ~UART_INTR_ROVR;
>> +    s->r[R_CISR] &= ~UART_INTR_TIMEOUT;
>> +}
>> +
>> +static void uart_tx_reset(UartState *s)
>> +{
>> +    s->r[R_CSR] |= UART_CSR_TEMPTY;
>> +    s->r[R_CSR] &= ~UART_CSR_TFUL;
>> +
>> +    s->r[R_CISR] &= ~UART_INTR_TEMPTY;
>> +    s->r[R_CISR] &= ~UART_INTR_TFUL;
>> +}
>> +
>> +static void uart_send_breaks(UartState *s)
>> +{
>> +    int break_enabled = 1;
>> +
>> +    qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
>> +                               &break_enabled);
>> +}
>> +
>> +static inline uint32_t mask_and_right_justify(uint32_t value, uint32_t mask)
>> +{
>> +    while (!mask & 0x1) {
>> +        mask >>= 1;
>> +        value >>= 1;
>> +    }
>> +    return value & mask;
>> +}
>
> This doesn't do what you think it does. (Hint, operator precedence.)
>
> I think that plus the fact it's not how we handle this in the rest
> of qemu is good enough to suggest that you just do this the way other
> device models do, and shift/mask by a constant amount.
>

Fixed

>> +
>> +static void uart_parameters_setup(UartState *s)
>> +{
>> +    QEMUSerialSetParams ssp;
>> +    unsigned int baud_rate, packet_size;
>> +
>> +    baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> +            UARK_INPUT_CLK / 8 : UARK_INPUT_CLK;
>> +
>> +    ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> +    packet_size = 1;
>> +
>> +    switch (mask_and_right_justify(s->r[R_MR], UART_MR_PAR)) {
>> +    case UART_PARITY_EVEN:
>> +        ssp.parity = 'E';
>> +        packet_size++;
>> +        break;
>> +    case UART_PARITY_ODD:
>> +        ssp.parity = 'O';
>> +        packet_size++;
>> +        break;
>> +    default:
>> +        ssp.parity = 'N';
>> +        break;
>> +    }
>> +
>> +    switch (mask_and_right_justify(s->r[R_MR], UART_MR_CHRL)) {
>> +    case UART_DATA_BITS_6:
>> +        ssp.data_bits = 6;
>> +        break;
>> +    case UART_DATA_BITS_7:
>> +        ssp.data_bits = 7;
>> +        break;
>> +    default:
>> +        ssp.data_bits = 8;
>> +        break;
>> +    }
>> +
>> +    switch (mask_and_right_justify(s->r[R_MR], UART_MR_NBSTOP)) {
>> +    case UART_STOP_BITS_1:
>> +        ssp.stop_bits = 1;
>> +        break;
>> +    default:
>> +        ssp.stop_bits = 2;
>> +        break;
>> +    }
>> +
>> +    packet_size += ssp.data_bits + ssp.stop_bits;
>> +    s->char_tx_time =  (get_ticks_per_sec() / ssp.speed) * packet_size;
>
> Stray double space.
>
>> +    qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
>> +}
>> +
>> +static void uart_stop_breaks(UartState *s)
>> +{
>> +    int break_enabled = 0;
>> +    qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
>> +                               &break_enabled);
>> +}
>
> Why not put this next to uart_send_breaks() ?
>

Deleted (see below)

>> +
>> +static int uart_can_receive(void *opaque)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +
>> +    return RX_FIFO_SIZE - s->rx_count;
>> +}
>> +
>> +static void uart_ctrl_update(UartState *s, uint32_t value)
>> +{
>
> When this is called by uart_write() we've already done
> s->r[offset] = value, but this code seems to assume that
> s->r[R_CR] and value are the old and new values. Who's right?
>

Removed value argument altogether

>> +    if (value & UART_CR_TXRST) {
>> +        uart_tx_reset(s);
>> +    }
>> +
>> +    if (value & UART_CR_RXRST) {
>> +        uart_rx_reset(s);
>> +    }
>> +
>> +    s->r[R_CR] &= ~(UART_CR_TXRST | UART_CR_RXRST);
>> +
>> +    if ((value & UART_CR_TX_EN) && !(s->r[R_CR] & UART_CR_TX_DIS)) {
>> +            uart_tx_redo(s);
>> +    }
>> +
>> +    if (value & UART_CR_STARTBRK) {
>> +        if (!(s->r[R_CR] & UART_CR_STOPBRK)) {
>> +            uart_send_breaks(s);
>> +        }
>> +    }
>> +    if (value & UART_CR_STARTBRK) {
>> +        uart_stop_breaks(s);
>
> ...if the STARTBRK bit is set we stop sending breaks??
>
> Why does the hardware have separate STARTBRK and STOPBRK
> bits? What happens if they're both set?
>

Yeh this is bizarre. I traced it and it turns out the stop_breaks path
is just a giant nop so i removed it completely.

>
>
>> +    }
>> +}
>> +
>> +static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +    uint64_t new_rx_time = qemu_get_clock_ns(vm_clock);
>> +    int i;
>> +
>> +    if ((s->r[R_CR] & UART_CR_RX_DIS) || !(s->r[R_CR] & UART_CR_RX_EN)) {
>> +        return;
>> +    }
>> +
>> +    s->r[R_CSR] &= ~UART_CSR_REMPTY;
>> +    s->r[R_CISR] &= ~UART_INTR_REMPTY;
>> +
>> +    if (s->rx_count == RX_FIFO_SIZE) {
>> +        s->r[R_CISR] |= UART_INTR_ROVR;
>> +        s->r[R_CSR] |= UART_CSR_ROVR;
>> +    } else {
>> +        for (i = 0; i < size; i++) {
>> +            s->r_fifo[s->rx_wpos] = buf[i];
>> +            s->rx_wpos = (s->rx_wpos + 1) % RX_FIFO_SIZE;
>> +            s->rx_count++;
>> +
>> +            if (s->rx_count == RX_FIFO_SIZE) {
>> +                s->r[R_CSR] |= UART_CSR_RFUL;
>> +                s->r[R_CISR] |= UART_INTR_RFUL;
>> +                break;
>> +            }
>> +
>> +            if (s->rx_count >= s->r[R_RTRIG]) {
>> +                s->r[R_CISR] |= UART_INTR_RTRIG;
>> +                s->r[R_CSR] |= UART_CSR_RTRIG;
>> +            }
>> +        }
>> +        qemu_mod_timer(s->fifo_trigger_handle, new_rx_time +
>> +                                                (s->char_tx_time * 4));
>> +    }
>> +    uart_update_status(s);
>> +}
>> +
>> +static void uart_write_tx_fifo(UartState *s, unsigned  char *c)
>> +{
>> +    unsigned  char ch = *c;
>> +
>> +    if ((s->r[R_CR] & UART_CR_TX_DIS) || !(s->r[R_CR] & UART_CR_TX_EN)) {
>> +        return;
>> +    }
>> +
>> +    while (!qemu_chr_fe_write(s->chr, &ch, 1)) {
>
> ...why not just pass c?
>
Done

>> +    }
>
> Is looping like this really a good idea? None of the other uarts
> seem to do it...
>
Wont this potentially back onto an write syscall where it can return
short of the specified write length? Or have we got that wrong?

>> +}
>> +
>> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +    uint32_t ch_mode = mask_and_right_justify(s->r[R_MR], UART_MR_CHMODE);
>> +
>> +    if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
>> +        uart_write_rx_fifo(opaque, buf, size);
>> +    }
>> +    if (ch_mode == REMOTE_LOOPBACK || ch_mode == ECHO_MODE) {
>> +        uart_write_tx_fifo(s, (unsigned char *)buf);
>> +    }
>
> Is it really correct that in loopback mode we write the whole buffer
> to the RX fifo but only one byte of it to the TX fifo?
> Why not make the write_tx_fifo prototype take a const uint8_t* like
> read_tx_fifo so we don't have to have a cast here?
>

Fixed

>> +}
>> +
>> +static void uart_event(void *opaque, int event)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +    uint8_t buf = '\0';
>> +
>> +    if (event == CHR_EVENT_BREAK) {
>> +        uart_write_rx_fifo(opaque, &buf, 1);
>> +    }
>> +
>> +    uart_update_status(s);
>> +}
>> +
>> +static void uart_read_rx_fifo(UartState *s, uint32_t *c)
>> +{
>> +    if ((s->r[R_CR] & UART_CR_RX_DIS) || !(s->r[R_CR] & UART_CR_RX_EN)) {
>> +        return;
>> +    }
>> +
>> +    s->r[R_CSR] &= ~UART_CSR_RFUL;
>> +    s->r[R_CSR] &= ~UART_CSR_ROVR;
>> +    s->r[R_CISR] &= ~UART_INTR_ROVR;
>> +    s->r[R_CISR] &= ~UART_INTR_RFUL;
>
> Are you sure this is right? I can't tell without a data sheet, but usually
> if there's a status register and an interrupt status register then the
> status register tracks current status but the interrupt status register
> latches it and is only cleared by explicit interrupt acknowledge.
>

TRM is vague at best, but you are probably right. Fixed

>> +
>> +    if (s->rx_count) {
>> +        uint32_t rx_rpos =
>> +                (RX_FIFO_SIZE + s->rx_wpos - s->rx_count) % RX_FIFO_SIZE;
>> +        *c = s->r_fifo[rx_rpos];
>> +        s->rx_count--;
>> +
>> +        if (!s->rx_count) {
>> +            s->r[R_CISR] |= UART_INTR_REMPTY;
>> +            s->r[R_CSR] |= UART_CSR_REMPTY;
>> +        }
>> +    } else {
>> +        *c = 0;
>> +        s->r[R_CISR] |= UART_INTR_REMPTY;
>> +        s->r[R_CSR] |= UART_CSR_REMPTY;
>> +    }
>> +
>> +    if (s->rx_count < s->r[R_RTRIG]) {
>> +        s->r[R_CSR] &= ~UART_CSR_RTRIG;
>> +        s->r[R_CISR] &= ~UART_INTR_RTRIG;
>> +    }
>> +    uart_update_status(s);
>> +}
>> +
>> +static void uart_write(void *opaque, target_phys_addr_t offset,
>> +                          uint64_t value, unsigned size)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +
>> +    qemu_debug(" offset:%x data:%08x\n", offset, (unsigned)value);
>> +    offset >>= 2;
>> +    switch (offset) {
>> +    case R_IER: /* ier (wts imr) */
>> +        s->r[R_IMR] |= value;
>> +        break;
>> +    case R_IDR: /* idr (wtc imr) */
>> +        s->r[R_IMR] &= ~value;
>> +        break;
>> +    case R_IMR: /* imr (read only) */
>> +        break;
>> +    case R_CISR: /* cisr (wtc) */
>> +        s->r[R_CISR] &= ~value;
>> +        break;
>> +    case R_TX_RX: /* UARTDR */
>> +        switch (mask_and_right_justify(s->r[R_MR], UART_MR_CHMODE)) {
>> +        case NORMAL_MODE:
>> +            uart_write_tx_fifo(s, (unsigned  char *) &value);
>> +            break;
>> +        case LOCAL_LOOPBACK:
>> +            uart_write_rx_fifo(opaque, (unsigned  char *) &value, 1);
>> +            break;
>> +        }
>> +        break;
>> +    default:
>> +        s->r[offset] = value;
>> +    }
>> +
>> +    switch (offset) {
>> +    case R_CR:
>> +        uart_ctrl_update(s, value);
>> +        break;
>> +    case R_MR:
>> +        uart_parameters_setup(s);
>> +        break;
>> +    }
>> +}
>
> As noted earlier, the handling of R_CR is suspect here. That would
> only leave R_MR in this second switch, so if I were you I'd just fold
> it all into the first switch (ie set s->[R_MR] by hand rather than trying
> to reuse the default: case).
>

Fixed elsewhere

>> +
>> +static uint64_t uart_read(void *opaque, target_phys_addr_t offset,
>> +        unsigned size)
>> +{
>> +    UartState *s = (UartState *)opaque;
>> +    uint32_t c = 0;
>> +
>> +    offset >>= 2;
>> +    if (offset > R_MAX) {
>> +        return 0;
>> +    } else if (offset == R_TX_RX) {
>> +        uart_read_rx_fifo(s, &c);
>> +        return c;
>> +    }
>> +    return s->r[offset];
>> +}
>> +
>> +static const MemoryRegionOps uart_ops = {
>> +    .read = uart_read,
>> +    .write = uart_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static int cadence_uart_init(SysBusDevice *dev)
>> +{
>> +    UartState *s = FROM_SYSBUS(UartState, dev);
>> +
>> +    memory_region_init_io(&s->iomem, &uart_ops, s, "uart", 0x1000);
>> +    sysbus_init_mmio(dev, &s->iomem);
>> +    sysbus_init_irq(dev, &s->irq);
>> +
>> +    s->fifo_trigger_handle = qemu_new_timer_ns(vm_clock,
>> +            (QEMUTimerCB *)fifo_trigger_update, s);
>> +
>> +    s->tx_time_handle = qemu_new_timer_ns(vm_clock,
>> +            (QEMUTimerCB *)uart_tx_write, s);
>> +
>> +    s->char_tx_time = (get_ticks_per_sec() / 9600) * 10;
>> +
>> +    s->chr = qemu_char_get_next_serial();
>> +
>> +    s->r[R_CR] = 0x00000128;
>> +    s->r[R_IMR] = 0;
>> +    s->r[R_CISR] = 0;
>> +    s->r[R_RTRIG] = 0x00000020;
>> +    s->r[R_BRGR] = 0x0000000F;
>> +    s->r[R_TTRIG] = 0x00000020;
>> +
>> +    uart_rx_reset(s);
>> +    uart_tx_reset(s);
>> +
>> +    s->rx_count = 0;
>> +    s->rx_wpos = 0;
>
> This sort of reset code should go in a reset function, not the
> init function.
>

Fixed

>> +
>> +    if (s->chr) {
>> +        qemu_chr_add_handlers(s->chr, uart_can_receive, uart_receive,
>> +                              uart_event, s);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int cadence_uart_post_load(void *opaque, int version_id)
>> +{
>> +    UartState *s = opaque;
>> +
>> +    uart_parameters_setup(s);
>> +    uart_update_status(s);
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_cadence_uart = {
>> +    .name = "cadence_uart",
>> +    .version_id = 3,
>> +    .minimum_version_id = 2,
>> +    .minimum_version_id_old = 2,
>
> ...who are we trying to be backward compatible with here?
>

I kinda just cloned some other random device for these fields. Is this
VMSD support a blocker on the series, cos to be honest, nobody wants
it and I cant effectively test it, so id rather just mark everything
unmigratable and come back to it.

>> +    .post_load = cadence_uart_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(r, UartState, R_MAX),
>> +        VMSTATE_UINT8_ARRAY(r_fifo, UartState, RX_FIFO_SIZE),
>> +        VMSTATE_UINT32(rx_count, UartState),
>> +        VMSTATE_UINT32(rx_wpos, UartState),
>> +        VMSTATE_TIMER(fifo_trigger_handle, UartState),
>> +        VMSTATE_TIMER(tx_time_handle, UartState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void cadence_uart_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> +    sdc->init = cadence_uart_init;
>> +    dc->vmsd = &vmstate_cadence_uart;
>> +}
>> +
>> +static TypeInfo cadence_uart_info = {
>> +    .name          = "cadence_uart",
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(UartState),
>> +    .class_init    = cadence_uart_class_init,
>> +};
>> +
>> +static void cadence_uart_register_types(void)
>> +{
>> +    type_register_static(&cadence_uart_info);
>> +}
>> +
>> +type_init(cadence_uart_register_types)
>> --
>> 1.7.3.2
>
> -- PMM