Apply fixes to coalesced explicit + SPM flush handling from main + Reduce Diff - #42
midnightveil wants to merge 9 commits into
Conversation
|
I sent an email to @ezelioli the other day asking about this; as the testbench I'd also be willing the spend the effort cleaning up the gwaihir changes so that it is part of mainline, instead of living in a branch. Specifically, merging the two variants of |
This probably misses some, these were generated with the 'typos' tool.
f3e5a5a to
6f96228
Compare
Hey @midnightveil , thanks for your efforts! This seems good to me but since I am not the original developer of the extension and did not directly push to the gwaihir branch (I did most of the rebase in the flamingo branch though, which the gwaihir branch cherry-picked afaik), I need to spend some time to review this properly. However, since I am on holidays and with limited connectivity, it will take me a while. Will try to come back to you asap after I am back next week |
|
No worries! If there's someone more relevant to CC in here it would be much appreciated :) (Same with other PRs) I'm working on trying to figure out what's going on with the test bench that is partitioned , so you might see some more ones. addendum, my earlier comment:
this is unfortunately not easy to do, unless we're okay with using the |
|
Hi @midnightveil , similar to @ezelioli I am just integrating this feature in a larger project and I am not super familiar with all the details. Nevertheless, the changes in this PR seem okay to me. I would just ask you to choose as target branch |
|
Done. |
* ci: Check version metadata consistency * ci: Simplify version consistency check
45a2ea1 to
bd5b1d6
Compare
|
(Pushed to fix a compilation error when the branch is based off gwaihir-common-cells-v2 due to commit a457a9c) |
* config: clearly delineate software-writable vs software-ro This does not change the behaviour. Also, because the HW we does not affect whether a RW register can be written to be SW (and in fact SW takes higher priority over HW if both try), let's just make all the _en bits default-disabled and only enable when needed. * config: use 'saved' value of cfg_spm sw register in 'spm_lock_q' Otherwise, software can manipulate the value of the cfg_spm register and cause the hardware to indicate that a way is flushed via the `conf_regs_o.flushed` register (then used by other parts of the hardware) when the actual way was never flushed and contains dirty data. This is under the assumption that software writes to the configuration registers after CommitCfg. * config: read the software-input CfgFlush register only once Previously, software could write to the register and every time we went through FsmInitFlush it would update the to_flush_d list. I believe that it was *fine*, in the sense that the list of thought to be 'flushed' ways would remain coherent with what was actually flushed no matter what software did. However, this is quite hard to reason about, and there's basically no reason why you would ever want to do this: you also don't expect that anything done after writing the CommitCfg register would take effect. * config: cleanup duplicated loop condition Previously, the loop condition for the FsmInitFlush case was duplicated in the FsmEndFlush case. This meant an extra set of the same code needed to be maintained. The tradeoff is that we take one extra cycle to finish clearing the flush on the last iteration. However, we've spent some large amount of cycles flushing all the cache lines to memory, so I don't think this is a big difference.
Where the diff was a non-functional change. This makes it easier to
tell how things have changed.
Compared via the git command:
git diff --no-index --ignore-all-space \
src/axi_llc_config_no_pat.sv src/axi_llc_config_pat.sv
And:
git diff master:src/axi_llc_config.sv \
gwaihir:src/axi_llc_config_no_pat.sv
We're already passing around this `llc_cfg_t` struct containing parameters such as `NumBlocks`/`NumLines`, so we can re-use it for the `CachePartition`-enabled bit, `MaxPartition`, and `RemapHash`, which removes a lot of changes to module ports/params just to pass these values around. We leave them added to the `axi_llc_config_pat` module as it uses them several times.
VCS reports these warnings when simulating: Warning-[SIOB] Select index out of bounds ./src/axi_llc_top.sv, 466 "to_isolate_req.aw.user[(AxiUserIdMsb - AxiUserIdLsb):0]" The select index is out of declared bounds : [0:0]. In module instance : i_axi_llc_top_raw In module : axi_llc_top. Warning-[SIOB] Select index out of bounds ./src/axi_llc_top.sv, 466 "slv_req_i.aw.user[AxiUserIdMsb:AxiUserIdLsb]" The select index is out of declared bounds : [0:0]. In module instance : i_axi_llc_top_raw In module : axi_llc_top. Warning-[SIOB] Select index out of bounds ./src/axi_llc_top.sv, 468 "to_isolate_req.ar.user[(AxiUserIdMsb - AxiUserIdLsb):0]" The select index is out of declared bounds : [0:0]. In module instance : i_axi_llc_top_raw In module : axi_llc_top. Warning-[SIOB] Select index out of bounds ./src/axi_llc_top.sv, 468 "slv_req_i.ar.user[AxiUserIdMsb:AxiUserIdLsb]" The select index is out of declared bounds : [0:0]. In module instance : i_axi_llc_top_raw In module : axi_llc_top.
bd5b1d6 to
dc8ebdd
Compare
|
I've force-pushed after remerging master and reducing the diff with #44's changes. I've not applied any hardening logic to the partition-flush registers in this PR, so the partition-flush logic should remain identical before/after this; the only change is the logic that's in the non-partitioned flush. |
This branch has not been deployed
4 participants
The goal of this changes is to minimise the difference between the partitionable-LLC on the Gwaihir branch and the mainline one.
This mostly focuses on
axi_llc_config_pat.sv.Reviewing these changes by themselves don't really make sense; what I was looking at when making these changes is
git diff --no-index --ignore-all-space src/axi_llc_config_no_pat.sv src/axi_llc_config_pat.svand
git diff master:src/axi_llc_config.sv src/axi_llc_config_no_pat.sv.Where possible, (in the diff to master) I avoid modifying existing lines and instead add new lines. This should hopefully minimise conflicts in future when rebasing the gwaihir branch.