Re: [PATCH V1 2/5] accel/amd_vpci: Add new driver for AMD Versal PCI accelerator

[Yidong Zhang <yidong.zhang@amd.com>]

Hi Chris,

On 11/11/25 01:38, Christophe JAILLET wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
> 
> 
> Le 11/11/2025 à 02:15, David Zhang a écrit :
>> This patch introduces a new PCI driver for AMD Versal-based accelerator
>> cards.
>>
>> The driver provides basic module and PCI device initialization, based on
>> BAR resources used to establish a hardware queue-based ring buffer 
>> between
>> the PCIe host and the Versal Management Runtime (VMR) service running on
>> the embedded SoC. This interface enables firmware management and board
>> health monitoring.
>>
>> Key features:
>>     - PCI probe and BAR resource initialization.
>>     - Integration with configfs for firmware management
>>     - Compatibility check using firmware-reported UUIDs
>>
>> The base firmware image is expected under /lib/firmware/xilinx/<fw_name>
>> and can be programmed to the device through the configfs interface.
>> Firmware transfer is handled via a remote queue service (added in a later
>> patch).
>>
>> 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>
>> Signed-off-by: David Zhang <yidong.zhang@amd.com>
>> ---
> 
> ...
> 
>> +static int versal_pci_device_setup(struct versal_pci_device *vdev)
>> +{
>> +     int ret;
>> +
>> +     ret = versal_pci_fw_init(vdev);
>> +     if (ret) {
>> +             vdev_err(vdev, "Failed to init fw, err %d", ret);
>> +             goto comm_chan_fini;
>> +     }
>> +
>> +     ret = versal_pci_cfs_init(vdev);
>> +     if (ret) {
> 
> Do we need to call versal_pci_fw_fini()?
> (here or in the error handling path to be future proof)
> 
>> +             vdev_err(vdev, "Failed to init configfs subsys, err %d", 
>> ret);
>> +             goto comm_chan_fini;
>> +     }
>> +
>> +     return 0;
>> +
>> +comm_chan_fini:
>> +
>> +     return ret;
>> +}
> 
> ...
> 
>> diff --git a/drivers/accel/amd_vpci/versal-pci.h 
>> b/drivers/accel/amd_vpci/versal-pci.h
>> new file mode 100644
>> index 000000000000..ca309aee87ad
>> --- /dev/null
>> +++ b/drivers/accel/amd_vpci/versal-pci.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Driver for Versal PCIe device
>> + *
>> + * Copyright (C) 2025 Advanced Micro Devices, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef __VERSAL_PCI_H
>> +#define __VERSAL_PCI_H
>> +
>> +#include <linux/configfs.h>
>> +#include <linux/firmware.h>
>> +
>> +#define MGMT_BAR             0
>> +
>> +#define vdev_info(vdev, fmt, 
>> args...)                                        \
>> +     dev_info(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
> 
> \n could be added after fmt, as it is not included in the messages
> themselves, when used.

I tested this with "\n" on latest linux kernel, it will give us one 
extra line. I think latest linux has no need to add this extra "\n" anymore.

Meanwhile, I will address all your other comments and also update the 
copyright to 2026.

Happy new year to everyone!

Thanks,
David

> 
> Same for the other macro below.
> 
>> +
>> +#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)
> 
> ...
> 
> CJ