Re: [PATCH 3/4] dt-bindings: mmc: cdns,sdhci: add SD6HC DDR50 read-path tuning

[NG, TZE YEE]

On 26/9/2026 6:48 pm, Kathpalia, Tanmay wrote:
> The subject should use cdns,sd6hc because that is the binding changed by 
> this
> patch. cdns,sdhci refers to the SD4HC binding.

Agreed. I will change to "dt-bindings: mmc: cdns,sd6hc:" in v2.

> On 22-09-2026 16:42, tze.yee.ng@xxxxxxxxxx wrote:
> > From: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
> >
> > DDR50 has no CMD19 tuning, so the SD6HC read path must be centred by
> > static, board/SoC-characterised PHY settings. Add three optional SD6HC
> > properties:
>
> The properties are specific to SD UHS DDR50, but the cover letter says 
> they will
> also be used by socfpga_agilex5_socdk_emmc. Since eMMC uses modes such 
> as DDR52
> rather than UHS DDR50, please clarify this and correct either the cover 
> letter or
> the property names and driver handling as appropriate.

They're meant for both SD DDR50 and eMMC DDR52 - both are extended-read 
DDR modes with no CMD19 tuning. You're right the naming and handling 
didn't match that. In v2, I'll rename them from cdns,ddr50-* -> 
cdns,ddr-* and apply them in DDR52 as well, and fix the cover
letter to say both modes.

> >    - cdns,ddr50-read-dqs-delay:   DLL_SLAVE[7:0] read-DQS delay that 
> > centres
> >      the read eye (0-255).
> >    - cdns,ddr50-use-lpbk-dqs:     DQS_TIMING[21] read-DQS source
> >      (0 = phony, 1 = loopback).
> >    - cdns,ddr50-phony-dqs-timing: PHY_CTRL[9:4] phony DQS assertion 
> > timing
> >      (0-63) that positions the fabricated strobe relative to the 
> > returning
> >      DDR data; not produced by the Cadence timing calculation.
>
> Please drop cdns,ddr50-phony-dqs-timing. The PHY guide defines this 
> field from
> extended_read_mode and the RE# pulse width. It is not a board flight-time
> setting, and patch 2 already calculates it.

I'd prefer to keep it with reworded. Patch 2 computes the nominal value 
from the RE# pulse width per the guide, which is correct for 
direct-attach boards. But the fabricated strobe still has to line up 
with when the DDR data actually returns, and that depends on board 
flight time: on Agilex5 modular devkit the computed value mis-samples 
and phony=0 is required - characterised on hardware. So this is a board 
override on top of patch 2's computed default, in the same class as 
read-dqs-delay. I'll reword the description so it no longer contradicts 
the guide:

default = value computed from the RE# pulse width; the property 
overrides it   for boards whose flight time shifts the DDR data return.

> > All three are disallowed for the SD4HC variant.
>
> This patch does not add an explicit SD4HC restriction. SD4HC and SD6HC use
> separate schemas, and the SD4HC schema already rejects unknown 
> properties. I
> suggest dropping this sentence.

Agreed, will drop.

> > Signed-off-by: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
> > ---
> >   .../devicetree/bindings/mmc/cdns,sd6hc.yaml   | 27 +++++++++++++++++++
> >   1 file changed, 27 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sd6hc.yaml b/ 
> > Documentation/devicetree/bindings/mmc/cdns,sd6hc.yaml
> > index d5ea2717904b..df86872603d0 100644
> > --- a/Documentation/devicetree/bindings/mmc/cdns,sd6hc.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/cdns,sd6hc.yaml
> > @@ -74,6 +74,33 @@ properties:
> >       maximum: 1000
> >       default: 24
> > +  cdns,ddr50-read-dqs-delay:
> > +    description: |
> > +      SD6HC DDR50 read-DQS delay (DLL_SLAVE[7:0]) used to centre the 
> > read
> > +      eye. DDR50 has no CMD19 tuning, so this is a board/SoC- 
> > characterised
> > +      value. If absent, the driver default is used.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 0xff
>
> default value?

No fixed constant. When the property is absent, the driver keeps the 
value it computes for the mode. I'll reword the descriptions to say that.

> > +
> > +  cdns,ddr50-use-lpbk-dqs:
> > +    description: |
> > +      SD6HC DDR50 read-DQS source (DQS_TIMING[21]): 0 = phony DQS,
> > +      1 = loopback DQS. If absent, the driver default is used.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> > +
>
> default value?

No fixed constant. When the property is absent, the driver keeps the 
value it computes for the mode. I'll reword the descriptions to say that.

> > +  cdns,ddr50-phony-dqs-timing:
> > +    description: |
> > +      SD6HC DDR50 phony DQS assertion timing (PHY_CTRL[9:4]). 
> > Positions the
> > +      fabricated read strobe relative to the returning DDR data; the 
> > correct
> > +      value depends on the board's SD flight time and is not produced 
> > by the
> > +      Cadence timing calculation. If absent, the driver default
> > +      (REBAR_PULSE_CYCLES-1) is used.
>
> The description conflicts with the PHY guide, which derives this value 
> from the
> RE# pulse width rather than PCB flight time.

I will reword the description in v2 to say the driver derives the value 
from the RE# pulse width per the guide, and the property only overrides 
that computed value for boards whose DDR data return is shifted (e.g. 
modular/SoM flight time). No static default is documented since the 
fallback is the computed value.

Thanks,
Tze Yee