Change in ...coreboot[master]: soc/intel/cannonlake: Add ASL function for setting pad mode

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31444

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

soc/intel/cannonlake: Add ASL function for setting pad mode

Add a funtion in gpio ASL library to set pad mode.

BUG=b:123350329

Change-Id: I6c683f27ddffc3132001706d1694c71bb5664577
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/cannonlake/acpi/gpio_op.asl
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/31444/1

diff --git a/src/soc/intel/cannonlake/acpi/gpio_op.asl b/src/soc/intel/cannonlake/acpi/gpio_op.asl
index 0618601..b051685 100644
--- a/src/soc/intel/cannonlake/acpi/gpio_op.asl
+++ b/src/soc/intel/cannonlake/acpi/gpio_op.asl
@@ -72,3 +72,24 @@
    }
    And (Not (GPIOTXSTATE_MASK), VAL0, VAL0)
 }
+
+/*
+ * Set Pad mode
+ * Arg0 - GPIO Number
+ * Arg1 - Pad mode
+ *     0 = GPIO control pad
+ *     1 = Native Funtion 1
+ *     2 = Native Funtion 2
+ *     3 = Native Funtion 3
+ */
+Method (GPMO, 2, Serialized)
+{
+	OperationRegion (PREG, SystemMemory, GADD (Arg0), 4)
+	Field (PREG, AnyAcc, NoLock, Preserve)
+	{
+		TMP1, 10,
+		PADM, 2,
+		TMP2, 20,
+	}
+	Store(Arg1, PADM)
+}

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31444 )

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/#/c/31444/1/src/soc/intel/cannonlake/acpi/gpio_o... 
File src/soc/intel/cannonlake/acpi/gpio_op.asl:

https://review.coreboot.org/#/c/31444/1/src/soc/intel/cannonlake/acpi/gpio_o... 
PS1, Line 81:  *     1 = Native Funtion 1
'Funtion' may be misspelled - perhaps 'Function'?

https://review.coreboot.org/#/c/31444/1/src/soc/intel/cannonlake/acpi/gpio_o... 
PS1, Line 82:  *     2 = Native Funtion 2
'Funtion' may be misspelled - perhaps 'Function'?

https://review.coreboot.org/#/c/31444/1/src/soc/intel/cannonlake/acpi/gpio_o... 
PS1, Line 83:  *     3 = Native Funtion 3
'Funtion' may be misspelled - perhaps 'Function'?

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

soc/intel/cannonlake: Add ASL function for setting pad mode

Add a funtion in gpio ASL library to set pad mode.

BUG=b:123350329

Change-Id: I6c683f27ddffc3132001706d1694c71bb5664577
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/cannonlake/acpi/gpio_op.asl
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/31444/2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31444/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31444/2//COMMIT_MSG@9 
PS2, Line 9: funtion
function

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
File src/soc/intel/cannonlake/acpi/gpio_op.asl:

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 91: PADM, 2,
IIRC, in the past we have had problems accessing individual bit fields in 32-bit regs. Instead we have used RMW operations to ensure there are no side-effects. Probably, we should be doing the same here?

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31444 )

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
File src/soc/intel/cannonlake/acpi/gpio_op.asl:

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 91: PADM, 2,
> IIRC, in the past we have had problems accessing individual bit fields in 32-bit regs. […]

Can you point me to the bug, I am not aware of this issue. Didn't see any issues with the pad config here.

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

soc/intel/cannonlake: Add ASL function for setting pad mode

Add a function in gpio ASL library to set pad mode.

BUG=b:123350329

Change-Id: I6c683f27ddffc3132001706d1694c71bb5664577
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/cannonlake/acpi/gpio_op.asl
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/31444/3

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31444 )

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31444/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31444/2//COMMIT_MSG@9 
PS2, Line 9: funtion
> function

Done

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
File src/soc/intel/cannonlake/acpi/gpio_op.asl:

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 91: PADM, 2,
> Can you point me to the bug, I am not aware of this issue. […]

We had run into this with PME status/control registers: https://review.coreboot.org/c/20364/

and https://review.coreboot.org/c/coreboot/+/23848

I am not sure if those apply to the GPIO regs, but just want to make sure we don't run into unexpected issues later.

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31444 )

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
File src/soc/intel/cannonlake/acpi/gpio_op.asl:

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 91: PADM, 2,
> We had run into this with PME status/control registers: https://review.coreboot.org/c/20364/ […]

Well, as per the specification those values should be preserved, if there is an issue that means the ACPI core in linux is not implemented as per spec. In any case will uprev with required change.

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

soc/intel/cannonlake: Add ASL function for setting pad mode

Add a function in gpio ASL library to set pad mode.

BUG=b:123350329

Change-Id: I6c683f27ddffc3132001706d1694c71bb5664577
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/cannonlake/acpi/gpio_op.asl
M src/soc/intel/cannonlake/include/soc/gpio_defs.h
M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h
3 files changed, 26 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/31444/4

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31444 )

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
File src/soc/intel/cannonlake/acpi/gpio_op.asl:

https://review.coreboot.org/#/c/31444/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 91: PADM, 2,
> Well, as per the specification those values should be preserved, if there is an issue that means the […]

I dumped the register values and confirmed that the other bits are being preserved with this change.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31444 )

Change subject: soc/intel/cannonlake: Add ASL function for setting pad mode
......................................................................

soc/intel/cannonlake: Add ASL function for setting pad mode

Add a function in gpio ASL library to set pad mode.

BUG=b:123350329

Change-Id: I6c683f27ddffc3132001706d1694c71bb5664577
Signed-off-by: Rizwan Qureshi 
Reviewed-on: https://review.coreboot.org/c/31444
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/cannonlake/acpi/gpio_op.asl
M src/soc/intel/cannonlake/include/soc/gpio_defs.h
M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h
3 files changed, 26 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/acpi/gpio_op.asl b/src/soc/intel/cannonlake/acpi/gpio_op.asl
index 0618601..0c89e99 100644
--- a/src/soc/intel/cannonlake/acpi/gpio_op.asl
+++ b/src/soc/intel/cannonlake/acpi/gpio_op.asl
@@ -72,3 +72,25 @@
    }
    And (Not (GPIOTXSTATE_MASK), VAL0, VAL0)
 }
+
+/*
+ * Set Pad mode
+ * Arg0 - GPIO Number
+ * Arg1 - Pad mode
+ *     0 = GPIO control pad
+ *     1 = Native Function 1
+ *     2 = Native Function 2
+ *     3 = Native Function 3
+ */
+Method (GPMO, 2, Serialized)
+{
+	OperationRegion (PREG, SystemMemory, GADD (Arg0), 4)
+	Field (PREG, AnyAcc, NoLock, Preserve)
+	{
+		VAL0, 32
+	}
+	Store (VAL0, Local0)
+	And (Not (GPIOPADMODE_MASK), Local0, Local0)
+	And (ShiftLeft (Arg1, GPIOPADMODE_SHIFT, Arg1), GPIOPADMODE_MASK, Arg1)
+	Or (Local0, Arg1, VAL0)
+}
diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs.h b/src/soc/intel/cannonlake/include/soc/gpio_defs.h
index c282000..e8b4f61 100644
--- a/src/soc/intel/cannonlake/include/soc/gpio_defs.h
+++ b/src/soc/intel/cannonlake/include/soc/gpio_defs.h
@@ -253,4 +253,6 @@
 #define GPIORXSTATE_MASK			0x1
 #define GPIORXSTATE_SHIFT			1
 #define GPIOTXSTATE_MASK			0x1
+#define GPIOPADMODE_MASK			0xC00
+#define GPIOPADMODE_SHIFT			10
 #endif
diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h b/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h
index d8d002c..c74c2d7 100644
--- a/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h
+++ b/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h
@@ -327,4 +327,6 @@
 #define GPIORXSTATE_MASK			0x1
 #define GPIORXSTATE_SHIFT			1
 #define GPIOTXSTATE_MASK			0x1
+#define GPIOPADMODE_MASK			0xC00
+#define GPIOPADMODE_SHIFT			10
 #endif