mturney mturney has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35495 )
Change subject: trogdor: Provide initial mainboard support ......................................................................
Patch Set 9:
(20 comments)
Patch Set 8:
(3 comments)
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)
Done
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:
Done
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 23: #define GPIO_EC_IN_RW GPIO(11)
118
Done
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 24: #define GPIO_AP_EC_INT GPIO(122)
94
Done
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 25: #define GPIO_AP_SUSPEND GPIO(126)
20
Done
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 26: #define GPIO_WP_STATE GPIO(128)
42
Done
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 27: #define GPIO_H1_AP_INT GPIO(129)
21
Done
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?
Done
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 pat […]
Done
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
Done
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
Done
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
Done
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 […]
Done
https://review.coreboot.org/c/coreboot/+/35495/8/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/35495/8/src/mainboard/google/trogdo... PS8, Line 19: #if 0
Why is this commented out? […]
Done
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... […]
Done
https://review.coreboot.org/c/coreboot/+/35495/5/src/mainboard/google/trogdo... PS5, Line 18: 3M
4M
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35495/8/src/mainboard/google/trogdo... File src/mainboard/google/trogdor/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/35495/8/src/mainboard/google/trogdo... PS8, Line 16: # TODO: update for Trogdor
Isn't this updated now? Anything still missing?
Done
https://review.coreboot.org/c/coreboot/+/35495/8/src/mainboard/google/trogdo... PS8, Line 22: #TODO: Move FMAP to 2M or 3M once FSG can be smaller
The only thing I'd consider is doing this right away... […]
Done
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)
Done