Attention is currently required from: Dinesh Gehlot.
Subrata Banik has posted comments on this change by Dinesh Gehlot. ( https://review.coreboot.org/c/coreboot/+/84120?usp=email )
Change subject: vc/google/chromeos: Skip boot info logging if cse sync at payload ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS2:
Apologies for missing this, I lost track of some emails in the last few weeks. But honestly, I'm still not convinced this is the right move. We have always had additional resets in depthcharge as well (e.g. due to EC sync or TCPC sync). We picked the end of ramstage for this because it was the most convenient point to implement it (it already has an elog driver that is always used on all platforms anyway), not because it's a huge issue to occasionally log this twice. Depthcharge does not normally write to elog on every boot, so by adding this you are adding the overhead of linking the elog code and initializing the driver (reading the area from flash) a second time to your critical boot path.
But my main concern here is that you are making something more complicated that doesn't deserve to be that complicated. Introducing such a difference between platforms is always an ongoing maintenance burden, even if you were to move the code to commonlib. People will always have to think about where and how exactly the logging works for each particular platform, and may get confused because it works different on some than on others. There is value in keeping things simple.
The EC and PD resets are rare occurrences that we often don't encounter, so we can assume that 98% of the time during boot, we don't see redundant FW boot information logs. As a result, the test case was never adjusted to account for such edge cases. With CSE sync moving to depthcharge (planned for PTL), we'll encounter this issue frequently, as we'll see redundant FW boot information due to CSE changing the slot from RO to RW. Therefore, the probability of the test failing is higher than before, unless the test script is modified to account for the additional reboots.
The complexity arises from the fact that we're migrating things from coreboot to depthcharge, which results in duplicate code across (to optimize SPI flash usage). Ideally, I'd like to see us avoid redundant code in DC and instead pass everything as commonlib via libpayload to reduce duplicate work.