Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33382 )
Change subject: [WIP] mb/google/{sarien, arcada} Fix for SSD can't be detected issue ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/ramstage... File src/mainboard/google/sarien/ramstage.c:
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/ramstage... PS3, Line 83: ssd_reset maybe ssd_deassert_reset() to be clear what it is doing.
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/ramstage... PS3, Line 85: GPP_H12 Since this is used in multiple places it is probably a good idea to add something to variant/gpio.h like
#define GPIO_SSD_RESET GPP_H12 #define GPIO_SSD_POWER GPP_H13
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/ramstage... PS3, Line 87: extra newline
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/romstage... File src/mainboard/google/sarien/romstage.c:
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/romstage... PS3, Line 67: ps->gen_pmcon_a & (PWR_FLR | SUS_PWR_FLR) soc/intel/cannonlake/pmutil.c:soc_prev_sleep_state() should be initializing ps->prev_sleep_state to ACPI_S5 based on this same check. Would it be possible to use that or even acpi_get_sleep_type() instead?
https://review.coreboot.org/#/c/33382/3/src/mainboard/google/sarien/romstage... PS3, Line 69: 75 Make this a #define.
Sigh, 75ms to every cold boot...