Message-ID: <7b9bd24f-8f89-4d6c-a079-47c4c0b88a35@amd.com>
Date: Thu, 6 Feb 2025 19:16:25 -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>, Hayden Laccabue
	<hayden.laccabue@....com>
Subject: Re: [PATCH V2 1/4] drivers/fpga/amd: Add new driver amd versal-pci



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

Powered by blists - more mailing lists