diff mbox series

[V2,1/4] drivers/fpga/amd: Add new driver amd versal-pci

Message ID 20241210183734.30803-2-yidong.zhang@amd.com (mailing list archive)
State New
Headers show
Series Add versal-pci driver | expand

Commit Message

Yidong Zhang Dec. 10, 2024, 6:37 p.m. UTC
AMD Versal based PCIe card, including V70, is designed for AI inference
efficiency and is tuned for video analytics and natural language processing
applications.

The driver architecture:

  +---------+  Communication +---------+  Remote  +-----+------+
  |         |  Channel       |         |  Queue   |     |      |
  | User PF | <============> | Mgmt PF | <=======>| FW  | FPGA |
  +---------+                +---------+          +-----+------+
    PL Data                    base FW
                               APU FW
                               PL Data (copy)
 - PL (FPGA Program Logic)
 - FW (Firmware)

There are 2 separate drivers from the original XRT[1] design.
 - UserPF driver
 - MgmtPF driver

The new AMD versal-pci driver will replace the MgmtPF driver for Versal
PCIe card.

The XRT[1] is already open-sourced. It includes solution of runtime for
many different type of PCIe Based cards. It also provides utilities for
managing and programming the devices.

The AMD versal-pci stands for AMD Versal brand PCIe device management
driver. This driver provides the following functionalities:

   - module and PCI device initialization
     this driver will attach to specific device id of V70 card;
     the driver will initialize itself based on bar resources for
     - communication channel:
       a hardware message service between mgmt PF and user PF
     - remote queue:
       a hardware queue based ring buffer service between mgmt PF and PCIe
       hardware firmware for programming FPGA Program Logic, loading
       firmware and checking card healthy status.

   - programming FW
     - The base FW is downloaded onto the flash of the card.
     - The APU FW is downloaded once after a POR (power on reset).
     - Reloading the MgmtPF driver will not change any existing hardware.

   - programming FPGA hardware binaries - PL Data
    - using fpga framework ops to support re-programing FPGA
    - the re-programming request will be initiated from the existing UserPF
      driver only, and the MgmtPF driver load the matched PL Data after
      receiving request from the communication channel. The matching PL
      Data is indexed by the PL Data UUID and Base FW UUID.
      - The Base FW UUID identifies unique based hardware. Often called the
      interface UUID.
      - The PL Data UUID identifies unique PL design that is generated
      based on the base hardware. Often called xclbin UUID.

    - Example:
      4fdebe35[...trimmed...]_96df7d[...trimmed...].xclbin
      |                     | |                   |
      +--  xclbin UUID    --+ +--interface UUID --+

[1] https://github.com/Xilinx/XRT/blob/master/README.rst

Co-developed-by: DMG Karthik <Karthik.DMG@amd.com>
Signed-off-by: DMG Karthik <Karthik.DMG@amd.com>
Co-developed-by: Nishad Saraf <nishads@amd.com>
Signed-off-by: Nishad Saraf <nishads@amd.com>
Co-developed-by: Prapul Krishnamurthy <prapulk@amd.com>
Signed-off-by: Prapul Krishnamurthy <prapulk@amd.com>
Co-developed-by: Hayden Laccabue <hayden.laccabue@amd.com>
Signed-off-by: Hayden Laccabue <hayden.laccabue@amd.com>
Signed-off-by: Yidong Zhang <yidong.zhang@amd.com>
---
 MAINTAINERS                        |   6 +
 drivers/fpga/Kconfig               |   3 +
 drivers/fpga/Makefile              |   3 +
 drivers/fpga/amd/Kconfig           |  15 ++
 drivers/fpga/amd/Makefile          |   5 +
 drivers/fpga/amd/versal-pci-main.c | 328 +++++++++++++++++++++++++++++
 drivers/fpga/amd/versal-pci.h      |  86 ++++++++
 7 files changed, 446 insertions(+)
 create mode 100644 drivers/fpga/amd/Kconfig
 create mode 100644 drivers/fpga/amd/Makefile
 create mode 100644 drivers/fpga/amd/versal-pci-main.c
 create mode 100644 drivers/fpga/amd/versal-pci.h

Comments

Xu Yilun Jan. 26, 2025, 9:24 a.m. UTC | #1
> +static int versal_pci_program_axlf(struct versal_pci_device *vdev, char *data, size_t size)
> +{
> +	const struct axlf *axlf = (struct axlf *)data;
> +	struct fpga_image_info *image_info;
> +	int ret;
> +
> +	image_info = fpga_image_info_alloc(&vdev->pdev->dev);
> +	if (!image_info)
> +		return -ENOMEM;
> +
> +	image_info->count = axlf->header.length;
> +	image_info->buf = (char *)axlf;
> +
> +	ret = fpga_mgr_load(vdev->fdev->mgr, image_info);

I see, but this is not working like this. fpga_mgr_load() is intended to be
called by fpga_region, any reprogramming API should come from fpga_region,
and fpga_region could provide uAPI for userspace reprogramming.

If your driver act both as a fpga_mgr backend and a fpga_mgr kAPI user,
then you don't have to bother using fpga framework at all.

Thanks,
Yilun

> +	if (ret) {
> +		vdev_err(vdev, "failed to load xclbin: %d", ret);
> +		goto exit;
> +	}
> +
> +	vdev_info(vdev, "Downloaded axlf %pUb of size %zu Bytes", &axlf->header.uuid, size);
> +	uuid_copy(&vdev->xclbin_uuid, &axlf->header.uuid);
> +
> +exit:
> +	fpga_image_info_free(image_info);
> +
> +	return ret;
> +}
Christophe JAILLET Jan. 26, 2025, 10:12 a.m. UTC | #2
Le 10/12/2024 à 19:37, Yidong Zhang a écrit :
> AMD Versal based PCIe card, including V70, is designed for AI inference
> efficiency and is tuned for video analytics and natural language processing
> applications.

...

> +static void versal_pci_uuid_parse(struct versal_pci_device *vdev, uuid_t *uuid)
> +{
> +	char str[UUID_STRING_LEN];
> +	u8 i, j;
> +
> +	/* parse uuid into a valid uuid string format */
> +	for (i  = 0, j = 0; i < strlen(vdev->fw_id) && i < sizeof(str); i++) {

Unneeded extra space in "i  = 0"

I think that the compiler already does it on its own, but the strlen 
could be computed before the for loop.

> +		str[j++] = vdev->fw_id[i];
> +		if (j == 8 || j == 13 || j == 18 || j == 23)
> +			str[j++] = '-';
> +	}
> +
> +	uuid_parse(str, uuid);
> +	vdev_info(vdev, "Interface uuid %pU", uuid);
> +}
> +
> +static struct fpga_device *versal_pci_fpga_init(struct versal_pci_device *vdev)
> +{
> +	struct device *dev = &vdev->pdev->dev;
> +	struct fpga_manager_info info = { 0 };

Is the { 0 } needed?
Isn't the assigment below enough?

> +	struct fpga_device *fdev;
> +	int ret;
> +
> +	fdev = devm_kzalloc(dev, sizeof(*fdev), GFP_KERNEL);
> +	if (!fdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	fdev->vdev = vdev;
> +
> +	info = (struct fpga_manager_info) {
> +		.name = "AMD Versal FPGA Manager",
> +		.mops = &versal_pci_fpga_ops,
> +		.priv = fdev,
> +	};
> +
> +	fdev->mgr = fpga_mgr_register_full(dev, &info);
> +	if (IS_ERR(fdev->mgr)) {
> +		ret = PTR_ERR(fdev->mgr);
> +		vdev_err(vdev, "Failed to register FPGA manager, err %d", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	/* Place holder for rm_queue_get_fw_id(vdev->rdev) */
> +	versal_pci_uuid_parse(vdev, &vdev->intf_uuid);
> +
> +	return fdev;
> +}

...

> +static struct firmware_device *versal_pci_fw_upload_init(struct versal_pci_device *vdev)
> +{
> +	struct device *dev = &vdev->pdev->dev;
> +	struct firmware_device *fwdev;
> +	u32 devid;
> +
> +	fwdev = devm_kzalloc(dev, sizeof(*fwdev), GFP_KERNEL);
> +	if (!fwdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	devid = versal_pci_devid(vdev);
> +	fwdev->name = kasprintf(GFP_KERNEL, "%s%x", DRV_NAME, devid);

Why is fwdev managed, and not fwdev->name?
It looks ok as-is, but using devm_kasprintf() would save a few lines of 
code.

> +	if (!fwdev->name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	fwdev->fw = firmware_upload_register(THIS_MODULE, dev, fwdev->name,
> +					     &versal_pci_fw_ops, fwdev);
> +	if (IS_ERR(fwdev->fw)) {
> +		kfree(fwdev->name);
> +		return ERR_CAST(fwdev->fw);
> +	}
> +
> +	fwdev->vdev = vdev;
> +
> +	return fwdev;
> +}

...

> +static int versal_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pdev_id)
> +{
> +	struct versal_pci_device *vdev;
> +	int ret;
> +
> +	vdev = devm_kzalloc(&pdev->dev, sizeof(*vdev), GFP_KERNEL);
> +	if (!vdev)
> +		return -ENOMEM;
> +
> +	pci_set_drvdata(pdev, vdev);
> +	vdev->pdev = pdev;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		vdev_err(vdev, "Failed to enable device %d", ret);
> +		return ret;
> +	}
> +
> +	vdev->io_regs = pcim_iomap_region(vdev->pdev, MGMT_BAR, DRV_NAME);
> +	if (IS_ERR(vdev->io_regs)) {
> +		vdev_err(vdev, "Failed to map RM shared memory BAR%d", MGMT_BAR);
> +		return PTR_ERR(vdev->io_regs);
> +	}
> +
> +	ret = versal_pci_device_setup(vdev);
> +	if (ret) {
> +		vdev_err(vdev, "Failed to setup Versal device %d", ret);
> +		return ret;
> +	}
> +
> +	vdev_dbg(vdev, "Successfully probed %s driver!", DRV_NAME);

Usually, such debug messages are not needed.
No strong opinion about it.

> +	return 0;
> +}

...

CJ
Christophe JAILLET Jan. 26, 2025, 10:16 a.m. UTC | #3
Le 10/12/2024 à 19:37, Yidong Zhang a écrit :
> AMD Versal based PCIe card, including V70, is designed for AI inference
> efficiency and is tuned for video analytics and natural language processing
> applications.

...

> +#define vdev_info(vdev, fmt, args...)					\
> +	dev_info(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
> +
> +#define vdev_warn(vdev, fmt, args...)					\
> +	dev_warn(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
> +
> +#define vdev_err(vdev, fmt, args...)					\
> +	dev_err(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
> +
> +#define vdev_dbg(vdev, fmt, args...)					\
> +	dev_dbg(&(vdev)->pdev->dev, fmt, ##args)

These helpers could also add the trailing \n.
It is apparently not added in the messages themselves.

CJ
Xu Yilun Jan. 26, 2025, 10:32 a.m. UTC | #4
On Tue, Dec 10, 2024 at 10:37:30AM -0800, Yidong Zhang wrote:
> AMD Versal based PCIe card, including V70, is designed for AI inference
> efficiency and is tuned for video analytics and natural language processing
> applications.
> 
> The driver architecture:
> 
>   +---------+  Communication +---------+  Remote  +-----+------+
>   |         |  Channel       |         |  Queue   |     |      |
>   | User PF | <============> | Mgmt PF | <=======>| FW  | FPGA |
>   +---------+                +---------+          +-----+------+
>     PL Data                    base FW
>                                APU FW
>                                PL Data (copy)
>  - PL (FPGA Program Logic)
>  - FW (Firmware)
> 
> There are 2 separate drivers from the original XRT[1] design.
>  - UserPF driver
>  - MgmtPF driver
> 
> The new AMD versal-pci driver will replace the MgmtPF driver for Versal
> PCIe card.
> 
> The XRT[1] is already open-sourced. It includes solution of runtime for
> many different type of PCIe Based cards. It also provides utilities for
> managing and programming the devices.
> 
> The AMD versal-pci stands for AMD Versal brand PCIe device management
> driver. This driver provides the following functionalities:
> 
>    - module and PCI device initialization
>      this driver will attach to specific device id of V70 card;
>      the driver will initialize itself based on bar resources for
>      - communication channel:
>        a hardware message service between mgmt PF and user PF
>      - remote queue:
>        a hardware queue based ring buffer service between mgmt PF and PCIe
>        hardware firmware for programming FPGA Program Logic, loading
>        firmware and checking card healthy status.
> 
>    - programming FW
>      - The base FW is downloaded onto the flash of the card.
>      - The APU FW is downloaded once after a POR (power on reset).
>      - Reloading the MgmtPF driver will not change any existing hardware.
> 
>    - programming FPGA hardware binaries - PL Data
>     - using fpga framework ops to support re-programing FPGA
>     - the re-programming request will be initiated from the existing UserPF
>       driver only, and the MgmtPF driver load the matched PL Data after
>       receiving request from the communication channel. The matching PL

I think this is not the way the FPGA generic framework should do. A FPGA
region user (your userPF driver) should not also be the reprogram requester.
The user driver cannot deal with the unexpected HW change if it happens.
Maybe after reprogramming, the user driver cannot match the device
anymore, and if user driver is still working on it, crash.

The expected behavior is, the FPGA region removes user devices (thus
detaches user drivers), does reprogramming, re-enumerates/rescans and
matches new devices with new drivers. And I think that's what Nava is
working on.

BTW, AFAICS the expected flow is easier to implement for of-fpga-region,
but harder for PCI devices. But I think that's the right direction and
should try to work it out.

Thanks,
Yilun
Yidong Zhang Jan. 26, 2025, 7:46 p.m. UTC | #5
On 1/26/25 02:32, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, Dec 10, 2024 at 10:37:30AM -0800, Yidong Zhang wrote:
>> AMD Versal based PCIe card, including V70, is designed for AI inference
>> efficiency and is tuned for video analytics and natural language processing
>> applications.
>>
>> The driver architecture:
>>
>>    +---------+  Communication +---------+  Remote  +-----+------+
>>    |         |  Channel       |         |  Queue   |     |      |
>>    | User PF | <============> | Mgmt PF | <=======>| FW  | FPGA |
>>    +---------+                +---------+          +-----+------+
>>      PL Data                    base FW
>>                                 APU FW
>>                                 PL Data (copy)
>>   - PL (FPGA Program Logic)
>>   - FW (Firmware)
>>
>> There are 2 separate drivers from the original XRT[1] design.
>>   - UserPF driver
>>   - MgmtPF driver
>>
>> The new AMD versal-pci driver will replace the MgmtPF driver for Versal
>> PCIe card.
>>
>> The XRT[1] is already open-sourced. It includes solution of runtime for
>> many different type of PCIe Based cards. It also provides utilities for
>> managing and programming the devices.
>>
>> The AMD versal-pci stands for AMD Versal brand PCIe device management
>> driver. This driver provides the following functionalities:
>>
>>     - module and PCI device initialization
>>       this driver will attach to specific device id of V70 card;
>>       the driver will initialize itself based on bar resources for
>>       - communication channel:
>>         a hardware message service between mgmt PF and user PF
>>       - remote queue:
>>         a hardware queue based ring buffer service between mgmt PF and PCIe
>>         hardware firmware for programming FPGA Program Logic, loading
>>         firmware and checking card healthy status.
>>
>>     - programming FW
>>       - The base FW is downloaded onto the flash of the card.
>>       - The APU FW is downloaded once after a POR (power on reset).
>>       - Reloading the MgmtPF driver will not change any existing hardware.
>>
>>     - programming FPGA hardware binaries - PL Data
>>      - using fpga framework ops to support re-programing FPGA
>>      - the re-programming request will be initiated from the existing UserPF
>>        driver only, and the MgmtPF driver load the matched PL Data after
>>        receiving request from the communication channel. The matching PL
> 
> I think this is not the way the FPGA generic framework should do. A FPGA
> region user (your userPF driver) should not also be the reprogram requester.
> The user driver cannot deal with the unexpected HW change if it happens.
> Maybe after reprogramming, the user driver cannot match the device
> anymore, and if user driver is still working on it, crash.

One thing to clarify. The current design is:

The userPF driver is the only requester. The mgmtPF has no uAPI to 
reprogram the FPGA.


> 
> The expected behavior is, the FPGA region removes user devices (thus
> detaches user drivers), does reprogramming, re-enumerates/rescans and
> matches new devices with new drivers. And I think that's what Nava is
> working on.
> 

Nava's work is different than our current design, our current design is:

the separate userPF driver will detach all services before requesting to 
the mgmtPF to program the FPGA, and after the programming is done, the 
userPF will re-enumerate/rescan the matching new devices.

The mgmtPF is a util driver which is responsible for communicating with 
the mgmtPF PCIe bar resources.


> BTW, AFAICS the expected flow is easier to implement for of-fpga-region,
> but harder for PCI devices. But I think that's the right direction and
> should try to work it out.

Could I recap the suggested design if I understand that correctly...

You are thinking that the mgmtPF (aka. versal-pci) driver should have a 
uAPI to trigger the FPGA re-programing; and using Nava's callback ops to 
detach the separate userPF driver; after re-programing is done, re-attch 
the userPF driver and allow the userPF driver re-enumerate all to match 
the new hardware.

I think my understanding is correct, it is doable.

As long as we can keep our userPF driver as separate driver, the code 
change won't be too big.

> 
> Thanks,
> Yilun
Yidong Zhang Jan. 26, 2025, 7:50 p.m. UTC | #6
On 1/26/25 01:24, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>> +static int versal_pci_program_axlf(struct versal_pci_device *vdev, char *data, size_t size)
>> +{
>> +     const struct axlf *axlf = (struct axlf *)data;
>> +     struct fpga_image_info *image_info;
>> +     int ret;
>> +
>> +     image_info = fpga_image_info_alloc(&vdev->pdev->dev);
>> +     if (!image_info)
>> +             return -ENOMEM;
>> +
>> +     image_info->count = axlf->header.length;
>> +     image_info->buf = (char *)axlf;
>> +
>> +     ret = fpga_mgr_load(vdev->fdev->mgr, image_info);
> 
> I see, but this is not working like this. fpga_mgr_load() is intended to be
> called by fpga_region, any reprogramming API should come from fpga_region,
> and fpga_region could provide uAPI for userspace reprogramming.
> 
> If your driver act both as a fpga_mgr backend and a fpga_mgr kAPI user,
> then you don't have to bother using fpga framework at all.

The versal-pci is more like a util driver that handles requests from the 
separate userPF driver.

Thanks,
David

> 
> Thanks,
> Yilun
> 
>> +     if (ret) {
>> +             vdev_err(vdev, "failed to load xclbin: %d", ret);
>> +             goto exit;
>> +     }
>> +
>> +     vdev_info(vdev, "Downloaded axlf %pUb of size %zu Bytes", &axlf->header.uuid, size);
>> +     uuid_copy(&vdev->xclbin_uuid, &axlf->header.uuid);
>> +
>> +exit:
>> +     fpga_image_info_free(image_info);
>> +
>> +     return ret;
>> +}
Yidong Zhang Jan. 26, 2025, 7:56 p.m. UTC | #7
On 1/26/25 02:12, Christophe JAILLET wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
> 
> 
> Le 10/12/2024 à 19:37, Yidong Zhang a écrit :
>> AMD Versal based PCIe card, including V70, is designed for AI inference
>> efficiency and is tuned for video analytics and natural language 
>> processing
>> applications.
> 
> ...
> 
>> +static void versal_pci_uuid_parse(struct versal_pci_device *vdev, 
>> uuid_t *uuid)
>> +{
>> +     char str[UUID_STRING_LEN];
>> +     u8 i, j;
>> +
>> +     /* parse uuid into a valid uuid string format */
>> +     for (i  = 0, j = 0; i < strlen(vdev->fw_id) && i < sizeof(str); 
>> i++) {
> 
> Unneeded extra space in "i  = 0"

Great catch! I will fix this.

> 
> I think that the compiler already does it on its own, but the strlen
> could be computed before the for loop.

I will change the code.

> 
>> +             str[j++] = vdev->fw_id[i];
>> +             if (j == 8 || j == 13 || j == 18 || j == 23)
>> +                     str[j++] = '-';
>> +     }
>> +
>> +     uuid_parse(str, uuid);
>> +     vdev_info(vdev, "Interface uuid %pU", uuid);
>> +}
>> +
>> +static struct fpga_device *versal_pci_fpga_init(struct 
>> versal_pci_device *vdev)
>> +{
>> +     struct device *dev = &vdev->pdev->dev;
>> +     struct fpga_manager_info info = { 0 };
> 
> Is the { 0 } needed?
> Isn't the assigment below enough?

Right. I will remove the unnecessary { 0 }.

> 
>> +     struct fpga_device *fdev;
>> +     int ret;
>> +
>> +     fdev = devm_kzalloc(dev, sizeof(*fdev), GFP_KERNEL);
>> +     if (!fdev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     fdev->vdev = vdev;
>> +
>> +     info = (struct fpga_manager_info) {
>> +             .name = "AMD Versal FPGA Manager",
>> +             .mops = &versal_pci_fpga_ops,
>> +             .priv = fdev,
>> +     };
>> +
>> +     fdev->mgr = fpga_mgr_register_full(dev, &info);
>> +     if (IS_ERR(fdev->mgr)) {
>> +             ret = PTR_ERR(fdev->mgr);
>> +             vdev_err(vdev, "Failed to register FPGA manager, err 
>> %d", ret);
>> +             return ERR_PTR(ret);
>> +     }
>> +
>> +     /* Place holder for rm_queue_get_fw_id(vdev->rdev) */
>> +     versal_pci_uuid_parse(vdev, &vdev->intf_uuid);
>> +
>> +     return fdev;
>> +}
> 
> ...
> 
>> +static struct firmware_device *versal_pci_fw_upload_init(struct 
>> versal_pci_device *vdev)
>> +{
>> +     struct device *dev = &vdev->pdev->dev;
>> +     struct firmware_device *fwdev;
>> +     u32 devid;
>> +
>> +     fwdev = devm_kzalloc(dev, sizeof(*fwdev), GFP_KERNEL);
>> +     if (!fwdev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     devid = versal_pci_devid(vdev);
>> +     fwdev->name = kasprintf(GFP_KERNEL, "%s%x", DRV_NAME, devid);
> 
> Why is fwdev managed, and not fwdev->name?
> It looks ok as-is, but using devm_kasprintf() would save a few lines of
> code.

I will change the code. Great suggestion.

> 
>> +     if (!fwdev->name)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     fwdev->fw = firmware_upload_register(THIS_MODULE, dev, fwdev->name,
>> +                                          &versal_pci_fw_ops, fwdev);
>> +     if (IS_ERR(fwdev->fw)) {
>> +             kfree(fwdev->name);
>> +             return ERR_CAST(fwdev->fw);
>> +     }
>> +
>> +     fwdev->vdev = vdev;
>> +
>> +     return fwdev;
>> +}
> 
> ...
> 
>> +static int versal_pci_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *pdev_id)
>> +{
>> +     struct versal_pci_device *vdev;
>> +     int ret;
>> +
>> +     vdev = devm_kzalloc(&pdev->dev, sizeof(*vdev), GFP_KERNEL);
>> +     if (!vdev)
>> +             return -ENOMEM;
>> +
>> +     pci_set_drvdata(pdev, vdev);
>> +     vdev->pdev = pdev;
>> +
>> +     ret = pcim_enable_device(pdev);
>> +     if (ret) {
>> +             vdev_err(vdev, "Failed to enable device %d", ret);
>> +             return ret;
>> +     }
>> +
>> +     vdev->io_regs = pcim_iomap_region(vdev->pdev, MGMT_BAR, DRV_NAME);
>> +     if (IS_ERR(vdev->io_regs)) {
>> +             vdev_err(vdev, "Failed to map RM shared memory BAR%d", 
>> MGMT_BAR);
>> +             return PTR_ERR(vdev->io_regs);
>> +     }
>> +
>> +     ret = versal_pci_device_setup(vdev);
>> +     if (ret) {
>> +             vdev_err(vdev, "Failed to setup Versal device %d", ret);
>> +             return ret;
>> +     }
>> +
>> +     vdev_dbg(vdev, "Successfully probed %s driver!", DRV_NAME);
> 
> Usually, such debug messages are not needed.
> No strong opinion about it.

I will remove this. The dbg only enable in the debug kernel, but this 
line isn't necessary. I will fix it.

> 
>> +     return 0;
>> +}
> 
> ...
> 
> CJ
Xu Yilun Feb. 6, 2025, 4:15 a.m. UTC | #8
On Sun, Jan 26, 2025 at 11:46:54AM -0800, Yidong Zhang wrote:
> 
> 
> On 1/26/25 02:32, Xu Yilun wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Tue, Dec 10, 2024 at 10:37:30AM -0800, Yidong Zhang wrote:
> > > AMD Versal based PCIe card, including V70, is designed for AI inference
> > > efficiency and is tuned for video analytics and natural language processing
> > > applications.
> > > 
> > > The driver architecture:
> > > 
> > >    +---------+  Communication +---------+  Remote  +-----+------+
> > >    |         |  Channel       |         |  Queue   |     |      |
> > >    | User PF | <============> | Mgmt PF | <=======>| FW  | FPGA |
> > >    +---------+                +---------+          +-----+------+
> > >      PL Data                    base FW
> > >                                 APU FW
> > >                                 PL Data (copy)
> > >   - PL (FPGA Program Logic)
> > >   - FW (Firmware)
> > > 
> > > There are 2 separate drivers from the original XRT[1] design.
> > >   - UserPF driver
> > >   - MgmtPF driver
> > > 
> > > The new AMD versal-pci driver will replace the MgmtPF driver for Versal
> > > PCIe card.
> > > 
> > > The XRT[1] is already open-sourced. It includes solution of runtime for
> > > many different type of PCIe Based cards. It also provides utilities for
> > > managing and programming the devices.
> > > 
> > > The AMD versal-pci stands for AMD Versal brand PCIe device management
> > > driver. This driver provides the following functionalities:
> > > 
> > >     - module and PCI device initialization
> > >       this driver will attach to specific device id of V70 card;
> > >       the driver will initialize itself based on bar resources for
> > >       - communication channel:
> > >         a hardware message service between mgmt PF and user PF
> > >       - remote queue:
> > >         a hardware queue based ring buffer service between mgmt PF and PCIe
> > >         hardware firmware for programming FPGA Program Logic, loading
> > >         firmware and checking card healthy status.
> > > 
> > >     - programming FW
> > >       - The base FW is downloaded onto the flash of the card.
> > >       - The APU FW is downloaded once after a POR (power on reset).
> > >       - Reloading the MgmtPF driver will not change any existing hardware.
> > > 
> > >     - programming FPGA hardware binaries - PL Data
> > >      - using fpga framework ops to support re-programing FPGA
> > >      - the re-programming request will be initiated from the existing UserPF
> > >        driver only, and the MgmtPF driver load the matched PL Data after
> > >        receiving request from the communication channel. The matching PL
> > 
> > I think this is not the way the FPGA generic framework should do. A FPGA
> > region user (your userPF driver) should not also be the reprogram requester.
> > The user driver cannot deal with the unexpected HW change if it happens.
> > Maybe after reprogramming, the user driver cannot match the device
> > anymore, and if user driver is still working on it, crash.
> 
> One thing to clarify. The current design is:
> 
> The userPF driver is the only requester. The mgmtPF has no uAPI to reprogram
> the FPGA.
> 
> 
> > 
> > The expected behavior is, the FPGA region removes user devices (thus
> > detaches user drivers), does reprogramming, re-enumerates/rescans and
> > matches new devices with new drivers. And I think that's what Nava is
> > working on.
> > 
> 
> Nava's work is different than our current design, our current design is:
> 
> the separate userPF driver will detach all services before requesting to the
> mgmtPF to program the FPGA, and after the programming is done, the userPF
> will re-enumerate/rescan the matching new devices.

That's not align with the Device-Driver Model, A device driver should not
re-enumerate/rescan a matching device.

> 
> The mgmtPF is a util driver which is responsible for communicating with the
> mgmtPF PCIe bar resources.
> 
> 
> > BTW, AFAICS the expected flow is easier to implement for of-fpga-region,
> > but harder for PCI devices. But I think that's the right direction and
> > should try to work it out.
> 
> Could I recap the suggested design if I understand that correctly...
> 
> You are thinking that the mgmtPF (aka. versal-pci) driver should have a uAPI

This should be the unified fpga-region class uAPI.

> to trigger the FPGA re-programing; and using Nava's callback ops to detach
> the separate userPF driver; after re-programing is done, re-attch the userPF

No need to detach a specific driver, remove all devices in the
fpga-region. I imagine this could also be a generic flow for all PCI/e
based FPGA cards.

Thanks,
Yilun

> driver and allow the userPF driver re-enumerate all to match the new
> hardware.
> 
> I think my understanding is correct, it is doable.
> 
> As long as we can keep our userPF driver as separate driver, the code change
> won't be too big.
> 
> > 
> > Thanks,
> > Yilun
Yidong Zhang Feb. 6, 2025, 4:31 a.m. UTC | #9
On 2/5/25 20:15, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Sun, Jan 26, 2025 at 11:46:54AM -0800, Yidong Zhang wrote:
>>
>>
>> On 1/26/25 02:32, Xu Yilun wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Tue, Dec 10, 2024 at 10:37:30AM -0800, Yidong Zhang wrote:
>>>> AMD Versal based PCIe card, including V70, is designed for AI inference
>>>> efficiency and is tuned for video analytics and natural language processing
>>>> applications.
>>>>
>>>> The driver architecture:
>>>>
>>>>     +---------+  Communication +---------+  Remote  +-----+------+
>>>>     |         |  Channel       |         |  Queue   |     |      |
>>>>     | User PF | <============> | Mgmt PF | <=======>| FW  | FPGA |
>>>>     +---------+                +---------+          +-----+------+
>>>>       PL Data                    base FW
>>>>                                  APU FW
>>>>                                  PL Data (copy)
>>>>    - PL (FPGA Program Logic)
>>>>    - FW (Firmware)
>>>>
>>>> There are 2 separate drivers from the original XRT[1] design.
>>>>    - UserPF driver
>>>>    - MgmtPF driver
>>>>
>>>> The new AMD versal-pci driver will replace the MgmtPF driver for Versal
>>>> PCIe card.
>>>>
>>>> The XRT[1] is already open-sourced. It includes solution of runtime for
>>>> many different type of PCIe Based cards. It also provides utilities for
>>>> managing and programming the devices.
>>>>
>>>> The AMD versal-pci stands for AMD Versal brand PCIe device management
>>>> driver. This driver provides the following functionalities:
>>>>
>>>>      - module and PCI device initialization
>>>>        this driver will attach to specific device id of V70 card;
>>>>        the driver will initialize itself based on bar resources for
>>>>        - communication channel:
>>>>          a hardware message service between mgmt PF and user PF
>>>>        - remote queue:
>>>>          a hardware queue based ring buffer service between mgmt PF and PCIe
>>>>          hardware firmware for programming FPGA Program Logic, loading
>>>>          firmware and checking card healthy status.
>>>>
>>>>      - programming FW
>>>>        - The base FW is downloaded onto the flash of the card.
>>>>        - The APU FW is downloaded once after a POR (power on reset).
>>>>        - Reloading the MgmtPF driver will not change any existing hardware.
>>>>
>>>>      - programming FPGA hardware binaries - PL Data
>>>>       - using fpga framework ops to support re-programing FPGA
>>>>       - the re-programming request will be initiated from the existing UserPF
>>>>         driver only, and the MgmtPF driver load the matched PL Data after
>>>>         receiving request from the communication channel. The matching PL
>>>
>>> I think this is not the way the FPGA generic framework should do. A FPGA
>>> region user (your userPF driver) should not also be the reprogram requester.
>>> The user driver cannot deal with the unexpected HW change if it happens.
>>> Maybe after reprogramming, the user driver cannot match the device
>>> anymore, and if user driver is still working on it, crash.
>>
>> One thing to clarify. The current design is:
>>
>> The userPF driver is the only requester. The mgmtPF has no uAPI to reprogram
>> the FPGA.
>>
>>
>>>
>>> The expected behavior is, the FPGA region removes user devices (thus
>>> detaches user drivers), does reprogramming, re-enumerates/rescans and
>>> matches new devices with new drivers. And I think that's what Nava is
>>> working on.
>>>
>>
>> Nava's work is different than our current design, our current design is:
>>
>> the separate userPF driver will detach all services before requesting to the
>> mgmtPF to program the FPGA, and after the programming is done, the userPF
>> will re-enumerate/rescan the matching new devices.
> 
> That's not align with the Device-Driver Model, A device driver should not
> re-enumerate/rescan a matching device.

Thanks! Yilun.

I will need to discuss this in my team. Our current userPF driver 
organize services (rom, sensor, msix, etc.) as platform driver and 
re-enumerate (online/offline) when there is a hardware change.


> 
>>
>> The mgmtPF is a util driver which is responsible for communicating with the
>> mgmtPF PCIe bar resources.
>>
>>
>>> BTW, AFAICS the expected flow is easier to implement for of-fpga-region,
>>> but harder for PCI devices. But I think that's the right direction and
>>> should try to work it out.
>>
>> Could I recap the suggested design if I understand that correctly...
>>
>> You are thinking that the mgmtPF (aka. versal-pci) driver should have a uAPI
> 
> This should be the unified fpga-region class uAPI.
> 
>> to trigger the FPGA re-programing; and using Nava's callback ops to detach
>> the separate userPF driver; after re-programing is done, re-attch the userPF
> 
> No need to detach a specific driver, remove all devices in the
> fpga-region. I imagine this could also be a generic flow for all PCI/e
> based FPGA cards.

I see your point. Is there an existing example in current fpga driver 
for PCI/e based cards?

We will need to talk to our management team and re-design our driver.
I think we have 2 approaches:
1) Align with linux fpga, having one driver for both userPF and mgmtPF; or
2) find a different location (maybe driver/misc) for the version-pci 
driver, because it is an utility driver, not need to be tied with fpga 
framework.

Please let me know you thoughts. Which way is acceptable by you.

Thanks,
David
> 
> Thanks,
> Yilun
> 
>> driver and allow the userPF driver re-enumerate all to match the new
>> hardware.
>>
>> I think my understanding is correct, it is doable.
>>
>> As long as we can keep our userPF driver as separate driver, the code change
>> won't be too big.
>>
>>>
>>> Thanks,
>>> Yilun
Xu Yilun Feb. 7, 2025, 2:19 a.m. UTC | #10
> > No need to detach a specific driver, remove all devices in the
> > fpga-region. I imagine this could also be a generic flow for all PCI/e
> > based FPGA cards.
> 
> I see your point. Is there an existing example in current fpga driver for
> PCI/e based cards?

No. The fpga-region re-enumeration arch is still WIP, so no existing
implementation.

> 
> We will need to talk to our management team and re-design our driver.
> I think we have 2 approaches:
> 1) Align with linux fpga, having one driver for both userPF and mgmtPF; or

Don't get you. Linux FPGA doesn't require one driver for both PFs.

> 2) find a different location (maybe driver/misc) for the version-pci driver,
> because it is an utility driver, not need to be tied with fpga framework.

I'm not the misc maintainer, but I assume in-tree utility driver +
out-of-tree client driver is not generally welcomed.

Thanks,
Yilun

> 
> Please let me know you thoughts. Which way is acceptable by you.
> 
> Thanks,
> David
> > 
> > Thanks,
> > Yilun
> > 
> > > driver and allow the userPF driver re-enumerate all to match the new
> > > hardware.
> > > 
> > > I think my understanding is correct, it is doable.
> > > 
> > > As long as we can keep our userPF driver as separate driver, the code change
> > > won't be too big.
> > > 
> > > > 
> > > > Thanks,
> > > > Yilun
Yidong Zhang Feb. 7, 2025, 3:16 a.m. UTC | #11
On 2/6/25 18:19, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>>> No need to detach a specific driver, remove all devices in the
>>> fpga-region. I imagine this could also be a generic flow for all PCI/e
>>> based FPGA cards.
>>
>> I see your point. Is there an existing example in current fpga driver for
>> PCI/e based cards?
> 
> No. The fpga-region re-enumeration arch is still WIP, so no existing
> implementation.

Got you. I can also help to test or provide feedback.
Actually, I had sent Nava my protype of using configfs for the non-OF 
driver. He should have the updated patch soon.

> 
>>
>> We will need to talk to our management team and re-design our driver.
>> I think we have 2 approaches:
>> 1) Align with linux fpga, having one driver for both userPF and mgmtPF; or
> 
> Don't get you. Linux FPGA doesn't require one driver for both PFs.

I assume when you said "generic flow for removing all devices in 
fpga-region" means that there is a single driver which use the 
fpga-region callbacks to remove all devices and then re-progream the FPGA.

On AMD V70 hardware, the userPF has different deviceid than the mgmtPF.
Thus, our current design is having 2 separate pcie drivers for each 
different deviceid.

Thus, in our current design, the fpga-region should be in the userPF 
driver which has callbacks to remove all devices. But in mgmtPF, it is 
more like a utility which only handles request from the userPF but it 
has the management privilege. Usually, cloud vendors require the mgmtPF 
deployed in their secured domain (can be a separate physical machine).

We can keep 2 drivers, one for userPF, one for mgmtPF. The userPF and 
mgmtPF communicate each other via the communication channel because they 
can be physically on different machine.

Are you looking for us to upstream both of them together?
If yes, I still think that the fpga-region should be in the userPF 
driver. The mgmtPF driver is still a utility driver.

Please correct me if I am wrong.:)

> 
>> 2) find a different location (maybe driver/misc) for the version-pci driver,
>> because it is an utility driver, not need to be tied with fpga framework.
> 
> I'm not the misc maintainer, but I assume in-tree utility driver +
> out-of-tree client driver is not generally welcomed.

Great info! Thanks! I will have to discuss this with my team too.

My understanding, so far based on your comments above, the drivers/fpga 
prefer to use fpga-region ops to handle FPGA re-program changes.

The current versal-pci driver is a utility driver.
The fpga-region should be within the userPF driver.

We can try to make our userPF driver in-tree as well. But the current 
plan is to do it later.

If you prefer we do both of them together. I can talk to my team.

Thanks,
David
> 
> Thanks,
> Yilun
> 
>>
>> Please let me know you thoughts. Which way is acceptable by you.
>>
>> Thanks,
>> David
>>>
>>> Thanks,
>>> Yilun
>>>
>>>> driver and allow the userPF driver re-enumerate all to match the new
>>>> hardware.
>>>>
>>>> I think my understanding is correct, it is doable.
>>>>
>>>> As long as we can keep our userPF driver as separate driver, the code change
>>>> won't be too big.
>>>>
>>>>>
>>>>> Thanks,
>>>>> Yilun
Xu Yilun Feb. 7, 2025, 4:40 a.m. UTC | #12
On Thu, Feb 06, 2025 at 07:16:25PM -0800, Yidong Zhang wrote:
> 
> 
> On 2/6/25 18:19, Xu Yilun wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > > > No need to detach a specific driver, remove all devices in the
> > > > fpga-region. I imagine this could also be a generic flow for all PCI/e
> > > > based FPGA cards.
> > > 
> > > I see your point. Is there an existing example in current fpga driver for
> > > PCI/e based cards?
> > 
> > No. The fpga-region re-enumeration arch is still WIP, so no existing
> > implementation.
> 
> Got you. I can also help to test or provide feedback.
> Actually, I had sent Nava my protype of using configfs for the non-OF
> driver. He should have the updated patch soon.
> 
> > 
> > > 
> > > We will need to talk to our management team and re-design our driver.
> > > I think we have 2 approaches:
> > > 1) Align with linux fpga, having one driver for both userPF and mgmtPF; or
> > 
> > Don't get you. Linux FPGA doesn't require one driver for both PFs.
> 
> I assume when you said "generic flow for removing all devices in
> fpga-region" means that there is a single driver which use the fpga-region
> callbacks to remove all devices and then re-progream the FPGA.
> 
> On AMD V70 hardware, the userPF has different deviceid than the mgmtPF.
> Thus, our current design is having 2 separate pcie drivers for each
> different deviceid.
> 
> Thus, in our current design, the fpga-region should be in the userPF driver
> which has callbacks to remove all devices. But in mgmtPF, it is more like a

A question, if the FPGA logic is cleared, does the userPF still exist on
PCIe bus?

> utility which only handles request from the userPF but it has the management
> privilege. Usually, cloud vendors require the mgmtPF deployed in their
> secured domain (can be a separate physical machine).

I though mgmtPF & userPF are just 2 functions on one PCIe card, they are not?
Then how the mgmtPF writes data to another physical machine and
influence the userPF?

Thanks,
Yilun

> 
> We can keep 2 drivers, one for userPF, one for mgmtPF. The userPF and mgmtPF
> communicate each other via the communication channel because they can be
> physically on different machine.
> 
> Are you looking for us to upstream both of them together?
> If yes, I still think that the fpga-region should be in the userPF driver.
> The mgmtPF driver is still a utility driver.
> 
> Please correct me if I am wrong.:)
> 
> > 
> > > 2) find a different location (maybe driver/misc) for the version-pci driver,
> > > because it is an utility driver, not need to be tied with fpga framework.
> > 
> > I'm not the misc maintainer, but I assume in-tree utility driver +
> > out-of-tree client driver is not generally welcomed.
> 
> Great info! Thanks! I will have to discuss this with my team too.
> 
> My understanding, so far based on your comments above, the drivers/fpga
> prefer to use fpga-region ops to handle FPGA re-program changes.
> 
> The current versal-pci driver is a utility driver.
> The fpga-region should be within the userPF driver.
> 
> We can try to make our userPF driver in-tree as well. But the current plan
> is to do it later.
> 
> If you prefer we do both of them together. I can talk to my team.
> 
> Thanks,
> David
> > 
> > Thanks,
> > Yilun
> > 
> > > 
> > > Please let me know you thoughts. Which way is acceptable by you.
> > > 
> > > Thanks,
> > > David
> > > > 
> > > > Thanks,
> > > > Yilun
> > > > 
> > > > > driver and allow the userPF driver re-enumerate all to match the new
> > > > > hardware.
> > > > > 
> > > > > I think my understanding is correct, it is doable.
> > > > > 
> > > > > As long as we can keep our userPF driver as separate driver, the code change
> > > > > won't be too big.
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > Yilun
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 17daa9ee9384..302c10004c5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1194,6 +1194,12 @@  L:	linux-spi@vger.kernel.org
 S:	Supported
 F:	drivers/spi/spi-amd.c
 
+AMD VERSAL PCI DRIVER
+M:	Yidong Zhang <yidong.zhang@amd.com>
+L:	linux-fpga@vger.kernel.org
+S:	Supported
+F:	drivers/fpga/amd/
+
 AMD XGBE DRIVER
 M:	"Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
 L:	netdev@vger.kernel.org
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 37b35f58f0df..dce060a7bd8f 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -290,4 +290,7 @@  config FPGA_MGR_LATTICE_SYSCONFIG_SPI
 
 source "drivers/fpga/tests/Kconfig"
 
+# Driver files
+source "drivers/fpga/amd/Kconfig"
+
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index aeb89bb13517..8412f3e211cc 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -58,5 +58,8 @@  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
 # Drivers for FPGAs which implement DFL
 obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
 
+# AMD PCIe Versal Management Driver
+obj-$(CONFIG_AMD_VERSAL_PCI)		+= amd/
+
 # KUnit tests
 obj-$(CONFIG_FPGA_KUNIT_TESTS)		+= tests/
diff --git a/drivers/fpga/amd/Kconfig b/drivers/fpga/amd/Kconfig
new file mode 100644
index 000000000000..b18a42a340ba
--- /dev/null
+++ b/drivers/fpga/amd/Kconfig
@@ -0,0 +1,15 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+
+config AMD_VERSAL_PCI
+	tristate "AMD Versal PCIe Management Driver"
+	select FW_LOADER
+	select FW_UPLOAD
+	depends on FPGA
+	depends on HAS_IOMEM
+	depends on PCI
+	help
+	  AMD Versal PCIe Management Driver provides management services to
+	  download firmware, program bitstream, and communicate with the User
+	  function.
+
+	  If "M" is selected, the driver module will be versal-pci
diff --git a/drivers/fpga/amd/Makefile b/drivers/fpga/amd/Makefile
new file mode 100644
index 000000000000..5d1ef04b5e80
--- /dev/null
+++ b/drivers/fpga/amd/Makefile
@@ -0,0 +1,5 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_AMD_VERSAL_PCI)			+= versal-pci.o
+
+versal-pci-$(CONFIG_AMD_VERSAL_PCI)		:= versal-pci-main.o
diff --git a/drivers/fpga/amd/versal-pci-main.c b/drivers/fpga/amd/versal-pci-main.c
new file mode 100644
index 000000000000..a10ccf86802b
--- /dev/null
+++ b/drivers/fpga/amd/versal-pci-main.c
@@ -0,0 +1,328 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Versal PCIe device
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
+ */
+
+#include <linux/pci.h>
+
+#include "versal-pci.h"
+
+#define DRV_NAME			"amd-versal-pci"
+
+#define PCI_DEVICE_ID_V70PQ2		0x50B0
+#define VERSAL_XCLBIN_MAGIC_ID		"xclbin2"
+
+static int versal_pci_fpga_write_init(struct fpga_manager *mgr, struct fpga_image_info *info,
+				      const char *buf, size_t count)
+{
+	/* TODO */
+	return 0;
+}
+
+static int versal_pci_fpga_write(struct fpga_manager *mgr, const char *buf,
+				 size_t count)
+{
+	/* TODO */
+	return 0;
+}
+
+static int versal_pci_fpga_write_complete(struct fpga_manager *mgr,
+					  struct fpga_image_info *info)
+{
+	/* TODO */
+	return 0;
+}
+
+static enum fpga_mgr_states versal_pci_fpga_state(struct fpga_manager *mgr)
+{
+	struct fpga_device *fdev = mgr->priv;
+
+	return fdev->state;
+}
+
+static const struct fpga_manager_ops versal_pci_fpga_ops = {
+	.write_init = versal_pci_fpga_write_init,
+	.write = versal_pci_fpga_write,
+	.write_complete = versal_pci_fpga_write_complete,
+	.state = versal_pci_fpga_state,
+};
+
+static void versal_pci_fpga_fini(struct fpga_device *fdev)
+{
+	fpga_mgr_unregister(fdev->mgr);
+}
+
+static void versal_pci_uuid_parse(struct versal_pci_device *vdev, uuid_t *uuid)
+{
+	char str[UUID_STRING_LEN];
+	u8 i, j;
+
+	/* parse uuid into a valid uuid string format */
+	for (i  = 0, j = 0; i < strlen(vdev->fw_id) && i < sizeof(str); i++) {
+		str[j++] = vdev->fw_id[i];
+		if (j == 8 || j == 13 || j == 18 || j == 23)
+			str[j++] = '-';
+	}
+
+	uuid_parse(str, uuid);
+	vdev_info(vdev, "Interface uuid %pU", uuid);
+}
+
+static struct fpga_device *versal_pci_fpga_init(struct versal_pci_device *vdev)
+{
+	struct device *dev = &vdev->pdev->dev;
+	struct fpga_manager_info info = { 0 };
+	struct fpga_device *fdev;
+	int ret;
+
+	fdev = devm_kzalloc(dev, sizeof(*fdev), GFP_KERNEL);
+	if (!fdev)
+		return ERR_PTR(-ENOMEM);
+
+	fdev->vdev = vdev;
+
+	info = (struct fpga_manager_info) {
+		.name = "AMD Versal FPGA Manager",
+		.mops = &versal_pci_fpga_ops,
+		.priv = fdev,
+	};
+
+	fdev->mgr = fpga_mgr_register_full(dev, &info);
+	if (IS_ERR(fdev->mgr)) {
+		ret = PTR_ERR(fdev->mgr);
+		vdev_err(vdev, "Failed to register FPGA manager, err %d", ret);
+		return ERR_PTR(ret);
+	}
+
+	/* Place holder for rm_queue_get_fw_id(vdev->rdev) */
+	versal_pci_uuid_parse(vdev, &vdev->intf_uuid);
+
+	return fdev;
+}
+
+static int versal_pci_program_axlf(struct versal_pci_device *vdev, char *data, size_t size)
+{
+	const struct axlf *axlf = (struct axlf *)data;
+	struct fpga_image_info *image_info;
+	int ret;
+
+	image_info = fpga_image_info_alloc(&vdev->pdev->dev);
+	if (!image_info)
+		return -ENOMEM;
+
+	image_info->count = axlf->header.length;
+	image_info->buf = (char *)axlf;
+
+	ret = fpga_mgr_load(vdev->fdev->mgr, image_info);
+	if (ret) {
+		vdev_err(vdev, "failed to load xclbin: %d", ret);
+		goto exit;
+	}
+
+	vdev_info(vdev, "Downloaded axlf %pUb of size %zu Bytes", &axlf->header.uuid, size);
+	uuid_copy(&vdev->xclbin_uuid, &axlf->header.uuid);
+
+exit:
+	fpga_image_info_free(image_info);
+
+	return ret;
+}
+
+int versal_pci_load_xclbin(struct versal_pci_device *vdev, uuid_t *xuuid)
+{
+	const char *xclbin_location = "xilinx/xclbins";
+	char fw_name[100];
+	const struct firmware *fw;
+	int ret;
+
+	snprintf(fw_name, sizeof(fw_name), "%s/%pUb_%s.xclbin",
+		 xclbin_location, xuuid, vdev->fw_id);
+
+	vdev_info(vdev, "trying to load %s", fw_name);
+	ret = request_firmware(&fw, fw_name, &vdev->pdev->dev);
+	if (ret) {
+		vdev_warn(vdev, "request xclbin fw %s failed %d", fw_name, ret);
+		return ret;
+	}
+	vdev_info(vdev, "loaded data size %zu", fw->size);
+
+	ret = versal_pci_program_axlf(vdev, (char *)fw->data, fw->size);
+	if (ret)
+		vdev_err(vdev, "program axlf %s failed %d", fw_name, ret);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+static enum fw_upload_err versal_pci_fw_prepare(struct fw_upload *fw_upload, const u8 *data,
+						u32 size)
+{
+	/* TODO */
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err versal_pci_fw_write(struct fw_upload *fw_upload, const u8 *data,
+					      u32 offset, u32 size, u32 *written)
+{
+	/* TODO */
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err versal_pci_fw_poll_complete(struct fw_upload *fw_upload)
+{
+	/* TODO */
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void versal_pci_fw_cancel(struct fw_upload *fw_upload)
+{
+	/* TODO */
+}
+
+static void versal_pci_fw_cleanup(struct fw_upload *fw_upload)
+{
+	/* TODO */
+}
+
+static const struct fw_upload_ops versal_pci_fw_ops = {
+	.prepare = versal_pci_fw_prepare,
+	.write = versal_pci_fw_write,
+	.poll_complete = versal_pci_fw_poll_complete,
+	.cancel = versal_pci_fw_cancel,
+	.cleanup = versal_pci_fw_cleanup,
+};
+
+static void versal_pci_fw_upload_fini(struct firmware_device *fwdev)
+{
+	firmware_upload_unregister(fwdev->fw);
+	kfree(fwdev->name);
+}
+
+static u32 versal_pci_devid(struct versal_pci_device *vdev)
+{
+	return ((pci_domain_nr(vdev->pdev->bus) << 16) |
+		PCI_DEVID(vdev->pdev->bus->number, vdev->pdev->devfn));
+}
+
+static struct firmware_device *versal_pci_fw_upload_init(struct versal_pci_device *vdev)
+{
+	struct device *dev = &vdev->pdev->dev;
+	struct firmware_device *fwdev;
+	u32 devid;
+
+	fwdev = devm_kzalloc(dev, sizeof(*fwdev), GFP_KERNEL);
+	if (!fwdev)
+		return ERR_PTR(-ENOMEM);
+
+	devid = versal_pci_devid(vdev);
+	fwdev->name = kasprintf(GFP_KERNEL, "%s%x", DRV_NAME, devid);
+	if (!fwdev->name)
+		return ERR_PTR(-ENOMEM);
+
+	fwdev->fw = firmware_upload_register(THIS_MODULE, dev, fwdev->name,
+					     &versal_pci_fw_ops, fwdev);
+	if (IS_ERR(fwdev->fw)) {
+		kfree(fwdev->name);
+		return ERR_CAST(fwdev->fw);
+	}
+
+	fwdev->vdev = vdev;
+
+	return fwdev;
+}
+
+static void versal_pci_device_teardown(struct versal_pci_device *vdev)
+{
+	versal_pci_fpga_fini(vdev->fdev);
+	versal_pci_fw_upload_fini(vdev->fwdev);
+}
+
+static int versal_pci_device_setup(struct versal_pci_device *vdev)
+{
+	int ret;
+
+	vdev->fwdev = versal_pci_fw_upload_init(vdev);
+	if (IS_ERR(vdev->fwdev)) {
+		ret = PTR_ERR(vdev->fwdev);
+		vdev_err(vdev, "Failed to init FW uploader, err %d", ret);
+		return ret;
+	}
+
+	vdev->fdev = versal_pci_fpga_init(vdev);
+	if (IS_ERR(vdev->fdev)) {
+		ret = PTR_ERR(vdev->fdev);
+		vdev_err(vdev, "Failed to init FPGA manager, err %d", ret);
+		goto upload_fini;
+	}
+
+	return 0;
+
+upload_fini:
+	versal_pci_fw_upload_fini(vdev->fwdev);
+
+	return ret;
+}
+
+static void versal_pci_remove(struct pci_dev *pdev)
+{
+	struct versal_pci_device *vdev = pci_get_drvdata(pdev);
+
+	versal_pci_device_teardown(vdev);
+}
+
+static int versal_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pdev_id)
+{
+	struct versal_pci_device *vdev;
+	int ret;
+
+	vdev = devm_kzalloc(&pdev->dev, sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	pci_set_drvdata(pdev, vdev);
+	vdev->pdev = pdev;
+
+	ret = pcim_enable_device(pdev);
+	if (ret) {
+		vdev_err(vdev, "Failed to enable device %d", ret);
+		return ret;
+	}
+
+	vdev->io_regs = pcim_iomap_region(vdev->pdev, MGMT_BAR, DRV_NAME);
+	if (IS_ERR(vdev->io_regs)) {
+		vdev_err(vdev, "Failed to map RM shared memory BAR%d", MGMT_BAR);
+		return PTR_ERR(vdev->io_regs);
+	}
+
+	ret = versal_pci_device_setup(vdev);
+	if (ret) {
+		vdev_err(vdev, "Failed to setup Versal device %d", ret);
+		return ret;
+	}
+
+	vdev_dbg(vdev, "Successfully probed %s driver!", DRV_NAME);
+	return 0;
+}
+
+static const struct pci_device_id versal_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_V70PQ2), },
+	{ 0 }
+};
+
+MODULE_DEVICE_TABLE(pci, versal_pci_ids);
+
+static struct pci_driver versal_pci_driver = {
+	.name = DRV_NAME,
+	.id_table = versal_pci_ids,
+	.probe = versal_pci_probe,
+	.remove = versal_pci_remove,
+};
+
+module_pci_driver(versal_pci_driver);
+
+MODULE_DESCRIPTION("AMD Versal PCIe Management Driver");
+MODULE_AUTHOR("XRT Team <runtimeca39d@amd.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/fpga/amd/versal-pci.h b/drivers/fpga/amd/versal-pci.h
new file mode 100644
index 000000000000..1509bd0532ea
--- /dev/null
+++ b/drivers/fpga/amd/versal-pci.h
@@ -0,0 +1,86 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Driver for Versal PCIe device
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
+ */
+
+#ifndef __VERSAL_PCI_H
+#define __VERSAL_PCI_H
+
+#include <linux/firmware.h>
+#include <linux/fpga/fpga-mgr.h>
+
+#define MGMT_BAR		0
+
+#define vdev_info(vdev, fmt, args...)					\
+	dev_info(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
+
+#define vdev_warn(vdev, fmt, args...)					\
+	dev_warn(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
+
+#define vdev_err(vdev, fmt, args...)					\
+	dev_err(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
+
+#define vdev_dbg(vdev, fmt, args...)					\
+	dev_dbg(&(vdev)->pdev->dev, fmt, ##args)
+
+struct versal_pci_device;
+
+struct axlf_header {
+	__u64				length;
+	__u8				reserved1[24];
+	uuid_t				rom_uuid;
+	__u8				reserved2[64];
+	uuid_t				uuid;
+	__u8				reserved3[24];
+} __packed;
+
+struct axlf {
+	__u8				magic[8];
+	__u8				reserved[296];
+	struct axlf_header		header;
+} __packed;
+
+struct fw_tnx {
+	struct rm_cmd			*cmd;
+	__u32				opcode;
+	__u32				id;
+};
+
+struct fpga_device {
+	enum fpga_mgr_states		state;
+	struct fpga_manager		*mgr;
+	struct versal_pci_device	*vdev;
+	struct fw_tnx			fw;
+};
+
+struct firmware_device {
+	struct versal_pci_device	*vdev;
+	struct fw_upload		*fw;
+	__u8				*name;
+	__u32				fw_name_id;
+	struct rm_cmd			*cmd;
+	__u32				id;
+	uuid_t				uuid;
+};
+
+struct versal_pci_device {
+	struct pci_dev			*pdev;
+
+	struct fpga_device		*fdev;
+	struct firmware_device		*fwdev;
+	struct device			*device;
+
+	void __iomem			*io_regs;
+	uuid_t				xclbin_uuid;
+	uuid_t				intf_uuid;
+	__u8				fw_id[UUID_STRING_LEN + 1];
+
+	__u8				*debugfs_root;
+};
+
+/* versal pci driver APIs */
+int versal_pci_load_xclbin(struct versal_pci_device *vdev, uuid_t *xclbin_uuid);
+
+#endif	/* __VERSAL_PCI_H */