From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmNGx-0000G4-7O for qemu-devel@nongnu.org; Mon, 19 May 2014 09:06:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WmNGo-0008FE-Qg for qemu-devel@nongnu.org; Mon, 19 May 2014 09:06:31 -0400 Received: from s16892447.onlinehome-server.info ([82.165.15.123]:46849) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmNGo-0008CW-LO for qemu-devel@nongnu.org; Mon, 19 May 2014 09:06:22 -0400 Message-ID: <537A0133.8010609@ilande.co.uk> Date: Mon, 19 May 2014 14:03:47 +0100 From: Mark Cave-Ayland MIME-Version: 1.0 References: <1392800720-2765-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1392800720-2765-2-git-send-email-mark.cave-ayland@ilande.co.uk> <20140219133502.GA20305@dorilex> <5305247D.7060705@ilande.co.uk> <536B95E4.5020009@suse.de> In-Reply-To: <536B95E4.5020009@suse.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCHv3 1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Andreas_F=E4rber?= , Leandro Dorileo Cc: Peter Maydell , qemu-devel@nongnu.org, Blue Swirl , Bob Breuer , Anthony Liguori , Artyom Tarasenko On 08/05/14 15:34, Andreas Färber wrote: > Hi, > > Am 19.02.2014 22:39, schrieb Mark Cave-Ayland: >> On 19/02/14 13:35, Leandro Dorileo wrote: >> >> Hi Leandro, >> >>>> +static void cg3_realizefn(DeviceState *dev, Error **errp) >>>> +{ >>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>>> + CG3State *s = CG3(dev); >>>> + int ret; >>>> + char *fcode_filename; >>>> + >>>> + /* FCode ROM */ >>>> + memory_region_init_ram(&s->rom, NULL, "cg3.prom", >>>> FCODE_MAX_ROM_SIZE); >>>> + vmstate_register_ram_global(&s->rom); >>>> + memory_region_set_readonly(&s->rom, true); >>>> + sysbus_init_mmio(sbd,&s->rom); >>>> + >>> >>> >>> I think this initialization code could be done in a SysBusDeviceClass >>> init operation, >>> don't you think? >> >> I think it's possible since these MemoryRegions don't depend upon >> properties, but I leave that to Andres who seems reasonably happy with >> the patchset in its current form. > > Just seeing this... > > memory_region_init_ram() and sysbus_init_mmio() could indeed be moved to > an .instance_init function, given that FCODE_MAX_ROM_SIZE is constant. > The others no. It makes a difference when considering reentrancy of the > property code via qom-set (just posted a patchset that makes playing > with that easier), although there's probably more corner cases to > consider. Could either of you follow up with a cleanup? Is something like this correct? It also seems that the register I/O region initialisation can get moved into the initfn since that doesn't depend on any properties either. If it looks good, I'll spin up a proper patchset that does the same to TCX too. --- a/hw/display/cg3.c +++ b/hw/display/cg3.c @@ -274,19 +274,30 @@ static const GraphicHwOps cg3_ops = { .gfx_update = cg3_update_display, }; -static void cg3_realizefn(DeviceState *dev, Error **errp) +static void cg3_initfn(Object *obj) { - SysBusDevice *sbd = SYS_BUS_DEVICE(dev); - CG3State *s = CG3(dev); - int ret; - char *fcode_filename; + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); + CG3State *s = CG3(obj); /* FCode ROM */ memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE); vmstate_register_ram_global(&s->rom); memory_region_set_readonly(&s->rom, true); sysbus_init_mmio(sbd, &s->rom); + + memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg", + CG3_REG_SIZE); + sysbus_init_mmio(sbd, &s->reg); +} +static void cg3_realizefn(DeviceState *dev, Error **errp) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + CG3State *s = CG3(dev); + int ret; + char *fcode_filename; + + /* FCode ROM */ fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE); if (fcode_filename) { ret = load_image_targphys(fcode_filename, s->prom_addr, @@ -296,10 +307,6 @@ static void cg3_realizefn(DeviceState *dev, Error **errp) } } - memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg", - CG3_REG_SIZE); - sysbus_init_mmio(sbd, &s->reg); - memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size); vmstate_register_ram_global(&s->vram_mem); sysbus_init_mmio(sbd, &s->vram_mem); @@ -374,6 +381,7 @@ static const TypeInfo cg3_info = { .name = TYPE_CG3, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(CG3State), + .instance_init = cg3_initfn, .class_init = cg3_class_init, }; ATB, Mark.