[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv3 1/2] sun4m: Add Sun CG3 framebuffer and corres
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCHv3 1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM |
Date: |
Mon, 19 May 2014 19:09:25 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
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,
> };
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg