diff mbox series

[v4,2/3] dt-bindings: fpga: Add Efinix SPI programming bindings

Message ID 20250228094732.54642-3-iansdannapel@gmail.com (mailing list archive)
State New
Headers show
Series Add Efinix FPGA SPI programming support | expand

Commit Message

Ian Dannapel Feb. 28, 2025, 9:47 a.m. UTC 
From: Ian Dannapel <iansdannapel@gmail.com>

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

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

Comments

Conor Dooley Feb. 28, 2025, 6:28 p.m. UTC | #1 
On Fri, Feb 28, 2025 at 10:47:31AM +0100, iansdannapel@gmail.com wrote:
> From: Ian Dannapel <iansdannapel@gmail.com>
> 
> Add device tree bindings documentation for configuring Efinix FPGA
> using serial SPI passive programming mode.
> 
> Signed-off-by: Ian Dannapel <iansdannapel@gmail.com>
> ---
>  .../devicetree/bindings/fpga/efinix,spi.yaml  | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/efinix,spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/fpga/efinix,spi.yaml b/Documentation/devicetree/bindings/fpga/efinix,spi.yaml
> new file mode 100644
> index 000000000000..145c96f38e45
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/efinix,spi.yaml

Filename matching a compatible please.

> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/efinix,spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Efinix SPI FPGA Manager
> +
> +maintainers:
> +  - Ian Dannapel <iansdannapel@gmail.com>
> +
> +description: |
> +  Efinix FPGAs (Trion, Topaz, and Titanium families) support loading bitstreams
> +  through "SPI Passive Mode".
> +  Note 1: Only bus width 1x is supported.
> +  Note 2: Additional pins hogs for bus width configuration must be set
> +  elsewhere, if necessary.
> +  Note 3: Topaz and Titanium support is based on documentation but remains
> +  untested.

Points 1 and 3 here seem to be driver limitations, and shouldn't really
be present in a document describing the hardware?

> +
> +  References:
> +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.3.pdf
> +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.8.pdf
> +  - https://www.efinixinc.com/docs/an061-configuring-topaz-fpgas-v1.1.pdf
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - efinix,trion-spi
> +      - efinix,titanium-spi
> +      - efinix,topaz-spi

> +      - efinix,fpga-spi

What hardware does this device represent? Other ones are obvious matches
to the families you mention, but what is this one?

Cheers,
Conor.

> +
> +  spi-cpha: true
> +
> +  spi-cpol: true
> +
> +  spi-max-frequency:
> +    maximum: 25000000
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      reset and re-configuration trigger pin (low active)
> +    maxItems: 1
> +
> +  cdone-gpios:
> +    description:
> +      optional configuration done status pin (high active)
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> +      fpga-mgr@0 {
> +        compatible = "efinix,trion-spi";
> +        reg = <0>;
> +        spi-max-frequency = <25000000>;
> +        spi-cpha;
> +        spi-cpol;
> +        reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> +        cdone-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> +      };
> +    };
> +...
> -- 
> 2.43.0
>
Krzysztof Kozlowski March 1, 2025, 1:13 p.m. UTC | #2 
On 28/02/2025 10:47, iansdannapel@gmail.com wrote:
> +
> +  References:
> +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.3.pdf
> +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.8.pdf
> +  - https://www.efinixinc.com/docs/an061-configuring-topaz-fpgas-v1.1.pdf
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - efinix,trion-spi
> +      - efinix,titanium-spi
> +      - efinix,topaz-spi


Same comments as before about compatibility. Address or implement.

> +      - efinix,fpga-spi


And this one is for which device? It is not even used.


Best regards,
Krzysztof
Ian Dannapel March 3, 2025, 10:10 a.m. UTC | #3 
Hi Conor, thanks for the quick response.

On Fri, Feb 28, 2025 at 7:28 PM Conor Dooley <conor@kernel.org> wrote:
> > +description: |
> > +  Efinix FPGAs (Trion, Topaz, and Titanium families) support loading bitstreams
> > +  through "SPI Passive Mode".
> > +  Note 1: Only bus width 1x is supported.
> > +  Note 2: Additional pins hogs for bus width configuration must be set
> > +  elsewhere, if necessary.
> > +  Note 3: Topaz and Titanium support is based on documentation but remains
> > +  untested.
>
> Points 1 and 3 here seem to be driver limitations, and shouldn't really
> be present in a document describing the hardware?
>
Yes, they are driver limitations and probably do not belong here.

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - efinix,trion-spi
> > +      - efinix,titanium-spi
> > +      - efinix,topaz-spi
>
> > +      - efinix,fpga-spi
>
> What hardware does this device represent? Other ones are obvious matches
> to the families you mention, but what is this one?
The proposed compatible is a generic fallback for any Efinix FPGA Series.

Regards
Ian
Ian Dannapel March 3, 2025, 10:29 a.m. UTC | #4 
Hi Krzysztof, thanks for the quick response.

On Sat, Mar 1, 2025 at 2:13 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 28/02/2025 10:47, iansdannapel@gmail.com wrote:
> > +
> > +  References:
> > +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.3.pdf
> > +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.8.pdf
> > +  - https://www.efinixinc.com/docs/an061-configuring-topaz-fpgas-v1.1.pdf
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - efinix,trion-spi
> > +      - efinix,titanium-spi
> > +      - efinix,topaz-spi
>
>
> Same comments as before about compatibility. Address or implement.
>
The compatibles are implemented in the device match table, what
exactly should be addressed or implemented here?

> > +      - efinix,fpga-spi
>
>
> And this one is for which device? It is not even used.
The proposed compatible is a generic fallback for any Efinix FPGA
Series. Isn't it used if the compatible is part of the drivers match
table?

Regards,
Ian
Conor Dooley March 3, 2025, 10:31 a.m. UTC | #5 
On Mon, Mar 03, 2025 at 11:10:53AM +0100, Ian Dannapel wrote:
> Hi Conor, thanks for the quick response.
> 
> On Fri, Feb 28, 2025 at 7:28 PM Conor Dooley <conor@kernel.org> wrote:
> > > +description: |
> > > +  Efinix FPGAs (Trion, Topaz, and Titanium families) support loading bitstreams
> > > +  through "SPI Passive Mode".
> > > +  Note 1: Only bus width 1x is supported.
> > > +  Note 2: Additional pins hogs for bus width configuration must be set
> > > +  elsewhere, if necessary.
> > > +  Note 3: Topaz and Titanium support is based on documentation but remains
> > > +  untested.
> >
> > Points 1 and 3 here seem to be driver limitations, and shouldn't really
> > be present in a document describing the hardware?
> >
> Yes, they are driver limitations and probably do not belong here.
> 
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - efinix,trion-spi
> > > +      - efinix,titanium-spi
> > > +      - efinix,topaz-spi
> >
> > > +      - efinix,fpga-spi
> >
> > What hardware does this device represent? Other ones are obvious matches
> > to the families you mention, but what is this one?

> The proposed compatible is a generic fallback for any Efinix FPGA Series.

If it is a fallback, your binding should look like:
compatible:
  items:
    - enum:
        - efinix,trion-spi
        - efinix,titanium-spi
        - efinix,topaz-spi
    - const: efinix,fpga-spi

|+static const struct of_device_id efinix_spi_of_match[] = {
|+       { .compatible = "efinix,trion-spi", },
|+       { .compatible = "efinix,titanium-spi", },
|+       { .compatible = "efinix,topaz-spi", },

And these three compatibles can/should be removed from the driver, since
the fallback is required.

|+       { .compatible = "efinix,fpga-spi", },
|+       {}
|+};
Krzysztof Kozlowski March 3, 2025, 10:33 a.m. UTC | #6 
On 03/03/2025 11:29, Ian Dannapel wrote:
> Hi Krzysztof, thanks for the quick response.
> 
> On Sat, Mar 1, 2025 at 2:13 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 28/02/2025 10:47, iansdannapel@gmail.com wrote:
>>> +
>>> +  References:
>>> +  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.3.pdf
>>> +  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.8.pdf
>>> +  - https://www.efinixinc.com/docs/an061-configuring-topaz-fpgas-v1.1.pdf
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - efinix,trion-spi
>>> +      - efinix,titanium-spi
>>> +      - efinix,topaz-spi
>>
>>
>> Same comments as before about compatibility. Address or implement.
>>
> The compatibles are implemented in the device match table, what
> exactly should be addressed or implemented here?

Comments from previous revision - they look compatible, so define this
as list of compatibles where one is of above is used as fallback. See
example schema or 90% of other bindings (oneOf:).


> 
>>> +      - efinix,fpga-spi
>>
>>
>> And this one is for which device? It is not even used.
> The proposed compatible is a generic fallback for any Efinix FPGA

You do not use here fallback at all. Fallback means last compatible in
the list, but you have here only one item.

> Series. Isn't it used if the compatible is part of the drivers match
> table?

Drop, compatibles are supposed to be specific.


Best regards,
Krzysztof
Krzysztof Kozlowski March 3, 2025, 10:34 a.m. UTC | #7 
On 03/03/2025 11:31, Conor Dooley wrote:
> On Mon, Mar 03, 2025 at 11:10:53AM +0100, Ian Dannapel wrote:
>> Hi Conor, thanks for the quick response.
>>
>> On Fri, Feb 28, 2025 at 7:28 PM Conor Dooley <conor@kernel.org> wrote:
>>>> +description: |
>>>> +  Efinix FPGAs (Trion, Topaz, and Titanium families) support loading bitstreams
>>>> +  through "SPI Passive Mode".
>>>> +  Note 1: Only bus width 1x is supported.
>>>> +  Note 2: Additional pins hogs for bus width configuration must be set
>>>> +  elsewhere, if necessary.
>>>> +  Note 3: Topaz and Titanium support is based on documentation but remains
>>>> +  untested.
>>>
>>> Points 1 and 3 here seem to be driver limitations, and shouldn't really
>>> be present in a document describing the hardware?
>>>
>> Yes, they are driver limitations and probably do not belong here.
>>
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - efinix,trion-spi
>>>> +      - efinix,titanium-spi
>>>> +      - efinix,topaz-spi
>>>
>>>> +      - efinix,fpga-spi
>>>
>>> What hardware does this device represent? Other ones are obvious matches
>>> to the families you mention, but what is this one?
> 
>> The proposed compatible is a generic fallback for any Efinix FPGA Series.
> 
> If it is a fallback, your binding should look like:
> compatible:
>   items:
>     - enum:
>         - efinix,trion-spi
>         - efinix,titanium-spi
>         - efinix,topaz-spi
>     - const: efinix,fpga-spi
> 
> |+static const struct of_device_id efinix_spi_of_match[] = {
> |+       { .compatible = "efinix,trion-spi", },
> |+       { .compatible = "efinix,titanium-spi", },
> |+       { .compatible = "efinix,topaz-spi", },
> 
> And these three compatibles can/should be removed from the driver, since
> the fallback is required.
> 
> |+       { .compatible = "efinix,fpga-spi", },
> |+       {}

Yes, except that one of the devices should be the fallback, not generic
"fpga".


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/fpga/efinix,spi.yaml b/Documentation/devicetree/bindings/fpga/efinix,spi.yaml
new file mode 100644
index 000000000000..145c96f38e45
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/efinix,spi.yaml
@@ -0,0 +1,81 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/efinix,spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Efinix SPI FPGA Manager
+
+maintainers:
+  - Ian Dannapel <iansdannapel@gmail.com>
+
+description: |
+  Efinix FPGAs (Trion, Topaz, and Titanium families) support loading bitstreams
+  through "SPI Passive Mode".
+  Note 1: Only bus width 1x is supported.
+  Note 2: Additional pins hogs for bus width configuration must be set
+  elsewhere, if necessary.
+  Note 3: Topaz and Titanium support is based on documentation but remains
+  untested.
+
+  References:
+  - https://www.efinixinc.com/docs/an006-configuring-trion-fpgas-v6.3.pdf
+  - https://www.efinixinc.com/docs/an033-configuring-titanium-fpgas-v2.8.pdf
+  - https://www.efinixinc.com/docs/an061-configuring-topaz-fpgas-v1.1.pdf
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - efinix,trion-spi
+      - efinix,titanium-spi
+      - efinix,topaz-spi
+      - efinix,fpga-spi
+
+  spi-cpha: true
+
+  spi-cpol: true
+
+  spi-max-frequency:
+    maximum: 25000000
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      reset and re-configuration trigger pin (low active)
+    maxItems: 1
+
+  cdone-gpios:
+    description:
+      optional configuration done status pin (high active)
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
+      fpga-mgr@0 {
+        compatible = "efinix,trion-spi";
+        reg = <0>;
+        spi-max-frequency = <25000000>;
+        spi-cpha;
+        spi-cpol;
+        reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
+        cdone-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
+      };
+    };
+...