Date:   Thu, 17 Aug 2017 12:42:22 -0700
From:   Moritz Fischer <moritz.fischer@...us.com>
To:     Alan Tull <atull@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fpga@...r.kernel.org
Subject: Re: [PATCH v3 01/16] doc: fpga: update documents for the FPGA API

Hi Alan,

I think that should go together with [3/16] in one commit,
to make API change go together with actual change?

On Thu, Jul 6, 2017 at 11:47 AM, Alan Tull <atull@...nel.org> wrote:
> The FPGA manager has been simplified to have a single
> fpga_mgr_load function which replaces the three
> fpga_mgr_*load* functions.
>
> The parameters presenting the FPGA image have been
> added to struct fpga_image_info.
>
> Additional functions have been added to alloc/free
> fpga_image_info.
>
> Getting a FPGA manager has been separated from
> locking it.  So an unlocked reference can be saved and
> only locked when we're about to program.
>
> A document for fpga-region has been added as well.
>
> Signed-off-by: Alan Tull <atull@...nel.org>
> ---
>  Documentation/fpga/fpga-mgr.txt    | 133 +++++++++++++++++++------------------
>  Documentation/fpga/fpga-region.txt |  95 ++++++++++++++++++++++++++
>  Documentation/fpga/overview.txt    |  23 +++++++
>  3 files changed, 188 insertions(+), 63 deletions(-)
>  create mode 100644 Documentation/fpga/fpga-region.txt
>  create mode 100644 Documentation/fpga/overview.txt
>
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index 78f197f..0fc41eb 100644
> --- a/Documentation/fpga/fpga-mgr.txt
> +++ b/Documentation/fpga/fpga-mgr.txt
> @@ -11,61 +11,66 @@ hidden away in a low level driver which registers a set of ops with the core.
>  The FPGA image data itself is very manufacturer specific, but for our purposes
>  it's just binary data.  The FPGA manager core won't parse it.
>
> +The FPGA image to be programmed can be in a scatter gather list, a single
> +contiguous buffer, or a firmware file.  Because allocating contiguous kernel
> +memory for the buffer should be avoided, users are encouraged to use a scatter
> +gather list instead if possible.
> +
> +The particulars for programming the image are presented in a structure (struct
> +fpga_image_info).  This struct contains parameters such as pointers to the
> +FPGA image as well as image-specific particulars such as whether the image was
> +built for full or partial reconfiguration.
>
>  API Functions:
>  ==============
>
> -To program the FPGA from a file or from a buffer:
> --------------------------------------------------
> -
> -       int fpga_mgr_buf_load(struct fpga_manager *mgr,
> -                             struct fpga_image_info *info,
> -                             const char *buf, size_t count);
> -
> -Load the FPGA from an image which exists as a contiguous buffer in
> -memory. Allocating contiguous kernel memory for the buffer should be avoided,
> -users are encouraged to use the _sg interface instead of this.
> -
> -        int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
> -                                struct fpga_image_info *info,
> -                                struct sg_table *sgt);
> +To program the FPGA:
> +--------------------
>
> -Load the FPGA from an image from non-contiguous in memory. Callers can
> -construct a sg_table using alloc_page backed memory.
> +       int fpga_mgr_load(struct fpga_manager *mgr,
> +                         struct fpga_image_info *info);
>
> -       int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> -                                  struct fpga_image_info *info,
> -                                  const char *image_name);
> -
> -Load the FPGA from an image which exists as a file.  The image file must be on
> -the firmware search path (see the firmware class documentation).  If successful,
> +Load the FPGA from an image which is indicated in the info.  If successful,
>  the FPGA ends up in operating mode.  Return 0 on success or a negative error
>  code.
>
> -A FPGA design contained in a FPGA image file will likely have particulars that
> -affect how the image is programmed to the FPGA.  These are contained in struct
> -fpga_image_info.  Currently the only such particular is a single flag bit
> -indicating whether the image is for full or partial reconfiguration.
> +To allocate or free a struct fpga_image_info:
> +---------------------------------------------
> +
> +       struct fpga_image_info *fpga_image_info_alloc(struct device *dev);
> +
> +       void fpga_image_info_free(struct device *dev,
> +                                 struct fpga_image_info *info);
>
>  To get/put a reference to a FPGA manager:
>  -----------------------------------------
>
>         struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>         struct fpga_manager *fpga_mgr_get(struct device *dev);
> +       void fpga_mgr_put(struct fpga_manager *mgr);
>
> -Given a DT node or device, get an exclusive reference to a FPGA manager.
> +Given a DT node or device, get a reference to a FPGA manager.  This pointer
> +can be saved until you are ready to program the FPGA.  fpga_mgr_put
> +releases the reference.
>
> -       void fpga_mgr_put(struct fpga_manager *mgr);
>
> -Release the reference.
> +To get exclusive control of a FPGA manager:
> +-------------------------------------------
> +
> +       int fpga_mgr_lock(struct fpga_magager *mgr);
> +       void fpga_mgr_unlock(struct fpga_magager *mgr);
> +
> +The user should call fpga_mgr_lock and verify that it returns 0 before
> +attempting to program the FPGA.  Likeqise, the user should call
> +fpga_mgr_unlock when done programming the FPGA.
>
>
>  To register or unregister the low level FPGA-specific driver:
>  -------------------------------------------------------------
>
>         int fpga_mgr_register(struct device *dev, const char *name,
> -                             const struct fpga_manager_ops *mops,
> -                             void *priv);
> +                             const struct fpga_manager_ops *mops,
> +                             void *priv);
>
>         void fpga_mgr_unregister(struct device *dev);
>
> @@ -78,59 +83,61 @@ How to write an image buffer to a supported FPGA
>  /* Include to get the API */
>  #include <linux/fpga/fpga-mgr.h>
>
> -/* device node that specifies the FPGA manager to use */
> -struct device_node *mgr_node = ...
> +struct fpga_manager *mgr;
> +struct fpga_image_info *info;
> +int ret;
>
> -/* FPGA image is in this buffer.  count is size of the buffer. */
> -char *buf = ...
> -int count = ...
> +/*
> + * Get a reference to FPGA manager.  This example uses the  device node of the
> + * manager.  You could use fpga_mgr_get() instead if you have the device instead
> + * of the device node.
> + */
> +mgr = of_fpga_mgr_get(mgr_node);
>
>  /* struct with information about the FPGA image to program. */
> -struct fpga_image_info info;
> +info = fpga_image_info_alloc(dev);
>
>  /* flags indicates whether to do full or partial reconfiguration */
> -info.flags = 0;
> +info->flags = FPGA_MGR_PARTIAL_RECONFIG;
>
> -int ret;
> +/*
> + * At this point, indicate where the image is. This is pseudo-code; you're
> + * probably going to use one of these three.
> + */
> +if (using scatter gather) {
>
> -/* Get exclusive control of FPGA manager */
> -struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
> +       info->sgt = [your scatter gather table]
>
> -/* Load the buffer to the FPGA */
> -ret = fpga_mgr_buf_load(mgr, &info, buf, count);
> -
> -/* Release the FPGA manager */
> -fpga_mgr_put(mgr);
> +} else if (using a buffer) {
>
> +       info->buf = [your image buffer]
> +       info->count = [image buffer size]
>
> -How to write an image file to a supported FPGA
> -==============================================
> -/* Include to get the API */
> -#include <linux/fpga/fpga-mgr.h>
> -
> -/* device node that specifies the FPGA manager to use */
> -struct device_node *mgr_node = ...
> +} else if (using a firmware file) {
>
> -/* FPGA image is in this file which is in the firmware search path */
> -const char *path = "fpga-image-9.rbf"
> +       info->firmware_name = devm_kstrdup(dev, firmware_name, GFP_KERNEL);
>
> -/* struct with information about the FPGA image to program. */
> -struct fpga_image_info info;
> +} else {
>
> -/* flags indicates whether to do full or partial reconfiguration */
> -info.flags = 0;
> +       not implemented!
>
> -int ret;
> +}
>
>  /* Get exclusive control of FPGA manager */
> -struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
> +ret = fpga_mgr_lock(mgr);
>
> -/* Get the firmware image (path) and load it to the FPGA */
> -ret = fpga_mgr_firmware_load(mgr, &info, path);
> +/* Load the buffer to the FPGA */
> +ret = fpga_mgr_buf_load(mgr, &info, buf, count);
>
>  /* Release the FPGA manager */
> +fpga_mgr_unlock(mgr);
>  fpga_mgr_put(mgr);
>
> +/* Free your image storage in some appropriate way */
> +...
> +
> +/* Deallocate the image info if you're done with it */
> +fpga_image_info_free(dev, info);
>
>  How to support a new FPGA device
>  ================================
> diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt
> new file mode 100644
> index 0000000..139a02b
> --- /dev/null
> +++ b/Documentation/fpga/fpga-region.txt
> @@ -0,0 +1,95 @@
> +FPGA Regions
> +
> +Alan Tull 2017
> +
> +CONTENTS
> + - Introduction
> + - The FPGA region API
> + - Usage example
> +
> +Introduction
> +============
> +
> +This document is meant to be an brief overview of the FPGA region API usage.  A
> +more conceptual look at regions can be found in [1].
> +
> +For the purposes of this API document, let's just say that a region associates
> +an FPGA Manager and a bridge (or bridges) with a reprogrammable region of an
> +FPGA or the whole FPGA.  The API provides a way to register a region and to
> +program a region.
> +
> +Currently the only layer above fpga-region.c in the kernel is the Device Tree
> +support (of-fpga-region.c) described in [1].  The DT support layer uses regions
> +to program the FPGA and then DT to handle enumeration.  The common region code
> +is intended to be used by other schemes that have other ways of accomplishing
> +enumeration after programming.
> +
> +An fpga-region can be set up to know the following things:
> +* which FPGA manager to use to do the programming
> +* which bridges to disable before programming and enable afterwards.
> +
> +Additional info needed to program the FPGA image is passed in the struct
> +fpga_image_info [2] including:
> +* pointers to the image as either a scatter-gather buffer, a contiguous
> +  buffer, or the name of firmware file
> +* flags indicating specifics such as whether the image if for partial
> +  reconfiguration.
> +
> +===================
> +The FPGA region API
> +===================
> +
> +To register or unregister a region:
> +-----------------------------------
> +
> +       int fpga_region_register(struct device *dev,
> +                                struct fpga_region *region);
> +       int fpga_region_unregister(struct fpga_region *region);
> +
> +An example of usage can be seen in the probe function of [3]
> +
> +To program an FPGA:
> +-------------------
> +       int fpga_region_program_fpga(struct fpga_region *region);
> +
> +This function operates on info passed in the fpga_image_info
> +(region->info).
> +
> +This function will attempt to:
> + * lock the region's mutex
> + * lock the region's FPGA manager
> + * build a list of FPGA bridges if a method has been specified to do so
> + * disable the bridges
> + * program the FPGA
> + * re-enable the bridges
> + * release the locks
> +
> +=============
> +Usage example
> +=============
> +
> +First, allocate the info struct:
> +
> +       info = fpga_image_info_alloc(dev);
> +       if (!info)
> +               return -ENOMEM;
> +
> +Set flags as needed, i.e.
> +
> +       info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> +
> +Point to your FPGA image, such as:
> +
> +       info->sgt = &sgt;
> +
> +Add info to region and do the programming:
> +
> +       region->info = info;
> +       ret = fpga_region_program_fpga(region);
> +
> +Then enumerate whatever hardware has appeared in the FPGA.
> +
> +--
> +[1] ../devicetree/bindings/fpga/fpga-region.txt
> +[2] ./fpga-mgr.txt
> +[3] ../../drivers/fpga/of-fpga-region.c
> diff --git a/Documentation/fpga/overview.txt b/Documentation/fpga/overview.txt
> new file mode 100644
> index 0000000..149ac8a
> --- /dev/null
> +++ b/Documentation/fpga/overview.txt
> @@ -0,0 +1,23 @@
> +Linux kernel FPGA support
> +
> +Alan Tull 2017
> +
> +The main point of this project has been to separate the out the upper layers
> +that know when to reprogram a FPGA from the lower layers that know how to
> +reprogram a specific FPGA device.  The intention is to make this manufacturor
> +agnostic, understanding that of course the FPGA images are very device specific
> +themselves.
> +
> +At this point it time, the framework in the kernel includes:
> +* low level FPGA manager drivers that know how to program a specific device
> +* the fpga-mgr framework they are registered with
> +* low level FPGA bridge drivers for hard/soft bridges which are intended to
> +  be disable during FPGA programming
> +* the fpga-bridge framework they are registered with
> +* the fpga-region framework which associates and controls managers and bridges
> +  as reconfigurable regions
> +* the of-fpga-region support for reprogramming FPGAs when device tree overlays
> +  are applied.
> +
> +I would encourage you the user to add code that creates fpga regions rather
> +that trying to control managers and bridges separately.
> --
> 2.7.4
>

Cheers,

Moritz

Powered by blists - more mailing lists