[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model
|
From: |
Anthony Liguori |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model |
|
Date: |
Thu, 30 May 2013 14:41:56 -0500 |
|
User-agent: |
Notmuch/0.15.2+77~g661dcf8 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Peter Crosthwaite <address@hidden> writes:
> Hi Anthony,
>
> On Thu, May 30, 2013 at 3:57 AM, Anthony Liguori <address@hidden> wrote:
>> address@hidden writes:
>>
>>> From: "Peter A. G. Crosthwaite" <address@hidden>
>>>
>>> Minimal device model for devcfg module of Zynq. DMA capabilities and
>>> interrupt generation supported.
>>>
>>> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
>>> ---
>>> 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 (address@hidden)
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> copy
>>> + * of this software and associated documentation files (the "Software"),
>>> to deal
>>> + * in the Software without restriction, including without limitation the
>>> rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
>>> sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included
>>> in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>> IN
>>> + * THE SOFTWARE.
>>> + */
>>> +
>>> +#include "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:
foo
Description: Binary data
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
>>
Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model, Anthony Liguori, 2013/05/29
[Qemu-devel] [PATCH v3 5/5] xilinx_zynq: added devcfg to machine model, peter . crosthwaite, 2013/05/24