Attention is currently required from: Nicholas Chin.
Václav Straka 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 6:
(20 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74248/comment/5c209e64_1c4880dc PS5, Line 12: Vesek
https://coreboot.org/developers.html under Sign-off Procedure. […]
Done
File Documentation/mainboard/hp/compaq_8200_sff.md:
https://review.coreboot.org/c/coreboot/+/74248/comment/13707846_6ea70d46 PS5, Line 36: ```
I would add a blank line here after the ``` at the end of the table and the Flash layout header
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/874d942b_3b8f377d PS5, Line 51: ```
Nit: add a blank line after this
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/ea47efbe_4031d0a2 PS5, Line 52: flash
flash, (comma)
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/4c8df4a6_a15a6ce1 PS5, Line 53: region
Maybe add something like "due to SPI Protected Range settings" after this to describe why the bios r […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/39345a9b_0975d1bb PS5, Line 56:
Nit: Remove this blank line as the rendered output has a blank line between the caption and the imag […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/9b1b49e3_83826a8e 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 […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/885ca800_45afaf6d 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. […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/1720b40b_858abc07 PS5, Line 64: write in the flash descriptor region you
...write in the flash descriptor region, (comma) you... […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/592afc70_587fc3d0 PS5, Line 65: originaly
Spelling: originally
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/5e4732aa_8e56e01b PS5, Line 66: flash
flash, (comma)
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/214ffd90_65ce16bf PS5, Line 70: ME shrinked
Maybe "trucated ME firmware" or "neutered ME firmware" to more closely match the Macbook instruction […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/a06efdb4_057c17ee PS5, Line 77: ```
Nit: Add a blank line after this
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/faa912fc_4c3bb76e PS5, Line 78: brick
Maybe add "and require external flashing" after this so that it's clear that it isn't a permanent br […]
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/3036e34d_00e8ab8b PS5, Line 79: should be already
should already be
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/2bdbd9cc_0b835acf PS5, Line 79: got so far
got this far
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/0ca0df3d_dfb89d52 PS5, Line 80: flash size
Maybe "ROM chip size" to match the appropriate menuconfig option
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/ebe50d34_a9ac0230 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:"
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/0545017b_79b47f45 PS5, Line 94: ```
Nit: Add a blank line after this
Done
https://review.coreboot.org/c/coreboot/+/74248/comment/f867f269_4a50c3ea 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
Done