Change in coreboot[master]: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT.
William Wei has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT. ......................................................................
soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT.
pmc_set_acpi_mode() should run after Chrome EC deal with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail.)
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/1
diff --git a/src/soc/intel/tigerlake/pmc.c b/src/soc/intel/tigerlake/pmc.c index 4d89870..a65aeb8 100644 --- a/src/soc/intel/tigerlake/pmc.c +++ b/src/soc/intel/tigerlake/pmc.c @@ -7,6 +7,7 @@ */
#include <acpi/acpigen.h> +#include <bootstate.h> #include <console/console.h> #include <device/mmio.h> #include <device/device.h> @@ -77,8 +78,6 @@ pmc_set_power_failure_state(true); pmc_gpe_init();
- pmc_set_acpi_mode(); - config_deep_s3(config->deep_s3_enable_ac, config->deep_s3_enable_dc); config_deep_s5(config->deep_s5_enable_ac, config->deep_s5_enable_dc); config_deep_sx(config->deep_sx_config); @@ -142,6 +141,30 @@ return child; }
+static void soc_acpi_mode_init(void *unused) +{ + /* + * PMC initialization happens earlier for this SoC because FSP-Silicon + * init hides PMC from PCI bus. However, pmc_set_acpi_mode, which + * disables ACPI mode doesn't need to happen that early and can be + * delayed till typical BS_DEV_INIT. This ensures that ACPI mode + * disabling happens the same way for all SoCs and hence the ordering of + * events is the same. + * + * This is important to ensure that the ordering does not break the + * assumptions of any other drivers (e.g. ChromeEC) which could be + * taking different actions based on disabling of ACPI (e.g. flushing of + * all EC hostevent bits). + * + * P.S.: This cannot be done as part of pmc_soc_init as PMC device is + * hidden and hence the PMC driver never gets enumerated and so init is + * not called for it. + */ + pmc_set_acpi_mode(); +} + +BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, soc_acpi_mode_init, NULL); + struct device_operations pmc_ops = { .read_resources = soc_pmc_read_resources, .set_resources = noop_set_resources,
William Wei has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT. ......................................................................
Patch Set 1: Code-Review+1
This solution sync from src/soc/intel/cannolake/pmc.c
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Kane Chen, Nick Vaccaro, Patrick Rudolph, Zhuohao Lee,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42677
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT ......................................................................
soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT
pmc_set_acpi_mode() should run after Chrome EC deal with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail.)
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/2
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT ......................................................................
Patch Set 2:
just try to think out Should we put this in finalize stage so that any other code like google_chromeec_set_smi_mask to override the smi mask where it shouldn't.
Zhuohao Lee has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... PS2, Line 146: /* : * PMC initialization happens earlier for this SoC because FSP-Silicon : * init hides PMC from PCI bus. However, pmc_set_acpi_mode, which : * disables ACPI mode doesn't need to happen that early and can be : * delayed till typical BS_DEV_INIT. This ensures that ACPI mode : * disabling happens the same way for all SoCs and hence the ordering of : * events is the same. : * : * This is important to ensure that the ordering does not break the : * assumptions of any other drivers (e.g. ChromeEC) which could be : * taking different actions based on disabling of ACPI (e.g. flushing of : * all EC hostevent bits). : * : * P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it. : */ It looks like this comment is from Cannonlake/pmc.c. Is this also true for TGL?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... PS2, Line 146: /* : * PMC initialization happens earlier for this SoC because FSP-Silicon : * init hides PMC from PCI bus. However, pmc_set_acpi_mode, which : * disables ACPI mode doesn't need to happen that early and can be : * delayed till typical BS_DEV_INIT. This ensures that ACPI mode : * disabling happens the same way for all SoCs and hence the ordering of : * events is the same. : * : * This is important to ensure that the ordering does not break the : * assumptions of any other drivers (e.g. ChromeEC) which could be : * taking different actions based on disabling of ACPI (e.g. flushing of : * all EC hostevent bits). : * : * P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it. : */
It looks like this comment is from Cannonlake/pmc.c. […]
yes cnl onwards all pch
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... PS2, Line 166: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, soc_acpi_mode_init, NULL); If this needs to happen in BS_DEV_INIT phase, then it would be better to add .init ops in pmc_ops below and call pmc_set_acpi_mode() from it. This is different than CNL because Tim enabled the PMC device driver for TGL. So, you can make you of device_operations instead of adding BOOT_STATE_* callbacks.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Kane Chen, Tim Wawrzynczak, Subrata Banik, Nick Vaccaro, Patrick Rudolph, Zhuohao Lee,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42677
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT ......................................................................
soc/intel/tigerlake: Change pmc_set_acpi_mode() run after BS_DEV_INIT
pmc_set_acpi_mode() should run after Chrome EC deal with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail.)
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/3
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Kane Chen, Tim Wawrzynczak, Subrata Banik, Nick Vaccaro, Patrick Rudolph, Zhuohao Lee,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42677
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops
pmc_set_acpi_mode() should run after Chrome EC deal with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail.)
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/4
William Wei has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... PS2, Line 146: /* : * PMC initialization happens earlier for this SoC because FSP-Silicon : * init hides PMC from PCI bus. However, pmc_set_acpi_mode, which : * disables ACPI mode doesn't need to happen that early and can be : * delayed till typical BS_DEV_INIT. This ensures that ACPI mode : * disabling happens the same way for all SoCs and hence the ordering of : * events is the same. : * : * This is important to ensure that the ordering does not break the : * assumptions of any other drivers (e.g. ChromeEC) which could be : * taking different actions based on disabling of ACPI (e.g. flushing of : * all EC hostevent bits). : * : * P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it. : */
yes cnl onwards all pch
Reserve comment here, done.
https://review.coreboot.org/c/coreboot/+/42677/2/src/soc/intel/tigerlake/pmc... PS2, Line 166: BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, soc_acpi_mode_init, NULL);
If this needs to happen in BS_DEV_INIT phase, then it would be better to add . […]
Thanks for your suggestion, it works on my side.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Kane Chen, Tim Wawrzynczak, Subrata Banik, Nick Vaccaro, Patrick Rudolph, Zhuohao Lee,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42677
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops
pmc_set_acpi_mode() should run after Chrome EC deal with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail.)
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC Is this part of the comment really correct?
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 158: P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it. Is this really correct?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC
Is this part of the comment really correct?
Depends on what you mean by initialization 😄 Furquan is right, this comment should explain more about how the PCI driver is now enumerated even though the device is "hidden." (see CB:41384, SHA dbcf7b162), so the device_operations are still valid, even though (for example CML) this wasn't the case.
William Wei has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC
Depends on what you mean by initialization 😄 […]
Tim, would you please help to improve the comment here? I just copy the comment from cannolake pmc.c.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC
Tim, would you please help to improve the comment here? I just copy the comment from cannolake pmc. […]
This PMC device is intentionally hide to avoid PCI enumeration. the idea of PCI enumeration is to allocate the DRAM based resources for its MMIO and IO BAR and override the SPI mapped/temp BAR.
Here for PMC device, we don't like to let those resource been overridden hence inside FSP-S, we have made this device disable, if anyone overrides those fixed BAR, the behavior is not known.
1. BB assign PMC BAR MMIO/IO 2. FSP-S make PMC hidden (masked VID/DID) 3. Coreboot publish API to access MMIO/IO resources directly without probing the device from bus 4. PCI enumeration don't see this device PCI: Static device PCI: 00:1f.2 not found, disabling it. 5. You don't see this device in PCI enumeration hence init won't get called as VID/DID would be 0xFFFF
i don't see how init() is getting called here https://review.coreboot.org/c/coreboot/+/41384/11/src/device/pci_device.c?an... pointer ?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42677/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42677/5//COMMIT_MSG@9 PS5, Line 9: deal Past tense: dealt?
https://review.coreboot.org/c/coreboot/+/42677/5//COMMIT_MSG@11 PS5, Line 11: will fail.) Please add the dot/period after the ).
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC
This PMC device is intentionally hide to avoid PCI enumeration. […]
Subrata, I should have added you to the reviews for https://review.coreboot.org/41384, sorry about that. The PMC device is now declared 'hidden' in the devicetree, which means that it is not enumerated, but rather assumed to exist as described and *not* probed. This allows us to do 2 things: 1) publish the resources under the PMC device (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...) 2) SSDT generation (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...)
William, sure, here's what I would write:
pmc_set_acpi_mode() should be delayed until BS_DEV_INIT in order to ensure the ordering does not break the assumptions that other drivers make about ACPI mode (e.g. Chrome EC). Since it disables ACPI mode, other drivers may take different actions based on this (e.g. Chrome EC will flush any pending hostevent bits). Because TGL has its PMC device available for device_operations, it can be done from the "ops->init" callback.
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 158: P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it.
Is this really correct?
suggested rewrite above.
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
Hi William,
"firmware_FwScreenCloseLid" FAFT Test is failing in Jasperlake as well. So i tried to port this CL to JSL and gave a try. But still this test is failing with below error in Jasperlake. Error: “Should shut the device down after calling wait_fw_screen_and_close_lid.”
Could you please help understand, how this CL is explicitly helping to solve the firmware_FwScreenCloseLid FAFT issue in TGL?
William Wei has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
Patch Set 5:
Hi William,
"firmware_FwScreenCloseLid" FAFT Test is failing in Jasperlake as well. So i tried to port this CL to JSL and gave a try. But still this test is failing with below error in Jasperlake. Error: “Should shut the device down after calling wait_fw_screen_and_close_lid.”
Could you please help understand, how this CL is explicitly helping to solve the firmware_FwScreenCloseLid FAFT issue in TGL?
Lid close will generate a SMI event to make the DUT shutdown, but the condition is Chrome EC host event SMI mask bit not 0. pmc_set_acpi_mode() function will set Chrome EC SMI mask to 0x01, but it will be cleared to 0x00 during google_chromeec_events_init(), so LID close during fw_screen, Chrome EC won't send SMI to host to make the DUT shutdown. This CL will delay the pmc_set_acpi_mode() function after google_chromeec_events_init(). Details please kindly refer to https://partnerissuetracker.corp.google.com/issues/153249055#comment125.
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Kane Chen, Tim Wawrzynczak, Subrata Banik, Nick Vaccaro, Patrick Rudolph, Zhuohao Lee,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42677
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops
pmc_set_acpi_mode() should run after Chrome EC dealt with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail).
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/6
William Wei has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42677/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42677/5//COMMIT_MSG@9 PS5, Line 9: deal
Past tense: dealt?
Done
https://review.coreboot.org/c/coreboot/+/42677/5//COMMIT_MSG@11 PS5, Line 11: will fail.)
Please add the dot/period after the ).
Done
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC
Subrata, I should have added you to the reviews for https://review.coreboot. […]
Many thanks, I've updated them to patchset 6.
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 158: P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it.
suggested rewrite above.
Done
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Kane Chen, Tim Wawrzynczak, Subrata Banik, Nick Vaccaro, Patrick Rudolph, Zhuohao Lee,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42677
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops
pmc_set_acpi_mode() should run after Chrome EC dealt with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail).
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/42677/7
William Wei has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 7: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops
pmc_set_acpi_mode() should run after Chrome EC dealt with all host event bits, like SMI mask (otherwise the FAFT firmware_FWScreenCloseLid test will fail).
BUG=b:153249055 TEST=FW_NAME=malefor emerge-volteer coreboot chromeos-bootimage Change the GBB flag to 0x140 then check SMI mask during depthcharge phase, make sure it's 0x0000000000000001.
Signed-off-by: William Wei wenxu.wei@bitland.corp-partner.google.com Change-Id: Icfff5cc5550f23938343e4d26ef76093bb9cf7c3 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42677 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/tigerlake/pmc.c 1 file changed, 15 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved William Wei: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/tigerlake/pmc.c b/src/soc/intel/tigerlake/pmc.c index 4d89870..b98cbbe 100644 --- a/src/soc/intel/tigerlake/pmc.c +++ b/src/soc/intel/tigerlake/pmc.c @@ -77,8 +77,6 @@ pmc_set_power_failure_state(true); pmc_gpe_init();
- pmc_set_acpi_mode(); - config_deep_s3(config->deep_s3_enable_ac, config->deep_s3_enable_dc); config_deep_s5(config->deep_s5_enable_ac, config->deep_s5_enable_dc); config_deep_sx(config->deep_sx_config); @@ -142,9 +140,24 @@ return child; }
+static void soc_acpi_mode_init(struct device *dev) +{ + /* + * pmc_set_acpi_mode() should be delayed until BS_DEV_INIT in order + * to ensure the ordering does not break the assumptions that other + * drivers make about ACPI mode (e.g. Chrome EC). Since it disables + * ACPI mode, other drivers may take different actions based on this + * (e.g. Chrome EC will flush any pending hostevent bits). Because + * TGL has its PMC device available for device_operations, it can be + * done from the "ops->init" callback. + */ + pmc_set_acpi_mode(); +} + struct device_operations pmc_ops = { .read_resources = soc_pmc_read_resources, .set_resources = noop_set_resources, + .init = soc_acpi_mode_init, .enable = pmc_init, #if CONFIG(HAVE_ACPI_TABLES) .acpi_fill_ssdt = soc_pmc_fill_ssdt,
-
Furquan Shaikh (Code Review) -
Kane Chen (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Usha P (Code Review)
-
William Wei (Code Review)
-
Zhuohao Lee (Code Review)