Re: [PATCH v2 2/9] dt-bindings: mmc: cdns,sdhci: add SD6HC support and PHY properties

[Krzysztof Kozlowski <krzk@kernel.org>]

On Sat, Jun 27, 2026 at 01:14:47PM -0700, Tanmay Kathpalia wrote:
> Extend the Cadence SDHCI binding to support the sixth-generation SD6HC
> controller. Add the cdns,sd6hc compatible string with two named clocks
> (ciu and biu) and three SD6HC-specific PHY timing properties for iocell
> input/output delay and delay element size.
> 
> Add the altr,agilex5-sd6hc compatible string with three named reset
> lines from the Altera HPS Reset Manager. Introduce per-variant
> constraints so SD6HC and SD4HC each enforce their own clock, reset, and
> PHY property requirements independently.

You just repeated the diff. Instead describe the hardware.

> 
> Signed-off-by: Tanmay Kathpalia <tanmay.kathpalia@altera.com>
> ---
>  .../devicetree/bindings/mmc/cdns,sdhci.yaml   | 122 ++++++++++++++++--
>  1 file changed, 111 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index 6c7317d13aa6..edd96e1d2bdc 100644
> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> @@ -4,21 +4,29 @@
>  $id: http://devicetree.org/schemas/mmc/cdns,sdhci.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Cadence SD/SDIO/eMMC Host Controller (SD4HC)
> +title: Cadence SD/SDIO/eMMC Host Controller (SD4HC and SD6HC)
>  
>  maintainers:
>    - Masahiro Yamada <yamada.masahiro@socionext.com>
> +  - Tanmay Kathpalia <tanmay.kathpalia@altera.com>
>  
>  properties:
>    compatible:
> -    items:
> -      - enum:
> -          - amd,pensando-elba-sd4hc
> -          - microchip,mpfs-sd4hc
> -          - microchip,pic64gx-sd4hc
> -          - mobileye,eyeq-sd4hc
> -          - socionext,uniphier-sd4hc
> -      - const: cdns,sd4hc
> +    oneOf:
> +      - description: Cadence SD4HC controller

Drop description, you repeat the fallback compatible, so this is obvious.

> +        items:
> +          - enum:
> +              - amd,pensando-elba-sd4hc
> +              - microchip,mpfs-sd4hc
> +              - microchip,pic64gx-sd4hc
> +              - mobileye,eyeq-sd4hc
> +              - socionext,uniphier-sd4hc
> +          - const: cdns,sd4hc
> +      - description: Cadence SD6HC controller

Same here

> +        items:
> +          - enum:
> +              - altr,agilex5-sd6hc
> +          - const: cdns,sd6hc
>  
>    reg:
>      minItems: 1
> @@ -28,10 +36,12 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
>  
>    resets:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 3
>  
>    # PHY DLL input delays:
>    # They are used to delay the data valid window, and align the window to
> @@ -115,6 +125,25 @@ properties:
>      minimum: 0
>      maximum: 0x7f
>  
> +  # SD6HC PHY timing properties:
> +  cdns,iocell-input-delay:

Use standard unit suffixes from dtschema. I am pretty sure we have
picoseconds.

> +    description: Input delay across IO cells in picoseconds
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 20000   # 20 ns
> +
> +  cdns,iocell-output-delay:
> +    description: Output delay across IO cells in picoseconds
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 20000   # 20 ns
> +
> +  cdns,delay-element:
> +    description: Delay element size in picoseconds

None of these are deducible from the compatible? IOW, they differ in
each board with the same SoC?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 1000    # 1 ns
> +
>  required:
>    - compatible
>    - reg
> @@ -139,6 +168,77 @@ allOf:
>          reg:
>            maxItems: 1
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: cdns,sd6hc
> +    then:
> +      description: SD6HC variant - use IO-cell and delay element properties
> +      properties:
> +        clocks:
> +          minItems: 2
> +          maxItems: 2
> +        clock-names:
> +          items:
> +            - const: ciu
> +            - const: biu

There is no property like clock-names. Look at the schema/binding.

> +        dma-coherent: true
> +        iommus:
> +          maxItems: 1a

Do not define properties in conditional block, but top level.

> +        cdns,phy-input-delay-sd-highspeed: false
> +        cdns,phy-input-delay-legacy: false
> +        cdns,phy-input-delay-sd-uhs-sdr12: false
> +        cdns,phy-input-delay-sd-uhs-sdr25: false
> +        cdns,phy-input-delay-sd-uhs-sdr50: false
> +        cdns,phy-input-delay-sd-uhs-ddr50: false
> +        cdns,phy-input-delay-mmc-highspeed: false
> +        cdns,phy-input-delay-mmc-ddr: false
> +        cdns,phy-dll-delay-sdclk: false
> +        cdns,phy-dll-delay-sdclk-hsmmc: false
> +        cdns,phy-dll-delay-strobe: false
> +      required:
> +        - clock-names

All this clearly suggests you should have own binding file.

Best regards,
Krzysztof