| Message ID | 20230302163715.129635-2-fbarrat@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | user-created PHB cleanup | expand |
Hi Frederic, On 2/3/23 17:37, Frederic Barrat wrote: > Add an array on the PEC object to keep track of the PHBs which are > instantiated. The array can be sparsely populated when using > user-created PHBs. It will be useful for the next patch to only export > instantiated PHBs in the device tree. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- > hw/pci-host/pnv_phb4_pec.c | 12 +++++++----- > hw/ppc/pnv.c | 1 + > include/hw/pci-host/pnv_phb4.h | 2 ++ > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c > index 43267a428f..97c06bb0a0 100644 > --- a/hw/pci-host/pnv_phb4_pec.c > +++ b/hw/pci-host/pnv_phb4_pec.c > +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec, > + int stack_no, > + Error **errp) > { > PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB)); > int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no); > @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec, > &error_fatal); > > if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) { > - return; > + return NULL; > } > + return phb; > } > diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h > index 28d61b96c7..0b72ef1471 100644 > --- a/include/hw/pci-host/pnv_phb4.h > +++ b/include/hw/pci-host/pnv_phb4.h > @@ -185,6 +185,8 @@ struct PnvPhb4PecState { > > /* PHBs */ > uint32_t num_phbs; > +#define MAX_PHBS_PER_PEC 3 > + PnvPHB *phbs[MAX_PHBS_PER_PEC]; > > PnvChip *chip; > }; From QOM PoV, better would be to 'embed' the PnvPHB structure (not only a pointer to it), and initialize the PnvPHB instance calling object_initialize_child() instead of qdev_new(). See for example the recent conversion of OHCISysBusState in commit 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset"). Regards, Phil.
On 3/2/23 19:21, Philippe Mathieu-Daudé wrote: > Hi Frederic, > > On 2/3/23 17:37, Frederic Barrat wrote: >> Add an array on the PEC object to keep track of the PHBs which are >> instantiated. The array can be sparsely populated when using >> user-created PHBs. It will be useful for the next patch to only export >> instantiated PHBs in the device tree. >> >> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> >> --- >> hw/pci-host/pnv_phb4_pec.c | 12 +++++++----- >> hw/ppc/pnv.c | 1 + >> include/hw/pci-host/pnv_phb4.h | 2 ++ >> 3 files changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c >> index 43267a428f..97c06bb0a0 100644 >> --- a/hw/pci-host/pnv_phb4_pec.c >> +++ b/hw/pci-host/pnv_phb4_pec.c > >> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec, >> + int stack_no, >> + Error **errp) >> { >> PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB)); >> int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no); >> @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec, >> &error_fatal); >> if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) { >> - return; >> + return NULL; >> } >> + return phb; >> } > > >> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h >> index 28d61b96c7..0b72ef1471 100644 >> --- a/include/hw/pci-host/pnv_phb4.h >> +++ b/include/hw/pci-host/pnv_phb4.h >> @@ -185,6 +185,8 @@ struct PnvPhb4PecState { >> /* PHBs */ >> uint32_t num_phbs; >> +#define MAX_PHBS_PER_PEC 3 >> + PnvPHB *phbs[MAX_PHBS_PER_PEC]; >> PnvChip *chip; >> }; > > From QOM PoV, better would be to 'embed' the PnvPHB structure (not only > a pointer to it), and initialize the PnvPHB instance calling > object_initialize_child() instead of qdev_new(). We were doing something similar in this PHB controllers one year ago. The reason we moved to host pointers instead of using embed structures was to allow user-created phbs. There's no way of telling beforehand if the user wants one, two or three PHBs in the PEC. Thanks, Daniel > > See for example the recent conversion of OHCISysBusState in commit > 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset"). > > Regards, > > Phil.
On 3/2/23 13:37, Frederic Barrat wrote: > Add an array on the PEC object to keep track of the PHBs which are > instantiated. The array can be sparsely populated when using > user-created PHBs. It will be useful for the next patch to only export > instantiated PHBs in the device tree. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > hw/pci-host/pnv_phb4_pec.c | 12 +++++++----- > hw/ppc/pnv.c | 1 + > include/hw/pci-host/pnv_phb4.h | 2 ++ > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c > index 43267a428f..97c06bb0a0 100644 > --- a/hw/pci-host/pnv_phb4_pec.c > +++ b/hw/pci-host/pnv_phb4_pec.c > @@ -112,9 +112,9 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = { > .endianness = DEVICE_BIG_ENDIAN, > }; > > -static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec, > - int stack_no, > - Error **errp) > +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec, > + int stack_no, > + Error **errp) > { > PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB)); > int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no); > @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec, > &error_fatal); > > if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) { > - return; > + return NULL; > } > + return phb; > } > > static void pnv_pec_realize(DeviceState *dev, Error **errp) > @@ -148,8 +149,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp) > > /* Create PHBs if running with defaults */ > if (defaults_enabled()) { > + g_assert(pec->num_phbs <= MAX_PHBS_PER_PEC); > for (i = 0; i < pec->num_phbs; i++) { > - pnv_pec_default_phb_realize(pec, i, errp); > + pec->phbs[i] = pnv_pec_default_phb_realize(pec, i, errp); > } > } > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 44b1fbbc93..24bf8461d6 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -314,6 +314,7 @@ static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb, > > for (j = 0; j < pec->num_phbs; j++) { > if (index == pnv_phb4_pec_get_phb_id(pec, j)) { > + pec->phbs[j] = phb->phb_base; > return pec; > } > } > diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h > index 28d61b96c7..0b72ef1471 100644 > --- a/include/hw/pci-host/pnv_phb4.h > +++ b/include/hw/pci-host/pnv_phb4.h > @@ -185,6 +185,8 @@ struct PnvPhb4PecState { > > /* PHBs */ > uint32_t num_phbs; > +#define MAX_PHBS_PER_PEC 3 > + PnvPHB *phbs[MAX_PHBS_PER_PEC]; > > PnvChip *chip; > };
On 02/03/2023 23:21, Philippe Mathieu-Daudé wrote: > Hi Frederic, > > On 2/3/23 17:37, Frederic Barrat wrote: >> Add an array on the PEC object to keep track of the PHBs which are >> instantiated. The array can be sparsely populated when using >> user-created PHBs. It will be useful for the next patch to only export >> instantiated PHBs in the device tree. >> >> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> >> --- >> hw/pci-host/pnv_phb4_pec.c | 12 +++++++----- >> hw/ppc/pnv.c | 1 + >> include/hw/pci-host/pnv_phb4.h | 2 ++ >> 3 files changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c >> index 43267a428f..97c06bb0a0 100644 >> --- a/hw/pci-host/pnv_phb4_pec.c >> +++ b/hw/pci-host/pnv_phb4_pec.c > >> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec, >> + int stack_no, >> + Error **errp) >> { >> PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB)); >> int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no); >> @@ -128,8 +128,9 @@ static void >> pnv_pec_default_phb_realize(PnvPhb4PecState *pec, >> &error_fatal); >> if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) { >> - return; >> + return NULL; >> } >> + return phb; >> } > > >> diff --git a/include/hw/pci-host/pnv_phb4.h >> b/include/hw/pci-host/pnv_phb4.h >> index 28d61b96c7..0b72ef1471 100644 >> --- a/include/hw/pci-host/pnv_phb4.h >> +++ b/include/hw/pci-host/pnv_phb4.h >> @@ -185,6 +185,8 @@ struct PnvPhb4PecState { >> /* PHBs */ >> uint32_t num_phbs; >> +#define MAX_PHBS_PER_PEC 3 >> + PnvPHB *phbs[MAX_PHBS_PER_PEC]; >> PnvChip *chip; >> }; > > From QOM PoV, better would be to 'embed' the PnvPHB structure (not only > a pointer to it), and initialize the PnvPHB instance calling > object_initialize_child() instead of qdev_new(). Hi Phil, Daniel beat me to it, but we used to do precisely that but went the opposite direction (see 0d512c7120a2), because we can now specify from the command line what PHB to implement so we don't want to allocate all of them. Fred > > See for example the recent conversion of OHCISysBusState in commit > 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset"). > > Regards, > > Phil.
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c index 43267a428f..97c06bb0a0 100644 --- a/hw/pci-host/pnv_phb4_pec.c +++ b/hw/pci-host/pnv_phb4_pec.c @@ -112,9 +112,9 @@ static const MemoryRegionOps pnv_pec_pci_xscom_ops = { .endianness = DEVICE_BIG_ENDIAN, }; -static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec, - int stack_no, - Error **errp) +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec, + int stack_no, + Error **errp) { PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB)); int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no); @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec, &error_fatal); if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) { - return; + return NULL; } + return phb; } static void pnv_pec_realize(DeviceState *dev, Error **errp) @@ -148,8 +149,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp) /* Create PHBs if running with defaults */ if (defaults_enabled()) { + g_assert(pec->num_phbs <= MAX_PHBS_PER_PEC); for (i = 0; i < pec->num_phbs; i++) { - pnv_pec_default_phb_realize(pec, i, errp); + pec->phbs[i] = pnv_pec_default_phb_realize(pec, i, errp); } } diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 44b1fbbc93..24bf8461d6 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -314,6 +314,7 @@ static PnvPhb4PecState *pnv_phb4_get_pec(PnvChip *chip, PnvPHB4 *phb, for (j = 0; j < pec->num_phbs; j++) { if (index == pnv_phb4_pec_get_phb_id(pec, j)) { + pec->phbs[j] = phb->phb_base; return pec; } } diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h index 28d61b96c7..0b72ef1471 100644 --- a/include/hw/pci-host/pnv_phb4.h +++ b/include/hw/pci-host/pnv_phb4.h @@ -185,6 +185,8 @@ struct PnvPhb4PecState { /* PHBs */ uint32_t num_phbs; +#define MAX_PHBS_PER_PEC 3 + PnvPHB *phbs[MAX_PHBS_PER_PEC]; PnvChip *chip; };
Add an array on the PEC object to keep track of the PHBs which are instantiated. The array can be sparsely populated when using user-created PHBs. It will be useful for the next patch to only export instantiated PHBs in the device tree. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- hw/pci-host/pnv_phb4_pec.c | 12 +++++++----- hw/ppc/pnv.c | 1 + include/hw/pci-host/pnv_phb4.h | 2 ++ 3 files changed, 10 insertions(+), 5 deletions(-)