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

[Kathpalia, Tanmay]

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 <tanmay.kathpalia@xxxxxxxxxx>
> > ---
> >   .../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@xxxxxxxxxxxxx>
> > +  - Tanmay Kathpalia <tanmay.kathpalia@xxxxxxxxxx>
> >   
> >   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.

>