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

["NG, TZE YEE" <tze.yee.ng@altera.com>]

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@altera.com wrote:
>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>
>> 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@altera.com>
>> ---
>>   .../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