From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49956) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTEJf-0004Uz-Tg for qemu-devel@nongnu.org; Tue, 09 Feb 2016 14:51:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTEJe-0002mk-Qs for qemu-devel@nongnu.org; Tue, 09 Feb 2016 14:51:15 -0500 Received: from mail-ob0-x243.google.com ([2607:f8b0:4003:c01::243]:33408) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTEJe-0002mg-KZ for qemu-devel@nongnu.org; Tue, 09 Feb 2016 14:51:14 -0500 Received: by mail-ob0-x243.google.com with SMTP id o4so9397020obb.0 for ; Tue, 09 Feb 2016 11:51:14 -0800 (PST) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: <87a8n9x3pt.fsf@linaro.org> References: <31e7c7dfd48ae5221f9459c16324a0bcd5660a04.1454115217.git.alistair.francis@xilinx.com> <87a8n9x3pt.fsf@linaro.org> From: Alistair Francis Date: Tue, 9 Feb 2016 11:50:44 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 07/16] register: Add block initialise helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: Edgar Iglesias , Peter Maydell , "qemu-devel@nongnu.org Developers" , Alistair Francis , Peter Crosthwaite , Edgar Iglesias , =?UTF-8?Q?Andreas_F=C3=A4rber?= , =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= On Tue, Feb 9, 2016 at 8:12 AM, Alex Benn=C3=A9e w= rote: > > Alistair Francis writes: > >> From: Peter Crosthwaite >> >> Add a helper that will scan a static RegisterAccessInfo Array >> and populate a container MemoryRegion with registers as defined. >> >> Signed-off-by: Peter Crosthwaite >> Signed-off-by: Alistair Francis >> --- >> V3: >> - Fix typo >> V2: >> - Use memory_region_add_subregion_no_print() >> >> hw/core/register.c | 29 +++++++++++++++++++++++++++++ >> include/hw/register.h | 20 ++++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/hw/core/register.c b/hw/core/register.c >> index 939f398..4d7dd95 100644 >> --- a/hw/core/register.c >> +++ b/hw/core/register.c >> @@ -258,6 +258,35 @@ uint64_t register_read_memory_le(void *opaque, hwad= dr 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 MemoryRegionO= ps *ops, >> + bool debug_enabled) > > Are there going to be register_init_block8, 16 and 64 variants? Perhaps > this should be a generic register_block function that takes the size and > skip of the registers? I think at some point there will be benefits in supporting different size registers. What do you mean size and skip? > >> +{ >> + const char *debug_prefix =3D object_get_typename(OBJECT(owner)); >> + int i; >> + >> + for (i =3D 0; i < num; i++) { >> + int index =3D rae[i].decode.addr / 4; >> + RegisterInfo *r =3D &ri[index]; >> + >> + *r =3D (RegisterInfo) { >> + .data =3D &data[index], >> + .data_size =3D sizeof(uint32_t), >> + .access =3D &rae[i], >> + .debug =3D debug_enabled, >> + .prefix =3D debug_prefix, >> + .opaque =3D owner, >> + }; >> + register_init(r); >> + >> + memory_region_init_io(&r->mem, OBJECT(owner), ops, r, r->access= ->name, >> + sizeof(uint32_t)); >> + memory_region_add_subregion_no_print(container, >> + r->access->decode.addr, >> &r->mem); > > Why a memory region for every register? Couldn't we have a shared region > for the whole block and handle dispatching in the register code? This is something else that Peter would know better. Looking at it I don't see any reason that it couldn't be an array or RegisterInfo and use that with one memory region. It would make the read/write logic more complex though. I would want a consensus to do it that way before I re-write it though. Thanks, Alistair > >> + } >> +} >> + >> static const TypeInfo register_info =3D { >> .name =3D TYPE_REGISTER, >> .parent =3D TYPE_DEVICE, >> diff --git a/include/hw/register.h b/include/hw/register.h >> index 3316458..30dedbf 100644 >> --- a/include/hw/register.h >> +++ b/include/hw/register.h >> @@ -182,6 +182,26 @@ void register_write_memory_le(void *opaque, hwaddr = addr, uint64_t value, >> uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned si= ze); >> uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned si= ze); >> >> +/** >> + * Init a block of consecutive registers into a container MemoryRegion.= A >> + * number of constant register definitions are parsed to create a corre= sponding >> + * 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 MemoryRegionO= ps *ops, >> + bool debug_enabled); >> + >> /* Define constants for a 32 bit register */ >> #define REG32(reg, addr) = \ >> enum { A_ ## reg =3D (addr) }; = \ > > > -- > Alex Benn=C3=A9e >