Attention is currently required from: Tim Wawrzynczak, Kyösti Mälkki. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58896 )
Change subject: ChromeOS: Add legacy mainboard_ec_running_ro() ......................................................................
Patch Set 13:
(1 comment)
File src/vendorcode/google/chromeos/gnvs.c:
https://review.coreboot.org/c/coreboot/+/58896/comment/1a737451_a32bff7f PS7, Line 67: }
Case of S3 resume path is addressed with line 47, right? […]
Yes, that's what I mean. I don't think this field was ever meant to be filled out by coreboot (because it can't know what the right value for this will be by the time the OS is running -- anything we could fill in here is premature). I think for CONFIG_CHROMEOS code we can assume that depthcharge and Chrome OS are the only use cases... and this field is only read by the chromeos-specific `crossystem` utility anyway (not by kernel code), if you didn't run through depthcharge there'll be many more broken assumptions in there anyway.
My point here is that if we don't need to fill out this field, then we don't need to add a mainboard_ec_running_ro() callback either, so I don't think you need this whole patch. I'd just remove the chromeos_set_ecfw_rw() calls and this code block without adding anything new in its stead.
-
Julius Werner (Code Review)