Message-ID: <20160205224446.GG9579@jcartwri.amer.corp.natinst.com>
Date:	Fri, 5 Feb 2016 16:44:46 -0600
From:	Josh Cartwright <joshc@...com>
To:	atull@...nsource.altera.com
Cc:	Rob Herring <robh+dt@...nel.org>, pantelis.antoniou@...sulko.com,
	Moritz Fischer <moritz.fischer@...us.com>,
	gregkh@...uxfoundation.org, monstr@...str.eu,
	michal.simek@...inx.com, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	delicious.quinoa@...il.com, dinguyen@...nsource.altera.com
Subject: Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

Hey Alan-

First off, thanks for all of your (and others') work on this.

On Fri, Feb 05, 2016 at 03:29:58PM -0600, atull@...nsource.altera.com wrote:
> From: Alan Tull <atull@...nsource.altera.com>
> 
> New bindings document for FPGA Region to support programming
> FPGA's under Device Tree control
> 
> Signed-off-by: Alan Tull <atull@...nsource.altera.com>
> Signed-off-by: Moritz Fischer <moritz.fischer@...us.com>
[..]
> ---
>  .../devicetree/bindings/fpga/fpga-region.txt       |  348 ++++++++++++++++++++
>  1 file changed, 348 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> new file mode 100644
> index 0000000..ccd7127
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
[..]
> +FPGA Manager & FPGA Manager Framework
> + * An FPGA Manager is a hardware block that programs an FPGA under the control
> +   of a host processor.
> + * The FPGA Manager Framework provides drivers and functions to program an
> +   FPGA.
> +
> +FPGA Bridge Framework
> + * Provides drivers and functions to control bridges that enable/disable
> +   data to the FPGA.
> + * FPGA Bridges should be disabled while the FPGA is being programmed to
> +   prevent spurious data on the bus.
> + * FPGA Bridges may not be needed in implementations where the FPGA Manager
> +   handles this.

It still seems strange for me architecturally for the FPGA Bridge to be
a first-class top-level concept in your architecture, as they are a
reflection of the SoC FPGA manager design.  That is, I would expect the
bridges not to be associated with the FPGA Region, but with the FPGA
manager.

Although, I will concede that you you've made it possible to not use
FPGA Bridges (like on Zynq where they aren't necessary), so maybe it
doesn't matter, just smells strangely.

> +Freeze Blocks
> + * Freeze Blocks function as FPGA Bridges within the FPGA fabric.  In the case
> +   of PR, the buses from the processor are split within the FPGA.  Each PR
> +   region gets its own split of the buses, protected by an independently
> +   controlled Freeze Block.  Several busses may be connected to a single
> +   PR region; a Freeze Block controls the traffic of all these busses
> +   together.
> +
> +
[..]
> +Device Tree Examples
> +====================
> +
> +The intention of this section is to give some simple examples, focusing on
> +the placement of the elements detailed above, especially:
> + * FPGA Manager
> + * FPGA Bridges
> + * FPGA Region
> + * ranges
> + * target-path or target
> +
> +For the purposes of this section, I'm dividing the Device Tree into two parts,
> +each with its own requirements.  The two parts are:
> + * The live DT prior to the overlay being added
> + * The DT overlay
> +
> +The live Device Tree must contain an FPGA Region, an FPGA Manager, and any FPGA
> +Bridges.  The FPGA Region's "fpga-mgr" property specifies the manager by phandle
> +to handle programming the FPGA.  If the FPGA Region is the child of another FPGA
> +Region, the parent's FPGA Manager is used.  If FPGA Bridges need to be involved,
> +they are specified in the FPGA Region by the "fpga-bridges" property.  During
> +FPGA programming, the FPGA Region will disable the bridges that are in its
> +"fpga-bridges" list and will re-enable them after FPGA programming has
> +succeeded.
> +
> +The Device Tree Overlay will contain:
> + * "target-path" or "target"
> +   The insertion point where the the contents of the overlay will go into the
> +   live tree.  target-path is a full path, while target is a phandle.
> + * "ranges"
> + * "firmware-name"
> +   Specifies the name of the FPGA image file on the firmware search
> +   path.  The search path is described in the firmware class documentation.
> + * "partial-reconfig"
> +   This binding is a boolean and should be present if partial reconfiguration
> +   is to be done.

Another architectural smell: there are categorically two different types
of properties here.  The first kind is those properties which describe
_what_ IP exists within the FPGA image (all of the nodes under the regions, etc).
The second kind of properties are those which describe _how_ the image
should be written (partial-reconfig, firmware-name).

It seems weird, but maybe it doesn't matter much.

Thanks,
  Josh

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists