review: first comments.

a99bde54 · Matthieu Cattin · 93209d87 · a99bde54
Commit a99bde54 authored Nov 11, 2013 by Matthieu Cattin
Hide whitespace changes
Inline Side-by-side

Showing with 69 additions and 0 deletions

matthieu.txt design-review/matthieu.txt +69 -0

No files found.
--- a/design-review/matthieu.txt
+++ b/design-review/matthieu.txt
+##################################################
+Comments on documentation:
+##################################################
+
+- Very nice documentations!
+- I was confused with userguide-conv-ttl-blo.pdf and userguide-conv-ttl-blo-v1.04.pdf, I didn't know which one to choose before opening them.
+  -> I'd remove the old outdated version.
+  -> In general, try not to store binary in git, but in the file section of the ohwr project.
+
+
+##################################################
+Comments on hdl:
+##################################################
+
+General comments:
+
+- I'd add a software reset bit in one of the registers.
+  This would allow reset the fpga remotely whitout power-cycling the crate.
+
+- You should explain what are the top/, syn/, sim/ folders in all the module folders.
+
+- What are those files for m25p_flash.vhd, m25p_flash-with-write.vhd ?
+  I guess they qoing to be clean-up.
+
+
+conv_ttl_blo.ucf:
+
+- System clock fpga_clk is unconstrained!
+- What is CLOCK_DEDICATED_ROUTE = FALSE for on fpga_input_ttl?
+- Are DRIVE and SLEW constraints useful on led outputs?
+
+
+conv_ttl_blo.vhd:
+
+- 
+
+
+xil_multiboot.vhd:
+
+- lines 345-377: Should work if you write:
+  icap_din(0 to 7) <= fsm_icap_dout(7 downto 0);
+  icap_din(8 to 15) <= fsm_icap_dout(15 downto 8);
+  fsm_icap_din(0 to 7) <= icap_dout(7 downto 0);
+  fsm_icap_din(8 to 15) <= icap_dout(15 downto 8);
+
+
+multiboot_fsm.vhd:
+
+- There is no watchdog mechanism. The fsm can be stuck forever in a state if something goes wrong with the spi.
+
+
+multiboot_regs.vhd:
+
+- Ask for the feature you need in wbgen2 and keep the auto-generated file unmodified.
+
+
+i2c_slave.vhd:
+
+- For clarity, I'd put the watchdog reset outside the fsm. Something like:
+   process(clk_i)
+   begin
+     if rising_edge(clk_i) then
+       if (scl_falling = '1') then
+         watchdog_rst <= '1';
+       else
+         watchdog_rst <= '0';
+       end if;
+     end if;
+   end process;