Change in coreboot[master]: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer...

[Michael Niewöhner (Code Review)]

Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Subrata Banik, Aamir Bohra, Aaron Durbin, Patrick Rudolph,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, emulation shall be always enabled.

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/45000/1

diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c
index 6682cdc..b0eaa5d 100644
--- a/src/soc/intel/cannonlake/cpu.c
+++ b/src/soc/intel/cannonlake/cpu.c
@@ -384,14 +384,8 @@
  */
 static void enable_pm_timer_emulation(void)
 {
-	const struct soc_intel_cannonlake_config *config;
+	/* ACPI PM timer emulation */
    msr_t msr;
-
-	config = config_of_path(SA_DEVFN_ROOT);
-
-	/* Enable PM timer emulation only if ACPI PM timer is disabled */
-	if (!config->PmTimerDisabled)
-		return;
    /*
     * The derived frequency is calculated as follows:
     *    (CTC_FREQ * msr[63:32]) >> 32 = target frequency.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/45000/1//COMMIT_MSG@11 
PS1, Line 11: BIOS spec says, emulation shall be always enabled.
BIOS spec actually says that it is *recommended* to always enable the ACPI Timer Emulation, regardless of the OS version. Plus, BIOS is *required* to configure the PM_TMR_EMULATION_CFG MSR 0x121 before BIOS_RESET_CPL.

[Michael Niewöhner (Code Review)]

Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Subrata Banik, Aamir Bohra, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, emulation shall be always enabled.

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/45000/2

[Michael Niewöhner (Code Review)]

Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Subrata Banik, Aamir Bohra, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, emulation shall be always enabled.

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
Signed-off-by: Michael Niewöhner 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/45000/3

[Michael Niewöhner (Code Review)]

Hello Bora Guvendik, build bot (Jenkins), Nico Huber, Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Subrata Banik, Aamir Bohra, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, it's recommended to always enable
emulation (regardless of the OS version).

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
Signed-off-by: Michael Niewöhner 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/45000/4

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/45000/1//COMMIT_MSG@11 
PS1, Line 11: BIOS spec says, emulation shall be always enabled.
> BIOS spec actually says that it is *recommended* to always enable the ACPI Timer Emulation, regardle […]

thanks, reworded this

[Michael Niewöhner (Code Review)]

Hello Bora Guvendik, build bot (Jenkins), Nico Huber, Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Subrata Banik, Aamir Bohra, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, it's recommended to always enable
emulation (regardless of the OS version).

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
Signed-off-by: Michael Niewöhner 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/45000/5

[Michael Niewöhner (Code Review)]

Hello Bora Guvendik, build bot (Jenkins), Nico Huber, Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Subrata Banik, Aamir Bohra, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, it's recommended to always enable
emulation (regardless of the OS version).

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
Signed-off-by: Michael Niewöhner 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 0 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/45000/6

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

Hey folks, does anyone have an opinion on this? I'd prefer to
get it in, simply because it avoids complexity. I don't think,
but can't prove, that it's really harmless to have both the
emulation and the actual timer available.

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

https://review.coreboot.org/c/coreboot/+/45000/1//COMMIT_MSG@11 
PS1, Line 11: BIOS spec says, emulation shall be always enabled.
> thanks, reworded this

It's not unambiguous... Could also mean that this particular order is
required if it's going to be enabled.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 6:
> Hey folks, does anyone have an opinion on this? I'd prefer to
> get it in, simply because it avoids complexity. I don't think,

s/don't//

[Furquan Shaikh (Code Review)]

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

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 6: Code-Review+2
> Patch Set 6: Code-Review+1
>
> (1 comment)
>
> Hey folks, does anyone have an opinion on this? I'd prefer to
> get it in, simply because it avoids complexity. I don't think,
> but can't prove, that it's really harmless to have both the
> emulation and the actual timer available.

Sorry about the delay in response. I digged through my notes as well as the specs I have access to, but I don't see any reason why we cannot have multiple timers available. I believe the confusion was around the comment in some of the specs that emulated timer needs to be enabled when the ACPI PM hardware timer is disabled which is required for power savings. But, it does not apply the other way around i.e. having emulated timer enabled does not require the ACPI PM hardware timer to be disabled.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 6:
> Patch Set 6: Code-Review+2
> > Patch Set 6: Code-Review+1
> >
> > (1 comment)
> >
> > Hey folks, does anyone have an opinion on this? I'd prefer to
> > get it in, simply because it avoids complexity. I don't think,
> > but can't prove, that it's really harmless to have both the
> > emulation and the actual timer available.
>
> Sorry about the delay in response. I digged through my notes as well as the specs I have access to, but I don't see any reason why we cannot have multiple timers available. I believe the confusion was around the comment in some of the specs that emulated timer needs to be enabled when the ACPI PM hardware timer is disabled which is required for power savings. But, it does not apply the other way around i.e. having emulated timer enabled does not require the ACPI PM hardware timer to be disabled.

Thanks for checking this. That's how I understood it, too

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6: Code-Review+2
> > > Patch Set 6: Code-Review+1
> > >
> > > (1 comment)
> > >
> > > Hey folks, does anyone have an opinion on this? I'd prefer to
> > > get it in, simply because it avoids complexity. I don't think,
> > > but can't prove, that it's really harmless to have both the
> > > emulation and the actual timer available.
> >
> > Sorry about the delay in response. I digged through my notes as well as the specs I have access to, but I don't see any reason why we cannot have multiple timers available. I believe the confusion was around the comment in some of the specs that emulated timer needs to be enabled when the ACPI PM hardware timer is disabled which is required for power savings. But, it does not apply the other way around i.e. having emulated timer enabled does not require the ACPI PM hardware timer to be disabled.
>
> Thanks for checking this. That's how I understood it, too

But keep in mind that the ACPI PM hardware timer would never be accessed because the emulation enablement will trap the IO access in the CPU. So it's sort of moot.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> > > Patch Set 6: Code-Review+2
> > > > Patch Set 6: Code-Review+1
> > > >
> > > > (1 comment)
> > > >
> > > > Hey folks, does anyone have an opinion on this? I'd prefer to
> > > > get it in, simply because it avoids complexity. I don't think,
> > > > but can't prove, that it's really harmless to have both the
> > > > emulation and the actual timer available.
> > >
> > > Sorry about the delay in response. I digged through my notes as well as the specs I have access to, but I don't see any reason why we cannot have multiple timers available. I believe the confusion was around the comment in some of the specs that emulated timer needs to be enabled when the ACPI PM hardware timer is disabled which is required for power savings. But, it does not apply the other way around i.e. having emulated timer enabled does not require the ACPI PM hardware timer to be disabled.
> >
> > Thanks for checking this. That's how I understood it, too
>
> But keep in mind that the ACPI PM hardware timer would never be accessed because the emulation enablement will trap the IO access in the CPU. So it's sort of moot.

That's mostly right, but there's one exception: PCH itself can access the PM timer, for example TCO, which needs the hardware timer

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"

This reverts commit e5269a8fd975fa0cba0655cd41f7f8cc99a1feb8.

Reason for revert: BIOS spec says, it's recommended to always enable
emulation (regardless of the OS version).

Change-Id: If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
Signed-off-by: Michael Niewöhner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45000
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 0 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c
index 0c24535..3d97c56 100644
--- a/src/soc/intel/cannonlake/cpu.c
+++ b/src/soc/intel/cannonlake/cpu.c
@@ -112,14 +112,8 @@
  */
 static void enable_pm_timer_emulation(void)
 {
-	const struct soc_intel_cannonlake_config *config;
    msr_t msr;

-	config = config_of_soc();
-
-	/* Enable PM timer emulation only if ACPI PM timer is disabled */
-	if (!config->PmTimerDisabled)
-		return;
    /*
     * The derived frequency is calculated as follows:
     *    (CTC_FREQ * msr[63:32]) >> 32 = target frequency.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45000 )

Change subject: Revert "soc/intel/cannonlake: Enable ACPI timer emulation if PM timer is disabled"
......................................................................

Patch Set 8:

(1 comment)

Patchset:

PS8:
> Patch Set 6:
> > Patch Set 6:
> > > Patch Set 6:
> > > > Patch Set 6: Code-Review+2
> > > > > Patch Set 6: Code-Review+1
> > > > >
> > > > > (1 comment)
> > > > >
> > > > > Hey folks, does anyone have an opinion on this? I'd prefer to
> > > > > get it in, simply because it avoids complexity. I don't think,
> > > > > but can't prove, that it's really harmless to have both the
> > > > > emulation and the actual timer available.
> > > >
> > > > Sorry about the delay in response. I digged through my notes as well as the specs I have access to, but I don't see any reason why we cannot have multiple timers available. I believe the confusion was around the comment in some of the specs that emulated timer needs to be enabled when the ACPI PM hardware timer is disabled which is required for power savings. But, it does not apply the other way around i.e. having emulated timer enabled does not require the ACPI PM hardware timer to be disabled.
> > >
> > > Thanks for checking this. That's how I understood it, too
> >
> > But keep in mind that the ACPI PM hardware timer would never be accessed because the emulation enablement will trap the IO access in the CPU. So it's sort of moot.
>
> That's mostly right, but there's one exception: PCH itself can access the PM timer, for example TCO, which needs the hardware timer

This needs to be discussed again. see CB:57930