siggen: Fix direction of rotation

The I and Q were swapped in sine_tone, which caused confusion and made
the rotation of REG_CARTESIAN clockwise by default. This effectively
made the resulting frequency negative. This PR makes the I and Q order
consistent with RFNoC and fixes the direction of rotation so that a
positive value for REG_PHASE_INC (phase increment) results in a
counter-clockwise rotation, which yields a positive frequency.

4 files changed, 44 insertions, 35 deletions

diff --git a/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_all_tb.sv b/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_all_tb.sv
index 0b23dcd0f..90a950668 100644
--- a/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_all_tb.sv
+++ b/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_all_tb.sv
@@ -21,7 +21,7 @@ module rfnoc_block_siggen_all_tb;
   rfnoc_block_siggen_tb #(.CHDR_W(64),  .NUM_PORTS(3))  test_siggen_2();
   rfnoc_block_siggen_tb #(.CHDR_W(128), .NUM_PORTS(2))  test_siggen_3();
   rfnoc_block_siggen_tb #(.CHDR_W(256), .NUM_PORTS(1))  test_siggen_4();
-  
+
 endmodule : rfnoc_block_siggen_all_tb
 
 
diff --git a/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_regs.vh b/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_regs.vh
index 10cda5425..a63a078a8 100644
--- a/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_regs.vh
+++ b/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_regs.vh
@@ -75,9 +75,9 @@ localparam REG_GAIN_LEN = 16;
 // real and imaginary components are treated as 16-bit signed fixed point
 // values with 15 fractional bits.
 //
-// [31:16] Real/I component
-// [15: 0] Imaginary/Q component
-//
+// [31:16] X/I/Real component
+// [15: 0] Y/Q/Imaginary component
+
 localparam REG_CONSTANT = 'h10;
 //
 localparam REG_CONSTANT_LEN = 32;
@@ -86,10 +86,11 @@ localparam REG_CONSTANT_LEN = 32;
 // REG_PHASE_INC (R/W)
 //
 // Sets the phase increment, in "scaled radians", for the sine waveform
-// generator. This is the amount by which REG_CARTESIAN is rotated each clock
-// cycle. In other words, it controls the rate of rotation, or the frequency,
-// of the sine wave. The range of the phase value is -1.0 to +1.0. In scaled
-// radians, the value range -1 to +1 corresponds to -Pi to Pi in radians.
+// generator. This is the amount by which REG_CARTESIAN is rotated
+// counter-clockwise each clock cycle. In other words, it controls the rate of
+// rotation, or the frequency, of the sine wave. The range of the phase value
+// is -1.0 to +1.0. In scaled radians, the value range -1 to +1 corresponds to
+// -Pi to Pi in radians.
 //
 // In other words, the normalized frequency (in cycles/sample) of the
 // sinusoidal output is equal to 0.5*REG_PHASE_INC.
@@ -123,11 +124,11 @@ localparam REG_PHASE_INC_LEN = 16;
 // fixed point with 2 integer and 14 fractional bits, which is accurate.
 // However, since we treat the output as sc16 (15 fractional bits), we need to
 // double the value of the CARTESIAN inputs to get the output we want for sc16.
-// This is mathematically inequivalent to simply saying the CARTESIAN inputs
-// have 15 fractional bits instead of 14.
+// This is mathematically equivalent to simply saying the CARTESIAN inputs have
+// 15 fractional bits instead of 14.
 //
-// [31:16] : Y (Imaginary) component
-// [15: 0] : X (Real) component
+// [31:16] : X/I/Real component
+// [15: 0] : Y/Q/Imaginary component
 //
 localparam REG_CARTESIAN = 'h18;
 //
diff --git a/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_tb.sv b/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_tb.sv
index 4cb701aaf..edc755d8e 100644
--- a/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_tb.sv
+++ b/fpga/usrp3/lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_tb.sv
@@ -54,7 +54,7 @@ module rfnoc_block_siggen_tb #(
 
   // Maximum real (floating point) values allowed for the different fixed
   // point formats (for range checking). All of the fixed point values are
-  // signed 16-bit. 
+  // signed 16-bit.
   localparam real MAX_GAIN_R  =  (2.0**15-1) / (2.0**GAIN_FRAC);
   localparam real MIN_GAIN_R  = -(2.0**15)   / (2.0**GAIN_FRAC);
   localparam real MAX_CONST_R =  (2.0**15-1) / (2.0**CONST_FRAC);
@@ -242,7 +242,9 @@ module rfnoc_block_siggen_tb #(
   endfunction : fixed_to_real
 
 
-  // Compute the next sine value we expect based on the previous
+  // Compute the next sine value we expect based on the previous. This should
+  // be a point (X,Y) rotated counter-clockwise around the origin, where X is
+  // in the MSBs and Y is in the LSBs.
   function automatic logic [31:0] next_sine_value(
     logic [31:0] sample,
     logic [15:0] phase_inc
@@ -252,10 +254,10 @@ module rfnoc_block_siggen_tb #(
     y        = fixed_to_real(sample[15: 0], CART_FRAC);
     phase    = fixed_to_real(phase_inc, PHASE_FRAC) * PI;
 
-    // Compute the rotated coordinates
-    new_x =  x*$cos(phase) + y*$sin(phase);
-    new_y = -x*$sin(phase) + y*$cos(phase);
-    
+    // Compute the counter-clockwise rotated coordinates
+    new_x = x*$cos(phase) - y*$sin(phase);
+    new_y = x*$sin(phase) + y*$cos(phase);
+
     return { real_to_fixed(new_x, CART_FRAC), real_to_fixed(new_y, CART_FRAC) };
   endfunction : next_sine_value
 
@@ -320,7 +322,7 @@ module rfnoc_block_siggen_tb #(
     end
 
     // Dump all the packets that were received
-    while (blk_ctrl.num_received(port)) begin 
+    while (blk_ctrl.num_received(port)) begin
       blk_ctrl.recv_items(port, items);
     end
   endtask : flush_output
@@ -376,7 +378,7 @@ module rfnoc_block_siggen_tb #(
     int                 spp         = SPP,
     logic signed [15:0] const_re    = 16'h7FFF,  // 0.99997
     logic signed [15:0] const_im    = 16'h7FFF,  // 0.99997
-    logic signed [15:0] phase_inc   = real_to_fixed(0.5, 13), //real_to_fixed(2.0/16, 13), // 2*pi/16 radians
+    logic signed [15:0] phase_inc   = real_to_fixed(2.0/16, 13), // 2*pi/16 radians
     logic signed [15:0] cart_x      = real_to_fixed(1.0, 14),
     logic signed [15:0] cart_y      = real_to_fixed(0.0, 14)
   );
@@ -387,7 +389,7 @@ module rfnoc_block_siggen_tb #(
       write_reg(port, REG_CONSTANT, {const_re, const_im});
     end else if (mode == WAVE_SINE) begin
       write_reg(port, REG_PHASE_INC, phase_inc);
-      write_reg(port, REG_CARTESIAN, {cart_y, cart_x});
+      write_reg(port, REG_CARTESIAN, {cart_x, cart_y});
     end
     write_reg(port, REG_ENABLE, 1);
 
@@ -400,7 +402,7 @@ module rfnoc_block_siggen_tb #(
 
       // Verify the length
       `ASSERT_ERROR(
-        items.size() == spp, 
+        items.size() == spp,
         "Packet length didn't match configured SPP"
       );
 
@@ -447,7 +449,7 @@ module rfnoc_block_siggen_tb #(
             );
           end
         end
-       
+
       end
     end
 
@@ -596,7 +598,7 @@ module rfnoc_block_siggen_tb #(
 
     max_val = 16'sh7FFF;
     min_val = 16'sh8000;
-    
+
     // Test max gain with min and max sample values
     run_waveform(.port(port), .mode(WAVE_CONST), .gain(max_val),
       .const_re(max_val), .const_im(min_val));
@@ -610,7 +612,7 @@ module rfnoc_block_siggen_tb #(
     run_waveform(
       .port(port),
       .mode(WAVE_CONST),
-      .const_re(real_to_fixed(0.5, CONST_FRAC)),  
+      .const_re(real_to_fixed(0.5, CONST_FRAC)),
       .const_im(real_to_fixed(0.25, CONST_FRAC)),
       .gain(real_to_fixed(0.5, GAIN_FRAC))
       );
diff --git a/fpga/usrp3/lib/rfnoc/sine_tone.v b/fpga/usrp3/lib/rfnoc/sine_tone.v
index a687472eb..f012214ff 100644
--- a/fpga/usrp3/lib/rfnoc/sine_tone.v
+++ b/fpga/usrp3/lib/rfnoc/sine_tone.v
@@ -11,19 +11,23 @@
 //   perform the rotate function in units of scaled radians. See the CORDIC IP
 //   Product Guide (PG105) for details.
 //
-//   The SR_PHASE_INC register controls the phase increment, in scaled
-//   radians, for the sine waveform generator. It is a 16-bit signed
-//   fixed-point phase value with 3 integer bits and 13 fractional bits. This
-//   is the amount by which REG_CARTESIAN is rotated each clock cycle. In
+//   This block outputs the X/I/real component in the most-significant bits and
+//   the Y/Q/imaginary component in the least-significant bits. This is
+//   opposite from the Xilinx IP but matches RFNoC.
+//
+//   The SR_PHASE_INC register controls the phase increment, in scaled radians,
+//   for the sine waveform generator. It is a 16-bit signed fixed-point phase
+//   value with 3 integer bits and 13 fractional bits. This is the amount by
+//   which REG_CARTESIAN is rotated counter-clockwise each clock cycle. In
 //   other words, it controls the rate of rotation, or the frequency, of the
 //   sine wave. In scaled radians, the phase value range -1 to +1 corresponds
 //   to -Pi to Pi in radians.
 //
 //   The SR_CARTESIAN register sets the sets the (X,Y) Cartesian coordinate
 //   that will be rotated to generate the sine output. Both X and Y are 16-bit
-//   signed fixed-point values with 2 integer bits and 14 fractional bits. Y
-//   is in the upper 16-bits and X is in the lower 16-bits.
-// 
+//   signed fixed-point values with 2 integer bits and 14 fractional bits.
+//   X/I/real is in the upper 16-bits and Y/Q/imaginary is in the lower 16-bits.
+//
 //   In addition to rotation, the SR_CARTESIAN input vector is also scaled by
 //   a "CORDIC scale factor" that equals about 1.1644 (that is, the product of
 //   sqrt(1 + 2^(-2i)) for i = 1 to n, where n = 14, the number of fractional
@@ -134,18 +138,20 @@ module sine_tone #(
     .o_tready (phase_out_tready & enable)
   );
 
-  // CORDIC
+  // CORDIC. Swap I and Q to match what the Xilinx IP expects.
   cordic_rotator cordic_inst (
     .aclk                    (clk),
     .aresetn                 (~(reset|clear)),
     .s_axis_phase_tdata      (phase_out_tdata),
     .s_axis_phase_tvalid     (phase_out_tvalid & cartesian_tvalid & enable),
     .s_axis_phase_tready     (phase_out_tready),
-    .s_axis_cartesian_tdata  (cartesian_tdata),
+    .s_axis_cartesian_tdata  ({cartesian_tdata[      0 +: WIDTH/2],   // Q
+                               cartesian_tdata[WIDTH/2 +: WIDTH/2]}), // I
     .s_axis_cartesian_tlast  (cartesian_tlast),
     .s_axis_cartesian_tvalid (phase_out_tvalid & cartesian_tvalid & enable),
     .s_axis_cartesian_tready (cartesian_tready),
-    .m_axis_dout_tdata       (sine_out_tdata),
+    .m_axis_dout_tdata       ({sine_out_tdata[      0 +: WIDTH/2],    // Q
+                               sine_out_tdata[WIDTH/2 +: WIDTH/2]}),  // I
     .m_axis_dout_tlast       (sine_out_tlast),
     .m_axis_dout_tvalid      (sine_out_tvalid),
     .m_axis_dout_tready      (sine_out_tready & enable)