Change in ...coreboot[master]: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by ...

[mikeb mikeb (Code Review)]

Hello Mike Banon,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware

G505S does not have any SAS or NVMe controllers and could not have a TPM,
so it makes sense to disable the related SeaBIOS options for this laptop.

Signed-off-by: Mike Banon 
Change-Id: I5b2ee6403d7d2298725729d8d833e37627a4f202
---
M src/mainboard/lenovo/g505s/Kconfig
A src/mainboard/lenovo/g505s/config_seabios
2 files changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/31357/1

diff --git a/src/mainboard/lenovo/g505s/Kconfig b/src/mainboard/lenovo/g505s/Kconfig
index 883ef27..71ab909 100644
--- a/src/mainboard/lenovo/g505s/Kconfig
+++ b/src/mainboard/lenovo/g505s/Kconfig
@@ -55,4 +55,8 @@
    string
    default "1002,990b"

+config PAYLOAD_CONFIGFILE
+       string
+       default "$(top)/src/mainboard/$(MAINBOARDDIR)/config_seabios" if PAYLOAD_SEABIOS
+
 endif # BOARD_LENOVO_G505S
diff --git a/src/mainboard/lenovo/g505s/config_seabios b/src/mainboard/lenovo/g505s/config_seabios
new file mode 100644
index 0000000..8d3957b
--- /dev/null
+++ b/src/mainboard/lenovo/g505s/config_seabios
@@ -0,0 +1,6 @@
+#
+# SeaBIOS custom configuration for Lenovo G505S
+#
+# CONFIG_MEGASAS is not set
+# CONFIG_NVME is not set
+# CONFIG_TCGBIOS is not set

[mikeb mikeb (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Mike Banon, build bot (Jenkins),

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

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

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

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware

G505S does not have any SAS or NVMe controllers and could not have a TPM,
so it makes sense to disable the related SeaBIOS options for this laptop.

Signed-off-by: Mike Banon 
Change-Id: I5b2ee6403d7d2298725729d8d833e37627a4f202
---
M src/mainboard/lenovo/g505s/Kconfig
A src/mainboard/lenovo/g505s/config_seabios
2 files changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/31357/2

[Paul Menzel (Code Review)]

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

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31357/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31357/2//COMMIT_MSG@11 
PS2, Line 11: 
For size reasons? What is the difference?

[mikeb mikeb (Code Review)]

mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31357 )

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31357/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31357/2//COMMIT_MSG@11 
PS2, Line 11:
> For size reasons? What is the difference?

bios.bin.elf with these three features: 128960 bytes, without them: 109600 bytes.
128960 - 109600 = 19360 bytes, about 19KB difference, seems pretty good.

Another nice benefit is a cleaner build: G505S could never have this hardware,
so these options should be disabled just like the Generic Drivers options
for SIL3114 and TPS65913 chips are disabled by default for this board

[Paul Menzel (Code Review)]

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

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 3: Code-Review+1

Normally, payloads are compressed, so the 19 KiB are even reduced more. The downside of diverting from defaults is, that you cannot just share the payload with other people/systems. And it is always a question, if the added complexity is worth it.

Anyway, could you please add the size differences to the commit message?

[mikeb mikeb (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Paul Menzel, Mike Banon, build bot (Jenkins),

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

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

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

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware

G505S does not have any SAS or NVMe controllers and could not have a TPM,
so it makes sense to disable the related SeaBIOS options for this laptop.
This reduces the size of compiled SeaBIOS by 128960-109600 = 19360 bytes.

Signed-off-by: Mike Banon 
Change-Id: I5b2ee6403d7d2298725729d8d833e37627a4f202
---
M src/mainboard/lenovo/g505s/Kconfig
A src/mainboard/lenovo/g505s/config_seabios
2 files changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/31357/4

[mikeb mikeb (Code Review)]

mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31357 )

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 4:
> Patch Set 3: Code-Review+1
>
> Anyway, could you please add the size differences to the commit message?

Done.

[mikeb mikeb (Code Review)]

mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31357 )

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 6:

Sadly there is no progress in the review of G505S patches, despite their great significance: discrete GPU support. Maybe that's expected - there are 1350+ "open" patches, some of which are quite valuable, but not enough reviewers to review them all, that's why so slow. I don't give up yet, but I will create the convenient download scripts for my G505S patches - like this CB:28425 ( AMD microcodes: scripts for applying the unofficial (not-merged-yet) updates ). A bit too early to fork the coreboot project to a separate one "for the AMD guys", so we will just keep applying our "unofficial patches" on top of coreboot.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31357 )

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 6:
> Patch Set 6:
>
> Sadly there is no progress in the review of G505S patches, despite their great significance:

You do reviews; you get reviews. And don't get offended when your particular solution is not merged as-is.

I thought there was a conclusion AMD / AtomBIOS only has to be loaded (from OS driver perspective), and the memory location of said blob filled in ACPI. If your proposal always forced running them, that's probably the reason for no-further-review and no merge, if that's what happened.

Did I miss work that only loads the discrete AtomBIOS tables?

[mikeb mikeb (Code Review)]

mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31357 )

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Patch Set 6:
> Patch Set 6:
>
> I thought there was a conclusion AMD / AtomBIOS only has to be loaded (from OS driver perspective), and the memory location of said blob filled in ACPI. If your proposal always forced running them, that's probably the reason for no-further-review and no merge, if that's what happened.

Please take a fresh look at CB:31448 . I don't force running the dGPU blobs since revision 5 (February 22), but no-one replied since revision 2 and that was a bit discouraging to be honest...

[Mike Banon (Code Review)]

Hello Kyösti Mälkki, Alexander Couzens, Patrick Rudolph, Paul Menzel, Mike Banon, build bot (Jenkins),

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

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

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

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware

G505S does not have any SAS or NVMe controllers and could not have a TPM,
so it makes sense to disable the related SeaBIOS options for this laptop.
This reduces the size of compiled SeaBIOS by 129344-110048 = 19296 bytes.

Signed-off-by: Mike Banon 
Change-Id: I5b2ee6403d7d2298725729d8d833e37627a4f202
---
M src/mainboard/lenovo/g505s/Kconfig
A src/mainboard/lenovo/g505s/config_seabios
2 files changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/31357/9

[Mike Banon (Code Review)]

Hello Kyösti Mälkki, Alexander Couzens, Patrick Rudolph, Paul Menzel, Mike Banon, build bot (Jenkins),

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

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

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

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware

G505S does not have any SAS or NVMe controllers and could not have a TPM,
so it makes sense to disable the related SeaBIOS options for this laptop.
This reduces the size of compiled SeaBIOS by 129344-110048 = 19296 bytes.

Signed-off-by: Mike Banon 
Change-Id: I5b2ee6403d7d2298725729d8d833e37627a4f202
---
M src/mainboard/lenovo/g505s/Kconfig
A src/mainboard/lenovo/g505s/config_seabios
2 files changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/31357/10

[Mike Banon (Code Review)]

Mike Banon has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31357 )

Change subject: src/mainboard/lenovo/g505s: Disable SeaBIOS options not supported by hardware
......................................................................

Abandoned

Too many revisions, need a fresh start.