coreboot-gerrit September 2022

coreboot-gerrit@coreboot.org

1 participants
4678 discussions

[M] Change in coreboot[master]: device: Clear lane error status
by Tim Wawrzynczak (Code Review) 06 Sep '22

06 Sep '22

Attention is currently required from: Wilson Chou, Marc Jones, Nico Huber, Jonathan Zhang, Ryback Hung, Johnny Lin, Paul Menzel, Shuming Chu (Shuming). Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67264 ) Change subject: device: Clear lane error status ...................................................................... Patch Set 5: Code-Review+1 (1 comment) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/67264/comment/4e384909_13168275 PS5, Line 551: printk(BIOS_DEBUG, "%s: Clear Lane Error Status.\n", dev_path(dev)); Would anyone ever be interested in the value it may have had before it gets cleared? -- To view, visit https://review.coreboot.org/c/coreboot/+/67264 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6344223636409d8fc25e365a6375fc81e69f41a5 Gerrit-Change-Number: 67264 Gerrit-PatchSet: 5 Gerrit-Owner: Wilson Chou <wilson.chou%quantatw.com(a)gtempaccount.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Ryback Hung <ryback.hung(a)quantatw.com> Gerrit-Reviewer: Shuming Chu (Shuming) <s1218944(a)gmail.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: Wilson Chou <wilson.chou%quantatw.com(a)gtempaccount.com> Gerrit-Attention: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Ryback Hung <ryback.hung(a)quantatw.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Sep 2022 21:15:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: mb/google/brya/var/skolas4es: Configure _DSC for camera devices
by Bora Guvendik (Code Review) 06 Sep '22

06 Sep '22

Attention is currently required from: Selma Bensaid, Nick Vaccaro. Hello build bot (Jenkins), Selma Bensaid, Tim Wawrzynczak, Nick Vaccaro, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/67021 to look at the new patch set (#5). Change subject: mb/google/brya/var/skolas4es: Configure _DSC for camera devices ...................................................................... mb/google/brya/var/skolas4es: Configure _DSC for camera devices Configure _DSC to ACPI_DEVICE_SLEEP_D3_COLD so that the driver skips initial probe during kernel boot and prevent privacy LED blink. BUG=b:194979741 BRANCH=firmware-brya-14505.B TEST=Build and boot skolas to OS. Verify entries in SSDT. Signed-off-by: Bora Guvendik <bora.guvendik(a)intel.com> Change-Id: I3c32dd71ab454227b15913bda7f542230e5568db --- M src/mainboard/google/brya/variants/skolas4es/overridetree.cb 1 file changed, 48 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/67021/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/67021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c32dd71ab454227b15913bda7f542230e5568db Gerrit-Change-Number: 67021 Gerrit-PatchSet: 5 Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: Documentation: document the new smbus console feature
by Ahamed Husni (Code Review) 06 Sep '22

06 Sep '22

Ahamed Husni has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/67386 ) Change subject: Documentation: document the new smbus console feature ...................................................................... Documentation: document the new smbus console feature Signed-off-by: Husni Faiz <ahamedhusni73(a)gmail.com> Change-Id: I50cafbbaaea133c9ea50131e455151287c96176a --- A Documentation/technotes/console.md M Documentation/technotes/index.md 2 files changed, 70 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/67386/1 diff --git a/Documentation/technotes/console.md b/Documentation/technotes/console.md new file mode 100644 index 0000000..9a84b5f --- /dev/null +++ b/Documentation/technotes/console.md @@ -0,0 +1,59 @@ +# coreboot Console + +coreboot supports multiple ways to access its console. +https://www.coreboot.org/Console_and_outputs + + +## SMBus Console + +SMBus is a two-wire interface which is based on the principles of operation +of I2C. SMBus, was first was designed to allow a battery to communicate with the +charger, the system host, and/or other power-related components in the system. + +Enable the SMBus console with `CONSOLE_I2C_SMBUS` Kconfig. Set +`CONSOLE_I2C_SMBUS_SLAVE_ADDRESS` and `CONSOLE_I2C_SMBUS_SLAVE_DATA_REGISTER` +configuration values of the slave I2C device which you will use to capture +I2C packets. + +Currently SMBus console is supported in `BOOTBLOCK`, `ROMSTAGE` and `RAMSTAGE`. + +Modern computer Random Access Memory (RAM) slot has SMBus in it according to +the JEDEC standards. We can use a breakoutboard to expose those SMBus pins. +Some mainboard have SMBus pins in the PCIe slot as well. + +This feature has been tested on the following platforms: +```eval_rst ++------------------------------------+-------------------------------+ +| Platform | Support | ++====================================+===============================+ +| GA-H61M-S2PV + Intel Ivy Bridge | BOOTBLOCK, ROMSTAGE, RAMSTAGE | ++---------------------+----------------------------------------------+ +``` + +A minimal DDR3 DIMM breakout board with only the SDA(Data line) and +SCL(Clock line) pins of I2C/SMBus can be found +[here](https://github.com/drac98/ram-breakout-board). + +NOTE: +To capture the I2C packets, an I2C slave device is required. The easiest way to +capture the log message is to use a I2C to UART converter chip with a UART to +USB converter chip. The setup would be as follows. +```text + +---------+ +-------------+ +-------------+ + + PC +--------+ UART to USB +--------+ I2C to UART | + +---------+ +-------------+ +-------------+ + | | +------------------------------------------------------------+-- System Management +----------------------------------------------------------+---- Bus +``` + +Watch this [video](https://youtu.be/Q0dK41n9db8) to see how it is set up. + +If you are using a `SC16IS750` I2C to UART converter chip, you can enable the +`SC16IS750_INIT` option to initialize the chip. + +If not we can use a Beagleboard or an Arduino as an I2C slave device. + +This feature was added as part of a GSoC 2022 project. Checkout the +[coreboot Console via SMBus — Part I](https://medium.com/@husnifaiz/coreboot-console-via-smbus-introduction-38… +blog post for more details. \ No newline at end of file diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md index fda8bd6..da5b864 100644 --- a/Documentation/technotes/index.md +++ b/Documentation/technotes/index.md @@ -5,3 +5,4 @@ * [Unit testing coreboot](2020-03-unit-testing-coreboot.md) * [Unit Test Code Coverage](2021-05-code-coverage.md) * [Address Sanitizer](asan.md) +* [coreboot Consoles](console.md) -- To view, visit https://review.coreboot.org/c/coreboot/+/67386 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I50cafbbaaea133c9ea50131e455151287c96176a Gerrit-Change-Number: 67386 Gerrit-PatchSet: 1 Gerrit-Owner: Ahamed Husni <ahamedhusni73(a)gmail.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: drivers/smbus: smbus console driver
by Ahamed Husni (Code Review) 06 Sep '22

06 Sep '22

Attention is currently required from: Felix Singer, Ahamed Husni, Tim Wawrzynczak. Hello Felix Singer, build bot (Jenkins), Raul Rangel, Tim Wawrzynczak, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/67339 to look at the new patch set (#2). Change subject: drivers/smbus: smbus console driver ...................................................................... drivers/smbus: smbus console driver Change-Id: Ife77fb2c3e1cc77678a4972701317d50624ceb95 Signed-off-by: Husni Faiz <ahamedhusni73(a)gmail.com> --- M src/console/Kconfig A src/drivers/smbus/i2c_smbus_console.c A src/include/console/i2c_smbus.h 3 files changed, 84 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/67339/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/67339 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ife77fb2c3e1cc77678a4972701317d50624ceb95 Gerrit-Change-Number: 67339 Gerrit-PatchSet: 2 Gerrit-Owner: Ahamed Husni <ahamedhusni73(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Ahamed Husni <ahamedhusni73(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: Documentation: Add wake source info to device tree documentation
by Raul Rangel (Code Review) 06 Sep '22

06 Sep '22

Raul Rangel has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/67385 ) Change subject: Documentation: Add wake source info to device tree documentation ...................................................................... Documentation: Add wake source info to device tree documentation The device tree documentation was promoting using a GPIO wake event and a GPE wake event. We should only ever have one. This wasn't actually causing a problem because the wake bit was set on the `irq` property, but the IO-APIC can't actually wake the system, so it was a no-op. This change fixes up the markdown so it's formatted correctly, and also adds a section explaining what the different wake configurations are. BUG=b:243700486 TEST=mdformat Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: Ifcdbd5371408784bf9b81c1ade90263de8c60e0f --- M Documentation/getting_started/devicetree.md 1 file changed, 97 insertions(+), 25 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/67385/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/67385 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifcdbd5371408784bf9b81c1ade90263de8c60e0f Gerrit-Change-Number: 67385 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: Documentation: Add wake source info to device tree documentation
by Raul Rangel (Code Review) 06 Sep '22

06 Sep '22

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/67385 ) Change subject: Documentation: Add wake source info to device tree documentation ...................................................................... Documentation: Add wake source info to device tree documentation The device tree documentation was promoting using a GPIO wake event and a GPE wake event. We should only ever have one. This wasn't actually causing a problem because the wake bit was set on the `irq` property, but the IO-APIC can't actually wake the system, so it was a no-op. This change fixes up the markdown so it's formatted correctly, and also adds a section explaining what the different wake configurations are. BUG=b:243700486 TEST=mdformat Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: Ifcdbd5371408784bf9b81c1ade90263de8c60e0f --- M Documentation/getting_started/devicetree.md 1 file changed, 99 insertions(+), 27 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/67385/1 diff --git a/Documentation/getting_started/devicetree.md b/Documentation/getting_started/devicetree.md index 41f5901..09f161b 100644 --- a/Documentation/getting_started/devicetree.md +++ b/Documentation/getting_started/devicetree.md @@ -86,7 +86,7 @@ chip drivers/i2c/generic register "hid" = ""ELAN0000"" register "desc" = ""ELAN Touchpad"" - register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)" + register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_A21_IRQ)" register "wake" = "GPE0_DW0_21" device i2c 15 on end end @@ -116,7 +116,7 @@ I2cSerialBusV2 (0x0015, ControllerInitiated, 400000, AddressingMode7Bit, "\\_SB.PCI0.I2C0", 0x00, ResourceConsumer, , Exclusive, ) - Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, ) + Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000002D, } @@ -131,7 +131,7 @@ } ``` -You can see it generates _HID, _UID, _DDN, _STA, _CRS, _S0W, and _PRW +You can see it generates \_HID, \_UID, \_DDN, \_STA, \_CRS, \_S0W, and \_PRW names/methods in the Device's scope. ## Utilizing a device driver @@ -165,7 +165,7 @@ register "hid" = ""ELAN0000"" ``` -This corresponds to **const char *hid** in the struct. In the ACPI ASL, it +This corresponds to **const char \*hid** in the struct. In the ACPI ASL, it translates to: ``` @@ -181,18 +181,18 @@ register "desc" = ""ELAN Touchpad"" ``` -corresponds to **const char *desc** and in ASL: +corresponds to **const char \*desc** and in ASL: ``` Name (_DDN, "ELAN Touchpad") // _DDN: DOS Device Name ``` -### irq +#### irq It also adds the interrupt, ``` - Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, ) + Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, ) { 0x0000002D, } @@ -201,24 +201,20 @@ which comes from: ``` - register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)" + register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_A21_IRQ)" ``` -The GPIO pin IRQ settings control the "Level", "ActiveLow", and -"ExclusiveAndWake" settings seen above (level means it is a level-triggered -interrupt as opposed to edge-triggered; active low means the interrupt is -triggered when the signal is low). +The GPIO pin IRQ settings control the "Level", and "ActiveLow" settings seen +above (level means it is a level-triggered interrupt as opposed to +edge-triggered; active low means the interrupt is triggered when the signal is +low). -Note that the ACPI_IRQ_WAKE_LEVEL_LOW macro informs the platform that the GPIO -will be routed through SCI (ACPI's System Control Interrupt) for use as a wake -source. Also note that the IRQ names are SoC-specific, and you will need to + +Also note that the IRQ names are SoC-specific, and you will need to find the names in your SoC's header file. The ACPI_* macros are defined in ``src/arch/x86/include/acpi/acpi_device.h``. -Using a GPIO as an IRQ requires that it is configured in coreboot correctly. -This is often done in a mainboard-specific file named ``gpio.c``. - -### wake +#### wake The last register is: @@ -231,6 +227,8 @@ this example. The "21" indicates GPP_X21, where GPP_X is mapped onto DW0 elsewhere in the devicetree. +### device + The last bit of the definition of that device includes: ``` @@ -244,6 +242,57 @@ "Scope" that the device names and methods will live under, in this case "\_SB.PCI0.I2C0". +## Wake sources + +The ACPI spec defines two methods to describe how a device can wake the system. +Only one of these method should be used, otherwise duplicate wake events will be +generated. + +### Using GPEs as a wake source + +The `wake` property specified above is used to tell the ACPI subsystem that the +device can use a GPE to wake the system. The OS can control whether to enable +or disable the wake source by unmasking/masking off the GPE. + +The GPIO -> GPE mapping must be configured in firmware. On AMD platforms this is +generally done by a mainboard specific `gpio.c` file that defines the GPIO +using `PAD_SCI`. The GPIO -> GPE mapping is returned by the +`soc_get_gpio_event_table` method that is defined in the SoC specific `gpio.c` +file. + +The linux kernel has great support for this method. + +Windows on the other hand will +[BSOD](https://github.com/MicrosoftDocs/windows-driver-docs/blob/staging/windows-driver-docs-pr/debugger/bug-check-0xa5--acpi-bios-error.md) +complaining about an invalid ACPI configuration. +> 0x1000D - A device used both GPE and GPIO interrupts, which is not supported. + +## Using GPIO interrupts as a wake source + +The `ACPI_IRQ_WAKE_{EDGE,LEVEL}_{LOW,HIGH}` macros can be used when setting the +`irq` or `gpio_irq` properties. This ends up setting `ExclusiveAndWake` or +`SharedAndWake` on the `Interrupt` or `GpioInt` ACPI resource. + +This method has a few caveats: +* On Intel and AMD platforms the IO-APIC can't wake the system. This means you + can't use the `ACPI_IRQ_WAKE_*` macros with the `irq` property. Instead you + need to use the `gpio_irq` property. +* The OS needs to know how to enable the `wake` bit on the GPIO. For linux this + means the platform specific GPIO controller driver must implement the + `irq_set_wake` callback. For AMD systems this wasn't implemented until + linux v5.15. If the controller doesn't define this callback, it's + possible for the firmware to manually set the `wake` bit on the GPIO. This is + often done in a mainboard-specific file named `gpio.c`. This is not + recommended because then it's not possible for the OS to disable the wake + source. +* As of linux v6.0, the ACPI subsystem doesn't take the GPIO `wake` bit into + account when deciding on which power state to put the device in before + suspending the system. This means that if you define a power resource for a + device via `has_power_resource`, `enable_gpio`, etc, then the linux kernel + will place the device into D3Cold. + +This is the method preferred by Windows since it doesn't result in a BSOD. + ## Other auto-generated names (see [ACPI specification @@ -251,17 +300,19 @@ for more details on ACPI methods) ### _S0W (S0 Device Wake State) -_S0W indicates the deepest S0 sleep state this device can wake itself from, -which in this case is ACPI_DEVICE_SLEEP_D3_HOT, representing _D3hot_. +\_S0W indicates the deepest S0 sleep state this device can wake itself from, +which in this case is `ACPI_DEVICE_SLEEP_D3_HOT`, representing _D3hot_. +D3Hot means the `PR3` power resources are still on and the device is still +responsive on the bus. For i2c devices this is generally the same state as `D0`. -### _PRW (Power Resources for Wake) -_PRW indicates the power resources and events required for wake. There are no +### \_PRW (Power Resources for Wake) +\_PRW indicates the power resources and events required for wake. There are no dependent power resources, but the GPE (GPE0_DW0_21) is mentioned here (0x15), as well as the deepest sleep state supporting waking the system (3), which is S3. -### _STA (Status) -The _STA method is generated automatically, and its values, 0xF, indicates the +### \_STA (Status) +The \_STA method is generated automatically, and its values, 0xF, indicates the following: Bit [0] – Set if the device is present. @@ -269,8 +320,8 @@ Bit [2] – Set if the device should be shown in the UI. Bit [3] – Set if the device is functioning properly (cleared if device failed its diagnostics). -### _CRS (Current resource settings) -The _CRS method is generated automatically, as the driver knows it is an I2C +### \_CRS (Current resource settings) +The \_CRS method is generated automatically, as the driver knows it is an I2C controller, and so specifies how to configure the controller for proper operation with the touchpad. -- To view, visit https://review.coreboot.org/c/coreboot/+/67385 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifcdbd5371408784bf9b81c1ade90263de8c60e0f Gerrit-Change-Number: 67385 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: util/amdfwtool: Include the header with __packed definition
by Robert Zieba (Code Review) 06 Sep '22

06 Sep '22

Attention is currently required from: Raul Rangel, Jon Murphy, Karthik Ramasubramanian, Felix Held. Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67384 ) Change subject: util/amdfwtool: Include the header with __packed definition ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/67384 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I448cbad533608dd5c2bd4f2d827fcc5db5dee5cb Gerrit-Change-Number: 67384 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Robert Zieba <robertzieba(a)google.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 06 Sep 2022 20:39:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/skyrim: Enable amdfw separation
by build bot (Jenkins) (Code Review) 06 Sep '22

06 Sep '22

Attention is currently required from: Jason Nien, Martin Roth, Karthik Ramasubramanian. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67261 ) Change subject: mb/google/skyrim: Enable amdfw separation ...................................................................... Patch Set 5: (6 comments) Commit Message: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-158177): https://review.coreboot.org/c/coreboot/+/67261/comment/07f9f09e_80d42442 PS5, Line 17: Before this patch series: Possible unwrapped commit description (prefer a maximum 72 chars per line) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-158177): https://review.coreboot.org/c/coreboot/+/67261/comment/26828a4d_cf98904d PS5, Line 18: 506:finished verifying keyblock/preamble (RSA) 1,977,851,803 (339,910) Possible unwrapped commit description (prefer a maximum 72 chars per line) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-158177): https://review.coreboot.org/c/coreboot/+/67261/comment/ecbf5fcf_c7b3af10 PS5, Line 19: 507:starting to verify body (load+SHA2+RSA) 1,977,851,914 (111) Possible unwrapped commit description (prefer a maximum 72 chars per line) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-158177): https://review.coreboot.org/c/coreboot/+/67261/comment/50a5f3cf_8297bdeb PS5, Line 21: After this patch series: Possible unwrapped commit description (prefer a maximum 72 chars per line) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-158177): https://review.coreboot.org/c/coreboot/+/67261/comment/cdd4410f_43dd1497 PS5, Line 22: 506:finished verifying keyblock/preamble (RSA) 7,948,714,284 (312,722) Possible unwrapped commit description (prefer a maximum 72 chars per line) Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-158177): https://review.coreboot.org/c/coreboot/+/67261/comment/744a7a24_1380850e PS5, Line 23: 507:starting to verify body (load+SHA2+RSA) 7,948,714,389 (105) Possible unwrapped commit description (prefer a maximum 72 chars per line) -- To view, visit https://review.coreboot.org/c/coreboot/+/67261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I78ec6d28b4c5fc40bdade47489d58180a54dee4d Gerrit-Change-Number: 67261 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Tue, 06 Sep 2022 20:23:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: util/amdfwtool: Include the header with __packed definition
by Karthik Ramasubramanian (Code Review) 06 Sep '22

06 Sep '22

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/67384 ) Change subject: util/amdfwtool: Include the header with __packed definition ...................................................................... util/amdfwtool: Include the header with __packed definition Checkpatch script recommends to use __packed instead of __attribute__((packed)). Currently the build rule for amdfwtool does not include the required header file with __packed definition. Update the compiler flag to include the required header file. BUG=None TEST=Build amdfwtool. Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: I448cbad533608dd5c2bd4f2d827fcc5db5dee5cb --- M util/amdfwtool/Makefile M util/amdfwtool/Makefile.inc 2 files changed, 21 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/67384/1 diff --git a/util/amdfwtool/Makefile b/util/amdfwtool/Makefile index bdc3dc3..1b4b824 100644 --- a/util/amdfwtool/Makefile +++ b/util/amdfwtool/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause +top ?= $(abspath ../..) HOSTCC ?= cc READ_SRC = amdfwread.c @@ -10,6 +11,7 @@ TARGETS = amdfwread amdfwtool WERROR=-Werror CFLAGS=-O2 -Wall -Wextra -Wshadow ${WERROR} +CFLAGS += -include $(top)/src/commonlib/bsd/include/commonlib/bsd/compiler.h all: $(TARGETS) diff --git a/util/amdfwtool/Makefile.inc b/util/amdfwtool/Makefile.inc index 4b7db76..d4fcd9b 100644 --- a/util/amdfwtool/Makefile.inc +++ b/util/amdfwtool/Makefile.inc @@ -3,6 +3,7 @@ amdfwtoolobj = amdfwtool.o data_parse.o AMDFWTOOLCFLAGS=-O2 -Wall -Wextra -Wshadow -Werror +AMDFWTOOLCFLAGS += -include $(top)/src/commonlib/bsd/include/commonlib/bsd/compiler.h $(objutil)/amdfwtool/%.o: $(top)/util/amdfwtool/%.c # $(HEADER) $(HOSTCC) $(AMDFWTOOLCFLAGS) $(HOSTCFLAGS) -c -o $@ $< -- To view, visit https://review.coreboot.org/c/coreboot/+/67384 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I448cbad533608dd5c2bd4f2d827fcc5db5dee5cb Gerrit-Change-Number: 67384 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: mb/google/skyrim: Enable amdfw separation
by Karthik Ramasubramanian (Code Review) 06 Sep '22

06 Sep '22

Attention is currently required from: Jason Nien, Martin Roth, Karthik Ramasubramanian. Hello build bot (Jenkins), Jason Nien, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/67261 to look at the new patch set (#5). Change subject: mb/google/skyrim: Enable amdfw separation ...................................................................... mb/google/skyrim: Enable amdfw separation Select the config to separate the AMDFW binary from the verified boot section. BUG=b:203597980 TEST=Build Skyrim BIOS image and boot to OS with PSP verstage passing the hash table and PSP verifying the binaries against the hash table. Observe boot time improvement of ~140 ms while operating SPI bus at 66 MHz with PSP verstage enabled. Before this patch series: 506:finished verifying keyblock/preamble (RSA) 1,977,851,803 (339,910) 507:starting to verify body (load+SHA2+RSA) 1,977,851,914 (111) 508:finished loading body 1,978,053,432 (201,518) After this patch series: 506:finished verifying keyblock/preamble (RSA) 7,948,714,284 (312,722) 507:starting to verify body (load+SHA2+RSA) 7,948,714,389 (105) 508:finished loading body 7,948,797,849 (83,460) Change-Id: I78ec6d28b4c5fc40bdade47489d58180a54dee4d Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- M src/mainboard/google/skyrim/Kconfig M src/mainboard/google/skyrim/chromeos.fmd 2 files changed, 30 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/67261/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/67261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I78ec6d28b4c5fc40bdade47489d58180a54dee4d Gerrit-Change-Number: 67261 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit September 2022