-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Switch from flashrom to flashprog #1769
Conversation
af6e9ff
to
a4ef189
Compare
@JonathonHall-Purism need review and testing on Librems. |
Wow! 😮 The progress bar shown in the video (in #1773) is extremely compelling to me. Thanks for this, I'm building the combined MR to test and will review now. |
@i-c-o-n comments in OP still needing feedback otherwise have to reach for community testing and more extensive testing from board owners. |
Quick reminder that CircleCi builds for quick testing. This #1773 was just a staging PR containing all the pending changes to ease testing to reduce delays in functional testing results :) |
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.
Amazing work. The progress indicator is awesome. Thank you so much for this @tlaurion 🙇
I tested Librem L1UM, 13v2, 14, and 11 (representing our range of chipsets pretty well) and all work perfectly.
Adding https://github.com/linuxboot/heads/blob/master/BOARD_TESTERS.md's needing testing to OP prior of merging this PR |
Testing needed as per OP post:
|
Have tested x220. Unfortunately, it needs export CONFIG_FLASH_OPTIONS="flashprog memory --progress --programmer internal:ich_spi_mode=hwseq" Otherwise I get: flashprog is free software, get the source code at https://flashprog.org/ |
Thanks @srgrint will update PR ASAP, I will expect the same for all xx20. |
endif | ||
|
||
#Only enable AST1100 if requested per board configs | ||
ifeq "$(CONFIG_FLASHPROG_AST1100)" "y" |
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.
Verify
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.
Patch missing. Even i currently activated under server board and passed at make, doesn't o anything without https://github.com/linuxboot/heads/blob/master/patches/flashrom-b1f858f65b2abd276542650d8cb9e382da258967/0100-enable-kgpe-d16.patch missing
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 can have a look at it. AFAIR, nobody made a real effort to upstream this. If I rebase this, we'd still need somebody to test it thoroughly. We are quite pedantic when it comes to internal flashing.
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-c-o-n it is a problem of upstreaming indeed, keeping track of patches downstream never proved to do any good.
If not under flashprog/flashrom: this means end users need to flash bmc externally which is not a desirable thing. Also as you may know, that bmc port is old but still in charge of best fan control out there for the d16. Having heads flash through https://github.com/linuxboot/heads/pull/1769/files#diff-08a8a9080a4ca4377e0de4dbef6bd94c84209343b4e12a8982352a1c1b09b71b would be useful.
That would be ideal, but not a blocker for this PR.
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-c-o-n do you see anything else that should be improved in the flashprog makefile for size/functionalities inclusion?
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.
Do you see anything that should be added in board config for flash options that should be passed? Something that would make speed of internal flashing faster?
Also not a requirement for this PR, but a nice thing to track in separate issue for improvements, let me know.
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-c-o-n do you see anything else that should be improved in the flashprog makefile for size/functionalities inclusion?
No, the current selection is already the minimum AFAICT. Anything else would need some hacking on the code. One low hanging fruit: For targets that use Intel hwseq or linux_mtd, you could drop everything else from flashchips.c
. Haven't tried, but it probably won't link the chip drivers then.
Do you see anything that should be added in board config for flash options that should be passed? Something that would make speed of internal flashing faster?
--noverify-all
gives you a little reading-speed advantage. But assuming that you update most of the flash anyway, it won't make much of a difference.
unmaintained_boards/UNMAINTAINED_t520-hotp-maximized/UNMAINTAINED_t520-hotp-maximized.config
Outdated
Show resolved
Hide resolved
unmaintained_boards/UNMAINTAINED_t520-maximized/UNMAINTAINED_t520-maximized.config
Outdated
Show resolved
Hide resolved
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.
Missed unmaintained boards for hwseq, d16 bmc internal flashing must be wrong
|
Haswell board owners, time to shine promptly Otherwise they will land as untested boards in the next days. |
@ResendeGHF 5cba23e commit message accurate? |
@pietrushnic is Dasharo considering switching to flashprog? |
…c FLASH_OPTIONS instead, might cchange in the future. Signed-off-by: Thierry Laurion <[email protected]>
Accurate. |
can confirm this works on kgpe-d16 and the #1778 patch works as intended aswell |
Signed-off-by: Thierry Laurion <[email protected]>
Updated OP #1769 (comment) Waiting for approval/comments/concerns. Started a thread for vPub today: https://matrix.to/#/!HAonmNlisbeoADmnrN:matrix.org/$SmBpUcZDlvATbUruP1sMfV16gZjEDIwVmm6Yi98n5cw?via=matrix.org&via=kit.edu&via=invisiblethingslab.com Starts in one hour, should talk on this after 14:00 EST. |
This is from old to new firmware flashing, from a firmware version still showing Heads internal flashing progress bar. The goal of this testing is double flashing, meaning you have to flash something FROM this PR. Flashing the same ROM is ok. Please redo.
Will check, meaning boardname is untested. With your update, can move to tested @natterangell Output should look like OP's screenshot on second flash, from within that new firmware being flashed @natterangell . EDIT: you flashed from a rom that WAS tagging t420 as untested. :) |
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.
Sorry, for the late reply. I think you tagged me six days ago. I am always willing to test the W541, but I cannot guarantee to do it as promptly as per your requirement. It is your decision whether you want to keep me listed as a tester here.
For the record the last few rounds of board testing I did test the W541 eventually. Personally, I also do not mind if the board is moved to untested and I create a PR after I tested it like I did here: #1750
That said, I understand your frustration with slow testing holding the project back. You know best whether it is worth keeping the board in spite of slow testing or not.
Yes, that was it. I was on an old rom :) Don’t worry, I enjoyed the improved progress bars flashing back to master 😎 |
@JonathonHall-Purism do we merge and deal with ecosystem switching to flashprog /not switching/ reverting to flashrom later if needed? Seems like we won't get feedback here. Feel free to merge. |
Dasharo/dasharo-issues#1060 (comment) Patch needed |
https://review.sourcearcade.org/c/flashprog/+/259 Should probably put more work into auto-detection... I'll try to get everything merged over the next weekend, then you can pull from the main branch 😄 |
This is where I don't know how to act. https://review.coreboot.org/c/flashrom/+/84102 is still unmerged while flashprog works for all platforms, producing needed progress output as well. @macpijan @SergiiDmytruk what do we do? Blocker for #1753 as well. |
We have used flashrom mostly so far, not flashprog, but if it works better for you, we do not mind. |
My personal preference for forks are for ones that are listening to downstream needs: here Heads is a downstream user of flashrom/flashprog that needs to be stable, functionally equivalent and provide proper UX not aimed at geeks but more and more non-technical end users under Heads umbrella.
Not specifically: This PR proved its point where https://review.coreboot.org/c/flashrom/+/84102 and whole #1753 discussion with patches missing under flashrom, and versions not including some patches at time of release, releases breaking compatibility with older platforms and the lack of progress output consideration from upstream for 2+ years under https://ticket.coreboot.org/issues/390 are all reasons for me to want tot get away of flashrom ASAP if possible. My question is more: what project do we want to use and promote. And then, if I switch to flashprog, does it generates a veto or not?
I just need understanding of the situation, where @i-c-o-n clearly surpassed my expectations in the past and moves things forward and fast and stabler then flashrom. This is actually why the project was forked. Again not wanting to go political, but this is why projects fragments: right? When things don't advance as fast as expected downstream: this is why projects gets forked, amongst many other reasons of course. Eg: why Heads tends to rely on more forks of coreboot then upstream for recent platforms. I am reluctant to change from flashrom to flashprog for political reasons, not technical ones. @macpijan If you are confortable, please approve PR. :) |
TODO:
|
@macpijan change of mind since input from @SergiiDmytruk at #1753 (comment)? I really don't know what do do with this fragmented ecosystem and need advice, really. |
Current commit pointer looks good. I have plans to go over the patches on the |
Signed-off-by: Thierry Laurion <[email protected]>
Tested on optiplex after rebasing on master: working. |
Flashrom is unfit for a while:
See #1546 for more details leading for this work.
Unknows @i-c-o-n:
hwseq automatic or still needed on command line?yes. Added. oklegacy boards: internal flashing on --ifd specified bios region: ok? (deprecating soon legacy boards): deprecated, but should work.Tested with amazing progress bar output:
Needs double flashing: reflashing within Heads to see if flashprog is able to flash again the firmware you had
Quick test for other boards untested: