From 8b8f1755532d7f12c5d7078ecf6c4876cdda1f20 Mon Sep 17 00:00:00 2001
From: Reinier van der Walle <walle@astron.nl>
Date: Tue, 2 Nov 2021 17:11:56 +0100
Subject: [PATCH] processed review comments

---
 .../src/vhdl/lofar2_unb2b_ring.vhd            |  3 --
 .../dp/src/vhdl/dp_block_validate_err.vhd     |  4 ---
 .../base/dp/src/vhdl/dp_fifo_fill_eop.vhd     | 33 +++++++++++--------
 .../base/dp/tb/vhdl/tb_dp_fifo_fill_eop.vhd   |  9 +++--
 .../dp/tb/vhdl/tb_tb_dp_fifo_fill_eop.vhd     |  7 ++--
 libraries/base/ring/src/vhdl/ring_lane.vhd    |  2 --
 libraries/base/ring/src/vhdl/ring_rx.vhd      |  8 +----
 7 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/applications/lofar2/designs/lofar2_unb2b_ring/src/vhdl/lofar2_unb2b_ring.vhd b/applications/lofar2/designs/lofar2_unb2b_ring/src/vhdl/lofar2_unb2b_ring.vhd
index 5891567b0a..612609097a 100644
--- a/applications/lofar2/designs/lofar2_unb2b_ring/src/vhdl/lofar2_unb2b_ring.vhd
+++ b/applications/lofar2/designs/lofar2_unb2b_ring/src/vhdl/lofar2_unb2b_ring.vhd
@@ -136,7 +136,6 @@ ARCHITECTURE str OF lofar2_unb2b_ring IS
   CONSTANT c_nof_tx_monitors           : NATURAL := c_sdp_N_pn_max; 
   CONSTANT c_err_bi                    : NATURAL := 0; 
   CONSTANT c_nof_err_counts            : NATURAL := 8; 
-  CONSTANT c_validate_err_fifo_size    : NATURAL := 1536; 
   CONSTANT c_bsn_at_sync_check_channel : NATURAL := 1; 
   CONSTANT c_validate_channel          : BOOLEAN := TRUE; 
   CONSTANT c_validate_channel_mode     : STRING  := "=";
@@ -770,7 +769,6 @@ BEGIN
       g_nof_tx_monitors           => c_nof_tx_monitors,
       g_err_bi                    => c_err_bi,
       g_nof_err_counts            => c_nof_err_counts,
-      g_validate_err_fifo_size    => c_validate_err_fifo_size,
       g_bsn_at_sync_check_channel => c_bsn_at_sync_check_channel,
       g_validate_channel          => c_validate_channel,
       g_validate_channel_mode     => c_validate_channel_mode,
@@ -822,7 +820,6 @@ BEGIN
       g_nof_tx_monitors           => c_nof_tx_monitors,
       g_err_bi                    => c_err_bi,
       g_nof_err_counts            => c_nof_err_counts,
-      g_validate_err_fifo_size    => c_validate_err_fifo_size,
       g_bsn_at_sync_check_channel => c_bsn_at_sync_check_channel,
       g_validate_channel          => c_validate_channel,
       g_validate_channel_mode     => c_validate_channel_mode,
diff --git a/libraries/base/dp/src/vhdl/dp_block_validate_err.vhd b/libraries/base/dp/src/vhdl/dp_block_validate_err.vhd
index f661fdbf53..021806547b 100644
--- a/libraries/base/dp/src/vhdl/dp_block_validate_err.vhd
+++ b/libraries/base/dp/src/vhdl/dp_block_validate_err.vhd
@@ -35,10 +35,6 @@
 --     result in multiple counters increasing per block. Therefore, it should not be 
 --     assumed that the sum of the err counters is the total amount of discarded
 --     blocks.
---   . Note that dp_fifo_fill_eop cannot handle continues stream of blocks without 
---     a gap between blocks the dp_fifo_fill_eop needs 1 cycle to process a block.
---     Streaming without gaps may cause the fifo to overflow. Bursts of blocks
---     can be handled by increasing g_fifo_size.
 --   . g_max/min_block_size indicate the minimum / maximum length of incoming blocks.
 --     The ratio of max / min is used to determine a fifo size for the outgoing
 --     sosi.valid signals. To minimize logic the g_min_block_size can be set to
diff --git a/libraries/base/dp/src/vhdl/dp_fifo_fill_eop.vhd b/libraries/base/dp/src/vhdl/dp_fifo_fill_eop.vhd
index 48a351687b..0bacb8c204 100644
--- a/libraries/base/dp/src/vhdl/dp_fifo_fill_eop.vhd
+++ b/libraries/base/dp/src/vhdl/dp_fifo_fill_eop.vhd
@@ -207,6 +207,7 @@ BEGIN
     rd_eop_new     <= '1';
   END GENERATE;
 
+  -- Set rd_eop_cnt outside generate statements to avoid Modelsim warning "Nonresolved signal 'rd_eop_cnt' may have multiple sources".
   rd_eop_cnt <= TO_UINT(reg_rd_eop_cnt) WHEN g_use_dual_clock ELSE wr_eop_cnt;
 
   p_eop_cnt: PROCESS(wr_clk, wr_rst)
@@ -217,37 +218,41 @@ BEGIN
       wr_eop_cnt <= 0;
       wr_eop_new <= '0'; 
     ELSIF rising_edge(wr_clk) THEN
-      IF g_use_dual_clock THEN -- transfer eop counter accross clock domains
-        v_wr_eop_cnt := wr_eop_cnt;
-        IF wr_eop_done = '1' THEN
+      -- We need to control in_new signal for common_reg_cross_domain. We can simply pulse in_new after in_done = '1'.
+      -- After we have send the wr_eop_cnt accross the clock domain by seting wr_eop_new, the wr_eop_cnt is reset to 0.
+      -- It is not possible to set in_new = snk_in.eop as there can be more snk_in.eop during the clock cross time necessary by common_reg_cross_domain.
+      IF g_use_dual_clock THEN 
+        v_wr_eop_cnt := wr_eop_cnt; 
+        
+        -- When done = 1, busy can be set to 0.
+        IF wr_eop_done = '1' THEN 
           wr_eop_busy <= '0';
         END IF;
-  
+        -- If common_reg_cross_domain is not busy transfering the register we can initiate a new transfer by setting wr_eop_new.
         IF wr_eop_busy = '0' THEN
           wr_eop_busy <= '1';
           wr_eop_new <= '1'; 
         END IF; 
   
+        -- After we transfered wr_eop_cnt, we can reset it to 0.
         IF wr_eop_new = '1' THEN
           wr_eop_new <= '0';
           v_wr_eop_cnt := 0;
         END IF;
   
-        IF snk_in.eop = '1' THEN
+        -- Count incoming snk_in.eop
+        IF snk_in.eop = '1' THEN 
           v_wr_eop_cnt := v_wr_eop_cnt + 1;
         END IF;
         wr_eop_cnt <= v_wr_eop_cnt;
 
-      ELSE -- No need to transfer eop counter across clock domains for single clock
-        IF wr_rst='1' THEN
+      -- No need to transfer eop counter across clock domains for single clock
+      ELSE 
+        IF snk_in.eop = '1' THEN
+          wr_eop_cnt <= 1; -- wr_eop_cnt can simply be set to 1 instead of counting as it is immidiatly processed due to having a single clock.
+        ELSE
           wr_eop_cnt <= 0;
-        ELSIF rising_edge(wr_clk) THEN
-          IF snk_in.eop = '1' THEN
-            wr_eop_cnt <= 1;
-          ELSE
-            wr_eop_cnt <= 0;
-          END IF; 
-        END IF;
+        END IF; 
       END IF;
     END IF;
   END PROCESS; 
diff --git a/libraries/base/dp/tb/vhdl/tb_dp_fifo_fill_eop.vhd b/libraries/base/dp/tb/vhdl/tb_dp_fifo_fill_eop.vhd
index f4cdef3d2f..c460f036be 100644
--- a/libraries/base/dp/tb/vhdl/tb_dp_fifo_fill_eop.vhd
+++ b/libraries/base/dp/tb/vhdl/tb_dp_fifo_fill_eop.vhd
@@ -55,7 +55,8 @@ ENTITY tb_dp_fifo_fill_eop IS
     g_dut_fifo_rl         : NATURAL := 1;                  -- internal RL,  use 0 for look ahead FIFO, default 1 for normal FIFO
     g_dut_fifo_size       : NATURAL := 128;
     g_dut_fifo_fill       : NATURAL := 100;               -- selectable >= 0 for dp_fifo_fill
-    g_dut_use_rd_fill_32b : BOOLEAN := FALSE 
+    g_dut_use_rd_fill_32b : BOOLEAN := FALSE;
+    g_dut_use_gap         : BOOLEAN := TRUE   
   );
 END tb_dp_fifo_fill_eop;
 
@@ -74,7 +75,7 @@ ARCHITECTURE tb OF tb_dp_fifo_fill_eop IS
   CONSTANT c_tx_void        : NATURAL := sel_a_b(c_tx_latency, 1, 0);  -- used to avoid empty range VHDL warnings when c_tx_latency=0
   CONSTANT c_tx_offset_sop  : NATURAL := 3;
   CONSTANT c_tx_period_sop  : NATURAL := 14;                  -- sop in data valid cycle 3,  17,  31, ...
-  CONSTANT c_tx_offset_eop  : NATURAL := 12;                  -- eop in data valid cycle 12,  26,  40, ...
+  CONSTANT c_tx_offset_eop  : NATURAL := sel_a_b(g_dut_use_gap, 12, 16);  -- eop in data valid cycle 12,  26,  40, ...
   CONSTANT c_tx_period_eop  : NATURAL := c_tx_period_sop;
   CONSTANT c_tx_offset_sync : NATURAL := 3;                  -- sync in data valid cycle 3, 20, 37, ...
   CONSTANT c_tx_period_sync : NATURAL := 17;
@@ -163,7 +164,9 @@ BEGIN
   proc_dp_tx_ctrl(c_tx_offset_sync, c_tx_period_sync, in_data, in_val, in_sync);
   proc_dp_tx_ctrl(c_tx_offset_sop, c_tx_period_sop, in_data, in_val, in_sop);
   proc_dp_tx_ctrl(c_tx_offset_eop, c_tx_period_eop, in_data, in_val, in_eop);
-  proc_dp_tx_ctrl(c_tx_offset_gap, c_tx_period_gap, in_data, in_val, gap_en);
+  gen_gap: IF g_dut_use_gap GENERATE
+    proc_dp_tx_ctrl(c_tx_offset_gap, c_tx_period_gap, in_data, in_val, gap_en);
+  END GENERATE;
 
   in_bsn     <= INCR_UVEC(in_data, c_bsn_offset);
   in_empty   <= INCR_UVEC(in_data, c_empty_offset);
diff --git a/libraries/base/dp/tb/vhdl/tb_tb_dp_fifo_fill_eop.vhd b/libraries/base/dp/tb/vhdl/tb_tb_dp_fifo_fill_eop.vhd
index 0eb35ac9ec..cd5259d783 100644
--- a/libraries/base/dp/tb/vhdl/tb_tb_dp_fifo_fill_eop.vhd
+++ b/libraries/base/dp/tb/vhdl/tb_tb_dp_fifo_fill_eop.vhd
@@ -45,10 +45,13 @@ END tb_tb_dp_fifo_fill_eop;
 ARCHITECTURE tb OF tb_tb_dp_fifo_fill_eop IS
   SIGNAL tb_end : STD_LOGIC := '0';  -- declare tb_end to avoid 'No objects found' error on 'when -label tb_end'
 BEGIN
-  -- Try FIFO settings : GENERIC MAP (g_dut_use_dual_clock, g_dut_use_bsn, g_dut_use_empty, g_dut_use_channel, g_dut_use_sync, g_dut_fifo_rl, g_dut_fifo_size, g_dut_fifo_fill, g_dut_use_rd_fill_32b)
+  -- Try FIFO settings : GENERIC MAP (g_dut_use_dual_clock, g_dut_use_bsn, g_dut_use_empty, g_dut_use_channel, g_dut_use_sync, g_dut_fifo_rl, g_dut_fifo_size, g_dut_fifo_fill, g_dut_use_rd_fill_32b, g_dut_use_gap)
   u_dut_sc_1          : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => FALSE, g_dut_fifo_rl => 1); 
   u_dut_dc_1          : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => TRUE,  g_dut_fifo_rl => 1);
   u_dut_sc_0          : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => FALSE, g_dut_fifo_rl => 0); 
   u_dut_dc_0          : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => TRUE,  g_dut_fifo_rl => 0);
-  
+  u_dut_sc_1_no_gap   : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => FALSE, g_dut_fifo_rl => 1, g_dut_use_gap => FALSE); 
+  u_dut_dc_1_no_gap   : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => TRUE,  g_dut_fifo_rl => 1, g_dut_use_gap => FALSE);
+  u_dut_sc_0_no_gap   : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => FALSE, g_dut_fifo_rl => 0, g_dut_use_gap => FALSE); 
+  u_dut_dc_0_no_gap   : ENTITY work.tb_dp_fifo_fill_eop GENERIC MAP (g_dut_use_dual_clock => TRUE,  g_dut_fifo_rl => 0, g_dut_use_gap => FALSE);  
 END tb;
diff --git a/libraries/base/ring/src/vhdl/ring_lane.vhd b/libraries/base/ring/src/vhdl/ring_lane.vhd
index bf2ba0fe87..e0afcc2eff 100644
--- a/libraries/base/ring/src/vhdl/ring_lane.vhd
+++ b/libraries/base/ring/src/vhdl/ring_lane.vhd
@@ -50,7 +50,6 @@ ENTITY ring_lane IS
     g_nof_tx_monitors           : NATURAL := 1;
     g_err_bi                    : NATURAL := 0; -- ring_rx bit index in sosi.err field to set for wrongly sized packets 
     g_nof_err_counts            : NATURAL := 1; -- nof counters to count the set err bits in range sosi.err(g_nof_err_counts-1 DOWNTO 0)
-    g_validate_err_fifo_size    : NATURAL := 1536; -- should be >= g_lane_packet_length
     g_bsn_at_sync_check_channel : NATURAL := 1; -- on which channel should the bsn be checked
     g_validate_channel          : BOOLEAN := TRUE;
     g_validate_channel_mode     : STRING := ">";
@@ -122,7 +121,6 @@ BEGIN
     g_err_bi          => g_err_bi, 
     g_block_size      => g_lane_packet_length, 
     g_nof_err_counts  => g_nof_err_counts, 
-    g_fifo_size       => g_validate_err_fifo_size, 
     g_check_channel   => g_bsn_at_sync_check_channel, 
     g_sync_timeout    => g_sync_timeout
   )
diff --git a/libraries/base/ring/src/vhdl/ring_rx.vhd b/libraries/base/ring/src/vhdl/ring_rx.vhd
index 7ac8a4a6e7..c7096b82a6 100644
--- a/libraries/base/ring/src/vhdl/ring_rx.vhd
+++ b/libraries/base/ring/src/vhdl/ring_rx.vhd
@@ -24,11 +24,6 @@
 
 -- Purpose: Handle RX side of ring design.
 -- Description: See https://support.astron.nl/confluence/x/jyu7Ag
--- Remark:
--- . Note that the dp_fifo_fill_eop in dp_block_validate_err cannot handle
---   continues stream of blocks without a gap between blocks the dp_fifo_fill_eop 
---   needs 1 cycle to process a block. Streaming without gaps may cause the fifo 
---   to overflow. Bursts of blocks can be handled by increasing g_fifo_size.
 
 -------------------------------------------------------------------------------
 
@@ -49,7 +44,6 @@ ENTITY ring_rx IS
     g_err_bi           : NATURAL := 0;  
     g_block_size       : NATURAL := 1024; 
     g_nof_err_counts   : NATURAL := 1; 
-    g_fifo_size        : NATURAL := 1536; 
     g_check_channel    : NATURAL := 1;
     g_sync_timeout     : NATURAL := 220*10**6  -- 10% margin
   );
@@ -124,7 +118,7 @@ BEGIN
     g_max_block_size  => c_packet_size,
     g_min_block_size  => c_packet_size,
     g_nof_err_counts  => g_nof_err_counts,
-    g_fifo_size       => g_fifo_size,
+    g_fifo_size       => c_packet_size, -- can be same as g_max_block_size as src_in.ready = '1'
     g_data_w          => g_data_w
   )
   PORT MAP (
-- 
GitLab