Change in coreboot[master]: soc/intel/cannonlake: Add GCFG method to GPIO ASL.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34179 )

Change subject: soc/intel/cannonlake: Add GCFG method to GPIO ASL.
......................................................................

soc/intel/cannonlake: Add GCFG method to GPIO ASL.

The new method returns the address of a GPIO community's
MISCCFG register.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 37 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/34179/1

diff --git a/src/soc/intel/cannonlake/acpi/gpio.asl b/src/soc/intel/cannonlake/acpi/gpio.asl
index 9df1cf7..71a11b1 100644
--- a/src/soc/intel/cannonlake/acpi/gpio.asl
+++ b/src/soc/intel/cannonlake/acpi/gpio.asl
@@ -107,3 +107,40 @@
    Add (Local2, PAD_CFG_BASE, Local2)
    Return (Add (Local2, Multiply (Local1, 16)))
 }
+
+/*
+ * Get GPIO Community MISCCFG Address
+ * Arg0 - GPIO Community Number
+ */
+Method (GCFG, 1, NotSerialized)
+{
+	/* GPIO Community 0 */
+	If (LEqual (Arg0, 0))
+	{
+		Store (PID_GPIOCOM0, Local0)
+	}
+	/* GPIO Community 1 */
+	If (LEqual (Arg0, 1))
+	{
+		Store (PID_GPIOCOM1, Local0)
+	}
+	/* GPIO Community 2 */
+	If (LEqual (Arg0, 2))
+	{
+		Store (PID_GPIOCOM2, Local0)
+	}
+	/* GPIO Community 3 */
+	If (LEqual (Arg0, 3))
+	{
+		Store (PID_GPIOCOM3, Local0)
+	}
+	/* GPIO Community 4*/
+	If (LEqual (Arg0, 4))
+	{
+		Store (PID_GPIOCOM4, Local0)
+	}
+
+	Store (PCRB (Local0), Local1)
+	Add (Local1, 16, Local1)
+	Return (Local1)
+}

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, Shelley Chen, Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add GCFG method to GPIO ASL.
......................................................................

soc/intel/cannonlake: Add GCFG method to GPIO ASL.

The new method returns the address of a GPIO community's
MISCCFG register.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 37 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/34179/2

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add GCFG method to GPIO ASL.
......................................................................

soc/intel/cannonlake: Add GCFG method to GPIO ASL.

The new method returns the address of a GPIO community's
MISCCFG register.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 37 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/34179/3

[Paul Fagerburg (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GCFG method to GPIO ASL.
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.
......................................................................

soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.

The GCFG method returns the address of a GPIO community's
MISCCFG register.  CGPM sets the PM bits of a GPIO community's
MISCCFG register.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 62 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/34179/5

Change in coreboot[master]: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.

[Paul Fagerburg (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.
......................................................................

Patch Set 5: Code-Review+1

Change in coreboot[master]: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.
......................................................................

Patch Set 5:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 122: ByteAcc
DWordAcc?

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 129: LAnd
I guess you want to use And and not LAnd? LAnd is Logical And whereas And is Bitwise AND. Same for Land and Lor in the statements below.

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 127: Store (REG, Local0)
             : 	/* Ensure only PM bits can be set */
             : 	Store (LAnd (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG), Local1)
             : 	/* Clear PM bits */
             : 	Store (LAnd (LNot (MISCCFG_ENABLE_GPIO_PM_CONFIG), Local0), Local0)
             : 	Store (LOr (Local1, Local0), Local1)
             : 	Store (Local1, REG)
             : }
Just curious. Any reason not to use PCRA and PCRO methods provided by pcr.asl? Then you won't need to define OpRegion, Field here or perform and/or operations. Less code:

Local0 <-- PID_GPIOCOMx

PCRA (Local0, GPIO_MISCCFG, Not (MISCCFG_ENABLE_GPIO_PM_CONFIG))
PCRO (Local0, GPIO_MISCCFG, And (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG))

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 169: 16
GPIO_MISCCFG

Change in coreboot[master]: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GCFG & CGPM method to GPIO ASL.
......................................................................

Patch Set 5:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 122: ByteAcc
> DWordAcc?

Done

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 129: LAnd
> I guess you want to use And and not LAnd? LAnd is Logical And whereas And is Bitwise AND. […]

Done

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 127: Store (REG, Local0)
             : 	/* Ensure only PM bits can be set */
             : 	Store (LAnd (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG), Local1)
             : 	/* Clear PM bits */
             : 	Store (LAnd (LNot (MISCCFG_ENABLE_GPIO_PM_CONFIG), Local0), Local0)
             : 	Store (LOr (Local1, Local0), Local1)
             : 	Store (Local1, REG)
             : }
> Just curious. Any reason not to use PCRA and PCRO methods provided by pcr. […]

Oh I missed those, that's great!

https://review.coreboot.org/c/coreboot/+/34179/5/src/soc/intel/cannonlake/ac... 
PS5, Line 169: 16
> GPIO_MISCCFG

Oh yeah, I was using that until I got the #include<> and then forgot.

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

The GPID method returns the PCR Port ID of the given GPIO community.
The CGPM method alters the given GPIO community's PM bits, given in
Arg1.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 45 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/34179/6

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Lance Zhao (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34179/6/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/6/src/soc/intel/cannonlake/ac... 
PS6, Line 134: Store (PID_GPIOCOM0, Local0)
If some one messed up with GPID number that will accidentally program community 0? What about dothing or return with something safe?

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 6: -Code-Review

(1 comment)

https://review.coreboot.org/c/coreboot/+/34179/6/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/6/src/soc/intel/cannonlake/ac... 
PS6, Line 134: Store (PID_GPIOCOM0, Local0)
> If some one messed up with GPID number that will accidentally program community 0? What about dothin […]

Humm.. I was thinking about the same when reviewing this. But what would be a safe value? Maybe 0? And then caller can just do nothing if 0 is returned?

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Lance Zhao (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 6:
> Patch Set 6: -Code-Review
>
> (1 comment)

Caller will do nothing sounds good to me.

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34179 )

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34179/6/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/6/src/soc/intel/cannonlake/ac... 
PS6, Line 134: Store (PID_GPIOCOM0, Local0)
> Humm.. I was thinking about the same when reviewing this. […]

i would recommend for a check above itself and return
If (LAnd (LGreaterEqual (Arg0, 1), LLessEqual (Arg0, 4))

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

The GPID method returns the PCR Port ID of the given GPIO community.
The CGPM method alters the given GPIO community's PM bits, given in
Arg1.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 50 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/34179/7

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34179 )

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34179/8/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/8/src/soc/intel/cannonlake/ac... 
PS8, Line 137: Return (0)
not sure if you have a check to skip PCR programming if PID is 0 ?

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 8:

I updated the GPID method to return 0 for an invalid GPIO community, and the one consumer of the function now checks for GPID returning 0, in which case it does nothing.

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34179/8/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/8/src/soc/intel/cannonlake/ac... 
PS8, Line 137: Return (0)
> not sure if you have a check to skip PCR programming if PID is 0 ?

Check line 153 below

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34179 )

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34179/8/src/soc/intel/cannonlake/ac... 
File src/soc/intel/cannonlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/34179/8/src/soc/intel/cannonlake/ac... 
PS8, Line 137: Return (0)
> Check line 153 below

got it, looks good now

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34179 )

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 8: Code-Review+1

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34179 )

Change subject: soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL
......................................................................

soc/intel/cannonlake: Add GPID and CGPM methods to GPIO ASL

The GPID method returns the PCR Port ID of the given GPIO community.
The CGPM method alters the given GPIO community's PM bits, given in
Arg1.

Change-Id: I098ee08573eb4f8a45d9b5ae84f2d85ce525c9b8
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34179
Tested-by: build bot (Jenkins) 
Reviewed-by: Subrata Banik 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/cannonlake/acpi/gpio.asl
1 file changed, 50 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Subrata Banik: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/acpi/gpio.asl b/src/soc/intel/cannonlake/acpi/gpio.asl
index 9df1cf7..65332ad 100644
--- a/src/soc/intel/cannonlake/acpi/gpio.asl
+++ b/src/soc/intel/cannonlake/acpi/gpio.asl
@@ -16,6 +16,7 @@
 #include <soc/gpio_defs.h>
 #include <soc/irq.h>
 #include <soc/pcr_ids.h>
+#include <intelblocks/gpio.h>
 #include "gpio_op.asl"

Device (GPIO)
@@ -107,3 +108,52 @@
    Add (Local2, PAD_CFG_BASE, Local2)
    Return (Add (Local2, Multiply (Local1, 16)))
 }
+
+/*
+ * Return PCR Port ID of GPIO Communities
+ *
+ * Arg0: GPIO Community (0-4)
+ */
+Method (GPID, 1, Serialized)
+{
+	Switch (ToInteger (Arg0))
+	{
+		Case (0) {
+			Store (PID_GPIOCOM0, Local0)
+		}
+		Case (1) {
+			Store (PID_GPIOCOM1, Local0)
+		}
+		Case (2) {
+			Store (PID_GPIOCOM2, Local0)
+		}
+		Case (3) {
+			Store (PID_GPIOCOM3, Local0)
+		}
+		Case (4) {
+			Store (PID_GPIOCOM4, Local0)
+		}
+		Default {
+			Return (0)
+		}
+	}
+
+	Return (Local0)
+}
+
+/*
+ * Configure GPIO Power Management bits
+ *
+ * Arg0: GPIO community (0-4)
+ * Arg1: PM bits in MISCCFG
+ */
+Method (CGPM, 2, Serialized)
+{
+	Store (GPID (Arg0), Local0)
+	If (LNotEqual (Local0, 0)) {
+		/* Mask off current PM bits */
+		PCRA (Local0, GPIO_MISCCFG, Not (MISCCFG_ENABLE_GPIO_PM_CONFIG))
+		/* Mask in requested bits */
+		PCRO (Local0, GPIO_MISCCFG, And (Arg1, MISCCFG_ENABLE_GPIO_PM_CONFIG))
+	}
+}