Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38978 )
Change subject: [WIP] mainboard: Add Acer ES1-572 ......................................................................
Patch Set 4:
(8 comments)
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... File src/mainboard/acer/es1-572/Kconfig:
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 1: ##
IMHO config is not copyrightable
I think I get yelled at by the lint scripts if there isn't a copyright notice here. Moreover, there are maaany more Kconfig files with such a notice.
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 16: if BOARD_ACER_ES1_572 || BOARD_ACER_EXTENSA_2540
if you're sure that there is no other device out there with that board, I'd be ok with that. […]
The two symbols only change the part number. Other than that, the boards are the same. No other machine model seems to exist with this board.
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 60: config ADD_FSP_BINARIES : default y
I read somewhere we normally don't set this to default "yes" because coreboot should be free by defa […]
Well, default settings enable microcode updates but USE_BLOBS is not set by default. It might be changed at some point. Also, adding FSP binaries by default results in a more comprehensive build test, and the resulting artifacts might as well be used for boot tests.
Adding to that, it took me a while to find where that option is to enable it. I wouldn't want others to suffer as much as I did. I mean, I wanted to make sure I could boot fine on the first try, and I somehow managed to do so.
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... File src/mainboard/acer/es1-572/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 261: device pci 1f.0 on end # LPC Interface
no superio?
It's a laptop board, and as commit message states, EC suppport is missing. In any case, ENE does not use SuperIO-like EC interfaces, unlike Winbond and ITE.
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... File src/mainboard/acer/es1-572/romstage.c:
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 25: static void mainboard_fill_dq_map_data(void *dq_map_ch0, void *dq_map_ch1)
I don't think you need this, since there is no soldered RAM. Can be removed IMO.
I'll check, I'm not sure.
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 47: mainboard_fill_rcomp_res_data
same as above
No. Don't you see that I actually went through the effort to check the schematics? I even added the names of the actual resistors 😄
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 54: mainboard_fill_rcomp_strength_data
same
No, Rcomp calibration is performed on this board.
https://review.coreboot.org/c/coreboot/+/38978/4/src/mainboard/acer/es1-572/... PS4, Line 74: d_fill_dq_map_data(&mem_cfg->DqByteMapCh0, &mem_cfg->DqByteMapCh1); : mainboard_fill_dqs_map_data(&mem_cfg->DqsMapCpu2DramCh0, &mem_cfg->DqsMapCpu2DramCh1); : mainboard_fill_rcomp_res_data(&mem_cfg->RcompResistor); : mainboard_fill_rcomp_strength_data(&mem_cfg->Rcomp
same
Maybe. In any case, I'll need to change this if I remove any of the functions.
-
Angel Pons (Code Review)