On 06/13/2018 02:47 AM, David Gibson wrote: > On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote: >> On 06/12/2018 07:58 AM, David Gibson wrote: >>> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote: >>>> On 06/06/2018 08:39 AM, David Gibson wrote: >>>>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote: >>>>>> Based on previous work done in skiboot, the physical mapping array >>>>>> helps in calculating the MMIO ranges of each controller depending on >>>>>> the chip id and the chip type. This is will be particularly useful for >>>>>> the P9 models which use less the XSCOM bus and rely more on MMIOs. >>>>>> >>>>>> A link on the chip is now necessary to calculate MMIO BARs and >>>>>> sizes. This is why such a link is introduced in the PSIHB model. >>>>> >>>>> I think this message needs some work. This says what it's for, but >>>>> what actually *is* this array, and how does it work? >>>> >>>> OK. It is relatively simple: each controller has an entry defining its >>>> MMIO range. >>>> >>>>> The outside-core differences between POWER8 and POWER9 are substantial >>>>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as >>>>> different machine types (sharing some routines, of course). >>>> >>>> yes and no. I have survived using a common PnvChip framework but >>>> it is true that I had to add P9 classes for each: LPC, PSI, OCC >>>> They are very similar but not enough. P9 uses much more MMIOs than >>>> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. >>> >>> Well, it's certainly *possible* to use the same machine type, I'm just >>> not convinced it's a good idea. It seems kind of dodgy to me that so >>> many peripherals on the system change as a side-effect of setting the >>> cpu. Compare to how x86 works where cpu really does change the CPU, >>> plugging it into the same virtual "chipset". Different chipsets *are* >>> different machine types there (pc vs. q35). >> >> OK, I agree, and we can use a set of common routines to instantiate the >> different chipset models. >> >> So we would have a common pnv_init() routine to initialize the different >> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a >> machine class attribute ? > > Well.. that's one option. Usually for these things, it works out > better to instead of parameterizing big high-level routines like > pnv_init(), you have separate versions of those calling a combination > of case-specific and common routines as necessary. > > Mostly it just comes down to what is simplest to implement for you, though.
I am introducing a powernv8 machine, the machine init routine is still generic and did not change much. But I have deepen the PnvChip class inheritance hierarchy with an intermediate class taking care of the Chip sub controllers, which gives us something like : pnv_init() . skiboot . kernel . initrd . chips creation . platform bus/device : isa bus pci layout bmc handling. p8 chip hierarchy: power8_v2.0-pnv-chip (gives the cpu type) pnv8-chip : creates the devices only pnv-chip : creates xscom and the cores The powervn9 machine has this hierarchy : power9_v2.0-pnv-chip pnv9-chip pnv-chip I had to introduce these new PnvChipClass ops : void (*realize)(PnvChip *chip, Error **errp); Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp); ISABus *(*isa_create)(PnvChip *chip); Overall, it's looking fine and it should remove most of these tests : pnv_chip_is_power9(chip) If not, it means we are missing a PnvChipClass ops anyway. I will send a patchset this week, the final one will shuffle quite a bit of code and the resulting diff will be a bit fuzy. You will have to trust me on this one. >> Nevertheless we would still need to introduce "a physical mapping array >> describing MMIO ranges" but we can start by differentiating the chipsets >> first. > > Well, maybe. I'm wondering if you can more easily encapsulate the > information in that array in a top-level init routine, that calls > common helpers with different addresses / device types / whatever. Hmm, I think I understand but could you give me a prototype example. Please. To make sure. I would like to keep the array somewhere because, in a quick look, it gives you an overview of the POWER Processor address space. Thanks, C.