Re: [PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[Balamuruhan S]

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(-)
> >>>
> >>
> > 
>