Change in coreboot[master]: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagen...

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41470 )

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl

Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and
1 lock bit. If lock bit is set, then systemagent.asl would end up
reporting the base address of low MMIO incorrectly i.e. off by 1.

This change masks the lower 20 bits of TOLUD and TOM registers when
exposing it in the ACPI tables to ensure that the base address of low
MMIO region is reported correctly.

Change-Id: I2ff7a30fabb7f77d13acadec1e6e4cb3a45b6139
Signed-off-by: Furquan Shaikh 
---
M src/soc/intel/skylake/acpi/systemagent.asl
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/41470/1

diff --git a/src/soc/intel/skylake/acpi/systemagent.asl b/src/soc/intel/skylake/acpi/systemagent.asl
index 6ea782b..bca4f63 100644
--- a/src/soc/intel/skylake/acpi/systemagent.asl
+++ b/src/soc/intel/skylake/acpi/systemagent.asl
@@ -187,14 +187,22 @@
    /*
     * Fix up PCI memory region
     * Start with Top of Lower Usable DRAM
+	 * Lower 20 bits of TOLUD register need to be masked since they contain lock and
+	 * reserved bits.
     */
    Store (_SB.PCI0.MCHC.TLUD, Local0)
+	And (Local0, ShiftLeft(0xfff, 20), Local0)
    Store (_SB.PCI0.MCHC.MEBA, Local1)

/*  Check if ME base is equal */
    If (LEqual (Local0, Local1)) {
    	/*  Use Top Of Memory instead */
    	Store (_SB.PCI0.MCHC.TOM, Local0)
+		/*
+		 * Lower 20 bits of TOM register need to be masked since they contain lock and
+		 * reserved bits.
+		 */
+		And (Local0, ShiftLeft(0x7ffff, 20), Local0)
    }

Store (Local0, PMIN)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41470 )

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41470/1/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/systemagent.asl:

https://review.coreboot.org/c/coreboot/+/41470/1/src/soc/intel/skylake/acpi/... 
PS1, Line 194: 	And (Local0, ShiftLeft(0xfff, 20), Local0)
Why use the old syntax here?

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Subrata Banik, Aaron Durbin, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41470

to look at the new patch set (#2).

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl

Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and
1 lock bit. If lock bit is set, then systemagent.asl would end up
reporting the base address of low MMIO incorrectly i.e. off by 1.

This change masks the lower 20 bits of TOLUD and TOM registers when
exposing it in the ACPI tables to ensure that the base address of low
MMIO region is reported correctly.

Change-Id: I2ff7a30fabb7f77d13acadec1e6e4cb3a45b6139
Signed-off-by: Furquan Shaikh 
---
M src/soc/intel/skylake/acpi/systemagent.asl
1 file changed, 9 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/41470/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41470 )

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41470/1/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/systemagent.asl:

https://review.coreboot.org/c/coreboot/+/41470/1/src/soc/intel/skylake/acpi/... 
PS1, Line 194: 	And (Local0, ShiftLeft(0xfff, 20), Local0)
> Why use the old syntax here?

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41470 )

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Angel Pons, Subrata Banik, Aaron Durbin, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41470

to look at the new patch set (#5).

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl

Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and
1 lock bit. If lock bit is set, then systemagent.asl would end up
reporting the base address of low MMIO incorrectly i.e. off by 1.

This change masks the lower 20 bits of TOLUD and TOM registers when
exposing it in the ACPI tables to ensure that the base address of low
MMIO region is reported correctly.

Change-Id: I2ff7a30fabb7f77d13acadec1e6e4cb3a45b6139
Signed-off-by: Furquan Shaikh 
---
M src/soc/intel/skylake/acpi/systemagent.asl
1 file changed, 9 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/41470/5

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41470 )

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl

Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and
1 lock bit. If lock bit is set, then systemagent.asl would end up
reporting the base address of low MMIO incorrectly i.e. off by 1.

This change masks the lower 20 bits of TOLUD and TOM registers when
exposing it in the ACPI tables to ensure that the base address of low
MMIO region is reported correctly.

Change-Id: I2ff7a30fabb7f77d13acadec1e6e4cb3a45b6139
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41470
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/skylake/acpi/systemagent.asl
1 file changed, 9 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/skylake/acpi/systemagent.asl b/src/soc/intel/skylake/acpi/systemagent.asl
index b2e691d..962d9ef 100644
--- a/src/soc/intel/skylake/acpi/systemagent.asl
+++ b/src/soc/intel/skylake/acpi/systemagent.asl
@@ -187,14 +187,20 @@
    /*
     * Fix up PCI memory region
     * Start with Top of Lower Usable DRAM
+	 * Lower 20 bits of TOLUD register need to be masked since they contain lock and
+	 * reserved bits.
     */
-	Local0 = _SB.PCI0.MCHC.TLUD
+	Local0 = _SB.PCI0.MCHC.TLUD & (0xfff << 20)
    Local1 = _SB.PCI0.MCHC.MEBA

/*  Check if ME base is equal */
    If (Local0 == Local1) {
-		/*  Use Top Of Memory instead */
-		Local0 = _SB.PCI0.MCHC.TOM
+		/*
+		 * Use Top Of Memory instead
+		 * Lower 20 bits of TOM register need to be masked since they contain lock and
+		 * reserved bits.
+		 */
+		Local0 = _SB.PCI0.MCHC.TOM & (0x7ffff << 20)
    }

Store (Local0, PMIN)

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41470 )

Change subject: soc/intel/skylake: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
......................................................................

Patch Set 6:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3594
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3593
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3592
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3591

Please note: This test is under development and might not be accurate at all!