From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Michal Simek <michal.simek@amd.com>,
linux-kernel@vger.kernel.org, monstr@monstr.eu,
michal.simek@xilinx.com, git@xilinx.com
Cc: Conor Dooley <conor+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Moritz Fischer <mdf@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Tom Rix <trix@redhat.com>, Wu Hao <hao.wu@intel.com>,
Xu Yilun <yilun.xu@intel.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
"open list:FPGA MANAGER FRAMEWORK" <linux-fpga@vger.kernel.org>
Subject: Re: [PATCH] dt-bindings: fpga: Conver fpga-region binding to yaml
Date: Mon, 15 Jan 2024 16:15:03 +0100 [thread overview]
Message-ID: <c3c92468-a1db-498b-b4a2-7926b84cb5ea@linaro.org> (raw)
In-Reply-To: <39e1362352959706b1f67c6feaeec2639ead4610.1705062319.git.michal.simek@amd.com>
On 12/01/2024 13:25, Michal Simek wrote:
> Convert the generic fpga region DT binding to json-schema.
> There are some differences compare to txt version.
> 1. DT overlay can't be described in example that's why directly include
> information from overlay to node which was referenced. It is visible in
> example with /* DT Overlay contains: &... */
>
> 2. All example have been rewritten to be simpler and describe only full
> reconfiguration and partial reconfiguration with one bridge.
> Completely drop the case where fpga region can inside partial
> reconfiguration region which is already described in description
>
> 3. Fixed some typos in descriptions compare to txt version but most of it
> is just c&p from txt file.
>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
>
> Please let me know if there is a way to describe overlays to dt root to be
> able to reference fpga region back.
> ---
...
> ---
> -[1] www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_partrecon.pdf
> -[2] tspace.library.utoronto.ca/bitstream/1807/67932/1/Byma_Stuart_A_201411_MAS_thesis.pdf
> -[3] https://www.xilinx.com/support/documentation/sw_manuals/xilinx14_1/ug702.pdf
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.yaml b/Documentation/devicetree/bindings/fpga/fpga-region.yaml
> new file mode 100644
> index 000000000000..bf4bb54bfbfa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.yaml
> @@ -0,0 +1,372 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
You relicense the GPL-2.0, so you need to ask all copyright holders to
ack your patch. At least try to identify them...
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/fpga-region.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: FPGA Region
> +
> +maintainers:
> + - Michal Simek <michal.simek@amd.com>
> +
> +description: |
> + CONTENTS
> + - Introduction
> + - Terminology
> + - Sequence
> + - FPGA Region
> + - Supported Use Models
> + - Constraints
...
> + Constraints
> + ===========
> +
> + It is beyond the scope of this document to fully describe all the FPGA design
> + constraints required to make partial reconfiguration work[1] [2] [3], but a few
> + deserve quick mention.
> +
> + A persona must have boundary connections that line up with those of the partition
> + or region it is designed to go into.
> +
> + During programming, transactions through those connections must be stopped and
> + the connections must be held at a fixed logic level. This can be achieved by
> + FPGA Bridges that exist on the FPGA fabric prior to the partial reconfiguration.
> +
> + --
> + [1] www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_partrecon.pdf
> + [2] tspace.library.utoronto.ca/bitstream/1807/67932/1/Byma_Stuart_A_201411_MAS_thesis.pdf
> + [3] https://www.xilinx.com/support/documentation/sw_manuals/xilinx14_1/ug702.pdf
> +
> +properties:
> + $nodename:
> + pattern: "^fpga-region([0-9]|[1-9][0-9]+)?$"
Missing -:
"^fpga-region(-([0-9]|[1-9][0-9]+))?$"
> +
> + compatible:
> + const: fpga-region
> +
> + ranges: true
This does not feel right - you do not allow any unit address, so ranges
should cause warnings.
> + "#address-cells": true
> + "#size-cells": true
> +
> + config-complete-timeout-us:
> + description:
> + The maximum time in microseconds time for the FPGA to go to operating
> + mode after the region has been programmed.
> +
> + encrypted-fpga-config:
> + type: boolean
> + description:
> + Set if the bitstream is encrypted.
> +
> + external-fpga-config:
> + type: boolean
> + description:
> + Set if the FPGA has already been configured prior to OS boot up.
> +
> + firmware-name:
> + $ref: /schemas/types.yaml#/definitions/string
This already has a type: string-array. You need maxItems instead.
> + description:
> + Should contain the name of an FPGA image file located on the firmware
> + search path. If this property shows up in a live device tree it indicates
> + that the FPGA has already been programmed with this image.
> + If this property is in an overlay targeting an FPGA region, it is
> + a request to program the FPGA with that image.
> +
> + fpga-bridges:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + Should contain a list of phandles to FPGA Bridges that must be
This says list of phandles, but you allow only one.
> + controlled during FPGA programming along with the parent FPGA bridge.
> + This property is optional if the FPGA Manager handles the bridges.
> + If the fpga-region is the child of an fpga-bridge, the list should not
> + contain the parent bridge.
> +
> + fpga-mgr:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + Should contain a phandle to an FPGA Manager. Child FPGA Regions
> + inherit this property from their ancestor regions. An fpga-mgr property
> + in a region will override any inherited FPGA manager.
> +
> + partial-fpga-config:
> + type: boolean
> + description:
> + Set if partial reconfiguration is to be done, otherwise full
> + reconfiguration is done.
> +
> + region-freeze-timeout-us:
> + description:
> + The maximum time in microseconds to wait for bridges to successfully
> + become disabled before the region has been programmed.
> +
> + region-unfreeze-timeout-us:
> + description:
> + The maximum time in microseconds to wait for bridges to successfully
> + become enabled after the region has been programmed.
> +
> +patternProperties:
> + "@[0-9a-f]+$":
> + type: object
> +
> +required:
> + - compatible
> + - ranges
> + - "#address-cells"
> + - "#size-cells"
> + - fpga-mgr
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + /*
> + * Full Reconfiguration without Bridges
> + */
> + fpga_mgr0: fpga-mgr@f8007000 {
> + compatible = "xlnx,zynq-devcfg-1.0";
> + reg = <0xf8007000 0x100>;
> + clocks = <&clkc 12>;
> + clock-names = "ref_clk";
> + interrupt-parent = <&intc>;
> + interrupts = <0 8 4>;
> + syscon = <&slcr>;
> + };
> +
> + fpga_region0: fpga-region0 {
fpga-region {
> + compatible = "fpga-region";
> + ranges;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + fpga-mgr = <&fpga_mgr0>;
> +
> + /* DT Overlay contains: &fpga_region0 */
> + firmware-name = "zynq-gpio.bin";
> + gpio@40000000 {
> + compatible = "xlnx,xps-gpio-1.00.a";
> + reg = <0x40000000 0x10000>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + };
> + };
> +
> + - |
> + /*
> + * Partial reconfiguration with Bridges
> + */
> + fpga_mgr1: fpga-mgr@f8007000 {
> + compatible = "xlnx,zynq-devcfg-1.0";
> + reg = <0xf8007000 0x100>;
> + clocks = <&clkc 12>;
> + clock-names = "ref_clk";
> + interrupt-parent = <&intc>;
> + interrupts = <0 8 4>;
> + syscon = <&slcr>;
> + };
> +
> + fpga_bridge1: fpga-bridge@100000450 {
> + compatible = "xlnx,pr-decoupler-1.00", "xlnx,pr-decoupler";
> + reg = <0x10000045 0x10>;
> + clocks = <&clkc 15>;
> + clock-names = "aclk";
> + };
> +
> + fpga_region1: fpga-region1 {
fpga-region {
> + compatible = "fpga-region";
Best regards,
Krzysztof
prev parent reply other threads:[~2024-01-15 15:15 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-12 12:25 [PATCH] dt-bindings: fpga: Conver fpga-region binding to yaml Michal Simek
2024-01-15 15:15 ` Krzysztof Kozlowski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c3c92468-a1db-498b-b4a2-7926b84cb5ea@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=git@xilinx.com \
--cc=hao.wu@intel.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mdf@kernel.org \
--cc=michal.simek@amd.com \
--cc=michal.simek@xilinx.com \
--cc=monstr@monstr.eu \
--cc=robh+dt@kernel.org \
--cc=trix@redhat.com \
--cc=yilun.xu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).