Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36811 )
Change subject: soc/amd: Move SCI enable outside table creation ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c@... PS3, Line 105: = fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ fadt->acpi_disable = 0; /* unused if SMI_CMD = 0 */ should still remain in the else section.
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/picasso/acpi.c@... PS3, Line 98: fadt->s4bios_req = 0; /* Not supported */ : fadt->pstate_cnt = 0; /* Not supported */ : Same comment as for stoney, 3 lines setting values to 0 should be outside the if/else
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... File src/soc/amd/stoneyridge/acpi.c:
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 105: fadt->smi_cmd = 0; /* disable system management mode */ : fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ : These 3 line: fadt->smi_cmd = 0; /* disable system management mode */ fadt->acpi_enable = 0; /* unused if SMI_CMD = 0 */ fadt->acpi_disable = 0; /* unused if SMI_CMD = 0 * Were not moved anywhere, so should still be within an else branch.
https://review.coreboot.org/c/coreboot/+/36811/3/src/soc/amd/stoneyridge/acp... PS3, Line 100: fadt->s4bios_req = 0; /* Not supported */ These 3 lines (setting variables to 0) were common on both sides of the if/else, so now it should be outside the if.