Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Kane Chen, Karthik Ramasubramanian. Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57683 )
Change subject: soc/intel/crashlog: Rip out crashlog drivers ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Patchset:
PS2: Additinal comments below: -> This would be a code re-organization rather than a "rip out". The bulk of the implementation in the follow up CLs is still the same as the previous Intel implementation.
"3) Code duplication" -> There are files in different SOC folders with same functions are similar code. These are not "code duplication" and therefore never moved to the common code. Duplicated device ids are an example. This was addressed in several reviews.
"5) Lots of unnecessary weak functions" -> please refer to the "general statement" below
"6) The "Enable PMC collection on all reboots" feature does not work" -> once again this is not a driver, but rather an IP, issue. Are the subsequent CL fixing this?
"7) Prior bugs have already been found in it" -> validation was performed and documented by Intel and Google, with the previous CL, in partner issues. Please list the bug id (or partner issues number) of those other bugs here.
general statement: CrashLog is a new and still evolving feature. Many architectural flow details (including underlining IPs implementation) are Intel's proprietary information and are not publicly available. The existing code, along with the needed weak functions, was implemented with knowledge of current and futures architectural, potential, changes being discussed internally. Consequently even warranted code re-organization, into pci devices, should be done after consultation with the coreboot owner of the feature on the Intel's side. Failure to do this might result in real code duplication, and true unnecessary weak function, when upcoming architectural changes are no longer in line with the newly re-organized code.