Denia's review comments

543aae9b · Denia Bouhired-Ferrag · 0c572bd8 · 543aae9b
Commit 543aae9b authored Mar 15, 2017 by Denia Bouhired-Ferrag
Hide whitespace changes
Inline Side-by-side

Showing with 108 additions and 0 deletions

denia_comments.txt doc/review_02032017/denia_comments.txt +108 -0

No files found.
--- a/doc/review_02032017/denia_comments.txt
+++ b/doc/review_02032017/denia_comments.txt
+masterFIP review comments: 
+
+General comments: 
+==================
+- Very readable code, useful comments, and very useful diagrams at the top.
+- Repo seems to be in good order.
+- Most processes are missing the p_ prefix.
+- Optionally, use the name of the process at the end, i.e.: end process p_process_name. 
+- Indentation at some places needs to be corrected.
+
+fmc_massterFIP_core.vhd
+========================
+- Line 517, 534 and 549: Expression core_rst = '1' or rx_rst = '1' used in 3 places.
+Use one signal assignement and use that instead for optimisation.
+- Same for core_rst_n = '0' or fd_host_rst = '1' which is used in various places.
+- Some indentation may be improved, eg in: cmp_masterfip_tx.
+- fd_wdgn signal should be renamed fd_wdg_n, same for fd_rstn_o-> fd_rst_n_o
+- I know chipscope is commented, but, the port map has the wrong syntax 
+(<= instead of => and the semicolons at the end) 
+
+masterfip_tx: 
+=============
+- I found the suffixes to express 1-tick delays a little confusing. You 
+    have in the same process: 
+    line 304 last_data_byte_p_d <= last_data_byte_p_tmp; 
+            last_data_byte_p <= last_data_byte_p_d; 
+    And this: 
+    line 307 
+            byte_request_accept_p_d1 <= byte_request_accept_p_tmp; 
+            byte_request_accept_p_d2 <= byte_request_accept_p_d1; 
+    Why not use the same suffix scheme? preferably use "_dn" for both sets of expressions, 
+    in agreement with VHDL guidelines table 3 p12. "_tmp" can be replaced by 
+    "_d0"? 
+
+
+masterfip_rx: 
+=============
+- s_ suffix for signals is optional. I think its use is especially 
+redundant since it is not used for all the signals in this module. 
+
+- Line 270: create_32bit_words process name should have the p_ prefix. 
+- Line 300: use p_ prefix for process name
+- Line 263: bytes_c_rst expression: 
+    bytes_c_rst <= '1' when (rst_i = '1' or rx_rst_i = '1') else '0'; 
+  effectively means that bytes_c_rst is the same value as (rst_i = '1' or rx_rst_i = '1'). 
+  Make it as an assignement (?) 
+    bytes_c_rst <= rst_i or rx_rst_i;
+
+- The expression (rst_i = '1' or rx_rst_i = '1') is evaluated at 4 
+separate occasions. Would it be better to assign it to a signal and 
+perform the OR only once? (ties in with previous remark). 
+
+- In process data_transfer_to_regs:
+    - add p_ prefix to process name.
+    - The operation:  word32_num             <= word32_num + 1;
+    seems to be performed no matter what the condition is (apart from reset condition)
+    could be performed once straight after the first else?
+    - In...
+    .....
+          if word32_num = 0 then             
+            word32_num             <= word32_num + 1;
+            rx_frame_o(word32_num) <= byte0 & byte1 & byte2 & byte3;
+
+          elsif (unsigned(rx_byte_index)-2) mod 4 = 3 then -- [CRC|CRC|BYTE|BYTE]
+            word32_num             <= word32_num + 1;
+            rx_frame_o(word32_num) <= byte0 & byte1 & byte2 & byte3;
+    .....
+            
+ It looks like the two conditions could be grouped under one condition 
+with an OR as the outcome is the same. In fact, it seems that most 
+outcomes are the same apart from Line 338. It seems the code could be 
+much more compact. 
+
+
+leds_manager.vhd
+=================
+Could this core be made more generic by removing all the TDC-specific 
+references in comments and port names and moved inside general core ? 
+
+
+- Line 96: add prefix g_ for generic values_for_simul 
+- In process pulse_generator, all acam_channel(2 downto 0) values other 
+than 0, 1, 2, 3 result in ch5 LED being lit. Is this okay functionally? 
+Wouldn it better to add the "="100" condition and send all the other 
+possibilities to the final else statement (all channels off?) 
+
+decr_counter.vhd
+=================
+- Use c_ prefix for constants.
+- Couldn't signal one <= zeroes + "1" be made into a constant, something like:
+    constant c_one : unsigned (width-1 downto 0):= to_unsigned (1, width-1) (?)
+- Is counter_is_zero_o supposed to be 1-tick long? if so add _p suffix, 
+    else it could be assigned outside the process like this? 
+    counter_is_zero_o <= '1' when counter = zeroes else "0";
+- Also seems like since decrement operation is present in most 
+    conditions, the code could be made more compact.
+
+incr_counter.vhd
+=================
+- s_counter does not need prefix.
+
+
+
+
+
+
+
+