Attention is currently required from: Raul Rangel, Eric Lai, Martin Roth, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer.
Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74093 )
Change subject: mb/google/myst: Add new mainboard ......................................................................
Patch Set 3:
(8 comments)
File src/mainboard/google/myst/Kconfig:
https://review.coreboot.org/c/coreboot/+/74093/comment/928b63ad_cc6993c8 PS2, Line 12: TODO: might need to be adapted for better placement of files in cbfs
This is a copy-paste from Skyrim. We hopefully don't need a TODO here.
Copy/Paste from Skyrim. Removed for now
File src/mainboard/google/myst/bootblock.c:
https://review.coreboot.org/c/coreboot/+/74093/comment/04fa26df_e268615e PS2, Line 8: /* TODO: Perform mainboard initialization */
Please add a bug number to track this.
Done
File src/mainboard/google/myst/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/74093/comment/fb370a85_8f5db8d2 PS2, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
I think Jon did this for ease of review as well as avoid pitfalls with blind copy-paste.
The entire process and stack of CL's is based on what has been done in previous programs. We build up the image in small incremental changes which allows us to review it and compare against the current code base.
See https://docs.google.com/spreadsheets/d/1Do70XMx1_DdJbEvvAPc36tR8jgHChxnqVdLo...
and https://docs.google.com/document/d/1Qd7j9Aqk6U2zjPg25D1CA9srxWq3wpzQbwZ8f1O4...
https://review.coreboot.org/c/coreboot/+/74093/comment/5dfd926b_c18ffce8 PS2, Line 16: Name(LIDS, 0)
why need this here?
See https://review.coreboot.org/c/coreboot/+/72185
File src/mainboard/google/myst/mainboard.c:
https://review.coreboot.org/c/coreboot/+/74093/comment/e335d349_6e74ba77 PS2, Line 19: /* TODO: Perform mainboard initialization */
Same. Add bug numbers.
Done
File src/mainboard/google/myst/port_descriptors.c:
https://review.coreboot.org/c/coreboot/+/74093/comment/328e15aa_e28653b4 PS2, Line 10: /* TODO: Initialize DXIO and DDI descriptors */
Bug number.
Done
File src/mainboard/google/myst/variants/baseboard/include/baseboard/baseboard.h:
PS2:
I think this file name should be gpio. […]
This file still exists in Skyrim, it looks like the original error was made there and carried forward here. Deleted and will push a CL to remove the file from skyrim as well.
https://review.coreboot.org/c/coreboot/+/74093/comment/51c38b38_03f97007 PS2, Line 3: __BASEBOARD_GPIO_H__
This guard name doesn't match the file name.
This is by design, but probably a little confusing due to the bad naming. gpio.h is included from a number of nested directories, this guard is named for the baseboard copy.
-
Jon Murphy (Code Review)