Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 3/2/23 19:21, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
> 
> On 2/3/23 17:37, Frederic Barrat wrote:
>> Add an array on the PEC object to keep track of the PHBs which are
>> instantiated. The array can be sparsely populated when using
>> user-created PHBs. It will be useful for the next patch to only export
>> instantiated PHBs in the device tree.
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
>>   hw/ppc/pnv.c                   |  1 +
>>   include/hw/pci-host/pnv_phb4.h |  2 ++
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 43267a428f..97c06bb0a0 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
> 
>> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>> +                                           int stack_no,
>> +                                           Error **errp)
>>   {
>>       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
>>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
>> @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>>                               &error_fatal);
>>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>> -        return;
>> +        return NULL;
>>       }
>> +    return phb;
>>   }
> 
> 
>> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
>> index 28d61b96c7..0b72ef1471 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
>>       /* PHBs */
>>       uint32_t num_phbs;
>> +#define MAX_PHBS_PER_PEC        3
>> +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
>>       PnvChip *chip;
>>   };
> 
>  From QOM PoV, better would be to 'embed' the PnvPHB structure (not only
> a pointer to it), and initialize the PnvPHB instance calling
> object_initialize_child() instead of qdev_new().


We were doing something similar in this PHB controllers one year ago. The
reason we moved to host pointers instead of using embed structures was to
allow user-created phbs. There's no way of telling beforehand if the user
wants one, two or three PHBs in the PEC.


Thanks,

Daniel

> 
> See for example the recent conversion of OHCISysBusState in commit
> 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset").
> 
> Regards,
> 
> Phil.