| 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: <e69145c0-6303-40a1-a33e-9173d72b04e5@kernel.org>
Date: Wed, 28 May 2025 13:48:59 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Albert Yang <yangzh0906@...ndersoft.com>,
Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Ge Gordon <gordon.ge@....ai>
Cc: BST Linux Kernel Upstream Group <bst-upstream@...ai.top>,
linux-arm-kernel@...ts.infradead.org, linux-mmc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 4/9] dt-bindings: mmc: add binding for BST DWCMSHC
SDHCI controller
On 28/05/2025 10:54, Albert Yang wrote:
> Add device tree binding documentation for the Black Sesame Technologies
> (BST) DWCMSHC SDHCI controller.
>
> This binding describes the required and optional properties for the
> bst,dwcmshc-sdhci compatible controller, including register layout,
> interrupts, bus width, clock configuration, and other controller-specific
> features.
Completely redundant paragraph, drop.
A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Signed-off-by: Ge Gordon <gordon.ge@....ai>
> Signed-off-by: Albert Yang <yangzh0906@...ndersoft.com>
> ---
> .../bindings/mmc/bst,dwcmshc-sdhci.yaml | 115 ++++++++++++++++++
> 1 file changed, 115 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/bst,dwcmshc-sdhci.yaml
>
> diff --git a/Documentation/devicetree/bindings/mmc/bst,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/bst,dwcmshc-sdhci.yaml
> new file mode 100644
> index 000000000000..429e7f50cdec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/bst,dwcmshc-sdhci.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/bst,dwcmshc-sdhci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Black Sesame Technologies DWCMSHC SDHCI Controller
> +
> +maintainers:
> + - Ge Gordon <gordon.ge@....ai>
> +
> +description: |
> + The BST DWCMSHC SDHCI controller is a Synopsys DesignWare Mobile Storage Host
> + Controller IP integrated in BST SoCs.
> +
> + This documents the differences between the core properties in mmc.yaml and the
> + properties used by the sdhci-bst driver.
redundant paragraph, drop.
Missing ref to proper schema. Look how other bindings do it.
> +
> +properties:
> + compatible:
> + const: bst,dwcmshc-sdhci
You have to use soc specific compatibles. Just look at other bindings.
> +
> + reg-names:
> + const: base
> + description: Specify the register name
Drop reg-names, useless.
> +
> + reg:
> + maxItems: 1
> + description: Host controller base address
Drop description, redundant.
> +
> + interrupts:
> + maxItems: 1
> + description: One MMC interrupt should be described here
Drop description, redundant.
> +
> + interrupt-names:
> + items:
> + - const: IRQDWMMC0
Drop interrupt-names
> +
> + non-removable:
> + type: boolean
> + description: Non-removable slot (like eMMC)
> +
> + bus-width:
> + description: Number of data lines
> + enum: [1, 4, 8]
> +
> + clock-frequency:
> + description: Base clock frequency in Hz
> +
> + max-frequency:
> + description: Maximum clock frequency in Hz
> +
> + fifo-depth:
> + description: |
> + FIFO depth in bytes. If this property is not specified, the default value
> + of the fifo size is determined from the controller registers.
> +
All of above are redundant, drop.
> + mmc_crm_base:
> + description: Base address of MMC CRM registers
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + mmc_crm_size:
> + description: Size of MMC CRM registers
> + $ref: /schemas/types.yaml#/definitions/uint32
No clue what are these but they look completely wrong. Why would you
define address size as different property than reg?
> +
> + memory-region:
> + maxItems: 1
> + description: Specify the MMC DMA buffer range
> +
> + sdhci,auto-cmd12:
> + type: boolean
> + description: Enable auto CMD12 support
Drop, your compatible defines it, there is no vendor sdhci and this is I
think legacy
> +
> + dma-coherent:
> + type: boolean
> + description: Enable coherent DMA operations
Drop description, redundant.
> +
> +required:
> + - compatible
> + - reg-names
> + - reg
> + - interrupts
> + - interrupt-names
> + - non-removable
> + - bus-width
> + - clock-frequency
> + - max-frequency
> + - fifo-depth
> + - mmc_crm_base
> + - mmc_crm_size
> +
> +examples:
> + - |
> + dwmmc0@...00000 {
Never tested
> + status = "okay";
Drop
> + compatible = "bst,dwcmshc-sdhci";
> + reg-names = "base";
Follow DTS coding style for order and naming style.
> + reg = <0x0 0x22200000 0x0 0x1000>;
> + interrupts = <0x0 0x90 0x4>;
> + interrupt-names = "IRQDWMMC0";
> + #address-cells = <0x2>;
> + #size-cells = <0x0>;
> + clock-frequency = <200000000>;
> + max-frequency = <200000000>;
> + mmc_crm_base = <0x23006000>;
> + mmc_crm_size = <0x1000>;
> + fifo-depth = <0x400>;
> + bus-width = <8>;
> + non-removable;
> + sdhci,auto-cmd12;
> + dma-coherent;
> + memory-region = <&mmc_dma_buf>;
> + };
> +
> +additionalProperties: true
This cannot be true. Do you see any device binding like that? No. This
means you are doing something completely different. Please do not
reinvent this but take latest reviewed binding and customize it.
> \ No newline at end of file
You have patch warnings.
Best regards,
Krzysztof
Powered by blists - more mailing lists