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