coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: soc/intel/alderlake: Hook up common code for thermal configuration
by Subrata Banik (Code Review) 20 Nov '21

20 Nov '21

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59271 ) Change subject: soc/intel/alderlake: Hook up common code for thermal configuration ...................................................................... soc/intel/alderlake: Hook up common code for thermal configuration Thermal configuration registers are now located behind PMC PWRMBASE for Alder Lake Point PCH. Hence, ADL SoC to select SOC_INTEL_COMMON_BLOCK_THERMAL_BEHIND_PMC to let thermal low threshold is being set as per mainboard provided `pch_thermal_trip`. Note: These thermal configuration registers are RW/O hence, setting those early prior to FSP-S helps coreboot to set the desired low thermal threshold for the platform. BUG=b:193774296 TEST=Dump thermal configuration registers PWRMBASE+0x150c etc. prior to FSP-S shows that registers are now programmed based on 'pch_thermal_trip' and lock register BIT31 is set. Change-Id: I0f972f47845c123f4f74fd75091c9703d54db796 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/59271 Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/soc/intel/alderlake/Kconfig M src/soc/intel/alderlake/romstage/romstage.c 2 files changed, 12 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, approved diff --git a/src/soc/intel/alderlake/Kconfig b/src/soc/intel/alderlake/Kconfig index 58b9051..2cbff00 100644 --- a/src/soc/intel/alderlake/Kconfig +++ b/src/soc/intel/alderlake/Kconfig @@ -68,6 +68,7 @@ select SOC_INTEL_COMMON_BLOCK_SMM select SOC_INTEL_COMMON_BLOCK_SMM_IO_TRAP select SOC_INTEL_COMMON_BLOCK_TCSS + select SOC_INTEL_COMMON_BLOCK_THERMAL_BEHIND_PMC select SOC_INTEL_COMMON_BLOCK_USB4 select SOC_INTEL_COMMON_BLOCK_USB4_PCIE select SOC_INTEL_COMMON_BLOCK_USB4_XHCI diff --git a/src/soc/intel/alderlake/romstage/romstage.c b/src/soc/intel/alderlake/romstage/romstage.c index d7ef14d..e84eca8 100644 --- a/src/soc/intel/alderlake/romstage/romstage.c +++ b/src/soc/intel/alderlake/romstage/romstage.c @@ -8,6 +8,7 @@ #include <intelblocks/cse.h> #include <intelblocks/pmclib.h> #include <intelblocks/smbus.h> +#include <intelblocks/thermal.h> #include <memory_info.h> #include <soc/intel/common/smbios.h> #include <soc/iomap.h> @@ -127,6 +128,16 @@ heci_init(HECI1_BASE_ADDRESS); s3wake = pmc_fill_power_state(ps) == ACPI_S3; + + /* + * Set low maximum temp threshold value used for dynamic thermal sensor + * shutdown consideration. + * + * If Dynamic Thermal Shutdown is enabled then PMC logic shuts down the + * thermal sensor when CPU is in a C-state and LTT >= DTS Temp. + */ + pch_thermal_configuration(); + fsp_memory_init(s3wake); pmc_set_disb(); if (!s3wake) { -- To view, visit https://review.coreboot.org/c/coreboot/+/59271 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0f972f47845c123f4f74fd75091c9703d54db796 Gerrit-Change-Number: 59271 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/intel/alderlake: Set `pch_thermal_trip` for Dynamic Thermal Shutdown
by Subrata Banik (Code Review) 20 Nov '21

20 Nov '21

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59270 ) Change subject: soc/intel/alderlake: Set `pch_thermal_trip` for Dynamic Thermal Shutdown ...................................................................... soc/intel/alderlake: Set `pch_thermal_trip` for Dynamic Thermal Shutdown Set low maximum temp threshold value used for dynamic thermal sensor shutdown consideration. BUG=b:193774296 Change-Id: I7ee199c19a9d926a4135eeef3b3b481fbff74a79 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/59270 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> --- M src/soc/intel/alderlake/chipset.cb 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified EricR Lai: Looks good to me, approved diff --git a/src/soc/intel/alderlake/chipset.cb b/src/soc/intel/alderlake/chipset.cb index 9a752dd..fc73a89 100644 --- a/src/soc/intel/alderlake/chipset.cb +++ b/src/soc/intel/alderlake/chipset.cb @@ -48,6 +48,11 @@ .tdp_pl4 = 123, }" + # NOTE: if any variant wants to override this value, use the same format + # as register "common_soc_config.pch_thermal_trip" = "value", instead of + # putting it under register "common_soc_config" in overridetree.cb file. + register "common_soc_config.pch_thermal_trip" = "100" + device domain 0 on device gpio 0 alias pch_gpio on end device pci 00.0 alias system_agent on end 5 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://review.coreboot.org/c/coreboot/+/59270 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7ee199c19a9d926a4135eeef3b3b481fbff74a79 Gerrit-Change-Number: 59270 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/intel/common/thermal: Allow thermal configuration over PMC
by Subrata Banik (Code Review) 20 Nov '21

20 Nov '21

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/59209 ) Change subject: soc/intel/common/thermal: Allow thermal configuration over PMC ...................................................................... soc/intel/common/thermal: Allow thermal configuration over PMC Thermal configuration has evolved over PCH generations where latest PCH has provided an option to allow thermal configuration using PMC PWRMBASE registers. This patch adds an option for impacted SoC to select the Kconfig for allowing thermal configuration using PMC PCH MMIO space. BUG=b:193774296 TEST=Able to build and boot hatch and adlrvp platform. Change-Id: I0c6ae72610da39fc18ff252c440d006e83c570a0 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/59209 Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Reviewed-by: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/soc/intel/common/block/include/intelblocks/thermal.h M src/soc/intel/common/block/thermal/Kconfig M src/soc/intel/common/block/thermal/Makefile.inc A src/soc/intel/common/block/thermal/thermal_pmc.c 4 files changed, 126 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, but someone else must approve EricR Lai: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved diff --git a/src/soc/intel/common/block/include/intelblocks/thermal.h b/src/soc/intel/common/block/include/intelblocks/thermal.h index 377b2f9..aa3318c 100644 --- a/src/soc/intel/common/block/include/intelblocks/thermal.h +++ b/src/soc/intel/common/block/include/intelblocks/thermal.h @@ -3,6 +3,33 @@ #ifndef _SOC_INTEL_COMMON_BLOCK_THERMAL_H_ #define _SOC_INTEL_COMMON_BLOCK_THERMAL_H_ +/* Catastrophic Trip Point Enable */ +#define PMC_PWRM_THERMAL_CTEN 0x150c +/* Policy Lock-Down Bit */ +#define PMC_PWRM_THERMAL_CTEN_CTENLOCK (1 << 31) +/* Catastrophic Power-Down Enable */ +#define PMC_PWRM_THERMAL_CTEN_CPDEN (1 << 0) +/* EC Thermal Sensor Reporting Enable */ +#define PMC_PWRM_THERMAL_ECRPTEN 0x1510 +/* Lock-Down Bit */ +#define PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK (1 << 31) +/* Enable PMC to EC Temp Reporting */ +#define PMC_PWRM_THERMAL_ECRPTEN_EN_RPT (1 << 0) +/* Throttle Levels */ +#define PMC_PWRM_THERMAL_TL 0x1520 +/* TL LOCK */ +#define PMC_PWRM_THERMAL_TL_TLLOCK (1 << 31) +/* TT Enable */ +#define PMC_PWRM_THERMAL_TL_TTEN (1 << 29) +/* Throttle Levels Enable */ +#define PMC_PWRM_THERMAL_TLEN 0x1528 +/* TLENLOCK */ +#define PMC_PWRM_THERMAL_TLEN_TLENLOCK (1 << 31) +/* PCH Hot Level Control */ +#define PMC_PWRM_THERMAL_PHLC 0x1540 +/* PHL Lock */ +#define PMC_PWRM_THERMAL_PHLC_PHLCLOCK (1 << 31) + /* Enable thermal sensor power management */ void pch_thermal_configuration(void); diff --git a/src/soc/intel/common/block/thermal/Kconfig b/src/soc/intel/common/block/thermal/Kconfig index 0605176..b39c74c 100644 --- a/src/soc/intel/common/block/thermal/Kconfig +++ b/src/soc/intel/common/block/thermal/Kconfig @@ -3,3 +3,10 @@ default n help This option allows to configure PCH thermal registers for supported PCH. + +config SOC_INTEL_COMMON_BLOCK_THERMAL_BEHIND_PMC + bool + default n + help + This option allows to configure PCH thermal registers using PMC PWRMBASE + for chipsets since Tiger Lake PCH. diff --git a/src/soc/intel/common/block/thermal/Makefile.inc b/src/soc/intel/common/block/thermal/Makefile.inc index 951065c..2a65219 100644 --- a/src/soc/intel/common/block/thermal/Makefile.inc +++ b/src/soc/intel/common/block/thermal/Makefile.inc @@ -1,2 +1,3 @@ romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_THERMAL) += thermal.c +romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_THERMAL_BEHIND_PMC) += thermal_pmc.c ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_THERMAL) += thermal.c diff --git a/src/soc/intel/common/block/thermal/thermal_pmc.c b/src/soc/intel/common/block/thermal/thermal_pmc.c new file mode 100644 index 0000000..d233e9d --- /dev/null +++ b/src/soc/intel/common/block/thermal/thermal_pmc.c @@ -0,0 +1,91 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <device/mmio.h> +#include <intelblocks/cfg.h> +#include <intelblocks/pmclib.h> +#include <intelblocks/thermal.h> + +#define MAX_TRIP_TEMP 205 +/* This is the safest default Trip Temp value */ +#define DEFAULT_TRIP_TEMP 50 + +/* + * Trip Point = T2L | T1L | T0L where T2L > T1L > T0L + * T2L = Bit 28:20 + * T1L = Bit 18:10 + * T0L = Bit 8:0 + */ +#define GET_LTT_VALUE(x) ((x + 10) << 20 | (x + 5) << 10 | x) + +static uint8_t get_thermal_trip_temp(void) +{ + const struct soc_intel_common_config *common_config; + common_config = chip_get_common_soc_structure(); + + return common_config->pch_thermal_trip; +} + +/* PCH Low Temp Threshold (LTT) */ +static uint32_t pch_get_ltt_value(void) +{ + uint8_t thermal_config; + + thermal_config = get_thermal_trip_temp(); + if (!thermal_config) + thermal_config = DEFAULT_TRIP_TEMP; + + if (thermal_config > MAX_TRIP_TEMP) + die("Input PCH temp trip is higher than allowed range!"); + + return GET_LTT_VALUE(thermal_config); +} + +/* + * Thermal configuration has evolved over time. With older platform the + * thermal device is sitting over PCI and allow to configure its configuration + * register by accessing the PCI configuration space or MMIO space. + * + * Since Tiger Lake, thermal registers are being moved behind the PMC PCI device + * hence, accessing thermal configuration registers would need making access + * to PWRMBASE. In this case SoC Kconfig to select + * SOC_INTEL_COMMON_BLOCK_THERMAL_BEHIND_PMC to allow thermal configuration. + */ +void pch_thermal_configuration(void) +{ + uintptr_t pmc_bar = soc_read_pmc_base(); + + struct pmc_thermal_config { + uint16_t offset; + uint32_t mask; + uint32_t value; + } config[] = { + { + .offset = PMC_PWRM_THERMAL_CTEN, + .value = PMC_PWRM_THERMAL_CTEN_CPDEN | PMC_PWRM_THERMAL_CTEN_CTENLOCK, + }, + { + .offset = PMC_PWRM_THERMAL_ECRPTEN, + .value = PMC_PWRM_THERMAL_ECRPTEN_EN_RPT + | PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK, + }, + { + .offset = PMC_PWRM_THERMAL_TL, + .mask = ~0, + .value = pch_get_ltt_value() | PMC_PWRM_THERMAL_TL_TTEN + | PMC_PWRM_THERMAL_TL_TLLOCK, + }, + { + .offset = PMC_PWRM_THERMAL_PHLC, + .value = PMC_PWRM_THERMAL_PHLC_PHLCLOCK, + }, + { + .offset = PMC_PWRM_THERMAL_TLEN, + .value = PMC_PWRM_THERMAL_TLEN_TLENLOCK, + }, + }; + + for (int i = 0; i < ARRAY_SIZE(config); i++) + clrsetbits32((void *)(pmc_bar + config[i].offset), config[i].mask, + config[i].value); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/59209 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0c6ae72610da39fc18ff252c440d006e83c570a0 Gerrit-Change-Number: 59209 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: lib: Add a mutex
by Kyösti Mälkki (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Raul Rangel, Julius Werner, Rob Barnes, Karthik Ramasubramanian. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 ) Change subject: lib: Add a mutex ...................................................................... Patch Set 6: (2 comments) File src/lib/mutex.c: https://review.coreboot.org/c/coreboot/+/59320/comment/56b84a91_c57defba PS5, Line 24: #if ENV_X86 No arch-dependency under lib/ please. We should enforce symmetry of headers placed under arch/, this could possibly be <arch/cpu.h> with cpu_relax(). https://review.coreboot.org/c/coreboot/+/59320/comment/7d896ffc_139930af PS5, Line 37: assert(thread_yield() >= 0); Are the assert() here and below why you did not want to change printk() to mutex? We have other places where printk() is not allowed, like inside udelay() and cbmem_top(), thread_yield() could be made one of them? -- To view, visit https://review.coreboot.org/c/coreboot/+/59320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I41e02a54a17b1f6513b36a0274e43fc715472d78 Gerrit-Change-Number: 59320 Gerrit-PatchSet: 6 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 20 Nov 2021 17:00:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/intel/hyperthreading: Add missing header <arch/cpu.h>
by Kyösti Mälkki (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Raul Rangel, Patrick Rudolph. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59356 ) Change subject: cpu/intel/hyperthreading: Add missing header <arch/cpu.h> ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59356 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idc3daa64562c4a4d57b678f13726509b480ba050 Gerrit-Change-Number: 59356 Gerrit-PatchSet: 4 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sat, 20 Nov 2021 16:22:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: treewide: Replace spinlock calls with mutex
by Kyösti Mälkki (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Raul Rangel, Julius Werner, Rob Barnes, Patrick Rudolph, Karthik Ramasubramanian. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59321 ) Change subject: treewide: Replace spinlock calls with mutex ...................................................................... Patch Set 5: (6 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/59321/comment/f5399dd0_099e85ab PS5, Line 22: I think printk() is exactly where you would want to yield in romstage for DMA performance, and that was the only case where I had expected to see use of a mutex. File src/cpu/intel/microcode/microcode.c: https://review.coreboot.org/c/coreboot/+/59321/comment/91885047_71caf618 PS5, Line 14: struct mutex microcode_lock; static File src/cpu/x86/lapic/lapic_cpu_init.c: https://review.coreboot.org/c/coreboot/+/59321/comment/b9674204_8752875a PS5, Line 218: struct mutex start_cpu_lock; static File src/cpu/x86/mp_init.c: https://review.coreboot.org/c/coreboot/+/59321/comment/770dbf3e_a5bd440f PS5, Line 662: struct mutex smm_relocation_lock; static File src/device/device.c: https://review.coreboot.org/c/coreboot/+/59321/comment/20a4bfbc_f9a0feb7 PS5, Line 74: struct mutex dev_lock; static File src/drivers/pc80/rtc/post.c: https://review.coreboot.org/c/coreboot/+/59321/comment/7dca46ad_39f9855b PS5, Line 38: struct mutex cmos_post_lock; static -- To view, visit https://review.coreboot.org/c/coreboot/+/59321 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibee331a9ccd491e819c7f2c9a19bca7dc4379d9d Gerrit-Change-Number: 59321 Gerrit-PatchSet: 5 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 20 Nov 2021 16:21:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib: Add a mutex
by Kyösti Mälkki (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Raul Rangel, Julius Werner, Rob Barnes, Karthik Ramasubramanian. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 ) Change subject: lib: Add a mutex ...................................................................... Patch Set 6: (2 comments) File src/include/mutex.h: https://review.coreboot.org/c/coreboot/+/59320/comment/e7f9117f_f72a440d PS6, Line 32: } I put the following in <rules.h> #if ENV_X86 #define STAGE_HAS_SPINLOCKS !ENV_ROMSTAGE_OR_BEFORE #elif ENV_RISCV #define STAGE_HAS_SPINLOCKS 1 #else #define STAGE_HAS_SPINLOCKS 0 #endif /* When set <arch/smp/spinlock.h> is included for the spinlock implementation. */ #define ENV_STAGE_SUPPORTS_SMP (CONFIG(SMP) && STAGE_HAS_SPINLOCKS) The implementation of spinlock needed .data while mutex can do with .bss? With that taken into account, you would never call the non-smp variants _mutex_lock_coop() or _mutex_unlock_coop() since romstage would also see ENV_STAGE_SUPPORTS_SMP=y ? File src/lib/mutex.c: https://review.coreboot.org/c/coreboot/+/59320/comment/9fcfa56c_42563eea PS6, Line 23: if (thread_yield() < 0) { This path already handles COOP_MULTITHREADING=y, do you really want a separate _mutex_lock_coop() below that does not quarantee atomicity? -- To view, visit https://review.coreboot.org/c/coreboot/+/59320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I41e02a54a17b1f6513b36a0274e43fc715472d78 Gerrit-Change-Number: 59320 Gerrit-PatchSet: 6 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 20 Nov 2021 16:06:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: arch/x86: Allocate stack on .bss
by Arthur Heymans (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Arthur Heymans. Hello Arthur Heymans, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/59508 to review the following change. Change subject: arch/x86: Allocate stack on .bss ...................................................................... arch/x86: Allocate stack on .bss Just like ramstage, the stack can be allocated on .bss instead of requiring special linker script symbols. Change-Id: I7504f78b1dc214f51caa85d0fbff5daf70251a31 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/intel/car/romstage.c M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/cpu/x86/entry32.S M src/drivers/amd/agesa/cache_as_ram.S M src/drivers/intel/fsp1_1/cache_as_ram.S M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S M src/soc/intel/quark/bootblock/esram_init.S M src/soc/intel/quark/romstage/fsp_params.c 16 files changed, 46 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/59508/1 diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 6e73027..889ae09 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -9,11 +9,19 @@ * clear .bss variables that are stage specific. */ -#if CONFIG(RESET_VECTOR_IN_RAM) - #define _STACK_TOP _eearlyram_stack -#else - #define _STACK_TOP _ecar_stack -#endif + +/* Place the stack in the bss section. It's not necessary to define it in the + * the linker script. */ + .section .bss, "aw", @nobits +.global _stack +.global _estack +.global _stack_size + +.align 8 +_stack: +.space CONFIG_DCACHE_BSP_STACK_SIZE +_estack: +.set _stack_size, _estack - _stack #if ENV_X86_64 .code64 @@ -33,7 +41,7 @@ #endif /* reset stack pointer to CAR/EARLYRAM stack */ - mov $_STACK_TOP, %esp + mov $_estack, %esp /* clear .bss section as it is not shared */ cld diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 5a46b8b..09e171f 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -23,10 +23,6 @@ * Needs to be transferred until CBMEM is available */ TPM_TCPA_LOG(., 2K) #endif - /* Stack for CAR stages. Since it persists across all stages that - * use CAR it can be reused. The chipset/SoC is expected to provide - * the stack size. */ - REGION(car_stack, ., CONFIG_DCACHE_BSP_STACK_SIZE, 4) /* The pre-ram cbmem console as well as the timestamp region are fixed * in size. Therefore place them above the car global section so that * multiple stages (romstage and verstage) have a consistent diff --git a/src/arch/x86/include/arch/symbols.h b/src/arch/x86/include/arch/symbols.h index caafdfa..df2536e 100644 --- a/src/arch/x86/include/arch/symbols.h +++ b/src/arch/x86/include/arch/symbols.h @@ -12,14 +12,6 @@ extern char _car_region_end[]; #define _car_region_size (_car_region_end - _car_region_start) -/* - * This is the stack area used for all stages that execute when cache-as-ram - * is up. Area is not cleared in between stages. - */ -extern char _car_stack[]; -extern char _ecar_stack[]; -#define _car_stack_size (_ecar_stack - _car_stack) - extern char _car_unallocated_start[]; extern char _car_ehci_dbg_info[]; diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index f47ba5f..29b617b 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -152,7 +152,7 @@ movl %eax, %cr0 /* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp /* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/non-evict/cache_as_ram.S b/src/cpu/intel/car/non-evict/cache_as_ram.S index 0451bb4..e006f04 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -204,7 +204,7 @@ movl %eax, %cr0 /* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp /* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 887bb22..2becd1e 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -139,7 +139,7 @@ movl %eax, %cr0 /* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp /* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/p4-netburst/cache_as_ram.S b/src/cpu/intel/car/p4-netburst/cache_as_ram.S index 9ac9e22..7187bc9 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -352,7 +352,7 @@ rep stosl /* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp /* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/romstage.c b/src/cpu/intel/car/romstage.c index 63ddd74..6b9a50f 100644 --- a/src/cpu/intel/car/romstage.c +++ b/src/cpu/intel/car/romstage.c @@ -15,6 +15,9 @@ #define DCACHE_RAM_ROMSTAGE_STACK_SIZE 0x2000 static struct postcar_frame early_mtrrs; +extern uint8_t *_stack; +extern uint8_t *_estack; +extern size_t _stack_size; static void romstage_main(void) { @@ -27,14 +30,14 @@ DCACHE_RAM_ROMSTAGE_STACK_SIZE); /* Size of unallocated CAR. */ - size = ALIGN_DOWN(_car_stack_size, 16); + size = ALIGN_DOWN(_stack_size, 16); size = MIN(size, stack_size); if (size < stack_size) printk(BIOS_DEBUG, "Romstage stack size limited to 0x%x!\n", size); - stack_base = (u32 *) (_ecar_stack - size); + stack_base = (u32 *) (_stack - size); for (i = 0; i < num_guards; i++) stack_base[i] = stack_guard; diff --git a/src/cpu/qemu-x86/cache_as_ram_bootblock.S b/src/cpu/qemu-x86/cache_as_ram_bootblock.S index f828d6f..38bea60 100644 --- a/src/cpu/qemu-x86/cache_as_ram_bootblock.S +++ b/src/cpu/qemu-x86/cache_as_ram_bootblock.S @@ -74,7 +74,7 @@ pages_done: #endif - movl $_ecar_stack, %esp + movl $_estack, %esp /* Align the stack and keep aligned for call to bootblock_c_entry() */ and $0xfffffff0, %esp diff --git a/src/cpu/x86/entry32.S b/src/cpu/x86/entry32.S index 215d601..78552db 100644 --- a/src/cpu/x86/entry32.S +++ b/src/cpu/x86/entry32.S @@ -15,6 +15,19 @@ #include <cpu/x86/cr.h> #include <cpu/x86/post_code.h> +/* Place the stack in the bss section. It's not necessary to define it in the + * the linker script. */ + .section .bss, "aw", @nobits +.global _stack +.global _estack +.global _stack_size + +.align 8 +_stack: +.space CONFIG_DCACHE_BSP_STACK_SIZE +_estack: +.set _stack_size, _estack - _stack + .section .init, "ax", @progbits .code32 diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 0d678d1..0a66012 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -38,14 +38,14 @@ * Set up bootblock stack on BSP. * AMD_ENABLE_STACK macro sets up a stack for BSP at BSP_STACK_BASE_ADDR * which is 0x30000 (_car_region_end), but for C bootblock the stack - * begins at _ecar_stack (see arch/x86/car.ld) + * begins at _estack (in .bss) */ mov $LAPIC_BASE_MSR, %ecx rdmsr test $LAPIC_BASE_MSR_BOOTSTRAP_PROCESSOR, %eax jz ap_entry - mov $_ecar_stack, %esp + mov $_estack, %esp /* Align the stack and keep aligned for call to bootblock_c_entry() */ and $0xfffffff0, %esp diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index e20d527..b2bb62d 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -135,7 +135,7 @@ jne halt2 /* Setup bootblock stack */ - movl $_ecar_stack, %esp + movl $_estack, %esp /* * ebp: FSP_INFO_HEADER address diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index cac7854..2443dc6 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -271,7 +271,7 @@ post_code(0x29) /* Setup bootblock stack */ - mov $_ecar_stack, %esp + mov $_estack, %esp /* Need to align stack to 16 bytes at call instruction. Account for the two pushes below. */ diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index 173ebf7..51ba0cb 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -64,7 +64,7 @@ jnz .halt_forever /* Setup bootblock stack */ - movl $_ecar_stack, %esp + movl $_estack, %esp /* * temp_memory_start/end reside in the .bss section, which gets cleared * below. Save the FSP return value to the stack before writing those diff --git a/src/soc/intel/quark/bootblock/esram_init.S b/src/soc/intel/quark/bootblock/esram_init.S index 39ce7bd..b146f26 100644 --- a/src/soc/intel/quark/bootblock/esram_init.S +++ b/src/soc/intel/quark/bootblock/esram_init.S @@ -476,7 +476,7 @@ */ /* Setup bootblock stack */ - movl $_ecar_stack, %esp + movl $_estack, %esp before_carstage: post_code(0x2b) diff --git a/src/soc/intel/quark/romstage/fsp_params.c b/src/soc/intel/quark/romstage/fsp_params.c index 11f7059..2ffe92f 100644 --- a/src/soc/intel/quark/romstage/fsp_params.c +++ b/src/soc/intel/quark/romstage/fsp_params.c @@ -12,6 +12,7 @@ #include <soc/romstage.h> #include <soc/reg_access.h> #include <soc/storage_test.h> +#include <stdint.h> void mainboard_romstage_entry(void) { @@ -103,8 +104,9 @@ printk(BIOS_SPEW, "+-------------------+ %p\n", _car_unallocated_start); printk(BIOS_SPEW, "| coreboot data |\n"); + extern uint8_t *_estack; printk(BIOS_SPEW, "+-------------------+ %p\n", - _ecar_stack); + _estack); printk(BIOS_SPEW, "| coreboot stack |\n"); printk(BIOS_SPEW, "+-------------------+ 0x80000000 - ESRAM start\n\n"); -- To view, visit https://review.coreboot.org/c/coreboot/+/59508 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7504f78b1dc214f51caa85d0fbff5daf70251a31 Gerrit-Change-Number: 59508 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: src/arch/ppc64/*: pass FDT address to payload
by Arthur Heymans (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Sergii Dmytruk, Timothy Pearson, Ron Minnich. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57084 ) Change subject: src/arch/ppc64/*: pass FDT address to payload ...................................................................... Patch Set 16: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/57084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0911f4b534c6f8cacfa057a5bad7576fec711637 Gerrit-Change-Number: 57084 Gerrit-PatchSet: 16 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Ron Minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Timothy Pearson <tpearson(a)raptorengineering.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Timothy Pearson <tpearson(a)raptorengineering.com> Gerrit-Attention: Ron Minnich <rminnich(a)gmail.com> Gerrit-Comment-Date: Sat, 20 Nov 2021 15:04:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/thermal: Allow thermal configuration over PMC
by EricR Lai (Code Review) 20 Nov '21

20 Nov '21

Attention is currently required from: Maulik V Vaghela, Subrata Banik, Patrick Rudolph. EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59209 ) Change subject: soc/intel/common/thermal: Allow thermal configuration over PMC ...................................................................... Patch Set 13: Code-Review+2 (2 comments) Patchset: PS13: LGTM 👍 File src/soc/intel/common/block/thermal/thermal_pmc.c: https://review.coreboot.org/c/coreboot/+/59209/comment/55fe573e_5d221979 PS13, Line 21: static uint8_t get_thermal_trip_temp(void) > Yes Eric, I will address this in follow up CL, hope that is okay with you. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/59209 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0c6ae72610da39fc18ff252c440d006e83c570a0 Gerrit-Change-Number: 59209 Gerrit-PatchSet: 13 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sat, 20 Nov 2021 14:55:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Comment-In-Reply-To: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021