Add Xilinx `tdpbram` to `clash-cores` #2442

martijnbastiaan · 2023-03-22T11:46:06Z

Still TODO:

Write a changelog entry (see changelog/README.md)
Check copyright notices are up to date in edited files
Improve documentation
Add tests to clash-testsuite
Reject negedge active domains? (Read docs)

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

DigitalBrains1

This is a nice improvement!

I do have some change requests and things I'd like to note. In fact, more than I have time for now, and I need to finish for the day. This is a partial review. I'm afraid GitHub loses my comments if I don't submit them now; I've had problems with it before. So here is part 1.

clash-prelude/src/Clash/Explicit/BlockRam.hs

clash-prelude/src/Clash/XException/MaybeX.hs

clash-prelude/src/Clash/Explicit/BlockRam.hs

clash-prelude/src/Clash/Explicit/BlockRam/Model.hs

DigitalBrains1

Still not quite done, but it's Easter. I didn't make it...

clash-prelude/src/Clash/Explicit/BlockRam/Model.hs

clash-prelude/src/Clash/XException/MaybeX.hs

clash-cores/src/Clash/Cores/Xilinx/BlockRam/Internal.hs

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

clash-cores/src/Clash/Cores/Xilinx/BlockRam/Internal.hs

changelog/2023-03-22T16_29_05+01_00_add_maybex

clash-cores/src/Clash/Cores/Xilinx/BlockRam/Internal.hs

clash-prelude/src/Clash/Explicit/BlockRam/Model.hs

clash-prelude/src/Clash/XException/MaybeX.hs

martijnbastiaan · 2023-04-12T08:37:19Z

@DigitalBrains1 Thanks for all the comments! I think I've squashed them all now. I've removed the trailing dots, but I do feel like this is a weird rule to have (see #2442 (comment)). Open coverstations:

martijnbastiaan · 2023-04-12T12:07:51Z

@DigitalBrains1 Resolved the last few conversations, and fixed the CI error caused by a new hashable release. Should be good now?

DigitalBrains1

As discussed

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

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

clash-prelude/src/Clash/Explicit/BlockRam.hs

clash-prelude/src/Clash/Explicit/BlockRam/Model.hs

DigitalBrains1

We did this in a pair review

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

tests/shouldwork/Cores/Xilinx/TdpBlockRam.hs

clash-prelude/src/Clash/Explicit/BlockRam/Model.hs

martijnbastiaan · 2023-04-14T14:45:15Z

@DigitalBrains1 Given the amount of effort you've put in, I've added you as co-author. Thanks for catching that glaring writeRam issue.

Also:

*TdpBlockRam> runUntil id normalWritesTB
Signal sampled for 23 cycles until value True
*TdpBlockRam> runUntil id writeEnableWritesTB
Signal sampled for 18 cycles until value True

clash-prelude/src/Clash/XException/MaybeX.hs

DigitalBrains1

We discovered some more things...

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

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

clash-prelude/src/Clash/XException/MaybeX.hs

martijnbastiaan · 2023-04-17T15:02:32Z

We discovered some more things...

Why can't I add the ":sob:" emoji to that message!

tests/shouldwork/Cores/Xilinx/TdpBlockRam.hs

clash-prelude/src/Clash/XException/MaybeX.hs

Paves the way for adding a true dual port block RAM with byte enables to `clash-cores`. It adds a new data type with smart constructors called 'MaybeX', which helps programmers deal with 'XException' in their modelling code. Fixes #2438 Fixes #2351 Fixes #2350 Co-authored-by: Peter Lebbing <peter@digitalbrains.com>

Co-authored-by: Peter Lebbing <peter@digitalbrains.com>

martijnbastiaan force-pushed the add-xilinx-tdpbram branch 3 times, most recently from f86857e to a403423 Compare March 22, 2023 15:26

leonschoorl reviewed Mar 22, 2023

View reviewed changes

clash-cores/src/Clash/Cores/Xilinx/BlockRam.hs Show resolved Hide resolved

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from a403423 to 9bd7340 Compare March 23, 2023 10:26

martijnbastiaan force-pushed the add-xilinx-tdpbram branch 2 times, most recently from d34d93e to 891e8da Compare March 31, 2023 13:40

martijnbastiaan marked this pull request as ready for review March 31, 2023 13:43

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from 891e8da to 64cf289 Compare March 31, 2023 16:03

DigitalBrains1 requested changes Apr 4, 2023

View reviewed changes

DigitalBrains1 requested changes Apr 6, 2023

View reviewed changes

DigitalBrains1 requested changes Apr 7, 2023

View reviewed changes

martijnbastiaan force-pushed the add-xilinx-tdpbram branch 3 times, most recently from 04688a6 to 9a91834 Compare April 12, 2023 08:35

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from 9a91834 to e8fd438 Compare April 12, 2023 12:06

martijnbastiaan requested a review from DigitalBrains1 April 12, 2023 14:03

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from e8fd438 to a65f5e1 Compare April 12, 2023 14:09

DigitalBrains1 reviewed Apr 12, 2023

View reviewed changes

clash-prelude/src/Clash/Explicit/BlockRam/Model.hs Outdated Show resolved Hide resolved

DigitalBrains1 force-pushed the add-xilinx-tdpbram branch from eb48bca to e1480e4 Compare April 13, 2023 10:03

martijnbastiaan force-pushed the add-xilinx-tdpbram branch 2 times, most recently from b72956e to d7d590d Compare April 14, 2023 14:21

martijnbastiaan requested a review from DigitalBrains1 April 14, 2023 14:21

martijnbastiaan force-pushed the add-xilinx-tdpbram branch 2 times, most recently from 6144522 to 1ca0f57 Compare April 14, 2023 14:42

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from 1ca0f57 to b8a9f99 Compare April 14, 2023 14:45

DigitalBrains1 reviewed Apr 16, 2023

View reviewed changes

clash-prelude/src/Clash/XException/MaybeX.hs Outdated Show resolved Hide resolved

clash-prelude/src/Clash/XException/MaybeX.hs Outdated Show resolved Hide resolved

clash-prelude/src/Clash/XException/MaybeX.hs Outdated Show resolved Hide resolved

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from 47fca11 to dffef91 Compare April 16, 2023 13:19

martijnbastiaan requested a review from DigitalBrains1 April 16, 2023 13:19

DigitalBrains1 force-pushed the add-xilinx-tdpbram branch from dffef91 to 85ca42f Compare April 17, 2023 14:27

DigitalBrains1 requested changes Apr 17, 2023

View reviewed changes

DigitalBrains1 reviewed Apr 17, 2023

View reviewed changes

tests/shouldwork/Cores/Xilinx/TdpBlockRam.hs Outdated Show resolved Hide resolved

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from 9fa08a1 to ec3fa85 Compare April 18, 2023 11:51

martijnbastiaan requested a review from DigitalBrains1 April 18, 2023 12:01

DigitalBrains1 reviewed Apr 18, 2023

View reviewed changes

clash-prelude/src/Clash/XException/MaybeX.hs Outdated Show resolved Hide resolved

martijnbastiaan and others added 3 commits April 18, 2023 14:15

Prevent superfluous signal declarations in 'fromBV'

1e7615f

Add Xilinx tdpbram to clash-cores

5885ace

Co-authored-by: Peter Lebbing <peter@digitalbrains.com>

martijnbastiaan force-pushed the add-xilinx-tdpbram branch from ec3fa85 to 5885ace Compare April 18, 2023 12:15

DigitalBrains1 approved these changes Apr 18, 2023

View reviewed changes

martijnbastiaan enabled auto-merge April 18, 2023 12:24

martijnbastiaan merged commit 95ee5b9 into master Apr 18, 2023

martijnbastiaan deleted the add-xilinx-tdpbram branch April 18, 2023 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Xilinx `tdpbram` to `clash-cores` #2442

Add Xilinx `tdpbram` to `clash-cores` #2442

martijnbastiaan commented Mar 22, 2023 •

edited

Loading

DigitalBrains1 left a comment

DigitalBrains1 left a comment

martijnbastiaan commented Apr 12, 2023 •

edited

Loading

martijnbastiaan commented Apr 12, 2023

DigitalBrains1 left a comment

DigitalBrains1 left a comment

martijnbastiaan commented Apr 14, 2023

DigitalBrains1 left a comment

martijnbastiaan commented Apr 17, 2023

Add Xilinx tdpbram to clash-cores #2442

Add Xilinx tdpbram to clash-cores #2442

Conversation

martijnbastiaan commented Mar 22, 2023 • edited Loading

Still TODO:

DigitalBrains1 left a comment

Choose a reason for hiding this comment

DigitalBrains1 left a comment

Choose a reason for hiding this comment

martijnbastiaan commented Apr 12, 2023 • edited Loading

martijnbastiaan commented Apr 12, 2023

DigitalBrains1 left a comment

Choose a reason for hiding this comment

DigitalBrains1 left a comment

Choose a reason for hiding this comment

martijnbastiaan commented Apr 14, 2023

DigitalBrains1 left a comment

Choose a reason for hiding this comment

martijnbastiaan commented Apr 17, 2023

Add Xilinx `tdpbram` to `clash-cores` #2442

Add Xilinx `tdpbram` to `clash-cores` #2442

martijnbastiaan commented Mar 22, 2023 •

edited

Loading

martijnbastiaan commented Apr 12, 2023 •

edited

Loading