Should probably be updated to fifo_v3.

I am working on "stream_xbar.sv" to verify it formally. I found one bug that when two different sel_i port have same destination then ready_o of these port can't be high at a same time. For example, if "sel_i[0] = 2" and "sel_i[3] = 2" then "ready_o[0]" and "ready_o[3]" can't be high at same time.
Is this a bug or design intent?

Hi I am a french intern in computer architecture at Université paris saclay and I work on the implementation of IBEX in a low power system.

I just started system verilog a week ago, so my understanding of it is quite poor. If the issue is not correct do not hesitate to close the issue.

For context : I recently changed my computer so I re-installed verilator from sources. Doing So broke many pieces of my system.
I am now getting errors like this when compiling:

 src/pulp-platform.org__common_cells_1.28.0/src/lzc.sv:23:51: Static access to non-static task/function 'idx_width'

      parameter int unsigned CNT_WIDTH = cf_math_pkg::idx_width(WIDTH) 

I also get the same errors in the tcdm interconnect package.

At first I though I did something wrong but after reading the reference of system verilog I found that the :: is indeed reserved for static functions. And so the call to idx_width which is labelled as automatic is indeed illegal. (or at least that's what I am thinking with my small understanding of the language)

I just discovered there has been a very recent change in verilator : verilator/verilator#4072
My understanding is that calls to automatic functions need to be hierarchical for verilator to accept them.

I will just checkout to a branch of verilator that doesn't have the patch and see where it goes.

The clock divider module clk_div.sv does not work:

If counter_q reaches RATIO-1 it toggles clk_q once and then stops counting.

Some files contain multiple modules that should live in their own files.
An incomplete list for ticking off, to be completed and worked off:

• graycode.sv - Contains binary_to_gray and gray_to_binary

popcount.sv contradicts itself in the description and this commit bf4db7c whether the case INPUT_WIDTH=1 is supported.

Furthermore, it seems like the bf4db7c doesn't properly add full support for INPUT_WIDTH=1 since

logic [PopcountWidth-2:0]         left_child_result, right_child_result;

(from here https://github.com/pulp-platform/common_cells/blob/master/src/popcount.sv#L30)

becomes negative causing at least VCS to crash. A simple hacky fix is to change the -2 to -1 but I'm not sure if that works out well.

Maybe @meggiman can you comment on this (since you seem to know the most being the designer of this module) ?

@alex96295 fyi

in lzc module the comment says:
/// If the input does not contain a zero, empty_o is asserted. Additionally cnt_o contains
/// the maximum number of zeros - 1.
if the WIDTH is power of 2, it works fine

but if the WIDTH is not the power of 2，i.e WIDTH is 29
when in_i is empty, cnt_o is zero instead of 29-1

because in the rtl
// if index is out of range
if (unsigned'(k) * 2 > WIDTH - 1) begin : g_out_of_range
assign sel_nodes[2 ** level - 1 + k] = 1'b0;
assign index_nodes[2 ** level - 1 + k] = '0; // why not assign index_nodes[2 ** level - 1 + k] = WIDTH-1;
end

Title

@zarubaf: Is there a reason pipe_reg_simple.sv is not in Bender.yml? And before putting it there (maybe in the 1.10 release?), would shift_reg not be a more appropriate name?

What's the stance on the usage of register macros from registers.svh in general and especially within this repo itself?

• Are we advocating for "safe" use of always_ff blocks by only using them through these macros?
• If so, should we replace always_ff blocks with the appropriate macros for IPs within this repo?

Hi,

it seems that most of the modules in this repository exclude assertions when simulating with Verilator. Typically, the assertions are surrounded by an ifndef - endif block that excludes them in Verilator, such as for instance in the fifo_v3:

Verilator has had support for assert properties since at least version 3.922 released in 2018 and support for the implication operator has been added in version 4.026 released in 2020. AFAIK the initial block is a bit more of a recent addition, but has also been supported for a while now.

Is there a particular reason why assertions are excluded in Verilator (e.g., this repo must support some version of Verilator that cannot handle assert properties)? Could this restriction be lifted given the time that Verilator already has had support for this?

Is there a corresponding issue in the fpnew repo? As far as I know, this is not an issue with Ara, but rather on how the fpnew handles the vector execution of those instructions.

Originally posted by @suehtamacv in pulp-platform/ara#4 (comment)

The PTW currently has an exclusive readport into the dcache. This is overprovisioning, as PTWs should not occur frequently. Sharing the read port with the load unit should lead to lower HW complexity and better timing in the dcache.

assrt2_stable_idx_o: // idx_o remains stable when there is no ready_i and valid_o is high
assert property (valid_o[i] && !ready_i[i] |=> $stable(idx_o[i]));

assrt3_stable_data_o: // data_o remains stable when there is no ready_i and valid_o is high
assert property (valid_o[i] && !ready_i[i] |=> $stable(data_o[i]));

These two assertions fail when OutSpillReg is set to 0. Shouldn't it be stable?

Our new generation FIFO does not use explicit clock gating anymore, hence it will be save to remove the testmode signal.

When elaborating the following problem occurs in DC. We should fix this pls.

Warning:  ../../src/fpnew/src/common_cells/src/lzc.sv:52: signed to unsigned conversion occurs. (VER-318)
Warning:  ../../src/fpnew/src/common_cells/src/lzc.sv:58: signed to unsigned conversion occurs. (VER-318)
Warning:  ../../src/fpnew/src/common_cells/src/lzc.sv:64: signed to unsigned conversion occurs. (VER-318)
Warning:  ../../src/fpnew/src/common_cells/src/lzc.sv:69: signed to unsigned conversion occurs. (VER-318)
Warning:  ../../src/fpnew/src/common_cells/src/lzc.sv:58: signed to unsigned conversion occurs. (VER-318)

In the below assignment, the RHS being a genvar is 32-bits wide, while the LHS which is parameterized and need not be always of 32-bits wide. Thus the compiler throws a width mismatch error.

Can the code be modified to be generic enough with the assignment like this,
assign tmp_i = i[BIN_WIDTH-1:0];

Thanks for this great job, but I have get an error message when re-using some module with the fusesoc tool in a simple test project. Partial content of my core file is as bellow:

filesets:
  rtl:
    depend:
      - ">=pulp-platform.org::common_cells:1.26.0"
    files:
      - rtl/blinky_pkg.sv
      - rtl/blinky.sv
      - rtl/step_counter.sv
    file_type: systemVerilogSource

What I want is only the delta_counter module while as the whole library is added to my project, fusesoc can't find the assertions.svh file for stream_fifo_optimal_wrap module when run lint or simulation with verilator. The problem is resovled by adding the header file in the common_cells.core file as bellow:

CAPI=2:

name : pulp-platform.org::common_cells:1.26.0

filesets:
  rtl:
    files:
      - include/common_cells/registers.svh : {is_include_file : true, include_path : include}
      - include/common_cells/assertions.svh : {is_include_file : true, include_path : include}
      # Source files grouped in levels. Files in level 0 have no dependencies on files in this package.
      # Files in level 1 only depend on files in level 0, files in level 2 on files in levels 1 and 0,
      # etc. Files within a level are ordered alphabetically.
      - src/binary_to_gray.sv
      # ...

My question is :

1. whether the assertions.svh is excluded intentionally or not ?
2. and if the answer is yes, how to avoid this problem in other designs excepting modify the common_cells.core file locally ?

General Wishlist for our common-cells repository

• Generic cross bar fabric
• Weighted RR arbiter
• More LFSR implementations @msfschaffner
• Register file

This line

is lacking a v at the end.

The usage_o port of fifo_v3 has currently a width of $clog(DEPTH).
This causes the pointer to wrap around to all zeros, if the FIFO has a power of 2 DEPTH and is full. This is an issue for the following exapmle:

localparam int unsigned Depth     = 32'd16;
localparam logic [3:0]  Threshold = 4'hF;
logic [3:0] usage;
if (usage > threshold) begin
  /* do something */
end

This if statement would not trigger in this case.
This could be resolved by assigning the whole status_cnt_q to the usage_o port instead of the truncated one as is currently the case.

The workaround I am using currently is prepending the full_o signal to the ussage_o, however this causes jumps in this new usage pointer value when the FIFO is not configured to a power of 2 DEPTH.

Right now there are a couple of ad-hoc implementations of the rand_stream protocol. This issue keeps track of the unification of those different implementations into one supported rand_stream_* class. CC #100

For example:
We can set up an always block generating a new clk_o based on whether the counter is gated or a new value is requested. Then we can do something like:

clk_o <= (clk_gated_cnt != 3'b0) ? clk_div : ((div_i == clk_div) ? clk_div : new_clk_div);

We no longer need the old div_i register and therefore can eliminate the logic to check between old and new registers.

We should probably update this to fifo_v3.

Following modules are in the repo, however not defined in Bender.yml:
Level 0

• src/unread.sv
• src/edgepropagator.sv
• src/edgepropagatorrtx.sv
• src/exp_backoff.sv
• src/lfsr.sv
  These are the ones I am seeing at the moment, could be that there are more.

The edge_detect, edge_propagator, edge_propagator_rx, and sync modules depend on some deprecated and pulp_* modules defined outside of the repo. Maybe we should just drop these cells? Or rewrite them to not use manual clock gating.

Hey!
I have question about assertion in src/stream_xbar.sv:

  for (genvar i = 0; unsigned'(i) < NumInp; i++) begin : gen_sel_assertions
    assert property (@(posedge clk_i) (valid_i[i] |-> sel_i[i] < sel_oup_t'(NumOut))) else
        $fatal(1, "Non-existing output is selected!");
  end

Why do you use : sel_oup_t'(NumOut) if for example NumOut = 4 so sel_oup_t is logic[1:0] so result from this cast will be always 0. So (valid_i[i] |-> sel_i[i] < 0)

When downstream has backpressure, the flush function cannot work properly. so can we add the following sva property?
assupe property (
@(posedge clk_i) disable iff (!rst_ni)
flush_i |-> ready_i
) else $warning("Trying to flush when downstream not ready");

Some constants declared in the parameter list are not actual parameters that accept user inputs but require a fixed value (assigned as default value). A deviation from the required value can cause the module to behave in an undefined way. Such constants should not be a parameter but a localparam instead, to cause an elaboration-time error if the parameter value is changed at instantiation.

To be fixed:

• common_cells/src/onehot_to_bin.sv

  Line 15 in d55cba3

  parameter int unsigned BIN_WIDTH = $clog2(ONEHOT_WIDTH)

• check other modules in this repo

Hi Everyone,
I am running a system made up in part of the modules provided in this repo and the AXI one. Additionally, I use some of the IP provided by Xilinx.
To simulate everything, I run vsim with all Xilinx libraries mapped. Unfortunately, some of the Xilinx internal modules share names with the modules in this repo, for example the "counter". In that case, simulation fails to start with errors like

# ** Fatal: (vsim-3350) Generic "<protected>" has not been given a value.
#    Time: 0 fs  Iteration: 0  Protected: /tb_sort_stage/mem_delay/i_axi_delayer/i_stream_delay_aw/gen_delay/<protected> File: {MYXILINXINSTALL}/data/ip/xilinx/cic_compiler_v4_0/hdl/cic_compiler_v4_0_vh_rfs.vhd Line: 73

The easy fix to this in my case is to rename the counter module to something unique. I wonder if/how this could be fixed in a more principled approach?

For completeness, here is my sim script:

vlog +incdir+../src/axi/include +incdir+../src/common_cells/include ../src/axi/src/*_pkg.sv
vlog +incdir+../src/axi/include +incdir+../src/common_cells/include ../src/common_cells/src/*_pkg.sv
vlog +incdir+../src/axi/include +incdir+../src/common_cells/include ../src/axi/src/*.sv
vlog +incdir+../src/axi/include +incdir+../src/common_cells/include ../src/common_cells/src/*.sv
#Own
vlog +incdir+.. ../tb/tb.sv
vlog +incdir+.. -cover bcst ../src/dut.sv

LIBS=""
# Xilinx Specific Stuff
for d in /eda/technology/xilinx/questa_lib/*/ ; do
	name=$(basename $d);
	vmap $name $d
	LIBS="$LIBS -L $name"
done
#vcom the xilinx ip simulation files here


#-coverage 
vsim $LIBS -voptargs="+acc" -suppress 3009 -debugDB work.tb

the cast should be to idx_t instead of DataType
there should be no functional changes
https://github.com/pulp-platform/common_cells/blob/master/src/rr_arb_tree.sv#L284

Hi all,

I'm facing a problem using the stream_register required for using Pulp AXI. This module is included in the axi_atop_filter. Unfortunately I get the following error message during compilation with Synopsys Synplify :

@e: CD671: /.../commom_cells/src/stream_register.sv: 36:31:36:31| Expecting array type.

If I change the code from

    fifo_v2 #(
        .FALL_THROUGH   (1'b0),
        .DATA_WIDTH     ($size(T)),                     // <= change done here: $size() replaced with $bits()
        .DEPTH          (1),
        .dtype          (T)
    ) i_fifo (
        .clk_i          (clk_i),
        .rst_ni         (rst_ni),
        .flush_i        (clr_i),
        .testmode_i     (testmode_i),
        .full_o         (fifo_full),
        .empty_o        (fifo_empty),
        .alm_full_o     ( ),
        .alm_empty_o    ( ),
        .data_i         (data_i),
        .push_i         (valid_i & ~fifo_full),
        .data_o         (data_o),
        .pop_i          (ready_i & ~fifo_empty)
    );

to

    fifo_v2 #(
        .FALL_THROUGH   (1'b0),
        .DATA_WIDTH     ($bits(T)),                    // <= this is working
        .DEPTH          (1),
        .dtype          (T)
    ) i_fifo (
        .clk_i          (clk_i),
        .rst_ni         (rst_ni),
        .flush_i        (clr_i),
        .testmode_i     (testmode_i),
        .full_o         (fifo_full),
        .empty_o        (fifo_empty),
        .alm_full_o     ( ),
        .alm_empty_o    ( ),
        .data_i         (data_i),
        .push_i         (valid_i & ~fifo_full),
        .data_o         (data_o),
        .pop_i          (ready_i & ~fifo_empty)
    );

but I think then the module does not have the intended behaviour any more. Does anybody know what the exact error is because I don't get the problem.

I simulate the whole design with MentorGraphics Questa and there it works without any problem. So it seems to be a problem with the Synthesis tool.

Kind regards and thank you very much for helping!
Sebastian

As noted in #148, cb_filter is currently not compatible with Vivado's IP packager.

• Fix IPX compatibility
• Revert #148

I have developed a new version of shift_reg which has a valid flag for data to enable DC insert ICG for the internal datapath, the new version shift_reg is backward compatible with the older one and has a better power comuption. Is it welcomed to add it to this library?

Right now the synchronizers are behavioral descriptions which should be changed to instantiation of synch primitives as more sophisticated cell libraries usually have dedicated synchronization cells which give maximum resolution time for mitigating meta stability effects.

As noted in #138, id_queue currently does not support multidimensional arrays:

On line 369, we loop over all bits in data_t, which only works for 1D vectors or packed structs

There are several options to address this:

• Add an assertion or comment to disallow multidimensional arrays as data_t. These would need to be flattened outside the module.
• Use a width parameter instead of a type parameter for data_t (as suggested in #138)
• Flatten within id_queue

The use of $clog2 on a (generally non-static) function argument does not compile unless using the last year's [synthesis tool that shall not be named] or newer.

Found here:

This is bad since functions from cf_math_pkg were shoehorned into other common cells and subsequently massively break tool compatibility of IPs using common cells such as fpnew.

I'm not sure what the correct solution to the problem is because the use of clog2 is not illegal per se, just not generally synthesizable unless the argument is known static.

@meggiman I ran into another issue with the clock divider.

Following the description

// It is thus safe to statically tie the valid signal to logic high if
// we can guarantee, that the div_i value remains stable long enough (upper
// limit 2 output clock cycles).

I tied valid_i to 1 and connected div_i to a register which has a reset value 0.

That's when I noticed that after div_q = 0 is loaded, I'm not able to reconfigure the clock divider.

As far as I understood, it hangs in the WAIT_END_PERIOD state, because the following condition can never be satisfied.

Thanks @niwis for pointing out that this is indeed independent of the configuration while the divider is turned off, it just popped up checking #171.

This is a feature request to extend the stream_join IP or provide the extended functionality in a separate IP.

Currently, the stream_join IP enforces:

1. all of the streams to be joined together
2. all input handshakes to occur simultaneously, i.e. an input stream is granted ready only after all input streams are valid

Conversely, we seek the following features:

1. dynamically select a subset of streams to be joined
2. allow input streams to be granted independently of each other

Feature (1) is similar to the functionality stream_fork_dynamic provides over stream_fork. The stream_fork IP is "specular" in its functionality to stream_join.

• Make payload data type parametric in arbiter tree
• Mark other arbiter modules as deprecated and move them as they are superseded by rr_arb_tree:
  • stream_arbiter
  • rrarbiter
• Update README.md to reflect these changes

Several IPs internally instantiate a more generic version, leaving unneeded ports tied or unconnected. To ensure that only the required logic is instantiated, cross-boundary optimisations and constant propagation are needed, putting a lot of weight on the tools, which often do not meet expectations.

Therefore, restructure the IPs that currently rely on wrappers in a more tool-friendly manner (i.e. using parameters). These IPs are

• spill_reg
• stream_fifo
• shift_reg
• stream_arbiter
• rstgen
• stream_join
• edge_propagator

Rename the CDC modules from 2phase to the correct 4phase in the next breaking change release.

Testbenches for:

• prioarbiter
• plru_tree

I wrote this assertion and it is failing.

generate
    for (genvar inp = 0; inp < NumInp ; inp++ ) 
     
     assrt8_flush_with_ready_0: // when flush_i is asserted ready_o should be zero
     assert property(flush_i |-> ready_o[inp]==0);

  endgenerate

It passes for all other ready_o, but fails for ready_o[31].

I am working on verifying the stream crossbar using formal verification. There is too much confusion regarding the stream_xbar.sv file. Is there any proper documentation available for the stream_xbar.sv file?

We're making use of sync_wedge and thus sync in our design (specifically, it's used in the CLINT from Ariane). The naming used to be nicely prefixed (pulp_) to not have any conflicts, but the name sync is generic enough that we're getting naming conflicts when working with Amazon's F1 infrastructure.

We tried using pulp_sync instead but the interface changed so we had to tinker some and it means we need to keep a fork for little good reason.

Is there any chance you could rename this upstream or adopt a SV package to avoid such conflicts?

I get a synthesis warning that the index of head_tail_q is out of bounds.
Relevant code Line 247:

    // Registers
    for (genvar i = 0; i < CAPACITY; i++) begin: gen_ffs
        always_ff @(posedge clk_i, negedge rst_ni) begin
            if (!rst_ni) begin
                head_tail_q[i]      <= '{free: 1'b1, default: 'x};
                // Set free bit of linked data entries, all other bits are don't care.
                linked_data_q[i]    <= 'x;
                linked_data_q[i][0] <= 1'b1;
            end else begin
                head_tail_q[i]      <= head_tail_d[i];
                linked_data_q[i]    <= linked_data_d[i];
            end
        end
    end

Should it not be?:

for (genvar i = 0; i < HT_CAPACITY; i++) begin: gen_ffs

As HT_CAPACITY can be lower than CAPACITY in certain configurations:

localparam int HT_CAPACITY = (N_IDS <= CAPACITY) ? N_IDS : CAPACITY;

As far as I can see the registers get not accessed outside the range of HT_CAPACITY.