Attention is currently required from: Nicholas Sudsgaard.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80411?usp=email )
Change subject: Documentation/mainboard/lenovo: Add ThinkCentre M710s ......................................................................
Patch Set 17:
(19 comments)
File Documentation/mainboard/lenovo/thinkcentre_m710s.md:
https://review.coreboot.org/c/coreboot/+/80411/comment/a86c00b7_b89de405 : PS17, Line 5: that even dummies could follow through I would prefer to avoid such expressions. If someone struggles to follow the instructions, would they be worse than a dummy? Ouch, that's a bit insulting.
Instead, please consider something along the lines of:
This guide is targeted at beginners with little to no prior coreboot experience
I feel that "beginner" (a person who is starting to do something or learn something for the first time) is both accurate and respectful.
https://review.coreboot.org/c/coreboot/+/80411/comment/6fe01ba1_1095f0f6 : 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 to write similar procedures for each and every mainboard? I would rather have a general guide that references mainboard-specific documentation where necessary.
I'm not sure if docs support it, but it would be really cool if the installation guide's page had a mainboard selector drop-down, and choosing the mainboard in there automatically merged the mainboard-specific details (preparation steps, getting required blobs, where is the flash chip, how to flash it, where is the console) into the installation guide.
https://review.coreboot.org/c/coreboot/+/80411/comment/cbedec3a_684f3f4a : 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.
https://review.coreboot.org/c/coreboot/+/80411/comment/9d833fba_75c63907 : 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:
``` flashrom -p internal -r vendor.rom flashrom -p internal -v vendor.rom ```
https://review.coreboot.org/c/coreboot/+/80411/comment/94bfac82_058acd2a : 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.
https://review.coreboot.org/c/coreboot/+/80411/comment/7a07a3a2_14d0fae5 : 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-prone.
https://review.coreboot.org/c/coreboot/+/80411/comment/143bf189_6cc9d00c : PS17, Line 53: 00000000:00000fff fd : 00200000:007fffff bios : 00003000:001fffff me : 00001000:00002fff gbe Please keep this information in this docs page.
https://review.coreboot.org/c/coreboot/+/80411/comment/5c03bb92_68af350d : 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 having each and every board repeat itself. Please remove.
https://review.coreboot.org/c/coreboot/+/80411/comment/eff1cc58_44636add : 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 known to work, and what hasn't been tested.
You already state that SeaBIOS and edk2 are known to work later on, so the only thing to add would be to mention that other payloads haven't been tested.
The "are most likely not going to work" part is unjustified, and I would simply omit it. I'm pretty sure that GRUB as a payload would work fine, it's just not tested.
TL;DR just state what works and what is untested. If people make bad decisions (e.g. end up with a non-booting machine) because they don't know what "untested" means, I'd say it's not our fault.
https://review.coreboot.org/c/coreboot/+/80411/comment/f6b6dc0e_02486b80 : 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 default because it's specifically tested with coreboot, whereas upstream edk2 is not.
https://review.coreboot.org/c/coreboot/+/80411/comment/c0cfa8c0_b6703539 : 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.
https://review.coreboot.org/c/coreboot/+/80411/comment/08239d15_010c6bb4 : 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.
https://review.coreboot.org/c/coreboot/+/80411/comment/ac5e204e_672d30a6 : 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.
https://review.coreboot.org/c/coreboot/+/80411/comment/7b8797b3_b1db4a82 : 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. Also, if not recommended, why is this the only explanation provided?
https://review.coreboot.org/c/coreboot/+/80411/comment/e8c8a803_4326c10c : PS17, Line 148: 'W25Q64BV/W25Q64CV/W25Q64FV' Earlier you stated that the chip is a `W25Q64JV`. Why use the wrong chip definition?
https://review.coreboot.org/c/coreboot/+/80411/comment/a0b79d7a_6b964823 : PS17, Line 160: In the unfortunate case, where your machine does not seem to boot, first check : for any mistakes. What constitutes a mistake?
https://review.coreboot.org/c/coreboot/+/80411/comment/0086af44_ca3f6b8b : 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.
https://review.coreboot.org/c/coreboot/+/80411/comment/b019c678_b1563192 : 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?
https://review.coreboot.org/c/coreboot/+/80411/comment/2ee3680b_12ca5cde : PS17, Line 200: * DRM issue when using EDK 2 and libgfxinit What does "DRM issue" mean?