Re: [PATCH v1 4/9] dt-bindings: mmc: add binding for BST DWCMSHC SDHCI controller

[Krzysztof Kozlowski <krzk@kernel.org>]

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@bst.ai>
> Signed-off-by: Albert Yang <yangzh0906@thundersoft.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@bst.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@22200000 {

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