Attention is currently required from: Ravi kumar, Shelley Chen, Paul Menzel, Julius Werner. Ravi Kumar Bokka has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45206 )
Change subject: herobrine: sc7280: Provide initial mainboard support ......................................................................
Patch Set 23:
(50 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/45206/comment/ba4dcabb_ebce6f9f PS2, Line 7: herobrine :
Please remove the space before the colon.
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/8d11552f_90efaa89 PS2, Line 7: herobrine : Provide initial mainboard support
Please mention the SoC in the commit message.
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/3f3d43d7_723be067 PS2, Line 10: Signed-off-by: Ravi
One space is enough.
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/45206/comment/7db188a5_495b8799 PS15, Line 7: herobrine :
Please remove the space before the colon.
Done
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/45206/comment/f2bcdb95_59c8c3e7 PS15, Line 262: default n
Please split out the libpayload changes into a separate commit.
This comment address with below gerrit: https://review.coreboot.org/c/coreboot/+/47527/5
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/45206/comment/31048a50_4bfdc632 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.
addressed below: https://review.coreboot.org/c/coreboot/+/47527/5/payloads/libpayload/Kconfig
File payloads/libpayload/configs/config.herobrine:
https://review.coreboot.org/c/coreboot/+/45206/comment/fa1f2018_fe261bee PS6, Line 5: CONFIG_LP_SC7280_SERIAL_CONSOLE=y
Is the serial driver actually going to be different? I was hoping the QUPs stayed the same between t […]
This changes addressed below gerrit: https://review.coreboot.org/c/coreboot/+/47527
File payloads/libpayload/configs/config.herobrine:
https://review.coreboot.org/c/coreboot/+/45206/comment/69526943_a01d3d93 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_CONSO […]
addressed below
https://review.coreboot.org/c/coreboot/+/47527/5/payloads/libpayload/configs...
File payloads/libpayload/drivers/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45206/comment/c10d78f4_b43e2849 PS22, Line 45: serial/sc7280.c
are we able to reuse serial/qcom_qupv3_serial. […]
addressed this comment with below gerrit: https://review.coreboot.org/c/coreboot/+/47527/5
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/43f88634_5fb25d25 PS1, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/9fa2b3f3_199cf84f PS2, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/8c468ffc_3c36394e PS3, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/893489da_825f5c49 PS4, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/8da2d404_733048c4 PS5, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/e8238c12_56a0f1b9 PS6, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/9fcc762c_8cd27a1b PS7, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/bc3c1209_19c8621c PS8, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/e4a3694f_509f3d2d PS9, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/85eacc22_ff454fd6 PS10, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/7c2522cc_cd6e605e PS11, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/d98a8b86_e7a0a5ac PS12, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/32aa59bc_17ec8dac PS13, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/2cf17a31_2bd4a7d8 PS14, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/a2693d2a_68d8d0f7 PS15, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/2858eb09_7d3c8b76 PS16, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/6b36ccb2_4b418d56 PS17, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/3e3253c7_795825f3 PS18, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/5f2d07db_883baeaf PS19, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/996332be_e25bc828 PS20, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/09e1c6dd_28e3ddd7 PS21, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/c180b98e_d084e2e0 PS22, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File payloads/libpayload/drivers/serial/sc7280.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/854fa903_0bcf8267 PS23, Line 5: /* For simplicity sake let's rely on coreboot initalizing the UART. */
'initalizing' may be misspelled - perhaps 'initializing'?
done
File src/mainboard/google/herobrine/Kconfig:
https://review.coreboot.org/c/coreboot/+/45206/comment/3ec4edfa_62111b23 PS6, Line 40: default 0xa
Please don't define these before they're known. Just don't select the EC Kconfig for now.
can i use below for disable EC default n
https://review.coreboot.org/c/coreboot/+/45206/comment/457c209e_4f4b0969 PS6, Line 50: config GBB_HWID
See the current Trogdor Kconfig, you don't need this option here anymore.
Done
File src/mainboard/google/herobrine/Kconfig:
https://review.coreboot.org/c/coreboot/+/45206/comment/4e496ab7_24d59184 PS22, Line 50: onfig GBB_HWID : string : depends on CHROMEOS : default "HEROBRINE TEST 1859" if BOARD_GOOGLE_HEROBRINE
Yes, this will get auto-generated correctly now, just leave it out here.
Done
File src/mainboard/google/herobrine/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/45206/comment/e8b99671_5631acd5 PS22, Line 1:
Add: […]
i will address this comment, once discuss with internally
File src/mainboard/google/herobrine/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45206/comment/55fdb6e4_38b5b27f PS22, Line 6: bootblock-y += reset.c
For utility functions like board_reset() we often just add it everywhere rather than worry about whe […]
Done
File src/mainboard/google/herobrine/board.h:
https://review.coreboot.org/c/coreboot/+/45206/comment/d4db269d_110d2e9f PS6, Line 8: #include <soc/gpio.h>
maybe you can remove direct 'include <soc/gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/90f4e82d_8fa6f599 PS6, Line 21: #define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30))
Please don't copy these GPIO tables, they'll be totally different for the new board.
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/ee0166ce_246a208a PS6, Line 21: #define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30))
Please don't copy these GPIO tables, they'll be totally different for the new board.
Done
File src/mainboard/google/herobrine/board.h:
https://review.coreboot.org/c/coreboot/+/45206/comment/5e3b99ad_0b8385d9 PS22, Line 3: #ifndef _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_ : #define _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_
herobrine
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/736292a2_59e1fd0e 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))
Since there are no herobrine schematics yet (I think?) none of these are known yet. […]
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/628066ef_3896f170 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))
Since there are no herobrine schematics yet (I think?) none of these are known yet. […]
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/4c3c34a2_48d0bc26 PS22, Line 25: _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_
herobrine
Done
File src/mainboard/google/herobrine/boardid.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/b701ab3c_483bd7a1 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
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/3330d813_094a3dc8 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
Done
File src/mainboard/google/herobrine/chromeos.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/0490794a_c4ff5460 PS6, Line 9: #if 0
Just don't add any of this stuff if you don't have code for it yet.
Done
https://review.coreboot.org/c/coreboot/+/45206/comment/9f8f7000_d4de2c8b PS6, Line 9: #if 0
Just don't add any of this stuff if you don't have code for it yet.
Done
File src/mainboard/google/herobrine/chromeos.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/107dbdc7_96a1a02b 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. […]
Done
File src/mainboard/google/herobrine/mainboard.c:
https://review.coreboot.org/c/coreboot/+/45206/comment/d9412d7c_eb1f6574 PS6, Line 5: #include <arch/mmio.h> : #include <gpio.h> : #include <timestamp.h>
maybe not used
Done