> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Tuesday, January 16, 2024 3:21 PM > To: Cvetic, Dragan <dragan.cvetic@xxxxxxx>; arnd@xxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; michal.simek@xxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; > devicetree@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx> > 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@xxxxxxx> > > 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@xxxxxxx> > > 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@xxxxxxx> > > + - Erim, Salih <salih.erim@xxxxxxx> > > + > > +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
