coreboot-gerrit July 2021

coreboot-gerrit@coreboot.org

1 participants
5824 discussions

Change in coreboot[master]: lib/thread: Add mutex
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Julius Werner, Eric Peers, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56230 ) Change subject: lib/thread: Add mutex ...................................................................... Patch Set 6: (4 comments) File src/include/thread.h: https://review.coreboot.org/c/coreboot/+/56230/comment/7d856fce_190386ea PS5, Line 88: mutex_lock > nit: I think it's better to keep everything here prefixed thread_ (e.g. […] Done File src/lib/thread.c: https://review.coreboot.org/c/coreboot/+/56230/comment/e220c9ac_9038adf3 PS5, Line 391: thread_yield_microseconds(0); > Should assert that this didn't fail (e.g. because can_yield is 0, because then you'll wait forever). Good point. https://review.coreboot.org/c/coreboot/+/56230/comment/a7fc040d_ecb1d6fc PS5, Line 392: 1 > Please only use `true` and `false` with bools, and only numbers with integers. Good catch. https://review.coreboot.org/c/coreboot/+/56230/comment/5eddd6a7_4e26dc94 PS5, Line 394: printk(BIOS_DEBUG, " took %lu us to acquire mutex\n", stopwatch_duration_usecs(&sw)); Made it SPEW. I found it useful to figure out how long things were blocked. It is usually a sign that nothing has yielded in a while: > APOB thread running > spi_dma_readat_dma: start: dest: 0xcb7aa640, source: 0x0, size: 65536 > took 0 us to acquire mutex > took 209446 us to acquire mutex > start_spi_dma_transaction: dest: 0x021c0000, source: 0x7f540, remaining: 174994 As for the space, I've removed it. -- To view, visit https://review.coreboot.org/c/coreboot/+/56230 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ife1ac95ec064ebcdd00fcaacec37a06ac52885ff Gerrit-Change-Number: 56230 Gerrit-PatchSet: 6 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 15 Jul 2021 21:56:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib/thread: Add mutex
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Julius Werner, Eric Peers, Felix Held. Hello build bot (Jenkins), Julius Werner, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56230 to look at the new patch set (#6). Change subject: lib/thread: Add mutex ...................................................................... lib/thread: Add mutex We need a way to protect shared resources. Since we are using cooperative multitasking the mutex implementation is pretty trivial. The delays are implemented using a timer queue. So the task with the next expiration date will always be at the beginning of the queue. If a task (A) wishes to yield control to the next task (B) that is holding onto the mutex, without encountering a delay of its own (i.e., udelay(0)), then we need to ensure the next task (B) is at the head of the queue. We do this by using a `0 us` delay in mutex_lock. If we were to use anything larger than a `0 us` delay, a `udelay(0)` by the task (A) that just unlocked the mutex will most likely not schedule the next task (B) that was holding onto the mutex. BUG=b:179699789 TEST=Verify thread lock and unlock. Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: Ife1ac95ec064ebcdd00fcaacec37a06ac52885ff --- M src/include/thread.h M src/lib/thread.c 2 files changed, 31 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/56230/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/56230 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ife1ac95ec064ebcdd00fcaacec37a06ac52885ff Gerrit-Change-Number: 56230 Gerrit-PatchSet: 6 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: x86/smp/spinlock: Disable thread coop when taking spinlock
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Furquan Shaikh, Julius Werner. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56320 ) Change subject: x86/smp/spinlock: Disable thread coop when taking spinlock ...................................................................... Patch Set 2: (1 comment) File src/arch/x86/include/arch/smp/spinlock.h: https://review.coreboot.org/c/coreboot/+/56320/comment/1c6fe351_9a7cafd6 PS1, Line 61: lock->coop = thread_coop_enabled(); > Oh... hmm... yeah okay, that's a fair point. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/56320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1b929070b7f175965d4f37be693462fef26be052 Gerrit-Change-Number: 56320 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Thu, 15 Jul 2021 21:56:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: x86/smp/spinlock: Disable thread coop when taking spinlock
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Furquan Shaikh, Julius Werner. Hello build bot (Jenkins), Furquan Shaikh, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56320 to look at the new patch set (#2). Change subject: x86/smp/spinlock: Disable thread coop when taking spinlock ...................................................................... x86/smp/spinlock: Disable thread coop when taking spinlock Switching threads while holding a spinlock can lead to a deadlock. This happens if you have two thread trying to print to the serial console because the uart code uses udelay. BUG=b:179699789 TEST=Boot guybrush and no longer see a deadlock when printing to console from a second thread. Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I1b929070b7f175965d4f37be693462fef26be052 --- M src/arch/x86/include/arch/smp/spinlock.h 1 file changed, 8 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/56320/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/56320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1b929070b7f175965d4f37be693462fef26be052 Gerrit-Change-Number: 56320 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionality
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Eric Peers, Karthik Ramasubramanian, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56228 ) Change subject: soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionality ...................................................................... Patch Set 6: (10 comments) File src/soc/amd/common/block/lpc/spi_dma.c: https://review.coreboot.org/c/coreboot/+/56228/comment/36466ccc_188ed1f7 PS3, Line 103: ctrl |= LPC_ROM_DMA_CTRL_ERROR; /* Clear error */ > I was using the stoney BKDG since it provides more details: […] Ack https://review.coreboot.org/c/coreboot/+/56228/comment/2bc879b7_9e7e8963 PS3, Line 209: val |= BIT(6); > Yeah this is the magic bit: b/179699789#comment11 has the explanation. […] Ack File src/soc/amd/common/block/lpc/spi_dma.c: https://review.coreboot.org/c/coreboot/+/56228/comment/f3b4d8ce_d6871220 PS5, Line 134: if (spi_dma_has_error()) > Print out an error in this case so that it is clear at what address the error occurred? Also, should […] Done. I didn't clear the error since we clear it before we start a new transaction. https://review.coreboot.org/c/coreboot/+/56228/comment/3718dd62_ae9302cc PS5, Line 144: else > nit: else is not required as the if block above returns. Done https://review.coreboot.org/c/coreboot/+/56228/comment/4f24e2d1_03b88f56 PS5, Line 173: udelay(10); This is what I replied to Eric: > It was just an arbitrary number. Doing very rough some math (not taking into account the transaction setup that happens every 64 bytes): > > 250 bytes / 10us @ 100 MHz (2-2-1) > 166.65 bytes / 10us @ 66.6 MHz (2-2-1) > 83.325 bytes / 10us @ 33.33 MHz (2-2-1) > > The min block size is 64 bytes. From what I've seen we normally do 1k - 64k transactions, so I think 10us should be fine. Thinking about it some more, there is no real downside to making this smaller. I'll make it 2us. That's 50 bytes / 2 us @ 100 MHz. https://review.coreboot.org/c/coreboot/+/56228/comment/5a2b421a_8d26049a PS5, Line 177: ); > Would it be helpful to also dump `stopwatch_duration_usecs()` to see how long transactions are takin […] Since we are yielding control for an unknown amount of time, there is no easy way to know how long the transaction took. If we get lucky, this is the only thread running, and it would show a pretty accurate value. But if there are multiple threads, there is no telling how long before they yield. There is no hardware timestamp that keeps track either. https://review.coreboot.org/c/coreboot/+/56228/comment/0439fcf5_d1e1380e PS5, Line 179: transaction.size - transaction.remaining > rdev API expects the function to return < 0 in case of any error. I am guessing if transaction. […] Ah good catch. I should have read the documentation on it. I saw code doing the size != expected check and just assumed... https://review.coreboot.org/c/coreboot/+/56228/comment/ae712466_5e3adea7 PS5, Line 185: assert(offset + size < CONFIG_ROM_SIZE); : assert(size); > Do we really need these checks? rdev API already takes care of this as part of `normalize_and_ok()` Nope, just left overs from me developing the driver. https://review.coreboot.org/c/coreboot/+/56228/comment/80ba8ef8_d51072a3 PS5, Line 188: size < LPC_ROM_DMA_MIN_ALIGNMENT > nit: Should this check be moved to can_use_dma() so that all attributes are checked in one place? Done https://review.coreboot.org/c/coreboot/+/56228/comment/a91e6413_ba66601a PS5, Line 222: /* Internal only registers: RN/CZN+ */ > If this is applicable only to certain platforms, I think it would be best to use a Kconfig that enab […] Updated the documentation. -- To view, visit https://review.coreboot.org/c/coreboot/+/56228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0be555956581fd82bbe1482d8afa8828c61aaa01 Gerrit-Change-Number: 56228 Gerrit-PatchSet: 6 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: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 15 Jul 2021 21:56:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionality
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Eric Peers, Karthik Ramasubramanian, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Karthik Ramasubramanian, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56228 to look at the new patch set (#6). Change subject: soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionality ...................................................................... soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionality This change will make it so the standard rdev readat call will use the SPI DMA controller if the alignment is correct, and the transfer size is larger than 64 bytes. There is a magic bit that needs to be set for the SPI DMA controller to function correctly. This is only available in RN/CZN+. BUG=b:179699789 TEST=Boot guybrush to OS. This reduces loading verstage by 40ms, verifying RW by 500us and loading romstage by 500 us. Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I0be555956581fd82bbe1482d8afa8828c61aaa01 --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/common/block/lpc/Kconfig M src/soc/amd/common/block/lpc/spi_dma.c 3 files changed, 203 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/56228/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/56228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0be555956581fd82bbe1482d8afa8828c61aaa01 Gerrit-Change-Number: 56228 Gerrit-PatchSet: 6 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: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: lib/thread: Verify threads are initialized before yielding
by Raul Rangel (Code Review) 15 Jul '21

15 Jul '21

Raul Rangel has submitted this change. ( https://review.coreboot.org/c/coreboot/+/56318 ) Change subject: lib/thread: Verify threads are initialized before yielding ...................................................................... lib/thread: Verify threads are initialized before yielding In hardwaremain.c we call console_init before threads_initialize. Part of setting up the uart requires calling udelay which then calls thread_yield_microseconds. Since threads have not been set up, trying to yield will result in bad things happening. This change guards the thread methods by making current_thread return NULL if the structures have not been initialized. BUG=b:179699789 TEST=Ramstage no longer hangs with serial enabled Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: If9e1eedfaebe584901d2937c8aa24e158706fa43 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56318 Reviewed-by: Krystian Hebel <krystian.hebel(a)3mdeb.com> Reviewed-by: Julius Werner <jwerner(a)chromium.org> Reviewed-by: Karthik Ramasubramanian <kramasub(a)google.com> Reviewed-by: Furquan Shaikh <furquan(a)google.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/lib/thread.c 1 file changed, 7 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Julius Werner: Looks good to me, approved Krystian Hebel: Looks good to me, but someone else must approve Karthik Ramasubramanian: Looks good to me, approved diff --git a/src/lib/thread.c b/src/lib/thread.c index e2280c6..782a63e 100644 --- a/src/lib/thread.c +++ b/src/lib/thread.c @@ -9,6 +9,8 @@ #include <thread.h> #include <timer.h> +static bool initialized; + static void idle_thread_init(void); /* There needs to be at least one thread to run the ramstate state machine. */ @@ -40,6 +42,9 @@ static inline struct thread *current_thread(void) { + if (!initialized) + return NULL; + return cpu_info_to_thread(cpu_info()); } @@ -265,6 +270,8 @@ free_thread(t); } + initialized = 1; + idle_thread_init(); } -- To view, visit https://review.coreboot.org/c/coreboot/+/56318 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If9e1eedfaebe584901d2937c8aa24e158706fa43 Gerrit-Change-Number: 56318 Gerrit-PatchSet: 3 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Raul Rangel <rrangel(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/amd/cezanne: add ACPI CPPC support for AMD
by Felix Held (Code Review) 15 Jul '21

15 Jul '21

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/56188 ) Change subject: soc/amd/cezanne: add ACPI CPPC support for AMD ...................................................................... soc/amd/cezanne: add ACPI CPPC support for AMD This leverages the existing Collaborative Processor Performance Control (CPPC) support and adds CPPC init for AMD/Cezanne. BUG=b:185814875 TEST=under Linux/ChromeOS, acpidump ssdt2, find expected CPPC entries Signed-off-by: Julian Schroeder <julianmarcusschroeder(a)gmail.com> Change-Id: I94172f40c7fa4b7b89237fd382448e598da00fbb Reviewed-on: https://review.coreboot.org/c/coreboot/+/56188 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Held <felix-coreboot(a)felixheld.de> --- M src/soc/amd/cezanne/Makefile.inc M src/soc/amd/cezanne/acpi.c A src/soc/amd/cezanne/cppc.c A src/soc/amd/cezanne/include/soc/cppc.h M src/soc/amd/cezanne/include/soc/msr.h 5 files changed, 215 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved diff --git a/src/soc/amd/cezanne/Makefile.inc b/src/soc/amd/cezanne/Makefile.inc index d68a6b6..478eecc 100644 --- a/src/soc/amd/cezanne/Makefile.inc +++ b/src/soc/amd/cezanne/Makefile.inc @@ -32,6 +32,8 @@ ramstage-y += i2c.c ramstage-y += acpi.c +ramstage-y += cppc.c + ramstage-y += agesa_acpi.c ramstage-y += chip.c ramstage-y += cpu.c diff --git a/src/soc/amd/cezanne/acpi.c b/src/soc/amd/cezanne/acpi.c index 6e2ae9c..3b3a88b 100644 --- a/src/soc/amd/cezanne/acpi.c +++ b/src/soc/amd/cezanne/acpi.c @@ -19,6 +19,7 @@ #include <soc/msr.h> #include <types.h> #include "chip.h" +#include <soc/cppc.h> unsigned long acpi_fill_madt(unsigned long current) { @@ -358,6 +359,8 @@ acpigen_write_CSD_package(cpu / threads_per_core, threads_per_core, CSD_HW_ALL, 0); + generate_cppc_entries(cpu); + acpigen_pop_len(); } diff --git a/src/soc/amd/cezanne/cppc.c b/src/soc/amd/cezanne/cppc.c new file mode 100644 index 0000000..79292a4 --- /dev/null +++ b/src/soc/amd/cezanne/cppc.c @@ -0,0 +1,180 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <acpi/acpi_pm.h> +#include <acpi/acpigen.h> +#include <arch/cpu.h> +#include <soc/cppc.h> +#include <soc/msr.h> + +/* + * version 2 is expected to be the typical use case. + * For now this function 'punts' on version 3 and just + * populates the additional fields with 'unsupported'. + */ +void cpu_init_cppc_config(struct cppc_config *config, u32 version) +{ + acpi_addr_t msr = { + .space_id = ACPI_ADDRESS_SPACE_FIXED, + .bit_width = 8, + .bit_offset = 0, + .access_size = ACPI_ACCESS_SIZE_QWORD_ACCESS, + .addrl = 0, + .addrh = 0, + }; + static const acpi_addr_t unsupported = { + .space_id = ACPI_ADDRESS_SPACE_MEMORY, + .bit_width = 0, + .bit_offset = 0, + .access_size = ACPI_ACCESS_SIZE_UNDEFINED, + .addrl = 0, + .addrh = 0, + }; + + config->version = version; + + /* + * Highest Performance: + */ + msr.addrl = MSR_CPPC_CAPABILITY_1; + msr.bit_offset = SHIFT_CPPC_CAPABILITY_1_HIGHEST_PERF; + config->regs[CPPC_HIGHEST_PERF] = msr; + + /* + * Lowest Nonlinear Performance -> Most Efficient Performance: + */ + msr.bit_offset = SHIFT_CPPC_CAPABILITY_1_LOW_NON_LIN_PERF; + config->regs[CPPC_LOWEST_NONL_PERF] = msr; + + /* + * Lowest Performance: + */ + msr.bit_offset = SHIFT_CPPC_CAPABILITY_1_LOWEST_PERF; + config->regs[CPPC_LOWEST_PERF] = msr; + + /* + * Guaranteed Performance Register: + */ + config->regs[CPPC_GUARANTEED_PERF] = unsupported; + + /* + * Nominal Performance -> Maximum Non-Turbo Ratio: + */ + msr.bit_offset = SHIFT_CPPC_CAPABILITY_1_NOMINAL_PERF; + config->regs[CPPC_NOMINAL_PERF] = msr; + + /* + * Desired Performance Register: + */ + msr.addrl = MSR_CPPC_REQUEST; + msr.bit_offset = SHIFT_CPPC_REQUEST_DES_PERF; + config->regs[CPPC_DESIRED_PERF] = msr; + + /* + * Minimum Performance Register: + */ + msr.bit_offset = SHIFT_CPPC_REQUEST_MIN_PERF; + config->regs[CPPC_MIN_PERF] = msr; + + /* + * Maximum Performance Register: + */ + msr.bit_offset = SHIFT_CPPC_REQUEST_MAX_PERF; + config->regs[CPPC_MAX_PERF] = msr; + + /* + * Performance Reduction Tolerance Register: + */ + config->regs[CPPC_PERF_REDUCE_TOLERANCE] = unsupported; + + /* + * Time Window Register: + */ + config->regs[CPPC_TIME_WINDOW] = unsupported; + + /* + * Counter Wraparound Time: + */ + config->regs[CPPC_COUNTER_WRAP] = unsupported; + + /* + * Reference Performance Counter Register: + */ + msr.addrl = MSR_MAX_PERFORMANCE_FREQUENCY_CLOCK_COUNT; + msr.bit_width = 64; + msr.bit_offset = 0; + config->regs[CPPC_REF_PERF_COUNTER] = msr; + + /* + * Delivered Performance Counter Register: + */ + msr.addrl = MSR_ACTUAL_PERFORMANCE_FREQUENCY_CLOCK_COUNT; + config->regs[CPPC_DELIVERED_PERF_COUNTER] = msr; + + /* + * Performance Limited Register: + */ + msr.bit_width = 1; + msr.addrl = MSR_CPPC_STATUS; + msr.bit_offset = 1; + config->regs[CPPC_PERF_LIMITED] = msr; + + /* + * CPPC Enable Register: + */ + msr.addrl = MSR_CPPC_ENABLE; + msr.bit_offset = 0; + config->regs[CPPC_ENABLE] = msr; + + if (version >= 2) { + /* Autonomous Selection Enable is populated below */ + + /* + * Autonomous Activity Window Register + */ + config->regs[CPPC_AUTO_ACTIVITY_WINDOW] = unsupported; + + /* + * Autonomous Energy Performance Preference Register + */ + msr.addrl = MSR_CPPC_REQUEST; + msr.bit_width = 8; + msr.bit_offset = SHIFT_CPPC_REQUEST_ENERGY_PERF_PREF; + config->regs[CPPC_PERF_PREF] = msr; + + /* Reference Performance */ + config->regs[CPPC_REF_PERF] = unsupported; + + if (version >= 3) { + /* Lowest Frequency */ + config->regs[CPPC_LOWEST_FREQ] = unsupported; + /* Nominal Frequency */ + config->regs[CPPC_NOMINAL_FREQ] = unsupported; + } + + /* + * Autonomous Selection Enable = 1 + * This field is actually the first addition in version 2 but + * it's so unlike the others I'm populating it last. + */ + msr.space_id = ACPI_ADDRESS_SPACE_MEMORY; + msr.bit_width = 32; + msr.bit_offset = 0; + msr.access_size = ACPI_ACCESS_SIZE_UNDEFINED; + msr.addrl = 1; + config->regs[CPPC_AUTO_SELECT] = unsupported; + } +} + + +void generate_cppc_entries(unsigned int core_id) +{ + /* Generate GCPC package in first logical core */ + if (core_id == 0) { + struct cppc_config cppc_config; + cpu_init_cppc_config(&cppc_config, CPPC_VERSION_3); + acpigen_write_CPPC_package(&cppc_config); + } + + /* Write _CPC entry for each logical core */ + acpigen_write_CPPC_method(); +} diff --git a/src/soc/amd/cezanne/include/soc/cppc.h b/src/soc/amd/cezanne/include/soc/cppc.h new file mode 100644 index 0000000..60a5efd --- /dev/null +++ b/src/soc/amd/cezanne/include/soc/cppc.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _CPU_AMD_COMMON_H +#define _CPU_AMD_COMMON_H + +#include <types.h> + +struct cppc_config; +void cpu_init_cppc_config(struct cppc_config *config, u32 version); +void generate_cppc_entries(unsigned int core_id); + +#endif diff --git a/src/soc/amd/cezanne/include/soc/msr.h b/src/soc/amd/cezanne/include/soc/msr.h index cc1f77e..ca2992a 100644 --- a/src/soc/amd/cezanne/include/soc/msr.h +++ b/src/soc/amd/cezanne/include/soc/msr.h @@ -21,4 +21,22 @@ #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) #define PSTATE_DEF_LO_CORE_FREQ_BASE 25 +#define MSR_CPPC_CAPABILITY_1 0xc00102b0 +#define SHIFT_CPPC_CAPABILITY_1_HIGHEST_PERF 24 +#define SHIFT_CPPC_CAPABILITY_1_NOMINAL_PERF 16 +#define SHIFT_CPPC_CAPABILITY_1_LOW_NON_LIN_PERF 8 +#define SHIFT_CPPC_CAPABILITY_1_LOWEST_PERF 0 + +#define MSR_CPPC_ENABLE 0xc00102b1 +#define MSR_CPPC_REQUEST 0xc00102b3 +#define SHIFT_CPPC_REQUEST_ENERGY_PERF_PREF 24 +#define SHIFT_CPPC_REQUEST_DES_PERF 16 +#define SHIFT_CPPC_REQUEST_MIN_PERF 8 +#define SHIFT_CPPC_REQUEST_MAX_PERF 0 + +#define MSR_CPPC_STATUS 0xc00102b4 + +#define MSR_MAX_PERFORMANCE_FREQUENCY_CLOCK_COUNT 0xe7 +#define MSR_ACTUAL_PERFORMANCE_FREQUENCY_CLOCK_COUNT 0xe8 + #endif /* AMD_CEZANNE_MSR_H */ -- To view, visit https://review.coreboot.org/c/coreboot/+/56188 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I94172f40c7fa4b7b89237fd382448e598da00fbb Gerrit-Change-Number: 56188 Gerrit-PatchSet: 8 Gerrit-Owner: Julian Schroeder <julianmarcusschroeder(a)gmail.com> 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-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: qualcomm/sc7280: gpio: Support eGPIO scheme
by Julius Werner (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Shelley Chen, Ravi kumar, Taniya Das, Paul Menzel, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55078 ) Change subject: qualcomm/sc7280: gpio: Support eGPIO scheme ...................................................................... Patch Set 12: (1 comment) File src/soc/qualcomm/sc7280/sc7280_egpio.c: https://review.coreboot.org/c/coreboot/+/55078/comment/446c0aef_0429c42b PS3, Line 56: setbits32(&regs->cfg, BIT(EGPIO_CFG_EN)); > Hi Julius, […] ...that bug doesn't contain any further information related to my question. Please read my previous comments again closely. I am not asking why we need eGPIO support, I am aware of the overall issue here. I am asking *why the code that enables eGPIO support by setting the EGPIO_CFG flag in the GPIO_CFG register must run in coreboot and cannot just run in the kernel*. Please answer that question specifically. -- To view, visit https://review.coreboot.org/c/coreboot/+/55078 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2c54a14c50fb7b5921d1961d2de83098ed2d4358 Gerrit-Change-Number: 55078 Gerrit-PatchSet: 12 Gerrit-Owner: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Taniya Das <tdas(a)codeaurora.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Attention: Taniya Das <tdas(a)codeaurora.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org> Gerrit-Comment-Date: Thu, 15 Jul 2021 21:22:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Shelley Chen <shchen(a)google.com> Comment-In-Reply-To: Taniya Das <tdas(a)codeaurora.org> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/../car: Calculate SF Mask#1 based on MSR 0xc87
by Subrata Banik (Code Review) 15 Jul '21

15 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51374 ) Change subject: soc/intel/common/../car: Calculate SF Mask#1 based on MSR 0xc87 ...................................................................... Patch Set 13: (1 comment) Patchset: PS13: > Looks OK to me, let others look too @Tim, requesting you to take one more look if possible ? -- To view, visit https://review.coreboot.org/c/coreboot/+/51374 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifd0b7e1a90cad4a4837adf6067fe8301dcd0a941 Gerrit-Change-Number: 51374 Gerrit-PatchSet: 13 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.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: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Thu, 15 Jul 2021 21:13:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2021