[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
 > 
 > Thanks for looking into it further. We can likely move back to 3.0.
 > Could you please file a ticket at ticket.coreboot.org and log your
 > findings there so we can track it? It's very good info, and then we
 > know what to do if we want to switch.

OK, I will file the bug. Thanks for your opinion here.

-- 
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