Add ECM support, improve stream handling, and add the committed_padder component. The RGMII driver now handles the AXI4-Stream interface, with various stream tools added and updated. #2

Thibault-Trombert · 2025-11-07T15:42:46Z

No description provided.

nipo

Overall assessment: The committed_padder component is well-designed and thoroughly tested (excellent work on the test coverage!). However, there are several critical issues that must be addressed before this can be merged.

Must Fix:

Type mismatch in committed_padder (committed_req vs committed_req_t)
FIFO generate condition logic needs verification
Process sensitivity list issue in packet_counter_proc
Test case 6 behavior discrepancy (cancelled frames + padding)

Needs Justification:

Severity downgrade in width_adapter (could hide bugs)
Impure function marking without clear side effects

Documentation:

Document ready_toggle feature purpose
Fix typo in comment
Consider renaming wwv function for clarity

Please address these issues and I'll be happy to re-review.

nipo · 2025-11-12T10:38:21Z

lib/nsl_bnoc/committed/committed_padder.vhd

+    reset_n_i   : in  std_ulogic;
+    clock_i     : in  std_ulogic;
+
+    in_i   : in  committed_req;


Type mismatch: should be committed_req_t not committed_req (and similarly for committed_ack_t). The package definition uses committed_req_t as the type name.

nipo · 2025-11-12T10:38:28Z

lib/nsl_amba/stream_fifo/axi4_stream_fifo.vhd

-      out_available_o => out_available_o,
-      in_free_o => in_free_o
-      );
+  generate_fifo_homogenous: if clock_i'length /= 1 generate


This condition looks problematic. Should this be checking clock_count_c /= 1 instead of clock_i'length /= 1? The generic clock_count_c indicates the intended number of clocks, but clock_i'length may not behave as expected here.

nipo · 2025-11-12T10:38:31Z

lib/nsl_amba/stream_fifo/axi4_stream_fifo.vhd

+    out_available_o <= to_integer(out_available);
+    in_free_o <= to_integer(in_free);
+
+    packet_counter_proc: process(reset_n_i, clock_i) is


The sensitivity list includes reset_n_i but the reset is checked inside the rising_edge block (line 106-108). This should be a synchronous-reset-only process with just process(clock_i) or the reset should be moved to an async section.

nipo · 2025-11-12T10:38:34Z

lib/nsl_amba/axi4_stream/axi4_stream_width_adapter.vhd

    assert in_config_c.has_keep = out_config_c.has_keep
      report "In/out keep do not match"
-      severity failure;
+      severity warning;


Why downgrade this from failure to warning? Mismatched keep signals between input and output could lead to data corruption. This seems like it should remain a fatal error. Please provide justification for this change or revert it.

nipo · 2025-11-12T10:38:36Z

lib/nsl_amba/axi4_stream/axi4_stream.pkg.vhd


  -- Factory function for a frame
-  function frame(
+impure function frame(


Why is this function now marked impure? There's no obvious state modification or side effects in the implementation. Impure functions can behave differently in synthesis vs simulation. Please document the reason for this change.

nipo · 2025-11-12T10:38:38Z

lib/nsl_amba/axi4_stream/axi4_stream.pkg.vhd

                           variable user : out std_ulogic_vector;
-                           variable dest : out std_ulogic_vector);
+                           variable dest : out std_ulogic_vector;
+                           constant ready_toggle : boolean := false);


What is the use case for ready_toggle? The PRBS-based ready toggling seems simulation-specific but isn't documented. Please add a comment explaining when and why this feature should be used.

nipo · 2025-11-12T10:38:40Z

lib/nsl_usb/descriptor/descriptor.pkg.vhd

+    return to_le(resize(n, 32));
+  end function;
+
+  function wwv(n: integer range 0 to 65535  )


This function takes a 16-bit integer but returns 32 bits (via resize to 32). Is this intentional? For ECM, the statistics field is indeed 32-bit, but the function name and parameter suggest 16-bit. Consider renaming to dwv (dword value) or adjusting the parameter range to match the 32-bit output.

nipo · 2025-11-12T10:38:41Z

lib/nsl_inet/func/func.pkg.vhd

    generic(
      clock_hz_c : natural;

+      -- Whether l1 has FCS appended ot frames


Typo: "appended ot frames" should be "appended to frames"

nipo · 2025-11-12T10:38:43Z

tests/bnoc/committed_padder/src/tb.vhd

+                    out_s.req, out_s.ack, clock_s,
+                    from_hex("ffffffffffffffff"), true, LOG_LEVEL_FATAL,
+                    1, 2);
+


This comment says "3 data + 5 padding bytes with cancel" but according to the implementation (committed_padder.vhd lines 117-118), cancelled frames skip padding and exit early. So this should actually be "3 data bytes + cancel (no padding)". Please verify the expected behavior and update either the test or the implementation.

Add a module that pads committed frames to a minimum size by appending a constant padding byte. Frames already meeting or exceeding the minimum size are passed through unchanged. Configuration: - min_size_c: Minimum frame size (positive generic) - padding_byte_c: Padding byte value (default x"00") Implementation uses a 2-deep FIFO to break combinatorial paths between input and output. The output stage maintains a down-counter initialized to min_size_c, decremented for each data byte output. When the input frame ends, padding is added if counter hasn't reached zero before outputting the commit/cancel status. Includes comprehensive testbench covering: - Short frames (require padding) - Exact size frames (no padding) - Long frames (no padding) - Empty frames (full padding) - Both commit and cancel status handling tests/bnoc/committed_padder: fix on comments.

…for P&R when header length is 0.

…cs is forwarded. nsl_inet/ethernet: ethernet host generics to specify whether or not fcs is forwarded. lib/nsl_inet/func: typo.

…namic reset.

…cancellable FIFO depending on the number of input clocks. When a single clock is provided, the FIFO tracks the number of available stored packets.

…h AXI4-Stream instead of the BNOC-specific committed type.

… in pin can alse be used to force CRC invalid.

…in_size_c' bytes.

…ackets between two independant clock domains. No back-pressure on the input interface: if the fifo in overrun the entire packet is dropped. t

…ived. If the packet is smaller, it is just outputted.

…aster_vector in order to use it as local fifo.

… match.

…e item at a specified index.

Cleaning. tests/amba: less packets played.

… drop management. Renaming of axi4_stream_async_packet_drop_fifo into axi4_stream_atomic_cancellable

…idth > 1.

…r of data.

Thibault-Trombert changed the title ~~I didn't click, mb~~ Add ECM support, stream handling improvements, and committed_padder component. Nov 10, 2025

nipo requested changes Nov 12, 2025

View reviewed changes

Thibault-Trombert force-pushed the master branch 4 times, most recently from b2bdf05 to cad2ce2 Compare December 8, 2025 09:46

Thibault-Trombert force-pushed the master branch 4 times, most recently from b0c132d to 3693ac2 Compare December 9, 2025 14:53

nipo and others added 19 commits December 29, 2025 14:07

lib/nsl_amba: frame() function are impure due to simulation time use.

799cd86

lib/nsl_amba: timeout inerface adding for simulation purposes.

52a94fe

nsl_bnoc/packetizer x nsl_data/bytestream: dereference_safe function …

27ce791

…for P&R when header length is 0.

nsl_inet/ethernet: ethernet host generics to specify whether or not f…

77422d3

…cs is forwarded. nsl_inet/ethernet: ethernet host generics to specify whether or not fcs is forwarded. lib/nsl_inet/func: typo.

nsl_amba/stream_fifo: case added to avoid out of bound issues when dy…

4726aa9

…namic reset.

nsl_usb/descriptor: handling of ECM Class descriptor.

1821eec

nsl_usb/usb: adding of bulk super speed max packet size.

7ee6589

lib/nsl_amba: AXI Stream FIFO now instantiates either homogeneous or …

a4023e4

…cancellable FIFO depending on the number of input clocks. When a single clock is provided, the FIFO tracks the number of available stored packets.

RGMII driver rework: the RGMII driver was redesigned to interface wit…

93a7efa

…h AXI4-Stream instead of the BNOC-specific committed type.

nsl_amba/stream_crc: able to validate packet CRC in-stream. The error…

7a7b4d6

… in pin can alse be used to force CRC invalid.

nsl_amba/stream_traffic : Ensures each outgoing packet is at least 'm…

a68e6a9

…in_size_c' bytes.

nsl_amba/stream_fifo: output only full AXI4-Stream packets.

4948e37

nsl_amba/stream_fifo: drops AXI4-Stream packets if in error is asserted.

ad9dc3f

nsl_amba/stream_fifo: Asynchronous AXI4-stream FIFO. Transfers full p…

e88b2ed

…ackets between two independant clock domains. No back-pressure on the input interface: if the fifo in overrun the entire packet is dropped. t

nsl_amba/axi4_stream: output data once prefill_count_c bytes are rece…

afa1edf

…ived. If the packet is smaller, it is just outputted.

nsl_amba/stream_fifo: fix on depth_c range.

b3b662d

nsl_amba/axi4_stream: config array type for simulation.

01102b2

nsl_amba/axi4_stream: shift function and '&' operand definition for m…

ec5749f

…aster_vector in order to use it as local fifo.

Thibault-Trombert added 12 commits December 29, 2025 14:07

nsl_amba/axi4_stream: print the queue size.

038a356

nsl_amba/axi4_stream: frame_clone procedure declaration for simulation.

8f718d9

nsl_amba/axi4_stream: send a frame on master queue and expect frm2 to…

fbf3290

… match.

nsl_amba/axi4_stream: procedures to delete last item of a queue of th…

65a082d

…e item at a specified index.

tests/bnoc: adding of committed_padder.

d6c7b3f

tests/amba: New tb using pkt generator.

d5d5b2b

Cleaning. tests/amba: less packets played.

lib/nsl_amba: fix on ready handling, resync pointer transimission and…

3694354

… drop management. Renaming of axi4_stream_async_packet_drop_fifo into axi4_stream_atomic_cancellable

lib/nsl_amba: has_keep and has_strone assertion only matter if data_w…

5029fe6

…idth > 1.

lib/nsl_amba: pkt_genertor has now inter gap packets if needed.

41dd8ab

lib/nsl_amba: fifo clean can now outputted in and out available numbe…

6240598

…r of data.

tests/amba: new stream crc using prbs random data and queue.

6150712

nsl_amba/stream_fifo:fix on reset, mix async/sync logic.

fa3f74e

Thibault-Trombert force-pushed the master branch from 3693ac2 to fa3f74e Compare December 29, 2025 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add ECM support, improve stream handling, and add the committed_padder component. The RGMII driver now handles the AXI4-Stream interface, with various stream tools added and updated. #2

Add ECM support, improve stream handling, and add the committed_padder component. The RGMII driver now handles the AXI4-Stream interface, with various stream tools added and updated. #2

Uh oh!

Thibault-Trombert commented Nov 7, 2025

Uh oh!

nipo left a comment

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

nipo Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add ECM support, improve stream handling, and add the committed_padder component. The RGMII driver now handles the AXI4-Stream interface, with various stream tools added and updated. #2

Are you sure you want to change the base?

Add ECM support, improve stream handling, and add the committed_padder component. The RGMII driver now handles the AXI4-Stream interface, with various stream tools added and updated. #2

Uh oh!

Conversation

Thibault-Trombert commented Nov 7, 2025

Uh oh!

nipo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants