-
Notifications
You must be signed in to change notification settings - Fork 361
target/riscv: access registers via reg->type
#1269
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
base: riscv
Are you sure you want to change the base?
Conversation
…pability Cherrry-picked form https://review.openocd.org/c/openocd/+/8070/21 1) OpenOCD has the capability to 'force' a register read from the target. This functionality however silently breaks the register cache: During 'get_reg force' or 'reg <name> force', reg->type->get() is called which will silently overwrite dirty items in the register cache, causing a loss of unwritten register values. This patch fixes that by adding a flush callback for registers, and by using it when it is needed. 2) The register write commands did not have the 'force' flag; this was present for register read commands only. This patch adds it. 3) This patch also introduces the flush_reg_cache command. It flushes all registers and can optionally invalidate the register cache after the flush. For targets which implement the register cache, the flush() callback in struct reg_arch_type should be implemented (in separate patches, by the maintainers of each of the target type). This functionality is also useful for test purposes. Example: - In RISC-V, some registers are WARL (write any read legal) and this command allows to check this behavior. We plan to implement the corresponding callback in the RISC-V target. Change-Id: I9537a5f05b46330f70aad17f77b2b80dedad068a Signed-off-by: Marek Vrbka <[email protected]> Signed-off-by: Jan Matyas <[email protected]>
* `int riscv_reg_get()` and `int riscv_reg_set()` are implemented in terms of `reg->type->get/set` instead of the other way around. This makes it easier to support custom behavior for some registers. * Cacheability is determined by `reg->type` instead of `riscv_reg_impl_gdb_regno_cacheable()`. * Issues with redirection of `priv` -> `dcsr` and `pc` -> `dpc` are addressed at the "topmost" level. - `priv` and `pc` are always invalid. - Fixed some issues, e.g. the first `pc` write printed-out an uninitialized value: ``` > reg pc 0 pc (/64): 0x000075da6b33db20 ``` Change-Id: I514547f455d62b289fb5dee62753bf5d9aa3b8ae Signed-off-by: Evgeniy Naydanov <[email protected]>
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 are the two things I have found so far, I will take another look later. So far I like the changes.
if (debug_level < LOG_LVL_DEBUG) | ||
return; |
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.
I don't think this early return is needed.
static int send_vreg(const struct reg *reg, const uint8_t *buf) | ||
{ | ||
return riscv013_set_register_buf(riscv_reg_impl_get_target(reg), | ||
reg->number, reg->value); | ||
} |
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.
Aren't we ignoring the buf
paramter here?
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.
Took another looks, seems fine other than one nitpick.
* target. Does not modify "reg" at all. | ||
*/ | ||
typedef int (*reg_fetcher)(struct reg *reg); | ||
typedef int (*reg_sender)(const struct reg *reg, const uint8_t *buf); |
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.
I have a feeling that the send
keyword isn't the best. Maybe reg_writer
or similar could be better. Up to your consideration.
int riscv_reg_get()
andint riscv_reg_set()
are implemented interms of
reg->type->get/set
instead of the other way around. Thismakes it easier to support custom behavior for some registers.
reg->type
instead ofriscv_reg_impl_gdb_regno_cacheable()
.priv
->dcsr
andpc
->dpc
areaddressed at the "topmost" level.
priv
andpc
are always invalid.pc
write printed-out anuninitialized value: