| Message ID | 20220110143346.455901-8-danielhb413@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | user creatable pnv-phb4 devices | expand |
On 1/10/22 15:33, Daniel Henrique Barboza wrote: > Before enabling pnv-phb4 user creatable devices we need to handle PHB4 > specific code in pnv_pec_stk_realize(). > > The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on > 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init() > and add_subregion() to phb4_realize() time is a natural thing to do and > it'll spare us from checking 'phb->stack != NULL' in stk_realize() when > user creatable pnv-phb4s are implemented. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/pci-host/pnv_phb4.c | 27 +++++++++++++++++++++++++++ > hw/pci-host/pnv_phb4_pec.c | 10 ---------- > 2 files changed, 27 insertions(+), 10 deletions(-) > > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index 1a7395772f..152911a285 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, > &error_abort); > } > > +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb) > +{ > + PnvPhb4PecState *pec; > + PnvPhb4PecClass *pecc; > + uint32_t pec_pci_base; > + char name[64]; > + > + assert(phb->stack); > + > + pec = phb->stack->pec; > + pecc = PNV_PHB4_PEC_GET_CLASS(pec); > + pec_pci_base = pecc->xscom_pci_base(pec); > + > + /* PHB pass-through */ > + snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb", > + pec->chip_id, pec->index, phb->stack->stack_no); > + pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb), > + &pnv_phb4_xscom_ops, phb, name, 0x40); > + > + pnv_xscom_add_subregion(pec->chip, > + pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 + > + 0x40 * phb->stack->stack_no, > + &phb->stack->phb_regs_mr); > +} > + > static void pnv_phb4_instance_init(Object *obj) > { > PnvPHB4 *phb = PNV_PHB4(obj); > @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) > memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb, > name, 0x2000); > > + pnv_phb4_init_xscom_passthrough(phb); > + > /* > * PHB4 doesn't support IO space. However, qemu gets very upset if > * we don't have an IO region to anchor IO BARs onto so we just > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c > index 042dc0b775..5e02a51f04 100644 > --- a/hw/pci-host/pnv_phb4_pec.c > +++ b/hw/pci-host/pnv_phb4_pec.c > @@ -571,12 +571,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) > &pnv_pec_stk_pci_xscom_ops, stack, name, > PHB4_PEC_PCI_STK_REGS_COUNT); > > - /* PHB pass-through */ > - snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb", > - pec->chip_id, pec->index, stack->stack_no); > - pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb), > - &pnv_phb4_xscom_ops, stack->phb, name, 0x40); > - > if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) { > return; > } > @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) > 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); > } Can't we simply move the XSCOM init and mapping under phb4 realize routine ? Thanks, C. > static Property pnv_pec_stk_properties[] = { >
On 1/10/22 12:57, Cédric Le Goater wrote: > On 1/10/22 15:33, Daniel Henrique Barboza wrote: >> Before enabling pnv-phb4 user creatable devices we need to handle PHB4 >> specific code in pnv_pec_stk_realize(). >> >> The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on >> 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init() >> and add_subregion() to phb4_realize() time is a natural thing to do and >> it'll spare us from checking 'phb->stack != NULL' in stk_realize() when >> user creatable pnv-phb4s are implemented. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/pci-host/pnv_phb4.c | 27 +++++++++++++++++++++++++++ >> hw/pci-host/pnv_phb4_pec.c | 10 ---------- >> 2 files changed, 27 insertions(+), 10 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c >> index 1a7395772f..152911a285 100644 >> --- a/hw/pci-host/pnv_phb4.c >> +++ b/hw/pci-host/pnv_phb4.c >> @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, >> &error_abort); >> } >> +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb) >> +{ >> + PnvPhb4PecState *pec; >> + PnvPhb4PecClass *pecc; >> + uint32_t pec_pci_base; >> + char name[64]; >> + >> + assert(phb->stack); >> + >> + pec = phb->stack->pec; >> + pecc = PNV_PHB4_PEC_GET_CLASS(pec); >> + pec_pci_base = pecc->xscom_pci_base(pec); >> + >> + /* PHB pass-through */ >> + snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb", >> + pec->chip_id, pec->index, phb->stack->stack_no); >> + pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb), >> + &pnv_phb4_xscom_ops, phb, name, 0x40); >> + >> + pnv_xscom_add_subregion(pec->chip, >> + pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 + >> + 0x40 * phb->stack->stack_no, >> + &phb->stack->phb_regs_mr); >> +} >> + >> static void pnv_phb4_instance_init(Object *obj) >> { >> PnvPHB4 *phb = PNV_PHB4(obj); >> @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) >> memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb, >> name, 0x2000); >> + pnv_phb4_init_xscom_passthrough(phb); >> + >> /* >> * PHB4 doesn't support IO space. However, qemu gets very upset if >> * we don't have an IO region to anchor IO BARs onto so we just >> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c >> index 042dc0b775..5e02a51f04 100644 >> --- a/hw/pci-host/pnv_phb4_pec.c >> +++ b/hw/pci-host/pnv_phb4_pec.c >> @@ -571,12 +571,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) >> &pnv_pec_stk_pci_xscom_ops, stack, name, >> PHB4_PEC_PCI_STK_REGS_COUNT); >> - /* PHB pass-through */ >> - snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb", >> - pec->chip_id, pec->index, stack->stack_no); >> - pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb), >> - &pnv_phb4_xscom_ops, stack->phb, name, 0x40); >> - >> if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) { >> return; >> } >> @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) >> 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); >> } > > > Can't we simply move the XSCOM init and mapping under phb4 realize routine ? There's a lot of code to be moved when doing that but in theory we can. It would simplify some things (e.g. we wouldn't need to check stack->phb != NULL in phb4_update_maps() like we need to do in the next patch). I also got cold feet about the repercussions of creating a stack without any XSCOM mapped on it, but perhaps this is the right time to see if that's possible and, if not, try to understand why. Thanks, Daniel > > Thanks, > > C. > > >> static Property pnv_pec_stk_properties[] = { >> >
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index 1a7395772f..152911a285 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, &error_abort); } +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb) +{ + PnvPhb4PecState *pec; + PnvPhb4PecClass *pecc; + uint32_t pec_pci_base; + char name[64]; + + assert(phb->stack); + + pec = phb->stack->pec; + pecc = PNV_PHB4_PEC_GET_CLASS(pec); + pec_pci_base = pecc->xscom_pci_base(pec); + + /* PHB pass-through */ + snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb", + pec->chip_id, pec->index, phb->stack->stack_no); + pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb), + &pnv_phb4_xscom_ops, phb, name, 0x40); + + pnv_xscom_add_subregion(pec->chip, + pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 + + 0x40 * phb->stack->stack_no, + &phb->stack->phb_regs_mr); +} + static void pnv_phb4_instance_init(Object *obj) { PnvPHB4 *phb = PNV_PHB4(obj); @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp) memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb, name, 0x2000); + pnv_phb4_init_xscom_passthrough(phb); + /* * PHB4 doesn't support IO space. However, qemu gets very upset if * we don't have an IO region to anchor IO BARs onto so we just diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c index 042dc0b775..5e02a51f04 100644 --- a/hw/pci-host/pnv_phb4_pec.c +++ b/hw/pci-host/pnv_phb4_pec.c @@ -571,12 +571,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) &pnv_pec_stk_pci_xscom_ops, stack, name, PHB4_PEC_PCI_STK_REGS_COUNT); - /* PHB pass-through */ - snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb", - pec->chip_id, pec->index, stack->stack_no); - pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb), - &pnv_phb4_xscom_ops, stack->phb, name, 0x40); - if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) { return; } @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) 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[] = {
Before enabling pnv-phb4 user creatable devices we need to handle PHB4 specific code in pnv_pec_stk_realize(). The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init() and add_subregion() to phb4_realize() time is a natural thing to do and it'll spare us from checking 'phb->stack != NULL' in stk_realize() when user creatable pnv-phb4s are implemented. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb4.c | 27 +++++++++++++++++++++++++++ hw/pci-host/pnv_phb4_pec.c | 10 ---------- 2 files changed, 27 insertions(+), 10 deletions(-)