Change in coreboot[master]: ec/google/chromeec: Append connector device to *-switch properties
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
ec/google/chromeec: Append connector device to *-switch properties
The orientation, etc. -switch properties are supposed to use the connector device (underneath the MUX device) that belongs to the port number in question. This patch finds the CONx device and uses that to pass to the acpigen API.
TEST=on Volteer, dump SSDT and verify the *-switch properties point to _SB.PCI0.PNC.MUX.CON0 and CON1
Change-Id: Ie65c2d750f3d9f83285e0e4cc9642110c804bbad Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/ec_acpi.c 1 file changed, 22 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/42045/1
diff --git a/src/ec/google/chromeec/ec_acpi.c b/src/ec/google/chromeec/ec_acpi.c index 0dbaa9c..a434b53 100644 --- a/src/ec/google/chromeec/ec_acpi.c +++ b/src/ec/google/chromeec/ec_acpi.c @@ -124,6 +124,17 @@ port_location_to_str(port_caps.port_location)); }
+extern struct chip_operations drivers_intel_pmc_mux_con_ops; +static int con_id_to_match; + +/* A callback to match a port's connector for dev_find_matching_device_on_bus */ +static bool match_connector(DEVTREE_CONST struct device *dev) +{ + return (dev->chip_ops == &drivers_intel_pmc_mux_con_ops && + dev->path.type == DEVICE_PATH_GENERIC && + dev->path.generic.id == con_id_to_match); +} + static void fill_ssdt_typec_device(const struct device *dev) { int rv; @@ -132,6 +143,7 @@ struct device *usb3_port; struct device *usb4_port; const struct device *mux; + const struct device *con;
if (google_chromeec_get_num_pd_ports(&num_ports)) return; @@ -147,7 +159,14 @@ if (rv) continue;
+ /* Get the MUX device, and find the matching connector on its bus */ + con = NULL; mux = soc_get_pmc_mux_device(i); + if (mux) { + con_id_to_match = i; + con = dev_find_matching_device_on_bus(mux->link_list, match_connector); + } + usb2_port = NULL; usb3_port = NULL; usb4_port = NULL; @@ -160,9 +179,9 @@ .usb2_port = usb2_port, .usb3_port = usb3_port, .usb4_port = usb4_port, - .orientation_switch = mux, - .usb_role_switch = mux, - .mode_switch = mux, + .orientation_switch = con, + .usb_role_switch = con, + .mode_switch = con, };
acpigen_write_typec_connector(&config, i, add_port_location);
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42045
to look at the new patch set (#2).
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
ec/google/chromeec: Append connector device to *-switch properties
The orientation, etc. -switch properties are supposed to use the connector device (underneath the MUX device) that belongs to the port number in question. This patch finds the CONx device and uses that to pass to the acpigen API.
BUG=b:154620502 TEST=on Volteer, dump SSDT and verify the *-switch properties point to _SB.PCI0.PNC.MUX.CON0 and CON1
Change-Id: Ie65c2d750f3d9f83285e0e4cc9642110c804bbad Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/ec_acpi.c 1 file changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/42045/2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 2:
I run into an issue trying to verify this patch. To verify, I 1) cherry-pick, flash and boot volteer 2) cat /sys/firmware/acpi/tables/DDST > /tmp/ssdt.dml" 3) scp DUT:/tmp/ssdt.dml back to my host linux box 4) iasl -d ./ssdt.dml > ./ssdt.txt
The contents of "./ssdt.txt" is the three lines : Intel ACPI Component Architecture ASL+ Optimizing Compiler/Disassembler version 20180810 Copyright (c) 2000 - 2018 Intel Corporation
The following is the stderr output of the "iasl -d ./ssdt.dml > ssdt.txt" :
Found 2 external control methods, reparsing with new information Pass 1 parse of [SSDT] Pass 2 parse of [SSDT] Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
Parsing completed Disassembly completed ASL Output: ./SSDT.dsl - 47137 bytes
iASL Warning: There were 2 external control methods found during disassembly, but only 0 were resolved (2 unresolved). Additional ACPI tables may be required to properly disassemble the code. This resulting disassembler output file may not compile because the disassembler did not know how many arguments to assign to the unresolved methods. Note: SSDTs can be dynamically loaded at runtime and may or may not be available via the host OS.
To specify the tables needed to resolve external control method references, the -e option can be used to specify the filenames. Example iASL invocations: iasl -e ssdt1.aml ssdt2.aml ssdt3.aml -d dsdt.aml iasl -e dsdt.aml ssdt2.aml -d ssdt1.aml iasl -e ssdt*.aml -d dsdt.aml
In addition, the -fe option can be used to specify a file containing control method external declarations with the associated method argument counts. Each line of the file must be of the form: External (<method pathname>, MethodObj, <argument count>) Invocation: iasl -fe refs.txt -d dsdt.aml
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 2:
Patch Set 2:
I run into an issue trying to verify this patch. To verify, I
- cherry-pick, flash and boot volteer
- cat /sys/firmware/acpi/tables/DDST > /tmp/ssdt.dml"
- scp DUT:/tmp/ssdt.dml back to my host linux box
- iasl -d ./ssdt.dml > ./ssdt.txt
The contents of "./ssdt.txt" is the three lines : Intel ACPI Component Architecture ASL+ Optimizing Compiler/Disassembler version 20180810 Copyright (c) 2000 - 2018 Intel Corporation
The following is the stderr output of the "iasl -d ./ssdt.dml > ssdt.txt" :
Found 2 external control methods, reparsing with new information Pass 1 parse of [SSDT] Pass 2 parse of [SSDT] Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
Parsing completed Disassembly completed ASL Output: ./SSDT.dsl - 47137 bytes
iASL Warning: There were 2 external control methods found during disassembly, but only 0 were resolved (2 unresolved). Additional ACPI tables may be required to properly disassemble the code. This resulting disassembler output file may not compile because the disassembler did not know how many arguments to assign to the unresolved methods. Note: SSDTs can be dynamically loaded at runtime and may or may not be available via the host OS.
To specify the tables needed to resolve external control method references, the -e option can be used to specify the filenames. Example iASL invocations: iasl -e ssdt1.aml ssdt2.aml ssdt3.aml -d dsdt.aml iasl -e dsdt.aml ssdt2.aml -d ssdt1.aml iasl -e ssdt*.aml -d dsdt.aml
In addition, the -fe option can be used to specify a file containing control method external declarations with the associated method argument counts. Each line of the file must be of the form: External (<method pathname>, MethodObj, <argument count>) Invocation: iasl -fe refs.txt -d dsdt.aml
FYI, iasl is installed on a test OS image, so if you're using one of those, you can run iasl right there.
Also you can get rid of the unresolved externals by adding the dsdt like so: iasl -e dsdt.dml -d ssdt.dml, etc. The output of this will be ssdt.dsl.
You should check that the _DSD of _SB.PCI0.EC0.CREC.USBC.CON0 has 3 properties, orientation-switch, usb-role-switch, and mode-switch, and all of them should point to _SB.PCI0.PMC.MUX.CON0. And the same thing goes for CON1, which should have the properties pointing to _SB.PCI0.PMC.MUX.CON1.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 2:
Patch Set 2:
I run into an issue trying to verify this patch. To verify, I
- cherry-pick, flash and boot volteer
- cat /sys/firmware/acpi/tables/DDST > /tmp/ssdt.dml"
- scp DUT:/tmp/ssdt.dml back to my host linux box
- iasl -d ./ssdt.dml > ./ssdt.txt
The contents of "./ssdt.txt" is the three lines : Intel ACPI Component Architecture ASL+ Optimizing Compiler/Disassembler version 20180810 Copyright (c) 2000 - 2018 Intel Corporation
The following is the stderr output of the "iasl -d ./ssdt.dml > ssdt.txt" :
Found 2 external control methods, reparsing with new information Pass 1 parse of [SSDT] Pass 2 parse of [SSDT] Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
Parsing completed Disassembly completed ASL Output: ./SSDT.dsl - 47137 bytes
iASL Warning: There were 2 external control methods found during disassembly, but only 0 were resolved (2 unresolved). Additional ACPI tables may be required to properly disassemble the code. This resulting disassembler output file may not compile because the disassembler did not know how many arguments to assign to the unresolved methods. Note: SSDTs can be dynamically loaded at runtime and may or may not be available via the host OS.
To specify the tables needed to resolve external control method references, the -e option can be used to specify the filenames. Example iASL invocations: iasl -e ssdt1.aml ssdt2.aml ssdt3.aml -d dsdt.aml iasl -e dsdt.aml ssdt2.aml -d ssdt1.aml iasl -e ssdt*.aml -d dsdt.aml
In addition, the -fe option can be used to specify a file containing control method external declarations with the associated method argument counts. Each line of the file must be of the form: External (<method pathname>, MethodObj, <argument count>) Invocation: iasl -fe refs.txt -d dsdt.aml
Can you add the output from the resulting .dsl file (the top part with the External declarations)
There are some dependencies that iasl can't resolve without the DSDT because the SSDT code generation is not explicitly adding External() references to method objects used.
If you use both the DSDT and SSDT it should be able to resolve these types of references:
iasl -d /tmp/DSDT /tmp/SSDT
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 2: Code-Review+2
Patch Set 2:
Patch Set 2:
I run into an issue trying to verify this patch. To verify, I
- cherry-pick, flash and boot volteer
- cat /sys/firmware/acpi/tables/DDST > /tmp/ssdt.dml"
- scp DUT:/tmp/ssdt.dml back to my host linux box
- iasl -d ./ssdt.dml > ./ssdt.txt
The contents of "./ssdt.txt" is the three lines : Intel ACPI Component Architecture ASL+ Optimizing Compiler/Disassembler version 20180810 Copyright (c) 2000 - 2018 Intel Corporation
The following is the stderr output of the "iasl -d ./ssdt.dml > ssdt.txt" :
Found 2 external control methods, reparsing with new information Pass 1 parse of [SSDT] Pass 2 parse of [SSDT] Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
Parsing completed Disassembly completed ASL Output: ./SSDT.dsl - 47137 bytes
iASL Warning: There were 2 external control methods found during disassembly, but only 0 were resolved (2 unresolved). Additional ACPI tables may be required to properly disassemble the code. This resulting disassembler output file may not compile because the disassembler did not know how many arguments to assign to the unresolved methods. Note: SSDTs can be dynamically loaded at runtime and may or may not be available via the host OS.
To specify the tables needed to resolve external control method references, the -e option can be used to specify the filenames. Example iASL invocations: iasl -e ssdt1.aml ssdt2.aml ssdt3.aml -d dsdt.aml iasl -e dsdt.aml ssdt2.aml -d ssdt1.aml iasl -e ssdt*.aml -d dsdt.aml
In addition, the -fe option can be used to specify a file containing control method external declarations with the associated method argument counts. Each line of the file must be of the form: External (<method pathname>, MethodObj, <argument count>) Invocation: iasl -fe refs.txt -d dsdt.aml
Can you add the output from the resulting .dsl file (the top part with the External declarations)
There are some dependencies that iasl can't resolve without the DSDT because the SSDT code generation is not explicitly adding External() references to method objects used.
If you use both the DSDT and SSDT it should be able to resolve these types of references:
iasl -d /tmp/DSDT /tmp/SSDT
Thanks, Duncan. Using both /tmp/DSDT and /tmp/SSDT, I still get the same error, but do see that the values in the resulting ssdt.dsl file do point to the correct locations.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
Patch Set 2:
Patch Set 2:
I run into an issue trying to verify this patch. To verify, I
- cherry-pick, flash and boot volteer
- cat /sys/firmware/acpi/tables/DDST > /tmp/ssdt.dml"
- scp DUT:/tmp/ssdt.dml back to my host linux box
- iasl -d ./ssdt.dml > ./ssdt.txt
The contents of "./ssdt.txt" is the three lines : Intel ACPI Component Architecture ASL+ Optimizing Compiler/Disassembler version 20180810 Copyright (c) 2000 - 2018 Intel Corporation
The following is the stderr output of the "iasl -d ./ssdt.dml > ssdt.txt" :
Found 2 external control methods, reparsing with new information Pass 1 parse of [SSDT] Pass 2 parse of [SSDT] Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
Parsing completed Disassembly completed ASL Output: ./SSDT.dsl - 47137 bytes
iASL Warning: There were 2 external control methods found during disassembly, but only 0 were resolved (2 unresolved). Additional ACPI tables may be required to properly disassemble the code. This resulting disassembler output file may not compile because the disassembler did not know how many arguments to assign to the unresolved methods. Note: SSDTs can be dynamically loaded at runtime and may or may not be available via the host OS.
To specify the tables needed to resolve external control method references, the -e option can be used to specify the filenames. Example iASL invocations: iasl -e ssdt1.aml ssdt2.aml ssdt3.aml -d dsdt.aml iasl -e dsdt.aml ssdt2.aml -d ssdt1.aml iasl -e ssdt*.aml -d dsdt.aml
In addition, the -fe option can be used to specify a file containing control method external declarations with the associated method argument counts. Each line of the file must be of the form: External (<method pathname>, MethodObj, <argument count>) Invocation: iasl -fe refs.txt -d dsdt.aml
Can you add the output from the resulting .dsl file (the top part with the External declarations)
There are some dependencies that iasl can't resolve without the DSDT because the SSDT code generation is not explicitly adding External() references to method objects used.
If you use both the DSDT and SSDT it should be able to resolve these types of references:
iasl -d /tmp/DSDT /tmp/SSDT
Thanks, Duncan. Using both /tmp/DSDT and /tmp/SSDT, I still get the same error, but do see that the values in the resulting ssdt.dsl file do point to the correct locations.
You shouldn't get any unresolved externals with both, or we have an issue with our AML: try this: cd /mnt/stateful_partition cat /sys/firmware/acpi/tables/SSDT > ssdt.dml cat /sys/firmware/acpi/tables/DSDT > dsdt.dml iasl -d dsdt.dml iasl -e dsdt.dml -d ssdt.dsl
I synced today and with this patch I don't have any unresolved externals. If you still do, can you post it somewhere?
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review+2
Patch Set 2:
Patch Set 2:
I run into an issue trying to verify this patch. To verify, I
- cherry-pick, flash and boot volteer
- cat /sys/firmware/acpi/tables/DDST > /tmp/ssdt.dml"
- scp DUT:/tmp/ssdt.dml back to my host linux box
- iasl -d ./ssdt.dml > ./ssdt.txt
The contents of "./ssdt.txt" is the three lines : Intel ACPI Component Architecture ASL+ Optimizing Compiler/Disassembler version 20180810 Copyright (c) 2000 - 2018 Intel Corporation
The following is the stderr output of the "iasl -d ./ssdt.dml > ssdt.txt" :
Found 2 external control methods, reparsing with new information Pass 1 parse of [SSDT] Pass 2 parse of [SSDT] Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
Parsing completed Disassembly completed ASL Output: ./SSDT.dsl - 47137 bytes
iASL Warning: There were 2 external control methods found during disassembly, but only 0 were resolved (2 unresolved). Additional ACPI tables may be required to properly disassemble the code. This resulting disassembler output file may not compile because the disassembler did not know how many arguments to assign to the unresolved methods. Note: SSDTs can be dynamically loaded at runtime and may or may not be available via the host OS.
To specify the tables needed to resolve external control method references, the -e option can be used to specify the filenames. Example iASL invocations: iasl -e ssdt1.aml ssdt2.aml ssdt3.aml -d dsdt.aml iasl -e dsdt.aml ssdt2.aml -d ssdt1.aml iasl -e ssdt*.aml -d dsdt.aml
In addition, the -fe option can be used to specify a file containing control method external declarations with the associated method argument counts. Each line of the file must be of the form: External (<method pathname>, MethodObj, <argument count>) Invocation: iasl -fe refs.txt -d dsdt.aml
Can you add the output from the resulting .dsl file (the top part with the External declarations)
There are some dependencies that iasl can't resolve without the DSDT because the SSDT code generation is not explicitly adding External() references to method objects used.
If you use both the DSDT and SSDT it should be able to resolve these types of references:
iasl -d /tmp/DSDT /tmp/SSDT
Thanks, Duncan. Using both /tmp/DSDT and /tmp/SSDT, I still get the same error, but do see that the values in the resulting ssdt.dsl file do point to the correct locations.
You shouldn't get any unresolved externals with both, or we have an issue with our AML: try this: cd /mnt/stateful_partition cat /sys/firmware/acpi/tables/SSDT > ssdt.dml cat /sys/firmware/acpi/tables/DSDT > dsdt.dml iasl -d dsdt.dml iasl -e dsdt.dml -d ssdt.dsl
I synced today and with this patch I don't have any unresolved externals. If you still do, can you post it somewhere?
I repo synced, rebuilt and reflashed and ran through your instructions and did not see any errors this time.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
ec/google/chromeec: Append connector device to *-switch properties
The orientation, etc. -switch properties are supposed to use the connector device (underneath the MUX device) that belongs to the port number in question. This patch finds the CONx device and uses that to pass to the acpigen API.
BUG=b:154620502 TEST=on Volteer, dump SSDT and verify the *-switch properties point to _SB.PCI0.PNC.MUX.CON0 and CON1
Change-Id: Ie65c2d750f3d9f83285e0e4cc9642110c804bbad Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/42045 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nick Vaccaro nvaccaro@google.com --- M src/ec/google/chromeec/ec_acpi.c 1 file changed, 27 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nick Vaccaro: Looks good to me, approved
diff --git a/src/ec/google/chromeec/ec_acpi.c b/src/ec/google/chromeec/ec_acpi.c index 0dbaa9c..8c80382 100644 --- a/src/ec/google/chromeec/ec_acpi.c +++ b/src/ec/google/chromeec/ec_acpi.c @@ -124,6 +124,22 @@ port_location_to_str(port_caps.port_location)); }
+static int con_id_to_match; + +/* A callback to match a port's connector for dev_find_matching_device_on_bus */ +static bool match_connector(DEVTREE_CONST struct device *dev) +{ + if (CONFIG(DRIVERS_INTEL_PMC)) { + extern struct chip_operations drivers_intel_pmc_mux_con_ops; + + return (dev->chip_ops == &drivers_intel_pmc_mux_con_ops && + dev->path.type == DEVICE_PATH_GENERIC && + dev->path.generic.id == con_id_to_match); + } + + return false; +} + static void fill_ssdt_typec_device(const struct device *dev) { int rv; @@ -132,6 +148,7 @@ struct device *usb3_port; struct device *usb4_port; const struct device *mux; + const struct device *con;
if (google_chromeec_get_num_pd_ports(&num_ports)) return; @@ -147,7 +164,14 @@ if (rv) continue;
+ /* Get the MUX device, and find the matching connector on its bus */ + con = NULL; mux = soc_get_pmc_mux_device(i); + if (mux) { + con_id_to_match = i; + con = dev_find_matching_device_on_bus(mux->link_list, match_connector); + } + usb2_port = NULL; usb3_port = NULL; usb4_port = NULL; @@ -160,9 +184,9 @@ .usb2_port = usb2_port, .usb3_port = usb3_port, .usb4_port = usb4_port, - .orientation_switch = mux, - .usb_role_switch = mux, - .mode_switch = mux, + .orientation_switch = con, + .usb_role_switch = con, + .mode_switch = con, };
acpigen_write_typec_connector(&config, i, add_port_location);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42045 )
Change subject: ec/google/chromeec: Append connector device to *-switch properties ......................................................................
Patch Set 3:
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/5305 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5304 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5303 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/5302
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Duncan Laurie (Code Review)
-
Nick Vaccaro (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)