From patchwork Mon May 19 13:03:47 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Mark Cave-Ayland X-Patchwork-Id: 350239 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 77CE6140082 for ; Mon, 19 May 2014 23:06:58 +1000 (EST) Received: from localhost ([::1]:47955 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmNHM-0000XG-9o for incoming@patchwork.ozlabs.org; Mon, 19 May 2014 09:06:56 -0400 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 Received: from 5ec2b707.skybroadband.com ([94.194.183.7] helo=[192.168.1.78]) by s16892447.onlinehome-server.info with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1WmNGZ-0002q6-Oa; Mon, 19 May 2014 14:06:09 +0100 Message-ID: <537A0133.8010609@ilande.co.uk> Date: Mon, 19 May 2014 14:03:47 +0100 From: Mark Cave-Ayland User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 MIME-Version: 1.0 To: =?ISO-8859-1?Q?Andreas_F=E4rber?= , Leandro Dorileo 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> X-SA-Exim-Connect-IP: 94.194.183.7 X-SA-Exim-Mail-From: mark.cave-ayland@ilande.co.uk X-SA-Exim-Version: 4.2.1 (built Sun, 08 Jan 2012 02:45:44 +0000) X-SA-Exim-Scanned: Yes (on s16892447.onlinehome-server.info) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 82.165.15.123 Cc: Peter Maydell , qemu-devel@nongnu.org, Blue Swirl , Bob Breuer , Anthony Liguori , Artyom Tarasenko Subject: Re: [Qemu-devel] [PATCHv3 1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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. 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. --- 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);