Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: DSDT upgrade ......................................................................
nb/intel/i440bx: DSDT upgrade
- Separate northbridge DSDT memory device into its own ACPI device, in its own file, to be placed in the _SB scope. The existing file goes to _SB.PCI0. - Add PMCR register. It'll come in handy for S3 support. - Add a memory device in ACPI to match ASUS P3B-F vendor DSDT. Memory ranges between TOM and 4GB was declared available for MMIO, now it is between TOM and (4GB - CONFIG_ROM_SIZE).
Change-Id: I2c74ef30a9bb48e02154f963b1ca3a4f5f3004df Signed-off-by: Keith Hui buurin@gmail.com --- A src/northbridge/intel/i440bx/acpi/i440bx.asl M src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl 2 files changed, 66 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41049/1
diff --git a/src/northbridge/intel/i440bx/acpi/i440bx.asl b/src/northbridge/intel/i440bx/acpi/i440bx.asl new file mode 100644 index 0000000..ae82625 --- /dev/null +++ b/src/northbridge/intel/i440bx/acpi/i440bx.asl @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +/* i440bx Northbridge resources that sits on _SB */ + +Device (MEM1) +{ + Name (_HID, EisaId ("PNP0C01") /* System Board */) // _HID: Hardware ID + Method (_CRS, 0) // _CRS: Current Resource Settings + { + Name (BUF1, ResourceTemplate () + { + Memory32Fixed (ReadWrite, + 0x00000000, // Address Base + 0x000A0000, // Address Length + ) + Memory32Fixed (ReadOnly, + 0x000F0000, // Address Base + 0x00010000, // Address Length + ) + /* + * Main memory. Length of this block will be adjusted to TOM1, + * TOM1-4GB is declared in sb_pci0_crs.asl for MMIO. + */ + Memory32Fixed (ReadWrite, + 0x00100000, // Address Base + 0x00000000, // Address Length + _Y00) + /* Reserved for firmware flash */ + Memory32Fixed (ReadOnly, + 0xFFFC0000, // Address Base + CONFIG_ROM_SIZE, // Address Length + _Y01) + }) + CreateDWordField (BUF1, _Y00._LEN, EMLN) // _LEN: Length + CreateDWordField (BUF1, _Y01._BAS, FLSB) // _BAS: Base + + /* + * Use ShiftLeft to avoid 64bit constant (for XP). + * This will work even if the OS does 32bit arithmetic, as + * 32bit (0x00000000 - TOM1) will wrap and give the same + * result as 64bit (0x100000000 - TOM1). + */ + + /* Top of 4GB */ + ShiftLeft(0x10000000, 4, Local0) + FLSB = Local0 - CONFIG_ROM_SIZE; + EMLN = _SB.PCI0.NB.TOM1 - 0x100000; + Return (BUF1) /* _SB_.MEM1._CRS.BUF1 */ + } +} diff --git a/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl b/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl index 856b3e8..476be30 100644 --- a/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl +++ b/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl @@ -1,22 +1,22 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */
-/* i440bx Northbridge */ +/* i440bx Northbridge resources that sits on _SB.PCI0 */ Device (NB) { Name(_ADR, 0x00000000) OperationRegion(PCIC, PCI_Config, 0x00, 0x100) -} - -Field (NB.PCIC, AnyAcc, NoLock, Preserve) -{ - Offset (0x67), // DRB7 - DRB7, 8, -} - -Method(TOM1, 0) { - /* Multiply by 8MB to get TOM */ - Return(ShiftLeft(DRB7, 23)) + Field (PCIC, ByteAcc, NoLock, Preserve) + { + Offset (0x67), // DRB7 + DRB7, 8, + Offset (0x7A), // PMCR + PMCR, 8 + } + Method(TOM1, 0) { + /* Multiply by 8MB to get TOM */ + Return(ShiftLeft(DRB7, 23)) + } }
Method(_CRS, 0) { @@ -61,10 +61,10 @@ * 32bit (0x00000000 - TOM1) will wrap and give the same * result as 64bit (0x100000000 - TOM1). */ - Store(TOM1, MM1B) + MM1B = _SB.PCI0.NB.TOM1 ShiftLeft(0x10000000, 4, Local0) - Subtract(Local0, TOM1, Local0) - Store(Local0, MM1L) + Local0 -= CONFIG_ROM_SIZE + MM1L = Local0 - MM1B
Return(TMP) }
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: DSDT upgrade ......................................................................
Patch Set 1:
(3 comments)
Does the new file need to be included somewhere?
https://review.coreboot.org/c/coreboot/+/41049/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41049/1//COMMIT_MSG@7 PS1, Line 7: nb/intel/i440bx: DSDT upgrade
nb/intel/i440bx: Upgrade DSDT
nb/intel/i440bx: Refactor DSDT
https://review.coreboot.org/c/coreboot/+/41049/1//COMMIT_MSG@15 PS1, Line 15: now it is between TOM and (4GB - CONFIG_ROM_SIZE). Three separate commits would be nice.
https://review.coreboot.org/c/coreboot/+/41049/1/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/i440bx.asl:
https://review.coreboot.org/c/coreboot/+/41049/1/src/northbridge/intel/i440b... PS1, Line 32: CONFIG_ROM_SIZE, // Address Length Is it a Gerrit bug, showing the misalignment of the second comment?
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41049
to look at the new patch set (#2).
Change subject: nb/intel/i440bx: Split main memory into its own ACPI device ......................................................................
nb/intel/i440bx: Split main memory into its own ACPI device
Main memory gets its own ACPI device, in its own file, to be placed in the _SB scope.
The existing file goes to _SB.PCI0.
Change-Id: I2c74ef30a9bb48e02154f963b1ca3a4f5f3004df Signed-off-by: Keith Hui buurin@gmail.com --- A src/northbridge/intel/i440bx/acpi/i440bx.asl M src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl 2 files changed, 47 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41049/2
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 3:
This change is ready for review.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 15: ShiftLeft Please use the new syntax here as well
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 15: ShiftLeft
Please use the new syntax here as well
DRB7 << 23
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 62: ShiftLeft(0x10000000, 4, Local0) Local0 = 0x10000000 << 4
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41049
to look at the new patch set (#4).
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
nb/intel/i440bx: Refactor ACPI code
Bring DRB7 OpRegion and top-of-memory indicator inside NB device.
Use more concise ASL syntax for TOM calculations.
Change-Id: I2c74ef30a9bb48e02154f963b1ca3a4f5f3004df Signed-off-by: Keith Hui buurin@gmail.com --- M src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl 1 file changed, 12 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41049/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41049/1/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/i440bx.asl:
https://review.coreboot.org/c/coreboot/+/41049/1/src/northbridge/intel/i440b... PS1, Line 32: CONFIG_ROM_SIZE, // Address Length
It's indented with spaces, maybe someone mechanically replaced CONFIG_ROM_SIZE aeons ago and then le […]
Ack (file is gone it seems)
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 15: ShiftLeft
DRB7 << 23
Done
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 62: ShiftLeft(0x10000000, 4, Local0)
Local0 = 0x10000000 << 4
*poke*
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 62: ShiftLeft(0x10000000, 4, Local0)
*poke*
I actually have concerns after checking the disassembly.
With both ASL 1 and ASL 2 syntax, iasl optimizes this line to: Local0 = 0x0000000100000000
A thoroughly 64-bit value. From previous notes we used bit shifts here so the math would work with both 32 and 64 bits.
I want to validate the actual results returned to Linux ACPI system before fixing this one.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 62: ShiftLeft(0x10000000, 4, Local0)
I actually have concerns after checking the disassembly. […]
I think this is intentional. After all, this chipset can handle at most 4 GiB of memory, right?
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 62: ShiftLeft(0x10000000, 4, Local0)
I think this is intentional. […]
I know that bit shift math is intentional, and yes because it's 32-bit.
Now I've confirmed the resource data returned is correct. I'll correct this in the next patch set.
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41049
to look at the new patch set (#6).
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
nb/intel/i440bx: Refactor ACPI code
Bring DRB7 OpRegion and top-of-memory indicator inside NB device.
Use more concise ASL 2.0 syntax for TOM calculations.
Change-Id: I2c74ef30a9bb48e02154f963b1ca3a4f5f3004df Signed-off-by: Keith Hui buurin@gmail.com --- M src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl 1 file changed, 13 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41049/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 6: Code-Review+1
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41049/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41049/1//COMMIT_MSG@7 PS1, Line 7: nb/intel/i440bx: DSDT upgrade
nb/intel/i440bx: Upgrade DSDT […]
Done
https://review.coreboot.org/c/coreboot/+/41049/1//COMMIT_MSG@15 PS1, Line 15: now it is between TOM and (4GB - CONFIG_ROM_SIZE).
Three separate commits would be nice.
Done
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... File src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl:
https://review.coreboot.org/c/coreboot/+/41049/3/src/northbridge/intel/i440b... PS3, Line 62: ShiftLeft(0x10000000, 4, Local0)
I know that bit shift math is intentional, and yes because it's 32-bit. […]
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41049 )
Change subject: nb/intel/i440bx: Refactor ACPI code ......................................................................
nb/intel/i440bx: Refactor ACPI code
Bring DRB7 OpRegion and top-of-memory indicator inside NB device.
Use more concise ASL 2.0 syntax for TOM calculations.
Change-Id: I2c74ef30a9bb48e02154f963b1ca3a4f5f3004df Signed-off-by: Keith Hui buurin@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41049 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Patrick Georgi pgeorgi@google.com --- M src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl 1 file changed, 13 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl b/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl index fe66f39..ce71aed 100644 --- a/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl +++ b/src/northbridge/intel/i440bx/acpi/sb_pci0_crs.asl @@ -1,21 +1,19 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-/* i440bx Northbridge */ +/* i440bx Northbridge resources that sits on _SB.PCI0 */ Device (NB) { Name(_ADR, 0x00000000) OperationRegion(PCIC, PCI_Config, 0x00, 0x100) -} - -Field (NB.PCIC, AnyAcc, NoLock, Preserve) -{ - Offset (0x67), // DRB7 - DRB7, 8, -} - -Method(TOM1, 0) { - /* Multiply by 8MB to get TOM */ - Return(ShiftLeft(DRB7, 23)) + Field (PCIC, ByteAcc, NoLock, Preserve) + { + Offset (0x67), // DRB7 + DRB7, 8, + } + Method(TOM1, 0) { + /* Multiply by 8MB to get TOM */ + Return(DRB7 << 23) + } }
Method(_CRS, 0) { @@ -60,10 +58,9 @@ * 32bit (0x00000000 - TOM1) will wrap and give the same * result as 64bit (0x100000000 - TOM1). */ - Store(TOM1, MM1B) - ShiftLeft(0x10000000, 4, Local0) - Subtract(Local0, TOM1, Local0) - Store(Local0, MM1L) + MM1B = _SB.PCI0.NB.TOM1 + Local0 = 0x10000000 << 4 + MM1L = Local0 - MM1B
Return(TMP) }
-
Angel Pons (Code Review) -
Keith Hui (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)