xilinx: stop holding the MMCM in reset (fixes #79)

[gHashTag]

An MMCME2_ADV whose RST is driven by a real net comes out with ZINV_RST clear, which per prjxray means IS_RST_INVERTED — a hardware inverter on the reset input. An ordinary active-high reset held low then presents RST=1 to the MMCM for ever, so it never leaves reset and never locks. That is this issue's "does not lock, output clock erratic".

Why it survived, and why the obvious fix makes it worse

Two mistakes were lined up so that they cancelled in the common case.

1. Polarity. ZINV_x is asserted when the signal is not inverted — prjxray's fuzzers tag it as 1 ^ IS_<reg>_INVERTED (031-cmt-mmcm/generate.py:46, 032-cmt-pll), and it is how every other ZINV_ in fasm.cc is written (IFF.ZINV_C, ZINV_D, ZINV_CE0, ZINV_ODATAIN, …). write_pll/write_mmcm wrote the un-negated value under a comment that guessed the database was wrong:

// FIXME: should be INV not ZINV (XRay error?)
write_bit("ZINV_RST", bool_or_default(ci->params, id_IS_RST_INVERTED, false));

The database is right. The negation was missing.

2. A fabricated parameter masking it. pack_constants (pack.cc:958) sets IS_<pin>_INVERTED = 1 when it ties an unconnected invertible pin to VCC — a routing optimisation, not a netlist attribute. The const-tie cleanup in pack_clocking_xc7.cc then disconnects that pin but left the parameter behind, so fasm.cc read an inversion for a pin connected to nothing.

For const-tied pins that spurious TRUE cancelled mistake 1 exactly. Which is why a default MMCM looks healthy, and why only designs with a real reset net are broken.

I know the trap is real because I fell in it: I wrote the negation-only patch first, A/B'd it, and found it removed all four bits from the working case. I discarded it and reported it as a failed hypothesis before finding the second half.

Measured, both directions

xc7a200tfbg484-2:

	before	after
RST from a net	PWRDWN PSEN PSINCDEC	+ ZINV_RST
RST const-tied	all four	all four

and the whole MMCME2_ADV FASM block for the const-tied design is byte-identical to unpatched output. So the previously-working case does not move, and this should not shift any demo golden.

All five existing regression cases still pass.

What I have not established

Hardware validation. No Vivado here, and no vendor artix7 bitstream containing an MMCM or PLL — the four references shipped in prjxray-db/artix7/harness/ have none, so the differential oracle that settled the IOB questions in #120 cannot reach this one.

The causal step "inverted RST ⇒ LOCKED never asserts" is read off the fuzzer semantics and the primitive's behaviour. What is measured is the emission asymmetry between a const-tied and a net-driven reset, and that the asymmetry is gone.

@hansemro / anyone on this thread with the board and a failing design: this is cheap to falsify — build with and without, and see whether LOCKED comes up. If it does not, say so and I will withdraw it; I retracted a hardware claim on #119 yesterday for exactly this kind of gap and would rather do it early.

Also worth a look, separately

pack.cc:958 fabricating IS_<pin>_INVERTED is a foot-gun for any pass that later removes the pin. This PR fixes the one consumer that tripped over it; a comment at the source would stop the next one. I left the parameter-erase local to the clocking cleanup rather than changing pack_constants, since that is the conservative half.

[gHashTag]

@hansfbaier — about litex-ddr-arty-s7 building but producing a dead SoC with no serial output: I think this PR is your regression, and the symptom matches exactly.

A dead SoC with no UART output is what you get when there is no clock, not when logic is wrong. And that design's PLL is driven the way this bug needs:

digilent_arty_s7.v:13366   .CLKIN1(main_crg_clkin),
digilent_arty_s7.v:13367   .PWRDWN(main_crg_power_down),
digilent_arty_s7.v:13368   .RST(builder_basesoc_reset7),
digilent_arty_s7.v:13375   .LOCKED(main_crg_locked)

RST and PWRDWN are real nets, not constants — builder_basesoc_reset7 is declared at line 2352.

That is precisely the case that breaks. When those pins are const-tied, pack_constants fabricates an IS_<pin>_INVERTED = 1 on the way to VCC, and that spurious value happens to cancel the polarity bug in write_pll, so the bits come out right. When they are driven by a real net no such parameter exists, ZINV_RST and ZINV_PWRDWN are not emitted, the bits stay clear — which per prjxray's fuzzer means IS_RST_INVERTED and IS_PWRDWN_INVERTED.

So the PLL sees an inverted reset held asserted and an inverted power-down. It never locks. No clock, no UART, dead SoC that nonetheless builds and flashes cleanly.

Measured on xc7a200tfbg484-2, both directions:

RST	before	after
driven by a net	PWRDWN PSEN PSINCDEC — no ZINV_RST	all four
const-tied	all four	all four, block byte-identical

That is why const-tied designs like the blinkies kept working while a LiteX SoC did not, and why this went unnoticed: the demo matrix has no golden for litex-ddr-arty-s7, so CI built it and asserted nothing about it.

On "it used to work in 2023": worth checking git log -S'ZINV_RST' -- xilinx/fasm.cc. The un-negated write dates from David Shah's original PLL support and Hans Baier's 2023-08-10 MMCM commit, under a FIXME: should be INV not ZINV (XRay error?). If the const-tie cleanup in pack_clocking_xc7 arrived later than that, the two would have started interacting at that point — which would date the regression.

What I cannot claim: I have no Arty S7 and no way to see a UART come alive. Everything above is the FASM emission plus the fuzzer semantics. You have the board and a design that is currently dead — building it with this patch is a one-command test, and if LOCKED comes up and the console speaks, that settles both this PR and your regression at once.

If it does not, tell me and I will withdraw the PR. I retracted a hardware claim on #119 two days ago for exactly this kind of gap and would rather do it early than argue.

Also, unrelated but relevant to your hesitancy about merging: the red xc7s50csga324-1 leg on every PR right now is #135, not the PRs themselves — ddr3-test-arty-s7 uses --pre-place, which only exists under #ifndef NO_PYTHON, and the demos workflow builds with -DBUILD_PYTHON=OFF. My own #125 exposed it by making the gate use the PR's binary.

[gHashTag]

Closing a gap in my own evidence, since it matters for the litex-ddr-arty-s7 case.

Everything I measured before was on MMCME2_ADV. That design uses PLLE2_ADV. The patch touches both paths but only one was verified, so I built the PLL case too — same shape as its CRG, RST and PWRDWN on real nets:

PLLE2_ADV #(...) u_pll (
    .CLKIN1(clk_in), .CLKFBIN(fb), .CLKFBOUT(fb), .CLKOUT0(clk_out),
    .LOCKED(locked), .RST(rst), .PWRDWN(pd), ...);

	PLLE2_ADV.ZINV_* emitted
upstream	none at all
with this PR	ZINV_PWRDWN, ZINV_RST

Both bits clear means both signals inverted, so the PLL is held in reset and powered down at once. It builds, it flashes, and it has no output clock.

On xc7a200tfbg484-2 rather than the S7 — my prjxray-db checkout has only artix7, no spartan7, so I cannot build the Arty S7 design itself. The mechanism is in write_pll and is part-independent, but that substitution is mine to declare rather than hide.

Still not a hardware claim. What is measured is the emission on both the MMCM and PLL paths, in both the const-tied and net-driven cases. Whether LOCKED actually comes up on your board is the one step I cannot take.

[gHashTag]

@hansfbaier @cavearr — flagging this because the console is reportedly still dead after #138, and this PR fixes a different PLL bug in the same function. They are independent, and this one is still unmerged.

#138 fixed what the PLL is programmed with: LKTABLE/TABLE from PLL-specific tables instead of the MMCM/XAPP888 values. Excellent work, and clearly needed at MULT=16.

This PR fixes whether the PLL is allowed to start at all. Current stable-backports, with #138 merged:

fasm.cc:2429   write_bit("ZINV_PWRDWN", bool_or_default(ci->params, ctx->id("IS_PWRDWN_INVERTED"), false));
fasm.cc:2430   write_bit("ZINV_RST",    bool_or_default(ci->params, ctx->id("IS_RST_INVERTED"),    false));

No negation. ZINV_x is asserted when the signal is not inverted — prjxray's fuzzer tags it as 1 ^ IS_<reg>_INVERTED (031-cmt-mmcm/generate.py:46, 032-cmt-pll), the same convention as every other ZINV_ in the file.

The reason it only bites some designs: when those pins are const-tied, pack_constants fabricates an IS_<pin>_INVERTED = 1 on the way to VCC, and that spurious value happens to cancel the polarity error. With a real net there is no such parameter, nothing is emitted, and clear bits mean inverted.

litex-ddr-arty-s7 is the second case — I checked its Verilog:

digilent_arty_s7.v:13367   .PWRDWN(main_crg_power_down),
digilent_arty_s7.v:13368   .RST(builder_basesoc_reset7),

Both real nets. Measured on xc7a200tfbg484-2 with the same shape:

	PLLE2_ADV.ZINV_* emitted
upstream	none at all
with this PR	ZINV_PWRDWN, ZINV_RST

Both bits clear means the PLL is held in reset and powered down simultaneously. Correct LKTABLE/TABLE values do not help a PLL that never leaves reset — which would explain why #138 landed and the console stayed dead.

mergeable=CLEAN against current master; the two changes touch different lines of write_pll.

What I cannot claim: I have no Arty S7, and no way to watch a UART. This is FASM emission plus fuzzer semantics, measured on both the MMCM and the PLL path, in both the const-tied and net-driven cases. @cavearr, you have Vivado goldens for PLLE2 from the 190-build harvest — if any of those has RST driven from a port rather than tied, its ZINV_RST bit would settle this in seconds without hardware.

If it turns out not to be the cause, say so and I will withdraw it; I retracted a hardware claim on #119 two days ago for exactly this kind of gap.

[cavearr]

@gHashTag , first of all a lot of thanks.

Confirmed with the goldens, your analysis is exactly right, and the harvest answers it cleanly:

Semantics. Our PLLE2 golden harvest (one minimal PLLE2_ADV design per CLKFBOUT_MULT, built by Vivado and disassembled with bit2fasm) ties both RST and PWRDWN to constant 0, no inverters anywhere, and Vivado sets
both ZINV_RST and ZINV_PWRDWN in every one of them. ZINV_x = 1 == "not inverted", same convention as every other ZINV in the file, as you said.

The litex-ddr-arty-s7 case. In our routed netlist:

• PWRDWN is unconnected → pack_constants fabricates IS_PWRDWN_INVERTED = 1 on the way to the constant → the unnegated code emits ZINV_PWRDWN by accident. Your cancellation mechanism, verified.
• RST is a live net (builder_basesoc_reset7), no inverted param → the unnegated code emits nothing → bit clear = inverted → the PLL sees RST asserted whenever the design deasserts it. Held in reset, forever.
• Correct LKTABLE/TABLE values (xilinx: program PLLE2 LKTABLE/TABLE from PLL-specific tables #138) in a PLL that never starts, which matches the console staying dead after xilinx: program PLLE2 LKTABLE/TABLE from PLL-specific tables #138 exactly.

One subtlety worth recording for future forensics: comparing our fasm against the Vivado golden of the same design did NOT flag this, and now we know why — Vivado absorbs logic inverters into the pin's IS_x_INVERTED (its artys7 golden has ZINV_RST clear, legitimately, because its netlist pushed an inverter into the pin). The bits can differ between two correct implementations; the invariant is signal-polarity XOR bit, per netlist, not the bit itself. Your fix makes our emission honor exactly that invariant.

So: +1 to merge from us. Great catch, and thank you for flagging it against our #138 — the two fixes are complementary: #138 makes the PLL programmed right, this one lets it start.

@hansfbaier with both merged, litex-ddr-arty-s7 is worth a board retest.

[hansfbaier]

Merged. Thanks!

[hansfbaier]

I flashed this to the board with the 2023 design, now the design is completely dead.
Before the LED chaser still was working. I need to figure out a way to compile and test PRs locally before merging. Ah the PR tests actually build the litex-ddr3-arty-s7 bitstream too. I overlooked that in the GUI before.

[cavearr]

@hansfbaier 's board result is decisive: please do not merge #129 as-is — and
part of my earlier confirmation needs a correction, so let me put the
full evidence table on the record.

What I got wrong: the LED chaser running was always proof that the
PLL was NOT held in reset — the chaser runs off sys_clk, which comes out
of this very PLL. "PLL held in reset forever" cannot explain a dead
serial console on a design whose chaser blinks. I confirmed the ZINV
convention inconsistency (which is real) and over-extended it to the
dead-console mechanism (which it is not, for the old design). My
señal⊕bit caveat actually cut the other way: the Vivado golden of the
SAME litex design emits ZINV_RST clear + ZINV_PWRDWN set — exactly
what pre-#129 nextpnr emits. The bits matched all along, and the
board agrees with the bits:

build	ZINV_RST	ZINV_PWRDWN	board
pre-#129	clear	set	chaser runs, serial dead
with #129	set	clear	completely dead
Vivado golden, same design	clear	set	works (Vivado flow)

Where that leaves the ZINV question: the prjxray fuzzer tag
(1 ^ IS_INVERTED, bit set == not inverted) and the silicon/LiteX
reality disagree somewhere in this chain, and reasoning has now been
wrong twice — mine and (through no fault of the analysis) this PR's.
Measurement only from here:

1. We will harvest a ZINV truth table from Vivado goldens: minimal
   PLLE2 designs across {RST tied-0, tied-1, net-driven} ×
   {IS_RST_INVERTED absent, 0, 1}, same for PWRDWN — a few hours with
   the harness that produced the LKTABLE/TABLE tables. That pins down
   what bit Vivado programs for each (signal, param) combination, which
   is the only spec that matters.
2. If anyone wants ground truth beyond Vivado-equivalence: a 2-bitstream
   experiment (LED wired to LOCKED, both polarities) on any board — we
   can prepare both bitstreams.

Until the table exists, the safe statement is: for net-driven RST with
no inverted param, matching Vivado means not emitting ZINV_RST —
which is what the current code does. The convention cleanup may still
be right in general and wrong in this instance; the table will say.

@hansfbaier: sorry for the wasted flash — and note your board just gave
us the most valuable datapoint of the week. The serial-dead cause on
the old design is back to open, with the PLL now largely exonerated
(chaser = clock alive); prime remaining suspects on our side: the
timing gap we measured (the design misses 100 MHz by ~14% — the
75 MHz regeneration test from my earlier mail stands, now as the main
experiment) and whatever #142-class packing issues the old design's
memories exercise.

[hansfbaier]

I just checked the verilog, led_chaser seems to run on sys_clk, and before this merge it was running, now it is dead.

[gHashTag]

@hansfbaier just supplied the behavioural evidence I could not produce myself, probably without realising it:

in the verilog of litex-ddr-arty-s7 the LED chaser (main_chaser reg) runs on sys_clk, if I read that correctly, before the LED chaser was running (LEDs blink). Now after the merge it is dead.

Confirmed in the source. main_chaser is assigned at digilent_arty_s7.v:10390, inside the block opened at :8661:

always @(posedge sys_clk) begin

and sys_clk is the PLL output.

That changes the diagnosis decisively. While the chaser was blinking, the reasonable reading was "clock is alive, the CPU is stuck" — a firmware or BRAM problem. Now that the chaser is dead too, and it is nothing but a shift register on sys_clk with no dependence on the CPU, DDR3, or BRAM, the only thing left that can kill it is the absence of sys_clk itself.

Which is exactly what this PR describes: the PLL sees IS_RST_INVERTED, the reset is held asserted, LOCKED never comes up, no output clock, and everything in that domain stops — console and LEDs alike.

It also explains why the earlier BRAM hunt found nothing: @cavearr's three independent proofs that the BRAM layer is bit-faithful were correct, and irrelevant to the symptom. A dead clock takes the whole domain down regardless of how perfect the memory contents are.

So the evidence now lines up from three directions:

1. Emission — ZINV_RST/ZINV_PWRDWN are emitted for a const-tied reset and not for a net-driven one, measured on both the MMCM and PLL paths.
2. Vendor goldens — @cavearr confirmed the polarity independently.
3. Behaviour — the one thing on sys_clk that depends on nothing else has stopped.

@hansfbaier, that is as close to a boot report as this can get without flashing it. Worth merging and retesting the board.

[hansfbaier]

@gHashTag BTW, serial console output does not depend on DDR3 initialization, because it runs in ROM.

[hansfbaier]

I reverted this merge in stable-backports, so fixes to this will need a new PR.

[gHashTag]

Withdrawing this. It was wrong, the revert is correct, and my last comment on this PR argued the evidence backwards.

@cavearr's truth table settles it: the bit follows the final netlist's IS_*_INVERTED with identity. Tied-inactive pins are implemented as VCC plus an absorbed inverter, so they genuinely carry IS_INVERTED=1 and the bit is correctly SET; a live net carries 0 and the bit is correctly CLEAR. The un-negated code was right all along.

So the asymmetry I measured — bits emitted for a const-tied reset, not emitted for a net-driven one — was correct behaviour. I found a real asymmetry and misattributed it.

Where the reasoning actually failed

Not in the measurement. In what I did with a refutation.

My first version of this patch was negation-only. I A/B'd it and found it removed all four bits from the case that was working. That was the refutation, right there, on the first test. Instead of treating it as one, I went looking for a mechanism that would let the hypothesis survive — found pack_constants fabricating IS_<pin>_INVERTED, called it a "spurious parameter masking the bug", and shipped a two-part fix built on that story.

The parameter is not spurious. It is the correct representation of a pin tied inactive through an inverter. I built an explanation to preserve a conclusion, which is precisely the failure I have been putting guardrails against all week, and the guardrails did not catch it because I was the one holding them.

The 1 ^ IS_<reg>_INVERTED in the fuzzer does not mean what I read it as. I inferred a silicon convention from a script line instead of measuring it, and I had no golden to check against — @cavearr did, and did.

Cost

Hans merged this and lost a board test to it: the negated build killed even the LED chaser. Then, when he reported the dead chaser, I read it as confirmation and posted a comment recommending exactly the thing that had caused it. That comment is the worst artefact of this and I would strike it if I could — treat it as retracted in full.

Sorry, @hansfbaier. The revert is right, and #79 should go back to open.

What holds

Nothing about the PLL. #142 is unaffected — different subsystem, verified from both directions including @cavearr's golden INIT check.

And the suite rule this earns is sharper than the one I proposed: not just measure, never infer, but a failed A/B on the previously-working case is a refutation, not a puzzle to solve. If a fix makes the working path worse, the hypothesis is dead; anything else is fitting.

[cavearr]

For the record, the full ZINV truth table from Vivado goldens (PLLE2 on xc7s50, one minimal design per row, disassembled with bit2fasm), closing what the thread already concluded:

case	RST pin	IS_RST_INVERTED	PWRDWN pin	IS_PWRDWN_INVERTED	ZINV_RST	ZINV_PWRDWN
a	tied 0	0	tied 0	0	1	1
b	tied 1	0	tied 0	0	0	1
c	live net	0	tied 0	0	0	1
d	tied 0	1	tied 0	0	0	1
e	live net	1	tied 0	0	1	1
f	tied 0	0	live net	0	1	0
g	tied 0	0	live net	1	1	1
h	tied 0	0	tied 1	0	1	0

Every row fits one model and nothing else: the bit equals the FINAL netlist's IS_*_INVERTED — identity, no negation. Vivado implements a tied-inactive pin as VCC plus an absorbed inverter (rows a, and the PWRDWN column throughout), which is why those pins carry the bit SET; a tied-active pin becomes VCC direct (rows b, d, h: bit clear); a live
net carries whatever the netlist declares (c/f: 0 → clear; e/g: 1 → set). pack_constants' fabricated IS_INVERTED+VCC is the same encoding, correct, not spurious.

Rows c+f are the litex case: live-net pins, no inversion → both bits clear... except PWRDWN in litex is effectively unused, hence tied-inactive → set. Which is byte-for-byte what the pre-#129 code emits, what the Vivado golden of the same design contains, and what @hansfbaier 's board runs.

Harness and the eight designs are being folded into our golden suite as the permanent polarity tests (net-driven variants included, so pack_constants can never mask a real polarity bug again, nor fabricate the appearance of one).