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
