Variable clock divider

[kpow8050]

Hi,

I am having trouble getting the following verilog HDL code to compile. My intent is to make a variable frequency divider. The way it works is you give it a divider number and and clock source, using an internal counter it will then count up until this value matches the divider number and change state of the output clock. The counter is then reset. However the compiler is not happy with my assignment of "assign count = count + 1;". Is this not allowed in HDL? How could I keep track of the count? Thanks

module clock_divider(input wire[12:0] divider, input clk, output reg clk_out);
   // This module will divide an input clock signal by the amount in divider (16 bit register)
   // For finer control of clock divisions change the divider register size
   
   // Internal counter memory
   reg [12:0] count;
   
   always @(posedge clk)
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count = 0;
      end
     
      assign count = count + 1;
     
endmodule

[ataradov]

It would be something like this:

Code: [Select]

   always @(posedge clk) begin
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count <= 0;
      end else
        count <= count + 1;
end


[fourfathom]

How about this:

 always @(posedge clk) begin
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count <= 0;
      end
     
      else count <= count + 1;
 end

Or like this:
always @(posedge clk) begin
      count <= count + 1;
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count <= 0;
      end
end

[BrianHG]

How about this:

 always @(posedge clk) begin
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count <= 0;
      end
     
      else count <= count + 1;
 end

Or like this:
always @(posedge clk) begin
      count <= count + 1;
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count <= 0;
      end
end

Some compilers and simulators will allow you to get away with simultaneously assigning 2 different results to 'count', but this is considered bad practice.  Your first solution and ataradov did it the right way with the 'else'.

[ataradov]

I think all tools should accept this. The last assignment wins.

In this case 'else' is better for clarity, but the 'default' value is often necessary in big state machines with a lot of cases and conditions inside them. I use that all the time and never seen any tool complain.

This is a very common practice when evaluating the next state, for example. In most cases you want to stay in the current state, and if you have to do the same assignment to the current state in all the blocks, you will go mad and likely miss something.

[fourfathom]

Some compilers and simulators will allow you to get away with simultaneously assigning 2 different results to 'count', but this is considered bad practice.  Your first solution and ataradov did it the right way with the 'else'.

Thanks.  I've seen it done, and probably have done it myself, but I usually do it the "correct" way.  Now I will make a point of it.  The shortcut method can save a few lines of code, but to me it feels unnecessarily obscure.

[and then this shows up]

I think all tools should accept this. The last assignment wins.

In this case 'else' is better for clarity, but the 'default' value is often necessary in big state machines with a lot of cases and conditions inside them. I use that all the time and never seen any tool complain.

Yes, complicated state machines  is where I've seen / used that form.  Since I'm doing this for fun these days, perhaps I will remain flexible.

[kpow8050]

Thanks guys. That has fixed the problem. Now I am faced with a new slightly unrelated problem. When I try to pass the divider to this module as a register [12:0] it is giving the following compile error "Error (10133): Verilog HDL Expression error at LaserControl.v(51): illegal part select of unpacked array "pixel_clk_divider_reg". I have been able to pass registers this way before, I'm confused as why it won't work in this instance. The relevant code is shown below


  // Pixel clock divider
   // This sets the pixel clock rate (how many pixels/per sec)
   reg pixel_clk_divider_reg[12:0];
   wire pixel_clk_src;
   wire pixel_clk_out;
   clock_divider pixel_clk_divider (   .divider (pixel_clk_divider_reg[12:0]),
                        .clk (pixel_clk_src),
                        .clkout (pixel_clk_out));

[BrianHG]

Thanks guys. That has fixed the problem. Now I am faced with a new slightly unrelated problem. When I try to pass the divider to this module as a register [12:0] it is giving the following compile error "Error (10133): Verilog HDL Expression error at LaserControl.v(51): illegal part select of unpacked array "pixel_clk_divider_reg". I have been able to pass registers this way before, I'm confused as why it won't work in this instance. The relevant code is shown below


  // Pixel clock divider
   // This sets the pixel clock rate (how many pixels/per sec)
   reg pixel_clk_divider_reg[12:0];
   wire pixel_clk_src;
   wire pixel_clk_out;
   clock_divider pixel_clk_divider (   .divider (pixel_clk_divider_reg[12:0]),
                        .clk (pixel_clk_src),
                        .clkout (pixel_clk_out));

   reg [12:0] pixel_clk_divider_reg;

What you did was make a 1 bit array reg 'pixel_clk_divider_reg' with 13 addresses.

[asmi]

Some compilers and simulators will allow you to get away with simultaneously assigning 2 different results to 'count'

And those which won't, need to be thrown away because they are not compliant with the standard.

this is considered bad practice.

By whom? Verilog specification explicitly defines how this should behave (last assignment wins) so it is fully compliant and supported. I myself use it all the time for "default" assignments to help avoid latches.

[BrianHG]

Not logically closing the bounds around your code can lead to uncleanly practices even if they are fully functional.

[asmi]

Not logically closing the bounds around your code can lead to uncleanly practices even if they are fully functional.

I strongly disagree. Using default assignment massively improve code readability because there is less noise in the code, especially in larger modules. And since code is read MUCH more often than it's written, any effort to make code easier to read and understand will be well rewarded later on. This is especially so for HDL, because many people have noted (myself included) that it's harder to read and understand someone else's code than in conventional programming languages. I suspect it's because "normal" programming languages tend to operate using higher level abstractions that HDL, even SystemVerilog seriously lags behind in this department, not to mention older languages. That's why there are so many attempts to devise higher-level language which would "compile" into lower level HDL like Verilog, using Python, Java, whatever else.

Also it leads to less code, and it's a well established by many studies fact that less code means less bugs.

[BrianHG]

That is a lack of coding design quality.  The ''default'' should be evident, not something you rely on the compiler to fabricate for you.  There should not be any addition of lines of code.  It should not generate more signalling noise in your design.

[kpow8050]

Thanks guys. That has fixed the problem. Now I am faced with a new slightly unrelated problem. When I try to pass the divider to this module as a register [12:0] it is giving the following compile error "Error (10133): Verilog HDL Expression error at LaserControl.v(51): illegal part select of unpacked array "pixel_clk_divider_reg". I have been able to pass registers this way before, I'm confused as why it won't work in this instance. The relevant code is shown below


  // Pixel clock divider
   // This sets the pixel clock rate (how many pixels/per sec)
   reg pixel_clk_divider_reg[12:0];
   wire pixel_clk_src;
   wire pixel_clk_out;
   clock_divider pixel_clk_divider (   .divider (pixel_clk_divider_reg[12:0]),
                        .clk (pixel_clk_src),
                        .clkout (pixel_clk_out));

   reg [12:0] pixel_clk_divider_reg;

What you did was make a 1 bit array reg 'pixel_clk_divider_reg' with 13 addresses.

Thanks BrianHG. One more thing, when I have a register declared within a module, is that local in scope or global for the whole project? The desired behavior I want is for the registers I declare within a module (outside of the always @ section) to never overwrite each other say in instances where I instantiate the same module multiple times. Thanks

[fourfathom]

The ''default'' should be evident, not something you rely on the compiler to fabricate for you. 

I don't think the issue at hand has anything to do with "something the complier fabricates for you".  In either case the default is clearly defined -- through the use of "else" in one case, and through the "last assignment" rule in the other.  In its favor, the "last assignment" lets you avoid having multiple "else"s scattered throughout a complicated state machine or similar code.

[fourfathom]

Thanks BrianHG. One more thing, when I have a register declared within a module, is that local in scope or global for the whole project? The desired behavior I want is for the registers I declare within a module (outside of the always @ section) to never overwrite each other say in instances where I instantiate the same module multiple times. Thanks

It's local.  You can instantiate multiple instances of a module, or have many different modules using a register with a common name (such as your "reg [12:0] count;"), and there will be no conflict.

[kpow8050]

Great, that's working how I expected.

[asmi]

That is a lack of coding design quality. 

You got this backwards. Defaults are always super-evident because they are always on top of the state machine. If not using them, you will have to hunt though all states and state transitions to figure out what the value is supposed to be. And the larger and more complex state machine is, the harger it will be to understand what the code is doing what the state of each variable is supposed to be.
Lack of coding quality is repeating the same statement over and over again in all branches and states.

[kpow8050]

I have decided to add a reset function to the variable clock divider, however i am unsure of the best way to implement it. The way I am thinking is to have it reset on the falling edge of the clock, since the rising edge is already being used to set the state of the output, if I can use the falling edge to reset the output if reset is high then I can do it synchronously. I have added another always section to the code as shown below, however it won't compile. The error "Error (10170): Verilog HDL syntax error at clock_divider.v(20) near text "endmodule";  expecting ";"". Is it not possible to have more than one always in a module? How should I write this? Sorry for all the questions

Code: [Select]

   // This module will divide an input clock signal by the amount in divider (16 bit register)
   // For finer control of clock divisions change the divider register size
   
   // Internal counter memory
   reg [12:0] count;
   
   always @(posedge clk)
      if (count >= divider) begin
         clk_out <= ~clk_out;
         count = 0;
      end else   
         count <= count + 1;
   
   always @(negedge clk)
      if (reset)
         clk_out <= 0;
      else
   
endmodule



[ataradov]

You are missing the statement after 'else'. But also, you can't assign the same register from two different always blocks.

Code: [Select]

   always @(posedge clk) begin
      if (reset)
         clk_out <= 0;
      else if (count >= divider) begin
         clk_out <= ~clk_out;
         count <= 0;
      end else   
         count <= count + 1;
   end

Also, pay attention to "<=" and "=". They are not the same and mean entirely different thing.

[asmi]

It's also a bad idea to use both edges in FPGA designs, as it effectively halves allowed combinatorial delays. Don't do that.

[ataradov]

Oh, yes, I "automatically" missed negedge. This is something you should not do for sure.

[kpow8050]

It's also a bad idea to use both edges in FPGA designs, as it effectively halves allowed combinatorial delays. Don't do that.

Ahh, I see. I will make sure not to do that in future designs. Was thinking it would save an extra clock pulse. But I guess if timing is that critical I can just up the clock frequency

[Bassman59]

That is a lack of coding design quality. 

You got this backwards. Defaults are always super-evident because they are always on top of the state machine. If not using them, you will have to hunt though all states and state transitions to figure out what the value is supposed to be. And the larger and more complex state machine is, the harger it will be to understand what the code is doing what the state of each variable is supposed to be.
Lack of coding quality is repeating the same statement over and over again in all branches and states.

Asmi is exactly right here. The idea that you set a default assignment at the top of a process and then override it below (like in a state machine) is well understood and is actually (in my humble opinion) less prone to errors of the sort of "I need to set the signal in state X and then clear it in the next state ... what if I add another state in between?" (Yes, yes, yes, you are supposed to re-run your verification after making this change.) And yes, this applies to both VHDL and Verilog.
 
I like to use the term "one-shot" for this sort of thing.

What's cool is that you can implement a state-duration timer with this kind of construct. Rather than having a decrementing counter in each state, have the counter outside of the machine decoder, decrementing to zero when it stops. Assign the "duration" value to the timer in one state, and in the next state, wait for that timer to go to zero before continuing. Something like this. Here, we'll strobe enable for one clock, and then wait for some number of clocks before continuing.

Code: [Select]

MyTimedStates : process (clk) is
begin
    if rising_edge(clk) then
        strobe <= '0'; -- clear previously-set one-shot strobe
       
        DurationTimer : if timer > 0
            timer <= timer - 1;
        end if DurationTimer;

        MyDecoder : case (foostate) is
            when S_FOO =>
                strobe <= '1'; -- strobe for one clock
                -- start the timer. Use a signal instead of constant and you
                -- have a run-time variable timer.
                timer  <= 100;   
                foostate <= S_BAR;

            when S_BAR =>
                -- Wait for timer to expire, then we can strobe again.
                WaitForDuration : if timer = 0 then
                    strobe <= '1';
                    timer <= 200; -- wait longer this time.
                    foostate <= S_BLETCH;
                end if WaitForDuration;
   
            when S_BLETCH =>
                WaitAgain : if timer = 0 then
                    -- we're done, go to beginning
                    foostate <= S_FOO;
                end if WaitAgain;           
        end case MyDecoder;
    end if;
end process MyTimedStates;

This makes a whole lot more sense than having a decrementing counter in each state for the timing.