🐛 fix atomic write/clear/set accesses of clear-only CSR bits (#829)

stnolting · Feb 24, 2024 · bf246fe · bf246fe
2 parents d5a7ab2 + 6a9bfcb
commit bf246fe
Show file tree

Hide file tree

Showing 9 changed files with 116 additions and 56 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,7 @@ mimpid = 0x01040312 -> Version 01.04.03.12 -> v1.4.3.12
 
 | Date | Version | Comment | Link |
 |:----:|:-------:|:--------|:----:|
+| 23.02.2024 | 1.9.5.9 | :bug: fix atomic write/clear/set accesses of clear-only CSR bits (re-fix of v1.9.5.6) | [#829](https://github.com/stnolting/neorv32/pull/829) |
 | 23.02.2024 | 1.9.5.8 | optimize FIFO component to improve technology mapping (inferring blockRAM for "async read" configuration); :bug: fix SLINK status flag delay | [#828](https://github.com/stnolting/neorv32/pull/828) |
 | 23.02.2024 | 1.9.5.7 | fix FIFO synthesis issue (Vivado cannot infer block RAM nor LUT-RAM) | [#827](https://github.com/stnolting/neorv32/pull/827) |
 | 20.02.2024 | 1.9.5.6 | :bug: fix bug in `mip.firq` CSR access; `mip.firq` bits are now read-write - software can trigger FIRQs by writing `1` to the according CSR bit | [#821](https://github.com/stnolting/neorv32/pull/821) |

diff --git a/docs/datasheet/cpu.adoc b/docs/datasheet/cpu.adoc
@@ -892,6 +892,9 @@ As a custom extension, the NEORV32 CPU features 16 fast interrupt request (FIRQ)
 entity signals. These interrupts have custom configuration and status flags in the <<_mie>> and <<_mip>> CSRs and also
 provide custom trap codes in <<_mcause>>. These FIRQs are reserved for NEORV32 processor-internal usage only.
 
+Once triggered, the according FIRQ bits in <<_mip>> has to be cleared _manually_ by the FIRQ interrupt handler
+to clear/acknowledge the fast interrupt request.
+
 
 :sectnums:
 ===== NEORV32 Trap Listing

diff --git a/docs/datasheet/cpu_csr.adoc b/docs/datasheet/cpu_csr.adoc
@@ -443,17 +443,14 @@ However, any write-access will be ignored and will not cause an exception to mai
 | 3     | `CSR_MIP_MSIP`                       | r/- | **MSIP**: Machine _software_ interrupt pending; _cleared by platform-defined mechanism_
 | 7     | `CSR_MIP_MTIP`                       | r/- | **MTIP**: Machine _timer_ interrupt pending; _cleared by platform-defined mechanism_
 | 11    | `CSR_MIP_MEIP`                       | r/- | **MEIP**: Machine _external_ interrupt pending; _cleared by platform-defined mechanism_
-| 31:16 | `CSR_MIP_FIRQ15P` : `CSR_MIP_FIRQ0P` | r/w | **FIRQxP**: Fast interrupt channel 15..0 pending; has to be cleared manually by writing zero
+| 31:16 | `CSR_MIP_FIRQ15P` : `CSR_MIP_FIRQ0P` | r/c | **FIRQxP**: Fast interrupt channel 15..0 pending; has to be cleared manually by writing zero; writing `1` has no effect
 |=======================
 
 .FIRQ Channel Mapping
 [TIP]
 See section <<_neorv32_specific_fast_interrupt_requests>> for the mapping of the FIRQ channels and the according
 interrupt-triggering processor module.
 
-[NOTE]
-The FIRQ channels can be triggered manually by software by writing `1` to the according `mip` bit.
-
 
 {empty} +
 [discrete]

diff --git a/docs/datasheet/on_chip_debugger.adoc b/docs/datasheet/on_chip_debugger.adoc
@@ -730,7 +730,7 @@ if it uses the trigger module for implementing a "hardware breakpoint"
 | 25    | `hit1`        | r/- | `0` - hardwired to zero, only `hit0` is used
 | 24    | `vs`          | r/- | `0` - VS-mode not supported
 | 23    | `vu`          | r/- | `0` - VU-mode not supported
-| 22    | `hit0`        | r/c | set when trigger has fired (**BEFORE** executing the triggering address); must be explicitly cleared by writing zero
+| 22    | `hit0`        | r/c | set when trigger has fired (**BEFORE** executing the triggering address); must be explicitly cleared by writing zero; writing 1 has no effect
 | 21    | `select`      | r/- | `0` - only address matching is supported
 | 20:19 | reserved      | r/- | `00` - hardwired to zero
 | 18:16 | `size`        | r/- | `000` -  match accesses of any size

diff --git a/rtl/core/neorv32_cpu_control.vhd b/rtl/core/neorv32_cpu_control.vhd
@@ -241,9 +241,11 @@ architecture neorv32_cpu_control_rtl of neorv32_cpu_control is
   type csr_t is record
     addr           : std_ulogic_vector(11 downto 0); -- csr address
     raddr          : std_ulogic_vector(11 downto 0); -- simplified csr read address
+    cmd            : std_ulogic_vector(2 downto 0); -- CSR access command
     we, we_nxt     : std_ulogic; -- csr write enable
     re, re_nxt     : std_ulogic; -- csr read enable
     wdata, rdata   : std_ulogic_vector(XLEN-1 downto 0); -- csr write/read data
+    wmask          : std_ulogic_vector(XLEN-1 downto 0); -- csr write data (mask)
     --
     mstatus_mie    : std_ulogic; -- machine-mode IRQ enable
     mstatus_mpie   : std_ulogic; -- previous machine-mode IRQ enable
@@ -255,7 +257,6 @@ architecture neorv32_cpu_control_rtl of neorv32_cpu_control is
     mie_mei        : std_ulogic; -- machine external interrupt enable
     mie_mti        : std_ulogic; -- machine timer interrupt enable
     mie_firq       : std_ulogic_vector(15 downto 0); -- fast interrupt enable
-    mip_firq_wdata : std_ulogic_vector(15 downto 0); -- fast interrupt pending write data
     mip_firq_we    : std_ulogic; -- fast interrupt pending write enable
     --
     privilege      : std_ulogic; -- current privilege mode
@@ -279,7 +280,7 @@ architecture neorv32_cpu_control_rtl of neorv32_cpu_control is
     dpc            : std_ulogic_vector(XLEN-1 downto 0); -- mode program counter
     dscratch0      : std_ulogic_vector(XLEN-1 downto 0); -- debug mode scratch register 0
     --
-    tdata1_hit_clr : std_ulogic; -- set to manually clear mcontrol6.hit0
+    tdata1_hit_we  : std_ulogic; -- mcontrol6.hit0 write access
     tdata1_execute : std_ulogic; -- enable instruction address match trigger
     tdata1_action  : std_ulogic; -- enter debug mode / ebreak exception when trigger fires
     tdata1_dmode   : std_ulogic; -- set to ignore tdata* CSR access from machine-mode
@@ -1453,10 +1454,10 @@ begin
 
       -- NEORV32-specific fast interrupts --
       for i in 0 to 15 loop
-        if (csr.mip_firq_we = '1') then -- write access to MIP(.FIRQ) CSR
-          trap_ctrl.irq_pnd(irq_firq_0_c+i) <= firq_i(i) or csr.mip_firq_wdata(i); -- keep buffering incoming FIRQs
-        else
-          trap_ctrl.irq_pnd(irq_firq_0_c+i) <= firq_i(i) or trap_ctrl.irq_pnd(irq_firq_0_c+i); -- keep pending FIRQs alive
+        if (firq_i(i) = '1') then -- new incoming FIRQs have priority
+          trap_ctrl.irq_pnd(irq_firq_0_c+i) <= '1';
+        elsif (csr.mip_firq_we = '1') and (csr.wmask(16+i) = '0') then -- clear-only
+          trap_ctrl.irq_pnd(irq_firq_0_c+i) <= '0';
         end if;
       end loop;
 
@@ -1592,23 +1593,32 @@ begin
 
   -- CSR Write Data -------------------------------------------------------------------------
   -- -------------------------------------------------------------------------------------------
-  csr_write_data: process(execute_engine.ir, csr.rdata, rs1_i)
-    variable tmp_v : std_ulogic_vector(XLEN-1 downto 0);
+  csr.cmd <= execute_engine.ir(instr_funct3_msb_c downto instr_funct3_lsb_c); -- access type
+
+  -- compute the write-data update mask --
+  csr_write_mask: process(csr.cmd, execute_engine.ir, rs1_i)
+    variable src_v : std_ulogic_vector(XLEN-1 downto 0);
   begin
-    -- immediate/register operand --
-    if (execute_engine.ir(instr_funct3_msb_c) = '1') then
-      tmp_v := (others => '0');
-      tmp_v(4 downto 0) := execute_engine.ir(19 downto 15); -- uimm5
+    -- CSR operand --
+    if (csr.cmd(2) = '1') then -- immediate source
+      src_v := (others => '0');
+      src_v(4 downto 0) := execute_engine.ir(19 downto 15); -- uimm5
+    else -- register source
+      src_v := rs1_i;
+    end if;
+    -- actual write mask --
+    if (csr.cmd(1 downto 0) = "11") then -- csrrc[i]: clear
+      csr.wmask <= not src_v;
     else
-      tmp_v := rs1_i;
+      csr.wmask <= src_v;
     end if;
-    -- tiny ALU to compute CSR write data --
-    case execute_engine.ir(instr_funct3_msb_c-1 downto instr_funct3_lsb_c) is
-      when "10"   => csr.wdata <= csr.rdata or tmp_v; -- set
-      when "11"   => csr.wdata <= csr.rdata and (not tmp_v); -- clear
-      when others => csr.wdata <= tmp_v; -- write
-    end case;
-  end process csr_write_data;
+  end process csr_write_mask;
+
+  -- tiny ALU to compute CSR write data --
+  with csr.cmd(1 downto 0) select csr.wdata <=
+    csr.rdata or  csr.wmask when "10",   -- csrrs[i]: set
+    csr.rdata and csr.wmask when "11",   -- csrrc[i]: clear
+                  csr.wmask when others; -- csrrw[i]: write
 
 
   -- External CSR Interface -----------------------------------------------------------------
@@ -1643,26 +1653,21 @@ begin
       csr.mtinst         <= (others => '0');
       csr.mcounteren     <= '0';
       csr.mcountinhibit  <= (others => '0');
-      csr.mip_firq_wdata <= (others => '0');
-      csr.mip_firq_we    <= '0';
       csr.dcsr_ebreakm   <= '0';
       csr.dcsr_ebreaku   <= '0';
       csr.dcsr_step      <= '0';
       csr.dcsr_prv       <= priv_mode_m_c;
       csr.dcsr_cause     <= (others => '0');
       csr.dpc            <= CPU_BOOT_ADDR(XLEN-1 downto 2) & "00"; -- 32-bit aligned boot address
       csr.dscratch0      <= (others => '0');
-      csr.tdata1_hit_clr <= '0';
       csr.tdata1_execute <= '0';
       csr.tdata1_action  <= '0';
       csr.tdata1_dmode   <= '0';
       csr.tdata2         <= (others => '0');
     elsif rising_edge(clk_i) then
 
-      -- defaults --
-      csr.we             <= csr.we_nxt and (not trap_ctrl.exc_buf(exc_illegal_c)); -- write if not an illegal instruction
-      csr.mip_firq_we    <= '0'; -- no write to MIP.FIRQ by default
-      csr.tdata1_hit_clr <= '0';
+      -- write if not an illegal instruction --
+      csr.we <= csr.we_nxt and (not trap_ctrl.exc_buf(exc_illegal_c));
 
       -- ********************************************************************************
       -- CSR access by application software
@@ -1721,9 +1726,8 @@ begin
           when csr_mcause_c => -- machine trap cause
             csr.mcause <= csr.wdata(31) & csr.wdata(4 downto 0); -- type (exception/interrupt) & identifier
 
-          when csr_mip_c => -- machine interrupt pending
-            csr.mip_firq_wdata <= csr.wdata(31 downto 16);
-            csr.mip_firq_we    <= '1'; -- trigger MIP.FIRQ write
+--        when csr_mip_c => -- machine interrupt pending
+            -- only the FIRQs are writable (clear-only); the actual mip.firq write logic is in <interrupt_buffer>
 
           -- --------------------------------------------------------------------
           -- machine counter setup --
@@ -1771,7 +1775,6 @@ begin
               if (csr.tdata1_dmode = '0') or (debug_ctrl.running = '1') then -- write access from debug-mode only?
                 csr.tdata1_execute <= csr.wdata(2);
                 csr.tdata1_action  <= csr.wdata(12);
-                csr.tdata1_hit_clr <= not csr.wdata(22);
               end if;
               if (debug_ctrl.running = '1') then -- writable from debug-mode only
                 csr.tdata1_dmode <= csr.wdata(27);
@@ -1917,6 +1920,10 @@ begin
     end if;
   end process csr_write_access;
 
+  -- out-of-process CSR write access --
+  csr.mip_firq_we   <= '1' when (csr.we = '1') and (csr.cmd(0) = '1') and (csr.addr = csr_mip_c)    else '0'; -- mip.firq: write/clear only
+  csr.tdata1_hit_we <= '1' when (csr.we = '1') and (csr.cmd(0) = '1') and (csr.addr = csr_tdata1_c) else '0'; -- tdata1.hit: write/clear only
+
   -- effective privilege mode is MACHINE when in debug mode --
   csr.privilege_eff <= priv_mode_m_c when (debug_ctrl.running = '1') else csr.privilege;
 
@@ -2413,7 +2420,7 @@ begin
       elsif rising_edge(clk_i) then
         if (hw_trigger_match = '1') and (trap_ctrl.exc_buf(exc_ebreak_c) = '1') then -- trigger has fired and breakpoint exception is pending
           hw_trigger_fired <= '1';
-        elsif (csr.tdata1_hit_clr = '1') then
+        elsif (csr.tdata1_hit_we = '1') and (csr.wmask(22) = '0') then -- clear-only
           hw_trigger_fired <= '0';
         end if;
       end if;

diff --git a/rtl/core/neorv32_package.vhd b/rtl/core/neorv32_package.vhd
@@ -53,7 +53,7 @@ package neorv32_package is
 
   -- Architecture Constants -----------------------------------------------------------------
   -- -------------------------------------------------------------------------------------------
-  constant hw_version_c : std_ulogic_vector(31 downto 0) := x"01090508"; -- hardware version
+  constant hw_version_c : std_ulogic_vector(31 downto 0) := x"01090509"; -- hardware version
   constant archid_c     : natural := 19; -- official RISC-V architecture ID
   constant XLEN         : natural := 32; -- native data path width
 

diff --git a/sim/neorv32_tb.vhd b/sim/neorv32_tb.vhd
@@ -185,7 +185,7 @@ begin
     if ci_mode then
       -- No need to send the full expectation in one big chunk
       check_uart(net, uart1_rx_handle, nul & nul);
-      check_uart(net, uart1_rx_handle, "0/57" & cr & lf);
+      check_uart(net, uart1_rx_handle, "0/58" & cr & lf);
     end if;
 
     -- Wait until all expected data has been received

diff --git a/sw/example/processor_check/main.c b/sw/example/processor_check/main.c
@@ -514,7 +514,7 @@ int main() {
   // No "real" CSR write access (because rs1 = r0)
   // ----------------------------------------------------------
   neorv32_cpu_csr_write(CSR_MCAUSE, mcause_never_c);
-  PRINT_STANDARD("[%i] Read-only CSR 'no-write' ", cnt_test);
+  PRINT_STANDARD("[%i] Read-only CSR (no-write) ", cnt_test);
 
   cnt_test++;
 
@@ -530,6 +530,58 @@ int main() {
   }
 
 
+  // ----------------------------------------------------------
+  // Test MIP.FIRQ clear-only bits
+  // ----------------------------------------------------------
+  neorv32_cpu_csr_write(CSR_MCAUSE, mcause_never_c);
+  PRINT_STANDARD("[%i] Clear-only CSR (mip.FIRQ) ", cnt_test);
+
+  if ((NEORV32_SYSINFO->SOC & (1 << SYSINFO_SOC_IO_SPI)) && (NEORV32_SYSINFO->SOC & (1 << SYSINFO_SOC_IO_GPTMR))) {
+    cnt_test++;
+
+    tmp_a = 0; // error counter
+
+    // disable all IRQ sources
+    neorv32_cpu_csr_write(CSR_MIE, 0);
+
+    // trigger two FIRQs
+    neorv32_gptmr_setup(CLK_PRSC_2, 1, -1); // fire GPTMR FIRQ
+    neorv32_spi_setup(CLK_PRSC_2, 0, 0, 0, -1); // fire SPI FIRQ
+    neorv32_gptmr_disable();
+    neorv32_spi_disable();
+
+    // both FIRQs should be pending now
+    if ((neorv32_cpu_csr_read(CSR_MIP) & 0xffff0000) != ((1 << SPI_FIRQ_PENDING) + (1 << GPTMR_FIRQ_PENDING))) {
+      tmp_a++;
+    }
+
+    // test write/set/clear access
+    neorv32_cpu_csr_write(CSR_MIP, 0xffffffff); // should have no effect at all
+    neorv32_cpu_csr_set(CSR_MIP, 0xffffffff); // should have no effect at all
+    neorv32_cpu_csr_clr(CSR_MIP, 1 << SPI_FIRQ_PENDING); // clear SPI FIRQ only
+
+    if ((neorv32_cpu_csr_read(CSR_MIP) & 0xffff0000) != (1 << GPTMR_FIRQ_PENDING)) {
+      tmp_a++;
+    }
+
+    // clear all mip.FIRQ bits
+    neorv32_cpu_csr_write(CSR_MIP, 0);
+    if ((neorv32_cpu_csr_read(CSR_MIP) & 0xffff0000) != 0) {
+      tmp_a++;
+    }
+
+    if (tmp_a == 0) {
+      test_ok();
+    }
+    else {
+      test_fail();
+    }
+  }
+  else {
+    PRINT_STANDARD("[n.a.]\n");
+  }
+
+
   // ----------------------------------------------------------
   // Unaligned instruction address
   // ----------------------------------------------------------

diff --git a/sw/lib/include/neorv32_cpu_csr.h b/sw/lib/include/neorv32_cpu_csr.h
@@ -304,22 +304,22 @@ enum NEORV32_CSR_MIP_enum {
   CSR_MIP_MEIP    = 11, /**< CPU mip CSR (11): MEIP - Machine external interrupt pending (r/-) */
 
   /* NEORV32-specific extension: Fast Interrupt Requests (FIRQ) */
-  CSR_MIP_FIRQ0P  = 16, /**< CPU mip CSR (16): FIRQ0P - Fast interrupt channel 0 pending (r/w) */
-  CSR_MIP_FIRQ1P  = 17, /**< CPU mip CSR (17): FIRQ1P - Fast interrupt channel 1 pending (r/w) */
-  CSR_MIP_FIRQ2P  = 18, /**< CPU mip CSR (18): FIRQ2P - Fast interrupt channel 2 pending (r/w) */
-  CSR_MIP_FIRQ3P  = 19, /**< CPU mip CSR (19): FIRQ3P - Fast interrupt channel 3 pending (r/w) */
-  CSR_MIP_FIRQ4P  = 20, /**< CPU mip CSR (20): FIRQ4P - Fast interrupt channel 4 pending (r/w) */
-  CSR_MIP_FIRQ5P  = 21, /**< CPU mip CSR (21): FIRQ5P - Fast interrupt channel 5 pending (r/w) */
-  CSR_MIP_FIRQ6P  = 22, /**< CPU mip CSR (22): FIRQ6P - Fast interrupt channel 6 pending (r/w) */
-  CSR_MIP_FIRQ7P  = 23, /**< CPU mip CSR (23): FIRQ7P - Fast interrupt channel 7 pending (r/w) */
-  CSR_MIP_FIRQ8P  = 24, /**< CPU mip CSR (24): FIRQ8P - Fast interrupt channel 8 pending (r/w) */
-  CSR_MIP_FIRQ9P  = 25, /**< CPU mip CSR (25): FIRQ9P - Fast interrupt channel 9 pending (r/w) */
-  CSR_MIP_FIRQ10P = 26, /**< CPU mip CSR (26): FIRQ10P - Fast interrupt channel 10 pending (r/w) */
-  CSR_MIP_FIRQ11P = 27, /**< CPU mip CSR (27): FIRQ11P - Fast interrupt channel 11 pending (r/w) */
-  CSR_MIP_FIRQ12P = 28, /**< CPU mip CSR (28): FIRQ12P - Fast interrupt channel 12 pending (r/w) */
-  CSR_MIP_FIRQ13P = 29, /**< CPU mip CSR (29): FIRQ13P - Fast interrupt channel 13 pending (r/w) */
-  CSR_MIP_FIRQ14P = 30, /**< CPU mip CSR (30): FIRQ14P - Fast interrupt channel 14 pending (r/w) */
-  CSR_MIP_FIRQ15P = 31  /**< CPU mip CSR (31): FIRQ15P - Fast interrupt channel 15 pending (r/w) */
+  CSR_MIP_FIRQ0P  = 16, /**< CPU mip CSR (16): FIRQ0P - Fast interrupt channel 0 pending (r/c) */
+  CSR_MIP_FIRQ1P  = 17, /**< CPU mip CSR (17): FIRQ1P - Fast interrupt channel 1 pending (r/c) */
+  CSR_MIP_FIRQ2P  = 18, /**< CPU mip CSR (18): FIRQ2P - Fast interrupt channel 2 pending (r/c) */
+  CSR_MIP_FIRQ3P  = 19, /**< CPU mip CSR (19): FIRQ3P - Fast interrupt channel 3 pending (r/c) */
+  CSR_MIP_FIRQ4P  = 20, /**< CPU mip CSR (20): FIRQ4P - Fast interrupt channel 4 pending (r/c) */
+  CSR_MIP_FIRQ5P  = 21, /**< CPU mip CSR (21): FIRQ5P - Fast interrupt channel 5 pending (r/c) */
+  CSR_MIP_FIRQ6P  = 22, /**< CPU mip CSR (22): FIRQ6P - Fast interrupt channel 6 pending (r/c) */
+  CSR_MIP_FIRQ7P  = 23, /**< CPU mip CSR (23): FIRQ7P - Fast interrupt channel 7 pending (r/c) */
+  CSR_MIP_FIRQ8P  = 24, /**< CPU mip CSR (24): FIRQ8P - Fast interrupt channel 8 pending (r/c) */
+  CSR_MIP_FIRQ9P  = 25, /**< CPU mip CSR (25): FIRQ9P - Fast interrupt channel 9 pending (r/c) */
+  CSR_MIP_FIRQ10P = 26, /**< CPU mip CSR (26): FIRQ10P - Fast interrupt channel 10 pending (r/c) */
+  CSR_MIP_FIRQ11P = 27, /**< CPU mip CSR (27): FIRQ11P - Fast interrupt channel 11 pending (r/c) */
+  CSR_MIP_FIRQ12P = 28, /**< CPU mip CSR (28): FIRQ12P - Fast interrupt channel 12 pending (r/c) */
+  CSR_MIP_FIRQ13P = 29, /**< CPU mip CSR (29): FIRQ13P - Fast interrupt channel 13 pending (r/c) */
+  CSR_MIP_FIRQ14P = 30, /**< CPU mip CSR (30): FIRQ14P - Fast interrupt channel 14 pending (r/c) */
+  CSR_MIP_FIRQ15P = 31  /**< CPU mip CSR (31): FIRQ15P - Fast interrupt channel 15 pending (r/c) */
 };