| 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: <f13fe182-b5e5-4938-8260-24f8f60e8f83@kernel.org>
Date: Fri, 16 May 2025 15:12:10 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: dongxuyang@...incomputing.com, ulf.hansson@...aro.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, linux-mmc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
adrian.hunter@...el.com, p.zabel@...gutronix.de, shanchun1218@...il.com
Cc: ningyu@...incomputing.com, linmin@...incomputing.com,
xuxiang@...incomputing.com
Subject: Re: [PATCH v1 1/2] dt-bindings: sdhci: eswin: Documentation for
eic7700 SoC
On 16/05/2025 11:16, dongxuyang@...incomputing.com wrote:
> From: Xuyang Dong <dongxuyang@...incomputing.com>
>
> Add device tree binding documentation for the ESWIN
> eic7700 sdhci controller module.
>
> Signed-off-by: Xiang Xu <xuxiang@...incomputing.com>
> Signed-off-by: Xuyang Dong <dongxuyang@...incomputing.com>
> ---
Folks,you need to slow down because you are sending patchsets with the
same issues, thus we need to repeat the same review comments. Learn from
the review first.
> .../bindings/mmc/eswin,sdhci-eic7700.yaml | 131 ++++++++++++++++++
> 1 file changed, 131 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml
>
> diff --git a/Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml b/Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml
> new file mode 100644
> index 000000000000..d4826f2aa619
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/eswin,sdhci-eic7700.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ESWIN EIC7700 SoC SDHCI controller
> +
> +maintainers:
> + - Shuang Liang <liangshuang@...incomputing.com>
> + - Xuyang Dong <dongxuyang@...incomputing.com>
> +
> +allOf:
> + - $ref: /schemas/mmc/mmc-controller.yaml#
> +
> +properties:
> + compatible:
> + const: eswin,eic7700-emmc-sdhci
> +
> + reg:
> + maxItems: 1
> + description: Common configuration registers
Blank line, drop description
> + "#address-cells":
> + const: 1
Blank line
> + "#size-cells":
> + const: 1
This is wrong.
Why do you need these properties? You are duplicating (and wrongly) MMC
schema.
> +
> + interrupts:
> + maxItems: 1
> +
> + assigned-clocks:
> + maxItems: 1
> + description: should contain "core_clk" phandle + clock-specifier pairs.
> +
> + assigned-clock-rates:
> + maxItems: 1
Drop both properties. Do you see this anywhere in the bindings?
> +
> + clocks:
> + minItems: 2
No list items instead.
> + description: handles to clock for the sdhci controller.
Drop description
> +
> + clock-names:
> + minItems: 2
> + description: the name of each clock.
No, list items instead. Do you see any bindings written like that?
> +
> + clock-output-names:
> + maxItems: 1
> +
> + '#clock-cells':
> + enum: [0]
> + description:
> + With this property in place we will export one clock
> + representing the Card Clock. These clocks are expected to be
> + consumed by our PHY.
> +
> + resets:
> + description: resets to be used by the controller.
Missing constraints. Drop description, useless.
> +
> + reset-names:
> + description: names of the resets listed in resets property in the same order.
Drop property
> +
> + bus-width:
> + enum: [4, 8]
> + description: for emmc bus-width is 8, for sdio bus-width is 4.
Why do you need it? Look at other bindings.
> +
> + eswin,hsp_sp_csr:
Look at DTS coding style first. What characters are allowed in properties?
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description: hsp_sp_csr regs to be used by the controller.
For what purpose?
> + items:
> + - description: phandle to HSP_SP_CSR register block
> + - description: status register offset
> + - description: control register offset
> + - description: configuration register offset
> +
> + eswin,syscrg_csr:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description: syscrg_csr regs to be used by the controller.
For what purpose?
> + items:
> + - description: phandle to SYS_CRG_CSR register block
> + - description: status register offset
> + - description: control register offset
> + - description: configuration register offset
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - assigned-clocks
> + - assigned-clock-rates
> + - clocks
> + - clock-names
> + - clock-output-names
> + - resets
> + - reset-names
> + - bus-width
> + - eswin,hsp_sp_csr
> + - eswin,syscrg_csr
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> +
Drop blank line
> + sdhci_emmc: mmc@...50000 {
Drop label
> + compatible = "eswin,eic7700-emmc-sdhci";
> + reg = <0x50450000 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupts = <79>;
> + clocks = <&clock 554>, <&clock 546>;
> + clock-names = "clk_xin", "clk_ahb";
> + assigned-clocks = <&clock 554>;
> + assigned-clock-rates = <200000000>;
> + clock-output-names = "emmc_cardclock";
> + #clock-cells = <0>;
> +
> + resets = <&reset 7 (1 << 6)>,
> + <&reset 7 (1 << 3)>,
> + <&reset 7 (1 << 19)>,
> + <&reset 7 (1 << 23)>;
> + reset-names = "txrx_rst", "phy_rst", "prstn", "arstn";
> +
> + core-clk-reg = <0x51828160>;
> + disable-cqe-dcmd;
> + bus-width = <8>;
> + non-removable;
> + mmc-hs400-1_8v;
> + max-frequency = <200000000>;
> + eswin,hsp_sp_csr = <&hsp_sp_csr 0x1038 0x508 0x50c>;
> + eswin,syscrg_csr = <&sys_crg 0x160 0x148 0x14c>;
> + status = "disabled";
No, drop.
Best regards,
Krzysztof
Powered by blists - more mailing lists