Fix DDR primitives

[DigitalBrains1]

All of our DDR primitives turned out to have some issues. This PR initially started as fixing some issues in Xilinx oddr but quickly turned into fixing more bugs and extending the Haddock documentation.

• Clash.Explicit.DDR:
  • ddrOut: VHDL: Fix incorrect usage of Enable signal. With asynchronous resets, the Enable was not connected to the registers. Furthermore, the Enable was combinatorially(!) connected to the mux that selects the output. The mux should not be affected by the Enable at all (with the current Haskell simulation model).
  • ddrIn: VHDL: Remove bogus sensitivity list. In the initial version of this primitive, the version for asynchronous resets included the data input in its sensitivity lists for the registers. A later commit broke this, making it render the empty string for ~VARS. But the input should not be in the sensitivity list anyway, so let's just remove the ~VARS without actually changing the rendering in HDL.
    Additionally, the register outputting the data clocked in on the edge other than the active edge (ddrIn_neg_latch) also had its data input in its sensitivity list like the other two registers. This is also bogus and has been removed.
• Clash.Xilinx.DDR:
  • oddr: Verilog correctly picked one of the possible arguments to inspect for ResetKind, but the others picked the KnownNat constraint. This caused Clash to error out during HDL generation.
  • Both in and out prims: Renamed symbols that were incorrectly copy-pasted from the Intel prims.
• Clash.Intel.DDR:
  • altddioIn:
    • VHDL: Fix ~ISACTIVEENABLE parameter reference that caused HDL generation to error out.
    • Verilog: Fix width parameter; it accidentally always rendered as 1.
    • SystemVerilog: Accidental comma generated invalid HDL.
  • altddioOut: VHDL: Several numeric identifiers were incorrect, referring to wrong arguments or symbols. HDL generation completed without error, but the resulting VHDL was completely broken.
  • Both prims: evaluate the SSymbol in Haskell to fix HDL generation warning about using an argument that is unused in Haskell.

• Add test benches for Xilinx DDR primitives
  A variant for Intel is also included, but we don't have a simulator in CI that supports Intel IP, so it can only be run manually for now. If the Verilog code is to be tested in Questa Intel FPGA Edition, one needs to define the preprocessor macro INTEL_VERILOG, which will skip verifying the output of the Clash.Explicit.DDR.ddrOut primitive as Questa trips on this: VendorDDR.hs comment. SystemVerilog doesn't need this. Vivado is also fine with the Verilog code, so we actually test the Verilog variant of the vendor-neutral output DDR primitive in our nightlies now.
  The new test bench for vendor primitives tests the vendor-neutral primitives more extensively than the existing test bench did. For this reason, we also no longer run the existing test bench in Vivado, as it would not increase test coverage.
  The new test bench is very slow in generating HDL; this is due to some unfortunate GHC transformations that are hard on Clash and not easily fixed. Compensating a bit: Vivado runs all test bench configurations in parallel, saving a lot of time spent in Vivado.

• The primitives for Xilinx and Intel only support the rising edge as the active edge of the domain. This is now enforced by a constraint, instead of silently leading to a malfunctioning implementation.
• The existing constraints on the domains could be redefined in terms of DomainPeriod. Since the virtual DDR domain only exists in simulation, it really doesn't matter if it doesn't match with the real domain in all its parameters, so we can drop those and make the constraints more intuitive to read.
• The primitives for Xilinx and Intel now support any data type that has a BitPack instance, instead of being restricted to BitVector.
• The Haddock for the functions is extended, explaining the purpose of the tuple elements with data.
• The internal functions for Xilinx and Intel are now exported (under the heading Internal) in the modules, as a general principle that we give people all the pieces so they can use them if they want to do something special.

I have written a variant for Clash 1.8 that does not change the API, but uses clashCompileError to prevent people instantiating the vendor primitives using a domain where the falling edge is the active edge. Once this PR has passed review, I'll open a new PR for that one (incorporating changes that are the result of review here).

Several of the bugs were introduced in 2019 in commit 0c38d65 part of PR #527. We could not test the vendor primitives, so it went unnoticed. Now we at least test automatically in Vivado, and manually testing in Questa Intel FPGA Edition is straightforward.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[DigitalBrains1]

Without Vivado, we can't actually test these primitives.

Of course, while we didn't have Vivado back when the bug was introduced, we do now. Maybe it's even worth it to make a combined test with IDDR and ODDR prims, but just a "does this generate HDL" test is much more quickly written.

Anyway, the names in HDL are a bit weird:

        D1 => dataout_l(i),    -- 1-bit data input (positive edge)
        D2 => dataout_h(i),    -- 1-bit data input (negative edge)

Why is the data for the positive edge labeled L and the data for the negative edge labeled H? Note that Clash.Explicit.DDR.ddrOut labels them data_Pos and data_Neg respectively, which makes more sense.

[christiaanb]

I think they're called like that due to copy/paste from the Intel primitives where the output ports of the Intel primivite are called dataout_l and dataout_h:

clash-compiler/clash-lib/prims/vhdl/Clash_Intel_DDR.primitives.yaml

Lines 43 to 44 in 5706eaf

	dataout_h => ~SYM[2],
	dataout_l => ~SYM[1]

[DigitalBrains1]

Shall I name them more obviously in this PR as well? I'm thinking dataout_p and dataout_n data_pos and data_neg, after the edge that they are presented. It's similar to what Clash.Explicit.DDR.ddrOut does, but without capitalisation. I think we more regularly use underscores as word separators in variables in HDL, and to use both underscores as well as case to separate words feels redundant.

[christiaanb]

Yes, please clean up the names, and also it would be nice if we tested these primitives on Vivado at least in nightly builds. I mean... we already do that for the generic DDRout primitives:

clash-compiler/tests/Main.hs

Lines 517 to 526 in 10f26ff

	, let _opts = def{ buildTargets = BuildSpecific [ "testBenchUA"
	, "testBenchUS"
	, "testBenchGA"
	, "testBenchGS"
	]
	, hdlLoad = [Vivado]
	, hdlSim = [Vivado]
	, clashFlags=["-fclash-hdlsyn", "Vivado"]
	}
	in runTest "DDRout" _opts

[martijnbastiaan]

Wow that's a lot of magic numbers! How did they come to be?

[DigitalBrains1]

Because I definitely did not feel like writing this in a comprehensible way which would be way much more work! :-D

They come from looking at waveforms, understanding them, agreeing with them, and stuffing them in a list of expected values here.

This kind of code would be inexcusible in src/ but we have more instances of magic numbers in tests/.

Also, don't bother running the Haskell simulation against them. I still need to file some bugs that our simulation doesn't agree with HDL simulation. Although I have to say the Xilinx DDR primitive has extremely surprising timing regarding resets and enables, I actually wonder whether their simulation matches hardware.

[DigitalBrains1]

(also, note that 69 plus 8 equals 77 and 31 plus 8 equals 39, these do not actually change the length of the list of samples, they overwrite samples that were affected by a deasserted enable)