diff mbox series

[2/3] dt-bindings: fpga: Add Efinix serial SPI programming binding description

Message ID 20240620144440.125374-1-iansdannapel@gmail.com (mailing list archive)
State New
Headers show
Series [1/3] fpga: Add Efinix Trion & Titanium serial SPI programming driver | expand

Commit Message

Ian Dannapel June 20, 2024, 2:44 p.m. UTC 
From: Ian Dannapel <iansdannapel@gmail.com>

Add device tree binding documentation for configuring Efinix FPGA
using serial SPI passive programming mode.

Signed-off-by: Ian Dannapel <iansdannapel@gmail.com>
---
 .../bindings/fpga/efnx,fpga-passive-spi.yaml  | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml

Comments

Conor Dooley June 20, 2024, 3:47 p.m. UTC | #1 
On Thu, Jun 20, 2024 at 04:44:40PM +0200, iansdannapel@gmail.com wrote:
> From: Ian Dannapel <iansdannapel@gmail.com>
> 
> Add device tree binding documentation for configuring Efinix FPGA
> using serial SPI passive programming mode.
> 
> Signed-off-by: Ian Dannapel <iansdannapel@gmail.com>
> ---
>  .../bindings/fpga/efnx,fpga-passive-spi.yaml  | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> new file mode 100644
> index 000000000000..855ceb3b89e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/efnx,fpga-passive-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Efinix SPI FPGA Manager
> +
> +description: |
> +  Efinix Trion and Titanium Series FPGAs support a method of loading the
> +  bitstream over what is referred to as "SPI Passive Programming".
> +  Only serial (1x bus width) is supported, setting the programming mode
> +  is not in the scope the this manager and must be done elsewhere.
> +
> +  References:
> +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.6.pdf
> +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.0.pdf
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - efnx,fpga-spi-passive

Ahh, here is the user. Can you please add specific compatibles for the
Trion and Titanium series FPGAs? And when you do, make the filename
match a compatible please.

Additionally, why "efnx" and not "efinix"?

> +
> +  spi-cpha: true
> +  spi-cpol: true
> +
> +  spi-max-frequency:
> +    maximum: 25000000
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      reset pin (low active)
> +    maxItems: 1
> +
> +  cs-gpios:
> +    description:
> +      chip-select pin (low active)
> +    maxItems: 1
> +
> +  done-gpios:
> +    description:
> +      optional programming done pin, referred as CDONE (high active)

Why not call it "cdone-gpios" if that;s what it is referred to as?

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - cs-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    &spi2 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fpga_mgr_spi: fpga-mgr@0 {
> +        compatible = "efnx,fpga-spi-passive";
> +        spi-max-frequency = <25000000>;
> +        spi-cpha;
> +        spi-cpol;
> +        reg = <0>;

order of compatible, reg, others here please.

> +        reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> +        cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> +        done-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> +      };
> +    };
> +...
> -- 
> 2.34.1
>
Rob Herring (Arm) June 20, 2024, 4:26 p.m. UTC | #2 
On Thu, 20 Jun 2024 16:44:40 +0200, iansdannapel@gmail.com wrote:
> From: Ian Dannapel <iansdannapel@gmail.com>
> 
> Add device tree binding documentation for configuring Efinix FPGA
> using serial SPI passive programming mode.
> 
> Signed-off-by: Ian Dannapel <iansdannapel@gmail.com>
> ---
>  .../bindings/fpga/efnx,fpga-passive-spi.yaml  | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml: 'maintainers' is a required property
	hint: Metaschema for devicetree binding documentation
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
Error: Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.example.dts:18.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240620144440.125374-1-iansdannapel@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Alexander Dahl June 21, 2024, 8:11 a.m. UTC | #3 
Hello,

Am Thu, Jun 20, 2024 at 04:47:41PM +0100 schrieb Conor Dooley:
> On Thu, Jun 20, 2024 at 04:44:40PM +0200, iansdannapel@gmail.com wrote:
> > From: Ian Dannapel <iansdannapel@gmail.com>
> > 
> > Add device tree binding documentation for configuring Efinix FPGA
> > using serial SPI passive programming mode.
> > 
> > Signed-off-by: Ian Dannapel <iansdannapel@gmail.com>
> > ---
> >  .../bindings/fpga/efnx,fpga-passive-spi.yaml  | 76 +++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> > new file mode 100644
> > index 000000000000..855ceb3b89e8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/fpga/efnx,fpga-passive-spi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Efinix SPI FPGA Manager
> > +
> > +description: |
> > +  Efinix Trion and Titanium Series FPGAs support a method of loading the
> > +  bitstream over what is referred to as "SPI Passive Programming".
> > +  Only serial (1x bus width) is supported, setting the programming mode
> > +  is not in the scope the this manager and must be done elsewhere.
> > +
> > +  References:
> > +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.6.pdf
> > +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.0.pdf
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - efnx,fpga-spi-passive
> 
> Ahh, here is the user. Can you please add specific compatibles for the
> Trion and Titanium series FPGAs? And when you do, make the filename
> match a compatible please.
> 
> Additionally, why "efnx" and not "efinix"?

FWIW, there already is "altr,fpga-passive-serial" for Altera devices.

Not sure why Altera got this short vendor prefix, but that was 2013
with commit 5db17a71a526 ("of: add vendor prefix for Altera Corp.")
and we probably never know?

The method of transferring the configuration data over SPI into the
FPGA is comparable.  I would go so far to claim a single driver could
support both device families for passive configuration over SPI.  I've
done that in a non-public driver for U-Boot few months ago, and used
"efinix,fpga-passive-serial" as a compatible there.  The difference is
basically Altera requiring more GPIOs considered, and Efinix keeping
the SPI clock on for some time after data is already transfered.

Greets
Alex

> 
> > +
> > +  spi-cpha: true
> > +  spi-cpol: true
> > +
> > +  spi-max-frequency:
> > +    maximum: 25000000
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description:
> > +      reset pin (low active)
> > +    maxItems: 1
> > +
> > +  cs-gpios:
> > +    description:
> > +      chip-select pin (low active)
> > +    maxItems: 1
> > +
> > +  done-gpios:
> > +    description:
> > +      optional programming done pin, referred as CDONE (high active)
> 
> Why not call it "cdone-gpios" if that;s what it is referred to as?
> 
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reset-gpios
> > +  - cs-gpios
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    &spi2 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      fpga_mgr_spi: fpga-mgr@0 {
> > +        compatible = "efnx,fpga-spi-passive";
> > +        spi-max-frequency = <25000000>;
> > +        spi-cpha;
> > +        spi-cpol;
> > +        reg = <0>;
> 
> order of compatible, reg, others here please.
> 
> > +        reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > +        cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> > +        done-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> > +      };
> > +    };
> > +...
> > -- 
> > 2.34.1
> >
Conor Dooley June 21, 2024, 10:42 a.m. UTC | #4 
On Fri, Jun 21, 2024 at 10:11:24AM +0200, Alexander Dahl wrote:
> Am Thu, Jun 20, 2024 at 04:47:41PM +0100 schrieb Conor Dooley:
> > On Thu, Jun 20, 2024 at 04:44:40PM +0200, iansdannapel@gmail.com wrote:
> > > From: Ian Dannapel <iansdannapel@gmail.com>
> > > 
> > > Add device tree binding documentation for configuring Efinix FPGA
> > > using serial SPI passive programming mode.
> > > 
> > > Signed-off-by: Ian Dannapel <iansdannapel@gmail.com>
> > > ---
> > >  .../bindings/fpga/efnx,fpga-passive-spi.yaml  | 76 +++++++++++++++++++
> > >  1 file changed, 76 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> > > new file mode 100644
> > > index 000000000000..855ceb3b89e8
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
> > > @@ -0,0 +1,76 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/fpga/efnx,fpga-passive-spi.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Efinix SPI FPGA Manager
> > > +
> > > +description: |
> > > +  Efinix Trion and Titanium Series FPGAs support a method of loading the
> > > +  bitstream over what is referred to as "SPI Passive Programming".
> > > +  Only serial (1x bus width) is supported, setting the programming mode
> > > +  is not in the scope the this manager and must be done elsewhere.
> > > +
> > > +  References:
> > > +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.6.pdf
> > > +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.0.pdf
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - efnx,fpga-spi-passive
> > 
> > Ahh, here is the user. Can you please add specific compatibles for the
> > Trion and Titanium series FPGAs? And when you do, make the filename
> > match a compatible please.
> > 
> > Additionally, why "efnx" and not "efinix"?
> 
> FWIW, there already is "altr,fpga-passive-serial" for Altera devices.
> 
> Not sure why Altera got this short vendor prefix, but that was 2013
> with commit 5db17a71a526 ("of: add vendor prefix for Altera Corp.")
> and we probably never know?

I think that was the stock ticker name for Altera.

> The method of transferring the configuration data over SPI into the
> FPGA is comparable.  I would go so far to claim a single driver could
> support both device families for passive configuration over SPI.  I've
> done that in a non-public driver for U-Boot few months ago, and used
> "efinix,fpga-passive-serial" as a compatible there.  The difference is
> basically Altera requiring more GPIOs considered, and Efinix keeping
> the SPI clock on for some time after data is already transfered.

That'd prob be helpful to comment on the driver patch. I'd still like to
see device specific compatibles here for the trion and titanium though.

Thanks,
Conor.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
new file mode 100644
index 000000000000..855ceb3b89e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/efnx,fpga-passive-spi.yaml
@@ -0,0 +1,76 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/efnx,fpga-passive-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Efinix SPI FPGA Manager
+
+description: |
+  Efinix Trion and Titanium Series FPGAs support a method of loading the
+  bitstream over what is referred to as "SPI Passive Programming".
+  Only serial (1x bus width) is supported, setting the programming mode
+  is not in the scope the this manager and must be done elsewhere.
+
+  References:
+  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.6.pdf
+  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.0.pdf
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - efnx,fpga-spi-passive
+
+  spi-cpha: true
+  spi-cpol: true
+
+  spi-max-frequency:
+    maximum: 25000000
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      reset pin (low active)
+    maxItems: 1
+
+  cs-gpios:
+    description:
+      chip-select pin (low active)
+    maxItems: 1
+
+  done-gpios:
+    description:
+      optional programming done pin, referred as CDONE (high active)
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - cs-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    &spi2 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fpga_mgr_spi: fpga-mgr@0 {
+        compatible = "efnx,fpga-spi-passive";
+        spi-max-frequency = <25000000>;
+        spi-cpha;
+        spi-cpol;
+        reg = <0>;
+        reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
+        cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
+        done-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
+      };
+    };
+...