processed review comments

applications/lofar2/libraries/sdp/src/vhdl/sdp_crosslets_subband_select.vhd

@@ -27,7 +27,8 @@
 -- The Crosslet subband select selects N_crosslets from each incoming block. Per 
 -- crosslet there are S_pn = 12 subbands, one from each signal input of the PN.
 -- Remark:
--- .
+-- . See L5 SDPFW Design Document: Subband Correlator
+--   Link: https://support.astron.nl/confluence/pages/viewpage.action?spaceKey=L2M&title=L5+SDPFW+Design+Document%3A+Subband+Correlator
 -------------------------------------------------------------------------------
 
 LIBRARY IEEE, common_lib, dp_lib, reorder_lib, st_lib;
@@ -67,14 +68,14 @@ ARCHITECTURE str OF sdp_crosslets_subband_select IS
 
 
   CONSTANT c_crosslets_info_dly  : NATURAL := 1;
-  CONSTANT c_row_select_slv_w    : NATURAL := ceil_log2(c_Sdp_P_pfb);
+  CONSTANT c_row_select_slv_w    : NATURAL := ceil_log2(c_sdp_P_pfb);
   CONSTANT c_row_select_pipeline : NATURAL := 1;
   CONSTANT c_out_sosi_pipeline   : NATURAL := 1;
 
   TYPE t_crosslets_control_reg IS RECORD  -- local registers
-    i_offset         : NATURAL;
-    i_row            : NATURAL;
-    i_col            : NATURAL;
+    offset_index     : NATURAL;
+    row_index        : NATURAL;
+    col_index        : NATURAL;
     step             : NATURAL RANGE 0 TO c_sdp_N_sub-1;
     offsets          : t_natural_arr(g_N_crosslets-1 DOWNTO 0);
     started          : STD_LOGIC;
@@ -167,11 +168,12 @@ BEGIN
     v := r;
     v.col_select_mosi := c_mem_mosi_rst;
 
+    -- start/restart
     IF start_trigger = '1' THEN
       v.started       := '1';
-      v.i_offset := 0;
-      v.i_row := 0;
-      v.i_col := 0;
+      v.offset_index  := 0;
+      v.row_index     := 0;
+      v.col_index     := 0;
       v.sync_detected := '0';
 
       v.step := TO_UINT(crosslets_info_reg(c_sdp_crosslets_info_reg_w-1 DOWNTO c_sdp_crosslets_info_reg_w - c_sdp_crosslets_index_w));
@@ -184,40 +186,44 @@ BEGIN
       v.sync_detected := '1';
     END IF;
 
-    IF r.started = '1' AND in_sosi_arr(0).eop = '1' AND r.sync_detected = '1' THEN -- change offsets 1 packet after the sync due to the buffered packet in reorder_col_wide_select
+    IF r.started = '1' THEN 
+      -- add step to offsets
+      IF in_sosi_arr(0).eop = '1' AND r.sync_detected = '1' THEN -- change offsets 1 packet after the sync due to the buffered packet in reorder_col_wide_select
         v.sync_detected := '0';
         FOR I IN 0 TO g_N_crosslets-1 LOOP
           v_offsets(I) := r.offsets(I) + TO_UINT(crosslets_info_reg(c_sdp_crosslets_info_reg_w-1 DOWNTO c_sdp_crosslets_info_reg_w - c_sdp_crosslets_index_w));
         END LOOP;
       END IF;
 
-    IF col_select_miso.waitrequest = '0' AND r.started = '1' THEN
-      IF r.i_col >= c_sdp_Q_fft-1 THEN
-        v.i_col := 0;
-        IF r.i_row >= c_sdp_P_pfb-1 THEN
-          v.i_row := 0;
-          IF r.i_offset >= g_N_crosslets-1 THEN
-            v.i_offset := 0;
+      -- Make col/row selection 
+      IF col_select_miso.waitrequest = '0' THEN
+        IF r.col_index >= c_sdp_Q_fft-1 THEN
+          v.col_index := 0;
+          IF r.row_index >= c_sdp_P_pfb-1 THEN
+            v.row_index := 0;
+            IF r.offset_index >= g_N_crosslets-1 THEN
+              v.offset_index := 0;
             ELSE
-            v.i_offset := r.i_offset+1;
+              v.offset_index := r.offset_index+1;
             END IF;
           ELSE
-          v.i_row := r.i_row+1;
+            v.row_index := r.row_index+1;
           END IF;
         ELSE 
-        v.i_col := r.i_col+1;
+          v.col_index := r.col_index+1;
         END IF;
 
         v.col_select_mosi.rd := '1';
-      v.col_select_mosi.address(c_sdp_crosslets_index_w-1 DOWNTO 0) := TO_UVEC(c_sdp_Q_fft*v_offsets(r.i_offset) + r.i_col, c_sdp_crosslets_index_w);
-      v.row_select_slv := TO_UVEC(r.i_row, c_row_select_slv_w);
+        v.col_select_mosi.address(c_sdp_crosslets_index_w-1 DOWNTO 0) := TO_UVEC(c_sdp_Q_fft*v_offsets(r.offset_index) + r.col_index, c_sdp_crosslets_index_w);
+        v.row_select_slv := TO_UVEC(r.row_index, c_row_select_slv_w);
+      END IF;
     END IF;
     v.offsets := v_offsets;
     nxt_r <= v;
   END PROCESS;
 
   col_select_mosi <= r.col_select_mosi;
-
+  -- pipeline to time row select
   u_pipe_row_select : ENTITY common_lib.common_pipeline
   GENERIC MAP(
     g_pipeline => c_row_select_pipeline,
@@ -285,6 +291,7 @@ BEGIN
   ---------------------------------------------------------------
   -- Out Crosslet info pipeline
   ---------------------------------------------------------------
+  -- pipeline for alignment with sync
   u_common_pipeline : ENTITY common_lib.common_pipeline
   GENERIC MAP(
     g_pipeline  => c_crosslets_info_dly,

applications/lofar2/libraries/sdp/src/vhdl/sdp_pkg.vhd

@@ -172,7 +172,7 @@ PACKAGE sdp_pkg is
   CONSTANT c_sdp_reg_stat_enable_addr_w     :NATURAL  := 1;  
 
   -- XSUB
-  CONSTANT c_sdp_crosslets_index_w : NATURAL := ceil_log2(c_sdp_N_sub); --8
+  CONSTANT c_sdp_crosslets_index_w : NATURAL := ceil_log2(c_sdp_N_sub);
   CONSTANT c_sdp_mm_reg_crosslets_info : t_c_mem := (latency  => 1,
                                                      adr_w    => 4,
                                                      dat_w    => c_sdp_crosslets_index_w,  

applications/lofar2/libraries/sdp/tb/vhdl/tb_sdp_crosslets_subband_select.vhd

@@ -27,7 +27,10 @@
 -- * The tb is self stopping and self checking,tb_end will stop the simulation by
 --   stopping the clk and thus all toggling.
 --
--- Description: The tb verifies out_sosi and out_crosslets_info of the dut.
+-- Description: The tb starts the dut by writing a scheduled bsn to the bsn_scheduler
+-- via MM. The offsets and step are configured using MM. The dut makes the subband 
+-- selection based on the MM configuration and N_crosslets. The TB then verifies out_sosi 
+-- and out_crosslets_info of the dut by comparing it to the expected output.
 
 LIBRARY IEEE, common_lib, dp_lib;
 USE IEEE.std_logic_1164.ALL;

libraries/base/reorder/src/vhdl/reorder_col_select.vhd

@@ -200,7 +200,7 @@ BEGIN
   retrieve_sosi.im    <= RESIZE_DP_DSP_DATA(i_col_select_miso.rddata(c_nof_complex*g_dsp_data_w-1 DOWNTO g_dsp_data_w));
   retrieve_sosi.data  <= RESIZE_DP_DATA(i_col_select_miso.rddata(    c_nof_complex*g_dsp_data_w-1 DOWNTO 0));
   retrieve_sosi.valid <= i_col_select_miso.rdval;
-  retrieve_sosi.sop   <= retrieve_sop_dly(c_retrieve_lat) AND i_col_select_miso.rdval;
+  retrieve_sosi.sop   <= retrieve_sop_dly(c_retrieve_lat) AND i_col_select_miso.rdval; -- Only set sop/eop when valid.
   retrieve_sosi.eop   <= retrieve_eop_dly(c_retrieve_lat) AND i_col_select_miso.rdval;
   -- Page delay the input_sosi info (sync, BSN, channel at sop and err, empty at eop) and combine it with the retrieved SS data to get the output_sosi
   info_sop_wr_en <= input_sosi.sop & store_done;