[Skiboot] [PATCH 03/13] hw/npu2: Move PHY/NTL/GENID BAR assignment to common code

[Andrew Donnellan]

On 14/12/18 2:18 am, Frederic Barrat wrote:
>> +        /* GENID BAR */
>> +        bar = &dev->genid_bar;
>> +        bar->type = NPU_GENID;
>> +        bar->index = NPU2DEV_STACK(dev);
>> +        bar->reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0 + 
>> NPU2DEV_STACK(dev),
>> +                       0, NPU2_GENID_BAR);
>> +        bar->flags = PCI_CFG_BAR_TYPE_MEM | PCI_CFG_BAR_MEM64;
>> +        npu2_get_bar(npu->chip_id, bar);
>> +        bar->size = 0x10000;
>> +        if (NPU2DEV_BRICK(dev))
>> +            bar->base += 0x10000;
>> +        npu2_write_bar(npu, bar, npu->chip_id, npu->xscom_base);
> 
> The GENID reg is per stack (same address for the 2 devices). For 2 
> devices on the same stack, isn't the 2nd one overwriting the 1st, with 
> only half the size available? I don't understand why we're doing it. And 
> it seems different from what we were having for opencapi. I'm guessing 
> it may not make much of a difference, since we're only addressing 4k of 
> config space.
> 
> It matches what nvlink was doing though, but the same comment applies.

If you look at npu2_write_bar(), you'll see that the size isn't actually 
used when writing the GENID BAR, and the base address is right shifted 
16 bits (i.e. 0x10000) so the adjusted base won't change the value 
that's written to the BAR register either.

The reason that we're doing it is that the NVLink code needs to 
advertise these BARs to Linux for... reasons.

Now that you've mentioned... I think there might be an issue when 
accessing config space through OTL1, will look at it and fix in v2 if 
needed.

> 
> 
> 
>> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
>> index 88e4eb1974a2..b374a1035ac9 100644
>> --- a/hw/npu2-opencapi.c
>> +++ b/hw/npu2-opencapi.c
> 
>> @@ -1582,8 +1530,8 @@ static void setup_device(struct npu2_dev *dev)
>>       uint64_t mm_win[2];
>>
>>       /* Populate PHB device node */
>> -    phys_map_get(dev->npu->chip_id, NPU_OCAPI_MMIO, dev->brick_index, 
>> &mm_win[0],
>> -             &mm_win[1]);
>> +    mm_win[0] = dev->ntl_bar.base;
>> +    mm_win[1] = dev->ntl_bar.size;
>>       prlog(PR_DEBUG, "OCAPI: Setting MMIO window to %016llx + 
>> %016llx\n",
>>             mm_win[0], mm_win[1]);
>>       dn_phb = dt_new_addr(dt_root, "pciex", mm_win[0]);
>> @@ -1629,7 +1577,8 @@ static void setup_device(struct npu2_dev *dev)
>>       /* Procedure 13.1.3.8 - AFU MMIO Range BARs */
>>       setup_afu_mmio_bars(dev->npu->chip_id, dev->npu->xscom_base, dev);
>>       /* Procedure 13.1.3.9 - AFU Config BARs */
>> -    setup_afu_config_bars(dev->npu->chip_id, dev->npu->xscom_base, dev);
>> +    dev->genid_bar.enabled = true;
>> +    npu2_write_bar(dev->npu, &dev->genid_bar, dev->npu->chip_id, 
>> dev->npu->xscom_base);
> 
> For readability, I wouldn't mind keeping a separate 
> setup_afu_config_bars() function with the (now minimal) genid setup.
> 

noted for v2.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited