Attention is currently required from: Václav Straka.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74248 )
Change subject: Documentation/mainboard/hp: Add more about internal flashing ......................................................................
Patch Set 5:
(24 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74248/comment/fdb7ef8b_06d03390 PS5, Line 12: Vesek As far as I know coreboot's current sign off policy requires a full, real name.
File Documentation/mainboard/hp/compaq_8200_sff.md:
PS5: Not critical but it might also be useful to add the PRx register values, similar to the Macbook tutorial, so that others could derive and check the values used here for the temporary layout
https://review.coreboot.org/c/coreboot/+/74248/comment/f7733bf7_8ccf0f2a PS5, Line 36: ``` I would add a blank line here after the ``` at the end of the table and the Flash layout header
https://review.coreboot.org/c/coreboot/+/74248/comment/b3f899c3_bd8b3ad9 PS5, Line 51: ``` Nit: add a blank line after this
https://review.coreboot.org/c/coreboot/+/74248/comment/8e18b749_0e5ac291 PS5, Line 52: flash flash, (comma)
https://review.coreboot.org/c/coreboot/+/74248/comment/2d1b09ec_88e2e4d2 PS5, Line 53: region Maybe add something like "due to SPI Protected Range settings" after this to describe why the bios region isn't writable.
https://review.coreboot.org/c/coreboot/+/74248/comment/3621b5e1_a867dfd3 PS5, Line 56: Nit: Remove this blank line as the rendered output has a blank line between the caption and the image
https://review.coreboot.org/c/coreboot/+/74248/comment/1022da9c_d28f7cc5 PS5, Line 61: [IFD Hack](https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/changes/70/38770...) You can use the `[reference]: url` syntax for links so that you don't need to have a long URL inline, similar to the syntax for the images.
https://review.coreboot.org/c/coreboot/+/74248/comment/1281e4da_cbc165f9 PS5, Line 61: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/changes/70/38770... I don't really have an issue with the review.coreboot.org link as it should be persistent, but if you wanted you could instead link to Evgeny's original blog post which he based that patch on: https://ch1p.io/coreboot-macbook-internal-flashing/
https://review.coreboot.org/c/coreboot/+/74248/comment/76314a6d_8aecfdae PS5, Line 61: You can use a modified [IFD Hack](https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/changes/70/38770...) originally used on MacBooks, you will : need to read both guides. The wording of this sentence could be clearer. Maybe "To write to the bios region you can use an IFD Hack originally developed for MacBooks, but with modified values described in this guide. You should read both guides before attempting the procedure."
https://review.coreboot.org/c/coreboot/+/74248/comment/c3f13330_91522be9 PS5, Line 64: write in the flash descriptor region you ...write in the flash descriptor region, (comma) you...
Alternatively, "write to the flash descriptor, you..." as "flash descriptor" implicitly refers to that area on the flash, making region somewhat redundant.
https://review.coreboot.org/c/coreboot/+/74248/comment/ae1cdc96_969f3ce8 PS5, Line 65: originaly Spelling: originally
https://review.coreboot.org/c/coreboot/+/74248/comment/1ec6fc9b_1f7b4f7e PS5, Line 66: flash flash, (comma)
https://review.coreboot.org/c/coreboot/+/74248/comment/eface641_6e8dddf6 PS5, Line 70: ME shrinked Maybe "trucated ME firmware" or "neutered ME firmware" to more closely match the Macbook instructions
https://review.coreboot.org/c/coreboot/+/74248/comment/1ebed95b_819ca4eb PS5, Line 77: ``` Nit: Add a blank line after this
https://review.coreboot.org/c/coreboot/+/74248/comment/3e0947f8_e30c2ad0 PS5, Line 78: brick Maybe add "and require external flashing" after this so that it's clear that it isn't a permanent brick
https://review.coreboot.org/c/coreboot/+/74248/comment/5fc12ef6_4b466932 PS5, Line 79: got so far got this far
https://review.coreboot.org/c/coreboot/+/74248/comment/9af90d40_03edc5eb PS5, Line 79: should be already should already be
https://review.coreboot.org/c/coreboot/+/74248/comment/a022948d_62925af0 PS5, Line 80: The temporary flash size to set in coreboot is 2 MB, and you can fill It would be good to mention the CBFS size that should be used as well, just for clarity. The `0xd00000` used in the Macbook tutorial should still work as it fits in the bios region used for this board, though up to `0x1d0000` should work as that utilizes more of the temporary bios region.
https://review.coreboot.org/c/coreboot/+/74248/comment/2b8ab0d6_5f392b4c PS5, Line 80: flash size Maybe "ROM chip size" to match the appropriate menuconfig option
https://review.coreboot.org/c/coreboot/+/74248/comment/8158c3f9_3c442bcf PS5, Line 80: fill : the rest with zeros to get the matching size using I would replace this with "pad the ROM to the required 8MB with zeros using:"
https://review.coreboot.org/c/coreboot/+/74248/comment/427b70ee_df881758 PS5, Line 87: The final flash layout after you already have the temporary coreboot : installation running should look like this: It's not clear that this layout should be used for Stage 2, and that it still is for continuing to use a truncated ME.
https://review.coreboot.org/c/coreboot/+/74248/comment/9742b22f_643af86e PS5, Line 94: ``` Nit: Add a blank line after this
https://review.coreboot.org/c/coreboot/+/74248/comment/cf513627_e05bca25 PS5, Line 95: More about flashing internally and getting the flash layout [here](../../tutorial/flashing_firmware/index.md). Please add a blank line between this and the "External programming" header