Re: [Qemu-devel] [PATCHv3 1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM

[Mark Cave-Ayland]

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.