From: "Alex Bennée" <alex.bennee@linaro.org>
To: Alistair Francis <alistair.francis@xilinx.com>
Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org,
qemu-devel@nongnu.org, crosthwaitepeter@gmail.com,
edgar.iglesias@gmail.com, afaerber@suse.de,
fred.konrad@greensocs.com
Subject: Re: [Qemu-devel] [PATCH v4 02/16] register: Add Register API
Date: Fri, 26 Feb 2016 17:13:06 +0000 [thread overview]
Message-ID: <87h9gv75ul.fsf@linaro.org> (raw)
In-Reply-To: <30297902aab7203c57444d3f6a3c0da6aee3ce97.1455055858.git.alistair.francis@xilinx.com>
Alistair Francis <alistair.francis@xilinx.com> writes:
> This API provides some encapsulation of registers and factors our some
> common functionality to common code. Bits of device state (usually MMIO
> registers), often have all sorts of access restrictions and semantics
> associated with them. This API allow you to define what those
> restrictions are on a bit-by-bit basis.
>
> Helper functions are then used to access the register which observe the
> semantics defined by the RegisterAccessInfo struct.
>
> Some features:
> Bits can be marked as read_only (ro field)
> Bits can be marked as write-1-clear (w1c field)
> Bits can be marked as reserved (rsvd field)
> Reset values can be defined (reset)
> Bits can throw guest errors when written certain values (ge0, ge1)
> Bits can throw unimp errors when written certain values (ui0, ui1)
> Bits can be marked clear on read (cor)
> Pre and post action callbacks can be added to read and write ops
> Verbose debugging info can be enabled/disabled
>
> Useful for defining device register spaces in a data driven way. Cuts
> down on a lot of the verbosity and repetition in the switch-case blocks
> in the standard foo_mmio_read/write functions.
>
> Also useful for automated generation of device models from hardware
> design sources.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 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 | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/register.h | 111 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 264 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..7e47df5
> --- /dev/null
> +++ b/hw/core/register.c
> @@ -0,0 +1,152 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2013 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "hw/register.h"
> +#include "hw/qdev.h"
> +#include "qemu/log.h"
> +
> +static inline void register_write_log(int mask, RegisterInfo *reg, int dir,
> + uint64_t val, const char *msg,
> + const char *reason)
> +{
> + qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
> + reg->prefix, reg->access->name, val, msg, dir,
> + reason ? ": " : "", reason ? reason : "");
> +}
This seems unused, I guess the compiler ignores unused inlines through :-/
> +
> +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)
> +{
> + 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", reg->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", reg->prefix, test);
> + }
> +
> + test = val & ac->unimp;
> + if (test) {
> + qemu_log_mask(LOG_UNIMP,
> + "%s:%s writing %#" PRIx64 " to unimplemented bits:" \
> + " %#" PRIx64 "",
> + reg->prefix, reg->access->name, val, ac->unimp);
> + }
> +
> +
> + 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);
A comment wouldn't go amiss here describing the concatenation of
restrictions, RAZ, and ROW bits.
> +
> + if (ac->pre_write) {
> + new_val = ac->pre_write(reg, new_val);
> + }
> +
> + if (reg->debug) {
> + qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, ac->name,
> + new_val);
> + }
> +
> + register_write_val(reg, new_val);
> +
> + if (ac->post_write) {
> + ac->post_write(reg, new_val);
> + }
> +}
> +
> +uint64_t register_read(RegisterInfo *reg)
> +{
> + uint64_t ret;
> + const RegisterAccessInfo *ac;
> +
> + assert(reg);
> +
> + ac = reg->access;
> + if (!ac || !ac->name) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state\n",
> + reg->prefix);
> + return 0;
> + }
> +
> + ret = reg->data ? register_read_val(reg) : ac->reset;
> +
> + register_write_val(reg, ret & ~ac->cor);
> +
> + if (ac->post_read) {
> + ret = ac->post_read(reg, ret);
> + }
> +
> + if (reg->debug) {
> + qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefix,
> + ac->name, ret);
> + }
> +
> + return ret;
> +}
> +
> +void register_reset(RegisterInfo *reg)
> +{
> + g_assert(reg);
> + g_assert(reg->data);
> + g_assert(reg->access);
> +
> + 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..444239c
> --- /dev/null
> +++ b/include/hw/register.h
> @@ -0,0 +1,111 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2013 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef REGISTER_H
> +#define REGISTER_H
> +
> +#include "exec/memory.h"
> +
> +typedef struct RegisterInfo RegisterInfo;
> +typedef struct RegisterAccessInfo RegisterAccessInfo;
> +
> +/**
> + * 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
> + *
> + * @ui1: Bits that when written 1 indicate use of an unimplemented feature
> + * @ui0: Bits that when written 0 indicate use of an unimplemented
> feature
Last two comments don't match actual definition.
> + *
> + * @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;
> +
> + bool debug;
> + const char *prefix;
> +
> + void *opaque;
> +};
> +
> +/**
> + * write a value to a register, subject to its restrictions
> + * @reg: register to write to
> + * @val: value to write
> + * @we: write enable mask
> + */
> +
> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we);
> +
> +/**
> + * read a value from a register, subject to its restrictions
> + * @reg: register to read from
> + * returns: value read
> + */
> +
> +uint64_t register_read(RegisterInfo *reg);
> +
> +/**
> + * reset a register
> + * @reg: register to reset
> + */
> +
> +void register_reset(RegisterInfo *reg);
> +
> +#endif
--
Alex Bennée
next prev parent reply other threads:[~2016-02-26 17:13 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-09 22:14 [Qemu-devel] [PATCH v4 00/16] data-driven device registers Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 01/16] memory: Allow subregions to not be printed by info mtree Alistair Francis
2016-02-26 16:51 ` Alex Bennée
2016-02-26 16:54 ` Peter Maydell
2016-03-08 0:49 ` Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 02/16] register: Add Register API Alistair Francis
2016-02-26 17:13 ` Alex Bennée [this message]
2016-03-08 0:54 ` Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 03/16] register: Add Memory API glue Alistair Francis
2016-02-26 17:25 ` Alex Bennée
2016-03-08 1:18 ` Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 04/16] register: Add support for decoding information Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 05/16] register: Define REG and FIELD macros Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 06/16] register: QOMify Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 07/16] register: Add block initialise helper Alistair Francis
2016-02-29 11:28 ` Alex Bennée
2016-03-08 0:09 ` Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 08/16] bitops: Add ONES macro Alistair Francis
2016-02-29 11:51 ` Alex Bennée
2016-03-04 22:42 ` Alistair Francis
2016-02-09 22:14 ` [Qemu-devel] [PATCH v4 09/16] dma: Add Xilinx Zynq devcfg device model Alistair Francis
2016-02-29 12:20 ` Alex Bennée
2016-03-04 19:41 ` Alistair Francis
2016-02-09 22:15 ` [Qemu-devel] [PATCH v4 11/16] qdev: Define qdev_get_gpio_out Alistair Francis
2016-02-09 22:15 ` [Qemu-devel] [PATCH v4 12/16] qdev: Add qdev_pass_all_gpios API Alistair Francis
2016-02-09 22:15 ` [Qemu-devel] [PATCH v4 13/16] irq: Add opaque setter routine Alistair Francis
2016-02-09 22:15 ` [Qemu-devel] [PATCH v4 14/16] register: Add GPIO API Alistair Francis
2016-02-29 12:22 ` Alex Bennée
2016-03-04 18:47 ` Alistair Francis
2016-02-09 22:15 ` [Qemu-devel] [PATCH v4 15/16] misc: Introduce ZynqMP IOU SLCR Alistair Francis
2016-02-09 22:15 ` [Qemu-devel] [PATCH v4 16/16] xlnx-zynqmp: Connect the " Alistair Francis
2016-02-26 16:15 ` [Qemu-devel] [PATCH v4 00/16] data-driven device registers Alex Bennée
2016-02-29 12:26 ` Alex Bennée
2016-03-03 21:27 ` Alistair Francis
2016-03-08 1:21 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h9gv75ul.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=afaerber@suse.de \
--cc=alistair.francis@xilinx.com \
--cc=crosthwaitepeter@gmail.com \
--cc=edgar.iglesias@gmail.com \
--cc=edgar.iglesias@xilinx.com \
--cc=fred.konrad@greensocs.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).