Skip to content

issues.md

Latest commit

 

History

History
193 lines (112 loc) · 7.67 KB

issues.md

File metadata and controls

193 lines (112 loc) · 7.67 KB
Raw

RTL Review Findings

Systematic review of each Leaf processor module. Started: 2026-05-29

Methodology

Each VHDL module is analyzed individually. Issues are categorized as:

Severity Description
BUG Functionally incorrect — affects instruction execution or ISA compliance
WARN Potentially problematic — may cause failures in certain scenarios
INFO Style, clarity, or non-critical improvement

Open Issues

WARN: Reset distribution asymmetry

rtl/leaf.vhdl:79-105, rtl/wb_ctrl.vhdl:122

The reset signal feeding the core is generated by the wb_ctrl FSM (reset <= '1' when curr_state = START else '0'). clk_ctrl and counters receive rst_i directly.

Sequence: rst_i deasserts → wb_ctrl moves from START to IDLE → reset goes low → core exits reset.

This introduces a 1-cycle delay between rst_i and reset for the core, while counters is released immediately. clk_ctrl is also released immediately (or rst_i gate on clock gate output).

Impact: Low — the core starts executing 1 cycle after the counters.

Suggestion: Either document this behavior explicitly, or unify reset distribution (use rst_i in all modules with a dedicated reset synchronizer separate from the Wishbone FSM).

WARN: COP interface lacks handshake signals

rtl/leaf.vhdl:27-30, rtl/core.vhdl:32-35

The coprocessor interface has cop_adr_o (6 bits), cop_dat_o, cop_we_o, cop_dat_i. No cop_ack_i, cop_err_i, or cop_ready_i.

Impact: A multi-cycle coprocessor cannot stall the pipeline. The interface is effectively single-cycle.

Suggestion: Add cop_stall_i (or cop_ready_i) for the core to freeze the pipeline while waiting. Alternatively, document that the interface is single-cycle only.

INFO: Gated clock via transparent latch

rtl/clk_ctrl.vhdl:28-33

Clock gating uses a transparent latch (enable sample on falling edge) + AND gate. Classic technique, but:

  • FPGAs generally don't synthesize latches well (LUT + routing, not dedicated latch)
  • ASIC flows prefer dedicated clock gating cells (ICG)
  • Yosys needs configuration to handle generated clocks

Suggestion: Use clock enables on registers (if clk_en = '1' in each clocked process) instead. Avoids the latch and doesn't create an additional clock domain.

INFO: Bus error reporting uses current enable signals

rtl/wb_ctrl.vhdl:124-126

imrd_err <= imrd_en when curr_state = ERROR else '0';
dmrd_err <= dmrd_en when curr_state = ERROR else '0';
dmwr_err <= dmwr_en when curr_state = ERROR else '0';

The transaction type that caused the error is not latched — uses the current value of imrd_en/dmrd_en/dmwr_en in the cycle when curr_state = ERROR. If the enable changes between the error and the ERROR state, the error may not be correctly attributed.

Impact: Low in practice — enables rarely change during a bus error — but fragile.

Suggestion: Latch the transaction type (error_source) when err_i is received.

INFO: No bus timeout

rtl/wb_ctrl.vhdl:61-111

The Wishbone FSM waits indefinitely for ack_i or err_i in READ_INSTR, READ_DATA, and WRITE_DATA states. No timeout counter.

Impact: A Wishbone slave that never responds locks up the processor forever.

Suggestion: Add an external timeout watchdog or document the limitation.

INFO: sel_o active when bus is idle

rtl/wb_ctrl.vhdl:117

sel_o <= dmwr_be when curr_state = WRITE_DATA else (others => '1');

sel_o is 1111 even when cyc_o is low. Wishbone slaves ignore sel_o when cyc_o is low, so not a functional error.

INFO: Flush condition includes not pcwr_en_i

rtl/if_stage.vhdl:71

flush_o <= taken_i or imrd_err_i or not pcwr_en_i — flush_o is asserted when pcwr_en_i is low (pipeline stall). The fetched instruction in the pipeline register receives flush_o = 1 and is discarded by ID/EX. On resume, the instruction must be re-fetched — wasting 1 cycle.

Suggestion: Don't update the pipeline register during stalls (gate out_pipe_proc with pcwr_en_i).

INFO: Speculative fetch wasted on taken branches

When taken_i = '1':

  1. pc_reg receives target_i(XLEN-1 downto 2)
  2. imrd_addr_o (pc_reg & "00") changes to the target address
  3. The previous Wishbone transaction (sequential) is already in flight — completes with discarded data
  4. wb_ctrl returns to IDLE, sees imrd_en_o = 1 with new address, starts correct fetch

Functionally correct, but wastes 1 bus transaction per taken branch. Inherent to the 2-stage pipeline without branch prediction.

INFO: Don't-care values in datapath during flush/invalid opcode

rtl/main_ctrl.vhdl:50-63, 77

The immediate generator injects don't-care values ('-') during flush and unknown opcodes. These propagate to ALU and shifter inputs, reaching numeric_std conversions — producing simulation warnings (metavalue detected). During flush = '1', exec_ctrl_o and regwr_en_o are zeroed, so metavalues on imm_o are never functionally captured. Yosys ignores don't-cares.

Not a functional bug — only visual pollution in simulation.

WONTFIX

Invalid CSR accesses don't generate traps

rtl/main_ctrl.vhdl:148, rtl/csrs.vhdl:116

  • main_ctrl treats all SYSTEM_OPCODE as valid
  • The CSR read path returns zero for unknown addresses

Intentional for this simple core — invalid CSR reads 0, writes ignored. Acceptable for Leaf's scope.

Validation Notes

  • make run fails because the default target depends on verif/tests/dump/out.bin, which doesn't exist in the repository.
  • verif/tests/addi was fixed — no longer diverges, matches Spike signature.
  • make -C verif/tests run TEST=... doesn't match the current Makefile; the correct invocation is make -C verif/tests/<test> run.

Planned Improvements

mcountinhibit (CSR 0x320) — counter inhibit

Add the mcountinhibit WARL register to allow software to pause mcycle and minstret.

Implementation:

  1. csrs.vhdl: add mcountinhibit_reg (bits 0 and 2 writable, others hardwired to 0), write process, read in read_csr, mcountinhibit_o port
  2. id_stage.vhdl: add mcountinhibit_o port — wire-through from csrs
  3. core.vhdl: add mcountinhibit_o port — wire-through from id_stage
  4. counters.vhdl: add inhibit_i port — inhibit_i(0) freezes cycle_reg, inhibit_i(2) freezes instret_reg
  5. leaf.vhdl: connect core.mcountinhibit_o → counters.inhibit_i
  6. leaf_pkg.vhdl: add CSR_ADDR_MCOUNTINHIBIT constant, update component declarations

mtimecmp — internal timer interrupt

The core does not generate tm_irq internally. The time counter (CSR 0xC01/0xC81) exists and is readable by software, but without an mtimecmp register the timer interrupt depends on external hardware.

Future implementation:

  1. Add mtimecmp to CSR space (address 0x321, alongside mtime at 0xC01)
  2. Hardware comparator: tm_irq <= '1' when timer >= mtimecmp
  3. Option 1: implement in csrs.vhdl with internal register and comparator
  4. Option 2: external module connected via Wishbone, with tm_irq as output

Modules Reviewed

All 17 RTL modules reviewed:

if_stage, main_ctrl, reg_file, csrs_logic, csrs, id_stage, alu_ctrl, alu, br_detector, ex_block, dmls_block, core, wb_ctrl, leaf, leaf_pkg, clk_ctrl, counters

Bug fixes applied to: csrs.vhdl (mret path, write_mcause condition, mtval priority), if_stage.vhdl (30-bit pc_reg), counters.vhdl (instret/timer counters), id_stage.vhdl (dmld_fault wiring).

All ports standardized to _i/_o convention and XLEN generic across every module.