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

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

On 30/9/2026 8:31 pm, NG, TZE YEE wrote:
> 
> 
> 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.
> 

Following up on the default question. I need to correct what I said earlier.

After Krzysztof's review I'm changing how two of these are represented, 
which also settles the defaults:

- read-dqs-delay: I said "no fixed constant", but that's wrong. When 
absent the driver uses 0 in these modes. It's now expressed in ps 
(cdns,ddr-read-dqs-delay-ps) with default: 0. (In the DDR modes the DLL 
is bypassed, where this field is a delay-element count, so ps is 
well-defined.)
- use-lpbk-dqs: now a boolean (cdns,ddr-use-lpbk-dqs); absent = phony, 
so there's no default to document.
- phony-dqs-timing: this is the only one with no fixed default. When 
absent the driver uses the value computed from the RE# pulse width 
(patch 2); that's described in the text rather than a schema default.

Thanks,
Tze Yee


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