Hi Tanmay,
Thanks for the review. You are right on all three points.
On 19/10/2026 3:52 am, Kathpalia, Tanmay wrote:
Hi Tze,
Thanks for your patience. It took me a while to go through the Cadence
PHY and host-controller documentation to brush up the register
semantics before replying.
On 05-10-2026 13:36, NG, TZE YEE wrote:
On 4/10/2026 2:53 am, Kathpalia, Tanmay wrote:
On 21-09-2026 09:20, NG, TZE YEE wrote:
On 13/9/2026 5:06 am, Kathpalia, Tanmay wrote:
Hi Tze,
On 26-08-2026 08:23, [email protected] wrote:
From: Tze Yee Ng <[email protected]>
The PHY control register (0x2080) holds phony_dqs_timing (bits
[9:4]),
which shifts the read-capture strobe relative to the data slices.
Extend the per-mode PHY configuration arrays with a "cdns,phy-ctrl-*"
entry (default 0x41F0) and program register 0x2080 during PHY
adjustment. This lets each speed mode - and each board via device
tree -
select an appropriate value.
Signed-off-by: Tze Yee Ng <[email protected]>
---
The PHY_CTRL default needs to be corrected. All six tables also use
the
same 0x41F0, so this is not actually per-mode.
Please fix the defaults.
Regards,
Tanmay
Hi Tanmay,
0x41F0 is the PHY_CTRL (0x2080) reset value from the PHY spec. The
driver never wrote this register before, so the PHY already ran at
0x41F0 in every mode. Using it as the default in all six tables just
preserves that (the write is a no-op unless overridden), while
making the register tunable per mode/board via cdns,phy-ctrl-<mode>
in DT, e.g. on Agilex5:
013b: cdns,phy-ctrl-emmc-ddr = <0x4010>; (phony_dqs_timing = 1)
modular: cdns,phy-ctrl-emmc-ddr = <0x4000>; (phony_dqs_timing = 0)
So the defaults match only because the reset value is mode-independent;
the real tuning lives in DT. You're right that six bare 0x41F0
literals are misleading. For v3 I'll add a named constant
(SDHCI_CDNS6_PHY_CTRL_REG_DEFAULT = 0x000041F0) for the default and
reword the commit to say the register is made programmable with the
reset value as default (no functional change), overridable per mode/
board via DT.
Does that work for you, or did you expect distinct per-mode defaults? I
don't have validated per-mode values, that tuning is meant to live
in DT.
I expect mode-specific, validated defaults, consistent with the other
PHY configuration values in each mode table.
Please determine the appropriate default for each speed mode rather
than using 0x41F0 universally. Board-specific DT values may still
override those defaults where required.
Hi Tanmay,
For v3, I'll drop the universal 0x41F0 and the per-mode cdns,phy-ctrl
table entirely. The PHY_CTRL reset value already works for the SD/eMMC
SDR and HS modes, so there are no per-mode defaults to validate there,
the driver simply leaves PHY_CTRL untouched in those modes.
I think the per-mode programming should remain, with corrected and
validated defaults. PHY_CTRL is not reset on each timing-mode
transition, so leaving it untouched can carry the previous mode’s
phony_dqs_timing into the next mode. Section 9.1 also requires
programming this field during every speed-mode update.
Agreed. v2 programs PHY_CTRL on every speed-mode update, not just for
DDR.
Only the DDR read path is sensitive to phony_dqs_timing, so the driver
programs it just for DDR50/DDR52 via a single mode-neutral property,
cdns,ddr-phony-dqs-timing (shared by SD DDR50 and eMMC DDR52). It
defaults to 0 (the neutral assertion timing)
I do not see this supported by the PHY guide. It defines the value
from extended_read_mode: zero when disabled, and RE# pulse width in
PHY-clock cycles minus one when enabled. DDR50/DDR52 enable
extended read, so zero is not documented as a neutral default.
Please provide the documented RE# pulse width before selecting a
default.
You're right that 0 isn't documented. The documented rebar (RE#) pulse
width is 2 clk_phy cycles (Cadence SD/eMMC DFI spec, extended-clock
DDR), so by the guide's formula phony_dqs_timing = rebar_width - 1 is 1
in extended-read mode and 0 otherwise.
v2 will derives it from the per-mode EXTENDED_RD_MODE bit
REBAR_PULSE_CYCLES = 2) instead of hardcoding; the value 1 matches our
DDR50 hardware and the Linux change. Only DDR50/DDR52 actually observe
it (the other modes sample on the loopback DQS path). This also fixes
the current 0x41F0, which programs phony_dqs_timing = 0x1F (31) --
either the reset nor a documented value.
and a board whose DDR read
path needs tuning sets it explicitly: SoCDK, eMMC and 013B use 1, while
the modular SoM's longer SD path uses the default 0.
The guide does not define this field from PCB path length; board
delay is handled by the IO timing parameters.
Agreed, I'll drop the path-length wording. With the derived default
every board gets phony_dqs_timing = 1 in extended-read DDR, so no board
needs a phony value for flight time, that is handled by read_dqs_delay
and the IO timing parameters. I'll keep cdns,ddr-phony-dqs-timing only
as an optional override for the exceptional case where a board's rebar
pulse genuinely differs from the design intent, not tied to trace length.
I will send v2 with these changes.
Thanks,
Tze Yee
