Attention is currently required from: Julius Werner, Kyösti Mälkki. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58896 )
Change subject: ChromeOS: Add legacy mainboard_ec_running_ro() ......................................................................
Patch Set 7:
(3 comments)
File src/include/bootmode.h:
https://review.coreboot.org/c/coreboot/+/58896/comment/4f496264_326bc929 PS7, Line 17: bool mainboard_ec_running_ro(void);
(see https://review.coreboot.org/c/coreboot/+/58896/7/src/vendorcode/google/chrom.... […]
ah right ec_is_trusted() has the latched behavior.
File src/vendorcode/google/chromeos/gnvs.c:
https://review.coreboot.org/c/coreboot/+/58896/comment/f961b7fc_f6c62ff7 PS5, Line 42: chromeos_acpi = cbmem_add(CBMEM_ID_ACPI_CNVS, sizeof(struct chromeos_acpi));
What should we do on S3 resume path? Never modify CNVS?
That would be my thinking, I can't see a good reason that any of this should be modified during resume.
File src/vendorcode/google/chromeos/gnvs.c:
https://review.coreboot.org/c/coreboot/+/58896/comment/6fb1b1a5_3945e4d9 PS7, Line 67: }
... Can't we just get rid of this and related callbacks?
That's a good point Julius, I guess is a fallback ? b/c of EC sync, it does make sense to just leave this to depthcharge.
What does the existing code do?
ACPI tables are not re-written during S3 suspend, the boot state machine resumes OS execution before BS_WRITE_TABLES anyway, so they are just left alone in the memory from the original boot. I don't think going in and tinkering with any existing tables seems like a good idea.
-
Tim Wawrzynczak (Code Review)