Clock frequency meter module

Question

The module measures input clocks. It requires some reference clock. There can be from one to five input clocks to measure it. Output values are usual unsigned ones. As expected, it should be reset before starting. If it is used in Vivado, the user can choose the number of ports to show, depending on the number of input clocks to measure.

Example for Vivado block diagram:

enter image description here

Accuracy for 125MHz, 150MHz, 50MHz, 100MHz:

enter image description here

Top module:

 `timescale 1ns / 1ps
module clock_meter_top #
(
 parameter integer CLK_MHZ = 50,
 parameter integer CLK_NUM = 1,
 
 localparam integer MSR_CLK_VAL_WIDTH = 32
)
(
 input logic clk,
 input logic a_rst_n,
 
 input logic msr_clk_0,
 input logic msr_clk_1,
 input logic msr_clk_2,
 input logic msr_clk_3,
 input logic msr_clk_4,
 
 output logic [MSR_CLK_VAL_WIDTH - 1 : 0 ] msr_clk_val_0,
 output logic [MSR_CLK_VAL_WIDTH - 1 : 0 ] msr_clk_val_1,
 output logic [MSR_CLK_VAL_WIDTH - 1 : 0 ] msr_clk_val_2,
 output logic [MSR_CLK_VAL_WIDTH - 1 : 0 ] msr_clk_val_3,
 output logic [MSR_CLK_VAL_WIDTH - 1 : 0 ] msr_clk_val_4
);
 logic msr_clk [CLK_NUM - 1: 0];
 logic [MSR_CLK_VAL_WIDTH - 1 : 0 ] msr_clk_val [CLK_NUM - 1: 0];
 genvar i;
 
 generate
 always_comb
 begin 
 msr_clk[0] = msr_clk_0;
 msr_clk_val_0 = msr_clk_val[0];
 end
 
 if (CLK_NUM > 1)
 begin
 always_comb
 begin
 msr_clk[1] = msr_clk_1;
 msr_clk_val_1 = msr_clk_val[1];
 end
 end
 
 if (CLK_NUM > 2)
 begin
 always_comb
 begin
 msr_clk[2] = msr_clk_2;
 msr_clk_val_2 = msr_clk_val[2];
 end
 end
 
 if (CLK_NUM > 3)
 begin
 always_comb
 begin
 msr_clk[3] = msr_clk_3;
 msr_clk_val_3 = msr_clk_val[3];
 end
 end
 
 if (CLK_NUM > 4)
 begin
 always_comb
 begin
 msr_clk[4] = msr_clk_4;
 msr_clk_val_4 = msr_clk_val[4];
 end
 end
 for (i = 0; i < CLK_NUM; i = i + 1)
 begin : gen_clock_meters
 clock_meter #
 (
 .CLK_MHZ (CLK_MHZ)
 )
 clock_meter_inst
 (
 .clk_i (clk ),
 .a_rst_n_i (a_rst_n ),
 .msr_clk_i (msr_clk[i] ),
 
 .msr_clk_val_o (msr_clk_val[i]) 
 );
 end
 endgenerate
endmodule

Module:

 `timescale 1ns / 1ps
module clock_meter #
(
 parameter integer CLK_MHZ = 100,
 
 localparam integer MSR_CLK_VAL_WIDTH = 32
)
(
 input logic clk_i,
 input logic a_rst_n_i,
 
 input logic msr_clk_i,
 
 output logic [MSR_CLK_VAL_WIDTH - 1 : 0] msr_clk_val_o
);
 
 
 localparam integer REF_CNT_NUM = 10000000;
 localparam integer REF_CNT_WIDTH = $clog2(REF_CNT_NUM);
 
 localparam integer DECIM_CLK_MHZ = CLK_MHZ / 10;
 
 
 logic [REF_CNT_WIDTH - 1 : 0] ref_cnt;
 
 logic [MSR_CLK_VAL_WIDTH - 1 : 0] msr_cnt;
 logic [MSR_CLK_VAL_WIDTH - 1 : 0] msr_clk_val;
 logic msr_cnt_rst;
 logic [MSR_CLK_VAL_WIDTH - 1 : 0] gray_msr_cnt;
 
 logic [MSR_CLK_VAL_WIDTH - 1 : 0] res_msr_cnt;
 
 
 integer i;
 
 always_ff @(posedge clk_i)
 begin
 if (a_rst_n_i == 1'b0)
 begin
 ref_cnt <= 'h0;
 end
 else
 begin
 if (ref_cnt == (REF_CNT_NUM - 1))
 begin
 ref_cnt <= 'h0;
 end
 else
 begin
 ref_cnt <= ref_cnt + 1'b1;
 end
 end
 end
 
 
 always_ff @(posedge clk_i)
 begin
 if (a_rst_n_i == 1'b0)
 begin
 msr_cnt_rst <= 'h0;
 end
 else
 begin
 if (ref_cnt == (REF_CNT_NUM - 1))
 begin
 msr_cnt_rst <= 'h1;
 end
 else
 begin
 msr_cnt_rst <= 1'b0;
 end
 end
 end
 
 always_ff @(posedge msr_clk_i)
 begin
 if (a_rst_n_i == 1'b0)
 begin
 gray_msr_cnt <= 'h0;
 msr_cnt <= 'h0;
 end
 else
 begin
 if (msr_cnt_rst == 1'b1)
 begin
 msr_cnt <= 'h0;
 gray_msr_cnt <= 'h0;
 end
 else
 begin
 msr_cnt <= msr_cnt + 1'b1;
 
 for(i = 0; i < MSR_CLK_VAL_WIDTH - 1; i = i + 1)
 begin
 gray_msr_cnt[i] <= msr_cnt[i+1] ^ msr_cnt[i];
 end
 
 gray_msr_cnt[MSR_CLK_VAL_WIDTH - 1] <= msr_cnt[MSR_CLK_VAL_WIDTH - 1];
 end
 end
 end
 
 always_ff @(posedge clk_i)
 begin
 if (a_rst_n_i == 1'b0)
 begin
 res_msr_cnt <= 'h0;
 end
 else if (ref_cnt == (REF_CNT_NUM - 1))
 begin
 for(i = 0; i < MSR_CLK_VAL_WIDTH; i = i + 1)
 res_msr_cnt[i] <= ^(gray_msr_cnt >> i);
 end
 end
 
 always_ff @(posedge clk_i)
 begin
 if (a_rst_n_i == 1'b0)
 begin
 msr_clk_val <= 'h0;
 end
 else
 begin
 msr_clk_val <= DECIM_CLK_MHZ * res_msr_cnt;
 end
 end
 
 always_comb
 begin
 msr_clk_val_o = msr_clk_val;
 end
endmodule

TB:

`timescale 1ps / 1ps
module clock_meter_tb;
 localparam integer MSR_CLK_VAL_WIDTH = 32;
 
 localparam integer CLOCK_100_00_PS = 10000; //ps == 10MHz
 localparam integer CLOCK_184_32_PS = 5425; //ps == 184.32Hz
 localparam integer CLOCK_368_64_PS = 2712; //ps == 368.64MHz
 
 localparam integer TEST_ITER_NUM = 1000000000;
 reg clk_100_00 = 'b0;
 reg a_rst_n = 'b0;
 reg msr_clk_184_32 = 'b0;
 reg msr_clk_368_64 = 'b0;
 
 wire [MSR_CLK_VAL_WIDTH - 1 : 0] msr_clk_val_184_32;
 wire [MSR_CLK_VAL_WIDTH - 1 : 0] msr_clk_val_368_64;
 clock_meter #
 (
 .CLK_MHZ (100)
 )
 clock_meter_dut_184_32
 (
 .clk_i (clk_100_00 ),
 .a_rst_n_i (a_rst_n ),
 
 .msr_clk_i (msr_clk_184_32 ),
 
 .msr_clk_val_o (msr_clk_val_184_32)
 );
 
 clock_meter #
 (
 .CLK_MHZ (100)
 )
 clock_meter_dut_368_64
 (
 .clk_i (clk_100_00 ),
 .a_rst_n_i (a_rst_n ),
 
 .msr_clk_i (msr_clk_368_64 ),
 
 .msr_clk_val_o (msr_clk_val_368_64)
 );
 
 always
 begin
 #(CLOCK_100_00_PS / 2) clk_100_00 = !clk_100_00;
 end
 
 always
 begin
 #(CLOCK_184_32_PS / 2) msr_clk_184_32 = !msr_clk_184_32;
 end
 
 always
 begin
 #(CLOCK_368_64_PS / 2) msr_clk_368_64 = !msr_clk_368_64;
 end
 
 initial
 begin
 a_rst_n <= 'b0;
 @(posedge clk_100_00);
 
 a_rst_n <= 'b1;
 @(posedge clk_100_00);
 
 repeat(TEST_ITER_NUM)
 begin
 @(posedge clk_100_00);
 end
 
 $stop();
 
 
 end
endmodule

Answer 1

Generally speaking, the code has consistent layout, is easy to understand, uses descriptive variable names and makes good use of parameters for constant values.

You have a good separation between sequential logic and combinational logic, and you take advantage of the additional automatic code check offered by the SystemVerilog always_ff and always_comb keywords. These keywords also convey design intent well.

However, I get compile errors with 2 different simulators due to the integer i declaration in the clock_meter module. Perhaps your simulator is more forgiving. Regardless, to make your code more portable, I recommend declaring the iterator variable locally within each for loop.

You can simplify the for loop iterator using the increment operator, ++. Change all:

i = i + 1

to

i++

As an example, for loop, change:

 for(i = 0; i < MSR_CLK_VAL_WIDTH - 1; i = i + 1)

to:

 for (int i = 0; i < (MSR_CLK_VAL_WIDTH - 1); i++)

I also added an extra set of parentheses to avoid potential operator precedence issues and to make the code easier to read.

Use underscores to make large integer values easier to read:

 localparam integer TEST_ITER_NUM = 1000000000;

to:

 localparam integer TEST_ITER_NUM = 1_000_000_000;

The `timescale compiler directive is not needed for the clock_meter module since there are no delays specified. Also, it can be confusing if you use multiple different time units: 1ps vs. 1ns. It is better to stick to one.

In the testbench (clock_meter_tb), consider replacing the reg types (4-state) with bit types (2-state) for driving the DUT input ports. It is rare to drive the inputs with x or z, and 2-state has simulation performance benefits. bit types default to 0, so there is no need to initialize them (but this is a preference)

 bit clk_100_00 ;
 bit a_rst_n ;
 bit msr_clk_184_32;
 bit msr_clk_368_64;

• Artem Shimko: Thanks toolic for your code review.