Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30939 )
Change subject: nb/intel/pineview: Move to C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
I just want some discussion here before hitting +2.
https://review.coreboot.org/#/c/30939/2/src/mainboard/foxconn/d41s/bootblock... File src/mainboard/foxconn/d41s/bootblock.c:
https://review.coreboot.org/#/c/30939/2/src/mainboard/foxconn/d41s/bootblock... PS2, Line 36: | COMA_LPC_EN); I think we should document how much of LPC configuration is to be done in bootblock and whether use of devicetree is allowed/forbidden.
Also, while the file was called romstage.c, there is really no reason why we could not include it for bootblock instead of splitting out the function we know we want? We seem to use prefix early_ for things pre-ram on filenames?
I am happy with what I see here, although we really only need COMA and CNF1 for now. Maybe IO 0x80 with AMD chipsets.
https://review.coreboot.org/#/c/30939/2/src/mainboard/foxconn/d41s/romstage.... File src/mainboard/foxconn/d41s/romstage.c:
https://review.coreboot.org/#/c/30939/2/src/mainboard/foxconn/d41s/romstage.... PS2, Line 27: Speculations, do we want to redo this in romstage? Or complete a minimal configuration (from bootblock) with settings from devicetree?