Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35495 )
Change subject: trogdor: initial mainboard + libpayload support ......................................................................
Patch Set 5:
(20 comments)
https://review.coreboot.org/c/coreboot/+/35495/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35495/4//COMMIT_MSG@7 PS4, Line 7: trogdor: initial mainboard + libpayload support
What's the dependency between libpayload and the mainboard enablement code in this commit? I don't s […]
Agreed, libpayload should be split out. libpayload and coreboot shouldn't affect each other build-wise. Can you link to where exactly Jenkins gave you trouble before so we can figure out what happened?
https://review.coreboot.org/c/coreboot/+/35495/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/serial/sc7180.c:
https://review.coreboot.org/c/coreboot/+/35495/5/payloads/libpayload/drivers... PS5, Line 34: void serial_console_init(void) Please don't commit empty stubs where possible, just remove the CONFIG_LP_SC7180_SERIAL_CONSOLE=y from the config for now.
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 50: config GBB_HWID The need for this was just recently eliminated (CB:35635)
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 22: /* TODO: update these for Trogdor hardware */ According to our draft schematics (not sure if we shared those with you yet), these should be:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 23: #define GPIO_EC_IN_RW GPIO(11) 118
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 24: #define GPIO_AP_EC_INT GPIO(122) 94
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 25: #define GPIO_AP_SUSPEND GPIO(126) 20
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 26: #define GPIO_WP_STATE GPIO(128) 42
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 27: #define GPIO_H1_AP_INT GPIO(129) 21
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 28: (Note that if you wait with committing sc7180 GPIO support until the patch that adds the implementation like I asked on the other CL, you'll either have to factor out the chromeos.c stuff from here into a later CL as well, or put the sc7180 GPIO implementation CL before this one in the patch order. Either way is fine.)
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/board_info.txt:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 2: Trogdor nit: "Trogdor" twice?
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/boardid.c:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 23: #if 0 Please don't submit commented out / disabled code, rather don't add this file at all until later patch.
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 25: const gpio_t pins[] = {[2] = GPIO(51), [1] = GPIO(62), [0] = GPIO(38)}; [2] = 31, [1] = 93, [0] = 33
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 38: const gpio_t pins[] = {[1] = GPIO(147), [0] = GPIO(146)}; [1] = 91, [0] = 29
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 51: const gpio_t pins[] = {[1] = GPIO(113), [0] = GPIO(79)}; [1] = 90, [0] = 114
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 31: #if 0 Same here, we don't need to commit disabled "example" code, just leave it out until you're ready to add the real thing.
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 17: 6M 8M (do 6M parts even exist? never heard of that...)
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 18: 3M 4M
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 42: RW_SECTION_A 1024K { With 8M I'd consider bumping these back up a bit (e.g. to 1280K like on Cheza), just to make sure we have enough room for potentially larger RW updates.
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 16: nit: not sure why there's a space here (here and in older boards)