Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines
Looks like i82801ix and i82801jx have those. Adding them does no harm.
Change-Id: Iad9699709f3e90670f2d4f9b5664a048971ea412 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/acpi/globalnvs.asl 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/43160/1
diff --git a/src/southbridge/intel/i82801gx/acpi/globalnvs.asl b/src/southbridge/intel/i82801gx/acpi/globalnvs.asl index 848005d..0a0f255 100644 --- a/src/southbridge/intel/i82801gx/acpi/globalnvs.asl +++ b/src/southbridge/intel/i82801gx/acpi/globalnvs.asl @@ -15,6 +15,7 @@ Field (GNVS, ByteAcc, NoLock, Preserve) { /* Miscellaneous */ + Offset (0x00), OSYS, 16, // 0x00 - Operating System SMIF, 8, // 0x02 - SMI function PRM0, 8, // 0x03 - SMI function parameter @@ -32,6 +33,7 @@ LINX, 8, // 0x12 - Linux OS DCKN, 8, // 0x13 - PCIe docking state /* Thermal policy */ + Offset (0x14), ACTT, 8, // 0x14 - active trip point TPSV, 8, // 0x15 - passive trip point TC1V, 8, // 0x16 - passive trip point TC1 @@ -70,6 +72,7 @@ UTIL, 8, // 0x3a - ACIN, 8, // 0x3b - /* Integrated Graphics Device */ + Offset (0x3c), IGDS, 8, // 0x3c - IGD state (primary = 1) TLST, 8, // 0x3d - Display Toggle List pointer CADL, 8, // 0x3e - Currently Attached Devices List
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43160/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43160/1//COMMIT_MSG@10 PS1, Line 10: This basically reverts commit 80505a6fef (sb/intel/i82801gx: Remove unnecessary/redundant ACPI offset operator) [1][2].
I believe the IASL warnings/notes are now disabled in our environment.
[1]: If53072c6a91dd794c70d1fab8697b1713d400fe8 [2]: https://review.coreboot.org/c/31672
Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43160
to look at the new patch set (#2).
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines
Looks like i82801ix and i82801jx have those. Adding them does no harm.
Change-Id: Iad9699709f3e90670f2d4f9b5664a048971ea412 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/acpi/globalnvs.asl 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/43160/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43160/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43160/1//COMMIT_MSG@7 PS1, Line 7: h hell, you are drunk
https://review.coreboot.org/c/coreboot/+/43160/1//COMMIT_MSG@10 PS1, Line 10:
This basically reverts commit 80505a6fef (sb/intel/i82801gx: Remove unnecessary/redundant ACPI offse […]
Ah, just learned of that change. In any case, having them makes the ASL code easier to follow. Otherwise, one needs to count the bits from the last Offset line. Plus, it makes the globalnvs.asl files identical, which is nice.
Want me to do anything about it?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
extraneous/redundant uses of the Offset() are not needed :
OperationRegion (OPR1, SystemMemory, 0x100, 0x100) Field (OPR1) { Offset (0), // Never needed FLD1, 32, Offset (4), // Redundant, offset is already 4 (bytes) FLD2, 8, Offset (64), // OK use of Offset. FLD3, 16, }
will give :
dsdt.asl 14: Offset (0), Remark 2158 - ^ Unnecessary/redundant use of Offset operator
dsdt.asl 16: Offset (4), Remark 2158 - ^ Unnecessary/redundant use of Offset operator
https://review.coreboot.org/c/coreboot/+/43160/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/2/src/southbridge/intel/i8280... PS2, Line 18: Offset (0x00), please remove this
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
(1 comment)
extraneous/redundant uses of the Offset() are not needed :
OperationRegion (OPR1, SystemMemory, 0x100, 0x100) Field (OPR1) { Offset (0), // Never needed FLD1, 32, Offset (4), // Redundant, offset is already 4 (bytes) FLD2, 8, Offset (64), // OK use of Offset. FLD3, 16,
}
will give :
dsdt.asl 14: Offset (0), Remark 2158 - ^ Unnecessary/redundant use of Offset operator
dsdt.asl 16: Offset (4), Remark 2158 - ^ Unnecessary/redundant use of Offset operator
I don't see such warnings anywhere: https://paste.flashrom.org/view.php?id=3337
https://review.coreboot.org/c/coreboot/+/43160/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/2/src/southbridge/intel/i8280... PS2, Line 18: Offset (0x00),
please remove this
Elyes, these are added for alignment purposes (i82801ix/i82801jx have them). This change makes globalnvs.asl for i82801{gx,ix,jx} *identical*
I can understand that Offset (0x00) is redundant, but I'd rather remove all instances of it on a separate commit. Does it sound good?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 3:
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
(1 comment)
extraneous/redundant uses of the Offset() are not needed :
OperationRegion (OPR1, SystemMemory, 0x100, 0x100) Field (OPR1) { Offset (0), // Never needed FLD1, 32, Offset (4), // Redundant, offset is already 4 (bytes) FLD2, 8, Offset (64), // OK use of Offset. FLD3, 16,
}
will give :
dsdt.asl 14: Offset (0), Remark 2158 - ^ Unnecessary/redundant use of Offset operator
dsdt.asl 16: Offset (4), Remark 2158 - ^ Unnecessary/redundant use of Offset operator
I don't see such warnings anywhere: https://paste.flashrom.org/view.php?id=3337
nano +267 Makefile.inc ;)
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43160/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/2/src/southbridge/intel/i8280... PS2, Line 18: Offset (0x00),
Elyes, these are added for alignment purposes (i82801ix/i82801jx have them). […]
I think you can remove redundant and make alignment to a clean file.
Please don't add them ... ( I spent time to remove many of them )
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... PS4, Line 18: Offset (0x00) This is the only one I'd complain about, because it's not very helpful. What about removing from i82801ix and i82801jx instead?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... PS4, Line 18: Offset (0x00)
This is the only one I'd complain about, because it's not very helpful. […]
Sounds good. I think we can drop all `Offset (0x00)` instances, but keep the rest for clarity.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... PS4, Line 18: Offset (0x00)
Sounds good. I think we can drop all `Offset (0x00)` instances, but keep the rest for clarity.
👍
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/43160/4/src/southbridge/intel/i8280... PS4, Line 18: Offset (0x00)
👍
please see https://review.coreboot.org/c/coreboot/+/43283 Thx
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43160 )
Change subject: sb/intel/i82801gx/acpi/globalnvs.h: Add some Offset lines ......................................................................
Abandoned