Re: [PATCH 2/4] mmc: sdhci-cadence6: program PHONY_DQS_TIMING for extended-read DDR

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

On 26/9/2026 6:45 pm, Kathpalia, Tanmay wrote:
> 
> On 22-09-2026 16:42, tze.yee.ng@altera.com wrote:
>> From: Tze Yee Ng<tze.yee.ng@altera.com>
>>
>> The SD6HC PHY left PHONY_DQS_TIMING (phy_ctrl_reg[9:4]) at 0 in all
>> modes. Per the Cadence DLL PHY documentation it must be the rebar (RE#)
>> pulse width in clk_phy cycles minus 1 in extended read mode, and 0
>> otherwise. Leaving it 0 in extended-read DDR duplicates one DDR edge
>> (the silent odd/even edge-capture defect).
>>
>> This controller's rebar pulse is a fixed 2 clk_phy cycles, so extended-
>> read DDR needs 1; confirmed on DDR50 hardware (1 captures both beats, 2
> 
> The code also applies this value to eMMC DDR52. Was the same behavior 
> verified in
> DDR52? If so, please mention both modes; otherwise, please test DDR52 as 
> well.
> 

Yes. The final setting (phony_dqs_timing=1, use_lpbk_dqs=0, 
read_dqs_delay=96) was validated on Agilex5 eMMC DDR52 as well as 
Agilex5 SD UHS-I DDR50, in both U-Boot SPL and Linux, read + write with 
byte-exact read-back. I will reword the commit message to name both modes.
> Also, please provide a reference showing that the SD6HC REBAR pulse is 
> fixed at
> two clk_phy cycles. The PHY guide defines the formula, but not the two- 
> cycle
> pulse width.
> 
> 

You are right. The formula (value = rebar pulse width - 1 while extended 
read is active) is from the Cadence DLL PHY guide. The "2 clk_phy 
cycles" is this controller integration's pulse. I don't have an IP-level 
document stating it generically. It is bracketed empirically:

- phony_dqs_timing=0 produces a silent odd/even read duplication (every 
odd byte replaced by a copy of the preceding even byte, zero controller CRC)

- phony_dqs_timing=1 captures both DDR beats cleanly on every card/board

- phony_dqs_timing=2 shifts the strobe a full clk_phy cycle and 
re-corrupts.

So the pulse is 2 and the correct value is 1. I will drop "generic to 
any SD6HC-PHY SoC" and describe it as this integration's value - it is 
not even SoC-generic, as the same SoC's modular SoM variant needs 0 due 
to its longer SD flight time (handled by a follow-up device-tree override).

>> corrupts reads). Derive it from the extended-read-mode state and apply
>> it only in DDR modes, since SDR extended-read samples a single edge and
>> is unaffected.
> 
> The driver sets sdhc_extended_rd_mode whenever t_sdclk != t_sdmclk, so 
> extended
> read is also on for divided SDR modes. Sampling only one edge may 
> explain why the
> corruption was observed in DDR, but it does not establish that zero is the
> correct value for extended-read SDR.
> 
> 

Agreed the single-edge argument alone is not proof. Empirically, the
divided-SDR extended-read modes pass with the field left at 0: on this
hardware only DDR50 ever exhibited the defect (SDR12/25/50/104 and HS 
all pass). So 0 is the verified-good value for the extended-read SDR 
modes, and I scope the non-zero value to DDR to avoid moving modes that 
already pass onto an untested value. I can switch to the simpler
`if (extended_rd_mode) ... else 0` form if you prefer, but that would 
put the SDR modes on an unvalidated setting. I would rather keep the DDR 
gate unless you feel strongly.

>> This is generic to any SD6HC-PHY SoC, so it is kept
>> separate from the per-SoC read-path tuning.
> 
> Also, "this controller" and "generic to any SD6HC-PHY SoC" are not the same
> claim. The PHY guide's example is a 4-cycle RE# pulse, programmed as 3. 
> I do not
> see a statement that this pulse is fixed at 2 clk_phy cycles. Please 
> cite where 2
> comes from, and whether that width is SD6HC IP behavior or specific to this
> integration.
> 
>>
>> Signed-off-by: Tze Yee Ng<tze.yee.ng@altera.com>
>> ---
>>   drivers/mmc/host/sdhci-cadence-phy-v6.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-cadence-phy-v6.c b/drivers/mmc/ 
>> host/sdhci-cadence-phy-v6.c
>> index 35f35ef9c710..84592ae42762 100644
>> --- a/drivers/mmc/host/sdhci-cadence-phy-v6.c
>> +++ b/drivers/mmc/host/sdhci-cadence-phy-v6.c
>> @@ -90,6 +90,9 @@
>>   #define SDHCI_CDNS6_PHY_CTRL_REG            0x2080
>>   #define   SDHCI_CDNS6_PHY_CTRL_PHONY_DQS_TIMING            
>> GENMASK(9, 4)
>> +/* Width of this controller's rebar (RE#) pulse in clk_phy cycles. */
>> +#define SDHCI_CDNS6_PHY_REBAR_PULSE_CYCLES    2
>> +
> 
> Same question as above. Please cite the source of 2, and make the 
> comment match
> whether this is SD6HC-generic or SoC-specific.
> 
>>   /* Default PHY settings */
>>   #define SDHCI_CDNS6_PHY_DEFAULT_IOCELL_DELAY        2500
>>   #define SDHCI_CDNS6_PHY_DEFAULT_DELAY_ELEMENT        24
>> @@ -143,6 +146,9 @@ struct sdhci_cdns6_phy {
>>       bool cp_use_phony_dqs;        /* bit [20] */
>>       bool cp_use_phony_dqs_cmd;    /* bit [19] */
>> +    /* PHY_CTRL register fields */
>> +    u32 cp_phony_dqs_timing;
>> +
>>       /* HRS07 register - IO delay Information */
>>       u8 sdhc_rw_compensate;        /* bits [20:16] */
>>       u8 sdhc_idelay_val;        /* bits [4:0] */
>> @@ -517,6 +523,13 @@ static void sdhci_cdns6_phy_calc_dat_in(struct 
>> sdhci_cdns6_phy *phy)
>>       if (phy->mode == MMC_TIMING_MMC_HS200)
>>           phy->cp_read_dqs_delay = phy->hs200_tune_val;
>> +    if (phy->sdhc_extended_rd_mode &&
>> +        (phy->mode == MMC_TIMING_UHS_DDR50 ||
>> +         phy->mode == MMC_TIMING_MMC_DDR52))
>> +        phy->cp_phony_dqs_timing = SDHCI_CDNS6_PHY_REBAR_PULSE_CYCLES 
>> - 1;
>> +    else
>> +        phy->cp_phony_dqs_timing = 0;
>> +
> 
> Based on the register description, I would expect:
> if (phy->sdhc_extended_rd_mode)
>      phy->cp_phony_dqs_timing = SDHCI_CDNS6_PHY_REBAR_PULSE_CYCLES - 1;
> else
>      phy->cp_phony_dqs_timing = 0;