Re: [PATCH 16/24] ppc/pnv: Put "*-pnv-chip" and "pnv-xive" on the main system bus

[Markus Armbruster]

Cédric Le Goater <address@hidden> writes:

> On 5/18/20 7:04 AM, Markus Armbruster wrote:
>> pnv_init() creates "power10_v1.0-pnv-chip", "power8_v2.0-pnv-chip",
>> "power8e_v2.1-pnv-chip", "power8nvl_v1.0-pnv-chip", or
>> "power9_v2.0-pnv-chip" sysbus devices in a way that leaves them
>> unplugged.
>> 
>> pnv_chip_power9_instance_init() creates a "pnv-xive" sysbus device in
>> a way that leaves it unplugged.
>> 
>> Create them the common way that puts them into the main system bus.
>> Affects machines powernv8, powernv9, and powernv10.  Visible in "info
>> qtree".  Here's the change for powernv9:
>> 
>>      bus: main-system-bus
>>        type System
>>     +  dev: power9_v2.0-pnv-chip, id ""
>>     +    chip-id = 0 (0x0)
>>     +    ram-start = 0 (0x0)
>>     +    ram-size = 1879048192 (0x70000000)
>>     +    nr-cores = 1 (0x1)
>>     +    cores-mask = 72057594037927935 (0xffffffffffffff)
>>     +    nr-threads = 1 (0x1)
>>     +    num-phbs = 6 (0x6)
>>     +    mmio 000603fc00000000/0000000400000000
>>     [...]
>>     +  dev: pnv-xive, id ""
>>     +    ic-bar = 1692157036462080 (0x6030203100000)
>>     +    vc-bar = 1689949371891712 (0x6010000000000)
>>     +    pc-bar = 1690499127705600 (0x6018000000000)
>>     +    tm-bar = 1692157036986368 (0x6030203180000)
>> 
>> Cc: "Cédric Le Goater" <address@hidden>
>> Cc: David Gibson <address@hidden>
>> Cc: address@hidden
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/ppc/pnv.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index da637822f9..8d4fc8109a 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -818,7 +818,7 @@ static void pnv_init(MachineState *machine)
>>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>>      for (i = 0; i < pnv->num_chips; i++) {
>>          char chip_name[32];
>> -        Object *chip = object_new(chip_typename);
>> +        Object *chip = OBJECT(qdev_create(NULL, chip_typename));
>>  
>>          pnv->chips[i] = PNV_CHIP(chip);
>>  
>> @@ -1317,8 +1317,8 @@ static void pnv_chip_power9_instance_init(Object *obj)
>>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
>>      int i;
>>  
>> -    object_initialize_child(obj, "xive", &chip9->xive, sizeof(chip9->xive),
>> -                            TYPE_PNV_XIVE, &error_abort, NULL);
>> +    sysbus_init_child_obj(obj, "xive", &chip9->xive, sizeof(chip9->xive),
>> +                          TYPE_PNV_XIVE);
>>      object_property_add_alias(obj, "xive-fabric", OBJECT(&chip9->xive),
>>                                "xive-fabric");
>
> OK. But why only XIVE and not all sub-devices of the PnvChip device ? 
>
> Shouldn't they be initialized in the same way, calling sysbus_init_child_obj 
> ? 
No, your code is just fine there.

sysbus_init_child_obj() is a convenience wrapper around
object_initialize_child() and qdev_set_parent_bus().  Only sysbus
devices may use it.  The other sub-devices are not susbus devices:

* TYPE_PNV8_PSI, TYPE_PNV9_PSI, TYPE_PNV10_PSI

  Subtypes of TYPE_PNV_PSI, which is a subtype of TYPE_DEVICE.

* TYPE_PNV8_LPC, TYPE_PNV9_LPC, TYPE_PNV10_LPC

  Subtypes of TYPE_PNV_LPC, which is a subtype of TYPE_DEVICE.

* TYPE_PNV8_OCC, TYPE_PNV9_OCC

  Subtypes of TYPE_PNV_OCC, which is a subtype of TYPE_DEVICE.

* TYPE_PNV8_HOMER, TYPE_PNV9_HOMER

  Subtypes of TYPE_PNV_HOMER, which is a subtype of TYPE_DEVICE.

* TYPE_PNV_PHB4_PEC

  Subtype of TYPE_DEVICE.

* TYPE_PNV_QUAD

  Subtype of TYPE_DEVICE.

Except for:

* TYPE_PNV_PHB3

  Subtype of TYPE_PCIE_HOST_BRIDGE, which is a subtype of
  TYPE_PCI_HOST_BRIDGE, which is a subtype of TYPE_SYS_BUS_DEVICE.

where you use object_initialize_child() and qdev_set_parent_bus()
directly.  Works.  We could perhaps change it to use
sysbus_init_child_obj(), but it would be a waste; my next series will
kill that helper :)