Skip to content
Projects
Groups
Snippets
Help
Loading...
Sign in
Toggle navigation
V
VME64x core
Project
Project
Details
Activity
Cycle Analytics
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Charts
Issues
0
Issues
0
List
Board
Labels
Milestones
Merge Requests
0
Merge Requests
0
Wiki
Wiki
image/svg+xml
Discourse
Discourse
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Charts
Create a new issue
Commits
Issue Boards
Open sidebar
Projects
VME64x core
Commits
f9505f15
Commit
f9505f15
authored
Nov 13, 2017
by
Tristan Gingold
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Greg review update.
parent
250f7da5
Hide whitespace changes
Inline
Side-by-side
Showing
1 changed file
with
19 additions
and
0 deletions
+19
-0
GD-vme64x-core-review.txt
doc/review-2017-11-13/GD-vme64x-core-review.txt
+19
-0
No files found.
doc/review-2017-11-13/GD-vme64x-core-review.txt
View file @
f9505f15
...
@@ -19,6 +19,10 @@
...
@@ -19,6 +19,10 @@
------------------------
------------------------
-- vme64x_pack.vhd --
-- vme64x_pack.vhd --
------------------------
------------------------
- line 44:
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
period value assigned to g_CLOCK_PERIOD if user does not overwrite it.
- 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
- cleanup, remove types that are not used e.g. c_ADER_C_XAM or c_ADER_C_AM
- cleanup, remove types that are not used e.g. c_ADER_C_XAM or c_ADER_C_AM
...
@@ -26,12 +30,21 @@
...
@@ -26,12 +30,21 @@
------------------------
------------------------
-- VME64xCore_Top.vhd --
-- VME64xCore_Top.vhd --
------------------------
------------------------
- 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
- the same for g_WB_ADDR_WIDTH (see also comment for VME_bus.vhd)
- prefix Wishbone ports with "wb_"
- prefix Wishbone ports with "wb_"
- 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).
------------------------
------------------------
-- VME_bus.vhd --
-- VME_bus.vhd --
------------------------
------------------------
- prefix Wishbone ports with "wb_"
- prefix Wishbone ports with "wb_"
- 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
- why reset is active high (rst_i) and not active low like in all other modules?
- constant num_latchDS misses "c_" prefix
- constant num_latchDS misses "c_" prefix
- line 431: 3 nested if-s. How about simplifying to:
- line 431: 3 nested if-s. How about simplifying to:
if decode_done_i = '1' and decode_sel_i = '1' and module_enable_i = '1'then
if decode_done_i = '1' and decode_sel_i = '1' and module_enable_i = '1'then
...
@@ -74,3 +87,9 @@
...
@@ -74,3 +87,9 @@
- line 170:
- line 170:
very very tiny thing, retry_count can be cleared always when we're in
very very tiny thing, retry_count can be cleared always when we're in
WAIT_IRQ (not only inside the _if_). This simplifies an already simple fsm.
WAIT_IRQ (not only inside the _if_). This simplifies an already simple fsm.
----------------------------
-- simulation --
----------------------------
- general-cores submodule in wrong location. Move it from hdl/sim to
ip_cores/general-cores like we have for all the other repos.
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment