* [Qemu-devel] [PATCH v3 00/16] data-driven device registers
@ 2016-01-30 1:00 Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 01/16] memory: Allow subregions to not be printed by info mtree Alistair Francis
` (15 more replies)
0 siblings, 16 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
This patch series is based on Peter C's original register API. His
original cover letter is below.
I have added a new function memory_region_add_subregion_no_print() which
stops memory regions from being printed by 'info mtree'. This is used to
avoid evey register being printed when running 'info mtree'.
NOTE: That info qom-tree will still print all of these registers.
Future work: Allow support for memory attributes.
V3:
- 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 (3):
memory: Allow subregions to not be printed by info mtree
register: Add Register API
xlnx-zynqmp: Connect the ZynqMP IOU SLCR
Peter Crosthwaite (13):
register: Add Memory API glue
register: Add support for decoding information
register: Define REG and FIELD macros
register: QOMify
register: Add block initialise helper
bitops: Add ONES macro
dma: Add Xilinx Zynq devcfg device model
xilinx_zynq: add devcfg to machine model
qdev: Define qdev_get_gpio_out
qdev: Add qdev_pass_all_gpios API
irq: Add opaque setter routine
register: Add GPIO API
misc: Introduce ZynqMP IOU SLCR
default-configs/arm-softmmu.mak | 1 +
hw/arm/xilinx_zynq.c | 8 +
hw/arm/xlnx-zynqmp.c | 13 ++
hw/core/Makefile.objs | 1 +
hw/core/irq.c | 5 +
hw/core/qdev.c | 21 ++
hw/core/register.c | 394 ++++++++++++++++++++++++++++++++
hw/dma/Makefile.objs | 1 +
hw/dma/xlnx-zynq-devcfg.c | 406 +++++++++++++++++++++++++++++++++
hw/misc/Makefile.objs | 1 +
hw/misc/xlnx-zynqmp-iou-slcr.c | 113 +++++++++
include/exec/memory.h | 17 ++
include/hw/arm/xlnx-zynqmp.h | 2 +
include/hw/dma/xlnx-zynq-devcfg.h | 62 +++++
include/hw/irq.h | 2 +
include/hw/misc/xlnx-zynqmp-iou-slcr.h | 47 ++++
include/hw/qdev-core.h | 3 +
include/hw/register.h | 269 ++++++++++++++++++++++
include/qemu/bitops.h | 2 +
memory.c | 10 +-
20 files changed, 1377 insertions(+), 1 deletion(-)
create mode 100644 hw/core/register.c
create mode 100644 hw/dma/xlnx-zynq-devcfg.c
create mode 100644 hw/misc/xlnx-zynqmp-iou-slcr.c
create mode 100644 include/hw/dma/xlnx-zynq-devcfg.h
create mode 100644 include/hw/misc/xlnx-zynqmp-iou-slcr.h
create mode 100644 include/hw/register.h
--
2.5.0
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 01/16] memory: Allow subregions to not be printed by info mtree
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 02/16] register: Add Register API Alistair Francis
` (14 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
Add a function called memory_region_add_subregion_no_print() that
creates memory subregions that won't be printed when running
the 'info mtree' command.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
include/exec/memory.h | 17 +++++++++++++++++
memory.c | 10 +++++++++-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index c92734a..25353df 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -186,6 +186,7 @@ struct MemoryRegion {
bool skip_dump;
bool enabled;
bool warning_printed; /* For reservations */
+ bool do_not_print;
uint8_t vga_logging_count;
MemoryRegion *alias;
hwaddr alias_offset;
@@ -954,6 +955,22 @@ void memory_region_del_eventfd(MemoryRegion *mr,
void memory_region_add_subregion(MemoryRegion *mr,
hwaddr offset,
MemoryRegion *subregion);
+
+/**
+ * memory_region_add_subregion_no_print: Add a subregion to a container.
+ *
+ * The same functionality as memory_region_add_subregion except that any
+ * memory regions added by this function are not printed by 'info mtree'.
+ *
+ * @mr: the region to contain the new subregion; must be a container
+ * initialized with memory_region_init().
+ * @offset: the offset relative to @mr where @subregion is added.
+ * @subregion: the subregion to be added.
+ */
+void memory_region_add_subregion_no_print(MemoryRegion *mr,
+ hwaddr offset,
+ MemoryRegion *subregion);
+
/**
* memory_region_add_subregion_overlap: Add a subregion to a container
* with overlap.
diff --git a/memory.c b/memory.c
index d2d0a92..4b9d961 100644
--- a/memory.c
+++ b/memory.c
@@ -1827,6 +1827,14 @@ void memory_region_add_subregion(MemoryRegion *mr,
memory_region_add_subregion_common(mr, offset, subregion);
}
+void memory_region_add_subregion_no_print(MemoryRegion *mr,
+ hwaddr offset,
+ MemoryRegion *subregion)
+{
+ memory_region_add_subregion(mr, offset, subregion);
+ subregion->do_not_print = true;
+}
+
void memory_region_add_subregion_overlap(MemoryRegion *mr,
hwaddr offset,
MemoryRegion *subregion,
@@ -2217,7 +2225,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
const MemoryRegion *submr;
unsigned int i;
- if (!mr) {
+ if (!mr || mr->do_not_print) {
return;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 02/16] register: Add Register API
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 01/16] memory: Allow subregions to not be printed by info mtree Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-02-09 16:06 ` Alex Bennée
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 03/16] register: Add Memory API glue Alistair Francis
` (13 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
This API provides some encapsulation of registers and factors our 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 allow 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 throw guest errors when written certain values (ge0, ge1)
Bits can throw unimp errors when written certain values (ui0, ui1)
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>
---
V3:
- Address some comments from Fred
hw/core/Makefile.objs | 1 +
hw/core/register.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
3 files changed, 322 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 abb3560..bf95db5 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -14,4 +14,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..f0fc39c
--- /dev/null
+++ b/hw/core/register.c
@@ -0,0 +1,189 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2013 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.
+ */
+
+#include "hw/register.h"
+#include "qemu/log.h"
+
+static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t val,
+ int mask, const char *msg,
+ const char *reason)
+{
+ qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
+ reg->prefix, reg->access->name, val, msg, dir,
+ reason ? ": " : "", reason ? reason : "");
+}
+
+static inline void register_write_val(RegisterInfo *reg, uint64_t val)
+{
+ if (!reg->data) {
+ return;
+ }
+ 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:
+ abort();
+ }
+}
+
+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:
+ abort();
+ }
+ return 0; /* unreachable */
+}
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
+{
+ uint64_t old_val, new_val, test, no_w_mask;
+ const RegisterAccessInfo *ac;
+ const RegisterAccessError *rae;
+
+ 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", reg->prefix, val);
+ return;
+ }
+
+ old_val = reg->data ? register_read_val(reg) : ac->reset;
+
+ if (reg->write_lite && !~we) { /* fast path!! */
+ new_val = val;
+ goto register_write_fast;
+ }
+
+ no_w_mask = ac->ro | ac->w1c | ~we;
+
+ if (reg->debug) {
+ qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, ac->name,
+ val);
+ }
+
+ if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
+ test = (old_val ^ val) & ac->rsvd;
+ if (test) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in reserved bit"
+ "fields: %#" PRIx64 ")\n", reg->prefix, test);
+ }
+ for (rae = ac->ge1; rae && rae->mask; rae++) {
+ test = val & rae->mask;
+ if (test) {
+ register_write_log(reg, 1, test, LOG_GUEST_ERROR,
+ "invalid", rae->reason);
+ }
+ }
+ for (rae = ac->ge0; rae && rae->mask; rae++) {
+ test = ~val & rae->mask;
+ if (test) {
+ register_write_log(reg, 0, test, LOG_GUEST_ERROR,
+ "invalid", rae->reason);
+ }
+ }
+ }
+
+ if (qemu_loglevel_mask(LOG_UNIMP)) {
+ for (rae = ac->ui1; rae && rae->mask; rae++) {
+ test = val & rae->mask;
+ if (test) {
+ register_write_log(reg, 1, test, LOG_GUEST_ERROR,
+ "unimplmented", rae->reason);
+ }
+ }
+ for (rae = ac->ui0; rae && rae->mask; rae++) {
+ test = ~val & rae->mask;
+ if (test) {
+ register_write_log(reg, 0, test, LOG_GUEST_ERROR,
+ "unimplemented", rae->reason);
+ }
+ }
+ }
+
+ 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);
+ }
+
+register_write_fast:
+ register_write_val(reg, new_val);
+ if (ac->post_write) {
+ ac->post_write(reg, new_val);
+ }
+}
+
+uint64_t register_read(RegisterInfo *reg)
+{
+ 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",
+ reg->prefix);
+ return 0;
+ }
+
+ ret = reg->data ? register_read_val(reg) : ac->reset;
+
+ if (!reg->read_lite) {
+ register_write_val(reg, ret & ~ac->cor);
+ }
+
+ if (ac->post_read) {
+ ret = ac->post_read(reg, ret);
+ }
+
+ if (!reg->read_lite) {
+ if (reg->debug) {
+ qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefix,
+ ac->name, ret);
+ }
+ }
+
+ return ret;
+}
+
+void register_reset(RegisterInfo *reg)
+{
+ 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..a80427b
--- /dev/null
+++ b/include/hw/register.h
@@ -0,0 +1,132 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2013 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;
+
+/**
+ * A register access error message
+ * @mask: Bits in the register the error applies to
+ * @reason: Reason why this access is an error
+ */
+
+typedef struct RegisterAccessError {
+ uint64_t mask;
+ const char *reason;
+} RegisterAccessError;
+
+/**
+ * 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
+ *
+ * @ge1: Bits that when written 1 indicate a guest error
+ * @ge0: Bits that when written 0 indicate a guest error
+ * @ui1: Bits that when written 1 indicate use of an unimplemented feature
+ * @ui0: Bits that when written 0 indicate use of an unimplemented feature
+ *
+ * @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;
+
+ const RegisterAccessError *ge0;
+ const RegisterAccessError *ge1;
+ const RegisterAccessError *ui0;
+ const RegisterAccessError *ui1;
+
+ 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 {
+ /* <private> */
+ bool read_lite;
+ bool write_lite;
+
+ /* <public> */
+ void *data;
+ int data_size;
+
+ const RegisterAccessInfo *access;
+
+ bool debug;
+ const char *prefix;
+
+ 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
+ */
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we);
+
+/**
+ * read a value from a register, subject to its restrictions
+ * @reg: register to read from
+ * returns: value read
+ */
+
+uint64_t register_read(RegisterInfo *reg);
+
+/**
+ * reset a register
+ * @reg: register to reset
+ */
+
+void register_reset(RegisterInfo *reg);
+
+#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 03/16] register: Add Memory API glue
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 01/16] memory: Allow subregions to not be printed by info mtree Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 02/16] register: Add Register API Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 04/16] register: Add support for decoding information Alistair Francis
` (12 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
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.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/core/register.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/register.h | 30 ++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index f0fc39c..e696c43 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -187,3 +187,51 @@ void register_reset(RegisterInfo *reg)
register_write_val(reg, reg->access->reset);
}
+
+static inline void register_write_memory(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size, bool be)
+{
+ RegisterInfo *reg = opaque;
+ uint64_t we = ~0;
+ int shift = 0;
+
+ if (reg->data_size != size) {
+ we = (size == 8) ? ~0ull : (1ull << size * 8) - 1;
+ shift = 8 * (be ? reg->data_size - size - addr : addr);
+ }
+
+ assert(size + addr <= reg->data_size);
+ register_write(reg, value << shift, we << shift);
+}
+
+void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value,
+ unsigned size)
+{
+ register_write_memory(opaque, addr, value, size, true);
+}
+
+
+void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
+ unsigned size)
+{
+ register_write_memory(opaque, addr, value, size, false);
+}
+
+static inline uint64_t register_read_memory(void *opaque, hwaddr addr,
+ unsigned size, bool be)
+{
+ RegisterInfo *reg = opaque;
+ int shift = 8 * (be ? reg->data_size - size - addr : addr);
+
+ return register_read(reg) >> shift;
+}
+
+uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size)
+{
+ return register_read_memory(opaque, addr, size, true);
+}
+
+uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
+{
+ return register_read_memory(opaque, addr, size, false);
+}
diff --git a/include/hw/register.h b/include/hw/register.h
index a80427b..285d4a3 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -86,6 +86,8 @@ struct RegisterAccessInfo {
* @prefix: String prefix for log and debug messages
*
* @opaque: Opaque data for the register
+ *
+ * @mem: optional Memory region for the register
*/
struct RegisterInfo {
@@ -93,6 +95,8 @@ struct RegisterInfo {
bool read_lite;
bool write_lite;
+ MemoryRegion mem;
+
/* <public> */
void *data;
int data_size;
@@ -129,4 +133,30 @@ uint64_t register_read(RegisterInfo *reg);
void register_reset(RegisterInfo *reg);
+/**
+ * Memory API MMIO write handler that will write to a Register API register.
+ * _be for big endian variant and _le for little endian.
+ * @opaque: RegisterInfo to write to
+ * @addr: Address to write
+ * @value: Value to write
+ * @size: Number of bytes to write
+ */
+
+void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value,
+ unsigned size);
+void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
+ unsigned size);
+
+/**
+ * Memory API MMIO read handler that will read from a Register API register.
+ * _be for big endian variant and _le for little endian.
+ * @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_be(void *opaque, hwaddr addr, unsigned size);
+uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
+
#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 04/16] register: Add support for decoding information
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (2 preceding siblings ...)
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 03/16] register: Add Memory API glue Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 05/16] register: Define REG and FIELD macros Alistair Francis
` (11 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Allow defining of optional address decoding information in register
definitions. This is useful for clients that want to associate
registers with specific addresses.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V3:
- Remove unused flags option
include/hw/register.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/hw/register.h b/include/hw/register.h
index 285d4a3..307a880 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -54,6 +54,11 @@ typedef struct RegisterAccessError {
* allowing this function to modify the value before return to the client.
*/
+#define REG_DECODE_READ (1 << 0)
+#define REG_DECODE_WRITE (1 << 1)
+#define REG_DECODE_EXECUTE (1 << 2)
+#define REG_DECODE_RW (REG_DECODE_READ | REG_DECODE_WRITE)
+
struct RegisterAccessInfo {
const char *name;
uint64_t ro;
@@ -71,6 +76,10 @@ struct RegisterAccessInfo {
void (*post_write)(RegisterInfo *reg, uint64_t val);
uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+
+ struct {
+ hwaddr addr;
+ } decode;
};
/**
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 05/16] register: Define REG and FIELD macros
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (3 preceding siblings ...)
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 04/16] register: Add support for decoding information Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 06/16] register: QOMify Alistair Francis
` (10 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
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 F_EX32, AF_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>
---
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 = AF_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 "F_" variants and still save 2 name stems:
uint32_t foobar_val = F_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. */
AF_DP32(s->regs, XYZ1, TRC, 5);
/* Incorrectly set XYZ1.TRC = 16. */
AF_DP32(s->regs, XYZ1, TRC, 16);
The latter assignment results in:
warning: large integer implicitly truncated to unsigned type [-Woverflow]
include/hw/register.h | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/include/hw/register.h b/include/hw/register.h
index 307a880..38ee6f5 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -168,4 +168,42 @@ void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
+/* Define constants for a 32 bit register */
+#define REG32(reg, addr) \
+ enum { A_ ## reg = (addr) }; \
+ enum { R_ ## reg = (addr) / 4 };
+
+/* Define SHIFT, LEGTH and MASK constants for a field within a register */
+#define FIELD(reg, field, shift, length) \
+ enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)}; \
+ enum { R_ ## reg ## _ ## field ## _LENGTH = (length)}; \
+ enum { R_ ## reg ## _ ## field ## _MASK = (((1ULL << (length)) - 1) \
+ << (shift)) };
+
+/* Extract a field from a register */
+
+#define F_EX32(storage, reg, field) \
+ extract32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
+ R_ ## reg ## _ ## field ## _LENGTH)
+
+/* Extract a field from an array of registers */
+
+#define AF_EX32(regs, reg, field) \
+ F_EX32((regs)[R_ ## reg], reg, field)
+
+/* Deposit a register field. */
+
+#define F_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 AF_DP32(regs, reg, field, val) \
+ (regs)[R_ ## reg] = F_DP32((regs)[R_ ## reg], reg, field, val);
#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 06/16] register: QOMify
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (4 preceding siblings ...)
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 05/16] register: Define REG and FIELD macros Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-02-09 11:49 ` Alex Bennée
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper Alistair Francis
` (9 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
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 as well as setup
the r/w fast paths.
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>
---
hw/core/register.c | 34 ++++++++++++++++++++++++++++++++++
include/hw/register.h | 14 ++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index e696c43..939f398 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -188,6 +188,28 @@ void register_reset(RegisterInfo *reg)
register_write_val(reg, reg->access->reset);
}
+void register_init(RegisterInfo *reg)
+{
+ assert(reg);
+ const RegisterAccessInfo *ac;
+
+ if (!reg->data || !reg->access) {
+ return;
+ }
+
+ object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
+
+ ac = reg->access;
+
+ /* if there are no debug msgs and no RMW requirement, mark for fast write */
+ reg->write_lite = reg->debug || ac->ro || ac->w1c || ac->pre_write ||
+ ((ac->ge0 || ac->ge1) && qemu_loglevel_mask(LOG_GUEST_ERROR)) ||
+ ((ac->ui0 || ac->ui1) && qemu_loglevel_mask(LOG_UNIMP))
+ ? false : true;
+ /* no debug and no clear-on-read is a fast read */
+ reg->read_lite = reg->debug || ac->cor ? false : true;
+}
+
static inline void register_write_memory(void *opaque, hwaddr addr,
uint64_t value, unsigned size, bool be)
{
@@ -235,3 +257,15 @@ uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
{
return register_read_memory(opaque, addr, size, false);
}
+
+static const TypeInfo register_info = {
+ .name = TYPE_REGISTER,
+ .parent = TYPE_DEVICE,
+};
+
+static void register_register_types(void)
+{
+ type_register_static(®ister_info);
+}
+
+type_init(register_register_types)
diff --git a/include/hw/register.h b/include/hw/register.h
index 38ee6f5..3316458 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;
@@ -101,6 +102,8 @@ struct RegisterAccessInfo {
struct RegisterInfo {
/* <private> */
+ DeviceState parent_obj;
+
bool read_lite;
bool write_lite;
@@ -118,6 +121,9 @@ struct RegisterInfo {
void *opaque;
};
+#define TYPE_REGISTER "qemu,register"
+#define REGISTER(obj) OBJECT_CHECK(RegisterInfo, (obj), TYPE_REGISTER)
+
/**
* write a value to a register, subject to its restrictions
* @reg: register to write to
@@ -143,6 +149,14 @@ uint64_t register_read(RegisterInfo *reg);
void register_reset(RegisterInfo *reg);
/**
+ * Initialize a register. GPIO's are setup as IOs to the specified device.
+ * Fast paths for eligible registers are enabled.
+ * @reg: Register to initialize
+ */
+
+void register_init(RegisterInfo *reg);
+
+/**
* Memory API MMIO write handler that will write to a Register API register.
* _be for big endian variant and _le for little endian.
* @opaque: RegisterInfo to write to
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (5 preceding siblings ...)
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 06/16] register: QOMify Alistair Francis
@ 2016-01-30 1:00 ` Alistair Francis
2016-02-09 16:12 ` Alex Bennée
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 08/16] bitops: Add ONES macro Alistair Francis
` (8 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:00 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
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>
---
V3:
- Fix typo
V2:
- Use memory_region_add_subregion_no_print()
hw/core/register.c | 29 +++++++++++++++++++++++++++++
include/hw/register.h | 20 ++++++++++++++++++++
2 files changed, 49 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index 939f398..4d7dd95 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -258,6 +258,35 @@ uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
return register_read_memory(opaque, addr, size, false);
}
+void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
+ int num, RegisterInfo *ri, uint32_t *data,
+ MemoryRegion *container, const MemoryRegionOps *ops,
+ bool debug_enabled)
+{
+ const char *debug_prefix = object_get_typename(OBJECT(owner));
+ int i;
+
+ for (i = 0; i < num; i++) {
+ int index = rae[i].decode.addr / 4;
+ RegisterInfo *r = &ri[index];
+
+ *r = (RegisterInfo) {
+ .data = &data[index],
+ .data_size = sizeof(uint32_t),
+ .access = &rae[i],
+ .debug = debug_enabled,
+ .prefix = debug_prefix,
+ .opaque = owner,
+ };
+ register_init(r);
+
+ memory_region_init_io(&r->mem, OBJECT(owner), ops, r, r->access->name,
+ sizeof(uint32_t));
+ memory_region_add_subregion_no_print(container,
+ r->access->decode.addr, &r->mem);
+ }
+}
+
static const TypeInfo register_info = {
.name = TYPE_REGISTER,
.parent = TYPE_DEVICE,
diff --git a/include/hw/register.h b/include/hw/register.h
index 3316458..30dedbf 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -182,6 +182,26 @@ void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
+/**
+ * Init a block of consecutive 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
+ * @data: Array to use for register data
+ * @container: Memory region to contain new registers
+ * @ops: Memory region ops to access registers.
+ * @debug enabled: turn on/off verbose debug information
+ */
+
+void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
+ int num, RegisterInfo *ri, uint32_t *data,
+ MemoryRegion *container, const MemoryRegionOps *ops,
+ bool debug_enabled);
+
/* Define constants for a 32 bit register */
#define REG32(reg, addr) \
enum { A_ ## reg = (addr) }; \
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 08/16] bitops: Add ONES macro
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (6 preceding siblings ...)
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model Alistair Francis
` (7 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Little macro that just gives you N ones (justified to LSB).
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
include/qemu/bitops.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 8164225..27bf98d 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -430,4 +430,6 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
return (value & ~mask) | ((fieldval << start) & mask);
}
+#define ONES(num) ((num) == 64 ? ~0ull : (1ull << (num)) - 1)
+
#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (7 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 08/16] bitops: Add ONES macro Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-02-09 17:08 ` Alex Bennée
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 10/16] xilinx_zynq: add devcfg to machine model Alistair Francis
` (6 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Minimal device model for devcfg module of Zynq. DMA capabilities and
interrupt generation supported.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
default-configs/arm-softmmu.mak | 1 +
hw/dma/Makefile.objs | 1 +
hw/dma/xlnx-zynq-devcfg.c | 406 ++++++++++++++++++++++++++++++++++++++
include/hw/dma/xlnx-zynq-devcfg.h | 62 ++++++
4 files changed, 470 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 d9b90a5..bc3914d 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -66,6 +66,7 @@ CONFIG_PXA2XX=y
CONFIG_BITBANG_I2C=y
CONFIG_FRAMEBUFFER=y
CONFIG_XILINX_SPIPS=y
+CONFIG_ZYNQ_DEVCFG=y
CONFIG_ARM11SCU=y
CONFIG_A9SCU=y
diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
index 0e65ed0..eaf0a81 100644
--- a/hw/dma/Makefile.objs
+++ b/hw/dma/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_PL330) += pl330.o
common-obj-$(CONFIG_I82374) += i82374.o
common-obj-$(CONFIG_I8257) += i8257.o
common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
+common-obj-$(CONFIG_ZYNQ_DEVCFG) += xlnx-zynq-devcfg.o
common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
common-obj-$(CONFIG_STP2000) += sparc32_dma.o
common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
new file mode 100644
index 0000000..747d830
--- /dev/null
+++ b/hw/dma/xlnx-zynq-devcfg.c
@@ -0,0 +1,406 @@
+/*
+ * 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 "hw/dma/xlnx-zynq-devcfg.h"
+#include "qemu/bitops.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
+
+#define FREQ_HZ 900000000
+
+#define BTT_MAX 0x400
+
+#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
+#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
+#endif
+
+#define DB_PRINT(...) do { \
+ if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
+ qemu_log("%s: ", __func__); \
+ qemu_log(__VA_ARGS__); \
+ } \
+} while (0);
+
+REG32(CTRL, 0x00)
+ FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr ignored */
+ FIELD(CTRL, PCAP_PR, 27, 1) /* Forced to 0 on bad unlock */
+ FIELD(CTRL, PCAP_MODE, 26, 1)
+ FIELD(CTRL, MULTIBOOT_EN, 24, 1)
+ FIELD(CTRL, USER_MODE, 15, 1)
+ FIELD(CTRL, PCFG_AES_FUSE, 12, 1)
+ FIELD(CTRL, PCFG_AES_EN, 9, 3)
+ FIELD(CTRL, SEU_EN, 8, 1)
+ FIELD(CTRL, SEC_EN, 7, 1)
+ FIELD(CTRL, SPNIDEN, 6, 1)
+ FIELD(CTRL, SPIDEN, 5, 1)
+ FIELD(CTRL, NIDEN, 4, 1)
+ FIELD(CTRL, DBGEN, 3, 1)
+ FIELD(CTRL, DAP_EN, 0, 3)
+
+REG32(LOCK, 0x04)
+ #define AES_FUSE_LOCK 4
+ #define AES_EN_LOCK 3
+ #define SEU_LOCK 2
+ #define SEC_LOCK 1
+ #define DBG_LOCK 0
+
+/* mapping bits in R_LOCK to what they lock in R_CTRL */
+static const uint32_t lock_ctrl_map[] = {
+ [AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
+ [AES_EN_LOCK] = R_CTRL_PCFG_AES_EN_MASK,
+ [SEU_LOCK] = R_CTRL_SEU_EN_MASK,
+ [SEC_LOCK] = R_CTRL_SEC_EN_MASK,
+ [DBG_LOCK] = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
+ R_CTRL_NIDEN_MASK | R_CTRL_DBGEN_MASK |
+ R_CTRL_DAP_EN_MASK,
+};
+
+REG32(CFG, 0x08)
+ FIELD(CFG, RFIFO_TH, 10, 2)
+ FIELD(CFG, WFIFO_TH, 8, 2)
+ FIELD(CFG, RCLK_EDGE, 7, 1)
+ FIELD(CFG, WCLK_EDGE, 6, 1)
+ FIELD(CFG, DISABLE_SRC_INC, 5, 1)
+ FIELD(CFG, DISABLE_DST_INC, 4, 1)
+#define R_CFG_RO 0xFFFFF000
+#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)
+{
+ for (;;) {
+ uint8_t buf[BTT_MAX];
+ XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
+ uint32_t btt = BTT_MAX;
+ bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
+
+ btt = MIN(btt, dmah->src_len);
+ if (loopback) {
+ btt = MIN(btt, dmah->dest_len);
+ }
+ DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
+ dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
+ dmah->src_len -= btt;
+ dmah->src_addr += btt;
+ if (loopback) {
+ DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
+ dma_memory_write(&address_space_memory, dmah->dest_addr, buf, btt);
+ dmah->dest_len -= btt;
+ dmah->dest_addr += btt;
+ }
+ if (!dmah->src_len && !dmah->dest_len) {
+ DB_PRINT("dma operation finished\n");
+ s->regs[R_INT_STS] |= R_INT_STS_DMA_DONE_MASK |
+ R_INT_STS_DMA_P_DONE_MASK;
+ s->dma_cmd_fifo_num--;
+ memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
+ sizeof(*s->dma_cmd_fifo) * XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN
+ - 1);
+ }
+ xlnx_zynq_devcfg_update_ixr(s);
+ if (!s->dma_cmd_fifo_num) { /* All done */
+ return;
+ }
+ }
+}
+
+static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ xlnx_zynq_devcfg_update_ixr(s);
+}
+
+static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
+ if (s->regs[R_LOCK] & 1 << i) {
+ val &= ~lock_ctrl_map[i];
+ val |= lock_ctrl_map[i] & s->regs[R_CTRL];
+ }
+ }
+ return val;
+}
+
+static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
+{
+ uint32_t aes_en = F_EX32(val, CTRL, PCFG_AES_EN);
+
+ if (aes_en != 0 && aes_en != 7) {
+ qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
+ "unimplemented security reset should happen!\n",
+ reg->prefix);
+ }
+}
+
+static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ if (val == R_UNLOCK_MAGIC) {
+ DB_PRINT("successful unlock\n");
+ /* BootROM will have already done the actual unlock so no need to do
+ * anything in successful subsequent unlock
+ */
+ } else { /* bad unlock attempt */
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", reg->prefix);
+ s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
+ s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
+ /* core becomes inaccessible */
+ memory_region_set_enabled(&s->iomem, false);
+ }
+}
+
+static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ /* once bits are locked they stay locked */
+ return s->regs[R_LOCK] | val;
+}
+
+static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
+ .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
+ .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
+ .src_len = s->regs[R_DMA_SRC_LEN] << 2,
+ .dest_len = s->regs[R_DMA_DST_LEN] << 2,
+ };
+ s->dma_cmd_fifo_num++;
+ DB_PRINT("dma transfer started; %d total transfers pending\n",
+ s->dma_cmd_fifo_num);
+ xlnx_zynq_devcfg_dma_go(s);
+}
+
+static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
+ { .name = "CTRL", .decode.addr = A_CTRL,
+ .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
+ .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
+ .ui1 = (RegisterAccessError[]) {
+ { .mask = R_CTRL_FORCE_RST_MASK,
+ .reason = "PS reset not implemented" },
+ { .mask = R_CTRL_PCAP_MODE_MASK,
+ .reason = "FPGA Fabric doesn't exist" },
+ { .mask = R_CTRL_PCFG_AES_EN_MASK,
+ .reason = "AES not implemented" },
+ {},
+ },
+ .pre_write = r_ctrl_pre_write,
+ .post_write = r_ctrl_post_write,
+ },
+ { .name = "LOCK", .decode.addr = A_LOCK,
+ .rsvd = ~ONES(5),
+ .pre_write = r_lock_pre_write,
+ },
+ { .name = "CFG", .decode.addr = A_CFG,
+ .reset = 1 << R_CFG_RFIFO_TH_SHIFT | 1 << R_CFG_WFIFO_TH_SHIFT | 0x8,
+ .rsvd = 0xfffff00f,
+ },
+ { .name = "INT_STS", .decode.addr = A_INT_STS,
+ .w1c = ~R_INT_STS_RSVD,
+ .reset = R_INT_STS_PSS_GTS_USR_B_MASK |
+ R_INT_STS_PSS_CFG_RESET_B_MASK |
+ R_INT_STS_WR_FIFO_LVL_MASK,
+ .rsvd = R_INT_STS_RSVD,
+ .post_write = r_ixr_post_write,
+ },
+ { .name = "INT_MASK", .decode.addr = A_INT_MASK,
+ .reset = ~0,
+ .rsvd = R_INT_STS_RSVD,
+ .post_write = r_ixr_post_write,
+ },
+ { .name = "STATUS", .decode.addr = A_STATUS,
+ .reset = R_STATUS_DMA_CMD_Q_E_MASK |
+ R_STATUS_PSS_GTS_USR_B_MASK |
+ R_STATUS_PSS_CFG_RESET_B_MASK,
+ .ro = ~0,
+ },
+ { .name = "DMA_SRC_ADDR", .decode.addr = A_DMA_SRC_ADDR, },
+ { .name = "DMA_DST_ADDR", .decode.addr = A_DMA_DST_ADDR, },
+ { .name = "DMA_SRC_LEN", .decode.addr = A_DMA_SRC_LEN,
+ .ro = ~ONES(27) },
+ { .name = "DMA_DST_LEN", .decode.addr = A_DMA_DST_LEN,
+ .ro = ~ONES(27),
+ .post_write = r_dma_dst_len_post_write,
+ },
+ { .name = "ROM_SHADOW", .decode.addr = A_ROM_SHADOW,
+ .rsvd = ~0ull,
+ },
+ { .name = "SW_ID", .decode.addr = A_SW_ID, },
+ { .name = "UNLOCK", .decode.addr = A_UNLOCK,
+ .post_write = r_unlock_post_write,
+ },
+ { .name = "MCTRL", .decode.addr = R_MCTRL * 4,
+ /* Silicon 3.0 for version field, the mysterious reserved bit 23
+ * and QEMU platform identifier.
+ */
+ .reset = 0x2 << R_MCTRL_PS_VERSION_SHIFT | 1 << 23 | R_MCTRL_QEMU_MASK,
+ .ro = ~R_MCTRL_INT_PCAP_LPBK_MASK,
+ .rsvd = 0x00f00303,
+ },
+};
+
+static const MemoryRegionOps xlnx_zynq_devcfg_reg_ops = {
+ .read = register_read_memory_le,
+ .write = register_write_memory_le,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ }
+};
+
+static const VMStateDescription vmstate_xlnx_zynq_devcfg_dma_cmd = {
+ .name = "xlnx_zynq_devcfg_dma_cmd",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(src_addr, XlnxZynqDevcfgDMACmd),
+ VMSTATE_UINT32(dest_addr, XlnxZynqDevcfgDMACmd),
+ VMSTATE_UINT32(src_len, XlnxZynqDevcfgDMACmd),
+ VMSTATE_UINT32(dest_len, XlnxZynqDevcfgDMACmd),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
+ .name = "xlnx_zynq_devcfg",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
+ XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
+ vmstate_xlnx_zynq_devcfg_dma_cmd,
+ XlnxZynqDevcfgDMACmd),
+ VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
+ VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void xlnx_zynq_devcfg_init(Object *obj)
+{
+ SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(obj);
+
+ sysbus_init_irq(sbd, &s->irq);
+
+ memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
+ register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
+ ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
+ s->regs_info, s->regs, &s->iomem,
+ &xlnx_zynq_devcfg_reg_ops,
+ XLNX_ZYNQ_DEVCFG_ERR_DEBUG);
+ sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void xlnx_zynq_devcfg_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->reset = xlnx_zynq_devcfg_reset;
+ dc->vmsd = &vmstate_xlnx_zynq_devcfg;
+}
+
+static const TypeInfo xlnx_zynq_devcfg_info = {
+ .name = TYPE_XLNX_ZYNQ_DEVCFG,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(XlnxZynqDevcfg),
+ .instance_init = xlnx_zynq_devcfg_init,
+ .class_init = xlnx_zynq_devcfg_class_init,
+};
+
+static void xlnx_zynq_devcfg_register_types(void)
+{
+ type_register_static(&xlnx_zynq_devcfg_info);
+}
+
+type_init(xlnx_zynq_devcfg_register_types)
diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
new file mode 100644
index 0000000..d40e5c8
--- /dev/null
+++ b/include/hw/dma/xlnx-zynq-devcfg.h
@@ -0,0 +1,62 @@
+/*
+ * QEMU model of the Xilinx Devcfg Interface
+ *
+ * (C) 2011 PetaLogix Pty Ltd
+ * (C) 2014 Xilinx Inc.
+ * Written by Peter Crosthwaite <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.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 10/16] xilinx_zynq: add devcfg to machine model
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (8 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 11/16] qdev: Define qdev_get_gpio_out Alistair Francis
` (5 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
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>
---
hw/arm/xilinx_zynq.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 66e7f27..4ae0b13 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -275,6 +275,14 @@ 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");
+ object_property_add_child(qdev_get_machine(), "xlnx-devcfg", OBJECT(dev),
+ NULL);
+ 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.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 11/16] qdev: Define qdev_get_gpio_out
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (9 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 10/16] xilinx_zynq: add devcfg to machine model Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 12/16] qdev: Add qdev_pass_all_gpios API Alistair Francis
` (4 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
An API similar to the existing qdev_get_gpio_in() except gets outputs.
Useful for:
1: Implementing lightweight devices that don't want to keep pointers
to their own GPIOs. They can get their GPIO pointers at runtime from
QOM using this API.
2: testing or debugging code which may wish to override the
hardware generated value of of a GPIO with a user specified value
(E.G. interrupt injection).
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/core/qdev.c | 12 ++++++++++++
include/hw/qdev-core.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 655f5d5..57ce4e7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -489,6 +489,18 @@ qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
return qdev_get_gpio_in_named(dev, NULL, n);
}
+qemu_irq qdev_get_gpio_out_named(DeviceState *dev, const char *name, int n)
+{
+ char *propname = g_strdup_printf("%s[%d]",
+ name ? name : "unnamed-gpio-out", n);
+ return (qemu_irq)object_property_get_link(OBJECT(dev), propname, NULL);
+}
+
+qemu_irq qdev_get_gpio_out(DeviceState *dev, int n)
+{
+ return qdev_get_gpio_out_named(dev, NULL, n);
+}
+
void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
qemu_irq pin)
{
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index abcdee8..0a09b8a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -287,6 +287,8 @@ bool qdev_machine_modified(void);
qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
+qemu_irq qdev_get_gpio_out(DeviceState *dev, int n);
+qemu_irq qdev_get_gpio_out_named(DeviceState *dev, const char *name, int n);
void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 12/16] qdev: Add qdev_pass_all_gpios API
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (10 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 11/16] qdev: Define qdev_get_gpio_out Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 13/16] irq: Add opaque setter routine Alistair Francis
` (3 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
For passing all GPIOs of all names from a contained device to a
container.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/core/qdev.c | 9 +++++++++
include/hw/qdev-core.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 57ce4e7..4cdba10 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -589,6 +589,15 @@ void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
QLIST_INSERT_HEAD(&container->gpios, ngl, node);
}
+void qdev_pass_all_gpios(DeviceState *dev, DeviceState *container)
+{
+ NamedGPIOList *ngl;
+
+ QLIST_FOREACH(ngl, &dev->gpios, node) {
+ qdev_pass_gpios(dev, container, ngl->name);
+ }
+}
+
BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
{
BusState *bus;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0a09b8a..753673c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -312,6 +312,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
const char *name);
+void qdev_pass_all_gpios(DeviceState *dev, DeviceState *container);
BusState *qdev_get_parent_bus(DeviceState *dev);
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 13/16] irq: Add opaque setter routine
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (11 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 12/16] qdev: Add qdev_pass_all_gpios API Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 14/16] register: Add GPIO API Alistair Francis
` (2 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Add a routine to set or override the opaque data of an IRQ.
Qdev currently always initialises IRQ opaque as the device itself.
This allows you to override to a custom opaque in the case where
there is extra or different data needed.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/core/irq.c | 5 +++++
include/hw/irq.h | 2 ++
2 files changed, 7 insertions(+)
diff --git a/hw/core/irq.c b/hw/core/irq.c
index 8a62a36..4a41059 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -76,6 +76,11 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
return irq;
}
+void qemu_irq_set_opaque(qemu_irq irq, void *opaque)
+{
+ irq->opaque = opaque;
+}
+
void qemu_free_irqs(qemu_irq *s, int n)
{
int i;
diff --git a/include/hw/irq.h b/include/hw/irq.h
index 4c4c2ea..edad0fc 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -44,6 +44,8 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n);
qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
void *opaque, int n);
+void qemu_irq_set_opaque(qemu_irq irq, void *opaque);
+
void qemu_free_irqs(qemu_irq *s, int n);
void qemu_free_irq(qemu_irq irq);
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 14/16] register: Add GPIO API
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (12 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 13/16] irq: Add opaque setter routine Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 15/16] misc: Introduce ZynqMP IOU SLCR Alistair Francis
2016-02-09 17:22 ` [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alex Bennée
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Add GPIO functionality to the register API. This allows association
and automatic connection of GPIOs to bits in registers. GPIO inputs
will attach to handlers that automatically set read-only bits in
registers. GPIO outputs will be updated to reflect their field value
when their respective registers are written (or reset). Supports
active low GPIOs.
This is particularly effective for implementing system level
controllers, where heterogenous collections of control signals are
placed is a SoC specific peripheral then propagated all over the
system.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[ EI Changes:
* register: Add a polarity field to GPIO connections
Makes it possible to directly connect active low signals
to generic interrupt pins.
]
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/core/register.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/register.h | 26 ++++++++++++++
2 files changed, 120 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index 4d7dd95..f461cd7 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -138,6 +138,8 @@ void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
register_write_fast:
register_write_val(reg, new_val);
+ register_refresh_gpios(reg, old_val);
+
if (ac->post_write) {
ac->post_write(reg, new_val);
}
@@ -180,18 +182,85 @@ uint64_t register_read(RegisterInfo *reg)
void register_reset(RegisterInfo *reg)
{
assert(reg);
+ uint64_t old_val;
if (!reg->data || !reg->access) {
return;
}
+ old_val = register_read_val(reg);
+
register_write_val(reg, reg->access->reset);
+ register_refresh_gpios(reg, old_val);
+}
+
+void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value)
+{
+ const RegisterAccessInfo *ac;
+ const RegisterGPIOMapping *gpio;
+
+ ac = reg->access;
+ for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
+ int i;
+
+ if (gpio->input) {
+ continue;
+ }
+
+ for (i = 0; i < gpio->num; ++i) {
+ uint64_t gpio_value, gpio_value_old;
+
+ qemu_irq gpo = qdev_get_gpio_out_named(DEVICE(reg), gpio->name, i);
+ gpio_value_old = extract64(old_value,
+ gpio->bit_pos + i * gpio->width,
+ gpio->width) ^ gpio->polarity;
+ gpio_value = extract64(register_read_val(reg),
+ gpio->bit_pos + i * gpio->width,
+ gpio->width) ^ gpio->polarity;
+ if (!(gpio_value_old ^ gpio_value)) {
+ continue;
+ }
+ if (reg->debug && gpo) {
+ qemu_log("refreshing gpio out %s to %" PRIx64 "\n",
+ gpio->name, gpio_value);
+ }
+ qemu_set_irq(gpo, gpio_value);
+ }
+ }
+}
+
+typedef struct DeviceNamedGPIOHandlerOpaque {
+ DeviceState *dev;
+ const char *name;
+} DeviceNamedGPIOHandlerOpaque;
+
+static void register_gpio_handler(void *opaque, int n, int level)
+{
+ DeviceNamedGPIOHandlerOpaque *gho = opaque;
+ RegisterInfo *reg = REGISTER(gho->dev);
+
+ const RegisterAccessInfo *ac;
+ const RegisterGPIOMapping *gpio;
+
+ ac = reg->access;
+ for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
+ if (gpio->input && !strcmp(gho->name, gpio->name)) {
+ register_write_val(reg, deposit64(register_read_val(reg),
+ gpio->bit_pos + n * gpio->width,
+ gpio->width,
+ level ^ gpio->polarity));
+ return;
+ }
+ }
+
+ abort();
}
void register_init(RegisterInfo *reg)
{
assert(reg);
const RegisterAccessInfo *ac;
+ const RegisterGPIOMapping *gpio;
if (!reg->data || !reg->access) {
return;
@@ -200,6 +269,30 @@ void register_init(RegisterInfo *reg)
object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
ac = reg->access;
+ for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
+ if (!gpio->num) {
+ ((RegisterGPIOMapping *)gpio)->num = 1;
+ }
+ if (!gpio->width) {
+ ((RegisterGPIOMapping *)gpio)->width = 1;
+ }
+ if (gpio->input) {
+ DeviceNamedGPIOHandlerOpaque gho = {
+ .name = gpio->name,
+ .dev = DEVICE(reg),
+ };
+ qemu_irq irq;
+
+ qdev_init_gpio_in_named(DEVICE(reg), register_gpio_handler,
+ gpio->name, gpio->num);
+ irq = qdev_get_gpio_in_named(DEVICE(reg), gpio->name, gpio->num);
+ qemu_irq_set_opaque(irq, g_memdup(&gho, sizeof(gho)));
+ } else {
+ qemu_irq *gpos = g_new0(qemu_irq, gpio->num);
+
+ qdev_init_gpio_out_named(DEVICE(reg), gpos, gpio->name, gpio->num);
+ }
+ }
/* if there are no debug msgs and no RMW requirement, mark for fast write */
reg->write_lite = reg->debug || ac->ro || ac->w1c || ac->pre_write ||
@@ -279,6 +372,7 @@ void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
.opaque = owner,
};
register_init(r);
+ qdev_pass_all_gpios(DEVICE(r), owner);
memory_region_init_io(&r->mem, OBJECT(owner), ops, r, r->access->name,
sizeof(uint32_t));
diff --git a/include/hw/register.h b/include/hw/register.h
index 30dedbf..9a8a94b 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -13,6 +13,7 @@
#include "hw/qdev-core.h"
#include "exec/memory.h"
+#include "hw/irq.h"
typedef struct RegisterInfo RegisterInfo;
typedef struct RegisterAccessInfo RegisterAccessInfo;
@@ -28,6 +29,18 @@ typedef struct RegisterAccessError {
const char *reason;
} RegisterAccessError;
+#define REG_GPIO_POL_HIGH 0
+#define REG_GPIO_POL_LOW 1
+
+typedef struct RegisterGPIOMapping {
+ const char *name;
+ uint8_t bit_pos;
+ bool input;
+ bool polarity;
+ uint8_t num;
+ uint8_t width;
+} RegisterGPIOMapping;
+
/**
* Access description for a register that is part of guest accessible device
* state.
@@ -78,6 +91,8 @@ struct RegisterAccessInfo {
uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+ const RegisterGPIOMapping *gpios;
+
struct {
hwaddr addr;
} decode;
@@ -157,6 +172,17 @@ void register_reset(RegisterInfo *reg);
void register_init(RegisterInfo *reg);
/**
+ * Refresh GPIO outputs based on diff between old value register current value.
+ * GPIOs are refreshed for fields where the old value differs to the current
+ * value.
+ *
+ * @reg: Register to refresh GPIO outs
+ * @old_value: previous value of register
+ */
+
+void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value);
+
+/**
* Memory API MMIO write handler that will write to a Register API register.
* _be for big endian variant and _le for little endian.
* @opaque: RegisterInfo to write to
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 15/16] misc: Introduce ZynqMP IOU SLCR
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (13 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 14/16] register: Add GPIO API Alistair Francis
@ 2016-01-30 1:01 ` Alistair Francis
2016-02-09 17:22 ` [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alex Bennée
15 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-01-30 1:01 UTC (permalink / raw)
To: qemu-devel, peter.maydell, fred.konrad
Cc: edgar.iglesias, edgar.iglesias, crosthwaitepeter, afaerber,
alistair.francis
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
IOU = I/O Unit
SLCR = System Level Control Registers
This IP is a misc collections of control registers that switch various
properties of system IPs. Currently the only thing implemented is the
SD_SLOTTYPE control (implemented as a GPIO output).
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/misc/Makefile.objs | 1 +
hw/misc/xlnx-zynqmp-iou-slcr.c | 113 +++++++++++++++++++++++++++++++++
include/hw/misc/xlnx-zynqmp-iou-slcr.h | 47 ++++++++++++++
3 files changed, 161 insertions(+)
create mode 100644 hw/misc/xlnx-zynqmp-iou-slcr.c
create mode 100644 include/hw/misc/xlnx-zynqmp-iou-slcr.h
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index d4765c2..6e01250 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -39,6 +39,7 @@ obj-$(CONFIG_OMAP) += omap_tap.o
obj-$(CONFIG_SLAVIO) += slavio_misc.o
obj-$(CONFIG_ZYNQ) += zynq_slcr.o
obj-$(CONFIG_ZYNQ) += zynq-xadc.o
+obj-$(CONFIG_ZYNQ) += xlnx-zynqmp-iou-slcr.o
obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
obj-$(CONFIG_PVPANIC) += pvpanic.o
diff --git a/hw/misc/xlnx-zynqmp-iou-slcr.c b/hw/misc/xlnx-zynqmp-iou-slcr.c
new file mode 100644
index 0000000..35b989c
--- /dev/null
+++ b/hw/misc/xlnx-zynqmp-iou-slcr.c
@@ -0,0 +1,113 @@
+/*
+ * Xilinx ZynqMP IOU System Level Control Registers (SLCR)
+ *
+ * Copyright (c) 2013 Xilinx Inc
+ * Copyright (c) 2013 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 "hw/misc/xlnx-zynqmp-iou-slcr.h"
+
+#ifndef XLNX_ZYNQMP_IOU_SLCR_ERR_DEBUG
+#define XLNX_ZYNQMP_IOU_SLCR_ERR_DEBUG 0
+#endif
+
+REG32(SD_SLOTTYPE, 0x310)
+ #define R_SD_SLOTTYPE_RSVD 0xffff7ffe
+
+static const RegisterAccessInfo xlnx_zynqmp_iou_slcr_regs_info[] = {
+ { .name = "SD Slot TYPE", .decode.addr = A_SD_SLOTTYPE,
+ .rsvd = R_SD_SLOTTYPE_RSVD,
+ .gpios = (RegisterGPIOMapping []) {
+ { .name = "SD0_SLOTTYPE", .bit_pos = 0 },
+ { .name = "SD1_SLOTTYPE", .bit_pos = 15 },
+ {},
+ }
+ }
+ /* FIXME: Complete device model */
+};
+
+static void xlnx_zynqmp_iou_slcr_reset(DeviceState *dev)
+{
+ XlnxZynqMPIOUSLCR *s = XLNX_ZYNQMP_IOU_SLCR(dev);
+ int i;
+
+ for (i = 0; i < XLNX_ZYNQ_MP_IOU_SLCR_R_MAX; ++i) {
+ register_reset(&s->regs_info[i]);
+ }
+}
+
+static const MemoryRegionOps xlnx_zynqmp_iou_slcr_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 xlnx_zynqmp_iou_slcr_init(Object *obj)
+{
+ XlnxZynqMPIOUSLCR *s = XLNX_ZYNQMP_IOU_SLCR(obj);
+
+ memory_region_init(&s->iomem, obj, "MMIO", XLNX_ZYNQ_MP_IOU_SLCR_R_MAX * 4);
+ register_init_block32(DEVICE(obj), xlnx_zynqmp_iou_slcr_regs_info,
+ ARRAY_SIZE(xlnx_zynqmp_iou_slcr_regs_info),
+ s->regs_info, s->regs, &s->iomem,
+ &xlnx_zynqmp_iou_slcr_ops,
+ XLNX_ZYNQMP_IOU_SLCR_ERR_DEBUG);
+ sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+}
+
+static const VMStateDescription vmstate_xlnx_zynqmp_iou_slcr = {
+ .name = "xlnx_zynqmp_iou_slcr",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32_ARRAY(regs, XlnxZynqMPIOUSLCR,
+ XLNX_ZYNQ_MP_IOU_SLCR_R_MAX),
+ VMSTATE_END_OF_LIST(),
+ }
+};
+
+static void xlnx_zynqmp_iou_slcr_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->reset = xlnx_zynqmp_iou_slcr_reset;
+ dc->vmsd = &vmstate_xlnx_zynqmp_iou_slcr;
+}
+
+static const TypeInfo xlnx_zynqmp_iou_slcr_info = {
+ .name = TYPE_XLNX_ZYNQMP_IOU_SLCR,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(XlnxZynqMPIOUSLCR),
+ .class_init = xlnx_zynqmp_iou_slcr_class_init,
+ .instance_init = xlnx_zynqmp_iou_slcr_init,
+};
+
+static void xlnx_zynqmp_iou_slcr_register_types(void)
+{
+ type_register_static(&xlnx_zynqmp_iou_slcr_info);
+}
+
+type_init(xlnx_zynqmp_iou_slcr_register_types)
diff --git a/include/hw/misc/xlnx-zynqmp-iou-slcr.h b/include/hw/misc/xlnx-zynqmp-iou-slcr.h
new file mode 100644
index 0000000..1a3f7e9
--- /dev/null
+++ b/include/hw/misc/xlnx-zynqmp-iou-slcr.h
@@ -0,0 +1,47 @@
+/*
+ * Xilinx ZynqMP IOU system level control registers (SLCR)
+ *
+ * Copyright (c) 2013 Xilinx Inc
+ * Copyright (c) 2013 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 "hw/sysbus.h"
+#include "hw/register.h"
+
+#define TYPE_XLNX_ZYNQMP_IOU_SLCR "xlnx_zynqmp-iou-slcr"
+
+#define XLNX_ZYNQMP_IOU_SLCR(obj) \
+ OBJECT_CHECK(XlnxZynqMPIOUSLCR, (obj), TYPE_XLNX_ZYNQMP_IOU_SLCR)
+
+#define XLNX_ZYNQ_MP_IOU_SLCR_R_MAX (0x314/4)
+
+typedef struct XlnxZynqMPIOUSLCR XlnxZynqMPIOUSLCR;
+
+struct XlnxZynqMPIOUSLCR {
+ /*< private >*/
+ SysBusDevice busdev;
+
+ /*< public >*/
+ MemoryRegion iomem;
+
+ uint32_t regs[XLNX_ZYNQ_MP_IOU_SLCR_R_MAX];
+ RegisterInfo regs_info[XLNX_ZYNQ_MP_IOU_SLCR_R_MAX];
+};
--
2.5.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/16] register: QOMify
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 06/16] register: QOMify Alistair Francis
@ 2016-02-09 11:49 ` Alex Bennée
2016-02-09 18:09 ` Alistair Francis
0 siblings, 1 reply; 27+ messages in thread
From: Alex Bennée @ 2016-02-09 11:49 UTC (permalink / raw)
To: Alistair Francis
Cc: edgar.iglesias, peter.maydell, qemu-devel, crosthwaitepeter,
edgar.iglesias, afaerber, fred.konrad
Alistair Francis <alistair.francis@xilinx.com> writes:
> 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 as well as setup
> the r/w fast paths.
I don't know if there has been bit-rot since posting but this currently
doesn't build:
CC hw/core/register.o
hw/core/register.c:394:1: error: return type defaults to ‘int’ [-Werror=return-type]
type_init(register_register_types)
^
hw/core/register.c:394:1: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
hw/core/register.c: In function ‘type_init’:
hw/core/register.c:394:1: error: old-style function definition [-Werror=old-style-definition]
hw/core/register.c:394:1: error: expected ‘{’ at end of input
hw/core/register.c: At top level:
hw/core/register.c:389:13: error: ‘register_register_types’ defined but not used [-Werror=unused-function]
static void register_register_types(void)
^
hw/core/register.c: In function ‘type_init’:
hw/core/register.c:394:1: error: control reaches end of non-void function [-Werror=return-type]
type_init(register_register_types)
^
cc1: all warnings being treated as errors
make: *** [hw/core/register.o] Error 1
make: Target `all' not remade because of errors.
>
> 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>
> ---
>
> hw/core/register.c | 34 ++++++++++++++++++++++++++++++++++
> include/hw/register.h | 14 ++++++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index e696c43..939f398 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -188,6 +188,28 @@ void register_reset(RegisterInfo *reg)
> register_write_val(reg, reg->access->reset);
> }
>
> +void register_init(RegisterInfo *reg)
> +{
> + assert(reg);
> + const RegisterAccessInfo *ac;
> +
> + if (!reg->data || !reg->access) {
> + return;
> + }
> +
> + object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
> +
> + ac = reg->access;
> +
> + /* if there are no debug msgs and no RMW requirement, mark for fast write */
> + reg->write_lite = reg->debug || ac->ro || ac->w1c || ac->pre_write ||
> + ((ac->ge0 || ac->ge1) && qemu_loglevel_mask(LOG_GUEST_ERROR)) ||
> + ((ac->ui0 || ac->ui1) && qemu_loglevel_mask(LOG_UNIMP))
> + ? false : true;
> + /* no debug and no clear-on-read is a fast read */
> + reg->read_lite = reg->debug || ac->cor ? false : true;
> +}
> +
> static inline void register_write_memory(void *opaque, hwaddr addr,
> uint64_t value, unsigned size, bool be)
> {
> @@ -235,3 +257,15 @@ uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
> {
> return register_read_memory(opaque, addr, size, false);
> }
> +
> +static const TypeInfo register_info = {
> + .name = TYPE_REGISTER,
> + .parent = TYPE_DEVICE,
> +};
> +
> +static void register_register_types(void)
> +{
> + type_register_static(®ister_info);
> +}
> +
> +type_init(register_register_types)
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 38ee6f5..3316458 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;
> @@ -101,6 +102,8 @@ struct RegisterAccessInfo {
>
> struct RegisterInfo {
> /* <private> */
> + DeviceState parent_obj;
> +
> bool read_lite;
> bool write_lite;
>
> @@ -118,6 +121,9 @@ struct RegisterInfo {
> void *opaque;
> };
>
> +#define TYPE_REGISTER "qemu,register"
> +#define REGISTER(obj) OBJECT_CHECK(RegisterInfo, (obj), TYPE_REGISTER)
> +
> /**
> * write a value to a register, subject to its restrictions
> * @reg: register to write to
> @@ -143,6 +149,14 @@ uint64_t register_read(RegisterInfo *reg);
> void register_reset(RegisterInfo *reg);
>
> /**
> + * Initialize a register. GPIO's are setup as IOs to the specified device.
> + * Fast paths for eligible registers are enabled.
> + * @reg: Register to initialize
> + */
> +
> +void register_init(RegisterInfo *reg);
> +
> +/**
> * Memory API MMIO write handler that will write to a Register API register.
> * _be for big endian variant and _le for little endian.
> * @opaque: RegisterInfo to write to
--
Alex Bennée
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/16] register: Add Register API
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 02/16] register: Add Register API Alistair Francis
@ 2016-02-09 16:06 ` Alex Bennée
2016-02-09 19:35 ` Alistair Francis
0 siblings, 1 reply; 27+ messages in thread
From: Alex Bennée @ 2016-02-09 16:06 UTC (permalink / raw)
To: Alistair Francis
Cc: edgar.iglesias, peter.maydell, qemu-devel, crosthwaitepeter,
edgar.iglesias, afaerber, fred.konrad
Alistair Francis <alistair.francis@xilinx.com> writes:
> This API provides some encapsulation of registers and factors our 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 allow 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 throw guest errors when written certain values (ge0, ge1)
> Bits can throw unimp errors when written certain values (ui0, ui1)
> 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>
> ---
> V3:
> - Address some comments from Fred
>
> hw/core/Makefile.objs | 1 +
> hw/core/register.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
> 3 files changed, 322 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 abb3560..bf95db5 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -14,4 +14,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..f0fc39c
> --- /dev/null
> +++ b/hw/core/register.c
> @@ -0,0 +1,189 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2013 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.
> + */
> +
> +#include "hw/register.h"
> +#include "qemu/log.h"
> +
> +static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t val,
> + int mask, const char *msg,
> + const char *reason)
> +{
> + qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
> + reg->prefix, reg->access->name, val, msg, dir,
> + reason ? ": " : "", reason ? reason : "");
I'm not sure mask needs to be passed down as every use of it seems to
imply LOG_GUEST_USER. If you think this might change I would consider
passing the mask as the first option to align with other logging functions.
> +}
> +
> +static inline void register_write_val(RegisterInfo *reg, uint64_t val)
> +{
> + if (!reg->data) {
> + return;
> + }
Can you have a defined register without a data field? If it is invalid I
would use a g_assert() instead.
> + 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:
> + abort();
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:
> + abort();
> + }
> + return 0; /* unreachable */
> +}
> +
> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
> +{
> + uint64_t old_val, new_val, test, no_w_mask;
> + const RegisterAccessInfo *ac;
> + const RegisterAccessError *rae;
> +
> + assert(reg);
> +
> + ac = reg->access;
Should this assert(reg->access), how would you create a register without one?
> +
> + if (!ac || !ac->name) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> + "(written value: %#" PRIx64 ")\n", reg->prefix, val);
> + return;
> + }
> +
> + old_val = reg->data ? register_read_val(reg) : ac->reset;
> +
> + if (reg->write_lite && !~we) { /* fast path!! */
> + new_val = val;
> + goto register_write_fast;
> + }
What is the point of this fast path? It looks like it saves a few debug
log checks and some bitops, hardly performance critical considering
we've already left the TCG code at this point.
I'd be minded to leave it out and if you can actually measure a
difference add it in a future patch.
> +
> + no_w_mask = ac->ro | ac->w1c | ~we;
Push this calculation down to where the rest are done.
> +
> + if (reg->debug) {
> + qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, ac->name,
> + val);
> + }
If this is for debugging purposes I'd be tempted to move the print down
to the bottom after you've calculated the eventual result.
> +
> + if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
> + test = (old_val ^ val) & ac->rsvd;
> + if (test) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in reserved bit"
> + "fields: %#" PRIx64 ")\n", reg->prefix, test);
> + }
> + for (rae = ac->ge1; rae && rae->mask; rae++) {
> + test = val & rae->mask;
> + if (test) {
> + register_write_log(reg, 1, test, LOG_GUEST_ERROR,
> + "invalid", rae->reason);
> + }
> + }
I think the walking of the RegisterAcceessError entries could be pushed
into a helper function. See later comments on the structure:
> + for (rae = ac->ge0; rae && rae->mask; rae++) {
> + test = ~val & rae->mask;
> + if (test) {
> + register_write_log(reg, 0, test, LOG_GUEST_ERROR,
> + "invalid", rae->reason);
> + }
> + }
> + }
> +
> + if (qemu_loglevel_mask(LOG_UNIMP)) {
> + for (rae = ac->ui1; rae && rae->mask; rae++) {
> + test = val & rae->mask;
> + if (test) {
> + register_write_log(reg, 1, test, LOG_GUEST_ERROR,
> + "unimplmented", rae->reason);
LOG_UNIMP surely, these are bits we aren't modelling yet??
> + }
> + }
> + for (rae = ac->ui0; rae && rae->mask; rae++) {
> + test = ~val & rae->mask;
> + if (test) {
> + register_write_log(reg, 0, test, LOG_GUEST_ERROR,
> + "unimplemented", rae->reason);
LOG_UNIMP?
> + }
> + }
> + }
> +
> + 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);
> + }
> +
> +register_write_fast:
> + register_write_val(reg, new_val);
> + if (ac->post_write) {
> + ac->post_write(reg, new_val);
> + }
> +}
> +
> +uint64_t register_read(RegisterInfo *reg)
> +{
> + 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",
> + reg->prefix);
> + return 0;
> + }
Again I think these are assertions if you shouldn't be able to create
registers without access rules.
> +
> + ret = reg->data ? register_read_val(reg) : ac->reset;
> +
> + if (!reg->read_lite) {
> + register_write_val(reg, ret & ~ac->cor);
> + }
I'm a little fuzzy on what read_lite is meant to mean? Later we say:
/* no debug and no clear-on-read is a fast read */
reg->read_lite = reg->debug || ac->cor ? false : true;
Why not simply test ac->cor here and act accordingly. It seems a fuzzy indirection.
> +
> + if (ac->post_read) {
> + ret = ac->post_read(reg, ret);
> + }
> +
> + if (!reg->read_lite) {
> + if (reg->debug) {
> + qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefix,
> + ac->name, ret);
> + }
> + }
Same here, read_lite seems superfluous.
> +
> + return ret;
> +}
> +
> +void register_reset(RegisterInfo *reg)
> +{
> + assert(reg);
> +
> + if (!reg->data || !reg->access) {
> + return;
> + }
The same general assert comment.
> +
> + 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..a80427b
> --- /dev/null
> +++ b/include/hw/register.h
> @@ -0,0 +1,132 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2013 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;
> +
> +/**
> + * A register access error message
> + * @mask: Bits in the register the error applies to
> + * @reason: Reason why this access is an error
> + */
> +
> +typedef struct RegisterAccessError {
> + uint64_t mask;
> + const char *reason;
> +} RegisterAccessError;
> +
> +/**
> + * 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
> + *
> + * @ge1: Bits that when written 1 indicate a guest error
> + * @ge0: Bits that when written 0 indicate a guest error
> + * @ui1: Bits that when written 1 indicate use of an unimplemented feature
> + * @ui0: Bits that when written 0 indicate use of an unimplemented feature
> + *
> + * @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;
> +
> + const RegisterAccessError *ge0;
> + const RegisterAccessError *ge1;
> + const RegisterAccessError *ui0;
> + const RegisterAccessError *ui1;
Granted there is only one example of these being used in this patch
series but it seems excessive to waste 4 pointers that are NULL most of
the time. I think you could push the bit polarity and error flag into
the RegisterAccessError structure and only walk the list of bits once.
In fact I think you could make rsvd a part of this array as well for
common handling. Wouldn't ge1/ui1 be equivalent of writing to a RESVD
value anyway? In fact you want to model RES0 and RES1 types don't you?
> +
> + 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 {
> + /* <private> */
> + bool read_lite;
> + bool write_lite;
As mentioned above, what do these mean?
> +
> + /* <public> */
> + void *data;
> + int data_size;
> +
> + const RegisterAccessInfo *access;
> +
> + bool debug;
> + const char *prefix;
> +
> + 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
> + */
> +
> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we);
> +
> +/**
> + * read a value from a register, subject to its restrictions
> + * @reg: register to read from
> + * returns: value read
> + */
> +
> +uint64_t register_read(RegisterInfo *reg);
> +
> +/**
> + * reset a register
> + * @reg: register to reset
> + */
> +
> +void register_reset(RegisterInfo *reg);
> +
> +#endif
Thanks,
--
Alex Bennée
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper Alistair Francis
@ 2016-02-09 16:12 ` Alex Bennée
2016-02-09 19:50 ` Alistair Francis
0 siblings, 1 reply; 27+ messages in thread
From: Alex Bennée @ 2016-02-09 16:12 UTC (permalink / raw)
To: Alistair Francis
Cc: edgar.iglesias, peter.maydell, qemu-devel, crosthwaitepeter,
edgar.iglesias, afaerber, fred.konrad
Alistair Francis <alistair.francis@xilinx.com> writes:
> 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>
> ---
> V3:
> - Fix typo
> V2:
> - Use memory_region_add_subregion_no_print()
>
> hw/core/register.c | 29 +++++++++++++++++++++++++++++
> include/hw/register.h | 20 ++++++++++++++++++++
> 2 files changed, 49 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 939f398..4d7dd95 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -258,6 +258,35 @@ uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
> return register_read_memory(opaque, addr, size, false);
> }
>
> +void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
> + int num, RegisterInfo *ri, uint32_t *data,
> + MemoryRegion *container, const MemoryRegionOps *ops,
> + bool debug_enabled)
Are there going to be register_init_block8, 16 and 64 variants? Perhaps
this should be a generic register_block function that takes the size and
skip of the registers?
> +{
> + const char *debug_prefix = object_get_typename(OBJECT(owner));
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + int index = rae[i].decode.addr / 4;
> + RegisterInfo *r = &ri[index];
> +
> + *r = (RegisterInfo) {
> + .data = &data[index],
> + .data_size = sizeof(uint32_t),
> + .access = &rae[i],
> + .debug = debug_enabled,
> + .prefix = debug_prefix,
> + .opaque = owner,
> + };
> + register_init(r);
> +
> + memory_region_init_io(&r->mem, OBJECT(owner), ops, r, r->access->name,
> + sizeof(uint32_t));
> + memory_region_add_subregion_no_print(container,
> + r->access->decode.addr,
> &r->mem);
Why a memory region for every register? Couldn't we have a shared region
for the whole block and handle dispatching in the register code?
> + }
> +}
> +
> static const TypeInfo register_info = {
> .name = TYPE_REGISTER,
> .parent = TYPE_DEVICE,
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 3316458..30dedbf 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -182,6 +182,26 @@ void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
> uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
> uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
>
> +/**
> + * Init a block of consecutive 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
> + * @data: Array to use for register data
> + * @container: Memory region to contain new registers
> + * @ops: Memory region ops to access registers.
> + * @debug enabled: turn on/off verbose debug information
> + */
> +
> +void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
> + int num, RegisterInfo *ri, uint32_t *data,
> + MemoryRegion *container, const MemoryRegionOps *ops,
> + bool debug_enabled);
> +
> /* Define constants for a 32 bit register */
> #define REG32(reg, addr) \
> enum { A_ ## reg = (addr) }; \
--
Alex Bennée
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model Alistair Francis
@ 2016-02-09 17:08 ` Alex Bennée
2016-02-09 21:47 ` Alistair Francis
0 siblings, 1 reply; 27+ messages in thread
From: Alex Bennée @ 2016-02-09 17:08 UTC (permalink / raw)
To: Alistair Francis
Cc: edgar.iglesias, peter.maydell, qemu-devel, crosthwaitepeter,
edgar.iglesias, afaerber, fred.konrad
Alistair Francis <alistair.francis@xilinx.com> writes:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Minimal device model for devcfg module of Zynq. DMA capabilities and
> interrupt generation supported.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> default-configs/arm-softmmu.mak | 1 +
> hw/dma/Makefile.objs | 1 +
> hw/dma/xlnx-zynq-devcfg.c | 406 ++++++++++++++++++++++++++++++++++++++
> include/hw/dma/xlnx-zynq-devcfg.h | 62 ++++++
> 4 files changed, 470 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 d9b90a5..bc3914d 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -66,6 +66,7 @@ CONFIG_PXA2XX=y
> CONFIG_BITBANG_I2C=y
> CONFIG_FRAMEBUFFER=y
> CONFIG_XILINX_SPIPS=y
> +CONFIG_ZYNQ_DEVCFG=y
>
> CONFIG_ARM11SCU=y
> CONFIG_A9SCU=y
> diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
> index 0e65ed0..eaf0a81 100644
> --- a/hw/dma/Makefile.objs
> +++ b/hw/dma/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_PL330) += pl330.o
> common-obj-$(CONFIG_I82374) += i82374.o
> common-obj-$(CONFIG_I8257) += i8257.o
> common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
> +common-obj-$(CONFIG_ZYNQ_DEVCFG) += xlnx-zynq-devcfg.o
> common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
> common-obj-$(CONFIG_STP2000) += sparc32_dma.o
> common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
> diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
> new file mode 100644
> index 0000000..747d830
> --- /dev/null
> +++ b/hw/dma/xlnx-zynq-devcfg.c
> @@ -0,0 +1,406 @@
> +/*
> + * 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 "hw/dma/xlnx-zynq-devcfg.h"
> +#include "qemu/bitops.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/dma.h"
> +
> +#define FREQ_HZ 900000000
> +
> +#define BTT_MAX 0x400
> +
> +#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
> +#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT(...) do { \
> + if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
> + qemu_log("%s: ", __func__); \
> + qemu_log(__VA_ARGS__); \
> + } \
> +} while (0);
What's wrong with:
#define DB_PRINT(fmt, ...) do { \
if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
qemu_log("%s: " fmt , __func__, ## __VA_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_RO 0xFFFFF000
> +#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)
I'm going to hold judgement on the register macros until I've inwardly
digested the macro magic. My only general point is I have no idea where
the memory backing of these registers is now.
> +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)
> +{
> + for (;;) {
> + uint8_t buf[BTT_MAX];
> + XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
> + uint32_t btt = BTT_MAX;
> + bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
> +
> + btt = MIN(btt, dmah->src_len);
> + if (loopback) {
> + btt = MIN(btt, dmah->dest_len);
> + }
> + DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
> + dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
> + dmah->src_len -= btt;
> + dmah->src_addr += btt;
> + if (loopback) {
> + DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
> + dma_memory_write(&address_space_memory, dmah->dest_addr, buf, btt);
> + dmah->dest_len -= btt;
> + dmah->dest_addr += btt;
> + }
> + if (!dmah->src_len && !dmah->dest_len) {
> + DB_PRINT("dma operation finished\n");
> + s->regs[R_INT_STS] |= R_INT_STS_DMA_DONE_MASK |
> + R_INT_STS_DMA_P_DONE_MASK;
> + s->dma_cmd_fifo_num--;
> + memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
> + sizeof(*s->dma_cmd_fifo) * XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN
> + - 1);
> + }
> + xlnx_zynq_devcfg_update_ixr(s);
> + if (!s->dma_cmd_fifo_num) { /* All done */
> + return;
> + }
> + }
Would a do { ... } while (s->dma_cmd_fifo_num) be cleaner than the for;; construct?
> +}
> +
> +static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + xlnx_zynq_devcfg_update_ixr(s);
> +}
> +
> +static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
> + if (s->regs[R_LOCK] & 1 << i) {
> + val &= ~lock_ctrl_map[i];
> + val |= lock_ctrl_map[i] & s->regs[R_CTRL];
> + }
> + }
> + return val;
> +}
> +
> +static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + uint32_t aes_en = F_EX32(val, CTRL, PCFG_AES_EN);
> +
> + if (aes_en != 0 && aes_en != 7) {
> + qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
> + "unimplemented security reset should happen!\n",
> + reg->prefix);
> + }
> +}
> +
> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + if (val == R_UNLOCK_MAGIC) {
> + DB_PRINT("successful unlock\n");
> + /* BootROM will have already done the actual unlock so no need to do
> + * anything in successful subsequent unlock
> + */
> + } else { /* bad unlock attempt */
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", reg->prefix);
> + s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
> + s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
> + /* core becomes inaccessible */
> + memory_region_set_enabled(&s->iomem, false);
> + }
I think it would be cleaner if the part that actually did something went
first, i.e:
if (val != R_UNLOCK_MAGIC) {
.. do fail stuff ..
} else {
...debug print...
}
> +}
> +
> +static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + /* once bits are locked they stay locked */
> + return s->regs[R_LOCK] | val;
> +}
> +
> +static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
> + .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
> + .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
> + .src_len = s->regs[R_DMA_SRC_LEN] << 2,
> + .dest_len = s->regs[R_DMA_DST_LEN] << 2,
> + };
> + s->dma_cmd_fifo_num++;
> + DB_PRINT("dma transfer started; %d total transfers pending\n",
> + s->dma_cmd_fifo_num);
> + xlnx_zynq_devcfg_dma_go(s);
> +}
> +
> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
> + { .name = "CTRL", .decode.addr = A_CTRL,
> + .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
> + .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
> + .ui1 = (RegisterAccessError[]) {
> + { .mask = R_CTRL_FORCE_RST_MASK,
> + .reason = "PS reset not implemented" },
> + { .mask = R_CTRL_PCAP_MODE_MASK,
> + .reason = "FPGA Fabric doesn't exist" },
> + { .mask = R_CTRL_PCFG_AES_EN_MASK,
> + .reason = "AES not implemented" },
> + {},
> + },
> + .pre_write = r_ctrl_pre_write,
> + .post_write = r_ctrl_post_write,
> + },
> + { .name = "LOCK", .decode.addr = A_LOCK,
This is the bit I don't like about the macro magic, perhaps a helper
macro would make it easier for someone grepping the code:
.decode.addr = REG32_ADDR(LOCK);
?
> + .rsvd = ~ONES(5),
> + .pre_write = r_lock_pre_write,
> + },
> + { .name = "CFG", .decode.addr = A_CFG,
> + .reset = 1 << R_CFG_RFIFO_TH_SHIFT | 1 << R_CFG_WFIFO_TH_SHIFT | 0x8,
> + .rsvd = 0xfffff00f,
> + },
> + { .name = "INT_STS", .decode.addr = A_INT_STS,
> + .w1c = ~R_INT_STS_RSVD,
> + .reset = R_INT_STS_PSS_GTS_USR_B_MASK |
> + R_INT_STS_PSS_CFG_RESET_B_MASK |
> + R_INT_STS_WR_FIFO_LVL_MASK,
> + .rsvd = R_INT_STS_RSVD,
> + .post_write = r_ixr_post_write,
> + },
> + { .name = "INT_MASK", .decode.addr = A_INT_MASK,
> + .reset = ~0,
> + .rsvd = R_INT_STS_RSVD,
> + .post_write = r_ixr_post_write,
> + },
> + { .name = "STATUS", .decode.addr = A_STATUS,
> + .reset = R_STATUS_DMA_CMD_Q_E_MASK |
> + R_STATUS_PSS_GTS_USR_B_MASK |
> + R_STATUS_PSS_CFG_RESET_B_MASK,
> + .ro = ~0,
> + },
> + { .name = "DMA_SRC_ADDR", .decode.addr = A_DMA_SRC_ADDR, },
> + { .name = "DMA_DST_ADDR", .decode.addr = A_DMA_DST_ADDR, },
> + { .name = "DMA_SRC_LEN", .decode.addr = A_DMA_SRC_LEN,
> + .ro = ~ONES(27) },
> + { .name = "DMA_DST_LEN", .decode.addr = A_DMA_DST_LEN,
> + .ro = ~ONES(27),
> + .post_write = r_dma_dst_len_post_write,
> + },
> + { .name = "ROM_SHADOW", .decode.addr = A_ROM_SHADOW,
> + .rsvd = ~0ull,
> + },
> + { .name = "SW_ID", .decode.addr = A_SW_ID, },
> + { .name = "UNLOCK", .decode.addr = A_UNLOCK,
> + .post_write = r_unlock_post_write,
> + },
> + { .name = "MCTRL", .decode.addr = R_MCTRL * 4,
> + /* Silicon 3.0 for version field, the mysterious reserved bit 23
> + * and QEMU platform identifier.
> + */
> + .reset = 0x2 << R_MCTRL_PS_VERSION_SHIFT | 1 << 23 | R_MCTRL_QEMU_MASK,
> + .ro = ~R_MCTRL_INT_PCAP_LPBK_MASK,
> + .rsvd = 0x00f00303,
> + },
> +};
> +
> +static const MemoryRegionOps xlnx_zynq_devcfg_reg_ops = {
> + .read = register_read_memory_le,
> + .write = register_write_memory_le,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + }
> +};
> +
> +static const VMStateDescription vmstate_xlnx_zynq_devcfg_dma_cmd = {
> + .name = "xlnx_zynq_devcfg_dma_cmd",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .minimum_version_id_old = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(src_addr, XlnxZynqDevcfgDMACmd),
> + VMSTATE_UINT32(dest_addr, XlnxZynqDevcfgDMACmd),
> + VMSTATE_UINT32(src_len, XlnxZynqDevcfgDMACmd),
> + VMSTATE_UINT32(dest_len, XlnxZynqDevcfgDMACmd),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
> + .name = "xlnx_zynq_devcfg",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .minimum_version_id_old = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
> + XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
> + vmstate_xlnx_zynq_devcfg_dma_cmd,
> + XlnxZynqDevcfgDMACmd),
> + VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
> + VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void xlnx_zynq_devcfg_init(Object *obj)
> +{
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(obj);
> +
> + sysbus_init_irq(sbd, &s->irq);
> +
> + memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
> + register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
> + ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
> + s->regs_info, s->regs, &s->iomem,
> + &xlnx_zynq_devcfg_reg_ops,
> + XLNX_ZYNQ_DEVCFG_ERR_DEBUG);
> + sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static void xlnx_zynq_devcfg_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->reset = xlnx_zynq_devcfg_reset;
> + dc->vmsd = &vmstate_xlnx_zynq_devcfg;
> +}
> +
> +static const TypeInfo xlnx_zynq_devcfg_info = {
> + .name = TYPE_XLNX_ZYNQ_DEVCFG,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(XlnxZynqDevcfg),
> + .instance_init = xlnx_zynq_devcfg_init,
> + .class_init = xlnx_zynq_devcfg_class_init,
> +};
> +
> +static void xlnx_zynq_devcfg_register_types(void)
> +{
> + type_register_static(&xlnx_zynq_devcfg_info);
> +}
> +
> +type_init(xlnx_zynq_devcfg_register_types)
> diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
> new file mode 100644
> index 0000000..d40e5c8
> --- /dev/null
> +++ b/include/hw/dma/xlnx-zynq-devcfg.h
> @@ -0,0 +1,62 @@
> +/*
> + * QEMU model of the Xilinx Devcfg Interface
> + *
> + * (C) 2011 PetaLogix Pty Ltd
> + * (C) 2014 Xilinx Inc.
> + * Written by Peter Crosthwaite <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
--
Alex Bennée
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/16] data-driven device registers
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
` (14 preceding siblings ...)
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 15/16] misc: Introduce ZynqMP IOU SLCR Alistair Francis
@ 2016-02-09 17:22 ` Alex Bennée
2016-02-09 21:56 ` Alistair Francis
15 siblings, 1 reply; 27+ messages in thread
From: Alex Bennée @ 2016-02-09 17:22 UTC (permalink / raw)
To: Alistair Francis
Cc: edgar.iglesias, peter.maydell, qemu-devel, crosthwaitepeter,
edgar.iglesias, afaerber, fred.konrad
Alistair Francis <alistair.francis@xilinx.com> writes:
> This patch series is based on Peter C's original register API. His
> original cover letter is below.
OK that's my first pass review. I seem to be missing 16/16 in my inbox
though.
My initial thoughts are it seems pretty useful from a validation point
of view. I'm a little uncomfortable with so much of the validation being
hidden behind debug statements though. I wonder if they should always be
checked but only optionally printed. Otherwise stuff is liable to
bit-rot until you turn on debug.
Also I'm not super crazy about the macro stuff. I can see what you are
trying to get to but I wonder if there is a neater way of defining this.
Obviously with C being what it is it could well be it is the least ugly
solution.
The case for this may be improved if in addition to new devices and
existing device could be converted to the data driven style. That would
give a nicer old-style <-> new-style comparison.
Once you've had a chance to go through the comments and fix up the
compile please CC me on future patches and I'll give the tyres a runtime
kicking ;-)
Cheers,
--
Alex Bennée
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/16] register: QOMify
2016-02-09 11:49 ` Alex Bennée
@ 2016-02-09 18:09 ` Alistair Francis
0 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-02-09 18:09 UTC (permalink / raw)
To: Alex Bennée
Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers,
Alistair Francis, Peter Crosthwaite, Edgar Iglesias,
Andreas Färber, KONRAD Frédéric
On Tue, Feb 9, 2016 at 3:49 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> 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 as well as setup
>> the r/w fast paths.
>
> I don't know if there has been bit-rot since posting but this currently
> doesn't build:
>
> CC hw/core/register.o
> hw/core/register.c:394:1: error: return type defaults to ‘int’ [-Werror=return-type]
> type_init(register_register_types)
> ^
> hw/core/register.c:394:1: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
> hw/core/register.c: In function ‘type_init’:
> hw/core/register.c:394:1: error: old-style function definition [-Werror=old-style-definition]
> hw/core/register.c:394:1: error: expected ‘{’ at end of input
> hw/core/register.c: At top level:
> hw/core/register.c:389:13: error: ‘register_register_types’ defined but not used [-Werror=unused-function]
> static void register_register_types(void)
> ^
> hw/core/register.c: In function ‘type_init’:
> hw/core/register.c:394:1: error: control reaches end of non-void function [-Werror=return-type]
> type_init(register_register_types)
> ^
> cc1: all warnings being treated as errors
> make: *** [hw/core/register.o] Error 1
> make: Target `all' not remade because of errors.
Yeah there appears to have been some changes with include files.
Thanks for pointing it out, fixed in V4.
Thanks,
Alistair
>
>
>>
>> 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>
>> ---
>>
>> hw/core/register.c | 34 ++++++++++++++++++++++++++++++++++
>> include/hw/register.h | 14 ++++++++++++++
>> 2 files changed, 48 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index e696c43..939f398 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -188,6 +188,28 @@ void register_reset(RegisterInfo *reg)
>> register_write_val(reg, reg->access->reset);
>> }
>>
>> +void register_init(RegisterInfo *reg)
>> +{
>> + assert(reg);
>> + const RegisterAccessInfo *ac;
>> +
>> + if (!reg->data || !reg->access) {
>> + return;
>> + }
>> +
>> + object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
>> +
>> + ac = reg->access;
>> +
>> + /* if there are no debug msgs and no RMW requirement, mark for fast write */
>> + reg->write_lite = reg->debug || ac->ro || ac->w1c || ac->pre_write ||
>> + ((ac->ge0 || ac->ge1) && qemu_loglevel_mask(LOG_GUEST_ERROR)) ||
>> + ((ac->ui0 || ac->ui1) && qemu_loglevel_mask(LOG_UNIMP))
>> + ? false : true;
>> + /* no debug and no clear-on-read is a fast read */
>> + reg->read_lite = reg->debug || ac->cor ? false : true;
>> +}
>> +
>> static inline void register_write_memory(void *opaque, hwaddr addr,
>> uint64_t value, unsigned size, bool be)
>> {
>> @@ -235,3 +257,15 @@ uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
>> {
>> return register_read_memory(opaque, addr, size, false);
>> }
>> +
>> +static const TypeInfo register_info = {
>> + .name = TYPE_REGISTER,
>> + .parent = TYPE_DEVICE,
>> +};
>> +
>> +static void register_register_types(void)
>> +{
>> + type_register_static(®ister_info);
>> +}
>> +
>> +type_init(register_register_types)
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 38ee6f5..3316458 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;
>> @@ -101,6 +102,8 @@ struct RegisterAccessInfo {
>>
>> struct RegisterInfo {
>> /* <private> */
>> + DeviceState parent_obj;
>> +
>> bool read_lite;
>> bool write_lite;
>>
>> @@ -118,6 +121,9 @@ struct RegisterInfo {
>> void *opaque;
>> };
>>
>> +#define TYPE_REGISTER "qemu,register"
>> +#define REGISTER(obj) OBJECT_CHECK(RegisterInfo, (obj), TYPE_REGISTER)
>> +
>> /**
>> * write a value to a register, subject to its restrictions
>> * @reg: register to write to
>> @@ -143,6 +149,14 @@ uint64_t register_read(RegisterInfo *reg);
>> void register_reset(RegisterInfo *reg);
>>
>> /**
>> + * Initialize a register. GPIO's are setup as IOs to the specified device.
>> + * Fast paths for eligible registers are enabled.
>> + * @reg: Register to initialize
>> + */
>> +
>> +void register_init(RegisterInfo *reg);
>> +
>> +/**
>> * Memory API MMIO write handler that will write to a Register API register.
>> * _be for big endian variant and _le for little endian.
>> * @opaque: RegisterInfo to write to
>
>
> --
> Alex Bennée
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/16] register: Add Register API
2016-02-09 16:06 ` Alex Bennée
@ 2016-02-09 19:35 ` Alistair Francis
2016-02-09 22:29 ` Peter Crosthwaite
0 siblings, 1 reply; 27+ messages in thread
From: Alistair Francis @ 2016-02-09 19:35 UTC (permalink / raw)
To: Alex Bennée
Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers,
Alistair Francis, Peter Crosthwaite, Edgar Iglesias,
Andreas Färber, KONRAD Frédéric
On Tue, Feb 9, 2016 at 8:06 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> This API provides some encapsulation of registers and factors our 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 allow 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 throw guest errors when written certain values (ge0, ge1)
>> Bits can throw unimp errors when written certain values (ui0, ui1)
>> 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>
>> ---
>> V3:
>> - Address some comments from Fred
>>
>> hw/core/Makefile.objs | 1 +
>> hw/core/register.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 322 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 abb3560..bf95db5 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -14,4 +14,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..f0fc39c
>> --- /dev/null
>> +++ b/hw/core/register.c
>> @@ -0,0 +1,189 @@
>> +/*
>> + * Register Definition API
>> + *
>> + * Copyright (c) 2013 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.
>> + */
>> +
>> +#include "hw/register.h"
>> +#include "qemu/log.h"
>> +
>> +static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t val,
>> + int mask, const char *msg,
>> + const char *reason)
>> +{
>> + qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
>> + reg->prefix, reg->access->name, val, msg, dir,
>> + reason ? ": " : "", reason ? reason : "");
>
> I'm not sure mask needs to be passed down as every use of it seems to
> imply LOG_GUEST_USER. If you think this might change I would consider
> passing the mask as the first option to align with other logging functions.
Removing this function as it is no longer required.
>
>> +}
>> +
>> +static inline void register_write_val(RegisterInfo *reg, uint64_t val)
>> +{
>> + if (!reg->data) {
>> + return;
>> + }
>
> Can you have a defined register without a data field? If it is invalid I
> would use a g_assert() instead.
Good point, I can't see any cases where there wouldn't be a data field.
>
>> + 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:
>> + abort();
>
> g_assert_not_reached();
Fixed
>
>> + }
>> +}
>> +
>> +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:
>> + abort();
>> + }
>> + return 0; /* unreachable */
>> +}
>> +
>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
>> +{
>> + uint64_t old_val, new_val, test, no_w_mask;
>> + const RegisterAccessInfo *ac;
>> + const RegisterAccessError *rae;
>> +
>> + assert(reg);
>> +
>> + ac = reg->access;
>
> Should this assert(reg->access), how would you create a register without one?
An assert seems a little harsh here. It seems reachable if the guest
writes at the end of the register list or if you didn't bother
implementing all of the registers. I think this gives more details
then an assert would.
>> +
>> + if (!ac || !ac->name) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
>> + "(written value: %#" PRIx64 ")\n", reg->prefix, val);
>> + return;
>> + }
>> +
>> + old_val = reg->data ? register_read_val(reg) : ac->reset;
>> +
>> + if (reg->write_lite && !~we) { /* fast path!! */
>> + new_val = val;
>> + goto register_write_fast;
>> + }
>
> What is the point of this fast path? It looks like it saves a few debug
> log checks and some bitops, hardly performance critical considering
> we've already left the TCG code at this point.
>
> I'd be minded to leave it out and if you can actually measure a
> difference add it in a future patch.
Peter would know more about this then me, I'm not sure if it sees a
huge speed increase.
I'll take it out now, then when we start adding more complex devices
(the ones with this patch set are pretty simple) we can add it back in
if there is a slowdown. Same with read lite
>
>> +
>> + no_w_mask = ac->ro | ac->w1c | ~we;
>
> Push this calculation down to where the rest are done.
Fixed
>
>> +
>> + if (reg->debug) {
>> + qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, ac->name,
>> + val);
>> + }
>
> If this is for debugging purposes I'd be tempted to move the print down
> to the bottom after you've calculated the eventual result.
Fixed
>
>> +
>> + if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
>> + test = (old_val ^ val) & ac->rsvd;
>> + if (test) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in reserved bit"
>> + "fields: %#" PRIx64 ")\n", reg->prefix, test);
>> + }
>> + for (rae = ac->ge1; rae && rae->mask; rae++) {
>> + test = val & rae->mask;
>> + if (test) {
>> + register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> + "invalid", rae->reason);
>> + }
>> + }
>
> I think the walking of the RegisterAcceessError entries could be pushed
> into a helper function. See later comments on the structure:
>
>> + for (rae = ac->ge0; rae && rae->mask; rae++) {
>> + test = ~val & rae->mask;
>> + if (test) {
>> + register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> + "invalid", rae->reason);
>> + }
>> + }
>> + }
>> +
>> + if (qemu_loglevel_mask(LOG_UNIMP)) {
>> + for (rae = ac->ui1; rae && rae->mask; rae++) {
>> + test = val & rae->mask;
>> + if (test) {
>> + register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> + "unimplmented", rae->reason);
>
> LOG_UNIMP surely, these are bits we aren't modelling yet??
Fixed
>
>> + }
>> + }
>> + for (rae = ac->ui0; rae && rae->mask; rae++) {
>> + test = ~val & rae->mask;
>> + if (test) {
>> + register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> + "unimplemented", rae->reason);
>
> LOG_UNIMP?
>
>> + }
>> + }
>> + }
>> +
>> + 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);
>> + }
>> +
>> +register_write_fast:
>> + register_write_val(reg, new_val);
>> + if (ac->post_write) {
>> + ac->post_write(reg, new_val);
>> + }
>> +}
>> +
>> +uint64_t register_read(RegisterInfo *reg)
>> +{
>> + 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",
>> + reg->prefix);
>> + return 0;
>> + }
>
> Again I think these are assertions if you shouldn't be able to create
> registers without access rules.
See above
>
>> +
>> + ret = reg->data ? register_read_val(reg) : ac->reset;
>> +
>> + if (!reg->read_lite) {
>> + register_write_val(reg, ret & ~ac->cor);
>> + }
>
> I'm a little fuzzy on what read_lite is meant to mean? Later we say:
>
> /* no debug and no clear-on-read is a fast read */
> reg->read_lite = reg->debug || ac->cor ? false : true;
>
> Why not simply test ac->cor here and act accordingly. It seems a fuzzy indirection.
I have removed read_lite
>
>> +
>> + if (ac->post_read) {
>> + ret = ac->post_read(reg, ret);
>> + }
>> +
>> + if (!reg->read_lite) {
>> + if (reg->debug) {
>> + qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefix,
>> + ac->name, ret);
>> + }
>> + }
>
> Same here, read_lite seems superfluous.
>
>> +
>> + return ret;
>> +}
>> +
>> +void register_reset(RegisterInfo *reg)
>> +{
>> + assert(reg);
>> +
>> + if (!reg->data || !reg->access) {
>> + return;
>> + }
>
> The same general assert comment.
Although I agree it should be an assert here as the QEMU model calls this.
>
>> +
>> + 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..a80427b
>> --- /dev/null
>> +++ b/include/hw/register.h
>> @@ -0,0 +1,132 @@
>> +/*
>> + * Register Definition API
>> + *
>> + * Copyright (c) 2013 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;
>> +
>> +/**
>> + * A register access error message
>> + * @mask: Bits in the register the error applies to
>> + * @reason: Reason why this access is an error
>> + */
>> +
>> +typedef struct RegisterAccessError {
>> + uint64_t mask;
>> + const char *reason;
>> +} RegisterAccessError;
>> +
>> +/**
>> + * 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
>> + *
>> + * @ge1: Bits that when written 1 indicate a guest error
>> + * @ge0: Bits that when written 0 indicate a guest error
>> + * @ui1: Bits that when written 1 indicate use of an unimplemented feature
>> + * @ui0: Bits that when written 0 indicate use of an unimplemented feature
>> + *
>> + * @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;
>> +
>> + const RegisterAccessError *ge0;
>> + const RegisterAccessError *ge1;
>> + const RegisterAccessError *ui0;
>> + const RegisterAccessError *ui1;
>
> Granted there is only one example of these being used in this patch
> series but it seems excessive to waste 4 pointers that are NULL most of
> the time. I think you could push the bit polarity and error flag into
> the RegisterAccessError structure and only walk the list of bits once.
>
> In fact I think you could make rsvd a part of this array as well for
> common handling. Wouldn't ge1/ui1 be equivalent of writing to a RESVD
> value anyway? In fact you want to model RES0 and RES1 types don't you?
Agreed! I have removed the guest error mask and updated write code accordingly.
I have also simplified the unimplemented masking. It does mean that
the reason can't be specified, but it is a lot neater and I'm not sure
how often specific reasons will be used.
Thanks,
Alistair
>
>> +
>> + 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 {
>> + /* <private> */
>> + bool read_lite;
>> + bool write_lite;
>
> As mentioned above, what do these mean?
>
>> +
>> + /* <public> */
>> + void *data;
>> + int data_size;
>> +
>> + const RegisterAccessInfo *access;
>> +
>> + bool debug;
>> + const char *prefix;
>> +
>> + 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
>> + */
>> +
>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we);
>> +
>> +/**
>> + * read a value from a register, subject to its restrictions
>> + * @reg: register to read from
>> + * returns: value read
>> + */
>> +
>> +uint64_t register_read(RegisterInfo *reg);
>> +
>> +/**
>> + * reset a register
>> + * @reg: register to reset
>> + */
>> +
>> +void register_reset(RegisterInfo *reg);
>> +
>> +#endif
>
> Thanks,
>
> --
> Alex Bennée
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper
2016-02-09 16:12 ` Alex Bennée
@ 2016-02-09 19:50 ` Alistair Francis
0 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-02-09 19:50 UTC (permalink / raw)
To: Alex Bennée
Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers,
Alistair Francis, Peter Crosthwaite, Edgar Iglesias,
Andreas Färber, KONRAD Frédéric
On Tue, Feb 9, 2016 at 8:12 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> 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>
>> ---
>> V3:
>> - Fix typo
>> V2:
>> - Use memory_region_add_subregion_no_print()
>>
>> hw/core/register.c | 29 +++++++++++++++++++++++++++++
>> include/hw/register.h | 20 ++++++++++++++++++++
>> 2 files changed, 49 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index 939f398..4d7dd95 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -258,6 +258,35 @@ uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
>> return register_read_memory(opaque, addr, size, false);
>> }
>>
>> +void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
>> + int num, RegisterInfo *ri, uint32_t *data,
>> + MemoryRegion *container, const MemoryRegionOps *ops,
>> + bool debug_enabled)
>
> Are there going to be register_init_block8, 16 and 64 variants? Perhaps
> this should be a generic register_block function that takes the size and
> skip of the registers?
I think at some point there will be benefits in supporting different
size registers.
What do you mean size and skip?
>
>> +{
>> + const char *debug_prefix = object_get_typename(OBJECT(owner));
>> + int i;
>> +
>> + for (i = 0; i < num; i++) {
>> + int index = rae[i].decode.addr / 4;
>> + RegisterInfo *r = &ri[index];
>> +
>> + *r = (RegisterInfo) {
>> + .data = &data[index],
>> + .data_size = sizeof(uint32_t),
>> + .access = &rae[i],
>> + .debug = debug_enabled,
>> + .prefix = debug_prefix,
>> + .opaque = owner,
>> + };
>> + register_init(r);
>> +
>> + memory_region_init_io(&r->mem, OBJECT(owner), ops, r, r->access->name,
>> + sizeof(uint32_t));
>> + memory_region_add_subregion_no_print(container,
>> + r->access->decode.addr,
>> &r->mem);
>
> Why a memory region for every register? Couldn't we have a shared region
> for the whole block and handle dispatching in the register code?
This is something else that Peter would know better. Looking at it I
don't see any reason that it couldn't be an array or RegisterInfo and
use that with one memory region. It would make the read/write logic
more complex though.
I would want a consensus to do it that way before I re-write it though.
Thanks,
Alistair
>
>> + }
>> +}
>> +
>> static const TypeInfo register_info = {
>> .name = TYPE_REGISTER,
>> .parent = TYPE_DEVICE,
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 3316458..30dedbf 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -182,6 +182,26 @@ void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
>> uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
>> uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
>>
>> +/**
>> + * Init a block of consecutive 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
>> + * @data: Array to use for register data
>> + * @container: Memory region to contain new registers
>> + * @ops: Memory region ops to access registers.
>> + * @debug enabled: turn on/off verbose debug information
>> + */
>> +
>> +void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
>> + int num, RegisterInfo *ri, uint32_t *data,
>> + MemoryRegion *container, const MemoryRegionOps *ops,
>> + bool debug_enabled);
>> +
>> /* Define constants for a 32 bit register */
>> #define REG32(reg, addr) \
>> enum { A_ ## reg = (addr) }; \
>
>
> --
> Alex Bennée
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model
2016-02-09 17:08 ` Alex Bennée
@ 2016-02-09 21:47 ` Alistair Francis
0 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-02-09 21:47 UTC (permalink / raw)
To: Alex Bennée
Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers,
Alistair Francis, Peter Crosthwaite, Edgar Iglesias,
Andreas Färber, KONRAD Frédéric
On Tue, Feb 9, 2016 at 9:08 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Minimal device model for devcfg module of Zynq. DMA capabilities and
>> interrupt generation supported.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> default-configs/arm-softmmu.mak | 1 +
>> hw/dma/Makefile.objs | 1 +
>> hw/dma/xlnx-zynq-devcfg.c | 406 ++++++++++++++++++++++++++++++++++++++
>> include/hw/dma/xlnx-zynq-devcfg.h | 62 ++++++
>> 4 files changed, 470 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 d9b90a5..bc3914d 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -66,6 +66,7 @@ CONFIG_PXA2XX=y
>> CONFIG_BITBANG_I2C=y
>> CONFIG_FRAMEBUFFER=y
>> CONFIG_XILINX_SPIPS=y
>> +CONFIG_ZYNQ_DEVCFG=y
>>
>> CONFIG_ARM11SCU=y
>> CONFIG_A9SCU=y
>> diff --git a/hw/dma/Makefile.objs b/hw/dma/Makefile.objs
>> index 0e65ed0..eaf0a81 100644
>> --- a/hw/dma/Makefile.objs
>> +++ b/hw/dma/Makefile.objs
>> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_PL330) += pl330.o
>> common-obj-$(CONFIG_I82374) += i82374.o
>> common-obj-$(CONFIG_I8257) += i8257.o
>> common-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
>> +common-obj-$(CONFIG_ZYNQ_DEVCFG) += xlnx-zynq-devcfg.o
>> common-obj-$(CONFIG_ETRAXFS) += etraxfs_dma.o
>> common-obj-$(CONFIG_STP2000) += sparc32_dma.o
>> common-obj-$(CONFIG_SUN4M) += sun4m_iommu.o
>> diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
>> new file mode 100644
>> index 0000000..747d830
>> --- /dev/null
>> +++ b/hw/dma/xlnx-zynq-devcfg.c
>> @@ -0,0 +1,406 @@
>> +/*
>> + * 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 "hw/dma/xlnx-zynq-devcfg.h"
>> +#include "qemu/bitops.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/dma.h"
>> +
>> +#define FREQ_HZ 900000000
>> +
>> +#define BTT_MAX 0x400
>> +
>> +#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
>> +#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
>> +#endif
>> +
>> +#define DB_PRINT(...) do { \
>> + if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
>> + qemu_log("%s: ", __func__); \
>> + qemu_log(__VA_ARGS__); \
>> + } \
>> +} while (0);
>
> What's wrong with:
>
> #define DB_PRINT(fmt, ...) do { \
> if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
> qemu_log("%s: " fmt , __func__, ## __VA_ARGS__); \
> } \
> } while (0)
Fixed
>
>> +
>> +REG32(CTRL, 0x00)
>> + FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr ignored */
>> + FIELD(CTRL, PCAP_PR, 27, 1) /* Forced to 0 on bad unlock */
>> + FIELD(CTRL, PCAP_MODE, 26, 1)
>> + FIELD(CTRL, MULTIBOOT_EN, 24, 1)
>> + FIELD(CTRL, USER_MODE, 15, 1)
>> + FIELD(CTRL, PCFG_AES_FUSE, 12, 1)
>> + FIELD(CTRL, PCFG_AES_EN, 9, 3)
>> + FIELD(CTRL, SEU_EN, 8, 1)
>> + FIELD(CTRL, SEC_EN, 7, 1)
>> + FIELD(CTRL, SPNIDEN, 6, 1)
>> + FIELD(CTRL, SPIDEN, 5, 1)
>> + FIELD(CTRL, NIDEN, 4, 1)
>> + FIELD(CTRL, DBGEN, 3, 1)
>> + FIELD(CTRL, DAP_EN, 0, 3)
>> +
>> +REG32(LOCK, 0x04)
>> + #define AES_FUSE_LOCK 4
>> + #define AES_EN_LOCK 3
>> + #define SEU_LOCK 2
>> + #define SEC_LOCK 1
>> + #define DBG_LOCK 0
>> +
>> +/* mapping bits in R_LOCK to what they lock in R_CTRL */
>> +static const uint32_t lock_ctrl_map[] = {
>> + [AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
>> + [AES_EN_LOCK] = R_CTRL_PCFG_AES_EN_MASK,
>> + [SEU_LOCK] = R_CTRL_SEU_EN_MASK,
>> + [SEC_LOCK] = R_CTRL_SEC_EN_MASK,
>> + [DBG_LOCK] = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
>> + R_CTRL_NIDEN_MASK | R_CTRL_DBGEN_MASK |
>> + R_CTRL_DAP_EN_MASK,
>> +};
>> +
>> +REG32(CFG, 0x08)
>> + FIELD(CFG, RFIFO_TH, 10, 2)
>> + FIELD(CFG, WFIFO_TH, 8, 2)
>> + FIELD(CFG, RCLK_EDGE, 7, 1)
>> + FIELD(CFG, WCLK_EDGE, 6, 1)
>> + FIELD(CFG, DISABLE_SRC_INC, 5, 1)
>> + FIELD(CFG, DISABLE_DST_INC, 4, 1)
>> +#define R_CFG_RO 0xFFFFF000
>> +#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)
>
> I'm going to hold judgement on the register macros until I've inwardly
> digested the macro magic. My only general point is I have no idea where
> the memory backing of these registers is now.
>
>> +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)
>> +{
>> + for (;;) {
>> + uint8_t buf[BTT_MAX];
>> + XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
>> + uint32_t btt = BTT_MAX;
>> + bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
>> +
>> + btt = MIN(btt, dmah->src_len);
>> + if (loopback) {
>> + btt = MIN(btt, dmah->dest_len);
>> + }
>> + DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
>> + dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
>> + dmah->src_len -= btt;
>> + dmah->src_addr += btt;
>> + if (loopback) {
>> + DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
>> + dma_memory_write(&address_space_memory, dmah->dest_addr, buf, btt);
>> + dmah->dest_len -= btt;
>> + dmah->dest_addr += btt;
>> + }
>> + if (!dmah->src_len && !dmah->dest_len) {
>> + DB_PRINT("dma operation finished\n");
>> + s->regs[R_INT_STS] |= R_INT_STS_DMA_DONE_MASK |
>> + R_INT_STS_DMA_P_DONE_MASK;
>> + s->dma_cmd_fifo_num--;
>> + memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
>> + sizeof(*s->dma_cmd_fifo) * XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN
>> + - 1);
>> + }
>> + xlnx_zynq_devcfg_update_ixr(s);
>> + if (!s->dma_cmd_fifo_num) { /* All done */
>> + return;
>> + }
>> + }
>
> Would a do { ... } while (s->dma_cmd_fifo_num) be cleaner than the for;; construct?
Fixed
>
>> +}
>> +
>> +static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + xlnx_zynq_devcfg_update_ixr(s);
>> +}
>> +
>> +static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
>> + if (s->regs[R_LOCK] & 1 << i) {
>> + val &= ~lock_ctrl_map[i];
>> + val |= lock_ctrl_map[i] & s->regs[R_CTRL];
>> + }
>> + }
>> + return val;
>> +}
>> +
>> +static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + uint32_t aes_en = F_EX32(val, CTRL, PCFG_AES_EN);
>> +
>> + if (aes_en != 0 && aes_en != 7) {
>> + qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
>> + "unimplemented security reset should happen!\n",
>> + reg->prefix);
>> + }
>> +}
>> +
>> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + if (val == R_UNLOCK_MAGIC) {
>> + DB_PRINT("successful unlock\n");
>> + /* BootROM will have already done the actual unlock so no need to do
>> + * anything in successful subsequent unlock
>> + */
>> + } else { /* bad unlock attempt */
>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", reg->prefix);
>> + s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
>> + s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
>> + /* core becomes inaccessible */
>> + memory_region_set_enabled(&s->iomem, false);
>> + }
>
> I think it would be cleaner if the part that actually did something went
> first, i.e:
>
> if (val != R_UNLOCK_MAGIC) {
> .. do fail stuff ..
> } else {
> ...debug print...
> }
>
I don't really mind either way, but generally the way it is normally
done if checking the true value first.
>
>
>> +}
>> +
>> +static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + /* once bits are locked they stay locked */
>> + return s->regs[R_LOCK] | val;
>> +}
>> +
>> +static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
>> + .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
>> + .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
>> + .src_len = s->regs[R_DMA_SRC_LEN] << 2,
>> + .dest_len = s->regs[R_DMA_DST_LEN] << 2,
>> + };
>> + s->dma_cmd_fifo_num++;
>> + DB_PRINT("dma transfer started; %d total transfers pending\n",
>> + s->dma_cmd_fifo_num);
>> + xlnx_zynq_devcfg_dma_go(s);
>> +}
>> +
>> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
>> + { .name = "CTRL", .decode.addr = A_CTRL,
>> + .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
>> + .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
>> + .ui1 = (RegisterAccessError[]) {
>> + { .mask = R_CTRL_FORCE_RST_MASK,
>> + .reason = "PS reset not implemented" },
>> + { .mask = R_CTRL_PCAP_MODE_MASK,
>> + .reason = "FPGA Fabric doesn't exist" },
>> + { .mask = R_CTRL_PCFG_AES_EN_MASK,
>> + .reason = "AES not implemented" },
>> + {},
>> + },
>> + .pre_write = r_ctrl_pre_write,
>> + .post_write = r_ctrl_post_write,
>> + },
>> + { .name = "LOCK", .decode.addr = A_LOCK,
>
> This is the bit I don't like about the macro magic, perhaps a helper
> macro would make it easier for someone grepping the code:
>
> .decode.addr = REG32_ADDR(LOCK);
I see what you mean, but would that help with grepping? If you grep
for LOCK you will still find this register.
Thanks,
Alistair
>
> ?
>
>> + .rsvd = ~ONES(5),
>> + .pre_write = r_lock_pre_write,
>> + },
>> + { .name = "CFG", .decode.addr = A_CFG,
>> + .reset = 1 << R_CFG_RFIFO_TH_SHIFT | 1 << R_CFG_WFIFO_TH_SHIFT | 0x8,
>> + .rsvd = 0xfffff00f,
>> + },
>> + { .name = "INT_STS", .decode.addr = A_INT_STS,
>> + .w1c = ~R_INT_STS_RSVD,
>> + .reset = R_INT_STS_PSS_GTS_USR_B_MASK |
>> + R_INT_STS_PSS_CFG_RESET_B_MASK |
>> + R_INT_STS_WR_FIFO_LVL_MASK,
>> + .rsvd = R_INT_STS_RSVD,
>> + .post_write = r_ixr_post_write,
>> + },
>> + { .name = "INT_MASK", .decode.addr = A_INT_MASK,
>> + .reset = ~0,
>> + .rsvd = R_INT_STS_RSVD,
>> + .post_write = r_ixr_post_write,
>> + },
>> + { .name = "STATUS", .decode.addr = A_STATUS,
>> + .reset = R_STATUS_DMA_CMD_Q_E_MASK |
>> + R_STATUS_PSS_GTS_USR_B_MASK |
>> + R_STATUS_PSS_CFG_RESET_B_MASK,
>> + .ro = ~0,
>> + },
>> + { .name = "DMA_SRC_ADDR", .decode.addr = A_DMA_SRC_ADDR, },
>> + { .name = "DMA_DST_ADDR", .decode.addr = A_DMA_DST_ADDR, },
>> + { .name = "DMA_SRC_LEN", .decode.addr = A_DMA_SRC_LEN,
>> + .ro = ~ONES(27) },
>> + { .name = "DMA_DST_LEN", .decode.addr = A_DMA_DST_LEN,
>> + .ro = ~ONES(27),
>> + .post_write = r_dma_dst_len_post_write,
>> + },
>> + { .name = "ROM_SHADOW", .decode.addr = A_ROM_SHADOW,
>> + .rsvd = ~0ull,
>> + },
>> + { .name = "SW_ID", .decode.addr = A_SW_ID, },
>> + { .name = "UNLOCK", .decode.addr = A_UNLOCK,
>> + .post_write = r_unlock_post_write,
>> + },
>> + { .name = "MCTRL", .decode.addr = R_MCTRL * 4,
>> + /* Silicon 3.0 for version field, the mysterious reserved bit 23
>> + * and QEMU platform identifier.
>> + */
>> + .reset = 0x2 << R_MCTRL_PS_VERSION_SHIFT | 1 << 23 | R_MCTRL_QEMU_MASK,
>> + .ro = ~R_MCTRL_INT_PCAP_LPBK_MASK,
>> + .rsvd = 0x00f00303,
>> + },
>> +};
>> +
>> +static const MemoryRegionOps xlnx_zynq_devcfg_reg_ops = {
>> + .read = register_read_memory_le,
>> + .write = register_write_memory_le,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> + .valid = {
>> + .min_access_size = 4,
>> + .max_access_size = 4,
>> + }
>> +};
>> +
>> +static const VMStateDescription vmstate_xlnx_zynq_devcfg_dma_cmd = {
>> + .name = "xlnx_zynq_devcfg_dma_cmd",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .minimum_version_id_old = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(src_addr, XlnxZynqDevcfgDMACmd),
>> + VMSTATE_UINT32(dest_addr, XlnxZynqDevcfgDMACmd),
>> + VMSTATE_UINT32(src_len, XlnxZynqDevcfgDMACmd),
>> + VMSTATE_UINT32(dest_len, XlnxZynqDevcfgDMACmd),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
>> + .name = "xlnx_zynq_devcfg",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .minimum_version_id_old = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
>> + XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
>> + vmstate_xlnx_zynq_devcfg_dma_cmd,
>> + XlnxZynqDevcfgDMACmd),
>> + VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
>> + VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static void xlnx_zynq_devcfg_init(Object *obj)
>> +{
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(obj);
>> +
>> + sysbus_init_irq(sbd, &s->irq);
>> +
>> + memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
>> + register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
>> + ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
>> + s->regs_info, s->regs, &s->iomem,
>> + &xlnx_zynq_devcfg_reg_ops,
>> + XLNX_ZYNQ_DEVCFG_ERR_DEBUG);
>> + sysbus_init_mmio(sbd, &s->iomem);
>> +}
>> +
>> +static void xlnx_zynq_devcfg_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->reset = xlnx_zynq_devcfg_reset;
>> + dc->vmsd = &vmstate_xlnx_zynq_devcfg;
>> +}
>> +
>> +static const TypeInfo xlnx_zynq_devcfg_info = {
>> + .name = TYPE_XLNX_ZYNQ_DEVCFG,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(XlnxZynqDevcfg),
>> + .instance_init = xlnx_zynq_devcfg_init,
>> + .class_init = xlnx_zynq_devcfg_class_init,
>> +};
>> +
>> +static void xlnx_zynq_devcfg_register_types(void)
>> +{
>> + type_register_static(&xlnx_zynq_devcfg_info);
>> +}
>> +
>> +type_init(xlnx_zynq_devcfg_register_types)
>> diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
>> new file mode 100644
>> index 0000000..d40e5c8
>> --- /dev/null
>> +++ b/include/hw/dma/xlnx-zynq-devcfg.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + * QEMU model of the Xilinx Devcfg Interface
>> + *
>> + * (C) 2011 PetaLogix Pty Ltd
>> + * (C) 2014 Xilinx Inc.
>> + * Written by Peter Crosthwaite <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
>
>
> --
> Alex Bennée
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/16] data-driven device registers
2016-02-09 17:22 ` [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alex Bennée
@ 2016-02-09 21:56 ` Alistair Francis
0 siblings, 0 replies; 27+ messages in thread
From: Alistair Francis @ 2016-02-09 21:56 UTC (permalink / raw)
To: Alex Bennée
Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers,
Alistair Francis, Peter Crosthwaite, Edgar Iglesias,
Andreas Färber, KONRAD Frédéric
On Tue, Feb 9, 2016 at 9:22 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> This patch series is based on Peter C's original register API. His
>> original cover letter is below.
>
> OK that's my first pass review. I seem to be missing 16/16 in my inbox
> though.
Thanks for going through it. I really appreciate it. I have problems
with all of my emails going through, you should get it this time.
>
> My initial thoughts are it seems pretty useful from a validation point
> of view. I'm a little uncomfortable with so much of the validation being
> hidden behind debug statements though. I wonder if they should always be
> checked but only optionally printed. Otherwise stuff is liable to
> bit-rot until you turn on debug.
I have addressed some of those in V4, let me know if you see any other
problem areas.
>
> Also I'm not super crazy about the macro stuff. I can see what you are
> trying to get to but I wonder if there is a neater way of defining this.
> Obviously with C being what it is it could well be it is the least ugly
> solution.
I see what you mean and the macro magic does take some getting used
to, but I don't have a better solution. If you do have one I can do it
that way instead.
>
> The case for this may be improved if in addition to new devices and
> existing device could be converted to the data driven style. That would
> give a nicer old-style <-> new-style comparison.
>
> Once you've had a chance to go through the comments and fix up the
> compile please CC me on future patches and I'll give the tyres a runtime
> kicking ;-)
Sending out V4 now, doesn't do us any good having something that
doesn't compile on list.
Thanks,
Alistair
>
> Cheers,
>
> --
> Alex Bennée
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/16] register: Add Register API
2016-02-09 19:35 ` Alistair Francis
@ 2016-02-09 22:29 ` Peter Crosthwaite
0 siblings, 0 replies; 27+ messages in thread
From: Peter Crosthwaite @ 2016-02-09 22:29 UTC (permalink / raw)
To: Alistair Francis
Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers,
Edgar Iglesias, Alex Bennée, Andreas Färber,
KONRAD Frédéric
On Tue, Feb 9, 2016 at 11:35 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Tue, Feb 9, 2016 at 8:06 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> This API provides some encapsulation of registers and factors our 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 allow 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 throw guest errors when written certain values (ge0, ge1)
>>> Bits can throw unimp errors when written certain values (ui0, ui1)
>>> 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>
>>> ---
>>> V3:
>>> - Address some comments from Fred
>>>
>>> hw/core/Makefile.objs | 1 +
>>> hw/core/register.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
>>> 3 files changed, 322 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 abb3560..bf95db5 100644
>>> --- a/hw/core/Makefile.objs
>>> +++ b/hw/core/Makefile.objs
>>> @@ -14,4 +14,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..f0fc39c
>>> --- /dev/null
>>> +++ b/hw/core/register.c
>>> @@ -0,0 +1,189 @@
>>> +/*
>>> + * Register Definition API
>>> + *
>>> + * Copyright (c) 2013 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.
>>> + */
>>> +
>>> +#include "hw/register.h"
>>> +#include "qemu/log.h"
>>> +
>>> +static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t val,
>>> + int mask, const char *msg,
>>> + const char *reason)
>>> +{
>>> + qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
>>> + reg->prefix, reg->access->name, val, msg, dir,
>>> + reason ? ": " : "", reason ? reason : "");
>>
>> I'm not sure mask needs to be passed down as every use of it seems to
>> imply LOG_GUEST_USER. If you think this might change I would consider
>> passing the mask as the first option to align with other logging functions.
>
> Removing this function as it is no longer required.
>
>>
>>> +}
>>> +
>>> +static inline void register_write_val(RegisterInfo *reg, uint64_t val)
>>> +{
>>> + if (!reg->data) {
>>> + return;
>>> + }
>>
>> Can you have a defined register without a data field? If it is invalid I
>> would use a g_assert() instead.
>
> Good point, I can't see any cases where there wouldn't be a data field.
>
>From the register API point of view it should be valid. It just so
happens when use the array based registration it never comes up. So I
think assert is not quite right even though the path is dead as of
this series.
Regards,
Peter
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-02-09 22:29 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-30 1:00 [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 01/16] memory: Allow subregions to not be printed by info mtree Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 02/16] register: Add Register API Alistair Francis
2016-02-09 16:06 ` Alex Bennée
2016-02-09 19:35 ` Alistair Francis
2016-02-09 22:29 ` Peter Crosthwaite
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 03/16] register: Add Memory API glue Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 04/16] register: Add support for decoding information Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 05/16] register: Define REG and FIELD macros Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 06/16] register: QOMify Alistair Francis
2016-02-09 11:49 ` Alex Bennée
2016-02-09 18:09 ` Alistair Francis
2016-01-30 1:00 ` [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper Alistair Francis
2016-02-09 16:12 ` Alex Bennée
2016-02-09 19:50 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 08/16] bitops: Add ONES macro Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 09/16] dma: Add Xilinx Zynq devcfg device model Alistair Francis
2016-02-09 17:08 ` Alex Bennée
2016-02-09 21:47 ` Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 10/16] xilinx_zynq: add devcfg to machine model Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 11/16] qdev: Define qdev_get_gpio_out Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 12/16] qdev: Add qdev_pass_all_gpios API Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 13/16] irq: Add opaque setter routine Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 14/16] register: Add GPIO API Alistair Francis
2016-01-30 1:01 ` [Qemu-devel] [PATCH v3 15/16] misc: Introduce ZynqMP IOU SLCR Alistair Francis
2016-02-09 17:22 ` [Qemu-devel] [PATCH v3 00/16] data-driven device registers Alex Bennée
2016-02-09 21:56 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).