On Tue, Sep 27, 2016 at 07:54:37AM +0200, Cédric Le Goater wrote: > On 09/27/2016 04:35 AM, David Gibson wrote: > > On Mon, Sep 26, 2016 at 06:11:36PM +0200, Cédric Le Goater wrote: > >> On 09/23/2016 04:46 AM, David Gibson wrote: > >>> On Thu, Sep 22, 2016 at 10:25:59AM +0200, Cédric Le Goater wrote: > >>>>>> @@ -493,6 +525,8 @@ static void pnv_chip_power9_class_init(ObjectClass > >>>>>> *klass, void *data) > >>>>>> k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */ > >>>>>> k->cores_mask = POWER9_CORE_MASK; > >>>>>> k->core_pir = pnv_chip_core_pir_p9; > >>>>>> + k->xscom_addr = pnv_chip_xscom_addr_p9; > >>>>>> + k->xscom_pcba = pnv_chip_xscom_pcba_p9; > >>>>> > >>>>> So if you do as BenH (and I) suggested and have the "scom address > >>>>> space" actually be addressed by (pcba << 3), I think you can probably > >>>>> avoid these. > >>>> > >>>> I will look at that option again. > >>>> > >>>> I was trying to untangle a few things at the same time. I have better > >>>> view of the problem to solve now. The bus is gone, that's was one > >>>> thing. How we map these xscom regions is the next. > >>>> > >>>> Ben suggested to add some P7/P8 mangling before the dispatch in > >>>> the &address_space_xscom. This should make things cleaner. I had > >>>> not thought of doing that and this is why I introduced these helpers : > >>>> > >>>> +uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr) > >>>> +uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba) > >>>> > >>>> which I don't really like ... > >>>> > >>>> but we must make sure that we can do the mapping of the xscom > >>>> subregions in the &address_space_xscom using (pcba << 3) > >>>> > >>>> > >>>>> Instead you can handle it in the chip or ADU realize function by either: > >>>>> > >>>>> P8: * map one big subregion for the ADU into &address_space_memory > >>>>> * have the handler for that subregion do the address mangling, > >>>>> then redispatch into the xscom address space > >>>>> > >>>>> P9: * Map the appropriate chunk of the xscom address space > >>>>> directly into address_space_memory > >>>> > >>>> Yes that was my feeling for a better solution but Ben chimed in with the > >>>> HMER topic. I need to look at that. > >>> > >>> Right. Doesn't change the basic concept though - it just means you > >>> need (slightly different) redispatchers for both P8 and P9. > >> > >> In fact they are the same, you only need an "addr to pcba" handler at the > >> chip class level : > > > > Ok. I'd been thinking of using different dispatchers as an > > alternative to using the chip class translator hook, > > ah. yes, why not. We could have per-chip dispatchers but they > would have a lot in common.
Would they? Unless you're counting the core register dispatch - and it sounds like splitting that for P8 vs. P9 would be a good idea anyway - I don't see that there's much in common besides the address translation. Note of course, that you can add a helper function that both dispatchers can use if it's useful. > However, I think we can get rid of > the xscom_pcba' handlers, they should not be needed any where > else than in the XSCOM dispatchers. > > > but I guess if you have the decoding of those "core" registers > > here as well, then that doesn't make so much sense. > > yes and there is also the handling of the XSCOM failures. Hm, ok. > I can add some prologue handler to cover those "core" registers > but adding a MemoryRegion, ops, init and mapping would be a lot > of churn just to return 0. > > Thanks, > > C. > > > >> static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width) > >> { > >> PnvChip *chip = opaque; > >> uint32_t pcba = PNV_CHIP_GET_CLASS(chip)->xscom_pcba(addr); > >> uint64_t val = 0; > >> MemTxResult result; > >> > >> ... > >> > >> val = address_space_ldq(&chip->xscom_as, pcba << 3, > >> MEMTXATTRS_UNSPECIFIED, &result); > >> if (result != MEMTX_OK) { > >> > >> > >> > >> And so, the result is pretty clean. I killed the proxy object and merged > >> the regions in the chip but I have kept the pnv_xscom.c file because the > >> code related to xscom is rather large : ~250 lines. > > > > Sure, makes sense. > > > >> The objects declaring a xscom region need to do some register shifting but > >> this is usual in mmio regions. > >> > >> You will see in v4. > > > > Ok. > > > >>>>>> +static bool xscom_dispatch_read(PnvXScom *xscom, hwaddr addr, > >>>>>> uint64_t *val) > >>>>>> +{ > >>>>>> + uint32_t success; > >>>>>> + uint8_t data[8]; > >>>>>> + > >>>>>> + success = !address_space_rw(&xscom->xscom_as, addr, > >>>>>> MEMTXATTRS_UNSPECIFIED, > >>>>>> + data, 8, false); > >>>>>> + *val = (((uint64_t) data[0]) << 56 | > >>>>>> + ((uint64_t) data[1]) << 48 | > >>>>>> + ((uint64_t) data[2]) << 40 | > >>>>>> + ((uint64_t) data[3]) << 32 | > >>>>>> + ((uint64_t) data[4]) << 24 | > >>>>>> + ((uint64_t) data[5]) << 16 | > >>>>>> + ((uint64_t) data[6]) << 8 | > >>>>>> + ((uint64_t) data[7])); > >>>>> > >>>>> AFAICT this is basically assuming data is always encoded BE. With the > >>>>> right choice of endian flags on the individual SCOM device > >>>>> registrations with the scom address space, I think you should be able > >>>>> to avoid this mangling. > >>>> > >>>> yes. I should but curiously I had to do this, and this works the same on > >>>> an intel host or a ppc64 host. > >>> > >>> Hmm.. I suspect what you actually need is NATIVE_ENDIAN on the > >>> individual SCOM devices, with BIG_ENDIAN on the redispatcher region. > >> > >> we should be using address_space_ldq and address_space_stq. > > > > Ok. > > > >>>>>> + > >>>>>> + success = !address_space_rw(&xscom->xscom_as, addr, > >>>>>> MEMTXATTRS_UNSPECIFIED, > >>>>>> + data, 8, true); > >>>>>> + return success; > >>>>>> +} > >>>>>> + > >>>>>> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width) > >>>>>> +{ > >>>>>> + PnvXScom *s = opaque; > >>>>>> + uint32_t pcba = s->chip_class->xscom_pcba(addr); > >>>>>> + uint64_t val = 0; > >>>>>> + > >>>>>> + /* Handle some SCOMs here before dispatch */ > >>>>>> + switch (pcba) { > >>>>>> + case 0xf000f: > >>>>>> + val = s->chip_class->chip_cfam_id; > >>>>>> + break; > >>>>>> + case 0x1010c00: /* PIBAM FIR */ > >>>>>> + case 0x1010c03: /* PIBAM FIR MASK */ > >>>>>> + case 0x2020007: /* ADU stuff */ > >>>>>> + case 0x2020009: /* ADU stuff */ > >>>>>> + case 0x202000f: /* ADU stuff */ > >>>>>> + val = 0; > >>>>>> + break; > >>>>>> + case 0x2013f00: /* PBA stuff */ > >>>>>> + case 0x2013f01: /* PBA stuff */ > >>>>>> + case 0x2013f02: /* PBA stuff */ > >>>>>> + case 0x2013f03: /* PBA stuff */ > >>>>>> + case 0x2013f04: /* PBA stuff */ > >>>>>> + case 0x2013f05: /* PBA stuff */ > >>>>>> + case 0x2013f06: /* PBA stuff */ > >>>>>> + case 0x2013f07: /* PBA stuff */ > >>>>>> + val = 0; > >>>>>> + break; > >>>>> > >>>>> It'd be theoretically nicer to actually register regions for these > >>>>> special case addresses, but handling it here is a reasonable hack to > >>>>> get things working quickly for the time being. > >>>> > >>>> I will make a default region on the whole xscomm address space to catch > >>>> these. > >>> > >>> Ok. > >> > >> Well, it does not bring much and we loose the ability to catch errors. > >> I will leave it that way. > >> > >> Thanks, > >> > >> C. > >> > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature