From ccd4ea65032c450bff04503677bf6dfb91a2ea99 Mon Sep 17 00:00:00 2001
From: Eric Kooistra <kooistra@astron.nl>
Date: Thu, 14 Apr 2022 15:16:16 +0200
Subject: [PATCH] Use dp_sop instead of mm_done and explained why. This makes
 the code for passing on dp_header_info more robust and clear.

---
 .../sdp/src/vhdl/sdp_statistics_offload.vhd   | 73 ++++++++++++++-----
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/applications/lofar2/libraries/sdp/src/vhdl/sdp_statistics_offload.vhd b/applications/lofar2/libraries/sdp/src/vhdl/sdp_statistics_offload.vhd
index 1e66291922..9ec1557d8c 100644
--- a/applications/lofar2/libraries/sdp/src/vhdl/sdp_statistics_offload.vhd
+++ b/applications/lofar2/libraries/sdp/src/vhdl/sdp_statistics_offload.vhd
@@ -25,10 +25,12 @@
 -- Purpose:
 -- . SDP statistics offload
 -- Description:
--- https://support.astron.nl/confluence/display/L2M/L5+SDPFW+Design+Document%3A+Subband+filterbank
--- . See figure 4.3
--- https://plm.astron.nl/polarion/#/project/LOFAR2System/wiki/L2%20Interface%20Control%20Documents/SC%20to%20SDP%20ICD
--- . See 2.9.4 Station Control (L3-SC) - SDP Firmware (L4-SDPFW)
+-- [1] https://support.astron.nl/confluence/display/L2M/L5+SDPFW+Design+Document%3A+Subband+filterbank
+--     . See figure 4.3
+-- [2] https://support.astron.nl/confluence/display/L2M/L5+SDPFW+Design+Document%3A+Subband+Correlator
+--     . See Figure 3.7
+-- [3] https://plm.astron.nl/polarion/#/project/LOFAR2System/wiki/L2%20Interface%20Control%20Documents/SC%20to%20SDP%20ICD
+--     . See 2.9.4 Station Control (L3-SC) - SDP Firmware (L4-SDPFW)
 --
 -- . endianess
 --   Within a 32bit MM word the values are stored with LSByte at lowest byte
@@ -225,6 +227,7 @@ ARCHITECTURE str OF sdp_statistics_offload IS
   SIGNAL trigger_en               : STD_LOGIC := '0';
   SIGNAL trigger_offload          : STD_LOGIC := '0';
   SIGNAL mm_done                  : STD_LOGIC := '0';
+  SIGNAL dp_sop                   : STD_LOGIC := '0';
   SIGNAL dp_block_from_mm_src_out : t_dp_sosi;
   SIGNAL dp_block_from_mm_src_in  : t_dp_siso;
   
@@ -349,7 +352,7 @@ BEGIN
 
   data_id_slv <= func_sdp_map_stat_data_id(g_statistics_type, data_id_rec);
 
-  p_control_packet_offload : PROCESS(r, in_sosi, local_si_offset, trigger_offload, nof_crosslets, crosslets_info, nof_packets, mm_done, dp_header_info)
+  p_control_packet_offload : PROCESS(r, in_sosi, local_si_offset, trigger_offload, nof_crosslets, crosslets_info, nof_packets, dp_sop, dp_header_info)
     VARIABLE v       : t_reg;
     VARIABLE v_index : NATURAL;
   BEGIN
@@ -372,18 +375,33 @@ BEGIN
       END IF;
     END IF;
 
-    -- Capture nof_crosslets and crosslets_info at in_sosi.sync, to make sure
-    -- they do not change during packets offload. The trigger_offload occurs
-    -- after the nof_cycles_dly and the offload will have finished before the
-    -- next in_sosi.sync
+
+    -- For XST offload capture nof_crosslets and crosslets_info at in_sosi.sync,
+    -- to make sure they do not change during packets offload.
+    -- . The sdp_crosslets_subband_select.vhd takes in [2] takes care that
+    --   nof_crosslets and crosslets_info are valid at the xsel_sosi.sync. The
+    --   mmp_dp_bsn_align_v2 in [2] then aligns the local xsel_sosi with the
+    --   remote data and passes on the sync. After some latency the sync
+    --   arrives at the sdp_statistics_offload. This latency is very short
+    --   compared to the sync period, so the nof_crosslets and crosslet_info
+    --   are still valid at the in_sosi.sync.
     IF in_sosi.sync = '1' THEN
       v.nof_crosslets      := TO_UINT(nof_crosslets);
       v.crosslets_info_rec := func_sdp_map_crosslets_info(crosslets_info);
     END IF;
 
-    -- Issue start_pulse per packet offload
+    -- The trigger_offload occurs nof_cycles_dly after the in_sosi.sync and the
+    -- offload will have finished before the next in_sosi.sync, because
+    -- c_sdp_offload_time is such that all offload will finish within 100 ms
+    -- and the integration interval (= sync interval) is 1 s for SST and BST
+    -- and minimal 0.1s (= c_sdp_xst_nof_clk_per_sync_min) for XST.
+    -- The trigger_offload initializes the control for the first packet offload
+    -- in every sync interval.
+    -- . Issue a start_pulse per packet offload. The start_pulse is used by
+    --   u_dp_block_from_mm_dc to read the packet from statistics memory.
     IF trigger_offload = '1' THEN
-      -- Use trigger_offload to start first packet offload, all g_statistics_type start from start address 0
+      -- Use trigger_offload to start first packet offload, all
+      -- g_statistics_type start from start address 0
       v.start_pulse        := '1';
       v.sync               := '1';
       v.start_address      := 0;
@@ -394,8 +412,20 @@ BEGIN
       v.instance_count     := 0;  -- only used for XST
       v.instance_address   := 0;  -- only used for XST
 
-    ELSIF mm_done = '1' THEN
-      -- Use mm_done to start next packets offloads.
+    -- The dp_sop = '1' when the packet has been read from statistics memory
+    -- and is about to get out of the dp_fifo_fill_eop in
+    -- u_dp_block_from_mm_dc. The difference between dp_sop and the mm_done
+    -- output of u_dp_block_from_mm_dc, is that dp_sop also includes any
+    -- dp_fifo_fill_eop latency. This ensures that the dp_sop identifies the
+    -- sop of the offload packet. At the dp_sop:
+    -- . the dp_header_info per packet offload can be released
+    -- . the next packet offload can be prepared
+    --
+    ELSIF dp_sop = '1' THEN
+      -- Release dp_header_info for current packet offload
+      v.dp_header_info := dp_header_info;
+
+      -- Start next packets offload.
       IF r.packet_count < nof_packets - 1 THEN
         IF g_statistics_type = "SST" THEN
           --                 step        step        step        step        step        step
@@ -444,10 +474,6 @@ BEGIN
       END IF;
     END IF;
 
-    -- Release dp_header_info per packet offload
-    IF trigger_offload = '1' OR mm_done = '1' THEN
-      v.dp_header_info := dp_header_info;
-    END IF;
     nxt_r <= v;
   END PROCESS;
 
@@ -487,13 +513,24 @@ BEGIN
     sync_in       => r.sync,
     bsn_at_sync   => bsn_at_sync, 
     start_address => r.start_address,
-    done          => mm_done,
+    done          => mm_done,  -- not used, use dp_sop instead
     mm_mosi       => master_mosi,
     mm_miso       => master_miso,
     out_sosi      => dp_block_from_mm_src_out,
     out_siso      => dp_block_from_mm_src_in
   );
 
+  -- Use dp_block_from_mm_src_out.sop as dp_sop, to include the
+  -- dp_fifo_fill_eop that is in dp_block_from_mm_dc. The dp_sop thus is the
+  -- sop of the packet that is about to be offloaded by u_dp_offload_tx_v3.
+  -- The r.dp_header_info must be available at the dp_offload_snk_in.sop.
+  -- This is guaranteed because:
+  -- . r.dp_header_info is available one clock cycle after dp_sop in
+  --   p_control_packet_offload.
+  -- . The dp_offload_snk_in is delayed also by one clock cycle by
+  --   u_dp_pipeline_ready.
+  dp_sop <= dp_block_from_mm_src_out.sop;
+
   u_dp_pipeline_ready : ENTITY dp_lib.dp_pipeline_ready
   PORT MAP(
     rst          => dp_rst,
-- 
GitLab