[v7,06/12] 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.

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    | 36 ++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 24 ++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

On 22 June 2016 at 21:23, 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>
> ---
> The reason that I'm not using GArray is because the array needs to store
> the memory region that covers all of the registers.
>
> 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    | 36 ++++++++++++++++++++++++++++++++++++
>  include/hw/register.h | 24 ++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 41c11c8..f32faac 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -224,6 +224,42 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      return extract64(read_val, 0, size * 8);
>  }
>
> +MemoryRegion *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_malloc(sizeof(RegisterInfoArray));

Prefer g_new0(RegisterInfoArray, 1)

> +    int i;
> +
> +    r_array->r = g_malloc_n(num, sizeof(RegisterInfo *));

Prefer 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->mem;

This API doesn't seem to have any provision for being able to
free the RegisterInfoArray later ? (Consider hotpluggable devices
which need to be able to free everything on hot-unplug.)
> +
>  static const TypeInfo register_info = {
>      .name  = TYPE_REGISTER,
>      .parent = TYPE_DEVICE,
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 18a63df..104d381 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -100,6 +100,8 @@ struct RegisterInfo {
>   */
>
>  struct RegisterInfoArray {
> +    MemoryRegion mem;
> +
>      int num_elements;
>      RegisterInfo **r;
>
> @@ -166,6 +168,28 @@ void register_write_memory(void *opaque, hwaddr addr, uint64_t value,
>
>  uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
>
> +/**
> + * Init a block of 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
> + * @ops: Memory region ops to access registers.
> + * @debug enabled: turn on/off verbose debug information

It's not clear to me which of these are input arguments to the
function, and which are things that the function initializes
(I think 'ri' is purely output?)

> + * returns: A pointer to an initalised memory region that the caller should

"initialized"

> + *          add to a container.
> + */
> +
> +MemoryRegion *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);
> +
>  /* Define constants for a 32 bit register */
>
>  /* This macro will define A_FOO, for the byte address of a register
> --
> 2.7.4

thanks
-- PMM

On Thu, Jun 23, 2016 at 5:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 June 2016 at 21:23, 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>
>> ---
>> The reason that I'm not using GArray is because the array needs to store
>> the memory region that covers all of the registers.
>>
>> 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    | 36 ++++++++++++++++++++++++++++++++++++
>>  include/hw/register.h | 24 ++++++++++++++++++++++++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index 41c11c8..f32faac 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -224,6 +224,42 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>>      return extract64(read_val, 0, size * 8);
>>  }
>>
>> +MemoryRegion *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_malloc(sizeof(RegisterInfoArray));
>
> Prefer g_new0(RegisterInfoArray, 1)
>
>> +    int i;
>> +
>> +    r_array->r = g_malloc_n(num, sizeof(RegisterInfo *));
>
> Prefer g_new0(RegisterInfo *, num)

Ok, I have fixed these.

>
>> +    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->mem;
>
> This API doesn't seem to have any provision for being able to
> free the RegisterInfoArray later ? (Consider hotpluggable devices
> which need to be able to free everything on hot-unplug.)

I have added a finalise function which can be used to free the register array.

This meant I had to change register_init_block32() to return
RegisterInfoArray instead of a MemoryRegion.

>> +
>>  static const TypeInfo register_info = {
>>      .name  = TYPE_REGISTER,
>>      .parent = TYPE_DEVICE,
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 18a63df..104d381 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -100,6 +100,8 @@ struct RegisterInfo {
>>   */
>>
>>  struct RegisterInfoArray {
>> +    MemoryRegion mem;
>> +
>>      int num_elements;
>>      RegisterInfo **r;
>>
>> @@ -166,6 +168,28 @@ void register_write_memory(void *opaque, hwaddr addr, uint64_t value,
>>
>>  uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
>>
>> +/**
>> + * Init a block of 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
>> + * @ops: Memory region ops to access registers.
>> + * @debug enabled: turn on/off verbose debug information
>
> It's not clear to me which of these are input arguments to the
> function, and which are things that the function initializes
> (I think 'ri' is purely output?)

The function expects everything to already be initialised, it just
wires them up.

ri should already be allocated, this function just sets up some pointers to it.

I added a comment in the description saying that ri and data must
already be allocated. Is that enough?

>
>> + * returns: A pointer to an initalised memory region that the caller should
>
> "initialized"

Fixed.

Thanks,

Alistair

>
>> + *          add to a container.
>> + */
>> +
>> +MemoryRegion *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);
>> +
>>  /* Define constants for a 32 bit register */
>>
>>  /* This macro will define A_FOO, for the byte address of a register
>> --
>> 2.7.4
>
> thanks
> -- PMM
>

On 23 June 2016 at 18:29, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Thu, Jun 23, 2016 at 5:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> It's not clear to me which of these are input arguments to the
>> function, and which are things that the function initializes
>> (I think 'ri' is purely output?)
>
> The function expects everything to already be initialised, it just
> wires them up.
>
> ri should already be allocated, this function just sets up some pointers to it.
>
> I added a comment in the description saying that ri and data must
> already be allocated. Is that enough?

Does the caller have to initialize the array that ri points to
before it calls this?

thanks
-- PMM

On Thu, Jun 23, 2016 at 11:02 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 June 2016 at 18:29, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Thu, Jun 23, 2016 at 5:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> It's not clear to me which of these are input arguments to the
>>> function, and which are things that the function initializes
>>> (I think 'ri' is purely output?)
>>
>> The function expects everything to already be initialised, it just
>> wires them up.
>>
>> ri should already be allocated, this function just sets up some pointers to it.
>>
>> I added a comment in the description saying that ri and data must
>> already be allocated. Is that enough?
>
> Does the caller have to initialize the array that ri points to
> before it calls this?

All the caller has to do is ensure that the array exists and is large
enough for the number of registers.

Thanks,

Alistair

>
> thanks
> -- PMM
>