Simplify Wally tracer CSRs

[jordancarlin]

Use a macro so that CSRs can be connected with a single line instead of needing to modify many locations. Does not currently work for PMP registers because of the loop, so they are left as they are.

@rosethompson

[davidharrishmc]

I really like this. Let’s see if we can encapsulate the state even further. For example, CSR_W is only used in rvvi_csrwb, and CSRArray is local too so I think we could change that to assign rvvi.csr[0][0][addr] = valid ? val : CSRArrayOld[addr]; assign rvvi.csr_wb[0][0][addr] = (rvvi.csr[0][0][addr] != CSRArrayOld[addr]); Can we declare unique local variables in the macro so that CSRArray and CSRArrayOld are also encapsulated? Maybe we can use a “prev" function for a flip-flop to get assign rvvi.csr_wb[0][0][addr] = (rvvi.csr[0][0][addr] != prev(rvvi.csr[0][0][addr]);

…

On Jan 29, 2025, at 8:25 AM, Jordan Carlin ***@***.***> wrote: Use a macro so that CSRs can be connected with a single line instead of needing to modify many locations. Does not currently work for PMP registers because of the loop, so they are left as they are. @rosethompson <https://github.com/rosethompson> You can view, comment on, or merge this pull request online at: #1255 Commit Summary eb38d07 <eb38d07> Use macro for CSR connections in wallyTracer a06a91f <a06a91f> Merge branch 'main' of https://github.com/openhwgroup/cvw into wallyTracer File Changes (1 file <https://github.com/openhwgroup/cvw/pull/1255/files>) M testbench/common/wallyTracer.sv <https://github.com/openhwgroup/cvw/pull/1255/files#diff-9dc525a9df4effd5232fc38c522b97c32febc1d4b88f3fceefa39c5d3bea421e> (574) Patch Links: https://github.com/openhwgroup/cvw/pull/1255.patch https://github.com/openhwgroup/cvw/pull/1255.diff — Reply to this email directly, view it on GitHub <#1255>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AR4AA33RXPZOPFRN2FK2HH32ND6H3AVCNFSM6AAAAABWDIJN22VHI2DSMVQWIX3LMV43ASLTON2WKOZSHAYTQNRYGE2DSNI>. You are receiving this because you are subscribed to this thread.

[jordancarlin]

The pmp CSRs make this more complicated because I haven't been able to get them working using the macros (can't have an always_comb block inside a for loop). I'll see if we can at least simplify the macro though even if it doesn't completely eliminate the extra arrays.

[davidharrishmc]

If you replace the always_comb with ?: and the flop with a function, maybe there is a way to handle the loop?

…

On Jan 29, 2025, at 9:30 AM, Jordan Carlin ***@***.***> wrote: The pmp CSRs make this more complicated because I haven't been able to get them working using the macros (can't have an always_comb block inside a for loop). I'll see if we can at least simplify the macro though even if it doesn't completely eliminate the extra arrays. — Reply to this email directly, view it on GitHub <#1255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AR4AA324V5YGRPBKEPBNBC32NEF2RAVCNFSM6AAAAABWDIJN22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRSGM4TKNJWGQ>. You are receiving this because you commented.

[jordancarlin]

Had to do a few strange things, but I was able to get the PMP CSRs using the macros. Down to just a few more issues at this point:

• The PMPCFG register loop is going out of bounds of the PMPCFG_ARRAY_REGW array. Need to figure out how to modify the loop to get that to match.
• I had to comment out the PRINT_CSRS option because we no longer have an array of all of the old CSR values to compare against. I still think this is better overall, but need to figure out a solution if we still want to be able to print CSR changes. It might just need to be included in the macro, but I'd rather keep the macro simple if possible.

[davidharrishmc]

Do the CSRs properly default to a non-x value if not connected?

genvar is not part of our usual SystemVerilog style for generate loops. Is it necessary in this code?

We haven't been printing CSRs anyway, so fine to remove.

[rosethompson]

I'm not a big fan of macros. The tend to obfuscate debugging. Could we turn the macro into a module?

For the pmp config can you just divide P.PMP_ENTRIES by 8 since there are always config's packed into a register for XLEN=64?

I originally used PRINT_CSRS only for debugging the tracer. Occasionally this is useful when we add features. If we want to add it, we could have a 2d array of valid CSR addresses and have a loop iterate over that loop and then print those addresses. However I'm not entirely sure how valuable this is now.

[jordancarlin]

@davidharrishmc I was getting errors from questa without the genvar, which is why I added it. We already had the CSRs gated on P.ZICSR and didn't have any assignment in the else case, so I'm assuming that has been working. If not that can be resolved when we get configurations with reduced sets of CSRs actually running.

@rosethompson I attempted to use a module and it seems to get upset about the way the RVVI trace is getting passed around, I can look further into it, but in some ways a macro might be a better fit here. It seems slightly excessive to be instantiating a module for every single CSR.

Not sure exactly what you mean by your suggestion for pmp config. It also needs to work with both XLEN=32 and 64.

I'll go ahead and leave PRINT_CSRS commented out and we can always add it back in later if it proves necessary.

[rosethompson]

I misread your pmp config code while looking for the source of your index out of bounds error. I saw the loop limit was PMP_ENTIRES but didn't realize you were incrementing by 4 or 8.