Change in ...coreboot[master]: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

[Rizwan Qureshi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

Add a function in gpio ASL library to enable/disable pad Rx/Tx Buffers.

BUG=b:123350329

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31619/1

diff --git a/src/soc/intel/cannonlake/acpi/gpio_op.asl b/src/soc/intel/cannonlake/acpi/gpio_op.asl
index 0c89e99..e1cfe04 100644
--- a/src/soc/intel/cannonlake/acpi/gpio_op.asl
+++ b/src/soc/intel/cannonlake/acpi/gpio_op.asl
@@ -94,3 +94,45 @@
    And (ShiftLeft (Arg1, GPIOPADMODE_SHIFT, Arg1), GPIOPADMODE_MASK, Arg1)
    Or (Local0, Arg1, VAL0)
 }
+
+/*
+ * Enable/Disable Tx buffer
+ * Arg0 - GPIO Number
+ * Arg1 - TxBuffer state
+ *     0 = Disable Tx Buffer
+ *     1 = Enable Tx Buffer
+ */
+Method (GTXE, 2, Serialized)
+{
+	OperationRegion (PREG, SystemMemory, GADD (Arg0), 4)
+	Field (PREG, AnyAcc, NoLock, Preserve)
+	{
+		VAL0, 32
+	}
+	Arg1 = LNot (Arg1)
+	Store (VAL0, Local0)
+	And (Not (GPIOTXBUFDIS_MASK), Local0, Local0)
+	And (ShiftLeft (Arg1, GPIOTXBUFDIS_SHIFT, Arg1), GPIOTXBUFDIS_MASK, Arg1)
+	Or (Local0, Arg1, VAL0)
+}
+
+/*
+ * Enable/Disable Rx buffer
+ * Arg0 - GPIO Number
+ * Arg1 - RxBuffer state
+ *     0 = Disable Rx Buffer
+ *     1 = Enable Rx Buffer
+ */
+Method (GRXE, 2, Serialized)
+{
+	OperationRegion (PREG, SystemMemory, GADD (Arg0), 4)
+	Field (PREG, AnyAcc, NoLock, Preserve)
+	{
+		VAL0, 32
+	}
+	Arg1 = LNot (Arg1)
+	Store (VAL0, Local0)
+	And (Not (GPIORXBUFDIS_MASK), Local0, Local0)
+	And (ShiftLeft (Arg1, GPIORXBUFDIS_SHIFT, Arg1), GPIORXBUFDIS_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 e8b4f61..3399526 100644
--- a/src/soc/intel/cannonlake/include/soc/gpio_defs.h
+++ b/src/soc/intel/cannonlake/include/soc/gpio_defs.h
@@ -255,4 +255,8 @@
 #define GPIOTXSTATE_MASK			0x1
 #define GPIOPADMODE_MASK			0xC00
 #define GPIOPADMODE_SHIFT			10
+#define GPIOTXBUFDIS_MASK			0x100
+#define GPIOTXBUFDIS_SHIFT			8
+#define GPIORXBUFDIS_MASK			0x200
+#define GPIORXBUFDIS_SHIFT			9
 #endif

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31619 )

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 2: Code-Review+1

(3 comments)

Header files need some more work to fix Jenkins' issue.

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

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 115: , Arg1
This is storing the intermediate result, right? Seems unnecessary.

The masking, too. We know it is a single bit, so what is
there to mask?

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/gpio_defs.h:

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... 
PS2, Line 253: #define GPIORXSTATE_MASK			0x1
             : #define GPIORXSTATE_SHIFT			1
NB. this seems odd

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... 
PS2, Line 261: #define GPIORXBUFDIS_SHIFT			9
The register bits should apply to both PCH-H and PCH-LP, don't they? Maybe `gpio.h` is a better place.

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

Add a function in gpio ASL library to enable/disable pad Rx/Tx Buffers.

BUG=b:123350329

Change-Id: I6c40d79debb61b0c4e96e485b410d446b77d9cf6
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, 50 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31619/3

[Rizwan Qureshi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 115: , Arg1
> This is storing the intermediate result, right? Seems unnecessary. […]

makes sense, i will just use the mask to set/unset the bit

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

Add a function in gpio ASL library to enable/disable pad Rx/Tx Buffers.

BUG=b:123350329

Change-Id: I6c40d79debb61b0c4e96e485b410d446b77d9cf6
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, 54 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31619/4

[Rizwan Qureshi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/gpio_defs.h:

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... 
PS2, Line 261: #define GPIORXBUFDIS_SHIFT			9
> The register bits should apply to both PCH-H and PCH-LP, don't they? Maybe `gpio. […]

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

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31619 )

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 4: Code-Review+1

(2 comments)

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

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 115: , Arg1
> makes sense, i will just use the mask to set/unset the bit

Good call, looks much better :)

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

https://review.coreboot.org/#/c/31619/4/src/soc/intel/cannonlake/acpi/gpio_o... 
PS4, Line 112: 	And (Arg1, 1, Arg1)
This only adds odd side-effects (every other value above 1 means enable /
disable?) and shouldn't be necessary.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/#/c/31619/4/src/soc/intel/cannonlake/acpi/gpio_o... 
PS4, Line 112: 	And (Arg1, 1, Arg1)
> This only adds odd side-effects (every other value above 1 means enable / […]

+1. We can just add explicit checks for 0 and 1 and not do anything otherwise.

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

Add a function in gpio ASL library to enable/disable pad Rx/Tx Buffers.

BUG=b:123350329

Change-Id: I6c40d79debb61b0c4e96e485b410d446b77d9cf6
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, 52 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31619/5

[Rizwan Qureshi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 5:

(2 comments)

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

https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/acpi/gpio_o... 
PS2, Line 115: , Arg1
> Good call, looks much better :)

Done

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

https://review.coreboot.org/#/c/31619/4/src/soc/intel/cannonlake/acpi/gpio_o... 
PS4, Line 112: 	And (Arg1, 1, Arg1)
> This only adds odd side-effects (every other value above 1 means enable / […]

Done

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

Add a function in gpio ASL library to enable/disable pad Rx/Tx Buffers.

BUG=b:123350329

Change-Id: I6c40d79debb61b0c4e96e485b410d446b77d9cf6
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, 52 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31619/6

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31619 )

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 6: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

Patch Set 6: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers
......................................................................

soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers

Add a function in gpio ASL library to enable/disable pad Rx/Tx Buffers.

BUG=b:123350329

Change-Id: I6c40d79debb61b0c4e96e485b410d446b77d9cf6
Signed-off-by: Rizwan Qureshi 
Reviewed-on: https://review.coreboot.org/c/31619
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
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, 52 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  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 0c89e99..b024367 100644
--- a/src/soc/intel/cannonlake/acpi/gpio_op.asl
+++ b/src/soc/intel/cannonlake/acpi/gpio_op.asl
@@ -94,3 +94,47 @@
    And (ShiftLeft (Arg1, GPIOPADMODE_SHIFT, Arg1), GPIOPADMODE_MASK, Arg1)
    Or (Local0, Arg1, VAL0)
 }
+
+/*
+ * Enable/Disable Tx buffer
+ * Arg0 - GPIO Number
+ * Arg1 - TxBuffer state
+ *     0 = Disable Tx Buffer
+ *     1 = Enable Tx Buffer
+ */
+Method (GTXE, 2, Serialized)
+{
+	OperationRegion (PREG, SystemMemory, GADD (Arg0), 4)
+	Field (PREG, AnyAcc, NoLock, Preserve)
+	{
+		VAL0, 32
+	}
+
+	If (LEqual (Arg1, 1)) {
+		And (Not (GPIOTXBUFDIS_MASK), VAL0, VAL0)
+	} ElseIf (LEqual (Arg1, 0)){
+		Or (GPIOTXBUFDIS_MASK, VAL0, VAL0)
+	}
+}
+
+/*
+ * Enable/Disable Rx buffer
+ * Arg0 - GPIO Number
+ * Arg1 - RxBuffer state
+ *     0 = Disable Rx Buffer
+ *     1 = Enable Rx Buffer
+ */
+Method (GRXE, 2, Serialized)
+{
+	OperationRegion (PREG, SystemMemory, GADD (Arg0), 4)
+	Field (PREG, AnyAcc, NoLock, Preserve)
+	{
+		VAL0, 32
+	}
+
+	If (LEqual (Arg1, 1)) {
+		And (Not (GPIORXBUFDIS_MASK), VAL0, VAL0)
+	} ElseIf (LEqual (Arg1, 0)){
+		Or (GPIORXBUFDIS_MASK, VAL0, VAL0)
+	}
+}
diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs.h b/src/soc/intel/cannonlake/include/soc/gpio_defs.h
index e8b4f61..3399526 100644
--- a/src/soc/intel/cannonlake/include/soc/gpio_defs.h
+++ b/src/soc/intel/cannonlake/include/soc/gpio_defs.h
@@ -255,4 +255,8 @@
 #define GPIOTXSTATE_MASK			0x1
 #define GPIOPADMODE_MASK			0xC00
 #define GPIOPADMODE_SHIFT			10
+#define GPIOTXBUFDIS_MASK			0x100
+#define GPIOTXBUFDIS_SHIFT			8
+#define GPIORXBUFDIS_MASK			0x200
+#define GPIORXBUFDIS_SHIFT			9
 #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 c74c2d7..c7f3c81 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
@@ -329,4 +329,8 @@
 #define GPIOTXSTATE_MASK			0x1
 #define GPIOPADMODE_MASK			0xC00
 #define GPIOPADMODE_SHIFT			10
+#define GPIOTXBUFDIS_MASK			0x100
+#define GPIOTXBUFDIS_SHIFT			8
+#define GPIORXBUFDIS_MASK			0x200
+#define GPIORXBUFDIS_SHIFT			9
 #endif