GD-vme64x-core-review.txt 4.38 KB
Newer Older
Tristan Gingold's avatar
Tristan Gingold committed
1 2 3
- top VME64x core entity is called "VME64xCore_Top" while
  svec/hdl/top/vmecore_test/ instantiates vme64xcore_top. Make it the same (all
  lower-case?) for easier grepping.
Tristan Gingold's avatar
Tristan Gingold committed
4
  Done.
Tristan Gingold's avatar
Tristan Gingold committed
5
- check comments in all the files, some of them are not complete or out of date
6
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
7 8 9
- 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
  svec vmecore_test)
Tristan Gingold's avatar
Tristan Gingold committed
10
  Done. Points to the svec repo.
Tristan Gingold's avatar
Tristan Gingold committed
11
- vme64x_pack.vhd -> we usually call these things *_pkg.vhd
Tristan Gingold's avatar
Tristan Gingold committed
12
  Done.
Tristan Gingold's avatar
Tristan Gingold committed
13 14
- from files headers remove _last changes_ and _TODO_ sections, anyway, they are
  empty.
15
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
16 17 18 19
- i'd prefer to have signals without "s_" prefix, but this is part of a larger
  coding style discussion.
  In this design most of the signals are with "s_" prefix, but some are also
  without (e.g. in VME_IRQ_Controller.vhd)
20
  'Detail'.
Tristan Gingold's avatar
Tristan Gingold committed
21 22
- how about using t_vme64x_in and t_vme64x_out types inside the design to make
  the core more compact and having only an std_logic wrapper for simulations?
23
  Good convention to be adopted.
Tristan Gingold's avatar
Tristan Gingold committed
24
- vme64x_pack.vhd and xvme64x_core_pkg.vhd should be merged into one package
Tristan Gingold's avatar
Tristan Gingold committed
25
  Done.
Tristan Gingold's avatar
Tristan Gingold committed
26 27 28 29

------------------------
--  vme64x_pack.vhd   --
------------------------
Tristan Gingold's avatar
Tristan Gingold committed
30 31 32 33
- 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.
Tristan Gingold's avatar
Tristan Gingold committed
34
  Done. Default removed.
Tristan Gingold's avatar
Tristan Gingold committed
35 36
- line 55:
  c_ADEM_M is a type not a constant, rename it to t_ADEM_M
37
  Add a comment.
Tristan Gingold's avatar
Tristan Gingold committed
38
- cleanup, remove types that are not used e.g. c_ADER_C_XAM or c_ADER_C_AM
39
  Add a comment as unused.
Tristan Gingold's avatar
Tristan Gingold committed
40 41 42 43

------------------------
-- VME64xCore_Top.vhd --
------------------------
Tristan Gingold's avatar
Tristan Gingold committed
44 45
- 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
Tristan Gingold's avatar
Tristan Gingold committed
46
  Done. Removed.
Tristan Gingold's avatar
Tristan Gingold committed
47
- the same for g_WB_ADDR_WIDTH (see also comment for VME_bus.vhd)
Tristan Gingold's avatar
Tristan Gingold committed
48
  Done. Removed.
Tristan Gingold's avatar
Tristan Gingold committed
49
- prefix Wishbone ports with "wb_"
Tristan Gingold's avatar
Tristan Gingold committed
50
  Done.
Tristan Gingold's avatar
Tristan Gingold committed
51 52
- 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).
53
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
54 55 56 57 58

------------------------
--    VME_bus.vhd     --
------------------------
- prefix Wishbone ports with "wb_"
59
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
60 61
- 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
Tristan Gingold's avatar
Tristan Gingold committed
62
  Done.
Tristan Gingold's avatar
Tristan Gingold committed
63
- why reset is active high (rst_i) and not active low like in all other modules?
64
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
65
- constant num_latchDS misses "c_" prefix
66
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
67 68 69 70 71 72 73 74
- 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
    (...)
  elsif decode_done_i = '1' then
    s_mainFSMstate <= WAIT_END;
  else
    s_mainFSMstate <= DECODE_ACCESS;
  end if;
75
  'As you like'.
Tristan Gingold's avatar
Tristan Gingold committed
76 77 78 79 80
- line 461:
  should you check if s_DS_latch_count >0 before decrementing it by 1?
  if for whatever reason you stay longer in DECODE_ACCESS waiting for
  decode_done_i='1' you might end up going to WAIT_FOR_DS when your counter is
  already 0, then after your decrement your counter rolls over
81 82
  OK.

Tristan Gingold's avatar
Tristan Gingold committed
83 84 85 86 87 88
--------------------------
-- VME_CR_CSR_Space.vhd --
--------------------------
- line 323:
  can use xor_reduce(vme_ga_i) from std_logic_misc instead of manually xoring
  all the bits
89
  No, because std_logic_misc is not standard.
Tristan Gingold's avatar
Tristan Gingold committed
90 91 92 93 94 95 96 97 98

--------------------------
-- VME_Funct_Match.vhd  --
--------------------------
- line 94:
  That's a weird way of describing registers in the process (at least to me).
  Instead of clearing the registers at every risinge_edge, and having null in
  rst_n_i='0', please clear the registers when rst_n_i ='0' and add another
  "else" if you want to clear them every time decode_start_i='0'.
99
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
100 101
- line 133:
  What's this construction??:
102
  mask(c_ADEM_M) := g_ADEM(s_function_sel)(c_ADEM_M);
Tristan Gingold's avatar
Tristan Gingold committed
103
  where c_ADEM_M is a subtype is integer range 31 downto  8;
104
  Covered.
Tristan Gingold's avatar
Tristan Gingold committed
105 106 107 108 109

----------------------------
-- VME_IRQ_Controller.vhd --
----------------------------
- reset_n_i, we always name it rst_n_i
110
  OK.
Tristan Gingold's avatar
Tristan Gingold committed
111 112 113
- line 170:
  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.
114
  'Tiny'.
Tristan Gingold's avatar
Tristan Gingold committed
115 116 117 118 119 120

----------------------------
--       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.
121
  Done. Cf Maciej.