----- On Jan 30, 2024, at 2:52 AM, Krzysztof Kozlowski krzysztof.kozlowski@xxxxxxxxxx wrote: > On 29/01/2024 23:56, Charles Perry wrote: >> Document the slave SelectMAP interface of Xilinx 7 series FPGA. >> >> Signed-off-by: Charles Perry <charles.perry@xxxxxxxxxxxxxxxxxxxx> >> --- >> .../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. >> + >> + 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. > >> + 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. > >> + 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. >> + 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. >> + >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + &weim { > > Drop or use some generic soc > Ok >> + status = "okay"; > > Drop > Ok >> + ranges = <0 0 0x08000000 0x04000000>; > > Drop > Ok >> + >> + 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. >> + 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
