| Message ID | 20240129225602.3832449-2-charles.perry@savoirfairelinux.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] fpga: xilinx-spi: extract a common driver core | expand |
On Mon, 29 Jan 2024 17:56:01 -0500, Charles Perry wrote: > Document the slave SelectMAP interface of Xilinx 7 series FPGA. > > Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com> > --- > .../fpga/xlnx,fpga-slave-selectmap.yaml | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.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/xlnx,fpga-slave-selectmap.example.dts:19.9-14 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1428: 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/20240129225602.3832449-2-charles.perry@savoirfairelinux.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.
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@savoirfairelinux.com> > --- > .../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. > + > + reg: > + description: > + At least 1 byte of memory mapped IO > + maxItems: 1 > + > + prog_b-gpios: No underscores in names. > + 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. > + description: > + initialization status and configuration error pin > + (referred to as INIT_B in the manual) > + maxItems: 1 > + > + csi-b-gpios: Where is csi-a? > + 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. > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + &weim { Drop or use some generic soc > + status = "okay"; Drop > + ranges = <0 0 0x08000000 0x04000000>; Drop > + > + 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 > + 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
On 30/01/2024 08:52, Krzysztof Kozlowski wrote:
> Please run your patch through Xilinx folks before sending.
Uh, you are not Xilinx folk, so disregard that. Yet still amount of
trivial issues here point that you need to start from scratch from
existing schema or example-schema.
Best regards,
Krzysztof
----- On Jan 30, 2024, at 2:52 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org 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@savoirfairelinux.com> >> --- >> .../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
On 30/01/2024 16:45, Charles Perry wrote: > >>> + >>> + 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. Inconsistency is not a problem. Duplicating technical debt is. > >> >>> + 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. Then just "init-gpios" ... >>> +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. Which bus controller? MMIO bus does not parse children properties. Anyway, if that's the case you miss $ref to respective peripheral-props.yaml matching your bus and then "unevaluatedProperties: false". Best regards, Krzysztof
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@savoirfairelinux.com> > --- > .../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 https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/process/coding-style.rst#L338 Everywhere: compatible, title, filename, descriptions. > + > +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 I am surprised that AMD/Xilinx still did not update the document to modern naming (slave->secondary). +Cc Michal, Maybe that's something you could push it. Best regards, Krzysztof
----- On Jan 30, 2024, at 11:05 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote: > On 30/01/2024 16:45, Charles Perry wrote: >> >>>> + >>>> + 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. > > Inconsistency is not a problem. Duplicating technical debt is. > >> >>> >>>> + 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. > > Then just "init-gpios" > > ... > >>>> +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. > > Which bus controller? MMIO bus does not parse children properties. > Anyway, if that's the case you miss $ref to respective > peripheral-props.yaml matching your bus and then "unevaluatedProperties: > false". This one: https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/devicetree/bindings/bus/imx-weim.txt#L56 > > Best regards, > Krzysztof Regards, Charles
On 30/01/2024 18:05, Charles Perry wrote: > > > ----- On Jan 30, 2024, at 11:05 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote: > >> On 30/01/2024 16:45, Charles Perry wrote: >>> >>>>> + >>>>> + 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. >> >> Inconsistency is not a problem. Duplicating technical debt is. >> >>> >>>> >>>>> + 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. >> >> Then just "init-gpios" >> >> ... >> >>>>> +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. >> >> Which bus controller? MMIO bus does not parse children properties. >> Anyway, if that's the case you miss $ref to respective >> peripheral-props.yaml matching your bus and then "unevaluatedProperties: >> false". > > This one: https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/devicetree/bindings/bus/imx-weim.txt#L56 Eh, ok, so after fast check WEIM looks like some memory interface bus, so the bus bindings should be moved to memory-controllers and converted to YAML. Then you add child node properties to own schema and reference in mc-peripheral-props, which is then referenced in your binding here, as I mentioned. Best regards, Krzysztof
----- On Jan 30, 2024, at 12:58 PM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote: > On 30/01/2024 18:05, Charles Perry wrote: >> >> >> ----- On Jan 30, 2024, at 11:05 AM, Krzysztof Kozlowski >> krzysztof.kozlowski@linaro.org wrote: >> >>> On 30/01/2024 16:45, Charles Perry wrote: >>>> >>>>>> + >>>>>> + 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. >>> >>> Inconsistency is not a problem. Duplicating technical debt is. >>> >>>> >>>>> >>>>>> + 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. >>> >>> Then just "init-gpios" >>> >>> ... >>> >>>>>> +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. >>> >>> Which bus controller? MMIO bus does not parse children properties. >>> Anyway, if that's the case you miss $ref to respective >>> peripheral-props.yaml matching your bus and then "unevaluatedProperties: >>> false". >> >> This one: >> https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/devicetree/bindings/bus/imx-weim.txt#L56 > > Eh, ok, so after fast check WEIM looks like some memory interface bus, > so the bus bindings should be moved to memory-controllers and converted > to YAML. Then you add child node properties to own schema and reference > in mc-peripheral-props, which is then referenced in your binding here, > as I mentioned. > > Best regards, > Krzysztof Thank you for pointing that out, mc-peripheral-props.yaml seems to be exactly what I was looking for. Regards, Charles
Hello Krzysztof, On 30/01/2024 16:09, Krzysztof Kozlowski wrote: >> + >> +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 > > I am surprised that AMD/Xilinx still did not update the document to > modern naming (slave->secondary). Thank you for bringing this up. We are moving away from using non-inclusive technical terminology and are removing non-inclusive language from our products and related collateral. You will for some time find examples of non-inclusive language, especially in our older products as we work to make these changes and align with industry standards. For new IP we're ensuring that we switch and stick to inclusive terminology, as you may have seen with my recent w1 driver submission. SelectMAP is a decades-old interface and as such it is unlikely that we will update this in all documentation dating back this time. I shall however look to understand what is planned here for active documentation and new driver submissions. regards Kris
On Wed, Jan 31, 2024 at 11:03:25AM +0000, Kris Chaplin wrote: > Hello Krzysztof, > > On 30/01/2024 16:09, Krzysztof Kozlowski wrote: > > > > + > > > +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 > > > > I am surprised that AMD/Xilinx still did not update the document to > > modern naming (slave->secondary). > > Thank you for bringing this up. > > We are moving away from using non-inclusive technical terminology and are > removing non-inclusive language from our products and related collateral. > You will for some time find examples of non-inclusive language, especially > in our older products as we work to make these changes and align with > industry standards. For new IP we're ensuring that we switch and stick to > inclusive terminology, as you may have seen with my recent w1 driver > submission. > > SelectMAP is a decades-old interface and as such it is unlikely that we will > update this in all documentation dating back this time. I shall however > look to understand what is planned here for active documentation and new > driver submissions. Yes, I need review from AMD/Xilinx side. Especially the HW parts, and some namings of variables, e.g. if xilinx-core is proper for what products it supports, and won't be an issue in future. Thanks, Yilun > > regards > Kris >
On Feb 4, 2024, at 3:30 AM, Xu Yilun yilun.xu@linux.intel.com wrote: > On Wed, Jan 31, 2024 at 11:03:25AM +0000, Kris Chaplin wrote: >> Hello Krzysztof, >> >> On 30/01/2024 16:09, Krzysztof Kozlowski wrote: >> >> > > + >> > > +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 >> > >> > I am surprised that AMD/Xilinx still did not update the document to >> > modern naming (slave->secondary). >> >> Thank you for bringing this up. >> >> We are moving away from using non-inclusive technical terminology and are >> removing non-inclusive language from our products and related collateral. >> You will for some time find examples of non-inclusive language, especially >> in our older products as we work to make these changes and align with >> industry standards. For new IP we're ensuring that we switch and stick to >> inclusive terminology, as you may have seen with my recent w1 driver >> submission. >> >> SelectMAP is a decades-old interface and as such it is unlikely that we will >> update this in all documentation dating back this time. I shall however >> look to understand what is planned here for active documentation and new >> driver submissions. > > Yes, I need review from AMD/Xilinx side. Especially the HW parts, and > some namings of variables, e.g. if xilinx-core is proper for what products > it supports, and won't be an issue in future. > > Thanks, > Yilun > >> >> regards >> Kris Hello, I chose the "-core" suffix as it seems widely used for core logic of device drivers for chips that comes in an i2c and spi flavour. It seems like "-common" is also widespread, let me know if you prefer that suffix. As for the HW parts, here's the compatibles used in the v3 patchset for convenience: - xlnx,fpga-xc7s-selectmap - xlnx,fpga-xc7a-selectmap - xlnx,fpga-xc7k-selectmap - xlnx,fpga-xc7v-selectmap We're trying to be a bit more specific than the spi interface which uses "xlnx,fpga-slave-serial" Regards, Charles
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 + + reg: + description: + At least 1 byte of memory mapped IO + maxItems: 1 + + prog_b-gpios: + 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: + description: + initialization status and configuration error pin + (referred to as INIT_B in the manual) + maxItems: 1 + + csi-b-gpios: + 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 + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + &weim { + status = "okay"; + ranges = <0 0 0x08000000 0x04000000>; + + fpga_mgr: fpga_programmer@0,0 { + compatible = "xlnx,fpga-slave-selectmap"; + reg = <0 0 0x4000000>; + fsl,weim-cs-timing = <0x00070031 0x00000142 + 0x00020000 0x00000000 + 0x0c000645 0x00000000>; + prog_b-gpios = <&gpio5 5 GPIO_ACTIVE_LOW>; + init-b-gpios = <&gpio5 8 GPIO_ACTIVE_LOW>; + done-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>; + csi-b-gpios = <&gpio3 19 GPIO_ACTIVE_LOW>; + rdwr-b-gpios = <&gpio3 10 GPIO_ACTIVE_LOW>; + }; + }; +...
Document the slave SelectMAP interface of Xilinx 7 series FPGA. Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com> --- .../fpga/xlnx,fpga-slave-selectmap.yaml | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml