Change in coreboot[master]: acpigen: Add some new helper functions

[Duncan Laurie (Code Review)]

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41985 )

Change subject: acpigen: Add some new helper functions
......................................................................

acpigen: Add some new helper functions

These build on existing functions but use different object types
in order to provide functions for upcoming changes:

acpigen_write_return_op(): Return an operator.
acpigen_write_if_lequal_op_op(): Check of 2 operands are equal.
acpigen_get_package_op_element(): Read an element from a package
                                  into an operator.

This one just provides the missing helper, the other 3 already exist:
acpigen_get_tx_gpio: Read TX gpio state.

Signed-off-by: Duncan Laurie 
Change-Id: I1141fd132d6f09cf482f74e95308947cba2c5846
---
M src/acpi/acpigen.c
M src/include/acpi/acpigen.h
2 files changed, 60 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/41985/1

diff --git a/src/acpi/acpigen.c b/src/acpi/acpigen.c
index 793841c..d734dad 100644
--- a/src/acpi/acpigen.c
+++ b/src/acpi/acpigen.c
@@ -339,6 +339,18 @@
    acpigen_emit_namestring(name);
 }

+void acpigen_get_package_op_element(uint8_t package_op, unsigned int element, uint8_t dest_op)
+{
+	/* <dest_op> = DeRefOf (<package_op>[<element]) */
+	acpigen_write_store();
+	acpigen_emit_byte(DEREF_OP);
+	acpigen_emit_byte(INDEX_OP);
+	acpigen_emit_byte(package_op);
+	acpigen_write_integer(element);
+	acpigen_emit_byte(ZERO_OP); /* Ignore Index() Destination */
+	acpigen_emit_byte(dest_op);
+}
+
 void acpigen_write_processor(u8 cpuindex, u32 pblock_addr, u8 pblock_len)
 {
 /*
@@ -1269,6 +1281,20 @@
 }

/*
+ * Generates ACPI code for checking if operand1 and operand2 are equal.
+ * Both operand1 and operand2 are ACPI ops.
+ *
+ * If (Lequal (op, op))
+ */
+void acpigen_write_if_lequal_op_op(uint8_t op, uint8_t val)
+{
+	acpigen_write_if();
+	acpigen_emit_byte(LEQUAL_OP);
+	acpigen_emit_byte(op);
+	acpigen_emit_byte(val);
+}
+
+/*
  * Generates ACPI code for checking if operand1 and operand2 are equal, where,
  * operand1 is ACPI op and operand2 is an integer.
  *
@@ -1341,6 +1367,12 @@
    acpigen_write_return_byte_buffer(&arg, 1);
 }

+void acpigen_write_return_op(uint8_t arg)
+{
+	acpigen_emit_byte(RETURN_OP);
+	acpigen_emit_byte(arg);
+}
+
 void acpigen_write_return_byte(uint8_t arg)
 {
    acpigen_emit_byte(RETURN_OP);
@@ -1790,6 +1822,14 @@
    	acpigen_write_xor(LOCAL0_OP, 1, LOCAL0_OP);
 }

+void acpigen_get_tx_gpio(struct acpi_gpio *gpio)
+{
+	acpigen_soc_get_tx_gpio(gpio->pins[0]);
+
+	if (gpio->polarity == ACPI_GPIO_ACTIVE_LOW)
+		acpigen_write_xor(LOCAL0_OP, 1, LOCAL0_OP);
+}
+
 /* refer to ACPI 6.4.3.5.3 Word Address Space Descriptor section for details */
 void acpigen_resource_word(u16 res_type, u16 gen_flags, u16 type_flags, u16 gran,
    u16 range_min, u16 range_max, u16 translation, u16 length)
diff --git a/src/include/acpi/acpigen.h b/src/include/acpi/acpigen.h
index a99489d..1dc2c02 100644
--- a/src/include/acpi/acpigen.h
+++ b/src/include/acpi/acpigen.h
@@ -360,6 +360,7 @@
 void acpigen_write_debug_op(uint8_t op);
 void acpigen_write_if(void);
 void acpigen_write_if_and(uint8_t arg1, uint8_t arg2);
+void acpigen_write_if_lequal_op_op(uint8_t op, uint8_t val);
 void acpigen_write_if_lequal_op_int(uint8_t op, uint64_t val);
 void acpigen_write_if_lequal_namestr_int(const char *namestr, uint64_t val);
 void acpigen_write_else(void);
@@ -368,6 +369,7 @@
 void acpigen_write_byte_buffer(uint8_t *arr, size_t size);
 void acpigen_write_return_byte_buffer(uint8_t *arr, size_t size);
 void acpigen_write_return_singleton_buffer(uint8_t arg);
+void acpigen_write_return_op(uint8_t arg);
 void acpigen_write_return_byte(uint8_t arg);
 void acpigen_write_upc(enum acpi_upc_type type);
 void acpigen_write_pld(const struct acpi_pld *pld);
@@ -439,6 +441,16 @@
 int get_cst_entries(acpi_cstate_t **);

/*
+ * Get element from package into specified desination op:
+ *   <dest_op> = DeRefOf (<package_op>[<element])
+ *
+ * Example:
+ *  acpigen_get_package_op_element(ARG0_OP, 0, LOCAL0_OP)
+ *  Local0 = DeRefOf (Arg0[0])
+ */
+void acpigen_get_package_op_element(uint8_t package_op, unsigned int element, uint8_t dest_op);
+
+/*
  * Soc-implemented functions for generating ACPI AML code for GPIO handling. All
  * these functions are expected to use only Local5, Local6 and Local7
  * variables. If the functions call into another ACPI method, then there is no
@@ -478,6 +490,14 @@
  */
 void acpigen_get_rx_gpio(struct acpi_gpio *gpio);

+/*
+ *  Helper function for getting a TX GPIO value based on the GPIO polarity.
+ *  The return value is stored in Local0 variable.
+ *  This function ends up calling acpigen_soc_get_rx_gpio to make callbacks
+ *  into SoC acpigen code
+ */
+void acpigen_get_tx_gpio(struct acpi_gpio *gpio);
+
 /* refer to ACPI 6.4.3.5.3 Word Address Space Descriptor section for details */
 void acpigen_resource_word(u16 res_type, u16 gen_flags, u16 type_flags, u16 gran,
    u16 range_min, u16 range_max, u16 translation, u16 length);

[build bot (Jenkins) (Code Review)]

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

Change subject: acpigen: Add some new helper functions
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h 
File src/include/acpi/acpigen.h:

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h@... 
PS1, Line 444:  * Get element from package into specified desination op:
'desination' may be misspelled - perhaps 'destination'?

[Duncan Laurie (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: acpigen: Add some new helper functions
......................................................................

acpigen: Add some new helper functions

These build on existing functions but use different object types
in order to provide functions for upcoming changes:

acpigen_write_return_op(): Return an operator.
acpigen_write_if_lequal_op_op(): Check of 2 operands are equal.
acpigen_get_package_op_element(): Read an element from a package
                                  into an operator.

This one just provides the missing helper, the other 3 already exist:
acpigen_get_tx_gpio: Read TX gpio state.

Signed-off-by: Duncan Laurie 
Change-Id: I1141fd132d6f09cf482f74e95308947cba2c5846
---
M src/acpi/acpigen.c
M src/include/acpi/acpigen.h
2 files changed, 60 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/41985/2

[Furquan Shaikh (Code Review)]

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

Change subject: acpigen: Add some new helper functions
......................................................................

Patch Set 1: Code-Review+1

(3 comments)

https://review.coreboot.org/c/coreboot/+/41985/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41985/1//COMMIT_MSG@13 
PS1, Line 13: of
if?

https://review.coreboot.org/c/coreboot/+/41985/1/src/acpi/acpigen.c 
File src/acpi/acpigen.c:

https://review.coreboot.org/c/coreboot/+/41985/1/src/acpi/acpigen.c@1289 
PS1, Line 1289: val
nit: Should these be op1 and op2?

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h 
File src/include/acpi/acpigen.h:

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h@... 
PS1, Line 496: rx
tx?

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41985 )

Change subject: acpigen: Add some new helper functions
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/41985/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41985/1//COMMIT_MSG@13 
PS1, Line 13: of
> if?

Done

https://review.coreboot.org/c/coreboot/+/41985/1/src/acpi/acpigen.c 
File src/acpi/acpigen.c:

https://review.coreboot.org/c/coreboot/+/41985/1/src/acpi/acpigen.c@1289 
PS1, Line 1289: val
> nit: Should these be op1 and op2?

Done

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h 
File src/include/acpi/acpigen.h:

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h@... 
PS1, Line 444:  * Get element from package into specified desination op:
> 'desination' may be misspelled - perhaps 'destination'?

Done

https://review.coreboot.org/c/coreboot/+/41985/1/src/include/acpi/acpigen.h@... 
PS1, Line 496: rx
> tx?

Done

[Duncan Laurie (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: acpigen: Add some new helper functions
......................................................................

acpigen: Add some new helper functions

These build on existing functions but use different object types
in order to provide functions for upcoming changes:

acpigen_write_return_op(): Return an operator.
acpigen_write_if_lequal_op_op(): Check if 2 operands are equal.
acpigen_get_package_op_element(): Read an element from a package
                                  into an operator.

This one just provides the missing helper, the other 3 already exist:
acpigen_get_tx_gpio: Read TX gpio state.

Signed-off-by: Duncan Laurie 
Change-Id: I1141fd132d6f09cf482f74e95308947cba2c5846
---
M src/acpi/acpigen.c
M src/include/acpi/acpigen.h
2 files changed, 60 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/41985/3

[Furquan Shaikh (Code Review)]

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

Change subject: acpigen: Add some new helper functions
......................................................................

Patch Set 3: Code-Review+2

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41985 )

Change subject: acpigen: Add some new helper functions
......................................................................

acpigen: Add some new helper functions

These build on existing functions but use different object types
in order to provide functions for upcoming changes:

acpigen_write_return_op(): Return an operator.
acpigen_write_if_lequal_op_op(): Check if 2 operands are equal.
acpigen_get_package_op_element(): Read an element from a package
                                  into an operator.

This one just provides the missing helper, the other 3 already exist:
acpigen_get_tx_gpio: Read TX gpio state.

Signed-off-by: Duncan Laurie 
Change-Id: I1141fd132d6f09cf482f74e95308947cba2c5846
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41985
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/acpi/acpigen.c
M src/include/acpi/acpigen.h
2 files changed, 60 insertions(+), 0 deletions(-)

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

diff --git a/src/acpi/acpigen.c b/src/acpi/acpigen.c
index 793841c..efc5a16 100644
--- a/src/acpi/acpigen.c
+++ b/src/acpi/acpigen.c
@@ -339,6 +339,18 @@
    acpigen_emit_namestring(name);
 }

+void acpigen_get_package_op_element(uint8_t package_op, unsigned int element, uint8_t dest_op)
+{
+	/* <dest_op> = DeRefOf (<package_op>[<element]) */
+	acpigen_write_store();
+	acpigen_emit_byte(DEREF_OP);
+	acpigen_emit_byte(INDEX_OP);
+	acpigen_emit_byte(package_op);
+	acpigen_write_integer(element);
+	acpigen_emit_byte(ZERO_OP); /* Ignore Index() Destination */
+	acpigen_emit_byte(dest_op);
+}
+
 void acpigen_write_processor(u8 cpuindex, u32 pblock_addr, u8 pblock_len)
 {
 /*
@@ -1269,6 +1281,20 @@
 }

/*
+ * Generates ACPI code for checking if operand1 and operand2 are equal.
+ * Both operand1 and operand2 are ACPI ops.
+ *
+ * If (Lequal (op,1 op2))
+ */
+void acpigen_write_if_lequal_op_op(uint8_t op1, uint8_t op2)
+{
+	acpigen_write_if();
+	acpigen_emit_byte(LEQUAL_OP);
+	acpigen_emit_byte(op1);
+	acpigen_emit_byte(op2);
+}
+
+/*
  * Generates ACPI code for checking if operand1 and operand2 are equal, where,
  * operand1 is ACPI op and operand2 is an integer.
  *
@@ -1341,6 +1367,12 @@
    acpigen_write_return_byte_buffer(&arg, 1);
 }

+void acpigen_write_return_op(uint8_t arg)
+{
+	acpigen_emit_byte(RETURN_OP);
+	acpigen_emit_byte(arg);
+}
+
 void acpigen_write_return_byte(uint8_t arg)
 {
    acpigen_emit_byte(RETURN_OP);
@@ -1790,6 +1822,14 @@
    	acpigen_write_xor(LOCAL0_OP, 1, LOCAL0_OP);
 }

+void acpigen_get_tx_gpio(struct acpi_gpio *gpio)
+{
+	acpigen_soc_get_tx_gpio(gpio->pins[0]);
+
+	if (gpio->polarity == ACPI_GPIO_ACTIVE_LOW)
+		acpigen_write_xor(LOCAL0_OP, 1, LOCAL0_OP);
+}
+
 /* refer to ACPI 6.4.3.5.3 Word Address Space Descriptor section for details */
 void acpigen_resource_word(u16 res_type, u16 gen_flags, u16 type_flags, u16 gran,
    u16 range_min, u16 range_max, u16 translation, u16 length)
diff --git a/src/include/acpi/acpigen.h b/src/include/acpi/acpigen.h
index a99489d..dffde85 100644
--- a/src/include/acpi/acpigen.h
+++ b/src/include/acpi/acpigen.h
@@ -360,6 +360,7 @@
 void acpigen_write_debug_op(uint8_t op);
 void acpigen_write_if(void);
 void acpigen_write_if_and(uint8_t arg1, uint8_t arg2);
+void acpigen_write_if_lequal_op_op(uint8_t op, uint8_t val);
 void acpigen_write_if_lequal_op_int(uint8_t op, uint64_t val);
 void acpigen_write_if_lequal_namestr_int(const char *namestr, uint64_t val);
 void acpigen_write_else(void);
@@ -368,6 +369,7 @@
 void acpigen_write_byte_buffer(uint8_t *arr, size_t size);
 void acpigen_write_return_byte_buffer(uint8_t *arr, size_t size);
 void acpigen_write_return_singleton_buffer(uint8_t arg);
+void acpigen_write_return_op(uint8_t arg);
 void acpigen_write_return_byte(uint8_t arg);
 void acpigen_write_upc(enum acpi_upc_type type);
 void acpigen_write_pld(const struct acpi_pld *pld);
@@ -439,6 +441,16 @@
 int get_cst_entries(acpi_cstate_t **);

/*
+ * Get element from package into specified destination op:
+ *   <dest_op> = DeRefOf (<package_op>[<element])
+ *
+ * Example:
+ *  acpigen_get_package_op_element(ARG0_OP, 0, LOCAL0_OP)
+ *  Local0 = DeRefOf (Arg0[0])
+ */
+void acpigen_get_package_op_element(uint8_t package_op, unsigned int element, uint8_t dest_op);
+
+/*
  * Soc-implemented functions for generating ACPI AML code for GPIO handling. All
  * these functions are expected to use only Local5, Local6 and Local7
  * variables. If the functions call into another ACPI method, then there is no
@@ -478,6 +490,14 @@
  */
 void acpigen_get_rx_gpio(struct acpi_gpio *gpio);

+/*
+ *  Helper function for getting a TX GPIO value based on the GPIO polarity.
+ *  The return value is stored in Local0 variable.
+ *  This function ends up calling acpigen_soc_get_tx_gpio to make callbacks
+ *  into SoC acpigen code
+ */
+void acpigen_get_tx_gpio(struct acpi_gpio *gpio);
+
 /* refer to ACPI 6.4.3.5.3 Word Address Space Descriptor section for details */
 void acpigen_resource_word(u16 res_type, u16 gen_flags, u16 type_flags, u16 gran,
    u16 range_min, u16 range_max, u16 translation, u16 length);

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41985 )

Change subject: acpigen: Add some new helper functions
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4738
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4737
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4736
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4735

Please note: This test is under development and might not be accurate at all!