javier.txt 2.62 KB
Newer Older
1 2 3
Disclaimer: not enough time to do this design justice. Focusing on VHDL
constructs, not trying to make sense of overall operation.

serrano's avatar
serrano committed
4 5 6 7 8 9 10 11 12 13 14
General Comment
===============
There are lots of abnormal conditions detected and treated now, but
they leave no trace for diagnostics. Maybe it would make sense to add
status bits for them such that people could have a sense of how often
these abnormal conditions are hit during real operation.
Also I wanted to do a general check to make sure all inputs are
correctly registered and protected against metastability and all
outputs are registered unless there is a good reason not
to. Unfortunately I did not have time to do it, could you?

15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53
wf_engine_control.vhd 
===================== 
- General comment: this file is huge and difficult to assimilate in
one go. I guess it's too late now, but it would have benefitted from a
bit of partitioning.
- Line 254. "independantly" -> "independently".
- Line 257. "rubust" -> "robust".  
- Line 257. "dependant" -> "dependent".
- Line 395. Going to rst_rx seems safer than going back to idle
directly. It's just one more tick of latency and we're then sure the
rx block is always in good shape to receive a new frame.


wf_jtag_controller.vhd
======================
- Line 115. State names could be in capitals.
- Line 239. This whole process looks redundant. I guess the goal was
to have signals that conveniently inform of the state the FSM is in,
but there is already one such signal: the state signal itself.
- Line 298. Ah! This is what these signals were for. Fine, but space
could be saved with statements like "s_play_byte <= '1' when
jc_st=play_byte else '0'".
- Line 399. "retreival" -> "retrieval".
- Line 484. The TDO signal is sampled when s_tck_r_edge_p='1', exactly
at the same time TCK goes high. If I am not mistaken, the JTAG
standard says chips should drive TDO on TCK falling edge, so for slow
chips one could argue that sampling a bit later could be safer. OTOH,
I guess the JTAG spec says we should sample TDO on the rising edge of
TCK, done now...
- Line 498. I don't quite understand why this counter should be 21
bits wide. That gives a very long time (52 ms when all we need is some
100 us), and gives more FFs than necessary. For rad-tol reasons it
would make sense to use only as many FFs as needed.

wf_rx_deserializer.vhd
======================
- OK, now I see there is a timeout here as well, so maybe my comment
about resetting the rx block systematically from wf_engine_control is
not so important.
serrano's avatar
serrano committed
54 55 56 57 58 59 60 61

wf_tx_serializer
================
- Line 557. There is no 'else' in this 'if' statement. Is this OK?

wf_reset_unit.vhd
=================
- Line 359. "unitl" -> "until"