-
Notifications
You must be signed in to change notification settings - Fork 14
ot_sram_ctrl: change the defaut pace delay for initialization
#308
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
ot_sram_ctrl: change the defaut pace delay for initialization
#308
Conversation
loiclefort
left a comment
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.
LGTM
hw/opentitan/ot_sram_ctrl.c
Outdated
| ((((_reg_) < REGS_COUNT) && REG_NAMES[_reg_]) ? REG_NAMES[_reg_] : "?") | ||
|
|
||
| #define INIT_TIMER_CHUNK_NS 100000 /* 100 us */ | ||
| #define INIT_TIMER_CHUNK_US 10u /* us */ |
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.
Nit: the comment feels redundant given the suffix (very minor nit, feel free not to change 😅)
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.
very true :)
hw/opentitan/ot_sram_ctrl.c
Outdated
| timer_mod(s->init_timer, (int64_t)(now + INIT_TIMER_CHUNK_NS)); | ||
| int64_t now = qemu_clock_get_ns(OT_VIRTUAL_CLOCK); | ||
| timer_mod(s->init_timer, | ||
| (int64_t)(now + ((int64_t)s->init_pace_us) * 1000u)); |
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.
Nit: the explicit cast of the result of the addition to (int64_t) here is now redundant given the changed type of now.
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.
removed.
| DEFINE_PROP_UINT32("wci_size", OtSramCtrlState, init_chunk_words, 0u), | ||
| DEFINE_PROP_UINT32("init-pace-us", OtSramCtrlState, init_pace_us, |
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.
Aside: It's not necessary to change in this PR, but it is still not clear to me: should we be using - or _ (or both) in property names?
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 now try to use - for properties; however properties that are managed by readconfig file may not follow this rule.
I guess a project-wide clean up would be required at some point.
5c00540 to
28a30d1
Compare
…tialization. Make it faster to avoid ROM initialization timeout. Also add a property for fine-grained control of the pace delay. Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
28a30d1 to
308b8bf
Compare
Make it faster to avoid ROM initialization timeout.
Also add a property for fine-grained control of the pace delay.