Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model

[Anthony Liguori <anthony@codemonkey.ws>]

[-- Attachment #1: Type: text/plain, Size: 17273 bytes --]

Anthony Liguori <anthony@codemonkey.ws> writes:

> peter.crosthwaite@xilinx.com writes:
>
>> From: "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com>
>>
>> Minimal device model for devcfg module of Zynq. DMA capabilities and
>> interrupt generation supported.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>> Changed since v2:
>> Some QOM styling updates.
>> Re-implemented nw0 for lock register as pre_write
>> Changed since v1:
>> Rebased against new version of Register API.
>> Use action callbacks for side effects rather than switch.
>> Documented reasons for ge0, ge1 (Verbatim from TRM)
>> Added ui1 definitions for unimplemented major features
>> Removed dead lock code
>>
>>  default-configs/arm-softmmu.mak |   1 +
>>  hw/dma/Makefile.objs            |   1 +
>>  hw/dma/xilinx_devcfg.c          | 495 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 497 insertions(+)
>>  create mode 100644 hw/dma/xilinx_devcfg.c
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 27cbe3d..5a17f64 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -61,6 +61,7 @@ CONFIG_PXA2XX=y
>>  CONFIG_BITBANG_I2C=y
>>  CONFIG_FRAMEBUFFER=y
>>  CONFIG_XILINX_SPIPS=y
>> +CONFIG_ZYNQ_DEVCFG=y
>>  
>>  CONFIG_A9SCU=y
>>  CONFIG_MARVELL_88W8618=y
>> diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
>> index 0e65ed0..96025cf 100644
>> --- a/hw/dma/Makefile.objs
>> +++ b/hw/dma/Makefile.objs
>> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_PL330) += pl330.o
>>  common-obj-$(CONFIG_I82374) += i82374.o
>>  common-obj-$(CONFIG_I8257) += i8257.o
>>  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
>> +common-obj-$(CONFIG_ZYNQ_DEVCFG) += xilinx_devcfg.o
>>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
>>  common-obj-$(CONFIG_STP2000) += sparc32_dma.o
>>  common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
>> diff --git a/hw/dma/xilinx_devcfg.c b/hw/dma/xilinx_devcfg.c
>> new file mode 100644
>> index 0000000..b82b7cc
>> --- /dev/null
>> +++ b/hw/dma/xilinx_devcfg.c
>> @@ -0,0 +1,495 @@
>> +/*
>> + * QEMU model of the Xilinx Devcfg Interface
>> + *
>> + * Copyright (c) 2011 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/dma.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/ptimer.h"
>> +#include "qemu/bitops.h"
>> +#include "hw/register.h"
>> +#include "qemu/bitops.h"
>> +
>> +#define TYPE_XILINX_DEVCFG "xlnx.ps7-dev-cfg"
>> +
>> +#define XILINX_DEVCFG(obj) \
>> +    OBJECT_CHECK(XilinxDevcfg, (obj), TYPE_XILINX_DEVCFG)
>> +
>> +/* FIXME: get rid of hardcoded nastiness */
>> +
>> +#define FREQ_HZ 900000000
>> +
>> +#define BTT_MAX 0x400 /* bytes to transfer per delay inteval */
>> +#define CYCLES_BTT_MAX 10000 /*approximate 10k cycles per delay interval */
>> +
>> +#ifndef XILINX_DEVCFG_ERR_DEBUG
>> +#define XILINX_DEVCFG_ERR_DEBUG 0
>> +#endif
>> +#define DB_PRINT(...) do { \
>> +    if (XILINX_DEVCFG_ERR_DEBUG) { \
>> +        fprintf(stderr,  ": %s: ", __func__); \
>> +        fprintf(stderr, ## __VA_ARGS__); \
>> +    } \
>> +} while (0);
>> +
>> +#define R_CTRL            (0x00/4)
>> +    #define FORCE_RST            (1 << 31) /* Not supported, writes ignored */
>> +    #define PCAP_PR              (1 << 27) /* Forced to 0 on bad unlock */
>> +    #define PCAP_MODE            (1 << 26)
>> +    #define MULTIBOOT_EN         (1 << 24)
>> +    #define USER_MODE            (1 << 15)
>> +    #define PCFG_AES_FUSE        (1 << 12) /* locked by AES_FUSE_LOCK */
>> +    #define PCFG_AES_EN_SHIFT    9 /* locked by AES_EN_LOCK */
>> +    #define PCFG_AES_EN_LEN      3 /* locked by AES_EN_LOCK */
>> +    #define PCFG_AES_EN_MASK     (((1 << PCFG_AES_EN_LEN) - 1) \
>> +                                    << PCFG_AES_EN_SHIFT)
>> +    #define SEU_EN               (1 << 8) /* locked by SEU_LOCK */
>> +    #define SEC_EN               (1 << 7) /* locked by SEC_LOCK */
>> +    #define SPNIDEN              (1 << 6) /* locked by DBG_LOCK */
>> +    #define SPIDEN               (1 << 5) /* locked by DBG_LOCK */
>> +    #define NIDEN                (1 << 4) /* locked by DBG_LOCK */
>> +    #define DBGEN                (1 << 3) /* locked by DBG_LOCK */
>> +    #define DAP_EN               (7 << 0) /* locked by DBG_LOCK */
>> +
>> +#define R_LOCK          (0x04/4)
>> +    #define AES_FUSE_LOCK        4
>> +    #define AES_EN_LOCK          3
>> +    #define SEU_LOCK             2
>> +    #define SEC_LOCK             1
>> +    #define DBG_LOCK             0
>> +
>> +/* mapping bits in R_LOCK to what they lock in R_CTRL */
>> +static const uint32_t lock_ctrl_map[] = {
>> +    [AES_FUSE_LOCK] = PCFG_AES_FUSE,
>> +    [AES_EN_LOCK] = PCFG_AES_EN_MASK,
>> +    [SEU_LOCK] = SEU_LOCK,
>> +    [SEC_LOCK] = SEC_LOCK,
>> +    [DBG_LOCK] =  SPNIDEN | SPIDEN | NIDEN | DBGEN | DAP_EN,
>> +};
>> +
>> +#define R_CFG           (0x08/4)
>> +    #define RFIFO_TH_SHIFT       10
>> +    #define RFIFO_TH_LEN         2
>> +    #define WFIFO_TH_SHIFT       8
>> +    #define WFIFO_TH_LEN         2
>> +    #define DISABLE_SRC_INC      (1 << 5)
>> +    #define DISABLE_DST_INC      (1 << 4)
>> +#define R_CFG_RO 0xFFFFF800
>> +#define R_CFG_RESET 0x50B
>> +
>> +#define R_INT_STS       (0x0C/4)
>> +    #define PSS_GTS_USR_B_INT    (1 << 31)
>> +    #define PSS_FST_CFG_B_INT    (1 << 30)
>> +    #define PSS_CFG_RESET_B_INT  (1 << 27)
>> +    #define RX_FIFO_OV_INT       (1 << 18)
>> +    #define WR_FIFO_LVL_INT      (1 << 17)
>> +    #define RD_FIFO_LVL_INT      (1 << 16)
>> +    #define DMA_CMD_ERR_INT      (1 << 15)
>> +    #define DMA_Q_OV_INT         (1 << 14)
>> +    #define DMA_DONE_INT         (1 << 13)
>> +    #define DMA_P_DONE_INT       (1 << 12)
>> +    #define P2D_LEN_ERR_INT      (1 << 11)
>> +    #define PCFG_DONE_INT        (1 << 2)
>> +    #define R_INT_STS_RSVD       ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
>> +
>> +#define R_INT_MASK      (0x10/4)
>> +
>> +#define R_STATUS        (0x14/4)
>> +    #define DMA_CMD_Q_F         (1 << 31)
>> +    #define DMA_CMD_Q_E         (1 << 30)
>> +    #define DMA_DONE_CNT_SHIFT  28
>> +    #define DMA_DONE_CNT_LEN    2
>> +    #define RX_FIFO_LVL_SHIFT   20
>> +    #define RX_FIFO_LVL_LEN     5
>> +    #define TX_FIFO_LVL_SHIFT   12
>> +    #define TX_FIFO_LVL_LEN     7
>> +    #define TX_FIFO_LVL         (0x7f << 12)
>> +    #define PSS_GTS_USR_B       (1 << 11)
>> +    #define PSS_FST_CFG_B       (1 << 10)
>> +    #define PSS_CFG_RESET_B     (1 << 5)
>> +
>> +#define R_DMA_SRC_ADDR  (0x18/4)
>> +#define R_DMA_DST_ADDR  (0x1C/4)
>> +#define R_DMA_SRC_LEN   (0x20/4)
>> +#define R_DMA_DST_LEN   (0x24/4)
>> +#define R_ROM_SHADOW    (0x28/4)
>> +#define R_SW_ID         (0x30/4)
>> +#define R_UNLOCK        (0x34/4)
>> +
>> +#define R_UNLOCK_MAGIC 0x757BDF0D
>> +
>> +#define R_MCTRL         (0x80/4)
>> +    #define PS_VERSION_SHIFT    28
>> +    #define PS_VERSION_MASK     (0xf << PS_VERSION_SHIFT)
>> +    #define PCFG_POR_B          (1 << 8)
>> +    #define INT_PCAP_LPBK       (1 << 4)
>> +
>> +#define R_MAX (0x118/4+1)
>> +
>> +#define RX_FIFO_LEN 32
>> +#define TX_FIFO_LEN 128
>> +
>> +#define DMA_COMMAND_FIFO_LEN 10
>> +
>> +typedef struct XilinxDevcfgDMACommand {
>> +    uint32_t src_addr;
>> +    uint32_t dest_addr;
>> +    uint32_t src_len;
>> +    uint32_t dest_len;
>> +} XilinxDevcfgDMACommand;
>> +
>> +static const VMStateDescription vmstate_xilinx_devcfg_dma_command = {
>> +    .name = "xilinx_devcfg_dma_command",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(src_addr, XilinxDevcfgDMACommand),
>> +        VMSTATE_UINT32(dest_addr, XilinxDevcfgDMACommand),
>> +        VMSTATE_UINT32(src_len, XilinxDevcfgDMACommand),
>> +        VMSTATE_UINT32(dest_len, XilinxDevcfgDMACommand),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +typedef struct XilinxDevcfg {
>> +    SysBusDevice parent_obj;
>> +
>> +    MemoryRegion iomem;
>> +    qemu_irq irq;
>> +
>> +    QEMUBH *timer_bh;
>> +    ptimer_state *timer;
>> +
>> +    XilinxDevcfgDMACommand dma_command_fifo[DMA_COMMAND_FIFO_LEN];
>> +    uint8_t dma_command_fifo_num;
>> +
>> +    uint32_t regs[R_MAX];
>> +    RegisterInfo regs_info[R_MAX];
>> +} XilinxDevcfg;
>> +
>> +static const VMStateDescription vmstate_xilinx_devcfg = {
>> +    .name = "xilinx_devcfg",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_PTIMER(timer, XilinxDevcfg),
>> +        VMSTATE_STRUCT_ARRAY(dma_command_fifo, XilinxDevcfg,
>> +                             DMA_COMMAND_FIFO_LEN, 0,
>> +                             vmstate_xilinx_devcfg_dma_command,
>> +                             XilinxDevcfgDMACommand),
>> +        VMSTATE_UINT8(dma_command_fifo_num, XilinxDevcfg),
>> +        VMSTATE_UINT32_ARRAY(regs, XilinxDevcfg, R_MAX),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void xilinx_devcfg_update_ixr(XilinxDevcfg *s)
>> +{
>> +    qemu_set_irq(s->irq, !!(~s->regs[R_INT_MASK] & s->regs[R_INT_STS]));
>> +}
>> +
>> +static void xilinx_devcfg_reset(DeviceState *dev)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(dev);
>> +    int i;
>> +
>> +    for (i = 0; i < R_MAX; ++i) {
>> +        register_reset(&s->regs_info[i]);
>> +    }
>> +}
>> +
>> +#define min(a, b) ((a) < (b) ? (a) : (b))
>> +
>> +static void xilinx_devcfg_dma_go(void *opaque)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(opaque);
>> +    uint8_t buf[BTT_MAX];
>> +    XilinxDevcfgDMACommand *dmah = s->dma_command_fifo;
>> +    uint32_t btt = BTT_MAX;
>> +
>> +    btt = min(btt, dmah->src_len);
>> +    if (s->regs[R_MCTRL] & INT_PCAP_LPBK) {
>> +        btt = min(btt, dmah->dest_len);
>> +    }
>> +    DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
>> +    dma_memory_read(&dma_context_memory, dmah->src_addr, buf, btt);
>> +    dmah->src_len -= btt;
>> +    dmah->src_addr += btt;
>> +    if (s->regs[R_MCTRL] & INT_PCAP_LPBK) {
>> +        DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
>> +        dma_memory_write(&dma_context_memory, dmah->dest_addr, buf, btt);
>> +        dmah->dest_len -= btt;
>> +        dmah->dest_addr += btt;
>> +    }
>> +    if (!dmah->src_len && !dmah->dest_len) {
>> +        DB_PRINT("dma operation finished\n");
>> +        s->regs[R_INT_STS] |= DMA_DONE_INT | DMA_P_DONE_INT;
>> +        s->dma_command_fifo_num = s->dma_command_fifo_num - 1;
>> +        memcpy(s->dma_command_fifo, &s->dma_command_fifo[1],
>> +               sizeof(*s->dma_command_fifo) * DMA_COMMAND_FIFO_LEN - 1);
>> +    }
>> +    xilinx_devcfg_update_ixr(s);
>> +    if (s->dma_command_fifo_num) { /* there is still work to do */
>> +        DB_PRINT("dma work remains, setting up callback ptimer\n");
>> +        ptimer_set_freq(s->timer, FREQ_HZ);
>> +        ptimer_set_count(s->timer, CYCLES_BTT_MAX);
>> +        ptimer_run(s->timer, 1);
>> +    }
>> +}
>> +
>> +static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
>> +
>> +    xilinx_devcfg_update_ixr(s);
>> +}
>> +
>> +static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
>> +        if (s->regs[R_LOCK] & 1 << i) {
>> +            val &= ~lock_ctrl_map[i];
>> +            val |= lock_ctrl_map[i] & s->regs[R_CTRL];
>> +        }
>> +    }
>> +    return val;
>> +}
>> +
>> +static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> +    uint32_t aes_en = extract32(val, PCFG_AES_EN_SHIFT, PCFG_AES_EN_LEN);
>> +
>> +    if (aes_en != 0 && aes_en != 7) {
>> +        qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
>> +                      "unimplemeneted security reset should happen!\n",
>> +                      reg->prefix);
>> +    }
>> +}
>> +
>> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
>> +
>> +    if (val == R_UNLOCK_MAGIC) {
>> +        DB_PRINT("successful unlock\n");
>> +    } else {/* bad unlock attempt */
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", reg->prefix);
>> +        qemu_log_mask(LOG_UNIMP, "%s: failed unlock, unimplmeneted crash of"
>> +                      "device should happen\n", reg->prefix);
>> +        s->regs[R_CTRL] &= ~PCAP_PR;
>> +        s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
>> +    }
>> +}
>> +
>> +static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
>> +
>> +    /* once bits are locked they stay locked */
>> +    return s->regs[R_LOCK] | val;
>> +}
>> +
>> +static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
>> +
>> +    /* TODO: implement command queue overflow check and interrupt */
>> +    s->dma_command_fifo[s->dma_command_fifo_num].src_addr =
>> +            s->regs[R_DMA_SRC_ADDR] & ~0x3UL;
>> +    s->dma_command_fifo[s->dma_command_fifo_num].dest_addr =
>> +            s->regs[R_DMA_DST_ADDR] & ~0x3UL;
>> +    s->dma_command_fifo[s->dma_command_fifo_num].src_len =
>> +            s->regs[R_DMA_SRC_LEN] << 2;
>> +    s->dma_command_fifo[s->dma_command_fifo_num].dest_len =
>> +            s->regs[R_DMA_DST_LEN] << 2;
>> +    s->dma_command_fifo_num++;
>> +    DB_PRINT("dma transfer started; %d total transfers pending\n",
>> +             s->dma_command_fifo_num);
>> +    xilinx_devcfg_dma_go(s);
>> +}
>> +
>> +static const RegisterAccessInfo xilinx_devcfg_regs_info[] = {
>> +    [R_CTRL] = { .name = "CTRL",
>> +        .reset = PCAP_PR | PCAP_MODE | 0x3 << 13,
>> +        .ro = 0x107f6000,
>> +        .ge1 = (RegisterAccessError[]) {
>> +            { .mask = 0x1 << 15, .reason = "Reserved - do not modify" },
>> +            {},
>> +        },
>> +        .ge0 = (RegisterAccessError[]) {
>> +            { .mask = 0x3 << 13, .reason = "Reserved - always write with 1" },
>> +            {},
>> +        },
>> +        .ui1 = (RegisterAccessError[]) {
>> +            { .mask = FORCE_RST, .reason = "PS reset not implemented" },
>> +            { .mask = PCAP_MODE, .reason = "FPGA Fabric doesnt exist" },
>> +            { .mask = PCFG_AES_EN_MASK, .reason = "AES not implmented" },
>> +            {},
>> +        },
>> +        .pre_write = r_ctrl_pre_write,
>> +        .post_write = r_ctrl_post_write,
>
> I dislike that this mechanism decentralizes register access.  What about
> a slightly different style so you could do something like:
>
> static void my_device_io_write(...)
> {
>     switch (register_decode(&my_device_reginfo, s, &info)) {
>     case R_CTRL:
>         // handle pre-write bits here
>     ...
>     }
>    
>     switch (register_write(&my_device_reginfo, s)) {
>     case R_CTRL:
>         //handle post write bits
>     ...
>     }
> }
>
> It makes it much easier to convert existing code.  We can then leave
> most of the switch statements intact and just introduce register
> descriptions.
>
> I think splitting decode from data processing is useful too because it
> makes it easier to support latching.

Here's a more illustrated example.  I didn't try to make anything data
driven but I hope you can see how that would fit in.

I think there's a lot of value in splitting decode vs. load/store.  I
think it's important to allow certain things to remain open coded and if
you see how I did it, there are a few registers that are decoded and
load/stored in an open coded fashion remaining.

By separating decode, we can have universal tracing of specific register
access (even if we're open coding it).

I think reset is also out of scope for an API like this.  Particularly
when talking about latching, there isn't a 1-1 relationship anymore
between things with initial values and then the visibility within the
address space.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Demonstrate-separating-register-decode-from-get-set.patch --]
[-- Type: text/x-diff, Size: 9950 bytes --]

>From 8825ec58285957b8586e9439a84bbd5eca773fbb Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Wed, 29 May 2013 13:44:35 -0500
Subject: [PATCH] Demonstrate separating register decode from get/set

---
 hw/char/serial.c | 253 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 167 insertions(+), 86 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 66b6348..be637f5 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -299,6 +299,113 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
     return FALSE;
 }
 
+enum {
+    UART_INVALID = 0,
+    UART_DIVIDER,
+    UART_RBR,
+    UART_THR,
+    UART_IER,
+    UART_IIR,
+    UART_FCR,
+    UART_LCR,
+    UART_MCR,
+    UART_LSR,
+    UART_MSR,
+    UART_SCR,
+};
+
+static int serial_decode(void *opaque, hwaddr addr, unsigned size, bool is_write)
+{
+    SerialState *s = opaque;
+
+    switch (addr) {
+    case 0:
+        if (s->lcr & UART_LCR_DLAB) {
+            return UART_DIVIDER;
+        }
+        if (is_write) {
+            return UART_THR;
+        }
+        return UART_RBR;
+    case 1:
+        if (s->lcr & UART_LCR_DLAB) {
+            return UART_DIVIDER;
+        }
+        return UART_IER;
+    case 2:
+        if (is_write) {
+            return UART_FCR;
+        }
+        return UART_IIR;
+    case 3:
+        return UART_LCR;
+    case 4:
+        return UART_MCR;
+    case 5:
+        if (is_write) {
+            return UART_INVALID;
+        }
+        return UART_LSR;
+    case 6:
+        if (is_write) {
+            return UART_INVALID;
+        }
+        return UART_MSR;
+    case 7:
+        return UART_SCR;
+    default:
+        return UART_INVALID;
+    }
+}
+
+static int serial_load(void *opaque, int regno, unsigned size, uint64_t *ret)
+{
+    SerialState *s = opaque;
+
+    switch (regno) {
+    case UART_IER:
+        *ret = s->ier;
+        break;
+    case UART_IIR:
+        *ret = s->iir;
+        break;
+    case UART_LCR:
+        *ret = s->lcr;
+        break;
+    case UART_MCR:
+        *ret = s->mcr;
+        break;
+    case UART_LSR:
+        *ret = s->lsr;
+        break;
+    case UART_SCR:
+        *ret = s->scr;
+        break;
+    default:
+        break;
+    }
+
+    return regno;
+}
+
+static int serial_store(void *opaque, int regno, unsigned size, uint64_t val)
+{
+    SerialState *s = opaque;
+
+    switch (regno) {
+    case UART_THR:
+        s->thr = val;
+        break;
+    case UART_IER:
+        s->ier = val & 0x0f;
+        break;
+    case UART_LCR:
+        s->lcr = val;
+        break;
+    }
+
+    return regno;
+}
 
 static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                                 unsigned size)
@@ -307,52 +414,48 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
 
     addr &= 7;
     DPRINTF("write addr=0x%" HWADDR_PRIx " val=0x%" PRIx64 "\n", addr, val);
-    switch(addr) {
+    switch(serial_store(s, serial_decode(s, addr, size, true), size, val)) {
     default:
-    case 0:
-        if (s->lcr & UART_LCR_DLAB) {
+    case UART_DIVIDER:
+        if (addr == 0) {
             s->divider = (s->divider & 0xff00) | val;
-            serial_update_parameters(s);
         } else {
-            s->thr = (uint8_t) val;
-            if(s->fcr & UART_FCR_FE) {
-                fifo_put(s, XMIT_FIFO, s->thr);
-                s->thr_ipending = 0;
-                s->lsr &= ~UART_LSR_TEMT;
-                s->lsr &= ~UART_LSR_THRE;
-                serial_update_irq(s);
-            } else {
-                s->thr_ipending = 0;
-                s->lsr &= ~UART_LSR_THRE;
-                serial_update_irq(s);
-            }
-            serial_xmit(NULL, G_IO_OUT, s);
+            s->divider = (s->divider & 0x00ff) | (val << 8);
         }
+        serial_update_parameters(s);
         break;
-    case 1:
-        if (s->lcr & UART_LCR_DLAB) {
-            s->divider = (s->divider & 0x00ff) | (val << 8);
-            serial_update_parameters(s);
+    case UART_THR:
+        if(s->fcr & UART_FCR_FE) {
+            fifo_put(s, XMIT_FIFO, s->thr);
+            s->thr_ipending = 0;
+            s->lsr &= ~UART_LSR_TEMT;
+            s->lsr &= ~UART_LSR_THRE;
+            serial_update_irq(s);
         } else {
-            s->ier = val & 0x0f;
-            /* If the backend device is a real serial port, turn polling of the modem
-               status lines on physical port on or off depending on UART_IER_MSI state */
-            if (s->poll_msl >= 0) {
-                if (s->ier & UART_IER_MSI) {
-                     s->poll_msl = 1;
-                     serial_update_msl(s);
-                } else {
-                     qemu_del_timer(s->modem_status_poll);
-                     s->poll_msl = 0;
-                }
-            }
-            if (s->lsr & UART_LSR_THRE) {
-                s->thr_ipending = 1;
-                serial_update_irq(s);
+            s->thr_ipending = 0;
+            s->lsr &= ~UART_LSR_THRE;
+            serial_update_irq(s);
+        }
+        serial_xmit(NULL, G_IO_OUT, s);
+        break;
+    case UART_IER:
+        /* If the backend device is a real serial port, turn polling of the modem
+           status lines on physical port on or off depending on UART_IER_MSI state */
+        if (s->poll_msl >= 0) {
+            if (s->ier & UART_IER_MSI) {
+                s->poll_msl = 1;
+                serial_update_msl(s);
+            } else {
+                qemu_del_timer(s->modem_status_poll);
+                s->poll_msl = 0;
             }
         }
+        if (s->lsr & UART_LSR_THRE) {
+            s->thr_ipending = 1;
+            serial_update_irq(s);
+        }
         break;
-    case 2:
+    case UART_FCR:
         val = val & 0xFF;
 
         if (s->fcr == val)
@@ -398,10 +501,9 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         s->fcr = val & 0xC9;
         serial_update_irq(s);
         break;
-    case 3:
+    case UART_LCR:
         {
             int break_enable;
-            s->lcr = val;
             serial_update_parameters(s);
             break_enable = (val >> 6) & 1;
             if (break_enable != s->last_break_enable) {
@@ -411,7 +513,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
             }
         }
         break;
-    case 4:
+    case UART_MCR:
         {
             int flags;
             int old_mcr = s->mcr;
@@ -437,75 +539,57 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
             }
         }
         break;
-    case 5:
-        break;
-    case 6:
-        break;
-    case 7:
-        s->scr = val;
-        break;
     }
 }
 
 static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
 {
     SerialState *s = opaque;
-    uint32_t ret;
+    uint64_t ret = 0xFF;
 
     addr &= 7;
-    switch(addr) {
+    switch(serial_load(s, serial_decode(s, addr, size, false), size, &ret)) {
     default:
-    case 0:
-        if (s->lcr & UART_LCR_DLAB) {
+        break;
+    case UART_DIVIDER:
+        if (addr == 0) {
             ret = s->divider & 0xff;
         } else {
-            if(s->fcr & UART_FCR_FE) {
-                ret = fifo_get(s,RECV_FIFO);
-                if (s->recv_fifo.count == 0)
-                    s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
-                else
-                    qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);
-                s->timeout_ipending = 0;
-            } else {
-                ret = s->rbr;
-                s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
-            }
-            serial_update_irq(s);
-            if (!(s->mcr & UART_MCR_LOOP)) {
-                /* in loopback mode, don't receive any data */
-                qemu_chr_accept_input(s->chr);
-            }
+            ret = (s->divider >> 8) & 0xff;
         }
         break;
-    case 1:
-        if (s->lcr & UART_LCR_DLAB) {
-            ret = (s->divider >> 8) & 0xff;
+    case UART_RBR:
+        if(s->fcr & UART_FCR_FE) {
+            ret = fifo_get(s,RECV_FIFO);
+            if (s->recv_fifo.count == 0)
+                s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
+            else
+                qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);
+            s->timeout_ipending = 0;
         } else {
-            ret = s->ier;
+            ret = s->rbr;
+            s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
+        }
+        serial_update_irq(s);
+        if (!(s->mcr & UART_MCR_LOOP)) {
+            /* in loopback mode, don't receive any data */
+            qemu_chr_accept_input(s->chr);
         }
         break;
-    case 2:
-        ret = s->iir;
-        if ((ret & UART_IIR_ID) == UART_IIR_THRI) {
+    case UART_IIR:
+        if ((s->iir & UART_IIR_ID) == UART_IIR_THRI) {
             s->thr_ipending = 0;
             serial_update_irq(s);
         }
         break;
-    case 3:
-        ret = s->lcr;
-        break;
-    case 4:
-        ret = s->mcr;
-        break;
-    case 5:
-        ret = s->lsr;
+    case UART_LSR:
         /* Clear break and overrun interrupts */
         if (s->lsr & (UART_LSR_BI|UART_LSR_OE)) {
             s->lsr &= ~(UART_LSR_BI|UART_LSR_OE);
             serial_update_irq(s);
         }
         break;
-    case 6:
+    case UART_MSR:
         if (s->mcr & UART_MCR_LOOP) {
             /* in loopback, the modem output pins are connected to the
                inputs */
@@ -523,9 +607,6 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
             }
         }
         break;
-    case 7:
-        ret = s->scr;
-        break;
     }
     DPRINTF("read addr=0x%" HWADDR_PRIx " val=0x%02x\n", addr, ret);
     return ret;
-- 
1.8.0


[-- Attachment #3: Type: text/plain, Size: 5367 bytes --]


Regards,

Anthony Liguori

>
> I think the current spin is probably over generalizing too.  I don't
> think supporting ge0/ge1 makes a lot of sense from the start.  Decisions
> aren't always that simple and it's weird to have sanity checking split
> across two places.
>
> BTW: I think it's also a good idea to model this as a QOM object so that
> device state can be access through the QOM tree.
>
> Regards,
>
> Anthony Liguori
>
>> +    },
>> +    [R_LOCK] = { .name = "LOCK",
>> +        .ro = ~ONES(5),
>> +        .pre_write = r_lock_pre_write,
>> +    },
>> +    [R_CFG] = { .name = "CFG",
>> +        .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8,
>> +        .ge1 = (RegisterAccessError[]) {
>> +            { .mask = 0x7, .reason = "Reserved - do not modify" },
>> +            {},
>> +        },
>> +        .ge0 = (RegisterAccessError[]) {
>> +            { .mask = 0x8, .reason = "Reserved - do not modify" },
>> +            {},
>> +        },
>> +        .ro = 0x00f | ~ONES(12),
>> +    },
>> +    [R_INT_STS] = { .name = "INT_STS",
>> +        .w1c = ~R_INT_STS_RSVD,
>> +        .reset = PSS_GTS_USR_B_INT | PSS_CFG_RESET_B_INT | WR_FIFO_LVL_INT,
>> +        .ro = R_INT_STS_RSVD,
>> +        .post_write = r_ixr_post_write,
>> +    },
>> +    [R_INT_MASK] = { .name = "INT_MASK",
>> +        .reset = ~0,
>> +        .ro = R_INT_STS_RSVD,
>> +        .post_write = r_ixr_post_write,
>> +    },
>> +    [R_STATUS] = { .name = "STATUS",
>> +        .reset = DMA_CMD_Q_E | PSS_GTS_USR_B | PSS_CFG_RESET_B,
>> +        .ro = ~0,
>> +        .ge1 = (RegisterAccessError[])  {
>> +            {.mask = 0x1, .reason = "Reserved - do not modify" },
>> +            {},
>> +        },
>> +    },
>> +    [R_DMA_SRC_ADDR] = { .name = "DMA_SRC_ADDR" },
>> +    [R_DMA_DST_ADDR] = { .name = "DMA_DST_ADDR" },
>> +    [R_DMA_SRC_LEN] = { .name = "DMA_SRC_LEN", .ro = ~ONES(27) },
>> +    [R_DMA_DST_LEN] = { .name = "DMA_DST_LEN",
>> +        .ro = ~ONES(27),
>> +        .post_write = r_dma_dst_len_post_write,
>> +    },
>> +    [R_ROM_SHADOW] = { .name = "ROM_SHADOW",
>> +        .ge1 = (RegisterAccessError[])  {
>> +            {.mask = ~0, .reason = "Reserved - do not modify" },
>> +            {},
>> +        },
>> +    },
>> +    [R_SW_ID] = { .name = "SW_ID" },
>> +    [R_UNLOCK] = { .name = "UNLOCK",
>> +        .post_write = r_unlock_post_write,
>> +    },
>> +    [R_MCTRL] = { .name = "MCTRL",
>> +        /* Silicon 3.0 for version field, and the mysterious reserved bit 23 */
>> +        .reset = 0x2 << PS_VERSION_SHIFT | 1 << 23,
>> +        /* some reserved bits are rw while others are ro */
>> +        .ro = ~INT_PCAP_LPBK,
>> +        .ge1 = (RegisterAccessError[]) {
>> +            { .mask = 0x00F00300, .reason = "Reserved - do not modify" },
>> +            { .mask = 0x00000003, .reason = "Reserved - always write with 0" },
>> +            {}
>> +        },
>> +        .ge0 = (RegisterAccessError[]) {
>> +            { .mask = 1 << 23, .reason = "Reserved - always write with 1" },
>> +            {}
>> +        },
>> +    },
>> +    [R_MAX] = {}
>> +};
>> +
>> +static const MemoryRegionOps devcfg_reg_ops = {
>> +    .read = register_read_memory_le,
>> +    .write = register_write_memory_le,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    }
>> +};
>> +
>> +static void xilinx_devcfg_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(dev);
>> +    const char *prefix = object_get_canonical_path(OBJECT(dev));
>> +    int i;
>> +
>> +    for (i = 0; i < R_MAX; ++i) {
>> +        RegisterInfo *r = &s->regs_info[i];
>> +
>> +        *r = (RegisterInfo) {
>> +            .data = &s->regs[i],
>> +            .data_size = sizeof(uint32_t),
>> +            .access = &xilinx_devcfg_regs_info[i],
>> +            .debug = XILINX_DEVCFG_ERR_DEBUG,
>> +            .prefix = prefix,
>> +            .opaque = s,
>> +        };
>> +        memory_region_init_io(&r->mem, &devcfg_reg_ops, r, "devcfg-regs", 4);
>> +        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
>> +    }
>> +}
>> +
>> +static void xilinx_devcfg_init(Object *obj)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    XilinxDevcfg *s = XILINX_DEVCFG(obj);
>> +
>> +    s->timer_bh = qemu_bh_new(xilinx_devcfg_dma_go, s);
>> +    s->timer = ptimer_init(s->timer_bh);
>> +
>> +    sysbus_init_irq(sbd, &s->irq);
>> +
>> +    memory_region_init(&s->iomem, "devcfg", R_MAX*4);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +}
>> +
>> +static void xilinx_devcfg_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->reset = xilinx_devcfg_reset;
>> +    dc->vmsd = &vmstate_xilinx_devcfg;
>> +    dc->realize = xilinx_devcfg_realize;
>> +}
>> +
>> +static const TypeInfo xilinx_devcfg_info = {
>> +    .name           = TYPE_XILINX_DEVCFG,
>> +    .parent         = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size  = sizeof(XilinxDevcfg),
>> +    .instance_init  = xilinx_devcfg_init,
>> +    .class_init     = xilinx_devcfg_class_init,
>> +};
>> +
>> +static void xilinx_devcfg_register_types(void)
>> +{
>> +    type_register_static(&xilinx_devcfg_info);
>> +}
>> +
>> +type_init(xilinx_devcfg_register_types)
>> -- 
>> 1.8.3.rc1.44.gb387c77.dirty