[PATCHv3,1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM

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.

Am 19.05.2014 15:03, schrieb Mark Cave-Ayland:
> On 08/05/14 15:34, Andreas Färber wrote:
>> Am 19.02.2014 22:39, schrieb Mark Cave-Ayland:
>>> On 19/02/14 13:35, Leandro Dorileo wrote:
>>>>> +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.
>>
>> 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);

This has global effects, so must stay in realize. The rest looks good.

Cheers,
Andreas

>      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,
>  };