xilinx: IFFDELMUXE3 changes IDDR capture on silicon (direction NOT established -- do not merge) - #119
Conversation
|
Reviewing my own patch against the The two bits are mutually exclusive on the ISERDESE2 pathstd::string iobdelay = str_or_default(ci->params, ctx->id("IOBDELAY"), "NONE");
write_bit("IFFDELMUXE3.P0", (iobdelay == "IFD"));
write_bit("ZINV_D", !bool_or_default(..., "IS_D_INVERTED", false) && (iobdelay != "IFD"));
Under that reading my patch sets a contradictory pairThe So one of these is true, and I cannot tell which from the database:
What would settle itTwo more bench variants on the same rig — I would rather flag this than have it found in review. The A/B/A measurement is solid — without the bit neither edge captures, with it both do, reproducibly in both directions. What is not solid is my account of why, and the fix may need to be Happy to hold this PR until those two variants are measured, if you would prefer the explanation to arrive with the fix. |
|
@gHashTag did you accidentally delete two sub-modules? |
…ing a fix Supersedes the previous version of this branch, which (a) accidentally converted xilinx/external/prjxray-db and xilinx/external/nextpnr-xilinx-meta from submodules into symlinks pointing at a local home directory, breaking all three CI legs, and (b) asserted more than the measurement supports. The submodule damage is gone -- this touches xilinx/fasm.cc only. On the claim: toggling IFFDELMUXE3.P0 does change what the two IDDR flops output on silicon, reproduced three times in both directions. That part stands. Which of the two states is the capturing one does not: the readout was four LEDs reported by position and the position-to-led[] mapping was never fixed, so the observation does not distinguish "now captures" from "now reads the other path". The database also cuts against the polarity used here. IDELMUXE3 is an arity-2 pair and its P0 is written exactly when an IDELAYE2 drives D, so P0 means "take the delayed path". IFFDELMUXE3 documents only .P0, and the ISERDESE2 path writes it on (iobdelay == "IFD") -- the same sense. On that reading an IDDR with no IDELAYE2 wants the bit clear, and this condition is inverted. Left in place rather than dropped because the effect is real and worth the next person's time, but the branch is a draft and should not merge on this evidence.
8db3ba3 to
8b33d91
Compare
|
Two things wrong with this PR, both mine. Force-pushed a fix for the first and downgraded the second; moving it to draft. 1. It was breaking your CI for a reason that had nothing to do with the patchThe branch had converted The branch is now 2. I am withdrawing the claim in the titleThe title says the IFF "captures nothing" without this bit. I can no longer support that, and you should not merge it on the evidence I gave. What still stands: toggling What does not stand: which of the two states is the capturing one. The readout was four LEDs reported by position, and I never pinned down the position-to- And the database argues against the polarity I used:
On that reading an IDDR with no What would settle itAn indicator whose value is tied to state independently of which LED is which: one LED, distinguishable blink rates — stuck-0 dark, stuck-1 slow, toggling fast. Then the observation is "does it blink and how fast", not "which of four is lit". I have the board and can do that; I have not yet, and I would rather leave this red-flagged than merge it. Leaving the branch open as a draft because the effect is real and worth the next person's time, with the code comment rewritten to say only what is established. Related: on #65 I also posted a diagnosis today that I have since found to be wrong, and I have corrected it there. |
|
Closing this myself. The claim it rests on — that Settling it needs a Vivado-built pair (IDDR straight from the pad vs through IDELAYE2) that I cannot produce. The two designs are in this gist and issue #114 tracks the open question, so nothing is lost by closing the PR. @hansfbaier — clearing this out of the review queue deliberately, given you are focusing on review and merge. It should not have sat here as a draft after I retracted the claim. |
|
Thanks |
Fixes #114.
Measurement
ALINX AX7203 (
xc7a200tfbg484-2). The IDDR'sDis held at a constant 1 by an internal pull-up and both outputs are sticky-ORed to LEDs — so no external signal, cable or link partner is needed to see the failure. Reflashing between variants, A/B/A:IFFDELMUXE3.P0IFFDELMUXE3.P0Reproducible in both directions. The
ILOGICE3_IFFpath never selected the IFF data path, so the flops captured nothing.Relation to the reported symptom
#114 reports Q1 dead and Q2 alive on live RGMII traffic. On a static input both are dead. Same omission — with a running clock and real data, something recovers half of it; with nothing to recover, both stay at their initial value.
Verification chain
The FASM emitted by this patch is byte-identical to the hand-edited FASM used for the silicon measurement, so what was measured is what this emits. Adding the line by hand changed 7 bytes of the bitstream.
Note for review — a polarity question I could not settle
The
ISERDESE2path writesIFFDELMUXE3.P0on the opposite condition,(iobdelay == "IFD"). This patch writes it when noIDELAYE2drivesD, matching the configuration measured.One of the two is likely inverted.
prjxray-dbdocuments only.P0for this feature and no.P1, so the polarity cannot be read off the database — and I have not measured the ISERDESE2 path. Worth a second measurement before trusting that branch.A retraction
Earlier on #114 I argued from that same database asymmetry that
IFFDELMUXE3was probably not the cause: only.P0exists, so "unset" is a meaningful state, and for a plain IDDR with no IDELAY unset looked like what you want.The reasoning about the encoding was sound; the conclusion drawn from it was not. That "unset is meaningful" says nothing about whether set is a no-op, and I presented it as though it did. The measurement took one pull-up and three flashes and settles what the argument could not.
IFF.ZINV_OCLKremains deliberately unwritten — tested separately, captures bit-identical with and without.Environment:
nextpnr-xilinx 0.9.2-44-ga70ae4a8, prjxray andprjxray-dbartix7 built from source, chipdb generated forxc7a200tfbg484-2, fully open flow, macOS arm64.🤖 Generated with Claude Code