[M] Change in coreboot[main]: soc/amd/glinda/cpu: Compute total L3 cache size

[Felix Held (Code Review)]

Attention is currently required from: Angel Pons, Naresh Solanki.

Felix Held has posted comments on this change by Naresh Solanki. ( https://review.coreboot.org/c/coreboot/+/85640?usp=email )

Change subject: soc/amd/glinda/cpu: Compute total L3 cache size
......................................................................

Patch Set 9:

(10 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85640/comment/bd4c7063_93d5b73f?usp... :
PS9, Line 22: 
maybe also point out that this also covers the case of l3 cache blocks having different sizes?

File src/soc/amd/glinda/cpu.c:

PS9: 
since glinda won't be the only amd soc where this is needed, please consider moving the implementation to a new c file in soc/amd/common/block/cpu/noncar/. i'd then introduce a new kconfig option and only include the new compilation unit when that's selected and select that kconfig option in the glinda code and only keep the ap_get_l3_cache_sizes call in here. that will allow to easily add this support to other socs that need it too. that kconfig could then also select the kconfig option i suggested adding in CB:85638

https://review.coreboot.org/c/coreboot/+/85640/comment/1de17095_65fd4e0f?usp... :
PS9, Line 22: get
i'd use a different word than 'get' here, since my expectation from a function where the name contains 'get' would be that it returns a return value to the caller. maybe 'ap_stash_l3_cache_sizes'?

https://review.coreboot.org/c/coreboot/+/85640/comment/97db6a1f_c40085d5?usp... :
PS9, Line 24: cpuid
since we only need the data in ebx, i'd use cpuid_ebx instead of cpuid here

https://review.coreboot.org/c/coreboot/+/85640/comment/2eabba92_115ca1e3?usp... :
PS9, Line 24: 0x8000001e
CPUID_EBX_CORE_ID

https://review.coreboot.org/c/coreboot/+/85640/comment/06043141_117f7cc4?usp... :
PS9, Line 27: 	uint8_t level = 3;
this could be made const

https://review.coreboot.org/c/coreboot/+/85640/comment/6860ea80_8719adcb?usp... :
PS9, Line 27: 3
CACHE_L3

https://review.coreboot.org/c/coreboot/+/85640/comment/3e735c13_7efc44ed?usp... :
PS9, Line 38: 3
CACHE_L3

https://review.coreboot.org/c/coreboot/+/85640/comment/74cc8ec1_d5c69b12?usp... :
PS9, Line 49: 	for (int i = 0; i < get_cpu_count(); i++) {
a comment on this for loop might make it a bit easier to understand. maybe "to calculate the total L3 cache size, iterate over cache_info_list and add up the sizes of the L3 cache blocks with unique cache ID" or something like that?

https://review.coreboot.org/c/coreboot/+/85640/comment/6e68e201_396243f4?usp... :
PS9, Line 59: 	}
i'd add a newline here