Attention is currently required from: Ravi kumar. Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45206 )
Change subject: herobrine : Provide initial mainboard support ......................................................................
Patch Set 22:
(11 comments)
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/45206/comment/21e1cd21_b74132a3 PS22, Line 267: config SC7280_SERIAL_CONSOLE : bool "SC7280 SOC compatible serial port driver" : depends on SERIAL_CONSOLE : default n : Please clarify why we need to add this.
File payloads/libpayload/configs/config.herobrine:
https://review.coreboot.org/c/coreboot/+/45206/comment/5b60f7b4_6bd327b7 PS22, Line 5: CONFIG_LP_SC7280_SERIAL_CONSOLE Do we need an soc specific config for this? I see sc7180 used CONFIG_LP_QUALCOMM_QUPV3_SERIAL_CONSOLE instead.
File payloads/libpayload/drivers/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45206/comment/3e39a11c_18c350a1 PS22, Line 45: serial/sc7280.c are we able to reuse serial/qcom_qupv3_serial.c instead? If not, what are the differences?
File src/mainboard/google/herobrine/Kconfig:
https://review.coreboot.org/c/coreboot/+/45206/comment/1b980650_6568406f PS22, Line 50: onfig GBB_HWID : string : depends on CHROMEOS : default "HEROBRINE TEST 1859" if BOARD_GOOGLE_HEROBRINE I do not believe that we define HWID in Kconfig anymore, though I'm not sure where we actually do it anymore. Maybe Julius has an idea?
File src/mainboard/google/herobrine/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/45206/comment/522418ef_cb4c7830 PS22, Line 1: Add:
if USE_QC_BLOBS
<add configs here>
endif
comment "(Herobrine requires 'Allow QC blobs repository')" depends on !USE_QC_BLOBS
File src/mainboard/google/herobrine/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45206/comment/c59cac16_4559eccb PS22, Line 6: bootblock-y += reset.c do we need to add reset.c to every stage?
File src/mainboard/google/herobrine/board.h:
https://review.coreboot.org/c/coreboot/+/45206/comment/ea2fc824_6c1ecba5 PS22, Line 3: #ifndef _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_ : #define _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_ herobrine
https://review.coreboot.org/c/coreboot/+/45206/comment/4fc367f3_9a7418b4 PS22, Line 10: #define GPIO_EC_IN_RW GPIO(118) : #define GPIO_AP_EC_INT GPIO(94) : #define GPIO_AP_SUSPEND GPIO(20) : #define GPIO_WP_STATE GPIO(42) : #define GPIO_H1_AP_INT (CONFIG(TROGDOR_REV0) ? GPIO(21) : GPIO(42)) : #define GPIO_SD_CD_L GPIO(69) : #define GPIO_AMP_ENABLE GPIO(23) : : /* Display specific GPIOS */ : #define GPIO_BACKLIGHT_ENABLE GPIO(12) : #define GPIO_EDP_BRIDGE_ENABLE (CONFIG(TROGDOR_REV0) ? GPIO(14) : GPIO(104)) : #define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30)) Seems like these have been copied from trodgor? Can you please make sure these are valid for herobrine?
https://review.coreboot.org/c/coreboot/+/45206/comment/58be7164_cea48730 PS22, Line 25: _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_ herobrine
File src/mainboard/google/herobrine/boardid.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/48b1d540_068a2df2 PS22, Line 9: #if 0 : /* TODO: update gpios for Herobrine */ : const gpio_t pins[] = {[2] = GPIO(31), [1] = GPIO(93), [0] = GPIO(33)}; : : if (id == UNDEFINED_STRAPPING_ID) : id = gpio_base2_value(pins, ARRAY_SIZE(pins)); : #endif : return id; : } : : uint32_t ram_code(void) : { : static uint32_t id = UNDEFINED_STRAPPING_ID; : #if 0 : /* TODO: update gpios for Herobrine */ : const gpio_t pins[] = {[2] = GPIO(13), [1] = GPIO(19), [0] = GPIO(29)}; : : if (id == UNDEFINED_STRAPPING_ID) : id = gpio_base2_value(pins, ARRAY_SIZE(pins)); : #endif : return id; : } : : uint32_t sku_id(void) : { : static uint32_t id = UNDEFINED_STRAPPING_ID; : #if 0 : /* TODO: update gpios for Herobrine */ : const gpio_t pins[] = {[2] = GPIO(20), [1] = GPIO(90), [0] = GPIO(105)}; : : if (id == UNDEFINED_STRAPPING_ID) : id = gpio_base2_value(pins, ARRAY_SIZE(pins)); : #endif : return id; Just leave these empty instead of using #if 0
File src/mainboard/google/herobrine/chromeos.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/64de7fdc_c08afe68 PS22, Line 9: #if 0 : return !gpio_get(GPIO_WP_STATE); : #else : return 0; : #endif : } : : void setup_chromeos_gpios(void) : { : #if 0 : /* Example of what needs to be done here */ : gpio_input_pullup(GPIO_EC_IN_RW); : gpio_input_pullup(GPIO_AP_EC_INT); : gpio_input_pullup(GPIO_SD_CD_L); : gpio_input_irq(GPIO_H1_AP_INT, IRQ_TYPE_RISING_EDGE, GPIO_PULL_UP); : gpio_output(GPIO_AMP_ENABLE, 0); : #endif : } : : void fill_lb_gpios(struct lb_gpios *gpios) : { : #if 0 : /* Example of what needs to be done here */ : struct lb_gpio chromeos_gpios[] = { : {GPIO_EC_IN_RW.addr, ACTIVE_LOW, gpio_get(GPIO_EC_IN_RW), : "EC in RW"}, : {GPIO_AP_EC_INT.addr, ACTIVE_LOW, gpio_get(GPIO_AP_EC_INT), : "EC interrupt"}, : {GPIO_H1_AP_INT.addr, ACTIVE_LOW, gpio_get(GPIO_H1_AP_INT), : "TPM interrupt"}, : {GPIO_SD_CD_L.addr, ACTIVE_LOW, gpio_get(GPIO_SD_CD_L), : "SD card detect"}, : {GPIO_AMP_ENABLE.addr, ACTIVE_HIGH, gpio_get(GPIO_AMP_ENABLE), : "speaker enable"}, : }; : : lb_add_gpios(gpios, chromeos_gpios, ARRAY_SIZE(chromeos_gpios)); : #endif Same here. Please don't just #if 0 blocks of code that isn't correct. IMHO it's harder to see code mods.