* [Qemu-devel] [PATCH v6 00/13] data-driven device registers
@ 2016-05-12 22:45 Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro Alistair Francis
` (12 more replies)
0 siblings, 13 replies; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:45 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
This patch series is based on Peter C's original register API. His
original cover letter is below.
Future work: Allow support for memory attributes.
V6:
- Small changes to the API based on Alex's comments
- Remove 'register: Add support for decoding information' patch
- Move prefix and debug into the RegisterInfoArray as it is the same
for every register.
V5:
- Only create a single memory region instead of a memory region for
each register
- General tidyups based on Alex's comments
V4:
- Rebase and fix build issue
- Simplify the register write logic
- Other small fixes suggested by Alex Bennee
V3:
- Small changes reported by Fred
V2:
- Rebase
- Fix up IOU SLCR connections
- Add the memory_region_add_subregion_no_print() function and use it
for the registers
Changes since RFC:
- Connect the ZynqMP IOU SLCR device
- Rebase
Original cover letter From Peter:
Hi All. This is a new scheme I've come up with handling device registers in a
data driven way. My motivation for this is to factor out a lot of the access
checking that seems to be replicated in every device. See P1 commit message for
further discussion.
P1 is the main patch, adds the register definition functionality
P2-3,6 add helpers that glue the register API to the Memory API
P4 Defines a set of macros that minimise register and field definitions
P5 is QOMfication
P7 is a trivial
P10-13 Work up to GPIO support
P8,9,14 add new devices (the Xilinx Zynq devcfg & ZynqMP SLCR) that use this
scheme.
P15: Connect the ZynqMP SLCR device
This Zynq devcfg device was particularly finnicky with per-bit restrictions.
I'm also looking for a higher-than-usual modelling fidelity
on the register space, with semantics defined for random reserved bits
in-between otherwise consistent fields.
Here's an example of the qemu_log output for the devcfg device. This is produced
by now generic sharable code:
/machine/unattached/device[44]:Addr 0x000008:CFG: write of value 00000508
/machine/unattached/device[44]:Addr 0x000080:MCTRL: write of value 00800010
/machine/unattached/device[44]:Addr 0x000010:INT_MASK: write of value ffffffff
/machine/unattached/device[44]:Addr 00000000:CTRL: write of value 0c00607f
And an example of a rogue guest banging on a bad bit:
/machine/unattached/device[44]:Addr 0x000014:STATUS bits 0x000001 may not be \
written to 1
A future feature I am interested in is implementing TCG optimisation of
side-effectless registers. The register API allows clear definition of
what registers have txn side effects and which ones don't. You could even
go a step further and translate such side-effectless accesses based on the
data pointer for the register.
Alistair Francis (6):
bitops: Add MAKE_64BIT_MASK macro
register: Add Register API
register: Add Memory API glue
dma: Add Xilinx Zynq devcfg device model
register: Add GPIO API
xlnx-zynqmp: Connect the ZynqMP IOU SLCR
Peter Crosthwaite (7):
register: Define REG and FIELD macros
register: QOMify
register: Add block initialise helper
xilinx_zynq: Connect devcfg to the Zynq machine model
qdev: Define qdev_get_gpio_out
irq: Add opaque setter routine
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 | 12 +
hw/core/register.c | 376 +++++++++++++++++++++++++++++++
hw/dma/Makefile.objs | 1 +
hw/dma/xlnx-zynq-devcfg.c | 396 +++++++++++++++++++++++++++++++++
hw/misc/Makefile.objs | 1 +
hw/misc/xlnx-zynqmp-iou-slcr.c | 115 ++++++++++
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 | 2 +
include/hw/register.h | 262 ++++++++++++++++++++++
include/qemu/bitops.h | 3 +
18 files changed, 1309 insertions(+)
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.7.4
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
@ 2016-05-12 22:45 ` Alistair Francis
2016-06-09 18:46 ` Peter Maydell
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 02/13] register: Add Register API Alistair Francis
` (11 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:45 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
Add a macro that creates a 64bit value which has length number of ones
shifted acrros by the value of shift.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
V5:
- Re-write to a 64-bit mask instead of ONES()
- Re-order this patch in the series
include/qemu/bitops.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 755fdd1..3c45791 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -24,6 +24,9 @@
#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
+#define MAKE_64BIT_MASK(shift, length) \
+ (((1ull << (length)) - 1) << shift)
+
/**
* set_bit - Set a bit in memory
* @nr: the bit to set
--
2.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 02/13] register: Add Register API
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro Alistair Francis
@ 2016-05-12 22:45 ` Alistair Francis
2016-06-09 18:55 ` Peter Maydell
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue Alistair Francis
` (10 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:45 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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 be marked clear on read (cor)
Pre and post action callbacks can be added to read and write ops
Verbose debugging info can be enabled/disabled
Useful for defining device register spaces in a data driven way. Cuts
down on a lot of the verbosity and repetition in the switch-case blocks
in the standard foo_mmio_read/write functions.
Also useful for automated generation of device models from hardware
design sources.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
V5:
- Convert to using only one memory region
V4:
- Rebase
- Remove the guest error masking
- Simplify the unimplemented masking
- Use the reserved value in the write calculations
- Remove read_lite and write_lite
- General fixes to asserts and log printing
V3:
- Address some comments from Fred
hw/core/Makefile.objs | 1 +
hw/core/register.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/register.h | 110 +++++++++++++++++++++++++++++++++++++
3 files changed, 260 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..5e6f621
--- /dev/null
+++ b/hw/core/register.c
@@ -0,0 +1,149 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/register.h"
+#include "hw/qdev.h"
+#include "qemu/log.h"
+
+static inline void register_write_val(RegisterInfo *reg, uint64_t val)
+{
+ g_assert(reg->data);
+
+ switch (reg->data_size) {
+ case 1:
+ *(uint8_t *)reg->data = val;
+ break;
+ case 2:
+ *(uint16_t *)reg->data = val;
+ break;
+ case 4:
+ *(uint32_t *)reg->data = val;
+ break;
+ case 8:
+ *(uint64_t *)reg->data = val;
+ break;
+ default:
+ g_assert_not_reached();
+ }
+}
+
+static inline uint64_t register_read_val(RegisterInfo *reg)
+{
+ switch (reg->data_size) {
+ case 1:
+ return *(uint8_t *)reg->data;
+ case 2:
+ return *(uint16_t *)reg->data;
+ case 4:
+ return *(uint32_t *)reg->data;
+ case 8:
+ return *(uint64_t *)reg->data;
+ default:
+ g_assert_not_reached();
+ }
+ return 0; /* unreachable */
+}
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
+ const char* prefix, bool debug)
+{
+ uint64_t old_val, new_val, test, no_w_mask;
+ const RegisterAccessInfo *ac;
+
+ assert(reg);
+
+ ac = reg->access;
+
+ if (!ac || !ac->name) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
+ "(written value: %#" PRIx64 ")\n", prefix, val);
+ return;
+ }
+
+ old_val = reg->data ? register_read_val(reg) : ac->reset;
+
+ test = (old_val ^ val) & ac->rsvd;
+ if (test) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in reserved bit"
+ "fields: %#" PRIx64 ")\n", prefix, test);
+ }
+
+ test = val & ac->unimp;
+ if (test) {
+ qemu_log_mask(LOG_UNIMP,
+ "%s:%s writing %#" PRIx64 " to unimplemented bits:" \
+ " %#" PRIx64 "",
+ prefix, reg->access->name, val, ac->unimp);
+ }
+
+ /* Create the no write mask based on the read only, write to clear and
+ * reserved bit masks.
+ */
+ no_w_mask = ac->ro | ac->w1c | ac->rsvd | ~we;
+ new_val = (val & ~no_w_mask) | (old_val & no_w_mask);
+ new_val &= ~(val & ac->w1c);
+
+ if (ac->pre_write) {
+ new_val = ac->pre_write(reg, new_val);
+ }
+
+ if (debug) {
+ qemu_log("%s:%s: write of value %#" PRIx64 "\n", prefix, ac->name,
+ new_val);
+ }
+
+ register_write_val(reg, new_val);
+
+ if (ac->post_write) {
+ ac->post_write(reg, new_val);
+ }
+}
+
+uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug)
+{
+ uint64_t ret;
+ const RegisterAccessInfo *ac;
+
+ assert(reg);
+
+ ac = reg->access;
+ if (!ac || !ac->name) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state\n",
+ prefix);
+ return 0;
+ }
+
+ ret = reg->data ? register_read_val(reg) : ac->reset;
+
+ register_write_val(reg, ret & ~ac->cor);
+
+ if (ac->post_read) {
+ ret = ac->post_read(reg, ret);
+ }
+
+ if (debug) {
+ qemu_log("%s:%s: read of value %#" PRIx64 "\n", prefix,
+ ac->name, ret);
+ }
+
+ return ret;
+}
+
+void register_reset(RegisterInfo *reg)
+{
+ g_assert(reg);
+
+ if (!reg->data || !reg->access) {
+ return;
+ }
+
+ register_write_val(reg, reg->access->reset);
+}
diff --git a/include/hw/register.h b/include/hw/register.h
new file mode 100644
index 0000000..07d0616
--- /dev/null
+++ b/include/hw/register.h
@@ -0,0 +1,110 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef REGISTER_H
+#define REGISTER_H
+
+#include "exec/memory.h"
+
+typedef struct RegisterInfo RegisterInfo;
+typedef struct RegisterAccessInfo RegisterAccessInfo;
+
+/**
+ * Access description for a register that is part of guest accessible device
+ * state.
+ *
+ * @name: String name of the register
+ * @ro: whether or not the bit is read-only
+ * @w1c: bits with the common write 1 to clear semantic.
+ * @reset: reset value.
+ * @cor: Bits that are clear on read
+ * @rsvd: Bits that are reserved and should not be changed
+ *
+ * @pre_write: Pre write callback. Passed the value that's to be written,
+ * immediately before the actual write. The returned value is what is written,
+ * giving the handler a chance to modify the written value.
+ * @post_write: Post write callback. Passed the written value. Most write side
+ * effects should be implemented here.
+ *
+ * @post_read: Post read callback. Passes the value that is about to be returned
+ * for a read. The return value from this function is what is ultimately read,
+ * allowing this function to modify the value before return to the client.
+ */
+
+struct RegisterAccessInfo {
+ const char *name;
+ uint64_t ro;
+ uint64_t w1c;
+ uint64_t reset;
+ uint64_t cor;
+ uint64_t rsvd;
+ uint64_t unimp;
+
+ uint64_t (*pre_write)(RegisterInfo *reg, uint64_t val);
+ void (*post_write)(RegisterInfo *reg, uint64_t val);
+
+ uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+};
+
+/**
+ * A register that is part of guest accessible state
+ * @data: pointer to the register data. Will be cast
+ * to the relevant uint type depending on data_size.
+ * @data_size: Size of the register in bytes. Must be
+ * 1, 2, 4 or 8
+ *
+ * @access: Access description of this register
+ *
+ * @debug: Whether or not verbose debug is enabled
+ * @prefix: String prefix for log and debug messages
+ *
+ * @opaque: Opaque data for the register
+ */
+
+struct RegisterInfo {
+ /* <public> */
+ void *data;
+ int data_size;
+
+ const RegisterAccessInfo *access;
+
+ void *opaque;
+};
+
+/**
+ * write a value to a register, subject to its restrictions
+ * @reg: register to write to
+ * @val: value to write
+ * @we: write enable mask
+ * @prefix: The device prefix that should be printed before the register name
+ * @debug: Should the write operation debug information be printed?
+ */
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
+ const char* prefix, bool debug);
+
+/**
+ * read a value from a register, subject to its restrictions
+ * @reg: register to read from
+ * @prefix: The device prefix that should be printed before the register name
+ * @debug: Should the read operation debug information be printed?
+ * returns: value read
+ */
+
+uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug);
+
+/**
+ * reset a register
+ * @reg: register to reset
+ */
+
+void register_reset(RegisterInfo *reg);
+
+#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 02/13] register: Add Register API Alistair Francis
@ 2016-05-12 22:45 ` Alistair Francis
2016-06-09 13:08 ` KONRAD Frederic
2016-06-09 19:03 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros Alistair Francis
` (9 subsequent siblings)
12 siblings, 2 replies; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:45 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
Add memory io handlers that glue the register API to the memory API.
Just translation functions at this stage. Although it does allow for
devices to be created without all-in-one mmio r/w handlers.
This patch also adds the RegisterInfoArray struct, which allows all of
the individual RegisterInfo structs to be grouped into a single memory
region.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V6:
- Add the memory region later
V5:
- Convert to using only one memory region
hw/core/register.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/register.h | 50 +++++++++++++++++++++++++++++++++++
2 files changed, 122 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index 5e6f621..25196e6 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -147,3 +147,75 @@ 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)
+{
+ RegisterInfoArray *reg_array = opaque;
+ RegisterInfo *reg = NULL;
+ uint64_t we = ~0;
+ int i, shift = 0;
+
+ for (i = 0; i < reg_array->num_elements; i++) {
+ if (reg_array->r[i]->access->decode.addr == addr) {
+ reg = reg_array->r[i];
+ break;
+ }
+ }
+ assert(reg);
+
+ /* Generate appropriate write enable mask and shift values */
+ if (reg->data_size < size) {
+ we = MAKE_64BIT_MASK(0, reg->data_size * 8);
+ shift = 8 * (be ? reg->data_size - size : 0);
+ } else if (reg->data_size >= size) {
+ we = MAKE_64BIT_MASK(0, size * 8);
+ }
+
+ register_write(reg, value << shift, we << shift, reg_array->prefix,
+ reg_array->debug);
+}
+
+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)
+{
+ RegisterInfoArray *reg_array = opaque;
+ RegisterInfo *reg = NULL;
+ int i, shift;
+
+ for (i = 0; i < reg_array->num_elements; i++) {
+ if (reg_array->r[i]->access->decode.addr == addr) {
+ reg = reg_array->r[i];
+ break;
+ }
+ }
+ assert(reg);
+
+ shift = 8 * (be ? reg->data_size - size : 0);
+
+ return (register_read(reg, reg_array->prefix, reg_array->debug) >> shift) &
+ MAKE_64BIT_MASK(0, size * 8);
+}
+
+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 07d0616..786707b 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -15,6 +15,7 @@
typedef struct RegisterInfo RegisterInfo;
typedef struct RegisterAccessInfo RegisterAccessInfo;
+typedef struct RegisterInfoArray RegisterInfoArray;
/**
* Access description for a register that is part of guest accessible device
@@ -51,6 +52,10 @@ struct RegisterAccessInfo {
void (*post_write)(RegisterInfo *reg, uint64_t val);
uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+
+ struct {
+ hwaddr addr;
+ } decode;
};
/**
@@ -79,6 +84,25 @@ struct RegisterInfo {
};
/**
+ * This structure is used to group all of the individual registers which are
+ * modeled using the RegisterInfo strucutre.
+ *
+ * @r is an aray containing of all the relevent RegisterInfo structures.
+ *
+ * @num_elements is the number of elements in the array r
+ *
+ * @mem: optional Memory region for the register
+ */
+
+struct RegisterInfoArray {
+ int num_elements;
+ RegisterInfo **r;
+
+ bool debug;
+ const char *prefix;
+};
+
+/**
* write a value to a register, subject to its restrictions
* @reg: register to write to
* @val: value to write
@@ -107,4 +131,30 @@ uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug);
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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (2 preceding siblings ...)
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 10:52 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 05/13] register: QOMify Alistair Francis
` (8 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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 786707b..e0aac91 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -157,4 +157,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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 05/13] register: QOMify
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (3 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 10:55 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper Alistair Francis
` (7 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
QOMify registers as a child of TYPE_DEVICE. This allows registers to
define GPIOs.
Define an init helper that will do QOM initialisation.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
V5:
- Convert to using only one memory region
hw/core/register.c | 23 +++++++++++++++++++++++
include/hw/register.h | 15 +++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index 25196e6..c5a2c78 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -148,6 +148,17 @@ void register_reset(RegisterInfo *reg)
register_write_val(reg, reg->access->reset);
}
+void register_init(RegisterInfo *reg)
+{
+ assert(reg);
+
+ if (!reg->data || !reg->access) {
+ return;
+ }
+
+ object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
+}
+
static inline void register_write_memory(void *opaque, hwaddr addr,
uint64_t value, unsigned size, bool be)
{
@@ -219,3 +230,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 e0aac91..eedd578 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;
@@ -74,6 +75,9 @@ struct RegisterAccessInfo {
*/
struct RegisterInfo {
+ /* <private> */
+ DeviceState parent_obj;
+
/* <public> */
void *data;
int data_size;
@@ -83,6 +87,9 @@ struct RegisterInfo {
void *opaque;
};
+#define TYPE_REGISTER "qemu,register"
+#define REGISTER(obj) OBJECT_CHECK(RegisterInfo, (obj), TYPE_REGISTER)
+
/**
* This structure is used to group all of the individual registers which are
* modeled using the RegisterInfo strucutre.
@@ -132,6 +139,14 @@ uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug);
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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (4 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 05/13] register: QOMify Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 11:02 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model Alistair Francis
` (6 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Add a helper that will scan a static RegisterAccessInfo Array
and populate a container MemoryRegion with registers as defined.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
The reason that I'm not using GArray is because the array needs to store
the memory region that covers all of the registers.
V6:
- Fixup the loop logic
V5:
- Convert to only using one memory region
V3:
- Fix typo
V2:
- Use memory_region_add_subregion_no_print()
hw/core/register.c | 36 ++++++++++++++++++++++++++++++++++++
include/hw/register.h | 22 ++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index c5a2c78..c68d510 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -231,6 +231,42 @@ 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, uint64_t memory_size)
+{
+ const char *device_prefix = object_get_typename(OBJECT(owner));
+ RegisterInfoArray *r_array = g_malloc(sizeof(RegisterInfoArray));
+ int i;
+
+ r_array->r = g_malloc_n(num, sizeof(RegisterInfo *));
+ r_array->num_elements = num;
+ r_array->debug = debug_enabled;
+ r_array->prefix = device_prefix;
+
+ for (i = 0; i < num; i++) {
+ int index = rae[i].decode.addr / 4;
+ RegisterInfo *r = &ri[index];
+
+ *r = (RegisterInfo) {
+ .data = &data[index],
+ .data_size = sizeof(uint32_t),
+ .access = &rae[i],
+ .opaque = owner,
+ };
+ register_init(r);
+
+ r_array->r[i] = r;
+ }
+
+ memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
+ device_prefix, memory_size);
+ memory_region_add_subregion(container,
+ r_array->r[0]->access->decode.addr,
+ &r_array->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 eedd578..c40cf03 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -102,6 +102,8 @@ struct RegisterInfo {
*/
struct RegisterInfoArray {
+ MemoryRegion mem;
+
int num_elements;
RegisterInfo **r;
@@ -172,6 +174,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, uint64_t memory_size);
+
/* Define constants for a 32 bit register */
#define REG32(reg, addr) \
enum { A_ ## reg = (addr) }; \
--
2.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (5 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 11:16 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model Alistair Francis
` (5 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
Add a minimal model for the devcfg device which is part of Zynq.
This model supports DMA capabilities and interrupt generation.
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V5:
- Corrections to the device model logic
default-configs/arm-softmmu.mak | 1 +
hw/dma/Makefile.objs | 1 +
hw/dma/xlnx-zynq-devcfg.c | 396 ++++++++++++++++++++++++++++++++++++++
include/hw/dma/xlnx-zynq-devcfg.h | 62 ++++++
4 files changed, 460 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 c63cdd0..40f94ec 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 a1abbcf..29b4520 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..9de4e75
--- /dev/null
+++ b/hw/dma/xlnx-zynq-devcfg.c
@@ -0,0 +1,396 @@
+/*
+ * QEMU model of the Xilinx Zynq Devcfg Interface
+ *
+ * (C) 2011 PetaLogix Pty Ltd
+ * (C) 2014 Xilinx Inc.
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/dma/xlnx-zynq-devcfg.h"
+#include "qemu/bitops.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
+
+#define FREQ_HZ 900000000
+
+#define BTT_MAX 0x400
+
+#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
+#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
+#endif
+
+#define DB_PRINT(fmt, args...) do { \
+ if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
+ qemu_log("%s: " fmt, __func__, ## args); \
+ } \
+} while (0);
+
+REG32(CTRL, 0x00)
+ FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr ignored */
+ FIELD(CTRL, PCAP_PR, 27, 1) /* Forced to 0 on bad unlock */
+ FIELD(CTRL, PCAP_MODE, 26, 1)
+ FIELD(CTRL, MULTIBOOT_EN, 24, 1)
+ FIELD(CTRL, USER_MODE, 15, 1)
+ FIELD(CTRL, PCFG_AES_FUSE, 12, 1)
+ FIELD(CTRL, PCFG_AES_EN, 9, 3)
+ FIELD(CTRL, SEU_EN, 8, 1)
+ FIELD(CTRL, SEC_EN, 7, 1)
+ FIELD(CTRL, SPNIDEN, 6, 1)
+ FIELD(CTRL, SPIDEN, 5, 1)
+ FIELD(CTRL, NIDEN, 4, 1)
+ FIELD(CTRL, DBGEN, 3, 1)
+ FIELD(CTRL, DAP_EN, 0, 3)
+
+REG32(LOCK, 0x04)
+ #define AES_FUSE_LOCK 4
+ #define AES_EN_LOCK 3
+ #define SEU_LOCK 2
+ #define SEC_LOCK 1
+ #define DBG_LOCK 0
+
+/* mapping bits in R_LOCK to what they lock in R_CTRL */
+static const uint32_t lock_ctrl_map[] = {
+ [AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
+ [AES_EN_LOCK] = R_CTRL_PCFG_AES_EN_MASK,
+ [SEU_LOCK] = R_CTRL_SEU_EN_MASK,
+ [SEC_LOCK] = R_CTRL_SEC_EN_MASK,
+ [DBG_LOCK] = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
+ R_CTRL_NIDEN_MASK | R_CTRL_DBGEN_MASK |
+ R_CTRL_DAP_EN_MASK,
+};
+
+REG32(CFG, 0x08)
+ FIELD(CFG, RFIFO_TH, 10, 2)
+ FIELD(CFG, WFIFO_TH, 8, 2)
+ FIELD(CFG, RCLK_EDGE, 7, 1)
+ FIELD(CFG, WCLK_EDGE, 6, 1)
+ FIELD(CFG, DISABLE_SRC_INC, 5, 1)
+ FIELD(CFG, DISABLE_DST_INC, 4, 1)
+#define R_CFG_RESET 0x50B
+
+REG32(INT_STS, 0x0C)
+ FIELD(INT_STS, PSS_GTS_USR_B, 31, 1)
+ FIELD(INT_STS, PSS_FST_CFG_B, 30, 1)
+ FIELD(INT_STS, PSS_CFG_RESET_B, 27, 1)
+ FIELD(INT_STS, RX_FIFO_OV, 18, 1)
+ FIELD(INT_STS, WR_FIFO_LVL, 17, 1)
+ FIELD(INT_STS, RD_FIFO_LVL, 16, 1)
+ FIELD(INT_STS, DMA_CMD_ERR, 15, 1)
+ FIELD(INT_STS, DMA_Q_OV, 14, 1)
+ FIELD(INT_STS, DMA_DONE, 13, 1)
+ FIELD(INT_STS, DMA_P_DONE, 12, 1)
+ FIELD(INT_STS, P2D_LEN_ERR, 11, 1)
+ FIELD(INT_STS, PCFG_DONE, 2, 1)
+ #define R_INT_STS_RSVD ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
+
+REG32(INT_MASK, 0x10)
+
+REG32(STATUS, 0x14)
+ FIELD(STATUS, DMA_CMD_Q_F, 31, 1)
+ FIELD(STATUS, DMA_CMD_Q_E, 30, 1)
+ FIELD(STATUS, DMA_DONE_CNT, 28, 2)
+ FIELD(STATUS, RX_FIFO_LVL, 20, 5)
+ FIELD(STATUS, TX_FIFO_LVL, 12, 7)
+ FIELD(STATUS, PSS_GTS_USR_B, 11, 1)
+ FIELD(STATUS, PSS_FST_CFG_B, 10, 1)
+ FIELD(STATUS, PSS_CFG_RESET_B, 5, 1)
+
+REG32(DMA_SRC_ADDR, 0x18)
+REG32(DMA_DST_ADDR, 0x1C)
+REG32(DMA_SRC_LEN, 0x20)
+REG32(DMA_DST_LEN, 0x24)
+REG32(ROM_SHADOW, 0x28)
+REG32(SW_ID, 0x30)
+REG32(UNLOCK, 0x34)
+
+#define R_UNLOCK_MAGIC 0x757BDF0D
+
+REG32(MCTRL, 0x80)
+ FIELD(MCTRL, PS_VERSION, 28, 4)
+ FIELD(MCTRL, PCFG_POR_B, 8, 1)
+ FIELD(MCTRL, INT_PCAP_LPBK, 4, 1)
+ FIELD(MCTRL, QEMU, 3, 1)
+
+static void xlnx_zynq_devcfg_update_ixr(XlnxZynqDevcfg *s)
+{
+ qemu_set_irq(s->irq, ~s->regs[R_INT_MASK] & s->regs[R_INT_STS]);
+}
+
+static void xlnx_zynq_devcfg_reset(DeviceState *dev)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(dev);
+ int i;
+
+ for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
+ register_reset(&s->regs_info[i]);
+ }
+}
+
+static void xlnx_zynq_devcfg_dma_go(XlnxZynqDevcfg *s)
+{
+ do {
+ uint8_t buf[BTT_MAX];
+ XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
+ uint32_t btt = BTT_MAX;
+ bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
+
+ btt = MIN(btt, dmah->src_len);
+ if (loopback) {
+ btt = MIN(btt, dmah->dest_len);
+ }
+ DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
+ dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
+ dmah->src_len -= btt;
+ dmah->src_addr += btt;
+ if (loopback) {
+ 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);
+ } while (s->dma_cmd_fifo_num);
+}
+
+static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ xlnx_zynq_devcfg_update_ixr(s);
+}
+
+static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
+ if (s->regs[R_LOCK] & 1 << i) {
+ val &= ~lock_ctrl_map[i];
+ val |= lock_ctrl_map[i] & s->regs[R_CTRL];
+ }
+ }
+ return val;
+}
+
+static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
+{
+ const char *device_prefix = object_get_typename(OBJECT(reg->opaque));
+ uint32_t aes_en = 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",
+ device_prefix);
+ }
+}
+
+static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+ const char *device_prefix = object_get_typename(OBJECT(s));
+
+ if (val == R_UNLOCK_MAGIC) {
+ DB_PRINT("successful unlock\n");
+ /* 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", device_prefix);
+ s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
+ s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
+ /* core becomes inaccessible */
+ memory_region_set_enabled(&s->iomem, false);
+ }
+}
+
+static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ /* once bits are locked they stay locked */
+ return s->regs[R_LOCK] | val;
+}
+
+static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
+{
+ XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
+
+ s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
+ .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
+ .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
+ .src_len = s->regs[R_DMA_SRC_LEN] << 2,
+ .dest_len = s->regs[R_DMA_DST_LEN] << 2,
+ };
+ s->dma_cmd_fifo_num++;
+ DB_PRINT("dma transfer started; %d total transfers pending\n",
+ s->dma_cmd_fifo_num);
+ xlnx_zynq_devcfg_dma_go(s);
+}
+
+static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
+ { .name = "CTRL", .decode.addr = A_CTRL,
+ .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
+ .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
+ .pre_write = r_ctrl_pre_write,
+ .post_write = r_ctrl_post_write,
+ },
+ { .name = "LOCK", .decode.addr = A_LOCK,
+ .rsvd = MAKE_64BIT_MASK(5, 64 - 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 = MAKE_64BIT_MASK(27, 64 - 27) },
+ { .name = "DMA_DST_LEN", .decode.addr = A_DMA_DST_LEN,
+ .ro = MAKE_64BIT_MASK(27, 64 - 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,
+ XLNX_ZYNQ_DEVCFG_R_MAX);
+ sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void xlnx_zynq_devcfg_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->reset = xlnx_zynq_devcfg_reset;
+ dc->vmsd = &vmstate_xlnx_zynq_devcfg;
+}
+
+static const TypeInfo xlnx_zynq_devcfg_info = {
+ .name = TYPE_XLNX_ZYNQ_DEVCFG,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(XlnxZynqDevcfg),
+ .instance_init = xlnx_zynq_devcfg_init,
+ .class_init = xlnx_zynq_devcfg_class_init,
+};
+
+static void xlnx_zynq_devcfg_register_types(void)
+{
+ type_register_static(&xlnx_zynq_devcfg_info);
+}
+
+type_init(xlnx_zynq_devcfg_register_types)
diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
new file mode 100644
index 0000000..d40e5c8
--- /dev/null
+++ b/include/hw/dma/xlnx-zynq-devcfg.h
@@ -0,0 +1,62 @@
+/*
+ * QEMU model of the Xilinx Devcfg Interface
+ *
+ * (C) 2011 PetaLogix Pty Ltd
+ * (C) 2014 Xilinx Inc.
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef XLNX_ZYNQ_DEVCFG_H
+
+#include "hw/register.h"
+#include "hw/sysbus.h"
+
+#define TYPE_XLNX_ZYNQ_DEVCFG "xlnx.ps7-dev-cfg"
+
+#define XLNX_ZYNQ_DEVCFG(obj) \
+ OBJECT_CHECK(XlnxZynqDevcfg, (obj), TYPE_XLNX_ZYNQ_DEVCFG)
+
+#define XLNX_ZYNQ_DEVCFG_R_MAX 0x118
+
+#define XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN 10
+
+typedef struct XlnxZynqDevcfgDMACmd {
+ uint32_t src_addr;
+ uint32_t dest_addr;
+ uint32_t src_len;
+ uint32_t dest_len;
+} XlnxZynqDevcfgDMACmd;
+
+typedef struct XlnxZynqDevcfg {
+ SysBusDevice parent_obj;
+
+ MemoryRegion iomem;
+ qemu_irq irq;
+
+ XlnxZynqDevcfgDMACmd dma_cmd_fifo[XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN];
+ uint8_t dma_cmd_fifo_num;
+
+ uint32_t regs[XLNX_ZYNQ_DEVCFG_R_MAX];
+ RegisterInfo regs_info[XLNX_ZYNQ_DEVCFG_R_MAX];
+} XlnxZynqDevcfg;
+
+#define XLNX_ZYNQ_DEVCFG_H
+#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (6 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 11:19 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out Alistair Francis
` (4 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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>
---
V4:
- Small corrections to the device model logic
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 98b17c9..ffea3be 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -293,6 +293,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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (7 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 11:48 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 10/13] irq: Add opaque setter routine Alistair Francis
` (3 subsequent siblings)
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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 db41aa1..e3015d2 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 1ce02b2..0e216af 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -285,6 +285,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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 10/13] irq: Add opaque setter routine
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (8 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API Alistair Francis
` (2 subsequent siblings)
12 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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 49ff2e6..9d125fb 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -77,6 +77,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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (9 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 10/13] irq: Add opaque setter routine Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-10 11:52 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 12/13] misc: Introduce ZynqMP IOU SLCR Alistair Francis
2016-06-09 0:30 ` [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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>
---
V6:
- Don't use qdev_pass_all_gpios() as it doesn't seem to work
V5
- Remove RegisterAccessError struct
hw/core/register.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/register.h | 27 +++++++++++++++
2 files changed, 123 insertions(+)
diff --git a/hw/core/register.c b/hw/core/register.c
index c68d510..306d196 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -101,6 +101,7 @@ void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
}
register_write_val(reg, new_val);
+ register_refresh_gpios(reg, old_val, debug);
if (ac->post_write) {
ac->post_write(reg, new_val);
@@ -140,23 +141,118 @@ uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug)
void register_reset(RegisterInfo *reg)
{
g_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, false);
+}
+
+void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value, bool debug)
+{
+ 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 (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;
}
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);
+ qdev_pass_gpios(DEVICE(reg), reg->opaque, gpio->name);
+ }
+ }
}
static inline void register_write_memory(void *opaque, hwaddr addr,
diff --git a/include/hw/register.h b/include/hw/register.h
index c40cf03..245d80e 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -13,11 +13,24 @@
#include "hw/qdev-core.h"
#include "exec/memory.h"
+#include "hw/irq.h"
typedef struct RegisterInfo RegisterInfo;
typedef struct RegisterAccessInfo RegisterAccessInfo;
typedef struct RegisterInfoArray RegisterInfoArray;
+#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.
@@ -54,6 +67,8 @@ struct RegisterAccessInfo {
uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+ const RegisterGPIOMapping *gpios;
+
struct {
hwaddr addr;
} decode;
@@ -149,6 +164,18 @@ 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
+ * @debug: Should the read operation debug information be printed?
+ */
+
+void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value, bool debug);
+
+/**
* 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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 12/13] misc: Introduce ZynqMP IOU SLCR
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (10 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API Alistair Francis
@ 2016-05-12 22:46 ` Alistair Francis
2016-06-09 0:30 ` [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
12 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-05-12 22:46 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Cc: alistair.francis, crosthwaitepeter, edgar.iglesias,
edgar.iglesias, afaerber, fred.konrad, alex.bennee
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 | 115 +++++++++++++++++++++++++++++++++
include/hw/misc/xlnx-zynqmp-iou-slcr.h | 47 ++++++++++++++
3 files changed, 163 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 93f9528..d772c50 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -42,6 +42,7 @@ obj-$(CONFIG_RASPI) += bcm2835_property.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_MIPS_CPS) += mips_cmgcr.o
obj-$(CONFIG_MIPS_CPS) += mips_cpc.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..00b617e
--- /dev/null
+++ b/hw/misc/xlnx-zynqmp-iou-slcr.c
@@ -0,0 +1,115 @@
+/*
+ * 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 "qemu/osdep.h"
+#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,
+ XLNX_ZYNQ_MP_IOU_SLCR_R_MAX);
+ 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..b3bcb19
--- /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.7.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/13] data-driven device registers
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
` (11 preceding siblings ...)
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 12/13] misc: Introduce ZynqMP IOU SLCR Alistair Francis
@ 2016-06-09 0:30 ` Alistair Francis
2016-06-10 11:53 ` Peter Maydell
12 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-06-09 0:30 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, Peter Maydell, Edgar Iglesias,
Peter Crosthwaite, Edgar Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Thu, May 12, 2016 at 3:45 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch series is based on Peter C's original register API. His
> original cover letter is below.
Ping!
>
> Future work: Allow support for memory attributes.
>
> V6:
> - Small changes to the API based on Alex's comments
> - Remove 'register: Add support for decoding information' patch
> - Move prefix and debug into the RegisterInfoArray as it is the same
> for every register.
> V5:
> - Only create a single memory region instead of a memory region for
> each register
> - General tidyups based on Alex's comments
> V4:
> - Rebase and fix build issue
> - Simplify the register write logic
> - Other small fixes suggested by Alex Bennee
> V3:
> - Small changes reported by Fred
> V2:
> - Rebase
> - Fix up IOU SLCR connections
> - Add the memory_region_add_subregion_no_print() function and use it
> for the registers
> Changes since RFC:
> - Connect the ZynqMP IOU SLCR device
> - Rebase
>
> Original cover letter From Peter:
> Hi All. This is a new scheme I've come up with handling device registers in a
> data driven way. My motivation for this is to factor out a lot of the access
> checking that seems to be replicated in every device. See P1 commit message for
> further discussion.
>
> P1 is the main patch, adds the register definition functionality
> P2-3,6 add helpers that glue the register API to the Memory API
> P4 Defines a set of macros that minimise register and field definitions
> P5 is QOMfication
> P7 is a trivial
> P10-13 Work up to GPIO support
> P8,9,14 add new devices (the Xilinx Zynq devcfg & ZynqMP SLCR) that use this
> scheme.
> P15: Connect the ZynqMP SLCR device
>
> This Zynq devcfg device was particularly finnicky with per-bit restrictions.
> I'm also looking for a higher-than-usual modelling fidelity
> on the register space, with semantics defined for random reserved bits
> in-between otherwise consistent fields.
>
> Here's an example of the qemu_log output for the devcfg device. This is produced
> by now generic sharable code:
>
> /machine/unattached/device[44]:Addr 0x000008:CFG: write of value 00000508
> /machine/unattached/device[44]:Addr 0x000080:MCTRL: write of value 00800010
> /machine/unattached/device[44]:Addr 0x000010:INT_MASK: write of value ffffffff
> /machine/unattached/device[44]:Addr 00000000:CTRL: write of value 0c00607f
>
> And an example of a rogue guest banging on a bad bit:
>
> /machine/unattached/device[44]:Addr 0x000014:STATUS bits 0x000001 may not be \
> written to 1
>
> A future feature I am interested in is implementing TCG optimisation of
> side-effectless registers. The register API allows clear definition of
> what registers have txn side effects and which ones don't. You could even
> go a step further and translate such side-effectless accesses based on the
> data pointer for the register.
>
>
> Alistair Francis (6):
> bitops: Add MAKE_64BIT_MASK macro
> register: Add Register API
> register: Add Memory API glue
> dma: Add Xilinx Zynq devcfg device model
> register: Add GPIO API
> xlnx-zynqmp: Connect the ZynqMP IOU SLCR
>
> Peter Crosthwaite (7):
> register: Define REG and FIELD macros
> register: QOMify
> register: Add block initialise helper
> xilinx_zynq: Connect devcfg to the Zynq machine model
> qdev: Define qdev_get_gpio_out
> irq: Add opaque setter routine
> 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 | 12 +
> hw/core/register.c | 376 +++++++++++++++++++++++++++++++
> hw/dma/Makefile.objs | 1 +
> hw/dma/xlnx-zynq-devcfg.c | 396 +++++++++++++++++++++++++++++++++
> hw/misc/Makefile.objs | 1 +
> hw/misc/xlnx-zynqmp-iou-slcr.c | 115 ++++++++++
> 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 | 2 +
> include/hw/register.h | 262 ++++++++++++++++++++++
> include/qemu/bitops.h | 3 +
> 18 files changed, 1309 insertions(+)
> 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.7.4
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue Alistair Francis
@ 2016-06-09 13:08 ` KONRAD Frederic
2016-06-21 16:52 ` Alistair Francis
2016-06-09 19:03 ` Peter Maydell
1 sibling, 1 reply; 41+ messages in thread
From: KONRAD Frederic @ 2016-06-09 13:08 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, peter.maydell, edgar.iglesias, crosthwaitepeter,
edgar.iglesias, alex.bennee, afaerber
Hi Alistair,
Le 13/05/2016 à 00:45, Alistair Francis a écrit :
> Add memory io handlers that glue the register API to the memory API.
> Just translation functions at this stage. Although it does allow for
> devices to be created without all-in-one mmio r/w handlers.
>
> This patch also adds the RegisterInfoArray struct, which allows all of
> the individual RegisterInfo structs to be grouped into a single memory
> region.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V6:
> - Add the memory region later
> V5:
> - Convert to using only one memory region
>
> hw/core/register.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/register.h | 50 +++++++++++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 5e6f621..25196e6 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -147,3 +147,75 @@ 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)
> +{
> + RegisterInfoArray *reg_array = opaque;
> + RegisterInfo *reg = NULL;
> + uint64_t we = ~0;
> + int i, shift = 0;
> +
> + for (i = 0; i < reg_array->num_elements; i++) {
> + if (reg_array->r[i]->access->decode.addr == addr) {
> + reg = reg_array->r[i];
> + break;
> + }
> + }
> + assert(reg);
> +
> + /* Generate appropriate write enable mask and shift values */
> + if (reg->data_size < size) {
> + we = MAKE_64BIT_MASK(0, reg->data_size * 8);
> + shift = 8 * (be ? reg->data_size - size : 0);
> + } else if (reg->data_size >= size) {
> + we = MAKE_64BIT_MASK(0, size * 8);
> + }
> +
> + register_write(reg, value << shift, we << shift, reg_array->prefix,
> + reg_array->debug);
> +}
> +
> +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)
> +{
> + RegisterInfoArray *reg_array = opaque;
> + RegisterInfo *reg = NULL;
> + int i, shift;
> +
> + for (i = 0; i < reg_array->num_elements; i++) {
> + if (reg_array->r[i]->access->decode.addr == addr) {
> + reg = reg_array->r[i];
> + break;
> + }
> + }
> + assert(reg);
> +
> + shift = 8 * (be ? reg->data_size - size : 0);
> +
> + return (register_read(reg, reg_array->prefix, reg_array->debug) >> shift) &
> + MAKE_64BIT_MASK(0, size * 8);
> +}
> +
> +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 07d0616..786707b 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -15,6 +15,7 @@
>
> typedef struct RegisterInfo RegisterInfo;
> typedef struct RegisterAccessInfo RegisterAccessInfo;
> +typedef struct RegisterInfoArray RegisterInfoArray;
>
> /**
> * Access description for a register that is part of guest accessible device
> @@ -51,6 +52,10 @@ struct RegisterAccessInfo {
> void (*post_write)(RegisterInfo *reg, uint64_t val);
>
> uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
> +
> + struct {
> + hwaddr addr;
> + } decode;
Is there any reason why there is a struct here?
Fred
> };
>
> /**
> @@ -79,6 +84,25 @@ struct RegisterInfo {
> };
>
> /**
> + * This structure is used to group all of the individual registers which are
> + * modeled using the RegisterInfo strucutre.
> + *
> + * @r is an aray containing of all the relevent RegisterInfo structures.
> + *
> + * @num_elements is the number of elements in the array r
> + *
> + * @mem: optional Memory region for the register
> + */
> +
> +struct RegisterInfoArray {
> + int num_elements;
> + RegisterInfo **r;
> +
> + bool debug;
> + const char *prefix;
> +};
> +
> +/**
> * write a value to a register, subject to its restrictions
> * @reg: register to write to
> * @val: value to write
> @@ -107,4 +131,30 @@ uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug);
>
> 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
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro Alistair Francis
@ 2016-06-09 18:46 ` Peter Maydell
2016-06-13 20:57 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-09 18:46 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a macro that creates a 64bit value which has length number of ones
> shifted acrros by the value of shift.
"across"
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> V5:
> - Re-write to a 64-bit mask instead of ONES()
> - Re-order this patch in the series
>
> include/qemu/bitops.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index 755fdd1..3c45791 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -24,6 +24,9 @@
> #define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>
> +#define MAKE_64BIT_MASK(shift, length) \
> + (((1ull << (length)) - 1) << shift)
> +
This is undefined behaviour for a 64-bit length. The expression
we use in deposit64() to create a mask is
((~0ULL >> (64 - length)) << start)
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/13] register: Add Register API
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 02/13] register: Add Register API Alistair Francis
@ 2016-06-09 18:55 ` Peter Maydell
2016-06-13 21:01 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-09 18:55 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
> This API provides some encapsulation of registers and factors our some
"out"
> common functionality to common code. Bits of device state (usually MMIO
> registers), often have all sorts of access restrictions and semantics
spurious comma
> associated with them. This API allow you to define what those
"allows"
> restrictions are on a bit-by-bit basis.
>
> Helper functions are then used to access the register which observe the
> semantics defined by the RegisterAccessInfo struct.
>
> Some features:
> Bits can be marked as read_only (ro field)
> Bits can be marked as write-1-clear (w1c field)
> Bits can be marked as reserved (rsvd field)
> Reset values can be defined (reset)
> Bits can be marked clear on read (cor)
> Pre and post action callbacks can be added to read and write ops
> Verbose debugging info can be enabled/disabled
>
> Useful for defining device register spaces in a data driven way. Cuts
> down on a lot of the verbosity and repetition in the switch-case blocks
> in the standard foo_mmio_read/write functions.
>
> Also useful for automated generation of device models from hardware
> design sources.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> V5:
> - Convert to using only one memory region
> V4:
> - Rebase
> - Remove the guest error masking
> - Simplify the unimplemented masking
> - Use the reserved value in the write calculations
> - Remove read_lite and write_lite
> - General fixes to asserts and log printing
> V3:
> - Address some comments from Fred
>
> hw/core/Makefile.objs | 1 +
> hw/core/register.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/register.h | 110 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 260 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..5e6f621
> --- /dev/null
> +++ b/hw/core/register.c
> @@ -0,0 +1,149 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2016 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
Is this deliberately GPL2-only rather than 2-or-later ?
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue Alistair Francis
2016-06-09 13:08 ` KONRAD Frederic
@ 2016-06-09 19:03 ` Peter Maydell
2016-06-21 0:46 ` Alistair Francis
1 sibling, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-09 19:03 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add memory io handlers that glue the register API to the memory API.
> Just translation functions at this stage. Although it does allow for
> devices to be created without all-in-one mmio r/w handlers.
>
> This patch also adds the RegisterInfoArray struct, which allows all of
> the individual RegisterInfo structs to be grouped into a single memory
> region.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V6:
> - Add the memory region later
> V5:
> - Convert to using only one memory region
>
> hw/core/register.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/register.h | 50 +++++++++++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 5e6f621..25196e6 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -147,3 +147,75 @@ 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)
> +{
> + RegisterInfoArray *reg_array = opaque;
> + RegisterInfo *reg = NULL;
> + uint64_t we = ~0;
> + int i, shift = 0;
> +
> + for (i = 0; i < reg_array->num_elements; i++) {
> + if (reg_array->r[i]->access->decode.addr == addr) {
> + reg = reg_array->r[i];
> + break;
> + }
> + }
> + assert(reg);
I'm surprised we don't support having the register array have
gaps for unimplemented/undefined registers. Presumably users
have to specify a lot of unimplemented entries ?
If you're going to assert() on undecoded addresses it would be
better to do a scan through at device init to sanity check
the register array, so missing elements are an obvious failure
rather than only showing up if the guest happens to access them.
> +
> + /* Generate appropriate write enable mask and shift values */
> + if (reg->data_size < size) {
> + we = MAKE_64BIT_MASK(0, reg->data_size * 8);
> + shift = 8 * (be ? reg->data_size - size : 0);
> + } else if (reg->data_size >= size) {
> + we = MAKE_64BIT_MASK(0, size * 8);
> + }
> +
> + register_write(reg, value << shift, we << shift, reg_array->prefix,
> + reg_array->debug);
> +}
> +
> +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)
> +{
> + RegisterInfoArray *reg_array = opaque;
> + RegisterInfo *reg = NULL;
> + int i, shift;
> +
> + for (i = 0; i < reg_array->num_elements; i++) {
> + if (reg_array->r[i]->access->decode.addr == addr) {
> + reg = reg_array->r[i];
> + break;
> + }
> + }
> + assert(reg);
> +
> + shift = 8 * (be ? reg->data_size - size : 0);
> +
> + return (register_read(reg, reg_array->prefix, reg_array->debug) >> shift) &
> + MAKE_64BIT_MASK(0, size * 8);
This kind of thing is reimplementing extract64().
> +}
> +
> +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);
> +}
Why do we need to handle big vs little endian separately rather
than just having the memory region say which it is and letting
the core memory system handle things appropriately ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros Alistair Francis
@ 2016-06-10 10:52 ` Peter Maydell
2016-06-21 17:41 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 10:52 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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.
Could we have the documentation of what these macros do in the code,
not just in the commit message and the extra remarks, please?
> 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);
These deposit functions seem a bit too cryptically named to me;
can we come up with something a bit less abbreviated?
> The latter assignment results in:
> warning: large integer implicitly truncated to unsigned type [-Woverflow]
This is inconsistent with the behaviour of deposit32() and
deposit64() which are documented to ignore oversized values.
> include/hw/register.h | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 786707b..e0aac91 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -157,4 +157,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)) };
We defined a MAKE_64BIT_MASK macro in patch 1, so we can use it here.
(Also this open-coded version has the same "undefined behaviour if
length is 64" issue.)
> +
> +/* 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.7.4
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/13] register: QOMify
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 05/13] register: QOMify Alistair Francis
@ 2016-06-10 10:55 ` Peter Maydell
2016-06-21 16:49 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 10:55 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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.
You should just squash this down into patch 2.
> 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>
> ---
> /**
> + * Initialize a register. GPIO's are setup as IOs to the specified device.
> + * Fast paths for eligible registers are enabled.
> + * @reg: Register to initialize
> + */
I can't work out what this documentation comment is trying to say.
How can a register have a GPIO? What does a fast path do, what
registers are elegible, why do I care whether they're enabled or not?
> +
> +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.7.4
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper Alistair Francis
@ 2016-06-10 11:02 ` Peter Maydell
2016-06-21 18:25 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 11:02 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Add a helper that will scan a static RegisterAccessInfo Array
> and populate a container MemoryRegion with registers as defined.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> The reason that I'm not using GArray is because the array needs to store
> the memory region that covers all of the registers.
>
> V6:
> - Fixup the loop logic
> V5:
> - Convert to only using one memory region
> V3:
> - Fix typo
> V2:
> - Use memory_region_add_subregion_no_print()
>
> hw/core/register.c | 36 ++++++++++++++++++++++++++++++++++++
> include/hw/register.h | 22 ++++++++++++++++++++++
> 2 files changed, 58 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index c5a2c78..c68d510 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -231,6 +231,42 @@ 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, uint64_t memory_size)
> +{
> + const char *device_prefix = object_get_typename(OBJECT(owner));
> + RegisterInfoArray *r_array = g_malloc(sizeof(RegisterInfoArray));
> + int i;
> +
> + r_array->r = g_malloc_n(num, sizeof(RegisterInfo *));
> + r_array->num_elements = num;
> + r_array->debug = debug_enabled;
> + r_array->prefix = device_prefix;
> +
> + for (i = 0; i < num; i++) {
> + int index = rae[i].decode.addr / 4;
> + RegisterInfo *r = &ri[index];
> +
> + *r = (RegisterInfo) {
> + .data = &data[index],
> + .data_size = sizeof(uint32_t),
> + .access = &rae[i],
> + .opaque = owner,
> + };
> + register_init(r);
> +
> + r_array->r[i] = r;
> + }
> +
> + memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
> + device_prefix, memory_size);
> + memory_region_add_subregion(container,
> + r_array->r[0]->access->decode.addr,
> + &r_array->mem);
It feels a bit more natural to me to return the MemoryRegion of
the registers from this function and let the caller put it into
the container wherever they want (or just use it directly as a
sysbus MemoryRegion, rather than having this function put it
into a container passed in by the parent.
> +}
> +
> static const TypeInfo register_info = {
> .name = TYPE_REGISTER,
> .parent = TYPE_DEVICE,
> diff --git a/include/hw/register.h b/include/hw/register.h
> index eedd578..c40cf03 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -102,6 +102,8 @@ struct RegisterInfo {
> */
>
> struct RegisterInfoArray {
> + MemoryRegion mem;
> +
> int num_elements;
> RegisterInfo **r;
>
> @@ -172,6 +174,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, uint64_t memory_size);
This doesn't seem to contemplate register arrays which are mostly
consecutive but have some unimplemented/reserved regions.
> +
> /* Define constants for a 32 bit register */
> #define REG32(reg, addr) \
> enum { A_ ## reg = (addr) }; \
> --
> 2.7.4
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model Alistair Francis
@ 2016-06-10 11:16 ` Peter Maydell
2016-06-21 18:34 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 11:16 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a minimal model for the devcfg device which is part of Zynq.
> This model supports DMA capabilities and interrupt generation.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V5:
> - Corrections to the device model logic
>
> default-configs/arm-softmmu.mak | 1 +
> hw/dma/Makefile.objs | 1 +
> hw/dma/xlnx-zynq-devcfg.c | 396 ++++++++++++++++++++++++++++++++++++++
> include/hw/dma/xlnx-zynq-devcfg.h | 62 ++++++
> 4 files changed, 460 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 c63cdd0..40f94ec 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 a1abbcf..29b4520 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..9de4e75
> --- /dev/null
> +++ b/hw/dma/xlnx-zynq-devcfg.c
> @@ -0,0 +1,396 @@
> +/*
> + * QEMU model of the Xilinx Zynq Devcfg Interface
> + *
> + * (C) 2011 PetaLogix Pty Ltd
> + * (C) 2014 Xilinx Inc.
> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/dma/xlnx-zynq-devcfg.h"
> +#include "qemu/bitops.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/dma.h"
> +
> +#define FREQ_HZ 900000000
> +
> +#define BTT_MAX 0x400
> +
> +#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
> +#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT(fmt, args...) do { \
> + if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
> + qemu_log("%s: " fmt, __func__, ## args); \
> + } \
> +} while (0);
> +
> +REG32(CTRL, 0x00)
> + FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr ignored */
> + FIELD(CTRL, PCAP_PR, 27, 1) /* Forced to 0 on bad unlock */
> + FIELD(CTRL, PCAP_MODE, 26, 1)
> + FIELD(CTRL, MULTIBOOT_EN, 24, 1)
> + FIELD(CTRL, USER_MODE, 15, 1)
> + FIELD(CTRL, PCFG_AES_FUSE, 12, 1)
> + FIELD(CTRL, PCFG_AES_EN, 9, 3)
> + FIELD(CTRL, SEU_EN, 8, 1)
> + FIELD(CTRL, SEC_EN, 7, 1)
> + FIELD(CTRL, SPNIDEN, 6, 1)
> + FIELD(CTRL, SPIDEN, 5, 1)
> + FIELD(CTRL, NIDEN, 4, 1)
> + FIELD(CTRL, DBGEN, 3, 1)
> + FIELD(CTRL, DAP_EN, 0, 3)
> +
> +REG32(LOCK, 0x04)
> + #define AES_FUSE_LOCK 4
> + #define AES_EN_LOCK 3
> + #define SEU_LOCK 2
> + #define SEC_LOCK 1
> + #define DBG_LOCK 0
Indented #defines are a bit weird.
> +
> +/* mapping bits in R_LOCK to what they lock in R_CTRL */
> +static const uint32_t lock_ctrl_map[] = {
> + [AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
> + [AES_EN_LOCK] = R_CTRL_PCFG_AES_EN_MASK,
> + [SEU_LOCK] = R_CTRL_SEU_EN_MASK,
> + [SEC_LOCK] = R_CTRL_SEC_EN_MASK,
> + [DBG_LOCK] = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
> + R_CTRL_NIDEN_MASK | R_CTRL_DBGEN_MASK |
> + R_CTRL_DAP_EN_MASK,
> +};
> +
> +REG32(CFG, 0x08)
> + FIELD(CFG, RFIFO_TH, 10, 2)
> + FIELD(CFG, WFIFO_TH, 8, 2)
> + FIELD(CFG, RCLK_EDGE, 7, 1)
> + FIELD(CFG, WCLK_EDGE, 6, 1)
> + FIELD(CFG, DISABLE_SRC_INC, 5, 1)
> + FIELD(CFG, DISABLE_DST_INC, 4, 1)
> +#define R_CFG_RESET 0x50B
> +
> +REG32(INT_STS, 0x0C)
> + FIELD(INT_STS, PSS_GTS_USR_B, 31, 1)
> + FIELD(INT_STS, PSS_FST_CFG_B, 30, 1)
> + FIELD(INT_STS, PSS_CFG_RESET_B, 27, 1)
> + FIELD(INT_STS, RX_FIFO_OV, 18, 1)
> + FIELD(INT_STS, WR_FIFO_LVL, 17, 1)
> + FIELD(INT_STS, RD_FIFO_LVL, 16, 1)
> + FIELD(INT_STS, DMA_CMD_ERR, 15, 1)
> + FIELD(INT_STS, DMA_Q_OV, 14, 1)
> + FIELD(INT_STS, DMA_DONE, 13, 1)
> + FIELD(INT_STS, DMA_P_DONE, 12, 1)
> + FIELD(INT_STS, P2D_LEN_ERR, 11, 1)
> + FIELD(INT_STS, PCFG_DONE, 2, 1)
> + #define R_INT_STS_RSVD ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
> +
> +REG32(INT_MASK, 0x10)
> +
> +REG32(STATUS, 0x14)
> + FIELD(STATUS, DMA_CMD_Q_F, 31, 1)
> + FIELD(STATUS, DMA_CMD_Q_E, 30, 1)
> + FIELD(STATUS, DMA_DONE_CNT, 28, 2)
> + FIELD(STATUS, RX_FIFO_LVL, 20, 5)
> + FIELD(STATUS, TX_FIFO_LVL, 12, 7)
> + FIELD(STATUS, PSS_GTS_USR_B, 11, 1)
> + FIELD(STATUS, PSS_FST_CFG_B, 10, 1)
> + FIELD(STATUS, PSS_CFG_RESET_B, 5, 1)
> +
> +REG32(DMA_SRC_ADDR, 0x18)
> +REG32(DMA_DST_ADDR, 0x1C)
> +REG32(DMA_SRC_LEN, 0x20)
> +REG32(DMA_DST_LEN, 0x24)
> +REG32(ROM_SHADOW, 0x28)
> +REG32(SW_ID, 0x30)
> +REG32(UNLOCK, 0x34)
> +
> +#define R_UNLOCK_MAGIC 0x757BDF0D
> +
> +REG32(MCTRL, 0x80)
> + FIELD(MCTRL, PS_VERSION, 28, 4)
> + FIELD(MCTRL, PCFG_POR_B, 8, 1)
> + FIELD(MCTRL, INT_PCAP_LPBK, 4, 1)
> + FIELD(MCTRL, QEMU, 3, 1)
> +
> +static void xlnx_zynq_devcfg_update_ixr(XlnxZynqDevcfg *s)
> +{
> + qemu_set_irq(s->irq, ~s->regs[R_INT_MASK] & s->regs[R_INT_STS]);
> +}
> +
> +static void xlnx_zynq_devcfg_reset(DeviceState *dev)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(dev);
> + int i;
> +
> + for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
> + register_reset(&s->regs_info[i]);
> + }
> +}
> +
> +static void xlnx_zynq_devcfg_dma_go(XlnxZynqDevcfg *s)
> +{
> + do {
> + uint8_t buf[BTT_MAX];
> + XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
> + uint32_t btt = BTT_MAX;
> + bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
> +
> + btt = MIN(btt, dmah->src_len);
> + if (loopback) {
> + btt = MIN(btt, dmah->dest_len);
> + }
> + DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
> + dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
> + dmah->src_len -= btt;
> + dmah->src_addr += btt;
> + if (loopback) {
> + 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) {
If this is true on entry to the function then you'll do a
dma_memory_read/write of 0 bytes, which is a bit odd.
> + 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);
Isn't this a longwinded way to say "sizeof(s->dma_cmd_fifo)" ?
> + }
> + xlnx_zynq_devcfg_update_ixr(s);
> + } while (s->dma_cmd_fifo_num);
> +}
> +
> +static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + xlnx_zynq_devcfg_update_ixr(s);
> +}
> +
> +static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
> + if (s->regs[R_LOCK] & 1 << i) {
> + val &= ~lock_ctrl_map[i];
> + val |= lock_ctrl_map[i] & s->regs[R_CTRL];
> + }
> + }
> + return val;
> +}
> +
> +static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + const char *device_prefix = object_get_typename(OBJECT(reg->opaque));
> + uint32_t aes_en = 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",
> + device_prefix);
> + }
> +}
> +
> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> + const char *device_prefix = object_get_typename(OBJECT(s));
> +
> + if (val == R_UNLOCK_MAGIC) {
> + DB_PRINT("successful unlock\n");
> + /* 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", device_prefix);
> + s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
> + s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
> + /* core becomes inaccessible */
> + memory_region_set_enabled(&s->iomem, false);
> + }
> +}
> +
> +static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + /* once bits are locked they stay locked */
> + return s->regs[R_LOCK] | val;
> +}
> +
> +static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
> +{
> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +
> + s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
> + .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
> + .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
> + .src_len = s->regs[R_DMA_SRC_LEN] << 2,
> + .dest_len = s->regs[R_DMA_DST_LEN] << 2,
> + };
> + s->dma_cmd_fifo_num++;
> + DB_PRINT("dma transfer started; %d total transfers pending\n",
> + s->dma_cmd_fifo_num);
> + xlnx_zynq_devcfg_dma_go(s);
> +}
> +
> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
> + { .name = "CTRL", .decode.addr = A_CTRL,
> + .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
> + .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
> + .pre_write = r_ctrl_pre_write,
> + .post_write = r_ctrl_post_write,
> + },
> + { .name = "LOCK", .decode.addr = A_LOCK,
> + .rsvd = MAKE_64BIT_MASK(5, 64 - 5),
> + .pre_write = r_lock_pre_write,
> + },
Hang on, isn't this actually a sparse register info array?
I thought the core register code insisted the arrays had an
entry for every address, so either they're buggy or this is
or I misread the earlier patches...
> + { .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 = MAKE_64BIT_MASK(27, 64 - 27) },
> + { .name = "DMA_DST_LEN", .decode.addr = A_DMA_DST_LEN,
> + .ro = MAKE_64BIT_MASK(27, 64 - 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,
You don't need to specify minimum_version_id_old unless
you are providing a load_state_old function.
> + .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,
> + XLNX_ZYNQ_DEVCFG_R_MAX);
> + 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
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model Alistair Francis
@ 2016-06-10 11:19 ` Peter Maydell
2016-06-21 18:36 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 11:19 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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>
> ---
> V4:
> - Small corrections to the device model logic
>
> 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 98b17c9..ffea3be 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -293,6 +293,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);
Why do we need to do this?
> + qdev_init_nofail(dev);
> + busdev = SYS_BUS_DEVICE(dev);
> + sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
> + sysbus_mmio_map(busdev, 0, 0xF8007000);
> +
> zynq_binfo.ram_size = ram_size;
> zynq_binfo.kernel_filename = kernel_filename;
> zynq_binfo.kernel_cmdline = kernel_cmdline;
> --
> 2.7.4
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out Alistair Francis
@ 2016-06-10 11:48 ` Peter Maydell
2016-06-21 19:05 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 11:48 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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 db41aa1..e3015d2 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);
> +}
This appears to be identical to the existing function
qdev_get_gpio_out_connector() ?
> +
> +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 1ce02b2..0e216af 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -285,6 +285,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);
Doc comments for new global functions would be nice.
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API Alistair Francis
@ 2016-06-10 11:52 ` Peter Maydell
2016-06-21 22:34 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 11:52 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Peter Crosthwaite, Edgar Iglesias,
Edgar E. Iglesias, Andreas Färber,
KONRAD Frédéric, Alex Bennée
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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>
I'm definitely not convinced of the utility of this. I think
almost all devices don't have registers with bits which map
1:1 to GPIO lines like this, and the few devices which do can
easily enough just implement them by hand. GPIOs are (in my
view) a device level concept, not a register level concept,
and I think they're better implemented at the device level.
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/13] data-driven device registers
2016-06-09 0:30 ` [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
@ 2016-06-10 11:53 ` Peter Maydell
2016-06-13 21:11 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-10 11:53 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, Edgar Iglesias,
Peter Crosthwaite, Edgar Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On 9 June 2016 at 01:30, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Thu, May 12, 2016 at 3:45 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> This patch series is based on Peter C's original register API. His
>> original cover letter is below.
>
> Ping!
I've finished reviewing this series now -- apologies for letting it
sit on my to-review queue for so long.
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro
2016-06-09 18:46 ` Peter Maydell
@ 2016-06-13 20:57 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-13 20:57 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Thu, Jun 9, 2016 at 11:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Add a macro that creates a 64bit value which has length number of ones
>> shifted acrros by the value of shift.
>
> "across"
>
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> V5:
>> - Re-write to a 64-bit mask instead of ONES()
>> - Re-order this patch in the series
>>
>> include/qemu/bitops.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index 755fdd1..3c45791 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>> @@ -24,6 +24,9 @@
>> #define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
>> #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>>
>> +#define MAKE_64BIT_MASK(shift, length) \
>> + (((1ull << (length)) - 1) << shift)
>> +
>
> This is undefined behaviour for a 64-bit length. The expression
> we use in deposit64() to create a mask is
> ((~0ULL >> (64 - length)) << start)
Fixed both.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/13] register: Add Register API
2016-06-09 18:55 ` Peter Maydell
@ 2016-06-13 21:01 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-13 21:01 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Thu, Jun 9, 2016 at 11:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> This API provides some encapsulation of registers and factors our some
>
> "out"
>
>> common functionality to common code. Bits of device state (usually MMIO
>> registers), often have all sorts of access restrictions and semantics
>
> spurious comma
>
>> associated with them. This API allow you to define what those
>
> "allows"
>
>> restrictions are on a bit-by-bit basis.
>>
>> Helper functions are then used to access the register which observe the
>> semantics defined by the RegisterAccessInfo struct.
>>
>> Some features:
>> Bits can be marked as read_only (ro field)
>> Bits can be marked as write-1-clear (w1c field)
>> Bits can be marked as reserved (rsvd field)
>> Reset values can be defined (reset)
>> Bits can be marked clear on read (cor)
>> Pre and post action callbacks can be added to read and write ops
>> Verbose debugging info can be enabled/disabled
>>
>> Useful for defining device register spaces in a data driven way. Cuts
>> down on a lot of the verbosity and repetition in the switch-case blocks
>> in the standard foo_mmio_read/write functions.
>>
>> Also useful for automated generation of device models from hardware
>> design sources.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> V5:
>> - Convert to using only one memory region
>> V4:
>> - Rebase
>> - Remove the guest error masking
>> - Simplify the unimplemented masking
>> - Use the reserved value in the write calculations
>> - Remove read_lite and write_lite
>> - General fixes to asserts and log printing
>> V3:
>> - Address some comments from Fred
>>
>> hw/core/Makefile.objs | 1 +
>> hw/core/register.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/register.h | 110 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 260 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..5e6f621
>> --- /dev/null
>> +++ b/hw/core/register.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * Register Definition API
>> + *
>> + * Copyright (c) 2016 Xilinx Inc.
>> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
>> + */
>
> Is this deliberately GPL2-only rather than 2-or-later ?
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Awesome! Thanks
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 00/13] data-driven device registers
2016-06-10 11:53 ` Peter Maydell
@ 2016-06-13 21:11 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-13 21:11 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias,
qemu-devel@nongnu.org Developers, Peter Crosthwaite,
Edgar Iglesias, Alex Bennée, Andreas Färber,
KONRAD Frédéric
On Fri, Jun 10, 2016 at 4:53 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 June 2016 at 01:30, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Thu, May 12, 2016 at 3:45 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> This patch series is based on Peter C's original register API. His
>>> original cover letter is below.
>>
>> Ping!
>
> I've finished reviewing this series now -- apologies for letting it
> sit on my to-review queue for so long.
Thanks for that Peter, I'll send the next version out as soon as possible.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue
2016-06-09 19:03 ` Peter Maydell
@ 2016-06-21 0:46 ` Alistair Francis
2016-06-21 6:48 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 0:46 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Thu, Jun 9, 2016 at 12:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Add memory io handlers that glue the register API to the memory API.
>> Just translation functions at this stage. Although it does allow for
>> devices to be created without all-in-one mmio r/w handlers.
>>
>> This patch also adds the RegisterInfoArray struct, which allows all of
>> the individual RegisterInfo structs to be grouped into a single memory
>> region.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V6:
>> - Add the memory region later
>> V5:
>> - Convert to using only one memory region
>>
>> hw/core/register.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/register.h | 50 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 122 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index 5e6f621..25196e6 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -147,3 +147,75 @@ 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)
>> +{
>> + RegisterInfoArray *reg_array = opaque;
>> + RegisterInfo *reg = NULL;
>> + uint64_t we = ~0;
>> + int i, shift = 0;
>> +
>> + for (i = 0; i < reg_array->num_elements; i++) {
>> + if (reg_array->r[i]->access->decode.addr == addr) {
>> + reg = reg_array->r[i];
>> + break;
>> + }
>> + }
>> + assert(reg);
>
> I'm surprised we don't support having the register array have
> gaps for unimplemented/undefined registers. Presumably users
> have to specify a lot of unimplemented entries ?
>
> If you're going to assert() on undecoded addresses it would be
> better to do a scan through at device init to sanity check
> the register array, so missing elements are an obvious failure
> rather than only showing up if the guest happens to access them.
You're right, this is a little harsh. I'm thinking I'll change it to a
qemu_log() (unimplemented, although it also could be a guest error)
and remove the assert().
>
>> +
>> + /* Generate appropriate write enable mask and shift values */
>> + if (reg->data_size < size) {
>> + we = MAKE_64BIT_MASK(0, reg->data_size * 8);
>> + shift = 8 * (be ? reg->data_size - size : 0);
>> + } else if (reg->data_size >= size) {
>> + we = MAKE_64BIT_MASK(0, size * 8);
>> + }
>> +
>> + register_write(reg, value << shift, we << shift, reg_array->prefix,
>> + reg_array->debug);
>> +}
>> +
>> +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)
>> +{
>> + RegisterInfoArray *reg_array = opaque;
>> + RegisterInfo *reg = NULL;
>> + int i, shift;
>> +
>> + for (i = 0; i < reg_array->num_elements; i++) {
>> + if (reg_array->r[i]->access->decode.addr == addr) {
>> + reg = reg_array->r[i];
>> + break;
>> + }
>> + }
>> + assert(reg);
>> +
>> + shift = 8 * (be ? reg->data_size - size : 0);
>> +
>> + return (register_read(reg, reg_array->prefix, reg_array->debug) >> shift) &
>> + MAKE_64BIT_MASK(0, size * 8);
>
> This kind of thing is reimplementing extract64().
Ok, I'll update it to use extract64()
>
>> +}
>> +
>> +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);
>> +}
>
> Why do we need to handle big vs little endian separately rather
> than just having the memory region say which it is and letting
> the core memory system handle things appropriately ?
I didn't realise that is an option. So I can remove all the endianess
handling from here and the core will handle it?
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue
2016-06-21 0:46 ` Alistair Francis
@ 2016-06-21 6:48 ` Peter Maydell
0 siblings, 0 replies; 41+ messages in thread
From: Peter Maydell @ 2016-06-21 6:48 UTC (permalink / raw)
To: Alistair Francis
Cc: Edgar Iglesias, QEMU Developers, Peter Crosthwaite,
Edgar E. Iglesias, Alex Bennée, Andreas Färber,
KONRAD Frédéric
On 21 June 2016 at 01:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Thu, Jun 9, 2016 at 12:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Why do we need to handle big vs little endian separately rather
>> than just having the memory region say which it is and letting
>> the core memory system handle things appropriately ?
>
> I didn't realise that is an option. So I can remove all the endianess
> handling from here and the core will handle it?
It should do, though you should test that it behaves the way you
expect.
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/13] register: QOMify
2016-06-10 10:55 ` Peter Maydell
@ 2016-06-21 16:49 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 16:49 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 3:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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.
>
> You should just squash this down into patch 2.
It relies on some work from the previous two patches so it becomes a
pretty big patch if they all get squashed together. I'd rather leave
it separate like this.
>
>> 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>
>> ---
>
>> /**
>> + * Initialize a register. GPIO's are setup as IOs to the specified device.
>> + * Fast paths for eligible registers are enabled.
>> + * @reg: Register to initialize
>> + */
>
> I can't work out what this documentation comment is trying to say.
> How can a register have a GPIO? What does a fast path do, what
> registers are elegible, why do I care whether they're enabled or not?
I have updated it to this, which removes the fast path (that was left
over) and explains more about GPIOs.
/**
* Initialize a register. This will also setup any GPIO links which are used
* to connect register updates in one device to other devices. Generally this
* is useful for interrupt propagation.
* @reg: Register to initialize
*/
Thanks,
Alistair
>
>> +
>> +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.7.4
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue
2016-06-09 13:08 ` KONRAD Frederic
@ 2016-06-21 16:52 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 16:52 UTC (permalink / raw)
To: KONRAD Frederic
Cc: Alistair Francis, Edgar Iglesias, Peter Maydell,
qemu-devel@nongnu.org Developers, Peter Crosthwaite,
Edgar Iglesias, Alex Bennée, Andreas Färber
On Thu, Jun 9, 2016 at 6:08 AM, KONRAD Frederic
<fred.konrad@greensocs.com> wrote:
> Hi Alistair,
>
>
> Le 13/05/2016 à 00:45, Alistair Francis a écrit :
>>
>> Add memory io handlers that glue the register API to the memory API.
>> Just translation functions at this stage. Although it does allow for
>> devices to be created without all-in-one mmio r/w handlers.
>>
>> This patch also adds the RegisterInfoArray struct, which allows all of
>> the individual RegisterInfo structs to be grouped into a single memory
>> region.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V6:
>> - Add the memory region later
>> V5:
>> - Convert to using only one memory region
>>
>> hw/core/register.c | 72
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/register.h | 50 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 122 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index 5e6f621..25196e6 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -147,3 +147,75 @@ 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)
>> +{
>> + RegisterInfoArray *reg_array = opaque;
>> + RegisterInfo *reg = NULL;
>> + uint64_t we = ~0;
>> + int i, shift = 0;
>> +
>> + for (i = 0; i < reg_array->num_elements; i++) {
>> + if (reg_array->r[i]->access->decode.addr == addr) {
>> + reg = reg_array->r[i];
>> + break;
>> + }
>> + }
>> + assert(reg);
>> +
>> + /* Generate appropriate write enable mask and shift values */
>> + if (reg->data_size < size) {
>> + we = MAKE_64BIT_MASK(0, reg->data_size * 8);
>> + shift = 8 * (be ? reg->data_size - size : 0);
>> + } else if (reg->data_size >= size) {
>> + we = MAKE_64BIT_MASK(0, size * 8);
>> + }
>> +
>> + register_write(reg, value << shift, we << shift, reg_array->prefix,
>> + reg_array->debug);
>> +}
>> +
>> +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)
>> +{
>> + RegisterInfoArray *reg_array = opaque;
>> + RegisterInfo *reg = NULL;
>> + int i, shift;
>> +
>> + for (i = 0; i < reg_array->num_elements; i++) {
>> + if (reg_array->r[i]->access->decode.addr == addr) {
>> + reg = reg_array->r[i];
>> + break;
>> + }
>> + }
>> + assert(reg);
>> +
>> + shift = 8 * (be ? reg->data_size - size : 0);
>> +
>> + return (register_read(reg, reg_array->prefix, reg_array->debug) >>
>> shift) &
>> + MAKE_64BIT_MASK(0, size * 8);
>> +}
>> +
>> +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 07d0616..786707b 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -15,6 +15,7 @@
>>
>> typedef struct RegisterInfo RegisterInfo;
>> typedef struct RegisterAccessInfo RegisterAccessInfo;
>> +typedef struct RegisterInfoArray RegisterInfoArray;
>>
>> /**
>> * Access description for a register that is part of guest accessible
>> device
>> @@ -51,6 +52,10 @@ struct RegisterAccessInfo {
>> void (*post_write)(RegisterInfo *reg, uint64_t val);
>>
>> uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
>> +
>> + struct {
>> + hwaddr addr;
>> + } decode;
>
>
> Is there any reason why there is a struct here?
I think it is just left over, I have removed it.
Thanks,
Alistair
>
> Fred
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros
2016-06-10 10:52 ` Peter Maydell
@ 2016-06-21 17:41 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 17:41 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 3:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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.
>
> Could we have the documentation of what these macros do in the code,
> not just in the commit message and the extra remarks, please?
Fixed.
>
>> 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);
>
> These deposit functions seem a bit too cryptically named to me;
> can we come up with something a bit less abbreviated?
Ok, I have changed the names to be longer.
>
>> The latter assignment results in:
>> warning: large integer implicitly truncated to unsigned type [-Woverflow]
>
> This is inconsistent with the behaviour of deposit32() and
> deposit64() which are documented to ignore oversized values.
Should we really just ignore this? This seems like a possible common
mistake and I think it is a good idea to warn against it.
>
>> include/hw/register.h | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 786707b..e0aac91 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -157,4 +157,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)) };
>
> We defined a MAKE_64BIT_MASK macro in patch 1, so we can use it here.
> (Also this open-coded version has the same "undefined behaviour if
> length is 64" issue.)
Fixed.
Thanks,
Alistair
>
>> +
>> +/* 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.7.4
>>
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper
2016-06-10 11:02 ` Peter Maydell
@ 2016-06-21 18:25 ` Alistair Francis
2016-06-21 19:45 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 18:25 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 4:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Add a helper that will scan a static RegisterAccessInfo Array
>> and populate a container MemoryRegion with registers as defined.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> The reason that I'm not using GArray is because the array needs to store
>> the memory region that covers all of the registers.
>>
>> V6:
>> - Fixup the loop logic
>> V5:
>> - Convert to only using one memory region
>> V3:
>> - Fix typo
>> V2:
>> - Use memory_region_add_subregion_no_print()
>>
>> hw/core/register.c | 36 ++++++++++++++++++++++++++++++++++++
>> include/hw/register.h | 22 ++++++++++++++++++++++
>> 2 files changed, 58 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index c5a2c78..c68d510 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -231,6 +231,42 @@ 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, uint64_t memory_size)
>> +{
>> + const char *device_prefix = object_get_typename(OBJECT(owner));
>> + RegisterInfoArray *r_array = g_malloc(sizeof(RegisterInfoArray));
>> + int i;
>> +
>> + r_array->r = g_malloc_n(num, sizeof(RegisterInfo *));
>> + r_array->num_elements = num;
>> + r_array->debug = debug_enabled;
>> + r_array->prefix = device_prefix;
>> +
>> + for (i = 0; i < num; i++) {
>> + int index = rae[i].decode.addr / 4;
>> + RegisterInfo *r = &ri[index];
>> +
>> + *r = (RegisterInfo) {
>> + .data = &data[index],
>> + .data_size = sizeof(uint32_t),
>> + .access = &rae[i],
>> + .opaque = owner,
>> + };
>> + register_init(r);
>> +
>> + r_array->r[i] = r;
>> + }
>> +
>> + memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
>> + device_prefix, memory_size);
>> + memory_region_add_subregion(container,
>> + r_array->r[0]->access->decode.addr,
>> + &r_array->mem);
>
> It feels a bit more natural to me to return the MemoryRegion of
> the registers from this function and let the caller put it into
> the container wherever they want (or just use it directly as a
> sysbus MemoryRegion, rather than having this function put it
> into a container passed in by the parent.
Ok, I have changed it to return the MemoryRegion.
>
>> +}
>> +
>> static const TypeInfo register_info = {
>> .name = TYPE_REGISTER,
>> .parent = TYPE_DEVICE,
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index eedd578..c40cf03 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -102,6 +102,8 @@ struct RegisterInfo {
>> */
>>
>> struct RegisterInfoArray {
>> + MemoryRegion mem;
>> +
>> int num_elements;
>> RegisterInfo **r;
>>
>> @@ -172,6 +174,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, uint64_t memory_size);
>
> This doesn't seem to contemplate register arrays which are mostly
> consecutive but have some unimplemented/reserved regions.
I disagree, we only init registers that are described in the device.
Doing work for blank spaces seems unnecessary.
I fixed the asserts() in the read and write functions so
unimplemented/reserved registers will now just read/write 0 and not
waste any look-up or init time.
Thanks,
Alistair
>
>> +
>> /* Define constants for a 32 bit register */
>> #define REG32(reg, addr) \
>> enum { A_ ## reg = (addr) }; \
>> --
>> 2.7.4
>>
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model
2016-06-10 11:16 ` Peter Maydell
@ 2016-06-21 18:34 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 18:34 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 4:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Add a minimal model for the devcfg device which is part of Zynq.
>> This model supports DMA capabilities and interrupt generation.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V5:
>> - Corrections to the device model logic
>>
>> default-configs/arm-softmmu.mak | 1 +
>> hw/dma/Makefile.objs | 1 +
>> hw/dma/xlnx-zynq-devcfg.c | 396 ++++++++++++++++++++++++++++++++++++++
>> include/hw/dma/xlnx-zynq-devcfg.h | 62 ++++++
>> 4 files changed, 460 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 c63cdd0..40f94ec 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 a1abbcf..29b4520 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..9de4e75
>> --- /dev/null
>> +++ b/hw/dma/xlnx-zynq-devcfg.c
>> @@ -0,0 +1,396 @@
>> +/*
>> + * QEMU model of the Xilinx Zynq Devcfg Interface
>> + *
>> + * (C) 2011 PetaLogix Pty Ltd
>> + * (C) 2014 Xilinx Inc.
>> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/dma/xlnx-zynq-devcfg.h"
>> +#include "qemu/bitops.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/dma.h"
>> +
>> +#define FREQ_HZ 900000000
>> +
>> +#define BTT_MAX 0x400
>> +
>> +#ifndef XLNX_ZYNQ_DEVCFG_ERR_DEBUG
>> +#define XLNX_ZYNQ_DEVCFG_ERR_DEBUG 0
>> +#endif
>> +
>> +#define DB_PRINT(fmt, args...) do { \
>> + if (XLNX_ZYNQ_DEVCFG_ERR_DEBUG) { \
>> + qemu_log("%s: " fmt, __func__, ## args); \
>> + } \
>> +} while (0);
>> +
>> +REG32(CTRL, 0x00)
>> + FIELD(CTRL, FORCE_RST, 31, 1) /* Not supported, wr ignored */
>> + FIELD(CTRL, PCAP_PR, 27, 1) /* Forced to 0 on bad unlock */
>> + FIELD(CTRL, PCAP_MODE, 26, 1)
>> + FIELD(CTRL, MULTIBOOT_EN, 24, 1)
>> + FIELD(CTRL, USER_MODE, 15, 1)
>> + FIELD(CTRL, PCFG_AES_FUSE, 12, 1)
>> + FIELD(CTRL, PCFG_AES_EN, 9, 3)
>> + FIELD(CTRL, SEU_EN, 8, 1)
>> + FIELD(CTRL, SEC_EN, 7, 1)
>> + FIELD(CTRL, SPNIDEN, 6, 1)
>> + FIELD(CTRL, SPIDEN, 5, 1)
>> + FIELD(CTRL, NIDEN, 4, 1)
>> + FIELD(CTRL, DBGEN, 3, 1)
>> + FIELD(CTRL, DAP_EN, 0, 3)
>> +
>> +REG32(LOCK, 0x04)
>> + #define AES_FUSE_LOCK 4
>> + #define AES_EN_LOCK 3
>> + #define SEU_LOCK 2
>> + #define SEC_LOCK 1
>> + #define DBG_LOCK 0
>
> Indented #defines are a bit weird.
It makes this a lot easier to read though.
>
>> +
>> +/* mapping bits in R_LOCK to what they lock in R_CTRL */
>> +static const uint32_t lock_ctrl_map[] = {
>> + [AES_FUSE_LOCK] = R_CTRL_PCFG_AES_FUSE_MASK,
>> + [AES_EN_LOCK] = R_CTRL_PCFG_AES_EN_MASK,
>> + [SEU_LOCK] = R_CTRL_SEU_EN_MASK,
>> + [SEC_LOCK] = R_CTRL_SEC_EN_MASK,
>> + [DBG_LOCK] = R_CTRL_SPNIDEN_MASK | R_CTRL_SPIDEN_MASK |
>> + R_CTRL_NIDEN_MASK | R_CTRL_DBGEN_MASK |
>> + R_CTRL_DAP_EN_MASK,
>> +};
>> +
>> +REG32(CFG, 0x08)
>> + FIELD(CFG, RFIFO_TH, 10, 2)
>> + FIELD(CFG, WFIFO_TH, 8, 2)
>> + FIELD(CFG, RCLK_EDGE, 7, 1)
>> + FIELD(CFG, WCLK_EDGE, 6, 1)
>> + FIELD(CFG, DISABLE_SRC_INC, 5, 1)
>> + FIELD(CFG, DISABLE_DST_INC, 4, 1)
>> +#define R_CFG_RESET 0x50B
>> +
>> +REG32(INT_STS, 0x0C)
>> + FIELD(INT_STS, PSS_GTS_USR_B, 31, 1)
>> + FIELD(INT_STS, PSS_FST_CFG_B, 30, 1)
>> + FIELD(INT_STS, PSS_CFG_RESET_B, 27, 1)
>> + FIELD(INT_STS, RX_FIFO_OV, 18, 1)
>> + FIELD(INT_STS, WR_FIFO_LVL, 17, 1)
>> + FIELD(INT_STS, RD_FIFO_LVL, 16, 1)
>> + FIELD(INT_STS, DMA_CMD_ERR, 15, 1)
>> + FIELD(INT_STS, DMA_Q_OV, 14, 1)
>> + FIELD(INT_STS, DMA_DONE, 13, 1)
>> + FIELD(INT_STS, DMA_P_DONE, 12, 1)
>> + FIELD(INT_STS, P2D_LEN_ERR, 11, 1)
>> + FIELD(INT_STS, PCFG_DONE, 2, 1)
>> + #define R_INT_STS_RSVD ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
>> +
>> +REG32(INT_MASK, 0x10)
>> +
>> +REG32(STATUS, 0x14)
>> + FIELD(STATUS, DMA_CMD_Q_F, 31, 1)
>> + FIELD(STATUS, DMA_CMD_Q_E, 30, 1)
>> + FIELD(STATUS, DMA_DONE_CNT, 28, 2)
>> + FIELD(STATUS, RX_FIFO_LVL, 20, 5)
>> + FIELD(STATUS, TX_FIFO_LVL, 12, 7)
>> + FIELD(STATUS, PSS_GTS_USR_B, 11, 1)
>> + FIELD(STATUS, PSS_FST_CFG_B, 10, 1)
>> + FIELD(STATUS, PSS_CFG_RESET_B, 5, 1)
>> +
>> +REG32(DMA_SRC_ADDR, 0x18)
>> +REG32(DMA_DST_ADDR, 0x1C)
>> +REG32(DMA_SRC_LEN, 0x20)
>> +REG32(DMA_DST_LEN, 0x24)
>> +REG32(ROM_SHADOW, 0x28)
>> +REG32(SW_ID, 0x30)
>> +REG32(UNLOCK, 0x34)
>> +
>> +#define R_UNLOCK_MAGIC 0x757BDF0D
>> +
>> +REG32(MCTRL, 0x80)
>> + FIELD(MCTRL, PS_VERSION, 28, 4)
>> + FIELD(MCTRL, PCFG_POR_B, 8, 1)
>> + FIELD(MCTRL, INT_PCAP_LPBK, 4, 1)
>> + FIELD(MCTRL, QEMU, 3, 1)
>> +
>> +static void xlnx_zynq_devcfg_update_ixr(XlnxZynqDevcfg *s)
>> +{
>> + qemu_set_irq(s->irq, ~s->regs[R_INT_MASK] & s->regs[R_INT_STS]);
>> +}
>> +
>> +static void xlnx_zynq_devcfg_reset(DeviceState *dev)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(dev);
>> + int i;
>> +
>> + for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
>> + register_reset(&s->regs_info[i]);
>> + }
>> +}
>> +
>> +static void xlnx_zynq_devcfg_dma_go(XlnxZynqDevcfg *s)
>> +{
>> + do {
>> + uint8_t buf[BTT_MAX];
>> + XlnxZynqDevcfgDMACmd *dmah = s->dma_cmd_fifo;
>> + uint32_t btt = BTT_MAX;
>> + bool loopback = s->regs[R_MCTRL] & R_MCTRL_INT_PCAP_LPBK_MASK;
>> +
>> + btt = MIN(btt, dmah->src_len);
>> + if (loopback) {
>> + btt = MIN(btt, dmah->dest_len);
>> + }
>> + DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
>> + dma_memory_read(&address_space_memory, dmah->src_addr, buf, btt);
>> + dmah->src_len -= btt;
>> + dmah->src_addr += btt;
>> + if (loopback) {
>> + 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) {
>
> If this is true on entry to the function then you'll do a
> dma_memory_read/write of 0 bytes, which is a bit odd.
I added an extra check.
>
>> + 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);
>
> Isn't this a longwinded way to say "sizeof(s->dma_cmd_fifo)" ?
It is, I fixed it to just use sizeof(s->dma_cmd_fifo).
>
>> + }
>> + xlnx_zynq_devcfg_update_ixr(s);
>> + } while (s->dma_cmd_fifo_num);
>> +}
>> +
>> +static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + xlnx_zynq_devcfg_update_ixr(s);
>> +}
>> +
>> +static uint64_t r_ctrl_pre_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
>> + if (s->regs[R_LOCK] & 1 << i) {
>> + val &= ~lock_ctrl_map[i];
>> + val |= lock_ctrl_map[i] & s->regs[R_CTRL];
>> + }
>> + }
>> + return val;
>> +}
>> +
>> +static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + const char *device_prefix = object_get_typename(OBJECT(reg->opaque));
>> + uint32_t aes_en = 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",
>> + device_prefix);
>> + }
>> +}
>> +
>> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> + const char *device_prefix = object_get_typename(OBJECT(s));
>> +
>> + if (val == R_UNLOCK_MAGIC) {
>> + DB_PRINT("successful unlock\n");
>> + /* 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", device_prefix);
>> + s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
>> + s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
>> + /* core becomes inaccessible */
>> + memory_region_set_enabled(&s->iomem, false);
>> + }
>> +}
>> +
>> +static uint64_t r_lock_pre_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + /* once bits are locked they stay locked */
>> + return s->regs[R_LOCK] | val;
>> +}
>> +
>> +static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> +
>> + s->dma_cmd_fifo[s->dma_cmd_fifo_num] = (XlnxZynqDevcfgDMACmd) {
>> + .src_addr = s->regs[R_DMA_SRC_ADDR] & ~0x3UL,
>> + .dest_addr = s->regs[R_DMA_DST_ADDR] & ~0x3UL,
>> + .src_len = s->regs[R_DMA_SRC_LEN] << 2,
>> + .dest_len = s->regs[R_DMA_DST_LEN] << 2,
>> + };
>> + s->dma_cmd_fifo_num++;
>> + DB_PRINT("dma transfer started; %d total transfers pending\n",
>> + s->dma_cmd_fifo_num);
>> + xlnx_zynq_devcfg_dma_go(s);
>> +}
>> +
>> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
>> + { .name = "CTRL", .decode.addr = A_CTRL,
>> + .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
>> + .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
>> + .pre_write = r_ctrl_pre_write,
>> + .post_write = r_ctrl_post_write,
>> + },
>> + { .name = "LOCK", .decode.addr = A_LOCK,
>> + .rsvd = MAKE_64BIT_MASK(5, 64 - 5),
>> + .pre_write = r_lock_pre_write,
>> + },
>
> Hang on, isn't this actually a sparse register info array?
> I thought the core register code insisted the arrays had an
> entry for every address, so either they're buggy or this is
> or I misread the earlier patches...
I fixed the earlier patches, sorry about that.
>
>> + { .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 = MAKE_64BIT_MASK(27, 64 - 27) },
>> + { .name = "DMA_DST_LEN", .decode.addr = A_DMA_DST_LEN,
>> + .ro = MAKE_64BIT_MASK(27, 64 - 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,
>
> You don't need to specify minimum_version_id_old unless
> you are providing a load_state_old function.
Removed.
Thanks,
Alistair
>
>> + .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,
>> + XLNX_ZYNQ_DEVCFG_R_MAX);
>> + 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
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model
2016-06-10 11:19 ` Peter Maydell
@ 2016-06-21 18:36 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 18:36 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 4:19 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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>
>> ---
>> V4:
>> - Small corrections to the device model logic
>>
>> 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 98b17c9..ffea3be 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -293,6 +293,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);
>
> Why do we need to do this?
I'm not sure why this is here, removing it.
Thanks,
Alistair
>
>> + qdev_init_nofail(dev);
>> + busdev = SYS_BUS_DEVICE(dev);
>> + sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
>> + sysbus_mmio_map(busdev, 0, 0xF8007000);
>> +
>> zynq_binfo.ram_size = ram_size;
>> zynq_binfo.kernel_filename = kernel_filename;
>> zynq_binfo.kernel_cmdline = kernel_cmdline;
>> --
>> 2.7.4
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out
2016-06-10 11:48 ` Peter Maydell
@ 2016-06-21 19:05 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 19:05 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 4:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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 db41aa1..e3015d2 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);
>> +}
>
> This appears to be identical to the existing function
> qdev_get_gpio_out_connector() ?
It is, sorry about that. That function must have been added since the
first patch series.
I have just removed this patch.
Thanks,
Alistair
>
>> +
>> +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 1ce02b2..0e216af 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -285,6 +285,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);
>
> Doc comments for new global functions would be nice.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper
2016-06-21 18:25 ` Alistair Francis
@ 2016-06-21 19:45 ` Peter Maydell
2016-06-21 22:21 ` Alistair Francis
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2016-06-21 19:45 UTC (permalink / raw)
To: Alistair Francis
Cc: Edgar Iglesias, QEMU Developers, Peter Crosthwaite,
Edgar E. Iglesias, Alex Bennée, Andreas Färber,
KONRAD Frédéric
On 21 June 2016 at 19:25, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Fri, Jun 10, 2016 at 4:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> +/**
>>> + * 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, uint64_t memory_size);
>>
>> This doesn't seem to contemplate register arrays which are mostly
>> consecutive but have some unimplemented/reserved regions.
>
> I disagree, we only init registers that are described in the device.
> Doing work for blank spaces seems unnecessary.
It says "consecutive registers" -- if that doesn't mean
"all these registers have to be in a single block what
does it mean?
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper
2016-06-21 19:45 ` Peter Maydell
@ 2016-06-21 22:21 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 22:21 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Tue, Jun 21, 2016 at 12:45 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 21 June 2016 at 19:25, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Fri, Jun 10, 2016 at 4:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> +/**
>>>> + * 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, uint64_t memory_size);
>>>
>>> This doesn't seem to contemplate register arrays which are mostly
>>> consecutive but have some unimplemented/reserved regions.
>>
>> I disagree, we only init registers that are described in the device.
>> Doing work for blank spaces seems unnecessary.
>
> It says "consecutive registers" -- if that doesn't mean
> "all these registers have to be in a single block what
> does it mean?
You are right, the comment is wrong. I have removed consecutive from
the comment.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API
2016-06-10 11:52 ` Peter Maydell
@ 2016-06-21 22:34 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2016-06-21 22:34 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Edgar Iglesias, QEMU Developers,
Peter Crosthwaite, Edgar E. Iglesias, Alex Bennée,
Andreas Färber, KONRAD Frédéric
On Fri, Jun 10, 2016 at 4:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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>
>
> I'm definitely not convinced of the utility of this. I think
> almost all devices don't have registers with bits which map
> 1:1 to GPIO lines like this, and the few devices which do can
> easily enough just implement them by hand. GPIOs are (in my
> view) a device level concept, not a register level concept,
> and I think they're better implemented at the device level.
I disagree, I think there are a lot of times where they are useful,
especially as SoCs become more complex and interlinked.
This device is a good example:
https://github.com/Xilinx/qemu/commit/880f24ab6ee2f869d199c0d2aecc92c1fbd8b8cb
They are also an optional feature, if someone wants to do it the
current way that is still supported (and sometimes still required).
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2016-06-21 22:34 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-12 22:45 [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 01/13] bitops: Add MAKE_64BIT_MASK macro Alistair Francis
2016-06-09 18:46 ` Peter Maydell
2016-06-13 20:57 ` Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 02/13] register: Add Register API Alistair Francis
2016-06-09 18:55 ` Peter Maydell
2016-06-13 21:01 ` Alistair Francis
2016-05-12 22:45 ` [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue Alistair Francis
2016-06-09 13:08 ` KONRAD Frederic
2016-06-21 16:52 ` Alistair Francis
2016-06-09 19:03 ` Peter Maydell
2016-06-21 0:46 ` Alistair Francis
2016-06-21 6:48 ` Peter Maydell
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 04/13] register: Define REG and FIELD macros Alistair Francis
2016-06-10 10:52 ` Peter Maydell
2016-06-21 17:41 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 05/13] register: QOMify Alistair Francis
2016-06-10 10:55 ` Peter Maydell
2016-06-21 16:49 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper Alistair Francis
2016-06-10 11:02 ` Peter Maydell
2016-06-21 18:25 ` Alistair Francis
2016-06-21 19:45 ` Peter Maydell
2016-06-21 22:21 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 07/13] dma: Add Xilinx Zynq devcfg device model Alistair Francis
2016-06-10 11:16 ` Peter Maydell
2016-06-21 18:34 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 08/13] xilinx_zynq: Connect devcfg to the Zynq machine model Alistair Francis
2016-06-10 11:19 ` Peter Maydell
2016-06-21 18:36 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 09/13] qdev: Define qdev_get_gpio_out Alistair Francis
2016-06-10 11:48 ` Peter Maydell
2016-06-21 19:05 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 10/13] irq: Add opaque setter routine Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 11/13] register: Add GPIO API Alistair Francis
2016-06-10 11:52 ` Peter Maydell
2016-06-21 22:34 ` Alistair Francis
2016-05-12 22:46 ` [Qemu-devel] [PATCH v6 12/13] misc: Introduce ZynqMP IOU SLCR Alistair Francis
2016-06-09 0:30 ` [Qemu-devel] [PATCH v6 00/13] data-driven device registers Alistair Francis
2016-06-10 11:53 ` Peter Maydell
2016-06-13 21:11 ` Alistair Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.