----- On Jan 30, 2024, at 2:52 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:

On 29/01/2024 23:56, Charles Perry wrote:

quoted

Document the slave SelectMAP interface of Xilinx 7 series FPGA.

Signed-off-by: Charles Perry <redacted>
---
 .../fpga/xlnx,fpga-slave-selectmap.yaml       | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644
 Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml

diff --git
a/Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml
b/Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml
new file mode 100644
index 0000000000000..20cea24e3e39a

--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml

@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/xlnx,fpga-slave-selectmap.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Slave SelectMAP FPGA
+
+description: |
+  Xilinx 7 Series FPGAs support a method of loading the bitstream over a
+  parallel port named the slave SelectMAP interface in the documentation. Only
+  the x8 mode is supported where data is loaded at one byte per rising edge of
+  the clock, with the MSB of each byte presented to the D0 pin.
+
+  Datasheets:
+

https://www.xilinx.com/support/documentation/user_guides/ug470_7Series_Config.pdf
+
+properties:
+  compatible:
+    enum:
+      - xlnx,fpga-slave-selectmap

You did not test bindings, so only limited review.

I had issues installing pylibfdt but that's fixed now, will do.

quoted

+
+  reg:
+    description:
+      At least 1 byte of memory mapped IO
+    maxItems: 1
+
+  prog_b-gpios:

No underscores in names.

This is heavily based on "xlnx,fpga-slave-serial.yaml" which uses an underscore.
I can use a dash instead but that would make things inconsistent across the two schemas. 

quoted

+    description:
+      config pin (referred to as PROGRAM_B in the manual)
+    maxItems: 1
+
+  done-gpios:
+    description:
+      config status pin (referred to as DONE in the manual)
+    maxItems: 1
+
+  init-b-gpios:

Is there init-a? Open other bindings and look how these are called there.

No, the "-b" is there to denote that the signal is active low. I think its shorthand
for "bar" which is the overline (‾) that electronic engineer put on top of the name of the
signal on schematics. It comes from the datasheet.

quoted

+    description:
+      initialization status and configuration error pin
+      (referred to as INIT_B in the manual)
+    maxItems: 1
+
+  csi-b-gpios:

Where is csi-a?

No "csi-a", this is the CSI signal which is active low.

quoted

+    description:
+      chip select pin (referred to as CSI_B in the manual)
+      Optional gpio for if the bus controller does not provide a chip select.
+    maxItems: 1
+
+  rdwr-b-gpios:
+    description:
+      read/write select pin (referred to as RDWR_B in the manual)
+      Optional gpio for if the bus controller does not provide this pin.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - prog_b-gpios
+  - done-gpios
+  - init-b-gpios
+
+additionalProperties: true

Nope, this cannot bue true.

Ok, I'll put this to false but I'm not quite sure I understand the implications.

My reasoning behind assigning this to true was that the FPGA is an external
device on a bus that needs to be configured by a bus controller. The bus controller
would be the parent of the fpga DT node and the later would contain properties
parsed by the bus controller driver.

quoted

+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    &weim {

Drop or use some generic soc

Ok

quoted

+      status = "okay";

Drop

Ok

quoted

+      ranges = <0 0 0x08000000 0x04000000>;

Drop

Ok

quoted

+
+      fpga_mgr: fpga_programmer@0,0 {

No underscores in names, drop label.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Ok, will use "fpga-mgr" as this seems to be the most common one for fpga managers.

quoted

+        compatible = "xlnx,fpga-slave-selectmap";
+        reg = <0 0 0x4000000>;
+        fsl,weim-cs-timing = <0x00070031 0x00000142
+              0x00020000 0x00000000
+              0x0c000645 0x00000000>;

NAK.

Please run your patch through Xilinx folks before sending.

Best regards,
Krzysztof

Thank you,
Charles