Re: [PATCH V2 7/8] accel/amdxdna: Read firmware interface version from registers

[Lizhi Hou <lizhi.hou@xxxxxxx>]

On 12/10/24 16:20, Mario Limonciello wrote:
> On 12/6/2024 16:00, Lizhi Hou wrote:
> > The latest released firmware supports reading firmware interface version
> > from registers directly. The driver's probe routine reads the major and
> > minor version numbers. If the firmware interface does not compatible 
> > with
> s/does/is/
Thanks. I will fix this.
> > the driver, the driver's probe routine returns failure.
> >
> > Co-developed-by: Min Ma <min.ma@xxxxxxx>
> > Signed-off-by: Min Ma <min.ma@xxxxxxx>
> > Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxx>
>
> Just to confirm you're not backing yourself into a corner the plan is 
> not to bump this major version any time soon for anything already 
> supported by the driver; right?

That is correct.


Thanks,

Lizhi

> Because once you do that this is going to get messy quickly.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> >   drivers/accel/amdxdna/aie2_message.c | 26 ----------
> >   drivers/accel/amdxdna/aie2_pci.c     | 74 ++++++++++++++++++++++------
> >   drivers/accel/amdxdna/aie2_pci.h     |  6 +--
> >   drivers/accel/amdxdna/npu1_regs.c    |  2 +-
> >   drivers/accel/amdxdna/npu2_regs.c    |  2 +-
> >   drivers/accel/amdxdna/npu4_regs.c    |  2 +-
> >   drivers/accel/amdxdna/npu5_regs.c    |  2 +-
> >   7 files changed, 64 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/accel/amdxdna/aie2_message.c 
> > b/drivers/accel/amdxdna/aie2_message.c
> > index 13b5a96f8d25..f6d46e1e5086 100644
> > --- a/drivers/accel/amdxdna/aie2_message.c
> > +++ b/drivers/accel/amdxdna/aie2_message.c
> > @@ -100,32 +100,6 @@ int aie2_get_runtime_cfg(struct amdxdna_dev_hdl 
> > *ndev, u32 type, u64 *value)
> >       return 0;
> >   }
> >   -int aie2_check_protocol_version(struct amdxdna_dev_hdl *ndev)
> > -{
> > -    DECLARE_AIE2_MSG(protocol_version, MSG_OP_GET_PROTOCOL_VERSION);
> > -    struct amdxdna_dev *xdna = ndev->xdna;
> > -    int ret;
> > -
> > -    ret = aie2_send_mgmt_msg_wait(ndev, &msg);
> > -    if (ret) {
> > -        XDNA_ERR(xdna, "Failed to get protocol version, ret %d", ret);
> > -        return ret;
> > -    }
> > -
> > -    if (resp.major != ndev->priv->protocol_major) {
> > -        XDNA_ERR(xdna, "Incompatible firmware protocol version major 
> > %d minor %d",
> > -             resp.major, resp.minor);
> > -        return -EINVAL;
> > -    }
> > -
> > -    if (resp.minor < ndev->priv->protocol_minor) {
> > -        XDNA_ERR(xdna, "Firmware minor version smaller than 
> > supported");
> > -        return -EINVAL;
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> >   int aie2_assign_mgmt_pasid(struct amdxdna_dev_hdl *ndev, u16 pasid)
> >   {
> >       DECLARE_AIE2_MSG(assign_mgmt_pasid, MSG_OP_ASSIGN_MGMT_PASID);
> > diff --git a/drivers/accel/amdxdna/aie2_pci.c 
> > b/drivers/accel/amdxdna/aie2_pci.c
> > index 489744a2e226..2d2b6b66617a 100644
> > --- a/drivers/accel/amdxdna/aie2_pci.c
> > +++ b/drivers/accel/amdxdna/aie2_pci.c
> > @@ -33,17 +33,51 @@ MODULE_PARM_DESC(aie2_max_col, "Maximum column 
> > could be used");
> >    * The related register and ring buffer information is on SRAM BAR.
> >    * This struct is the register layout.
> >    */
> > +#define MGMT_MBOX_MAGIC 0x55504e5f /* _NPU */
> >   struct mgmt_mbox_chann_info {
> > -    u32    x2i_tail;
> > -    u32    x2i_head;
> > -    u32    x2i_buf;
> > -    u32    x2i_buf_sz;
> > -    u32    i2x_tail;
> > -    u32    i2x_head;
> > -    u32    i2x_buf;
> > -    u32    i2x_buf_sz;
> > +    __u32    x2i_tail;
> > +    __u32    x2i_head;
> > +    __u32    x2i_buf;
> > +    __u32    x2i_buf_sz;
> > +    __u32    i2x_tail;
> > +    __u32    i2x_head;
> > +    __u32    i2x_buf;
> > +    __u32    i2x_buf_sz;
> > +    __u32    magic;
> > +    __u32    msi_id;
> > +    __u32    prot_major;
> > +    __u32    prot_minor;
> > +    __u32    rsvd[4];
> >   };
> >   +static int aie2_check_protocol(struct amdxdna_dev_hdl *ndev, u32 
> > fw_major, u32 fw_minor)
> > +{
> > +    struct amdxdna_dev *xdna = ndev->xdna;
> > +
> > +    /*
> > +     * The driver supported mailbox behavior is defined by
> > +     * ndev->priv->protocol_major and protocol_minor.
> > +     *
> > +     * When protocol_major and fw_major are different, it means driver
> > +     * and firmware are incompatible.
> > +     */
> > +    if (ndev->priv->protocol_major != fw_major) {
> > +        XDNA_ERR(xdna, "Incompatible firmware protocol major %d 
> > minor %d",
> > +             fw_major, fw_minor);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /*
> > +     * When protocol_minor is greater then fw_minor, that means driver
> > +     * relies on operation the installed firmware does not support.
> > +     */
> > +    if (ndev->priv->protocol_minor > fw_minor) {
> > +        XDNA_ERR(xdna, "Firmware minor version smaller than 
> > supported");
> > +        return -EINVAL;
> > +    }
> > +    return 0;
> > +}
> > +
> >   static void aie2_dump_chann_info_debug(struct amdxdna_dev_hdl *ndev)
> >   {
> >       struct amdxdna_dev *xdna = ndev->xdna;
> > @@ -57,6 +91,8 @@ static void aie2_dump_chann_info_debug(struct 
> > amdxdna_dev_hdl *ndev)
> >       XDNA_DBG(xdna, "x2i ringbuf 0x%x", ndev->mgmt_x2i.rb_start_addr);
> >       XDNA_DBG(xdna, "x2i rsize   0x%x", ndev->mgmt_x2i.rb_size);
> >       XDNA_DBG(xdna, "x2i chann index 0x%x", ndev->mgmt_chan_idx);
> > +    XDNA_DBG(xdna, "mailbox protocol major 0x%x", 
> > ndev->mgmt_prot_major);
> > +    XDNA_DBG(xdna, "mailbox protocol minor 0x%x", 
> > ndev->mgmt_prot_minor);
> >   }
> >     static int aie2_get_mgmt_chann_info(struct amdxdna_dev_hdl *ndev)
> > @@ -87,6 +123,12 @@ static int aie2_get_mgmt_chann_info(struct 
> > amdxdna_dev_hdl *ndev)
> >       for (i = 0; i < sizeof(info_regs) / sizeof(u32); i++)
> >           reg[i] = readl(ndev->sram_base + off + i * sizeof(u32));
> >   +    if (info_regs.magic != MGMT_MBOX_MAGIC) {
> > +        XDNA_ERR(ndev->xdna, "Invalid mbox magic 0x%x", 
> > info_regs.magic);
> > +        ret = -EINVAL;
> > +        goto done;
> > +    }
> > +
> >       i2x = &ndev->mgmt_i2x;
> >       x2i = &ndev->mgmt_x2i;
> >   @@ -99,14 +141,20 @@ static int aie2_get_mgmt_chann_info(struct 
> > amdxdna_dev_hdl *ndev)
> >       x2i->mb_tail_ptr_reg = AIE2_MBOX_OFF(ndev, info_regs.x2i_tail);
> >       x2i->rb_start_addr   = AIE2_SRAM_OFF(ndev, info_regs.x2i_buf);
> >       x2i->rb_size         = info_regs.x2i_buf_sz;
> > -    ndev->mgmt_chan_idx  = CHANN_INDEX(ndev, x2i->rb_start_addr);
> >   +    ndev->mgmt_chan_idx  = info_regs.msi_id;
> > +    ndev->mgmt_prot_major = info_regs.prot_major;
> > +    ndev->mgmt_prot_minor = info_regs.prot_minor;
> > +
> > +    ret = aie2_check_protocol(ndev, ndev->mgmt_prot_major, 
> > ndev->mgmt_prot_minor);
> > +
> > +done:
> >       aie2_dump_chann_info_debug(ndev);
> >         /* Must clear address at FW_ALIVE_OFF */
> >       writel(0, SRAM_GET_ADDR(ndev, FW_ALIVE_OFF));
> >   -    return 0;
> > +    return ret;
> >   }
> >     int aie2_runtime_cfg(struct amdxdna_dev_hdl *ndev,
> > @@ -155,12 +203,6 @@ static int aie2_mgmt_fw_init(struct 
> > amdxdna_dev_hdl *ndev)
> >   {
> >       int ret;
> >   -    ret = aie2_check_protocol_version(ndev);
> > -    if (ret) {
> > -        XDNA_ERR(ndev->xdna, "Check header hash failed");
> > -        return ret;
> > -    }
> > -
> >       ret = aie2_runtime_cfg(ndev, AIE2_RT_CFG_INIT, NULL);
> >       if (ret) {
> >           XDNA_ERR(ndev->xdna, "Runtime config failed");
> > diff --git a/drivers/accel/amdxdna/aie2_pci.h 
> > b/drivers/accel/amdxdna/aie2_pci.h
> > index 8c17b74654ce..cc159cadff9f 100644
> > --- a/drivers/accel/amdxdna/aie2_pci.h
> > +++ b/drivers/accel/amdxdna/aie2_pci.h
> > @@ -39,9 +39,6 @@
> >   })
> >     #define CHAN_SLOT_SZ SZ_8K
> > -#define CHANN_INDEX(ndev, rbuf_off) \
> > -    (((rbuf_off) - SRAM_REG_OFF((ndev), MBOX_CHANN_OFF)) / 
> > CHAN_SLOT_SZ)
> > -
> >   #define MBOX_SIZE(ndev) \
> >   ({ \
> >       typeof(ndev) _ndev = (ndev); \
> > @@ -170,6 +167,8 @@ struct amdxdna_dev_hdl {
> >       struct xdna_mailbox_chann_res    mgmt_x2i;
> >       struct xdna_mailbox_chann_res    mgmt_i2x;
> >       u32                mgmt_chan_idx;
> > +    u32                mgmt_prot_major;
> > +    u32                mgmt_prot_minor;
> >         u32                total_col;
> >       struct aie_version        version;
> > @@ -262,7 +261,6 @@ int aie2_suspend_fw(struct amdxdna_dev_hdl *ndev);
> >   int aie2_resume_fw(struct amdxdna_dev_hdl *ndev);
> >   int aie2_set_runtime_cfg(struct amdxdna_dev_hdl *ndev, u32 type, 
> > u64 value);
> >   int aie2_get_runtime_cfg(struct amdxdna_dev_hdl *ndev, u32 type, 
> > u64 *value);
> > -int aie2_check_protocol_version(struct amdxdna_dev_hdl *ndev);
> >   int aie2_assign_mgmt_pasid(struct amdxdna_dev_hdl *ndev, u16 pasid);
> >   int aie2_query_aie_version(struct amdxdna_dev_hdl *ndev, struct 
> > aie_version *version);
> >   int aie2_query_aie_metadata(struct amdxdna_dev_hdl *ndev, struct 
> > aie_metadata *metadata);
> > diff --git a/drivers/accel/amdxdna/npu1_regs.c 
> > b/drivers/accel/amdxdna/npu1_regs.c
> > index c8f4d1cac65d..e408af57e378 100644
> > --- a/drivers/accel/amdxdna/npu1_regs.c
> > +++ b/drivers/accel/amdxdna/npu1_regs.c
> > @@ -65,7 +65,7 @@ const struct dpm_clk_freq npu1_dpm_clk_table[] = {
> >   const struct amdxdna_dev_priv npu1_dev_priv = {
> >       .fw_path        = "amdnpu/1502_00/npu.sbin",
> >       .protocol_major = 0x5,
> > -    .protocol_minor = 0x1,
> > +    .protocol_minor = 0x7,
> >       .rt_config    = npu1_default_rt_cfg,
> >       .dpm_clk_tbl    = npu1_dpm_clk_table,
> >       .col_align    = COL_ALIGN_NONE,
> > diff --git a/drivers/accel/amdxdna/npu2_regs.c 
> > b/drivers/accel/amdxdna/npu2_regs.c
> > index ac63131f9c7c..286bd0d475e2 100644
> > --- a/drivers/accel/amdxdna/npu2_regs.c
> > +++ b/drivers/accel/amdxdna/npu2_regs.c
> > @@ -64,7 +64,7 @@
> >   const struct amdxdna_dev_priv npu2_dev_priv = {
> >       .fw_path        = "amdnpu/17f0_00/npu.sbin",
> >       .protocol_major = 0x6,
> > -    .protocol_minor = 0x1,
> > +    .protocol_minor = 0x6,
> >       .rt_config    = npu4_default_rt_cfg,
> >       .dpm_clk_tbl    = npu4_dpm_clk_table,
> >       .col_align    = COL_ALIGN_NATURE,
> > diff --git a/drivers/accel/amdxdna/npu4_regs.c 
> > b/drivers/accel/amdxdna/npu4_regs.c
> > index a713ac18adfc..00c52833ce89 100644
> > --- a/drivers/accel/amdxdna/npu4_regs.c
> > +++ b/drivers/accel/amdxdna/npu4_regs.c
> > @@ -85,7 +85,7 @@ const struct dpm_clk_freq npu4_dpm_clk_table[] = {
> >   const struct amdxdna_dev_priv npu4_dev_priv = {
> >       .fw_path        = "amdnpu/17f0_10/npu.sbin",
> >       .protocol_major = 0x6,
> > -    .protocol_minor = 0x1,
> > +    .protocol_minor = 12,
> >       .rt_config    = npu4_default_rt_cfg,
> >       .dpm_clk_tbl    = npu4_dpm_clk_table,
> >       .col_align    = COL_ALIGN_NATURE,
> > diff --git a/drivers/accel/amdxdna/npu5_regs.c 
> > b/drivers/accel/amdxdna/npu5_regs.c
> > index 67a5d5bc8a49..118849272f27 100644
> > --- a/drivers/accel/amdxdna/npu5_regs.c
> > +++ b/drivers/accel/amdxdna/npu5_regs.c
> > @@ -64,7 +64,7 @@
> >   const struct amdxdna_dev_priv npu5_dev_priv = {
> >       .fw_path        = "amdnpu/17f0_11/npu.sbin",
> >       .protocol_major = 0x6,
> > -    .protocol_minor = 0x1,
> > +    .protocol_minor = 12,
> >       .rt_config    = npu4_default_rt_cfg,
> >       .dpm_clk_tbl    = npu4_dpm_clk_table,
> >       .col_align    = COL_ALIGN_NATURE,