Change in coreboot[master]: intel/common/block/memory: Add saving memory info API to common block

[Furquan Shaikh (Code Review)]

Attention is currently required from: Martin Roth, Selma Bensaid, Tim Wawrzynczak, Subrata Banik, Bernardo Perez Priego, Usha P, Andrey Petrov, Patrick Rudolph.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51105 )

Change subject: intel/common/block/memory: Add saving memory info API to common block
......................................................................

Patch Set 7:

(13 comments)

File src/soc/intel/common/block/memory/Kconfig:

https://review.coreboot.org/c/coreboot/+/51105/comment/ab4e4ea3_8f53a89a 
PS7, Line 30: endif
The new Kconfig should probably be under this if...endif block?

File src/soc/intel/common/block/memory/meminfo.c:

https://review.coreboot.org/c/coreboot/+/51105/comment/a0a9fe0e_7a4bae19 
PS7, Line 3: #include <arch/symbols.h>
Is this required?

https://review.coreboot.org/c/coreboot/+/51105/comment/11085c5a_fc276ba6 
PS7, Line 4: #include <assert.h>
           : #include <cbfs.h>
I don't think these are required.

https://review.coreboot.org/c/coreboot/+/51105/comment/75fbb5af_b69e020f 
PS7, Line 13: #include <smbios.h>
Is this required?

https://review.coreboot.org/c/coreboot/+/51105/comment/7352e9c4_670040fb 
PS7, Line 22: CONTROLLER_INFO
Just curious:
Are CONTROLLER_INFO, CHANNEL_INFO, DIMM_INFO, MAX_NODE, MAX_CH, etc. definitions pulled in by fsp/api.h? It would be good to capture these expectations in meminfo.h and outline all the macros that are expected to be defined by the SoC FSP headers.

https://review.coreboot.org/c/coreboot/+/51105/comment/bd56dfe8_da3802f8 
PS7, Line 36: SMBIOS
Add "ERROR:" prefix so that it gets caught by scripts.

https://review.coreboot.org/c/coreboot/+/51105/comment/b8015820_44e5aba6 
PS7, Line 46: CBMEM
Same here with "ERROR:" prefix.

https://review.coreboot.org/c/coreboot/+/51105/comment/1b127c7c_445fc1ed 
PS7, Line 46: entry for DIMM info missing
Rather than missing, it could not be allocated.

https://review.coreboot.org/c/coreboot/+/51105/comment/8a0af272_0d296877 
PS7, Line 64: channel++) {
This most likely fits on the previous line with 96-character limit.

https://review.coreboot.org/c/coreboot/+/51105/comment/51d76e0f_09ba2cd6 
PS7, Line 70: dimm++) {
This too most likely fits on the previous line.

https://review.coreboot.org/c/coreboot/+/51105/comment/a79d500f_1cc91a26 
PS7, Line 77: MRC_DDR_TYPE_DDR4
Are these MRC memory types supposed to be different than the SMBIOS memory types? I remember that MRC was using a different memory type for LPDDR3, but haven't seen that for any other memory types. It would be good to restrict this to only those types that actually need a conversion and also add a comment here explaining why that is being done.

https://review.coreboot.org/c/coreboot/+/51105/comment/2efc8de7_8c697d0d 
PS7, Line 90: /* If there is no DRAM part number overridden by
            : 				 * mainboard then use original one. */
Please use one of the recommended comment styles: https://doc.coreboot.org/contributing/coding_style.html#commenting

https://review.coreboot.org/c/coreboot/+/51105/comment/bd92c034_41231d09 
PS7, Line 100: SPD_SAVE_OFFSET_SERIAL;
This probably fits on the previous line.