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

Hi Krzysztof,

Thanks for the review.

On 6/29/2026 12:34 PM, Krzysztof Kozlowski wrote:
> 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.

Ack, I'll update the commit message to describe the SD6HC hardware
instead of summarizing the changes.
>
>> Signed-off-by: Tanmay Kathpalia <[email protected]>
>> ---
>>   .../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 <[email protected]>
>> +  - Tanmay Kathpalia <[email protected]>
>>   
>>   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.

I'll remove the redundant descriptions.
>
>> +        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

Ack.

>
>> +        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.

I'll rename these properties to use the standard dtschema unit
suffixes.

>
>> +    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?

These values are board/platform dependent and are provided by
the platform integration rather than being fixed by the controller
compatible.

>
>> +    $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.

Ack, I'll fix this.

>
>> +        dma-coherent: true
>> +        iommus:
>> +          maxItems: 1a
> Do not define properties in conditional block, but top level.

I'll move them to the top level as suggested.

>
>> +        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.

Agreed. I'll split the SD6HC support into a separate binding
file and address the above comments in v3.

>