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

["Kathpalia, Tanmay" <tanmay.kathpalia@altera.com>]

On 06-11-2026 11:46, 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).
>
> On this integration the rebar pulse is 2 clk_phy cycles, so extended-

In the U-Boot thread you said this width is documented in the Cadence 
SD/eMMC DFI spec; in v1 here
you said there is no IP-level document. Which is it? If documented, 
please cite it here and in the
macro comment. If it is Agilex5-specific, please take it from the 
altr,agilex5-sd6hc match data
instead of a constant for every cdns,sd6hc user.

> read DDR needs 1; confirmed on DDR50 hardware (1 captures both beats, 2

In v1 you confirmed DDR52 was validated too and said you would name both 
modes.

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

In v1 you agreed this is not proof ("Agreed the single-edge argument 
alone is not proof") and kept
these modes at 0 because they pass there. On 5 Nov, in the U-Boot 
thread, you wrote the opposite
rule: by the PHY guide the value "is 1 in extended-read mode and 0 
otherwise", and U-Boot v2 does
set 1 for the divided SDR modes. This patch still leaves them at 0, so 
the two trees disagree. The
PHY guide is not limited to DDR: 0 when extended read is off, and the 
rebar width minus 1 when it
is on. With the rebar width of 2, that is 1 in every extended-read mode. 
Please program 1 in these
modes here as well.

>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> ---
> Changes in v3:
> - No change; retains Adrian's Acked-by.
>
> Changes in v2:
> - Added Adrian's Acked-by.
> - Reworded the commit message to drop the "generic to any SD6HC-PHY SoC"
>    framing. (Adrian Hunter)
> ---
>   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 22d56bb46d75..8d6a9c26c139 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
> +

The commit says "this integration", the comment says "this controller". 
Please make them match,
with a source (see above).

>   /* 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;
> +

u8 with /* bits [9:4] */, similar to the other cp_* fields.

>   	/* 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;
> +

The PHY guide asks for width - 1 whenever extended read is on, which 
here also covers DS/HS/SDR
modes. In U-Boot v2 you now do exactly that (1 in every extended-read 
mode), and said it matches
the Linux change, but this patch keeps those modes at 0. It's confusing, 
I would suggest to follow
the Cadence PHY user guide.

>   	if (strobe_dat) {
>   		/* dqs loopback input via IO cell */
>   		hcsdclkadj += phy->iocell_input_delay;
> @@ -715,6 +728,8 @@ int sdhci_cdns6_phy_init(struct sdhci_cdns_priv *priv)
>   
>   	reg = sdhci_cdns6_read_phy_reg(priv, SDHCI_CDNS6_PHY_CTRL_REG);
>   	reg &= ~SDHCI_CDNS6_PHY_CTRL_PHONY_DQS_TIMING;
> +	reg |= FIELD_PREP(SDHCI_CDNS6_PHY_CTRL_PHONY_DQS_TIMING,
> +			  phy->cp_phony_dqs_timing);

This write always runs with the loopback DQS selected. The setting you 
tested was the phony DQS
(use_lpbk_dqs = 0) and a read delay of 96. The PHY guide says this field 
times the phony strobe,
so with loopback it may change nothing. Does this patch by itself fix 
the bad reads on the
default loopback path? If not, please put it after patch 4, or merge it 
into patch 4.