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. 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.
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.
>>
>
