[entropy_src/rtl] Move the 1-4 esbit packer #21626
Conversation
There was a problem hiding this comment.
Thanks for working on this @h-filali , this looks mostly good!
We'll also need to take care of the documentation including the block diagram. Since the RTL code follows the block diagram, the code section related to the 1-to-4 esbit packer FIFO should maybe be moved down a bit. As it will however complicate the review a lot, I would say let's first iron out the other stuff.
| assign pfifo_esbit_push = rng_bit_en && sfifo_esrng_not_empty; | ||
| assign pfifo_esbit_push = rng_bit_en && sfifo_esrng_not_empty && pfifo_esbit_not_full; |
There was a problem hiding this comment.
What is the motivation of this change? Do we really need this?
My problem with this is the following: right now the code just writes the FIFO even if it's full. Of course this is bad but it's captured with an err_code that software can inspect. I.e., if this happens, software will see this and can restart the entropy complex.
If we don't push at this point, the alert will not fire. In this particular case, the esrng FIFO will become full and eventually disable the noise source. The noise source is restarted and everything continues without software getting notified and without repeating the startup test which is very bad from a validation viewpoint. Of course we need to change this, but at this point I would not do such changes unless it's really required.
There was a problem hiding this comment.
You're absolutely right. I did this because it just made sense to add this, as it would in the usual case, because why would we push into a full FIFO.
However, I forgot to think about the errors we have for this. So you are absolutely right, the added signal should not be there and should not be changed.
There was a problem hiding this comment.
Thanks for reverting this!
| @@ -1097,8 +1097,9 @@ module entropy_src_core import entropy_src_pkg::*; #( | |||
|
|
|||
| // select source for health testing | |||
There was a problem hiding this comment.
This comment is now outdated. Please adapt it accordingly.
h-filali
force-pushed
the
es-move-1-4-packer
branch
4 times, most recently
from
6e22742
to
12c6ca4
Compare
|
Thanks for reviewing @vogelpi, there were some really important insights in there. I have addressed all your comments. Let's see what the CI has to say and move on from there! |
h-filali
force-pushed
the
es-move-1-4-packer
branch
from
12c6ca4
to
1473a6c
Compare
| assign pfifo_esbit_push = rng_bit_en && sfifo_esrng_not_empty; | ||
| assign pfifo_esbit_push = rng_bit_en && sfifo_esrng_not_empty && pfifo_esbit_not_full; |
There was a problem hiding this comment.
Thanks for reverting this!
This commit moves the 1-4 esbit packer back in front of the postht FIFO. To complement this change this commit also adds functionality to the specific health test blocks such that single lanes can be tested sequentially and selectively. Signed-off-by: Hakim Filali <[email protected]>
This commit aligns the scoreboard with the esbit FIFO change. Signed-off-by: Hakim Filali <[email protected]>
This commit aligns the documentation with the RTL changes. It also explains how to set up the single bit lane mode in the programmer's guide. Signed-off-by: Hakim Filali <[email protected]>
This commit aligns the entropy_src block diagram with the added RTL changes. Signed-off-by: Hakim Filali <[email protected]>
This commit moves the esbit FIFO code down in the sv file according to the esbit FIFO's placement in the block diagram. This is in a separate commit to make reviewing easier. Signed-off-by: Hakim Filali <[email protected]>
h-filali
force-pushed
the
es-move-1-4-packer
branch
from
1473a6c
to
3366452
Compare
This PR moves the esbit packer FIFO in front of the postht FIFO.
For a more detailed explanation have a look at the commit messages.
Closes #20954