Re: [Qemu-devel] [PATCH 1/3] sun4u: switch to using qdev to instantiate fw_cfg interface

[Mark Cave-Ayland]

On 10/06/17 18:55, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 06/10/2017 10:00 AM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> ---
>>  hw/sparc64/sun4u.c |   10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 69f565d..98ee6f5 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -512,7 +512,15 @@ static void sun4uv_init(MemoryRegion
>> *address_space_mem,
>>                             graphic_width, graphic_height, graphic_depth,
>>                             (uint8_t *)&nd_table[0].macaddr);
>>
>> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
>> +    dev = qdev_create(NULL, TYPE_FW_CFG_IO);
>> +    qdev_prop_set_uint32(dev, "iobase", BIOS_CFG_IOPORT);
>> +    qdev_prop_set_bit(dev, "dma_enabled", false);
>> +    qdev_init_nofail(dev);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    memory_region_add_subregion(get_system_io(), BIOS_CFG_IOPORT,
>> +                                sysbus_mmio_get_region(s, 0));
> 
> Now that you exported TYPE_FW_CFG_IO I think this might be
> useful/cleaner to have that code in an static inlined function in fw_cfg.h:
> 
> DeviceState *fw_cfg_create(uint32_t iobase[, bool dma_enabled]);
> 
> What do you think?
> 
> Anyway:
> 
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

I'd be inclined to leave the patch in its current form unless anyone
strongly needs more wrapper functions. The important part of the
patchset from my perspective is that it brings the fw_cfg MMIO and
ioport implementations a lot closer together.


ATB,

Mark.