[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d27e09ac-2bd2-46e3-8f92-dfbacb9cc436@amd.com>
Date: Tue, 19 Nov 2024 22:15:59 -0800
From: Yidong Zhang <yidong.zhang@....com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
CC: <linux-kernel@...r.kernel.org>, <linux-fpga@...r.kernel.org>,
<mdf@...nel.org>, <hao.wu@...el.com>, <yilun.xu@...el.com>,
<lizhi.hou@....com>, DMG Karthik <Karthik.DMG@....com>, Nishad Saraf
<nishads@....com>, Prapul Krishnamurthy <prapulk@....com>
Subject: Re: [PATCH V1 1/3] drivers/fpga/amd: Add new driver for AMD Versal
PCIe card
On 11/18/24 23:07, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
>>>> +obj-$(CONFIG_AMD_VERSAL_MGMT) += amd-vmgmt.o
>>>
>>> IMHO the naming vmgmt is hard to understand, any better idea?
>>
>> The "v" stand for Versal. We would change to amd-vpci for Versal based pcie
>
> "v" + "pci" is quite a misleading term, maybe just versal-pci?
Sound good, I will rename the driver to versal-pci.
>
>> devices.
>>
>>
>
> [...]
>
>>>
>>>> +{
>>>> + struct comms_device *ccdev;
>>>> +
>>>> + ccdev = devm_kzalloc(&vdev->pdev->dev, sizeof(*ccdev), GFP_KERNEL);
>>>> + if (!ccdev)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + ccdev->vdev = vdev;
>>>> +
>>>> + ccdev->regmap = devm_regmap_init_mmio(&vdev->pdev->dev,
>>>> + vdev->tbl + COMMS_PCI_BAR_OFF,
>>>> + &comms_regmap_config);
>>>
>>> I'm not sure why a regmap is needed. All register accessing is within
>>> the same module/file, and I assume a base+offset is enough to position
>>> the register addr.
>>
>> I thought the regmap is preferred. We can use some common APIs like
>> regmap_bulk_*. The base+offset works too, then we will implement our own
>> bulk_* functions. Please let me know.
>
> I didn't see any regmap_bulk_*. AFAICS regmap is not needed here.
The bulk_* is in the patch 0003. I can switch to base+offset in next
version of the patches.
>
> [...]
>
>>>> + /* create fgpa bridge, region for the base shell */
>>>> + fdev->bridge = fpga_bridge_register(dev, "AMD Versal FPGA Bridge",
>>>> + &vmgmt_br_ops, fdev);
>>>
>>> I didn't find the br_ops anywhere in this patchset. So how to gate the
>>> FPGA region when it is being reprogrammed? What is the physical link
>>> between the FPGA region and outside visitors?
>>
>> The FPGA region gate operation is done in the FW running in this PCIe card.
>> The FW will "freeze" the gate before programing the PL. After downloading
>> the new hardware. The FW will then "free" the gate.
>
> So no OS operation is needed, then seems no need the fpga_bridge object.
Thanks, I will simplify this code.
>
>>
>> No physical link between FPGA region and outside visitors, the FW handles
>> all requests.
>>
>>>
>>>> + if (IS_ERR(fdev->bridge)) {
>>>> + vmgmt_err(vdev, "Failed to register FPGA bridge, err %ld",
>>>> + PTR_ERR(fdev->bridge));
>>>> + ret = PTR_ERR(fdev->bridge);
>>>> + goto unregister_fpga_mgr;
>>>> + }
>>>> +
>>>> + region = (struct fpga_region_info) {
>>>> + .compat_id = (struct fpga_compat_id *)&vdev->intf_uuid,
>>>> + .get_bridges = vmgmt_get_bridges,
>>>> + .mgr = fdev->mgr,
>>>> + .priv = fdev,
>>>> + };
>>>> +
>>>> + fdev->region = fpga_region_register_full(dev, ®ion);
>>>
>>> I assume the fpga region represents the user PF, is it? If you
>>> reprogram the FPGA region, how does the user PF driver aware the HW is
>>> changing?
>>
>> The HW changing request is always requested from the user PF driver. The
>
> I don't understand. In your patch the FPGA reprograming is triggered by
> an IOCTL, usually a userspace application calls it. But here says it is
> triggered by the user PF *driver*, which IIUC is a kernel driver.
> Anything I missed?
>
I will remove the IOCTL in the patch because this IOCTL is internal test
only. The official request should always come from the user PF to avoid
hardware crash due to hardware changes.
The userPF flow is described below.
>> user PF driver will make sure it is safe to change hardware. Then, the user
>> PF driver notify the mgmt PF driver by a unique identify of the HW bitstream
>> (PL Data).
>>
>> The mgmt PF driver, the amd-vpci driver, will check the unique identify and
>> then find the same PL Data from its local storage which is previously
>> installed, and start downloading it.
>
> Is the flow included in this patchset? Please elaborate more.
>
The userFP driver will send request to the mgmt PF driver (versal-pci).
The versal-pci driver will then find the same PL Data from its local
storage (e.g. /lib/xilinx/fw_uuid/xclbin_uuid.xclbin).
The versal-pci driver will then read the firmware and download it.
I will port more code in next patch. The 1st patch did not include
everything. I can use a single/separate patch for this specific flow so
that it would be easy for you to review.
> [...]
>
>>>> +/**
>>>> + * VERSAL_MGMT_LOAD_XCLBIN_IOCTL - Download XCLBIN to the device
>>>> + *
>>>> + * This IOCTL is used to download XCLBIN down to the device.
>>>> + * Return: 0 on success, -errno on failure.
>>>> + */
>>>> +#define VERSAL_MGMT_LOAD_XCLBIN_IOCTL _IOW(VERSAL_MGMT_MAGIC, \
>>>> + VERSAL_MGMT_BASE + 0, void *)
>>>
>>> Many definitions are added in a batch but some are not used in this
>>> patch. Please reorganize the patches for easer review, even for first
>>> version.
>>>
>>> Thanks,
>>> Yilun
>>
>> Hi Yilun,
>>
>> Thanks for taking your time, and yes for sure I will make each patch more
>> self-contained.
>>
>> Here is my thoughts on upcoming patches structure:
>> 1st patch, adding driver probe and FPGA framework; the actual ops
>
> Just adding driver probe for 1st patch please.
>
Sure.
Thank,
David
> Thanks,
> Yilun
>
>> of handling communication channel message and remote queue message
>> will present as no-op with comments.
>>
>> 2nd patch, adding the communication channel services
>> 3rd patch, adding the remote queue services
>> 4th patch, adding the callers of using the remote queue services
>>
>> Thanks,
>> David
>>
>>>
>>>> +
>>>> +#endif /* _UAPI_LINUX_VMGMT_H */
>>>> --
>>>> 2.34.1
>>>>
>>>>
Powered by blists - more mailing lists