[Skiboot] [PATCH v2 02/15] hw/npu2: Merge implementations of BAR accessor functions
Andrew Donnellan
andrew.donnellan at au1.ibm.com
Thu Jan 31 14:18:49 AEDT 2019
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.
>
>
>
>> + npu2_get_bar(gcid, &dev->bars[0]);
>> +
>> + prlog(PR_DEBUG, "OCAPI: AFU MMIO set to %llx, size %llx\n",
>> + dev->bars[0].base, dev->bars[0].size);
>> + npu2_write_bar(NULL, &dev->bars[0], gcid, scom_base);
>> +
>> + reg = SETFIELD(NPU2_CQ_CTL_MISC_MMIOPA_ADDR, 0ull, dev->bars[0].base >> 16);
>> + reg = SETFIELD(NPU2_CQ_CTL_MISC_MMIOPA_SIZE, reg, ilog2(dev->bars[0].size >> 16));
>> prlog(PR_DEBUG, "OCAPI: PA translation %llx\n", reg);
>> npu2_scom_write(gcid, scom_base,
>> NPU2_REG_OFFSET(stack, NPU2_BLOCK_CTL,
>> @@ -825,15 +793,15 @@ static void setup_afu_config_bars(uint32_t gcid, uint32_t scom_base,
>> {
>> uint64_t stack = index_to_stack(dev->brick_index);
>> int stack_num = stack - NPU2_STACK_STCK_0;
>> - uint64_t addr, size;
>>
>> prlog(PR_DEBUG, "OCAPI: %s: Setup AFU Config BARs\n", __func__);
>> - phys_map_get(gcid, NPU_GENID, stack_num, &addr, &size);
>> - prlog(PR_DEBUG, "OCAPI: Assigning GENID BAR: %016llx\n", addr);
>> - write_bar(gcid, scom_base, NPU2_REG_OFFSET(stack, 0, NPU2_GENID_BAR),
>> - addr, size);
>> - dev->bars[1].base = addr;
>> - dev->bars[1].size = size;
>> + dev->bars[1].type = NPU_GENID;
>> + dev->bars[1].index = stack_num;
>> + dev->bars[1].reg = NPU2_REG_OFFSET(stack, 0, NPU2_GENID_BAR);
>> + dev->bars[1].enabled = true;
>
>
> Same here.
Same as above
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com IBM Australia Limited
More information about the Skiboot
mailing list