Re: [Qemu-devel] [PATCH v6 06/13] register: Add block initialise helper

[Peter Maydell]

On 12 May 2016 at 23:46, Alistair Francis <address@hidden> wrote:
> From: Peter Crosthwaite <address@hidden>
>
> Add a helper that will scan a static RegisterAccessInfo Array
> and populate a container MemoryRegion with registers as defined.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> 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