Commit 0343da4d authored by Tristan Gingold's avatar Tristan Gingold

Update review status.

parent fa8b16ac
VME64x Core VME64x Core
=========== ===========
Using the core
--------------
There is an example in the svec repository. See svec/hdl/syn/vmecore_test
The design implements the core on a svec card, with a WB slave that implements
various features (an SRAM, a pattern area to check DMA, a simple timer to
check interrupts, a bus-error generator...).
There is a C program (in svec/software/vmecore_test) to test all these
features.
Running the testbench Running the testbench
--------------------- ---------------------
......
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
- shall we have some reference design in the VME64x core repo? With necessary - shall we have some reference design in the VME64x core repo? With necessary
constraints for the clock and putting registers in IOBs (like vme_irq_n_o in constraints for the clock and putting registers in IOBs (like vme_irq_n_o in
svec vmecore_test) svec vmecore_test)
Points to the svec repo. Done. Points to the svec repo.
- vme64x_pack.vhd -> we usually call these things *_pkg.vhd - vme64x_pack.vhd -> we usually call these things *_pkg.vhd
Done. Done.
- from files headers remove _last changes_ and _TODO_ sections, anyway, they are - from files headers remove _last changes_ and _TODO_ sections, anyway, they are
...@@ -22,7 +22,7 @@ ...@@ -22,7 +22,7 @@
the core more compact and having only an std_logic wrapper for simulations? the core more compact and having only an std_logic wrapper for simulations?
Good convention to be adopted. Good convention to be adopted.
- vme64x_pack.vhd and xvme64x_core_pkg.vhd should be merged into one package - vme64x_pack.vhd and xvme64x_core_pkg.vhd should be merged into one package
OK. Done.
------------------------ ------------------------
-- vme64x_pack.vhd -- -- vme64x_pack.vhd --
...@@ -31,7 +31,7 @@ ...@@ -31,7 +31,7 @@
rename c_CLOCK_PERIOD to something like c_default_CLK_PERIOD rename c_CLOCK_PERIOD to something like c_default_CLK_PERIOD
because as far as I've seen in the code, that's what it is, a default clk because as far as I've seen in the code, that's what it is, a default clk
period value assigned to g_CLOCK_PERIOD if user does not overwrite it. period value assigned to g_CLOCK_PERIOD if user does not overwrite it.
Default to be removed. Done. Default removed.
- line 55: - line 55:
c_ADEM_M is a type not a constant, rename it to t_ADEM_M c_ADEM_M is a type not a constant, rename it to t_ADEM_M
Add a comment. Add a comment.
...@@ -43,11 +43,11 @@ ...@@ -43,11 +43,11 @@
------------------------ ------------------------
- if g_WB_DATA_WIDTH must be 32-bit, then don't make it a generic, just use - if g_WB_DATA_WIDTH must be 32-bit, then don't make it a generic, just use
everywhere c_DATA_WIDTH declared in the package everywhere c_DATA_WIDTH declared in the package
OK: to be removed. Done. Removed.
- the same for g_WB_ADDR_WIDTH (see also comment for VME_bus.vhd) - the same for g_WB_ADDR_WIDTH (see also comment for VME_bus.vhd)
OK: to be removed. Done. Removed.
- prefix Wishbone ports with "wb_" - prefix Wishbone ports with "wb_"
OK. Done.
- if you make VME_bus.vhd with reset active low, then you don't need s_reset - if you make VME_bus.vhd with reset active low, then you don't need s_reset
signal anymore (see also comment in VME_bus.vhd). signal anymore (see also comment in VME_bus.vhd).
OK. OK.
...@@ -59,7 +59,7 @@ ...@@ -59,7 +59,7 @@
OK. OK.
- either fix adr_o handling or remove g_WB_ADDR_WIDTH because setting anything - either fix adr_o handling or remove g_WB_ADDR_WIDTH because setting anything
else than 32-bit to g_WB_ADDR_WIDTH will result in synthesis/simulation error else than 32-bit to g_WB_ADDR_WIDTH will result in synthesis/simulation error
OK. Done.
- why reset is active high (rst_i) and not active low like in all other modules? - why reset is active high (rst_i) and not active low like in all other modules?
OK. OK.
- constant num_latchDS misses "c_" prefix - constant num_latchDS misses "c_" prefix
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment