coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: [WIP] [DO NOT MERGE] src/drivers/intel/fsp2_0: Add FSP 2.3 support
by build bot (Jenkins) (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Andrey Petrov, Patrick Rudolph. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59324 ) Change subject: [WIP] [DO NOT MERGE] src/drivers/intel/fsp2_0: Add FSP 2.3 support ...................................................................... Patch Set 1: (2 comments) File src/drivers/intel/fsp2_0/header_display.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-133357): https://review.coreboot.org/c/coreboot/+/59324/comment/149f9366_edb376cb PS1, Line 23: ((ext_revision.rev << 8) | revision.rev.revision), line over 96 characters Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-133357): https://review.coreboot.org/c/coreboot/+/59324/comment/a98c1021_acc37f8f PS1, Line 24: ((ext_revision.bld_num << 8) | revision.rev.bld_num)); line over 96 characters -- To view, visit https://review.coreboot.org/c/coreboot/+/59324 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica1bd004286c785aa8a431f39d8efc69982874c1 Gerrit-Change-Number: 59324 Gerrit-PatchSet: 1 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 16 Nov 2021 00:12:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib/thread,soc/amd/common: Delete thread_mutex
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Hello Jason Glenesk, Marshall Dawson, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59323 to look at the new patch set (#5). Change subject: lib/thread,soc/amd/common: Delete thread_mutex ...................................................................... lib/thread,soc/amd/common: Delete thread_mutex The functionality has been moved into `struct mutex`. BUG=b:179699789 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I9c6d00a72a37bcc3f9f772c96b61003c280c6b19 --- M src/include/thread.h M src/lib/thread.c M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/lpc/spi_dma.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/common/block/spi/fch_spi_util.c 6 files changed, 13 insertions(+), 40 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/59323/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/59323 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9c6d00a72a37bcc3f9f772c96b61003c280c6b19 Gerrit-Change-Number: 59323 Gerrit-PatchSet: 5 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: arch/x86: Remove spinlock dependency on threading.
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Raul Rangel has uploaded a new patch set (#4). ( https://review.coreboot.org/c/coreboot/+/59322 ) Change subject: arch/x86: Remove spinlock dependency on threading. ...................................................................... arch/x86: Remove spinlock dependency on threading. Since we only have one spinlock caller left, we can move the thread_coop_ calls out of the spinlock. This is mostly a revert of CB:56320. BUG=b:179699789 TEST=Boot guybrush to OS with threading enabled Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I55d0cd2bcd82721098ca24ae2368bb6a16a07df4 --- M src/arch/x86/include/arch/smp/spinlock.h M src/console/printk.c 2 files changed, 8 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/59322/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/59322 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I55d0cd2bcd82721098ca24ae2368bb6a16a07df4 Gerrit-Change-Number: 59322 Gerrit-PatchSet: 4 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: [WIP] [DO NOT MERGE] src/drivers/intel/fsp2_0: Add FSP 2.3 support
by Anil Kumar K (Code Review) 16 Nov '21

16 Nov '21

Anil Kumar K has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59324 ) Change subject: [WIP] [DO NOT MERGE] src/drivers/intel/fsp2_0: Add FSP 2.3 support ...................................................................... [WIP] [DO NOT MERGE] src/drivers/intel/fsp2_0: Add FSP 2.3 support Signed-off-by: Anil Kumar <anil.kumar.k(a)intel.com> Change-Id: Ica1bd004286c785aa8a431f39d8efc69982874c1 --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/util.c 5 files changed, 37 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/59324/1 diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index a0e02a8..b4caa12 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -29,6 +29,20 @@ 3. Added EnableMultiPhaseSiliconInit, bootloaders designed for FSP2.0/2.1 can disable the FspMultiPhaseSiInit() API and continue to use FspSiliconInit() without change. +config PLATFORM_USES_FSP2_3 + bool + default n + select PLATFORM_USES_FSP2_2 + help + Include FSP 2.3 wrappers and functionality. + Features added into FSP 2.3 specification that impact coreboot are: + 1. FSP_INFO_HEADER changes + -Updated SpecVersion from 0x22 to 0x23 + -Updated HeaderRevision from 5 to 6 + -Added ExtendedImageRevision + -FSP_INFO_HEADER length changed to 0x50 + 2. Added FSP_NON_VOLATILE_STORAGE_HOB2 + if PLATFORM_USES_FSP2_0 config PLATFORM_USES_FSP2_X86_32 diff --git a/src/drivers/intel/fsp2_0/header_display.c b/src/drivers/intel/fsp2_0/header_display.c index 4f93666..9213699 100644 --- a/src/drivers/intel/fsp2_0/header_display.c +++ b/src/drivers/intel/fsp2_0/header_display.c @@ -6,16 +6,22 @@ void fsp_print_header_info(const struct fsp_header *hdr) { union fsp_revision revision; + union extended_fsp_revision ext_revision; + ext_revision.val = 0; + + /* For FSP 2.3 and later use externded image revision field present in header + * for build number and revision calculation */ + if (hdr->revision >= 6) + ext_revision.val = hdr->extended_fsp_revision; revision.val = hdr->fsp_revision; - printk(BIOS_SPEW, "Spec version: v%u.%u\n", (hdr->spec_version >> 4), hdr->spec_version & 0xf); printk(BIOS_SPEW, "Revision: %u.%u.%u, Build Number %u\n", revision.rev.major, revision.rev.minor, - revision.rev.revision, - revision.rev.bld_num); + ((ext_revision.rev << 8) | revision.rev.revision), + ((ext_revision.bld_num << 8) | revision.rev.bld_num)); printk(BIOS_SPEW, "Type: %s/%s\n", (hdr->component_attribute & 1) ? "release" : "debug", (hdr->component_attribute & 2) ? "official" : "test"); diff --git a/src/drivers/intel/fsp2_0/include/fsp/info_header.h b/src/drivers/intel/fsp2_0/include/fsp/info_header.h index 5b6318c..136fc2b 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/info_header.h +++ b/src/drivers/intel/fsp2_0/include/fsp/info_header.h @@ -14,6 +14,7 @@ #if CONFIG(PLATFORM_USES_FSP2_X86_32) struct fsp_header { uint32_t fsp_revision; + uint16_t extended_fsp_revision; uint32_t image_size; uint32_t image_base; uint16_t image_attribute; diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h index d05b644..7347034 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/util.h +++ b/src/drivers/intel/fsp2_0/include/fsp/util.h @@ -60,6 +60,14 @@ } rev; }; +union extended_fsp_revision { + uint16_t val; + struct { + uint8_t bld_num; + uint8_t revision; + } rev; +}; + #if CONFIG_UDK_VERSION < CONFIG_UDK_2017_VERSION enum resource_type { EFI_RESOURCE_SYSTEM_MEMORY = 0, diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c index 49934fe..ff79fc7 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -14,7 +14,9 @@ static uint32_t fsp_hdr_get_expected_min_length(void) { - if (CONFIG(PLATFORM_USES_FSP2_2)) + if (CONFIG(PLATFORM_USES_FSP2_3)) + return 80; + else if (CONFIG(PLATFORM_USES_FSP2_2)) return 76; else if (CONFIG(PLATFORM_USES_FSP2_1)) return 72; @@ -70,6 +72,8 @@ hdr->silicon_init_entry_offset = read32(raw_hdr + 68); if (CONFIG(PLATFORM_USES_FSP2_2)) hdr->multi_phase_si_init_entry_offset = read32(raw_hdr + 72); + if (CONFIG(PLATFORM_USES_FSP2_3)) + hdr->extended_fsp_revision = read16(raw_hdr + 76); return CB_SUCCESS; } -- To view, visit https://review.coreboot.org/c/coreboot/+/59324 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica1bd004286c785aa8a431f39d8efc69982874c1 Gerrit-Change-Number: 59324 Gerrit-PatchSet: 1 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: arch/x86: Remove spinlock dependency on threading.
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Raul Rangel has uploaded a new patch set (#3). ( https://review.coreboot.org/c/coreboot/+/59322 ) Change subject: arch/x86: Remove spinlock dependency on threading. ...................................................................... arch/x86: Remove spinlock dependency on threading. Since we only have one spinlock caller left, we can move the thread_coop_ calls out of the spinlock. This is mostly a revert of CB:56320. BUG=b:179699789 TEST=Boot guybrush to OS with threading enabled Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I55d0cd2bcd82721098ca24ae2368bb6a16a07df4 --- M src/arch/x86/include/arch/smp/spinlock.h M src/console/printk.c 2 files changed, 9 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/59322/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/59322 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I55d0cd2bcd82721098ca24ae2368bb6a16a07df4 Gerrit-Change-Number: 59322 Gerrit-PatchSet: 3 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/../thermal: Add support for thermal config behind PMC device
by Tim Wawrzynczak (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Maulik V Vaghela, Subrata Banik, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59209 ) Change subject: soc/intel/../thermal: Add support for thermal config behind PMC device ...................................................................... Patch Set 8: (3 comments) File src/soc/intel/common/block/thermal/thermal.c: https://review.coreboot.org/c/coreboot/+/59209/comment/443102ab_5ed4d8e5 PS8, Line 57: static void thermal_or32(uintptr_t addr, uint32_t ordata) : { : uint32_t data32; : : data32 = read32p(addr); : data32 |= ordata; : write32p(addr, data32); : } `setbits32` ? https://review.coreboot.org/c/coreboot/+/59209/comment/24286a3e_163eceb3 PS8, Line 112: struct pmc_thermal_config { : void (*func)(uintptr_t addr, uint32_t data); : uint16_t offset; : uint32_t value; : } config[] = { : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_CTEN, : .value = PMC_PWRM_THERMAL_CTEN_CPDEN | PMC_PWRM_THERMAL_CTEN_CTENLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_ECRPTEN, : .value = PMC_PWRM_THERMAL_ECRPTEN_EN_RPT : | PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK, : }, : { : .func = write32p, : .offset = PMC_PWRM_THERMAL_TL, : .value = pch_get_ltt_value(dev) | PMC_PWRM_THERMAL_TL_TTEN : | PMC_PWRM_THERMAL_TL_TLLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_PHLC, : .value = PMC_PWRM_THERMAL_PHLC_PHLCLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_TLEN, : .value = PMC_PWRM_THERMAL_TLEN_TLENLOCK, : }, : }; Is this really necessary for ~10 10 lines of code ? ``` setbits32(pmc_bar + PMC_PWRM_THERMAL_CTEN, PMC_PWRM_THERMAL_CTEN_CPDEN | PMC_PWRM_THERMAL_CTEN_CTENLOCK); setbits32(pmc_bar + PMC_PWRM_THERMAL_ECRPTEN, PMC_PWRM_THERMAL_ECRPTEN_EN_RPT | PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK); ``` etc. ? Also should any others of these settings be configurable, e.g. EC thermal reporting? https://review.coreboot.org/c/coreboot/+/59209/comment/0dd56b87_162f942e PS8, Line 106: /* Enable thermal sensor power management using PMC PCH device */ : static void pch_pmc_thermal_configuration(void) : { : struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT); : uintptr_t pmc_bar = soc_read_pmc_base(); : : struct pmc_thermal_config { : void (*func)(uintptr_t addr, uint32_t data); : uint16_t offset; : uint32_t value; : } config[] = { : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_CTEN, : .value = PMC_PWRM_THERMAL_CTEN_CPDEN | PMC_PWRM_THERMAL_CTEN_CTENLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_ECRPTEN, : .value = PMC_PWRM_THERMAL_ECRPTEN_EN_RPT : | PMC_PWRM_THERMAL_ECRPTEN_ECRPTENLOCK, : }, : { : .func = write32p, : .offset = PMC_PWRM_THERMAL_TL, : .value = pch_get_ltt_value(dev) | PMC_PWRM_THERMAL_TL_TTEN : | PMC_PWRM_THERMAL_TL_TLLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_PHLC, : .value = PMC_PWRM_THERMAL_PHLC_PHLCLOCK, : }, : { : .func = thermal_or32, : .offset = PMC_PWRM_THERMAL_TLEN, : .value = PMC_PWRM_THERMAL_TLEN_TLENLOCK, : }, : }; : : for (int i = 0; i < ARRAY_SIZE(config); i++) : config[i].func(pmc_bar + config[i].offset, config[i].value); : } : just thinking out loud, should this go in src/soc/intel/common/block/pmclib.c? then alderlake & tigerlake can just call it from their `soc_pmc_init()` function? Since there is no separate thermal device anymore? WDYT? -- 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: 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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Mon, 15 Nov 2021 23:52:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib: Add a mutex
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Raul Rangel has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/59320 ) Change subject: lib: Add a mutex ...................................................................... lib: Add a mutex We currently have two synchronization primatives, spinlock and thread_mutex. spinlock is meant to block multiple CPUs from entering a critical section. thread_mutex is meant to block multiple coop-threads from entering a critical section. It is not AP aware at all. This CL introduces a mutex that can handle both concepts. The implementation is using the GCC/LLVM atomic builtin functions. The generated code uses the xchg instruction vs spinlock which uses (lock) decb. 8: b0 01 mov $0x1,%al a: 86 03 xchg %al,(%ebx) c: 84 c0 test %al,%al BUG=b:179699789 TEST=Boot guybrush to OS Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I41e02a54a17b1f6513b36a0274e43fc715472d78 --- A src/include/mutex.h M src/lib/Makefile.inc A src/lib/mutex.c 3 files changed, 75 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/59320/2 -- 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: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: lib/thread,soc/amd/common: Delete thread_mutex
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59323 ) Change subject: lib/thread,soc/amd/common: Delete thread_mutex ...................................................................... lib/thread,soc/amd/common: Delete thread_mutex The functionality has been moved into `struct mutex`. BUG=b:179699789 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I9c6d00a72a37bcc3f9f772c96b61003c280c6b19 --- M src/include/thread.h M src/lib/thread.c M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/lpc/spi_dma.c M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/common/block/spi/fch_spi_util.c 6 files changed, 13 insertions(+), 40 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/59323/1 diff --git a/src/include/thread.h b/src/include/thread.h index 19b69fa..74be485 100644 --- a/src/include/thread.h +++ b/src/include/thread.h @@ -5,12 +5,9 @@ #include <arch/cpu.h> #include <bootstate.h> #include <commonlib/bsd/cb_err.h> +#include <mutex.h> #include <types.h> -struct thread_mutex { - bool locked; -}; - enum thread_state { THREAD_UNINITIALIZED, THREAD_STARTED, @@ -70,9 +67,6 @@ void thread_coop_enable(void); void thread_coop_disable(void); -void thread_mutex_lock(struct thread_mutex *mutex); -void thread_mutex_unlock(struct thread_mutex *mutex); - /* Architecture specific thread functions. */ asmlinkage void switch_to_thread(uintptr_t new_stack, uintptr_t *saved_stack); /* Set up the stack frame for a new thread so that a switch_to_thread() call @@ -91,10 +85,6 @@ } static inline void thread_coop_enable(void) {} static inline void thread_coop_disable(void) {} - -static inline void thread_mutex_lock(struct thread_mutex *mutex) {} - -static inline void thread_mutex_unlock(struct thread_mutex *mutex) {} #endif #endif /* THREAD_H_ */ diff --git a/src/lib/thread.c b/src/lib/thread.c index 0393ca7..87a7d1b 100644 --- a/src/lib/thread.c +++ b/src/lib/thread.c @@ -409,22 +409,3 @@ return handle->error; } - -void thread_mutex_lock(struct thread_mutex *mutex) -{ - struct stopwatch sw; - - stopwatch_init(&sw); - - while (mutex->locked) - assert(thread_yield() == 0); - mutex->locked = true; - - printk(BIOS_SPEW, "took %lu us to acquire mutex\n", stopwatch_duration_usecs(&sw)); -} - -void thread_mutex_unlock(struct thread_mutex *mutex) -{ - assert(mutex->locked); - mutex->locked = 0; -} diff --git a/src/soc/amd/common/block/include/amdblocks/spi.h b/src/soc/amd/common/block/include/amdblocks/spi.h index 81da5dd..6fa2637 100644 --- a/src/soc/amd/common/block/include/amdblocks/spi.h +++ b/src/soc/amd/common/block/include/amdblocks/spi.h @@ -3,7 +3,7 @@ #ifndef AMD_BLOCK_SPI_H #define AMD_BLOCK_SPI_H -#include <thread.h> +#include <mutex.h> #include <types.h> #define SPI_CNTRL0 0x00 @@ -121,6 +121,6 @@ void mainboard_spi_fast_speed_override(uint8_t *fast_speed); /* Ensure you hold the mutex when performing SPI transactions */ -extern struct thread_mutex spi_hw_mutex; +extern struct mutex spi_hw_mutex; #endif /* AMD_BLOCK_SPI_H */ diff --git a/src/soc/amd/common/block/lpc/spi_dma.c b/src/soc/amd/common/block/lpc/spi_dma.c index baf1eb8..b998989 100644 --- a/src/soc/amd/common/block/lpc/spi_dma.c +++ b/src/soc/amd/common/block/lpc/spi_dma.c @@ -14,6 +14,7 @@ #include <spi_flash.h> #include <string.h> #include <thread.h> +#include <mutex.h> #include <types.h> /* The ROM is memory mapped just below 4GiB. Form a pointer for the base. */ @@ -126,7 +127,7 @@ * Ensure we have exclusive access to the SPI controller before starting the LPC SPI DMA * transaction. */ - thread_mutex_lock(&spi_hw_mutex); + mutex_lock(&spi_hw_mutex); pci_write_config32(SOC_LPC_DEV, LPC_ROM_DMA_EC_HOST_CONTROL, ctrl); } @@ -145,7 +146,7 @@ * Unlock the SPI mutex between DMA transactions to allow other users of the SPI * controller to interleave their transactions. */ - thread_mutex_unlock(&spi_hw_mutex); + mutex_unlock(&spi_hw_mutex); if (spi_dma_has_error()) { printk(BIOS_ERR, @@ -177,7 +178,7 @@ return false; } -static struct thread_mutex spi_dma_hw_mutex; +static struct mutex spi_dma_hw_mutex; static ssize_t spi_dma_readat_dma(const struct region_device *rd, void *destination, size_t source, size_t size) @@ -192,7 +193,7 @@ printk(BIOS_SPEW, "%s: start: dest: %p, source: %#zx, size: %zu\n", __func__, destination, source, size); - thread_mutex_lock(&spi_dma_hw_mutex); + mutex_lock(&spi_dma_hw_mutex); start_spi_dma_transaction(&transaction); @@ -200,7 +201,7 @@ udelay(2); } while (continue_spi_dma_transaction(rd, &transaction)); - thread_mutex_unlock(&spi_dma_hw_mutex); + mutex_unlock(&spi_dma_hw_mutex); printk(BIOS_SPEW, "%s: end: dest: %p, source: %#zx, remaining: %zu\n", __func__, destination, source, transaction.remaining); diff --git a/src/soc/amd/common/block/spi/fch_spi_ctrl.c b/src/soc/amd/common/block/spi/fch_spi_ctrl.c index d95b642..62456e3 100644 --- a/src/soc/amd/common/block/spi/fch_spi_ctrl.c +++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c @@ -144,9 +144,9 @@ { int rc; - thread_mutex_lock(&spi_hw_mutex); + mutex_lock(&spi_hw_mutex); rc = spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer); - thread_mutex_unlock(&spi_hw_mutex); + mutex_unlock(&spi_hw_mutex); return rc; } diff --git a/src/soc/amd/common/block/spi/fch_spi_util.c b/src/soc/amd/common/block/spi/fch_spi_util.c index 862962f..6f5920c 100644 --- a/src/soc/amd/common/block/spi/fch_spi_util.c +++ b/src/soc/amd/common/block/spi/fch_spi_util.c @@ -5,9 +5,10 @@ #include <arch/mmio.h> #include <assert.h> #include <stdint.h> +#include <mutex.h> /* Global SPI controller mutex */ -struct thread_mutex spi_hw_mutex; +struct mutex spi_hw_mutex; static uintptr_t spi_base; -- To view, visit https://review.coreboot.org/c/coreboot/+/59323 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9c6d00a72a37bcc3f9f772c96b61003c280c6b19 Gerrit-Change-Number: 59323 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: arch/x86: Remove spinlock dependency on threading.
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59322 ) Change subject: arch/x86: Remove spinlock dependency on threading. ...................................................................... arch/x86: Remove spinlock dependency on threading. Since we only have one spinlock caller left, we can move the thread_coop_ calls out of the spinlock. This is mostly a revert of CB:56320. BUG=b:179699789 TEST=Boot guybrush to OS with threading enabled Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I55d0cd2bcd82721098ca24ae2368bb6a16a07df4 --- M src/arch/x86/include/arch/smp/spinlock.h M src/console/printk.c 2 files changed, 9 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/59322/1 diff --git a/src/arch/x86/include/arch/smp/spinlock.h b/src/arch/x86/include/arch/smp/spinlock.h index c7008c1..f8fffdb 100644 --- a/src/arch/x86/include/arch/smp/spinlock.h +++ b/src/arch/x86/include/arch/smp/spinlock.h @@ -3,8 +3,6 @@ #ifndef ARCH_SMP_SPINLOCK_H #define ARCH_SMP_SPINLOCK_H -#include <thread.h> - /* * Your basic SMP spinlocks, allowing only a single CPU anywhere */ @@ -52,16 +50,10 @@ __asm__ __volatile__( spin_lock_string : "=m" (lock->lock) : : "memory"); - - /* Switching contexts while holding a spinlock will lead to deadlocks */ - thread_coop_disable(); - } static __always_inline void spin_unlock(spinlock_t *lock) { - thread_coop_enable(); - __asm__ __volatile__( spin_unlock_string : "=m" (lock->lock) : : "memory"); diff --git a/src/console/printk.c b/src/console/printk.c index 1ed39cb..99e0151 100644 --- a/src/console/printk.c +++ b/src/console/printk.c @@ -8,11 +8,12 @@ #include <console/console.h> #include <console/streams.h> #include <console/vtxprintf.h> -#include <smp/spinlock.h> #include <smp/node.h> +#include <smp/spinlock.h> +#include <thread.h> #include <timer.h> -DECLARE_SPIN_LOCK(console_lock) +DECLARE_SPIN_LOCK(console_lock); #define TRACK_CONSOLE_TIME (!ENV_SMM && CONFIG(HAVE_MONOTONIC_TIMER)) @@ -80,6 +81,10 @@ if (log_this < CONSOLE_LOG_FAST) return 0; + /* Context switching while holding a spin lock can lead to a dead lock. */ + thread_coop_disable(); + + /* We use a spinlock instead of a mutex to allow coop-threading to use printk. */ spin_lock(&console_lock); console_time_run(); @@ -95,6 +100,8 @@ spin_unlock(&console_lock); + thread_coop_enable(); + return i; } -- To view, visit https://review.coreboot.org/c/coreboot/+/59322 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I55d0cd2bcd82721098ca24ae2368bb6a16a07df4 Gerrit-Change-Number: 59322 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: treewide: Replace spinlock calls with mutex
by Raul Rangel (Code Review) 16 Nov '21

16 Nov '21

Attention is currently required from: Patrick Rudolph. Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59321 ) Change subject: treewide: Replace spinlock calls with mutex ...................................................................... treewide: Replace spinlock calls with mutex spinlock does not currently work nicely with coop-threads. If a `udelay` is called while in a critical section, it will context switch. If that second thread also tries to acquire the same lock then we deadlock since we never yield again. There is currently a hack in x86/spinlock.h where we disable coop threads when acquiring a spinlock. This hack only works for ramstage because in romstage we use the noop spinlock implementation. By replacing the spinlocks with mutex we no longer need to rely on the hack in the x86/spinlock.h. I didn't convert the printk spinlock into a mutex. Since printk is the primary debugging tool, I wanted to keep it decoupled from threading mutex. This continues to allow debugging the threading code with printks. BUG=b:179699789 TEST=Boot guybrush to OS Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: Ibee331a9ccd491e819c7f2c9a19bca7dc4379d9d --- M src/cpu/intel/microcode/microcode.c M src/cpu/x86/lapic/lapic_cpu_init.c M src/cpu/x86/mp_init.c M src/device/device.c M src/drivers/pc80/rtc/post.c 5 files changed, 28 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/59321/1 diff --git a/src/cpu/intel/microcode/microcode.c b/src/cpu/intel/microcode/microcode.c index 469bd25..ea0cd80 100644 --- a/src/cpu/intel/microcode/microcode.c +++ b/src/cpu/intel/microcode/microcode.c @@ -9,9 +9,9 @@ #include <console/console.h> #include <cpu/x86/msr.h> #include <cpu/intel/microcode.h> -#include <smp/spinlock.h> +#include <mutex.h> -DECLARE_SPIN_LOCK(microcode_lock) +struct mutex microcode_lock; struct microcode { u32 hdrver; /* Header Version */ @@ -243,11 +243,11 @@ { const void *patch = intel_microcode_find(); - spin_lock(&microcode_lock); + mutex_lock(&microcode_lock); intel_microcode_load_unlocked(patch); - spin_unlock(&microcode_lock); + mutex_unlock(&microcode_lock); } #if ENV_RAMSTAGE diff --git a/src/cpu/x86/lapic/lapic_cpu_init.c b/src/cpu/x86/lapic/lapic_cpu_init.c index c35888a..9af6519 100644 --- a/src/cpu/x86/lapic/lapic_cpu_init.c +++ b/src/cpu/x86/lapic/lapic_cpu_init.c @@ -13,9 +13,9 @@ #include <device/device.h> #include <device/path.h> #include <smp/atomic.h> -#include <smp/spinlock.h> #include <cpu/cpu.h> #include <cpu/intel/speedstep.h> +#include <mutex.h> #include <smp/node.h> #include <stdlib.h> #include <thread.h> @@ -215,7 +215,7 @@ * start_cpu returns. */ -DECLARE_SPIN_LOCK(start_cpu_lock); +struct mutex start_cpu_lock; static unsigned int last_cpu_index = 0; static void *stacks[CONFIG_MAX_CPUS]; volatile unsigned long secondary_stack; @@ -231,7 +231,7 @@ unsigned long count; int result; - spin_lock(&start_cpu_lock); + mutex_lock(&start_cpu_lock); /* Get the CPU's apicid */ apicid = cpu->path.apic.apic_id; @@ -277,7 +277,7 @@ } } secondary_stack = 0; - spin_unlock(&start_cpu_lock); + mutex_unlock(&start_cpu_lock); return result; } @@ -286,7 +286,7 @@ { atomic_inc(&active_cpus); - spin_lock(&start_cpu_lock); + mutex_lock(&start_cpu_lock); #ifdef __SSE3__ /* @@ -300,7 +300,7 @@ #endif cpu_initialize(index); - spin_unlock(&start_cpu_lock); + mutex_unlock(&start_cpu_lock); atomic_dec(&active_cpus); diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index c99732f..156b963 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -18,8 +18,8 @@ #include <delay.h> #include <device/device.h> #include <device/path.h> +#include <mutex.h> #include <smp/atomic.h> -#include <smp/spinlock.h> #include <symbols.h> #include <timer.h> #include <thread.h> @@ -659,14 +659,14 @@ printk(BIOS_DEBUG, "Relocation complete.\n"); } -DECLARE_SPIN_LOCK(smm_relocation_lock); +struct mutex smm_relocation_lock; /* Send SMI to self with single user serialization. */ void smm_initiate_relocation(void) { - spin_lock(&smm_relocation_lock); + mutex_lock(&smm_relocation_lock); smm_initiate_relocation_parallel(); - spin_unlock(&smm_relocation_lock); + mutex_unlock(&smm_relocation_lock); } struct mp_state { diff --git a/src/device/device.c b/src/device/device.c index 8337d55..e06e659 100644 --- a/src/device/device.c +++ b/src/device/device.c @@ -8,10 +8,10 @@ #include <device/device.h> #include <device/pci_def.h> #include <device/pci_ids.h> +#include <mutex.h> #include <post.h> #include <stdlib.h> #include <string.h> -#include <smp/spinlock.h> #if ENV_X86 #include <arch/ebda.h> #endif @@ -71,7 +71,7 @@ } } -DECLARE_SPIN_LOCK(dev_lock) +struct mutex dev_lock; /** * Allocate a new device structure. @@ -122,9 +122,9 @@ struct device *alloc_dev(struct bus *parent, struct device_path *path) { struct device *dev; - spin_lock(&dev_lock); + mutex_lock(&dev_lock); dev = __alloc_dev(parent, path); - spin_unlock(&dev_lock); + mutex_unlock(&dev_lock); return dev; } @@ -138,11 +138,11 @@ struct device *alloc_find_dev(struct bus *parent, struct device_path *path) { struct device *child; - spin_lock(&dev_lock); + mutex_lock(&dev_lock); child = find_dev_path(parent, path); if (!child) child = __alloc_dev(parent, path); - spin_unlock(&dev_lock); + mutex_unlock(&dev_lock); return child; } diff --git a/src/drivers/pc80/rtc/post.c b/src/drivers/pc80/rtc/post.c index aa8ea18..eff42ed 100644 --- a/src/drivers/pc80/rtc/post.c +++ b/src/drivers/pc80/rtc/post.c @@ -1,11 +1,11 @@ /* SPDX-License-Identifier: GPL-2.0-only */ #include <stdint.h> +#include <mutex.h> #include <post.h> #include <console/console.h> #include <device/device.h> #include <pc80/mc146818rtc.h> -#include <smp/spinlock.h> #if CONFIG(USE_OPTION_TABLE) # include "option_table.h" @@ -35,14 +35,14 @@ #define CMOS_POST_EXTRA_DEV_PATH 0x01 -DECLARE_SPIN_LOCK(cmos_post_lock) +struct mutex cmos_post_lock; int cmos_post_previous_boot(u8 *code, u32 *extra) { *code = 0; *extra = 0; - spin_lock(&cmos_post_lock); + mutex_lock(&cmos_post_lock); /* Get post code from other bank */ switch (cmos_read(CMOS_POST_BANK_OFFSET)) { @@ -56,7 +56,7 @@ break; } - spin_unlock(&cmos_post_lock); + mutex_unlock(&cmos_post_lock); /* Check last post code in previous boot against normal list */ switch (*code) { @@ -96,7 +96,7 @@ void cmos_post_code(u8 value) { - spin_lock(&cmos_post_lock); + mutex_lock(&cmos_post_lock); switch (cmos_read(CMOS_POST_BANK_OFFSET)) { case CMOS_POST_BANK_0_MAGIC: @@ -107,12 +107,12 @@ break; } - spin_unlock(&cmos_post_lock); + mutex_unlock(&cmos_post_lock); } void cmos_post_extra(u32 value) { - spin_lock(&cmos_post_lock); + mutex_lock(&cmos_post_lock); switch (cmos_read(CMOS_POST_BANK_OFFSET)) { case CMOS_POST_BANK_0_MAGIC: @@ -123,7 +123,7 @@ break; } - spin_unlock(&cmos_post_lock); + mutex_unlock(&cmos_post_lock); } void cmos_post_path(const struct device *dev) -- 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: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021