| 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
| ||
|
Message-ID: <ad7cd2e3-c061-4f17-83dc-2a6df8095d30@kernel.org>
Date: Wed, 28 May 2025 07:48:52 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: weishangjuan@...incomputing.com, andrew+netdev@...n.ch,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, mcoquelin.stm32@...il.com,
alexandre.torgue@...s.st.com, vladimir.oltean@....com,
rmk+kernel@...linux.org.uk, yong.liang.choong@...ux.intel.com,
prabhakar.mahadev-lad.rj@...renesas.com, inochiama@...il.com,
jan.petrous@....nxp.com, jszhang@...nel.org, p.zabel@...gutronix.de,
0x1207@...il.com, boon.khai.ng@...era.com,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org
Cc: ningyu@...incomputing.com, linmin@...incomputing.com,
lizhi2@...incomputing.com
Subject: Re: [PATCH v2 1/2] dt-bindings: ethernet: eswin: Document for EIC7700
SoC
On 28/05/2025 06:15, weishangjuan@...incomputing.com wrote:
> From: Shangjuan Wei <weishangjuan@...incomputing.com>
>
> Add ESWIN EIC7700 Ethernet controller, supporting
> multi-rate (10M/100M/1G) auto-negotiation, clock/reset control,
> and AXI bus parameter optimization.
>
> Signed-off-by: Zhi Li <lizhi2@...incomputing.com>
> Signed-off-by: Shangjuan Wei <weishangjuan@...incomputing.com>
> ---
> .../bindings/net/eswin,eic7700-eth.yaml | 200 ++++++++++++++++++
> 1 file changed, 200 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> new file mode 100644
> index 000000000000..c76d2fbf0ebd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> @@ -0,0 +1,200 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/eswin,eic7700-eth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Eswin EIC7700 SOC Eth Controller
> +
> +maintainers:
> + - Shuang Liang <liangshuang@...incomputing.com>
> + - Zhi Li <lizhi2@...incomputing.com>
> + - Shangjuan Wei <weishangjuan@...incomputing.com>
> +
> +description:
> + The eth controller registers are part of the syscrg block on
> + the EIC7700 SoC.
> +
> +select:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - eswin,eic7700-qos-eth
> + required:
> + - compatible
> +
> +allOf:
> + - $ref: snps,dwmac.yaml#
> +
> +properties:
> + compatible:
> + items:
> + - const: eswin,eic7700-qos-eth
> + - const: snps,dwmac
> +
> + reg:
> + description: Base address and size of the Ethernet controller registers
Drop description, wasn't here.
> + items:
> + - description: Register base address
> + - description: Register size
The second item makes no sense. Size is part of one entry. Looking at
your example you have only one item. Last time you said this is
extension region, so I really do not understand what is happening here.
> +
> + interrupt-names:
> + const: macirq
> +
> + interrupts:
> + maxItems: 1
> +
> + phy-mode:
> + $ref: /schemas/types.yaml#/definitions/string
> + enum:
> + - rgmii
> + - rgmii-rxid
> + - rgmii-txid
> + - rgmii-id
> +
> + phy-handle:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: Reference to the PHY device
> +
> + clocks:
> + minItems: 3
> + maxItems: 7
No improvements. Read feedback given to other eswin patches. This cannot
be flexible.
> +
> + clock-names:
> + minItems: 3
> + contains:
> + enum:
> + - app
> + - stmmaceth
> + - tx
> +
> + resets:
> + maxItems: 1
> +
> + reset-names:
> + items:
> + - const: stmmaceth
> +
> + dma-noncoherent: true
> +
> + # Custom properties
Drop
> + eswin,hsp_sp_csr:
Nothing improved. Eswin upstreamed several bindings at once with same
issues. I commented on multiple of them, but here I stopped commenting
asking you to fix the same things I asked your colleagues. The point is
that it is beneficial if you learn together, not each repeat same
mistake and receive same feedback from reviewer.
Follow DTS coding style naming for property.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: HSP peripheral control register area
What is HSP?
> + - description: Control registers (such as used to select TX clock
> + source, PHY interface mode)
> + - description: AXI bus low-power control related registers
> + - description: Status register
> + description:
> + Configure TX clock source selection, set PHY interface mode,
> + and control low-power bus behavior
> +
> + eswin,syscrg_csr:
Nothing improved.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: CRG's device tree node handle
What is CRG? Anyway, use the same style. If previously phandle is
control register area, why this is phandle? Drop redundant parts,
because this cannot be anything else than phandle, and describe what is
the device you are pointing here.
> + - description: Enable and divide HSP ACLK control
> + - description: Behavior of configuring HSP controller
> + description:
> + Register that accesses the system clock controller.
> + Used to configure HSP clocks for Ethernet subsystems
> +
> + eswin,dly_hsp_reg:
Nothing improved.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: TX delay control register offset
> + - description: RX delay control register offset
No... These are not phandles. Look at your example - where is a phandle
there?
> + - description: Switch for controlling delay function
> + description:
> + Register for configuring delay compensation between MAC/PHY
> +
> + snps,axi-config:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: AXI bus configuration
Do not redefine properties. Include proper ref and fix your
additionalProps to unevaluateadProperties.
> +
> + stmmac-axi-config:
Don't duplicate with snps dwmac.
> + type: object
> + unevaluatedProperties: false
> + properties:
> + snps,blen:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: Set the burst transmission length of AXI bus
> + snps,rd_osr_lmt:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Set OSR restrictions for read operations
> + snps,wr_osr_lmt:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Set OSR restrictions for write operations
> + snps,lpi_en:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: Low Power Interface enable flag (true/false)
> + required:
> + - snps,blen
> + - snps,rd_osr_lmt
> + - snps,wr_osr_lmt
> + - snps,lpi_en
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupt-names
> + - interrupts
> + - phy-mode
> + - clocks
> + - clock-names
> + - resets
> + - reset-names
> + - eswin,hsp_sp_csr
> + - eswin,syscrg_csr
> + - eswin,dly_hsp_reg
> + - snps,axi-config
> +
> +additionalProperties: false
unevaluatedProperties instead
> +
> +examples:
> + - |
> + gmac0: ethernet@...00000 {
> + compatible = "eswin,eic7700-qos-eth", "snps,dwmac";
> + reg = <0x0 0x50400000 0x0 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupt-names = "macirq";
> + interrupts = <61>;
> + phy-mode = "rgmii-txid";
> + phy-handle = <&phy0>;
> + status = "okay";
Drop
> + clocks = <&clock 550>,
> + <&clock 551>,
> + <&clock 552>;
> + clock-names = "app", "stmmaceth", "tx";
> + resets = <&reset 0x07 (1 << 26)>;
> + reset-names = "stmmaceth";
> + dma-noncoherent;
> + eswin,hsp_sp_csr = <&hsp_sp_csr 0x1030 0x100 0x108>;
> + eswin,syscrg_csr = <&sys_crg 0x148 0x14c>;
> + eswin,dly_hsp_reg = <0x114 0x118 0x11c>;
> + snps,axi-config = <&stmmac_axi_setup>;
> + stmmac_axi_setup: stmmac-axi-config {
> + snps,blen = <0 0 0 0 16 8 4>;
> + snps,rd_osr_lmt = <2>;
> + snps,wr_osr_lmt = <2>;
> + snps,lpi_en;
> + };
> + /* mdio {
Don't post dead code.
> + compatible = "snps,dwmac-mdio";
> + status = "okay";
Drop
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phy0: ethernet-phy@0 {
> + device_type = "ethernet-phy";
> + reg = <0>;
> + compatible = "ethernet-phy-id001c.c916", "realtek,rtl8211f";
> + };
> + }; */
> + };
Best regards,
Krzysztof
Powered by blists - more mailing lists