Attention is currently required from: Jamie Ryu, Subrata Banik, Ethan Tsao, Ravishankar Sarawadi, Paul Menzel, Angel Pons, Nick Vaccaro, Tim Wawrzynczak. Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63170 )
Change subject: intel/common/block: Add gpmr common driver ......................................................................
Patch Set 14:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63170/comment/279b409b_c4c619b9 PS13, Line 8:
Please elaborate, what GPMR is, and what you used as base of your implemenation.
I'll add info in commit message. implementation is same as dmi.c. With 2 more patches, it'll migrate dmi.c to gpmr.c
File src/soc/intel/common/block/gpmr/Kconfig:
https://review.coreboot.org/c/coreboot/+/63170/comment/ae79a2c3_734ffca5 PS13, Line 6: Required for extending BIOS region.
Please spell out what GPMR means.
Ack
File src/soc/intel/common/block/gpmr/gpmr.c:
https://review.coreboot.org/c/coreboot/+/63170/comment/887ad4f1_f7bb3d29 PS13, Line 28: /* Check for available free gpmr */
As the function is well named, remove the redundant comment.
Ack
https://review.coreboot.org/c/coreboot/+/63170/comment/963f958c_01da3c3f PS13, Line 29: static int get_available_gpmr(void)
Mixing CB_ERR and returning values is not wanted I think.
Ack
https://review.coreboot.org/c/coreboot/+/63170/comment/9921d84b_c52847b6 PS13, Line 31: int i;
unsigned int
Ack
https://review.coreboot.org/c/coreboot/+/63170/comment/6dd4d534_cef6a62b PS13, Line 44: uint32_t
size_t?
We want to have fixed size(32bit)
https://review.coreboot.org/c/coreboot/+/63170/comment/9961ee1b_fe3b7892 PS13, Line 46: int
unsigned int
This should be int as get_available_gpmr can return -1 (error case)
https://review.coreboot.org/c/coreboot/+/63170/comment/479883bf_b2b2e248 PS13, Line 71: Range
range
Ack