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

[Aamir Bohra (Code Review)]

Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

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

Add a check to enable ACPI timer emulation only when the APCI PM timer
is disabled.

Change-Id: I21c0b89218d0df9336e0b0e15f1b575b8508fb96
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/34563/1

diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c
index 7eb413c..1058443 100644
--- a/src/soc/intel/cannonlake/cpu.c
+++ b/src/soc/intel/cannonlake/cpu.c
@@ -384,8 +384,12 @@
  */
 static void enable_pm_timer_emulation(void)
 {
-	/* ACPI PM timer emulation */
+	config_t *conf = config_of_path(SA_DEVFN_ROOT);
    msr_t msr;
+
+	/* 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.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
File src/soc/intel/cannonlake/cpu.c:

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
PS1, Line 387: config_t 
can we use real structure rather typedef?

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
File src/soc/intel/cannonlake/cpu.c:

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
PS1, Line 387: config_t
> can we use real structure rather typedef?

Used it to be consistent with similar usage in the file.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
File src/soc/intel/cannonlake/cpu.c:

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
PS1, Line 387: config_t
> Used it to be consistent with similar usage in the file.

yeah, i got it, but lately i got a feedback from some reviewer that Cb practice would be avoid typedef as much as we can.

if you see we have mixed approach in some patches where we are using struct soc_intel_cannonlake_config *config and in some cases we are using config_t :)

[Aamir Bohra (Code Review)]

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

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

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

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

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

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

Add a check to enable ACPI timer emulation only when the APCI PM timer
is disabled.

Change-Id: I21c0b89218d0df9336e0b0e15f1b575b8508fb96
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/34563/2

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
File src/soc/intel/cannonlake/cpu.c:

https://review.coreboot.org/c/coreboot/+/34563/1/src/soc/intel/cannonlake/cp... 
PS1, Line 387: config_t
> yeah, i got it, but lately i got a feedback from some reviewer that Cb practice would be avoid typed […]

Ok. Revised under PS#2

[Furquan Shaikh (Code Review)]

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

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

Patch Set 2: Code-Review+2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

Patch Set 2: Code-Review+2

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

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

Add a check to enable ACPI timer emulation only when the APCI PM timer
is disabled.

Change-Id: I21c0b89218d0df9336e0b0e15f1b575b8508fb96
Signed-off-by: Aamir Bohra 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34563
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Subrata Banik 
---
M src/soc/intel/cannonlake/cpu.c
1 file changed, 7 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Subrata Banik: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c
index b0eaa5d..6682cdc 100644
--- a/src/soc/intel/cannonlake/cpu.c
+++ b/src/soc/intel/cannonlake/cpu.c
@@ -384,8 +384,14 @@
  */
 static void enable_pm_timer_emulation(void)
 {
-	/* ACPI PM timer emulation */
+	const struct soc_intel_cannonlake_config *config;
    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.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/34563 )

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

[Nico Huber (Code Review)]

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

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

Patch Set 3:
> Created a revert of this change as If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681

I guess we should first ask why this check was added?

Aamir, Subrata, Furquan, do you know the reason for this
change? The BIOS spec seems to suggest to always enable
it. Are there any downsides?

Please always mention the reason for a change in the
commit message.

[Furquan Shaikh (Code Review)]

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

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

Patch Set 3:
> Patch Set 3:
> > Created a revert of this change as If0d7fa6f9766c7c4e2fa9e846c179adc6a4e1681
>
> I guess we should first ask why this check was added?
>
> Aamir, Subrata, Furquan, do you know the reason for this
> change? The BIOS spec seems to suggest to always enable
> it. Are there any downsides?
>
> Please always mention the reason for a change in the
> commit message.

Posted response on https://review.coreboot.org/c/coreboot/+/45000.