[Skiboot] [PATCH v2 02/15] hw/npu2: Merge implementations of BAR accessor functions
Alexey Kardashevskiy
aik at ozlabs.ru
Fri Feb 1 15:44:35 AEDT 2019
On 31/01/2019 14:18, Andrew Donnellan wrote:
>
>
> On 17/1/19 3:55 pm, Alexey Kardashevskiy wrote:
>>
>>
>> On 11/01/2019 12:09, Andrew Donnellan wrote:
>>> Move npu2_{get,read,write}_bar() to common code. Get rid of the OpenCAPI
>>> write_bar() function. Fix the OpenCAPI code to use the NVLink-style BAR
>>> accessor functions and struct npu2_bar.
>>>
>>> While we're there, fix a trivial bug in npu2_read_bar() when reading PHY
>>> BARs - the global MMIO BAR is stack 0, not stack 2. I don't think we
>>> ever
>>> read that BAR, so this has never been a problem.
>>
>> Out of curiosity I tried to locate this trivial bugfix and I could not
>> ;) imho it deserves a separate patch.
>>
>>>
>>> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
>>> Reviewed-by: Frederic Barrat <fbarrat at linux.ibm.com>
>>>
>>> ---
>>> v1->v2:
>>> - remove dead phys_map_get() (Fred)
>>> ---
>>> hw/npu2-common.c | 77 ++++++++++++++++++++++++++++++++-
>>> hw/npu2-opencapi.c | 112
>>> ++++++++++++++++------------------------------
>>> hw/npu2.c | 78 +--------------------------------
>>> include/npu2.h | 4 ++-
>>> 4 files changed, 121 insertions(+), 150 deletions(-)
>>>
>>> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
>>> index 6e6b12f0d1ae..3446acb45bea 100644
>>> --- a/hw/npu2-common.c
>>> +++ b/hw/npu2-common.c
>>> @@ -22,6 +22,7 @@
>>> #include <bitutils.h>
>>> #include <nvram.h>
>>> #include <i2c.h>
>>> +#include <phys-map.h>
>>> /*
>>> * We use the indirect method because it uses the same addresses as
>>> @@ -97,6 +98,82 @@ void npu2_write_mask_4b(struct npu2 *p, uint64_t
>>> reg, uint32_t val, uint32_t mas
>>> (uint64_t)new_val << 32);
>>> }
>>> +void npu2_get_bar(uint32_t gcid, struct npu2_bar *bar)
>>
>>
>> I'd call it npu2_setup_bar and pass type/index/reg/enabled as well.
>
> I see what you're getting at but with the changes in later patches I
> prefer to keep as is
>>
>>
>>> +{
>>> + phys_map_get(gcid, bar->type, bar->index, &bar->base, &bar->size);
>>> +}
>>> +
>>> +void npu2_read_bar(struct npu2 *p, struct npu2_bar *bar)
>>> +{
>>> + uint64_t reg, val;
>>> +
>>> + reg = NPU2_REG_OFFSET(0, NPU2_BLOCK_SM_0, bar->reg);
>>> + val = npu2_read(p, reg);
>>> +
>>> + switch (NPU2_REG(bar->reg)) {
>>> + case NPU2_PHY_BAR:
>>> + bar->base = GETFIELD(NPU2_PHY_BAR_ADDR, val) << 21;
>>> + bar->enabled = GETFIELD(NPU2_PHY_BAR_ENABLE, val);
>>> +
>>> + if (NPU2_REG_STACK(reg) == NPU2_STACK_STCK_0)
>>> + /* This is the global MMIO BAR */
>>> + bar->size = 0x1000000;
>>> + else
>>> + bar->size = 0x200000;
>>> + break;
>>> + case NPU2_NTL0_BAR:
>>> + case NPU2_NTL1_BAR:
>>> + bar->base = GETFIELD(NPU2_NTL_BAR_ADDR, val) << 16;
>>> + bar->enabled = GETFIELD(NPU2_NTL_BAR_ENABLE, val);
>>> + bar->size = 0x10000 << GETFIELD(NPU2_NTL_BAR_SIZE, val);
>>> + break;
>>> + case NPU2_GENID_BAR:
>>> + bar->base = GETFIELD(NPU2_GENID_BAR_ADDR, val) << 16;
>>> + bar->enabled = GETFIELD(NPU2_GENID_BAR_ENABLE, val);
>>> + bar->size = 0x20000;
>>> + break;
>>> + default:
>>> + bar->base = 0ul;
>>> + bar->enabled = false;
>>> + bar->size = 0;
>>> + break;
>>> + }
>>> +}
>>> +
>>> +void npu2_write_bar(struct npu2 *p, struct npu2_bar *bar, uint32_t
>>> gcid,
>>> + uint32_t scom)
>>> +{
>>> + uint64_t reg, val;
>>> + int block;
>>> +
>>> + switch (NPU2_REG(bar->reg)) {
>>> + case NPU2_PHY_BAR:
>>> + val = SETFIELD(NPU2_PHY_BAR_ADDR, 0ul, bar->base >> 21);
>>> + val = SETFIELD(NPU2_PHY_BAR_ENABLE, val, bar->enabled);
>>> + break;
>>> + case NPU2_NTL0_BAR:
>>> + case NPU2_NTL1_BAR:
>>> + val = SETFIELD(NPU2_NTL_BAR_ADDR, 0ul, bar->base >> 16);
>>> + val = SETFIELD(NPU2_NTL_BAR_ENABLE, val, bar->enabled);
>>> + val = SETFIELD(NPU2_NTL_BAR_SIZE, val, ilog2(bar->size >> 16));
>>> + break;
>>> + case NPU2_GENID_BAR:
>>> + val = SETFIELD(NPU2_GENID_BAR_ADDR, 0ul, bar->base >> 16);
>>> + val = SETFIELD(NPU2_GENID_BAR_ENABLE, val, bar->enabled);
>>> + break;
>>> + default:
>>> + val = 0ul;
>>> + }
>>> +
>>> + for (block = NPU2_BLOCK_SM_0; block <= NPU2_BLOCK_SM_3; block++) {
>>> + reg = NPU2_REG_OFFSET(0, block, bar->reg);
>>> + if (p)
>>> + npu2_write(p, reg, val);
>>> + else
>>> + npu2_scom_write(gcid, scom, reg, NPU2_MISC_DA_LEN_8B, val);
>>> + }
>>> +}
>>> +
>>> static bool _i2c_presence_detect(struct npu2_dev *dev)
>>> {
>>> uint8_t state, data;
>>> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
>>> index c10cf9863dda..ffa4d706bbad 100644
>>> --- a/hw/npu2-opencapi.c
>>> +++ b/hw/npu2-opencapi.c
>>> @@ -730,63 +730,29 @@ static void address_translation_config(uint32_t
>>> gcid, uint32_t scom_base,
>>> }
>>> }
>>> -/* TODO: Merge this with NVLink implementation - we don't use the
>>> npu2_bar
>>> - * wrapper for the PHY BARs yet */
>>> -static void write_bar(uint32_t gcid, uint32_t scom_base, uint64_t reg,
>>> - uint64_t addr, uint64_t size)
>>> -{
>>> - uint64_t val;
>>> - int block;
>>> - switch (NPU2_REG(reg)) {
>>> - case NPU2_PHY_BAR:
>>> - val = SETFIELD(NPU2_PHY_BAR_ADDR, 0ul, addr >> 21);
>>> - val = SETFIELD(NPU2_PHY_BAR_ENABLE, val, 1);
>>> - break;
>>> - case NPU2_NTL0_BAR:
>>> - case NPU2_NTL1_BAR:
>>> - val = SETFIELD(NPU2_NTL_BAR_ADDR, 0ul, addr >> 16);
>>> - val = SETFIELD(NPU2_NTL_BAR_SIZE, val, ilog2(size >> 16));
>>> - val = SETFIELD(NPU2_NTL_BAR_ENABLE, val, 1);
>>> - break;
>>> - case NPU2_GENID_BAR:
>>> - val = SETFIELD(NPU2_GENID_BAR_ADDR, 0ul, addr >> 16);
>>> - val = SETFIELD(NPU2_GENID_BAR_ENABLE, val, 1);
>>> - break;
>>> - default:
>>> - val = 0ul;
>>> - }
>>> -
>>> - for (block = NPU2_BLOCK_SM_0; block <= NPU2_BLOCK_SM_3; block++) {
>>> - npu2_scom_write(gcid, scom_base, NPU2_REG_OFFSET(0, block,
>>> reg),
>>> - NPU2_MISC_DA_LEN_8B, val);
>>> - prlog(PR_DEBUG, "OCAPI: Setting BAR %llx to %llx\n",
>>> - NPU2_REG_OFFSET(0, block, reg), val);
>>> - }
>>> -}
>>> -
>>> static void setup_global_mmio_bar(uint32_t gcid, uint32_t scom_base,
>>> uint64_t reg[])
>>> {
>>> - uint64_t addr, size;
>>> -
>>> - prlog(PR_DEBUG, "OCAPI: patching up PHY0 bar, %s\n", __func__);
>>> - phys_map_get(gcid, NPU_PHY, 0, &addr, &size);
>>> - write_bar(gcid, scom_base,
>>> - NPU2_REG_OFFSET(NPU2_STACK_STCK_2, 0, NPU2_PHY_BAR),
>>> - addr, size);
>>> - prlog(PR_DEBUG, "OCAPI: patching up PHY1 bar, %s\n", __func__);
>>> - phys_map_get(gcid, NPU_PHY, 1, &addr, &size);
>>> - write_bar(gcid, scom_base,
>>> - NPU2_REG_OFFSET(NPU2_STACK_STCK_1, 0, NPU2_PHY_BAR),
>>> - addr, size);
>>> -
>>> - prlog(PR_DEBUG, "OCAPI: setup global mmio, %s\n", __func__);
>>> - phys_map_get(gcid, NPU_REGS, 0, &addr, &size);
>>> - write_bar(gcid, scom_base,
>>> - NPU2_REG_OFFSET(NPU2_STACK_STCK_0, 0, NPU2_PHY_BAR),
>>> - addr, size);
>>> - reg[0] = addr;
>>> - reg[1] = size;
>>> + struct npu2_bar *bar;
>>> + struct npu2_bar phy_bars[] = {
>>> + { .type = NPU_PHY, .index = 0,
>>> + .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_2, 0, NPU2_PHY_BAR),
>>> + .enabled = true },
>>> + { .type = NPU_PHY, .index = 1,
>>> + .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_1, 0, NPU2_PHY_BAR),
>>> + .enabled = true },
>>> + { .type = NPU_REGS, .index = 0,
>>> + .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0, 0, NPU2_PHY_BAR),
>>> + .enabled = true },
>>> + };
>>> +
>>> + for (int i = 0; i < ARRAY_SIZE(phy_bars); i++) {
>>> + bar = &phy_bars[i];
>>> + npu2_get_bar(gcid, bar);
>>> + npu2_write_bar(NULL, bar, gcid, scom_base);
>>> + }
>>> + reg[0] = phy_bars[2].base;
>>> + reg[1] = phy_bars[2].size;
>>> }
>>> /* Procedure 13.1.3.8 - AFU MMIO Range BARs */
>>> @@ -799,19 +765,21 @@ static void setup_afu_mmio_bars(uint32_t gcid,
>>> uint32_t scom_base,
>>> uint64_t pa_offset = index_to_block(dev->brick_index) ==
>>> NPU2_BLOCK_OTL0 ?
>>> NPU2_CQ_CTL_MISC_MMIOPA0_CONFIG :
>>> NPU2_CQ_CTL_MISC_MMIOPA1_CONFIG;
>>> - uint64_t addr, size, reg;
>>> + uint64_t reg;
>>> prlog(PR_DEBUG, "OCAPI: %s: Setup AFU MMIO BARs\n", __func__);
>>> - phys_map_get(gcid, NPU_OCAPI_MMIO, dev->brick_index, &addr, &size);
>>> -
>>> - prlog(PR_DEBUG, "OCAPI: AFU MMIO set to %llx, size %llx\n",
>>> addr, size);
>>> - write_bar(gcid, scom_base, NPU2_REG_OFFSET(stack, 0, offset), addr,
>>> - size);
>>> - dev->bars[0].base = addr;
>>> - dev->bars[0].size = size;
>>> -
>>> - reg = SETFIELD(NPU2_CQ_CTL_MISC_MMIOPA_ADDR, 0ull, addr >> 16);
>>> - reg = SETFIELD(NPU2_CQ_CTL_MISC_MMIOPA_SIZE, reg, ilog2(size >>
>>> 16));
>>> + dev->bars[0].type = NPU_OCAPI_MMIO;
>>> + dev->bars[0].index = dev->brick_index;
>>> + dev->bars[0].reg = NPU2_REG_OFFSET(stack, 0, offset);
>>> + dev->bars[0].enabled = true;
>>
>>
>> Here you add type/index/reg/enabled initialization, not move from
>> elsewhere so there either was a bug with unitialized bar struct or now
>> we have 2 places where such initialization occurs. The commit log does
>> not mentioned this. I am confused.
>
> Previously those fields weren't used at all, we only used base and size.
> Now that we're merging the BAR accessor functions, we need to populate
> those fields because we're using the shared functions npu2_get_bar() and
> npu2_write_bar(), rather than open-coding phys_map_get() and having an
> OpenCAPI-only function called write_bar() that took the reg directly as
> an argument.
>
> This code all disappears in a later patch.
This tells us that either this later patch should be squashed into this
one or it should move before this one.
--
Alexey
More information about the Skiboot
mailing list