Re: [PATCH v5 7/8] ppc/pnv: Add a Power11 Pnv11Chip, and a Power11 Machine

["Cédric Le Goater" <clg@kaod.org>]

On 3/24/25 08:13, Aditya Gupta wrote:
> Hi Cedric,
> 
> On 09/03/25 19:38, Cédric Le Goater wrote:
>> On 3/8/25 21:51, Aditya Gupta wrote:
>>> <...snip...>
>>>
>>>   static void pnv_chip_power8_instance_init(Object *obj)
>>>   {
>>>       Pnv8Chip *chip8 = PNV8_CHIP(obj);
>>> @@ -1966,6 +1996,20 @@ static void pnv_chip_power10_instance_init(Object *obj)
>>>       PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
>>>       int i;
>>>   +    /*
>>> +     * Power11 declares Power10 as it's parent class, to be able to reuse
>>> +     * most of the Power10 code.
>>> +     * But this causes Power10 and Power11's both instance init to be
>>> +     * called for PowerNV11 chip
>>> +     *
>>> +     * Skip initialising Power10 specific child objects, if the chip is
>>> +     * Power11 chip, in which case power11's instance init will initialise
>>> +     * the child objects
>>> +     */
>>> +    if (!strcmp(object_get_typename(obj), TYPE_PNV_CHIP_POWER11)) {
>>> +        return;
>>> +    }
>>> +
>>
>> This is a hack !  Please duplicate the code like done for other chips.
>>
>>
>>> <...snip...>
>>>
>>>   +    /*
>>> +     * P11 chip and variants
>>> +     */
>>> +    {
>>> +        .name          = TYPE_PNV11_CHIP,
>>> +        .parent        = TYPE_PNV10_CHIP,
>>
>> why is the parent not TYPE_PNV_CHIP like the other chips ?
>>
>> This is a hack which can be easily avoided with more work :)
> 
> 
> When I do this, every function using Pnv10Chip* will need to be duplicated to use Pnv11Chip*.
> 
> I will do that.
> 
> But what do you say about changing Pnv10 chip to Pnv1x chip ? It may only be used by Pnv10 and Pnv11, not future chips. But is that unnecessary for just two versions of the chip ?
> 
> Just tried to think of some ways to reduce duplication, or having different files such as pnv10.c, pnv11.c ? I am not sure because these might be unnecessary things too.

Please open code (and duplicate) first. Then we will see how we
can make things common. Checking the typename in the instance_init
handleris a hack not for upstream.

Thanks,

C.