Attention is currently required from: Anil Kumar K, Arthur Heymans, Bora Guvendik, Chen, Gang C, Cliff Huang, Jamie Ryu, Jérémy Compostella, Kane Chen, Karthik Ramasubramanian, Krishna P Bhat D, Lance Zhao, Pratikkumar V Prajapati, Ravishankar Sarawadi, Ronak Kanabar, Shuo Liu, Subrata Banik, Tim Wawrzynczak, V Sowmya, Wonkyu Kim.
Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77255?usp=email )
Change subject: [Squash]: Add ACPI BDAT support ......................................................................
Patch Set 16:
(22 comments)
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/77255/comment/27f588f9_a1e5a9e3 : PS15, Line 1106: unsigned long long
IMO, `uint64_t` would keep the line shorter while also making the understanding of this line slightl […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/935615f9_7f923593 : PS15, Line 1108: /* Calculate checksum */ : header->checksum = acpi_checksum((void *)bdat, header->length);
why? […]
Acknowledged
File src/soc/intel/common/block/acpi/acpi_bdat.c:
https://review.coreboot.org/c/coreboot/+/77255/comment/d90aa1fe_cf4f15ea : PS15, Line 12: #define INTEL_FSP_BDAT_SCHEMA_LIST_HOB_GUID \
What about ? […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/993c8668_b8725d78 : PS15, Line 26: if (guid != NULL) {
that test seems unnecessary for a static function called with a non-NULL parameter according the cod […]
Replaced it with fsp_print_guid per other reviewer's comments.
https://review.coreboot.org/c/coreboot/+/77255/comment/f8f1988d_3d6fd2f3 : PS15, Line 23: static void print_guid(EFI_GUID *guid) : { : int i; : if (guid != NULL) { : printk(BIOS_DEBUG, "GUID = "); : printk(BIOS_DEBUG, "0x%08x-", guid->Data1); : printk(BIOS_DEBUG, "0x%04x-", guid->Data2); : printk(BIOS_DEBUG, "0x%04x-", guid->Data3); : for (i = 0; i < GUID_TAIL_BYTES; i++) { : printk(BIOS_DEBUG, "0x%02x", guid->Data4[i]); : if (i != (GUID_TAIL_BYTES-1)) : printk(BIOS_DEBUG, "-"); : } : printk(BIOS_DEBUG, "\n"); : } : }
this is reimplemented a lot. Can it be added to common code somewhere or reuse existing code.
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/0d90f660_68378e06 : PS15, Line 40: get_crc16
Use CRC marcro?
IMHO, it might be too complex converting to a macro. Let me know if you're fine with it.
https://review.coreboot.org/c/coreboot/+/77255/comment/d90a0f37_aa4fd3e8 : PS15, Line 62: EFI_GUID *guid;
Please use the <efi/efi_datatypes.h> version of the UEFI types.
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/b3baa188_0de25b2e : PS15, Line 63: const uint32_t *schema[MAX_SCHEMA_LIST_LENGTH] = {NULL}; : uint32_t schema_size[MAX_SCHEMA_LIST_LENGTH];
This intermediate data structure could be simplified as a structure: […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/3883b0c3_b10ae00c : PS15, Line 67: const uint8_t bdat_header_sign[] = {'B', 'D', 'A', 'T', 'H', 'E', 'A', 'D'};
I would make is a static variable of the C module instead of a stack variable. […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/7c06f692_f7e23090 : PS15, Line 74: printk(BIOS_ERR, "No BDAT data exists.\n");
`No BDAT data found. […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/e43a3763_5fdb192e : PS15, Line 77: ld
zu
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/0f54d42f_1055175d : PS15, Line 78: (void *)
Is that case really necessary ?
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/0c28405e_b1bd5ffd : PS15, Line 86: if (guid != NULL)
reduce indentation: […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/961f431f_29c89c3f : PS15, Line 86: if (guid != NULL) {
How can `guid` be `NULL` ?
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/3d30374a_436c5bb2 : PS15, Line 88: uint8_t *p = (uint8_t *)guid;
This intermediate variable seems unnecessary.
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/30be2b6e_efc49f86 : PS15, Line 91: ld
zu
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/4c18a0e7_1836ee67 : PS15, Line 92: (void *) schema[schema_count], hob_size);
weird indentation.
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/c8374445_d60e754d : PS15, Line 146: ld
zu
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/eca3d54a_a01b2a04 : PS15, Line 155: ) b
Coding style: Usually there is no space between the closing parenthesis of the cast and the name of […]
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/7616221d_a5164d1c : PS15, Line 162: &bdat_header->bios_data_sig[0]
`bdat_header->bios_data_sig` ?
Done
https://review.coreboot.org/c/coreboot/+/77255/comment/b4effdb8_f078c793 : PS15, Line 174: schema_data
unnecessary intermediate variable.
Done
File src/soc/intel/common/block/include/intelblocks/acpi_bdat.h:
https://review.coreboot.org/c/coreboot/+/77255/comment/9faa9d36_889f2023 : PS15, Line 69:
that's quite a log of tabs. A single space would do it.
Done