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/+/58926 )
Change subject: soc/amd/common/block: Add spi_hw mutex
......................................................................
soc/amd/common/block: Add spi_hw mutex
There are currently two users of the SPI hardware, the LPC SPI DMA
controller, and the boot_device_rw device. We need to ensure exclusivity
to the SPI hardware otherwise the SPI DMA controller can be interrupted
and it will silently skip transferring some blocks.
Depending on the SPI speed, this change might add a small delay when
clearing the elog since a DMA transaction might be in flight. I'll
continue optimizing the boot flow to avoid the delay.
BUG=b:179699789
TEST=Hack up the code to interleave SPI transactions and verify this
patch fixes the silent data corruption.
Signed-off-by: Raul E Rangel <rrangel(a)chromium.org>
Change-Id: I5eee812a6979c8c0fb313dd2fbccc14b73d7d741
---
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
4 files changed, 27 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/58926/1
diff --git a/src/soc/amd/common/block/include/amdblocks/spi.h b/src/soc/amd/common/block/include/amdblocks/spi.h
index 35a3782..81da5dd 100644
--- a/src/soc/amd/common/block/include/amdblocks/spi.h
+++ b/src/soc/amd/common/block/include/amdblocks/spi.h
@@ -3,6 +3,7 @@
#ifndef AMD_BLOCK_SPI_H
#define AMD_BLOCK_SPI_H
+#include <thread.h>
#include <types.h>
#define SPI_CNTRL0 0x00
@@ -118,4 +119,8 @@
void fch_spi_config_modes(void);
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;
+
#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 5c69779..baf1eb8 100644
--- a/src/soc/amd/common/block/lpc/spi_dma.c
+++ b/src/soc/amd/common/block/lpc/spi_dma.c
@@ -122,6 +122,12 @@
ctrl |= LPC_ROM_DMA_CTRL_ERROR; /* Clear error */
ctrl |= LPC_ROM_DMA_CTRL_START;
+ /*
+ * Ensure we have exclusive access to the SPI controller before starting the LPC SPI DMA
+ * transaction.
+ */
+ thread_mutex_lock(&spi_hw_mutex);
+
pci_write_config32(SOC_LPC_DEV, LPC_ROM_DMA_EC_HOST_CONTROL, ctrl);
}
@@ -135,6 +141,12 @@
if (spi_dma_is_busy())
return true;
+ /*
+ * 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);
+
if (spi_dma_has_error()) {
printk(BIOS_ERR,
"ERROR: SPI DMA failure: dest: %p, source: %#zx, size: %zu\n",
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 565fdbe..d95b642 100644
--- a/src/soc/amd/common/block/spi/fch_spi_ctrl.c
+++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c
@@ -142,7 +142,13 @@
static int xfer_vectors(const struct spi_slave *slave,
struct spi_op vectors[], size_t count)
{
- return spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer);
+ int rc;
+
+ thread_mutex_lock(&spi_hw_mutex);
+ rc = spi_flash_vector_helper(slave, vectors, count, spi_ctrlr_xfer);
+ thread_mutex_unlock(&spi_hw_mutex);
+
+ return rc;
}
static int protect_a_range(u32 value)
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 5cef565..862962f 100644
--- a/src/soc/amd/common/block/spi/fch_spi_util.c
+++ b/src/soc/amd/common/block/spi/fch_spi_util.c
@@ -6,6 +6,9 @@
#include <assert.h>
#include <stdint.h>
+/* Global SPI controller mutex */
+struct thread_mutex spi_hw_mutex;
+
static uintptr_t spi_base;
void spi_set_base(void *base)
--
To view, visit https://review.coreboot.org/c/coreboot/+/58926
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5eee812a6979c8c0fb313dd2fbccc14b73d7d741
Gerrit-Change-Number: 58926
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
Attention is currently required from: Furquan Shaikh, Julius Werner, Angel Pons, Karthik Ramasubramanian.
Hello build bot (Jenkins), Furquan Shaikh, Julius Werner, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/56580
to look at the new patch set (#12).
Change subject: commonlib/mem_pool: Allow configuring the alignment
......................................................................
commonlib/mem_pool: Allow configuring the alignment
AMD platforms require the destination to be 64 byte aligned in order to
use the SPI DMA controller. This is enforced by the destination address
register because the first 6 bits are marked as reserved.
This change adds an option to the mem_pool so the alignment can be
configured.
BUG=b:179699789
TEST=Boot guybrush to OS
Signed-off-by: Raul E Rangel <rrangel(a)chromium.org>
Change-Id: I8d77ffe4411f86c54450305320c9f52ab41a3075
---
M src/commonlib/include/commonlib/mem_pool.h
M src/commonlib/mem_pool.c
M src/lib/cbfs.c
3 files changed, 21 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/56580/12
--
To view, visit https://review.coreboot.org/c/coreboot/+/56580
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8d77ffe4411f86c54450305320c9f52ab41a3075
Gerrit-Change-Number: 56580
Gerrit-PatchSet: 12
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Nick Vaccaro, Balaji Manigandan, Krishna P Bhat D.
Hello build bot (Jenkins), Tim Wawrzynczak, Sridhar Siricilla, Nick Vaccaro, Balaji Manigandan, Krishna P Bhat D, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/58679
to look at the new patch set (#10).
Change subject: util/spd_tools: Add LP5 support for ADL
......................................................................
util/spd_tools: Add LP5 support for ADL
Add LP5 support to spd_tools. Currently, only Intel Alder Lake (ADL) is
supported.
The SPDs are generated based on a combination of:
- The LPDDR5 spec JESD209-5B.
- The SPD spec SPD4.1.2.M-2 (the LPDDR3/4 spec is used since JEDEC has
not released an SPD spec for LPDDR5).
- Intel recommendations in advisory #616599.
BUG=b:201234943, b:198704251
TEST=Generate the SPD and manifests for a test part, and check that the
SPD matches Intel's expectation. More details in CB:58680.
Change-Id: Ic1e68d44f7c0ad64aa9904b7e1297d24bd5db56e
Signed-off-by: Reka Norman <rekanorman(a)google.com>
---
M util/spd_tools/README.md
M util/spd_tools/src/part_id_gen/part_id_gen.go
A util/spd_tools/src/spd_gen/lp5.go
M util/spd_tools/src/spd_gen/spd_gen.go
4 files changed, 726 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/58679/10
--
To view, visit https://review.coreboot.org/c/coreboot/+/58679
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic1e68d44f7c0ad64aa9904b7e1297d24bd5db56e
Gerrit-Change-Number: 58679
Gerrit-PatchSet: 10
Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(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-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Reka Norman.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/58680
to look at the new patch set (#10).
Change subject: [TESTONLY] Generate the SPD for a LP5 test part
......................................................................
[TESTONLY] Generate the SPD for a LP5 test part
Add spd/lp5/memory_parts.json with a single test part.
Generate the SPD and manifests using:
util/spd_tools/bin/spd_gen spd/lp5/memory_parts.json lp5
The generated spd-1.hex is identical to the example SPD provided by
Intel for this part, except that the following "don't care" bytes have
been zeroed:
- 9 Other SDRAM Optional Features
- 16 Signal Loading
- 19 Maximum Cycle Time
- 20-23 CAS Latencies Supported
- 124 Fine Offset for Maximum Cycle Time
- 325 Module ID: Module Serial Number
Change-Id: Id96399c17577b1350c6e04c87cf40c4b2ff1478e
Signed-off-by: Reka Norman <rekanorman(a)google.com>
---
A spd/lp5/memory_parts.json
A spd/lp5/platforms_manifest.generated.txt
A spd/lp5/set-0/parts_spd_manifest.generated.txt
A spd/lp5/set-0/spd-1.hex
A spd/lp5/set-0/spd-empty.hex
5 files changed, 86 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/58680/10
--
To view, visit https://review.coreboot.org/c/coreboot/+/58680
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id96399c17577b1350c6e04c87cf40c4b2ff1478e
Gerrit-Change-Number: 58680
Gerrit-PatchSet: 10
Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58843 )
Change subject: amd/lpc/espi: Remove weak function
......................................................................
Patch Set 6: Code-Review-1
(1 comment)
Patchset:
PS6:
same here: since this is a mainboard-level override which is only needed in very few cases, i'd just keep the weak function
--
To view, visit https://review.coreboot.org/c/coreboot/+/58843
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I39ae6405589d645eaa3e05759b1ffbd36e688d8f
Gerrit-Change-Number: 58843
Gerrit-PatchSet: 6
Gerrit-Owner: Bao Zheng <fishbaozi(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: Zheng Bao
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Comment-Date: Wed, 03 Nov 2021 22:28:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58844 )
Change subject: amd/common/spi: Remove weak function
......................................................................
Patch Set 4: Code-Review-1
--
To view, visit https://review.coreboot.org/c/coreboot/+/58844
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I99509b6162f568c202cb82a8a238a895f6e93eb4
Gerrit-Change-Number: 58844
Gerrit-PatchSet: 4
Gerrit-Owner: Bao Zheng <fishbaozi(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: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Comment-Date: Wed, 03 Nov 2021 22:27:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment