[Skiboot] [PATCH v2 02/15] hw/npu2: Merge implementations of BAR accessor functions
Alexey Kardashevskiy
aik at ozlabs.ru
Thu Jan 17 15:55:17 AEDT 2019
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.
> +{
> + 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.
> + 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.
> + npu2_get_bar(gcid, &dev->bars[1]);
> + prlog(PR_DEBUG, "OCAPI: Assigning GENID BAR: %016llx\n", dev->bars[1].base);
> + npu2_write_bar(NULL, &dev->bars[1], gcid, scom_base);
> }
>
> static void otl_enabletx(uint32_t gcid, uint32_t scom_base,
> diff --git a/hw/npu2.c b/hw/npu2.c
> index 79b87f196b3a..8176a81262f0 100644
> --- a/hw/npu2.c
> +++ b/hw/npu2.c
> @@ -106,84 +106,6 @@ static struct npu2_dev *npu2_bdf_to_dev(struct npu2 *p,
> return NULL;
> }
>
> -static inline void npu2_get_bar(uint32_t gcid, struct npu2_bar *bar)
> -{
> - phys_map_get(gcid, bar->type, bar->index, &bar->base, &bar->size);
> -}
> -
> -static 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_2)
> - /* 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;
> - }
> -}
> -
> -static 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, 1);
> - 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);
> - }
> -}
> -
> /* Trap for PCI command (0x4) to enable or disable device's BARs */
> static int64_t npu2_cfg_write_cmd(void *dev,
> struct pci_cfg_reg_filter *pcrf __unused,
> diff --git a/include/npu2.h b/include/npu2.h
> index fcaeb55f67dc..b2a3ead858d2 100644
> --- a/include/npu2.h
> +++ b/include/npu2.h
> @@ -204,6 +204,10 @@ void npu2_write(struct npu2 *p, uint64_t reg, uint64_t val);
> uint64_t npu2_read(struct npu2 *p, uint64_t reg);
> void npu2_write_mask(struct npu2 *p, uint64_t reg, uint64_t val, uint64_t mask);
> void npu2_write_mask_4b(struct npu2 *p, uint64_t reg, uint32_t val, uint32_t mask);
> +void npu2_get_bar(uint32_t gcid, struct npu2_bar *bar);
> +void npu2_read_bar(struct npu2 *p, struct npu2_bar *bar);
> +void npu2_write_bar(struct npu2 *p, struct npu2_bar *bar, uint32_t gcid,
> + uint32_t scom);
> int64_t npu2_dev_procedure(void *dev, struct pci_cfg_reg_filter *pcrf,
> uint32_t offset, uint32_t len, uint32_t *data,
> bool write);
>
--
Alexey
More information about the Skiboot
mailing list