David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35367 )
Change subject: mb/ocp/monolake: Add GPIO table to initialize custom configs ......................................................................
Patch Set 5:
(20 comments)
BTW, we recently started requiring that all comments are resolved prior to a patch being merged. I went ahead and marked all comments as such, though I'm not 100% sure about the lint warning (>96 characters on some lines, which is due to responding to a reviewer request).
I've asked others about the column width, and will merge this patch if they're OK with it.
https://review.coreboot.org/c/coreboot/+/35367/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35367/1//COMMIT_MSG@9 PS1, Line 9: Add GPIO table for Monolake to initialize GPIOs.
We simply want to have a gpio table that can be configured in a centrally managed way.
Ack
https://review.coreboot.org/c/coreboot/+/35367/1//COMMIT_MSG@10 PS1, Line 10: Tested on Monolake
Verified by using ITP to check the configurations
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/gpio.h:
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 31: {9, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
GPIO9 and GPIO10 are overcurrent indicators, should they be left in native function (default) state?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 36: {14, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
GPIO14 is an overcurrent indicators, should it be left in native function (default) state?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 46: {24, GPIO_MODE_NATIVE, 0, 0, 0, 0},
I think this is acting as MGPIO0?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 50: {28, GPIO_MODE_NATIVE, 0, 0, 0, 0},
I think GPIO27 and GPIO28 are acting as MGPIO6 and MGPIO7?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 53: {31, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
I think this is acting as MGPIO2?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 56: {34, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
Oddly enough I don't even see GPIO34 mentioned in schematic, EDS, or other datasheets... […]
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 61: {39, GPIO_MODE_NATIVE, 0, 0, 0, 0},
Should SATA0GP (GPIO21) be set to native function as well?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 65: {43, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
GPIO40-43 are overcurrent indicators, should they be left in default (native function) state?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 69: {47, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
Same as GPIO34 - This pin doesn't seem to really exist... […]
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 78: {56, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
Another missing GPIO (like 47 and 34)?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 79: {57, GPIO_MODE_NATIVE, 0, 0, 0, 0},
I think this is acting as MGPIO5 (configured and driven by ME)?
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 86: {64, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
GPIOs 63 and 64 don't seem to exist? (Should be safe to set as inputs as you are doing... […]
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 88: {66, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
Another missing one...
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 95: {73, GPIO_MODE_GPIO, GPIO_INPUT, 0, 0, 0},
Another phantom GPIO...
Ack
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/35367/1/src/mainboard/ocp/monolake/... PS1, Line 66: init_gpios(gpio_tables);
We initialize GPIOs when fsp setup_gpio_io_address() finished, […]
Ack
https://review.coreboot.org/c/coreboot/+/35367/5/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/35367/5/src/mainboard/ocp/monolake/... PS5, Line 64: {29, GPIO_MODE_GPIO, GPIO_OUTPUT, GPIO_OUT_LEVEL_HIGH, 0, 0}, /* H_BDXDE_PROCHOT_DISABLE */
line over 96 characters
Ack. The comment was added in response to reviewer feedback.
https://review.coreboot.org/c/coreboot/+/35367/5/src/mainboard/ocp/monolake/... PS5, Line 68: {33, GPIO_MODE_GPIO, GPIO_OUTPUT, GPIO_OUT_LEVEL_HIGH, 0, 0}, /* PD_DMI_RX_TERMINATION */
line over 96 characters
Ack. The comment was added in response to reviewer feedback.
https://review.coreboot.org/c/coreboot/+/35367/5/src/mainboard/ocp/monolake/... PS5, Line 81: {46, GPIO_MODE_GPIO, GPIO_OUTPUT, GPIO_OUT_LEVEL_HIGH, 0, 0}, /* FM_BIOS_POST_CMPLT_N */
line over 96 characters
Ack. The comment was added in response to reviewer feedback.