[coreboot-gerrit] [L] Change in coreboot[main]: [Squash]: Add ACPI BDAT support

[Jérémy Compostella (Code Review)]

Attention is currently required from: Anil Kumar K, Bora Guvendik, Chen, Gang C, Cliff Huang, Gaggery Tsai, Jamie Ryu, Kane Chen, Karthik Ramasubramanian, Krishna P Bhat D, Lance Zhao, Pratikkumar V Prajapati, Ravishankar Sarawadi, Ronak Kanabar, Subrata Banik, Tim Wawrzynczak, V Sowmya, Wonkyu Kim.

Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77255?usp=email )

Change subject: [Squash]: Add ACPI BDAT support
......................................................................

Patch Set 15:

(15 comments)

File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/77255/comment/e5be25d6_65bcd6d0 :
PS15, Line 1106: unsigned long long
IMO, `uint64_t` would keep the line shorter while also making the understanding of this line slightly more straightforward.

File src/soc/intel/common/block/acpi/acpi_bdat.c:

https://review.coreboot.org/c/coreboot/+/77255/comment/da16c5cf_25f61254 :
PS15, Line 12: #define INTEL_FSP_BDAT_SCHEMA_LIST_HOB_GUID \
What about ?
```
static const efi_guid_t BDAT_SCHEMA_LIST_HOB_GUID = { ... };
```

https://review.coreboot.org/c/coreboot/+/77255/comment/2fde28b2_30344b67 :
PS15, Line 26: 	if (guid != NULL) {
that test seems unnecessary for a static function called with a non-NULL parameter according the code below.

https://review.coreboot.org/c/coreboot/+/77255/comment/5ea7c9ca_357b25f3 :
PS15, Line 62: 	EFI_GUID *guid;
Please use the <efi/efi_datatypes.h> version of the UEFI types.

https://review.coreboot.org/c/coreboot/+/77255/comment/576bf960_ac795803 :
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:
```
const struct {
    uint32_t *address;
    uint32_t size;
} schema[MAX_SCHEMA_LIST_LENGTH];
```

It should make your code less cumbersome IMO

https://review.coreboot.org/c/coreboot/+/77255/comment/e23e9ae2_7ccf94b3 :
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.
```
static const uint8_t BDAT_SIGNATURE[] = {'B', 'D', 'A', 'T', 'H', 'E', 'A', 'D'};
```

https://review.coreboot.org/c/coreboot/+/77255/comment/053687c3_1d525d02 :
PS15, Line 74: 		printk(BIOS_ERR, "No BDAT data exists.\n");
`No BDAT data found.\n` ?

https://review.coreboot.org/c/coreboot/+/77255/comment/ff2fa4ba_11696f44 :
PS15, Line 78: (void *)
Is that case really necessary ?

https://review.coreboot.org/c/coreboot/+/77255/comment/0bebf937_ec9a889d :
PS15, Line 86: 		if (guid != NULL) {
How can `guid` be `NULL` ?

https://review.coreboot.org/c/coreboot/+/77255/comment/b8238e4c_06b2c328 :
PS15, Line 88: uint8_t *p = (uint8_t *)guid;
This intermediate variable seems unnecessary.

https://review.coreboot.org/c/coreboot/+/77255/comment/b6ae903a_1034e495 :
PS15, Line 92: 								(void *) schema[schema_count], hob_size);
weird indentation.

https://review.coreboot.org/c/coreboot/+/77255/comment/e06b9cc8_02972f30 :
PS15, Line 155: ) b
Coding style: Usually there is no space between the closing parenthesis of the cast and the name of the variable.

https://review.coreboot.org/c/coreboot/+/77255/comment/4bef3325_852989b4 :
PS15, Line 162: &bdat_header->bios_data_sig[0]
`bdat_header->bios_data_sig` ?

https://review.coreboot.org/c/coreboot/+/77255/comment/e60220ca_89c51733 :
PS15, Line 174: schema_data
unnecessary intermediate variable.

File src/soc/intel/common/block/include/intelblocks/acpi_bdat.h:

https://review.coreboot.org/c/coreboot/+/77255/comment/ae5ebae7_0864278c :
PS15, Line 69: 				
that's quite a log of tabs. A single space would do it.