Fix build failure when TIMER_CNT=1 (#7)

src/apb_timer.sv

@@ -30,11 +30,25 @@ module apb_timer
     output logic [(TIMER_CNT * 2) - 1:0] irq_o // overflow and cmp interrupt
 );
 
+    // $clog2(TIMER_CNT) is 0 when TIMER_CNT == 1, which would make the
+    // address-select vector below [-1:0] (an inverted, 2-bit-wide range
+    // instead of the intended 0-width) and the PADDR part-select below it
+    // an inverted [x:x+1] range -- both illegal/misleading on a
+    // descending-declared vector. Force a minimum width of 1 bit and
+    // special-case TIMER_CNT == 1 (there is only one timer, so no address
+    // bits are needed to select among timers) to sidestep the $clog2(1)==0
+    // edge case entirely instead of computing a negative width from it.
+    localparam int unsigned SLAVE_ADDR_BITS = (TIMER_CNT > 1) ? $clog2(TIMER_CNT) : 1;
+
     logic [TIMER_CNT-1:0] psel_int, pready, pslverr;
-    logic [$clog2(TIMER_CNT) - 1:0] slave_address_int;
+    logic [SLAVE_ADDR_BITS - 1:0] slave_address_int;
     logic [TIMER_CNT-1:0] [31:0] prdata;
 
-    assign slave_address_int = PADDR[$clog2(TIMER_CNT)+ `REGS_MAX_ADR + 1:`REGS_MAX_ADR + 2];
+    if (TIMER_CNT > 1) begin : gen_slave_address_multi
+        assign slave_address_int = PADDR[SLAVE_ADDR_BITS + `REGS_MAX_ADR + 1:`REGS_MAX_ADR + 2];
+    end else begin : gen_slave_address_single
+        assign slave_address_int = '0;
+    end
 
     always_comb
     begin

verify/Makefile

@@ -0,0 +1,28 @@
+SIM ?= verilator
+TOPLEVEL_LANG ?= verilog
+
+VERILOG_SOURCES = $(shell pwd)/../src/timer.sv $(shell pwd)/../src/apb_timer.sv
+TOPLEVEL = apb_timer
+MODULE = test_apb_timer
+
+# TIMER_CNT=1 is the upstream issue #7 configuration ("Timer does not
+# build if NUM_TIMERS is set to 1"). WIDTHEXPAND/CASEINCOMPLETE/ASCRANGE/
+# WIDTHTRUNC each have at least one instance confirmed pre-existing and
+# unrelated to this bug -- present in timer.sv for every TIMER_CNT value,
+# before and after this fix (checked by diffing lint output across
+# TIMER_CNT=1/2/4 and before/after the fix -- see
+# reproduce_bug_before_fix.log). WIDTHTRUNC in particular also has
+# bug-specific instances (apb_timer.sv:42/52/53), but since Verilator's
+# -Wno-<CODE> can't be scoped per file, suppressing it here trades away
+# using it as a regression signal. SELRANGE does not have that problem:
+# it is 100% unique to the bug (apb_timer.sv:37, the inverted PADDR
+# part-select -- matches the reporter's own "part-select ... does not
+# match declaration" symptom) and appears nowhere else in this codebase
+# for any TIMER_CNT. Left enabled, SELRANGE alone is what makes `make`
+# genuinely fail to build against the unmodified RTL and succeed only
+# once the fix in src/apb_timer.sv is applied -- deliberately not using
+# a blanket -Wno-fatal, which was tried first and silenced SELRANGE too,
+# making this testbench falsely pass against the *unfixed* RTL.
+EXTRA_ARGS += -GTIMER_CNT=1 -Wno-WIDTHEXPAND -Wno-CASEINCOMPLETE -Wno-ASCRANGE -Wno-WIDTHTRUNC
+
+include $(shell cocotb-config --makefiles)/Makefile.sim

verify/test_apb_timer.py

@@ -0,0 +1,116 @@
+"""cocotb regression test for pulp-platform/apb_timer issue #7:
+"Timer does not build if NUM_TIMERS is set to 1"
+https://github.com/pulp-platform/apb_timer/issues/7
+
+Against the unmodified RTL, elaborating apb_timer with TIMER_CNT=1 fails:
+`$clog2(1)` is 0, making `slave_address_int`'s declared range [-1:0]
+(an inverted, 2-bit-wide range instead of the intended 0-width) and the
+PADDR part-select that drives it [3:4] (inverted relative to PADDR's own
+descending declaration) -- see reproduce_bug_before_fix.log for the raw
+`verilator --lint-only` output this produces (matches the reporter's
+"part-select ... does not match declaration" symptom, via a different
+tool). This Makefile builds against the *fixed* RTL (TIMER_CNT=1 is now
+special-cased to force a 1-bit, constant-zero address-select instead of
+computing a negative width from $clog2(1)) and functionally exercises
+the one timer instance over its APB interface, to confirm the fix
+doesn't just compile but is actually wired correctly.
+"""
+
+import cocotb
+from cocotb.clock import Clock
+from cocotb.triggers import RisingEdge
+
+REG_TIMER = 0x0
+REG_TIMER_CTRL = 0x4
+REG_CMP = 0x8
+
+ENABLE_BIT = 1 << 0
+
+
+async def reset(dut):
+    dut.HRESETn.value = 0
+    dut.PSEL.value = 0
+    dut.PENABLE.value = 0
+    dut.PWRITE.value = 0
+    dut.PADDR.value = 0
+    dut.PWDATA.value = 0
+    await RisingEdge(dut.HCLK)
+    await RisingEdge(dut.HCLK)
+    dut.HRESETn.value = 1
+    await RisingEdge(dut.HCLK)
+
+
+async def apb_write(dut, addr, data):
+    """One APB3 setup+access write transfer. Waits on PREADY rather than
+    an assumed fixed cycle count (this RTL always drives PREADY=1'b1, but
+    a wait-loop is the portable pattern regardless -- see this program's
+    own counter3/uart cocotb testbenches for why an assumed fixed timing
+    relationship is fragile)."""
+    dut.PADDR.value = addr
+    dut.PWDATA.value = data
+    dut.PWRITE.value = 1
+    dut.PSEL.value = 1
+    dut.PENABLE.value = 0
+    await RisingEdge(dut.HCLK)
+    dut.PENABLE.value = 1
+    await RisingEdge(dut.HCLK)
+    while int(dut.PREADY.value) == 0:
+        await RisingEdge(dut.HCLK)
+    assert int(dut.PSLVERR.value) == 0, "unexpected PSLVERR on write"
+    dut.PSEL.value = 0
+    dut.PENABLE.value = 0
+    dut.PWRITE.value = 0
+
+
+async def apb_read(dut, addr):
+    """One APB3 setup+access read transfer."""
+    dut.PADDR.value = addr
+    dut.PWRITE.value = 0
+    dut.PSEL.value = 1
+    dut.PENABLE.value = 0
+    await RisingEdge(dut.HCLK)
+    dut.PENABLE.value = 1
+    await RisingEdge(dut.HCLK)
+    while int(dut.PREADY.value) == 0:
+        await RisingEdge(dut.HCLK)
+    assert int(dut.PSLVERR.value) == 0, "unexpected PSLVERR on read"
+    data = int(dut.PRDATA.value)
+    dut.PSEL.value = 0
+    dut.PENABLE.value = 0
+    return data
+
+
+@cocotb.test()
+async def test_single_timer_builds_and_counts(dut):
+    """With TIMER_CNT=1, apb_timer must (a) elaborate at all, and (b)
+    actually reach and drive its one timer instance via APB -- proving
+    the TIMER_CNT==1 special-cased address-select path is wired
+    correctly, not just that it compiles."""
+    cocotb.start_soon(Clock(dut.HCLK, 10, units="ns").start())
+    await reset(dut)
+
+    # Enable the timer (no prescaler -> counts every cycle).
+    await apb_write(dut, REG_TIMER_CTRL, ENABLE_BIT)
+
+    count_before = await apb_read(dut, REG_TIMER)
+    for _ in range(20):
+        await RisingEdge(dut.HCLK)
+    count_after = await apb_read(dut, REG_TIMER)
+
+    assert count_after > count_before, (
+        f"timer did not count: before={count_before}, after={count_after} "
+        "-- APB access to the single TIMER_CNT=1 instance is not reaching it"
+    )
+
+
+@cocotb.test()
+async def test_single_timer_compare_register(dut):
+    """Write and read back REG_CMP through the same TIMER_CNT=1 address
+    path, as a second, independent register -- catches an address-decode
+    bug that might only affect one register by coincidence."""
+    cocotb.start_soon(Clock(dut.HCLK, 10, units="ns").start())
+    await reset(dut)
+
+    await apb_write(dut, REG_CMP, 0x1234)
+    readback = await apb_read(dut, REG_CMP)
+    assert readback == 0x1234, f"expected 0x1234, got {readback:#x}"