Change in ...coreboot[master]: soc/amd: Update ACPI code

[Richard Spiegel (Code Review)]

Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33620


Change subject: soc/amd: Update ACPI code
......................................................................

soc/amd: Update ACPI code

In preparation to adding merlinfalcon SOC, update ACPI code: Update OSVL
(old code does not test for anything newer than WINXP). Update cpu.asl to
accept SOC with up to 4 cores. Update lpc.asl to avoid a bug with WIN7
and WIN10.

Notice: Fixes are needed (later patch), as Windows still fails (not fully
ACPI compatible). Currently only Linux was tested and worked.

BUG=b:none.
TEST=Tested later with padmelon board.

Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Signed-off-by: Richard Spiegel <[e-mail]>
---
M src/soc/amd/common/acpi/lpc.asl
M src/soc/amd/stoneyridge/acpi/cpu.asl
M src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl
3 files changed, 153 insertions(+), 31 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/33620/1

diff --git a/src/soc/amd/common/acpi/lpc.asl b/src/soc/amd/common/acpi/lpc.asl
index 93b4056..0844a9a 100644
--- a/src/soc/amd/common/acpi/lpc.asl
+++ b/src/soc/amd/common/acpi/lpc.asl
@@ -32,25 +32,29 @@
 		Offset(0xA0),
 		BAR,32}		// SPI Controller Base Address Register (Index 0xA0)
 
-	Device(LDRC)	// LPC device: Resource consumption
-	{
-		Name (_HID, EISAID("PNP0C02"))  // ID for Motherboard resources
-		Name (CRS, ResourceTemplate ()  // Current Motherboard resources
+	/* PNP0C02 apparently fails with WIN77 and WIN10. Until better
+	 * understood, block LDRC for these two OS */
+	if(LLess(OSVR, 0x06)) {
+		Device(LDRC)	// LPC device: Resource consumption
 		{
-			Memory32Fixed(ReadWrite,	// Setup for fixed resource location for SPI base address
-			0x00000000,			// Address Base
-			0x00000000,			// Address Length
-			BAR0				// Descriptor Name
-			)
-		})
+			Name (_HID, EISAID("PNP0C02"))  // ID for Motherboard resources
+			Name (CRS, ResourceTemplate ()  // Current Motherboard resources
+			{
+				Memory32Fixed(ReadWrite,	// Setup for fixed resource location for SPI base address
+				0x00000000,			// Address Base
+				0x00000000,			// Address Length
+				BAR0				// Descriptor Name
+				)
+			})
 
-		Method(_CRS,0,Serialized)
-		{
-			CreateDwordField(^CRS,^BAR0._BAS,SPIB)	// Field to hold SPI base address
-			CreateDwordField(^CRS,^BAR0._LEN,SPIL)	// Field to hold SPI address length
-			Store(BAR,SPIB)		// SPI base address mapped
-			Store(0x1000,SPIL)	// 4k space mapped
-			Return(CRS)
+			Method(_CRS,0,Serialized)
+			{
+				CreateDwordField(^CRS,^BAR0._BAS,SPIB)	// Field to hold SPI base address
+				CreateDwordField(^CRS,^BAR0._LEN,SPIL)	// Field to hold SPI address length
+				Store(BAR,SPIB)		// SPI base address mapped
+				Store(0x1000,SPIL)	// 4k space mapped
+				Return(CRS)
+			}
 		}
 	}
 
diff --git a/src/soc/amd/stoneyridge/acpi/cpu.asl b/src/soc/amd/stoneyridge/acpi/cpu.asl
index 414326e..aa59040 100644
--- a/src/soc/amd/stoneyridge/acpi/cpu.asl
+++ b/src/soc/amd/stoneyridge/acpi/cpu.asl
@@ -34,7 +34,27 @@
 /* Return a package containing enabled processor entries */
 Method (PPKG)
 {
-	If (LGreaterEqual (\PCNT, 2)) {
+	If (LGreaterEqual (\PCNT, 8)) {
+		Return (Package ()
+		{
+			\_PR.P000,
+			\_PR.P001,
+			\_PR.P002,
+			\_PR.P003,
+			\_PR.P004,
+			\_PR.P005,
+			\_PR.P006,
+			\_PR.P007
+		})
+	} ElseIf (LGreaterEqual (\PCNT, 4)) {
+		Return (Package ()
+		{
+			\_PR.P000,
+			\_PR.P001,
+			\_PR.P002,
+			\_PR.P003
+		})
+	} ElseIf (LGreaterEqual (\PCNT, 2)) {
 		Return (Package ()
 		{
 			\_PR.P000,
diff --git a/src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl b/src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl
index 3623814..90000e6 100644
--- a/src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl
+++ b/src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl
@@ -175,27 +175,125 @@
 
 } /* End Method(_SB._INI) */
 
-Method(OSFL, 0){
-
-	if (LNotEqual(OSVR, Ones)) {Return(OSVR)}	/* OS version was already detected */
-
-	if (CondRefOf(\_OSI))
+Method(OSFL, 0, NotSerialized)
+{
+	If(LNotEqual(OSVR, Ones))
 	{
-		Store(1, OSVR)					/* Assume some form of XP */
-		if (\_OSI("Windows 2006"))		/* Vista */
+		Return(OSVR)
+	}
+	Store(0x03, OSVR)
+	If(CondRefOf(\_OSI, Local0))
+	{
+		If(_OSI("Windows 2001"))
 		{
-			Store(2, OSVR)
+			Store(0x04, OSVR)
 		}
-	} else {
-		If(WCMP(\_OS,"Linux")) {
-			Store(3, OSVR)				/* Linux */
-		} Else {
-			Store(4, OSVR)				/* Gotta be WinCE */
+		If(_OSI("Windows 2001.1"))
+		{
+			Store(0x05, OSVR)
+		}
+		If(_OSI("FreeBSD"))
+		{
+			Store(0x06, OSVR)
+		}
+		If(_OSI("HP-UX"))
+		{
+			Store(0x07, OSVR)
+		}
+		If(_OSI("OpenVMS"))
+		{
+			Store(0x08, OSVR)
+		}
+		If(_OSI("Windows 2001 SP1"))
+		{
+			Store(0x09, OSVR)
+		}
+		If(_OSI("Windows 2001 SP2"))
+		{
+			Store(0x0A, OSVR)
+		}
+		If(_OSI("Windows 2001 SP3"))
+		{
+			Store(0x0B, OSVR)
+		}
+		If(_OSI("Windows 2006"))
+		{
+			Store(0x0C, OSVR)
+		}
+		If(_OSI("Windows 2006 SP1"))
+		{
+			Store(0x0D, OSVR)
+		}
+		If(_OSI("Windows 2009"))
+		{
+			Store(0x0E, OSVR)
+		}
+		If(_OSI("Windows 2012"))
+		{
+			Store(0x0F, OSVR)
+		}
+		If(_OSI("Windows 2013"))
+		{
+			Store(0x10, OSVR)
+		}
+	}
+	Else
+	{
+		Store (\_OS, local0)
+		If(MCTH(local0, "Microsoft Windows NT"))
+		{
+			Store(Zero, OSVR)
+		}
+		If(MCTH(local0, "Microsoft Windows"))
+		{
+			Store(One, OSVR)
+		}
+		If(MCTH(local0, "Microsoft WindowsME: Millennium Edition"))
+		{
+			Store(0x02, OSVR)
+		}
+		If(MCTH(local0, "Linux"))
+		{
+			Store(0x03, OSVR)
+		}
+		If(MCTH(local0, "FreeBSD"))
+		{
+			Store(0x06, OSVR)
+		}
+		If(MCTH(local0, "HP-UX"))
+		{
+			Store(0x07, OSVR)
+		}
+		If(MCTH(local0, "OpenVMS"))
+		{
+			Store(0x08, OSVR)
 		}
 	}
 	Return(OSVR)
 }
 
+Method(MCTH, 2, Serialized)
+{
+	If(LLess(SizeOf(Arg0), SizeOf(Arg1)))
+	{
+		Return(Zero)
+	}
+	Add(SizeOf(Arg0), One, Local0)
+	Name(BUF0, Buffer(Local0){})
+	Name(BUF1, Buffer(Local0){})
+	Store(Arg0, BUF0)
+	Store(Arg1, BUF1)
+	While(Local0)
+	{
+		Decrement(Local0)
+		If(LNotEqual(DerefOf(Index(BUF0, Local0)), DerefOf(Index(BUF1, Local0))))
+		{
+			Return(Zero)
+		}
+	}
+	Return(One)
+}
+
 OperationRegion(SMIC, SystemMemory, 0xfed80000, 0x80000)
 Field( SMIC, ByteAcc, NoLock, Preserve) {
 	/* MISC registers */

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-MessageType: newchange

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl@a41 
PS1, Line 41: 
            : 
Using value other than 0, since the base and length will be updated anyway, but I remember that stay 0 will cause windows report error.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Lance Zhao <[e-mail]>
Gerrit-Comment-Date: Thu, 20 Jun 2019 08:33:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl@35 
PS1, Line 35: WIN77
WIN7



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Lance Zhao <[e-mail]>
Gerrit-Comment-Date: Thu, 20 Jun 2019 09:44:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl@a41 
PS1, Line 41: 
            :
> Using value other than 0, since the base and length will be updated anyway, but I remember that stay […]
> Thanks, I'll check if this solves this particular problem.

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl@35 
PS1, Line 35: WIN77
> WIN7
> My keyboard sometimes double type the 7. Thanks for catching.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 1:

(2 comments)

I’d prefer a separate commit for each fix.

https://review.coreboot.org/#/c/33620/1/src/soc/amd/stoneyridge/acpi/sb_pci0... 
File src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/stoneyridge/acpi/sb_pci0... 
PS1, Line 178: Method(OSFL, 0, NotSerialized)
Separate commit please.


https://review.coreboot.org/#/c/33620/1/src/soc/amd/stoneyridge/acpi/sb_pci0... 
PS1, Line 178: Method(OSFL, 0, NotSerialized)
             : {
             : 	If(LNotEqual(OSVR, Ones))
             : 	{
             : 		Return(OSVR)
             : 	}
             : 	Store(0x03, OSVR)
             : 	If(CondRefOf(\_OSI, Local0))
             : 	{
             : 		If(_OSI("Windows 2001"))
             : 		{
             : 			Store(0x04, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001.1"))
             : 		{
             : 			Store(0x05, OSVR)
             : 		}
             : 		If(_OSI("FreeBSD"))
             : 		{
             : 			Store(0x06, OSVR)
             : 		}
             : 		If(_OSI("HP-UX"))
             : 		{
             : 			Store(0x07, OSVR)
             : 		}
             : 		If(_OSI("OpenVMS"))
             : 		{
             : 			Store(0x08, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001 SP1"))
             : 		{
             : 			Store(0x09, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001 SP2"))
             : 		{
             : 			Store(0x0A, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001 SP3"))
             : 		{
             : 			Store(0x0B, OSVR)
             : 		}
             : 		If(_OSI("Windows 2006"))
             : 		{
             : 			Store(0x0C, OSVR)
             : 		}
             : 		If(_OSI("Windows 2006 SP1"))
             : 		{
             : 			Store(0x0D, OSVR)
             : 		}
             : 		If(_OSI("Windows 2009"))
             : 		{
             : 			Store(0x0E, OSVR)
             : 		}
             : 		If(_OSI("Windows 2012"))
             : 		{
             : 			Store(0x0F, OSVR)
             : 		}
             : 		If(_OSI("Windows 2013"))
             : 		{
             : 			Store(0x10, OSVR)
             : 		}
             : 	}
             : 	Else
             : 	{
             : 		Store (\_OS, local0)
             : 		If(MCTH(local0, "Microsoft Windows NT"))
             : 		{
             : 			Store(Zero, OSVR)
             : 		}
             : 		If(MCTH(local0, "Microsoft Windows"))
             : 		{
             : 			Store(One, OSVR)
             : 		}
             : 		If(MCTH(local0, "Microsoft WindowsME: Millennium Edition"))
             : 		{
             : 			Store(0x02, OSVR)
             : 		}
             : 		If(MCTH(local0, "Linux"))
             : 		{
             : 			Store(0x03, OSVR)
             : 		}
             : 		If(MCTH(local0, "FreeBSD"))
             : 		{
             : 			Store(0x06, OSVR)
             : 		}
             : 		If(MCTH(local0, "HP-UX"))
             : 		{
             : 			Store(0x07, OSVR)
             : 		}
             : 		If(MCTH(local0, "OpenVMS"))
             : 		{
             : 			Store(0x08, OSVR)
             : 		}
             : 	}
             : 	Return(OSVR)
             : }
Can this be factored out into a common location?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Lance Zhao <[e-mail]>
Gerrit-Comment-Date: Thu, 20 Jun 2019 15:19:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33620/1/src/soc/amd/stoneyridge/acpi/sb_pci0... 
File src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/stoneyridge/acpi/sb_pci0... 
PS1, Line 178: Method(OSFL, 0, NotSerialized)
             : {
             : 	If(LNotEqual(OSVR, Ones))
             : 	{
             : 		Return(OSVR)
             : 	}
             : 	Store(0x03, OSVR)
             : 	If(CondRefOf(\_OSI, Local0))
             : 	{
             : 		If(_OSI("Windows 2001"))
             : 		{
             : 			Store(0x04, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001.1"))
             : 		{
             : 			Store(0x05, OSVR)
             : 		}
             : 		If(_OSI("FreeBSD"))
             : 		{
             : 			Store(0x06, OSVR)
             : 		}
             : 		If(_OSI("HP-UX"))
             : 		{
             : 			Store(0x07, OSVR)
             : 		}
             : 		If(_OSI("OpenVMS"))
             : 		{
             : 			Store(0x08, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001 SP1"))
             : 		{
             : 			Store(0x09, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001 SP2"))
             : 		{
             : 			Store(0x0A, OSVR)
             : 		}
             : 		If(_OSI("Windows 2001 SP3"))
             : 		{
             : 			Store(0x0B, OSVR)
             : 		}
             : 		If(_OSI("Windows 2006"))
             : 		{
             : 			Store(0x0C, OSVR)
             : 		}
             : 		If(_OSI("Windows 2006 SP1"))
             : 		{
             : 			Store(0x0D, OSVR)
             : 		}
             : 		If(_OSI("Windows 2009"))
             : 		{
             : 			Store(0x0E, OSVR)
             : 		}
             : 		If(_OSI("Windows 2012"))
             : 		{
             : 			Store(0x0F, OSVR)
             : 		}
             : 		If(_OSI("Windows 2013"))
             : 		{
             : 			Store(0x10, OSVR)
             : 		}
             : 	}
             : 	Else
             : 	{
             : 		Store (\_OS, local0)
             : 		If(MCTH(local0, "Microsoft Windows NT"))
             : 		{
             : 			Store(Zero, OSVR)
             : 		}
             : 		If(MCTH(local0, "Microsoft Windows"))
             : 		{
             : 			Store(One, OSVR)
             : 		}
             : 		If(MCTH(local0, "Microsoft WindowsME: Millennium Edition"))
             : 		{
             : 			Store(0x02, OSVR)
             : 		}
             : 		If(MCTH(local0, "Linux"))
             : 		{
             : 			Store(0x03, OSVR)
             : 		}
             : 		If(MCTH(local0, "FreeBSD"))
             : 		{
             : 			Store(0x06, OSVR)
             : 		}
             : 		If(MCTH(local0, "HP-UX"))
             : 		{
             : 			Store(0x07, OSVR)
             : 		}
             : 		If(MCTH(local0, "OpenVMS"))
             : 		{
             : 			Store(0x08, OSVR)
             : 		}
             : 	}
             : 	Return(OSVR)
             : }
> Can this be factored out into a common location?
> You mean, something that can be used by all boards from now on as an include? Sure.

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl@a41 
PS1, Line 41: 
            :
> Thanks, I'll check if this solves this particular problem.
> I have tried with final values, it did not help. Thanks anyway.

[Richard Spiegel (Code Review)]

Hello Charles Marslett, Marshall Dawson, 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/+/33620

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

Change subject: soc/amd: Update ACPI code
......................................................................

soc/amd: Update ACPI code

In preparation to adding merlinfalcon SOC, update ACPI code: Update OSVL
(old code does not test for anything newer than WINXP). Update cpu.asl to
accept SOC with up to 4 cores. Update lpc.asl to avoid a bug with WIN7
and WIN10.

Notice: Fixes are needed (later patch), as Windows still fails (not fully
ACPI compatible). Currently only Linux was tested and worked.

BUG=b:none.
TEST=Tested later with padmelon board.

Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Signed-off-by: Richard Spiegel <[e-mail]>
---
M src/soc/amd/common/acpi/lpc.asl
M src/soc/amd/stoneyridge/acpi/cpu.asl
M src/soc/amd/stoneyridge/acpi/sb_pci0_fch.asl
3 files changed, 43 insertions(+), 38 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/33620/2
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Lance Zhao <[e-mail]>
Gerrit-MessageType: newpatchset

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/1/src/soc/amd/common/acpi/lpc.asl@a41 
PS1, Line 41: 
            :
> I have tried with final values, it did not help. Thanks anyway.
> Sorry that was not helpful.

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 2:

(1 comment)

I think you should separate your OS stuff into a separate stack.  Keep the additional cores change for the merlinfalcon work.

https://review.coreboot.org/#/c/33620/2/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/2/src/soc/amd/common/acpi/lpc.asl@35 
PS2, Line 35: PNP0C02 apparently fails with WIN7 and WIN10. Until better
            : 	 * understood,
I'm not sure why "apparently" here.  Either it fails or it doesn't.  Also, I wouldn't call it PNP0C02 in the comment.  That's simply describing LDRC as a generic motherboard resource.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-Comment-Date: Mon, 24 Jun 2019 14:08:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 2:

(1 comment)
> Patch Set 2:
>
> (1 comment)
>
> I think you should separate your OS stuff into a separate stack.  Keep the additional cores change for the merlinfalcon work.

This is for merlinfalcon, as the same ASL is used both for stoneyridge and merlinfalcon.

https://review.coreboot.org/#/c/33620/2/src/soc/amd/common/acpi/lpc.asl 
File src/soc/amd/common/acpi/lpc.asl:

https://review.coreboot.org/#/c/33620/2/src/soc/amd/common/acpi/lpc.asl@35 
PS2, Line 35: PNP0C02 apparently fails with WIN7 and WIN10. Until better
            : 	 * understood,
> I'm not sure why "apparently" here.  Either it fails or it doesn't. […]
> I'll do some test later to confirm or reject my suspicion. Then I'll change this comment based on what I find.

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 2:
> > I think you should separate your OS stuff into a separate stack.  Keep the additional cores change for the merlinfalcon work.
> > This is for merlinfalcon, as the same ASL is used both for stoneyridge and merlinfalcon.

I understand that.  My suggestion is that maybe you shouldn't be trying to fix OS incompatibilities on Stoney Ridge until you have more information.  Simply make a stack that adds Merlin Falcon support to the stoneyridge directory.  This will be a lot easier to review and approve than a stack containing unrelated ACPI changes.  Or you could move the ACPI changes to the top of the stack, maybe, because this shouldn't affect making stoneyridge backward compatible.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-Comment-Date: Mon, 24 Jun 2019 14:46:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Patch Set 2:
> Patch Set 2:
> > > I think you should separate your OS stuff into a separate stack.  Keep the additional cores change for the merlinfalcon work.
> > > This is for merlinfalcon, as the same ASL is used both for stoneyridge and merlinfalcon.
>
> I understand that.  My suggestion is that maybe you shouldn't be trying to fix OS incompatibilities on Stoney Ridge until you have more information.  Simply make a stack that adds Merlin Falcon support to the stoneyridge directory.  This will be a lot easier to review and approve than a stack containing unrelated ACPI changes.  Or you could move the ACPI changes to the top of the stack, maybe, because this shouldn't affect making stoneyridge backward compatible.

Agree on that. I had a reason when I started, but OSFL was already separated and maybe abandoned, so it really makes no sense to separate it anymore. Will abandon this patch and add cpu.asl to the actual merlinfalson soc patch.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-Comment-Date: Mon, 24 Jun 2019 16:52:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Richard Spiegel (Code Review)]

Richard Spiegel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33620 )

Change subject: soc/amd: Update ACPI code
......................................................................


Abandoned

Merged back to whole stoneyridge patch.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/33620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id3615a4460dd4e0c95e890a38210263dd8ea6887
Gerrit-Change-Number: 33620
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: Charles Marslett <[e-mail]>
Gerrit-Reviewer: Lance Zhao <[e-mail]>
Gerrit-Reviewer: Marshall Dawson <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Richard Spiegel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-MessageType: abandon