[coreboot-gerrit] Change in coreboot[master]: intel/apollolake: Switch FADT to ACPI version 3.0

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/19146 )

Change subject: intel/apollolake: Switch FADT to ACPI version 3.0
......................................................................


Patch Set 2:

Hey Aaron.
It took me some time to get back to this issue but now I had a closer look and have tried a bit on Apollo Lake.
The following change is the minimum required set to make Windows 10 booting without errors/hangs/bluescreens:

diff --git a/src/soc/intel/apollolake/acpi.c b/src/soc/intel/apollolake/acpi.c
index 3c1faa5..493e0b8 100644
--- a/src/soc/intel/apollolake/acpi.c
+++ b/src/soc/intel/apollolake/acpi.c
@@ -123,14 +123,19 @@ void acpi_fill_fadt(acpi_fadt_t * fadt)
 	fadt->reset_value = 6;
 
 	fadt->x_pm1a_evt_blk.space_id = 1;
-	fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
+	//ACPI Spec requires: "Size: PM1_EVT_LEN / 2"
+	fadt->x_pm1a_evt_blk.bit_width = (fadt->pm1_evt_len * 8) >> 1;
 	fadt->x_pm1a_evt_blk.addrl = pmbase + PM1_STS;
+	//If access_size is missing, Win 10 will crash
+	fadt->x_pm1a_evt_blk.access_size = 2;
 
 	fadt->x_pm1b_evt_blk.space_id = 1;
 
 	fadt->x_pm1a_cnt_blk.space_id = 1;
 	fadt->x_pm1a_cnt_blk.bit_width = fadt->pm1_cnt_len * 8;
 	fadt->x_pm1a_cnt_blk.addrl = pmbase + PM1_CNT;
+	//If access_size is missing, Win 10 will crash
+	fadt->x_pm1a_cnt_blk.access_size = 2;
 
 	fadt->x_pm1b_cnt_blk.space_id = 1;
 
@@ -138,6 +143,11 @@ void acpi_fill_fadt(acpi_fadt_t * fadt)
 	fadt->x_pm_tmr_blk.bit_width = fadt->pm_tmr_len * 8;
 	fadt->x_pm_tmr_blk.addrl = pmbase + PM1_TMR;
 
+	// If the GPE0 entries are missing, Win 10 will end up in a blue screen
+	fadt->x_gpe0_blk.space_id = 1;
+	fadt->x_gpe0_blk.access_size = 1;
+	fadt->x_gpe0_blk.addrl = pmbase + GPE0_STS(0);
+
 	fadt->x_gpe1_blk.space_id = 1;
 }

You will see that the missing sleep_stat and sleep_cnt entries are not needed for the pure booting.
Maybe there are features missing now in Windows 10 that I didn't checked yet. In any case, to be
specification conform I would like to add them. But as ACPI3.0 does not define these fields, we maybe want to
add a Kconfig option to select ACPI5.0 feature set and then add these fields to the structure only if ACPI5.0 is selected. But I am not sure about it yet and would love to hear your opinion here.

So we now have two ways to go:
1. We go back to ACPI3.0 for Apollo Lake and Sky Lake (these are the only two platforms using ACPI5.0)
2. We add the needed code to be ACPI5.0 conform.

I can push a patch with the posted change set and abond this one if you prefer the 2nd. way.
I just want to make it clean in the tree.

Werner

-- 
To view, visit https://review.coreboot.org/19146
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I51c7a7a84d10283f5c2a8a2c57257d53bbdee7ed
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Prabal Saha <coolstarorganization at gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: No