Skip to content

new cache code for rc1 #731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

RicoLi424
Copy link

Copy link
Contributor

@tommydcjung tommydcjung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay... some preliminary comments made.

@@ -31,7 +32,7 @@ module bsg_wormhole_to_cache_dma_fanout

, parameter wh_ready_and_link_sif_width_lp=`bsg_ready_and_link_sif_width(wh_flit_width_p)
, parameter wh_then_ready_link_sif_width_lp=`bsg_then_ready_link_sif_width(wh_flit_width_p)
, parameter dma_pkt_width_lp=`bsg_cache_dma_pkt_width(dma_addr_width_p, dma_mask_width_p)
, parameter dma_pkt_width_lp=`bsg_cache_dma_pkt_width(dma_addr_width_p, dma_mask_width_p, dma_ways_p)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any more changes/testing required here?

@@ -372,7 +430,7 @@ module bsg_cache_miss

// If it's not AINV, and the chosen set is dirty and valid, evict the
// block.
miss_state_n = (~decode_v_i.ainv_op & stat_info_in.dirty[flush_way_n] & valid_v_i[flush_way_n])
miss_state_n = (~decode_v_i.ainv_op & ((notification_en_i & decode_v_i.aflinv_op) | stat_info_in.dirty[flush_way_n]) & valid_v_i[flush_way_n])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what functionality is this adding and when is it needed? Flushing clean data on AFL when notification_en_i is enabled?

Comment on lines 356 to 360
assign io_read_in_progress_n = ((send_state_r == SEND_ADDR) & ~dma_pkt_lo.write_not_read & dma_pkt_lo.uncached_op & wh_link_sif_in.ready_and_rev)
? 1'b1
: (((recv_state_r == RECV_IO_DATA) & return_fifo_yumi_li)
? 1'b0
: io_read_in_progress_r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be in the alway_comb block above?

// for convinience, we use the unused field to store the way_id, evict_pending bit, uncached_op and write_validate bit here
// instead of splitting them into separate new fields
// assign header_flit.unused = {'0, dma_pkt_lo.write_validate, dma_pkt_lo.uncached_op, dma_pkt_lo.evict_pending, dma_pkt_lo.way_id};
assign header_flit.unused = {'0, dma_pkt_lo.evict_pending, dma_pkt_lo.way_id};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when does write_validate or uncached_op get set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better to make into separate fields so that old modules break noisily instead silently?

@taylor-bsg
Copy link
Contributor

@tommydcjung @RicoLi424 @farzamgl @dpetrisko any convergence
on this pull regress? we need to converge soon for tapeout

@tommydcjung
Copy link
Contributor

@RicoLi424 where are the changes to the current testbenches and unit-testing for the new features? How do I know these changes are tested?

@RicoLi424 RicoLi424 requested review from tommydcjung and farzamgl June 3, 2025 12:09
@tommydcjung
Copy link
Contributor

@RicoLi424

Error-[NOODV] No override or default value
basejump_stl/bsg_misc/bsg_arb_round_robin.sv, 117
  Parameter 'width_p' has no default value or override for this instance. 

@tommydcjung
Copy link
Contributor

@RicoLi424 please update the existing benchmarks as well to pass with the new features disabled.

@Yuan-Mao
Copy link
Contributor

Yuan-Mao commented Jun 3, 2025

Hey @RicoLi424. One minor thing: I saw some comment blocks in this PR (for example here: https://github.com/RicoLi424/basejump_stl/blob/mini_rebase_new/bsg_cache/bsg_cache.svh#L69). If they are not necessary anymore, maybe we could delete them.

@Yuan-Mao
Copy link
Contributor

Yuan-Mao commented Jun 3, 2025

Note that I have put a PR at your local basejump repo. (RicoLi424#1)

Redirect uncached IO to separate wh dest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants