[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/4] cadence_uart: initial version of device
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/4] cadence_uart: initial version of device model |
Date: |
Thu, 23 Feb 2012 11:41:27 +1000 |
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
- [Qemu-devel] [PATCH v6 0/4] Zynq-7000 EPP platform model, Peter A. G. Crosthwaite, 2012/02/19
- [Qemu-devel] [PATCH v6 1/4] cadence_uart: initial version of device model, Peter A. G. Crosthwaite, 2012/02/19
- [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter A. G. Crosthwaite, 2012/02/19
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Maydell, 2012/02/20
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Paul Brook, 2012/02/21
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/22
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Paul Brook, 2012/02/27
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/27
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/27
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Paul Brook, 2012/02/28
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/28
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/22