Change in coreboot[master]: mb/up/squared: Make optional settings configurable

[Felix Singer (Code Review)]

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: mb/up/squared: Make optional settings configurable
......................................................................

mb/up/squared: Make optional settings configurable

Change-Id: I6e1deb2d3ef676829034d0b67aa8b72a5f2fd766
Signed-off-by: Felix Singer 
---
M src/mainboard/up/squared/Kconfig
1 file changed, 16 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/34642/1

diff --git a/src/mainboard/up/squared/Kconfig b/src/mainboard/up/squared/Kconfig
index b9f5b27..7019b30 100644
--- a/src/mainboard/up/squared/Kconfig
+++ b/src/mainboard/up/squared/Kconfig
@@ -2,18 +2,26 @@

config BOARD_SPECIFIC_OPTIONS
    def_bool y
-	select USE_BLOBS
-	select ADD_FSP_BINARIES
-	select FSP_USE_REPO
    select HAVE_ACPI_TABLES
    select HAVE_ACPI_RESUME
    select INTEL_GMA_HAVE_VBT
    select INTEL_LPSS_UART_FOR_CONSOLE
    select SOC_INTEL_APOLLOLAKE
    select BOARD_ROMSIZE_KB_16384
-	select ONBOARD_VGA_IS_PRIMARY
    select MAINBOARD_HAS_LIBGFXINIT

+config USE_BLOBS
+	bool
+	default y
+
+config ADD_FSP_BINARIES
+	bool
+	default y
+
+config FSP_USE_REPO
+	bool
+	default y
+
 config VBOOT
    select VBOOT_NO_BOARD_SUPPORT
    select GBB_FLAG_DISABLE_LID_SHUTDOWN
@@ -65,6 +73,10 @@
    int
    default 2

+config ONBOARD_VGA_IS_PRIMARY
+	bool
+	default y
+
 config LINUX_COMMAND_LINE
    string
    default "console=ttyS4,115200 earlyprintk=ttyS4,115200,keep" if PAYLOAD_LINUXBOOT && UART_FOR_CONSOLE=0

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: mb/up/squared: Make optional settings configurable
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
File src/mainboard/up/squared/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 15: 	default y
AIUI, this is intentionally not the default, so binary repositories are not
pulled unless explicitly requested by the user.

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 17: config ADD_FSP_BINARIES
            : 	bool
            : 	default y
            : 
            : config FSP_USE_REPO
            : 	bool
            : 	default y
Can't these be platform defaults? If it works for one APL board, why wouldn't
it for all APL boards?

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: mb/up/squared: Make optional settings configurable
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
File src/mainboard/up/squared/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 15: 	default y
> AIUI, this is intentionally not the default, so binary repositories are not […]

If we enable ADD_FSP_BINARIES AND FSP_USE_REPO by default, we need this option to get the build properly done. So I would leave this as before.

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 17: config ADD_FSP_BINARIES
            : 	bool
            : 	default y
            : 
            : config FSP_USE_REPO
            : 	bool
            : 	default y
> Can't these be platform defaults? If it works for one APL board, why wouldn't […]

You are right, but see my comment before.

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Felix Singer (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34642

to look at the new patch set (#2).

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

soc/intel/apl: Enable FSP usage by default

This patch enables the usage of the FSP and the FSP repository by
default, since it is needed for booting. If someone would like to,
these options can still be disabled.

Change-Id: I6e1deb2d3ef676829034d0b67aa8b72a5f2fd766
Signed-off-by: Felix Singer 
---
M src/mainboard/up/squared/Kconfig
M src/soc/intel/apollolake/Kconfig
2 files changed, 12 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/34642/2

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34642/2/src/mainboard/up/squared/Kc... 
File src/mainboard/up/squared/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/2/src/mainboard/up/squared/Kc... 
PS2, Line 67: 	default y
Why was this added?

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Felix Singer (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34642

to look at the new patch set (#3).

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

soc/intel/apl: Enable FSP usage by default

This patch enables the usage of the FSP and the FSP repository by
default, since it is needed for booting. If someone would like to,
these options can still be disabled.

Change-Id: I6e1deb2d3ef676829034d0b67aa8b72a5f2fd766
Signed-off-by: Felix Singer 
---
M src/mainboard/up/squared/Kconfig
M src/soc/intel/apollolake/Kconfig
2 files changed, 8 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/34642/3

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34642/2/src/mainboard/up/squared/Kc... 
File src/mainboard/up/squared/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/2/src/mainboard/up/squared/Kc... 
PS2, Line 67: 	default y
> Why was this added?

Sorry, I forgot to remove this.

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
File src/mainboard/up/squared/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 15: 	default y
> If we enable ADD_FSP_BINARIES AND FSP_USE_REPO by default, we need this option to get the build prop […]

Done

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 17: config ADD_FSP_BINARIES
            : 	bool
            : 	default y
            : 
            : config FSP_USE_REPO
            : 	bool
            : 	default y
> You are right, but see my comment before.

Done

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

Patch Set 3: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/34642/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34642/3//COMMIT_MSG@7 
PS3, Line 7: FSP
FSP *repo* usage?

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
File src/mainboard/up/squared/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/1/src/mainboard/up/squared/Kc... 
PS1, Line 15: 	default y
> Done

Ack. Not part of this patch, but generally no board should select or change
the default of USE_BLOBS. Otherwise, an argument could be made that it should
always default to `y`. I wouldn't mind, but it defeats the purpose of that option.
We could rather add a NEED_BLOBS Kconfig, then, selected by any platform /
board that needs something from the repo.

Change in coreboot[master]: soc/intel/apl: Enable FSP usage by default

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34642 )

Change subject: soc/intel/apl: Enable FSP usage by default
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34642/3/src/soc/intel/apollolake/Kc... 
File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/34642/3/src/soc/intel/apollolake/Kc... 
PS3, Line 194: config ADD_FSP_BINARIES
             : 	bool
             : 	default y
             : 
             : config FSP_USE_REPO
             : 	bool
             : 	default y
Meh, missed something: GLK is not in the Github FSP, it seems, so this
should depend on !SOC_INTEL_GLK.

Also, the former could always `default y if FSP_USE_REPO` independent of
the platform, I guess.