CSR Improvements and Cleanup #339
Conversation
There was a problem hiding this comment.
Just a random thought. Could we add a "check CSRs access work" type function which checks the endian / width match the functions? Should just be checking the scratch register at
litex/litex/soc/integration/soc_core.py
Line 46 in f70dd48
| * NOTE: The same optimization considerations apply here as with _csr_rd_buf() | ||
| * above. | ||
| */ | ||
| #define _csr_wr_buf(a, uintX_t, buf, cnt) \ |
There was a problem hiding this comment.
Is there any reason this whole thing should be a macro? It looks like you just use sizeof(type uintX_t)?
I would expect that if this was an inline function then it would end up as the same ASM?
There was a problem hiding this comment.
There's also buf[i], which is of type uintX_t. I'd have to have some sort of switch statement and a bunch of casts to each actual width (X), and make buf a void *. I figured the loss of legibility wasn't worth it, but if there's a nice elegant way to still do an inline function instead of a macro that'd be awesome!
There was a problem hiding this comment.
... but at least I might be able to avoid passing uintX_t to the macro, and use sizeof(buf[0]) instead... I'll take another look after I get some sleep, and force-push a cleaned-up version if it still makes sense in the morning :)
There was a problem hiding this comment.
force-pushed a simplified pair of macros, without the uintX_t parameter, which is implicit in the type of buf[0] (tested, and working same as before). I think we're stuck with the macros, since implementing the casts implied by the type of buf[0] manually would be much less readable, IMHO.
|
On Sat, Jan 11, 2020 at 05:33:10PM -0800, Tim Ansell wrote:
Could we add a "check CSRs access work" type function which checks
the endian / width match the functions? Should just be checking
the scratch register [...]
Not sure I follow -- do you mean read the CSR, check if we get the
original/initial value hard-coded in the gateware, then write it with
something, read it back, compare to check if we got back the same
thing we wrote?
If that's what you mean, it should be relatively easy. Although maybe
the scratch CSR should be wider (64, 128 bits?) to support a more
thorough checking functionality? Maybe this would be better as an
independent, follow-up PR...
|
Implement CSR accessors for all standard integer types and combinations of subregister alignments (32 or 64 bit) and sizes (i.e., csr_data_width 8, 16, or 32). Rename accessors to better reflect the size of the register being accessed, and correspondingly update the generation of "csr.h" in "export.py". Additionally, provide read/write accessors that superimpose arrays of standard unsigned C types over a CSR register (which may itself be spread across multiple subregisters). Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
Revert to treating SDRAM_DFII_PIX_[RD|WR]DATA CSRs as arrays of bytes, but use the new uintX_t array accessors for improved legibility, and to avoid unnecessary byteswapping. Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
|
@gsomlo: i've been able to test write leveling with it and don't see regression, so i'm merging. Thanks again. |
Reimplement CSR accessors to be flexible w.r.t. alignment (i.e., CPU word width) and csr_data_width.
Implement additional accessors (targeted mainly at CSRs with total width > 64 bits) which transfer
arrays of standard C uintX_t types (8, 16, 32, and 64 bit uints supported).
Use CSR array accessors to clean up sdram initialization (@enjoy-digital, please test write leveling,
and all the other stuff that's not available on nexys4ddr, trellis, or ecp5versa before pulling).
This replaces PR #324 (which also contains a patch to the bios main() function testing the correctness
of all new accessors). I didn't want the actual test patch to be applied, but did want it to remain available publicly, hence this new PR.