Change in coreboot[master]: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46967 )

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods

Creating named objects within a method is highly inefficient. So, pass a
reference to the OperationRegion field that needs to be updated instead.

Change-Id: I88272fc5cbe35427ccb5fc50789d47b66ace88fe
Signed-off-by: Angel Pons 
---
M src/soc/intel/broadwell/pch/acpi/serialio.asl
1 file changed, 54 insertions(+), 30 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/46967/1

diff --git a/src/soc/intel/broadwell/pch/acpi/serialio.asl b/src/soc/intel/broadwell/pch/acpi/serialio.asl
index b03bcdd..35b15b4 100644
--- a/src/soc/intel/broadwell/pch/acpi/serialio.asl
+++ b/src/soc/intel/broadwell/pch/acpi/serialio.asl
@@ -6,34 +6,22 @@
 #define SIO_BAR_LEN 0x1000

// Put SerialIO device in D0 state
-// Arg0 - BAR1 of device
+// Arg0 - Ref to offset 0x84 of device's PCI config space
 Method (LPD0, 1, Serialized)
 {
-	OperationRegion (SPRT, SystemMemory, Arg0 + 0x84, 4)
-	Field (SPRT, DWordAcc, NoLock, Preserve)
-	{
-		SPCS, 32
-	}
-
-	SPCS &= 0xFFFFFFFC
-	Local0 = SPCS // Read back after writing
+	DeRefOf (Arg0) &= 0xFFFFFFFC
+	Local0 = DeRefOf (Arg0) // Read back after writing

// Use Local0 to avoid iasl warning: Method Local is set but never used
    Local0 &= Ones
 }

// Put SerialIO device in D3 state
-// Arg0 - BAR1 of device
+// Arg0 - Ref to offset 0x84 of device's PCI config space
 Method (LPD3, 1, Serialized)
 {
-	OperationRegion (SPRT, SystemMemory, Arg0 + 0x84, 4)
-	Field (SPRT, DWordAcc, NoLock, Preserve)
-	{
-		SPCS, 32
-	}
-
-	SPCS |= 0x3
-	Local0 = SPCS // Read back after writing
+	DeRefOf (Arg0) |= 0x3
+	Local0 = DeRefOf (Arg0) // Read back after writing

// Use Local0 to avoid iasl warning: Method Local is set but never used
    Local0 &= Ones
@@ -226,14 +214,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S1B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S1B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S1B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -297,14 +291,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S2B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S2B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S2B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -353,14 +353,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S3B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S3B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S3B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -421,14 +427,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S4B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S4B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S4B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -489,14 +501,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S5B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S5B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S5B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -545,14 +563,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S6B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S6B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S6B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46967 )

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46967/1/src/soc/intel/broadwell/pch... 
File src/soc/intel/broadwell/pch/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/46967/1/src/soc/intel/broadwell/pch... 
PS1, Line 9: 0x84
Is there a better name for this register?

[Angel Pons (Code Review)]

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

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46967/1/src/soc/intel/broadwell/pch... 
File src/soc/intel/broadwell/pch/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/46967/1/src/soc/intel/broadwell/pch... 
PS1, Line 9: 0x84
> Is there a better name for this register?

Reference code ACPI and BIOS spec do not give it any name. Reference code has a PME_CTRL_STS macro, and EDS calls it PME_CTRL_STA. I doubt using either name would be an improvement.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46967 )

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

Patch Set 1: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46967 )

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46967/1/src/soc/intel/broadwell/pch... 
File src/soc/intel/broadwell/pch/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/46967/1/src/soc/intel/broadwell/pch... 
PS1, Line 9: 0x84
> Reference code ACPI and BIOS spec do not give it any name. […]

Ack

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Tim Wawrzynczak, Patrick Rudolph,

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

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

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

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods

Creating named objects within a method is highly inefficient. So, pass a
reference to the OperationRegion field that needs to be updated instead.

Change-Id: I88272fc5cbe35427ccb5fc50789d47b66ace88fe
Signed-off-by: Angel Pons 
---
M src/soc/intel/broadwell/pch/acpi/serialio.asl
1 file changed, 54 insertions(+), 30 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/46967/6

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46967 )

Change subject: soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods
......................................................................

soc/intel/broadwell: Improve LPD0/LPD3 SerialIO ACPI methods

Creating named objects within a method is highly inefficient. So, pass a
reference to the OperationRegion field that needs to be updated instead.

Change-Id: I88272fc5cbe35427ccb5fc50789d47b66ace88fe
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46967
Tested-by: build bot (Jenkins) 
Reviewed-by: Tim Wawrzynczak 
---
M src/soc/intel/broadwell/pch/acpi/serialio.asl
1 file changed, 54 insertions(+), 30 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/soc/intel/broadwell/pch/acpi/serialio.asl b/src/soc/intel/broadwell/pch/acpi/serialio.asl
index b03bcdd..35b15b4 100644
--- a/src/soc/intel/broadwell/pch/acpi/serialio.asl
+++ b/src/soc/intel/broadwell/pch/acpi/serialio.asl
@@ -6,34 +6,22 @@
 #define SIO_BAR_LEN 0x1000

// Put SerialIO device in D0 state
-// Arg0 - BAR1 of device
+// Arg0 - Ref to offset 0x84 of device's PCI config space
 Method (LPD0, 1, Serialized)
 {
-	OperationRegion (SPRT, SystemMemory, Arg0 + 0x84, 4)
-	Field (SPRT, DWordAcc, NoLock, Preserve)
-	{
-		SPCS, 32
-	}
-
-	SPCS &= 0xFFFFFFFC
-	Local0 = SPCS // Read back after writing
+	DeRefOf (Arg0) &= 0xFFFFFFFC
+	Local0 = DeRefOf (Arg0) // Read back after writing

// Use Local0 to avoid iasl warning: Method Local is set but never used
    Local0 &= Ones
 }

// Put SerialIO device in D3 state
-// Arg0 - BAR1 of device
+// Arg0 - Ref to offset 0x84 of device's PCI config space
 Method (LPD3, 1, Serialized)
 {
-	OperationRegion (SPRT, SystemMemory, Arg0 + 0x84, 4)
-	Field (SPRT, DWordAcc, NoLock, Preserve)
-	{
-		SPCS, 32
-	}
-
-	SPCS |= 0x3
-	Local0 = SPCS // Read back after writing
+	DeRefOf (Arg0) |= 0x3
+	Local0 = DeRefOf (Arg0) // Read back after writing

// Use Local0 to avoid iasl warning: Method Local is set but never used
    Local0 &= Ones
@@ -226,14 +214,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S1B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S1B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S1B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -297,14 +291,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S2B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S2B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S2B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -353,14 +353,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S3B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S3B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S3B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -421,14 +427,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S4B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S4B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S4B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -489,14 +501,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S5B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S5B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S5B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }

@@ -545,14 +563,20 @@
    	}
    }

+	OperationRegion (SPRT, SystemMemory, \S6B1 + 0x84, 4)
+	Field (SPRT, DWordAcc, NoLock, Preserve)
+	{
+		SPCS, 32
+	}
+
    Method (_PS0, 0, Serialized)
    {
-		^^LPD0 (\S6B1)
+		^^LPD0 (RefOf (SPCS))
    }

Method (_PS3, 0, Serialized)
    {
-		^^LPD3 (\S6B1)
+		^^LPD3 (RefOf (SPCS))
    }
 }