Jason Glenesk 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 4:
(11 comments)
https://review.coreboot.org/c/coreboot/+/47727/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47727/3//COMMIT_MSG@15 PS3, Line 15: FPS
FSP
Done
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
I agree with the argument about not using typedefs (not to mention using name_t). […]
Adding to the list to scrub/convert typedefs to structs for this file in a follow up.
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
Done
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.
Done
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. […]
Done
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. […]
Done
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.
Done
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.
Done
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. […]
I'm glad you mentioned this. I had read in the spec that all the sub categories were required, but i missed the end of the end of the sentence. I removed these entirely.
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. […]
It is zeroed out when the header is created in acpi_create_crat(). Each node is a memory locality.
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
Done
-
Jason Glenesk (Code Review)