Attention is currently required from: Fred Reitberger, Jason Glenesk, Matt DeVillier, ritul guru.
Felix Held has posted comments on this change by Felix Held. ( https://review.coreboot.org/c/coreboot/+/83740?usp=email )
Change subject: soc/amd/common/psp_smi: implement P2C mailbox handling
......................................................................
Patch Set 4:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83740?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I50479bed2332addae652026c6818460eeb6403af
Gerrit-Change-Number: 83740
Gerrit-PatchSet: 4
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Comment-Date: Mon, 05 Aug 2024 23:11:09 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Attention is currently required from: Fred Reitberger, Jason Glenesk, Matt DeVillier.
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83778?usp=email )
Change subject: soc/amd/common/psp_smi_flash: add buffer overflow checks
......................................................................
soc/amd/common/psp_smi_flash: add buffer overflow checks
Before 'handle_psp_command' calls any of the functions in this file, it
make sure that the 'size' field in the command buffer's header doesn't
indicate that the command buffer is larger than the SMM memory region
reserved for it.
The read/write command buffer has a 'num_bytes' field to indicate how
many bytes should be read from the SPI flash and put into the data
buffer within the command buffer or how many bytes from this buffer
should be written to the flash. While we should be able to assume that
the PSP won't send us malformed command buffer, we should still better
check this just to be sure.
Test=When selecting SOC_AMD_COMMON_BLOCK_PSP_SMI, Mandolin still builds
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Change-Id: Ib4e8514eedc3ad154a705c8a1e85d367e452dbed
---
M src/soc/amd/common/block/psp/psp_smi_flash.c
1 file changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/83778/1
diff --git a/src/soc/amd/common/block/psp/psp_smi_flash.c b/src/soc/amd/common/block/psp/psp_smi_flash.c
index 2d075ef..779b191 100644
--- a/src/soc/amd/common/block/psp/psp_smi_flash.c
+++ b/src/soc/amd/common/block/psp/psp_smi_flash.c
@@ -104,6 +104,16 @@
*num_blocks = read64(&cmd_buf->req.num_blocks);
}
+static bool is_valid_rw_byte_count(struct mbox_pspv2_cmd_spi_read_write *cmd_buf,
+ u64 num_bytes)
+{
+ const u32 cmd_buf_size = read32(&cmd_buf->header.size);
+ const size_t payload_buffer_offset =
+ offsetof(struct mbox_pspv2_cmd_spi_read_write, req) +
+ offsetof(struct pspv2_spi_read_write_request, buffer);
+ return num_bytes <= cmd_buf_size - payload_buffer_offset;
+}
+
static const char *id_to_region_name(u64 target_nv_id)
{
switch (target_nv_id) {
@@ -238,6 +248,9 @@
get_psp_spi_read_write(cmd_buf, &target_nv_id, &lba, &offset, &num_bytes, &data);
+ if (!is_valid_rw_byte_count(cmd_buf, num_bytes))
+ return MBOX_PSP_COMMAND_PROCESS_ERROR;
+
ret = find_psp_spi_flash_device_region(target_nv_id, &store, &flash);
if (ret != MBOX_PSP_SUCCESS)
@@ -281,6 +294,9 @@
get_psp_spi_read_write(cmd_buf, &target_nv_id, &lba, &offset, &num_bytes, &data);
+ if (!is_valid_rw_byte_count(cmd_buf, num_bytes))
+ return MBOX_PSP_COMMAND_PROCESS_ERROR;
+
ret = find_psp_spi_flash_device_region(target_nv_id, &store, &flash);
if (ret != MBOX_PSP_SUCCESS)
--
To view, visit https://review.coreboot.org/c/coreboot/+/83778?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ib4e8514eedc3ad154a705c8a1e85d367e452dbed
Gerrit-Change-Number: 83778
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Attention is currently required from: Fred Reitberger, Jason Glenesk, Matt DeVillier, ritul guru.
Hello ritul guru,
I'd like you to do a code review.
Please visit
https://review.coreboot.org/c/coreboot/+/83776?usp=email
to review the following change.
Change subject: soc/amd/common/psp_smi_flash: implement SPI info command
......................................................................
soc/amd/common/psp_smi_flash: implement SPI info command
Detect the block size of the SPI flash and number of flash blocks
reserved for the flash region corresponding to the 'target_nv_id' field
in the command buffer. This information is then written to the
corresponding fields in the command buffer. Since detecting the flash
chip still might result in accesses to it, make sure that it's available
for use and not currently used by an OS driver.
This patch is a modified version of parts of CB:65523.
Document #55758 Rev. 2.04 was used as a reference.
Test=When selecting SOC_AMD_COMMON_BLOCK_PSP_SMI, Mandolin still builds
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Signed-off-by: Ritul Guru <ritul.bits(a)gmail.com>
Change-Id: I19041a27a9e8f901d42c3f60af834df625455ea6
---
M src/soc/amd/common/block/psp/psp_smi_flash.c
1 file changed, 43 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/83776/1
diff --git a/src/soc/amd/common/block/psp/psp_smi_flash.c b/src/soc/amd/common/block/psp/psp_smi_flash.c
index 367baef..420fb94 100644
--- a/src/soc/amd/common/block/psp/psp_smi_flash.c
+++ b/src/soc/amd/common/block/psp/psp_smi_flash.c
@@ -72,6 +72,19 @@
return is_valid_psp_spi_id(read64(&cmd_buf->req.target_nv_id));
}
+static u64 get_psp_spi_info_id(struct mbox_pspv2_cmd_spi_info *cmd_buf)
+{
+ return read64(&cmd_buf->req.target_nv_id);
+}
+
+static void set_psp_spi_info(struct mbox_pspv2_cmd_spi_info *cmd_buf,
+ u64 lba, u64 block_size, u64 num_blocks)
+{
+ write64(&cmd_buf->req.lba, lba);
+ write64(&cmd_buf->req.block_size, block_size);
+ write64(&cmd_buf->req.num_blocks, num_blocks);
+}
+
static const char *id_to_region_name(u64 target_nv_id)
{
switch (target_nv_id) {
@@ -117,7 +130,7 @@
return rdev_chain(rstore, rdev, 0, region_device_sz(rdev));
}
-static inline enum mbox_p2c_status find_psp_spi_flash_device_region(u64 target_nv_id,
+static enum mbox_p2c_status find_psp_spi_flash_device_region(u64 target_nv_id,
struct region_device *store,
const struct spi_flash **flash)
{
@@ -135,7 +148,7 @@
return MBOX_PSP_SUCCESS;
}
-static inline bool spi_controller_available(void)
+static bool spi_controller_available(void)
{
return !(spi_read8(SPI_MISC_CNTRL) & SPI_SEMAPHORE_DRIVER_LOCKED);
}
@@ -144,13 +157,40 @@
{
struct mbox_pspv2_cmd_spi_info *const cmd_buf =
(struct mbox_pspv2_cmd_spi_info *)buffer;
+ const struct spi_flash *flash;
+ struct region_device store;
+ u64 target_nv_id;
+ u64 block_size;
+ u64 num_blocks;
+ enum mbox_p2c_status ret;
printk(BIOS_SPEW, "PSP: SPI info request\n");
if (!is_valid_psp_spi_info(cmd_buf))
return MBOX_PSP_COMMAND_PROCESS_ERROR;
- return MBOX_PSP_UNSUPPORTED;
+ if (!spi_controller_available()) {
+ printk(BIOS_NOTICE, "PSP: SPI controller busy\n");
+ return MBOX_PSP_SPI_BUSY;
+ }
+
+ target_nv_id = get_psp_spi_info_id(cmd_buf);
+
+ ret = find_psp_spi_flash_device_region(target_nv_id, &store, &flash);
+
+ if (ret != MBOX_PSP_SUCCESS)
+ return ret;
+
+ block_size = flash->sector_size;
+
+ if (!block_size)
+ return MBOX_PSP_COMMAND_PROCESS_ERROR;
+
+ num_blocks = region_device_sz(&store) / block_size;
+
+ set_psp_spi_info(cmd_buf, 0, block_size, num_blocks);
+
+ return MBOX_PSP_SUCCESS;
}
enum mbox_p2c_status psp_smi_spi_read(struct mbox_default_buffer *buffer)
--
To view, visit https://review.coreboot.org/c/coreboot/+/83776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I19041a27a9e8f901d42c3f60af834df625455ea6
Gerrit-Change-Number: 83776
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Attention is currently required from: Fred Reitberger, Jason Glenesk, Matt DeVillier, ritul guru.
Hello ritul guru,
I'd like you to do a code review.
Please visit
https://review.coreboot.org/c/coreboot/+/83775?usp=email
to review the following change.
Change subject: soc/amd/common/psp_smi_flash: add spi_controller_available
......................................................................
soc/amd/common/psp_smi_flash: add spi_controller_available
Add the 'spi_controller_available' helper function to check
SPI_SEMAPHORE_DRIVER_LOCKED in SPI_MISC_CNTRL to see if some software or
driver outside of SMM is currently using the SPI flash controller to
avoid interfering with that operation.
This patch is a slightly reworked version of parts of CB:65523.
Test=When selecting SOC_AMD_COMMON_BLOCK_PSP_SMI, Mandolin still builds
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Signed-off-by: Ritul Guru <ritul.bits(a)gmail.com>
Change-Id: I49218e03a5dd555b2b2d34eaad86673e9fc908c3
---
M src/soc/amd/common/block/psp/psp_smi_flash.c
1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/83775/1
diff --git a/src/soc/amd/common/block/psp/psp_smi_flash.c b/src/soc/amd/common/block/psp/psp_smi_flash.c
index 4c83fcd..367baef 100644
--- a/src/soc/amd/common/block/psp/psp_smi_flash.c
+++ b/src/soc/amd/common/block/psp/psp_smi_flash.c
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
+#include <amdblocks/spi.h>
#include <boot_device.h>
#include <commonlib/region.h>
#include <console/console.h>
@@ -134,6 +135,11 @@
return MBOX_PSP_SUCCESS;
}
+static inline bool spi_controller_available(void)
+{
+ return !(spi_read8(SPI_MISC_CNTRL) & SPI_SEMAPHORE_DRIVER_LOCKED);
+}
+
enum mbox_p2c_status psp_smi_spi_get_info(struct mbox_default_buffer *buffer)
{
struct mbox_pspv2_cmd_spi_info *const cmd_buf =
--
To view, visit https://review.coreboot.org/c/coreboot/+/83775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I49218e03a5dd555b2b2d34eaad86673e9fc908c3
Gerrit-Change-Number: 83775
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Attention is currently required from: Fred Reitberger, Jason Glenesk, Matt DeVillier, ritul guru.
Hello ritul guru,
I'd like you to do a code review.
Please visit
https://review.coreboot.org/c/coreboot/+/83774?usp=email
to review the following change.
Change subject: soc/amd/common/psp_smi_flash: add find_psp_spi_flash_device_region
......................................................................
soc/amd/common/psp_smi_flash: add find_psp_spi_flash_device_region
Add 'find_psp_spi_flash_device_region' to get a pointer to the spi_flash
struct of the SPI flash used in the system and the region_device struct
for the target FMAP region specified by the target NV ID from the PSP
to x86 mailbox command. In order to have small patches, the newly added
static 'find_psp_spi_flash_device_region' function is marked as inline;
that inline will be removed in a following patch that calls this new
function.
This patch is a slightly reworked version of parts of CB:65523.
Document #55758 Rev. 2.04 was used as a reference.
Test=When selecting SOC_AMD_COMMON_BLOCK_PSP_SMI, Mandolin still builds
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Signed-off-by: Ritul Guru <ritul.bits(a)gmail.com>
Change-Id: I64b8fba2392de46ecd4c786cef0d5b6acdbd865a
---
M src/soc/amd/common/block/psp/psp_smi_flash.c
1 file changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/83774/1
diff --git a/src/soc/amd/common/block/psp/psp_smi_flash.c b/src/soc/amd/common/block/psp/psp_smi_flash.c
index f1b9d85..4c83fcd 100644
--- a/src/soc/amd/common/block/psp/psp_smi_flash.c
+++ b/src/soc/amd/common/block/psp/psp_smi_flash.c
@@ -1,7 +1,11 @@
/* SPDX-License-Identifier: GPL-2.0-only */
+#include <boot_device.h>
+#include <commonlib/region.h>
#include <console/console.h>
#include <device/mmio.h>
+#include <fmap.h>
+#include <spi_flash.h>
#include <types.h>
#include "psp_def.h"
@@ -67,6 +71,69 @@
return is_valid_psp_spi_id(read64(&cmd_buf->req.target_nv_id));
}
+static const char *id_to_region_name(u64 target_nv_id)
+{
+ switch (target_nv_id) {
+ case SMI_TARGET_NVRAM:
+ return "PSP_NVRAM";
+ case SMI_TARGET_RPMC_NVRAM:
+ return "PSP_RPMC_NVRAM";
+ }
+ return NULL;
+}
+
+/*
+ * Do not cache the location to cope with flash changing underneath (e.g. due
+ * to an update)
+ */
+static int lookup_store(u64 target_nv_id, struct region_device *rstore)
+{
+ /* read_rdev, write_rdev and store_irdev need to be static to not go out of scope when
+ this function returns */
+ static struct region_device read_rdev, write_rdev;
+ static struct incoherent_rdev store_irdev;
+ const char *name;
+ struct region region;
+ const struct region_device *rdev;
+
+ name = id_to_region_name(target_nv_id);
+ if (!name)
+ return -1;
+
+ if (fmap_locate_area(name, ®ion) < 0)
+ return -1;
+
+ if (boot_device_ro_subregion(®ion, &read_rdev) < 0)
+ return -1;
+
+ if (boot_device_rw_subregion(®ion, &write_rdev) < 0)
+ return -1;
+
+ rdev = incoherent_rdev_init(&store_irdev, ®ion, &read_rdev, &write_rdev);
+ if (rdev == NULL)
+ return -1;
+
+ return rdev_chain(rstore, rdev, 0, region_device_sz(rdev));
+}
+
+static inline enum mbox_p2c_status find_psp_spi_flash_device_region(u64 target_nv_id,
+ struct region_device *store,
+ const struct spi_flash **flash)
+{
+ *flash = boot_device_spi_flash();
+ if (*flash == NULL) {
+ printk(BIOS_ERR, "PSP: Unable to find SPI device\n");
+ return MBOX_PSP_COMMAND_PROCESS_ERROR;
+ }
+
+ if (lookup_store(target_nv_id, store) < 0) {
+ printk(BIOS_ERR, "PSP: Unable to find PSP SPI region\n");
+ return MBOX_PSP_COMMAND_PROCESS_ERROR;
+ }
+
+ return MBOX_PSP_SUCCESS;
+}
+
enum mbox_p2c_status psp_smi_spi_get_info(struct mbox_default_buffer *buffer)
{
struct mbox_pspv2_cmd_spi_info *const cmd_buf =
--
To view, visit https://review.coreboot.org/c/coreboot/+/83774?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I64b8fba2392de46ecd4c786cef0d5b6acdbd865a
Gerrit-Change-Number: 83774
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: ritul guru <ritul.bits(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83773?usp=email )
Change subject: soc/amd/common/include/spi: add and use SPI_MISC_CNTRL defines
......................................................................
soc/amd/common/include/spi: add and use SPI_MISC_CNTRL defines
This register is currently used by the SPI DMA code that sets an
undocumented bit. A following patch will use the
SPI_SEMAPHORE_DRIVER_LOCKED bit definition. Since that one won't affect
the hardware it's marked as reserved in the PPR, but is used as a
semaphore for arbitration of the SPI controller access between SMM code
and non-SMM code like OS drivers in the AGESA reference code.
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Change-Id: I48447dcfb3cee07619a9b42434731f0b21458021
---
M src/soc/amd/common/block/include/amdblocks/spi.h
M src/soc/amd/common/block/lpc/spi_dma.c
2 files changed, 7 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/83773/1
diff --git a/src/soc/amd/common/block/include/amdblocks/spi.h b/src/soc/amd/common/block/include/amdblocks/spi.h
index cf91814..d6f35f8 100644
--- a/src/soc/amd/common/block/include/amdblocks/spi.h
+++ b/src/soc/amd/common/block/include/amdblocks/spi.h
@@ -76,6 +76,11 @@
#define SPI_FIFO_LAST_BYTE 0xc6 /* 0xc7 for Cezanne */
#define SPI_FIFO_DEPTH (SPI_FIFO_LAST_BYTE - SPI_FIFO + 1)
+#define SPI_MISC_CNTRL 0xfc
+/* Re-purpose unused SPI controller register bit for semaphore to synchronize
+ SPI access between SMM and non-SMM software/OS driver. */
+#define SPI_SEMAPHORE_DRIVER_LOCKED BIT(4)
+
struct spi_config {
/*
* Default values if not overridden by mainboard:
diff --git a/src/soc/amd/common/block/lpc/spi_dma.c b/src/soc/amd/common/block/lpc/spi_dma.c
index 701b61a..9ff9b0e 100644
--- a/src/soc/amd/common/block/lpc/spi_dma.c
+++ b/src/soc/amd/common/block/lpc/spi_dma.c
@@ -277,9 +277,9 @@
static void spi_dma_fix(void)
{
/* Internal only registers */
- uint8_t val = spi_read8(0xfc);
+ uint8_t val = spi_read8(SPI_MISC_CNTRL);
val |= BIT(6);
- spi_write8(0xfc, val);
+ spi_write8(SPI_MISC_CNTRL, val);
}
void boot_device_init(void)
--
To view, visit https://review.coreboot.org/c/coreboot/+/83773?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I48447dcfb3cee07619a9b42434731f0b21458021
Gerrit-Change-Number: 83773
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Christian Walter, Jonathan Zhang, Krystian Hebel, Martin L Roth, Martin Roth, Michał Żygowski, Sergii Dmytruk.
Paul Menzel has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/coreboot/+/67057?usp=email )
Change subject: drivers/ipmi: add Block Transfer (BT) interface
......................................................................
Patch Set 33:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/67057/comment/f7f63dcf_e4da8dac?us… :
PS33, Line 14:
How did you test this? Can it be tested with QEMU? Maybe mention the datasheet name?
File Documentation/drivers/index.md:
https://review.coreboot.org/c/coreboot/+/67057/comment/a5093476_59576fd7?us… :
PS33, Line 15: BT
Include Block Transfer?
File Documentation/drivers/ipmi_bt.md:
https://review.coreboot.org/c/coreboot/+/67057/comment/6948a3f8_e71444a6?us… :
PS33, Line 14: ```
: chip drivers/ipmi
: device pnp e4.0 on end # IPMI BT
: end
: ```
I’d prefer intending by four spaces. But feel free to mark as resolved.
https://review.coreboot.org/c/coreboot/+/67057/comment/3693746d_58c865d4?us… :
PS33, Line 22: The following registers can be set:
Where can they be set?
File src/drivers/ipmi/Kconfig:
https://review.coreboot.org/c/coreboot/+/67057/comment/d237785a_0c27c694?us… :
PS33, Line 85: for implementation
*an* implementation?
or
> … for implementations using polling.
File src/drivers/ipmi/ipmi_bt_ops.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/1d91adfd_d628ad3a?us… :
PS33, Line 103: printk(BIOS_ERR, "%s: Unsupported device type\n",
Print both values?
https://review.coreboot.org/c/coreboot/+/67057/comment/2351f63b_23c414b8?us… :
PS33, Line 106: printk(BIOS_ERR, "%s: Base address needs to be aligned to 4\n",
Print the address?
--
To view, visit https://review.coreboot.org/c/coreboot/+/67057?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Idb67972d1c38bbae04c7b4de3405350c229a05b9
Gerrit-Change-Number: 67057
Gerrit-PatchSet: 33
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 05 Aug 2024 20:32:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No