Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add suspend to RAM support for nRF54H20 #73095
Add suspend to RAM support for nRF54H20 #73095
Changes from all commits
c226dbc
4428bc2
96a525f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Similarly to above, should this clean up in the failure case?
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 think that this is not the same case (#73095 (comment)) -
__set_BASEPRI(0)
doesn't disable interrupts but clears the interrupt mask. @hubertmis , correct me if I'm wrong.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.
How about the modifications to RESETINFO like setting RESTOREVALID, could those have unintended consequences when we don't actually suspend here? I guess the stored CPU context is no longer valid at this point
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.
You're right, the CPU context is no longer valid, but we don't check the RESETINFO then (and hence no CPU context restoring) - it happens only when reset vector is called after wake-up.
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 think the only edge case that could cause issues would be domain resets initiated from the secure domain, but I think the UNRETAINEDWAKE bit in RESETREAS.LOCAL will always be cleared in those instances so it should be fine.
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.
From ARMv8M manual:
Setting BASEPRI to 0 effectively locks all IRQs (including ZLI).
When this function returns, it should enter the "resume" procedure which in both cases of wake-up and returning error here should clean up after suspending things like LRCCONF settings or RESETINFO.RESTOREVALID.
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.
Ok, I see there is no such "resume" procedure and LRCCONFs are never restored. That's something we need to improve.
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 is it
z_pm_sys_resume()
? This code is so all over the place with functions with similar naming it's hard to understand what's going on in which procedure ;)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.
Yup - it is
z_pm_sys_resume()
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 missed that
pm_s2ram_mark_check_and_clear()
should always be called when this function returns, so my particular question about RESETINFO is not valid since it is indeed cleaned up on failure.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.
If you are not doing anything you should call
k_cpu_idle()
otherwise the idle thread will loop on it until the next scheduled time is lower than the suspend-to-ram residency time.What does not make much sense is having PM enabled if you don't handle any power state, you could simply disable PM when
CONFIG_PM_S2RAM
is disabled.Note that device power management does not require
CONFIG_PM
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.