[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure |
Date: |
Wed, 28 Sep 2016 11:40:11 +1000 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, (continued)
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, David Gibson, 2016/09/21
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Cédric Le Goater, 2016/09/22
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, David Gibson, 2016/09/22
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Cédric Le Goater, 2016/09/26
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, David Gibson, 2016/09/26
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Cédric Le Goater, 2016/09/27
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Benjamin Herrenschmidt, 2016/09/27
- Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Cédric Le Goater, 2016/09/27
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure,
David Gibson <=
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Cédric Le Goater, 2016/09/27
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Cédric Le Goater, 2016/09/27
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Benjamin Herrenschmidt, 2016/09/27
Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure, Benjamin Herrenschmidt, 2016/09/27
[Qemu-ppc] [PATCH v3 08/10] ppc/pnv: add a XScomDevice to PnvCore, Cédric Le Goater, 2016/09/15
[Qemu-ppc] [PATCH v3 09/10] ppc/pnv: add a LPC controller, Cédric Le Goater, 2016/09/15