Patchwork [v3,4/5] xilinx_devcfg: Zynq devcfg device model

login
register
mail settings
Submitter Peter Crosthwaite
Date May 24, 2013, 5:49 a.m.
Message ID <ec9a490e1bde245667fd70ee23ca9f02644296c6.1369374096.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/246077/
State New
Headers show

Comments

Peter Crosthwaite - May 24, 2013, 5:49 a.m.
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
Paolo Bonzini - May 29, 2013, 8:51 a.m.
Il 24/05/2013 07:49, peter.crosthwaite@xilinx.com ha scritto:
> +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,

What happens if you have registers of mixed size within the same "bank"?

> +    }
> +};
> +
> +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);

Could you add a register_init function that does register_reset +
memory_region_init_io?

> +        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);

Paolo
Peter Crosthwaite - May 29, 2013, 1:54 p.m.
Hi Paolo,

On Wed, May 29, 2013 at 6:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/05/2013 07:49, peter.crosthwaite@xilinx.com ha scritto:
>> +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,
>
> What happens if you have registers of mixed size within the same "bank"?
>

You Probably should change these access restrictions per-register
accordingly. Note that min_access_size = 4 is a restriction of the
devcfg device and not the register API. We could change that to 1 no
problems. max_access should probably be no greater than the size of
the register, otherwise you have defined a non-nonsensical memory
region for the register. But note that defining it as less than the
register size is perfectly valid (for the weird case where you allow
e.g. only byte accesses to a register described as 32 bits).

The degenerate case is allowing greater-than-register size or
misaligned accesses, which is dependent on the behaviour of the memory
API. I can do some research, but do you know if the memory API
supports misaligned transactions that span a memory region boundary?
If so then there are no concerns, as the memory API will handle it for
us. If not then I don't see it as an issue as its very rare (at least
in my experience) for registers to support such strange misaligned or
mulit-register accesses. That or we can look into memory API for
answers.

>> +    }
>> +};
>> +
>> +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);
>
> Could you add a register_init function that does register_reset +
> memory_region_init_io?
>

I wanted to factor this loop out into a helper as a next step so I
think its already in my todo list, but i'm confused as to why reset
and memory_region_init would bundled together - arent they normally in
Device::reset and Device::realize (or Object::Init) respectively?

Regards,
Peter

>> +        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
>
> Paolo
>
Paolo Bonzini - May 29, 2013, 2:03 p.m.
Il 29/05/2013 15:54, Peter Crosthwaite ha scritto:
> Hi Paolo,
> 
> On Wed, May 29, 2013 at 6:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 24/05/2013 07:49, peter.crosthwaite@xilinx.com ha scritto:
>>> +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,
>>
>> What happens if you have registers of mixed size within the same "bank"?
>>
> 
> You Probably should change these access restrictions per-register
> accordingly.

Can you add a generic .valid.accepts callback for the register API that
will check against the size of the register at that offset?

> The degenerate case is allowing greater-than-register size or
> misaligned accesses, which is dependent on the behaviour of the memory
> API. I can do some research, but do you know if the memory API
> supports misaligned transactions that span a memory region boundary?

I don't really care about that for now.

But the way to do that would be to add a .impl.accepts method (right now
there's only .valid.accepts).  Then you would point the register API's
implementation to .impl.accepts, and use

    .impl.accepts = register_accepts,
    .valid = {
         .min_access_size = 1,
         .max_access_size = 4,
     }

The memory API will do a RMW operation if needed.  Won't make much sense
with write-1-clear bits and the like, but for simple registers it would
work.

Paolo

> If so then there are no concerns, as the memory API will handle it for
> us. If not then I don't see it as an issue as its very rare (at least
> in my experience) for registers to support such strange misaligned or
> mulit-register accesses. That or we can look into memory API for
> answers.
> 
>>> +    }
>>> +};
>>> +
>>> +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);
>>
>> Could you add a register_init function that does register_reset +
>> memory_region_init_io?
>>
> 
> I wanted to factor this loop out into a helper as a next step so I
> think its already in my todo list, but i'm confused as to why reset
> and memory_region_init would bundled together - arent they normally in
> Device::reset and Device::realize (or Object::Init) respectively?
> 
> Regards,
> Peter
> 
>>> +        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
>>
>> Paolo
>>
Edgar Iglesias - May 29, 2013, 5:04 p.m.
On Fri, May 24, 2013 at 03:49:00PM +1000, peter.crosthwaite@xilinx.com wrote:
> 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,
> +    },
> +    [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);

Hi Peter, Should we be putting r->access->name here instead of "devcfg-regs"?
Paolo Bonzini - May 29, 2013, 5:08 p.m.
Il 29/05/2013 19:04, Edgar E. Iglesias ha scritto:
>> > +    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);
> Hi Peter, Should we be putting r->access->name here instead of "devcfg-regs"?

Yes, that's why I preferred to wrap the memory_region_init_io into an
API that takes a RegisterInfo. :)

Paolo
Anthony Liguori - May 29, 2013, 5:57 p.m.
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.

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
Peter Crosthwaite - May 30, 2013, 1:43 a.m.
Hi Anthony,

On Thu, May 30, 2013 at 3:57 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> 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
>     ...
>     }
> }
>

That's still possible using just the register API (Patch 2 content
only) and throwing away the memory API glue. I think its actually
quite similar to V1 of the patch series where I did not have the
callbacks, and use the switch-cases for register side-effects only.
This looks like the intention of your patch. Gerd made a push for the
callbacks in an earlier review and there was push to integrate to
Memory API . Is it reasonable to leave it up to the developer whether
they want to do full conversion (Patches 2 & 3) or half conversion
(Patch 2 only + your decode refactoring). V1 of this patch at:

http://patchwork.ozlabs.org/patch/224534/

heres the relevant snippet (comments from me inline):

+static void xilinx_devcfg_write(void *opaque, hwaddr addr, uint64_t value,
+                                unsigned size)
+{
+    XilinxDevcfg *s = opaque;
+    int i;
+    uint32_t aes_en;
+    const char *prefix = "";
+
+    /* FIXME: use tracing to avoid these gymnastics */
+    if (XILINX_DEVCFG_ERR_DEBUG || qemu_get_log_flags() & LOG_GUEST_ERROR) {
+        prefix = g_strdup_printf("%s:Addr %#08x",
+                                 object_get_canonical_path(OBJECT(s)),
+                                 (unsigned)addr);
+    }
+    addr >>= 2;

This is the open coded decode logic but is trivial in this case.

+    assert(addr < R_MAX);
+
+    if (s->lock && addr != R_UNLOCK) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s:write to non-lock register while"
+                      " locked\n", prefix);
+        return;
+    }
+

some pre write logic (I dropped it later as it was actually unimplemented).

+    uint32_write(&s->regs[addr], &xilinx_devcfg_regs_info[addr], value, prefix,
+                 XILINX_DEVCFG_ERR_DEBUG);
+

this is the data-driven register_write() call under its old name.

+    /*side effects */
+    switch (addr) {
+    case R_CTRL:
+        for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
+            if (s->regs[R_LOCK] & 1 << i) {
+                value &= ~lock_ctrl_map[i];
+                value |= lock_ctrl_map[i] & s->regs[R_CTRL];
+            }
+        }
+        aes_en = extract32(value, 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",
+                          prefix);
+        }
+        break;
+
+    case R_DMA_DST_LEN:
+        /* 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);
+        break;
+
+    case R_UNLOCK:
+        if (value == R_UNLOCK_MAGIC) {
+            s->lock = 0;
+            DB_PRINT("successful unlock\n");
+        } else if (s->lock) {/* bad unlock attempt */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s:failed unlock\n", prefix);
+            s->regs[R_CTRL] &= ~PCAP_PR;
+            s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
+        }
+        break;
+    }

And the post-write logic.

+    xilinx_devcfg_update_ixr(s);
+
+    if (*prefix) {
+        g_free((void *)prefix);
+    }
+}
+

> 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.
>
> 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.
>

My goal is to pretty much copy paste out the TRM for the clear GE
write. Some of my register fields in the TRM for this device say
things like "Reserved, always write as 1" which im trying to capture
in a data driven way without having to pollute my switch-cases with
this junk. It would be nice to autogenerate this as well.

> 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.
>

Hmm ill have to think on this one.

Regards,
Peter

> 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
>
Peter Crosthwaite - May 30, 2013, 7:15 a.m.
On Thu, May 30, 2013 at 3:08 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/05/2013 19:04, Edgar E. Iglesias ha scritto:
>>> > +    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);
>> Hi Peter, Should we be putting r->access->name here instead of "devcfg-regs"?
>
> Yes, that's why I preferred to wrap the memory_region_init_io into an
> API that takes a RegisterInfo. :)

ACK,

You've convinced me :). Will be in v4 (pending outcome of discussion
with Anthony RE decoding)

Regards,
Peter

>
> Paolo
>
Paolo Bonzini - May 30, 2013, 1:08 p.m.
Il 30/05/2013 09:15, Peter Crosthwaite ha scritto:
>>> >> Hi Peter, Should we be putting r->access->name here instead of "devcfg-regs"?
>> >
>> > Yes, that's why I preferred to wrap the memory_region_init_io into an
>> > API that takes a RegisterInfo. :)
> ACK,
> 
> You've convinced me :). Will be in v4 (pending outcome of discussion
> with Anthony RE decoding)

I would also have preferred (I was too terse in mentioning
.accepts.valid earlier so this is the less concise explanation) an API
that does a single memory_region_init_io region for a whole array of
registers.  Basically having a RegisterArrayInfo in addition to the
RegisterInfo.

Paolo
Anthony Liguori - May 30, 2013, 7:41 p.m.
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> Hi Anthony,
>
> On Thu, May 30, 2013 at 3:57 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> 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
>>     ...
>>     }
>> }
>>
>
> That's still possible using just the register API (Patch 2 content
> only) and throwing away the memory API glue. I think its actually
> quite similar to V1 of the patch series where I did not have the
> callbacks, and use the switch-cases for register side-effects only.
> This looks like the intention of your patch. Gerd made a push for the
> callbacks in an earlier review and there was push to integrate to
> Memory API . Is it reasonable to leave it up to the developer whether
> they want to do full conversion (Patches 2 & 3) or half conversion
> (Patch 2 only + your decode refactoring). V1 of this patch at:
>
> http://patchwork.ozlabs.org/patch/224534/
>
> heres the relevant snippet (comments from me inline):
>
> +static void xilinx_devcfg_write(void *opaque, hwaddr addr, uint64_t value,
> +                                unsigned size)
> +{
> +    XilinxDevcfg *s = opaque;
> +    int i;
> +    uint32_t aes_en;
> +    const char *prefix = "";
> +
> +    /* FIXME: use tracing to avoid these gymnastics */
> +    if (XILINX_DEVCFG_ERR_DEBUG || qemu_get_log_flags() & LOG_GUEST_ERROR) {
> +        prefix = g_strdup_printf("%s:Addr %#08x",
> +                                 object_get_canonical_path(OBJECT(s)),
> +                                 (unsigned)addr);
> +    }
> +    addr >>= 2;
>
> This is the open coded decode logic but is trivial in this case.

But this is invisible to the common layer.

I think being able to have a common layer insight into "this value is
being written to this device register" would be extremely useful.

But my fear is that the current proposal will not work for a large set
of devices.

Let me provide another example (attached below) but highlighting the
description:

> static RegisterDecodeEntry decoder[] = {
>     /* addresses 0-1 must be open decoded due to latching */
>     { .addr = 2, .regno = UART_FCR, .flags = REG_WRITE },
>     { .addr = 2, .regno = UART_IIR, .flags = REG_READ },
>     { .addr = 3, .regno = UART_LCR, .flags = REG_RW },
>     { .addr = 4, .regno = UART_MCR, .flags = REG_RW },
>     { .addr = 5, .regno = UART_LSR, .flags = REG_READ },
>     { .addr = 6, .regno = UART_MSR, .flags = REG_READ },
>     { .addr = 7, .regno = UART_SCR, .flags = REG_RW },
> };
> 
> static RegisterMapEntry regmap[] = {
>     DEFINE_REG_U8(SerialState, ier, UART_IER),
>     DEFINE_REG_U8(SerialState, iir, UART_IIR),
>     DEFINE_REG_U8(SerialState, lcr, UART_LCR),
>     DEFINE_REG_U8(SerialState, mcr, UART_MCR),
>     DEFINE_REG_U8(SerialState, lsr, UART_LSR),
>     DEFINE_REG_U8(SerialState, scr, UART_SCR),
>     DEFINE_REG_U8(SerialState, thr, UART_THR),
> };

This is a clear separation between decode logic and load/store logic.
They are very different things from a hardware point of view.


> +    assert(addr < R_MAX);
> +
> +    if (s->lock && addr != R_UNLOCK) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s:write to non-lock register while"
> +                      " locked\n", prefix);
> +        return;
> +    }
> +
>
> some pre write logic (I dropped it later as it was actually unimplemented).
>
> +    uint32_write(&s->regs[addr], &xilinx_devcfg_regs_info[addr], value, prefix,
> +                 XILINX_DEVCFG_ERR_DEBUG);
> +
>
> this is the data-driven register_write() call under its old name.
>
> +    /*side effects */
> +    switch (addr) {
> +    case R_CTRL:
> +        for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
> +            if (s->regs[R_LOCK] & 1 << i) {
> +                value &= ~lock_ctrl_map[i];
> +                value |= lock_ctrl_map[i] & s->regs[R_CTRL];
> +            }
> +        }
> +        aes_en = extract32(value, 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",
> +                          prefix);
> +        }
> +        break;
> +
> +    case R_DMA_DST_LEN:
> +        /* 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);
> +        break;
> +
> +    case R_UNLOCK:
> +        if (value == R_UNLOCK_MAGIC) {
> +            s->lock = 0;
> +            DB_PRINT("successful unlock\n");
> +        } else if (s->lock) {/* bad unlock attempt */
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:failed unlock\n", prefix);
> +            s->regs[R_CTRL] &= ~PCAP_PR;
> +            s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
> +        }
> +        break;
> +    }
>
> And the post-write logic.
>
> +    xilinx_devcfg_update_ixr(s);
> +
> +    if (*prefix) {
> +        g_free((void *)prefix);
> +    }
> +}
> +
>
>> 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.
>>
>> 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.
>>
>
> My goal is to pretty much copy paste out the TRM for the clear GE
> write. Some of my register fields in the TRM for this device say
> things like "Reserved, always write as 1" which im trying to capture
> in a data driven way without having to pollute my switch-cases with
> this junk. It would be nice to autogenerate this as well.

I think you're trying to solve too many problems at once.

I don't think putting error messages in a register layout description is
a good idea.


>> 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.

FWIW, I now think this is a bad idea :-)

Here's the full example:
Regards,

Anthony Liguori

>>
>
> Hmm ill have to think on this one.
>
> Regards,
> Peter
>
>> 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
>>
Peter Crosthwaite - May 31, 2013, 11:34 p.m.
Hi Anthony,

On Fri, May 31, 2013 at 5:41 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
[snip]
>>> }
>>>
>>
>> That's still possible using just the register API (Patch 2 content
>> only) and throwing away the memory API glue. I think its actually
>> quite similar to V1 of the patch series where I did not have the
>> callbacks, and use the switch-cases for register side-effects only.
>> This looks like the intention of your patch. Gerd made a push for the
>> callbacks in an earlier review and there was push to integrate to
>> Memory API . Is it reasonable to leave it up to the developer whether
>> they want to do full conversion (Patches 2 & 3) or half conversion
>> (Patch 2 only + your decode refactoring). V1 of this patch at:
>>
>> http://patchwork.ozlabs.org/patch/224534/
>>
>> heres the relevant snippet (comments from me inline):
>>
>> +static void xilinx_devcfg_write(void *opaque, hwaddr addr, uint64_t value,
>> +                                unsigned size)
>> +{
>> +    XilinxDevcfg *s = opaque;
>> +    int i;
>> +    uint32_t aes_en;
>> +    const char *prefix = "";
>> +
>> +    /* FIXME: use tracing to avoid these gymnastics */
>> +    if (XILINX_DEVCFG_ERR_DEBUG || qemu_get_log_flags() & LOG_GUEST_ERROR) {
>> +        prefix = g_strdup_printf("%s:Addr %#08x",
>> +                                 object_get_canonical_path(OBJECT(s)),
>> +                                 (unsigned)addr);
>> +    }
>> +    addr >>= 2;
>>
>> This is the open coded decode logic but is trivial in this case.
>
> But this is invisible to the common layer.
>
> I think being able to have a common layer insight into "this value is
> being written to this device register" would be extremely useful.
>
> But my fear is that the current proposal will not work for a large set
> of devices.
>
> Let me provide another example (attached below) but highlighting the
> description:
>
>> static RegisterDecodeEntry decoder[] = {
>>     /* addresses 0-1 must be open decoded due to latching */
>>     { .addr = 2, .regno = UART_FCR, .flags = REG_WRITE },
>>     { .addr = 2, .regno = UART_IIR, .flags = REG_READ },

The TRM im reading (for the Xilinx 16550 serial) has the read decoding
of FCR vs IIR dependent on LCR, so I think these two are open decoded
as well.

>>     { .addr = 3, .regno = UART_LCR, .flags = REG_RW },
>>     { .addr = 4, .regno = UART_MCR, .flags = REG_RW },
>>     { .addr = 5, .regno = UART_LSR, .flags = REG_READ },
>>     { .addr = 6, .regno = UART_MSR, .flags = REG_READ },
>>     { .addr = 7, .regno = UART_SCR, .flags = REG_RW },
>> };
>>
>> static RegisterMapEntry regmap[] = {
>>     DEFINE_REG_U8(SerialState, ier, UART_IER),
>>     DEFINE_REG_U8(SerialState, iir, UART_IIR),
>>     DEFINE_REG_U8(SerialState, lcr, UART_LCR),
>>     DEFINE_REG_U8(SerialState, mcr, UART_MCR),
>>     DEFINE_REG_U8(SerialState, lsr, UART_LSR),
>>     DEFINE_REG_U8(SerialState, scr, UART_SCR),
>>     DEFINE_REG_U8(SerialState, thr, UART_THR),
>> };

I like it, but can we merge the two to avoid the replicated lists?
Append the decode information to the existing RegisterMapEntry (or
other way round if you want to think of it like that). I know there is
not a 1:1 correlation between decode lines and storage, but register
API already supports no-storage entries for this very purpose - you
can describe a "register" with no storage and define only side effects
(currently implemented via post_foo callbacks). Then we dont need the
enum to perform mapping from decode to register (except for the open
decode case).

Perhaps its better to think of the RegisterAccessInfo array in the
patch as corresponding to decode lines, rather than storage, with
optional storage information appended

Will send rework shortly.

Regards,
Peter

>
> This is a clear separation between decode logic and load/store logic.
> They are very different things from a hardware point of view.
>
>
>> +    assert(addr < R_MAX);
>> +
>> +    if (s->lock && addr != R_UNLOCK) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s:write to non-lock register while"
>> +                      " locked\n", prefix);
>> +        return;
>> +    }
>> +
>>
>> some pre write logic (I dropped it later as it was actually unimplemented).
>>
>> +    uint32_write(&s->regs[addr], &xilinx_devcfg_regs_info[addr], value, prefix,
>> +                 XILINX_DEVCFG_ERR_DEBUG);
>> +
>>
>> this is the data-driven register_write() call under its old name.
>>
>> +    /*side effects */
>> +    switch (addr) {
>> +    case R_CTRL:
>> +        for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
>> +            if (s->regs[R_LOCK] & 1 << i) {
>> +                value &= ~lock_ctrl_map[i];
>> +                value |= lock_ctrl_map[i] & s->regs[R_CTRL];
>> +            }
>> +        }
>> +        aes_en = extract32(value, 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",
>> +                          prefix);
>> +        }
>> +        break;
>> +
>> +    case R_DMA_DST_LEN:
>> +        /* 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);
>> +        break;
>> +
>> +    case R_UNLOCK:
>> +        if (value == R_UNLOCK_MAGIC) {
>> +            s->lock = 0;
>> +            DB_PRINT("successful unlock\n");
>> +        } else if (s->lock) {/* bad unlock attempt */
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:failed unlock\n", prefix);
>> +            s->regs[R_CTRL] &= ~PCAP_PR;
>> +            s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
>> +        }
>> +        break;
>> +    }
>>
>> And the post-write logic.
>>
>> +    xilinx_devcfg_update_ixr(s);
>> +
>> +    if (*prefix) {
>> +        g_free((void *)prefix);
>> +    }
>> +}
>> +
>>
>>> 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.
>>>
>>> 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.
>>>
>>
>> My goal is to pretty much copy paste out the TRM for the clear GE
>> write. Some of my register fields in the TRM for this device say
>> things like "Reserved, always write as 1" which im trying to capture
>> in a data driven way without having to pollute my switch-cases with
>> this junk. It would be nice to autogenerate this as well.
>
> I think you're trying to solve too many problems at once.
>
> I don't think putting error messages in a register layout description is
> a good idea.
>
>
>>> 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.
>
> FWIW, I now think this is a bad idea :-)
>
> Here's the full example:
>
>
>
> Regards,
>
> Anthony Liguori
>
>>>
>>
>> Hmm ill have to think on this one.
>>
>> Regards,
>> Peter
>>
>>> 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
>>>
>

Patch

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,
+    },
+    [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)