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.


Original-commit: 4e6531f30648ede5be8f93fa49fdcd4973b73813

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
+// [31:16] X/I/Real component
-// [15: 0] Imaginary/Q 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
+// generator. This is the amount by which REG_CARTESIAN is rotated
-// cycle. In other words, it controls the rate of rotation, or the frequency,
+// counter-clockwise each clock cycle. In other words, it controls the rate of
-// of the sine wave. The range of the phase value is -1.0 to +1.0. In scaled
+// rotation, or the frequency, of the sine wave. The range of the phase value
-// radians, the value range -1 to +1 corresponds to -Pi to Pi in radians.
+// 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
+// This is mathematically equivalent to simply saying the CARTESIAN inputs have
-// have 15 fractional bits instead of 14.
+// 15 fractional bits instead of 14.
 //
-// [31:16] : Y (Imaginary) component
+// [31:16] : X/I/Real component
-// [15: 0] : X (Real) component
+// [15: 0] : Y/Q/Imaginary component
 //
 localparam REG_CARTESIAN = 'h18;
 //

lib/rfnoc/blocks/rfnoc_block_siggen/rfnoc_block_siggen_tb.sv

@@ -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,9 +254,9 @@ 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
+    // Compute the counter-clockwise rotated coordinates
-    new_x =  x*$cos(phase) + y*$sin(phase);
+    new_x = x*$cos(phase) - y*$sin(phase);
-    new_y = -x*$sin(phase) + y*$cos(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
@@ -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);
 

lib/rfnoc/sine_tone.v

@@ -11,18 +11,22 @@
 //   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
+//   This block outputs the X/I/real component in the most-significant bits and
-//   radians, for the sine waveform generator. It is a 16-bit signed
+//   the Y/Q/imaginary component in the least-significant bits. This is
-//   fixed-point phase value with 3 integer bits and 13 fractional bits. This
+//   opposite from the Xilinx IP but matches RFNoC.
-//   is the amount by which REG_CARTESIAN is rotated each clock cycle. In
+//
+//   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
+//   signed fixed-point values with 2 integer bits and 14 fractional bits.
-//   is in the upper 16-bits and X is in the lower 16-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
@@ -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)