Add Xilinx dual clock FIFO primitive to `clash-cores` #2270

vmchale · 2022-07-05T14:35:01Z

This adds Xilinx's DcFifo to clash-cores. The Haskell model and the IP are both tested.

Past discussion is here; it is bit cluttered.

It's a draft PR because it depends on #2257

Still TODO:

~~Write a changelog entry (see changelog/README.md)~~ (clash-cores isn't on Hackage)
Check copyright notices are up to date in edited files
Fix full behavior and add a test for it
Fix overflow behavior and add a test for it
Remove unused / obsolete code
Test reset+full behavior

martijnbastiaan

In general this looks good!

I don't think you're leveraging Hedgehog as well as you could, so I've left a few comments on that.

tests/src/Test/Tasty/Clash.hs

tests/shouldwork/Cores/Xilinx/DcFifo.hs

clash-lib/src/Clash/Backend/VHDL.hs

clash-cores/test/Test/Cores/Xilinx/DcFifo.hs

martijnbastiaan · 2022-07-08T14:14:03Z

@DigitalBrains1 and I just talked a little about the tests that are currently there. Peter noticed that we only test the happy path; that is, when both the write side and the read side behave properly with regards to the full/empty signals:

Not writing when full is asserted
Not reading when empty is asserted

We should add two basic tests:

What happens when you write when full is asserted. Both the Haskell model and Xilinx model should drop the incoming write, but otherwise continue functioning correctly.
What happens if you read when empty. This is mostly relevant for the Haskell model: it should continue to work when this happens. I.e., the simulation shouldn't crash completely.

vmchale · 2022-07-08T14:18:25Z

Oh fair! I will get on that.

vmchale · 2022-07-08T17:19:56Z

Ok just pushed those two. It now tests that ordering remains upon overflow, and it tests that the simulation completes when it underflows, that is, the DcFifo doesn't depend on undefined values to progress!

vmchale · 2022-07-11T12:40:03Z

What I have now is inelegant - it has multiple modules since I can't simulate multiple top-level entities in vivado. This is because of #2264 (which I would like to put off for the moment).

I still need to see what happens in RTL when the driver circuit doesn't respect the full signal.

martijnbastiaan · 2022-07-11T12:41:06Z

This is because of #2264 (which I would like to put off for the moment).

Seems sensible

vmchale · 2022-07-11T14:51:50Z

This depends on my hack here, but now has the thorough tests (RTL)

vmchale · 2022-07-11T14:59:44Z

Ok, the RTL tests:

Test of different (and same) clock domains, read>write and write>read.
Test that nothing goes "backwards" even if we write when the FIFO is full. So we don't always get the expected data out, but we test that it's in the right order.

DigitalBrains1 · 2022-07-14T10:40:02Z

What I have now is inelegant - it has multiple modules since I can't simulate multiple top-level entities in vivado. This is because of #2264

That's not the only reason. If you want to have multiple top-level entities simulated from the same clash (gen) then those will need separate temporary directories and obviously also separate test names. I was planning on adding that anyway since I want to use it for Xilinx Floating in clash-cores.

Our SymbiYosys and Vivado CI environments both only support a single element in the list BuildSpecific [String], the other tools all correctly handle the case of a list with multiple elements. I wasn't planning on fixing SymbiYosys until we actually need it, there's more important things to do.

Note: I did not understand #2264 at a glance, so I might be missing something you think is obvious ;-)

martijnbastiaan

Looking good, almost done! 👀

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Explicit.hs

martijnbastiaan · 2022-07-14T10:42:58Z

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Explicit.hs

+    { writeReset :: Signal write ResetBusy -- ^ @wr_rst@
+    , isFull :: Signal write Full -- ^ @full@
+    , isOverflow :: Signal write Bool -- ^ @overflow@
+    , writeCount :: Signal write (DataCount depth) -- ^ @wr_data_count@
+    , readReset :: Signal read ResetBusy -- ^ @rd_rst@
+    , isEmpty :: Signal read Empty -- ^ @empty@
+    , isUnderflow :: Signal read Bool -- ^ @underflow@
+    , readCount :: Signal read (DataCount depth) -- ^ @rd_data_count@
+    , fifoData :: Signal read (BitVector n) -- ^ @dout@


Could you copy+paste+reformat the documentation in de datasheet for these signals?

I have found a problem I think - the rd_rst_busy signal is available only for built-in FIFOs. Which is not what we use!

Erm you mean they are only available for BlockRAM implementations, right?

Removed comment

Yeah we're not using common clock, right? We are using dual-clock.

I'll just note a few things that are no longer an issue now that we only do Independent_Clocks_Block_RAM:

Not all variants have wr_rst_busy and rd_rst_busy, even depending on the chip family (UltraScale or not).

The UltraScale variant of builtin FIFO has no input named rst; it has an srst synchronous to the write clock.

Buitin FIFO requires you to specify the properties CONFIG.Read_Clock_Frequency and CONFIG.Write_Clock_Frequency (values in MHz).

Builtin FIFO does not support data counts.

Instantiating a FIFO that has no ..._rst_busy signals does not simulate:

ERROR: [VRFC 10-718] formal port <wr_rst_busy> does not exist in entity <dcfifo>. Please compare the definition of block <dcfifo> to its component declaration and its instantion to detect the mismatch. [/tmp/clash-test-d6df6e43cacf34db/DcFifo0.topEntity/topEntity.vhdl:67]

When I edit one of the tests in the test bench to instantiate an Independent_Clocks_Builtin_FIFO, the test bench fails.

I'll simply have a look whether full is asserted in simulation.

Yes, it is asserted during the reset phase. I don't think we need the busy signals.

[edit]
Ah, found a section on reset behaviour in the product guide. For Independent Clock BlockRAM, it asserts full by default, but some modes do not have this option.
[/edit]

I tried to disable the reset_busy signals but this tripped up one of the examples (the one where we don't check overflows). I need to rethink that test.

I think we should have a test for the behaviour of full and empty during the reset phase, but it isn't trivial at all. I think I did come up with a fair test, but I'd like to make an issue for it that we need to add such a test rather than hold up this PR.

Sure. I've removed DcFifoBroken; I think that's fine if there's a plan for an RTL test.

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Explicit.hs

clash-cores/test/Test/Cores/Xilinx/DcFifo.hs

tests/src/Test/Tasty/Vivado.hs

DigitalBrains1

Ah the big one! It looks good!

A general comment: to what line length did you confgure your editor? I see multi-line comments, flowed paragraphs, with lines up to a 100 columns. Please flow those comments to 80 chars. Lines over 80 need a good reason to exist; they're not forbidden but they are discouraged.

clash-cores/src/Clash/Cores/Xilinx/Common.hs

clash-cores/src/Clash/Cores/Xilinx/DcFifo.hs

clash-cores/src/Clash/Cores/Xilinx/DcFifo/BlackBoxes.hs

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Explicit.hs

clash-cores/src/Clash/Cores/Xilinx/DcFifo/BlackBoxes.hs

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Explicit.hs

martijnbastiaan · 2022-09-12T10:14:21Z

With this comment:

I replied to a comment that asked whether we should make a CHANGELOG entry for this.

vmchale · 2022-09-12T10:25:53Z

@martijnbastiaan yes! I added the changelog entry that mentioned records but I was confused by @DigitalBrains1's comment:

we can now process records and type-level naturals

I don't see how the new code allows us to process type-level naturals.

DigitalBrains1 · 2022-09-12T11:31:45Z

I don't see how the new code allows us to process type-level naturals.

It was loose phrasing to refer to the fact we can now do termToData on data with an SNat at least when you define it as

data DcConfig depth = DcConfig { dcDepth :: !depth, ... }

and then supply an SNat n as depth, thanks to the addition of TermLiteralSNat.

I suppose your confusion is that you thought I referred to the problematic formulation of:

data DcConfig' depth = DcConfig' { dcDepth' :: !(SNat depth), ... }

which unfortunately doesn't work?

vmchale · 2022-09-12T11:43:58Z

Oh yes I see, thank you! I have just pushed the full changelog.

clash-lib/src/Clash/Core/TermLiteral.hs

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Internal/Instances.hs

.ci/gitlab/test.yml

changelog/2022-09-12T06_00_33-04_00_derive_term_data

martijnbastiaan · 2022-10-22T07:45:19Z

@vmchale The current commit message isn't valuable for future code archeology. A good commit body should mention:

What change was introduced
Why the change was introduced
Explain shortcomings / future work statements (although more often than not this is what inline -- TODO comments are for)

These aren't strict rules - sometimes reasons are completely obvious (like adding support for a Xilinx IP). Sometimes the change is fully explained in the code diff itself (do not assume code is self-explanatory though!). Sometimes there's no future work the author can think of.

Finally, in the footer it should refer to:

Any issues (if applicable)
Any co-authors (if applicable)

vmchale · 2022-10-24T10:40:27Z

Ah I'm so sorry I didn't realize the Co-authored-by lines had to be at the end of the commit 😬

DigitalBrains1 · 2022-10-24T11:54:09Z

The behavior of full/empty signals in the Haskell model is not tested.

What do you mean? It's in the test bench, right? I have to admit those are only run automatically in Vivado, which is a shortcoming we should one day fix, but I did run them manually in Haskell as well. Do you mean the test bench doesn't check cross-domain timing? That reminds me. The Haddock mentions

Note that the behavior of the FIFO in Haskell simulation does not correspond
exactly to the behavior in HDL simulation, and that neither behaviors correspond
exactly to hardware. All the different behaviors are functionally correct and
equivalent when the usage guidelines in the product guide are observed.

and the original PR cover letter said

We don't care too much about having the Haskell model correspond 1:1 to the RTL for our client's project. Please de-prioritize.

I think a message to this effect should be in the commit message.

Only a limited number of configurations are supported; see module documentation. The Haskell model does not correspond exactly to RTL. This closes bittide/bittide-hardware#59 Co-authored-by: Martijn Bastiaan <martijn@hmbastiaan.nl> Co-authored-by: Peter Lebbing <peter@digitalbrains.com>

vmchale added the needs-review label Jul 5, 2022

vmchale force-pushed the add-xilinx-fifo branch 3 times, most recently from a3ee6e2 to a5eee15 Compare July 6, 2022 16:04

vmchale marked this pull request as ready for review July 6, 2022 16:04

martijnbastiaan requested changes Jul 7, 2022

View reviewed changes

vmchale force-pushed the add-xilinx-fifo branch 2 times, most recently from 796b9a1 to 44a2205 Compare July 7, 2022 15:26

DigitalBrains1 reviewed Jul 7, 2022

View reviewed changes

clash-cores/test/Test/Cores/Xilinx/DcFifo.hs Outdated Show resolved Hide resolved

DigitalBrains1 mentioned this pull request Jul 8, 2022

Add vivado support to clash-testsuite #2257

Merged

DigitalBrains1 changed the title ~~Add xilinx fifo to clash-cores~~ Add Xilinx dual clock FIFO primitive to clash-cores Jul 8, 2022

vmchale force-pushed the add-xilinx-fifo branch from ef0b1e0 to 762f255 Compare July 8, 2022 13:29

vmchale force-pushed the add-xilinx-fifo branch from 817e2d8 to 12b3b91 Compare July 8, 2022 17:22

vmchale force-pushed the add-xilinx-fifo branch 2 times, most recently from 5e07b7f to 4061ccd Compare July 11, 2022 14:50

vmchale requested review from DigitalBrains1 and martijnbastiaan July 11, 2022 14:59

vmchale force-pushed the add-xilinx-fifo branch from 1371a9f to 5ccc6a2 Compare July 11, 2022 15:53

martijnbastiaan requested changes Jul 14, 2022

View reviewed changes

vmchale force-pushed the add-xilinx-fifo branch from 8f22ec6 to 7b63027 Compare July 14, 2022 12:50

DigitalBrains1 requested changes Jul 15, 2022

View reviewed changes

vmchale force-pushed the add-xilinx-fifo branch from a500753 to 25a3bf7 Compare July 19, 2022 14:04

vmchale force-pushed the add-xilinx-fifo branch from 8cc5f00 to ffe5f8d Compare September 12, 2022 10:39

vmchale force-pushed the add-xilinx-fifo branch from ffe5f8d to 21d37e1 Compare September 12, 2022 11:43

martijnbastiaan mentioned this pull request Sep 12, 2022

Use components in bittide in elastic-buffer-sim bittide/bittide-hardware#124

Closed

martijnbastiaan reviewed Sep 13, 2022

View reviewed changes

clash-lib/src/Clash/Core/TermLiteral.hs Outdated Show resolved Hide resolved

martijnbastiaan mentioned this pull request Sep 14, 2022

Add termTypePrec to TermLiteral to allow for TermLiteral SNat #2329

Merged

2 tasks

martijnbastiaan force-pushed the add-xilinx-fifo branch from 21d37e1 to a30641b Compare September 16, 2022 11:47

DigitalBrains1 reviewed Sep 20, 2022

View reviewed changes

clash-cores/src/Clash/Cores/Xilinx/DcFifo/Internal/Instances.hs Outdated Show resolved Hide resolved

vmchale force-pushed the add-xilinx-fifo branch from 3a03c2d to f15a75c Compare October 3, 2022 11:04

vmchale force-pushed the add-xilinx-fifo branch 2 times, most recently from 61a1e6d to 1b64f65 Compare October 20, 2022 12:50

DigitalBrains1 reviewed Oct 21, 2022

View reviewed changes

.ci/gitlab/test.yml Outdated Show resolved Hide resolved

changelog/2022-09-12T06_00_33-04_00_derive_term_data Outdated Show resolved Hide resolved

vmchale force-pushed the add-xilinx-fifo branch from 71fffc6 to 3a869ec Compare October 21, 2022 15:35

vmchale enabled auto-merge (squash) October 21, 2022 15:42

vmchale disabled auto-merge October 21, 2022 15:49

vmchale force-pushed the add-xilinx-fifo branch from 15d0b00 to 7451e8f Compare October 21, 2022 15:53

vmchale force-pushed the add-xilinx-fifo branch 2 times, most recently from fb4d7aa to 688a002 Compare October 24, 2022 11:39

vmchale force-pushed the add-xilinx-fifo branch from 688a002 to aed4587 Compare October 24, 2022 12:15

DigitalBrains1 enabled auto-merge (squash) October 24, 2022 13:21

DigitalBrains1 approved these changes Oct 24, 2022

View reviewed changes

DigitalBrains1 merged commit 92e3427 into master Oct 24, 2022

DigitalBrains1 deleted the add-xilinx-fifo branch October 24, 2022 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Xilinx dual clock FIFO primitive to `clash-cores` #2270

Add Xilinx dual clock FIFO primitive to `clash-cores` #2270

vmchale commented Jul 5, 2022 •

edited

Loading

martijnbastiaan left a comment

martijnbastiaan commented Jul 8, 2022

vmchale commented Jul 8, 2022

vmchale commented Jul 8, 2022

vmchale commented Jul 11, 2022

martijnbastiaan commented Jul 11, 2022

vmchale commented Jul 11, 2022 •

edited

Loading

vmchale commented Jul 11, 2022 •

edited

Loading

DigitalBrains1 commented Jul 14, 2022 •

edited

Loading

martijnbastiaan left a comment

martijnbastiaan Jul 14, 2022

vmchale Jul 14, 2022

DigitalBrains1 Jul 14, 2022

martijnbastiaan Jul 14, 2022 •

edited

Loading

vmchale Jul 14, 2022

DigitalBrains1 Jul 14, 2022 •

edited

Loading

DigitalBrains1 Jul 14, 2022 •

edited

Loading

vmchale Jul 14, 2022 •

edited

Loading

DigitalBrains1 Jul 15, 2022

vmchale Jul 19, 2022

DigitalBrains1 left a comment

martijnbastiaan commented Sep 12, 2022

vmchale commented Sep 12, 2022

DigitalBrains1 commented Sep 12, 2022

vmchale commented Sep 12, 2022

martijnbastiaan commented Oct 22, 2022

vmchale commented Oct 24, 2022

DigitalBrains1 commented Oct 24, 2022 •

edited

Loading

Add Xilinx dual clock FIFO primitive to clash-cores #2270

Add Xilinx dual clock FIFO primitive to clash-cores #2270

Conversation

vmchale commented Jul 5, 2022 • edited Loading

Still TODO:

martijnbastiaan left a comment

Choose a reason for hiding this comment

martijnbastiaan commented Jul 8, 2022

vmchale commented Jul 8, 2022

vmchale commented Jul 8, 2022

vmchale commented Jul 11, 2022

martijnbastiaan commented Jul 11, 2022

vmchale commented Jul 11, 2022 • edited Loading

vmchale commented Jul 11, 2022 • edited Loading

DigitalBrains1 commented Jul 14, 2022 • edited Loading

martijnbastiaan left a comment

Choose a reason for hiding this comment

martijnbastiaan Jul 14, 2022

Choose a reason for hiding this comment

vmchale Jul 14, 2022

Choose a reason for hiding this comment

DigitalBrains1 Jul 14, 2022

Choose a reason for hiding this comment

martijnbastiaan Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

vmchale Jul 14, 2022

Choose a reason for hiding this comment

DigitalBrains1 Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

DigitalBrains1 Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

vmchale Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

DigitalBrains1 Jul 15, 2022

Choose a reason for hiding this comment

vmchale Jul 19, 2022

Choose a reason for hiding this comment

DigitalBrains1 left a comment

Choose a reason for hiding this comment

martijnbastiaan commented Sep 12, 2022

vmchale commented Sep 12, 2022

DigitalBrains1 commented Sep 12, 2022

vmchale commented Sep 12, 2022

martijnbastiaan commented Oct 22, 2022

vmchale commented Oct 24, 2022

DigitalBrains1 commented Oct 24, 2022 • edited Loading

Add Xilinx dual clock FIFO primitive to `clash-cores` #2270

Add Xilinx dual clock FIFO primitive to `clash-cores` #2270

vmchale commented Jul 5, 2022 •

edited

Loading

vmchale commented Jul 11, 2022 •

edited

Loading

vmchale commented Jul 11, 2022 •

edited

Loading

DigitalBrains1 commented Jul 14, 2022 •

edited

Loading

martijnbastiaan Jul 14, 2022 •

edited

Loading

DigitalBrains1 Jul 14, 2022 •

edited

Loading

DigitalBrains1 Jul 14, 2022 •

edited

Loading

vmchale Jul 14, 2022 •

edited

Loading

DigitalBrains1 commented Oct 24, 2022 •

edited

Loading