Re: [PATCH v5 3/3] hw/ppc: Nest1 chiplet wiring

[Cédric Le Goater]

On 11/24/23 12:28, Nicholas Piggin wrote:
> On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
> > This part of the patchset connects the nest1 chiplet model to p10 chip.
>
> Seems fine to me. Should it just be squashed into patch 2?

It is better to keep the model a part from the wiring because the
same model could be plugged in different board/machine. It clarifies
the interfaces, which should be limited to irq connects and memory
mappings and it makes modeling shortcuts more visible: backpointers,
looping on the machine mappings to find a core, etc.

I didn't comment on the PnvChiptod proposal but it could/should
be done the same.

This patch proposal is nice and clean.

Thanks,

C.




> Thanks,
> Nick
>
> > Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> > ---
> >   include/hw/ppc/pnv_chip.h |  2 ++
> >   hw/ppc/pnv.c              | 14 ++++++++++++++
> >   2 files changed, 16 insertions(+)
> >
> > diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
> > index 0ab5c42308..59a3158a6b 100644
> > --- a/include/hw/ppc/pnv_chip.h
> > +++ b/include/hw/ppc/pnv_chip.h
> > @@ -4,6 +4,7 @@
> >   #include "hw/pci-host/pnv_phb4.h"
> >   #include "hw/ppc/pnv_core.h"
> >   #include "hw/ppc/pnv_homer.h"
> > +#include "hw/ppc/pnv_nest_chiplet.h"
> >   #include "hw/ppc/pnv_lpc.h"
> >   #include "hw/ppc/pnv_occ.h"
> >   #include "hw/ppc/pnv_psi.h"
> > @@ -113,6 +114,7 @@ struct Pnv10Chip {
> >       PnvOCC       occ;
> >       PnvSBE       sbe;
> >       PnvHomer     homer;
> > +    PnvNest1     nest1;
> >   
> >       uint32_t     nr_quads;
> >       PnvQuad      *quads;
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0297871bdd..ba3dfab557 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1680,6 +1680,7 @@ static void pnv_chip_power10_instance_init(Object *obj)
> >       object_initialize_child(obj, "occ",  &chip10->occ, TYPE_PNV10_OCC);
> >       object_initialize_child(obj, "sbe",  &chip10->sbe, TYPE_PNV10_SBE);
> >       object_initialize_child(obj, "homer", &chip10->homer, TYPE_PNV10_HOMER);
> > +    object_initialize_child(obj, "nest1", &chip10->nest1, TYPE_PNV_NEST1);
> >   
> >       chip->num_pecs = pcc->num_pecs;
> >   
> > @@ -1849,6 +1850,19 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
> >       memory_region_add_subregion(get_system_memory(), PNV10_HOMER_BASE(chip),
> >                                   &chip10->homer.regs);
> >   
> > +    /* nest1 chiplet */
> > +    if (!qdev_realize(DEVICE(&chip10->nest1), NULL, errp)) {
> > +        return;
> > +    }
> > +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE,
> > +             &chip10->nest1.perv.xscom_perv_ctrl_regs);
> > +
> > +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE,
> > +                           &chip10->nest1.xscom_pb_eq_regs);
> > +
> > +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE,
> > +                           &chip10->nest1.xscom_pb_es_regs);
> > +
> >       /* PHBs */
> >       pnv_chip_power10_phb_realize(chip, &local_err);
> >       if (local_err) {