-
Notifications
You must be signed in to change notification settings - Fork 551
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
software, integration/export: (re-)expose CSR subregister accessors #366
Conversation
FYI - @xobs |
litex/soc/integration/export.py
Outdated
r += "extern uint32_t csr_rd_uint32(unsigned long a);\n" | ||
r += "extern uint64_t csr_rd_uint64(unsigned long a);\n" | ||
r += "extern void csr_write_sub(unsigned long v, unsigned long a);\n" | ||
r += "extern unsigned long csr_read_sub(unsigned long a);\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These being marked as extern
will mean the compiler can't optimize them without lto turned on (see #253 for that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the case where they're externally provided by downstream, and downstream defines CSR_ACCESSORS_DEFINED to let us know we shouldn't be using the built-in ones.
That's the way it was before I ever got involved :) The litex-provided accessors (in common.h) are definitely NOT marked as external
#define CSR_DW_BYTES (CONFIG_CSR_DATA_WIDTH/8) | ||
|
||
/* CSR subregisters are embedded inside native CPU word aligned locations: */ | ||
#define MMPTR(a) (*((volatile unsigned long *)(a))) | ||
|
||
static inline void csr_write_sub(unsigned long v, unsigned long a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe csr_write_part
or csr_write_section
are better names than csr_write_sub
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'sub' is for "subregister" (how about 'subreg' then)? I could also go with "slice". Both "part" and "section" are IMHO terms that are too overloaded to ensure we don't confuse future readers of the source code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we call it csr_write_simple
to mach the simple_csrs
used in https://github.com/enjoy-digital/litex/blob/master/litex/soc/interconnect/csr.py? (if simple is confusing we could change it to something better in both software/gateware later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we call it
csr_write_simple
or to mach thesimple_csrs
...
That works for me, thanks! Force-pushed an amended commit reflecting that.
Expose a pair of `csr_[read|write]_simple()` subregister accessors, and restore the way dedicated accessors are generated in "generated/csr.h" to use hard-coded combinations of shifts and subregister accessor calls. This restores downstream ability to override CSR handling at the subregister accessor level. Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
This works for me. It should get inlined easily, and should still work on both 32 and 64 bit architectures. And the new name should prevent confusion, and is guaranteed to work on any new architectures we come across. Maybe we'll find someday that we need to have it accept a Looks good! |
Expose a pair of
csr_[read|write]_sub()
subregister accessors, andrestore the way dedicated accessors are generated in "generated/csr.h"
to use hard-coded combinations of shifts and subregister accessor call.
This restores downstream ability to override CSR handling at the
subregister accessor level.
Signed-off-by: Gabriel Somlo gsomlo@gmail.com
This is a followup to PR #339, and should hopefully address issue #365.