Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

Josh Cartwright
2016-02-05T22:45:07Z

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

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@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> New bindings document for FPGA Region to support programming
> FPGA's under Device Tree control
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.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

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