Attention is currently required from: Alexander Couzens, Maciej Pijanowski, Michał Kopeć, Paul Menzel.
Angel Pons has posted comments on this change by Maciej Pijanowski. ( https://review.coreboot.org/c/coreboot/+/80609?usp=email )
Change subject: mb/lenovo: Add ThinkCentre M920q (Coffee Lake) ......................................................................
Patch Set 10: Code-Review+1
(10 comments)
Patchset:
PS10: Ah, I just saw you added docs.
File Documentation/mainboard/lenovo/m920q.md:
https://review.coreboot.org/c/coreboot/+/80609/comment/93946f4b_7b30607d?usp... : PS10, Line 1: # Dell OptiPlex 9010 Ahem.
```suggestion # Lenovo M920 Tiny ```
https://review.coreboot.org/c/coreboot/+/80609/comment/e637b452_bd559420?usp... : PS10, Line 9: 8nd Gen or 9rd Gen Oof, copy-pasta error? ("2***nd*** Gen or 3***rd*** Gen)
```suggestion | CPU | Intel Core 8th Gen or 9th Gen (Coffee Lake (Refresh)) | ```
https://review.coreboot.org/c/coreboot/+/80609/comment/75537d77_5b281acb?usp... : PS10, Line 11: DIMM ```suggestion | DRAM | 2 SO-DIMM slots, DDR4-2400/2666 | ```
https://review.coreboot.org/c/coreboot/+/80609/comment/66fe8fdc_b6f54d82?usp... : PS10, Line 45: yes? I feel this could confuse people into believing that installing coreboot via the internal programmer is working. How about:
```suggestion | Internal flashing | after flashing coreboot | ```
https://review.coreboot.org/c/coreboot/+/80609/comment/08a4ed04_9563cf70?usp... : PS10, Line 53: flashrom -p internal -w coreboot.rom --ifd -i bios I thought one needs `--noverify-all` too?
https://review.coreboot.org/c/coreboot/+/80609/comment/b47ed278_d682af1d?usp... : PS10, Line 55: will Huh? Will... Will what? The sentence doesn't quite parse, could you please check it?
https://review.coreboot.org/c/coreboot/+/80609/comment/e560c9b4_2d365be7?usp... : PS10, Line 71: BIOS1 and BIOS1 I hope they're not marked the same 😄
```suggestion The flash chips are marked on the mainboard as BIOS1 and BIOS2 respectively. ```
https://review.coreboot.org/c/coreboot/+/80609/comment/123475db_c46738c9?usp... : PS10, Line 85: - PCIe x8 port This port works, but see comments about riser problems.
File src/mainboard/lenovo/m920q/Kconfig:
https://review.coreboot.org/c/coreboot/+/80609/comment/0df8394d_20c5fa7d?usp... : PS10, Line 33: 0x900000 If this was 0x800000 instead, then by default one could flash only the second flash chip (8 MiB) externally. I find this quite convenient as it reduces the chances of messing up the IFD/ME.