[Qemu-devel] [PATCH v8 0/8] data-driven device registers

[Qemu-devel] [PATCH v8 0/8] data-driven device registers

[Alistair Francis]

This patch series is based on Peter C's original register API. His
original cover letter is below.

Future work: Allow support for memory attributes.

V8:
 - Remove GPIO infrastructure from patch set
V7:
 - Rebase
 - Fixup the DecCFG model
 - Fixes and corrections to the register API model and functions
V6:
 - Small changes to the API based on Alex's comments
 - Remove 'register: Add support for decoding information' patch
 - Move prefix and debug into the RegisterInfoArray as it is the same
   for every register.
V5:
 - Only create a single memory region instead of a memory region for
   each register
 - General tidyups based on Alex's comments
V4:
 - Rebase and fix build issue
 - Simplify the register write logic
 - Other small fixes suggested by Alex Bennee
V3:
 - Small changes reported by Fred
V2:
 - Rebase
 - Fix up IOU SLCR connections
 - Add the memory_region_add_subregion_no_print() function and use it
   for the registers
Changes since RFC:
 - Connect the ZynqMP IOU SLCR device
 - Rebase

Original cover letter From Peter:
Hi All. This is a new scheme I've come up with handling device registers in a
data driven way. My motivation for this is to factor out a lot of the access
checking that seems to be replicated in every device. See P1 commit message for
further discussion.

P1 is the main patch, adds the register definition functionality
P2-3,6 add helpers that glue the register API to the Memory API
P4 Defines a set of macros that minimise register and field definitions
P5 is QOMfication
P7 is a trivial
P10-13 Work up to GPIO support
P8,9,14 add new devices (the Xilinx Zynq devcfg & ZynqMP SLCR) that use this
        scheme.
P15: Connect the ZynqMP SLCR device

This Zynq devcfg device was particularly finnicky with per-bit restrictions.
I'm also looking for a higher-than-usual modelling fidelity
on the register space, with semantics defined for random reserved bits
in-between otherwise consistent fields.

Here's an example of the qemu_log output for the devcfg device. This is produced
by now generic sharable code:

/machine/unattached/device[44]:Addr 0x000008:CFG: write of value 00000508
/machine/unattached/device[44]:Addr 0x000080:MCTRL: write of value 00800010
/machine/unattached/device[44]:Addr 0x000010:INT_MASK: write of value ffffffff
/machine/unattached/device[44]:Addr 00000000:CTRL: write of value 0c00607f

And an example of a rogue guest banging on a bad bit:

/machine/unattached/device[44]:Addr 0x000014:STATUS bits 0x000001 may not be \
								written to 1

A future feature I am interested in is implementing TCG optimisation of
side-effectless registers. The register API allows clear definition of
what registers have txn side effects and which ones don't. You could even
go a step further and translate such side-effectless accesses based on the
data pointer for the register.


Alistair Francis (4):
  bitops: Add MAKE_64BIT_MASK macro
  register: Add Register API
  register: Add Memory API glue
  dma: Add Xilinx Zynq devcfg device model

Peter Crosthwaite (4):
  register: Define REG and FIELD macros
  register: QOMify
  register: Add block initialise helper
  xilinx_zynq: Connect devcfg to the Zynq machine model

 default-configs/arm-softmmu.mak   |   1 +
 hw/arm/xilinx_zynq.c              |   6 +
 hw/core/Makefile.objs             |   1 +
 hw/core/register.c                | 285 +++++++++++++++++++++++++++
 hw/dma/Makefile.objs              |   1 +
 hw/dma/xlnx-zynq-devcfg.c         | 400 ++++++++++++++++++++++++++++++++++++++
 include/hw/dma/xlnx-zynq-devcfg.h |  62 ++++++
 include/hw/register.h             | 251 ++++++++++++++++++++++++
 include/qemu/bitops.h             |   3 +
 9 files changed, 1010 insertions(+)
 create mode 100644 hw/core/register.c
 create mode 100644 hw/dma/xlnx-zynq-devcfg.c
 create mode 100644 include/hw/dma/xlnx-zynq-devcfg.h
 create mode 100644 include/hw/register.h

-- 
2.7.4

[Qemu-devel] [PATCH v8 1/8] bitops: Add MAKE_64BIT_MASK macro

[Alistair Francis]

Add a macro that creates a 64bit value which has length number of ones
shifted across by the value of shift.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
V7:
 - Fixup commit typo
 - Use the method from deposit64()
V5:
 - Re-write to a 64-bit mask instead of ONES()
 - Re-order this patch in the series

 include/qemu/bitops.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 15418a8..98fb005 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -24,6 +24,9 @@
 #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
 #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
 
+#define MAKE_64BIT_MASK(shift, length) \
+    (((~0ULL) >> (64 - (length))) << (shift))
+
 /**
  * set_bit - Set a bit in memory
  * @nr: the bit to set
-- 
2.7.4

[Qemu-devel] [PATCH v8 2/8] register: Add Register API

[Alistair Francis]

This API provides some encapsulation of registers and factors out some
common functionality to common code. Bits of device state (usually MMIO
registers) often have all sorts of access restrictions and semantics
associated with them. This API allows you to define what those
restrictions are on a bit-by-bit basis.

Helper functions are then used to access the register which observe the
semantics defined by the RegisterAccessInfo struct.

Some features:
Bits can be marked as read_only (ro field)
Bits can be marked as write-1-clear (w1c field)
Bits can be marked as reserved (rsvd field)
Reset values can be defined (reset)
Bits can be marked clear on read (cor)
Pre and post action callbacks can be added to read and write ops
Verbose debugging info can be enabled/disabled

Useful for defining device register spaces in a data driven way. Cuts
down on a lot of the verbosity and repetition in the switch-case blocks
in the standard foo_mmio_read/write functions.

Also useful for automated generation of device models from hardware
design sources.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
V8:
 - Add read enable mask
V5:
 - Convert to using only one memory region
V4:
 - Rebase
 - Remove the guest error masking
 - Simplify the unimplemented masking
 - Use the reserved value in the write calculations
 - Remove read_lite and write_lite
 - General fixes to asserts and log printing
V3:
 - Address some comments from Fred

 hw/core/Makefile.objs |   1 +
 hw/core/register.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 112 +++++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)
 create mode 100644 hw/core/register.c
 create mode 100644 include/hw/register.h

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 82a9ef8..cfd4840 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -15,4 +15,5 @@ common-obj-$(CONFIG_SOFTMMU) += machine.o
 common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
+common-obj-$(CONFIG_SOFTMMU) += register.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
diff --git a/hw/core/register.c b/hw/core/register.c
new file mode 100644
index 0000000..e36169d
--- /dev/null
+++ b/hw/core/register.c
@@ -0,0 +1,159 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/register.h"
+#include "hw/qdev.h"
+#include "qemu/log.h"
+
+static inline void register_write_val(RegisterInfo *reg, uint64_t val)
+{
+    g_assert(reg->data);
+
+    switch (reg->data_size) {
+    case 1:
+        *(uint8_t *)reg->data = val;
+        break;
+    case 2:
+        *(uint16_t *)reg->data = val;
+        break;
+    case 4:
+        *(uint32_t *)reg->data = val;
+        break;
+    case 8:
+        *(uint64_t *)reg->data = val;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static inline uint64_t register_read_val(RegisterInfo *reg)
+{
+    switch (reg->data_size) {
+    case 1:
+        return *(uint8_t *)reg->data;
+    case 2:
+        return *(uint16_t *)reg->data;
+    case 4:
+        return *(uint32_t *)reg->data;
+    case 8:
+        return *(uint64_t *)reg->data;
+    default:
+        g_assert_not_reached();
+    }
+    return 0; /* unreachable */
+}
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
+                    const char *prefix, bool debug)
+{
+    uint64_t old_val, new_val, test, no_w_mask;
+    const RegisterAccessInfo *ac;
+
+    assert(reg);
+
+    ac = reg->access;
+
+    if (!ac || !ac->name) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
+                      "(written value: %#" PRIx64 ")\n", prefix, val);
+        return;
+    }
+
+    old_val = reg->data ? register_read_val(reg) : ac->reset;
+
+    test = (old_val ^ val) & ac->rsvd;
+    if (test) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in reserved bit"
+                      "fields: %#" PRIx64 ")\n", prefix, test);
+    }
+
+    test = val & ac->unimp;
+    if (test) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s:%s writing %#" PRIx64 " to unimplemented bits:" \
+                      " %#" PRIx64 "",
+                      prefix, reg->access->name, val, ac->unimp);
+    }
+
+    /* Create the no write mask based on the read only, write to clear and
+     * reserved bit masks.
+     */
+    no_w_mask = ac->ro | ac->w1c | ac->rsvd | ~we;
+    new_val = (val & ~no_w_mask) | (old_val & no_w_mask);
+    new_val &= ~(val & ac->w1c);
+
+    if (ac->pre_write) {
+        new_val = ac->pre_write(reg, new_val);
+    }
+
+    if (debug) {
+        qemu_log("%s:%s: write of value %#" PRIx64 "\n", prefix, ac->name,
+                 new_val);
+    }
+
+    register_write_val(reg, new_val);
+
+    if (ac->post_write) {
+        ac->post_write(reg, new_val);
+    }
+}
+
+uint64_t register_read(RegisterInfo *reg, uint64_t re, const char* prefix,
+                       bool debug)
+{
+    uint64_t ret;
+    const RegisterAccessInfo *ac;
+
+    assert(reg);
+
+    ac = reg->access;
+    if (!ac || !ac->name) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state\n",
+                      prefix);
+        return 0;
+    }
+
+    ret = reg->data ? register_read_val(reg) : ac->reset;
+
+    /* Mask based on the read enable size */
+    ret &= re;
+    register_write_val(reg, ret & ~ac->cor);
+
+    if (ac->post_read) {
+        ret = ac->post_read(reg, ret);
+    }
+
+    if (debug) {
+        qemu_log("%s:%s: read of value %#" PRIx64 "\n", prefix,
+                 ac->name, ret);
+    }
+
+    return ret;
+}
+
+void register_reset(RegisterInfo *reg)
+{
+    g_assert(reg);
+
+    if (!reg->data || !reg->access) {
+        return;
+    }
+
+    register_write_val(reg, reg->access->reset);
+}
diff --git a/include/hw/register.h b/include/hw/register.h
new file mode 100644
index 0000000..6a2bc75
--- /dev/null
+++ b/include/hw/register.h
@@ -0,0 +1,112 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef REGISTER_H
+#define REGISTER_H
+
+#include "exec/memory.h"
+
+typedef struct RegisterInfo RegisterInfo;
+typedef struct RegisterAccessInfo RegisterAccessInfo;
+
+/**
+ * Access description for a register that is part of guest accessible device
+ * state.
+ *
+ * @name: String name of the register
+ * @ro: whether or not the bit is read-only
+ * @w1c: bits with the common write 1 to clear semantic.
+ * @reset: reset value.
+ * @cor: Bits that are clear on read
+ * @rsvd: Bits that are reserved and should not be changed
+ *
+ * @pre_write: Pre write callback. Passed the value that's to be written,
+ * immediately before the actual write. The returned value is what is written,
+ * giving the handler a chance to modify the written value.
+ * @post_write: Post write callback. Passed the written value. Most write side
+ * effects should be implemented here.
+ *
+ * @post_read: Post read callback. Passes the value that is about to be returned
+ * for a read. The return value from this function is what is ultimately read,
+ * allowing this function to modify the value before return to the client.
+ */
+
+struct RegisterAccessInfo {
+    const char *name;
+    uint64_t ro;
+    uint64_t w1c;
+    uint64_t reset;
+    uint64_t cor;
+    uint64_t rsvd;
+    uint64_t unimp;
+
+    uint64_t (*pre_write)(RegisterInfo *reg, uint64_t val);
+    void (*post_write)(RegisterInfo *reg, uint64_t val);
+
+    uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+};
+
+/**
+ * A register that is part of guest accessible state
+ * @data: pointer to the register data. Will be cast
+ * to the relevant uint type depending on data_size.
+ * @data_size: Size of the register in bytes. Must be
+ * 1, 2, 4 or 8
+ *
+ * @access: Access description of this register
+ *
+ * @debug: Whether or not verbose debug is enabled
+ * @prefix: String prefix for log and debug messages
+ *
+ * @opaque: Opaque data for the register
+ */
+
+struct RegisterInfo {
+    /* <public> */
+    void *data;
+    int data_size;
+
+    const RegisterAccessInfo *access;
+
+    void *opaque;
+};
+
+/**
+ * write a value to a register, subject to its restrictions
+ * @reg: register to write to
+ * @val: value to write
+ * @we: write enable mask
+ * @prefix: The device prefix that should be printed before the register name
+ * @debug: Should the write operation debug information be printed?
+ */
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
+                    const char *prefix, bool debug);
+
+/**
+ * read a value from a register, subject to its restrictions
+ * @reg: register to read from
+ * @re: read enable mask
+ * @prefix: The device prefix that should be printed before the register name
+ * @debug: Should the read operation debug information be printed?
+ * returns: value read
+ */
+
+uint64_t register_read(RegisterInfo *reg, uint64_t re, const char* prefix,
+                       bool debug);
+
+/**
+ * reset a register
+ * @reg: register to reset
+ */
+
+void register_reset(RegisterInfo *reg);
+
+#endif
-- 
2.7.4

[Qemu-devel] [PATCH v8 3/8] register: Add Memory API glue

[Alistair Francis]

Add memory io handlers that glue the register API to the memory API.
Just translation functions at this stage. Although it does allow for
devices to be created without all-in-one mmio r/w handlers.

This patch also adds the RegisterInfoArray struct, which allows all of
the individual RegisterInfo structs to be grouped into a single memory
region.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V8:
 - Mask reads based on size as well as writes
V7:
 - Remove endianess handling
 - Remove assert() and log missing registers
V6:
 - Add the memory region later
V5:
 - Convert to using only one memory region

 hw/core/register.c    | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/hw/core/register.c b/hw/core/register.c
index e36169d..07f5d4f 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -157,3 +157,62 @@ void register_reset(RegisterInfo *reg)
 
     register_write_val(reg, reg->access->reset);
 }
+
+void register_write_memory(void *opaque, hwaddr addr,
+                           uint64_t value, unsigned size)
+{
+    RegisterInfoArray *reg_array = opaque;
+    RegisterInfo *reg = NULL;
+    uint64_t we;
+    int i;
+
+    for (i = 0; i < reg_array->num_elements; i++) {
+        if (reg_array->r[i]->access->addr == addr) {
+            reg = reg_array->r[i];
+            break;
+        }
+    }
+
+    if (!reg) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Write to unimplemented register at " \
+                      "address: %#" PRIx64 "\n", addr);
+        return;
+    }
+
+    /* Generate appropriate write enable mask */
+    if (reg->data_size < size) {
+        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
+    } else {
+        we = MAKE_64BIT_MASK(0, size * 8);
+    }
+
+    register_write(reg, value, we, reg_array->prefix,
+                   reg_array->debug);
+}
+
+uint64_t register_read_memory(void *opaque, hwaddr addr,
+                              unsigned size)
+{
+    RegisterInfoArray *reg_array = opaque;
+    RegisterInfo *reg = NULL;
+    uint64_t read_val;
+    int i;
+
+    for (i = 0; i < reg_array->num_elements; i++) {
+        if (reg_array->r[i]->access->addr == addr) {
+            reg = reg_array->r[i];
+            break;
+        }
+    }
+
+    if (!reg) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Read to unimplemented register at " \
+                      "address: %#" PRIx64 "\n", addr);
+        return 0;
+    }
+
+    read_val = register_read(reg, size * 8, reg_array->prefix,
+                             reg_array->debug);
+
+    return extract64(read_val, 0, size * 8);
+}
diff --git a/include/hw/register.h b/include/hw/register.h
index 6a2bc75..00bbfe5 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -15,6 +15,7 @@
 
 typedef struct RegisterInfo RegisterInfo;
 typedef struct RegisterAccessInfo RegisterAccessInfo;
+typedef struct RegisterInfoArray RegisterInfoArray;
 
 /**
  * Access description for a register that is part of guest accessible device
@@ -51,6 +52,8 @@ struct RegisterAccessInfo {
     void (*post_write)(RegisterInfo *reg, uint64_t val);
 
     uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+
+    hwaddr addr;
 };
 
 /**
@@ -79,6 +82,25 @@ struct RegisterInfo {
 };
 
 /**
+ * This structure is used to group all of the individual registers which are
+ * modeled using the RegisterInfo structure.
+ *
+ * @r is an aray containing of all the relevent RegisterInfo structures.
+ *
+ * @num_elements is the number of elements in the array r
+ *
+ * @mem: optional Memory region for the register
+ */
+
+struct RegisterInfoArray {
+    int num_elements;
+    RegisterInfo **r;
+
+    bool debug;
+    const char *prefix;
+};
+
+/**
  * write a value to a register, subject to its restrictions
  * @reg: register to write to
  * @val: value to write
@@ -109,4 +131,25 @@ uint64_t register_read(RegisterInfo *reg, uint64_t re, const char* prefix,
 
 void register_reset(RegisterInfo *reg);
 
+/**
+ * Memory API MMIO write handler that will write to a Register API register.
+ * @opaque: RegisterInfo to write to
+ * @addr: Address to write
+ * @value: Value to write
+ * @size: Number of bytes to write
+ */
+
+void register_write_memory(void *opaque, hwaddr addr, uint64_t value,
+                           unsigned size);
+
+/**
+ * Memory API MMIO read handler that will read from a Register API register.
+ * @opaque: RegisterInfo to read from
+ * @addr: Address to read
+ * @size: Number of bytes to read
+ * returns: Value read from register
+ */
+
+uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
+
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v8 4/8] register: Define REG and FIELD macros

[Alistair Francis]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Define some macros that can be used for defining registers and fields.

The REG32 macro will define A_FOO, for the byte address of a register
as well as R_FOO for the uint32_t[] register number (A_FOO / 4).

The FIELD macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and
FOO_BAR_LENGTH constants for field BAR in register FOO.

Finally, there are some shorthand helpers for extracting/depositing
fields from registers based on these naming schemes.

Usage can greatly reduce the verbosity of device code.

The deposit and extract macros (eg FIELD_EX32, FIELD_DP32  etc.) can be
used to generate extract and deposits without any repetition of the name
stems.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[ EI Changes:
  * Add Deposit macros
]
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
E.g. Currently you have to define something like:

\#define R_FOOREG (0x84/4)
\#define R_FOOREG_BARFIELD_SHIFT 10
\#define R_FOOREG_BARFIELD_LENGTH 5

uint32_t foobar_val = extract32(s->regs[R_FOOREG],
                                R_FOOREG_BARFIELD_SHIFT,
                                R_FOOREG_BARFIELD_LENGTH);

Which has:
2 macro definitions per field
3 register names ("FOOREG") per extract
2 field names ("BARFIELD") per extract

With these macros this becomes:

REG32(FOOREG, 0x84)
FIELD(FOOREG, BARFIELD, 10, 5)

uint32_t foobar_val = ARRAY_FIELD_EX32(s->regs, FOOREG, BARFIELD)

Which has:
1 macro definition per field
1 register name per extract
1 field name per extract

If you are not using arrays for the register data you can just use the
non-array "FIELD_" variants and still save 2 name stems:

uint32_t foobar_val = FIELD_EX32(s->fooreg, FOOREG, BARFIELD)

Deposit is similar for depositing values. Deposit has compile-time
overflow checking for literals.
For example:

REG32(XYZ1, 0x84)
FIELD(XYZ1, TRC, 0, 4)

/* Correctly set XYZ1.TRC = 5.  */
ARRAY_FIELD_DP32(s->regs, XYZ1, TRC, 5);

/* Incorrectly set XYZ1.TRC = 16.  */
ARRAY_FIELD_DP32(s->regs, XYZ1, TRC, 16);

The latter assignment results in:
warning: large integer implicitly truncated to unsigned type [-Woverflow]


 include/hw/register.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/include/hw/register.h b/include/hw/register.h
index 00bbfe5..e8c58a1 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -152,4 +152,50 @@ void register_write_memory(void *opaque, hwaddr addr, uint64_t value,
 
 uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
 
+/* Define constants for a 32 bit register */
+
+/* This macro will define A_FOO, for the byte address of a register
+ * as well as R_FOO for the uint32_t[] register number (A_FOO / 4).
+ */
+#define REG32(reg, addr)                                                  \
+    enum { A_ ## reg = (addr) };                                          \
+    enum { R_ ## reg = (addr) / 4 };
+
+/* Define SHIFT, LENGTH and MASK constants for a field within a register */
+
+/* This macro will define FOO_BAR_MASK, FOO_BAR_SHIFT and FOO_BAR_LENGTH 
+ * constants for field BAR in register FOO.
+ */
+#define FIELD(reg, field, shift, length)                                  \
+    enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
+    enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
+    enum { R_ ## reg ## _ ## field ## _MASK =                             \
+                                        MAKE_64BIT_MASK(shift, length)};
+
+/* Extract a field from a register */
+#define FIELD_EX32(storage, reg, field)                                   \
+    extract32((storage), R_ ## reg ## _ ## field ## _SHIFT,               \
+              R_ ## reg ## _ ## field ## _LENGTH)
+
+/* Extract a field from an array of registers */
+#define ARRAY_FIELD_EX32(regs, reg, field)                                \
+    FIELD_EX32((regs)[R_ ## reg], reg, field)
+
+/* Deposit a register field.
+ * Assigning values larger then the target field will result in
+ * compilation warnings.
+ */
+#define FIELD_DP32(storage, reg, field, val) ({                           \
+    struct {                                                              \
+        unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;                \
+    } v = { .v = val };                                                   \
+    uint32_t d;                                                           \
+    d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT,           \
+                  R_ ## reg ## _ ## field ## _LENGTH, v.v);               \
+    d; })
+
+/* Deposit a field to array of registers.  */
+#define ARRAY_FIELD_DP32(regs, reg, field, val)                           \
+    (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
+
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v8 5/8] register: QOMify

[Alistair Francis]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

QOMify registers as a child of TYPE_DEVICE. This allows registers to
define GPIOs.

Define an init helper that will do QOM initialisation.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
V7:
 - Remove uneeded struct
 - Fixup documentation
V5:
 - Convert to using only one memory region

 hw/core/register.c    | 23 +++++++++++++++++++++++
 include/hw/register.h | 14 ++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/hw/core/register.c b/hw/core/register.c
index 07f5d4f..058c998 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -158,6 +158,17 @@ void register_reset(RegisterInfo *reg)
     register_write_val(reg, reg->access->reset);
 }
 
+void register_init(RegisterInfo *reg)
+{
+    assert(reg);
+
+    if (!reg->data || !reg->access) {
+        return;
+    }
+
+    object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
+}
+
 void register_write_memory(void *opaque, hwaddr addr,
                            uint64_t value, unsigned size)
 {
@@ -216,3 +227,15 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
 
     return extract64(read_val, 0, size * 8);
 }
+
+static const TypeInfo register_info = {
+    .name  = TYPE_REGISTER,
+    .parent = TYPE_DEVICE,
+};
+
+static void register_register_types(void)
+{
+    type_register_static(&register_info);
+}
+
+type_init(register_register_types)
diff --git a/include/hw/register.h b/include/hw/register.h
index e8c58a1..61c53fb 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -11,6 +11,7 @@
 #ifndef REGISTER_H
 #define REGISTER_H
 
+#include "hw/qdev-core.h"
 #include "exec/memory.h"
 
 typedef struct RegisterInfo RegisterInfo;
@@ -72,6 +73,9 @@ struct RegisterAccessInfo {
  */
 
 struct RegisterInfo {
+    /* <private> */
+    DeviceState parent_obj;
+
     /* <public> */
     void *data;
     int data_size;
@@ -81,6 +85,9 @@ struct RegisterInfo {
     void *opaque;
 };
 
+#define TYPE_REGISTER "qemu,register"
+#define REGISTER(obj) OBJECT_CHECK(RegisterInfo, (obj), TYPE_REGISTER)
+
 /**
  * This structure is used to group all of the individual registers which are
  * modeled using the RegisterInfo structure.
@@ -132,6 +139,13 @@ uint64_t register_read(RegisterInfo *reg, uint64_t re, const char* prefix,
 void register_reset(RegisterInfo *reg);
 
 /**
+ * Initialize a register.
+ * @reg: Register to initialize
+ */
+
+void register_init(RegisterInfo *reg);
+
+/**
  * Memory API MMIO write handler that will write to a Register API register.
  * @opaque: RegisterInfo to write to
  * @addr: Address to write
-- 
2.7.4

[Qemu-devel] [PATCH v8 6/8] register: Add block initialise helper

[Alistair Francis]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Add a helper that will scan a static RegisterAccessInfo Array
and populate a container MemoryRegion with registers as defined.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
The reason that I'm not using GArray is because the array needs to store
the memory region that covers all of the registers.

V8:
 - Add a finalise function used to free the allocated memory
V7:
 - Return the memory region instead of adding it as a subregion
V6:
 - Fixup the loop logic
V5:
 - Convert to only using one memory region
V3:
 - Fix typo
V2:
 - Use memory_region_add_subregion_no_print()

 hw/core/register.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/hw/core/register.c b/hw/core/register.c
index 058c998..7b73bf4 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -228,6 +228,50 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
     return extract64(read_val, 0, size * 8);
 }
 
+RegisterInfoArray *register_init_block32(DeviceState *owner,
+                                         const RegisterAccessInfo *rae,
+                                         int num, RegisterInfo *ri,
+                                         uint32_t *data,
+                                         const MemoryRegionOps *ops,
+                                         bool debug_enabled,
+                                         uint64_t memory_size)
+{
+    const char *device_prefix = object_get_typename(OBJECT(owner));
+    RegisterInfoArray *r_array = g_new0(RegisterInfoArray, 1);
+    int i;
+
+    r_array->r = g_new0(RegisterInfo *, num);
+    r_array->num_elements = num;
+    r_array->debug = debug_enabled;
+    r_array->prefix = device_prefix;
+
+    for (i = 0; i < num; i++) {
+        int index = rae[i].addr / 4;
+        RegisterInfo *r = &ri[index];
+
+        *r = (RegisterInfo) {
+            .data = &data[index],
+            .data_size = sizeof(uint32_t),
+            .access = &rae[i],
+            .opaque = owner,
+        };
+        register_init(r);
+
+        r_array->r[i] = r;
+    }
+
+    memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
+                          device_prefix, memory_size);
+
+    return r_array;
+}
+
+void register_finlise_block(RegisterInfoArray *r_array)
+{
+    g_free(r_array->r);
+    g_free(r_array);
+}
+
 static const TypeInfo register_info = {
     .name  = TYPE_REGISTER,
     .parent = TYPE_DEVICE,
diff --git a/include/hw/register.h b/include/hw/register.h
index 61c53fb..b8d464d 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -100,6 +100,8 @@ struct RegisterInfo {
  */
 
 struct RegisterInfoArray {
+    MemoryRegion mem;
+
     int num_elements;
     RegisterInfo **r;
 
@@ -166,6 +168,40 @@ void register_write_memory(void *opaque, hwaddr addr, uint64_t value,
 
 uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
 
+/**
+ * Init a block of registers into a container MemoryRegion. A
+ * number of constant register definitions are parsed to create a corresponding
+ * array of RegisterInfo's.
+ *
+ * @owner: device owning the registers
+ * @rae: Register definitions to init
+ * @num: number of registers to init (length of @rae)
+ * @ri: Register array to init, must already be allocated
+ * @data: Array to use for register data, must already be allocated
+ * @ops: Memory region ops to access registers.
+ * @debug enabled: turn on/off verbose debug information
+ * returns: An structure containing all of the registers and an initialized
+            memory region (r_array->mem) the caller should add to a container.
+ */
+
+RegisterInfoArray *register_init_block32(DeviceState *owner,
+                                         const RegisterAccessInfo *rae,
+                                         int num, RegisterInfo *ri,
+                                         uint32_t *data,
+                                         const MemoryRegionOps *ops,
+                                         bool debug_enabled,
+                                         uint64_t memory_size);
+
+/**
+ * This function should be called to cleanup the registers that were initialized
+ * when calling register_init_block32()
+ *
+ * @r_array: An structure containing all of the registers. The caller is in
+ *           charge of cleaning up the memory region (r_array->mem).
+ */
+
+void register_finlise_block(RegisterInfoArray *r_array);
+
 /* Define constants for a 32 bit register */
 
 /* This macro will define A_FOO, for the byte address of a register
-- 
2.7.4

[Qemu-devel] [PATCH v8 7/8] dma: Add Xilinx Zynq devcfg device model

[Alistair Francis]

Add a minimal model for the devcfg device which is part of Zynq.
This model supports DMA capabilities and interrupt generation.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V7:
 - Corrections to the internal device
V5:
 - Corrections to the device model logic

 default-configs/arm-softmmu.mak   |   1 +
 hw/dma/Makefile.objs              |   1 +
 hw/dma/xlnx-zynq-devcfg.c         | 400 ++++++++++++++++++++++++++++++++++++++
 include/hw/dma/xlnx-zynq-devcfg.h |  62 ++++++
 4 files changed, 464 insertions(+)
 create mode 100644 hw/dma/xlnx-zynq-devcfg.c
 create mode 100644 include/hw/dma/xlnx-zynq-devcfg.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index c5bcba7..7a19863 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -66,6 +66,7 @@ CONFIG_PXA2XX=y
 CONFIG_BITBANG_I2C=y
 CONFIG_FRAMEBUFFER=y
 CONFIG_XILINX_SPIPS=y
+CONFIG_ZYNQ_DEVCFG=y
 
 CONFIG_ARM11SCU=y
 CONFIG_A9SCU=y
diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
index 8b0823e..087c8e6 100644
--- a/hw/dma/Makefile.objs
+++ b/hw/dma/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_PL330) += pl330.o
 common-obj-$(CONFIG_I82374) += i82374.o
 common-obj-$(CONFIG_I8257) += i8257.o
 common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
+common-obj-$(CONFIG_ZYNQ_DEVCFG) += xlnx-zynq-devcfg.o
 common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
 common-obj-$(CONFIG_STP2000) += sparc32_dma.o
 common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
new file mode 100644
index 0000000..f0c6dc7
--- /dev/null
+++ b/hw/dma/xlnx-zynq-devcfg.c
@@ -0,0 +1,400 @@
+/*
+ * QEMU model of the Xilinx Zynq Devcfg Interface
+ *
+ * (C) 2011 PetaLogix Pty Ltd
+ * (C) 2014 Xilinx Inc.
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.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 "qemu/osdep.h"
+#include "hw/dma/xlnx-zynq-devcfg.h"
+#include "qemu/bitops.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
+#include "qemu/log.h"
+
+#define FREQ_HZ 900000000
+
+#define BTT_MAX 0x400
+
+#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
+#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
+#endif
+
+#define DB_PRINT(fmt, args...) do { \
+    if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
+        qemu_log("%s: " fmt, __func__, ## args); \
+    } \
+} while (0);
+
+REG32(CTRL, 0x00)
+    FIELD(CTRL,     FORCE_RST,          31,  1) /* Not supported, wr ignored */
+    FIELD(CTRL,     PCAP_PR,            27,  1) /* Forced to 0 on bad unlock */
+    FIELD(CTRL,     PCAP_MODE,          26,  1)
+    FIELD(CTRL,     MULTIBOOT_EN,       24,  1)
+    FIELD(CTRL,     USER_MODE,          15,  1)
+    FIELD(CTRL,     PCFG_AES_FUSE,      12,  1)
+    FIELD(CTRL,     PCFG_AES_EN,         9,  3)
+    FIELD(CTRL,     SEU_EN,              8,  1)
+    FIELD(CTRL,     SEC_EN,              7,  1)
+    FIELD(CTRL,     SPNIDEN,             6,  1)
+    FIELD(CTRL,     SPIDEN,              5,  1)
+    FIELD(CTRL,     NIDEN,               4,  1)
+    FIELD(CTRL,     DBGEN,               3,  1)
+    FIELD(CTRL,     DAP_EN,              0,  3)
+
+REG32(LOCK, 0x04)
+    #define AES_FUSE_LOCK        4
+    #define AES_EN_LOCK          3
+    #define SEU_LOCK             2
+    #define SEC_LOCK             1
+    #define DBG_LOCK             0
+
+/* mapping bits in R_LOCK to what they lock in R_CTRL */
+static const uint32_t lock_ctrl_map[] = {
+    [AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
+    [AES_EN_LOCK]   = R_CTRL_PCFG_AES_EN_MASK,
+    [SEU_LOCK]      = R_CTRL_SEU_EN_MASK,
+    [SEC_LOCK]      = R_CTRL_SEC_EN_MASK,
+    [DBG_LOCK]      = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
+                      R_CTRL_NIDEN_MASK   | R_CTRL_DBGEN_MASK  |
+                      R_CTRL_DAP_EN_MASK,
+};
+
+REG32(CFG, 0x08)
+    FIELD(CFG,      RFIFO_TH,           10,  2)
+    FIELD(CFG,      WFIFO_TH,            8,  2)
+    FIELD(CFG,      RCLK_EDGE,           7,  1)
+    FIELD(CFG,      WCLK_EDGE,           6,  1)
+    FIELD(CFG,      DISABLE_SRC_INC,     5,  1)
+    FIELD(CFG,      DISABLE_DST_INC,     4,  1)
+#define R_CFG_RESET 0x50B
+
+REG32(INT_STS, 0x0C)
+    FIELD(INT_STS,  PSS_GTS_USR_B,      31,  1)
+    FIELD(INT_STS,  PSS_FST_CFG_B,      30,  1)
+    FIELD(INT_STS,  PSS_CFG_RESET_B,    27,  1)
+    FIELD(INT_STS,  RX_FIFO_OV,         18,  1)
+    FIELD(INT_STS,  WR_FIFO_LVL,        17,  1)
+    FIELD(INT_STS,  RD_FIFO_LVL,        16,  1)
+    FIELD(INT_STS,  DMA_CMD_ERR,        15,  1)
+    FIELD(INT_STS,  DMA_Q_OV,           14,  1)
+    FIELD(INT_STS,  DMA_DONE,           13,  1)
+    FIELD(INT_STS,  DMA_P_DONE,         12,  1)
+    FIELD(INT_STS,  P2D_LEN_ERR,        11,  1)
+    FIELD(INT_STS,  PCFG_DONE,           2,  1)
+    #define R_INT_STS_RSVD       ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
+
+REG32(INT_MASK, 0x10)
+
+REG32(STATUS, 0x14)
+    FIELD(STATUS,   DMA_CMD_Q_F,        31,  1)
+    FIELD(STATUS,   DMA_CMD_Q_E,        30,  1)
+    FIELD(STATUS,   DMA_DONE_CNT,       28,  2)
+    FIELD(STATUS,   RX_FIFO_LVL,        20,  5)
+    FIELD(STATUS,   TX_FIFO_LVL,        12,  7)
+    FIELD(STATUS,   PSS_GTS_USR_B,      11,  1)
+    FIELD(STATUS,   PSS_FST_CFG_B,      10,  1)
+    FIELD(STATUS,   PSS_CFG_RESET_B,     5,  1)
+
+REG32(DMA_SRC_ADDR, 0x18)
+REG32(DMA_DST_ADDR, 0x1C)
+REG32(DMA_SRC_LEN, 0x20)
+REG32(DMA_DST_LEN, 0x24)
+REG32(ROM_SHADOW, 0x28)
+REG32(SW_ID, 0x30)
+REG32(UNLOCK, 0x34)
+
+#define R_UNLOCK_MAGIC 0x757BDF0D
+
+REG32(MCTRL, 0x80)
+    FIELD(MCTRL,    PS_VERSION,         28,  4)
+    FIELD(MCTRL,    PCFG_POR_B,          8,  1)
+    FIELD(MCTRL,    INT_PCAP_LPBK,       4,  1)
+    FIELD(MCTRL,    QEMU,                3,  1)
+
+static void xlnx_zynq_devcfg_update_ixr(XlnxZynqDevcfg *s)
+{
+    qemu_set_irq(s->irq, ~s->regs[R_INT_MASK] & s->regs[R_INT_STS]);
+}
+
+static void xlnx_zynq_devcfg_reset(DeviceState *dev)
+{
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(dev);
+    int i;
+
+    for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
+        register_reset(&s->regs_info[i]);
+    }
+}
+
+static void xlnx_zynq_devcfg_dma_go(XlnxZynqDevcfg *s)
+{
+    do {
+        uint8_t buf[BTT_MAX];
+        XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
+        uint32_t btt = BTT_MAX;
+        bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
+
+        btt = MIN(btt, dmah->src_len);
+        if (loopback) {
+            btt = MIN(btt, dmah->dest_len);
+        }
+        DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
+        dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
+        dmah->src_len -= btt;
+        dmah->src_addr += btt;
+        if (loopback && (dmah->src_len || dmah->dest_len)) {
+            DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
+            dma_memory_write(&address_space_memory, dmah->dest_addr, buf, btt);
+            dmah->dest_len -= btt;
+            dmah->dest_addr += btt;
+        }
+        if (!dmah->src_len && !dmah->dest_len) {
+            DB_PRINT("dma operation finished\n");
+            s->regs[R_INT_STS] |= R_INT_STS_DMA_DONE_MASK |
+                                  R_INT_STS_DMA_P_DONE_MASK;
+            s->dma_cmd_fifo_num--;
+            memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
+                    sizeof(s->dma_cmd_fifo) - sizeof(s->dma_cmd_fifo[0]));
+        }
+        xlnx_zynq_devcfg_update_ixr(s);
+    } while (s->dma_cmd_fifo_num);
+}
+
+static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
+{
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+    xlnx_zynq_devcfg_update_ixr(s);
+}
+
+static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
+{
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
+        if (s->regs[R_LOCK] & 1 << i) {
+            val &= ~lock_ctrl_map[i];
+            val |= lock_ctrl_map[i] & s->regs[R_CTRL];
+        }
+    }
+    return val;
+}
+
+static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
+{
+    const char *device_prefix = object_get_typename(OBJECT(reg->opaque));
+    uint32_t aes_en = FIELD_EX32(val, CTRL, PCFG_AES_EN);
+
+    if (aes_en != 0 && aes_en != 7) {
+        qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
+                      "unimplemented security reset should happen!\n",
+                      device_prefix);
+    }
+}
+
+static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
+{
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+    const char *device_prefix = object_get_typename(OBJECT(s));
+
+    if (val == R_UNLOCK_MAGIC) {
+        DB_PRINT("successful unlock\n");
+        s->regs[R_CTRL] |= R_CTRL_PCAP_PR_MASK;
+        s->regs[R_CTRL] |= R_CTRL_PCFG_AES_EN_MASK;
+        memory_region_set_enabled(&s->iomem, true);
+    } else { /* bad unlock attempt */
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", device_prefix);
+        s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
+        s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
+        /* core becomes inaccessible */
+        memory_region_set_enabled(&s->iomem, false);
+    }
+}
+
+static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
+{
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+    /* once bits are locked they stay locked */
+    return s->regs[R_LOCK] | val;
+}
+
+static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
+{
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+    s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
+            .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
+            .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
+            .src_len = s->regs[R_DMA_SRC_LEN] << 2,
+            .dest_len = s->regs[R_DMA_DST_LEN] << 2,
+    };
+    s->dma_cmd_fifo_num++;
+    DB_PRINT("dma transfer started; %d total transfers pending\n",
+             s->dma_cmd_fifo_num);
+    xlnx_zynq_devcfg_dma_go(s);
+}
+
+static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
+    {   .name = "CTRL",                 .addr = A_CTRL,
+        .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
+        .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
+        .pre_write = r_ctrl_pre_write,
+        .post_write = r_ctrl_post_write,
+    },
+    {   .name = "LOCK",                 .addr = A_LOCK,
+        .rsvd = MAKE_64BIT_MASK(5, 64 - 5),
+        .pre_write = r_lock_pre_write,
+    },
+    {   .name = "CFG",                  .addr = A_CFG,
+        .reset = R_CFG_RESET,
+        .rsvd = 0xfffff00f,
+    },
+    {   .name = "INT_STS",              .addr = A_INT_STS,
+        .w1c = ~R_INT_STS_RSVD,
+        .reset = R_INT_STS_PSS_GTS_USR_B_MASK   |
+                 R_INT_STS_PSS_CFG_RESET_B_MASK |
+                 R_INT_STS_WR_FIFO_LVL_MASK,
+        .rsvd = R_INT_STS_RSVD,
+        .post_write = r_ixr_post_write,
+    },
+    {   .name = "INT_MASK",            .addr = A_INT_MASK,
+        .reset = ~0,
+        .rsvd = R_INT_STS_RSVD,
+        .post_write = r_ixr_post_write,
+    },
+    {   .name = "STATUS",               .addr = A_STATUS,
+        .reset = R_STATUS_DMA_CMD_Q_E_MASK      |
+                 R_STATUS_PSS_GTS_USR_B_MASK    |
+                 R_STATUS_PSS_CFG_RESET_B_MASK,
+        .ro = ~0,
+    },
+    {   .name = "DMA_SRC_ADDR",         .addr = A_DMA_SRC_ADDR, },
+    {   .name = "DMA_DST_ADDR",         .addr = A_DMA_DST_ADDR, },
+    {   .name = "DMA_SRC_LEN",          .addr = A_DMA_SRC_LEN,
+        .ro = MAKE_64BIT_MASK(27, 64 - 27) },
+    {   .name = "DMA_DST_LEN",          .addr = A_DMA_DST_LEN,
+        .ro = MAKE_64BIT_MASK(27, 64 - 27),
+        .post_write = r_dma_dst_len_post_write,
+    },
+    {   .name = "ROM_SHADOW",           .addr = A_ROM_SHADOW,
+        .rsvd = ~0ull,
+    },
+    {   .name = "SW_ID",                .addr = A_SW_ID, },
+    {   .name = "UNLOCK",               .addr = A_UNLOCK,
+        .post_write = r_unlock_post_write,
+    },
+    {   .name = "MCTRL",                .addr = R_MCTRL * 4,
+       /* Silicon 3.0 for version field, the mysterious reserved bit 23
+        * and QEMU platform identifier.
+        */
+       .reset = 0x2 << R_MCTRL_PS_VERSION_SHIFT | 1 << 23 | R_MCTRL_QEMU_MASK,
+       .ro = ~R_MCTRL_INT_PCAP_LPBK_MASK,
+       .rsvd = 0x00f00303,
+    },
+};
+
+static const MemoryRegionOps xlnx_zynq_devcfg_reg_ops = {
+    .read = register_read_memory,
+    .write = register_write_memory,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    }
+};
+
+static const VMStateDescription vmstate_xlnx_zynq_devcfg_dma_cmd = {
+    .name = "xlnx_zynq_devcfg_dma_cmd",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(src_addr, XlnxZynqDevcfgDMACmd),
+        VMSTATE_UINT32(dest_addr, XlnxZynqDevcfgDMACmd),
+        VMSTATE_UINT32(src_len, XlnxZynqDevcfgDMACmd),
+        VMSTATE_UINT32(dest_len, XlnxZynqDevcfgDMACmd),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
+    .name = "xlnx_zynq_devcfg",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
+                             XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
+                             vmstate_xlnx_zynq_devcfg_dma_cmd,
+                             XlnxZynqDevcfgDMACmd),
+        VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
+        VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void xlnx_zynq_devcfg_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(obj);
+    RegisterInfoArray *reg_array;
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
+    reg_array =
+        register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
+                              ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
+                              s->regs_info, s->regs,
+                              &xlnx_zynq_devcfg_reg_ops,
+                              XLNX_ZYNQ_DEVCFG_ERR_DEBUG,
+                              XLNX_ZYNQ_DEVCFG_R_MAX);
+    memory_region_add_subregion(&s->iomem,
+                                A_CTRL,
+                                &reg_array->mem);
+
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void xlnx_zynq_devcfg_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = xlnx_zynq_devcfg_reset;
+    dc->vmsd = &vmstate_xlnx_zynq_devcfg;
+}
+
+static const TypeInfo xlnx_zynq_devcfg_info = {
+    .name           = TYPE_XLNX_ZYNQ_DEVCFG,
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(XlnxZynqDevcfg),
+    .instance_init  = xlnx_zynq_devcfg_init,
+    .class_init     = xlnx_zynq_devcfg_class_init,
+};
+
+static void xlnx_zynq_devcfg_register_types(void)
+{
+    type_register_static(&xlnx_zynq_devcfg_info);
+}
+
+type_init(xlnx_zynq_devcfg_register_types)
diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
new file mode 100644
index 0000000..d40e5c8
--- /dev/null
+++ b/include/hw/dma/xlnx-zynq-devcfg.h
@@ -0,0 +1,62 @@
+/*
+ * QEMU model of the Xilinx Devcfg Interface
+ *
+ * (C) 2011 PetaLogix Pty Ltd
+ * (C) 2014 Xilinx Inc.
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.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.
+ */
+
+#ifndef XLNX_ZYNQ_DEVCFG_H
+
+#include "hw/register.h"
+#include "hw/sysbus.h"
+
+#define TYPE_XLNX_ZYNQ_DEVCFG "xlnx.ps7-dev-cfg"
+
+#define XLNX_ZYNQ_DEVCFG(obj) \
+    OBJECT_CHECK(XlnxZynqDevcfg, (obj), TYPE_XLNX_ZYNQ_DEVCFG)
+
+#define XLNX_ZYNQ_DEVCFG_R_MAX 0x118
+
+#define XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN 10
+
+typedef struct XlnxZynqDevcfgDMACmd {
+    uint32_t src_addr;
+    uint32_t dest_addr;
+    uint32_t src_len;
+    uint32_t dest_len;
+} XlnxZynqDevcfgDMACmd;
+
+typedef struct XlnxZynqDevcfg {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    XlnxZynqDevcfgDMACmd dma_cmd_fifo[XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN];
+    uint8_t dma_cmd_fifo_num;
+
+    uint32_t regs[XLNX_ZYNQ_DEVCFG_R_MAX];
+    RegisterInfo regs_info[XLNX_ZYNQ_DEVCFG_R_MAX];
+} XlnxZynqDevcfg;
+
+#define XLNX_ZYNQ_DEVCFG_H
+#endif
-- 
2.7.4

[Qemu-devel] [PATCH v8 8/8] xilinx_zynq: Connect devcfg to the Zynq machine model

[Alistair Francis]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
V7:
 - Remove unused property add
V4:
 - Small corrections to the device model logic

 hw/arm/xilinx_zynq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index aefebcf..f26c273 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -294,6 +294,12 @@ static void zynq_init(MachineState *machine)
         sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);
     }
 
+    dev = qdev_create(NULL, "xlnx.ps7-dev-cfg");
+    qdev_init_nofail(dev);
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
+    sysbus_mmio_map(busdev, 0, 0xF8007000);
+
     zynq_binfo.ram_size = ram_size;
     zynq_binfo.kernel_filename = kernel_filename;
     zynq_binfo.kernel_cmdline = kernel_cmdline;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v8 2/8] register: Add Register API

[Peter Maydell]

On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
> This API provides some encapsulation of registers and factors out some
> common functionality to common code. Bits of device state (usually MMIO
> registers) often have all sorts of access restrictions and semantics
> associated with them. This API allows you to define what those
> restrictions are on a bit-by-bit basis.
>
> Helper functions are then used to access the register which observe the
> semantics defined by the RegisterAccessInfo struct.
>
> Some features:
> Bits can be marked as read_only (ro field)
> Bits can be marked as write-1-clear (w1c field)
> Bits can be marked as reserved (rsvd field)
> Reset values can be defined (reset)
> Bits can be marked clear on read (cor)
> Pre and post action callbacks can be added to read and write ops
> Verbose debugging info can be enabled/disabled

> +uint64_t register_read(RegisterInfo *reg, uint64_t re, const char* prefix,
> +                       bool debug)
> +{
> +    uint64_t ret;
> +    const RegisterAccessInfo *ac;
> +
> +    assert(reg);
> +
> +    ac = reg->access;
> +    if (!ac || !ac->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state\n",
> +                      prefix);
> +        return 0;
> +    }
> +
> +    ret = reg->data ? register_read_val(reg) : ac->reset;
> +
> +    /* Mask based on the read enable size */
> +    ret &= re;
> +    register_write_val(reg, ret & ~ac->cor);

If you do a byte read on a word-size register then the
masking with re will result in our writing back zeroes
to the high bits of the word, won't it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 6/8] register: Add block initialise helper

[Peter Maydell]

On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Add a helper that will scan a static RegisterAccessInfo Array
> and populate a container MemoryRegion with registers as defined.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---

> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -228,6 +228,50 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      return extract64(read_val, 0, size * 8);
>  }
>
> +RegisterInfoArray *register_init_block32(DeviceState *owner,
> +                                         const RegisterAccessInfo *rae,
> +                                         int num, RegisterInfo *ri,
> +                                         uint32_t *data,
> +                                         const MemoryRegionOps *ops,
> +                                         bool debug_enabled,
> +                                         uint64_t memory_size)
> +{
> +    const char *device_prefix = object_get_typename(OBJECT(owner));
> +    RegisterInfoArray *r_array = g_new0(RegisterInfoArray, 1);
> +    int i;
> +
> +    r_array->r = g_new0(RegisterInfo *, num);
> +    r_array->num_elements = num;
> +    r_array->debug = debug_enabled;
> +    r_array->prefix = device_prefix;
> +
> +    for (i = 0; i < num; i++) {
> +        int index = rae[i].addr / 4;
> +        RegisterInfo *r = &ri[index];
> +
> +        *r = (RegisterInfo) {
> +            .data = &data[index],
> +            .data_size = sizeof(uint32_t),
> +            .access = &rae[i],
> +            .opaque = owner,
> +        };
> +        register_init(r);
> +
> +        r_array->r[i] = r;
> +    }
> +
> +    memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
> +                          device_prefix, memory_size);
> +
> +    return r_array;
> +}
> +
> +void register_finlise_block(RegisterInfoArray *r_array)

"finalize" (typo, and we prefer the -z- spelling in APIs.)

> +/**
> + * This function should be called to cleanup the registers that were initialized
> + * when calling register_init_block32()
> + *
> + * @r_array: An structure containing all of the registers. The caller is in
> + *           charge of cleaning up the memory region (r_array->mem).

What cleanup does the memory region require?

> + */
> +
> +void register_finlise_block(RegisterInfoArray *r_array);
> +
>  /* Define constants for a 32 bit register */
>
>  /* This macro will define A_FOO, for the byte address of a register

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 7/8] dma: Add Xilinx Zynq devcfg device model

[Peter Maydell]

On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a minimal model for the devcfg device which is part of Zynq.
> This model supports DMA capabilities and interrupt generation.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---

> +REG32(CFG, 0x08)
> +    FIELD(CFG,      RFIFO_TH,           10,  2)
> +    FIELD(CFG,      WFIFO_TH,            8,  2)
> +    FIELD(CFG,      RCLK_EDGE,           7,  1)
> +    FIELD(CFG,      WCLK_EDGE,           6,  1)
> +    FIELD(CFG,      DISABLE_SRC_INC,     5,  1)
> +    FIELD(CFG,      DISABLE_DST_INC,     4,  1)
> +#define R_CFG_RESET 0x50B
> +
> +REG32(INT_STS, 0x0C)
> +    FIELD(INT_STS,  PSS_GTS_USR_B,      31,  1)
> +    FIELD(INT_STS,  PSS_FST_CFG_B,      30,  1)
> +    FIELD(INT_STS,  PSS_CFG_RESET_B,    27,  1)
> +    FIELD(INT_STS,  RX_FIFO_OV,         18,  1)
> +    FIELD(INT_STS,  WR_FIFO_LVL,        17,  1)
> +    FIELD(INT_STS,  RD_FIFO_LVL,        16,  1)
> +    FIELD(INT_STS,  DMA_CMD_ERR,        15,  1)
> +    FIELD(INT_STS,  DMA_Q_OV,           14,  1)
> +    FIELD(INT_STS,  DMA_DONE,           13,  1)
> +    FIELD(INT_STS,  DMA_P_DONE,         12,  1)
> +    FIELD(INT_STS,  P2D_LEN_ERR,        11,  1)
> +    FIELD(INT_STS,  PCFG_DONE,           2,  1)
> +    #define R_INT_STS_RSVD       ((0x7 << 24) | (0x1 << 19) | (0xF < 7))

Consistency about #define indentation would be nice
(my vote is for "don't indent").

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 2/8] register: Add Register API

[Alistair Francis]

On Mon, Jun 27, 2016 at 7:24 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> This API provides some encapsulation of registers and factors out some
>> common functionality to common code. Bits of device state (usually MMIO
>> registers) often have all sorts of access restrictions and semantics
>> associated with them. This API allows you to define what those
>> restrictions are on a bit-by-bit basis.
>>
>> Helper functions are then used to access the register which observe the
>> semantics defined by the RegisterAccessInfo struct.
>>
>> Some features:
>> Bits can be marked as read_only (ro field)
>> Bits can be marked as write-1-clear (w1c field)
>> Bits can be marked as reserved (rsvd field)
>> Reset values can be defined (reset)
>> Bits can be marked clear on read (cor)
>> Pre and post action callbacks can be added to read and write ops
>> Verbose debugging info can be enabled/disabled
>
>> +uint64_t register_read(RegisterInfo *reg, uint64_t re, const char* prefix,
>> +                       bool debug)
>> +{
>> +    uint64_t ret;
>> +    const RegisterAccessInfo *ac;
>> +
>> +    assert(reg);
>> +
>> +    ac = reg->access;
>> +    if (!ac || !ac->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state\n",
>> +                      prefix);
>> +        return 0;
>> +    }
>> +
>> +    ret = reg->data ? register_read_val(reg) : ac->reset;
>> +
>> +    /* Mask based on the read enable size */
>> +    ret &= re;
>> +    register_write_val(reg, ret & ~ac->cor);
>
> If you do a byte read on a word-size register then the
> masking with re will result in our writing back zeroes
> to the high bits of the word, won't it?

You are right it will. I have changed it to use the re (read enabled)
variable to mask the cor (clear on read) variable in instead.

Then after the write function is called the return variable is masked
so that the post_write hook gets passed the correct value.

Thanks,

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v8 7/8] dma: Add Xilinx Zynq devcfg device model

[Alistair Francis]

On Mon, Jun 27, 2016 at 7:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Add a minimal model for the devcfg device which is part of Zynq.
>> This model supports DMA capabilities and interrupt generation.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>
>> +REG32(CFG, 0x08)
>> +    FIELD(CFG,      RFIFO_TH,           10,  2)
>> +    FIELD(CFG,      WFIFO_TH,            8,  2)
>> +    FIELD(CFG,      RCLK_EDGE,           7,  1)
>> +    FIELD(CFG,      WCLK_EDGE,           6,  1)
>> +    FIELD(CFG,      DISABLE_SRC_INC,     5,  1)
>> +    FIELD(CFG,      DISABLE_DST_INC,     4,  1)
>> +#define R_CFG_RESET 0x50B
>> +
>> +REG32(INT_STS, 0x0C)
>> +    FIELD(INT_STS,  PSS_GTS_USR_B,      31,  1)
>> +    FIELD(INT_STS,  PSS_FST_CFG_B,      30,  1)
>> +    FIELD(INT_STS,  PSS_CFG_RESET_B,    27,  1)
>> +    FIELD(INT_STS,  RX_FIFO_OV,         18,  1)
>> +    FIELD(INT_STS,  WR_FIFO_LVL,        17,  1)
>> +    FIELD(INT_STS,  RD_FIFO_LVL,        16,  1)
>> +    FIELD(INT_STS,  DMA_CMD_ERR,        15,  1)
>> +    FIELD(INT_STS,  DMA_Q_OV,           14,  1)
>> +    FIELD(INT_STS,  DMA_DONE,           13,  1)
>> +    FIELD(INT_STS,  DMA_P_DONE,         12,  1)
>> +    FIELD(INT_STS,  P2D_LEN_ERR,        11,  1)
>> +    FIELD(INT_STS,  PCFG_DONE,           2,  1)
>> +    #define R_INT_STS_RSVD       ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
>
> Consistency about #define indentation would be nice
> (my vote is for "don't indent").

I removed all of the indentation for the #defines.

>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v8 6/8] register: Add block initialise helper

[Alistair Francis]

On Mon, Jun 27, 2016 at 7:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Add a helper that will scan a static RegisterAccessInfo Array
>> and populate a container MemoryRegion with registers as defined.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -228,6 +228,50 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>>      return extract64(read_val, 0, size * 8);
>>  }
>>
>> +RegisterInfoArray *register_init_block32(DeviceState *owner,
>> +                                         const RegisterAccessInfo *rae,
>> +                                         int num, RegisterInfo *ri,
>> +                                         uint32_t *data,
>> +                                         const MemoryRegionOps *ops,
>> +                                         bool debug_enabled,
>> +                                         uint64_t memory_size)
>> +{
>> +    const char *device_prefix = object_get_typename(OBJECT(owner));
>> +    RegisterInfoArray *r_array = g_new0(RegisterInfoArray, 1);
>> +    int i;
>> +
>> +    r_array->r = g_new0(RegisterInfo *, num);
>> +    r_array->num_elements = num;
>> +    r_array->debug = debug_enabled;
>> +    r_array->prefix = device_prefix;
>> +
>> +    for (i = 0; i < num; i++) {
>> +        int index = rae[i].addr / 4;
>> +        RegisterInfo *r = &ri[index];
>> +
>> +        *r = (RegisterInfo) {
>> +            .data = &data[index],
>> +            .data_size = sizeof(uint32_t),
>> +            .access = &rae[i],
>> +            .opaque = owner,
>> +        };
>> +        register_init(r);
>> +
>> +        r_array->r[i] = r;
>> +    }
>> +
>> +    memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
>> +                          device_prefix, memory_size);
>> +
>> +    return r_array;
>> +}
>> +
>> +void register_finlise_block(RegisterInfoArray *r_array)
>
> "finalize" (typo, and we prefer the -z- spelling in APIs.)

Sorry it's just a habit.

>
>> +/**
>> + * This function should be called to cleanup the registers that were initialized
>> + * when calling register_init_block32()
>> + *
>> + * @r_array: An structure containing all of the registers. The caller is in
>> + *           charge of cleaning up the memory region (r_array->mem).
>
> What cleanup does the memory region require?

The device init function that calls the register init function will
also call memory_region_add_subregion(). Doesn't it need to call
memory_region_del_subregion() afterwards?

Thanks,

Alistair

>
>> + */
>> +
>> +void register_finlise_block(RegisterInfoArray *r_array);
>> +
>>  /* Define constants for a 32 bit register */
>>
>>  /* This macro will define A_FOO, for the byte address of a register
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v8 6/8] register: Add block initialise helper

[Peter Maydell]

On 27 June 2016 at 17:16, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Jun 27, 2016 at 7:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> +/**
>>> + * This function should be called to cleanup the registers that were initialized
>>> + * when calling register_init_block32()
>>> + *
>>> + * @r_array: An structure containing all of the registers. The caller is in
>>> + *           charge of cleaning up the memory region (r_array->mem).
>>
>> What cleanup does the memory region require?
>
> The device init function that calls the register init function will
> also call memory_region_add_subregion(). Doesn't it need to call
> memory_region_del_subregion() afterwards?

Somebody also needs to call object_unparent() if the MemoryRegion
is part of a lump of memory that's being g_free()d (see docs/memory.txt).
That probably ought to be this code, and we should say that this
function should only be called from the owner device's instance_finalize.

More generally: I think we should be specific, not vague, about
exactly what the caller's responsibilities are [what they are
expected to have done before calling this function; when they
are permitted to call it]; otherwise the callers will likely get
it wrong.

thanks
-- PMM