axi_demux: enforce exactly MaxTrans in-flight transactions (fixes #249) by arjunsv98 · Pull Request #440 · pulp-platform/axi

arjunsv98 · 2026-08-02T19:48:06Z

Summary

The demux's per-ID in-flight counters are sized with idx_width(MaxTrans) bits — enough to index MaxTrans values (0..MaxTrans-1), not enough to count to MaxTrans. Consequently:

For power-of-two MaxTrans — including the module default of 8 — the counters report full at MaxTrans - 1: a demux configured for 8 outstanding transactions accepts only 7, on the AR path, the AW path, and the open-W-burst (w_open) ceiling.
For other values, the all-ones full condition allows more than configured (e.g. 15 in flight for MaxTrans = 10).

This is exactly the behavior diagnosed by the reporter of #249 in 2022; this PR implements the fix (with exact-limit semantics) and adds the regression that was missing.

Fix

axi_demux_simple: widen IdCounterWidth to idx_width(MaxTrans + 1); gate w_open with < MaxTrans instead of != all-ones; pass MaxTrans to the ID counters.
axi_demux_id_counters: new MaxTrans parameter (defaults to the legacy saturation value, so standalone instantiations are unaffected); full condition compares in_flight >= MaxTrans instead of &in_flight. >= rather than == because a simultaneous push + ATOP injection advances a counter by 2.

Note on semantics: this makes MaxTrans an exact limit. Non-power-of-two configurations previously enjoyed slack up to the counter's all-ones value (e.g. 15 for MaxTrans=10); they are now limited to exactly MaxTrans. This matches the parameter's documentation (see also #336).

Verification

New directed test tb_axi_demux_maxtrans: back-to-back same-ID reads into a demux whose selected master port accepts every AR but withholds all R responses, so the in-flight count can only grow; the number of accepted AR handshakes equals the true counter limit.

config	before	after
`MaxTrans = 8`	7 accepted (bug)	8
`MaxTrans = 10`	15 accepted (slack)	10

Full tb_axi_xbar regression (Verilator): 8072 checks, 0 failures, in both the default (10/6) and an all-power-of-two (8/8) MaxMstTrans/MaxSlvTrans configuration.

No existing test could observe this bug: the shipped testbenches all use non-power-of-two limits, and none measures achieved concurrency — the new directed test closes that gap.

Fixes #249

Document INCR/FIXED/WRAP burst behavior in the module headers of axi_dw_converter, axi_dw_downsizer, and axi_dw_upsizer. Closes pulp-platform#429.

… interfaces. The original multi-dimensional interface module is not supported by many tools. Downstream users can still trivially write their own intf wrapper. Close pulp-platform#353.

…meter (pulp-platform#399) * src: axi_fifo_delay_dyn: Fix incorrect power-of-two check for `Depth` parameter The `axi_fifo_delay_dyn` module's parameter constraint checker incorrectly identified valid `Depth` values as not being a power of two. This commit corrects the logic to accurately verify that the `Depth` parameter is a power of two. * Update src/axi_fifo_delay_dyn.sv --------- Co-authored-by: Chen Wu <chenwu@iis.ee.ethz.ch>

…m#434)

* Bump to common_cells v2 * fix(cc): fix cc_ module parameter names for common_cells v2 Update parameter names to match the actual cc_ module API in v2-test: - cc_delta_counter: Width->WIDTH, StickyOverflow->STICKY_OVERFLOW - cc_stream_register: data_t->T - cc_fifo: FallThrough->FALL_THROUGH, Depth->DEPTH, data_t->dtype - cc_counter: Width->WIDTH Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * hw: Align mux and demux to common_cells v2 * hw: Align params name to common cells v2-test * hw: Remove test_i ports * hw: clean up dead test_i ports and use cc_pkg::lzc_mode_e enum for the cc_lzc MODE parameter. * fixed indentation * bumped common-cells v2.0.0-beta and tech-cells-generic 0.2.14 * bumped common2 fixes for synopsis tools * scripts: add target cc_no_deprecated * deps: Bump `common_cells` to `v2.0.0-beta.2` --------- Co-authored-by: Lorenzo Leone <lleone@iis.ee.ethz.ch> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Chen Wu <chenwu@iis.ee.ethz.ch> Co-authored-by: Niccolò Giuliani <niccolo.giuliani44@gmail.com>

imchenwu

Hi, thx for working on this. :) As this PR requires exposing a new port, we'll have to merge it in the next major release, so I modified the base branch to devel.

imchenwu · 2026-08-10T05:49:07Z

+
+`include "axi/typedef.svh"
+
+module tb_axi_demux_maxtrans #(


Is this tb necessary? I suggest to add assertions inside the axi_demux_id_counters module instead.

Replied in the main thread with more detail, but short version: the existing tb configs all use non-power-of-2 MaxTrans, so module assertions alone would never fire in CI — this tb is what actually builds a triggering config. Can add assertions on top or drop the tb, whichever you prefer.

arjunsv98 · 2026-08-11T20:13:14Z

Thanks for taking a look! Applied your occupied suggestion and rebased onto devel.

About the tb — I considered moving the check into assertions in axi_demux_id_counters instead, but realized they'd never actually trigger in CI: all the existing testbenches use non-power-of-2 MaxTrans values (10/6/5), and the bug only manifests for power-of-2 configs. So an assertion would protect downstream users, but the repo's own CI would still have passed with the original bug in place. The directed tb is what actually builds a triggering config.

Happy to add the assertions on top if you'd like both, or drop the tb if you feel strongly — just wanted to flag why I kept it for now.

The per-ID in-flight counters were sized with idx_width(MaxTrans) bits, which can only represent values 0 to MaxTrans-1. As a result: - For power-of-two MaxTrans (including the module default of 8), the counters reported full at MaxTrans-1, silently delivering one less outstanding transaction than configured on both the AR and AW paths as well as the open-W-burst counter. - For other values, the all-ones full condition allowed more than MaxTrans outstanding transactions (e.g. 15 for MaxTrans = 10). Widen the counters to idx_width(MaxTrans + 1) bits and compare against MaxTrans directly instead of the counter's saturation value, so the configured limit is enforced exactly. The comparison uses >= rather than == because a simultaneous push and ATOP injection advances a counter by two. Add a directed regression, tb_axi_demux_maxtrans, which issues back-to-back same-ID reads into a demux whose selected master port withholds all R responses: the number of accepted AR handshakes is exactly the counter limit. Before this change it stalls at 7 of a configured 8; after, at exactly MaxTrans for both power-of-two and other values. Fixes pulp-platform#249

Review feedback: with 2**CounterWidth outstanding transactions the counter wraps and any_outstanding_trx_o would report no outstanding transactions. Include the overflow flag in the occupied computation.

imchenwu · 2026-08-18T08:38:22Z

Hi, I checked the current CI. Actually tb_axi_dw_upsizer sets AXI_MAX_READS as 4, which reaches axi_demux inside axi_dw_upsizer as MaxTrans=4. Therefore, I still prefer to use the assertion instead of this specialized tb.

Review feedback: replace the specialized testbench with an elaboration-time assertion. The assertion fails as soon as any instantiation configures a counter width that cannot hold MaxTrans, which covers configurations reached through other DUTs (e.g. tb_axi_dw_upsizer instantiates axi_demux with MaxTrans=4 through axi_dw_upsizer) without a dedicated test.

arjunsv98 · 2026-08-21T00:20:13Z

Fair enough, you're right — I'd only checked the top-level tb parameters and missed configs reaching the demux through other DUTs. Swapped the tb for an elaboration-time assertion in axi_demux_id_counters (2**CounterWidth > MaxTrans). Verified it fires at time 0 with the original counter sizing and is silent with the fix.

imchenwu · 2026-08-21T09:25:23Z

+  // inclusive), so they are sized for `MaxTrans + 1` states.  Sizing them with
+  // `idx_width(MaxTrans)` lets them saturate at `MaxTrans - 1` when `MaxTrans` is a power of
+  // two, silently reducing the configured concurrency by one transaction (issue #249).
+  localparam int unsigned IdCounterWidth = cc_pkg::idx_width(MaxTrans + 1);


Suggested change

localparam int unsigned IdCounterWidth = cc_pkg::idx_width(MaxTrans + 1);

localparam int unsigned IdCounterWidth = cc_pkg::cnt_width(MaxTrans);

TIL cnt_width exists — that's exactly the right helper here (and kind of poetic that the bug was idx_width being used where cnt_width belonged). Applied.

imchenwu · 2026-08-21T10:28:14Z

+// pragma translate_off
+  initial begin : validate_params
+    counter_holds_maxtrans : assert (2**CounterWidth > MaxTrans) else
+      $fatal(1, "CounterWidth (%0d bits, maximum value %0d) cannot represent MaxTrans (%0d): \
+                 the counters would report full at %0d in-flight transactions.",
+             CounterWidth, 2**CounterWidth - 1, MaxTrans, 2**CounterWidth - 1);
+  end
+// pragma translate_on
+


Maybe replace by runtime assertions, see the other comment.

Review feedback: size the counters with cc_pkg::cnt_width, which exists for exactly this purpose, and watch the in-flight limit with the suggested runtime assertion properties (full only at or above MaxTrans, no push at or beyond MaxTrans) alongside the existing underflow check.

imchenwu

Thx!

A stalled AR remains locked while an ATOP can fill its ID counter. The pending AR must still complete, so pushing at MaxTrans is valid in this case.

imchenwu and others added 5 commits July 28, 2026 16:12

docs: clarify data-width converters' burst-type support

c8369d1

Document INCR/FIXED/WRAP burst behavior in the module headers of axi_dw_converter, axi_dw_downsizer, and axi_dw_upsizer. Closes pulp-platform#429.

axi_xbar_unmuxed: delete the intf module containing multi-dimensional…

aafac8f

… interfaces. The original multi-dimensional interface module is not supported by many tools. Downstream users can still trivially write their own intf wrapper. Close pulp-platform#353.

changed XSIM macro to tool pre-defined XILINX_SIMULATOR (pulp-platfor…

6255cca

…m#434)

arjunsv98 force-pushed the fix/demux-maxtrans-249 branch from 08dbeae to 88939f5 Compare August 2, 2026 19:58

imchenwu changed the base branch from master to devel August 10, 2026 06:52

imchenwu requested changes Aug 10, 2026

View reviewed changes

arjunsv98 force-pushed the fix/demux-maxtrans-249 branch from 88939f5 to dc46d0c Compare August 11, 2026 20:13

arjunsv98 added 3 commits August 11, 2026 16:15

axi_demux_id_counters: include overflow in occupied status

2a48435

Review feedback: with 2**CounterWidth outstanding transactions the counter wraps and any_outstanding_trx_o would report no outstanding transactions. Include the overflow flag in the occupied computation.

tb_axi_demux_maxtrans: drop test_i, removed on devel

5d78989

arjunsv98 force-pushed the fix/demux-maxtrans-249 branch from dc46d0c to 5d78989 Compare August 11, 2026 20:15

arjunsv98 requested a review from imchenwu August 13, 2026 13:39

imchenwu requested changes Aug 21, 2026

View reviewed changes

arjunsv98 requested a review from imchenwu August 24, 2026 03:58

imchenwu approved these changes Aug 25, 2026

View reviewed changes

nikgiu force-pushed the devel branch from 0ccc838 to aa799ec Compare September 8, 2026 13:15

nikgiu changed the base branch from devel to master September 9, 2026 06:09

imchenwu added 2 commits September 10, 2026 17:10

Merge branch 'master' into fix/demux-maxtrans-249

e09bec2

axi_demux: remove overly strict push-limit assertion

864a7c7

A stalled AR remains locked while an ATOP can fill its ID counter. The pending AR must still complete, so pushing at MaxTrans is valid in this case.

imchenwu merged commit da8793b into pulp-platform:master Sep 10, 2026

imchenwu linked an issue Sep 10, 2026 that may be closed by this pull request

"MaxTrans" defined incorrectly in the module documentation #336

Closed

Navigation Menu

axi_demux: enforce exactly MaxTrans in-flight transactions (fixes #249) - #440

axi_demux: enforce exactly MaxTrans in-flight transactions (fixes #249)#440
imchenwu merged 12 commits into
pulp-platform:masterfrom
arjunsv98:fix/demux-maxtrans-249

arjunsv98 commented Aug 2, 2026 •

edited

Loading

Uh oh!

imchenwu left a comment

Uh oh!

imchenwu Aug 10, 2026

Uh oh!

arjunsv98 Aug 11, 2026

Uh oh!

Uh oh!

arjunsv98 commented Aug 11, 2026

Uh oh!

imchenwu commented Aug 18, 2026

Uh oh!

arjunsv98 commented Aug 21, 2026

Uh oh!

imchenwu Aug 21, 2026

Uh oh!

arjunsv98 Aug 24, 2026

Uh oh!

imchenwu Aug 21, 2026

Uh oh!

Uh oh!

imchenwu left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	localparam int unsigned IdCounterWidth = cc_pkg::idx_width(MaxTrans + 1);
	localparam int unsigned IdCounterWidth = cc_pkg::cnt_width(MaxTrans);

Conversation

arjunsv98 commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Verification

Uh oh!

imchenwu left a comment

Choose a reason for hiding this comment

Uh oh!

imchenwu Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

arjunsv98 Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arjunsv98 commented Aug 11, 2026

Uh oh!

imchenwu commented Aug 18, 2026

Uh oh!

arjunsv98 commented Aug 21, 2026

Uh oh!

imchenwu Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

arjunsv98 Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

imchenwu Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imchenwu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arjunsv98 commented Aug 2, 2026 •

edited

Loading