-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
nitropad-nx: use standard shutdown/reboot commands #1704
Conversation
This commit effectively reverts commits a1c13ff and 902866c. There is no need for this special EC-based poweroff command. See more details in issue linked below. Fixes: Dasharo/dasharo-issues#711 Signed-off-by: Maciej Pijanowski <[email protected]>
@macpijan : Will let it build and happily test CircleCI zip update file on my nv41; but should work out of the box.
I guess it's an artifact of using proprietary EC, but I do not know the whole story nor was owning a nv41 back then.
|
Yes, that's a "historic artifact" .... I'd suggest then also removing iotools as this is only needed here. |
Flashing the ME when it's not disabled may result in shutdown / reboot issues indeed, but it shouldn't matter as long as ME is disabled (by HMRFPO or any other method) |
@daringer so this was not related to EC but ME. Retesting. |
Reboot works. Inspecting cbmem |
|
This circleci rom artifacts contains hotp-verification v1.5 not 1.6, inspecting (commit id +2005 in filename) Also bootsplash is bigger (1024*768 un resized) : seems like head of this PR is not master |
@macpijan Please rebase on master. Retesting this as https://github.com/tlaurion/heads/tree/Dasharo_nitropad-shutdown which is this commit on master. |
@macpijan nevermind. Merging this. |
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.
Tested https://app.circleci.com/pipelines/github/Dasharo/heads/122/workflows/7c0cdcf1-4a02-4ea1-9d98-28aef5d4209c/jobs/2117/artifacts all ok.
Suggested adding c7a5fbd but unrelated. Merging
For prosperity @daringer @mkopec @macpijan: ME was shipped activated in the past see https://github.com/Nitrokey/heads/releases/tag/v2.4.1 This explains iotools having been required. Will do PR removing iotools |
As discussed in: linuxboot#1704 there is no need to include iotools module for nitropads. Since there is no board using it, and we see no reason to use it in the future (the EC udpate will not require it, as update will be server by coreboot in the future), drop the module as well. Signed-off-by: Maciej Pijanowski <[email protected]>
@macpijan other boards include it under master. Can you generalize and push commit sedding all the boards please? Otherwise breaks other builds upstream. |
As discussed in: linuxboot#1704 there is no need to include iotools module for nitropads. Signed-off-by: Maciej Pijanowski <[email protected]>
As discussed in: linuxboot#1704 there is no need to include iotools module for nitropads. Signed-off-by: Maciej Pijanowski <[email protected]>
As discussed in: linuxboot#1704 there is no need to include iotools module for nitropads. Since there is no board using it, and we see no reason to use it in the future (the EC udpate will not require it, as update will be server by coreboot in the future), drop the module as well. Signed-off-by: Maciej Pijanowski <[email protected]>
This commit effectively reverts commits a1c13ff and 902866c. There is no need for this special EC-based poweroff command. See more details in issue linked below.
Fixes: Dasharo/dasharo-issues#711
Not actually tested on master yet, only on this branch and forward picked: Dasharo#11
Not sure why it was implemented (guessing EC update?). Maybe we will find out in this review.