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, but I guess if
you have the decoding of those "core" registers here as well, then
that doesn't make so much sense.
> 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
