[PATCH] fpga: add simple userspace interface to trigger FPGA programming

[PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Thomas Petazzoni]

The current FPGA subsystem only allows programming the FPGA bitstream
through Device Tree overlays. This assumes that the devices inside the
FPGA are described through a Device Tree.

However, some platforms have their FPGA connected to the main CPU over
PCIe and the devices in the FPGA are therefore dynamically
discoverable using the normal PCIe enumeration mechanisms. There is
therefore no Device Tree overlay describing the devices in the
FPGA. Furthermore, on my platform (an old SH7786), there is no Device
Tree at all, as there is no support for Device Tree for this SoC.

Adding a userspace interface to trigger the programming of the FPGA
has already been requested in the past (see [1]) showing that there is
a need for such a feature.

This commit therefore introduces a very simple interface, in the form
of a "load" sysfs file. Writing the name of the firmware file to
program into the FPGA to this "load" file triggers the programming
process.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/443034.html

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Documentation/ABI/testing/sysfs-class-fpga-manager | 11 +++++++++
 drivers/fpga/fpga-mgr.c                            | 28 ++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
index 23056c532fdd..46785ad8b878 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-manager
+++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
@@ -35,3 +35,14 @@ Description:	Read fpga manager state as a string.
 		* write complete	= Doing post programming steps
 		* write complete error	= Error while doing post programming
 		* operating		= FPGA is programmed and operating
+
+What:		/sys/class/fpga_manager/<fpga>/load
+Date:		December 2017
+KernelVersion:	4.16
+Contact:	Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+
+Description:	Program a bitstream firmware into the FPGA.
+
+		Writing the name of a firmware file into this "load"
+		file will trigger the programming of the FPGA using
+		that firmware.
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 188ffefa3cc3..1c7ca77c96c2 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -351,12 +351,40 @@ static ssize_t state_show(struct device *dev,
 	return sprintf(buf, "%s\n", state_str[mgr->state]);
 }
 
+static ssize_t load_store(struct device *dev,
+			  struct device_attribute *attr, const char *buf,
+			  size_t count)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+	char *name;
+	int ret;
+
+	if (count > 0 && buf[count - 1] == '\n')
+		count--;
+
+	name = kstrndup(buf, count, GFP_KERNEL);
+	if (!name)
+		return -ENOSPC;
+
+	ret = fpga_mgr_firmware_load(mgr, NULL, name);
+	if (ret < 0) {
+		kfree(name);
+		return ret;
+	}
+
+	kfree(name);
+
+	return count;
+}
+
 static DEVICE_ATTR_RO(name);
 static DEVICE_ATTR_RO(state);
+static DEVICE_ATTR_WO(load);
 
 static struct attribute *fpga_mgr_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_state.attr,
+	&dev_attr_load.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(fpga_mgr);
-- 
2.13.6

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Mon, Dec 4, 2017 at 9:43 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> The current FPGA subsystem only allows programming the FPGA bitstream
> through Device Tree overlays. This assumes that the devices inside the
> FPGA are described through a Device Tree.
>
> However, some platforms have their FPGA connected to the main CPU over
> PCIe and the devices in the FPGA are therefore dynamically
> discoverable using the normal PCIe enumeration mechanisms. There is
> therefore no Device Tree overlay describing the devices in the
> FPGA. Furthermore, on my platform (an old SH7786), there is no Device
> Tree at all, as there is no support for Device Tree for this SoC.
>
> Adding a userspace interface to trigger the programming of the FPGA
> has already been requested in the past (see [1]) showing that there is
> a need for such a feature.
>
> This commit therefore introduces a very simple interface, in the form
> of a "load" sysfs file. Writing the name of the firmware file to
> program into the FPGA to this "load" file triggers the programming
> process.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/443034.html
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Hi Thomas,

The problem I see with this is that it's allowing userspace to switch
out what's on the FPGA without controlling bridges and without
unloading drivers first.

Alan

> ---
>  Documentation/ABI/testing/sysfs-class-fpga-manager | 11 +++++++++
>  drivers/fpga/fpga-mgr.c                            | 28 ++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
> index 23056c532fdd..46785ad8b878 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
> @@ -35,3 +35,14 @@ Description: Read fpga manager state as a string.
>                 * write complete        = Doing post programming steps
>                 * write complete error  = Error while doing post programming
>                 * operating             = FPGA is programmed and operating
> +
> +What:          /sys/class/fpga_manager/<fpga>/load
> +Date:          December 2017
> +KernelVersion: 4.16
> +Contact:       Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> +
> +Description:   Program a bitstream firmware into the FPGA.
> +
> +               Writing the name of a firmware file into this "load"
> +               file will trigger the programming of the FPGA using
> +               that firmware.
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 188ffefa3cc3..1c7ca77c96c2 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -351,12 +351,40 @@ static ssize_t state_show(struct device *dev,
>         return sprintf(buf, "%s\n", state_str[mgr->state]);
>  }
>
> +static ssize_t load_store(struct device *dev,
> +                         struct device_attribute *attr, const char *buf,
> +                         size_t count)
> +{
> +       struct fpga_manager *mgr = to_fpga_manager(dev);
> +       char *name;
> +       int ret;
> +
> +       if (count > 0 && buf[count - 1] == '\n')
> +               count--;
> +
> +       name = kstrndup(buf, count, GFP_KERNEL);
> +       if (!name)
> +               return -ENOSPC;
> +
> +       ret = fpga_mgr_firmware_load(mgr, NULL, name);
> +       if (ret < 0) {
> +               kfree(name);
> +               return ret;
> +       }
> +
> +       kfree(name);
> +
> +       return count;
> +}
> +
>  static DEVICE_ATTR_RO(name);
>  static DEVICE_ATTR_RO(state);
> +static DEVICE_ATTR_WO(load);
>
>  static struct attribute *fpga_mgr_attrs[] = {
>         &dev_attr_name.attr,
>         &dev_attr_state.attr,
> +       &dev_attr_load.attr,
>         NULL,
>  };
>  ATTRIBUTE_GROUPS(fpga_mgr);
> --
> 2.13.6
>

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Thomas Petazzoni]

Hello,

On Mon, 4 Dec 2017 09:50:14 -0600, Alan Tull wrote:

> Hi Thomas,
> 
> The problem I see with this is that it's allowing userspace to switch
> out what's on the FPGA without controlling bridges and without
> unloading drivers first.

Correct. Do you have an idea on how to handle this? I'm not sure there's
an easy way to do that, but simply preventing people from programming
their FPGAs seems like a rather poor solution to the problem :-)

Right now, the way it works for me is that at boot time the FPGA is
unprogrammed so the initial PCI enumeration doesn't find any PCI
device. The FPGA is programmed, and I then force a PCI bus rescan
using /sys/bus/pci/rescan, which detects the device and probes the
driver.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Mon, Dec 4, 2017 at 9:58 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Mon, 4 Dec 2017 09:50:14 -0600, Alan Tull wrote:
>
>> Hi Thomas,
>>
>> The problem I see with this is that it's allowing userspace to switch
>> out what's on the FPGA without controlling bridges and without
>> unloading drivers first.
>
> Correct. Do you have an idea on how to handle this? I'm not sure there's
> an easy way to do that, but simply preventing people from programming
> their FPGAs seems like a rather poor solution to the problem :-)

Yes.  It's not an easy one to solve or we would have added something
like this by now.  There is one large patchset that is currently under
review.
It's called "Intel FPGA Drivers"[1] but I don't see anything Intel
specific in it.  It's dependent on the FPGA having a base image that
contains structures for some enumeration.  When the PR regions are
programmed, the FPGA based hardware in those regions also have similar
structures.  The first iteration of this is PCIe based.

Others have suggested using device tree on platforms that aren't
normally using device tree.  If I had time I'd be experimenting with
that.

Alan

[1] https://lkml.org/lkml/2017/11/27/56

>
> Right now, the way it works for me is that at boot time the FPGA is
> unprogrammed so the initial PCI enumeration doesn't find any PCI
> device. The FPGA is programmed, and I then force a PCI bus rescan
> using /sys/bus/pci/rescan, which detects the device and probes the
> driver.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Moritz Fischer]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On Mon, Dec 04, 2017 at 10:25:47AM -0600, Alan Tull wrote:
> On Mon, Dec 4, 2017 at 9:58 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Hello,
> >
> > On Mon, 4 Dec 2017 09:50:14 -0600, Alan Tull wrote:
> >
> >> Hi Thomas,
> >>
> >> The problem I see with this is that it's allowing userspace to switch
> >> out what's on the FPGA without controlling bridges and without
> >> unloading drivers first.
> >
> > Correct. Do you have an idea on how to handle this? I'm not sure there's
> > an easy way to do that, but simply preventing people from programming
> > their FPGAs seems like a rather poor solution to the problem :-)
> 
> Yes.  It's not an easy one to solve or we would have added something
> like this by now.  There is one large patchset that is currently under
> review.
> It's called "Intel FPGA Drivers"[1] but I don't see anything Intel
> specific in it.  It's dependent on the FPGA having a base image that
> contains structures for some enumeration.  When the PR regions are
> programmed, the FPGA based hardware in those regions also have similar
> structures.  The first iteration of this is PCIe based.

The difference here is, that in Thomas' case there's no base device to
bind to, i.e. there's no base image in the FPGA the first time around.

In the Intel FPGA case there's something that connects to the bus that
the FPGA manager driver can bind to.

> Others have suggested using device tree on platforms that aren't
> normally using device tree.  If I had time I'd be experimenting with
> that.
> 

Moritz

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Mon, Dec 4, 2017 at 10:49 AM, Moritz Fischer <mdf@kernel.org> wrote:
> On Mon, Dec 04, 2017 at 10:25:47AM -0600, Alan Tull wrote:
>> On Mon, Dec 4, 2017 at 9:58 AM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > Hello,
>> >
>> > On Mon, 4 Dec 2017 09:50:14 -0600, Alan Tull wrote:
>> >
>> >> Hi Thomas,
>> >>
>> >> The problem I see with this is that it's allowing userspace to switch
>> >> out what's on the FPGA without controlling bridges and without
>> >> unloading drivers first.
>> >
>> > Correct. Do you have an idea on how to handle this? I'm not sure there's
>> > an easy way to do that, but simply preventing people from programming
>> > their FPGAs seems like a rather poor solution to the problem :-)
>>
>> Yes.  It's not an easy one to solve or we would have added something
>> like this by now.  There is one large patchset that is currently under
>> review.
>> It's called "Intel FPGA Drivers"[1] but I don't see anything Intel
>> specific in it.  It's dependent on the FPGA having a base image that
>> contains structures for some enumeration.  When the PR regions are
>> programmed, the FPGA based hardware in those regions also have similar
>> structures.  The first iteration of this is PCIe based.
>
> The difference here is, that in Thomas' case there's no base device to
> bind to, i.e. there's no base image in the FPGA the first time around.
>
> In the Intel FPGA case there's something that connects to the bus that
> the FPGA manager driver can bind to.
>

Yeah, I know. I'm mentioning it only because it is the only pcie based
non-DT solution that has been offered so far that handles the bridges
and drivers.

>> Others have suggested using device tree on platforms that aren't
>> normally using device tree.  If I had time I'd be experimenting with
>> that.
>>
>
> Moritz

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Florian Fainelli]

On 12/04/2017 07:50 AM, Alan Tull wrote:
> On Mon, Dec 4, 2017 at 9:43 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
>> The current FPGA subsystem only allows programming the FPGA bitstream
>> through Device Tree overlays. This assumes that the devices inside the
>> FPGA are described through a Device Tree.
>>
>> However, some platforms have their FPGA connected to the main CPU over
>> PCIe and the devices in the FPGA are therefore dynamically
>> discoverable using the normal PCIe enumeration mechanisms. There is
>> therefore no Device Tree overlay describing the devices in the
>> FPGA. Furthermore, on my platform (an old SH7786), there is no Device
>> Tree at all, as there is no support for Device Tree for this SoC.
>>
>> Adding a userspace interface to trigger the programming of the FPGA
>> has already been requested in the past (see [1]) showing that there is
>> a need for such a feature.
>>
>> This commit therefore introduces a very simple interface, in the form
>> of a "load" sysfs file. Writing the name of the firmware file to
>> program into the FPGA to this "load" file triggers the programming
>> process.
>>
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/443034.html
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> Hi Thomas,
> 
> The problem I see with this is that it's allowing userspace to switch
> out what's on the FPGA without controlling bridges and without
> unloading drivers first.

We can actually use the Linux device driver model to do that though.
Assuming all peripherals behind the FPGA do have the FPGA manager's
struct device as a parent (which they should), when there is a bistream
load request, we ought to be able to teardown all of these child struct
device's and their corresponding drivers (unbind), load the bistream,
and then rebind the drivers accordingly.

In case you have a DT enabled system, the parent/child relationship is
established through Device Tree, in case you don't, you should be able
to use bus notifiers to establish that parenting.

Thoughts?
-- 
Florian

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Sat, Dec 9, 2017 at 12:05 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
> On 12/04/2017 07:50 AM, Alan Tull wrote:
>> On Mon, Dec 4, 2017 at 9:43 AM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>>> The current FPGA subsystem only allows programming the FPGA bitstream
>>> through Device Tree overlays. This assumes that the devices inside the
>>> FPGA are described through a Device Tree.
>>>
>>> However, some platforms have their FPGA connected to the main CPU over
>>> PCIe and the devices in the FPGA are therefore dynamically
>>> discoverable using the normal PCIe enumeration mechanisms. There is
>>> therefore no Device Tree overlay describing the devices in the
>>> FPGA. Furthermore, on my platform (an old SH7786), there is no Device
>>> Tree at all, as there is no support for Device Tree for this SoC.
>>>
>>> Adding a userspace interface to trigger the programming of the FPGA
>>> has already been requested in the past (see [1]) showing that there is
>>> a need for such a feature.
>>>
>>> This commit therefore introduces a very simple interface, in the form
>>> of a "load" sysfs file. Writing the name of the firmware file to
>>> program into the FPGA to this "load" file triggers the programming
>>> process.
>>>
>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/443034.html
>>>
>>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>
>> Hi Thomas,
>>
>> The problem I see with this is that it's allowing userspace to switch
>> out what's on the FPGA without controlling bridges and without
>> unloading drivers first.
>
> We can actually use the Linux device driver model to do that though.
> Assuming all peripherals behind the FPGA do have the FPGA manager's
> struct device as a parent (which they should), when there is a bistream
> load request, we ought to be able to teardown all of these child struct
> device's and their corresponding drivers (unbind), load the bistream,
> and then rebind the drivers accordingly.

Hi Florain,

FPGA regions are a way of handling all that plus the bridges [1] or
are you proposing something else?  An FPGA region knows what manager
to use and what bridges (if any) needs to be disabled while
programming is happening.  Applying a DT overlay targeting a FPGA
region is used to program the FPGA.  After programming, the child
nodes of the region are added to the DT and drivers get probed.  The
interface I've been using to apply overlays is Pantelis' configfs DT
overlay interface [2] which hasn't been accepted upstream and probably
won't for the foreseeable future [3] [4].

Alan

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/fpga/fpga-region.txt

[2] https://github.com/pantoniou/linux-beagle-track-mainline/commit/4f0ed58518749c1a8ce8999cb615b1eb04b8c035

[3] https://lkml.org/lkml/2017/10/18/609

[4] https://lkml.org/lkml/2017/12/6/264

>
> In case you have a DT enabled system, the parent/child relationship is
> established through Device Tree, in case you don't, you should be able
> to use bus notifiers to establish that parenting.
>
> Thoughts?



> --
> Florian

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Thomas Petazzoni]

Hello,

On Sat, 9 Dec 2017 10:05:55 -0800, Florian Fainelli wrote:

> > The problem I see with this is that it's allowing userspace to switch
> > out what's on the FPGA without controlling bridges and without
> > unloading drivers first.  
> 
> We can actually use the Linux device driver model to do that though.
> Assuming all peripherals behind the FPGA do have the FPGA manager's
> struct device as a parent (which they should),

Well in my setup, that isn't the case. The device implemented in the
FPGA is a PCIe endpoint, so its parent device from a device model point
of view if the PCIe root complex, which is a platform_device in the SoC.

So there is currently no connection between the PCIe endpoint
implemented in the FPGA, and the FPGA device itself.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Thomas Petazzoni]

Hello,

On Sat, 9 Dec 2017 22:03:06 -0600, Alan Tull wrote:

> > We can actually use the Linux device driver model to do that though.
> > Assuming all peripherals behind the FPGA do have the FPGA manager's
> > struct device as a parent (which they should), when there is a bistream
> > load request, we ought to be able to teardown all of these child struct
> > device's and their corresponding drivers (unbind), load the bistream,
> > and then rebind the drivers accordingly.  
> 
> Hi Florain,
> 
> FPGA regions are a way of handling all that plus the bridges [1] or
> are you proposing something else?  An FPGA region knows what manager
> to use and what bridges (if any) needs to be disabled while
> programming is happening.  Applying a DT overlay targeting a FPGA
> region is used to program the FPGA.

I thought my initial patch made it clear: DT overlays are not an
applicable solution for my use case, for two reasons:

 1. My platform doesn't use Device Tree at all.

 2. The device implemented in the FPGA is self-discoverable because it
    sits on a PCI bus, so there is no point in doing a static
    description of the device in a DT overlay.

So, solutions based on DT overlays are quite irrelevant for my use
case. Am I missing something here ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Florian Fainelli]

On 12/10/2017 02:44 PM, Thomas Petazzoni wrote:
> Hello,
> 
> On Sat, 9 Dec 2017 22:03:06 -0600, Alan Tull wrote:
> 
>>> We can actually use the Linux device driver model to do that though.
>>> Assuming all peripherals behind the FPGA do have the FPGA manager's
>>> struct device as a parent (which they should), when there is a bistream
>>> load request, we ought to be able to teardown all of these child struct
>>> device's and their corresponding drivers (unbind), load the bistream,
>>> and then rebind the drivers accordingly.  
>>
>> Hi Florain,
>>
>> FPGA regions are a way of handling all that plus the bridges [1] or
>> are you proposing something else?  An FPGA region knows what manager
>> to use and what bridges (if any) needs to be disabled while
>> programming is happening.  Applying a DT overlay targeting a FPGA
>> region is used to program the FPGA.
> 
> I thought my initial patch made it clear: DT overlays are not an
> applicable solution for my use case, for two reasons:
> 
>  1. My platform doesn't use Device Tree at all.
> 
>  2. The device implemented in the FPGA is self-discoverable because it
>     sits on a PCI bus, so there is no point in doing a static
>     description of the device in a DT overlay.

Agreed, sorry for side tracking the discussion a bit on that topic.

> 
> So, solutions based on DT overlays are quite irrelevant for my use
> case. Am I missing something here ?

I don't think you are, in fact, as it stands today, unless your FPGA has
a bistream which is already loaded, in which case you don't really need
the FPGA manager framework at all, this framework is completely useless
in offering people the ability to load bistreams on non-DT enabled
platforms without resorting to out of tree hacks like this one for instance:

https://github.com/ffainelli/linux/commit/78a4824165d7bf627c1b2dbb645ce013185331de

So what needs to be done so we can allow people to take full advantage
of the fact that FPGAs are (re)programmable, even when the platform does
not use DT?
-- 
Florian

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Moritz Fischer]

[-- Attachment #1: Type: text/plain, Size: 2670 bytes --]

On Sun, Dec 10, 2017 at 02:59:36PM -0800, Florian Fainelli wrote:
> 
> 
> On 12/10/2017 02:44 PM, Thomas Petazzoni wrote:
> > Hello,
> > 
> > On Sat, 9 Dec 2017 22:03:06 -0600, Alan Tull wrote:
> > 
> >>> We can actually use the Linux device driver model to do that though.
> >>> Assuming all peripherals behind the FPGA do have the FPGA manager's
> >>> struct device as a parent (which they should), when there is a bistream
> >>> load request, we ought to be able to teardown all of these child struct
> >>> device's and their corresponding drivers (unbind), load the bistream,
> >>> and then rebind the drivers accordingly.  
> >>
> >> Hi Florain,
> >>
> >> FPGA regions are a way of handling all that plus the bridges [1] or
> >> are you proposing something else?  An FPGA region knows what manager
> >> to use and what bridges (if any) needs to be disabled while
> >> programming is happening.  Applying a DT overlay targeting a FPGA
> >> region is used to program the FPGA.
> > 
> > I thought my initial patch made it clear: DT overlays are not an
> > applicable solution for my use case, for two reasons:
> > 
> >  1. My platform doesn't use Device Tree at all.
> > 
> >  2. The device implemented in the FPGA is self-discoverable because it
> >     sits on a PCI bus, so there is no point in doing a static
> >     description of the device in a DT overlay.
> 
> Agreed, sorry for side tracking the discussion a bit on that topic.

I do agree that wee need to find a solution for this. We should not
force people to use DT overlays in a system that is discoverable,
or out of tree hacks.

> 
> > 
> > So, solutions based on DT overlays are quite irrelevant for my use
> > case. Am I missing something here ?
> 
> I don't think you are, in fact, as it stands today, unless your FPGA has
> a bistream which is already loaded, in which case you don't really need
> the FPGA manager framework at all, this framework is completely useless
> in offering people the ability to load bistreams on non-DT enabled
> platforms without resorting to out of tree hacks like this one for instance:
> 
> https://github.com/ffainelli/linux/commit/78a4824165d7bf627c1b2dbb645ce013185331de
> 
> So what needs to be done so we can allow people to take full advantage
> of the fact that FPGAs are (re)programmable, even when the platform does
> not use DT?


I like your suggestion of tying into the device model, to tear down
everything underneath.

Would making the PCIe root complex a child to a FPGA region work? Seems
clumsy. Do we have other examples of devices reloading firmware
triggered by userland?

- Moritz

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Mon, Dec 11, 2017 at 4:05 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Sun, Dec 10, 2017 at 02:59:36PM -0800, Florian Fainelli wrote:
>>
>>
>> On 12/10/2017 02:44 PM, Thomas Petazzoni wrote:
>> > Hello,
>> >
>> > On Sat, 9 Dec 2017 22:03:06 -0600, Alan Tull wrote:
>> >
>> >>> We can actually use the Linux device driver model to do that though.
>> >>> Assuming all peripherals behind the FPGA do have the FPGA manager's
>> >>> struct device as a parent (which they should), when there is a bistream
>> >>> load request, we ought to be able to teardown all of these child struct
>> >>> device's and their corresponding drivers (unbind), load the bistream,
>> >>> and then rebind the drivers accordingly.
>> >>
>> >> Hi Florain,
>> >>
>> >> FPGA regions are a way of handling all that plus the bridges [1] or
>> >> are you proposing something else?  An FPGA region knows what manager
>> >> to use and what bridges (if any) needs to be disabled while
>> >> programming is happening.  Applying a DT overlay targeting a FPGA
>> >> region is used to program the FPGA.
>> >
>> > I thought my initial patch made it clear: DT overlays are not an
>> > applicable solution for my use case, for two reasons:
>> >
>> >  1. My platform doesn't use Device Tree at all.
>> >
>> >  2. The device implemented in the FPGA is self-discoverable because it
>> >     sits on a PCI bus, so there is no point in doing a static
>> >     description of the device in a DT overlay.
>>
>> Agreed, sorry for side tracking the discussion a bit on that topic.
>
> I do agree that wee need to find a solution for this. We should not
> force people to use DT overlays in a system that is discoverable,
> or out of tree hacks.
>
>>
>> >
>> > So, solutions based on DT overlays are quite irrelevant for my use
>> > case. Am I missing something here ?
>>
>> I don't think you are, in fact, as it stands today, unless your FPGA has
>> a bistream which is already loaded, in which case you don't really need
>> the FPGA manager framework at all, this framework is completely useless
>> in offering people the ability to load bistreams on non-DT enabled
>> platforms without resorting to out of tree hacks like this one for instance:
>>
>> https://github.com/ffainelli/linux/commit/78a4824165d7bf627c1b2dbb645ce013185331de

I'd like to call attention to the history of the FPGA framework.  I've
posted lots of attempts at userspace interfaces on the list, using
sysfs or configfs mainly.  One by one they have all have gotten shot
down :)  This led to one central thing about this framework: it
separates the devices from the interface.  If someone can come up with
an upper layer that calls the framework functions, it can work on a
broad set of FPGAs.  That's the real usefulness of this useless
framework.

A second piece I want to mention is that I want to urge people to
think in terms of FPGA regions rather than managers.  If you have a
region, that can point to a manager only or to a manager and
bridge(s).  So if you can add an interface that communicates with a
region, it's can support a broader range of devices.  So I'm not
looking for something hooked directly into fpga-mgr.c.

The current code in linux-next separates FPGA region common code from
its DT dependencies.  This enables developing layers to use PCIe
FPGA's using FPGA regions without DT.  Currently the first user of
this is the DFL patchset (which isn't going to suit everybody).  If
someone has a good idea for handling full reconfiguration on PCIe
FPGAs without DT, they can base it on that code.  And yes, I tried to
add an interface to that FPGA region code when I was writing these
changes and that interface also got shot down (it was DT so no huge
loss in this discussion).  Plus the RFC that I followed it up with got
total silence on the mailing list.  It turns out it's hard to get FPGA
people to agree on things :)

>>
>> So what needs to be done so we can allow people to take full advantage
>> of the fact that FPGAs are (re)programmable, even when the platform does
>> not use DT?
>
>
> I like your suggestion of tying into the device model, to tear down
> everything underneath.

Me too.

>
> Would making the PCIe root complex a child to a FPGA region work? Seems
> clumsy. Do we have other examples of devices reloading firmware
> triggered by userland?
>
> - Moritz

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Thomas Petazzoni]

Hello,

On Wed, 13 Dec 2017 06:30:22 +0000 (UTC),
yves.vandervennet@linux.intel.com wrote:

> > 2. The device implemented in the FPGA is self-discoverable because it
> >    sits on a PCI bus, so there is no point in doing a static
> >    description of the device in a DT overlay.  
> How is the enumeration triggered?

I am manually forcing a rescan of the PCI bus by doing

 echo 1 > /sys/bus/pci/rescan

Right now my process is:

 1. Remove the PCIe device instantiated in the FPGA if it exists (the
    FPGA programming is preserved across a warm reset of the platform).
    Indeed, as was mentioned in the this thread earlier, things don't
    work really well if your PCIe device instantiated in the FPGA is
    enumerated, and the FPGA gets reprogrammed underneath.

 2. Program the FPGA using the additional sysfs file that my patch
    proposes to add.

 3. Rescan the PCI bus by echoing to /sys/bus/pci/rescan.

> Do you have any bridges to take care of?

I am not sure what you mean by bridge in this context. My use case is
pretty simple:

 - The SoC has a PCIE controller. The PCIE controller is a hard IP
   block in the SoC.

 - There is a FPGA on the board which is connected directly to the SoC
   over PCIe + over a control bus that allows programming of the FPGA.

Therefore, there is no bridge involved: once the FPGA is programmed,
the PCIE controller in the SoC can enumerate and see a new PCIE device,
which happens to be implemented in the FPGA.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[yves.vandervennet]

On Sun, 10 Dec 2017, Thomas Petazzoni wrote:

> Hello,
>
> On Sat, 9 Dec 2017 22:03:06 -0600, Alan Tull wrote:
>
>>> We can actually use the Linux device driver model to do that though.
>>> Assuming all peripherals behind the FPGA do have the FPGA manager's
>>> struct device as a parent (which they should), when there is a bistream
>>> load request, we ought to be able to teardown all of these child struct
>>> device's and their corresponding drivers (unbind), load the bistream,
>>> and then rebind the drivers accordingly.
>>
>> Hi Florain,
>>
>> FPGA regions are a way of handling all that plus the bridges [1] or
>> are you proposing something else?  An FPGA region knows what manager
>> to use and what bridges (if any) needs to be disabled while
>> programming is happening.  Applying a DT overlay targeting a FPGA
>> region is used to program the FPGA.
>

Thomas,

> I thought my initial patch made it clear: DT overlays are not an
> applicable solution for my use case, for two reasons:
>
> 1. My platform doesn't use Device Tree at all.
>
>
> 2. The device implemented in the FPGA is self-discoverable because it
>    sits on a PCI bus, so there is no point in doing a static
>    description of the device in a DT overlay.
How is the enumeration triggered? Do you have any bridges to take care of?

>
> So, solutions based on DT overlays are quite irrelevant for my use
> case. Am I missing something here ?

Cheers
Yves

>
> Best regards,
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Tue, Dec 12, 2017 at 11:23 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Wed, 13 Dec 2017 06:30:22 +0000 (UTC),
> yves.vandervennet@linux.intel.com wrote:
>
>> > 2. The device implemented in the FPGA is self-discoverable because it
>> >    sits on a PCI bus, so there is no point in doing a static
>> >    description of the device in a DT overlay.
>> How is the enumeration triggered?
>
> I am manually forcing a rescan of the PCI bus by doing
>
>  echo 1 > /sys/bus/pci/rescan
>
> Right now my process is:
>
>  1. Remove the PCIe device instantiated in the FPGA if it exists (the
>     FPGA programming is preserved across a warm reset of the platform).
>     Indeed, as was mentioned in the this thread earlier, things don't
>     work really well if your PCIe device instantiated in the FPGA is
>     enumerated, and the FPGA gets reprogrammed underneath.
>
>  2. Program the FPGA using the additional sysfs file that my patch
>     proposes to add.
>
>  3. Rescan the PCI bus by echoing to /sys/bus/pci/rescan.
>
>> Do you have any bridges to take care of?
>
> I am not sure what you mean by bridge in this context.

Hi Thomas,

An FPGA bridge is something that connects the FPGA fabric to the world
outside the FPGA.  Not every setup will have this.  During programming
the bridge can be disabled to protect the outside world from bad
randomness (the fabric may wiggle some).   It the case of full
reconfiguration, it would be a hardware bridge/bridges on the bus.  In
the case of partial reconfiguration, it would be a freeze bridge
surrounding the partial reconfiguration region since the same manager
could be used to reprogram each of several regions, but each will have
their own bridge.  It sounds like in your case, it's full
reconfiguration and either there's no bridge or it's left enabled or
the programming code is handling the bridge or you're enabling the
bridge after programming.  FPGA regions were created as a way of
allowing an interface to potentially work for both setups that have
bridges or don't have them.  This is all just to give some context as
to why interfaces should be added on top of a region rather than a
manager.  So people could use the interface whether they have a bridge
or not.

> My use case is
> pretty simple:
>
>  - The SoC has a PCIE controller. The PCIE controller is a hard IP
>    block in the SoC.
>
>  - There is a FPGA on the board which is connected directly to the SoC
>    over PCIe + over a control bus that allows programming of the FPGA.

Just to be clear, the programming is not through PCIe, right?

>
> Therefore, there is no bridge involved: once the FPGA is programmed,
> the PCIE controller in the SoC can enumerate and see a new PCIE device,
> which happens to be implemented in the FPGA.

Yes, full reconfiguration of a FPGA that's on PCIe is a common use
case and one we want to support.  This subsystem started out
supporting embedded FPGAs so some things may need to adapt for PCIe.
The code on linux-next is a step towards that, but not a complete
solution.

Alan

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Thomas Petazzoni]

Hello,

On Wed, 13 Dec 2017 09:59:55 -0600, Alan Tull wrote:

> An FPGA bridge is something that connects the FPGA fabric to the world
> outside the FPGA.  Not every setup will have this.  During programming
> the bridge can be disabled to protect the outside world from bad
> randomness (the fabric may wiggle some).   It the case of full
> reconfiguration, it would be a hardware bridge/bridges on the bus.  In
> the case of partial reconfiguration, it would be a freeze bridge
> surrounding the partial reconfiguration region since the same manager
> could be used to reprogram each of several regions, but each will have
> their own bridge.  It sounds like in your case, it's full
> reconfiguration and either there's no bridge or it's left enabled or
> the programming code is handling the bridge or you're enabling the
> bridge after programming.  FPGA regions were created as a way of
> allowing an interface to potentially work for both setups that have
> bridges or don't have them.  This is all just to give some context as
> to why interfaces should be added on top of a region rather than a
> manager.  So people could use the interface whether they have a bridge
> or not.

Hum, OK. I'm not sure to grasp all the details here. In my use case, I
don't have anything that looks like a software-controlled FPGA bridge.
There is a memory-mapped CPLD, which provides some registers that the
SoC uses to program the FPGA. Based on those register writes, the CPLD
programs the FPGA.

> > My use case is
> > pretty simple:
> >
> >  - The SoC has a PCIE controller. The PCIE controller is a hard IP
> >    block in the SoC.
> >
> >  - There is a FPGA on the board which is connected directly to the SoC
> >    over PCIe + over a control bus that allows programming of the FPGA.  
> 
> Just to be clear, the programming is not through PCIe, right?

Correct, there's a side-band channel in the form of a memory-mapped
CPLD.

> > Therefore, there is no bridge involved: once the FPGA is programmed,
> > the PCIE controller in the SoC can enumerate and see a new PCIE device,
> > which happens to be implemented in the FPGA.  
> 
> Yes, full reconfiguration of a FPGA that's on PCIe is a common use
> case and one we want to support.  This subsystem started out
> supporting embedded FPGAs so some things may need to adapt for PCIe.
> The code on linux-next is a step towards that, but not a complete
> solution.

Could you summarize what is in linux-next and how it is a step towards
that, and what is missing ?

Thanks a lot for your feedback,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Alan Tull]

On Wed, Dec 13, 2017 at 11:27 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Wed, 13 Dec 2017 09:59:55 -0600, Alan Tull wrote:
>
>> An FPGA bridge is something that connects the FPGA fabric to the world
>> outside the FPGA.  Not every setup will have this.  During programming
>> the bridge can be disabled to protect the outside world from bad
>> randomness (the fabric may wiggle some).   It the case of full
>> reconfiguration, it would be a hardware bridge/bridges on the bus.  In
>> the case of partial reconfiguration, it would be a freeze bridge
>> surrounding the partial reconfiguration region since the same manager
>> could be used to reprogram each of several regions, but each will have
>> their own bridge.  It sounds like in your case, it's full
>> reconfiguration and either there's no bridge or it's left enabled or
>> the programming code is handling the bridge or you're enabling the
>> bridge after programming.  FPGA regions were created as a way of
>> allowing an interface to potentially work for both setups that have
>> bridges or don't have them.  This is all just to give some context as
>> to why interfaces should be added on top of a region rather than a
>> manager.  So people could use the interface whether they have a bridge
>> or not.
>
> Hum, OK. I'm not sure to grasp all the details here. In my use case, I
> don't have anything that looks like a software-controlled FPGA bridge.
> There is a memory-mapped CPLD, which provides some registers that the
> SoC uses to program the FPGA. Based on those register writes, the CPLD
> programs the FPGA.
>
>> > My use case is
>> > pretty simple:
>> >
>> >  - The SoC has a PCIE controller. The PCIE controller is a hard IP
>> >    block in the SoC.
>> >
>> >  - There is a FPGA on the board which is connected directly to the SoC
>> >    over PCIe + over a control bus that allows programming of the FPGA.
>>
>> Just to be clear, the programming is not through PCIe, right?
>
> Correct, there's a side-band channel in the form of a memory-mapped
> CPLD.
>
>> > Therefore, there is no bridge involved: once the FPGA is programmed,
>> > the PCIE controller in the SoC can enumerate and see a new PCIE device,
>> > which happens to be implemented in the FPGA.
>>
>> Yes, full reconfiguration of a FPGA that's on PCIe is a common use
>> case and one we want to support.  This subsystem started out
>> supporting embedded FPGAs so some things may need to adapt for PCIe.
>> The code on linux-next is a step towards that, but not a complete
>> solution.
>
> Could you summarize what is in linux-next and how it is a step towards
> that, and what is missing ?

fpga-region.c gains a fpga_region_register() function [1] for creating
regions, similar to the register function for bridges and managers.  A
new file of-fpga-region.c uses fpga_region_register to create a FPGA
region under device tree control.  The intent is that other code could
create fpga regions for their (non-DT) use cases and be more reusable
than if interfaces were added to FPGA manager [2].

In your case a layer could be written that would create a region which
would would just have the manager, plus an interface to trigger
programming, and some code to kick off enumerating/rescan after
successful programming.  If it were done right, someone who has a
similar use case but a different FPGA device could use that same code
but with a different manager and add a bridge if they need it.  That's
the high level view.  I understand that the DFL's use case is not
close enough to your to work for you, but they are an another example
of a layer that is creating regions, managers, bridges, plus doing
enumeration.  Anyway, I'm not suggesting that you use the DFL code,
I'm just trying to explain that now there's two upper layers that have
been implemented sitting on top of FPGA regions.  The end goal is I'd
like to see interfaces added that could be used on multiple FPGA
devices where the use case is similar.

Alan

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/fpga/fpga-region.txt

[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/fpga/overview.txt


>
> Thanks a lot for your feedback,is
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH] fpga: add simple userspace interface to trigger FPGA programming

[Philippe De Muyter]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

Hi Thomas,

On Mon, Dec 04, 2017 at 01:43:15PM +0000, Thomas Petazzoni wrote:
> The current FPGA subsystem only allows programming the FPGA bitstream
> through Device Tree overlays. This assumes that the devices inside the
> FPGA are described through a Device Tree.
> 
> However, some platforms have their FPGA connected to the main CPU over
> PCIe and the devices in the FPGA are therefore dynamically
> discoverable using the normal PCIe enumeration mechanisms. There is
> therefore no Device Tree overlay describing the devices in the
> FPGA. Furthermore, on my platform (an old SH7786), there is no Device
> Tree at all, as there is no support for Device Tree for this SoC.
> 
> Adding a userspace interface to trigger the programming of the FPGA
> has already been requested in the past (see [1]) showing that there is
> a need for such a feature.
> 
> This commit therefore introduces a very simple interface, in the form
> of a "load" sysfs file. Writing the name of the firmware file to
> program into the FPGA to this "load" file triggers the programming
> process.

Thank you for your patch.  I have used it succesfully to program and
reprogram with different bit files an 'invisible' FPGA which is actually
a piece of hardware bridging a Sub LVDS CMOS sensor output to a CSI
MIPI-2 input.

If this patch is not suitable in the general case it could at least be
made available for development context.

Attached is a small patch on top of yours that fixes a minor annoyance
when using 'echo' without the '-n' option.

Philippe
-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

[-- Attachment #2: 0001-FIX-fpga-add-simple-userspace-interface-to-trigger-F.patch --]
[-- Type: text/x-patch, Size: 1124 bytes --]

From 284a904c4f8bf3978735d234c7ee2bacfd10edaf Mon Sep 17 00:00:00 2001
From: Philippe De Muyter <phdm@macqel.be>
Date: Fri, 10 Aug 2018 09:35:41 +0200
Subject: [PATCH] FIX "fpga: add simple userspace interface to trigger FPGA
 programming"

When fpga-mgr load_store strips the received string to remove the
terminating '\n', it should still return the whole count, not the
stripped one.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>
---
 drivers/fpga/fpga-mgr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 751a447..61492787 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -404,11 +404,15 @@ static ssize_t load_store(struct device *dev,
 	struct fpga_manager *mgr = to_fpga_manager(dev);
 	char *name;
 	int ret;
+	size_t stripped_count = count;
 
 	if (count > 0 && buf[count - 1] == '\n')
-		count--;
+		stripped_count--;
 
-	name = kstrndup(buf, count, GFP_KERNEL);
+	if (!stripped_count)
+		return count;
+
+	name = kstrndup(buf, stripped_count, GFP_KERNEL);
 	if (!name)
 		return -ENOSPC;
 
-- 
2.7.4