From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhgyT-0007Yg-7q for qemu-devel@nongnu.org; Wed, 29 May 2013 10:03:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UhgyM-0002zU-37 for qemu-devel@nongnu.org; Wed, 29 May 2013 10:03:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57134) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhgyL-0002zN-RR for qemu-devel@nongnu.org; Wed, 29 May 2013 10:03:26 -0400 Message-ID: <51A60A9D.5090003@redhat.com> Date: Wed, 29 May 2013 16:03:09 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <51A5C1AE.6070909@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: blauwirbel@gmail.com, peter.maydell@linaro.org, kraxel@redhat.com, qemu-devel@nongnu.org, edgar.iglesias@gmail.com Il 29/05/2013 15:54, Peter Crosthwaite ha scritto: > Hi Paolo, > > On Wed, May 29, 2013 at 6:51 PM, Paolo Bonzini wrote: >> Il 24/05/2013 07:49, peter.crosthwaite@xilinx.com ha scritto: >>> +static const MemoryRegionOps devcfg_reg_ops = { >>> + .read = register_read_memory_le, >>> + .write = register_write_memory_le, >>> + .endianness = DEVICE_LITTLE_ENDIAN, >>> + .valid = { >>> + .min_access_size = 4, >>> + .max_access_size = 4, >> >> What happens if you have registers of mixed size within the same "bank"? >> > > You Probably should change these access restrictions per-register > accordingly. Can you add a generic .valid.accepts callback for the register API that will check against the size of the register at that offset? > The degenerate case is allowing greater-than-register size or > misaligned accesses, which is dependent on the behaviour of the memory > API. I can do some research, but do you know if the memory API > supports misaligned transactions that span a memory region boundary? I don't really care about that for now. But the way to do that would be to add a .impl.accepts method (right now there's only .valid.accepts). Then you would point the register API's implementation to .impl.accepts, and use .impl.accepts = register_accepts, .valid = { .min_access_size = 1, .max_access_size = 4, } The memory API will do a RMW operation if needed. Won't make much sense with write-1-clear bits and the like, but for simple registers it would work. Paolo > If so then there are no concerns, as the memory API will handle it for > us. If not then I don't see it as an issue as its very rare (at least > in my experience) for registers to support such strange misaligned or > mulit-register accesses. That or we can look into memory API for > answers. > >>> + } >>> +}; >>> + >>> +static void xilinx_devcfg_realize(DeviceState *dev, Error **errp) >>> +{ >>> + XilinxDevcfg *s = XILINX_DEVCFG(dev); >>> + const char *prefix = object_get_canonical_path(OBJECT(dev)); >>> + int i; >>> + >>> + for (i = 0; i < R_MAX; ++i) { >>> + RegisterInfo *r = &s->regs_info[i]; >>> + >>> + *r = (RegisterInfo) { >>> + .data = &s->regs[i], >>> + .data_size = sizeof(uint32_t), >>> + .access = &xilinx_devcfg_regs_info[i], >>> + .debug = XILINX_DEVCFG_ERR_DEBUG, >>> + .prefix = prefix, >>> + .opaque = s, >>> + }; >>> + memory_region_init_io(&r->mem, &devcfg_reg_ops, r, "devcfg-regs", 4); >> >> Could you add a register_init function that does register_reset + >> memory_region_init_io? >> > > I wanted to factor this loop out into a helper as a next step so I > think its already in my todo list, but i'm confused as to why reset > and memory_region_init would bundled together - arent they normally in > Device::reset and Device::realize (or Object::Init) respectively? > > Regards, > Peter > >>> + memory_region_add_subregion(&s->iomem, i * 4, &r->mem); >> >> Paolo >>