On Thu, Jun 23, 2016 at 5:21 AM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 22 June 2016 at 21:23, Alistair Francis <alistair.fran...@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.crosthwa...@xilinx.com> >> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com> >> --- >> V7: >> - Remove endianess handling >> - Remove assert() and log missing registers >> V6: >> - Add the memory region later >> V5: >> - Convert to using only one memory region >> >> hw/core/register.c | 58 >> +++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/register.h | 43 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 101 insertions(+) >> >> diff --git a/hw/core/register.c b/hw/core/register.c >> index cc067f1..149aebb 100644 >> --- a/hw/core/register.c >> +++ b/hw/core/register.c >> @@ -154,3 +154,61 @@ void register_reset(RegisterInfo *reg) >> >> register_write_val(reg, reg->access->reset); >> } >> + >> +void register_write_memory(void *opaque, hwaddr addr, >> + uint64_t value, unsigned size) >> +{ >> + RegisterInfoArray *reg_array = opaque; >> + RegisterInfo *reg = NULL; >> + uint64_t we = ~0; > > Really ~0 and not ~0ULL ? In any case this shouldn't need > initializing as we will always set it later. > >> + int i; >> + >> + for (i = 0; i < reg_array->num_elements; i++) { >> + if (reg_array->r[i]->access->addr == addr) { >> + reg = reg_array->r[i]; >> + break; >> + } >> + } >> + >> + if (!reg) { >> + qemu_log_mask(LOG_UNIMP, "Write to unimplemented register at " \ >> + "address: %#" PRIx64 "\n", addr); > > Shouldn't this be a LOG_GUEST_ERROR ? > >> + return; >> + } >> + >> + /* Generate appropriate write enable mask */ >> + if (reg->data_size < size) { >> + we = MAKE_64BIT_MASK(0, reg->data_size * 8); >> + } else if (reg->data_size >= size) { > > Why not just "else {" ? > >> + we = MAKE_64BIT_MASK(0, size * 8); >> + } >> + >> + register_write(reg, value, we, reg_array->prefix, >> + reg_array->debug); >> +} >> + >> +uint64_t register_read_memory(void *opaque, hwaddr addr, >> + unsigned size) >> +{ >> + RegisterInfoArray *reg_array = opaque; >> + RegisterInfo *reg = NULL; >> + uint64_t read_val; >> + int i; >> + >> + for (i = 0; i < reg_array->num_elements; i++) { >> + if (reg_array->r[i]->access->addr == addr) { >> + reg = reg_array->r[i]; >> + break; >> + } >> + } >> + >> + if (!reg) { >> + qemu_log_mask(LOG_UNIMP, "Read to unimplemented register at " \ >> + "address: %#" PRIx64 "\n", addr); >> + return 0; >> + } >> + >> + read_val = register_read(reg, reg_array->prefix, reg_array->debug); > > This will silently affect bits of the register outside the > specified size with clear-on-read or other "reads have > side effects" behaviour, which isn't consistent with how > we handle writes. > >> /** >> + * This structure is used to group all of the individual registers which are >> + * modeled using the RegisterInfo strucutre. > > "structure"
Fixed all. Thanks, Alistair > >> + * >> + * @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; >> +}; >> + > > thanks > -- PMM >