When enabling user created PHBs (a change reverted by commit 9c10d86fee)
we were handling PHBs created by default versus by the user in different
manners. The only difference between these PHBs is that one will have a
valid phb3->chip that is assigned during pnv_chip_power8_realize(),
while the user created needs to search which chip it belongs to.

Aside from that there shouldn't be any difference. Making the default
PHBs behave in line with the user created ones will make it easier to
re-introduce them later on. It will also make the code easier to follow
since we are dealing with them in equal manner.

The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
This will allow us to assign user created PHBs into it later on. The way
we initilize the default case is now more in line with that would happen
with the user created case: the object is created, parented by the chip
because pnv_xscom_dt() relies on it, and then assigned to the array.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
---
 hw/ppc/pnv.c         | 19 ++++++++++++++-----
 include/hw/ppc/pnv.h |  6 +++++-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 5e3323e950..6ce9e94e05 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
     ics_pic_print_info(&chip8->psi.ics, mon);
 
     for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB3 *phb3 = &chip8->phbs[i];
+        PnvPHB3 *phb3 = chip8->phbs[i];
 
         pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
         ics_pic_print_info(&phb3->lsis, mon);
@@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
     chip8->num_phbs = pcc->num_phbs;
 
     for (i = 0; i < chip8->num_phbs; i++) {
-        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
+        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
+
+        /*
+         * We need the chip to parent the PHB to allow the DT
+         * to build correctly (via pnv_xscom_dt()).
+         *
+         * TODO: the PHB should be parented by a PEC device.
+         */
+        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
+        chip8->phbs[i] = phb3;
     }
 
 }
@@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
 
     /* PHB3 controllers */
     for (i = 0; i < chip8->num_phbs; i++) {
-        PnvPHB3 *phb = &chip8->phbs[i];
+        PnvPHB3 *phb = chip8->phbs[i];
 
         object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
         object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
@@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
         }
 
         for (j = 0; j < chip8->num_phbs; j++) {
-            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
+            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
 
             if (args.ics) {
                 return args.ics;
@@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
         Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
 
         for (j = 0; j < chip8->num_phbs; j++) {
-            PnvPHB3 *phb3 = &chip8->phbs[j];
+            PnvPHB3 *phb3 = chip8->phbs[j];
 
             ics_resend(&phb3->lsis);
             ics_resend(ICS(&phb3->msis));
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 033890a23f..11f1089289 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -80,7 +80,11 @@ struct Pnv8Chip {
     PnvHomer     homer;
 
 #define PNV8_CHIP_PHB3_MAX 4
-    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
+    /*
+     * The array is used to allow quick access to the phbs by
+     * pnv_ics_get_child() and pnv_ics_resend_child().
+     */
+    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
     uint32_t     num_phbs;
 
     XICSFabric    *xics;
-- 
2.36.1

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> When enabling user created PHBs (a change reverted by commit 9c10d86fee)
> we were handling PHBs created by default versus by the user in different
> manners. The only difference between these PHBs is that one will have a
> valid phb3->chip that is assigned during pnv_chip_power8_realize(),
> while the user created needs to search which chip it belongs to.
> 
> Aside from that there shouldn't be any difference. Making the default
> PHBs behave in line with the user created ones will make it easier to
> re-introduce them later on. It will also make the code easier to follow
> since we are dealing with them in equal manner.
> 
> The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
> This will allow us to assign user created PHBs into it later on. The way
> we initilize the default case is now more in line with that would happen
> with the user created case: the object is created, parented by the chip
> because pnv_xscom_dt() relies on it, and then assigned to the array.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---


This patch is more prep work for the user-created device instead of 
general cleanup like the previous ones, but I don't see anything wrong 
with it. So:

Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred



>   hw/ppc/pnv.c         | 19 ++++++++++++++-----
>   include/hw/ppc/pnv.h |  6 +++++-
>   2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 5e3323e950..6ce9e94e05 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>       ics_pic_print_info(&chip8->psi.ics, mon);
>   
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb3 = &chip8->phbs[i];
> +        PnvPHB3 *phb3 = chip8->phbs[i];
>   
>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
>           ics_pic_print_info(&phb3->lsis, mon);
> @@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
>       chip8->num_phbs = pcc->num_phbs;
>   
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
> +        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
> +
> +        /*
> +         * We need the chip to parent the PHB to allow the DT
> +         * to build correctly (via pnv_xscom_dt()).
> +         *
> +         * TODO: the PHB should be parented by a PEC device.
> +         */
> +        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
> +        chip8->phbs[i] = phb3;
>       }
>   
>   }
> @@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>   
>       /* PHB3 controllers */
>       for (i = 0; i < chip8->num_phbs; i++) {
> -        PnvPHB3 *phb = &chip8->phbs[i];
> +        PnvPHB3 *phb = chip8->phbs[i];
>   
>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
> @@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>           }
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
> +            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
>   
>               if (args.ics) {
>                   return args.ics;
> @@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>   
>           for (j = 0; j < chip8->num_phbs; j++) {
> -            PnvPHB3 *phb3 = &chip8->phbs[j];
> +            PnvPHB3 *phb3 = chip8->phbs[j];
>   
>               ics_resend(&phb3->lsis);
>               ics_resend(ICS(&phb3->msis));
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 033890a23f..11f1089289 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -80,7 +80,11 @@ struct Pnv8Chip {
>       PnvHomer     homer;
>   
>   #define PNV8_CHIP_PHB3_MAX 4
> -    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
> +    /*
> +     * The array is used to allow quick access to the phbs by
> +     * pnv_ics_get_child() and pnv_ics_resend_child().
> +     */
> +    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
>       uint32_t     num_phbs;
>   
>       XICSFabric    *xics;

On 6/14/22 06:53, Frederic Barrat wrote:
> 
> 
> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>> When enabling user created PHBs (a change reverted by commit 9c10d86fee)
>> we were handling PHBs created by default versus by the user in different
>> manners. The only difference between these PHBs is that one will have a
>> valid phb3->chip that is assigned during pnv_chip_power8_realize(),
>> while the user created needs to search which chip it belongs to.
>>
>> Aside from that there shouldn't be any difference. Making the default
>> PHBs behave in line with the user created ones will make it easier to
>> re-introduce them later on. It will also make the code easier to follow
>> since we are dealing with them in equal manner.
>>
>> The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
>> This will allow us to assign user created PHBs into it later on. The way
>> we initilize the default case is now more in line with that would happen
>> with the user created case: the object is created, parented by the chip
>> because pnv_xscom_dt() relies on it, and then assigned to the array.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>> ---
> 
> 
> This patch is more prep work for the user-created device instead of general cleanup like the previous ones, but I don't see anything wrong with it. So:
> 
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


I've been thinking about it and I guess I could do better with this
and the proxy pnv-phb series that is already in v2. What I'm thinking
is:

- crop patches 8-11 from this series. Patches 1-7 would be the prep cleanup
series;

- split the pnv-phb series in two:

   - first series will just introduce the pnv-phb devices and consolidate the
root ports. We're going to deal just with default devices. No consideration
about future user-created devices will be made;

   - a second series would then re-introduce user creatable phbs and root ports.
Patches 8-11 of this series would be handled in this second patch set since it's
closely related to user devices.


Does that sound fair?


Thanks,


Daniel




> 
>    Fred
> 
> 
> 
>>   hw/ppc/pnv.c         | 19 ++++++++++++++-----
>>   include/hw/ppc/pnv.h |  6 +++++-
>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 5e3323e950..6ce9e94e05 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>>       ics_pic_print_info(&chip8->psi.ics, mon);
>>       for (i = 0; i < chip8->num_phbs; i++) {
>> -        PnvPHB3 *phb3 = &chip8->phbs[i];
>> +        PnvPHB3 *phb3 = chip8->phbs[i];
>>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
>>           ics_pic_print_info(&phb3->lsis, mon);
>> @@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
>>       chip8->num_phbs = pcc->num_phbs;
>>       for (i = 0; i < chip8->num_phbs; i++) {
>> -        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>> +        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
>> +
>> +        /*
>> +         * We need the chip to parent the PHB to allow the DT
>> +         * to build correctly (via pnv_xscom_dt()).
>> +         *
>> +         * TODO: the PHB should be parented by a PEC device.
>> +         */
>> +        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
>> +        chip8->phbs[i] = phb3;
>>       }
>>   }
>> @@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>>       /* PHB3 controllers */
>>       for (i = 0; i < chip8->num_phbs; i++) {
>> -        PnvPHB3 *phb = &chip8->phbs[i];
>> +        PnvPHB3 *phb = chip8->phbs[i];
>>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
>> @@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>>           }
>>           for (j = 0; j < chip8->num_phbs; j++) {
>> -            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
>> +            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
>>               if (args.ics) {
>>                   return args.ics;
>> @@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>>           for (j = 0; j < chip8->num_phbs; j++) {
>> -            PnvPHB3 *phb3 = &chip8->phbs[j];
>> +            PnvPHB3 *phb3 = chip8->phbs[j];
>>               ics_resend(&phb3->lsis);
>>               ics_resend(ICS(&phb3->msis));
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 033890a23f..11f1089289 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -80,7 +80,11 @@ struct Pnv8Chip {
>>       PnvHomer     homer;
>>   #define PNV8_CHIP_PHB3_MAX 4
>> -    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>> +    /*
>> +     * The array is used to allow quick access to the phbs by
>> +     * pnv_ics_get_child() and pnv_ics_resend_child().
>> +     */
>> +    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
>>       uint32_t     num_phbs;
>>       XICSFabric    *xics;

On 6/14/22 17:39, Daniel Henrique Barboza wrote:
> 
> 
> On 6/14/22 06:53, Frederic Barrat wrote:
>>
>>
>> On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
>>> When enabling user created PHBs (a change reverted by commit 9c10d86fee)
>>> we were handling PHBs created by default versus by the user in different
>>> manners. The only difference between these PHBs is that one will have a
>>> valid phb3->chip that is assigned during pnv_chip_power8_realize(),
>>> while the user created needs to search which chip it belongs to.
>>>
>>> Aside from that there shouldn't be any difference. Making the default
>>> PHBs behave in line with the user created ones will make it easier to
>>> re-introduce them later on. It will also make the code easier to follow
>>> since we are dealing with them in equal manner.
>>>
>>> The first step is to turn chip8->phbs[] into a PnvPHB3 pointer array.
>>> This will allow us to assign user created PHBs into it later on. The way
>>> we initilize the default case is now more in line with that would happen
>>> with the user created case: the object is created, parented by the chip
>>> because pnv_xscom_dt() relies on it, and then assigned to the array.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
>>> ---
>>
>>
>> This patch is more prep work for the user-created device instead of general cleanup like the previous ones, but I don't see anything wrong with it. So:
>>
>> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> 
> I've been thinking about it and I guess I could do better with this
> and the proxy pnv-phb series that is already in v2. What I'm thinking
> is:
> 
> - crop patches 8-11 from this series. Patches 1-7 would be the prep cleanup
> series;
> 
> - split the pnv-phb series in two:
> 
>    - first series will just introduce the pnv-phb devices and consolidate the
> root ports. We're going to deal just with default devices. No consideration
> about future user-created devices will be made;

Yes. From what I have read, this looks very feasible with a v2.

Thanks,

C.

> 
>    - a second series would then re-introduce user creatable phbs and root ports.
> Patches 8-11 of this series would be handled in this second patch set since it's
> closely related to user devices.
> 
> 
> Does that sound fair?
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
>>
>>    Fred
>>
>>
>>
>>>   hw/ppc/pnv.c         | 19 ++++++++++++++-----
>>>   include/hw/ppc/pnv.h |  6 +++++-
>>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 5e3323e950..6ce9e94e05 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -660,7 +660,7 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>>>       ics_pic_print_info(&chip8->psi.ics, mon);
>>>       for (i = 0; i < chip8->num_phbs; i++) {
>>> -        PnvPHB3 *phb3 = &chip8->phbs[i];
>>> +        PnvPHB3 *phb3 = chip8->phbs[i];
>>>           pnv_phb3_msi_pic_print_info(&phb3->msis, mon);
>>>           ics_pic_print_info(&phb3->lsis, mon);
>>> @@ -1149,7 +1149,16 @@ static void pnv_chip_power8_instance_init(Object *obj)
>>>       chip8->num_phbs = pcc->num_phbs;
>>>       for (i = 0; i < chip8->num_phbs; i++) {
>>> -        object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
>>> +        PnvPHB3 *phb3 = PNV_PHB3(object_new(TYPE_PNV_PHB3));
>>> +
>>> +        /*
>>> +         * We need the chip to parent the PHB to allow the DT
>>> +         * to build correctly (via pnv_xscom_dt()).
>>> +         *
>>> +         * TODO: the PHB should be parented by a PEC device.
>>> +         */
>>> +        object_property_add_child(obj, "phb[*]", OBJECT(phb3));
>>> +        chip8->phbs[i] = phb3;
>>>       }
>>>   }
>>> @@ -1278,7 +1287,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>>>       /* PHB3 controllers */
>>>       for (i = 0; i < chip8->num_phbs; i++) {
>>> -        PnvPHB3 *phb = &chip8->phbs[i];
>>> +        PnvPHB3 *phb = chip8->phbs[i];
>>>           object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
>>>           object_property_set_int(OBJECT(phb), "chip-id", chip->chip_id,
>>> @@ -1963,7 +1972,7 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>>>           }
>>>           for (j = 0; j < chip8->num_phbs; j++) {
>>> -            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
>>> +            pnv_ics_get_phb_ics(chip8->phbs[j], &args);
>>>               if (args.ics) {
>>>                   return args.ics;
>>> @@ -1996,7 +2005,7 @@ static void pnv_ics_resend(XICSFabric *xi)
>>>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>>>           for (j = 0; j < chip8->num_phbs; j++) {
>>> -            PnvPHB3 *phb3 = &chip8->phbs[j];
>>> +            PnvPHB3 *phb3 = chip8->phbs[j];
>>>               ics_resend(&phb3->lsis);
>>>               ics_resend(ICS(&phb3->msis));
>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>> index 033890a23f..11f1089289 100644
>>> --- a/include/hw/ppc/pnv.h
>>> +++ b/include/hw/ppc/pnv.h
>>> @@ -80,7 +80,11 @@ struct Pnv8Chip {
>>>       PnvHomer     homer;
>>>   #define PNV8_CHIP_PHB3_MAX 4
>>> -    PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
>>> +    /*
>>> +     * The array is used to allow quick access to the phbs by
>>> +     * pnv_ics_get_child() and pnv_ics_resend_child().
>>> +     */
>>> +    PnvPHB3      *phbs[PNV8_CHIP_PHB3_MAX];
>>>       uint32_t     num_phbs;
>>>       XICSFabric    *xics;

On 14/06/2022 17:39, Daniel Henrique Barboza wrote:
> I've been thinking about it and I guess I could do better with this
> and the proxy pnv-phb series that is already in v2. What I'm thinking
> is:
> 
> - crop patches 8-11 from this series. Patches 1-7 would be the prep cleanup
> series;
> 
> - split the pnv-phb series in two:
> 
>    - first series will just introduce the pnv-phb devices and 
> consolidate the
> root ports. We're going to deal just with default devices. No consideration
> about future user-created devices will be made;
> 
>    - a second series would then re-introduce user creatable phbs and 
> root ports.
> Patches 8-11 of this series would be handled in this second patch set 
> since it's
> closely related to user devices.
> 
> 
> Does that sound fair?


Sounds good to me. That should keep series smaller and easier to review 
and merge.

   Fred