Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40625
to look at the new patch set (#7).
Change subject: [WIP] skl: PEG for Optimus
......................................................................
[WIP] skl: PEG for Optimus
Creates PEG ACPI device for Optimus.
Optimus is not working (dGPU does not appear in the OS; perhaps an issue
with optimus_mb.asl - implementation of dGPU power functions).
Change-Id: I107bd5f7c192b8ffc83de6d8f1ac314bb5dcbfbd
Signed-off-by: Benjamin Doron <benjamin.doron00(a)gmail.com>
---
A src/soc/intel/skylake/acpi/peg.asl
M src/soc/intel/skylake/acpi/systemagent.asl
2 files changed, 158 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/40625/7
--
To view, visit https://review.coreboot.org/c/coreboot/+/40625
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I107bd5f7c192b8ffc83de6d8f1ac314bb5dcbfbd
Gerrit-Change-Number: 40625
Gerrit-PatchSet: 7
Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Hello Marco Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40833
to review the following change.
Change subject: soc/intel/jasperlake: fix arguments of dimm_info_fill() for dram_part_num
......................................................................
soc/intel/jasperlake: fix arguments of dimm_info_fill() for dram_part_num
BUG=b:152019429
BRANCH=None
TEST=1. provision dram_part_num field of CBI
2. modify mainboard - dedede to report DRAM part number from CBI
3. check DRAM part number is correct in SMBIOS for memory device
Change-Id: I509d06a81bd005c5afe6e74a2da2ca408dee7b29
Signed-off-by: Marco Chen <marcochen(a)google.com>
---
M src/soc/intel/jasperlake/romstage/romstage.c
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/40833/1
diff --git a/src/soc/intel/jasperlake/romstage/romstage.c b/src/soc/intel/jasperlake/romstage/romstage.c
index b8e9032..dc5dcf1 100644
--- a/src/soc/intel/jasperlake/romstage/romstage.c
+++ b/src/soc/intel/jasperlake/romstage/romstage.c
@@ -108,8 +108,8 @@
src_dimm->RankInDimm,
channel_info->ChannelId,
src_dimm->DimmId,
- (const char *)src_dimm->ModulePartNum,
- sizeof(src_dimm->ModulePartNum),
+ dram_part_num,
+ dram_part_num_len,
serial_num,
meminfo_hob->DataWidth,
meminfo_hob->VddVoltage[memProfNum],
--
To view, visit https://review.coreboot.org/c/coreboot/+/40833
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I509d06a81bd005c5afe6e74a2da2ca408dee7b29
Gerrit-Change-Number: 40833
Gerrit-PatchSet: 1
Gerrit-Owner: Marco Chen <marcochen(a)google.com>
Gerrit-Reviewer: Marco Chen <marcochen(a)chromium.org>
Gerrit-MessageType: newchange
Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39491 )
Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................
soc/intel/cannonlake/bootblock: Fix FSP CAR
Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
Tested on Intel CFL, the new code allows to boot using FSP-T.
Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com>
---
M src/soc/intel/cannonlake/bootblock/bootblock.c
1 file changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/39491/1
diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c
index 4cc15fc..861f2ca 100644
--- a/src/soc/intel/cannonlake/bootblock/bootblock.c
+++ b/src/soc/intel/cannonlake/bootblock/bootblock.c
@@ -23,6 +23,18 @@
#if CONFIG(FSP_CAR)
#include <FsptUpd.h>
+/*
+ * Limit CodeRegionSize to 16 MiB as the CAR area is placed right below the memory mapped
+ * BIOS region. A ROM size of 32 MiB will cause wrong MTRRs to be set for the CAR area,
+ * which will brick the platform. Assumes that the memory mapped BIOS region is no more
+ * than 16 MiB.
+ */
+#if CONFIG_ROM_SIZE > 16 * 1024 * 1024
+#define CODE_CACHE_SIZE (16 * 1024 * 1024)
+#else
+#define CODE_CACHE_SIZE CONFIG_ROM_SIZE
+#endif
+
const FSPT_UPD temp_ram_init_params = {
.FspUpdHeader = {
.Signature = 0x545F4450554C4643ULL, /* 'CFLUPD_T' */
@@ -44,8 +56,8 @@
.MicrocodeRegionBase = 0,
.MicrocodeRegionSize = 0,
.CodeRegionBase =
- (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE),
- .CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE,
+ (uint32_t)(0x100000000ULL - CODE_CACHE_SIZE,
+ .CodeRegionSize = (uint32_t)CODE_CACHE_SIZE,
},
};
#endif
--
To view, visit https://review.coreboot.org/c/coreboot/+/39491
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Gerrit-Change-Number: 39491
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: newchange
Sheng-Liang Pan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40303 )
Change subject: mb/google/octopus/variants/bobba: Disable XHCI LFPS power management
......................................................................
mb/google/octopus/variants/bobba: Disable XHCI LFPS power management
Disable XHCI LFPS power management.
If the option is set in the devicetree, the bits[7:4] in
XHCI MMIO BAR + offset 0x80A4 (PMCTRL_REG) will be updated
from default 9 to 0.
BUG=b:146768983
BRANCH=None
TEST=build coreboot with DisableXhciLfpsPM being set to 1 and flash
the image to the device. Run following command to check if
bits[7:4] is set 0:
>iotools mmio_read32 "XHCI MMIO BAR + 0x80A4"
Signed-off-by: Pan Sheng-Liang <sheng-liang.pan(a)quanta.corp-partner.google.com>
Change-Id: Ib8e5ae79e097debf0c75ead232ddbb2baced2a2a
---
M src/mainboard/google/octopus/variants/bobba/overridetree.cb
M src/mainboard/google/octopus/variants/bobba/variant.c
2 files changed, 27 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/40303/1
diff --git a/src/mainboard/google/octopus/variants/bobba/overridetree.cb b/src/mainboard/google/octopus/variants/bobba/overridetree.cb
index 6cd4c61..67bb9d5 100644
--- a/src/mainboard/google/octopus/variants/bobba/overridetree.cb
+++ b/src/mainboard/google/octopus/variants/bobba/overridetree.cb
@@ -214,4 +214,5 @@
# Disable compliance mode
register "DisableComplianceMode" = "1"
+ register "disable_xhci_lfps_pm" = "1"
end
diff --git a/src/mainboard/google/octopus/variants/bobba/variant.c b/src/mainboard/google/octopus/variants/bobba/variant.c
index 57b7067..ca18898 100644
--- a/src/mainboard/google/octopus/variants/bobba/variant.c
+++ b/src/mainboard/google/octopus/variants/bobba/variant.c
@@ -8,6 +8,7 @@
#include <delay.h>
#include <gpio.h>
#include <ec/google/chromeec/ec.h>
+#include <soc/intel/apollolake/chip.h>
enum {
SKU_37_DROID = 37, /* LTE */
@@ -74,3 +75,28 @@
return;
}
}
+
+
+void variant_update_devtree(struct device *dev)
+{
+ struct soc_intel_apollolake_config *cfg = (struct soc_intel_apollolake_config *)dev->chip_info;
+
+ if (cfg != NULL) {
+ if (cfg->DisableXhciLfpsPM) {
+ switch (get_board_sku()) {
+ case 33:
+ case 34:
+ case 35:
+ case 36:
+ case 41:
+ case 42:
+ case 43:
+ case 44:
+ cfg->DisableXhciLfpsPM = 0;
+ return;
+ default:
+ return;
+ }
+ }
+ }
+}
--
To view, visit https://review.coreboot.org/c/coreboot/+/40303
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib8e5ae79e097debf0c75ead232ddbb2baced2a2a
Gerrit-Change-Number: 40303
Gerrit-PatchSet: 1
Gerrit-Owner: Sheng-Liang Pan <sheng-liang.pan(a)quanta.corp-partner.google.com>
Gerrit-MessageType: newchange
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40936 )
Change subject: acpi: Move ACPI table support out of arch/x86 (1/5)
......................................................................
acpi: Move ACPI table support out of arch/x86 (1/5)
This change moves all ACPI table support in coreboot currently living
under arch/x86 into common code to make it architecture
independent. ACPI table generation is not really tied to any
architecture and hence it makes sense to move this to its own directory.
In order to make it easier to review, this change is being split into
multiple CLs. This is change 1/5 which moves .c files from arch/x86 to
acpi/.
The only acpi files that are still retained under arch/x86 are:
a. acpi_s3.c: This doesn't really deal with ACPI tables. Also, there
are some assumptions in there about SMM which will have to be resolved
if this file needs to be moved to common code.
b. acpi_bert_storage.c/bert_storage.h: This file is currently written
specifically with x86 in mind. So, not moving the file for now.
Motivation for this change: Not all stages on Picasso SoC are targeted
for the same architecture. For example, verstage (if runs before
bootblock) will be targeted for non-x86. This makes it difficult to
add device tree to verstage which would be required to get to SoC
configs from the tree. This is because the device tree on x86
platforms currently contains a lot of devices that require ACPI
related enums and structs (like acpi_gpio, acpi_pld, acpi_dp and so
on). Hence, this change removes all ACPI table support out of
arch/x86.
BUG=b:155428745
Change-Id: I01470da8f8db9e28c1617e78fc3104e954f8af1a
Signed-off-by: Furquan Shaikh <furquan(a)google.com>
---
M src/acpi/Makefile.inc
R src/acpi/acpi.c
R src/acpi/acpi_device.c
R src/acpi/acpi_pld.c
R src/acpi/acpigen.c
R src/acpi/acpigen_dsm.c
R src/acpi/acpigen_ps2_keybd.c
M src/arch/x86/Makefile.inc
8 files changed, 12 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/40936/1
diff --git a/src/acpi/Makefile.inc b/src/acpi/Makefile.inc
index e99110e..068c592 100644
--- a/src/acpi/Makefile.inc
+++ b/src/acpi/Makefile.inc
@@ -1,4 +1,15 @@
# SPDX-License-Identifier: GPL-2.0-only
# This file is part of the coreboot project.
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += sata.c
+ifeq ($(CONFIG_HAVE_ACPI_TABLES),y)
+
+ramstage-y += acpi.c
+ramstage-y += acpi_device.c
+ramstage-y += acpi_pld.c
+ramstage-y += acpigen.c
+ramstage-y += acpigen_dsm.c
+ramstage-y += acpigen_ps2_keybd.c
+
+ramstage-y += sata.c
+
+endif # CONFIG_GENERATE_ACPI_TABLES
diff --git a/src/arch/x86/acpi.c b/src/acpi/acpi.c
similarity index 100%
rename from src/arch/x86/acpi.c
rename to src/acpi/acpi.c
diff --git a/src/arch/x86/acpi_device.c b/src/acpi/acpi_device.c
similarity index 100%
rename from src/arch/x86/acpi_device.c
rename to src/acpi/acpi_device.c
diff --git a/src/arch/x86/acpi_pld.c b/src/acpi/acpi_pld.c
similarity index 100%
rename from src/arch/x86/acpi_pld.c
rename to src/acpi/acpi_pld.c
diff --git a/src/arch/x86/acpigen.c b/src/acpi/acpigen.c
similarity index 100%
rename from src/arch/x86/acpigen.c
rename to src/acpi/acpigen.c
diff --git a/src/arch/x86/acpigen_dsm.c b/src/acpi/acpigen_dsm.c
similarity index 100%
rename from src/arch/x86/acpigen_dsm.c
rename to src/acpi/acpigen_dsm.c
diff --git a/src/arch/x86/acpigen_ps2_keybd.c b/src/acpi/acpigen_ps2_keybd.c
similarity index 100%
rename from src/arch/x86/acpigen_ps2_keybd.c
rename to src/acpi/acpigen_ps2_keybd.c
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index 2d00709..aa1f5fe 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -230,12 +230,6 @@
ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32)$(CONFIG_ARCH_RAMSTAGE_X86_64),y)
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpigen.c
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpigen_dsm.c
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi_device.c
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi_pld.c
-ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpigen_ps2_keybd.c
ramstage-$(CONFIG_HAVE_ACPI_RESUME) += acpi_s3.c
ramstage-$(CONFIG_ACPI_BERT) += acpi_bert_storage.c
ramstage-y += c_start.S
--
To view, visit https://review.coreboot.org/c/coreboot/+/40936
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I01470da8f8db9e28c1617e78fc3104e954f8af1a
Gerrit-Change-Number: 40936
Gerrit-PatchSet: 1
Gerrit-Owner: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-MessageType: newchange
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40934 )
Change subject: cpu: Add a helper function cpu_get_lapic_addr
......................................................................
cpu: Add a helper function cpu_get_lapic_addr
This change adds a helper function cpu_get_lapic_addr() that returns
LOCAL_APIC_ADDR for x86. It also adds a weak default implementation
which returns 0 if platform does not support LAPIC. This is being
done in preparation to move all ACPI table support in coreboot out of
arch/x86.
BUG=b:155428745
Change-Id: I2ad0c47472eb4cc2bcd9fdd11ca49abf318a5444
Signed-off-by: Furquan Shaikh <furquan(a)google.com>
---
M src/arch/x86/acpi.c
M src/arch/x86/cpu.c
M src/include/cpu/cpu.h
3 files changed, 16 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/40934/1
diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c
index 5c15a5e..8e7b51d 100644
--- a/src/arch/x86/acpi.c
+++ b/src/arch/x86/acpi.c
@@ -22,7 +22,6 @@
#include <device/pci.h>
#include <cbmem.h>
#include <commonlib/helpers.h>
-#include <cpu/x86/lapic_def.h>
#include <cpu/cpu.h>
#include <cbfs.h>
#include <version.h>
@@ -222,6 +221,15 @@
return lapic_nmi->length;
}
+__weak uintptr_t cpu_get_lapic_addr(void)
+{
+ /*
+ * If an architecture does not support LAPIC, this weak implementation returns LAPIC
+ * addr as 0.
+ */
+ return 0;
+}
+
void acpi_create_madt(acpi_madt_t *madt)
{
acpi_header_t *header = &(madt->header);
@@ -242,7 +250,7 @@
header->length = sizeof(acpi_madt_t);
header->revision = get_acpi_table_revision(MADT);
- madt->lapic_addr = LOCAL_APIC_ADDR;
+ madt->lapic_addr = cpu_get_lapic_addr();
if (CONFIG(ACPI_HAVE_PCAT_8259))
madt->flags |= 1;
diff --git a/src/arch/x86/cpu.c b/src/arch/x86/cpu.c
index 1ee8fb3..b523768 100644
--- a/src/arch/x86/cpu.c
+++ b/src/arch/x86/cpu.c
@@ -354,3 +354,8 @@
}
return -1;
}
+
+uintptr_t cpu_get_lapic_addr(void)
+{
+ return LOCAL_APIC_ADDR;
+}
diff --git a/src/include/cpu/cpu.h b/src/include/cpu/cpu.h
index cdb6817..db324b6 100644
--- a/src/include/cpu/cpu.h
+++ b/src/include/cpu/cpu.h
@@ -6,6 +6,7 @@
void cpu_initialize(unsigned int cpu_index);
/* Returns default APIC id based on logical_cpu number or < 0 on failure. */
int cpu_get_apic_id(int logical_cpu);
+uintptr_t cpu_get_lapic_addr(void);
/* Function to keep track of cpu default apic_id */
void cpu_add_map_entry(unsigned int index);
struct bus;
--
To view, visit https://review.coreboot.org/c/coreboot/+/40934
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I2ad0c47472eb4cc2bcd9fdd11ca49abf318a5444
Gerrit-Change-Number: 40934
Gerrit-PatchSet: 1
Gerrit-Owner: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: newchange
Name of user not set #1002873 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39893 )
Change subject: Documentation: Add proposal for firmware unit testing
......................................................................
Documentation: Add proposal for firmware unit testing
Signed-off-by: Jan Dabros <jsd(a)semihalf.com>
Change-Id: I552d6c3373219978b8e5fd4304f993d920425431
---
A Documentation/technotes/2020-03-unit-testing-coreboot.md
M Documentation/technotes/index.md
2 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39893/1
diff --git a/Documentation/technotes/2020-03-unit-testing-coreboot.md b/Documentation/technotes/2020-03-unit-testing-coreboot.md
new file mode 100644
index 0000000..fcc9b0c
--- /dev/null
+++ b/Documentation/technotes/2020-03-unit-testing-coreboot.md
@@ -0,0 +1,256 @@
+# Unit testing coreboot
+
+## Preface
+First part of this document, Introduction, comprises disambiguation for what
+unit testing is and what is not. This definition will be a basis for the whole
+paper.
+
+Next, Rationale, explains why to use unit testing and how coreboot specifically
+may benefit from it.
+
+This is followed by evaluation of different available free C unit test
+frameworks. Firstly, collection of requirements is provided. Secondly, there is
+a description of a few selected candidates. Finally, requirements are applied to
+candidates to see if they might be a good fit.
+
+Fourth part is a summary of evaluation, with proposal of unit test frameworks
+for coreboot to be used. This may be a good starting point for a discussion
+within community.
+
+Finally, Implementation proposal paragraph touches how build system and coreboot
+codebase in general should be organized, in order to support unit testing. This
+comprises couple of design considerations which need to be addressed.
+
+## Introduction
+A unit test is supposed to test a single unit of code in isolation. In C
+language (in contrary to OOP) unit usually means a function. One may also
+consider unit under test to be a single compilation unit which exposes some
+API (set of functions). A function, talking to some external component can be
+tested if this component can be mocked out.
+
+In other words (looking from C compilation angle), there should be no extra
+dependencies (executables) required beside unit under test and test harness in
+order to compile unit test binary. Test harness, beside code examining a
+routines, may comprise test framework implementation.
+
+Unit testing is not an integration testing and it doesn't replace it. First of
+all, integration tests cover larger set of components and interactions between
+them. Positive integration test result gives more confidence than a positive
+unit test does. Furthermore, unit tests are running on the build machine, while
+integration tests usually are executed on the target (or simulator).
+
+## Rationale
+Considering above, what is the benefit of unit testing, especially keeping in
+mind that coreboot is low-level firmware? Unit tests should be quick, thus may
+be executed frequently during development process. It is much easier to build
+and run a unit test on a build machine, than any integration test. This in turn
+may be used by dev to gather extra confidence early during code development
+process. Actually developer may even write unit tests earlier than the code -
+see [TDD](https://en.wikipedia.org/wiki/Test-driven_development) concept.
+
+That being said, unit testing embedded C code is a difficult task, due to
+significant amount of dependencies on underlying hardware. This dependencies
+may be broken using mocking concept, to some degree. However when mocks need to
+be too complex, then such a unit test cannot address an idea of being quick and
+simple.
+
+Writing unit tests for a code (both new and currently existing) may be favorable
+for the code quality. It is not only about finding bugs, but in general - easily
+testable code is a good code.
+
+coreboot benefits the most from testing common libraries (lib/, commonlib/,
+payloads/libpayload), coreboot infrastructure (console/, device/, security/) and
+to some extent hardware-related code.
+
+## Evaluation of unit testing frameworks
+
+### Requirements
+Requirements for unit testing frameworks:
+
+* Easy to use
+* Few dependencies
+
+ Standard C library is all we should need
+
+* Isolation between tests
+* Support for mocking
+* Easy to integrate with build system/build tools
+
+ JUnit-like XML output format for Jenkins
+
+* Compiler similarity
+
+ Ideally the same compiler should be used for building firmware executables
+ and test binaries, however this is very unlikely (even if the ARCH for build
+ machine and target is the same). At least the compiler on build machine should
+ support the same dialect as the compiler for target executables.
+
+* Popularity is a plus
+
+ The bigger community the better
+
+* Language similarity
+
+ Assumption that all firmware devs know C
+
+* Extra features may be a plus
+* Proper license
+
+ This should not be a blocker, since test binaries are not distributed.
+ However ideally GPL.
+
+### Candidates
+There is a lot of frameworks which allow unit testing C code
+([list](https://en.wikipedia.org/wiki/List_of_unit_testing_frameworks#C) from
+Wikipedia). While not all of them were evaluated for obvious reasons, couple
+of them were selected based on the good opinions among C devs, popularity and
+fitting above criteria.
+
+* [SputUnit](https://www.use-strict.de/sput-unit-testing/)
+* [GoogleTest](https://github.com/google/googletest)
+* [Cmocka](https://cmocka.org/)
+* [Unity](http://www.throwtheswitch.org/unity) (CMock, Ceedling)
+
+Three more frameworks should be mentioned here, however they weren't tried
+within coreboot:
+ * [Check](https://libcheck.github.io/check/)
+ * [Criterion](https://github.com/Snaipe/Criterion/)
+
+They offer similar functionality as Unity, while at the same time don’t seem
+to have similarly active communities.
+
+ * [CUnit](http://cunit.sourceforge.net/)
+
+Another one which is rather widely used, however it doesn't have good support
+for mocking.
+
+If anyone has good experience with some framework which is not listed above, it
+will be highly appreciated to give a note about this to the author. In such
+cases, it may be necessary to investigate such tools deeply and then reconsider
+the decision.
+
+### Evaluation
+* [SputUnit](https://www.use-strict.de/sput-unit-testing/)
+ * Pros
+ * No dependencies, one header file to include - that’s all
+ * Pure C
+ * Very easy to use
+ * Cons
+ * Main repo doesn’t have support for generating JUnit XML reports for
+ Jenkins to consume - this feature is available only on the fork from
+ SputUnit called “Sput_report”. It makes it niche in a niche, so there are
+ some reservations whether support for this will be satisfying
+ * No support for mocks
+ * Not too popular
+ * No automatic test registration
+ * BSD license
+* [GoogleTest](https://github.com/google/googletest)
+ * Pros
+ * Automatic test registration
+ * Support for different output formats (including XML for Jenkins)
+ * Good support, widely used, the biggest and the most active community ouf
+ of all frameworks that were investigated
+ * Available as a package in the most common distributions
+ * Test fixtures easily available
+ * Well documented
+ * Easy to integrate with an IDE
+ * Cons
+ * Requires C++11 compiler
+ * To make most out of it (use GMock) C++ knowledge is required
+ * BSD license
+* [Cmocka](https://cmocka.org/)
+ * Pros
+ * Self-contained, autonomous framework
+ * Pure C
+ * API is well documented
+ * Multiple output formats (including XML for Jenkins)
+ * Available as a package in the most common distributions
+ * Used in some popular open source projects (libssh, OpenVPN, Samba)
+ * Test fixtures available
+ * Support for exception handling
+ * Cons
+ * No automatic test registration
+ * It will require some effort to make it works from within an IDE
+ * Apache 2.0 license (not compatible with GPLv2)
+* [Unity](http://www.throwtheswitch.org/unity) (CMock, Ceedling)
+ * Pros
+ * Pure C (Unity testing framework itself, not test runner)
+ * Support for different output formats (including XML for Jenkins)
+ * There are some (rather easy) hints how to use this from an IDE (e.g. Eclipse)
+ * Cons
+ * Test runner (Ceedling) is not written in C - uses Ruby
+ * Mocking/Exception handling functionalities are actually separate tools
+ * No automatic test registration
+ * Not too popular
+
+### Summary & framework proposal
+After research, proposal is to go with Cmocka unit test framework. It fulfills
+all evaluation criteria we have. It is rather easy to use, doesn’t have
+extra dependencies, written fully in C, allows for tests fixtures and some
+popular open source projects already are using it. Cmocka also include support
+for mocks.
+
+There are some cons, like for example lack of automatic test registration,
+however this is minor issue, just requires minimal additional work from
+developer. In the same time, it may be worth to propose improvement to Cmocka
+community or simply apply some extra wrapper with demanded functionality.
+
+That being said, Unity and GoogleTest frameworks are also very good. However
+GoogleTest requires C++ familiarity and compiler, and Unity is rather not too
+popular framework (which doesn’t mean worse). It is worth to mention, that such
+evaluation will always be impacted by author subjective point of view.
+
+## Implementation proposal
+
+### Framework as a submodule or external package
+Unit test frameworks may be either compiled from source (form a git submodule
+under 3rdparty/) or pre-compiled as a package. The second option seems to be
+easier to maintain, while in the same time may bring some unwanted consequences
+(different version across distributions, frequent changes in API). It make sense
+to initially experiment with packages and check how it works. If this will
+brings any issues, then it is always possible to switch to submodule approach.
+
+### Build configuration
+While building unit under test object file, it is necessary to apply some
+configuration (config) just like when building whole firmware. For simplicity,
+one config.h may be pre-built and then used for building all unit tests. In the
+same time, if integrated with Jenkins, it may be preferred to use every
+available config for periodic builds.
+
+My proposal is to go with `qemu_x86_i440fx` config for usual developer builds.
+
+### Integration with build system
+To get the most out of unit testing framework, it should be integrated with
+Jenkins automation server. Verification of all unit tests for new changes may
+improve code reliability to some extent.
+
+### Directory structure
+Tests should be kept separate from the code, while in the same time it must be
+easy to match code with test harness.
+
+My proposal is to create new directory for test files and every test file name
+should have added a prefix "-test" to the basename of a corresponding unit
+under test module. Below example shows how this may be organized:
+
+```bash
+├── build
+│ ├── tests <-test binaries
+│
+├── src
+│ ├── lib
+│ │ ├── malloc.c <- unit under test
+│ │ ├── string.c <- unit under test
+│ ├── commonlib
+│ ├── sort.c <- unit under test
+│
+├── tests
+│ ├── include
+│ │ ├── config.h <- pre-built config used for tests' builds
+│ ├── Makefile.inc
+│ ├── lib
+│ │ ├── malloc-test.c <- test code for src/lib/malloc.c
+│ │ ├── string-test.c <- test code for src/lib/string.c
+│ ├── commonlib
+│ ├── sort-test.c <- test code for src/commonlib/sort.c
+├──
+```
diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md
index 7c231fc..5367e69 100644
--- a/Documentation/technotes/index.md
+++ b/Documentation/technotes/index.md
@@ -2,3 +2,4 @@
* [Dealing with Untrusted Input in SMM](2017-02-dealing-with-untrusted-input-in-smm.md)
* [Rebuilding coreboot image generation](2015-11-rebuilding-coreboot-image-generation.md)
+* [Unit testing coreboot](2020-03-unit-testing-coreboot.md)
--
To view, visit https://review.coreboot.org/c/coreboot/+/39893
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I552d6c3373219978b8e5fd4304f993d920425431
Gerrit-Change-Number: 39893
Gerrit-PatchSet: 1
Gerrit-Owner: Name of user not set #1002873
Gerrit-MessageType: newchange