From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54621) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ajEIc-0007vd-Hz for qemu-devel@nongnu.org; Thu, 24 Mar 2016 19:04:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ajEIb-0004QM-7I for qemu-devel@nongnu.org; Thu, 24 Mar 2016 19:04:18 -0400 Received: from mail-oi0-x242.google.com ([2607:f8b0:4003:c06::242]:34456) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ajEIa-0004Q9-WC for qemu-devel@nongnu.org; Thu, 24 Mar 2016 19:04:17 -0400 Received: by mail-oi0-x242.google.com with SMTP id f188so7295246oig.1 for ; Thu, 24 Mar 2016 16:04:16 -0700 (PDT) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: <8760wesb6w.fsf@linaro.org> References: <0250cbb0a6b9d18e12369720a47e96772347829c.1457470980.git.alistair.francis@xilinx.com> <8760wesb6w.fsf@linaro.org> From: Alistair Francis Date: Thu, 24 Mar 2016 16:03:46 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 03/15] register: Add Memory API glue 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, Mar 22, 2016 at 9:56 AM, Alex Benn=C3=A9e = wrote: > > Alistair Francis writes: > >> 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 >> Signed-off-by: Alistair Francis >> --- >> V5: >> - Convert to using only one memory region >> >> hw/core/register.c | 70 ++++++++++++++++++++++++++++++++++++++++++++= +++++++ >> include/hw/register.h | 51 +++++++++++++++++++++++++++++++++++++ >> 2 files changed, 121 insertions(+) >> >> diff --git a/hw/core/register.c b/hw/core/register.c >> index 1656f71..e1cd0c4 100644 >> --- a/hw/core/register.c >> +++ b/hw/core/register.c >> @@ -146,3 +146,73 @@ void register_reset(RegisterInfo *reg) >> >> register_write_val(reg, reg->access->reset); >> } >> + >> +static inline void register_write_memory(void *opaque, hwaddr addr, >> + uint64_t value, unsigned size,= bool be) >> +{ >> + RegisterInfoArray *reg_array =3D opaque; >> + RegisterInfo *reg =3D NULL; >> + uint64_t we =3D ~0; >> + int i, shift =3D 0; >> + >> + for (i =3D 0; i < reg_array->num_elements; i++) { >> + if (reg_array->r[i]->access->decode.addr =3D=3D addr) { >> + reg =3D reg_array->r[i]; >> + break; >> + } >> + } >> + assert(reg); >> + >> + /* Generate appropriate write enable mask and shift values */ >> + if (reg->data_size < size) { >> + we =3D MAKE_64BIT_MASK(0, reg->data_size * 8); >> + shift =3D 8 * (be ? reg->data_size - size : 0); >> + } else if (reg->data_size >=3D size) { >> + we =3D MAKE_64BIT_MASK(0, size * 8); >> + } >> + >> + register_write(reg, value << shift, we << shift); >> +} >> + >> +void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value= , >> + unsigned size) >> +{ >> + register_write_memory(opaque, addr, value, size, true); >> +} >> + >> + >> +void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value= , >> + unsigned size) >> +{ >> + register_write_memory(opaque, addr, value, size, false); >> +} >> + >> +static inline uint64_t register_read_memory(void *opaque, hwaddr addr, >> + unsigned size, bool be) >> +{ >> + RegisterInfoArray *reg_array =3D opaque; >> + RegisterInfo *reg =3D NULL; >> + int i, shift; >> + >> + for (i =3D 0; i < reg_array->num_elements; i++) { >> + if (reg_array->r[i]->access->decode.addr =3D=3D addr) { >> + reg =3D reg_array->r[i]; >> + break; >> + } >> + } >> + assert(reg); >> + >> + shift =3D 8 * (be ? reg->data_size - size : 0); >> + >> + return (register_read(reg) >> shift) & MAKE_64BIT_MASK(0, size * 8)= ; >> +} >> + >> +uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned si= ze) >> +{ >> + return register_read_memory(opaque, addr, size, true); >> +} >> + >> +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned si= ze) >> +{ >> + return register_read_memory(opaque, addr, size, false); >> +} >> diff --git a/include/hw/register.h b/include/hw/register.h >> index baa08f5..726a914 100644 >> --- a/include/hw/register.h >> +++ b/include/hw/register.h >> @@ -15,6 +15,7 @@ >> >> typedef struct RegisterInfo RegisterInfo; >> typedef struct RegisterAccessInfo RegisterAccessInfo; >> +typedef struct RegisterInfoArray RegisterInfoArray; >> >> /** >> * Access description for a register that is part of guest accessible d= evice >> @@ -51,6 +52,10 @@ struct RegisterAccessInfo { >> void (*post_write)(RegisterInfo *reg, uint64_t val); >> >> uint64_t (*post_read)(RegisterInfo *reg, uint64_t val); >> + >> + struct { >> + hwaddr addr; >> + } decode; >> }; >> >> /** >> @@ -82,6 +87,26 @@ struct RegisterInfo { >> }; >> >> /** >> + * This structure is used to group all of the individual registers whic= h are >> + * modeled using the RegisterInfo strucutre. >> + * >> + * @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 { >> + /* */ >> + MemoryRegion mem; > > I never see this used. I thought with the other changes the memory It isn't used here, it is used later on in the series when the register_init_block32() function is added. I can move it back to the patch if you wish. > region enclosed the group of registers. Wouldn't a private mem violate th= at? Yes, you are right. I'll remove the private comment. > >> + >> + /* */ >> + int num_elements; >> + RegisterInfo **r; >> +}; > > Without the extra bits why re-invent GArray? What do you mean? Thanks, Alistair > >> + >> +/** >> * write a value to a register, subject to its restrictions >> * @reg: register to write to >> * @val: value to write >> @@ -105,4 +130,30 @@ uint64_t register_read(RegisterInfo *reg); >> >> void register_reset(RegisterInfo *reg); >> >> +/** >> + * Memory API MMIO write handler that will write to a Register API regi= ster. >> + * _be for big endian variant and _le for little endian. >> + * @opaque: RegisterInfo to write to >> + * @addr: Address to write >> + * @value: Value to write >> + * @size: Number of bytes to write >> + */ >> + >> +void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value= , >> + unsigned size); >> +void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value= , >> + unsigned size); >> + >> +/** >> + * Memory API MMIO read handler that will read from a Register API regi= ster. >> + * _be for big endian variant and _le for little endian. >> + * @opaque: RegisterInfo to read from >> + * @addr: Address to read >> + * @size: Number of bytes to read >> + * returns: Value read from register >> + */ >> + >> +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); >> + >> #endif > > > -- > Alex Benn=C3=A9e >