Skip to content

V
VME64x core
Commit 6ce9cc00 authored by Tristan Gingold's avatar Tristan Gingold
Browse files
Options

Update review status (and minor fixes).

parent b5a88704
Hide whitespace changes
Inline Side-by-side
Showing with 24 additions and 17 deletions
doc/review-2017-11-13/ML-vme64x-review.txt
View file @ 6ce9cc00
...@@ -25,18 +25,18 @@ about simulatin will come first): ...@@ -25,18 +25,18 @@ about simulatin will come first):
folder is out-of-date. folder is out-of-date.
I see several improvements that are spread among several points below. I see several improvements that are spread among several points below.
Done: let's follow this convention, with a README. Done: let's follow this convention, with a README.
0.2. I would recommend to follow our folder convention (which used to be on 0.2. I would recommend to follow our folder convention (which used to be on
ohwr.org and which is now gone, probably due to migration, so I copy+paste ohwr.org and which is now gone, probably due to migration, so I copy+paste
it at the end of this file). Using this convention would require the it at the end of this file). Using this convention would require the
following changes: following changes:
a) change /documents to /doc a) change /documents to /doc
Done. Done.
b) change hdl/sim to hdl/testbench b) change hdl/sim to hdl/testbench
Done. Done.
c) keep vme64x_bfm in hdl/sim/vme64x_bfm if this is a c) keep vme64x_bfm in hdl/sim/vme64x_bfm if this is a
model of the vme64x (I do not know because it is undocumented) model of the vme64x (I do not know because it is undocumented)
Done. Done.
0.3. the arrangement of simple_tb is confusing: first I thought that the folder 0.3. the arrangement of simple_tb is confusing: first I thought that the folder
"modelsim" contains some modelsim-specific stuff. Again, how should I know "modelsim" contains some modelsim-specific stuff. Again, how should I know
if there is no description/documentation/README. I'd recommend to do one if there is no description/documentation/README. I'd recommend to do one
...@@ -44,9 +44,9 @@ about simulatin will come first): ...@@ -44,9 +44,9 @@ about simulatin will come first):
a) move content from hdl/sim/simple_tb/modelsim/ to hdl/sim/simple_tb a) move content from hdl/sim/simple_tb/modelsim/ to hdl/sim/simple_tb
(preferred if there are no plans to add simulation for other tools) (preferred if there are no plans to add simulation for other tools)
b) add README explaining this structure b) add README explaining this structure
Done: Add a README. Done: Add a README.
0.4. hdl/sim/simple_tb/modelsim/run_all.sh is not working [committed fix] 0.4. hdl/sim/simple_tb/modelsim/run_all.sh is not working [committed fix]
OK. Done (merged).
0.5. It would be extremely useful to have some waveforms when running simulation. 0.5. It would be extremely useful to have some waveforms when running simulation.
I recommend doing one of the following: I recommend doing one of the following:
a) modify run_all.sh to show the run simulations in waveform a) modify run_all.sh to show the run simulations in waveform
...@@ -55,7 +55,7 @@ about simulatin will come first): ...@@ -55,7 +55,7 @@ about simulatin will come first):
signals. Ideally, the documentation provided to the user would match the signals. Ideally, the documentation provided to the user would match the
waveform so that when a developer runs the simulation, he can easily map waveform so that when a developer runs the simulation, he can easily map
the output to the documentation. the output to the documentation.
Done: your run.do file was cherry-picked. Done: your run.do file was cherry-picked.
0.6. Add general-cores as submodule in /hdl/ip_cores [committed] 0.6. Add general-cores as submodule in /hdl/ip_cores [committed]
even if vme64x is never used standalone and general-cores are used in this even if vme64x is never used standalone and general-cores are used in this
project only when running simulation, I would still include it as submodule project only when running simulation, I would still include it as submodule
...@@ -64,14 +64,14 @@ about simulatin will come first): ...@@ -64,14 +64,14 @@ about simulatin will come first):
know which ip_cores and which version he/she needs know which ip_cores and which version he/she needs
b) to make the simulation easier to to run b) to make the simulation easier to to run
c) it seems to be a general policy c) it seems to be a general policy
Done. Done.
0.7. README in the main directory is completely out of date ! 0.7. README in the main directory is completely out of date !
Done. Done.
0.8. /documention folder: 0.8. /documention folder:
- the notes from the 2012 review should stay, - the notes from the 2012 review should stay,
Still in the repo. Still in the repo.
- a new folder for the current review should be added. - a new folder for the current review should be added.
Done. Done.
- I would guess that some of the removed documents will be replaced with - I would guess that some of the removed documents will be replaced with
updated versions. If this is not the case, it seems to me that the updated versions. If this is not the case, it seems to me that the
currently provided documentation (the one reminding in the folder) is currently provided documentation (the one reminding in the folder) is
...@@ -87,18 +87,22 @@ about simulatin will come first): ...@@ -87,18 +87,22 @@ about simulatin will come first):
- I do not understand why you need two separate packages for the - I do not understand why you need two separate packages for the
VME64xCore_Top and its wrapper xvme64x_core. I would recommend to VME64xCore_Top and its wrapper xvme64x_core. I would recommend to
merge vme64x_pack.vhd and xvme64x_core_pkg.vhd into one file/pacakge merge vme64x_pack.vhd and xvme64x_core_pkg.vhd into one file/pacakge
Done.
- a "private" package seems like a good idea and something that is used in - a "private" package seems like a good idea and something that is used in
other projects. I would recommend to add vme64x_core_priv_pkg.vhd. Thanks other projects. I would recommend to add vme64x_core_priv_pkg.vhd. Thanks
to this, you will be able to "use" this package and import the modules to this, you will be able to "use" this package and import the modules
from it, rather than having to call directly work.module from it, rather than having to call directly work.module
(as suggested in a latter comment). (as suggested in a latter comment).
I don't see the point of adding extra code.
0.11. the naming of the files in hdl/rtl is very chaotic: 0.11. the naming of the files in hdl/rtl is very chaotic:
- Ideally, and by the convention used in other projects, the name of a - Ideally, and by the convention used in other projects, the name of a
wrapper differs from the name of the module it wrapps only by the suffix wrapper differs from the name of the module it wrapps only by the suffix
"x", e.g.: wr_core.vhd and xwr_core.vhd, wrsw_rtu.vhd and xwrsw_rtu.vhd. "x", e.g.: wr_core.vhd and xwr_core.vhd, wrsw_rtu.vhd and xwrsw_rtu.vhd.
I'd recommend to follow this convention I'd recommend to follow this convention
Done.
- the capitalization and using (or not using) of "_" is very inconsistent, - the capitalization and using (or not using) of "_" is very inconsistent,
please fix it. please fix it.
Done.
0.12. In the description of all files, there exists section "dependencies", It 0.12. In the description of all files, there exists section "dependencies", It
does not seem to be appropriately filled in. Remove it from all files or does not seem to be appropriately filled in. Remove it from all files or
fill in appropriately.I recommend the former. fill in appropriately.I recommend the former.
...@@ -115,6 +119,7 @@ about simulatin will come first): ...@@ -115,6 +119,7 @@ about simulatin will come first):
xvme64x_core_pkg.vhd and vme64x_pack.vhd: xvme64x_core_pkg.vhd and vme64x_pack.vhd:
-------------------------------------------------------------------------------- --------------------------------------------------------------------------------
1.1 rename both, merge packages, see 0.11 1.1 rename both, merge packages, see 0.11
Done.
-------------------------------------------------------------------------------- --------------------------------------------------------------------------------
VME64xCore_Top.vhd VME64xCore_Top.vhd
...@@ -144,6 +149,7 @@ VME64xCore_Top.vhd ...@@ -144,6 +149,7 @@ VME64xCore_Top.vhd
git? (it is similar in many files) git? (it is similar in many files)
2.3 [L:367]use new private library for internal components, do not refer directly 2.3 [L:367]use new private library for internal components, do not refer directly
to "work.VME_Funct_Match" etc to "work.VME_Funct_Match" etc
Why ?
2.4 [L:367-397] There is a dedicated module that is used in many projects for 2.4 [L:367-397] There is a dedicated module that is used in many projects for
synchronizing input asynchronous signals. It seems to include the same synchronizing input asynchronous signals. It seems to include the same
functionality as the module used here gc_sync_register. My suggestion is functionality as the module used here gc_sync_register. My suggestion is
...@@ -205,6 +211,7 @@ VME_Funct_Match: ...@@ -205,6 +211,7 @@ VME_Funct_Match:
end if; end if;
end if; end if;
end process; end process;
Done.
-------------------------------------------------------------------------------- --------------------------------------------------------------------------------
VME_IRQ_Controller.vhd VME_IRQ_Controller.vhd
...@@ -234,6 +241,7 @@ VME_IRQ_Controller.vhd ...@@ -234,6 +241,7 @@ VME_IRQ_Controller.vhd
-- by the interrupter. -- by the interrupter.
5.3 [L:159-225] It seems to me that these two process should be a single FSM 5.3 [L:159-225] It seems to me that these two process should be a single FSM
5.4 [L:190] either remove "fsm" from the process name, or make it a real FSM. 5.4 [L:190] either remove "fsm" from the process name, or make it a real FSM.
Done.
-------------------------------------------------------------------------------- --------------------------------------------------------------------------------
VME_User_CSR.vhd VME_User_CSR.vhd
......
hdl/rtl/vme_funct_match.vhd
View file @ 6ce9cc00
...@@ -94,11 +94,11 @@ begin ...@@ -94,11 +94,11 @@ begin
process (clk_i) process (clk_i)
begin begin
if rising_edge(clk_i) then if rising_edge(clk_i) then
if rst_n_i = '0' then if rst_n_i = '0' or decode_start_i = '0' then
s_decode_start_1 <= '0'; s_decode_start_1 <= '0';
s_function_sel <= 0; s_function_sel <= 0;
s_function_sel_valid <= '0'; s_function_sel_valid <= '0';
elsif decode_start_i = '1' then else
s_decode_start_1 <= '1'; s_decode_start_1 <= '1';
for i in 0 to 7 loop for i in 0 to 7 loop
if s_function(i) = '1' then if s_function(i) = '1' then
...@@ -107,10 +107,6 @@ begin ...@@ -107,10 +107,6 @@ begin
exit; exit;
end if; end if;
end loop; end loop;
else
s_decode_start_1 <= '0';
s_function_sel <= 0;
s_function_sel_valid <= '0';
end if; end if;
end if; end if;
end process; end process;
......
hdl/rtl/vme_irq_controller.vhd
View file @ 6ce9cc00
...@@ -136,7 +136,10 @@ entity vme_irq_controller is ...@@ -136,7 +136,10 @@ entity vme_irq_controller is
rst_n_i : in std_logic; rst_n_i : in std_logic;
INT_Level_i : in std_logic_vector (2 downto 0); INT_Level_i : in std_logic_vector (2 downto 0);
INT_Req_i : in std_logic; INT_Req_i : in std_logic;
-- Set when an irq is pending (not yet acknowledged).
irq_pending_o : out std_logic; irq_pending_o : out std_logic;
irq_ack_i : in std_logic; irq_ack_i : in std_logic;
VME_IRQ_n_o : out std_logic_vector (7 downto 1) VME_IRQ_n_o : out std_logic_vector (7 downto 1)
); );
...@@ -187,7 +190,7 @@ begin ...@@ -187,7 +190,7 @@ begin
end if; end if;
end process; end process;
p_main_fsm : process (clk_i) p_main : process (clk_i)
begin begin
if rising_edge(clk_i) then if rising_edge(clk_i) then
if rst_n_i = '0' then if rst_n_i = '0' then
......
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