Generic PMA agent for multiple memory interfaces (AXI / OBI) #2514
Conversation
|
Thanks for this PR @abdelhak-chkirid. Hi @silabs-hfegran, I know you did not write this code, but AFAIK, SiLabs is the only user of the PMA Agent. Can you (or your deligate) add a review to this PR? Thanks. |
There was a problem hiding this comment.
In general it looks good to me, though with one part that looks functionally incorrect (or redundant?), see my review comment.
One improvement I think that we could benefit from would be to introduce more type parameterization to avoid duplicating files and code for AXI/OBI, as most of the code is fairly similar. That would reduce the maintenance effort.
| `uvm_fatal("CFG", "Configuration does not contain valid memory interface") | ||
| end | ||
| end | ||
| for (int i = 0; i < regions.size(); i++) begin |
There was a problem hiding this comment.
This looks incorrect to me, why is this code duplicated? (the code inside if (memory_intf == UVMA_PMA_AXI_INTF) is repeated below)
There was a problem hiding this comment.
thanks for the review , i think now it's ok
|
@silabs-hfegran the duplicated part of code is deleted now , can you check please . |
|
Pinging @silabs-hfegran |
| string name | ||
| ) with function sample(uvma_pma_mon_trn_c trn, | ||
| uvma_core_cntrl_pma_region_c pma_region); | ||
| string name , bit is_main , bit support_atomic , bit support_cacheable , bit support_buff ) |
There was a problem hiding this comment.
I would prefer this to remain a type that is overridable in the core-specific code.
E.g. we have the cv32e40s which overrides the atomic bit to be an integrity bit instead, and thus explicitly naming it here makes the code less flexible and readable for other than this specific case.
This applies to other code in these files as well, not only on this line.
Modified CV32E40 PMA agent to be generic for multiple memory interfaces such as AXI and OBI