coreboot-gerrit February 2020

coreboot-gerrit@coreboot.org

1 participants
1796 discussions

Change in coreboot[master]: mb/dell: Add Dell Optiplex 790
by HAOUAS Elyes (Code Review) 03 Feb '20

03 Feb '20

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/21774 ) Change subject: mb/dell: Add Dell Optiplex 790 ...................................................................... Patch Set 60: (1 comment) https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optipl… File src/mainboard/dell/optiplex_790/romstage.c: https://review.coreboot.org/c/coreboot/+/21774/60/src/mainboard/dell/optipl… PS60, Line 17: : #include <stdint.h> : #include <string.h> : #include <timestamp.h> : #include <arch/byteorder.h> : #include <arch/io.h> : #include <device/mmio.h> : #include <device/pci_ops.h> : #include <device/pnp_ops.h> : #include <cpu/x86/lapic.h> : #include <console/console.h> : #include <northbridge/intel/sandybridge/sandybridge.h> : #include <northbridge/intel/sandybridge/raminit_native.h> : #include <southbridge/intel/bd82x6x/pch.h> : #include <southbridge/intel/common/gpio.h> include only what you use. please remove unused includes -- To view, visit https://review.coreboot.org/c/coreboot/+/21774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If3d3a13163d5da1368259a7498019d42fb3ed57f Gerrit-Change-Number: 21774 Gerrit-PatchSet: 60 Gerrit-Owner: Christoph Pomaska <github(a)slrie.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christoph Pomaska <github(a)slrie.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Mimoja <coreboot(a)mimoja.de> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 03 Feb 2020 16:55:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/dell: Add Dell Optiplex 790
by Christoph Pomaska (Code Review) 03 Feb '20

03 Feb '20

Hello Kyösti Mälkki, Patrick Rudolph, Felix Held, Angel Pons, Mimoja, Arthur Heymans, Paul Menzel, Jonathan Neuschäfer, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/21774 to look at the new patch set (#60). Change subject: mb/dell: Add Dell Optiplex 790 ...................................................................... mb/dell: Add Dell Optiplex 790 This port was generated by autoport and has been tweaked (see below) There are (at least) three different mainboards: - DT: 4 RAM slots, PEG, PCIe x1, PCI, PCIe x16 - MT: 4 RAM slots, PEG, PCIe x1, PCI, PCIe x16 - SFF: 4 RAM slots, PEG, PCIe x16 - USFF: 2 RAM slots, mPCIe The variants have different PCI/PCIe configurations and are not exactly compatible towards each other. This port has been tested with: SFF, USFF What works: - Booting Arch Linux with kernel 5.2.3-arch1-1-ARCH - PCIe (Hotplug on PCIe X1 only!) - Onboard graphics with libgfxinit (via VGA) - Turning on and off (S5) - Harddrive activity LED - Onboard sound - Onboard ethernet - SATA (hotplug) - IOMMU - Suspend/resume (S3) - EHCI debug (rear side, bottom port on the block with ethernet) What does not work: - SuperI/O (Chip is a SCH5544-NS) - Serial port - PS/2 - Fan control (fans go to full speed) - VBT is missing Further notes: - Default IFD settings block reads/writes to some regions. This can be bypassed by plugging the SERVICE_MODE jumper. BIOS version A05 does not set any protected ranges, so internal flashing is possible (use a layout if the SERVICE_MODE jumper is not plugged). - Setting the jumper slows down the boot process of coreboot significantly, as coreboot waits 900ms for the ME to report an OK DRAM (which doesn't happen with the jumper set) - The controller that controls the POST code LEDs on the front of the case (likely the SuperIO) stays on slow blinking POST 234, corrupt or defect BIOS according to [1]. - The mainboard has one SOIC16 8192KiB and one SOIC8 2048KiB BIOS chip that are recognized as one "Opaque flash chip" of 10240K in size. [1] http://www.dell.com/support/article/us/en/04/sln284978/a-reference-guide-to… Change-Id: If3d3a13163d5da1368259a7498019d42fb3ed57f Signed-off-by: Christoph Pomaska <github(a)aufmachen.jetzt> Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- A Documentation/mainboard/dell/optiplex790.md A Documentation/mainboard/dell/servicemode.md M Documentation/mainboard/index.md A src/mainboard/dell/Kconfig A src/mainboard/dell/Kconfig.name A src/mainboard/dell/optiplex_790/Kconfig A src/mainboard/dell/optiplex_790/Kconfig.name A src/mainboard/dell/optiplex_790/Makefile.inc A src/mainboard/dell/optiplex_790/acpi/ec.asl A src/mainboard/dell/optiplex_790/acpi/platform.asl A src/mainboard/dell/optiplex_790/acpi/superio.asl A src/mainboard/dell/optiplex_790/acpi_tables.c A src/mainboard/dell/optiplex_790/board_info.txt A src/mainboard/dell/optiplex_790/dsdt.asl A src/mainboard/dell/optiplex_790/gma-mainboard.ads A src/mainboard/dell/optiplex_790/hda_verb.c A src/mainboard/dell/optiplex_790/mainboard.c A src/mainboard/dell/optiplex_790/romstage.c A src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/devicetree.cb A src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/gpio.c A src/mainboard/dell/optiplex_790/variants/optiplex_790_mt-sff/hda_verb.c A src/mainboard/dell/optiplex_790/variants/optiplex_790_usff/devicetree.cb A src/mainboard/dell/optiplex_790/variants/optiplex_790_usff/gpio.c A src/mainboard/dell/optiplex_790/variants/optiplex_790_usff/hda_verb.c 24 files changed, 1,220 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/21774/60 -- To view, visit https://review.coreboot.org/c/coreboot/+/21774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If3d3a13163d5da1368259a7498019d42fb3ed57f Gerrit-Change-Number: 21774 Gerrit-PatchSet: 60 Gerrit-Owner: Christoph Pomaska <github(a)slrie.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christoph Pomaska <github(a)slrie.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Mimoja <coreboot(a)mimoja.de> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: drivers/generic/gfx: Add null pointer error check
by Jacob Garber (Code Review) 03 Feb '20

03 Feb '20

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38669 ) Change subject: drivers/generic/gfx: Add null pointer error check ...................................................................... drivers/generic/gfx: Add null pointer error check acpi_device_scope() will return NULL if it is unable to find the path of the parent device. Return early if this is the case to prevent a null pointer dereference. Change-Id: I3eff1c1e3477c75c7130b52898de7d59692ba412 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 1409672 --- M src/drivers/generic/gfx/gfx.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/38669/1 diff --git a/src/drivers/generic/gfx/gfx.c b/src/drivers/generic/gfx/gfx.c index b2bda4a..0386e9b 100644 --- a/src/drivers/generic/gfx/gfx.c +++ b/src/drivers/generic/gfx/gfx.c @@ -69,6 +69,9 @@ const char *scope = acpi_device_scope(dev); + if (!scope) + return; + acpigen_write_scope(scope); /* Method (_DOD, 0) */ -- To view, visit https://review.coreboot.org/c/coreboot/+/38669 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3eff1c1e3477c75c7130b52898de7d59692ba412 Gerrit-Change-Number: 38669 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

4 3

Change in coreboot[master]: nb/intel/sandybridge/raminit: Add ECC detection support
by Jonathan Kollasch (Code Review) 03 Feb '20

03 Feb '20

Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22214 ) Change subject: nb/intel/sandybridge/raminit: Add ECC detection support ...................................................................... Patch Set 9: Code-Review+1 (1 comment) https://review.coreboot.org/c/coreboot/+/22214/8/src/northbridge/intel/sand… File src/northbridge/intel/sandybridge/raminit_common.c: https://review.coreboot.org/c/coreboot/+/22214/8/src/northbridge/intel/sand… PS8, Line 588: presedence > Should probably be spelled "precedence". Done -- To view, visit https://review.coreboot.org/c/coreboot/+/22214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5b7599746195cfa996a48320404a8dbe6820483a Gerrit-Change-Number: 22214 Gerrit-PatchSet: 9 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 03 Feb 2020 15:48:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP]nb/intel/sandybridge/raminit: Add ECC support
by build bot (Jenkins) (Code Review) 03 Feb '20

03 Feb '20

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22215 ) Change subject: [WIP]nb/intel/sandybridge/raminit: Add ECC support ...................................................................... Patch Set 10: (1 comment) https://review.coreboot.org/c/coreboot/+/22215/10/src/northbridge/intel/san… File src/northbridge/intel/sandybridge/raminit_common.c: https://review.coreboot.org/c/coreboot/+/22215/10/src/northbridge/intel/san… PS10, Line 3135: for (row = 0; row < rowsize; row += 16) { suspect code indent for conditional statements (24, 16) -- To view, visit https://review.coreboot.org/c/coreboot/+/22215 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1206746332c9939a78b67e7b48d3098bdef8a2ed Gerrit-Change-Number: 22215 Gerrit-PatchSet: 10 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Christoph Pomaska <github(a)slrie.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 03 Feb 2020 15:46:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP]nb/intel/sandybridge/raminit: Add ECC support
by Jonathan Kollasch (Code Review) 03 Feb '20

03 Feb '20

Jonathan Kollasch has uploaded a new patch set (#10) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/22215 ) Change subject: [WIP]nb/intel/sandybridge/raminit: Add ECC support ...................................................................... [WIP]nb/intel/sandybridge/raminit: Add ECC support Add ECC support for native raminit on SandyBridge/IvyBridge. Change-Id: I1206746332c9939a78b67e7b48d3098bdef8a2ed Signed-off-by: Patrick Rudolph <siro(a)das-labor.org> --- M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_ivy.c M src/northbridge/intel/sandybridge/raminit_sandy.c 6 files changed, 100 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/22215/10 -- To view, visit https://review.coreboot.org/c/coreboot/+/22215 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1206746332c9939a78b67e7b48d3098bdef8a2ed Gerrit-Change-Number: 22215 Gerrit-PatchSet: 10 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Christoph Pomaska <github(a)slrie.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: nb/intel/sandybridge/raminit: Add ECC detection support
by Jonathan Kollasch (Code Review) 03 Feb '20

03 Feb '20

Jonathan Kollasch has uploaded a new patch set (#9) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/22214 ) Change subject: nb/intel/sandybridge/raminit: Add ECC detection support ...................................................................... nb/intel/sandybridge/raminit: Add ECC detection support Add support for detection ECC capability and forced ECC mode. Print the ECC mode in verbose debugging mode. Change-Id: I5b7599746195cfa996a48320404a8dbe6820483a Signed-off-by: Patrick Rudolph <siro(a)das-labor.org> Signed-off-by: Alexander Couzens <lynxis(a)fe80.eu> Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h 3 files changed, 45 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/22214/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/22214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5b7599746195cfa996a48320404a8dbe6820483a Gerrit-Change-Number: 22214 Gerrit-PatchSet: 9 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: nb/intel/sandybridge/raminit: Add ECC detection support
by Jonathan Kollasch (Code Review) 03 Feb '20

03 Feb '20

Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22214 ) Change subject: nb/intel/sandybridge/raminit: Add ECC detection support ...................................................................... Patch Set 8: (1 comment) > Patch Set 8: > > (1 comment) https://review.coreboot.org/c/coreboot/+/22214/8//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/22214/8//COMMIT_MSG@9 PS8, Line 9: forced > I don't see what makes "forced" a frustration-charged term. In any case, "required" is too vague. […] Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/22214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5b7599746195cfa996a48320404a8dbe6820483a Gerrit-Change-Number: 22214 Gerrit-PatchSet: 8 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 03 Feb 2020 15:41:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Jonathan Kollasch <jakllsch(a)kollasch.net> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add bootblock common stage file
by Patrick Rudolph (Code Review) 03 Feb '20

03 Feb '20

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18457 ) Change subject: soc/intel/common: Add bootblock common stage file ...................................................................... Patch Set 38: (2 comments) https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… File src/soc/intel/common/basecode/bootblock/bootblock.c: https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… PS37, Line 39: bootblock_soc_early_init(void) > I think this is just to allow arbitrary function names. […] The platform you mention doesn't support this kind of runtime detection. You have to specify the exact generation, otherwise it won't boot. For example: * It doesn't include all microcodes * It has 3 different FSP for 4 different socs https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… PS37, Line 102: (CONFIG(PAGING_IN_CACHE_AS_RAM)) > I think APL/GLK differ too much from the big core platforms to have common code everywhere. […] you consider 3 lines of code as too different? -- To view, visit https://review.coreboot.org/c/coreboot/+/18457 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If84c08d33f6f8fd3cd9722ee893653f1d1ae90c1 Gerrit-Change-Number: 18457 Gerrit-PatchSet: 38 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.corp-partner.google.com> Gerrit-Reviewer: Usha P <usha.p(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Varun Joshi <varun.joshi(a)intel.com> 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: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 03 Feb 2020 13:11:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro(a)das-labor.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add bootblock common stage file
by Angel Pons (Code Review) 03 Feb '20

03 Feb '20

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18457 ) Change subject: soc/intel/common: Add bootblock common stage file ...................................................................... Patch Set 38: (3 comments) https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… File src/soc/intel/common/basecode/bootblock/bootblock.c: https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… PS37, Line 39: bootblock_soc_early_init(void) > This reminds me on UEFI implementations that use function pointers everywhere […] I think this is just to allow arbitrary function names. To allow that, I would just use thunk functions inside SoC-specific code: void bootblock_soc_early_init(void) { bootblock_my_ultra_awesome_soc_early_init(); } In the above example, things can be simplified by a rename. However, when multiple SoCs are supported with the same code, things get more complex. For instance, soc/intel/cannonlake supports Cannonlake, Coffeelake, Whiskeylake and Cometlake. The code on that SoC could be something like this: void bootblock_soc_early_init(void) { switch (get_soc_generation()) { case SOC_GENERATION_CANNONLAKE: bootblock_cnl_early_init(); break; case SOC_GENERATION_COFFEELAKE: case SOC_GENERATION_WHISKEYLAKE: bootblock_cfl_whl_early_init(); break; case SOC_GENERATION_COMETLAKE: bootblock_cml_early_init(); break; default: die("%s: Unsupported SoC generation!", __func__); } } This is a good idea if the same coreboot binary has to support more than one generation of SoC, as the specific generation can only be determined at runtime. However, if only one generation is supported, it is known at build-time, so things can be improved even further using something like the variants mechanism for the mainboards. This allows building only the necessary files, and enables build-time checking. Using build-time checking, the Makefiles sanity-check the selected Kconfig options and abort the build process if they are invalid. Also, if a SoC generation has not implemented the required functions, it will result in a linker error. And thanks to Jenkins, this will be checked on every change, so accidentally breaking something is harder :) https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… PS37, Line 46: die("%s has not been implemented by SoC", __func__); With build-time checking, these errors would just be linker errors. https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/base… PS37, Line 102: (CONFIG(PAGING_IN_CACHE_AS_RAM)) > Why is there APL/GLK specific code here? Wasn't the idea to make it more flexible? I think APL/GLK differ too much from the big core platforms to have common code everywhere. If the next little core SoCs also need this, I would then consider an abstraction, but not now. -- To view, visit https://review.coreboot.org/c/coreboot/+/18457 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If84c08d33f6f8fd3cd9722ee893653f1d1ae90c1 Gerrit-Change-Number: 18457 Gerrit-PatchSet: 38 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.corp-partner.google.com> Gerrit-Reviewer: Usha P <usha.p(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Varun Joshi <varun.joshi(a)intel.com> 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: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 03 Feb 2020 12:29:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2020