Hi All,

PowerNV fails to boot in multichip systems due to some misinterpretation
and mapping in Homer/Occ device models, this patchset fixes the
following,

 - Homer size is 4MB per chip and Occ common area size is 8MB
 - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
   return appropriate value
 - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
   currently
 - OCC common area is shared across chips and should be mapped only once
   for multichip systems

Request for your review and suggestions to make it better. I would like to
thank Cedric for his time and help to figure out the issues.

Balamuruhan S (5):
  hw/ppc/pnv: incorrect homer and occ common area size
  hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
    sizes
  hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
  hw/ppc/pnv_xscom: occ common area to be mapped only once
  hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image

 hw/ppc/pnv_occ.c     |  2 +-
 hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
 include/hw/ppc/pnv.h | 12 ++++++++----
 3 files changed, 36 insertions(+), 15 deletions(-)

-- 
2.14.5

Hello,

On 19/11/2019 18:50, Balamuruhan S wrote:
> Hi All,
> 
> PowerNV fails to boot in multichip systems due to some misinterpretation
> and mapping in Homer/Occ device models, this patchset fixes the
> following,
> 
>  - Homer size is 4MB per chip and Occ common area size is 8MB
>  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
>    return appropriate value
>  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
>    currently
>  - OCC common area is shared across chips and should be mapped only once
>    for multichip systems

The first thing to address is the HOMER XSCOM region. 

Introduce an empty skeleton for P8 and P9 with different mem ops handers
because the registers have a different layout. From there, add the support
for the different PBA* regs and move them out from the default XSCOM
handlers. That should fix most of the current problems and it will provide 
a nice framework for future extensions.

Why not add the associated HOMER MMIO region while we are it ? the PBA
registers have all the definitions we need and it will gives us access
to the pstate table.


Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
not clear. Please check skiboot. I think a MMIO region should be enough
because this is how sensor data from the OCC is accessed. 

On that topic, we could define properties on the PnvOCC model for each 
sensor and tune the value from the QEMU monitor. It really shouldn't be
too complex.

Also the same address is used, we should only map it once but we need 
to invent something to know from which chip it is accessed. 


C.


> 
> Request for your review and suggestions to make it better. I would like to
> thank Cedric for his time and help to figure out the issues.
>
> Balamuruhan S (5):
>   hw/ppc/pnv: incorrect homer and occ common area size
>   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
>     sizes
>   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
>   hw/ppc/pnv_xscom: occ common area to be mapped only once
>   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
> 
>  hw/ppc/pnv_occ.c     |  2 +-
>  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
>  include/hw/ppc/pnv.h | 12 ++++++++----
>  3 files changed, 36 insertions(+), 15 deletions(-)
> 

On Wed, Nov 20, 2019 at 08:46:30AM +0100, Cédric Le Goater wrote:
> Hello,
> 
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > Hi All,
> > 
> > PowerNV fails to boot in multichip systems due to some misinterpretation
> > and mapping in Homer/Occ device models, this patchset fixes the
> > following,
> > 
> >  - Homer size is 4MB per chip and Occ common area size is 8MB
> >  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
> >    return appropriate value
> >  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
> >    currently
> >  - OCC common area is shared across chips and should be mapped only once
> >    for multichip systems
> 
> The first thing to address is the HOMER XSCOM region. 
> 
> Introduce an empty skeleton for P8 and P9 with different mem ops handers
> because the registers have a different layout. From there, add the support
> for the different PBA* regs and move them out from the default XSCOM
> handlers. That should fix most of the current problems and it will provide 
> a nice framework for future extensions.

sure, I will work on it.

> 
> Why not add the associated HOMER MMIO region while we are it ? the PBA
> registers have all the definitions we need and it will gives us access
> to the pstate table.

so, idea is to have HOMER MMIO for us to use it accessing pstate table / data
and HOMER XSCOM for homer associated xscom access for PBA* registers to
P8 and P9 respectively.

> 
> 
> Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
> not clear. Please check skiboot. I think a MMIO region should be enough
> because this is how sensor data from the OCC is accessed.

Okay, I will do the change for OCC to use MMIO, and will check skiboot
for making it better.

> 
> On that topic, we could define properties on the PnvOCC model for each 
> sensor and tune the value from the QEMU monitor. It really shouldn't be
> too complex.

How can we tune value from QEMU monitor ? This is new to me and will
need to check it. I remember you have advised this with the error
injection framework patches and Rashmica's patch that provides way to
use Qemu monitor to feed data, but I need to do some study.

> 
> Also the same address is used, we should only map it once but we need 
> to invent something to know from which chip it is accessed.

This is something need to check how real hardware handles it while
accessing shared occ region from different chip and think how to make it
for us.

Thanks a lot!

-- Bala

> 
> 
> C.
> 
> 
> > 
> > Request for your review and suggestions to make it better. I would like to
> > thank Cedric for his time and help to figure out the issues.
> >
> > Balamuruhan S (5):
> >   hw/ppc/pnv: incorrect homer and occ common area size
> >   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
> >     sizes
> >   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
> >   hw/ppc/pnv_xscom: occ common area to be mapped only once
> >   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
> > 
> >  hw/ppc/pnv_occ.c     |  2 +-
> >  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
> >  include/hw/ppc/pnv.h | 12 ++++++++----
> >  3 files changed, 36 insertions(+), 15 deletions(-)
> > 
> 

On 21/11/2019 10:11, Balamuruhan S wrote:
> On Wed, Nov 20, 2019 at 08:46:30AM +0100, Cédric Le Goater wrote:
>> Hello,
>>
>> On 19/11/2019 18:50, Balamuruhan S wrote:
>>> Hi All,
>>>
>>> PowerNV fails to boot in multichip systems due to some misinterpretation
>>> and mapping in Homer/Occ device models, this patchset fixes the
>>> following,
>>>
>>>  - Homer size is 4MB per chip and Occ common area size is 8MB
>>>  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
>>>    return appropriate value
>>>  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
>>>    currently
>>>  - OCC common area is shared across chips and should be mapped only once
>>>    for multichip systems
>>
>> The first thing to address is the HOMER XSCOM region. 
>>
>> Introduce an empty skeleton for P8 and P9 with different mem ops handers
>> because the registers have a different layout. From there, add the support
>> for the different PBA* regs and move them out from the default XSCOM
>> handlers. That should fix most of the current problems and it will provide 
>> a nice framework for future extensions.
> 
> sure, I will work on it.
> 
>>
>> Why not add the associated HOMER MMIO region while we are it ? the PBA
>> registers have all the definitions we need and it will gives us access
>> to the pstate table.
> 
> so, idea is to have HOMER MMIO for us to use it accessing pstate table / data
> and HOMER XSCOM for homer associated xscom access for PBA* registers to
> P8 and P9 respectively.

yes. 

>> Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
>> not clear. Please check skiboot. I think a MMIO region should be enough
>> because this is how sensor data from the OCC is accessed.
> 
> Okay, I will do the change for OCC to use MMIO, and will check skiboot
> for making it better.
> 
>>
>> On that topic, we could define properties on the PnvOCC model for each 
>> sensor and tune the value from the QEMU monitor. It really shouldn't be
>> too complex.
> 
> How can we tune value from QEMU monitor ? This is new to me and will
> need to check it. I remember you have advised this with the error
> injection framework patches and Rashmica's patch that provides way to
> use Qemu monitor to feed data, but I need to do some study.


See Joel's patch which has a simple example :  

   patchwork.ozlabs.org/patch/1196519

It simply generates object properties : 


+    for (led = 0; led < s->nr_leds; led++) {
+        char *name;
+
+        name = g_strdup_printf("led%d", led);
+        object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
+                            NULL, NULL, NULL);
+    }

with defined get and set accessors. 

We could do the same for the OCC sensors with a table describing the 
sensor layout. Accessors would just simply update the table. we could
even trigger the OCC interrupt if needed.

This is the initial table :

  https://github.com/open-power/occ/blob/master/src/occ_405/sensor/sensor_info.c

Linux should be able to grab the values through hwmon just as on real HW.
This is the case today for the DTS.

>>
>> Also the same address is used, we should only map it once but we need 
>> to invent something to know from which chip it is accessed.
> 
> This is something need to check how real hardware handles it while
> accessing shared occ region from different chip and think how to make it
> for us.

Yes. I suppose there is some chip id in the powerbus message.

C.

  
> 
> Thanks a lot!
> 
> -- Bala
> 
>>
>>
>> C.
>>
>>
>>>
>>> Request for your review and suggestions to make it better. I would like to
>>> thank Cedric for his time and help to figure out the issues.
>>>
>>> Balamuruhan S (5):
>>>   hw/ppc/pnv: incorrect homer and occ common area size
>>>   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
>>>     sizes
>>>   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
>>>   hw/ppc/pnv_xscom: occ common area to be mapped only once
>>>   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
>>>
>>>  hw/ppc/pnv_occ.c     |  2 +-
>>>  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
>>>  include/hw/ppc/pnv.h | 12 ++++++++----
>>>  3 files changed, 36 insertions(+), 15 deletions(-)
>>>
>>
> 

On Thu, Nov 21, 2019 at 11:00:12AM +0100, Cédric Le Goater wrote:
> On 21/11/2019 10:11, Balamuruhan S wrote:
> > On Wed, Nov 20, 2019 at 08:46:30AM +0100, Cédric Le Goater wrote:
> >> Hello,
> >>
> >> On 19/11/2019 18:50, Balamuruhan S wrote:
> >>> Hi All,
> >>>
> >>> PowerNV fails to boot in multichip systems due to some misinterpretation
> >>> and mapping in Homer/Occ device models, this patchset fixes the
> >>> following,
> >>>
> >>>  - Homer size is 4MB per chip and Occ common area size is 8MB
> >>>  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
> >>>    return appropriate value
> >>>  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
> >>>    currently
> >>>  - OCC common area is shared across chips and should be mapped only once
> >>>    for multichip systems
> >>
> >> The first thing to address is the HOMER XSCOM region. 
> >>
> >> Introduce an empty skeleton for P8 and P9 with different mem ops handers
> >> because the registers have a different layout. From there, add the support
> >> for the different PBA* regs and move them out from the default XSCOM
> >> handlers. That should fix most of the current problems and it will provide 
> >> a nice framework for future extensions.
> > 
> > sure, I will work on it.
> > 
> >>
> >> Why not add the associated HOMER MMIO region while we are it ? the PBA
> >> registers have all the definitions we need and it will gives us access
> >> to the pstate table.
> > 
> > so, idea is to have HOMER MMIO for us to use it accessing pstate table / data
> > and HOMER XSCOM for homer associated xscom access for PBA* registers to
> > P8 and P9 respectively.
> 
> yes. 
> 
> >> Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
> >> not clear. Please check skiboot. I think a MMIO region should be enough
> >> because this is how sensor data from the OCC is accessed.
> > 
> > Okay, I will do the change for OCC to use MMIO, and will check skiboot
> > for making it better.
> > 
> >>
> >> On that topic, we could define properties on the PnvOCC model for each 
> >> sensor and tune the value from the QEMU monitor. It really shouldn't be
> >> too complex.
> > 
> > How can we tune value from QEMU monitor ? This is new to me and will
> > need to check it. I remember you have advised this with the error
> > injection framework patches and Rashmica's patch that provides way to
> > use Qemu monitor to feed data, but I need to do some study.
> 
> 
> See Joel's patch which has a simple example :  
> 
>    patchwork.ozlabs.org/patch/1196519
> 
> It simply generates object properties : 
> 
> 
> +    for (led = 0; led < s->nr_leds; led++) {
> +        char *name;
> +
> +        name = g_strdup_printf("led%d", led);
> +        object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
> +                            NULL, NULL, NULL);
> +    }
> 
> with defined get and set accessors. 
> 
> We could do the same for the OCC sensors with a table describing the 
> sensor layout. Accessors would just simply update the table. we could
> even trigger the OCC interrupt if needed.
> 
> This is the initial table :
> 
>   https://github.com/open-power/occ/blob/master/src/occ_405/sensor/sensor_info.c
> 
> Linux should be able to grab the values through hwmon just as on real HW.
> This is the case today for the DTS.

cool...

> 
> >>
> >> Also the same address is used, we should only map it once but we need 
> >> to invent something to know from which chip it is accessed.
> > 
> > This is something need to check how real hardware handles it while
> > accessing shared occ region from different chip and think how to make it
> > for us.
> 
> Yes. I suppose there is some chip id in the powerbus message.

:+1:

> 
> C.
> 
>   
> > 
> > Thanks a lot!
> > 
> > -- Bala
> > 
> >>
> >>
> >> C.
> >>
> >>
> >>>
> >>> Request for your review and suggestions to make it better. I would like to
> >>> thank Cedric for his time and help to figure out the issues.
> >>>
> >>> Balamuruhan S (5):
> >>>   hw/ppc/pnv: incorrect homer and occ common area size
> >>>   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
> >>>     sizes
> >>>   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
> >>>   hw/ppc/pnv_xscom: occ common area to be mapped only once
> >>>   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
> >>>
> >>>  hw/ppc/pnv_occ.c     |  2 +-
> >>>  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
> >>>  include/hw/ppc/pnv.h | 12 ++++++++----
> >>>  3 files changed, 36 insertions(+), 15 deletions(-)
> >>>
> >>
> > 
>