Use fw_cfg_init_io_nodma() instead of open-coding it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sparc64/sun4u.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 82c3e7c855b..6dc9f64b74d 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                            graphic_width, graphic_height, graphic_depth,
                            (uint8_t *)&macaddr);
 
-    dev = qdev_new(TYPE_FW_CFG_IO);
-    qdev_prop_set_bit(dev, "dma_enabled", false);
-    object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
-                                &FW_CFG_IO(dev)->comb_iomem);
-
-    fw_cfg = FW_CFG(dev);
+    fw_cfg = fw_cfg_init_io_nodma(pci_address_space_io(ebus), BIOS_CFG_IOPORT);
     fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)machine->smp.cpus);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)machine->smp.max_cpus);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
-- 
2.51.0

Hi Philippe,

> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 82c3e7c855b..6dc9f64b74d 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>                             graphic_width, graphic_height, graphic_depth,
>                             (uint8_t *)&macaddr);
>  
> -    dev = qdev_new(TYPE_FW_CFG_IO);
> -    qdev_prop_set_bit(dev, "dma_enabled", false);
> -    object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev));

There's another difference: fw_cfg_init_io_nodma() uses `machine` as the
parent and here sun4uv uses `ebus`.

I think maybe one reason to use `ebus` is because...

> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
... because the parent region is managed by ebus.

Perhaps we should add another argument: Object *parent?

> -                                &FW_CFG_IO(dev)->comb_iomem);
> -

Thanks,
Zhao

On 3/12/25 16:51, Zhao Liu wrote:
> Hi Philippe,
> 
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 82c3e7c855b..6dc9f64b74d 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>                              graphic_width, graphic_height, graphic_depth,
>>                              (uint8_t *)&macaddr);
>>   
>> -    dev = qdev_new(TYPE_FW_CFG_IO);
>> -    qdev_prop_set_bit(dev, "dma_enabled", false);
>> -    object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev));
> 
> There's another difference: fw_cfg_init_io_nodma() uses `machine` as the
> parent and here sun4uv uses `ebus`.

Ah yeah I wanted to comment it but forgot :facepalm:

> 
> I think maybe one reason to use `ebus` is because...
> 
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> -    memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
> ... because the parent region is managed by ebus.
> 
> Perhaps we should add another argument: Object *parent?

I thought about it but don't think so, all instances but this one use
the machine container.

I'll improve the description.

> 
>> -                                &FW_CFG_IO(dev)->comb_iomem);
>> -
> 
> Thanks,
> Zhao
>

On 03/12/2025 17:14, Philippe Mathieu-Daudé wrote:

> On 3/12/25 16:51, Zhao Liu wrote:
>> Hi Philippe,
>>
>>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>>> index 82c3e7c855b..6dc9f64b74d 100644
>>> --- a/hw/sparc64/sun4u.c
>>> +++ b/hw/sparc64/sun4u.c
>>> @@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>>                              graphic_width, graphic_height, graphic_depth,
>>>                              (uint8_t *)&macaddr);
>>> -    dev = qdev_new(TYPE_FW_CFG_IO);
>>> -    qdev_prop_set_bit(dev, "dma_enabled", false);
>>> -    object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev));
>>
>> There's another difference: fw_cfg_init_io_nodma() uses `machine` as the
>> parent and here sun4uv uses `ebus`.
> 
> Ah yeah I wanted to comment it but forgot :facepalm:
> 
>>
>> I think maybe one reason to use `ebus` is because...
>>
>>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> -    memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
>>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
>> ... because the parent region is managed by ebus.
>>
>> Perhaps we should add another argument: Object *parent?
> 
> I thought about it but don't think so, all instances but this one use
> the machine container.
> 
> I'll improve the description.

The reason that the fw_cfg device lives under ebus on sun4u is because the ebus 
device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped into I/O 
address space along with other ISA devices. I'm not sure that setting the parent to 
the machine is the right thing to do here.


ATB,

Mark.