[v8,6/8] register: Add block initialise helper

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.

V8:
 - Add a finalise function used to free the allocated memory
V7:
 - Return the memory region instead of adding it as a subregion
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    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

On 24 June 2016 at 16:42, 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>
> ---

> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -228,6 +228,50 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      return extract64(read_val, 0, size * 8);
>  }
>
> +RegisterInfoArray *register_init_block32(DeviceState *owner,
> +                                         const RegisterAccessInfo *rae,
> +                                         int num, RegisterInfo *ri,
> +                                         uint32_t *data,
> +                                         const MemoryRegionOps *ops,
> +                                         bool debug_enabled,
> +                                         uint64_t memory_size)
> +{
> +    const char *device_prefix = object_get_typename(OBJECT(owner));
> +    RegisterInfoArray *r_array = g_new0(RegisterInfoArray, 1);
> +    int i;
> +
> +    r_array->r = g_new0(RegisterInfo *, num);
> +    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].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);
> +
> +    return r_array;
> +}
> +
> +void register_finlise_block(RegisterInfoArray *r_array)

"finalize" (typo, and we prefer the -z- spelling in APIs.)

> +/**
> + * This function should be called to cleanup the registers that were initialized
> + * when calling register_init_block32()
> + *
> + * @r_array: An structure containing all of the registers. The caller is in
> + *           charge of cleaning up the memory region (r_array->mem).

What cleanup does the memory region require?

> + */
> +
> +void register_finlise_block(RegisterInfoArray *r_array);
> +
>  /* Define constants for a 32 bit register */
>
>  /* This macro will define A_FOO, for the byte address of a register

thanks
-- PMM

On Mon, Jun 27, 2016 at 7:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2016 at 16:42, 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>
>> ---
>
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -228,6 +228,50 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>>      return extract64(read_val, 0, size * 8);
>>  }
>>
>> +RegisterInfoArray *register_init_block32(DeviceState *owner,
>> +                                         const RegisterAccessInfo *rae,
>> +                                         int num, RegisterInfo *ri,
>> +                                         uint32_t *data,
>> +                                         const MemoryRegionOps *ops,
>> +                                         bool debug_enabled,
>> +                                         uint64_t memory_size)
>> +{
>> +    const char *device_prefix = object_get_typename(OBJECT(owner));
>> +    RegisterInfoArray *r_array = g_new0(RegisterInfoArray, 1);
>> +    int i;
>> +
>> +    r_array->r = g_new0(RegisterInfo *, num);
>> +    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].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);
>> +
>> +    return r_array;
>> +}
>> +
>> +void register_finlise_block(RegisterInfoArray *r_array)
>
> "finalize" (typo, and we prefer the -z- spelling in APIs.)

Sorry it's just a habit.

>
>> +/**
>> + * This function should be called to cleanup the registers that were initialized
>> + * when calling register_init_block32()
>> + *
>> + * @r_array: An structure containing all of the registers. The caller is in
>> + *           charge of cleaning up the memory region (r_array->mem).
>
> What cleanup does the memory region require?

The device init function that calls the register init function will
also call memory_region_add_subregion(). Doesn't it need to call
memory_region_del_subregion() afterwards?

Thanks,

Alistair

>
>> + */
>> +
>> +void register_finlise_block(RegisterInfoArray *r_array);
>> +
>>  /* Define constants for a 32 bit register */
>>
>>  /* This macro will define A_FOO, for the byte address of a register
>
> thanks
> -- PMM
>

On 27 June 2016 at 17:16, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Jun 27, 2016 at 7:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 June 2016 at 16:42, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> +/**
>>> + * This function should be called to cleanup the registers that were initialized
>>> + * when calling register_init_block32()
>>> + *
>>> + * @r_array: An structure containing all of the registers. The caller is in
>>> + *           charge of cleaning up the memory region (r_array->mem).
>>
>> What cleanup does the memory region require?
>
> The device init function that calls the register init function will
> also call memory_region_add_subregion(). Doesn't it need to call
> memory_region_del_subregion() afterwards?

Somebody also needs to call object_unparent() if the MemoryRegion
is part of a lump of memory that's being g_free()d (see docs/memory.txt).
That probably ought to be this code, and we should say that this
function should only be called from the owner device's instance_finalize.

More generally: I think we should be specific, not vague, about
exactly what the caller's responsibilities are [what they are
expected to have done before calling this function; when they
are permitted to call it]; otherwise the callers will likely get
it wrong.

thanks
-- PMM