Code review: comments on the comments (but still not everything finished)

git-svn-id: http://svn.ohwr.org/cern-fip/trunk/hdl/design@171 7f0067c9-7624-46c7-bd39-3fb5400c0213

Code review: comments on the comments (but still not everything finished)
git-svn-id: http://svn.ohwr.org/cern-fip/trunk/hdl/design@171 7f0067c9-7624-46c7-bd39-3fb5400c0213
c2a427ea · egousiou · ad26318d · c2a427ea · c2a427ea · c2a427ea
Commit c2a427ea authored Mar 01, 2011 by egousiou
5 changed files
--- a/CodeReview_24Jan11/Gonzalo_EvasComments.txt
+++ b/CodeReview_24Jan11/Gonzalo_EvasComments.txt
+General comments:
+----------------
+I don't agree with Matthieu's and Tom's comments about entities ports. 
+I don't know if you have a guideline on this, but in my case I prefer having 
+inputs all together and outputs all together, then maybe organized by interface 
+but only within the inputs or the within the outputs.
+
+wf_cons_frame_validator.vhd
+---------------------------
+Line 173: why does s_cons_lgth_byte_ok depend on rx_fss_crc_fes_manch_ok_p_i?
+In any case the output cons_frame_ok_p_o will only be valid when 
+rx_fss_crc_fec_manch_ok_p_i is valid.
+In my opinion s_cons_lgth_byte_ok should not be forced to 0 because there 
+is (for example) an error on the crc. When debugging a simulation it is easy
+assume that if s_cons_lgth_byte_ok is 0, it's only because the length was wrong.
+In fact as the file is written now, the AND between rx_fss_crc_fes_manch_ok_p_i
+and s_cons_lgth_byte_ok on lines 205 and 206 is redundant with the 
+"if then else"  of the lines 173, 185 and 187.
+
+---> bug  corrected! input rx_crc_or_manch_wrong_p_i added and the Length byte
+     is checked if either rx_fss_crc_fec_manch_ok_p_i OR rx_crc_or_manch_wrong_p_i
+     (the output of this OR signals the end of a frame) is activated
+
+
+wf_cons_bytes_processor.vhd
+---------------------------
+Line 191: the comment could be re-phrased to 
+PORT A: connected to Wishbone interface for reading by the user.
+PORT B: used by nanoFIP memory to write into memory.
+
+---> OK! 
+
+
+Line 267: s_base_addr is an input and an output of the combinatorial process.
+Synplify should give a "combinatorial loop" warning about this.
+This line could be written as a separate process.
+
+---> s_base_addr is out of the process, as a concurrent statement
+
+
+Lines 290 to 294: the comment is not very clear. Is this to avoid writing
+the CRC in the memory?
+
+---> Yes! Extra comment added.
+
+
+In general for this module, the only reason to have the RAM block memory - and
+therefore the Wishbone interface signals wb_clk_i, wb_adr_i and data_o - 
+instantiated in this entity is to be able to integrate the multiplexor of lines
+226 and 227.
+In my opinion it would much clearer for the general structure, and for the
+functionality of this particular block, to take out the memory and instantiate
+it in an independent block along with the multiplexor for data_o.
+
+---> I think i will not follow that:-s
+
+
+wf_engine_control.vhd
+---------------------
+Line 377-380: What happens if 2 consecutive ID_DAT arrive within the silence time. 
+The second one will not be processed correctly since it will be treated as an 
+RP_DAT and therefore discarded because of wrong CTRL byte. But if the second was 
+a correct ID_DAT, nanoFIP should interpret it correctly.
+
+---> ..........................................................................
+
+
+Line 775: 
+One signal is assigned in statements that are not mutually exclusive.
+Is this process going to be synthesized the same way by all the tools?
+Inside the FOR loop there is an IF-THEN without an ELSE.
+Does it generate a latch?
+Is the construction with FOR and EXIT statement necessary?
+Line 817: The same as above
+
+---> Processes for the var identification rewritten!
+
+
+wf_rx_tx_osc.vhd
+----------------
+
+
+wf_rx_deserializer.vhd
+----------------------
+I am not sure if in case a frame is received with one bit missing inside the 
+data, but with a CRC that is correct with respect to the bits actually
+received, the frame would effectively be discarded as it should.
+(As responsible for the testbench, I take note that I should check that :-)
+
+---> bug corrected! The FES_detected_p pulse is combined with the byte_ready_p pulse, 
+     that arrives only after the reception of 8 bits.
+
+
+nanofip.vhd
+-----------
+Line 480: Coherency in signal names (the _p, or the _rx_)
+Line 474: the same....
+
+---> OK!
--- a/CodeReview_24Jan11/Javier_EvasComments.txt
+++ b/CodeReview_24Jan11/Javier_EvasComments.txt
+Comments on NanoFIP VHDL code from Javier Serrano
+
+General comments
+----------------
+
+The code is very clear and the abundance of comments makes it easy to
+follow. Some signal names are way too long though. It would be more
+useful in general to give some good comments about the meaning of
+signals next to their declaration, instead of trying to embed too much
+info in the signal name.
+
+The guidelines specify that active low signals should be suffixed with
+_n. Sometimes we see signals or ports were the underscore has been ommitted,
+like rstpon_i, which make it difficult to figure out if the n if part
+of the name ("power oN reset") or if it is there to mark the signal as
+active low (which in this particular case it is).
+---> signals of nanoFIP's pinout couldn't be changed:-s
+     corrections applied to the rest!   
+
+If a file is not listed below it does not mean all is good, just that
+I did not have time to look at it ;)
+
+Question for Eva (did not have time to look): we know all inputs are
+well registered. Are all outputs registered as well?
+---> Yes!
+
+nanofip.vhd
+-----------
+
+Line 313 and 314. Synplify attributes make code less portable. Could
+they be pushed out of the code into some Synplify config file? I don't
+have strong feelings about this though.
+---> OK!
+
+wf_engine_control.vhd
+---------------------
+
+Line 198. It is preferable to have all ports be std_logic or
+std_logic_vector, see comment on line 178 of the reset unit.
+---> Pablo: "Using std_logic on entity ports is a strong rule in cores.
+     As wf_engine_control.vhd is interfaced with other custom entities
+     this rule should become only a recomendation. Passing var as a custom
+     type gives the possibility to the designer to chose the suitible encoding
+     scheme easily wich is very handy for this critical signal."
+
+Line 290. This state machine can be stuck in a given state for a
+number of reasons, most notably the physical interruption of the wfip
+traffic. Granted, things should go back to normal (i.e. Idle) once the
+link is re-established, thanks to the appearance of rx_byte_ready_p_i
+pulses with incoherent byte contents, but that's a bit weak in terms
+of protection. I would favor timeouts relying only on the system clock
+as an additional way to go back to Idle. In fact this is probably a
+comment that applies to other state machines in the design. The
+"finger-crossing" nature of the current protection is even more
+apparent in the id_dat_frame_ok state, where after a link cut you have
+to wait until a valid received frame while the counter (free running)
+is more than 2 in value. Again, not a high risk for things to go
+wrong, but we can get quite a lot of peace of mind with independent
+timeouts. After a bit of thinking, something worse could happen: the
+state machine could be stopped in the id_dat_frame_ok state and
+re-awakened after a while (when the link is back) and then go to
+consumption of some dummy data that was not even destined to this
+station. IMHO independent timeouts should be mandatory in all states
+of all state machines bound to have this type of problems, unless the
+cost in logic is prohibitive.
+---> OK! Timeouts relying only on system clock applied to all state machines.
+
+
+wf_incr_counter.vhd
+-------------------
+
+Line 72. The ports nfip_rst_i and reinit_counter_i do exactly the same
+thing. One should go. Also, the counter does not know it's inside a
+nanofip, I think a reset should be called rst_i and not nfip_rst_i. 
+---> OK! nfip_rst_i removed.
+
+Line 81. Minor point. The output port counter_o should be
+std_logic_vector.  See similar comment for line 178 of
+wf_reset_unit.vhd.
+---> I think i won't follow this one:-s; the output of the counter is usually
+     used for further calculations+making it a std_logic_vector would mean
+     applying several conversions.
+
+
+wf_inputs_synchronizer.vhd
+--------------------------
+
+Line 240. These two signals are very critical: the fd_rxd_edge_p_o
+output is made from them. And then this edge detection signal is fed
+to the wf_rx_tx_osc block, where it is used to manufacture very
+critical "clock" signals whose malfunctioning would compromise the
+deglitcher block. It is therefore important to try to have these edge
+detectors as robust as possible, and add deglitching here. With the
+current implementation if there is a glitch in the rx line, even
+narrower than one uclk period, there is a risk of generating a
+spurious edge pulse. Making something much more robust is easy and
+cheap: add four more stages to the pipeline (for a total of six usable
+signals) and do a super-robust edge detector by requiring that at
+least two signals are zero (out of the first three) and at least two
+are one (out of the last three). Viceversa of course for detecting the
+other edge. For extra-robustness, inhibit detection for a while after
+a detected edge. This will induce a delay, which might have to be
+compensated in other signals (fd_rxd_o) so the whole thing is
+coherent.
+---> OK! New deglitcher at the entrance of FD_RXD.
+
+Line 250 onwards. Cosmetics: the vector notation is shorter, so I'd
+stick to it for all cases, including the varX_access.
+---> OK!
+
+Line 326. I don't think it's a good idea to synchronize the data from
+the user in slone mode. If there is an incoherent state (i.e. some
+bits have flipped and others haven't) this state will propagate
+through the pipeline. Only control signals need to be synchronized (as
+is done for the wishbone). The user *must* ensure the data are stable
+by the time VAR3_RDY goes to zero, so the 3*16 FFs spent here can only
+get us trouble. Incidentally, I think we should change the spec so it
+does not say we sample the data on the first clock tick after VAR3_RDY
+goes down. We can also sample a couple of ticks later, the user should
+not care.
+---> OK! No synchronization buffers for DAT_I.
+
+Oops, now I see these lines are also used in wishbone mode
+for data_in. I am pretty sure the argument still applies. In fact we
+are not synchronizing the WB address, why synchronize the data_in?
+---> JFTR these lines were only used for the slone operation; in memory mode
+     only the control signals were synched (neither the address nor the data)
+
+Line 347. One could argue about the need for these. We know these
+inputs are static. While I can't really think how these FFs could get
+us in trouble, they are certainly not needed, so why risk it?
+---> OK! Removed synch of constant signals.
+
+wf_reset_unit.vhd
+-----------------
+
+Line 178. Minor point. It is better to use only std_logic and
+std_logic_vector for ports, for the reasons explained in paragraph
+4.15 in the guidelines. This is most important for the top entity, so
+not very important here. Internal conversion to t_var should be
+simple.
+
+
+Line 419. Minor detail. "reception or" -> "reception of"
+Line 421. Same thing.
+---> OK!
+
+wf_rx_deglitcher.vhd
+--------------------
+
+Line 90. Minor point. It should be "g_DEGLITCH_LGTH" instead of
+"c_DEGLITCH_LGTH".
+---> OK!
+
+Line 103. Minor: it should be "WF_rx_tx_osc" instead of "WF_tx_rx_osc".
+---> OK!
+
+
+wf_rx_deserializer.vhd (only looked at part of it)
+----------------------
+
+Line 240. I wonder if we should have an additional condition which
+takes this state machine to Idle, namely the wfip link being inactive,
+i.e. maintaining a constant level for a long (to be defined)
+time. This would ensure the state machine is always sitting in the
+Idle state before the beginning of a frame. With the current
+implementation, it might well be the case, but there are many
+scenarios to analyze and we risk forgetting something.
+--->................................................................
+
+wf_rx_tx_osc.vhd
+----------------
+
+Line 106. Minor point. Generics should start with "g_".
+---> OK!
--- a/CodeReview_24Jan11/Matthieu_EvasComments.txt
+++ b/CodeReview_24Jan11/Matthieu_EvasComments.txt
--- a/CodeReview_24Jan11/Pablo_EvasComments.txt
+++ b/CodeReview_24Jan11/Pablo_EvasComments.txt
+WF_inputs_synchronizer.vhd
+
+Considering radiation hardness registering constant inputs on three flip flops stages is probably a weekness. Proabably only one TMR flip-flop per input would be enough.
+---> Corrected! Removed the 3ple registration for the constant inputs; only TMR.
+
+
+The wishbone bus is actually properly implemented as it removes any flip-flops from the data and address bus. Notice that the traditional synchronous implementation would require to register the data and address on the IOs, preventing the use of TMR flip-flops.
+
+
+I made a synthesis with XST on an Spartan6. No serious problems. It only found the following warnings: 
+WARNING:HDLCompiler:92 - "E:\ohr\cern-fip\trunk\hdl\design\wf_crc.vhd" Line 184: s_q_check_mask should be on the sensitivity list of the process
+WARNING:HDLCompiler:1127 - "E:\ohr\cern-fip\trunk\hdl\design\wf_DualClkRAM_clka_rd_clkb_wr.vhd" Line 122: Assignment to zero ignored, since the identifier is never used
+WARNING:HDLCompiler:1127 - "E:\ohr\cern-fip\trunk\hdl\design\wf_bits_to_txd.vhd" Line 165: Assignment to s_fss ignored, since the identifier is never used
+WARNING:HDLCompiler:1127 - "E:\ohr\cern-fip\trunk\hdl\design\wf_engine_control.vhd" Line 424: Assignment to s_id_dat_ctrl_byte ignored, since the identifier is never used
+---> Bugs (which were not reported in Synplify!) corrected!
+
+
+
+wf.crc.vhd
+
+c_GENERATOR_POLY_length and c_VERIFICATION_MASK should both come from a generic]
+---> I think i wont follow this:-s It would be clear if the: poly_lgth, gener_poly and verif_poly were generics, but this is not possible as gener_poly and verif_poly depend on poly_lgth.
+Leaving the 3 signals as constants in the package i think is clearer.
+
+
+To tom.  crc_ok_p is not registered to reduce the number of flip flops, remember this is a rad hard design. It is marcked as p because it is generated using  s_crc_bit_ready_p which is already a pulse. 
+
+
+wf_cons_bytes_processor.vhd
+line 422. Isn't it possible to check c_CTRL_BYTE_INDEX, c_PDU_BYTE_INDEX upon reception? That would save 16 flip-flops. 
+---> I think i won't follow that:-s
+
+wf_rx_tx_oscillator.vhd
+
+Interesting note from IEC 61158-2
+
+9.2.6 Preamble
+In order to synchronize bit times a preamble shall be transmitted at the beginning of each
+PhPDU consisting of the following sequence of bits, shown from left to right in order of
+transmission:
+1, 0, 1, 0, 1, 0, 1, 0.
+(shown as a waveform in Figure 32)
+NOTE 1 Received preamble can contain as few as four bits due to loss of one bit through each of four repeaters
+(as specified in the MAU Network Configuration Rules).
+The period may be extended, but not reduced, by Systems management as given in Table 4.
+A preamble extension sequence as listed in Table 4 shall be defined as the following
+sequence of bits, shown from left to right in order of transmission:
+1, 0, 1, 0, 1, 0, 1, 0.
+
+
+
+wf_engine_control.vhd
+
+To Javier. Using std_logic on entity ports is a strong rule in cores. As wf_engine_control.vhd is interfaced with other custom entities this rule should become only a recomendation. Passing var as a custom type gives the possibility to the designer to chose the suitible encoding scheme easily wich is very handy for this critical signal. 
+
+To Gonzalo. Receiving to consecutive ID_DAT would be a legal situation? I only see it happening in case an RP_DAT is lost. In that case the watchdog should have reset the control state machine.
+
+Line 775: This process is equivalent to a case statement. The exit is needed to select the case that is driving s_var_id.
+
+
+WF_status_bytes_gen.vhd
+
+L293. 
+          if (s_var3_rdy_extended = '0' and var3_acc_i = '1') then  
+                                                                 -- since the last time the status 
+            s_nFIP_status_byte(c_U_PACER_INDEX) <= '1';          -- byte was delivered,
+                                                                 -- the user logic accessed a prod.
+                                                                 -- var. when it was not ready
+          end if;  
+
+Would it not be easier to use some kind of 
+---> didnt get that?
+
+wf_rx.vhd
+
+LINE 569: The  16 bit register s_arriving_fes could be removed if the FES arrival is verified bit by bit upon arrival. 
+---> I think i won't follow that:-s
+
+LINE 663: Is s_CRC_ok_p_buff really needed? I think a flag (or inserting a new state) would be enough. 
+---> OK! Buffer removed.
--- a/CodeReview_24Jan11/Tom_EvasComments.txt
+++ b/CodeReview_24Jan11/Tom_EvasComments.txt