Skip to content

M
MasterFIP - Gateware
Commit 6a3520b7 authored by Dimitris Lampridis's avatar Dimitris Lampridis
Browse files
Options

doc: added my input for the gw review in text (.org) and html format

parent 543aae9b
Hide whitespace changes
Inline Side-by-side
Showing with 382 additions and 0 deletions
doc/review_02032017/dlamprid_comments.html 0 → 100644
View file @ 6a3520b7
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en">
<head>
<title>Comments for masterFIP GW review <span class="timestamp-wrapper"><span class="timestamp">&lt;2017-03-16 Thu&gt;</span></span></title>
<!-- 2017-03-16 Thu 12:14 -->
<meta http-equiv="Content-Type" content="text/html;charset=utf-8" />
<meta name="generator" content="Org-mode" />
<meta name="author" content="Dimitrios Lampridis" />
<style type="text/css">
<!--/*--><![CDATA[/*><!--*/
.title { text-align: center; }
.todo { font-family: monospace; color: red; }
.done { color: green; }
.tag { background-color: #eee; font-family: monospace;
padding: 2px; font-size: 80%; font-weight: normal; }
.timestamp { color: #bebebe; }
.timestamp-kwd { color: #5f9ea0; }
.right { margin-left: auto; margin-right: 0px; text-align: right; }
.left { margin-left: 0px; margin-right: auto; text-align: left; }
.center { margin-left: auto; margin-right: auto; text-align: center; }
.underline { text-decoration: underline; }
#postamble p, #preamble p { font-size: 90%; margin: .2em; }
p.verse { margin-left: 3%; }
pre {
border: 1px solid #ccc;
box-shadow: 3px 3px 3px #eee;
padding: 8pt;
font-family: monospace;
overflow: auto;
margin: 1.2em;
}
pre.src {
position: relative;
overflow: visible;
padding-top: 1.2em;
}
pre.src:before {
display: none;
position: absolute;
background-color: white;
top: -10px;
right: 10px;
padding: 3px;
border: 1px solid black;
}
pre.src:hover:before { display: inline;}
pre.src-sh:before { content: 'sh'; }
pre.src-bash:before { content: 'sh'; }
pre.src-emacs-lisp:before { content: 'Emacs Lisp'; }
pre.src-R:before { content: 'R'; }
pre.src-perl:before { content: 'Perl'; }
pre.src-java:before { content: 'Java'; }
pre.src-sql:before { content: 'SQL'; }
table { border-collapse:collapse; }
caption.t-above { caption-side: top; }
caption.t-bottom { caption-side: bottom; }
td, th { vertical-align:top; }
th.right { text-align: center; }
th.left { text-align: center; }
th.center { text-align: center; }
td.right { text-align: right; }
td.left { text-align: left; }
td.center { text-align: center; }
dt { font-weight: bold; }
.footpara:nth-child(2) { display: inline; }
.footpara { display: block; }
.footdef { margin-bottom: 1em; }
.figure { padding: 1em; }
.figure p { text-align: center; }
.inlinetask {
padding: 10px;
border: 2px solid gray;
margin: 10px;
background: #ffffcc;
}
#org-div-home-and-up
{ text-align: right; font-size: 70%; white-space: nowrap; }
textarea { overflow-x: auto; }
.linenr { font-size: smaller }
.code-highlighted { background-color: #ffff00; }
.org-info-js_info-navigation { border-style: none; }
#org-info-js_console-label
{ font-size: 10px; font-weight: bold; white-space: nowrap; }
.org-info-js_search-highlight
{ background-color: #ffff00; color: #000000; font-weight: bold; }
/*]]>*/-->
</style>
<script type="text/javascript">
/*
@licstart The following is the entire license notice for the
JavaScript code in this tag.
Copyright (C) 2012-2013 Free Software Foundation, Inc.
The JavaScript code in this tag is free software: you can
redistribute it and/or modify it under the terms of the GNU
General Public License (GNU GPL) as published by the Free Software
Foundation, either version 3 of the License, or (at your option)
any later version. The code is distributed WITHOUT ANY WARRANTY;
without even the implied warranty of MERCHANTABILITY or FITNESS
FOR A PARTICULAR PURPOSE. See the GNU GPL for more details.
As additional permission under GNU GPL version 3 section 7, you
may distribute non-source (e.g., minimized or compacted) forms of
that code without the copy of the GNU GPL normally required by
section 4, provided you include this license notice and a URL
through which recipients can access the Corresponding Source.
@licend The above is the entire license notice
for the JavaScript code in this tag.
*/
<!--/*--><![CDATA[/*><!--*/
function CodeHighlightOn(elem, id)
{
var target = document.getElementById(id);
if(null != target) {
elem.cacheClassElem = elem.className;
elem.cacheClassTarget = target.className;
target.className = "code-highlighted";
elem.className = "code-highlighted";
}
}
function CodeHighlightOff(elem, id)
{
var target = document.getElementById(id);
if(elem.cacheClassElem)
elem.className = elem.cacheClassElem;
if(elem.cacheClassTarget)
target.className = elem.cacheClassTarget;
}
/*]]>*///-->
</script>
</head>
<body>
<div id="content">
<h1 class="title">Comments for masterFIP GW review <span class="timestamp-wrapper"><span class="timestamp">&lt;2017-03-16 Thu&gt;</span></span></h1>
<div id="outline-container-sec-1" class="outline-2">
<h2 id="sec-1"><span class="section-number-2">1</span> Project/Synthesis/PAR</h2>
<div class="outline-text-2" id="text-1">
<ul class="org-ul">
<li>[+] 3943 warnings during synthesis
</li>
<li>[+] ucf: "TS_U_Node_Template_U_GN4124_Core_cmp_clk_in_feedback" points to non-existing
node 'U_Node_Template/U_GN4124_Core/cmp_clk_in/feedback'.
</li>
<li><code>[-]</code> 120+ warnings about missing/duplicates files when we first open the .xise project file, and
the messages keep reappearing while we use the tool
</li>
<li><code>[-]</code> Project hierarchy shows too many unused files (not under spec_masterfip_mt entity)
</li>
<li><code>[-]</code> running hdlmake in syn/spec does not work
</li>
<li><code>[-]</code> many (~50) synthesis-generated files not git-ignored
</li>
<li><code>[-]</code> if not a build script for wbgen, at least a README with how you expect the wbgen2 command to
be invoked should be provided
</li>
</ul>
</div>
</div>
<div id="outline-container-sec-2" class="outline-2">
<h2 id="sec-2"><span class="section-number-2">2</span> Simulation</h2>
<div class="outline-text-2" id="text-2">
<ul class="org-ul">
<li>[!] Error: (vcom-19) Failed to access library 'nanofip_lib' at "nanofip_lib" (Modelsim SE-64
10.2a, Linux) This is due to case-sensitivity in Linux. Solution: replace "vlib nanoFIP_lib"
with "vlib nanofip_lib" or, alternatively, replace "vcom -work nanofip_lib" with "vcom -work
nanoFIP_lib"
</li>
<li>[!] Error: (vcom-7) Failed to open design unit file "../../sim/spec/testbench/nanofip_lib/*" in
read mode. Again, this is due to case-sensitivity in Linux, since the folder commited in git is
actually sim/spec/testbench/nanoFIP_lib
</li>
<li>[+] Trying to step into the code with Modelsim produces: Error opening
/opt/Xilinx/14.7/ISE_DS/ISE/vhdl/src/unisims/primitive/PLL_ADV.vhd. This path does not exist in
my system.
</li>
<li>[+] Fix warnings "numeric_std.to_integer metavalue detected, returning 0" (alternatively, use
"set NumericStdNoWarnings 1" after the call to vsim in your do files, suboptimal solution because
it might also hide useful metavalue warnings)
</li>
<li><code>[-]</code> After clearing the metavalue warnings, have a look also at the remaining warnings
</li>
<li><code>[-]</code> Some signals are always 'X', if they are not useful in simulation, just remove them
</li>
<li><code>[-]</code> group signal waveforms
</li>
<li><code>[-]</code> split compilation (vcom) from running in separate "do" files, invoke them both from a top do
file (eg. compile.do + run.do =&gt; sim.do)
</li>
<li><code>[-]</code> many (~100) simulation-generated files not git-ignored
</li>
<li>[?] Do you need the "vlog $env(XILINX)/verilog/src/glbl.v"? If not, remove it, otherwise users
might get the error "can't read "env(XILINX)": no such variable" if they haven't set this
environment variable.
</li>
</ul>
</div>
</div>
<div id="outline-container-sec-3" class="outline-2">
<h2 id="sec-3"><span class="section-number-2">3</span> Design</h2>
<div class="outline-text-2" id="text-3">
<ul class="org-ul">
<li>[?] rtl: is leds_manager.vhd used at all? what about carrier_info.vhd, free_counter.vhd, and
perhaps others? If not, it's better to delete them from the repository.
</li>
<li>[?] rtl: why is wf_package.vhd declared here AND in ip-cores/nanofip?
</li>
<li>[?] top: is synthesis_descriptor.vhd used?
</li>
<li>[*] the whole nanofip as ip-core of masterfip seems a bit counter-intuitive. If there are things
used by both, the should belong to a "fip" project, and both nano- and master- should use them.
</li>
</ul>
</div>
<div id="outline-container-sec-3-1" class="outline-3">
<h3 id="sec-3-1"><span class="section-number-3">3.1</span> masterFIP_pkg</h3>
<div class="outline-text-3" id="text-3-1">
<ul class="org-ul">
<li><code>[-]</code> constants should have lower case "c" (I think)
</li>
</ul>
</div>
</div>
<div id="outline-container-sec-3-2" class="outline-3">
<h3 id="sec-3-2"><span class="section-number-3">3.2</span> spec_masterfip_mt</h3>
<div class="outline-text-3" id="text-3-2">
</div><div id="outline-container-sec-3-2-1" class="outline-4">
<h4 id="sec-3-2-1"><span class="section-number-4">3.2.1</span> fmc_masterFIP_core</h4>
<div class="outline-text-4" id="text-3-2-1">
<ul class="org-ul">
<li><code>[-]</code> it would be nice to have a bit more hierachy in this module, with less low-level processes
and modules lying around (eg. counter modules, assignments to wb registers) , to highlight the
high-level structure of the core.
</li>
<li>[?] why do you use two decreasing counter modules (wf_decr_counter and decr_counter)?
</li>
<li>[?] why is speed_X_i not a 2-bit vector?
</li>
<li>[?] would it be interesting for diagnostics to monitor if/when counters overflow?
</li>
<li>[?] why is reg_to_mt.fd_wdg_tstamp_i one bit larger than macrocyc_cnt?
</li>
<li>[*] cmp_ext_sync_deglitch_p_detect and cmp_fd_wdgn_deglitch_p_detect introduce one unnecessary
FF/latency cycle
</li>
<li>[*] we should introduce generic up/down counters to general-cores
</li>
</ul>
</div>
<ol class="org-ol"><li><a id="sec-3-2-1-1" name="sec-3-2-1-1"></a>masterfip_tx<br /><div class="outline-text-5" id="text-3-2-1-1">
<ul class="org-ul">
<li><code>[-]</code> synch_signals process could be replaced by 2x gc_sync_ffs
</li>
<li>[?] more importantly, why do you resync these two signals?
</li>
</ul>
</div>
</li>
<li><a id="sec-3-2-1-2" name="sec-3-2-1-2"></a>masterfip_rx<br /><div class="outline-text-5" id="text-3-2-1-2">
<ul class="org-ul">
<li>[?] can't you use general-cores for cmp_rx_deglitcher? (perhaps think about it also in nanofip
project)
</li>
<li>[?] did you make up your mind about what to connect to nfip_rst_i port of cmp_rx_deglitcher? if
yes, remove the inline comment
</li>
</ul>
</div>
</li></ol>
</div>
</div>
</div>
<div id="outline-container-sec-4" class="outline-2">
<h2 id="sec-4"><span class="section-number-2">4</span> Legend</h2>
<div class="outline-text-2" id="text-4">
<ul class="org-ul">
<li>[!] = fatal
</li>
<li>[+] = important
</li>
<li><code>[-]</code> = minor
</li>
<li>[?] = question
</li>
<li>[*] = note
</li>
</ul>
</div>
</div>
</div>
<div id="postamble" class="status">
<p class="author">Author: Dimitrios Lampridis</p>
<p class="date">Created: 2017-03-16 Thu 12:14</p>
<p class="creator"><a href="http://www.gnu.org/software/emacs/">Emacs</a> 24.5.1 (<a href="http://orgmode.org">Org</a> mode 8.2.10)</p>
<p class="validation"><a href="http://validator.w3.org/check?uri=referer">Validate</a></p>
</div>
</body>
</html>
doc/review_02032017/dlamprid_comments.org 0 → 100644
View file @ 6a3520b7
# emacs org-mode options and definitions, just ignore
#+OPTIONS: toc:nil
#+OPTIONS: ^:nil
#+TITLE: Comments for masterFIP GW review <2017-03-16 Thu>
* Project/Synthesis/PAR
- [+] 3943 warnings during synthesis
- [+] ucf: "TS_U_Node_Template_U_GN4124_Core_cmp_clk_in_feedback" points to non-existing
node 'U_Node_Template/U_GN4124_Core/cmp_clk_in/feedback'.
- [-] 120+ warnings about missing/duplicates files when we first open the .xise project file, and
the messages keep reappearing while we use the tool
- [-] Project hierarchy shows too many unused files (not under spec_masterfip_mt entity)
- [-] running hdlmake in syn/spec does not work
- [-] many (~50) synthesis-generated files not git-ignored
- [-] if not a build script for wbgen, at least a README with how you expect the wbgen2 command to
be invoked should be provided
* Simulation
- [!] Error: (vcom-19) Failed to access library 'nanofip_lib' at "nanofip_lib" (Modelsim SE-64
10.2a, Linux) This is due to case-sensitivity in Linux. Solution: replace "vlib nanoFIP_lib"
with "vlib nanofip_lib" or, alternatively, replace "vcom -work nanofip_lib" with "vcom -work
nanoFIP_lib"
- [!] Error: (vcom-7) Failed to open design unit file "../../sim/spec/testbench/nanofip_lib/*" in
read mode. Again, this is due to case-sensitivity in Linux, since the folder commited in git is
actually sim/spec/testbench/nanoFIP_lib
- [+] Trying to step into the code with Modelsim produces: Error opening
/opt/Xilinx/14.7/ISE_DS/ISE/vhdl/src/unisims/primitive/PLL_ADV.vhd. This path does not exist in
my system.
- [+] Fix warnings "numeric_std.to_integer metavalue detected, returning 0" (alternatively, use
"set NumericStdNoWarnings 1" after the call to vsim in your do files, suboptimal solution because
it might also hide useful metavalue warnings)
- [-] After clearing the metavalue warnings, have a look also at the remaining warnings
- [-] Some signals are always 'X', if they are not useful in simulation, just remove them
- [-] group signal waveforms
- [-] split compilation (vcom) from running in separate "do" files, invoke them both from a top do
file (eg. compile.do + run.do => sim.do)
- [-] many (~100) simulation-generated files not git-ignored
- [?] Do you need the "vlog $env(XILINX)/verilog/src/glbl.v"? If not, remove it, otherwise users
might get the error "can't read "env(XILINX)": no such variable" if they haven't set this
environment variable.
* Design
- [?] rtl: is leds_manager.vhd used at all? what about carrier_info.vhd, free_counter.vhd, and
perhaps others? If not, it's better to delete them from the repository.
- [?] rtl: why is wf_package.vhd declared here AND in ip-cores/nanofip?
- [?] top: is synthesis_descriptor.vhd used?
- [*] the whole nanofip as ip-core of masterfip seems a bit counter-intuitive. If there are things
used by both, the should belong to a "fip" project, and both nano- and master- should use them.
** masterFIP_pkg
- [-] constants should have lower case "c" (I think)
** spec_masterfip_mt
*** fmc_masterFIP_core
- [-] it would be nice to have a bit more hierachy in this module, with less low-level processes
and modules lying around (eg. counter modules, assignments to wb registers) , to highlight the
high-level structure of the core.
- [?] why do you use two decreasing counter modules (wf_decr_counter and decr_counter)?
- [?] why is speed_X_i not a 2-bit vector?
- [?] would it be interesting for diagnostics to monitor if/when counters overflow?
- [?] why is reg_to_mt.fd_wdg_tstamp_i one bit larger than macrocyc_cnt?
- [*] cmp_ext_sync_deglitch_p_detect and cmp_fd_wdgn_deglitch_p_detect introduce one unnecessary
FF/latency cycle
- [*] we should introduce generic up/down counters to general-cores
**** masterfip_tx
- [-] synch_signals process could be replaced by 2x gc_sync_ffs
- [?] more importantly, why do you resync these two signals?
**** masterfip_rx
- [?] can't you use general-cores for cmp_rx_deglitcher? (perhaps think about it also in nanofip
project)
- [?] did you make up your mind about what to connect to nfip_rst_i port of cmp_rx_deglitcher? if
yes, remove the inline comment
* Legend
- [!] = fatal
- [+] = important
- [-] = minor
- [?] = question
- [*] = note
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment