Received-SPF: pass (google.com: domain of linux-kernel+bounces-44866-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 30 Jan 2024 10:45:44 -0500 (EST)
From: Charles Perry <charles.perry@savoirfairelinux.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: mdf@kernel.org, hao wu <hao.wu@intel.com>, yilun xu <yilun.xu@intel.com>, 
	trix@redhat.com, 
	krzysztof kozlowski+dt <krzysztof.kozlowski+dt@linaro.org>, 
	Brian CODY <bcody@markem-imaje.com>, 
	Allen VANDIVER <avandiver@markem-imaje.com>, 
	linux-fpga@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Message-ID: <1489222458.382780.1706629544559.JavaMail.zimbra@savoirfairelinux.com>
In-Reply-To: <f3cfffa0-5089-4bf7-b424-d5e949e36d67@linaro.org>
References: <20240129225602.3832449-1-charles.perry@savoirfairelinux.com> <20240129225602.3832449-2-charles.perry@savoirfairelinux.com> <f3cfffa0-5089-4bf7-b424-d5e949e36d67@linaro.org>
Subject: Re: [PATCH 2/3] dt-bindings: fpga: xlnx,fpga-slave-selectmap: add
 DT schema
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Thread-Topic: dt-bindings: fpga: xlnx,fpga-slave-selectmap: add DT schema
Thread-Index: /XNXbd6Eoqkwp5+V9vA2kJJ0vFwOwg==

----- 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.
>>=20
>> 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
>>=20
>> 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.y=
aml
>> @@ -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 documentatio=
n. 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_C=
onfig.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - xlnx,fpga-slave-selectmap
>=20
> You did not test bindings, so only limited review.
>=20

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:
>=20
>=20
> No underscores in names.
>=20

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

>=20
>> +    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:
>=20
> Is there init-a? Open other bindings and look how these are called there.
>=20

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

>=20
>> +    description:
>> +      initialization status and configuration error pin
>> +      (referred to as INIT_B in the manual)
>> +    maxItems: 1
>> +
>> +  csi-b-gpios:
>=20
> Where is csi-a?
>=20

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 s=
elect.
>> +    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
>=20
> Nope, this cannot bue true.
>=20

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

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 co=
ntroller
would be the parent of the fpga DT node and the later would contain propert=
ies
parsed by the bus controller driver.

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    &weim {
>=20
> Drop or use some generic soc
>=20

Ok

>> +      status =3D "okay";
>=20
> Drop
>=20

Ok

>> +      ranges =3D <0 0 0x08000000 0x04000000>;
>=20
> Drop
>=20

Ok

>> +
>> +      fpga_mgr: fpga_programmer@0,0 {
>=20
> No underscores in names, drop label.
>=20
> 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-device=
tree-basics.html#generic-names-recommendation
>=20
>=20

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

>> +        compatible =3D "xlnx,fpga-slave-selectmap";
>> +        reg =3D <0 0 0x4000000>;
>> +        fsl,weim-cs-timing =3D <0x00070031 0x00000142
>> +              0x00020000 0x00000000
>> +              0x0c000645 0x00000000>;
>=20
> NAK.
>=20
> Please run your patch through Xilinx folks before sending.
>=20
> Best regards,
> Krzysztof

Thank you,
Charles