coreboot-gerrit April 2022

coreboot-gerrit@coreboot.org

1 participants
5879 discussions

Change in coreboot[master]: ChromeOS: Add legacy mainboard_ec_running_ro()
by Kyösti Mälkki (Code Review) 06 Apr '22

06 Apr '22

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/58896 ) Change subject: ChromeOS: Add legacy mainboard_ec_running_ro() ...................................................................... ChromeOS: Add legacy mainboard_ec_running_ro() Motivation is to have mainboard_chromeos_acpi_generate() do nothing else than fill ACPI \OIPG package. Change-Id: I3cb95268424dc27f8c1e26b3d34eff1a7b8eab7f Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/58896 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Julius Werner <jwerner(a)chromium.org> --- M src/include/bootmode.h M src/mainboard/google/butterfly/chromeos.c M src/mainboard/google/parrot/chromeos.c M src/mainboard/google/stout/chromeos.c M src/mainboard/samsung/lumpy/chromeos.c M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/gnvs.c 7 files changed, 34 insertions(+), 29 deletions(-) Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/src/include/bootmode.h b/src/include/bootmode.h index da2dbf6..e2d1cb9 100644 --- a/src/include/bootmode.h +++ b/src/include/bootmode.h @@ -3,6 +3,8 @@ #ifndef __BOOTMODE_H__ #define __BOOTMODE_H__ +#include <stdbool.h> + /* functions implemented per mainboard: */ void init_bootmode_straps(void); int get_write_protect_state(void); @@ -12,6 +14,7 @@ int get_wipeout_mode_switch(void); int get_lid_switch(void); int get_ec_is_trusted(void); +bool mainboard_ec_running_ro(void); /* Return 1 if display initialization is required. 0 if not. */ int display_init_required(void); diff --git a/src/mainboard/google/butterfly/chromeos.c b/src/mainboard/google/butterfly/chromeos.c index a7d56ae..fb249d4 100644 --- a/src/mainboard/google/butterfly/chromeos.c +++ b/src/mainboard/google/butterfly/chromeos.c @@ -48,6 +48,15 @@ return 0; } +bool mainboard_ec_running_ro(void) +{ + // TODO: MLR + // The firmware read/write status is a "virtual" switch and + // will be handled elsewhere. Until then hard-code to + // read/write instead of read-only for developer mode. + return false; +} + static const struct cros_gpio cros_gpios[] = { CROS_GPIO_REC_AH(CROS_GPIO_VIRTUAL, CROS_GPIO_DEVICE_NAME), CROS_GPIO_WP_AL(WP_GPIO, CROS_GPIO_DEVICE_NAME), @@ -55,13 +64,6 @@ void mainboard_chromeos_acpi_generate(void) { - // TODO: MLR - // The firmware read/write status is a "virtual" switch and - // will be handled elsewhere. Until then hard-code to - // read/write instead of read-only for developer mode. - if (CONFIG(CHROMEOS_NVS)) - chromeos_set_ecfw_rw(); - chromeos_acpi_gpio_generate(cros_gpios, ARRAY_SIZE(cros_gpios)); } diff --git a/src/mainboard/google/parrot/chromeos.c b/src/mainboard/google/parrot/chromeos.c index 2b361ac..e0bc0b0 100644 --- a/src/mainboard/google/parrot/chromeos.c +++ b/src/mainboard/google/parrot/chromeos.c @@ -46,7 +46,7 @@ return !get_gpio(GPIO_REC_MODE); } -static int parrot_ec_running_ro(void) +bool mainboard_ec_running_ro(void) { return get_recovery_mode_switch(); } @@ -58,9 +58,6 @@ void mainboard_chromeos_acpi_generate(void) { - if (CONFIG(CHROMEOS_NVS) && !parrot_ec_running_ro()) - chromeos_set_ecfw_rw(); - chromeos_acpi_gpio_generate(cros_gpios, ARRAY_SIZE(cros_gpios)); } diff --git a/src/mainboard/google/stout/chromeos.c b/src/mainboard/google/stout/chromeos.c index 964217e..fcbbc8a 100644 --- a/src/mainboard/google/stout/chromeos.c +++ b/src/mainboard/google/stout/chromeos.c @@ -49,8 +49,7 @@ * The recovery-switch is virtual on Stout and is handled via the EC. * Stout recovery mode is only valid if RTC_PWR_STS is set and the EC * indicated the recovery keys were pressed. We use a global flag for - * rec_mode to be used after RTC_POWER_STS has been cleared. This function - * is complicated by romstage support, which can't use a global variable. + * rec_mode to be used after RTC_POWER_STS has been cleared. * Note, rec_mode is the only time the EC is in RO mode, otherwise, RW. */ int get_recovery_mode_switch(void) @@ -75,6 +74,11 @@ return ec_in_rec_mode; } +bool mainboard_ec_running_ro(void) +{ + return !!get_recovery_mode_switch(); +} + static const struct cros_gpio cros_gpios[] = { CROS_GPIO_REC_AH(CROS_GPIO_VIRTUAL, CROS_GPIO_DEVICE_NAME), CROS_GPIO_WP_AL(GPIO_SPI_WP, CROS_GPIO_DEVICE_NAME), @@ -82,8 +86,5 @@ void mainboard_chromeos_acpi_generate(void) { - if (CONFIG(CHROMEOS_NVS) && !get_recovery_mode_switch()) - chromeos_set_ecfw_rw(); - chromeos_acpi_gpio_generate(cros_gpios, ARRAY_SIZE(cros_gpios)); } diff --git a/src/mainboard/samsung/lumpy/chromeos.c b/src/mainboard/samsung/lumpy/chromeos.c index 22b936c..1b9c7c4 100644 --- a/src/mainboard/samsung/lumpy/chromeos.c +++ b/src/mainboard/samsung/lumpy/chromeos.c @@ -85,6 +85,11 @@ pci_s_write_config32(dev, SATA_SP, flags); } +bool mainboard_ec_running_ro(void) +{ + return !ec_read(0xcb); +} + static const struct cros_gpio cros_gpios[] = { CROS_GPIO_REC_AL(GPIO_REC_MODE, CROS_GPIO_DEVICE_NAME), CROS_GPIO_WP_AH(GPIO_SPI_WP, CROS_GPIO_DEVICE_NAME), @@ -92,8 +97,5 @@ void mainboard_chromeos_acpi_generate(void) { - if (CONFIG(CHROMEOS_NVS) && ec_read(0xcb)) - chromeos_set_ecfw_rw(); - chromeos_acpi_gpio_generate(cros_gpios, ARRAY_SIZE(cros_gpios)); } diff --git a/src/vendorcode/google/chromeos/chromeos.h b/src/vendorcode/google/chromeos/chromeos.h index abdd6c2..da25617 100644 --- a/src/vendorcode/google/chromeos/chromeos.h +++ b/src/vendorcode/google/chromeos/chromeos.h @@ -25,7 +25,6 @@ void cbmem_add_vpd_calibration_data(void); void chromeos_set_me_hash(u32*, int); void chromeos_set_ramoops(void *ram_oops, size_t size); -void chromeos_set_ecfw_rw(void); void chromeos_init_chromeos_acpi(void); /** diff --git a/src/vendorcode/google/chromeos/gnvs.c b/src/vendorcode/google/chromeos/gnvs.c index 7821786..b1090a4 100644 --- a/src/vendorcode/google/chromeos/gnvs.c +++ b/src/vendorcode/google/chromeos/gnvs.c @@ -2,6 +2,7 @@ #include <acpi/acpi.h> #include <acpi/acpigen.h> +#include <bootmode.h> #include <types.h> #include <string.h> #include <stdlib.h> @@ -31,6 +32,8 @@ return region_device_sz(&vpd); } +__weak bool mainboard_ec_running_ro(void) { return true; } + void chromeos_init_chromeos_acpi(void) { size_t vpd_size; @@ -59,8 +62,13 @@ /* EC can override to ECFW_RW. */ chromeos_acpi->vbt2 = ACTIVE_ECFW_RO; - if (CONFIG(EC_GOOGLE_CHROMEEC) && !google_ec_running_ro()) - chromeos_acpi->vbt2 = ACTIVE_ECFW_RW; + if (CONFIG(EC_GOOGLE_CHROMEEC)) { + if (!google_ec_running_ro()) + chromeos_acpi->vbt2 = ACTIVE_ECFW_RW; + } else { + if (!mainboard_ec_running_ro()) + chromeos_acpi->vbt2 = ACTIVE_ECFW_RW; + } } void chromeos_set_me_hash(u32 *hash, int len) @@ -83,13 +91,6 @@ chromeos_acpi->ramoops_len = size; } -void chromeos_set_ecfw_rw(void) -{ - if (!chromeos_acpi) - return; - chromeos_acpi->vbt2 = ACTIVE_ECFW_RW; -} - void smbios_type0_bios_version(uintptr_t address) { if (!chromeos_acpi) -- To view, visit https://review.coreboot.org/c/coreboot/+/58896 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3cb95268424dc27f8c1e26b3d34eff1a7b8eab7f Gerrit-Change-Number: 58896 Gerrit-PatchSet: 16 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: nb/intel/gm45: Don't use __SIMPLE_DEVICE__
by build bot (Jenkins) (Code Review) 06 Apr '22

06 Apr '22

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63393 ) Change subject: nb/intel/gm45: Don't use __SIMPLE_DEVICE__ ...................................................................... Patch Set 3: (9 comments) File src/northbridge/intel/gm45/igd.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/74409718_ef003c95 PS3, Line 18: const struct device * mch_dev = __pci_0_00_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/dd49438d_bf5b0103 PS3, Line 19: const struct device * peg_dev = __pci_0_01_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/35a583dd_4d4a3d85 PS3, Line 20: const struct device * igd_dev = __pci_0_02_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/8664c54e_6a8f699b PS3, Line 80: const struct device * mch_dev = PCI_DEV(0, 0, 0); "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/d3302450_5c1fd9cf PS3, Line 101: const struct device * mch_dev = PCI_DEV(0, 0, 0); "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/4ca147ce_51785548 PS3, Line 112: const struct device * mch_dev = PCI_DEV(0, 0, 0); "foo * bar" should be "foo *bar" File src/northbridge/intel/gm45/iommu.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/fb4b1f1f_13bbdb85 PS3, Line 36: const struct device * igd = __pci_0_02_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/3d7bad19_3c4c2d78 PS3, Line 52: const struct device * peg = __pci_0_01_0; "foo * bar" should be "foo *bar" File src/northbridge/intel/gm45/pcie.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145496): https://review.coreboot.org/c/coreboot/+/63393/comment/d6f03b9a_e2374147 PS3, Line 274: const struct device * mch = __pci_0_00_0; "foo * bar" should be "foo *bar" -- To view, visit https://review.coreboot.org/c/coreboot/+/63393 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I27445f39d9ce9c995606229903a10d8c81c0ddb2 Gerrit-Change-Number: 63393 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:20:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/denverton_ns: add common device function macros
by Mariusz Szafrański (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Jeff Daly, Suresh Bellampalli, Angel Pons, Patrick Rudolph. Mariusz Szafrański has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60990 ) Change subject: soc/intel/denverton_ns: add common device function macros ...................................................................... Patch Set 11: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/60990 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I75daaf4907515f80a10c003eb473bbe557a42acc Gerrit-Change-Number: 60990 Gerrit-PatchSet: 11 Gerrit-Owner: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Mariusz Szafrański <mariuszx.szafranski(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Attention: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:13:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/usb/pci_ehci.c: Move away from __SIMPLE_DEVICE__
by build bot (Jenkins) (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Marshall Dawson, Kyösti Mälkki, Felix Held. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63387 ) Change subject: drivers/usb/pci_ehci.c: Move away from __SIMPLE_DEVICE__ ...................................................................... Patch Set 4: (5 comments) File src/drivers/usb/pci_ehci.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145492): https://review.coreboot.org/c/coreboot/+/63387/comment/02f997ab_64ff2a93 PS4, Line 91: const struct device *dbg_dev = pci_ehci_dbg_dev(); code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145492): https://review.coreboot.org/c/coreboot/+/63387/comment/b872585b_6bd02b26 PS4, Line 91: const struct device *dbg_dev = pci_ehci_dbg_dev(); please, no spaces at the start of a line File src/southbridge/intel/common/usb_debug.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145492): https://review.coreboot.org/c/coreboot/+/63387/comment/638225f6_cb61cbd9 PS4, Line 13: const struct device *dev; code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145492): https://review.coreboot.org/c/coreboot/+/63387/comment/4b6819a3_4022ff0d PS4, Line 13: const struct device *dev; please, no spaces at the start of a line Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145492): https://review.coreboot.org/c/coreboot/+/63387/comment/a8d5bfa4_901ea7ac PS4, Line 27: if (class != PCI_EHCI_CLASSCODE) { braces {} are not necessary for single statement blocks -- To view, visit https://review.coreboot.org/c/coreboot/+/63387 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iea0598e01d8fc25b08fa8f22d0622ecc729e8160 Gerrit-Change-Number: 63387 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:12:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/denverton_ns/chip.c: add soc_acpi_name function
by Mariusz Szafrański (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Jeff Daly, Suresh Bellampalli, Patrick Rudolph. Mariusz Szafrański has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60991 ) Change subject: soc/intel/denverton_ns/chip.c: add soc_acpi_name function ...................................................................... Patch Set 13: Code-Review+2 (1 comment) Patchset: PS13: looks OK -- To view, visit https://review.coreboot.org/c/coreboot/+/60991 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f50733656ca7724caf8a6570bcb21f7b761c3ce Gerrit-Change-Number: 60991 Gerrit-PatchSet: 13 Gerrit-Owner: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Mariusz Szafrański <mariuszx.szafranski(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Attention: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:10:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: nb/intel/gm45: Don't use __SIMPLE_DEVICE__
by build bot (Jenkins) (Code Review) 06 Apr '22

06 Apr '22

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63393 ) Change subject: nb/intel/gm45: Don't use __SIMPLE_DEVICE__ ...................................................................... Patch Set 2: (9 comments) File src/northbridge/intel/gm45/igd.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/1030e558_de3723a5 PS2, Line 18: const struct device * mch_dev = __pci_0_00_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/e5d8196e_f9c0c69d PS2, Line 19: const struct device * peg_dev = __pci_0_01_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/439a951b_6fbbd7b2 PS2, Line 20: const struct device * igd_dev = __pci_0_02_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/4eb3e58f_8b89b7ff PS2, Line 80: const struct device * mch_dev = PCI_DEV(0, 0, 0); "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/6659e716_6487eae9 PS2, Line 101: const struct device * mch_dev = PCI_DEV(0, 0, 0); "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/0bf8771c_509f14dd PS2, Line 112: const struct device * mch_dev = PCI_DEV(0, 0, 0); "foo * bar" should be "foo *bar" File src/northbridge/intel/gm45/iommu.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/c7f60dbc_f24a5b19 PS2, Line 36: const struct device * igd = __pci_0_02_0; "foo * bar" should be "foo *bar" Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/0b0a6cec_7fb9018f PS2, Line 52: const struct device * peg = __pci_0_01_0; "foo * bar" should be "foo *bar" File src/northbridge/intel/gm45/pcie.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145490): https://review.coreboot.org/c/coreboot/+/63393/comment/e5bd4888_ab77135b PS2, Line 274: const struct device * mch = __pci_0_00_0; "foo * bar" should be "foo *bar" -- To view, visit https://review.coreboot.org/c/coreboot/+/63393 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I27445f39d9ce9c995606229903a10d8c81c0ddb2 Gerrit-Change-Number: 63393 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:06:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/efi: Add EFI variable store option support
by Patrick Rudolph (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Sean Rhodes, Martin Roth, Jakub Czapiga, Matt DeVillier, Angel Pons, Arthur Heymans, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52564 ) Change subject: drivers/efi: Add EFI variable store option support ...................................................................... Patch Set 11: (25 comments) File src/drivers/efi/Kconfig: https://review.coreboot.org/c/coreboot/+/52564/comment/6923294e_98485a2c PS10, Line 4: Adds a driver that is able to read and write an EFI formated > > 'formated' may be misspelled - perhaps 'formatted'? […] Done File src/drivers/efi/Makefile.inc: PS10: > Missing SPDX-License-Identifier Done File src/drivers/efi/efivars.c: https://review.coreboot.org/c/coreboot/+/52564/comment/3da9bf50_87dcef67 PS10, Line 25: static void print_guid(int log_level, const EFI_GUID *g) : { : printk(log_level, "GUID: "); : : for (int i = 0; i < sizeof(*g); i++) : printk(log_level, "%02x", ((uint8_t *)g)[i]); : } > nit: Should not GUID be printed to match general format? […] Done https://review.coreboot.org/c/coreboot/+/52564/comment/0e619b5f_d391989e PS10, Line 75: WA > rdev_strcmp_wchar_ascii I guess? I've never seen `WA` in this context before. Done https://review.coreboot.org/c/coreboot/+/52564/comment/d111200b_8ef69c60 PS10, Line 90: ; > not needed Done https://review.coreboot.org/c/coreboot/+/52564/comment/3dd2cc49_53ac19db PS10, Line 110: ; > Not needed Done https://review.coreboot.org/c/coreboot/+/52564/comment/b83ac055_55f0b7d8 PS10, Line 119: ; > Not needed Done https://review.coreboot.org/c/coreboot/+/52564/comment/87a9e7f6_d5e7d571 PS10, Line 208: zd > zu Done https://review.coreboot.org/c/coreboot/+/52564/comment/1d2d0ae0_5f0f6ed1 PS10, Line 361: else { : if (rdev_readat(rdev, &hdr, 0, sizeof(hdr)) != sizeof(hdr)) : return CB_EFI_ACCESS_ERROR; : } > These can be joined to form one `else if` block. Done https://review.coreboot.org/c/coreboot/+/52564/comment/009cd77e_09905e1c PS10, Line 387: 4 > nit: I think, that align value should be defined somewhere in the header file. […] Done https://review.coreboot.org/c/coreboot/+/52564/comment/2d52f6f2_ed44ed82 PS10, Line 532: (hdr.StartId != UINT16_MAX || : hdr.State != UINT8_MAX || : hdr.DataSize != UINT32_MAX || : hdr.NameSize != UINT32_MAX || : hdr.Attributes != UINT32_MAX) > Why not use `memcmp()` instead? Fields are not continous. File tests/Makefile.inc: https://review.coreboot.org/c/coreboot/+/52564/comment/d443dee7_f988bfc7 PS10, Line 85: tests/drivers tests/drivers > No need to add tests/drivers directory two times Done File tests/drivers/Makefile.inc: https://review.coreboot.org/c/coreboot/+/52564/comment/886af505_6d627daa PS10, Line 5: efi > I think, that it should be named efivars-test to match it with test file name Done File tests/drivers/efivars.c: https://review.coreboot.org/c/coreboot/+/52564/comment/dbe588aa_8bcd59f5 PS10, Line 10: uint8_t > #include <types. […] Done https://review.coreboot.org/c/coreboot/+/52564/comment/14a040a8_afd8bf37 PS10, Line 62: size = sizeof(buf); > Value stored in `size` by `efi_fv_get_option()` not checked Done https://review.coreboot.org/c/coreboot/+/52564/comment/27a006a7_5f32dd11 PS10, Line 67: 8 > What does this "8" represent? Can it be represented as const/define? Done https://review.coreboot.org/c/coreboot/+/52564/comment/918714cd_78938d54 PS10, Line 73: size = sizeof(buf); > This probably should be checked (assert_int_equal()), as it it modified by efi_fv_get_option() Done https://review.coreboot.org/c/coreboot/+/52564/comment/a7d29e42_d1194d0a PS10, Line 91: "is great", : 9 > I think, it would be better to store value in array and use ARRAY_SIZE() instead of static value. Done https://review.coreboot.org/c/coreboot/+/52564/comment/198064f2_afcb4903 PS10, Line 111: name > Here you are using name from variable, and above it is a bare value. Please, unify it. Done https://review.coreboot.org/c/coreboot/+/52564/comment/6f6262fe_872a9960 PS10, Line 111: "is awesome", 11 > Same as above with value and size Done https://review.coreboot.org/c/coreboot/+/52564/comment/f38c631b_1fd6ceac PS10, Line 116: + > nit: spaces around `+` Done https://review.coreboot.org/c/coreboot/+/52564/comment/48157881_e07d552b PS10, Line 119: + > nit: spaces around `+` Done https://review.coreboot.org/c/coreboot/+/52564/comment/d4da4308_ddb56984 PS10, Line 119: 89 > What does this magic 89 number mean? Length of the newly written option. https://review.coreboot.org/c/coreboot/+/52564/comment/2b97e9d4_ae26e16f PS10, Line 125: size = sizeof(buf); > Same as above Done https://review.coreboot.org/c/coreboot/+/52564/comment/f58f9d98_6842bb4c PS10, Line 135: const struct CMUnitTest tests[] = {cmocka_unit_test(efi_test_header), : cmocka_unit_test(efi_test_noop_existing_write), : cmocka_unit_test(efi_test_new_write)}; > nit: […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/52564 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8079f71d29da5dc2db956fc68bef1486fe3906bb Gerrit-Change-Number: 52564 Gerrit-PatchSet: 11 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Patrick Rudolph Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:02:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com> Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/smmstore: Expose region device
by Patrick Rudolph (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62014 ) Change subject: drivers/smmstore: Expose region device ...................................................................... Patch Set 4: (1 comment) File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/62014/comment/f62e6d5d_b338486e PS3, Line 96: else > Coding style says that the else branch should have braces. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/62014 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6316b703829590bd6f41c3d4013b4a4660b9cbab Gerrit-Change-Number: 62014 Gerrit-PatchSet: 4 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 06 Apr 2022 09:02:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: bsd/cb_err: Add error code for UEFI variable store
by Patrick Rudolph (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Angel Pons, Julius Werner, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61960 ) Change subject: bsd/cb_err: Add error code for UEFI variable store ...................................................................... Patch Set 3: (2 comments) File src/commonlib/bsd/include/commonlib/bsd/cb_err.h: https://review.coreboot.org/c/coreboot/+/61960/comment/dbd7de02_8cbfe834 PS2, Line 19: TO > TOO Done https://review.coreboot.org/c/coreboot/+/61960/comment/56ad7ee7_7c0b782f PS2, Line 46: CB_EFI_FVH_INVALID = -500, /**< UEFI FVH is corrupted */ > > please, no space before tabs […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/61960 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6baea9fb138d1a2755d22a3d587105793adb9c90 Gerrit-Change-Number: 61960 Gerrit-PatchSet: 3 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Joey Madafferi <joeymadafferi(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Patrick Rudolph Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Patrick Rudolph Gerrit-Comment-Date: Wed, 06 Apr 2022 09:02:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/efi: Add EFI variable store option support
by Patrick Rudolph (Code Review) 06 Apr '22

06 Apr '22

Attention is currently required from: Sean Rhodes, Martin Roth, Matt DeVillier, Arthur Heymans, Patrick Rudolph. Hello build bot (Jenkins), Sean Rhodes, Martin Roth, Jakub Czapiga, Matt DeVillier, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/52564 to look at the new patch set (#11). Change subject: drivers/efi: Add EFI variable store option support ...................................................................... drivers/efi: Add EFI variable store option support Add a driver to read and write EFI variables stored in a region device. This is particullary useful for EDK2 as payload and allows to reuse existing EFI tools to set/get options used by the firmware. The write implementation is fault tolerant and doesn't corrupt the variable store. A faulting write might result in using the old value even though a 'newer' had been completely written. Implemented basic unit tests for header corruption, writing existing data and append new data into the store. Initial firmware region state: Initially the variable store region isn't formatted. Usually this is done in the EDK2 payload when no valid firmware volume could be found. It might be useful to do this offline or in coreboot to have a working option store on the first boot or when it was corrupted. Performance improvements: Right now the code always checks if the firmware volume header is valid. This could be optimised by caching the test result in heap. For write operations it would be good to cache the end of the variable store in the heap as well, instead of walking the whole store. Reclaiming memory: The EFI variable store is append write only. To update an existing variable, first a new is written to the end of the store and then the previous is marked invalid. This only works on PNOR flash that allow to clear set bits, but keep cleared bits state. This mechanisms allows a fault tolerant write, but it also requires to "clean" the variable store for time to time. This cleaning would remove variables that have been marked "deleted". Such cleaning mechanism in turn must be fault tolerant and thus must use a second parition in the SPI flash as backup/working region. For now to cleaning is done in coreboot. Fault checking: The driver should check if a previous write was successfull and if not mark variables as deleted on the next operation. Tested and working: - Could enumerate all existing variables - Could read variables - Could write variables Change-Id: I8079f71d29da5dc2db956fc68bef1486fe3906bb Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- A src/drivers/efi/Kconfig A src/drivers/efi/Makefile.inc A src/drivers/efi/efivars.c A src/drivers/efi/efivars.h M tests/Makefile.inc A tests/drivers/Makefile.inc A tests/drivers/efivars.c 7 files changed, 876 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/52564/11 -- To view, visit https://review.coreboot.org/c/coreboot/+/52564 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8079f71d29da5dc2db956fc68bef1486fe3906bb Gerrit-Change-Number: 52564 Gerrit-PatchSet: 11 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Patrick Rudolph Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2022