Attention is currently required from: Angel Pons.
Nicholas Sudsgaard has posted comments on this change by Nicholas Sudsgaard. ( https://review.coreboot.org/c/coreboot/+/80411?usp=email )
Change subject: Documentation/mainboard/lenovo: Add ThinkCentre M710s ......................................................................
Patch Set 20:
(18 comments)
File Documentation/mainboard/lenovo/thinkcentre_m710s.md:
https://review.coreboot.org/c/coreboot/+/80411/comment/a8e2776a_e0012b65?usp... : PS17, Line 5: that even dummies could follow through
Thanks for the detailed review! […]
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/8aceee93_3a40a3a9?usp... : PS17, Line 12: ```eval_rst : Start by following the :doc:`tutorial <../../tutorial/part1>` until step 3. : ```
Wouldn't it be nice to have a tutorial for "installing" coreboot onto any board, instead of having t […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/3eab2081_19552632?usp... : PS17, Line 22: # Take 2 images of your original flash image : flashrom -p internal -r vendor.rom : flashrom -p internal -r vendor.rom.backup
That won't work properly because the ME region is not readable by the host.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/44bffc64_71cfbb93?usp... : PS17, Line 26: # Compare the images to see if there is any differences (should be none) : sha1sum vendor.rom vendor.rom.backup
This is unnecessary. Just tell flashrom to verify the file against the flash chip contents: […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/d9965d27_c5a7b230?usp... : PS17, Line 16: ### Step 2 - Preserving the original flash image (IMPORTANT) : : Before proceeding, it is **VERY IMPORTANT** to keep the original (vendor) flash image. : Without this, it would be very difficult to recover from mistakes during : flashing (or if you decide coreboot is not for you). : : # Take 2 images of your original flash image : flashrom -p internal -r vendor.rom : flashrom -p internal -r vendor.rom.backup : : # Compare the images to see if there is any differences (should be none) : sha1sum vendor.rom vendor.rom.backup : : # Make the images immutable to prevent accidental modification/deletion : chattr +i vendor.rom vendor.rom.backup : # Check whether the 'i' flag is set : lsattr vendor.rom vendor.rom.backup : : # To be absolutely sure you can attempt to modifiy/delete the images : # You should get 'Operation not permitted' : echo "test" > vendor.rom : rm -f vendor.rom : : If you ever need to undo the installation, simply flash this image. : As long as you do this, flashing coreboot is not as scary as it sounds. :)
This is not mainboard specific. It doesn't belong in here.
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/911aa949_6ebe1993?usp... : PS17, Line 44: Let's also use a layout file to be sure that we only modify the BIOS region : of your flash chip. This makes things simpler and reduces and chance of messing : up your flash chip.
Adding the `--ifd -i bios` arguments to flashrom achieves the same effect and is a lot less error-pr […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/eeeae4db_bb3a3097?usp... : PS17, Line 64: coreboot provides an easy way of configuration by using a TUI menu. To open this : run the following command: : : make menuconfig : : While it should be quite intuitive, the navigation is written at the top if you : get stuck. : : Every mainboard is different and you need to instruct coreboot to build for : Lenovo ThinkCentre M710s specifically. : : Mainboard ---> : Mainboard vendor (Lenovo) ---> : Mainboard model (ThinkCentre M710s) --->
It would be much better to have generic "how to build a coreboot image" instructions instead of havi […]
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/66d0ab48_2e384148?usp... : PS17, Line 81: Now you can choose the payload that you are going to use: : : * SeaBIOS : * Tianocore's EDK 2 (choose this if unsure) : : It is highly recommend not choosing anything else, as they have not been tested : and are most likely not going to work.
I wouldn't tell people to use any payloads, just document what has been tested on this board and is […]
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/61f6062f_f6c53dac?usp... : PS17, Line 98: Tianocore's EDK II payload (Official edk2 repository) --->
But why wasn't the temptation (what you wrote below) resisted here? MrChromebox's edk2 is the defaul […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/be0a08c2_f5aa5025?usp... : PS17, Line 79: ### Step 2 - Choosing the payload : : Now you can choose the payload that you are going to use: : : * SeaBIOS : * Tianocore's EDK 2 (choose this if unsure) : : It is highly recommend not choosing anything else, as they have not been tested : and are most likely not going to work. : : #### SeaBIOS : : Payload ---> : Payload to add (SeaBIOS) ---> : : #### EDK 2 : : Payload ---> : Payload to add (edk2 payload) ---> : Tianocore's EDK II payload (Official edk2 repository) ---> : : While understandable, it is recommended to resist the temptation of doing any : additional configuration if this is your first time building coreboot.
Not mainboard specific.
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/08d551cf_2ba1c064?usp... : PS17, Line 103: ### Step 3 - Building coreboot : : Finally, save your changes and build coreboot (this should only take a couple of minutes). : : make -j$(nproc)
Not mainboard specific.
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/25461248_41eae2ee?usp... : PS17, Line 133: **Make sure you attach the clip using the correct pin configurations** (page 6 of : the datasheet or the image above), as it may **damage your chip** if done incorrectly. : How to do this depends on your programmer of choice and refer to other : documentation which can be found on the internet.
Not mainboard specific.
Done
https://review.coreboot.org/c/coreboot/+/80411/comment/ae73368b_13f6b215?usp... : PS17, Line 138: While [not recommended](https://libreboot.org/docs/install/spi.html#do-not-use-ch341a) : the CH431A programmer seems to be very popular. However, it can be slightly : confusing at first. Here is the pinout and which half to use ([datasheet](https://www.alldatasheet.com/datasheet-pdf/pdf/1132609/ETC2/CH341A.html)). : : 
Not mainboard specific. […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/bfe25357_7bcd382e?usp... : PS17, Line 148: 'W25Q64BV/W25Q64CV/W25Q64FV'
I don't think flashrom has support for `W25Q64JV` specifically (at least when I wrote the documentat […]
I now use the correct chip definition: `W25Q64JV-.Q`.
https://review.coreboot.org/c/coreboot/+/80411/comment/e14962db_2afb3ed9?usp... : PS17, Line 160: In the unfortunate case, where your machine does not seem to boot, first check : for any mistakes.
What constitutes a mistake?
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/ab6d727b_5ac9302f?usp... : PS17, Line 161: If you are confident that you did not make any mistakes : reach out to the :doc:`community <../../community/forums>` for some help.
And if you made mistakes, what? I really don't like how this section reads.
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/ddb96bdb_f9cec0a6?usp... : PS17, Line 165: ## Updating : : Luckily, once you have coreboot, updating can be done internally using the following command: : : flashrom -p internal --fmap -i COREBOOT -w build/coreboot.rom
Any reason to update coreboot using the fmap here?
Acknowledged
https://review.coreboot.org/c/coreboot/+/80411/comment/7b5cd13c_36d38aba?usp... : PS17, Line 200: * DRM issue when using EDK 2 and libgfxinit
What does "DRM issue" mean?
Unfortunately I don't remember what error message I was receiving in `dmesg` (I know... I should have noted that down). I tried to replicate the issue running Ubuntu 22.04 LiveCD but everything seems to work fine now.
-
Nicholas Sudsgaard (Code Review)