Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34497 )
Change subject: mb/google/drallion: Add new mainboard ......................................................................
Patch Set 5:
(10 comments)
https://review.coreboot.org/c/coreboot/+/34497/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34497/1//COMMIT_MSG@9 PS1, Line 9:
can you mention that you copied sarien board as a starting point, and changed soc to CML.
Done
https://review.coreboot.org/c/coreboot/+/34497/1//COMMIT_MSG@10 PS1, Line 10: uses the Chrome EC, that will be enabled in a seperate commit
it is not using Chrome EC
Modified to Wilco EC
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/Kconfig:
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 7: if BOARD_GOOGLE_DRALLION
not needed? (throughout)
Done
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 23: select SYSTEM_TYPE_LAPTOP if BOARD_GOOGLE_DRALLION : select SYSTEM_TYPE_CONVERTIBLE if BOARD_GOOGLE_DRALLION
Need to pick one here to avoid confusion.
Done
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 27: select MAINBOARD_USES_IFD_GBE_REGION if BOARD_GOOGLE_DRALLION
can remove
Arcada uses GBE region. May I know why to remove it for Drallion?
https://review.coreboot.org/c/coreboot/+/34497/1/src/mainboard/google/dralli... File src/mainboard/google/drallion/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/34497/1/src/mainboard/google/dralli... PS1, Line 7: config BOARD_GOOGLE_DRALLION : bool "-> Drallion" : select BOARD_GOOGLE_BASEBOARD_DRALLION
You'll need to delete this second instance - This is the reason your build is failing.
Agree, done.
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 79: /* Disable unused pads for devices with board ID > 2 */ : if (board_id() > 2) : gpio_configure_pads(gpio_unused, ARRAY_SIZE(gpio_unused));
The board id check is probably not needed.
For the EV drallion boards we will be using the Arcada and Sarien boards with its SOC replaced by Cometlake. I am not sure if this check is required in that case or not. Please advice!
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 62: cannonlake_memcfg_init
cometlake?
Done
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 1: cannonlake
cometlake […]
Done
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/34497/4/src/mainboard/google/dralli... PS4, Line 19: /* Need to update for Drallion with right SKU IDs : Arcada is SKU ID 2 and 4 */ : #define VARIANT_SKU_ID 2 : #define VARIANT_SKU_NAME "sku2" : #define VARIANT_SKU_ID_SIGNED_EC 4 : #define VARIANT_SKU_NAME_SIGNED_EC "sku4"
This needs to be defined still, but probably at least remove the "Arcada" string.
Done