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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.