From 2e177fec5112f3e8fe31e0493a0c828480160770 Mon Sep 17 00:00:00 2001 From: Tuan Nguyen Date: Wed, 16 Sep 2026 00:45:12 +0700 Subject: [PATCH 1/2] apb_timer.sv: Fix build failure when TIMER_CNT=1 (#7) $clog2(TIMER_CNT) is 0 when TIMER_CNT == 1, which made 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 driving it an inverted [x:x+1] range -- both illegal on a descending-declared vector, matching the reporter's "part-select ... does not match declaration" symptom. Add a SLAVE_ADDR_BITS localparam that special-cases TIMER_CNT == 1 to a constant-zero, 1-bit slave_address_int instead of computing a negative width from $clog2(1). There is only one timer in that configuration, so no address bits are needed to select among timers. The TIMER_CNT > 1 path is untouched, just moved into its own generate branch alongside the new single-timer branch. Fixes #7 --- src/apb_timer.sv | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/apb_timer.sv b/src/apb_timer.sv index 90134ca..2f68876 100644 --- a/src/apb_timer.sv +++ b/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 From 68e7471aa7995bf20a8fae655683c9268c5c0532 Mon Sep 17 00:00:00 2001 From: Tuan Nguyen Date: Wed, 16 Sep 2026 00:45:24 +0700 Subject: [PATCH 2/2] verify: Add cocotb regression test for the TIMER_CNT=1 fix (#7) Two tests against the sole timer instance in a TIMER_CNT=1 build, driven over its APB interface (Verilator, cocotb classic Makefile flow): - test_single_timer_builds_and_counts: enables the timer and confirms its count register actually advances, proving the TIMER_CNT==1 address-select path reaches the instance (not just that it elaborates). - test_single_timer_compare_register: writes and reads back REG_CMP through the same path, as a second, independent register. Against the unmodified RTL this fails to even build (the SELRANGE lint error from #7); against the fix in the parent commit both tests pass. --- verify/Makefile | 28 ++++++++++ verify/test_apb_timer.py | 116 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 verify/Makefile create mode 100644 verify/test_apb_timer.py diff --git a/verify/Makefile b/verify/Makefile new file mode 100644 index 0000000..165a0cc --- /dev/null +++ b/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- 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 diff --git a/verify/test_apb_timer.py b/verify/test_apb_timer.py new file mode 100644 index 0000000..8625aab --- /dev/null +++ b/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}"