[1/2] dt-bindings: misc: xlnx,sd-fec: convert bindings to yaml
Commit Message
Convert AMD (Xilinx) sd-fec bindings to yaml format, so it can validate
dt-entries as well as any future additions to yaml.
Conversion txt to yamal is done "one to one", no new changes in txt file
has been made.
Reviewed-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Dragan Cvetic <dragan.cvetic@amd.com>
---
.../devicetree/bindings/misc/xlnx,sd-fec.txt | 58 --------
.../devicetree/bindings/misc/xlnx,sd-fec.yaml | 132 ++++++++++++++++++
2 files changed, 132 insertions(+), 58 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
create mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
Comments
On 16/01/2024 12:11, Dragan Cvetic wrote:
> Convert AMD (Xilinx) sd-fec bindings to yaml format, so it can validate
> dt-entries as well as any future additions to yaml.
> Conversion txt to yamal is done "one to one", no new changes in txt file
> has been made.
>
> Reviewed-by: Michal Simek <michal.simek@amd.com>
Where? Please provide a link. Judging by amount of issues here, I have
some doubts, because I know Michal writes good schema. :)
> Signed-off-by: Dragan Cvetic <dragan.cvetic@amd.com>
All your patches were marked as spam. Please work with your IT to
resolve it, otherwise your future submissions might get ignored by me,
because I will just not see them.
> ---
> .../devicetree/bindings/misc/xlnx,sd-fec.txt | 58 --------
> .../devicetree/bindings/misc/xlnx,sd-fec.yaml | 132 ++++++++++++++++++
> 2 files changed, 132 insertions(+), 58 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
> create mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.
Looks like you either based it on some downstream tree (don't do this)
or used random list of recipients (also don't do this).
Please reach to other AMD folks to explain you how patches should be
submitted. There are a lot of experienced guys there, so use them.
> diff --git a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
> deleted file mode 100644
> index e3289634fa30..000000000000
> --- a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
> +++ /dev/null
> @@ -1,58 +0,0 @@
> -* Xilinx SDFEC(16nm) IP *
> -
..
> - };
> diff --git a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
> new file mode 100644
> index 000000000000..05bc01cb5075
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
> @@ -0,0 +1,132 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/xlnx,sd-fec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx SDFEC(16nm) IP
> +
> +maintainers:
> + - Cvetic, Dragan <dragan.cvetic@amd.com>
> + - Erim, Salih <salih.erim@amd.com>
> +
> +description: |
> + The Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP block
> + which provides high-throughput LDPC and Turbo Code implementations.
> + The LDPC decode & encode functionality is capable of covering a range of
> + customer specified Quasi-cyclic (QC) codes. The Turbo decode functionality
> + principally covers codes used by LTE. The FEC Engine offers significant
> + power and area savings versus implementations done in the FPGA fabric.
> +
> +properties:
> + compatible:
> + const: xlnx,sd-fec-1.1
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + description: List of clock specifiers.
Drop description, it's obvious. Do you see it anywhere in existing code?
> + additionalItems: true
Drop, you cannot have it.
> + minItems: 2
> + maxItems: 8
Drop maxItems, not needed.
> + items:
> + - description: Main processing clock for processing core.
Drop trailing full-stops.
> + - description: AXI4-Lite memory-mapped slave interface clock.
> + - description: Control input AXI4-Stream Slave interface clock.
> + - description: DIN AXI4-Stream Slave interface clock.
> + - description: Status output AXI4-Stream Master interface clock.
> + - description: DOUT AXI4-Stream Master interface clock.
> + - description: DIN_WORDS AXI4-Stream Slave interface clock
> + - description: DOUT_WORDS AXI4-Stream Master interface clock
> +
> + clock-names:
> + additionalItems: true
Nope
> + minItems: 2
> + maxItems: 8
Nope
> + items:
> + - const: core_clk
> + - const: s_axi_aclk
> + - enum:
> + - s_axis_ctrl_aclk
> + - s_axis_din_aclk
> + - m_axis_status_aclk
> + - m_axis_dout_aclk
> + - s_axis_din_words_aclk
> + - m_axis_dout_words_aclk
Why order is not enforced?
> +
> + interrupts:
> + maxItems: 1
> +
> + xlnx,sdfec-code:
> + description:
> + Should contain "ldpc" or "turbo" to describe the codes being used.
Useless description, you just copied schema. Instead say something
useful, e.g. the meaning.
> + $ref: /schemas/types.yaml#/definitions/string-array
It's not an array, but string, is it?
> + items:
How many items? Is it a string?
> + enum: [ ldpc, turbo ]
> +
> + xlnx,sdfec-din-width:
> + description: |
> + Configures the DIN AXI stream where a value of 1
> + configures a width of "1x128b", 2 a width of "2x128b" and 4 configures a width
> + of "4x128b".
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 1, 2, 4 ]
> +
> + xlnx,sdfec-din-words:
> + description: |
> + A value 0 indicates that the DIN_WORDS interface is
> + driven with a fixed value and is not present on the device, a value of 1
> + configures the DIN_WORDS to be block based, while a value of 2 configures the
> + DIN_WORDS input to be supplied for each AXI transaction.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2 ]
> +
> + xlnx,sdfec-dout-width:
> + description: |
> + Configures the DOUT AXI stream where a value of 1 configures a width of "1x128b",
> + 2 a width of "2x128b" and 4 configures a width of "4x128b".
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 1, 2, 4 ]
> +
> + xlnx,sdfec-dout-words:
> + description: |
> + A value 0 indicates that the DOUT_WORDS interface is
> + driven with a fixed value and is not present on the device, a value of 1
> + configures the DOUT_WORDS to be block based, while a value of 2 configures the
> + DOUT_WORDS input to be supplied for each AXI transaction.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [ 0, 1, 2 ]
> +
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - xlnx,sdfec-code
> + - xlnx,sdfec-din-width
> + - xlnx,sdfec-din-words
> + - xlnx,sdfec-dout-width
> + - xlnx,sdfec-dout-words
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + sd-fec@a0040000 {
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,sd-fec-1.1";
> + reg = <0xa0040000 0x40000>;
> + clocks = <&misc_clk_2>, <&misc_clk_0>, <&misc_clk_1>, <&misc_clk_1>,
> + <&misc_clk_1>, <&misc_clk_1>;
> + clock-names = "core_clk", "s_axi_aclk", "s_axis_ctrl_aclk",
> + "s_axis_din_aclk", "m_axis_status_aclk",
> + "m_axis_dout_aclk";
> + interrupts = <1 0>;
If 0 is a flag, use proper defines. Then think twice whether NONE is
really a correct type of interrupt.
Best regards,
Krzysztof
On 1/16/24 16:20, Krzysztof Kozlowski wrote:
> On 16/01/2024 12:11, Dragan Cvetic wrote:
>> Convert AMD (Xilinx) sd-fec bindings to yaml format, so it can validate
>> dt-entries as well as any future additions to yaml.
>> Conversion txt to yamal is done "one to one", no new changes in txt file
>> has been made.
>>
>> Reviewed-by: Michal Simek <michal.simek@amd.com>
>
> Where? Please provide a link. Judging by amount of issues here, I have
> some doubts, because I know Michal writes good schema. :)
I reviewed it internally. But yes, I didn't provide this line.
And never said that 2/2 should be separate patch too. :-)
>
>> Signed-off-by: Dragan Cvetic <dragan.cvetic@amd.com>
>
> All your patches were marked as spam. Please work with your IT to
> resolve it, otherwise your future submissions might get ignored by me,
> because I will just not see them.
>
>> ---
>> .../devicetree/bindings/misc/xlnx,sd-fec.txt | 58 --------
>> .../devicetree/bindings/misc/xlnx,sd-fec.yaml | 132 ++++++++++++++++++
>> 2 files changed, 132 insertions(+), 58 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
>> create mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
>>
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
>
> Looks like you either based it on some downstream tree (don't do this)
> or used random list of recipients (also don't do this).
>
> Please reach to other AMD folks to explain you how patches should be
> submitted. There are a lot of experienced guys there, so use them.
>
>> diff --git a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
>> deleted file mode 100644
>> index e3289634fa30..000000000000
>> --- a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
>> +++ /dev/null
>> @@ -1,58 +0,0 @@
>> -* Xilinx SDFEC(16nm) IP *
>> -
>
> ...
>
>> - };
>> diff --git a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
>> new file mode 100644
>> index 000000000000..05bc01cb5075
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
>> @@ -0,0 +1,132 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/misc/xlnx,sd-fec.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Xilinx SDFEC(16nm) IP
>> +
>> +maintainers:
>> + - Cvetic, Dragan <dragan.cvetic@amd.com>
>> + - Erim, Salih <salih.erim@amd.com>
>> +
>> +description: |
>> + The Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP block
>> + which provides high-throughput LDPC and Turbo Code implementations.
>> + The LDPC decode & encode functionality is capable of covering a range of
>> + customer specified Quasi-cyclic (QC) codes. The Turbo decode functionality
>> + principally covers codes used by LTE. The FEC Engine offers significant
>> + power and area savings versus implementations done in the FPGA fabric.
>> +
>> +properties:
>> + compatible:
>> + const: xlnx,sd-fec-1.1
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + description: List of clock specifiers.
>
> Drop description, it's obvious. Do you see it anywhere in existing code?
>
>> + additionalItems: true
>
> Drop, you cannot have it.
>
>> + minItems: 2
>> + maxItems: 8
>
> Drop maxItems, not needed.
>
>> + items:
>> + - description: Main processing clock for processing core.
>
> Drop trailing full-stops.
>
>> + - description: AXI4-Lite memory-mapped slave interface clock.
>> + - description: Control input AXI4-Stream Slave interface clock.
>> + - description: DIN AXI4-Stream Slave interface clock.
>> + - description: Status output AXI4-Stream Master interface clock.
>> + - description: DOUT AXI4-Stream Master interface clock.
>> + - description: DIN_WORDS AXI4-Stream Slave interface clock
>> + - description: DOUT_WORDS AXI4-Stream Master interface clock
>> +
>> + clock-names:
>> + additionalItems: true
>
> Nope
>
>> + minItems: 2
>> + maxItems: 8
>
> Nope
>
>> + items:
>> + - const: core_clk
>> + - const: s_axi_aclk
>> + - enum:
>> + - s_axis_ctrl_aclk
>> + - s_axis_din_aclk
>> + - m_axis_status_aclk
>> + - m_axis_dout_aclk
>> + - s_axis_din_words_aclk
>> + - m_axis_dout_words_aclk
>
> Why order is not enforced?
Let me comment this one. Based on my discussion with Dragan IP itself is
configurable and only the first two clocks are in all combinations. But based on
his description that last 6 clocks can be present in some of them.
It means order is not really fixed and any combination is possible.
That's why I have suggested him to use this description because I didn't find
any better one.
I actually tested this schema here but didn't get a feedback on it yet.
https://lore.kernel.org/r/3e86244a840a45c970289ba6d2fa700a74f5b259.1705051222.git.michal.simek@amd.com
It means not sure about not defining maxItems but when I don't do it it is not
passing dtbs_check.
Thanks,
Michal
On 16/01/2024 16:36, Michal Simek wrote:
>>> + clock-names:
>>> + additionalItems: true
>>
>> Nope
>>
>>> + minItems: 2
>>> + maxItems: 8
>>
>> Nope
>>
>>> + items:
>>> + - const: core_clk
>>> + - const: s_axi_aclk
>>> + - enum:
>>> + - s_axis_ctrl_aclk
>>> + - s_axis_din_aclk
>>> + - m_axis_status_aclk
>>> + - m_axis_dout_aclk
>>> + - s_axis_din_words_aclk
>>> + - m_axis_dout_words_aclk
>>
>> Why order is not enforced?
>
> Let me comment this one. Based on my discussion with Dragan IP itself is
> configurable and only the first two clocks are in all combinations. But based on
> his description that last 6 clocks can be present in some of them.
> It means order is not really fixed and any combination is possible.
> That's why I have suggested him to use this description because I didn't find
> any better one.
> I actually tested this schema here but didn't get a feedback on it yet.
> https://lore.kernel.org/r/3e86244a840a45c970289ba6d2fa700a74f5b259.1705051222.git.michal.simek@amd.com
>
> It means not sure about not defining maxItems but when I don't do it it is not
> passing dtbs_check.
This would explain why you want additionalItems:true, but it should be
also explained in commit msg. Old code did not have such relaxed
statement, at least not explicitly written, and commit msg explicitly
says it is 1-to-1 conversion.
Anyway, current solution won't work, because additional items can be
anything. Try it. Put as fourth clock "yellow_duck" and see what happens.
I don't find such names as useful and maybe the drivers should just get
by index. Especially that Linux driver does not care. It would be a ABI
change, though, so up to you.
If you want to keep the names, then:
1. Look at snps,dwmac.yaml
2. or just list 6 enums with all possibilities.
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, January 16, 2024 3:21 PM
> To: Cvetic, Dragan <dragan.cvetic@amd.com>; arnd@arndb.de;
> gregkh@linuxfoundation.org; michal.simek@xilinx.com; linux-arm-
> kernel@lists.infradead.org; robh+dt@kernel.org; mark.rutland@arm.com;
> devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH 1/2] dt-bindings: misc: xlnx,sd-fec: convert bindings to
> yaml
>
> On 16/01/2024 12:11, Dragan Cvetic wrote:
> > Convert AMD (Xilinx) sd-fec bindings to yaml format, so it can validate
> > dt-entries as well as any future additions to yaml.
> > Conversion txt to yamal is done "one to one", no new changes in txt file
> > has been made.
> >
> > Reviewed-by: Michal Simek <michal.simek@amd.com>
>
> Where? Please provide a link. Judging by amount of issues here, I have
> some doubts, because I know Michal writes good schema. :)
>
> > Signed-off-by: Dragan Cvetic <dragan.cvetic@amd.com>
>
> All your patches were marked as spam. Please work with your IT to
> resolve it, otherwise your future submissions might get ignored by me,
> because I will just not see them.
>
I've communicated the issue to internal IT, still waiting for response.
Will send v2 after resolving spam issue.
> > ---
> > .../devicetree/bindings/misc/xlnx,sd-fec.txt | 58 --------
> > .../devicetree/bindings/misc/xlnx,sd-fec.yaml | 132 ++++++++++++++++++
> > 2 files changed, 132 insertions(+), 58 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-
> fec.txt
> > create mode 100644 Documentation/devicetree/bindings/misc/xlnx,sd-
> fec.yaml
> >
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
>
> Looks like you either based it on some downstream tree (don't do this)
> or used random list of recipients (also don't do this).
>
> Please reach to other AMD folks to explain you how patches should be
> submitted. There are a lot of experienced guys there, so use them.
Accepted. Yes I communicated AMD experts. Sorry for the mistake.
>
> > diff --git a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
> b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
> > deleted file mode 100644
> > index e3289634fa30..000000000000
> > --- a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
> > +++ /dev/null
> > @@ -1,58 +0,0 @@
> > -* Xilinx SDFEC(16nm) IP *
> > -
>
> ...
>
> > - };
> > diff --git a/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
> b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
> > new file mode 100644
> > index 000000000000..05bc01cb5075
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/xlnx,sd-fec.yaml
> > @@ -0,0 +1,132 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/misc/xlnx,sd-fec.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx SDFEC(16nm) IP
> > +
> > +maintainers:
> > + - Cvetic, Dragan <dragan.cvetic@amd.com>
> > + - Erim, Salih <salih.erim@amd.com>
> > +
> > +description: |
> > + The Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP
> block
> > + which provides high-throughput LDPC and Turbo Code implementations.
> > + The LDPC decode & encode functionality is capable of covering a range of
> > + customer specified Quasi-cyclic (QC) codes. The Turbo decode
> functionality
> > + principally covers codes used by LTE. The FEC Engine offers significant
> > + power and area savings versus implementations done in the FPGA fabric.
> > +
> > +properties:
> > + compatible:
> > + const: xlnx,sd-fec-1.1
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + description: List of clock specifiers.
>
> Drop description, it's obvious. Do you see it anywhere in existing code?
Accepted
>
> > + additionalItems: true
>
> Drop, you cannot have it.
Will update commit message explaining the clocks structure
>
> > + minItems: 2
> > + maxItems: 8
>
> Drop maxItems, not needed.
Will update commit message explaining the clocks structure
>
> > + items:
> > + - description: Main processing clock for processing core.
>
> Drop trailing full-stops.
Accepted
>
> > + - description: AXI4-Lite memory-mapped slave interface clock.
> > + - description: Control input AXI4-Stream Slave interface clock.
> > + - description: DIN AXI4-Stream Slave interface clock.
> > + - description: Status output AXI4-Stream Master interface clock.
> > + - description: DOUT AXI4-Stream Master interface clock.
> > + - description: DIN_WORDS AXI4-Stream Slave interface clock
> > + - description: DOUT_WORDS AXI4-Stream Master interface clock
> > +
> > + clock-names:
> > + additionalItems: true
>
> Nope
Will update commit message explaining the clocks structure
>
> > + minItems: 2
> > + maxItems: 8
>
> Nope
Will update commit message explaining the clocks structure
>
> > + items:
> > + - const: core_clk
> > + - const: s_axi_aclk
> > + - enum:
> > + - s_axis_ctrl_aclk
> > + - s_axis_din_aclk
> > + - m_axis_status_aclk
> > + - m_axis_dout_aclk
> > + - s_axis_din_words_aclk
> > + - m_axis_dout_words_aclk
>
> Why order is not enforced?
Will update commit message explaining the clocks structure
>
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + xlnx,sdfec-code:
> > + description:
> > + Should contain "ldpc" or "turbo" to describe the codes being used.
>
> Useless description, you just copied schema. Instead say something
> useful, e.g. the meaning.
I assumed if description was accepted in .txt file it should be good in yaml.
I accept your comment anyway, will put better description
>
> > + $ref: /schemas/types.yaml#/definitions/string-array
>
> It's not an array, but string, is it?
The item is a string which defines device working mode. Can be either "ldpc" or "turbo".
Please, advice what would be the correct way to set this item?
>
> > + items:
>
> How many items? Is it a string?
It's one item with two options "ldpc" and "turbo"
>
> > + enum: [ ldpc, turbo ]
> > +
> > + xlnx,sdfec-din-width:
> > + description: |
> > + Configures the DIN AXI stream where a value of 1
> > + configures a width of "1x128b", 2 a width of "2x128b" and 4 configures
> a width
> > + of "4x128b".
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [ 1, 2, 4 ]
> > +
> > + xlnx,sdfec-din-words:
> > + description: |
> > + A value 0 indicates that the DIN_WORDS interface is
> > + driven with a fixed value and is not present on the device, a value of 1
> > + configures the DIN_WORDS to be block based, while a value of 2
> configures the
> > + DIN_WORDS input to be supplied for each AXI transaction.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [ 0, 1, 2 ]
> > +
> > + xlnx,sdfec-dout-width:
> > + description: |
> > + Configures the DOUT AXI stream where a value of 1 configures a width
> of "1x128b",
> > + 2 a width of "2x128b" and 4 configures a width of "4x128b".
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [ 1, 2, 4 ]
> > +
> > + xlnx,sdfec-dout-words:
> > + description: |
> > + A value 0 indicates that the DOUT_WORDS interface is
> > + driven with a fixed value and is not present on the device, a value of 1
> > + configures the DOUT_WORDS to be block based, while a value of 2
> configures the
> > + DOUT_WORDS input to be supplied for each AXI transaction.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [ 0, 1, 2 ]
> > +
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - xlnx,sdfec-code
> > + - xlnx,sdfec-din-width
> > + - xlnx,sdfec-din-words
> > + - xlnx,sdfec-dout-width
> > + - xlnx,sdfec-dout-words
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + sd-fec@a0040000 {
>
> 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
>
The device is "Soft Decision - Forward Eror Correction", I cannot find appropriate name in the list for this device.
This looks generic enough to me. What would be your suggestion?
>
> > + compatible = "xlnx,sd-fec-1.1";
> > + reg = <0xa0040000 0x40000>;
> > + clocks = <&misc_clk_2>, <&misc_clk_0>, <&misc_clk_1>,
> <&misc_clk_1>,
> > + <&misc_clk_1>, <&misc_clk_1>;
> > + clock-names = "core_clk", "s_axi_aclk", "s_axis_ctrl_aclk",
> > + "s_axis_din_aclk", "m_axis_status_aclk",
> > + "m_axis_dout_aclk";
> > + interrupts = <1 0>;
>
> If 0 is a flag, use proper defines. Then think twice whether NONE is
> really a correct type of interrupt.
>
Yes, this is correct.
Also, the interrupt may or may not be present, I will put this in commit message.
>
>
> Best regards,
> Krzysztof
Hi Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, January 16, 2024 3:45 PM
> To: Simek, Michal <michal.simek@amd.com>; Cvetic, Dragan
> <dragan.cvetic@amd.com>; arnd@arndb.de; gregkh@linuxfoundation.org;
> michal.simek@xilinx.com; linux-arm-kernel@lists.infradead.org;
> robh+dt@kernel.org; mark.rutland@arm.com; devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH 1/2] dt-bindings: misc: xlnx,sd-fec: convert bindings to
> yaml
>
> On 16/01/2024 16:36, Michal Simek wrote:
> >>> + clock-names:
> >>> + additionalItems: true
> >>
> >> Nope
> >>
> >>> + minItems: 2
> >>> + maxItems: 8
> >>
> >> Nope
> >>
> >>> + items:
> >>> + - const: core_clk
> >>> + - const: s_axi_aclk
> >>> + - enum:
> >>> + - s_axis_ctrl_aclk
> >>> + - s_axis_din_aclk
> >>> + - m_axis_status_aclk
> >>> + - m_axis_dout_aclk
> >>> + - s_axis_din_words_aclk
> >>> + - m_axis_dout_words_aclk
> >>
> >> Why order is not enforced?
> >
> > Let me comment this one. Based on my discussion with Dragan IP itself is
> > configurable and only the first two clocks are in all combinations. But based
> on
> > his description that last 6 clocks can be present in some of them.
> > It means order is not really fixed and any combination is possible.
> > That's why I have suggested him to use this description because I didn't find
> > any better one.
> > I actually tested this schema here but didn't get a feedback on it yet.
> >
> https://lore.kernel.org/r/3e86244a840a45c970289ba6d2fa700a74f5b259.170
> 5051222.git.michal.simek@amd.com
> >
> > It means not sure about not defining maxItems but when I don't do it it is
> not
> > passing dtbs_check.
>
>
> This would explain why you want additionalItems:true, but it should be
> also explained in commit msg. Old code did not have such relaxed
> statement, at least not explicitly written, and commit msg explicitly
> says it is 1-to-1 conversion.
Accepted, Will update commit message
>
> Anyway, current solution won't work, because additional items can be
> anything. Try it. Put as fourth clock "yellow_duck" and see what happens.
>
> I don't find such names as useful and maybe the drivers should just get
> by index. Especially that Linux driver does not care. It would be a ABI
> change, though, so up to you.
>
> If you want to keep the names, then:
> 1. Look at snps,dwmac.yaml
Accepted, will keep the names and will apply solution from snps,dwmac.yaml.
Regards
Dragan
Hi Dragan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on soc/for-next]
[also build test WARNING on robh/for-next linus/master v6.7 next-20240119]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dragan-Cvetic/dt-bindings-misc-xlnx-sd-fec-convert-bindings-to-yaml/20240116-191349
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240116111135.3059-2-dragan.cvetic%40amd.com
patch subject: [PATCH 1/2] dt-bindings: misc: xlnx,sd-fec: convert bindings to yaml
reproduce: (https://download.01.org/0day-ci/archive/20240120/202401200159.p9T2rwq9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401200159.p9T2rwq9-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Warning: Documentation/misc-devices/xilinx_sdfec.rst references a file that doesn't exist: linux-xlnx/Documentation/devicetree/bindings/misc/xlnx,sd-fec.txt
deleted file mode 100644
@@ -1,58 +0,0 @@
-* Xilinx SDFEC(16nm) IP *
-
-The Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP block
-which provides high-throughput LDPC and Turbo Code implementations.
-The LDPC decode & encode functionality is capable of covering a range of
-customer specified Quasi-cyclic (QC) codes. The Turbo decode functionality
-principally covers codes used by LTE. The FEC Engine offers significant
-power and area savings versus implementations done in the FPGA fabric.
-
-
-Required properties:
-- compatible: Must be "xlnx,sd-fec-1.1"
-- clock-names : List of input clock names from the following:
- - "core_clk", Main processing clock for processing core (required)
- - "s_axi_aclk", AXI4-Lite memory-mapped slave interface clock (required)
- - "s_axis_din_aclk", DIN AXI4-Stream Slave interface clock (optional)
- - "s_axis_din_words-aclk", DIN_WORDS AXI4-Stream Slave interface clock (optional)
- - "s_axis_ctrl_aclk", Control input AXI4-Stream Slave interface clock (optional)
- - "m_axis_dout_aclk", DOUT AXI4-Stream Master interface clock (optional)
- - "m_axis_dout_words_aclk", DOUT_WORDS AXI4-Stream Master interface clock (optional)
- - "m_axis_status_aclk", Status output AXI4-Stream Master interface clock (optional)
-- clocks : Clock phandles (see clock_bindings.txt for details).
-- reg: Should contain Xilinx SDFEC 16nm Hardened IP block registers
- location and length.
-- xlnx,sdfec-code : Should contain "ldpc" or "turbo" to describe the codes
- being used.
-- xlnx,sdfec-din-words : A value 0 indicates that the DIN_WORDS interface is
- driven with a fixed value and is not present on the device, a value of 1
- configures the DIN_WORDS to be block based, while a value of 2 configures the
- DIN_WORDS input to be supplied for each AXI transaction.
-- xlnx,sdfec-din-width : Configures the DIN AXI stream where a value of 1
- configures a width of "1x128b", 2 a width of "2x128b" and 4 configures a width
- of "4x128b".
-- xlnx,sdfec-dout-words : A value 0 indicates that the DOUT_WORDS interface is
- driven with a fixed value and is not present on the device, a value of 1
- configures the DOUT_WORDS to be block based, while a value of 2 configures the
- DOUT_WORDS input to be supplied for each AXI transaction.
-- xlnx,sdfec-dout-width : Configures the DOUT AXI stream where a value of 1
- configures a width of "1x128b", 2 a width of "2x128b" and 4 configures a width
- of "4x128b".
-Optional properties:
-- interrupts: should contain SDFEC interrupt number
-
-Example
----------------------------------------
- sd_fec_0: sd-fec@a0040000 {
- compatible = "xlnx,sd-fec-1.1";
- clock-names = "core_clk","s_axi_aclk","s_axis_ctrl_aclk","s_axis_din_aclk","m_axis_status_aclk","m_axis_dout_aclk";
- clocks = <&misc_clk_2>,<&misc_clk_0>,<&misc_clk_1>,<&misc_clk_1>,<&misc_clk_1>, <&misc_clk_1>;
- reg = <0x0 0xa0040000 0x0 0x40000>;
- interrupt-parent = <&axi_intc>;
- interrupts = <1 0>;
- xlnx,sdfec-code = "ldpc";
- xlnx,sdfec-din-words = <0>;
- xlnx,sdfec-din-width = <2>;
- xlnx,sdfec-dout-words = <0>;
- xlnx,sdfec-dout-width = <1>;
- };
new file mode 100644
@@ -0,0 +1,132 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/xlnx,sd-fec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx SDFEC(16nm) IP
+
+maintainers:
+ - Cvetic, Dragan <dragan.cvetic@amd.com>
+ - Erim, Salih <salih.erim@amd.com>
+
+description: |
+ The Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP block
+ which provides high-throughput LDPC and Turbo Code implementations.
+ The LDPC decode & encode functionality is capable of covering a range of
+ customer specified Quasi-cyclic (QC) codes. The Turbo decode functionality
+ principally covers codes used by LTE. The FEC Engine offers significant
+ power and area savings versus implementations done in the FPGA fabric.
+
+properties:
+ compatible:
+ const: xlnx,sd-fec-1.1
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description: List of clock specifiers.
+ additionalItems: true
+ minItems: 2
+ maxItems: 8
+ items:
+ - description: Main processing clock for processing core.
+ - description: AXI4-Lite memory-mapped slave interface clock.
+ - description: Control input AXI4-Stream Slave interface clock.
+ - description: DIN AXI4-Stream Slave interface clock.
+ - description: Status output AXI4-Stream Master interface clock.
+ - description: DOUT AXI4-Stream Master interface clock.
+ - description: DIN_WORDS AXI4-Stream Slave interface clock
+ - description: DOUT_WORDS AXI4-Stream Master interface clock
+
+ clock-names:
+ additionalItems: true
+ minItems: 2
+ maxItems: 8
+ items:
+ - const: core_clk
+ - const: s_axi_aclk
+ - enum:
+ - s_axis_ctrl_aclk
+ - s_axis_din_aclk
+ - m_axis_status_aclk
+ - m_axis_dout_aclk
+ - s_axis_din_words_aclk
+ - m_axis_dout_words_aclk
+
+ interrupts:
+ maxItems: 1
+
+ xlnx,sdfec-code:
+ description:
+ Should contain "ldpc" or "turbo" to describe the codes being used.
+ $ref: /schemas/types.yaml#/definitions/string-array
+ items:
+ enum: [ ldpc, turbo ]
+
+ xlnx,sdfec-din-width:
+ description: |
+ Configures the DIN AXI stream where a value of 1
+ configures a width of "1x128b", 2 a width of "2x128b" and 4 configures a width
+ of "4x128b".
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 1, 2, 4 ]
+
+ xlnx,sdfec-din-words:
+ description: |
+ A value 0 indicates that the DIN_WORDS interface is
+ driven with a fixed value and is not present on the device, a value of 1
+ configures the DIN_WORDS to be block based, while a value of 2 configures the
+ DIN_WORDS input to be supplied for each AXI transaction.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2 ]
+
+ xlnx,sdfec-dout-width:
+ description: |
+ Configures the DOUT AXI stream where a value of 1 configures a width of "1x128b",
+ 2 a width of "2x128b" and 4 configures a width of "4x128b".
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 1, 2, 4 ]
+
+ xlnx,sdfec-dout-words:
+ description: |
+ A value 0 indicates that the DOUT_WORDS interface is
+ driven with a fixed value and is not present on the device, a value of 1
+ configures the DOUT_WORDS to be block based, while a value of 2 configures the
+ DOUT_WORDS input to be supplied for each AXI transaction.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2 ]
+
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - xlnx,sdfec-code
+ - xlnx,sdfec-din-width
+ - xlnx,sdfec-din-words
+ - xlnx,sdfec-dout-width
+ - xlnx,sdfec-dout-words
+
+additionalProperties: false
+
+examples:
+ - |
+ sd-fec@a0040000 {
+ compatible = "xlnx,sd-fec-1.1";
+ reg = <0xa0040000 0x40000>;
+ clocks = <&misc_clk_2>, <&misc_clk_0>, <&misc_clk_1>, <&misc_clk_1>,
+ <&misc_clk_1>, <&misc_clk_1>;
+ clock-names = "core_clk", "s_axi_aclk", "s_axis_ctrl_aclk",
+ "s_axis_din_aclk", "m_axis_status_aclk",
+ "m_axis_dout_aclk";
+ interrupts = <1 0>;
+ xlnx,sdfec-code = "ldpc";
+ xlnx,sdfec-din-width = <2>;
+ xlnx,sdfec-din-words = <0>;
+ xlnx,sdfec-dout-width = <1>;
+ xlnx,sdfec-dout-words = <0>;
+ };
+