Synchronous counter in VHDL with compare match and load

I created the following Counter with a compare match functionality:

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use ieee.numeric_std.all;

entity Counter is
    generic (
        N : natural := 24
    );

    port (
        -- Input counter clock
        clk                 : in std_logic := '0';
        -- Enable the counter
        enable              : in std_logic := '0';
        -- Preload value loaded when clk is rising and load is 1
        load_value          : in std_logic_vector((N-1) downto 0) := (others => '0');
        -- Set to 1 to load a value
        load                : in std_logic := '0';
        -- Compare match input is compared with the counter value
        compare_match_value : in std_logic_vector((N-1) downto 0) := (others => '0');
        -- Is 1 when compare_match_value = counter_value
        compare_match       : out std_logic := '0';
        output_value        : out std_logic_vector((N-1) downto 0) := (others => '0') 
    );
end Counter;

architecture Behavioral of Counter is
    signal counter_value      : unsigned((N - 1) downto 0) := to_unsigned(0, N);
begin
    output_value <= std_logic_vector(counter_value);

    process (clk) is 
    begin 
        if rising_edge(clk) then
            if enable = '1' then
                if load = '1' then
                    counter_value <= unsigned(load_value);
                else 
                    counter_value <= counter_value + 1;
                end if;
            else 
                if load = '1' then 
                    counter_value <= unsigned(load_value);
                end if;
            end if;
        end if;
    end process;

    process (counter_value) is
    begin 
        if unsigned(compare_match_value) = counter_value then 
            compare_match <= '1';
        else
            compare_match <= '0';
        end if;
    end process;
end Behavioral;

The behavior of my counter is to be fully synchronous with the input clk signal. Disabling the counter is always possible and the value is held at the current count value. A load value can be assigned with the load and load_value signal. Whenever the load signal is high and a rising edge is detected, the counter value is updated to the load_value.

Another feature is the compare unit which outputs high on compare_match output. The simulation works as expected but I have a few questions when synthesizing this design on spartan 3 fpga.

Is this considered a good design of my counter because I'm still not much experienced in VHDL.
Are there any undefined states when using the compare unit in further logic in my design? As I see it compare_match is calculated whenever the counter_value is updated.
When using a large number for N, is there anything special about the delay I need to consider?

Solution

In general it seems to me a quite good description. However, I would like to point out some minor things (that might me some answers to your 1st question).

1) As, I see right now your counter does not contain any reset (neither asynchronous nor synchronous). In general you cannot predict the starting point of your counting (even if, probably, it will be all '0's at start-up). In my opinion, it would be a neater design if you could have a reset signal. I also noticed that the loading is activated regardless of the fact that the counter is enabled or not. I have no comment about this since it could be a specification for your design. Maybe you can compact the code by moving the "if load" part outside of the "if enable" (i.e. changing the order to the comparisons). To improve the readability (especially when the designs will be more complex), I advise you to label the process. This will help you to identify the different part of the design. You can skip lot of the extra-typing if you use the VHDL-mode of emacs. It has built in templates that would take care of the "boring" part related to coding. I also see that you have default values for your input ports. In my opinion, this is not a very good practice; they would be ignored by synthesizer leading to an IP that might behave differently than what you expect. In general, do not make assumptions (a part those that are specified) on the external signals. Finally, I have a comment about the compare part. This goes for both question 1) & 2)

1-2) In the compare process, you have just listed counter_value in the sensitivity-list. This means that the process would be activated only when counter_value changes. Since you compare it with a signal (compare_match_value) that is an input to the block (hence it can change values) it would be better to have it too in the sensitivity-list. Otherwise, the comparison would not apply (i.e. the process would not be activated) when you change compare_match_value. Linting tools and synthesizer might complain about it (stating warning like incomplete sensitivity-list). As matter of fact it is good practice to list all the signals that may change in the list for combinatorial processes.

Regarding the comparison itself, the way you described it is absolutely fine and you would not have uncovered states. Basically you have specified all possible conditions so no surprises there.

3) Regarding your 3rd question, since you are targeting an FPGA, you could "relax" about it. FPGAs have dedicated structure for fast arithmetic operations and (as long as you do not use all of them) the synthesizer would use them to close time. Also in ASIC, synthesizer would probably select an appropriate arithmetic structure to close time. If you want to be on the safe side, you can add a register at the output of the comparison block. This will prevent creating a long combinatorial path especially if you IP has to be integrated with other blocks. Of course this extra-register would add an 1-clock-cycle latenc, but it will improve your overall timing.

I hope these suggestions could be useful to you and cover (at least partially) your doubts. Keep on coding :)