| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 18 Jan 2024 00:49:13 +0000
From: "Cvetic, Dragan" <dragan.cvetic@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, "arnd@...db.de"
<arnd@...db.de>, "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"michal.simek@...inx.com" <michal.simek@...inx.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "robh+dt@...nel.org"
<robh+dt@...nel.org>, "mark.rutland@....com" <mark.rutland@....com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "git
(AMD-Xilinx)" <git@....com>
Subject: RE: [PATCH 1/2] dt-bindings: misc: xlnx,sd-fec: convert bindings to
yaml
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Tuesday, January 16, 2024 3:21 PM
> To: Cvetic, Dragan <dragan.cvetic@....com>; arnd@...db.de;
> gregkh@...uxfoundation.org; michal.simek@...inx.com; linux-arm-
> kernel@...ts.infradead.org; robh+dt@...nel.org; mark.rutland@....com;
> devicetree@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org; git (AMD-Xilinx) <git@....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@....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@....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@....com>
> > + - Erim, Salih <salih.erim@....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@...40000 {
>
> 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
Powered by blists - more mailing lists