40p: don't use legacy fw_cfg_init_mem() function

Message ID 20180810120418.6695-1-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series
  • 40p: don't use legacy fw_cfg_init_mem() function
Related show

Commit Message

Mark Cave-Ayland Aug. 10, 2018, 12:04 p.m.
Instead initialise the device via qdev to allow us to set device properties
directly as required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/prep.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Hervé Poussineau Aug. 10, 2018, 8:18 p.m. | #1
Le 10/08/2018 à 14:04, Mark Cave-Ayland a écrit :
> Instead initialise the device via qdev to allow us to set device properties
> directly as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/ppc/prep.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570d98..9cf4a2adc3 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
>       uint16_t cmos_checksum;
>       PowerPCCPU *cpu;
>       DeviceState *dev;
> -    SysBusDevice *pcihost;
> +    SysBusDevice *pcihost, *s;
>       Nvram *m48t59 = NULL;
>       PCIBus *pci_bus;
>       ISABus *isa_bus;
> @@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
>       }
>   
>       /* Prepare firmware configuration for OpenBIOS */
> -    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
> +    fw_cfg = FW_CFG(dev);
> +    qdev_prop_set_uint32(dev, "data_width", 1);
> +    qdev_prop_set_bit(dev, "dma_enabled", false);
> +    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
> +                              OBJECT(fw_cfg), NULL);
> +    qdev_init_nofail(dev);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(s, 0, CFG_ADDR);
> +    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
>   
>       if (machine->kernel_filename) {
>           /* load kernel */
> 

So, you're inlining fw_cfg_init_mem() and resolving parameters and conditions.
Any reason to do this, as this function is also used in other places?

However,
Acked-by: Hervé Poussineau <hpoussin@reactos.org>

Hervé
Mark Cave-Ayland Aug. 10, 2018, 9:13 p.m. | #2
On 10/08/18 21:18, Hervé Poussineau wrote:

> Le 10/08/2018 à 14:04, Mark Cave-Ayland a écrit :
>> Instead initialise the device via qdev to allow us to set device
>> properties
>> directly as required.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/ppc/prep.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>> index 3401570d98..9cf4a2adc3 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
>>       uint16_t cmos_checksum;
>>       PowerPCCPU *cpu;
>>       DeviceState *dev;
>> -    SysBusDevice *pcihost;
>> +    SysBusDevice *pcihost, *s;
>>       Nvram *m48t59 = NULL;
>>       PCIBus *pci_bus;
>>       ISABus *isa_bus;
>> @@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
>>       }
>>         /* Prepare firmware configuration for OpenBIOS */
>> -    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
>> +    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
>> +    fw_cfg = FW_CFG(dev);
>> +    qdev_prop_set_uint32(dev, "data_width", 1);
>> +    qdev_prop_set_bit(dev, "dma_enabled", false);
>> +    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
>> +                              OBJECT(fw_cfg), NULL);
>> +    qdev_init_nofail(dev);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    sysbus_mmio_map(s, 0, CFG_ADDR);
>> +    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
>>         if (machine->kernel_filename) {
>>           /* load kernel */
>>
> 
> So, you're inlining fw_cfg_init_mem() and resolving parameters and
> conditions.
> Any reason to do this, as this function is also used in other places?

Well, not any more as I've sent patches to remove just about all of the
others.

The problem with using *_init() functions is that you end up needing one
for every combination of parameters that you need, or having to map
function parameters to qdev parameters. And if you're doing that, you
might as well use the qdev parameters directly.

> However,
> Acked-by: Hervé Poussineau <hpoussin@reactos.org>


ATB,

Mark.
David Gibson Aug. 13, 2018, 2 a.m. | #3
On Fri, Aug 10, 2018 at 01:04:18PM +0100, Mark Cave-Ayland wrote:
> Instead initialise the device via qdev to allow us to set device properties
> directly as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-3.1, thanks.

> ---
>  hw/ppc/prep.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570d98..9cf4a2adc3 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
>      uint16_t cmos_checksum;
>      PowerPCCPU *cpu;
>      DeviceState *dev;
> -    SysBusDevice *pcihost;
> +    SysBusDevice *pcihost, *s;
>      Nvram *m48t59 = NULL;
>      PCIBus *pci_bus;
>      ISABus *isa_bus;
> @@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
>      }
>  
>      /* Prepare firmware configuration for OpenBIOS */
> -    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
> +    fw_cfg = FW_CFG(dev);
> +    qdev_prop_set_uint32(dev, "data_width", 1);
> +    qdev_prop_set_bit(dev, "dma_enabled", false);
> +    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
> +                              OBJECT(fw_cfg), NULL);
> +    qdev_init_nofail(dev);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(s, 0, CFG_ADDR);
> +    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
>  
>      if (machine->kernel_filename) {
>          /* load kernel */

Patch

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 3401570d98..9cf4a2adc3 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -706,7 +706,7 @@  static void ibm_40p_init(MachineState *machine)
     uint16_t cmos_checksum;
     PowerPCCPU *cpu;
     DeviceState *dev;
-    SysBusDevice *pcihost;
+    SysBusDevice *pcihost, *s;
     Nvram *m48t59 = NULL;
     PCIBus *pci_bus;
     ISABus *isa_bus;
@@ -799,7 +799,16 @@  static void ibm_40p_init(MachineState *machine)
     }
 
     /* Prepare firmware configuration for OpenBIOS */
-    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
+    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
+    fw_cfg = FW_CFG(dev);
+    qdev_prop_set_uint32(dev, "data_width", 1);
+    qdev_prop_set_bit(dev, "dma_enabled", false);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(fw_cfg), NULL);
+    qdev_init_nofail(dev);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(s, 0, CFG_ADDR);
+    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
 
     if (machine->kernel_filename) {
         /* load kernel */