coreboot-gerrit April 2019

coreboot-gerrit@coreboot.org

1 participants
1219 discussions

Change in ...coreboot[master]: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2
by Frans Hendriks (Code Review) 11 Apr '19

11 Apr '19

Hello Patrick Rudolph, Aaron Durbin, Nathaniel L Desimone, Subrata Banik, Matt DeVillier, build bot (Jenkins), Hannah Williams, Patrick Georgi, David Guckian, Huang Jin, Lee Leahy, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/29661 to look at the new patch set (#9). Change subject: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2 ...................................................................... {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2 In soc_silicon_init_params() and soc_display_silicon_init_params() SILICON_INIT_UPD element are used which do not exist in MR2. Modify these functions using MR2 elements only. Configuration of 'pre-MR2' elements is placed in mainboard code to be backwards compatible. For Braswell set FSP_VENDORCODE_HEADER_PATH to "../../../3rdparty/fsp/BraswellFspBinPkg" BUG=NA TEST=Portwell PQ7-M107 Change-Id: Id40b5d46ddda93845d9739b56aaf7ad24ee89246 Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com> --- M src/drivers/intel/fsp1_1/Makefile.inc M src/mainboard/google/cyan/Makefile.inc A src/mainboard/google/cyan/ramstage.c M src/mainboard/intel/strago/ramstage.c M src/soc/intel/braswell/Makefile.inc M src/soc/intel/braswell/chip.c 6 files changed, 98 insertions(+), 33 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/29661/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/29661 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id40b5d46ddda93845d9739b56aaf7ad24ee89246 Gerrit-Change-Number: 29661 Gerrit-PatchSet: 9 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: David Guckian Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in ...coreboot[master]: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2
by Frans Hendriks (Code Review) 11 Apr '19

11 Apr '19

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29661 ) Change subject: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2 ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/#/c/29661/8/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc: https://review.coreboot.org/#/c/29661/8/src/soc/intel/braswell/Makefile.inc… PS8, Line 61: CPPFLAGS_common += -I$(src)/vendorcode/intel/$(FSP_PATH)/include > Shouldnt it point to 3rdparty submodule? To be compatible with vendorcode where this CONFIG_FSP_VENDCODE_HEADER_PATH is used for baytrail and rangeley also I used CONFIG_FSP_VENDORCODE_HEADER_PATH="../../../3rdparty/fsp/BraswellFspBinPkg" for Braswell. This will point to 3rdparty. Will add CONFIG string to comment of this patch. -- To view, visit https://review.coreboot.org/c/coreboot/+/29661 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id40b5d46ddda93845d9739b56aaf7ad24ee89246 Gerrit-Change-Number: 29661 Gerrit-PatchSet: 8 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: David Guckian Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 11 Apr 2019 13:49:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2
by Michał Żygowski (Code Review) 11 Apr '19

11 Apr '19

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29661 ) Change subject: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2 ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/#/c/29661/8/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc: https://review.coreboot.org/#/c/29661/8/src/soc/intel/braswell/Makefile.inc… PS8, Line 61: CPPFLAGS_common += -I$(src)/vendorcode/intel/$(FSP_PATH)/include Shouldnt it point to 3rdparty submodule? -- To view, visit https://review.coreboot.org/c/coreboot/+/29661 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id40b5d46ddda93845d9739b56aaf7ad24ee89246 Gerrit-Change-Number: 29661 Gerrit-PatchSet: 8 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: David Guckian Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 11 Apr 2019 13:40:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: soc/intel/braswell/acpi/lpss.asl: Remove SPI1 and PWM asl code
by Michał Żygowski (Code Review) 11 Apr '19

11 Apr '19

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29417 ) Change subject: soc/intel/braswell/acpi/lpss.asl: Remove SPI1 and PWM asl code ...................................................................... Patch Set 4: Matt, have You possibly tested the patch already? -- To view, visit https://review.coreboot.org/c/coreboot/+/29417 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iec2ca7520081d00bf7a53d58ee054aa6f23e5606 Gerrit-Change-Number: 29417 Gerrit-PatchSet: 4 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Thu, 11 Apr 2019 13:23:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: soc/intel/braswell: Remove disabled LPE ACPI code
by Frans Hendriks (Code Review) 11 Apr '19

11 Apr '19

Hello Patrick Rudolph, Piotr Król, build bot (Jenkins), Hannah Williams, Michał Żygowski, Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/29414 to look at the new patch set (#8). Change subject: soc/intel/braswell: Remove disabled LPE ACPI code ...................................................................... soc/intel/braswell: Remove disabled LPE ACPI code The ACPI code for LPE device was included regardless of the availability of the LPE controller. Move the LPE ACPI code to separate SSDT and hide it when LPE is disabled. BUG=N/A TEST=Intel CherryHill CRB Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73 Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com> --- M src/mainboard/google/cyan/Makefile.inc M src/mainboard/google/cyan/acpi/codec_maxim.asl M src/mainboard/google/cyan/acpi/codec_realtek.asl A src/mainboard/google/cyan/acpi/jack_board.asl A src/mainboard/google/cyan/acpi/jack_detect.asl A src/mainboard/google/cyan/ssdtlpe.asl A src/mainboard/google/cyan/variants/banon/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/celes/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/cyan/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/edgar/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/kefka/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/reks/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/relm/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/setzer/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/terra/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/ultima/include/variant/acpi/lpe.asl A src/mainboard/google/cyan/variants/wizpig/include/variant/acpi/lpe.asl M src/mainboard/intel/strago/Makefile.inc M src/mainboard/intel/strago/acpi/mainboard.asl A src/mainboard/intel/strago/ssdtlpe.asl M src/soc/intel/braswell/acpi/southcluster.asl M src/soc/intel/braswell/include/soc/acpi.h M src/soc/intel/braswell/lpe.c 23 files changed, 392 insertions(+), 37 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/29414/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/29414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73 Gerrit-Change-Number: 29414 Gerrit-PatchSet: 8 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in ...coreboot[master]: soc/intel/braswell/acpi/lpss.asl: Remove SPI1 and PWM asl code
by Frans Hendriks (Code Review) 11 Apr '19

11 Apr '19

Hello Patrick Rudolph, Piotr Król, build bot (Jenkins), Michał Żygowski, Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/29417 to look at the new patch set (#4). Change subject: soc/intel/braswell/acpi/lpss.asl: Remove SPI1 and PWM asl code ...................................................................... soc/intel/braswell/acpi/lpss.asl: Remove SPI1 and PWM asl code Linux remains using SPI1 and PWM ASL even if these devices are disabled. SPI1 and PWM are disabled by Intel FSP. Remove ASL code. BUG=N/A TEST=Boot Ubuntu on Intel CherryHill CRB Change-Id: Iec2ca7520081d00bf7a53d58ee054aa6f23e5606 Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com> --- M src/soc/intel/braswell/acpi/lpss.asl 1 file changed, 1 insertion(+), 109 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/29417/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/29417 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iec2ca7520081d00bf7a53d58ee054aa6f23e5606 Gerrit-Change-Number: 29417 Gerrit-PatchSet: 4 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in ...coreboot[master]: soc/intel/braswell/chip.c: Configure LPSS devices in correct mode
by Frans Hendriks (Code Review) 11 Apr '19

11 Apr '19

Hello Patrick Rudolph, Aaron Durbin, Piotr Król, Paul Menzel, build bot (Jenkins), Hannah Williams, Michał Żygowski, Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/29284 to look at the new patch set (#4). Change subject: soc/intel/braswell/chip.c: Configure LPSS devices in correct mode ...................................................................... soc/intel/braswell/chip.c: Configure LPSS devices in correct mode LPSS devices can be configured in ACPI or PCI mode. The correct mode must be reported to FSP. Use config structure to report the correct mode. BUG=N/A TEST=Intel Cherry Hill Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8 Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com> --- M src/soc/intel/braswell/chip.c 1 file changed, 23 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/29284/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/29284 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8 Gerrit-Change-Number: 29284 Gerrit-PatchSet: 4 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in ...coreboot[master]: mainboard/facebook/fbg1701: Do initial mainboard commit
by Michał Żygowski (Code Review) 11 Apr '19

11 Apr '19

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30414 ) Change subject: mainboard/facebook/fbg1701: Do initial mainboard commit ...................................................................... Patch Set 17: (22 comments) Regarding the measured boot, I have looked a little bit at Your patches Frans and I saw crypto lib that is already implemented in the vboot submodule. IMO it should be more generic and reuse as much as possible from vboot submodule. Duplicating code is rather not necessary. I understand that Your implementation was submitted first to gerrit, however, it would be great if You could integrate Your work into the current measured boot implementation. As it still is not perfect and complete (for example in terms of the TPM2.0 support), You have the field to shine on. :) If You really have something in Your implementation that makes Your measured boot implementation the only option for this mainboard, please tell us. I am adding Philipp Deppenwiese on CC so he can advise how to proceed with integration. Philipp could You please point the right direction to Frans? https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/f… File Documentation/mainboard/facebook/fbg1701.md: https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/f… PS17, Line 7: This board currently requires Braswell fsp blob Is microcode required? https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/f… PS17, Line 21: The system has an external flash chip which is a 8 MiB soldered SOIC-8 chip. It What is the purpose of the external flash? Which one is used as firmware storage? https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/f… PS17, Line 32: - flashing coreboot Why untested? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Kco… File src/mainboard/facebook/fbg1701/Kconfig: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Kco… PS17, Line 1: ## Consider committing a miniconfig file to get rid of a few configuration settings here https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Kco… PS17, Line 43: config MAINBOARD_SPD0_FILE_NAME Please use GENERIC_SPD_BIN option in board specific options https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Mak… File src/mainboard/facebook/fbg1701/Makefile.inc: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Mak… PS17, Line 27: ramstage-y += logo.c Why not use such notation: ramstage-$(CONFIG_FSP1_1_DISPLAY_LOGO) += logo.c And remove the #if IS_ENABLED(CONFIG_FSP1_1_DISPLAY_LOGO) macro from logo.c ? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Mak… PS17, Line 41: ## Please use GENERIC_SPD_BIn and define SPD_SOURCES to point to SPD files. https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acp… File src/mainboard/facebook/fbg1701/acpi/dptf.asl: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acp… PS17, Line 1: /* Is this ASL file included anywhere? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acp… File src/mainboard/facebook/fbg1701/acpi/superio.asl: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acp… PS17, Line 21: Device (COM1) { Any reason for the indentation? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boa… File src/mainboard/facebook/fbg1701/board_info.txt: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boa… PS17, Line 3: Category: misc Does the board really not fit into any category? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boo… File src/mainboard/facebook/fbg1701/bootblock.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boo… PS17, Line 27: #if IS_ENABLED(CONFIG_VERIFIED_BOOT) coreboot uses now CONFIG() macro. https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/com… File src/mainboard/facebook/fbg1701/com_init.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/com… PS17, Line 24: #if !IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) Please use CONFIG() macro https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/dev… File src/mainboard/facebook/fbg1701/devicetree.cb: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/dev… PS17, Line 75: register "DptfDisable" = "1" Why DPTF is disabled here but enabled in domain 0? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/dev… PS17, Line 102: device pci 14.0 on end # 8086 22b5 - USB XHCI - Only 1 USB controller at a time Consider adding SATA device with off state https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/fma… File src/mainboard/facebook/fbg1701/fmap.fmd: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/fma… PS17, Line 1: # layout for firmware residing at top of 4GB address space If the mainboard is intended to use VBOOT I also recommend to provide second fmd file for that case. https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpi… File src/mainboard/facebook/fbg1701/gpio.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpi… PS17, Line 116: NATIVE_PU1K_CSEN_INVTX(1), /* 61 I2C0_SDA */ All I2C devices are disabled, why configuring the pins for I2C? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpi… PS17, Line 120: NATIVE_PU1K_CSEN_INVTX(1), /* 65 I2C0_SCL */ a/a https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpi… PS17, Line 256: Empty line, please remove https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/log… File src/mainboard/facebook/fbg1701/logo.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/log… PS17, Line 36: size_t file_size = cbfs_boot_load_file(filename, logo_data, Why not cbfs_boot_map_with_leak? This hardcoded logo_data array looks a little bit terrible. https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/ram… File src/mainboard/facebook/fbg1701/ramstage.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/ram… PS17, Line 21: #if IS_ENABLED(CONFIG_FSP1_1_DISPLAY_LOGO) Why is this split into two blocks with macro? https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/rom… File src/mainboard/facebook/fbg1701/romstage.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/rom… PS17, Line 41: ps->spd_data_ch0 = cbfs_boot_map_with_leak(buf, CBFS_TYPE_SPD, NULL); Change filename to spd.bin after selecting GENERIC_SPD_BIN https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/smi… File src/mainboard/facebook/fbg1701/smihandler.c: https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/smi… PS17, Line 60: case ACPI_S3: Board does not seem to support S3, why is this case implemented? -- To view, visit https://review.coreboot.org/c/coreboot/+/30414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28ac78a630ee705b1e546031f024bfe7f952ab39 Gerrit-Change-Number: 30414 Gerrit-PatchSet: 17 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> 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: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Comment-Date: Thu, 11 Apr 2019 12:38:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: option table: Don't pad string entries with garbage
by Thomas Heijligen (Code Review) 11 Apr '19

11 Apr '19

Hello Nico Huber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/31348 to review the following change. Change subject: option table: Don't pad string entries with garbage ...................................................................... option table: Don't pad string entries with garbage set_option_with() expects a buffer of the exact size of the option. Change-Id: I21332394f88cf2daa4f733a544627d6d3c6ef26c Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M payloads/libpayload/drivers/options.c 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/31348/1 diff --git a/payloads/libpayload/drivers/options.c b/payloads/libpayload/drivers/options.c index 70c2b17..2b0a42e 100644 --- a/payloads/libpayload/drivers/options.c +++ b/payloads/libpayload/drivers/options.c @@ -357,7 +357,11 @@ *(u64*)raw = strtoull(value, NULL, 0); break; case 's': - raw = strdup(value); + raw = malloc(cmos_entry->length); + if (!raw) + return 1; + memset(raw, 0x00, cmos_entry->length); + strncpy(raw, value, cmos_entry->length); break; case 'e': cmos_enum = lookup_cmos_enum_by_label(option_table, cmos_entry->config_id, value); -- To view, visit https://review.coreboot.org/c/coreboot/+/31348 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I21332394f88cf2daa4f733a544627d6d3c6ef26c Gerrit-Change-Number: 31348 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: libpayload/storage: Add Apollo Lake AHCI ID
by Thomas Heijligen (Code Review) 11 Apr '19

11 Apr '19

Hello Nico Huber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/31347 to review the following change. Change subject: libpayload/storage: Add Apollo Lake AHCI ID ...................................................................... libpayload/storage: Add Apollo Lake AHCI ID Change-Id: Iee244d0cd7d64934fbfc34778a45e21e97646628 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M payloads/libpayload/drivers/storage/ahci.c 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/31347/1 diff --git a/payloads/libpayload/drivers/storage/ahci.c b/payloads/libpayload/drivers/storage/ahci.c index bb0d37e..6cc3866 100644 --- a/payloads/libpayload/drivers/storage/ahci.c +++ b/payloads/libpayload/drivers/storage/ahci.c @@ -224,6 +224,7 @@ 0x8086 | 0x1c03 << 16, /* Mobile Cougar Point PCH */ 0x8086 | 0x1e03 << 16, /* Mobile Panther Point PCH */ 0x8086 | 0xa102 << 16, /* Desktop / Mobile-Wks Sunrise Point PCH */ + 0x8086 | 0x5ae3 << 16, /* Apollo Lake */ }; #endif static void ahci_init_pci(pcidev_t dev) -- To view, visit https://review.coreboot.org/c/coreboot/+/31347 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iee244d0cd7d64934fbfc34778a45e21e97646628 Gerrit-Change-Number: 31347 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

3 4

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2019