Re: [PATCH 13/14] ppc/pnv: Move realize of PEC stacks under the PEC model

[Frederic Barrat <fbarrat@linux.ibm.com>]

On 02/12/2021 15:42, Cédric Le Goater wrote:
> This change will help us providing support for user created PHB4
> devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/pci-host/pnv_phb4_pec.c | 36 ++++++++++++++++++++++++++++++++----
>   hw/ppc/pnv.c               | 31 +------------------------------
>   2 files changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index dfed2af0f7df..9b081d543057 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -556,6 +556,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
>       PnvPhb4PecState *pec = stack->pec;
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    PnvChip *chip = pec->chip;
> +    uint32_t pec_nest_base;
> +    uint32_t pec_pci_base;
>       char name[64];
>   
>       assert(pec);
> @@ -579,10 +583,34 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
>                             &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
>   
> -    /*
> -     * Let the machine/chip realize the PHB object to customize more
> -     * easily some fields
> -     */
> +    {
> +        Object *obj = OBJECT(&stack->phb);
> +
> +        object_property_set_int(obj, "chip-id", pec->chip_id, &error_fatal);
> +        object_property_set_int(obj, "version", pecc->version, &error_fatal);
> +        object_property_set_int(obj, "device-id", pecc->device_id,
> +                                &error_fatal);
> +        object_property_set_link(obj, "stack", OBJECT(stack),
> +                                 &error_abort);
> +        if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
> +            return;
> +        }
> +    }


Do we really need the extra sub-scope here? It looks off.


> +
> +    pec_nest_base = pecc->xscom_nest_base(pec);
> +    pec_pci_base = pecc->xscom_pci_base(pec);
> +
> +    /* Populate the XSCOM address space. */
> +    pnv_xscom_add_subregion(chip,
> +                            pec_nest_base + 0x40 * (stack->stack_no + 1),
> +                            &stack->nest_regs_mr);
> +    pnv_xscom_add_subregion(chip,
> +                            pec_pci_base + 0x40 * (stack->stack_no + 1),
> +                            &stack->pci_regs_mr);
> +    pnv_xscom_add_subregion(chip,
> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> +                            0x40 * stack->stack_no,
> +                            &stack->phb_regs_mr);
>   }
>   
>   static Property pnv_pec_stk_properties[] = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3a550eed0f36..7e13b15241fd 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1370,7 +1370,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
>   static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   {


With that change, we should really rename pnv_chip_power9_phb_realize() 
to pnv_chip_power9_pec_realize().

   Fred


>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
> -    int i, j;
> +    int i;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
> @@ -1392,35 +1392,6 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>   
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
> -
> -        for (j = 0; j < pec->num_stacks; j++) {
> -            PnvPhb4PecStack *stack = &pec->stacks[j];
> -            Object *obj = OBJECT(&stack->phb);
> -
> -            object_property_set_int(obj, "chip-id", chip->chip_id,
> -                                    &error_fatal);
> -            object_property_set_int(obj, "version", pecc->version,
> -                                    &error_fatal);
> -            object_property_set_int(obj, "device-id", pecc->device_id,
> -                                    &error_fatal);
> -            object_property_set_link(obj, "stack", OBJECT(stack),
> -                                     &error_abort);
> -            if (!sysbus_realize(SYS_BUS_DEVICE(obj), errp)) {
> -                return;
> -            }
> -
> -            /* Populate the XSCOM address space. */
> -            pnv_xscom_add_subregion(chip,
> -                                   pec_nest_base + 0x40 * (stack->stack_no + 1),
> -                                   &stack->nest_regs_mr);
> -            pnv_xscom_add_subregion(chip,
> -                                    pec_pci_base + 0x40 * (stack->stack_no + 1),
> -                                    &stack->pci_regs_mr);
> -            pnv_xscom_add_subregion(chip,
> -                                    pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> -                                    0x40 * stack->stack_no,
> -                                    &stack->phb_regs_mr);
> -        }
>       }
>   }
>   
>