Message-ID:
 <DS7PR12MB6070C18273E3B9435A471045CD6F2@DS7PR12MB6070.namprd12.prod.outlook.com>
Date: Mon, 23 Sep 2024 04:28:01 +0000
From: "Manne, Nava kishore" <nava.kishore.manne@....com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
CC: "git (AMD-Xilinx)" <git@....com>, "mdf@...nel.org" <mdf@...nel.org>,
	"hao.wu@...el.com" <hao.wu@...el.com>, "yilun.xu@...el.com"
	<yilun.xu@...el.com>, "trix@...hat.com" <trix@...hat.com>, "robh@...nel.org"
	<robh@...nel.org>, "saravanak@...gle.com" <saravanak@...gle.com>,
	"linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA
 configuration

> -----Original Message-----
> From: Xu Yilun <yilun.xu@...ux.intel.com>
> Sent: Wednesday, September 18, 2024 1:50 PM
> To: Manne, Nava kishore <nava.kishore.manne@....com>
> Cc: git (AMD-Xilinx) <git@....com>; mdf@...nel.org; hao.wu@...el.com;
> yilun.xu@...el.com; trix@...hat.com; robh@...nel.org; saravanak@...gle.com;
> linux-fpga@...r.kernel.org; linux-kernel@...r.kernel.org;
> devicetree@...r.kernel.org
> Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support for FPGA
> configuration
> 
> On Tue, Sep 17, 2024 at 11:16:08AM +0000, Manne, Nava kishore wrote:
> > > -----Original Message-----
> > > From: Xu Yilun <yilun.xu@...ux.intel.com>
> > > Sent: Monday, August 5, 2024 11:51 PM
> > > To: Manne, Nava kishore <nava.kishore.manne@....com>
> > > Cc: git (AMD-Xilinx) <git@....com>; mdf@...nel.org;
> > > hao.wu@...el.com; yilun.xu@...el.com; trix@...hat.com;
> > > robh@...nel.org; saravanak@...gle.com; linux-fpga@...r.kernel.org;
> > > linux-kernel@...r.kernel.org; devicetree@...r.kernel.org
> > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface support
> > > for FPGA configuration
> > >
> > > On Thu, Aug 01, 2024 at 04:25:42AM +0000, Manne, Nava kishore wrote:
> > > > Hi Yilun,
> > > >
> > > > > -----Original Message-----
> > > > > From: Xu Yilun <yilun.xu@...ux.intel.com>
> > > > > Sent: Monday, July 29, 2024 9:27 PM
> > > > > To: Manne, Nava kishore <nava.kishore.manne@....com>
> > > > > Cc: git (AMD-Xilinx) <git@....com>; mdf@...nel.org;
> > > > > hao.wu@...el.com; yilun.xu@...el.com; trix@...hat.com;
> > > > > robh@...nel.org; saravanak@...gle.com;
> > > > > linux-fpga@...r.kernel.org;
> > > > > linux- kernel@...r.kernel.org; devicetree@...r.kernel.org
> > > > > Subject: Re: [RFC 1/1] of-fpga-region: Add sysfs interface
> > > > > support for FPGA configuration
> > > > >
> > > > > On Fri, Jul 26, 2024 at 12:08:19PM +0530, Nava kishore Manne wrote:
> > > > > > Adds sysfs interface as part of the of-fpga-region. This newly
> > > > > > added sysfs interface uses Device Tree Overlay (DTO) files to
> > > > > > configure/reprogram an FPGA while an operating system is
> > > > > > running.This solution will not change the existing sequence
> > > > > > When a DT overlay that targets an FPGA Region is applied.
> > > > > > 	- Disable appropriate FPGA bridges.
> > > > > > 	- Program the FPGA using the FPGA manager.
> > > > > > 	- Enable the FPGA bridges.
> > > > > > 	- The Device Tree overlay is accepted into the live tree.
> > > > > > 	- Child devices are populated.
> > > > > >
> > > > > > When the overlay is removed, the child nodes will be removed,
> > > > > > and the FPGA Region will disable the bridges.
> > > > > >
> > > > > > Usage:
> > > > > > To configure/reprogram an FPGA region:
> > > > > > echo "fpga.dtbo" > /sys/class/fpga_region/<region>/device/load
> > > > >
> > > > > IIRC, last time we are considering some generic interface for
> > > > > both OF & non- OF FPGA region, but this is still OF specific.
> > > > >
> > > > At AMD, we exclusively use OF for FPGA
> > > > configuration/reconfiguration, utilizing
> > > overlay files as outlined in the fpga-region.txt documentation.
> > > > However, some devices, like dfl.c those relying solely on the FPGA
> > > > region, do not
> > > use OF.
> > > > For these non-OF devices, should we expect them to follow the
> > > > fpga-region.txt
> > > guidelines for FPGA configuration/reconfiguration?
> > >
> > > I assume it is Documentation/devicetree/bindings/fpga/fpga-region.yaml.
> > >
> > > No, Non-OF devices don't have to follow the DT binding.
> > >
> > > > If so, it may be advantageous to develop a common interface for
> > > > both OF and
> > > non-OF.
> > > > If not, it might be more appropriate to establish distinct
> > > > interfaces to cater to their
> > > specific requirements.
> > >
> > > I think each vendor may have specific way for device enumeration,
> > > but that doesn't mean we need distinct user interfaces. For all FPGA
> > > devices, we should avoid the situation that the HW is changed but
> > > system SW knows nothing. So the common needs are:
> > >
> > >  - Find out and remove all devices within the fpga region before
> > >    reprograming.
> > >  - Re-enumerate devices in fpga region after reprograming.
> > >
> > > I expect the fpga region class could generally enforce a flow for
> > > the reprograming interface. And of-fpga-region could specifically implement it
> using DT overlay.
> > >
> >
> > To address the vendor-specific nature(either of or non-of) of device
> > enumeration in FPGA regions, As you suggested, we can develop a common
> > programming interface that abstracts these vendor-specifc differences.
> > This can be achieved by integrating vendor-specific callbacks(ex: of
> > and non-of) for device configuration, enumeration and removal to fpga-region.
> >
> > I have outlined the top-level framework changes here:
> >
> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> > index b364a929425c..7d4b755dc8e0 100644
> > --- a/drivers/fpga/fpga-region.c
> > +++ b/drivers/fpga/fpga-region.c
> > @@ -213,6 +213,7 @@ fpga_region_register_full(struct device *parent, const
> struct fpga_region_info *
> >  	region->compat_id = info->compat_id;
> >  	region->priv = info->priv;
> >  	region->get_bridges = info->get_bridges;
> > +	region->region_ops = info->region_ops;
> >
> >  	mutex_init(&region->mutex);
> >  	INIT_LIST_HEAD(&region->bridge_list);
> > @@ -257,17 +258,46 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >   */
> >  struct fpga_region *
> >  fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> > +		     struct fpga_region_ops *region_ops,
> >  		     int (*get_bridges)(struct fpga_region *))  {
> >  	struct fpga_region_info info = { 0 };
> >
> >  	info.mgr = mgr;
> >  	info.get_bridges = get_bridges;
> > +	info.region_ops = region_ops;
> >
> >  	return fpga_region_register_full(parent, &info);  }
> > EXPORT_SYMBOL_GPL(fpga_region_register);
> >
> > +static int fpga_region_device_open(struct inode *inode, struct file *file) {
> > +	struct miscdevice *miscdev = file->private_data;
> > +	struct fpga_region *region = container_of(miscdev, struct
> > +fpga_region, miscdev);
> > +
> > +	file->private_data = region;
> > +
> > +	return 0;
> > +}
> > +
> > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned
> long arg) {
> > +	char __user *argp = (char __user *)arg;
> > +	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
> > +	int err;
> > +
> > +	switch (cmd) {
> > +	case FPGA_REGION_IOCTL_LOAD:
> > +		err = region->region_ops->fpga_region_config_enumerate (region,
> > +argp);
> 
> Not sure "void *args" is a proposal or something yet to be decided.
> I think we should try best not to give up parameter type and have a clear API
> definition.
> 

I agree fixing the parameter type is necessary, and I will work on
Implementing this change in the RFC. Following our discussion,
If any additional modifications or clarifications are required,
I will ensure to incorporate them in the later stages.

> > +		break;
> > +	case FPGA_REGION_IOCTL_REMOVE:
> > +		err = region->region_ops->fpga_region_remove(region, argp);
> > +		break;
> > +	case FPGA_REGION_IOCTL_STATUS:
> > +		err = region->region_ops->fpga_region_status(region, argp);
> > +	default:
> > +		err = -ENOTTY;
> > +}
> > +
> >  /**
> >   * fpga_region_unregister - unregister an FPGA region
> >   * @region: FPGA region
> > diff --git a/include/linux/fpga/fpga-region.h
> > b/include/linux/fpga/fpga-region.h
> > index 9d4d32909340..725fdcbab3d8 100644
> > --- a/include/linux/fpga/fpga-region.h
> > +++ b/include/linux/fpga/fpga-region.h
> > @@ -9,6 +9,20 @@
> >
> >  struct fpga_region;
> >
> > +/**
> > + * struct fpga_region_ops - ops for low level FPGA region ops for
> > +device
> > + * enumeration/removal
> > + * @region_status: returns the FPGA region status
> > + * @region_config_enumeration: Configure and enumerate the FPGA region.
> 
> region config could be a common existing operation, fpga_region_program_fpga().
> So maybe only enumeration is needed?
> 

I agree configuration and enumeration can be handled separately. However,
for some vendors, pre-configuration details such as port IDs(AFU) and
lower-level connected device information needs to be set before configuration.
To accommodate both vendor-specific pre and post configuration (enumerations),
I chose to use a single API for handling both FPGA configuration and enumeration.

> > + * @region_remove: Remove all devices within the fpga region
> > + * (which are added as part of the enumeration).
> > + */
> > +struct fpga_region_ops {
> > +	int (*region_status)(struct fpga_region *bridge);
> > +	int (*region_config_enumeration)(struct fpga_region *region, void *args);
> > +	void (*region_remove)(struct fpga_region *region, void *args); };
> > +
> >  /**
> >   * struct fpga_region_info - collection of parameters an FPGA Region
> >   * @mgr: fpga region manager
> > @@ -26,6 +40,7 @@ struct fpga_region_info {
> >  	struct fpga_compat_id *compat_id;
> >  	void *priv;
> >  	int (*get_bridges)(struct fpga_region *region);
> > +	struct fpga_region_ops *region_ops;
> >  };
> >
> >  /**
> > @@ -48,6 +63,7 @@ struct fpga_region {
> >  	struct fpga_compat_id *compat_id;
> >  	void *priv;
> >  	int (*get_bridges)(struct fpga_region *region);
> > +	struct fpga_region_ops *region_ops;
> >  };
> >
> >  #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
> >
> > In this approach, we utilized an IOCTL-based user interface, but it
> > doesn't have to be confined to IOCTL. We can also use sysfs or
> > configfs, or other appropriate options as we finalized on it.
> >
> > This call-backs approach works for both OF and non-OF devices.
> > If this aligns with your expectations, I can do the necessary changes
> 
> There are still much to discuss, but yes this is a good start.
> 

Thanks for providing the quick confirmation will post RFC soon.

Regards,
Navakishore

Powered by blists - more mailing lists