Commit 4afd6871 authored by Tristan Gingold's avatar Tristan Gingold

Remove obsolete documents.

parent c8c252f8
-- vme64x_core review
-- Severity:
-- (!) - serious
-- (m) - medium
-- (_) - cosmetic
-- (o) - optimization
-- (?) - question
-- I've gone sequentially reading the files,
-- so probably some of the details written
-- here are already solved within the cores.
Project in repo:
-- (o) Repo could be restructured. There are references (such as some schematics
and reference pdfs quite outdated) that should be replaced/removed. Take
a look to /trunk/documentation/references.
-- (_) Use Vim/Emacs for better alignment.
-- (_) Comment the Doxygen way and remove unnecesary "End of file" comments?
-- (_) Add name to processes.
-- (m) Reference the "AM table" (line 75) to a document. It could be probably
added the HEX CODEs from the VME6$ reference manual as comments.
-- (o) Probably c_FUNC[X]_ADER_[Y] constants can be all replaced by a
function call. Less error prone.
-- (_) t_transferType error could be renamed to something different: TT_Error.
-- (o) I would put a prefix at the begining of every state of the fsm. For instance,
in t_mainFSMstates (line 303 on):
Besides, I would put a reset state, R0_RESET, to ease the task of
resetting the core (probably already done in IDLE).
-- (o) Same as above for t_initState (line 344).
-- (_) Indenting (Vim is '=' key over a visual block) ;-)
-- (_) Hidden characters (return line in DOS systems) in some parts of the
code (line 689 i.e.)
-- (_) Use consistent initialization of output signal, either inside the
component or outside. See FlipFlopD and EdgeDetection.
-- (_) I prefer having all the inputs and outputs in different lines while
declarating a component.
-- (_) Notation: start with lower cases. See FlipFlopD, EdgeDetection,
-- (_) Notation: some input signals have missed the _i prefix.
-- (_) Probably better to put slv's in VHDL'93 hexadecimal style
(c_amcap, c_amcap0,...).
Notation: c_[HIGERCASES]. Add a reference note to where are the values gotten from.
-- (?) If all the operations are controlled by DS lines (and others), is it necessary
to place double or triple buffering in the address and data lines?
In any case, which is the signal that triggers the FSM? In case there are two,
probably it will be good placed a window wait time to overcome the problems
you had while decoding operation to perform.
-- (_) Probably better to move the process to freeze the data received from the
wishbone operation (line 531) inside VME_Wb_master.vhd
-- (_) Added suffixes _i, _o to the ports.
-- (_) Probably mainFSMreset can drive the 'reset' port directly.
"One reset to rule them all".
In case of if then structures with a little of combinatorial, instead of:
if reset = '1' or mainFSMreset = '1' or (stall_i = '0' and s_cyc = '1') then
I prefer:
if (reset = '1')
or (mainFSMreset = '1')
or (stall_i = '0' and s_cyc = '1') then
think, it is easier to read.
-- (_) In processes in which one of the signals to be updated is updated in much
less situations than the rest of signals, I try to put it in a different
process to have all the cases correctly covered.
Maybe s_bar_written and reset_flag in p_CSR_Write can get independency.
-- (_) Change (clk'event and clk = '1') for rising_edge(clk)
-- (o) Remove "use IEEE.numeric_std.unsigned;", redundant.
-- (o) Maybe better to have a common global reset. Processes for s_wberr1, s_rty1.
-- (?) Debug leds should be removed?
-- (?) VME_RST_n_i what for?
-- (_) Hidden characters.
-- (o) As Tom said, it is probably better group FUNC into an array. Then add some
translation functions in one of the packages, it will help to reduce errors
and being a little less verbose in the components.
-- (o) I would put a general reset control process to keep coherence in the reset
of the module.
-- (_) In the process in line 317, it would be better have records that
which are assigned with predefined record constants. It will reduce the
length and possible errors while assigning values to the outputs.
-- (o) In the same process the when others => clause is missing. Probably it
would be better set a deferred output before the case, so it will always
set an output (no latches here??).
-- (o) Coming back to the process in line 243, the previous comment is applicable.
Recommend to preassign before the case to avoid unwanted latches.
-- (o) Reset as Tom pointed out. Keep consistent notation to avoid this kind of
-- (_) The ports can be reduced if reagrouped. Use of translation function to
keep STD_LOGIC_VECTOR boundaries easy to work with.
This diff is collapsed.
In the comments, it would be useful to explain a bit about the context
this core expects in the PCB, e.g. what chips are used in the SVEC
board (which is the reference platform for this core). This would help
people understand e.g. how open collector outputs are dealt with, or
what the VME_LWORD_n_o and VME_ADDR_o outputs are for.
VME_AS_n_i and VME_DS_n_i are passed to two different synchronizer
blocks in parallel. The actual implementation of this will depend on
how clever the synthesis tool is. If it is not clever, it will not
notice it can tap the three-FF synchronizer to get the double-FF
output, and we will have two parallel synchronizers: not good,
especially considering that one of them will not use the I/O FFs. If
these double-clocked signals are really needed, the triple-clocking
block could deliver an extra ouput port for them.
There are a number of outputs which are supposed to go to the outside
world (VME_DTACK_n_o, WE_o, VME_DATA_DIR_o...) and which are not using
I/O FFs because they are the result of some combinational logic. This
will result in tricky timing issues, and need complex constraints to
P&R to guarantee consistent results. I would strongly advise to make
sure all these signals are outputs of I/O FFs, with any necessary
logic placed *before* these FFs. In general, all inputs to the core
should be sampled before using them, and all outputs should be the
result of clocking in I/O FFs.
There is a comment at line 527 saying:
-- If the S_FPGA will be provided to a core who drives these lines
-- without
-- erase the A_FPGA the above mentioned lines should be changed to 'Z'
-- !!!
This should probably change to become a generic so that people can
decide on the behaviour they want. It will then be more visible, not
hidden in the middle of the code. The sentence itself it quite
cryptic. This core should not assume it is used in a board with a
S_FPGA and a A_FPGA.
Text diagram in the initial comments not clear regarding state
Line 69: "To avoid the time constraint" -> "To respect the timing
Signal naming is not consistent. If the "s_" prefix is used, it should
be used for all internal signals.
Line 104: "reset" is clearly used as an active-low signal in this
entity, and it is an input port, so its name should be "reset_n_i".
Instantiation of many FlipFlopD components: a single clocked process
would be more compact and easier to understand.
Line 247: why should the interrupt controller not be allowed to go to
the IRQ state when VME_IACKIN_n_i='0'?
Line 256: once we have allowed going to the IRQ state even if
VME_IACKIN_n_i is low, we should detect a falling edge of
VME_IACKIN_n_i while in the IRQ state to proceed. A simple detection
of '0' won't work anymore.
Line 321: In the IDLE state this block should drive whatever it gets
from IACKIN into IACKOUT. Otherwise it is blocking interrupt
acknowledgement for any cards to the right of the module where it is
instantiated. In general the IACKIN-IACKOUT daisy-chain needs to be
looked at in detail. Driving IACKOUT with AS as is done in other
states looks weird. IACKOUT should be driven by IACKIN when the IACK
chain is not intercepted by this block.
Line 445: these are active-low signals so their value after reset
should be '1', not '0'.
I don't understand the whole DS_LATCH thing. We detect in state
WAIT_DS that there was some change in the DS lines. This makes us go
to state LATCH_DS, where the DS lines are latched. Then we check if
VME_DS_latched(0) is '0' in the ACK_INT state. Wouldn't it be simpler
to just wait for DS(0) to become '0'?
Line 467: s_enable opens the sampling gate for INT_Req only when
either we have not latched a request (with INT_Req_sample) or when
DTACK is being driven low. In this last case, we are in the DTACK
state (until AS goes up) and any INT_Req pulse will go through the
sampling but in a transient way, i.e. it will not "stick" and we will
lose it. This behavior should be documented so that users of the core
Synthesis warnings:
/vme64x_pack.vhd" Line 805: Function f_div8 does not always return a value.
/VME_CR_CSR_Space.vhd" Line 223: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 227: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 232: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 246: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 253: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 256: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 263: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 267: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 270: Case choice must be a locally static expression
/VME_CR_CSR_Space.vhd" Line 273: Case choice must be a locally static expression
/VME_bus.vhd" Line 506: Assignment to s_2elatchaddr ignored, since the identifier is never used
/VME_bus.vhd" Line 508: Assignment to s_berr ignored, since the identifier is never used
/VME_bus.vhd" Line 1344: Assignment to s_cyclecount ignored, since the identifier is never used
/VME_Funct_Match.vhd" Line 213: Assignment to debugfunct ignored, since the identifier is never used
/VME_Am_Match.vhd" Line 124: Assignment to debugam ignored, since the identifier is never used
/VME_bus.vhd" Line 188: Net <s_phase1addr[63]> does not have a driver.
/VME_bus.vhd" Line 189: Net <s_phase2addr[7]> does not have a driver.
/VME_bus.vhd" Line 204: Net <s_XAM[7]> does not have a driver.
/VME_CR_CSR_Space.vhd" Line 191: Assignment to s_barerror ignored, since the identifier is never used
/VME_bus.vhd" line 1726. All outputs of instance <DS1EdgeDetect> of block <EdgeDetection> are unconnected in block <VME_bus>. Underlying logic will be removed.
/VME_bus.vhd" line 1492: Output port <Funct_Sel> of the instance <Inst_Access_Decode> is unconnected or connected to loadless signal.
/VME_bus.vhd" line 1726: Output port <sigEdge_o> of the instance <DS1EdgeDetect> is unconnected or connected to loadless signal.
Signal <s_phase1addr<63:8>> is used but never assigned. This sourceless signal will be automatically connected to value GND.
Signal <s_phase2addr<7:0>> is used but never assigned. This sourceless signal will be automatically connected to value GND.
Signal <s_XAM> is used but never assigned. This sourceless signal will be automatically connected to value GND.
INFO:Xst:2261 - The FF/Latch <DSinputSample/reg_1_0> in Unit <VME64xCore_Top> is equivalent to the following FF/Latch, which will be removed : <DSinputSample1/reg_1_0>
INFO:Xst:2261 - The FF/Latch <DSinputSample/reg_1_1> in Unit <VME64xCore_Top> is equivalent to the following FF/Latch, which will be removed : <DSinputSample1/reg_1_1>
INFO:Xst:2261 - The FF/Latch <Inst_VME_bus/ASrisingEdge/s_1> in Unit <VME64xCore_Top> is equivalent to the following FF/Latch, which will be removed : <Inst_VME_bus/ASfallingEdge/s_1>
INFO:Xst:2261 - The FF/Latch <ASinputSample/s_1> in Unit <VME64xCore_Top> is equivalent to the following FF/Latch, which will be removed : <ASinputSample1/s_1>
General comments:
* Add a reset output port on core's top level.
This reset should come from the software reset bit in CSR.
* Update the Manifest file.
-> Remove VME_DpBlockRam.vhd
-> Add VME_CRAM.vhd
* Change MBLT endian register name to SWAP configuration register (or something similar).
* Move to a git repository.
-- vme64x_core review
-- Severity:
-- (!) - serious
-- (m) - medium
-- (_) - cosmetic
-- (o) - optimization
-- (?) - question
(_) There is some random use of capital letters throughout the document.
(m) It was pointed out in the presentation that the DS toggle could eventually not be properlly detected. In the datasheet it is stated that the skew between this lines can be up to a maximum of 20ns. A single wait state may be not enough if the vme core is clocked at 100MHz. On the other hand it is not necessary if the core is clocked at f<50Mhz. Probably it could be a good idea to use some generic (as carlos sugested) to insert/remove wait states depending on the clocking frequency.
(m!) As it is already pointed out by Javier the IACKIN-IACKOUT should be looked in more detail. I doubt the interrupt handler can deal correctly with rule 4.41 (30ns min skew between AS release and IACKOUT release). Notice that this rule applies on the VME connector edge so even with an internal latency of 2 cycles at 10ns you still have all the propagations delays to take care of.
This diff is collapsed.
This diff is collapsed.
(!) - serious
(m) - medium
(_) - cosmetic
(o) - optimization
- (m) avoid std_logic_unsigned/std_logic_arith libraries. These are Synopsys's own libraries
(although widesperad and more-or-less supported by every tool), being slowly phased out.
Use numeric_std instead, which the official standard.
- (m) comment ports and generics in each module.
Top Level:
- (_) Process at lines 530+: I would place it in VME_bus. This way the top level module will only
act as an interconnect putting together all VME core components.
- (!) place "KEEP" attribute in synchronizer chain modules to make sure Xilinx's software wont
try to optimize/merge them. Switch off LUT-based shift register inference for these blocks.
- (_) I would recommend grouping the ader/amcap/xamcap ports into an array of records, this makes the code
much easier to read and maintain.
- (!) process driving cardsel and base_addr: sequential nonblocking assignments are potentially
dangerous - you are relying on assumption that if any of bits in s_func_sel is 1, cardsel (previously set
to 0) will be overwritten by the loop. It's safer to use variables and blocking assignments in such cases.
- (o) provide a generic allowing for reduction of device functions. In most cases, we need only 1 A24/A32/D32 BAR.
Decoding and storing BAR configurations takes large percentage of the FPGA resources consumed by the VME core.
- (m) lines 223+ (case statement) wouldn't it be simpler to do the slicing / zero padding in case statement or adjust
the constants in the package file instead of appending zeroes and slicing in each case entry?
- (m) GADER_1 loop: please put a single-line comment describing what the generate loop does.
- (!) line 235: is reset signal active low or high?
- (m) don't comment out code that is not used. Just remove it.
Coding style:
- (_) pass the code via indenting tool (emacs Ctrl-C/Ctrl-B for example)
- (m) _i/_o/_b suffixes on ports, g_ prefixes on generics, c_ prefixes on constants
- (_) avoid comments that do not provide any useful information (for example:
"----------------- entity declaration --- (lots of dashes....)" followed by "entity (".
This diff is collapsed.
This diff is collapsed.
-------------- VmePackage.vhdl ----------------------------------------------
-- Date : Wed Apr 10 15:07:29 2002
-- Author : David Dominguez
-- Company : CERN
-- Description : MTG constants and parameters
library ieee;
use ieee.std_logic_1164.all;
package vme is
-- selects the interrupt number the VME module
-- answer when VmeIackNA cycle is going on
subtype VmeAddressType is std_logic_vector(23 downto 1);
subtype VmeDataType is std_logic_vector(31 downto 0);
subtype ModuleAddrType is std_logic_vector(7 downto 0);
subtype ModuleAddrLType is std_logic_vector(23 downto 23 + ModuleAddrType'right - ModuleAddrType'left);--(23 downto 16);
subtype SlaveAddrOutType is std_logic_vector(ModuleAddrLType'right - 1 downto 1);--(15 downto 1);
subtype IntAddrOutType is std_logic_vector(SlaveAddrOutType'left downto 0);--(15 downto 0);
subtype VmeAddresModType is std_logic_vector(4 downto 0);
subtype VmeIntLevelType is std_logic_vector(2 downto 0);
constant MODULE_ADDRESS_C : ModuleAddrLType := X"C6";--(23 => '1', 22 => '1', others => '0');
constant module_am : VmeAddresModType := "11101";
constant ZEROVMEADDRESS : VmeAddressType := (others => '0');
constant ONEVMEADDRESS : VmeAddressType := (ZEROVMEADDRESS'right => '1', others => '0');
constant STDNONPRIVPROGACC : VmeAddresModType := "11101";
type VmeCellType is
Address : VmeAddressType;
Data : VmeDataType;
Am : VmeAddresModType;
end record VmeCellType;
type VmeAddAmType is
Address : VmeAddressType;
Am : VmeAddresModType;
end record VmeAddAmType;
type IntAddDataType is
Address : integer;
Data : VmeDataType;
end record IntAddDataType;
type VmeBusOutRecord is
-- clk : std_logic;
VmeAddrA : VmeAddressType; -- Top level entity for the GPS2TIM module
-- This module receive the GPS ppsclk and 1 Mhz + the Date and Time in RS232
-- and send the corresponding Timing Frame.
-- PN and BMT le 21 Fevrier 2002
-- Version 1.0
VmeAsNA : std_logic;
VmeAmA : VmeAddresModType;
VmeDs1NA : std_logic;
VmeDs0NA : std_logic;
VmeLwordNA: std_logic;
VmeWriteNA : std_logic;
VmeIackNA : std_logic;
IackInNA : std_logic;
-- ModuleAddr : std_logic_vector(4 downto 0);
-- DataFromMemValid : std_logic;
-- DataFromMem: VmeDataType;
-- data_writen_valid : std_logic;
-- ResetNA : std_logic;
-- UserIntReqN : std_logic;
-- UserBlocks : std_logic;
VmeData : VmeDataType;
writeFinished : boolean;
readFinished : boolean;
takeControl : boolean;
ProcessOnControl : integer;
end record VmeBusOutRecord;
type VmeBusOutRecordArray is array (natural range <>) of VmeBusOutRecord;
type VmeBusInRecord is
VmeData : VmeDataType;
VmeDir : std_logic;
VmeBufOeN : std_logic;
IackOutNA: std_logic;
VmeAsNA : std_logic;
VmeIntReqN : std_logic_vector(7 downto 0);
dtack_n : std_logic;
end record VmeBusInRecord;
--type StdLogVecArray is array (natural range <>) of Std_logic_vector;
-- Interrupt vector to be put on data bus during
-- the VME interrupt cycle
function PullUpStdLogVec(input1, input2 : Std_logic_vector) return Std_logic_vector;
function PullUpStdLog(input1, input2 : Std_logic) return Std_logic ;
function PullUpVmeBusOut(inputs : VmeBusOutRecordArray) return VmeBusOutRecord;
--function PullUpStdLog (inputs : std_logic_vector) return
package body vme is
function PullUpStdLogVec(input1, input2 : Std_logic_vector) return Std_logic_vector is
variable vStdLogVec : Std_logic_vector(31 downto 0);
vStdLogVec := (others => 'U');
for M in input2'range loop
case input2(M) is
when 'U' |'X' | 'W' => if input1(M) = '0' or input1(M) = 'L' then
vStdLogVec(M) := '0';
end if;
-- when 'X' => vStdLog := 'U';
when '0' => vStdLogVec(M) := '0';
when '1' => vStdLogVec(M) := input1(M);
when 'Z' => vStdLogVec(M) := input1(M);
-- when 'W' => vStdLogVec(M) := 'U';
when 'L' => vStdLogVec(M) := '0';
when 'H' => vStdLogVec(M) := input1(M);
when others => null;
end case;
end loop;
return vStdLogVec(input2'range);
function PullUpStdLog(input1, input2 : Std_logic) return Std_logic is
variable vStdLog : Std_logic;
vStdLog := 'U';
case input2 is
when 'U' |'X' | 'W' => if input1 = '0' or input1 = 'L' then
vStdLog := '0';
end if;
-- when 'X' => vStdLog := 'U';
when '0' => vStdLog := '0';
when '1' => vStdLog := input1;
when 'Z' => vStdLog := input1;
-- when 'W' => vStdLog := 'U';
when 'L' => vStdLog := '0';
when 'H' => vStdLog := input1;
-- when '-' => vStdLog := 'U';
when others => null;
end case;
return vStdLog;
function PullUpVmeBusOut(inputs : VmeBusOutRecordArray) return VmeBusOutRecord is
variable vVmeBusOut : VmeBusOutRecord;
vVmeBusOut.VmeAddrA := (others => '1');
vVmeBusOut.VmeAsNA := '1';
vVmeBusOut.VmeAmA := (others => '1');
vVmeBusOut.VmeDs1NA := '1';
vVmeBusOut.VmeDs0NA := '1';
vVmeBusOut.VmeLwordNA:= '1';
vVmeBusOut.VmeWriteNA := '1';
vVmeBusOut.VmeIackNA := '1';
vVmeBusOut.IackInNA := '1';
vVmeBusOut.VmeData := (others => 'Z');
vVmeBusOut.writeFinished := false;
vVmeBusOut.readFinished := false;
vVmeBusOut.ProcessOnControl := -1;
for I in inputs'range loop
if inputs(I).takeControl then
vVmeBusOut.VmeAddrA := inputs(I).VmeAddrA;
vVmeBusOut.VmeAsNA := inputs(I).VmeAsNA ;
vVmeBusOut.VmeAmA := inputs(I).VmeAmA;
vVmeBusOut.VmeDs1NA := inputs(I).VmeDs1NA ;
vVmeBusOut.VmeDs0NA := inputs(I).VmeDs0NA ;
vVmeBusOut.VmeLwordNA:= inputs(I).VmeLwordNA;
vVmeBusOut.VmeWriteNA := inputs(I).VmeWriteNA ;
vVmeBusOut.VmeIackNA := inputs(I).VmeIackNA ;
vVmeBusOut.IackInNA := inputs(I).IackInNA ;
vVmeBusOut.VmeData := inputs(I).VmeData ;
vVmeBusOut.writeFinished := inputs(I).writeFinished;
vVmeBusOut.readFinished := inputs(I).readFinished;
vVmeBusOut.ProcessOnControl := I;
end if;
end loop;
return vVmeBusOut;
This diff is collapsed.
module ga_decoder(GA, BAR);
input[4:0] GA;
output[7:0] BAR;
wire[4:0] GA;
reg[7:0] BAR;
always @(GA)
5'h1E: BAR <= 8'h08; //SLOT 1
5'h1D: BAR <= 8'h10; //SLOT 2
5'h1C: BAR <= 8'h18; //SLOT 3
5'h1B: BAR <= 8'h20; //SLOT 4
5'h1A: BAR <= 8'h28; //SLOT 5
5'h19: BAR <= 8'h30; //SLOT 6
5'h18: BAR <= 8'h38; //SLOT 7
5'h17: BAR <= 8'h40; //SLOT 8
5'h16: BAR <= 8'h48; //SLOT 9
5'h15: BAR <= 8'h50; //SLOT 10
5'h14: BAR <= 8'h58; //SLOT 11
5'h13: BAR <= 8'h60; //SLOT 12
5'h12: BAR <= 8'h68; //SLOT 13
5'h11: BAR <= 8'h70; //SLOT 14
5'h10: BAR <= 8'h78; //SLOT 15
5'h0F: BAR <= 8'h80; //SLOT 16
5'h0E: BAR <= 8'h88; //SLOT 17
5'h0D: BAR <= 8'h90; //SLOT 18
5'h0C: BAR <= 8'h98; //SLOT 19
5'h0B: BAR <= 8'hA0; //SLOT 20
5'h0A: BAR <= 8'hA8; //SLOT 21
`timescale 1 ns / 1 ps
module glbl ();
wire GR;
wire GSR;
wire GTS;
wire PRLD;