Re: [PATCH v2 03/10] ppc/pnv: Add a Power11 Pnv11Chip, and a Power11 Machine

[Aditya Gupta <adityag@linux.ibm.com>]

Hello Cédric,

Sorry I missed this mail earlier.
>>>> @@ -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".
>
> Do the Power11 and Power10 processors have the same XSCOM and MMIO
> address spaces ?

Yes. Hence using the same base and sizes.


Thanks,

Aditya Gupta

>
>> 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' ?
>
> yes.
>
>
>>
>>>
>>>>    };
>>>>    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'.
>
> yes.
>
>
> Thanks,
>
> C.
>
>
>>
>> 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;
>>>
>