Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47727 )
Change subject: soc/amd/picasso: Generate ACPI CRAT objects in cb ......................................................................
Patch Set 3:
(10 comments)
I'll go ahead and give you some stuff to look over. I probably won't be able to finish agesa_acpi.c this afternoon.
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi.h File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi.h@311 PS3, Line 311: struct acpi_crat_header Maybe typedef this, like on line 303. And to be consistent, I'd name it acpi_crat and the typedef to acpi_crat_t.
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi_crat.... File src/include/acpi/acpi_crat.h:
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi_crat.... PS3, Line 57: #define CRAT_PICASSO_NUM_DRAM_REG 2 This looks like it doesn't belong in a src/include file
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/acpi/acpi_crat.... PS3, Line 121: = 0 I didn't see that this needs to be initialized. If I missed it, then this needs a comment.
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/cpu/amd/msr.h@1... PS3, Line 12: This change adds a whole lot of cpuid info into a file called msr.h. We should probably start to make an amd/cpuid.h file.
https://review.coreboot.org/c/coreboot/+/47727/3/src/include/cpu/amd/msr.h@1... PS3, Line 14: /* EAX Leaf */ The term "leaf" may be more Intel than AMD. I'd recommend using something similar to what's in the BKDG or MSR, e.g. Fn80000005_EAX
https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_a... File src/soc/amd/picasso/agesa_acpi.c:
https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_a... PS3, Line 499: Please split the picasso file out of the generic crat patch, and make it follow the other.
https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_a... PS3, Line 791: tlb_type It always feels a bit odd having a variable/argument that's the same name as its type.
https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_a... PS3, Line 1016: &= ~ It's not clear why we would clear an enable bit. And we just cleared the mem above.
Here plus line 1031 too.
https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_a... PS3, Line 1048: num_nodes++ Was this ever initialized? Or was it initialized to 0 by zeroing the memory in acpi.c. I never found a spec, but I can see how you're following AGESA. What's the meaning of the field, and should we be setting it instead of bumping it?
https://review.coreboot.org/c/coreboot/+/47727/3/src/soc/amd/picasso/agesa_a... PS3, Line 1064: Meh I'd remove the blank line to make the code similar to below