Is it bad design to have additional logic on your reset?

Question

I have always been told it is bad design to have anything but a reset in your reset clause. For instance, see the two circuits below:

---

process (CLK)
begin
  if rising_edge(CLK) then
    if (RST = '1') then
      Q0 <= '0';
    else
      if (CLR = '1') then
        Q0 <= '0';
      else
        Q0 <= D;
      end if;
    end if;
  end if;
end process;

---

process (CLK)
begin
  if rising_edge(CLK) then
    if (RST = '1' or CLR = '1') then
      Q1 <= '0';
    else
      Q1 <= D;
    end if;
  end if;
end process;

---

I've been told the first is more correct, but...

Ive tested them and they appear to be equivalent logically:

They appear equivalent in synthesis and implementation (in fact Vivado synthesizes them as the second case more accurately):

So where is the disconnect? Did older tools not synthesize this properly? Is it actually bad design to do the second case?

---

The accepted answer below made me wonder how it would look if the resets were asynchronous:

process (CLK, RST)
begin
  if (RST = '1') then
    Q0 <= '0';
  else
    if rising_edge(CLK) then
      if (CLR = '1') then
        Q0 <= '0';
      else
        Q0 <= D;
      end if;
    end if;
  end if;
end process;

process (CLK, RST, CLR)
begin
  if (RST = '1' or CLR = '1') then
    Q1 <= '0';
  else
    if rising_edge(CLK) then
      Q1 <= D;
    end if;
  end if;
end process;

Synthesis results are very different for the asynchronous case. This makes more sense now from a timing perspective as now you'd have an asynchronous signal running around, thank you.

---

---

---

Minimum reproducible example (synchronous case):

top.vhd

library IEEE;
use IEEE.std_logic_1164.all;

entity top is
    port (
    CLK : in std_logic;
    RST : in std_logic;
    CLR : in std_logic;
    D   : in std_logic;
    Q0  : out std_logic;
    Q1  : out std_logic
    );
end top;

architecture rtl of top is
begin

  process (CLK)
  begin
    if rising_edge(CLK) then
      if (RST = '1') then
        Q0 <= '0';
      else
        if (CLR = '1') then
          Q0 <= '0';
        else
          Q0 <= D;
        end if;
      end if;
    end if;
  end process;
        
  process (CLK)
  begin
    if rising_edge(CLK) then
      if (RST = '1' or CLR = '1') then
        Q1 <= '0';
      else
        Q1 <= D;
      end if;
    end if;
  end process;
      
end architecture rtl;

tb.vhd

library IEEE;
use IEEE.std_logic_1164.all;

library std;
use std.env.all;

entity tb is
end entity tb;

architecture behav of tb is
  constant CLK_FREQ           : real             := 100.0e6;
  constant CLK_HALF_P         : time             := (((1.0/CLK_FREQ)*10.0e8)/2.0) * 1 ns;
  signal   clk                : std_logic;
  signal   rst                : std_logic;
  signal   clr                : std_logic;
  signal   d                  : std_logic;
  signal   q0                 : std_logic;
  signal   q1                 : std_logic;
begin

  dut : entity work.top(rtl)
  port map (
    CLK => clk,
    RST => rst,
    CLR => clr,
    D   => d,
    Q0  => q0,
    Q1  => q1
  );

  sysClkProc : process ---------------------------------------------------------
    begin
      clk <= '1';
      wait for CLK_HALF_P;
      clk <= '0';
      wait for CLK_HALF_P;
  end process sysClkProc; ------------------------------------------------------

  stimulusProc : process -------------------------------------------------------
    begin
      report ("Starting Simulation");
      rst <= '1';
      d   <= '0';
      clr <= '0';

      wait for 100 ns;

      rst <= '0';

      for i in 1 to 10 loop
        wait until rising_edge(clk);
      end loop;

      d <= '1';

      for i in 1 to 10 loop
        wait until rising_edge(clk);
      end loop;

      d <= '0';

      for i in 1 to 10 loop
        wait until rising_edge(clk);
      end loop;

      d <= '1';

      for i in 1 to 5 loop
        wait until rising_edge(clk);
      end loop;

      clr <= '1';

      for i in 1 to 5 loop
        wait until rising_edge(clk);
      end loop;

      clr <= '0';

      for i in 1 to 5 loop
        wait until rising_edge(clk);
      end loop;

      d <= '0';

      wait for 100 ns;
      finish(0);
  end process stimulusProc; ----------------------------------------------------

end architecture behav;

constr.xdc - Target Nexys A7-100T (xc7a100tcsg324-1)

create_clock -period 10.000 -name sys_clock [get_ports CLK]

set_property -dict {PACKAGE_PIN J15 IOSTANDARD LVCMOS18} [get_ports CLK]
set_property -dict {PACKAGE_PIN J15 IOSTANDARD LVCMOS18} [get_ports RST]
set_property -dict {PACKAGE_PIN L16 IOSTANDARD LVCMOS18} [get_ports CLR]
set_property -dict {PACKAGE_PIN M13 IOSTANDARD LVCMOS18} [get_ports D  ]
set_property -dict {PACKAGE_PIN H17 IOSTANDARD LVCMOS18} [get_ports Q0 ]
set_property -dict {PACKAGE_PIN K15 IOSTANDARD LVCMOS18} [get_ports Q1 ]

Answer 1

There are 2 different concepts: Asynchronous reset and synchronous reset. You are working with synchronous reset (getting active only at clock edges), which does not have any restrictions. So both versions of your design are ok, I would chose the first solution with "or", because it is easier to read. At asynchronous reset you do not have so much freedom, at many places in the design flow (static timing analysis, synthesis, layout) the tools do not like logic in the reset path, because it makes the situation more complicated. But of course, if logic in the asynchronous is really needed, you can implement it but you must live with more work effort.