[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 03/10] ppc/pnv: Add a Power11 Pnv11Chip, and a Power11 Mac
From: |
Aditya Gupta |
Subject: |
Re: [PATCH v2 03/10] ppc/pnv: Add a Power11 Pnv11Chip, and a Power11 Machine |
Date: |
Fri, 26 Apr 2024 23:04:08 +0530 |
Hello Cédric,
> >
> > <...snip...>
> >
> > - * Multi processor support for POWER8, POWER8NVL and POWER9.
> > + * Multi processor support for POWER8, POWER8NVL, POWER9, POWER10 and
> > Power11.
>
> POWER10 -> Power10. Don't ask me why.
Sure, got it !
>
> > * XSCOM, serial communication sideband bus to configure chiplets.
> > * Simple LPC Controller.
> > * Processor Service Interface (PSI) Controller.
> > - * Interrupt Controller, XICS (POWER8) and XIVE (POWER9) and XIVE2
> > (Power10).
> > + * Interrupt Controller, XICS (POWER8) and XIVE (POWER9) and XIVE2
> > (Power10 &
> > + Power11).
> > * POWER8 PHB3 PCIe Host bridge and POWER9 PHB4 PCIe Host bridge.
> > * Simple OCC is an on-chip micro-controller used for power management
> > tasks.
> > * iBT device to handle BMC communication, with the internal BMC simulator
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 74e7908e5ffb..06e272f3bdd3 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -431,13 +431,27 @@ static const uint8_t pa_features_31[] = { 74, 0,
> > static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt)
> > {
> > - static const char compat[] = "ibm,power10-xscom\0ibm,xscom";
> > + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> > + static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom";
> > + static const char compat_p11[] = "ibm,power11-xscom\0ibm,xscom";
> > +
> > + PnvChipType chip_type = pcc->chip_type;
> > + const char *compat;
> > + int compat_len;
> > int i;
> > + if (chip_type == PNV_TYPE_POWER11) {
> > + compat = compat_p11;
> > + compat_len = sizeof(compat_p11);
> > + } else {
> > + compat = compat_p10;
> > + compat_len = sizeof(compat_p10);
> > + }
>
> please introduce a pnv_chip_power11_dt_populate() routine instead.
Okay.
>
> > pnv_dt_xscom(chip, fdt, 0,
> > cpu_to_be64(PNV10_XSCOM_BASE(chip)),
> > cpu_to_be64(PNV10_XSCOM_SIZE),
> > - compat, sizeof(compat));
> > + compat, compat_len);
> > for (i = 0; i < chip->nr_cores; i++) {
> > PnvCore *pnv_core = chip->cores[i];
> > @@ -1288,6 +1302,8 @@ static void pnv_chip_power10_intc_print_info(PnvChip
> > *chip, PowerPCCPU *cpu,
> > #define POWER10_CORE_MASK (0xffffffffffffffull)
> > +#define POWER11_CORE_MASK (0xffffffffffffffull)
> > +
> > static void pnv_chip_power8_instance_init(Object *obj)
> > {
> > Pnv8Chip *chip8 = PNV8_CHIP(obj);
> > @@ -1838,6 +1854,7 @@ static void pnv_chip_power10_instance_init(Object
> > *obj)
> > static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp)
> > {
> > PnvChip *chip = PNV_CHIP(chip10);
> > + PnvChipClass *chip_class = PNV_CHIP_GET_CLASS(chip);
> > int i;
> > chip10->nr_quads = DIV_ROUND_UP(chip->nr_cores, 4);
> > @@ -1846,7 +1863,11 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip
> > *chip10, Error **errp)
> > for (i = 0; i < chip10->nr_quads; i++) {
> > PnvQuad *eq = &chip10->quads[i];
> > - pnv_chip_quad_realize_one(chip, eq, chip->cores[i * 4],
> > + if (chip_class->chip_type == PNV_TYPE_POWER11)
> > + pnv_chip_quad_realize_one(chip, eq, chip->cores[i * 4],
> > + PNV_QUAD_TYPE_NAME("power11"));
> > + else
> > + pnv_chip_quad_realize_one(chip, eq, chip->cores[i * 4],
> > PNV_QUAD_TYPE_NAME("power10"));
>
>
> Please change introduce a new pnv_chip_quad_realize() routine taking an
> extra type_name argument.
Sure, can do it. But as it's called from 'pnv_chip_power10_realize',
might require a 'pnv_chip_power11_realize' function also, so it can pass
type_name as "power11" vs "power10".
Will do it.
>
>
> > pnv_xscom_add_subregion(chip, PNV10_XSCOM_EQ_BASE(eq->quad_id),
> > @@ -2116,6 +2137,35 @@ static void pnv_chip_power10_class_init(ObjectClass
> > *klass, void *data)
> > &k->parent_realize);
> > }
> > +static void pnv_chip_power11_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + PnvChipClass *k = PNV_CHIP_CLASS(klass);
> > +
> > + static const int i2c_ports_per_engine[PNV10_CHIP_MAX_I2C] = {14, 14,
> > 2, 16};
> > +
> > + k->chip_cfam_id = 0x120da04900008000ull; /* P11 (with NX) */
> > + k->chip_type = PNV_TYPE_POWER11;
> > + k->cores_mask = POWER11_CORE_MASK;
> > + k->chip_pir = pnv_chip_pir_p10;
> > + k->intc_create = pnv_chip_power10_intc_create;
> > + k->intc_reset = pnv_chip_power10_intc_reset;
> > + k->intc_destroy = pnv_chip_power10_intc_destroy;
> > + k->intc_print_info = pnv_chip_power10_intc_print_info;
> > + k->isa_create = pnv_chip_power10_isa_create;
> > + k->dt_populate = pnv_chip_power10_dt_populate;
> > + k->pic_print_info = pnv_chip_power10_pic_print_info;
> > + k->xscom_core_base = pnv_chip_power10_xscom_core_base;
> > + k->xscom_pcba = pnv_chip_power10_xscom_pcba;
> > + dc->desc = "PowerNV Chip POWER11";
> > + k->num_pecs = PNV10_CHIP_MAX_PEC;
> > + k->i2c_num_engines = PNV10_CHIP_MAX_I2C;
> > + k->i2c_ports_per_engine = i2c_ports_per_engine;
> > +
> > + device_class_set_parent_realize(dc, pnv_chip_power10_realize,
> > + &k->parent_realize);
> > +}
> > +
> > static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
> > {
> > PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> > @@ -2510,6 +2560,22 @@ static void
> > pnv_machine_p10_rainier_class_init(ObjectClass *oc, void *data)
> > pmc->i2c_init = pnv_rainier_i2c_init;
> > }
> > +static void pnv_machine_power11_class_init(ObjectClass *oc, void *data)
> > +{
> > + MachineClass *mc = MACHINE_CLASS(oc);
> > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > + static const char compat[] = "qemu,powernv11\0ibm,powernv";
> > +
> > + /* do power10_class_init as p11 core is same as p10 */
> > + pnv_machine_p10_common_class_init(oc, data);
> > +
> > + mc->desc = "IBM PowerNV (Non-Virtualized) POWER11";
> > + mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power11");
>
> This should be using a versionned CPU "power11_vx.y".
Okay.
>
> > +
> > + pmc->compat = compat;
> > + pmc->compat_size = sizeof(compat);
> > +}
> > +
> > static bool pnv_machine_get_hb(Object *obj, Error **errp)
> > {
> > PnvMachineState *pnv = PNV_MACHINE(obj);
> > @@ -2613,7 +2679,23 @@ static void pnv_machine_class_init(ObjectClass *oc,
> > void *data)
> > .parent = TYPE_PNV10_CHIP, \
> > }
> > +#define DEFINE_PNV11_CHIP_TYPE(type, class_initfn) \
> > + { \
> > + .name = type, \
> > + .class_init = class_initfn, \
> > + .parent = TYPE_PNV11_CHIP, \
> > + }
> > +
> > static const TypeInfo types[] = {
> > + {
> > + .name = MACHINE_TYPE_NAME("powernv11"),
> > + .parent = TYPE_PNV_MACHINE,
> > + .class_init = pnv_machine_power11_class_init,
> > + .interfaces = (InterfaceInfo[]) {
> > + { TYPE_XIVE_FABRIC },
> > + { },
> > + },
> > + },
> > {
> > .name = MACHINE_TYPE_NAME("powernv10-rainier"),
> > .parent = MACHINE_TYPE_NAME("powernv10"),
> > @@ -2668,6 +2750,16 @@ static const TypeInfo types[] = {
> > .abstract = true,
> > },
> > + /*
> > + * P11 chip and variants
> > + */
> > + {
> > + .name = TYPE_PNV11_CHIP,
> > + .parent = TYPE_PNV10_CHIP,
> > + .instance_size = sizeof(Pnv11Chip),
> > + },
> > + DEFINE_PNV11_CHIP_TYPE(TYPE_PNV_CHIP_POWER11,
> > pnv_chip_power11_class_init),
> > +
> > /*
> > * P10 chip and variants
> > */
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index f40ab721d6fc..4522655793da 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -402,7 +402,16 @@ static const TypeInfo pnv_core_infos[] = {
> > DEFINE_PNV_CORE_TYPE(power8, "power8_v2.0"),
> > DEFINE_PNV_CORE_TYPE(power8, "power8nvl_v1.0"),
> > DEFINE_PNV_CORE_TYPE(power9, "power9_v2.2"),
> > - DEFINE_PNV_CORE_TYPE(power10, "power10_v2.0"),
> > + {
> > + .parent = TYPE_PNV_CORE,
> > + .name = PNV_CORE_TYPE_NAME("power10_v2.0"),
> > + .class_init = pnv_core_power10_class_init,
> > + .class_base_init = pnv_core_power10_class_init,
> > + },
>
> hmm, what is this change ?
I expanded the macro, to add '.class_base_init', as .class_base_init
will be run by QEMU during initialisation of any child classes also. And
I wanted to let QEMU call 'pnv_core_power10_class_init' when it
initialises 'power11' type.
>
>
> > + {
> > + .parent = PNV_CORE_TYPE_NAME("power10_v2.0"),
> > + .name = PNV_CORE_TYPE_NAME("power11"),
> > + }
>
> nope. please use DEFINE_PNV_CORE_TYPE()
Okay, then, should I have a 'pnv_core_power11_class_init' call
'pnv_core_power10_class_init' ?
>
> > };
> > DEFINE_TYPES(pnv_core_infos)
> > @@ -633,6 +642,11 @@ static const TypeInfo pnv_quad_infos[] = {
> > .parent = TYPE_PNV_QUAD,
> > .name = PNV_QUAD_TYPE_NAME("power10"),
> > .class_init = pnv_quad_power10_class_init,
> > + .class_base_init = pnv_quad_power10_class_init,
> > + },
> > + {
> > + .parent = PNV_QUAD_TYPE_NAME("power10"),
> > + .name = PNV_QUAD_TYPE_NAME("power11"),
>
> hmm, I'd rather introduce pnv_quad_power11_class_init.
Same, in that case, I can have a 'pnv_quad_power11_class_init' which
just calls 'pnv_quad_power10_class_init'.
Any comments ?
> > <...snip...>
> >
> > +struct Pnv11Chip {
> > + /*< private >*/
> > + Pnv10Chip parent_obj;
>
> Can't you use a typedef instead ?
Yes, makes sense. Thanks Cédric.
- Aditya Gupta
>
> Thanks,
>
> C.
>
>
> > +};
> > +
> > struct PnvChipClass {
> > /*< private >*/
> > SysBusDeviceClass parent_class;
>
[PATCH v2 04/10] ppc/pnv: Add HOMER for POWER11, Aditya Gupta, 2024/04/26