Re: [Qemu-devel] [PATCH v4 09/16] dma: Add Xilinx Zynq devcfg device model

[Alex Bennée]

Alistair Francis <address@hidden> writes:

> From: Peter Crosthwaite <address@hidden>
>
> Minimal device model for devcfg module of Zynq. DMA capabilities and
> interrupt generation supported.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>
>  default-configs/arm-softmmu.mak   |   1 +
>  hw/dma/Makefile.objs              |   1 +
>  hw/dma/xlnx-zynq-devcfg.c         | 397 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/dma/xlnx-zynq-devcfg.h |  62 ++++++
>  4 files changed, 461 insertions(+)
>  create mode 100644 hw/dma/xlnx-zynq-devcfg.c
>  create mode 100644 include/hw/dma/xlnx-zynq-devcfg.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index a9f82a1..d524584 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -66,6 +66,7 @@ CONFIG_PXA2XX=y
>  CONFIG_BITBANG_I2C=y
>  CONFIG_FRAMEBUFFER=y
>  CONFIG_XILINX_SPIPS=y
> +CONFIG_ZYNQ_DEVCFG=y
>
>  CONFIG_ARM11SCU=y
>  CONFIG_A9SCU=y
> diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
> index 0e65ed0..eaf0a81 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) += xlnx-zynq-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/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
> new file mode 100644
> index 0000000..0420123
> --- /dev/null
> +++ b/hw/dma/xlnx-zynq-devcfg.c
> @@ -0,0 +1,397 @@
> +/*
> + * QEMU model of the Xilinx Zynq Devcfg Interface
> + *
> + * (C) 2011 PetaLogix Pty Ltd
> + * (C) 2014 Xilinx Inc.
> + * Written by Peter Crosthwaite <address@hidden>
> + *
> + * 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 "hw/dma/xlnx-zynq-devcfg.h"
> +#include "qemu/bitops.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/dma.h"
> +
> +#define FREQ_HZ 900000000
> +
> +#define BTT_MAX 0x400
> +
> +#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
> +#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT(...) do { \
> +    if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
> +        qemu_log("%s: ", __func__); \
> +        qemu_log(__VA_ARGS__); \
> +    } \
> +} while (0);

This can be done in one qemu_log invocation as per other patches.

> +
> +REG32(CTRL, 0x00)
> +    FIELD(CTRL,     FORCE_RST,          31,  1) /* Not supported, wr ignored 
> */
> +    FIELD(CTRL,     PCAP_PR,            27,  1) /* Forced to 0 on bad unlock 
> */
> +    FIELD(CTRL,     PCAP_MODE,          26,  1)
> +    FIELD(CTRL,     MULTIBOOT_EN,       24,  1)
> +    FIELD(CTRL,     USER_MODE,          15,  1)
> +    FIELD(CTRL,     PCFG_AES_FUSE,      12,  1)
> +    FIELD(CTRL,     PCFG_AES_EN,         9,  3)
> +    FIELD(CTRL,     SEU_EN,              8,  1)
> +    FIELD(CTRL,     SEC_EN,              7,  1)
> +    FIELD(CTRL,     SPNIDEN,             6,  1)
> +    FIELD(CTRL,     SPIDEN,              5,  1)
> +    FIELD(CTRL,     NIDEN,               4,  1)
> +    FIELD(CTRL,     DBGEN,               3,  1)
> +    FIELD(CTRL,     DAP_EN,              0,  3)
> +
> +REG32(LOCK, 0x04)
> +    #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] = R_CTRL_PCFG_AES_FUSE_MASK,
> +    [AES_EN_LOCK]   = R_CTRL_PCFG_AES_EN_MASK,
> +    [SEU_LOCK]      = R_CTRL_SEU_EN_MASK,
> +    [SEC_LOCK]      = R_CTRL_SEC_EN_MASK,
> +    [DBG_LOCK]      = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
> +                      R_CTRL_NIDEN_MASK   | R_CTRL_DBGEN_MASK  |
> +                      R_CTRL_DAP_EN_MASK,
> +};
> +
> +REG32(CFG, 0x08)
> +    FIELD(CFG,      RFIFO_TH,           10,  2)
> +    FIELD(CFG,      WFIFO_TH,            8,  2)
> +    FIELD(CFG,      RCLK_EDGE,           7,  1)
> +    FIELD(CFG,      WCLK_EDGE,           6,  1)
> +    FIELD(CFG,      DISABLE_SRC_INC,     5,  1)
> +    FIELD(CFG,      DISABLE_DST_INC,     4,  1)
> +#define R_CFG_RO 0xFFFFF000

If this was:

FIELD(CFG, RO, 12, 20)

Wouldn't you get:

R_CFG_RO_MASK for free?

> +#define R_CFG_RESET 0x50B
> +
> +REG32(INT_STS, 0x0C)
> +    FIELD(INT_STS,  PSS_GTS_USR_B,      31,  1)
> +    FIELD(INT_STS,  PSS_FST_CFG_B,      30,  1)
> +    FIELD(INT_STS,  PSS_CFG_RESET_B,    27,  1)
> +    FIELD(INT_STS,  RX_FIFO_OV,         18,  1)
> +    FIELD(INT_STS,  WR_FIFO_LVL,        17,  1)
> +    FIELD(INT_STS,  RD_FIFO_LVL,        16,  1)
> +    FIELD(INT_STS,  DMA_CMD_ERR,        15,  1)
> +    FIELD(INT_STS,  DMA_Q_OV,           14,  1)
> +    FIELD(INT_STS,  DMA_DONE,           13,  1)
> +    FIELD(INT_STS,  DMA_P_DONE,         12,  1)
> +    FIELD(INT_STS,  P2D_LEN_ERR,        11,  1)
> +    FIELD(INT_STS,  PCFG_DONE,           2,  1)
> +    #define R_INT_STS_RSVD       ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
> +
> +REG32(INT_MASK, 0x10)
> +
> +REG32(STATUS, 0x14)
> +    FIELD(STATUS,   DMA_CMD_Q_F,        31,  1)
> +    FIELD(STATUS,   DMA_CMD_Q_E,        30,  1)
> +    FIELD(STATUS,   DMA_DONE_CNT,       28,  2)
> +    FIELD(STATUS,   RX_FIFO_LVL,        20,  5)
> +    FIELD(STATUS,   TX_FIFO_LVL,        12,  7)
> +    FIELD(STATUS,   PSS_GTS_USR_B,      11,  1)
> +    FIELD(STATUS,   PSS_FST_CFG_B,      10,  1)
> +    FIELD(STATUS,   PSS_CFG_RESET_B,     5,  1)
> +
> +REG32(DMA_SRC_ADDR, 0x18)
> +REG32(DMA_DST_ADDR, 0x1C)
> +REG32(DMA_SRC_LEN, 0x20)
> +REG32(DMA_DST_LEN, 0x24)
> +REG32(ROM_SHADOW, 0x28)
> +REG32(SW_ID, 0x30)
> +REG32(UNLOCK, 0x34)
> +
> +#define R_UNLOCK_MAGIC 0x757BDF0D
> +
> +REG32(MCTRL, 0x80)
> +    FIELD(MCTRL,    PS_VERSION,         28,  4)
> +    FIELD(MCTRL,    PCFG_POR_B,          8,  1)
> +    FIELD(MCTRL,    INT_PCAP_LPBK,       4,  1)
> +    FIELD(MCTRL,    QEMU,                3,  1)
> +
> +static void xlnx_zynq_devcfg_update_ixr(XlnxZynqDevcfg *s)
> +{
> +    qemu_set_irq(s->irq, ~s->regs[R_INT_MASK] & s->regs[R_INT_STS]);

What defines R_INT_MASK? It looks as though only FIELD() definitions
give R_reg_field_MASK defines?

This is the problem with the "auto-magic" derived names. If I grep the
source code for R_INT_MASK I only find the use. Actually now I realise
that INT_MASK is a register name not a field. I think we need access
helpers to guide the user. ~s->regs[GET_REG_NUM(INT_MASK)] would prompt
the user to search for INT_MASK to find the original definitions.

> +}
> +
> +static void xlnx_zynq_devcfg_reset(DeviceState *dev)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(dev);
> +    int i;
> +
> +    for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
> +        register_reset(&s->regs_info[i]);
> +    }
> +}
> +
> +static void xlnx_zynq_devcfg_dma_go(XlnxZynqDevcfg *s)
> +{
> +    for (;;) {
> +        uint8_t buf[BTT_MAX];
> +        XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
> +        uint32_t btt = BTT_MAX;
> +        bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
> +
> +        btt = MIN(btt, dmah->src_len);
> +        if (loopback) {
> +            btt = MIN(btt, dmah->dest_len);
> +        }
> +        DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
> +        dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
> +        dmah->src_len -= btt;
> +        dmah->src_addr += btt;
> +        if (loopback) {
> +            DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
> +            dma_memory_write(&address_space_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] |= R_INT_STS_DMA_DONE_MASK |
> +                                  R_INT_STS_DMA_P_DONE_MASK;
> +            s->dma_cmd_fifo_num--;
> +            memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
> +                    sizeof(*s->dma_cmd_fifo) * 
> XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN
> +                                                                        - 1);
> +        }
> +        xlnx_zynq_devcfg_update_ixr(s);
> +        if (!s->dma_cmd_fifo_num) { /* All done */
> +            return;
> +        }
> +    }

I noticed this before in other patches I think. I guess it is a
stylistic choice but certainly for a single well defined end condition
do () while {} reads more naturally.

> +}
> +
> +static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> +    xlnx_zynq_devcfg_update_ixr(s);
> +}
> +
> +static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_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 = F_EX32(val, CTRL, PCFG_AES_EN);
> +
> +    if (aes_en != 0 && aes_en != 7) {
> +        qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
> +                      "unimplemented security reset should happen!\n",
> +                      reg->prefix);
> +    }
> +}
> +
> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> +    if (val == R_UNLOCK_MAGIC) {
> +        DB_PRINT("successful unlock\n");
> +        /* BootROM will have already done the actual unlock so no need to do
> +         * anything in successful subsequent unlock
> +         */
> +    } else { /* bad unlock attempt */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", reg->prefix);
> +        s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
> +        s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
> +        /* core becomes inaccessible */
> +        memory_region_set_enabled(&s->iomem, false);
> +    }
> +}
> +
> +static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_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)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> +    s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
> +            .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
> +            .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
> +            .src_len = s->regs[R_DMA_SRC_LEN] << 2,
> +            .dest_len = s->regs[R_DMA_DST_LEN] << 2,
> +    };
> +    s->dma_cmd_fifo_num++;
> +    DB_PRINT("dma transfer started; %d total transfers pending\n",
> +             s->dma_cmd_fifo_num);
> +    xlnx_zynq_devcfg_dma_go(s);
> +}
> +
> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
> +    {   .name = "CTRL",                 .decode.addr = A_CTRL,
> +        .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
> +        .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
> +        .pre_write = r_ctrl_pre_write,
> +        .post_write = r_ctrl_post_write,
> +    },
> +    {   .name = "LOCK",                 .decode.addr = A_LOCK,

The same comment can be mode for the automagic addresses.
GET_REG_ADDR(LOCK) would be clearer.

> +        .rsvd = ~ONES(5),
> +        .pre_write = r_lock_pre_write,
> +    },
> +    {   .name = "CFG",                  .decode.addr = A_CFG,
> +        .reset = 1 << R_CFG_RFIFO_TH_SHIFT | 1 << R_CFG_WFIFO_TH_SHIFT | 0x8,
> +        .rsvd = 0xfffff00f,
> +    },
> +    {   .name = "INT_STS",              .decode.addr = A_INT_STS,
> +        .w1c = ~R_INT_STS_RSVD,
> +        .reset = R_INT_STS_PSS_GTS_USR_B_MASK   |
> +                 R_INT_STS_PSS_CFG_RESET_B_MASK |
> +                 R_INT_STS_WR_FIFO_LVL_MASK,
> +        .rsvd = R_INT_STS_RSVD,
> +        .post_write = r_ixr_post_write,
> +    },
> +    {   .name = "INT_MASK",            .decode.addr = A_INT_MASK,
> +        .reset = ~0,
> +        .rsvd = R_INT_STS_RSVD,
> +        .post_write = r_ixr_post_write,
> +    },
> +    {   .name = "STATUS",               .decode.addr = A_STATUS,
> +        .reset = R_STATUS_DMA_CMD_Q_E_MASK      |
> +                 R_STATUS_PSS_GTS_USR_B_MASK    |
> +                 R_STATUS_PSS_CFG_RESET_B_MASK,
> +        .ro = ~0,
> +    },
> +    {   .name = "DMA_SRC_ADDR",         .decode.addr = A_DMA_SRC_ADDR, },
> +    {   .name = "DMA_DST_ADDR",         .decode.addr = A_DMA_DST_ADDR, },
> +    {   .name = "DMA_SRC_LEN",          .decode.addr = A_DMA_SRC_LEN,
> +        .ro = ~ONES(27) },
> +    {   .name = "DMA_DST_LEN",          .decode.addr = A_DMA_DST_LEN,
> +        .ro = ~ONES(27),
> +        .post_write = r_dma_dst_len_post_write,
> +    },
> +    {   .name = "ROM_SHADOW",           .decode.addr = A_ROM_SHADOW,
> +        .rsvd = ~0ull,
> +    },
> +    {   .name = "SW_ID",                .decode.addr = A_SW_ID, },
> +    {   .name = "UNLOCK",               .decode.addr = A_UNLOCK,
> +        .post_write = r_unlock_post_write,
> +    },
> +    {   .name = "MCTRL",                .decode.addr = R_MCTRL * 4,
> +       /* Silicon 3.0 for version field, the mysterious reserved bit 23
> +        * and QEMU platform identifier.
> +        */
> +       .reset = 0x2 << R_MCTRL_PS_VERSION_SHIFT | 1 << 23 | 
> R_MCTRL_QEMU_MASK,
> +       .ro = ~R_MCTRL_INT_PCAP_LPBK_MASK,
> +       .rsvd = 0x00f00303,
> +    },
> +};
> +
> +static const MemoryRegionOps xlnx_zynq_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 const VMStateDescription vmstate_xlnx_zynq_devcfg_dma_cmd = {
> +    .name = "xlnx_zynq_devcfg_dma_cmd",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(src_addr, XlnxZynqDevcfgDMACmd),
> +        VMSTATE_UINT32(dest_addr, XlnxZynqDevcfgDMACmd),
> +        VMSTATE_UINT32(src_len, XlnxZynqDevcfgDMACmd),
> +        VMSTATE_UINT32(dest_len, XlnxZynqDevcfgDMACmd),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
> +    .name = "xlnx_zynq_devcfg",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
> +                             XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
> +                             vmstate_xlnx_zynq_devcfg_dma_cmd,
> +                             XlnxZynqDevcfgDMACmd),
> +        VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
> +        VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void xlnx_zynq_devcfg_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(obj);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
> +    register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
> +                          ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
> +                          s->regs_info, s->regs, &s->iomem,
> +                          &xlnx_zynq_devcfg_reg_ops,
> +                          XLNX_ZYNQ_DEVCFG_ERR_DEBUG);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static void xlnx_zynq_devcfg_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = xlnx_zynq_devcfg_reset;
> +    dc->vmsd = &vmstate_xlnx_zynq_devcfg;
> +}
> +
> +static const TypeInfo xlnx_zynq_devcfg_info = {
> +    .name           = TYPE_XLNX_ZYNQ_DEVCFG,
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(XlnxZynqDevcfg),
> +    .instance_init  = xlnx_zynq_devcfg_init,
> +    .class_init     = xlnx_zynq_devcfg_class_init,
> +};
> +
> +static void xlnx_zynq_devcfg_register_types(void)
> +{
> +    type_register_static(&xlnx_zynq_devcfg_info);
> +}
> +
> +type_init(xlnx_zynq_devcfg_register_types)
> diff --git a/include/hw/dma/xlnx-zynq-devcfg.h 
> b/include/hw/dma/xlnx-zynq-devcfg.h
> new file mode 100644
> index 0000000..d40e5c8
> --- /dev/null
> +++ b/include/hw/dma/xlnx-zynq-devcfg.h
> @@ -0,0 +1,62 @@
> +/*
> + * QEMU model of the Xilinx Devcfg Interface
> + *
> + * (C) 2011 PetaLogix Pty Ltd
> + * (C) 2014 Xilinx Inc.
> + * Written by Peter Crosthwaite <address@hidden>
> + *
> + * 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.
> + */
> +
> +#ifndef XLNX_ZYNQ_DEVCFG_H
> +
> +#include "hw/register.h"
> +#include "hw/sysbus.h"
> +
> +#define TYPE_XLNX_ZYNQ_DEVCFG "xlnx.ps7-dev-cfg"
> +
> +#define XLNX_ZYNQ_DEVCFG(obj) \
> +    OBJECT_CHECK(XlnxZynqDevcfg, (obj), TYPE_XLNX_ZYNQ_DEVCFG)
> +
> +#define XLNX_ZYNQ_DEVCFG_R_MAX 0x118
> +
> +#define XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN 10
> +
> +typedef struct XlnxZynqDevcfgDMACmd {
> +    uint32_t src_addr;
> +    uint32_t dest_addr;
> +    uint32_t src_len;
> +    uint32_t dest_len;
> +} XlnxZynqDevcfgDMACmd;
> +
> +typedef struct XlnxZynqDevcfg {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +
> +    XlnxZynqDevcfgDMACmd dma_cmd_fifo[XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN];
> +    uint8_t dma_cmd_fifo_num;
> +
> +    uint32_t regs[XLNX_ZYNQ_DEVCFG_R_MAX];
> +    RegisterInfo regs_info[XLNX_ZYNQ_DEVCFG_R_MAX];
> +} XlnxZynqDevcfg;
> +
> +#define XLNX_ZYNQ_DEVCFG_H
> +#endif


Sp having looked through the macro magic I'm happier with what it is
doing. Using accessors will help with the navigation though. The common
question someone looking over the code will want to answer is where is X
defined.

--
Alex Bennée