[L] Change in coreboot[master]: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI

[Angel Pons (Code Review)]

Attention is currently required from: Patrick Rudolph, Lean Sheng Tan, [e-mail].

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68137 )

Change subject: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI
......................................................................

Patch Set 2:

(7 comments)

File src/mainboard/prodrive/atlas/vpd.h:

https://review.coreboot.org/c/coreboot/+/68137/comment/bae73984_f7243b45 
PS2, Line 3: #ifndef __VPD_H__
It's a good idea to "namespace" include guards, to avoid funky issues when two different headers use the same symbol for the include guard. `src/drivers/vpd/vpd.h` already uses the `__VPD_H__` symbol, so if you try including both headers into the same compilation unit, only one of them (the first one found by the preprocessor) will be included.

TL;DR: Let's play it safe and use something like `__ATLAS_VPD_H__` to avoid potential issues with include guards.

https://review.coreboot.org/c/coreboot/+/68137/comment/c39d1da9_9db3a28d 
PS2, Line 6: #include <stddef.h>
You use u16 and u8 below, so you'd need <stdint.h>. But you can just #include <types.h> for the sake of simplicity

https://review.coreboot.org/c/coreboot/+/68137/comment/a64c9ca3_64d0f10e 
PS2, Line 12: vpd_section
> the first field must a length field or a version field in order to extend this struct in the future.

Not sure if we'll need to extend this in the future, but it's a good idea. It would also allow providing backwards compatibility with older EC firmware.

On the Hermes, we ended up (ab)using the checksum for board settings in the Hermes to add backwards compatibility with older BMC firmware, see CB:67381 for the change.

https://review.coreboot.org/c/coreboot/+/68137/comment/e6148420_06715aba 
PS2, Line 17: } vpd_section_t;
> How do you know if the returned data is valid? Can you add a checksum?

If you want to add a checksum, it's a good idea to have it at the start, so that the "header" doesn't change:

```
struct __packed vpd_section {
    u32 revision;
    u32 checksum;
    union {
    	struct __packed {
    		char part_number[PAGESIZE];
    		char serial_number[PAGESIZE];
    	};
    	u8 raw_vpd[PAGESIZE + PAGESIZE];
    };
};
```

https://review.coreboot.org/c/coreboot/+/68137/comment/f9912860_55b23ed8 
PS2, Line 24:  * */
nit: Coding style says that these comments should end like this:

```
 * within the vpd section.
 */
```

https://review.coreboot.org/c/coreboot/+/68137/comment/87b1857d_9c49c1a9 
PS2, Line 32: extern
We typically don't use `extern` in declarations inside a header.

https://review.coreboot.org/c/coreboot/+/68137/comment/829970df_d0debb8f 
PS2, Line 36: 
There are two trailing newlines, we only need one.