coreboot-gerrit June 2019

coreboot-gerrit@coreboot.org

1 participants
1454 discussions

Change in ...coreboot[master]: ec/google/wilco: Read back from EC RAM after S0ix entry
by Duncan Laurie (Code Review) 13 Jun '19

13 Jun '19

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33455 Change subject: ec/google/wilco: Read back from EC RAM after S0ix entry ...................................................................... ec/google/wilco: Read back from EC RAM after S0ix entry We are seeing an EC interrupt after setting the EC RAM offset that indicates that the EC should transition to S0ix mode and this is preventing the kernel from going into S0ix on the first try. As a workaround if we read back from the EC RAM while still in the _DSM handler it seems to prevent this problem. BUG=b:130644677 BRANCH=sarien TEST=ensure s0ix entry works on the first try with sarien Change-Id: Id607c4c2b14b79d0cd1bcea0c2032be2f2c0c141 Signed-off-by: Duncan Laurie <dlaurie(a)google.com> --- M src/ec/google/wilco/acpi/ec.asl 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33455/1 diff --git a/src/ec/google/wilco/acpi/ec.asl b/src/ec/google/wilco/acpi/ec.asl index 4fa887b..532f421 100644 --- a/src/ec/google/wilco/acpi/ec.asl +++ b/src/ec/google/wilco/acpi/ec.asl @@ -154,6 +154,12 @@ If (Arg0) { Printf ("EC Enter S0ix") W (CSEX, One) + + /* + * Read back from EC RAM after enabling S0ix + * to prevent EC from aborting S0ix entry. + */ + R (EVT1) } Else { Printf ("EC Exit S0ix") W (CSEX, Zero) -- To view, visit https://review.coreboot.org/c/coreboot/+/33455 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id607c4c2b14b79d0cd1bcea0c2032be2f2c0c141 Gerrit-Change-Number: 33455 Gerrit-PatchSet: 1 Gerrit-Owner: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: mb/google/sarien: Disable unused GPIOs
by Duncan Laurie (Code Review) 13 Jun '19

13 Jun '19

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33450 Change subject: mb/google/sarien: Disable unused GPIOs ...................................................................... mb/google/sarien: Disable unused GPIOs These 4 GPIOs are being disconnected in the next board so use the board ID to configure these pins as not connected to ensure they do not cause leakage. Also remove the ACPI _PTS S5 code that was configuring the GPIOs. This does mean they will cause small leakage in S5 on existing boards, but it will not affect the new boards. BUG=b:132393441 TEST=boot on sarien with fake board ID and ensure that coreboot configures these pads as expected. Change-Id: I6ac04b9a635829811a09aeab7cba3bb58cfcff47 Signed-off-by: Duncan Laurie <dlaurie(a)google.com> --- M src/mainboard/google/sarien/ramstage.c M src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl M src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 12 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/33450/1 diff --git a/src/mainboard/google/sarien/ramstage.c b/src/mainboard/google/sarien/ramstage.c index 1d22046..811afa1 100644 --- a/src/mainboard/google/sarien/ramstage.c +++ b/src/mainboard/google/sarien/ramstage.c @@ -14,6 +14,7 @@ */ #include <arch/acpi.h> +#include <boardid.h> #include <drivers/vpd/vpd.h> #include <smbios.h> #include <soc/gpio.h> @@ -60,6 +61,13 @@ } #endif +static const struct pad_config gpio_unused[] = { +/* SUSWARN# */ PAD_NC(GPP_A13, NONE), +/* SUSACK# */ PAD_NC(GPP_A15, NONE), +/* M2_SKT2_CFG0 */ PAD_NC(GPP_H12, NONE), +/* M2_SKT2_CFG1 */ PAD_NC(GPP_H13, NONE), +}; + void mainboard_silicon_init_params(FSP_S_CONFIG *params) { const struct pad_config *gpio_table; @@ -67,6 +75,10 @@ gpio_table = variant_gpio_table(&num_gpios); cnl_configure_pads(gpio_table, num_gpios); + + /* Disable unused pads for devices with board ID > 2 */ + if (board_id() > 2) + gpio_configure_pads(gpio_unused, ARRAY_SIZE(gpio_unused)); } static void mainboard_enable(struct device *dev) diff --git a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl index 4b05ba8..41121d2 100644 --- a/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl +++ b/src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl @@ -15,8 +15,6 @@ #define CAM_EN GPP_B11 /* Active low */ #define TS_PD GPP_E7 -#define SSD_EN GPP_H13 -#define SSD_RST GPP_H12 /* Method called from LPIT prior to enter s0ix state */ Method (MS0X, 1) @@ -37,13 +35,6 @@ /* Clear touch screen pd pin to avoid leakage */ \_SB.PCI0.CTXS (TS_PD) - - /* Clear SSD EN adn RST pin to avoid leakage */ - If (Arg0 == 5) { - \_SB.PCI0.CTXS (SSD_RST) - Sleep(1) - \_SB.PCI0.CTXS (SSD_EN) - } } /* Method called from _WAK prior to wakeup */ diff --git a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl index 4b05ba8..41121d2 100644 --- a/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl +++ b/src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl @@ -15,8 +15,6 @@ #define CAM_EN GPP_B11 /* Active low */ #define TS_PD GPP_E7 -#define SSD_EN GPP_H13 -#define SSD_RST GPP_H12 /* Method called from LPIT prior to enter s0ix state */ Method (MS0X, 1) @@ -37,13 +35,6 @@ /* Clear touch screen pd pin to avoid leakage */ \_SB.PCI0.CTXS (TS_PD) - - /* Clear SSD EN adn RST pin to avoid leakage */ - If (Arg0 == 5) { - \_SB.PCI0.CTXS (SSD_RST) - Sleep(1) - \_SB.PCI0.CTXS (SSD_EN) - } } /* Method called from _WAK prior to wakeup */ -- To view, visit https://review.coreboot.org/c/coreboot/+/33450 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6ac04b9a635829811a09aeab7cba3bb58cfcff47 Gerrit-Change-Number: 33450 Gerrit-PatchSet: 1 Gerrit-Owner: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: mb/asrock/h110m: Unselect incorrect Kconfig symbol
by Angel Pons (Code Review) 13 Jun '19

13 Jun '19

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33439 ) Change subject: mb/asrock/h110m: Unselect incorrect Kconfig symbol ...................................................................... Patch Set 1: > Patch Set 1: -Code-Review > > > AFAIUI, this is writing the value 0x40 to the global register 0x2A, which sets bit7=0 and bit6=1. > > > > For the NCT6776, this selects the COMA function on all the COMA pins. > > > > For the NCT6791, this selects the COMA function on pin 29, and selects the GPIO8 function on pins 30-36. > > > > Guess what happens to serial. > > Ok, that does indeed sound wrong, so this patch probably won't break things that aren't already broken. Would probably be good to ping the author of that board port to get things sorted out. @Maxim Polyakov, any comments? -- To view, visit https://review.coreboot.org/c/coreboot/+/33439 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f3fde161844f919b070f2b6ce7e106411439a9a Gerrit-Change-Number: 33439 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 13 Jun 2019 20:34:20 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: mb/asrock/h110m: Unselect incorrect Kconfig symbol
by Felix Held (Code Review) 13 Jun '19

13 Jun '19

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33439 ) Change subject: mb/asrock/h110m: Unselect incorrect Kconfig symbol ...................................................................... Patch Set 1: -Code-Review > AFAIUI, this is writing the value 0x40 to the global register 0x2A, which sets bit7=0 and bit6=1. > > For the NCT6776, this selects the COMA function on all the COMA pins. > > For the NCT6791, this selects the COMA function on pin 29, and selects the GPIO8 function on pins 30-36. > > Guess what happens to serial. Ok, that does indeed sound wrong, so this patch probably won't break things that aren't already broken. Would probably be good to ping the author of that board port to get things sorted out. -- To view, visit https://review.coreboot.org/c/coreboot/+/33439 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f3fde161844f919b070f2b6ce7e106411439a9a Gerrit-Change-Number: 33439 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 13 Jun 2019 20:25:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: lint/clang-format: set to 96 chars per line
by Martin Roth (Code Review) 13 Jun '19

13 Jun '19

Hello Patrick Georgi, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/33405 to review the following change. Change subject: lint/clang-format: set to 96 chars per line ...................................................................... lint/clang-format: set to 96 chars per line coreboot has decided to go with 96 characters per line. Original Signed-off-by: Patrick Georgi <pgeorgi(a)google.com> Originnal Reviewed-on: https://review.coreboot.org/c/coreboot/+/31651 Change-Id: I9c99e5cca6548e23cf755dc37193ff2aa669ac10 Signed-off-by: Martin Roth <martinroth(a)google.com> --- M .clang-format M util/lint/lint-007-checkpatch 2 files changed, 5 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/33405/1 diff --git a/.clang-format b/.clang-format index d853f50..5c8aa3c 100644 --- a/.clang-format +++ b/.clang-format @@ -7,7 +7,7 @@ IndentCaseLabels: false SortIncludes: false ContinuationIndentWidth: 8 -ColumnLimit: 0 +ColumnLimit: 96 AlwaysBreakBeforeMultilineStrings: true AllowShortLoopsOnASingleLine: false AllowShortFunctionsOnASingleLine: false diff --git a/util/lint/lint-007-checkpatch b/util/lint/lint-007-checkpatch index afa593e..a7b63e8 100755 --- a/util/lint/lint-007-checkpatch +++ b/util/lint/lint-007-checkpatch @@ -28,6 +28,8 @@ ^src/vendorcode\|\ ^Documentation" +opts="--max-line-length 96" + # default: test src and util if [ "$1" = "" ]; then INCLUDED_DIRS="src util" @@ -35,7 +37,7 @@ elif [ "$1" = "diff" ]; then args=$( echo $EXCLUDED_DIRS | \ sed -e 's,\\|, ,g' -e 's,\^,--exclude=,g' ) - util/lint/checkpatch.pl --quiet --no-signoff $args - + util/lint/checkpatch.pl --quiet --no-signoff $opts $args - exit $? # Space separated list of directories to test else @@ -49,5 +51,5 @@ grep -v $EXCLUDED_DIRS ) for FILE in $FILELIST; do - util/lint/checkpatch.pl --show-types --file --quiet "$FILE" + util/lint/checkpatch.pl --show-types --file --quiet $opts "$FILE" done -- To view, visit https://review.coreboot.org/c/coreboot/+/33405 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9c99e5cca6548e23cf755dc37193ff2aa669ac10 Gerrit-Change-Number: 33405 Gerrit-PatchSet: 1 Gerrit-Owner: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

4 5

Change in ...coreboot[master]: nb/amd/amdfam10: die() on out of bounds reads
by Jacob Garber (Code Review) 13 Jun '19

13 Jun '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33404 Change subject: nb/amd/amdfam10: die() on out of bounds reads ...................................................................... nb/amd/amdfam10: die() on out of bounds reads These two functions try to access arrays of lengths 32 and 64 at indices of at most 259 and 71 (respectively). Something here is seriously wrong. This code was introduced in 2007, and aside from cosmetic changes, has had no modifications since then. I don't know what this code is supposed to do, and asking around on IRC, no one else did either. Until someone has the interest and time to work on it, let's at least add a die() to prevent the out of bounds access and alert the user that something is wrong. Change-Id: I5fc15a50a9f0e97add31e3a40da82a15f7427358 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 12296{79-82} --- M src/northbridge/amd/amdfam10/ht_config.c 1 file changed, 8 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/33404/1 diff --git a/src/northbridge/amd/amdfam10/ht_config.c b/src/northbridge/amd/amdfam10/ht_config.c index 4810b99..1742fcd 100644 --- a/src/northbridge/amd/amdfam10/ht_config.c +++ b/src/northbridge/amd/amdfam10/ht_config.c @@ -127,6 +127,10 @@ u32 index; for (index = 0; index < 256; index++) { + + if (index + 4 >= ARRAY_SIZE(sysconf.conf_io_addrx)) + die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); + if (sysconf.conf_io_addrx[index+4] == 0) { sysconf.conf_io_addr[index+4] = (nodeid & 0x3f); sysconf.conf_io_addrx[index+4] = 1 | ((linkn & 0x7)<<4); @@ -142,6 +146,10 @@ u32 index; for (index = 0; index < 64; index++) { + + if (index + 8 >= ARRAY_SIZE(sysconf.conf_mmio_addrx)) + die("Error! Out of bounds read in %s:%s\n", __FILE__, __func__); + if (sysconf.conf_mmio_addrx[index+8] == 0) { sysconf.conf_mmio_addr[index+8] = (nodeid & 0x3f); sysconf.conf_mmio_addrx[index+8] = 1 | ((linkn & 0x7)<<4); -- To view, visit https://review.coreboot.org/c/coreboot/+/33404 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5fc15a50a9f0e97add31e3a40da82a15f7427358 Gerrit-Change-Number: 33404 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

4 5

Change in ...coreboot[master]: Documentation/soc/amd: Add picasso
by Marshall Dawson (Code Review) 13 Jun '19

13 Jun '19

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33399 Change subject: Documentation/soc/amd: Add picasso ...................................................................... Documentation/soc/amd: Add picasso Begin a directory for AMD soc devices and add an explanation of how picasso, the first Family 17h offering in coreboot, works. Family 17h represents a unique paradigm for initializing x86 processors due to the work offloaded to the PSP, and the system state at the time of x86 first fetch. Change-Id: I7bd8649996add80747f6a60b9dfd35a94a560be1 Signed-off-by: Marshall Dawson <marshalldawson3rd(a)gmail.com> --- A Documentation/soc/amd/index.md A Documentation/soc/amd/picasso/family17h.md M Documentation/soc/index.md 3 files changed, 131 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/33399/1 diff --git a/Documentation/soc/amd/index.md b/Documentation/soc/amd/index.md new file mode 100644 index 0000000..6f42d87 --- /dev/null +++ b/Documentation/soc/amd/index.md @@ -0,0 +1,7 @@ +# AMD SOC-specific documentation + +This section contains documentation about coreboot on specific AMD SOCs. + +## Platforms + +- [Picasso/Family 17h](picasso/family17h.md) diff --git a/Documentation/soc/amd/picasso/family17h.md b/Documentation/soc/amd/picasso/family17h.md new file mode 100755 index 0000000..6df3ae3 --- /dev/null +++ b/Documentation/soc/amd/picasso/family17h.md @@ -0,0 +1,123 @@ +# AMD Family 17h in coreboot + +## Abstract + +Beginning with Family 17h products (a.k.a. “Zen” cores), AMD changed their paradigm for initializing the system and this requires major modifications to the execution flow of coreboot. This file discusses the new boot flow, and challenges, and the tradeoffs of the initial port into coreboot. + +## Introduction + +Family 17h products are x86-based designs. This documentation assumes familiarity with x86, its reset state and its early initialization requirements. + +To the extent necessary, the role of the Platform Security Processor (a.k.a. PSP) in system initialization is addressed here. AMD has historically required an NDA for access to the PSP specification1. coreboot relies on util/amdfwtool to build the structures and add various other firmware to the final image. The Family 17h PSP design guide adds a new BIOS Directory Table, similar to the PSP Directory Table. + +Support in coreboot for modern AMD products is based on AMD’s reference code: AMD Generic Encapsulated Software Architecture (AGESATM). AGESA contains the technology for enabling DRAM, configuring proprietary core logic, assistance with generating ACPI tables, and other features. + +AGESA for products earlier than Family 17h is known as v5 or Arch20082. Also note that coreboot currently contains both open source AGESA and closed source implementations (binaryPI) compiled from AGESA. + +The first AMD Family 17h device ported to coreboot is codenamed “Picasso”3, and will be added to soc/amd/picasso. + +## Additional Definitions + +* PSP, Platform Security Processor: Onboard ARM processor that runs alongside the main x86 processor; may be viewed as analogous to the IntelR Management Engine +* FCH, Fusion Control Hub, the logical southbridge within the SOC +* ABL - AGESA Bootloader - Processor initialization code that runs on the PSP +* PSP Directory Table - A structured list of pointers to PSP firmware and other controller binaries +* BIOS Directory Table - A structured list of pointers to BIOS related firmware images +* Embedded Firmware Structure - Signature and pointers used by the PSP to locate the PSP Directory Table and BIOS Directory Table; these items are generated during coreboot build and are located in the SPI ROM +* Verstage - The code to verify the firmware contained in the writable section of the SPI ROM +* APCB - AMD PSP Customization Block - A binary containing PSP and system configuration preferences (analogous to v5 BUILDOPT_ options), and generated by APCBTool to be added to coreboot/utils later +* APOB - AGESA PSP Output Buffer - A buffer in main memory for storing AGESA BootLoader output. There are no plans for this to be parsed by coreboot + +## Problem Statements + +AMD has ported early AGESA features to the PSP, which now discovers, enables and trains DRAM. Unlike any other x86 device in coreboot, a Picasso system has DRAM online prior to the first instruction fetch. + +Cache-as-RAM (CAR) is no longer a supportable feature in AMD hardware. Early code expecting CAR behavior must account for writes escaping the L2 cache and going to DRAM. + +Without any practical need for CAR, or DRAM initialization, coreboot should arguably skip bootblock and romstage, and possibly use ramstage as the BIOS image. This approach presents a number of challenges: + +* At the entry of ramstage, x86 processors are in flat protected mode. Picasso’s initial state is nearly identical to any other x86 at reset, except its CS shadow register’s base and limit put its execution within DRAM, not at 0xfffffff0. Picasso requires initial programming and entry into protected mode prior to ramstage. +* coreboot expects cbmem initialization during romstage. + +AGESA supporting Picasso is now at v9. Unlike Arch2008, which defines granular entry points for easy inclusion to a legacy BIOS, v9 is rewritten for compilation into a UEFI. The source follows UEFI standards, i.e. assumes the presence of UEFI phases, implements dependency expressions, much functionality is rewritten as libraries, etc. It would, in no way, fit into the v5 model used in coreboot. + +* For the foreseeable future, AGESA source will distributed only under NDA. + +## Basic Pre-x86 Boot Flow + +The following steps occur prior to x86 processor operation. + +* System power on +* PSP executes immutable on-chip boot ROM +* PSP locates the Embedded Firmware Table and PSP Directory Table in the SPI ROM +* PSP verifies and executes the PSP off-chip bootloader +* ChromeOS systems: + * Off-chip bootloader attempts to locate verstage via the RO BIOS Directory Table + * If verstage is not found, booting continues with ABLs below + * Verstage initializes, setting up GPIOs, UART if needed, communication path to the EC, and the SPI controller for direct access to the flash device. + * Verstage verifies the RW sections (as is typically performed by the main processor) + * Verstage locates the Embedded Firmware Directory within the verified FMAP section and passes a pointer to the PSP bootloader. If the verification fails, it passes a pointer to the RO header to the bootloader. +* PSP parses the PSP Directory Table to find the ABLs and executes them +* An ABL parses the APCB for system configuration preferences +* An ABL initializes system main memory, locates the compressed BIOS image in the SPI ROM, and decompresses it into DRAM +* An ABL writes the APOB to DRAM for consumption by the x86-based AGESA +* PSP releases the x86 processor from reset. The x86 core fetches and executes instructions from the reset vector + +## Picasso Reset Vector and First Instructions + +As mentioned above, prior to releasing the x86 main core from reset, the PSP decompresses a BIOS image into DRAM. The PSP uses a specific BIOS Directory Table entry type to determine the source address (in flash), the destination address (in DRAM), and the destination size. The decompressed image is at the top of the destination region. The PSP then + +Calculates the x86 reset vector as + + reset_vector = dest_addr + dest_size - 0x10 + +Sets x86 CS descriptor shadow register to + + base = dest_addr + dest_size - 0x10000 + limit = 0xffff + +Like all x86 devices, the main core is allowed to begin executing instructions with + + CS:IP = 0xf000:0xfff0 + +For example, assume the BIOS Directory Table indicates + + destination = 0x9b00000 + size = 0x300000 + +… then the BIOS image is placed at the topmost position the region 0x9b00000-0x9dfffff and + + reset_vector = 0x9dffff0 + CS_shdw_base = 0x9df0000 + CS:IP = 0xf000:0xfff0 + +Although the x86 behaves as though it began executing at 0xfffffff0 i.e. 0xf000:0xfff0, the initial GDT load must use the physical address of the table and not the typical CS-centric address. And, the first jump to protected mode must jump to the physical address in DRAM. Any code that is position-dependent must be linked to run at the final destination. + +## Initial coreboot Implementation + +Supporting Picasso doesn’t fit well with many of the coreboot assumptions. Initial porting shall attempt to fit within existing coreboot paradigms and make minimal changes to common code. + +### CAR and bootblock + +The coreboot bootblock contains features Picasso doesn’t require or can’t use, and is assumed to execute in an unusable location. Picasso’s requirement for bootblock in coreboot will be eliminated. + +### Hybrid romstage + +Picasso’s x86 reset state doesn’t meet the coreboot expectations for jumping directly to ramstage. The primary feature of romstage is also not needed, however there are other important features that are typically in romstage that Picasso does need. + +The romstage architecture is designed around the presence of CAR. Several features implement ROMSTAGE_CBMEM_INIT_HOOK, expecting to move data from CAR to cbmem. The hybrid romstage consumes DRAM for the purpose of implementing the expected CAR storage. This region as well as the DRAM where romstage is decompressed must be reserved and unavailable to the OS. + +The initial Picasso port implements a hybrid romstage that contains the first instruction fetched at the reset vector. It minimally configures flat protected mode, initializes cbmem, then loads the next stage. Future work will consider breaking the dependencies mentioned above and/or potentially loading ramstage directly from the PSP. + +## AGESA v9 on Picasso + +Due to the current inability to publish AGESA source, a pre-built binary solution remains a requirement. The rewrite from v5 to v9 for direct inclusion into UEFI source makes modifying it for conforming to the existing v5 interface impractical. + +Given the UEFI nature of modern AGESA, and the existing open source work from Intel, Picasso shall support AGESA via an FSP-like prebuilt image. The Intel Firmware Support Package4 combines reference code with EDK II source to create a modular image with discoverable entry points. coreboot source already contains knowledge of FSP, how to parse it, integrate it, and how to communicate with it. + +## Footnotes + +1. “AMD Platform Security Processor BIOS Architecture Design Guide for AMD Family 17h Processors” (PID #55758) and “AMD Platform Security Processor BIOS Architecture Design Guide” (PID #54267) for earlier products +2. [https://www.amd.com/system/files/TechDocs/44065_Arch2008.pdf](https://www.a… +3. [https://en.wikichip.org/wiki/amd/cores/picasso](https://en.wikichip.org/wik… +4. [https://www.intel.com/content/www/us/en/intelligent-systems/intel-firmware-… diff --git a/Documentation/soc/index.md b/Documentation/soc/index.md index 04e2b2f..fccddbd 100644 --- a/Documentation/soc/index.md +++ b/Documentation/soc/index.md @@ -4,5 +4,6 @@ ## Vendor +- [AMD](amd/index.md) - [Cavium](cavium/index.md) - [Intel](intel/index.md) -- To view, visit https://review.coreboot.org/c/coreboot/+/33399 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7bd8649996add80747f6a60b9dfd35a94a560be1 Gerrit-Change-Number: 33399 Gerrit-PatchSet: 1 Gerrit-Owner: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-MessageType: newchange

2 6

Change in ...coreboot[master]: util/amdfwtool: Align PSP NVRAM
by Marshall Dawson (Code Review) 13 Jun '19

13 Jun '19

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33397 Change subject: util/amdfwtool: Align PSP NVRAM ...................................................................... util/amdfwtool: Align PSP NVRAM Align the PSP's NVRAM item since it's intended to be updateable in the flash device. Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Signed-off-by: Marshall Dawson <marshalldawson3rd(a)gmail.com> --- M util/amdfwtool/amdfwtool.c 1 file changed, 26 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/33397/1 diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 51e913c..f2ce34c 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -79,6 +79,7 @@ #define ALIGN(val, by) (((val) + (by) - 1) & ~((by) - 1)) #define TABLE_ALIGNMENT 0x1000U #define BLOB_ALIGNMENT 0x100U +#define ERASE_ALIGNMENT 0x1000U #define EMBEDDED_FW_SIGNATURE 0x55aa55aa #define PSP_COOKIE 0x50535024 /* 'PSP$' */ @@ -457,6 +458,31 @@ else pspdir->entries[count].addr = 1; count++; + } else if (fw_table[i].type == AMD_FW_PSP_NVRAM) { + if (fw_table[i].filename == NULL) + continue; + /* TODO: Add a way to reserve for NVRAM without + * requiring a filename. This isn't a feature used + * by coreboot systems, so priority is very low. + */ + ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT); + bytes = copy_blob(BUFF_CURRENT(*ctx), + fw_table[i].filename, BUFF_ROOM(*ctx)); + if (bytes <= 0) { + free(ctx->rom); + exit(1); + } + + pspdir->entries[count].type = fw_table[i].type; + pspdir->entries[count].subprog = fw_table[i].subprog; + pspdir->entries[count].rsvd = 0; + pspdir->entries[count].size = (uint32_t)bytes; + pspdir->entries[count].addr = RUN_CURRENT(*ctx); + + ctx->current = ALIGN(ctx->current + bytes, + BLOB_ALIGNMENT); + ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT); + count++; } else if (fw_table[i].filename != NULL) { bytes = copy_blob(BUFF_CURRENT(*ctx), fw_table[i].filename, BUFF_ROOM(*ctx)); -- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 1 Gerrit-Owner: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-MessageType: newchange

3 7

Change in ...coreboot[master]: util/amdfwtool: Add argument for soft fuse override
by Marshall Dawson (Code Review) 13 Jun '19

13 Jun '19

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33396 Change subject: util/amdfwtool: Add argument for soft fuse override ...................................................................... util/amdfwtool: Add argument for soft fuse override Allow the soc build to pass a soft fuse value to the utility. This helps maintain compatibility across PSP generations. Add a generic 'other' item to the amd_fw_entry structure that may be used by non-fuse entries in the future. TEST=Verify google/grunt amdfw.rom unchanged before and after. Compare internal board using override before and after. Change-Id: I26223f0b42ad28c43d9bd87419a2a8f719ee91cb Signed-off-by: Marshall Dawson <marshalldawson3rd(a)gmail.com> --- M util/amdfwtool/amdfwtool.c 1 file changed, 26 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/33396/1 diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 4a13b29..51e913c 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -184,6 +184,7 @@ printf("-u | --trustletkey <FILE> Add trustletkey\n"); printf("-w | --smufirmware2 <FILE> Add smufirmware2\n"); printf("-m | --smuscs <FILE> Add smuscs\n"); + printf("-T | --soft-fuse <HEX_VAL> Override default soft fuse values\n"); printf("\n-o | --output <filename> output filename\n"); printf("-f | --flashsize <HEX_VAL> ROM size in bytes\n"); printf(" size must be larger than %dKB\n", @@ -218,6 +219,7 @@ amd_fw_type type; uint8_t subprog; char *filename; + uint64_t other; } amd_fw_entry; static amd_fw_entry amd_psp_fw_table[] = { @@ -450,7 +452,10 @@ pspdir->entries[count].subprog = fw_table[i].subprog; pspdir->entries[count].rsvd = 0; pspdir->entries[count].size = 0xFFFFFFFF; - pspdir->entries[count].addr = 1; + if (fw_table[i].other) + pspdir->entries[count].addr = fw_table[i].other; + else + pspdir->entries[count].addr = 1; count++; } else if (fw_table[i].filename != NULL) { bytes = copy_blob(BUFF_CURRENT(*ctx), @@ -483,7 +488,7 @@ fill_dir_header(pspdir, count, PSP_COOKIE); } -static const char *optstring = "x:i:g:AS:p:b:s:r:k:c:n:d:t:u:w:m:o:f:l:h"; +static const char *optstring = "x:i:g:AS:p:b:s:r:k:c:n:d:t:u:w:m:T:o:f:l:h"; static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -504,6 +509,7 @@ {"trustletkey", required_argument, 0, 'u' }, {"smufirmware2", required_argument, 0, 'w' }, {"smuscs", required_argument, 0, 'm' }, + {"soft-fuse", required_argument, 0, 'T' }, {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, {"location", required_argument, 0, 'l' }, @@ -512,6 +518,20 @@ {NULL, 0, 0, 0 } }; +static void register_fw_fuse(char *str) +{ + int i; + char *tmp; + + for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { + if (amd_psp_fw_table[i].type != AMD_PSP_FUSE_CHAIN) + continue; + + amd_psp_fw_table[i].other = strtoull(str, &tmp, 16); + return; + } +} + static void register_fw_filename(amd_fw_type type, uint8_t sub, char filename[]) { unsigned int i; @@ -635,6 +655,10 @@ register_fw_filename(AMD_FW_PSP_SMUSCS, sub, optarg); sub = 0; break; + case 'T': + register_fw_fuse(optarg); + sub = 0; + break; case 'o': output = optarg; break; -- To view, visit https://review.coreboot.org/c/coreboot/+/33396 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I26223f0b42ad28c43d9bd87419a2a8f719ee91cb Gerrit-Change-Number: 33396 Gerrit-PatchSet: 1 Gerrit-Owner: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-MessageType: newchange

4 10

Change in ...coreboot[master]: mb/asrock/h110m: Unselect incorrect Kconfig symbol
by Angel Pons (Code Review) 13 Jun '19

13 Jun '19

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33439 ) Change subject: mb/asrock/h110m: Unselect incorrect Kconfig symbol ...................................................................... Patch Set 1: > Patch Set 1: Code-Review-1 > > this changes behaviour in src/superio/nuvoton/common/early_serial.c:67 The behavior change is intended. It does not make sense for the NCT6791D. On both the NCT6776 and the NCT6791: - The global configuration register 0x2A defaults to 0xC0. - Pins 29-36 are muxed COMA/GPIO8: 0 = COMA; 1 = GPIO8. On the NCT6776: - CR 0x2A bit7 muxes pins 29-36. On the NCT6791: - CR 0x2A bit7 muxes pin 29. - CR 0x2A bit6 muxes pins 30-36. The relevant code fragment on nuvoton/common/early_serial.c: if (CONFIG(SUPERIO_NUVOTON_NCT6776_COM_A)) /* Route GPIO8 pin group to COM A */ pnp_write_config(dev, 0x2a, 0x40); AFAIUI, this is writing the value 0x40 to the global register 0x2A, which sets bit7=0 and bit6=1. For the NCT6776, this selects the COMA function on all the COMA pins. For the NCT6791, this selects the COMA function on pin 29, and selects the GPIO8 function on pins 30-36. Guess what happens to serial. -- To view, visit https://review.coreboot.org/c/coreboot/+/33439 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f3fde161844f919b070f2b6ce7e106411439a9a Gerrit-Change-Number: 33439 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 13 Jun 2019 15:05:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2019