Name of user not set #1002476 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-code revision via macro ......................................................................
acpi_table_header: Replace hard-code revision via macro
Minimize use of hard code value for acpi_table_header->revision to soft code. Replace with macro defined in arch/acpi.h
Change-Id: I99e59afc1a87203499d2da6dedaedfa643ca7eac Signed-off-by: Sourabh Kashyap Sourabhka@hcl.com --- M src/mainboard/amd/torpedo/fadt.c M src/mainboard/intel/galileo/acpi_tables.c M src/mainboard/intel/wtm2/fadt.c M src/mainboard/portwell/m107/fadt.c M src/southbridge/intel/i82371eb/fadt.c M src/southbridge/intel/lynxpoint/acpi.c M src/southbridge/nvidia/mcp55/fadt.c 7 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/35539/1
diff --git a/src/mainboard/amd/torpedo/fadt.c b/src/mainboard/amd/torpedo/fadt.c index 9e33c07..25fb448 100644 --- a/src/mainboard/amd/torpedo/fadt.c +++ b/src/mainboard/amd/torpedo/fadt.c @@ -50,7 +50,7 @@ memset((void *)fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = 244; - header->revision = 1; + header->revision = ACPI_FADT_REV_ACPI_1_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/mainboard/intel/galileo/acpi_tables.c b/src/mainboard/intel/galileo/acpi_tables.c index 78b05ab..bba8ced 100644 --- a/src/mainboard/intel/galileo/acpi_tables.c +++ b/src/mainboard/intel/galileo/acpi_tables.c @@ -26,7 +26,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); - header->revision = 5; + header->revision = ACPI_FADT_REV_ACPI_5_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/mainboard/intel/wtm2/fadt.c b/src/mainboard/intel/wtm2/fadt.c index a7c6536..7d9096c 100644 --- a/src/mainboard/intel/wtm2/fadt.c +++ b/src/mainboard/intel/wtm2/fadt.c @@ -24,7 +24,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); - header->revision = 5; + header->revision = ACPI_FADT_REV_ACPI_5_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/mainboard/portwell/m107/fadt.c b/src/mainboard/portwell/m107/fadt.c index 7814106..82986f6 100644 --- a/src/mainboard/portwell/m107/fadt.c +++ b/src/mainboard/portwell/m107/fadt.c @@ -25,7 +25,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); - header->revision = 3; + header->revision = ACPI_FADT_REV_ACPI_2_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/southbridge/intel/i82371eb/fadt.c b/src/southbridge/intel/i82371eb/fadt.c index 9515c01..26a1570 100644 --- a/src/southbridge/intel/i82371eb/fadt.c +++ b/src/southbridge/intel/i82371eb/fadt.c @@ -39,7 +39,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = 244; - header->revision = 1; + header->revision = ACPI_FADT_REV_ACPI_1_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/southbridge/intel/lynxpoint/acpi.c b/src/southbridge/intel/lynxpoint/acpi.c index 8d4c4e6..83c455f 100644 --- a/src/southbridge/intel/lynxpoint/acpi.c +++ b/src/southbridge/intel/lynxpoint/acpi.c @@ -74,7 +74,7 @@ /* Fill the SSDT header */ memset((void *)ssdt, 0, sizeof(acpi_header_t)); memcpy(&ssdt->signature, "SSDT", 4); - ssdt->revision = 2; + ssdt->revision = get_acpi_table_revision(SSDT); memcpy(&ssdt->oem_id, OEM_ID, 6); memcpy(&ssdt->oem_table_id, "SERIALIO", 8); ssdt->oem_revision = 43; diff --git a/src/southbridge/nvidia/mcp55/fadt.c b/src/southbridge/nvidia/mcp55/fadt.c index 6aaa702..9a70ba1 100644 --- a/src/southbridge/nvidia/mcp55/fadt.c +++ b/src/southbridge/nvidia/mcp55/fadt.c @@ -39,7 +39,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); - header->revision = 1; + header->revision = ACPI_FADT_REV_ACPI_1_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-code revision via macro ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35539/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35539/1//COMMIT_MSG@9 PS1, Line 9: hard code hard-coded
https://review.coreboot.org/c/coreboot/+/35539/1//COMMIT_MSG@11 PS1, Line 11: Replace with macro defined in arch/acpi.h Please put it on the line above.
https://review.coreboot.org/c/coreboot/+/35539/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/acpi.c:
https://review.coreboot.org/c/coreboot/+/35539/1/src/southbridge/intel/lynxp... PS1, Line 77: ssdt->revision = get_acpi_table_revision(SSDT); This change is not mentioned in the commit message.
Hello Patrick Rudolph, Frans Hendriks, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35539
to look at the new patch set (#2).
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
acpi_table_header: Replace hard-coded revision via macro and function
Minimize use of hard-coded value for acpi_table_header->revision to soft code. Replace with macro defined in arch/acpi.h for FADT and with the get_acpi_table_revision function for SSDT.
Change-Id: I99e59afc1a87203499d2da6dedaedfa643ca7eac Signed-off-by: Sourabh Kashyap Sourabhka@hcl.com --- M src/mainboard/amd/torpedo/fadt.c M src/mainboard/intel/galileo/acpi_tables.c M src/mainboard/intel/wtm2/fadt.c M src/mainboard/portwell/m107/fadt.c M src/southbridge/intel/i82371eb/fadt.c M src/southbridge/intel/lynxpoint/acpi.c M src/southbridge/nvidia/mcp55/fadt.c 7 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/35539/2
Name of user not set #1002476 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2:
(3 comments)
Done
https://review.coreboot.org/c/coreboot/+/35539/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35539/1//COMMIT_MSG@9 PS1, Line 9: hard code
hard-coded
Ack
https://review.coreboot.org/c/coreboot/+/35539/1//COMMIT_MSG@11 PS1, Line 11: Replace with macro defined in arch/acpi.h
Please put it on the line above.
Done
https://review.coreboot.org/c/coreboot/+/35539/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/acpi.c:
https://review.coreboot.org/c/coreboot/+/35539/1/src/southbridge/intel/lynxp... PS1, Line 77: ssdt->revision = get_acpi_table_revision(SSDT);
This change is not mentioned in the commit message.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2: Code-Review+1
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2:
why don't you use "get_acpi_table_revision();" function?
Name of user not set #1002476 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2:
Patch Set 2: why don't you use "get_acpi_table_revision();" function? arch/acpi.h contains multiple macros for FADT Major Revisions as per the ACPI specification. I just changed according to the previous hard-coded values in the files via the specific macros in reference. arch/x86/acpi.c defines the function get_acpi_table_revision which returns ACPI_FADT_REV_ACPI_3_0 for FADT. Should I modify the FADT return case value as per latest ACPI spec(6). What do you think?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35539/2/src/mainboard/portwell/m107... File src/mainboard/portwell/m107/fadt.c:
https://review.coreboot.org/c/coreboot/+/35539/2/src/mainboard/portwell/m107... PS2, Line 28: header->revision = ACPI_FADT_REV_ACPI_2_0; hard coded number was 3, but this is 2. What's the rationale?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35539/2/src/mainboard/portwell/m107... File src/mainboard/portwell/m107/fadt.c:
https://review.coreboot.org/c/coreboot/+/35539/2/src/mainboard/portwell/m107... PS2, Line 28: header->revision = ACPI_FADT_REV_ACPI_2_0;
hard coded number was 3, but this is 2. […]
The encoding is weird: src/arch/x86/include/arch/acpi.h:#define ACPI_FADT_REV_ACPI_2_0 3
Name of user not set #1002476 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35539/2/src/mainboard/portwell/m107... File src/mainboard/portwell/m107/fadt.c:
https://review.coreboot.org/c/coreboot/+/35539/2/src/mainboard/portwell/m107... PS2, Line 28: header->revision = ACPI_FADT_REV_ACPI_2_0;
hard coded number was 3, but this is 2. […]
According to the ACPI spec(2), the fadt revision is 3. In, ACPI spec(3), the fadt revision is 4. So, in these macro ACPI_FADT_REV_ACPI_x_0, `x` implies the ACPI spec version and the corresponding values defines the fadt revision accordingly.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35539 )
Change subject: acpi_table_header: Replace hard-coded revision via macro and function ......................................................................
acpi_table_header: Replace hard-coded revision via macro and function
Minimize use of hard-coded value for acpi_table_header->revision to soft code. Replace with macro defined in arch/acpi.h for FADT and with the get_acpi_table_revision function for SSDT.
Change-Id: I99e59afc1a87203499d2da6dedaedfa643ca7eac Signed-off-by: Sourabh Kashyap Sourabhka@hcl.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35539 Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/amd/torpedo/fadt.c M src/mainboard/portwell/m107/fadt.c M src/southbridge/intel/i82371eb/fadt.c M src/southbridge/intel/lynxpoint/acpi.c M src/southbridge/nvidia/mcp55/fadt.c 5 files changed, 5 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Aaron Durbin: Looks good to me, approved
diff --git a/src/mainboard/amd/torpedo/fadt.c b/src/mainboard/amd/torpedo/fadt.c index 9e33c07..25fb448 100644 --- a/src/mainboard/amd/torpedo/fadt.c +++ b/src/mainboard/amd/torpedo/fadt.c @@ -50,7 +50,7 @@ memset((void *)fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = 244; - header->revision = 1; + header->revision = ACPI_FADT_REV_ACPI_1_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/mainboard/portwell/m107/fadt.c b/src/mainboard/portwell/m107/fadt.c index 7814106..82986f6 100644 --- a/src/mainboard/portwell/m107/fadt.c +++ b/src/mainboard/portwell/m107/fadt.c @@ -25,7 +25,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); - header->revision = 3; + header->revision = ACPI_FADT_REV_ACPI_2_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/southbridge/intel/i82371eb/fadt.c b/src/southbridge/intel/i82371eb/fadt.c index 9515c01..26a1570 100644 --- a/src/southbridge/intel/i82371eb/fadt.c +++ b/src/southbridge/intel/i82371eb/fadt.c @@ -39,7 +39,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = 244; - header->revision = 1; + header->revision = ACPI_FADT_REV_ACPI_1_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4); diff --git a/src/southbridge/intel/lynxpoint/acpi.c b/src/southbridge/intel/lynxpoint/acpi.c index 8d4c4e6..83c455f 100644 --- a/src/southbridge/intel/lynxpoint/acpi.c +++ b/src/southbridge/intel/lynxpoint/acpi.c @@ -74,7 +74,7 @@ /* Fill the SSDT header */ memset((void *)ssdt, 0, sizeof(acpi_header_t)); memcpy(&ssdt->signature, "SSDT", 4); - ssdt->revision = 2; + ssdt->revision = get_acpi_table_revision(SSDT); memcpy(&ssdt->oem_id, OEM_ID, 6); memcpy(&ssdt->oem_table_id, "SERIALIO", 8); ssdt->oem_revision = 43; diff --git a/src/southbridge/nvidia/mcp55/fadt.c b/src/southbridge/nvidia/mcp55/fadt.c index 6aaa702..9a70ba1 100644 --- a/src/southbridge/nvidia/mcp55/fadt.c +++ b/src/southbridge/nvidia/mcp55/fadt.c @@ -39,7 +39,7 @@ memset((void *) fadt, 0, sizeof(acpi_fadt_t)); memcpy(header->signature, "FACP", 4); header->length = sizeof(acpi_fadt_t); - header->revision = 1; + header->revision = ACPI_FADT_REV_ACPI_1_0; memcpy(header->oem_id, OEM_ID, 6); memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8); memcpy(header->asl_compiler_id, ASLC, 4);
-
Aaron Durbin (Code Review) -
HAOUAS Elyes (Code Review)
-
Name of user not set (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)