Re: [PATCH V1 1/3] drivers/fpga/amd: Add new driver for AMD Versal PCIe card

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

On 3/12/23 14:30, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Wed, Dec 25, 2024 at 10:10:06PM -0800, Yidong Zhang wrote:
>>
>>
>> On 3/12/23 11:03, Xu Yilun wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Sun, Dec 22, 2024 at 05:53:30PM -0800, Yidong Zhang wrote:
>>>>
>>>>
>>>> On 11/18/24 23:07, Xu Yilun wrote:
>>>>>
>>>>>>>> +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?
>>>>
>>>> Hi Yilun,
>>>>
>>>> I sent the V2 patch and refactored the driver as versal-pci now.
>>>> One more thing that I kept in V2 was the firmware_upload. I forgot to
>>>> mention that I'd love to switch to the newly proposed interface once
>>>> it is ready. I saw the proposal was now as config_fs and it was not merged
>>>
>>> Good to know that.
>>>
>>> I didn't start reviewing the v2 yet. But one thing is that now the
>>> versal-pci FPGA manager has no user because of the ongoing uAPI, so
>>> cannot be merged, and I won't pay much effort on this series for now.
>>
>> Hi Yilun,
>>
>> Can we add this as TODO in the future when the uAPI solution is ready to
>> switch? We spent some time to refactor the driver and address most of your
>

Hi Yilun,

Happy New Year!

> Sorry, generally kernel won't merge code that has no user, which means
> the code are not tested.

Just confirm "the code are not tested".

We use XRT opensource userPF driver (called xocl.ko) to test the 
versal-pci driver. You can find the source code here:
https://github.com/xdavidz/XRT1/commits/dtb

We are planing to let user use this mgmtPF driver + our open-sourced 
userPF driver + our library as full stack. I will provide full test
result if needed.

We are also working on some linux drm driver. We use the similar way
to prove that the drive has been tested.

Or, you are referring the "user" as user PF driver?
We do have user PF driver as a separate driver.

Please confirm if you are thinking that only userPF + mgmtPF as one 
driver? We architect our driver to 2 drivers due to cloud vendor 
specific requirement.

> 
>> comments in the V2. Hopefully, can you please start reviewing the fpga_mgr
>> and other driver code?
> 
> Yes, I can review the code when possible. But will not merge it.

Thanks. Please see questions above.

> 
>>
>> We'd think that we use the firmware_upload for 1st approach and start
>> letting user use this driver.
>>
>> We definitely will switch to the new uAPI as soon as it is ready in the
>> linux fpga driver. But we'd not like this uAPI holds up everything we
>> already spent times.
> 
> If you want speed up, then collaborate to develop/review the dependent
> patchset, that's how the community works. I actually don't want to be
> the only reviewer in this mail list, and others just send patches.

I'd like to help for the patchset. Let me ping the developer and see how
he would need help on this.

Thanks,
David

> 
> Thanks,
> Yilun