Change in coreboot[master]: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS

The config option APL_SKIP_SET_POWER_LIMITS was accidentally left out
during the set_power_limits refactor (SHA 2adb50d32e8). This patch
reinstates the config option which will cause APL boards to not set any
power limits.

TEST=util/abuild/abuild -p none -t siemens/mc_apl1 -a

Change-Id: Iec9f9f340d50a1212b6ef20c2c0e1b66385ae1b2
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41786/1

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index cc190ba..1bd7332 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -322,6 +322,11 @@
    /* Allocate ACPI NVS in CBMEM */
    cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs_t));

+	if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) {
+		printk(BIOS_INFO, "Skip the RAPL settings.\n");
+		return;
+	}
+
    config = config_of_soc();
    /* Set RAPL MSR for Package power limits */
    soc_config = &config->power_limits_config;

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 1: Code-Review+2

Thanks for submitting this.

[Werner Zeh (Code Review)]

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

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/41786/1/src/soc/intel/apollolake/ch... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/41786/1/src/soc/intel/apollolake/ch... 
PS1, Line 325: if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) {
             : 		printk(BIOS_INFO, "Skip the RAPL settings.\n");
             : 		return;
             : 	}
This is better but having it here would meen that in the case where APL_SKIP_SET_POWER_LIMITS is set the last function set_sci_irq() will not be reached. Would you mind to change it like this:

if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) {
    printk(BIOS_INFO, "Skip the RAPL settings.\n");
} else {
    config = config_of_soc();
    /* Set RAPL MSR for Package power limits */
    soc_config = &config->power_limits_config;
    set_power_limits(MOBILE_SKU_PL1_TIME_SEC, soc_config);
}

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 1: -Code-Review

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Sumeet R Pawnikar, Werner Zeh, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS

The config option APL_SKIP_SET_POWER_LIMITS was accidentally left out
during the set_power_limits refactor (SHA 2adb50d32e8). This patch
reinstates the config option which will cause APL boards to not set any
power limits.

TEST=util/abuild/abuild -p none -t siemens/mc_apl1 -a

Change-Id: Iec9f9f340d50a1212b6ef20c2c0e1b66385ae1b2
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41786/2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41786/1/src/soc/intel/apollolake/ch... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/41786/1/src/soc/intel/apollolake/ch... 
PS1, Line 325: if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) {
             : 		printk(BIOS_INFO, "Skip the RAPL settings.\n");
             : 		return;
             : 	}
> This is better but having it here would meen that in the case where APL_SKIP_SET_POWER_LIMITS is set […]

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41786/2/src/soc/intel/apollolake/ch... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/41786/2/src/soc/intel/apollolake/ch... 
PS2, Line 326: 		printk(BIOS_INFO, "Skip the RAPL settings.\n");
Skip setting RAPL per configuration

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Sumeet R Pawnikar, Werner Zeh, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS

The config option APL_SKIP_SET_POWER_LIMITS was accidentally left out
during the set_power_limits refactor (SHA 2adb50d32e8). This patch
reinstates the config option which will cause APL boards to not set any
power limits.

TEST=util/abuild/abuild -p none -t siemens/mc_apl1 -a

Change-Id: Iec9f9f340d50a1212b6ef20c2c0e1b66385ae1b2
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/41786/3

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41786/2/src/soc/intel/apollolake/ch... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/41786/2/src/soc/intel/apollolake/ch... 
PS2, Line 326: 		printk(BIOS_INFO, "Skip the RAPL settings.\n");
> Skip setting RAPL per configuration

Done

[Werner Zeh (Code Review)]

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

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 3: Code-Review+2

Thank you Tim, that looks good now.

[Sumeet R Pawnikar (Code Review)]

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 3: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS

The config option APL_SKIP_SET_POWER_LIMITS was accidentally left out
during the set_power_limits refactor (SHA 2adb50d32e8). This patch
reinstates the config option which will cause APL boards to not set any
power limits.

TEST=util/abuild/abuild -p none -t siemens/mc_apl1 -a

Change-Id: Iec9f9f340d50a1212b6ef20c2c0e1b66385ae1b2
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41786
Tested-by: build bot (Jenkins) 
Reviewed-by: Werner Zeh 
Reviewed-by: Sumeet R Pawnikar 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 8 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Werner Zeh: Looks good to me, approved
  Sumeet R Pawnikar: Looks good to me, approved

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index cc190ba..f9af4f4 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -322,10 +322,14 @@
    /* Allocate ACPI NVS in CBMEM */
    cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs_t));

-	config = config_of_soc();
-	/* Set RAPL MSR for Package power limits */
-	soc_config = &config->power_limits_config;
-	set_power_limits(MOBILE_SKU_PL1_TIME_SEC, soc_config);
+	if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) {
+		printk(BIOS_INFO, "Skip setting RAPL per configuration\n");
+	} else {
+		config = config_of_soc();
+		/* Set RAPL MSR for Package power limits */
+		soc_config = &config->power_limits_config;
+		set_power_limits(MOBILE_SKU_PL1_TIME_SEC, soc_config);
+	}

/*
    * FSP-S routes SCI to IRQ 9. With the help of this function you can

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41786 )

Change subject: soc/intel/apollolake: Reinstate APL_SKIP_SET_POWER_LIMITS
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4722
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4721
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4720
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4719

Please note: This test is under development and might not be accurate at all!