Attention is currently required from: Furquan Shaikh, Francois Toguo Fotso, Tim Wawrzynczak, Nick Vaccaro, Karthik Ramasubramanian. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57785 )
Change subject: soc/intel/block: Add small crashlog helper library ......................................................................
Patch Set 3:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57785/comment/02908934_106404db PS3, Line 7: soc/intel/block: Add small crashlog helper library how about ?
soc/intel/block/crashlog: Add helper functions for telemetry dvsec
File src/soc/intel/common/block/crashlog/crashlog_lib.h:
https://review.coreboot.org/c/coreboot/+/57785/comment/d9f48d3f_3e651eb3 PS3, Line 13: DVSEC_VSEC0_OFFSET TELEM_VSEC_0 to make it more relevant with while reviewing EDS
Also, https://review.coreboot.org/c/coreboot/+/57785/3/src/soc/intel/common/block/... you have actually followed EDS, so better we maintain the parity between offset and bit definitions. WDYT?
https://review.coreboot.org/c/coreboot/+/57785/comment/39026c88_6413a252 PS3, Line 16: 0x04
Curious why this is 0x04 v/s 0x4 for DVSEC_VSEC0_OFFSET? Is it because the other field is 8-bit and […]
I could see the same in FSP reference code https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/adl/... and https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/adl/...
so, you are right.
https://review.coreboot.org/c/coreboot/+/57785/comment/862b8b12_2d302622 PS3, Line 97: discovery_type
Is this discovery_type or is this dvsec_id?
As per EDS, its same, one is bit field name and another macro name. I guess Tim maintain bit fields name across hence its okay to say "discovery_type".
https://review.coreboot.org/c/coreboot/+/57785/comment/44e5c969_3446e5ed PS3, Line 118: crashlog_read64 any reason for implementing 64-bit read ?
https://review.coreboot.org/c/coreboot/+/57785/comment/7d5dc344_c5c317c4 PS3, Line 132: offset src?
https://review.coreboot.org/c/coreboot/+/57785/3/src/soc/intel/common/block/...
File src/soc/intel/common/block/crashlog/crashlog_lib.c:
https://review.coreboot.org/c/coreboot/+/57785/comment/7f373de6_36a260c9 PS3, Line 4: #include "crashlog_lib.h"
nit - local "" includes should follow system <> includes ordering-wise, with blank line between the […]
echo to what Nick said. Example: fast_spi_def.h inside common code fast_spi module.
https://review.coreboot.org/c/coreboot/+/57785/comment/7c02a296_7e9fc992 PS3, Line 10: dest care to check NULL pointer for `dest` ?
-
Subrata Banik (Code Review)