Message-ID: <172cde68-930f-381e-df9f-da2a48955828@amd.com>
Date: Wed, 14 Aug 2024 11:16:41 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: Jeffrey Hugo <quic_jhugo@...cinc.com>, <ogabbay@...nel.org>,
	<dri-devel@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <min.ma@....com>, <max.zhen@....com>,
	<sonal.santan@....com>, <king.tam@....com>, Narendra Gutta
	<VenkataNarendraKumar.Gutta@....com>, George Yang <George.Yang@....com>
Subject: Re: [PATCH V2 01/10] accel/amdxdna: Add a new driver for AMD AI
 Engine


On 8/9/24 09:11, Jeffrey Hugo wrote:
> On 8/5/2024 11:39 AM, Lizhi Hou wrote:
>> AMD AI Engine forms the core of AMD NPU and can be used for accelerating
>> machine learning applications.
>>
>> Add the driver to support AI Engine integrated to AMD CPU.
>> Only very basic functionalities are added.
>>    - module and PCI device initialization
>>    - firmware load
>>    - power up
>>    - low level hardware initialization
>>
>> Co-developed-by: Narendra Gutta <VenkataNarendraKumar.Gutta@....com>
>> Signed-off-by: Narendra Gutta <VenkataNarendraKumar.Gutta@....com>
>> Co-developed-by: George Yang <George.Yang@....com>
>> Signed-off-by: George Yang <George.Yang@....com>
>> Co-developed-by: Min Ma <min.ma@....com>
>> Signed-off-by: Min Ma <min.ma@....com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@....com>
>> ---
>>   MAINTAINERS                             |   8 ++
>>   drivers/accel/Kconfig                   |   1 +
>>   drivers/accel/Makefile                  |   1 +
>>   drivers/accel/amdxdna/Kconfig           |  15 ++
>>   drivers/accel/amdxdna/Makefile          |  14 ++
>>   drivers/accel/amdxdna/TODO              |   4 +
>>   drivers/accel/amdxdna/aie2_pci.c        | 182 ++++++++++++++++++++++++
>>   drivers/accel/amdxdna/aie2_pci.h        | 144 +++++++++++++++++++
>>   drivers/accel/amdxdna/aie2_psp.c        | 137 ++++++++++++++++++
>>   drivers/accel/amdxdna/aie2_smu.c        | 112 +++++++++++++++
>>   drivers/accel/amdxdna/amdxdna_drm.c     |  20 +++
>>   drivers/accel/amdxdna/amdxdna_drm.h     |  65 +++++++++
>>   drivers/accel/amdxdna/amdxdna_pci_drv.c | 118 +++++++++++++++
>>   drivers/accel/amdxdna/amdxdna_pci_drv.h |  31 ++++
>>   drivers/accel/amdxdna/amdxdna_sysfs.c   |  47 ++++++
>>   drivers/accel/amdxdna/npu1_regs.c       |  94 ++++++++++++
>>   drivers/accel/amdxdna/npu2_regs.c       | 111 +++++++++++++++
>>   drivers/accel/amdxdna/npu4_regs.c       | 111 +++++++++++++++
>>   drivers/accel/amdxdna/npu5_regs.c       | 111 +++++++++++++++
>>   include/uapi/drm/amdxdna_accel.h        |  27 ++++
>>   20 files changed, 1353 insertions(+)
>>   create mode 100644 drivers/accel/amdxdna/Kconfig
>>   create mode 100644 drivers/accel/amdxdna/Makefile
>>   create mode 100644 drivers/accel/amdxdna/TODO
>>   create mode 100644 drivers/accel/amdxdna/aie2_pci.c
>>   create mode 100644 drivers/accel/amdxdna/aie2_pci.h
>>   create mode 100644 drivers/accel/amdxdna/aie2_psp.c
>>   create mode 100644 drivers/accel/amdxdna/aie2_smu.c
>>   create mode 100644 drivers/accel/amdxdna/amdxdna_drm.c
>>   create mode 100644 drivers/accel/amdxdna/amdxdna_drm.h
>>   create mode 100644 drivers/accel/amdxdna/amdxdna_pci_drv.c
>>   create mode 100644 drivers/accel/amdxdna/amdxdna_pci_drv.h
>>   create mode 100644 drivers/accel/amdxdna/amdxdna_sysfs.c
>>   create mode 100644 drivers/accel/amdxdna/npu1_regs.c
>>   create mode 100644 drivers/accel/amdxdna/npu2_regs.c
>>   create mode 100644 drivers/accel/amdxdna/npu4_regs.c
>>   create mode 100644 drivers/accel/amdxdna/npu5_regs.c
>>   create mode 100644 include/uapi/drm/amdxdna_accel.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 36d66b141352..3d2b9f1f1a07 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1146,6 +1146,14 @@ M:    Sanjay R Mehta <sanju.mehta@....com>
>>   S:    Maintained
>>   F:    drivers/spi/spi-amd.c
>>   +AMD XDNA DRIVER
>> +M:    Min Ma <min.ma@....com>
>> +M:    Lizhi Hou <lizhi.hou@....com>
>> +L:    dri-devel@...ts.freedesktop.org
>> +S:    Supported
>> +F:    drivers/accel/amdxdna
>
> Trailing "/"?
>
>> +F:    include/uapi/drm/amdxdna_accel.h
>
> T: ?
Sure. I will fix above two.
>
>> +
>>   AMD XGBE DRIVER
>>   M:    "Shyam Sundar S K" <Shyam-sundar.S-k@....com>
>>   L:    netdev@...r.kernel.org
>
>> diff --git a/drivers/accel/amdxdna/aie2_pci.c 
>> b/drivers/accel/amdxdna/aie2_pci.c
>> new file mode 100644
>> index 000000000000..3660967c00e6
>> --- /dev/null
>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>> @@ -0,0 +1,182 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/amd-iommu.h>
>> +#include <linux/errno.h>
>> +#include <linux/firmware.h>
>> +#include <linux/iommu.h>
>
> You are clearly missing linux/pci.h and I suspect many more.
Other headers are indirectly included by "aie2_pci.h" underneath.
>
>> +
>> +#include "aie2_pci.h"
>> +
>> +static void aie2_hw_stop(struct amdxdna_dev *xdna)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
>> +    struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
>> +
>> +    aie2_psp_stop(ndev->psp_hdl);
>> +    aie2_smu_fini(ndev);
>> +    pci_clear_master(pdev);
>> +    pci_disable_device(pdev);
>
> pci_clear_master() is redundant with pci_disable_device(), no?
You are right. I will remove it.
>
>> +}
>> +
>> +static int aie2_hw_start(struct amdxdna_dev *xdna)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
>> +    struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
>> +    int ret;
>> +
>> +    ret = pci_enable_device(pdev);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "failed to enable device, ret %d", ret);
>> +        return ret;
>> +    }
>> +    pci_set_master(pdev);
>> +
>> +    ret = aie2_smu_init(ndev);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "failed to init smu, ret %d", ret);
>> +        goto disable_dev;
>> +    }
>> +
>> +    ret = aie2_psp_start(ndev->psp_hdl);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "failed to start psp, ret %d", ret);
>> +        goto fini_smu;
>> +    }
>> +
>> +    return 0;
>> +
>> +fini_smu:
>> +    aie2_smu_fini(ndev);
>> +disable_dev:
>> +    pci_disable_device(pdev);
>> +    pci_clear_master(pdev);
>
> Again, clear_master appears redundant
>
>> +
>> +    return ret;
>> +}
>> +
>> +static int aie2_init(struct amdxdna_dev *xdna)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
>> +    struct amdxdna_dev_hdl *ndev;
>> +    struct psp_config psp_conf;
>> +    const struct firmware *fw;
>> +    void __iomem * const *tbl;
>
> Is there an extra "*" here?
It looks a match with pcim_iomap_table() return type.
>
>> +    int i, bars, nvec, ret;
>> +
>> +    ndev = devm_kzalloc(&pdev->dev, sizeof(*ndev), GFP_KERNEL);
>> +    if (!ndev)
>> +        return -ENOMEM;
>> +
>> +    ndev->priv = xdna->dev_info->dev_priv;
>> +    ndev->xdna = xdna;
>> +
>> +    ret = request_firmware(&fw, ndev->priv->fw_path, &pdev->dev);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "failed to request_firmware %s, ret %d",
>> +             ndev->priv->fw_path, ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = pcim_enable_device(pdev);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "pcim enable device failed, ret %d", ret);
>> +        goto release_fw;
>> +    }
>> +
>> +    bars = pci_select_bars(pdev, IORESOURCE_MEM);
>> +    for (i = 0; i < PSP_MAX_REGS; i++) {
>> +        if (!(BIT(PSP_REG_BAR(ndev, i)) && bars)) {
>> +            XDNA_ERR(xdna, "does not get pci bar%d",
>> +                 PSP_REG_BAR(ndev, i));
>> +            ret = -EINVAL;
>> +            goto release_fw;
>> +        }
>> +    }
>> +
>> +    ret = pcim_iomap_regions(pdev, bars, "amdxdna-npu");
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "map regions failed, ret %d", ret);
>> +        goto release_fw;
>> +    }
>> +
>> +    tbl = pcim_iomap_table(pdev);
>> +    if (!tbl) {
>> +        XDNA_ERR(xdna, "Cannot get iomap table");
>> +        ret = -ENOMEM;
>> +        goto release_fw;
>> +    }
>> +    ndev->sram_base = tbl[xdna->dev_info->sram_bar];
>> +    ndev->smu_base = tbl[xdna->dev_info->smu_bar];
>
> Doesn't this throw a sparse error from directly accessing memory 
> marked iomem?

It does not. tbl[] returns the iomem pointer and the driver uses 
writel/readl to access.

e.g. writel(0, SMU_REG(ndev, SMU_RESP_REG));

>
>> +
>> +    ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Failed to set DMA mask: %d", ret);
>> +        goto release_fw;
>> +    }
>> +
>> +    nvec = pci_msix_vec_count(pdev);
>
> This feels weird.  Can your device advertise variable number of MSI-X 
> vectors?  It only works if all of the vectors are used?
That is possible. the driver supports different hardware. And the fw 
assigns vector for hardware context dynamically. So the driver needs to 
allocate all vectors ahead.
>
>> +    if (nvec <= 0) {
>> +        XDNA_ERR(xdna, "does not get number of interrupt vector");
>> +        ret = -EINVAL;
>> +        goto release_fw;
>> +    }
>> +
>> +    ret = pci_alloc_irq_vectors(pdev, nvec, nvec, PCI_IRQ_MSIX);
>> +    if (ret < 0) {
>> +        XDNA_ERR(xdna, "failed to alloc irq vectors, ret %d", ret);
>> +        goto release_fw;
>> +    }
>> +
>> +    ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Enable PASID failed, ret %d", ret);
>> +        goto free_irq;
>> +    }
>> +
>> +    psp_conf.fw_size = fw->size;
>> +    psp_conf.fw_buf = fw->data;
>> +    for (i = 0; i < PSP_MAX_REGS; i++)
>> +        psp_conf.psp_regs[i] = tbl[PSP_REG_BAR(ndev, i)] + 
>> PSP_REG_OFF(ndev, i);
>> +    ndev->psp_hdl = aie2m_psp_create(&pdev->dev, &psp_conf);
>> +    if (!ndev->psp_hdl) {
>> +        XDNA_ERR(xdna, "failed to create psp");
>> +        ret = -ENOMEM;
>> +        goto disable_sva;
>> +    }
>> +    xdna->dev_handle = ndev;
>> +
>> +    ret = aie2_hw_start(xdna);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "start npu failed, ret %d", ret);
>> +        goto disable_sva;
>> +    }
>> +
>> +    release_firmware(fw);
>> +    return 0;
>> +
>> +disable_sva:
>> +    iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
>> +free_irq:
>> +    pci_free_irq_vectors(pdev);
>> +release_fw:
>> +    release_firmware(fw);
>> +
>> +    return ret;
>> +}
>> +
>> +static void aie2_fini(struct amdxdna_dev *xdna)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
>> +
>> +    aie2_hw_stop(xdna);
>> +    iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
>> +    pci_free_irq_vectors(pdev);
>> +}
>> +
>> +const struct amdxdna_dev_ops aie2_ops = {
>> +    .init           = aie2_init,
>> +    .fini           = aie2_fini,
>> +};
>> diff --git a/drivers/accel/amdxdna/aie2_pci.h 
>> b/drivers/accel/amdxdna/aie2_pci.h
>> new file mode 100644
>> index 000000000000..984bf65b7a2b
>> --- /dev/null
>> +++ b/drivers/accel/amdxdna/aie2_pci.h
>> @@ -0,0 +1,144 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2023-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef _AIE2_PCI_H_
>> +#define _AIE2_PCI_H_
>> +
>> +#include <linux/device.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/io.h>
>> +
>> +#include "amdxdna_pci_drv.h"
>> +
>> +#define AIE2_INTERVAL    20000    /* us */
>> +#define AIE2_TIMEOUT    1000000    /* us */
>> +
>> +/* Firmware determines device memory base address and size */
>> +#define AIE2_DEVM_BASE    0x4000000
>> +#define AIE2_DEVM_SIZE    (48 * 1024 * 1024)
>
> I'd expect to see some SZ_ macros used here
Sure.
>
>> +
>> +#define NDEV2PDEV(ndev) \
>> +    (to_pci_dev((ndev)->xdna->ddev.dev))
>> +
>> +#define AIE2_SRAM_OFF(ndev, addr) \
>> +    ((addr) - (ndev)->priv->sram_dev_addr)
>> +#define AIE2_MBOX_OFF(ndev, addr) \
>> +    ((addr) - (ndev)->priv->mbox_dev_addr)
>> +
>> +#define PSP_REG_BAR(ndev, idx) \
>> +    ((ndev)->priv->psp_regs_off[(idx)].bar_idx)
>> +#define PSP_REG_OFF(ndev, idx) \
>> +    ((ndev)->priv->psp_regs_off[(idx)].offset)
>> +#define SRAM_REG_OFF(ndev, idx) \
>> +    ((ndev)->priv->sram_offs[(idx)].offset)
>
> Really looks like these 6 macros can all fit on a single line, why 
> split them up?
I will fix these. and other places.
>
>> +
>> +#define SMU_REG(ndev, idx) \
>> +({ \
>> +    typeof(ndev) _ndev = ndev; \
>> +    ((_ndev)->smu_base + (_ndev)->priv->smu_regs_off[(idx)].offset); \
>> +})
>> +#define SRAM_GET_ADDR(ndev, idx) \
>> +({ \
>> +    typeof(ndev) _ndev = ndev; \
>> +    ((_ndev)->sram_base + SRAM_REG_OFF((_ndev), (idx))); \
>> +})
>> +
>> +#define SMU_MPNPUCLK_FREQ_MAX(ndev) \
>> +    ((ndev)->priv->smu_mpnpuclk_freq_max)
>> +#define SMU_HCLK_FREQ_MAX(ndev) \
>> +    ((ndev)->priv->smu_hclk_freq_max)
>> +
>> +enum aie2_smu_reg_idx {
>> +    SMU_CMD_REG = 0,
>> +    SMU_ARG_REG,
>> +    SMU_INTR_REG,
>> +    SMU_RESP_REG,
>> +    SMU_OUT_REG,
>> +    SMU_MAX_REGS /* Kepp this at the end */
>
> "keep"
Got it. Thanks.
>
>> diff --git a/drivers/accel/amdxdna/aie2_psp.c 
>> b/drivers/accel/amdxdna/aie2_psp.c
>> new file mode 100644
>> index 000000000000..c24207c252a1
>> --- /dev/null
>> +++ b/drivers/accel/amdxdna/aie2_psp.c
>> @@ -0,0 +1,137 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2022-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/firmware.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/slab.h>
>> +#include "aie2_pci.h"
>> +
>> +#define PSP_STATUS_READY    BIT(31)
>> +
>> +/* PSP commands */
>> +#define PSP_VALIDATE        1
>> +#define PSP_START        2
>> +#define PSP_RELEASE_TMR        3
>> +
>> +/* PSP special arguments */
>> +#define PSP_START_COPY_FW    1
>> +
>> +/* PSP response error code */
>> +#define PSP_ERROR_CANCEL    0xFFFF0002
>> +#define PSP_ERROR_BAD_STATE    0xFFFF0007
>> +
>> +#define PSP_FW_ALIGN        0x10000
>> +#define PSP_POLL_INTERVAL    20000    /* us */
>> +#define PSP_POLL_TIMEOUT    1000000    /* us */
>> +
>> +#define PSP_REG(p, reg) \
>> +    ((p)->psp_regs[reg])
>
> This is not valid with __iomem
Sorry, I am not fully understand the comment.

Each element of ->psp_regs[] is a void __iomem *. And readl/writel are 
used with it.

e.g. writel(reg_vals[i], PSP_REG(psp, i));

>
>> +struct psp_device *aie2m_psp_create(struct device *dev, struct 
>> psp_config *conf)
>> +{
>> +    struct psp_device *psp;
>> +    u64 offset;
>> +
>> +    psp = devm_kzalloc(dev, sizeof(*psp), GFP_KERNEL);
>> +    if (!psp)
>> +        return NULL;
>> +
>> +    psp->dev = dev;
>> +    memcpy(psp->psp_regs, conf->psp_regs, sizeof(psp->psp_regs));
>> +
>> +    psp->fw_buf_sz = ALIGN(conf->fw_size, PSP_FW_ALIGN) + PSP_FW_ALIGN;
>> +    psp->fw_buffer = devm_kmalloc(psp->dev, psp->fw_buf_sz, 
>> GFP_KERNEL);
>
> Feels like this (and a bunch of other instances I haven't commented 
> on) should be drmm_* allocs.

The PSP code is kind of low level and directly interact with hardware. 
All the PSP interfaces use struct device * instead of drm_device. I 
think it is kind make sense because PSP is not related to drm.

I will scan all other allocs and change them to drmm_* allocs for the 
code related to drm_device. Does this sound ok to you?

>
>> +    if (!psp->fw_buffer) {
>> +        dev_err(psp->dev, "no memory for fw buffer");
>> +        return NULL;
>> +    }
>> +
>> +    psp->fw_paddr = virt_to_phys(psp->fw_buffer);
>
> I'm pretty sure virt_to_phys() is always wrong

The hardware exposes several registers to communicate with platform PSP 
(AMD Platform Security Processor) to load NPU firmware. And PSP only 
accept host physical address with current hardware.

I understand usually virt_to_phys() should not be needed for device 
driver. And maybe it is ok to use if there is hardware requirement? I 
can see some drivers use it as well.

>
>> +    offset = ALIGN(psp->fw_paddr, PSP_FW_ALIGN) - psp->fw_paddr;
>> +    psp->fw_paddr += offset;
>> +    memcpy(psp->fw_buffer + offset, conf->fw_buf, conf->fw_size);
>> +
>> +    return psp;
>> +}
>> diff --git a/drivers/accel/amdxdna/amdxdna_drm.c 
>> b/drivers/accel/amdxdna/amdxdna_drm.c
>> new file mode 100644
>> index 000000000000..91e4f9c9dac9
>> --- /dev/null
>> +++ b/drivers/accel/amdxdna/amdxdna_drm.c
>
> What is the point of this file?  Seems like all of this could just be 
> in amdxdna_pci_drv.c
The future product may have NPU with non-pci device. So it might be a 
amdxdna_plat_drv.c and share the same amdxdna_drm.c in the future.
>
>> @@ -0,0 +1,20 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2022-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <drm/drm_ioctl.h>
>> +#include <drm/drm_accel.h>
>> +
>> +#include "amdxdna_drm.h"
>> +
>> +DEFINE_DRM_ACCEL_FOPS(amdxdna_fops);
>> +
>> +const struct drm_driver amdxdna_drm_drv = {
>> +    .driver_features = DRIVER_GEM | DRIVER_COMPUTE_ACCEL,
>> +    .fops = &amdxdna_fops,
>> +    .name = "amdxdna_accel_driver",
>> +    .desc = "AMD XDNA DRM implementation",
>> +    .major = AMDXDNA_DRIVER_MAJOR,
>> +    .minor = AMDXDNA_DRIVER_MINOR,
>
> These are deprecated.  You should drop them
Sure. I will remove them.
>
>> +};
>> diff --git a/drivers/accel/amdxdna/amdxdna_drm.h 
>> b/drivers/accel/amdxdna/amdxdna_drm.h
>> new file mode 100644
>> index 000000000000..2b18bcbdc23e
>> --- /dev/null
>> +++ b/drivers/accel/amdxdna/amdxdna_drm.h
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef _AMDXDNA_DRM_H_
>> +#define _AMDXDNA_DRM_H_
>> +
>> +#include <drm/amdxdna_accel.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_gem.h>
>> +#include <drm/drm_managed.h>
>> +#include <drm/drm_print.h>
>> +#include <drm/drm_file.h>
>> +
>> +#define XDNA_INFO(xdna, fmt, args...) drm_info(&(xdna)->ddev, fmt, 
>> ##args)
>> +#define XDNA_WARN(xdna, fmt, args...) drm_warn(&(xdna)->ddev, "%s: 
>> "fmt, __func__, ##args)
>> +#define XDNA_ERR(xdna, fmt, args...) drm_err(&(xdna)->ddev, "%s: 
>> "fmt, __func__, ##args)
>> +#define XDNA_DBG(xdna, fmt, args...) drm_dbg(&(xdna)->ddev, fmt, 
>> ##args)
>> +#define XDNA_INFO_ONCE(xdna, fmt, args...) 
>> drm_info_once(&(xdna)->ddev, fmt, ##args)
>> +
>> +#define to_xdna_dev(drm_dev) \
>> +    ((struct amdxdna_dev *)container_of(drm_dev, struct amdxdna_dev, 
>> ddev))
>> +
>> +extern const struct drm_driver amdxdna_drm_drv;
>> +
>> +struct amdxdna_dev;
>> +
>> +/*
>> + * struct amdxdna_dev_ops - Device hardware operation callbacks
>> + */
>> +struct amdxdna_dev_ops {
>> +    int (*init)(struct amdxdna_dev *xdna);
>> +    void (*fini)(struct amdxdna_dev *xdna);
>> +};
>> +
>> +/*
>> + * struct amdxdna_dev_info - Device hardware information
>> + * Record device static information, like reg, mbox, PSP, SMU bar 
>> index,
>
> The last "," is weird
Will fix it. Thanks.
>
>> + */
>> +struct amdxdna_dev_info {
>> +    int                reg_bar;
>> +    int                mbox_bar;
>> +    int                sram_bar;
>> +    int                psp_bar;
>> +    int                smu_bar;
>> +    int                device_type;
>> +    int                first_col;
>> +    u32                dev_mem_buf_shift;
>> +    u64                dev_mem_base;
>> +    size_t                dev_mem_size;
>> +    char                *vbnv;
>> +    const struct amdxdna_dev_priv    *dev_priv;
>> +    const struct amdxdna_dev_ops    *ops;
>> +};
>> +
>> +struct amdxdna_dev {
>> +    struct drm_device        ddev;
>> +    struct amdxdna_dev_hdl        *dev_handle;
>> +    const struct amdxdna_dev_info    *dev_info;
>> +
>> +    struct mutex            dev_lock; /* per device lock */
>> +};
>> +
>> +#endif /* _AMDXDNA_DRM_H_ */
>> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c 
>> b/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> new file mode 100644
>> index 000000000000..7d0cfd918b0e
>> --- /dev/null
>> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2022-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/module.h>
>> +
>> +#include "amdxdna_pci_drv.h"
>> +
>> +/*
>> + *  There are platforms which share the same PCI device ID
>> + *  but have different PCI revision IDs. So, let the PCI class
>> + *  determine the probe and later use the (device_id, rev_id)
>> + *  pair as a key to select the devices.
>> + */
>
> Huh?  So, VID == AMD, DID == 0x17f0, rev == 0x1 is a completely 
> different device?  That feels like a PCIe spec violation...
Maybe the comment is misleading. The hardware with same device id 0x17f0 
uses the same commands, registers etc. And they are same device with 
different revisions.
>
>> +static const struct pci_device_id pci_ids[] = {
>> +    { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_ANY_ID),
>> +        .class = PCI_CLASS_SP_OTHER << 8,
>
> Weird.  I would have expected the Accelerator class to be used
We contacted our hardware team to figure out why accelerator class is 
not used here. Some of hardware is already released. Hopefully hardware 
team may consider to use accelerator class with new products.
>
>> +        .class_mask = 0xFFFF00,
>> +    },
>> +    {0}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(pci, pci_ids);
>> +
>> +static const struct amdxdna_device_id amdxdna_ids[] = {
>> +    { 0x1502, 0x0,  &dev_npu1_info },
>> +    { 0x17f0, 0x0,  &dev_npu2_info },
>> +    { 0x17f0, 0x10, &dev_npu4_info },
>> +    { 0x17f0, 0x11, &dev_npu5_info },
>> +    {0}
>> +};
>> +
>
>> +module_pci_driver(amdxdna_pci_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("XRT Team <runtimeca39d@....com>");
>> +MODULE_VERSION("0.1");
>
> This is redundant with the kernel version, no?  How are you going to 
> have different module versions for the same kernel version?
This is used in out of tree driver. It is meaningless for upstream. I 
will remove it.
>
>> +MODULE_DESCRIPTION("amdxdna driver");
>> diff --git a/include/uapi/drm/amdxdna_accel.h 
>> b/include/uapi/drm/amdxdna_accel.h
>> new file mode 100644
>> index 000000000000..1b699464150e
>> --- /dev/null
>> +++ b/include/uapi/drm/amdxdna_accel.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (C) 2022-2024, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef _UAPI_AMDXDNA_ACCEL_H_
>> +#define _UAPI_AMDXDNA_ACCEL_H_
>> +
>> +#include "drm.h"
>> +
>> +#if defined(__cplusplus)
>> +extern "C" {
>> +#endif
>> +
>> +#define AMDXDNA_DRIVER_MAJOR    1
>> +#define AMDXDNA_DRIVER_MINOR    0
>
> Drop these.

Sure.


Thanks,

Lizhi

>
>> +
>> +enum amdxdna_device_type {
>> +    AMDXDNA_DEV_TYPE_UNKNOWN = -1,
>> +    AMDXDNA_DEV_TYPE_KMQ,
>> +};
>> +
>> +#if defined(__cplusplus)
>> +} /* extern c end */
>> +#endif
>> +
>> +#endif /* _UAPI_AMDXDNA_ACCEL_H_ */
>

Powered by blists - more mailing lists