| Message ID | 20230707071213.9924-1-joel@jms.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ppc/pnv: Add QME region for P10 | expand |
On 7/7/23 09:12, Joel Stanley wrote: > The Quad Management Engine (QME) manages power related settings for its > quad. The xscom region is separate from the quad xscoms, therefore a new > region is added. The xscoms in a QME select a given core by selecting > the forth nibble. > > Implement dummy reads for the stop state history (SSH) and special > wakeup (SPWU) registers. This quietens some sxcom errors when skiboot > boots on p10. > > Power9 does not have a QME. > > Signed-off-by: Joel Stanley <joel@jms.id.au> Nice, how about these now : [ 24.482066616,3] Could not set special wakeup on 0:0: operation timeout. [ 25.022003091,3] Could not set special wakeup on 0:0: operation timeout. [ 25.073902795,3] Could not set special wakeup on 0:0: operation timeout. [ 1593.383133413,3] Could not set special wakeup on 0:0: timeout waiting for SPECIAL_WKUP_DONE. [ 1593.435173594,3] Could not set special wakeup on 0:0: timeout waiting for SPECIAL_WKUP_DONE. :) Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > v2: > Clean up extra whitespace > Make realize quad specific so power9 doesn't end up with the qme region > --- > include/hw/ppc/pnv_core.h | 4 ++ > include/hw/ppc/pnv_xscom.h | 11 ++++++ > hw/ppc/pnv.c | 3 ++ > hw/ppc/pnv_core.c | 78 +++++++++++++++++++++++++++++++++++++- > 4 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h > index 77ef00f47a72..c829a18aa9c6 100644 > --- a/include/hw/ppc/pnv_core.h > +++ b/include/hw/ppc/pnv_core.h > @@ -65,6 +65,9 @@ struct PnvQuadClass { > > const MemoryRegionOps *xscom_ops; > uint64_t xscom_size; > + > + const MemoryRegionOps *xscom_qme_ops; > + uint64_t xscom_qme_size; > }; > > #define TYPE_PNV_QUAD "powernv-cpu-quad" > @@ -79,5 +82,6 @@ struct PnvQuad { > > uint32_t quad_id; > MemoryRegion xscom_regs; > + MemoryRegion xscom_qme_regs; > }; > #endif /* PPC_PNV_CORE_H */ > diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h > index a4c9d95dc5d3..9bc64635471e 100644 > --- a/include/hw/ppc/pnv_xscom.h > +++ b/include/hw/ppc/pnv_xscom.h > @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { > #define PNV10_XSCOM_EC(proc) \ > ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) > > +#define PNV10_XSCOM_QME(chiplet) \ > + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) > + > +/* > + * Make the region larger by 0x1000 (instead of starting at an offset) so the > + * modelled addresses start from 0 > + */ > +#define PNV10_XSCOM_QME_BASE(core) \ > + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) > +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) > + > #define PNV10_XSCOM_EQ_BASE(core) \ > ((uint64_t) PNV10_XSCOM_EQ(PNV10_XSCOM_EQ_CHIPLET(core))) > #define PNV10_XSCOM_EQ_SIZE 0x20000 > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 23740f9d0733..eb54f93986df 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -1685,6 +1685,9 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp) > > pnv_xscom_add_subregion(chip, PNV10_XSCOM_EQ_BASE(eq->quad_id), > &eq->xscom_regs); > + > + pnv_xscom_add_subregion(chip, PNV10_XSCOM_QME_BASE(eq->quad_id), > + &eq->xscom_qme_regs); > } > } > > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > index 1f244ed181d0..09eb2bf94b9e 100644 > --- a/hw/ppc/pnv_core.c > +++ b/hw/ppc/pnv_core.c > @@ -493,7 +493,67 @@ static const MemoryRegionOps pnv_quad_power10_xscom_ops = { > .endianness = DEVICE_BIG_ENDIAN, > }; > > -static void pnv_quad_realize(DeviceState *dev, Error **errp) > +#define P10_QME_SPWU_HYP 0x83c > +#define P10_QME_SSH_HYP 0x82c > + > +static uint64_t pnv_qme_power10_xscom_read(void *opaque, hwaddr addr, > + unsigned int width) > +{ > + uint32_t offset = addr >> 3; > + uint64_t val = -1; > + > + /* > + * Forth nibble selects the core within a quad, mask it to process read > + * for any core. > + */ > + switch (offset & ~0xf000) { > + case P10_QME_SPWU_HYP: > + case P10_QME_SSH_HYP: > + return 0; > + default: > + qemu_log_mask(LOG_UNIMP, "%s: unimp read 0x%08x\n", __func__, > + offset); > + } > + > + return val; > +} > + > +static void pnv_qme_power10_xscom_write(void *opaque, hwaddr addr, > + uint64_t val, unsigned int width) > +{ > + uint32_t offset = addr >> 3; > + > + switch (offset) { > + default: > + qemu_log_mask(LOG_UNIMP, "%s: unimp write 0x%08x\n", __func__, > + offset); > + } > +} > + > +static const MemoryRegionOps pnv_qme_power10_xscom_ops = { > + .read = pnv_qme_power10_xscom_read, > + .write = pnv_qme_power10_xscom_write, > + .valid.min_access_size = 8, > + .valid.max_access_size = 8, > + .impl.min_access_size = 8, > + .impl.max_access_size = 8, > + .endianness = DEVICE_BIG_ENDIAN, > +}; > + > +static void pnv_quad_power9_realize(DeviceState *dev, Error **errp) > +{ > + PnvQuad *eq = PNV_QUAD(dev); > + PnvQuadClass *pqc = PNV_QUAD_GET_CLASS(eq); > + char name[32]; > + > + snprintf(name, sizeof(name), "xscom-quad.%d", eq->quad_id); > + pnv_xscom_region_init(&eq->xscom_regs, OBJECT(dev), > + pqc->xscom_ops, > + eq, name, > + pqc->xscom_size); > +} > + > +static void pnv_quad_power10_realize(DeviceState *dev, Error **errp) > { > PnvQuad *eq = PNV_QUAD(dev); > PnvQuadClass *pqc = PNV_QUAD_GET_CLASS(eq); > @@ -504,6 +564,12 @@ static void pnv_quad_realize(DeviceState *dev, Error **errp) > pqc->xscom_ops, > eq, name, > pqc->xscom_size); > + > + snprintf(name, sizeof(name), "xscom-qme.%d", eq->quad_id); > + pnv_xscom_region_init(&eq->xscom_qme_regs, OBJECT(dev), > + pqc->xscom_qme_ops, > + eq, name, > + pqc->xscom_qme_size); > } > > static Property pnv_quad_properties[] = { > @@ -514,6 +580,9 @@ static Property pnv_quad_properties[] = { > static void pnv_quad_power9_class_init(ObjectClass *oc, void *data) > { > PnvQuadClass *pqc = PNV_QUAD_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = pnv_quad_power9_realize; > > pqc->xscom_ops = &pnv_quad_power9_xscom_ops; > pqc->xscom_size = PNV9_XSCOM_EQ_SIZE; > @@ -522,16 +591,21 @@ static void pnv_quad_power9_class_init(ObjectClass *oc, void *data) > static void pnv_quad_power10_class_init(ObjectClass *oc, void *data) > { > PnvQuadClass *pqc = PNV_QUAD_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = pnv_quad_power10_realize; > > pqc->xscom_ops = &pnv_quad_power10_xscom_ops; > pqc->xscom_size = PNV10_XSCOM_EQ_SIZE; > + > + pqc->xscom_qme_ops = &pnv_qme_power10_xscom_ops; > + pqc->xscom_qme_size = PNV10_XSCOM_QME_SIZE; > } > > static void pnv_quad_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > > - dc->realize = pnv_quad_realize; > device_class_set_props(dc, pnv_quad_properties); > dc->user_creatable = false; > }
On Fri, 7 Jul 2023 at 07:30, Cédric Le Goater <clg@kaod.org> wrote: > > On 7/7/23 09:12, Joel Stanley wrote: > > The Quad Management Engine (QME) manages power related settings for its > > quad. The xscom region is separate from the quad xscoms, therefore a new > > region is added. The xscoms in a QME select a given core by selecting > > the forth nibble. > > > > Implement dummy reads for the stop state history (SSH) and special > > wakeup (SPWU) registers. This quietens some sxcom errors when skiboot > > boots on p10. > > > > Power9 does not have a QME. > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > Nice, how about these now : > > > [ 24.482066616,3] Could not set special wakeup on 0:0: operation timeout. > [ 25.022003091,3] Could not set special wakeup on 0:0: operation timeout. > [ 25.073902795,3] Could not set special wakeup on 0:0: operation timeout. > > [ 1593.383133413,3] Could not set special wakeup on 0:0: timeout waiting for SPECIAL_WKUP_DONE. > [ 1593.435173594,3] Could not set special wakeup on 0:0: timeout waiting for SPECIAL_WKUP_DONE. Yes, something like below, except hard coding is not sufficient. We need to pass the core state into the quad model so the qme callbacks can keep track of the wakeup state. From: Joel Stanley <joel@jms.id.au> Date: Fri, 7 Jul 2023 13:37:17 +0930 Subject: [PATCH] ppc/pnv: Implement more sleep related registers We need to get the core object into the quad callback so we can update the sleep state. Signed-off-by: Joel Stanley <joel@jms.id.au> --- hw/ppc/pnv_core.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c index 09eb2bf94b9e..359b341c748f 100644 --- a/hw/ppc/pnv_core.c +++ b/hw/ppc/pnv_core.c @@ -179,6 +179,7 @@ static const MemoryRegionOps pnv_core_power9_xscom_ops = { */ #define PNV10_XSCOM_EC_CORE_THREAD_STATE 0x412 +#define PNV10_XSCOM_EC_RAS_STATUS 0x454 static uint64_t pnv_core_power10_xscom_read(void *opaque, hwaddr addr, unsigned int width) @@ -190,6 +191,9 @@ static uint64_t pnv_core_power10_xscom_read(void *opaque, hwaddr addr, case PNV10_XSCOM_EC_CORE_THREAD_STATE: val = 0; break; + case PNV10_XSCOM_EC_RAS_STATUS: + val = -1; + break; default: qemu_log_mask(LOG_UNIMP, "%s: unimp read 0x%08x\n", __func__, offset); @@ -494,7 +498,12 @@ static const MemoryRegionOps pnv_quad_power10_xscom_ops = { }; #define P10_QME_SPWU_HYP 0x83c +#define P10_SPWU_REQ PPC_BIT(0) +#define P10_SPWU_DONE PPC_BIT(4) + #define P10_QME_SSH_HYP 0x82c +#define P10_SSH_CORE_GATED PPC_BIT(0) +#define P10_SSH_SPWU_DONE PPC_BIT(1) static uint64_t pnv_qme_power10_xscom_read(void *opaque, hwaddr addr, unsigned int width) @@ -508,8 +517,11 @@ static uint64_t pnv_qme_power10_xscom_read(void *opaque, hwaddr addr, */ switch (offset & ~0xf000) { case P10_QME_SPWU_HYP: + val = 0; + break; case P10_QME_SSH_HYP: - return 0; + val = P10_SSH_SPWU_DONE; + break; default: qemu_log_mask(LOG_UNIMP, "%s: unimp read 0x%08x\n", __func__, offset); @@ -524,6 +536,8 @@ static void pnv_qme_power10_xscom_write(void *opaque, hwaddr addr, uint32_t offset = addr >> 3; switch (offset) { + case P10_QME_SSH_HYP: + case P10_QME_SPWU_HYP: default: qemu_log_mask(LOG_UNIMP, "%s: unimp write 0x%08x\n", __func__, offset);
Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel On 7/7/23 04:12, Joel Stanley wrote: > The Quad Management Engine (QME) manages power related settings for its > quad. The xscom region is separate from the quad xscoms, therefore a new > region is added. The xscoms in a QME select a given core by selecting > the forth nibble. > > Implement dummy reads for the stop state history (SSH) and special > wakeup (SPWU) registers. This quietens some sxcom errors when skiboot > boots on p10. > > Power9 does not have a QME. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > v2: > Clean up extra whitespace > Make realize quad specific so power9 doesn't end up with the qme region > --- > include/hw/ppc/pnv_core.h | 4 ++ > include/hw/ppc/pnv_xscom.h | 11 ++++++ > hw/ppc/pnv.c | 3 ++ > hw/ppc/pnv_core.c | 78 +++++++++++++++++++++++++++++++++++++- > 4 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h > index 77ef00f47a72..c829a18aa9c6 100644 > --- a/include/hw/ppc/pnv_core.h > +++ b/include/hw/ppc/pnv_core.h > @@ -65,6 +65,9 @@ struct PnvQuadClass { > > const MemoryRegionOps *xscom_ops; > uint64_t xscom_size; > + > + const MemoryRegionOps *xscom_qme_ops; > + uint64_t xscom_qme_size; > }; > > #define TYPE_PNV_QUAD "powernv-cpu-quad" > @@ -79,5 +82,6 @@ struct PnvQuad { > > uint32_t quad_id; > MemoryRegion xscom_regs; > + MemoryRegion xscom_qme_regs; > }; > #endif /* PPC_PNV_CORE_H */ > diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h > index a4c9d95dc5d3..9bc64635471e 100644 > --- a/include/hw/ppc/pnv_xscom.h > +++ b/include/hw/ppc/pnv_xscom.h > @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { > #define PNV10_XSCOM_EC(proc) \ > ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) > > +#define PNV10_XSCOM_QME(chiplet) \ > + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) > + > +/* > + * Make the region larger by 0x1000 (instead of starting at an offset) so the > + * modelled addresses start from 0 > + */ > +#define PNV10_XSCOM_QME_BASE(core) \ > + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) > +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) > + > #define PNV10_XSCOM_EQ_BASE(core) \ > ((uint64_t) PNV10_XSCOM_EQ(PNV10_XSCOM_EQ_CHIPLET(core))) > #define PNV10_XSCOM_EQ_SIZE 0x20000 > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 23740f9d0733..eb54f93986df 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -1685,6 +1685,9 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp) > > pnv_xscom_add_subregion(chip, PNV10_XSCOM_EQ_BASE(eq->quad_id), > &eq->xscom_regs); > + > + pnv_xscom_add_subregion(chip, PNV10_XSCOM_QME_BASE(eq->quad_id), > + &eq->xscom_qme_regs); > } > } > > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > index 1f244ed181d0..09eb2bf94b9e 100644 > --- a/hw/ppc/pnv_core.c > +++ b/hw/ppc/pnv_core.c > @@ -493,7 +493,67 @@ static const MemoryRegionOps pnv_quad_power10_xscom_ops = { > .endianness = DEVICE_BIG_ENDIAN, > }; > > -static void pnv_quad_realize(DeviceState *dev, Error **errp) > +#define P10_QME_SPWU_HYP 0x83c > +#define P10_QME_SSH_HYP 0x82c > + > +static uint64_t pnv_qme_power10_xscom_read(void *opaque, hwaddr addr, > + unsigned int width) > +{ > + uint32_t offset = addr >> 3; > + uint64_t val = -1; > + > + /* > + * Forth nibble selects the core within a quad, mask it to process read > + * for any core. > + */ > + switch (offset & ~0xf000) { > + case P10_QME_SPWU_HYP: > + case P10_QME_SSH_HYP: > + return 0; > + default: > + qemu_log_mask(LOG_UNIMP, "%s: unimp read 0x%08x\n", __func__, > + offset); > + } > + > + return val; > +} > + > +static void pnv_qme_power10_xscom_write(void *opaque, hwaddr addr, > + uint64_t val, unsigned int width) > +{ > + uint32_t offset = addr >> 3; > + > + switch (offset) { > + default: > + qemu_log_mask(LOG_UNIMP, "%s: unimp write 0x%08x\n", __func__, > + offset); > + } > +} > + > +static const MemoryRegionOps pnv_qme_power10_xscom_ops = { > + .read = pnv_qme_power10_xscom_read, > + .write = pnv_qme_power10_xscom_write, > + .valid.min_access_size = 8, > + .valid.max_access_size = 8, > + .impl.min_access_size = 8, > + .impl.max_access_size = 8, > + .endianness = DEVICE_BIG_ENDIAN, > +}; > + > +static void pnv_quad_power9_realize(DeviceState *dev, Error **errp) > +{ > + PnvQuad *eq = PNV_QUAD(dev); > + PnvQuadClass *pqc = PNV_QUAD_GET_CLASS(eq); > + char name[32]; > + > + snprintf(name, sizeof(name), "xscom-quad.%d", eq->quad_id); > + pnv_xscom_region_init(&eq->xscom_regs, OBJECT(dev), > + pqc->xscom_ops, > + eq, name, > + pqc->xscom_size); > +} > + > +static void pnv_quad_power10_realize(DeviceState *dev, Error **errp) > { > PnvQuad *eq = PNV_QUAD(dev); > PnvQuadClass *pqc = PNV_QUAD_GET_CLASS(eq); > @@ -504,6 +564,12 @@ static void pnv_quad_realize(DeviceState *dev, Error **errp) > pqc->xscom_ops, > eq, name, > pqc->xscom_size); > + > + snprintf(name, sizeof(name), "xscom-qme.%d", eq->quad_id); > + pnv_xscom_region_init(&eq->xscom_qme_regs, OBJECT(dev), > + pqc->xscom_qme_ops, > + eq, name, > + pqc->xscom_qme_size); > } > > static Property pnv_quad_properties[] = { > @@ -514,6 +580,9 @@ static Property pnv_quad_properties[] = { > static void pnv_quad_power9_class_init(ObjectClass *oc, void *data) > { > PnvQuadClass *pqc = PNV_QUAD_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = pnv_quad_power9_realize; > > pqc->xscom_ops = &pnv_quad_power9_xscom_ops; > pqc->xscom_size = PNV9_XSCOM_EQ_SIZE; > @@ -522,16 +591,21 @@ static void pnv_quad_power9_class_init(ObjectClass *oc, void *data) > static void pnv_quad_power10_class_init(ObjectClass *oc, void *data) > { > PnvQuadClass *pqc = PNV_QUAD_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = pnv_quad_power10_realize; > > pqc->xscom_ops = &pnv_quad_power10_xscom_ops; > pqc->xscom_size = PNV10_XSCOM_EQ_SIZE; > + > + pqc->xscom_qme_ops = &pnv_qme_power10_xscom_ops; > + pqc->xscom_qme_size = PNV10_XSCOM_QME_SIZE; > } > > static void pnv_quad_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > > - dc->realize = pnv_quad_realize; > device_class_set_props(dc, pnv_quad_properties); > dc->user_creatable = false; > }
On Fri Jul 7, 2023 at 5:12 PM AEST, Joel Stanley wrote: > The Quad Management Engine (QME) manages power related settings for its > quad. The xscom region is separate from the quad xscoms, therefore a new > region is added. The xscoms in a QME select a given core by selecting > the forth nibble. > > Implement dummy reads for the stop state history (SSH) and special > wakeup (SPWU) registers. This quietens some sxcom errors when skiboot > boots on p10. > > Power9 does not have a QME. > > Signed-off-by: Joel Stanley <joel@jms.id.au> Nice, already merged but looks good to me. Just one thing... > diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h > index a4c9d95dc5d3..9bc64635471e 100644 > --- a/include/hw/ppc/pnv_xscom.h > +++ b/include/hw/ppc/pnv_xscom.h > @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { > #define PNV10_XSCOM_EC(proc) \ > ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) > > +#define PNV10_XSCOM_QME(chiplet) \ > + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) > + > +/* > + * Make the region larger by 0x1000 (instead of starting at an offset) so the > + * modelled addresses start from 0 > + */ > +#define PNV10_XSCOM_QME_BASE(core) \ > + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) > +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) I couldn't work out this comment. Thanks, Nick
On Sat, 8 Jul 2023 at 01:17, Nicholas Piggin <npiggin@gmail.com> wrote: > > --- a/include/hw/ppc/pnv_xscom.h > > +++ b/include/hw/ppc/pnv_xscom.h > > @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { > > #define PNV10_XSCOM_EC(proc) \ > > ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) > > > > +#define PNV10_XSCOM_QME(chiplet) \ > > + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) > > + > > +/* > > + * Make the region larger by 0x1000 (instead of starting at an offset) so the > > + * modelled addresses start from 0 > > + */ > > +#define PNV10_XSCOM_QME_BASE(core) \ > > + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) > > +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) > > I couldn't work out this comment. I was trying to describe why we have the + 0x1000. Each core sets a bit in the xscom address space, with the first core setting bit 12, second bit 13, etc. So there's actually no registers at PNV10_XSCOM_QME_BASE, but so the addressing is easier to follow, I chose to start the base where we do, and make the region 0x1000 bigger. That was my understanding at least. Cheers, Joel > > Thanks, > Nick
On 7/7/23 22:16, Nicholas Piggin wrote: > On Fri Jul 7, 2023 at 5:12 PM AEST, Joel Stanley wrote: >> The Quad Management Engine (QME) manages power related settings for its >> quad. The xscom region is separate from the quad xscoms, therefore a new >> region is added. The xscoms in a QME select a given core by selecting >> the forth nibble. >> >> Implement dummy reads for the stop state history (SSH) and special >> wakeup (SPWU) registers. This quietens some sxcom errors when skiboot >> boots on p10. >> >> Power9 does not have a QME. >> >> Signed-off-by: Joel Stanley <joel@jms.id.au> > > Nice, already merged but looks good to me. Just one thing... > >> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h >> index a4c9d95dc5d3..9bc64635471e 100644 >> --- a/include/hw/ppc/pnv_xscom.h >> +++ b/include/hw/ppc/pnv_xscom.h >> @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { >> #define PNV10_XSCOM_EC(proc) \ >> ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) >> >> +#define PNV10_XSCOM_QME(chiplet) \ >> + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) >> + >> +/* >> + * Make the region larger by 0x1000 (instead of starting at an offset) so the >> + * modelled addresses start from 0 >> + */ >> +#define PNV10_XSCOM_QME_BASE(core) \ >> + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) >> +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) > > I couldn't work out this comment. FWIW if a comment is inaccurate/wrong it is fine to patch it during the code freeze if needed. Thanks, Daniel > > Thanks, > Nick >
On 7/7/23 22:16, Nicholas Piggin wrote: > On Fri Jul 7, 2023 at 5:12 PM AEST, Joel Stanley wrote: >> The Quad Management Engine (QME) manages power related settings for its >> quad. The xscom region is separate from the quad xscoms, therefore a new >> region is added. The xscoms in a QME select a given core by selecting >> the forth nibble. >> >> Implement dummy reads for the stop state history (SSH) and special >> wakeup (SPWU) registers. This quietens some sxcom errors when skiboot >> boots on p10. >> >> Power9 does not have a QME. >> >> Signed-off-by: Joel Stanley <joel@jms.id.au> > > Nice, already merged but looks good to me. Just one thing... > >> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h >> index a4c9d95dc5d3..9bc64635471e 100644 >> --- a/include/hw/ppc/pnv_xscom.h >> +++ b/include/hw/ppc/pnv_xscom.h >> @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { >> #define PNV10_XSCOM_EC(proc) \ >> ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) >> >> +#define PNV10_XSCOM_QME(chiplet) \ >> + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) >> + >> +/* >> + * Make the region larger by 0x1000 (instead of starting at an offset) so the >> + * modelled addresses start from 0 >> + */ >> +#define PNV10_XSCOM_QME_BASE(core) \ >> + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) >> +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) > > I couldn't work out this comment. FWIW if a comment is inaccurate/wrong it is OK to fix it during the code freeze window. No need to hold it until after 8.1 is released. Thanks, Daniel > > Thanks, > Nick >
On Mon Jul 10, 2023 at 10:25 AM AEST, Joel Stanley wrote: > On Sat, 8 Jul 2023 at 01:17, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > --- a/include/hw/ppc/pnv_xscom.h > > > +++ b/include/hw/ppc/pnv_xscom.h > > > @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { > > > #define PNV10_XSCOM_EC(proc) \ > > > ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) > > > > > > +#define PNV10_XSCOM_QME(chiplet) \ > > > + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) > > > + > > > +/* > > > + * Make the region larger by 0x1000 (instead of starting at an offset) so the > > > + * modelled addresses start from 0 > > > + */ > > > +#define PNV10_XSCOM_QME_BASE(core) \ > > > + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) > > > +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) > > > > I couldn't work out this comment. > > I was trying to describe why we have the + 0x1000. > > Each core sets a bit in the xscom address space, with the first core > setting bit 12, second bit 13, etc. So there's actually no registers > at PNV10_XSCOM_QME_BASE, but so the addressing is easier to follow, I > chose to start the base where we do, and make the region 0x1000 > bigger. > > That was my understanding at least. Ah okay that does make sense. Because you have the core number in the address and core number encoding is one-hot, you don't start at zero or end at 0x4000. It makes sense after you stare at QME model, but maybe could comment the scheme briefly there... and now I think about it, I wonder if QME can actually do broadcast ops to the cores. We don't use that in skiboot but ISTR it could be done, so size might be 0x10000. Thanks, Nick
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h index 77ef00f47a72..c829a18aa9c6 100644 --- a/include/hw/ppc/pnv_core.h +++ b/include/hw/ppc/pnv_core.h @@ -65,6 +65,9 @@ struct PnvQuadClass { const MemoryRegionOps *xscom_ops; uint64_t xscom_size; + + const MemoryRegionOps *xscom_qme_ops; + uint64_t xscom_qme_size; }; #define TYPE_PNV_QUAD "powernv-cpu-quad" @@ -79,5 +82,6 @@ struct PnvQuad { uint32_t quad_id; MemoryRegion xscom_regs; + MemoryRegion xscom_qme_regs; }; #endif /* PPC_PNV_CORE_H */ diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h index a4c9d95dc5d3..9bc64635471e 100644 --- a/include/hw/ppc/pnv_xscom.h +++ b/include/hw/ppc/pnv_xscom.h @@ -127,6 +127,17 @@ struct PnvXScomInterfaceClass { #define PNV10_XSCOM_EC(proc) \ ((0x2 << 16) | ((1 << (3 - (proc))) << 12)) +#define PNV10_XSCOM_QME(chiplet) \ + (PNV10_XSCOM_EQ(chiplet) | (0xE << 16)) + +/* + * Make the region larger by 0x1000 (instead of starting at an offset) so the + * modelled addresses start from 0 + */ +#define PNV10_XSCOM_QME_BASE(core) \ + ((uint64_t) PNV10_XSCOM_QME(PNV10_XSCOM_EQ_CHIPLET(core))) +#define PNV10_XSCOM_QME_SIZE (0x8000 + 0x1000) + #define PNV10_XSCOM_EQ_BASE(core) \ ((uint64_t) PNV10_XSCOM_EQ(PNV10_XSCOM_EQ_CHIPLET(core))) #define PNV10_XSCOM_EQ_SIZE 0x20000 diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 23740f9d0733..eb54f93986df 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1685,6 +1685,9 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp) pnv_xscom_add_subregion(chip, PNV10_XSCOM_EQ_BASE(eq->quad_id), &eq->xscom_regs); + + pnv_xscom_add_subregion(chip, PNV10_XSCOM_QME_BASE(eq->quad_id), + &eq->xscom_qme_regs); } } diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c index 1f244ed181d0..09eb2bf94b9e 100644 --- a/hw/ppc/pnv_core.c +++ b/hw/ppc/pnv_core.c @@ -493,7 +493,67 @@ static const MemoryRegionOps pnv_quad_power10_xscom_ops = { .endianness = DEVICE_BIG_ENDIAN, }; -static void pnv_quad_realize(DeviceState *dev, Error **errp) +#define P10_QME_SPWU_HYP 0x83c +#define P10_QME_SSH_HYP 0x82c + +static uint64_t pnv_qme_power10_xscom_read(void *opaque, hwaddr addr, + unsigned int width) +{ + uint32_t offset = addr >> 3; + uint64_t val = -1; + + /* + * Forth nibble selects the core within a quad, mask it to process read + * for any core. + */ + switch (offset & ~0xf000) { + case P10_QME_SPWU_HYP: + case P10_QME_SSH_HYP: + return 0; + default: + qemu_log_mask(LOG_UNIMP, "%s: unimp read 0x%08x\n", __func__, + offset); + } + + return val; +} + +static void pnv_qme_power10_xscom_write(void *opaque, hwaddr addr, + uint64_t val, unsigned int width) +{ + uint32_t offset = addr >> 3; + + switch (offset) { + default: + qemu_log_mask(LOG_UNIMP, "%s: unimp write 0x%08x\n", __func__, + offset); + } +} + +static const MemoryRegionOps pnv_qme_power10_xscom_ops = { + .read = pnv_qme_power10_xscom_read, + .write = pnv_qme_power10_xscom_write, + .valid.min_access_size = 8, + .valid.max_access_size = 8, + .impl.min_access_size = 8, + .impl.max_access_size = 8, + .endianness = DEVICE_BIG_ENDIAN, +}; + +static void pnv_quad_power9_realize(DeviceState *dev, Error **errp) +{ + PnvQuad *eq = PNV_QUAD(dev); + PnvQuadClass *pqc = PNV_QUAD_GET_CLASS(eq); + char name[32]; + + snprintf(name, sizeof(name), "xscom-quad.%d", eq->quad_id); + pnv_xscom_region_init(&eq->xscom_regs, OBJECT(dev), + pqc->xscom_ops, + eq, name, + pqc->xscom_size); +} + +static void pnv_quad_power10_realize(DeviceState *dev, Error **errp) { PnvQuad *eq = PNV_QUAD(dev); PnvQuadClass *pqc = PNV_QUAD_GET_CLASS(eq); @@ -504,6 +564,12 @@ static void pnv_quad_realize(DeviceState *dev, Error **errp) pqc->xscom_ops, eq, name, pqc->xscom_size); + + snprintf(name, sizeof(name), "xscom-qme.%d", eq->quad_id); + pnv_xscom_region_init(&eq->xscom_qme_regs, OBJECT(dev), + pqc->xscom_qme_ops, + eq, name, + pqc->xscom_qme_size); } static Property pnv_quad_properties[] = { @@ -514,6 +580,9 @@ static Property pnv_quad_properties[] = { static void pnv_quad_power9_class_init(ObjectClass *oc, void *data) { PnvQuadClass *pqc = PNV_QUAD_CLASS(oc); + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = pnv_quad_power9_realize; pqc->xscom_ops = &pnv_quad_power9_xscom_ops; pqc->xscom_size = PNV9_XSCOM_EQ_SIZE; @@ -522,16 +591,21 @@ static void pnv_quad_power9_class_init(ObjectClass *oc, void *data) static void pnv_quad_power10_class_init(ObjectClass *oc, void *data) { PnvQuadClass *pqc = PNV_QUAD_CLASS(oc); + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = pnv_quad_power10_realize; pqc->xscom_ops = &pnv_quad_power10_xscom_ops; pqc->xscom_size = PNV10_XSCOM_EQ_SIZE; + + pqc->xscom_qme_ops = &pnv_qme_power10_xscom_ops; + pqc->xscom_qme_size = PNV10_XSCOM_QME_SIZE; } static void pnv_quad_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); - dc->realize = pnv_quad_realize; device_class_set_props(dc, pnv_quad_properties); dc->user_creatable = false; }
The Quad Management Engine (QME) manages power related settings for its quad. The xscom region is separate from the quad xscoms, therefore a new region is added. The xscoms in a QME select a given core by selecting the forth nibble. Implement dummy reads for the stop state history (SSH) and special wakeup (SPWU) registers. This quietens some sxcom errors when skiboot boots on p10. Power9 does not have a QME. Signed-off-by: Joel Stanley <joel@jms.id.au> --- v2: Clean up extra whitespace Make realize quad specific so power9 doesn't end up with the qme region --- include/hw/ppc/pnv_core.h | 4 ++ include/hw/ppc/pnv_xscom.h | 11 ++++++ hw/ppc/pnv.c | 3 ++ hw/ppc/pnv_core.c | 78 +++++++++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 2 deletions(-)