Re: [PATCH 10/11] ppc/pnv: move PHB3 initialization to realize time

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> There's nothing special that is being done in
> pnv_chip_power8_instance_init() that can't be done during
> pnv_chip_power8_realize(). Move the PHB creating and phbs[] assignment
> to power8_realize().
>
> We also need to assign a proper phb->chip parent and bus. This is done
> by the PHB itself, in pnv_phb3_realize(), in a similar fashion that user
> created PHB3s are going to do.
>
> After all this we're left with logic that, aside from phb chip
> assignment that are still being done in power8_realize(), behaves the
> same for default and user created PHB3s.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb3.c | 14 ++++++++++++++
>   hw/ppc/pnv.c           | 24 +++++-------------------
>   2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index bda23fd20b..c1c73fb88d 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -998,6 +998,20 @@ static void pnv_phb3_realize(DeviceState *dev, Error 
> **errp)
>           return;
>       }
>   
> +    /*
> +     * We need the chip to parent the PHB to allow the DT
> +     * to build correctly (via pnv_xscom_dt()).
> +     *
> +     * TODO: the PHB should be parented by a PHB3 PEC device.
> +     */
> +    pnv_parent_qom_fixup(OBJECT(phb->chip), OBJECT(phb), phb->phb_id);
> +


Wouldn't we get the same result in a cleaner way by adding the phb as a 
child of the chip in pnv_chip_power8_realize() ? Right next to when the 
PnvPHB3 object pointer is added to the chip8->phbs array

  Fred


> +    /*
> +     * pnv-phb3 buses are child of the main-system-bus, same as
> +     * the chip.
> +     */
> +    pnv_parent_bus_fixup(DEVICE(phb->chip), dev);
> +
>       /* LSI sources */
>       object_property_set_link(OBJECT(&phb->lsis), "xics", OBJECT(pnv),
>                                &error_abort);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d77c90d64a..e4080a98e1 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1130,8 +1130,6 @@ static void pnv_chip_power10_intc_print_info(PnvChip 
> *chip, PowerPCCPU *cpu,
>   static void pnv_chip_power8_instance_init(Object *obj)
>   {
>       Pnv8Chip *chip8 = PNV8_CHIP(obj);
> -    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
> -    int i;
>   
>       object_property_add_link(obj, "xics", TYPE_XICS_FABRIC,
>                                (Object **)&chip8->xics,
> @@ -1145,22 +1143,6 @@ static void pnv_chip_power8_instance_init(Object *obj)
>       object_initialize_child(obj, "occ", &chip8->occ, TYPE_PNV8_OCC);
>   
>       object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
> -
> -    chip8->num_phbs = pcc->num_phbs;
> -
> -    for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
> -
> -        /*
> -         * We need the chip to parent the PHB to allow the DT
> -         * to build correctly (via pnv_xscom_dt()).
> -         *
> -         * TODO: the PHB should be parented by a PEC device.
> -         */
> -        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
> -        chip8->phbs[i] = phb3;
> -    }
> -
>   }
>   
>   static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
> @@ -1286,8 +1268,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, 
> Error **errp)
>                                   &chip8->homer.regs);
>   
>       /* PHB3 controllers */
> +    chip8->num_phbs = pcc->num_phbs;
> +
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb = chip8->phbs[i];
> +        PnvPHB3 *phb = PNV_PHB3(object_new(TYPE_PNV_PHB3));
> +
> +        chip8->phbs[i] = phb;
>   
>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,