On Fri, Feb 26, 2016 at 9:13 AM, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Alistair Francis <alistair.fran...@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.crosthwa...@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.fran...@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.crosthwa...@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 :-/
Good catch, removed.
>
>> +
>> +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.
Agreed, I have added a comment.
>
>> +
>> + 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.crosthwa...@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.
Yep, they were left over. Removed.
Thanks,
Alistair
>
>> + *
>> + * @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
>
