| Message ID | 20231124101534.19454-4-chalapathi.v@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pnv nest1 chiplet model | expand |
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? 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) {
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) { >
On Fri Nov 24, 2023 at 10:26 PM AEST, Cédric Le Goater wrote: > 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. Okay that makes sense. > I didn't comment on the PnvChiptod proposal but it could/should > be done the same. I'll look at splitting it too then. Thanks, Nick
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) {
This part of the patchset connects the nest1 chiplet model to p10 chip. 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(+)