Change in coreboot[master]: amd/mandolin: unbreak SeaBIOS VBIOS support

[Felix Held (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

amd/mandolin: unbreak SeaBIOS VBIOS support

This is a workaround and not a clean solution, but avoids breakage.

Change-Id: I4d9042615965b6a2d9255c194cf23368264ffe54
Signed-off-by: Felix Held 
---
M src/mainboard/amd/mandolin/Kconfig
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42433/1

diff --git a/src/mainboard/amd/mandolin/Kconfig b/src/mainboard/amd/mandolin/Kconfig
index c93bc34..bd01dec 100644
--- a/src/mainboard/amd/mandolin/Kconfig
+++ b/src/mainboard/amd/mandolin/Kconfig
@@ -104,4 +104,16 @@
      Picasso's LPC bus signals are MUXed with some of the EMMC signals.
      Select this option if LPC signals are required.

+#TODO: remove this hack to not break graphics in combination with SeaBIOS
+config VGA_BIOS_DGPU_ID
+	string
+	default "1002,15d8"
+	help
+	  The default VGA BIOS PCI vendor/device ID should be set to the
+	  result of the map_oprom_vendev() function in northbridge.c.
+
+config VGA_BIOS_DGPU_FILE
+	string
+	default "3rdparty/amd_blobs/picasso/PicassoGenericVbios.bin"
+
 endif # BOARD_AMD_MANDOLIN

[Felix Held (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 1:

i split this into a different commit than the main mandolin one to make the revert easier when the underlying issue is fixed

[Felix Held (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

amd/mandolin: unbreak SeaBIOS VBIOS support

This is a workaround and not a clean solution, but avoids breakage.
It's separated from the rest of the Mandolin support, so it can just be
reverted after a proper fix is implemented.

Change-Id: I4d9042615965b6a2d9255c194cf23368264ffe54
Signed-off-by: Felix Held 
---
M src/mainboard/amd/mandolin/Kconfig
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42433/3

[Paul Menzel (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42433/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42433/4//COMMIT_MSG@12 
PS4, Line 12: 
Could you please add the problem description?

Is there a bug report?

[Felix Held (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

amd/mandolin: unbreak SeaBIOS VBIOS support

Commit 86ba0d73f34185533e5e2d4258aa3bf3dba40ed4 added VBIOS support for
Raven2 silicon and change the VBIOS file names to the format including
the PCI device revision number. SeaBIOS expects the file to have only
the PCI vendor and device IDs in the CBFS file name, so it doesn't find
the VBIOS any more after that patch got applied. This patch adds the
path and CBFS file name to include the Picasso VBIOS a second time under
the CBFS file name SeaBIOS expects.
This is a workaround and not a clean solution, but avoids breakage.
It's separated from the rest of the Mandolin support, so it can just be
reverted after a proper fix is implemented.

BUG=b:153675508

Change-Id: I4d9042615965b6a2d9255c194cf23368264ffe54
Signed-off-by: Felix Held 
---
M src/mainboard/amd/mandolin/Kconfig
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42433/5

[Felix Held (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42433/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42433/4//COMMIT_MSG@12 
PS4, Line 12:
> Could you please add the problem description? […]

Done

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42433 )

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 5: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 6: Code-Review+1

(4 comments)

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@10 
PS6, Line 10: change
changed

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@11 
PS6, Line 11: SeaBIOS
Upstream SeaBIOS, right?

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@16 
PS6, Line 16: This is a workaround and not a clean solution, but avoids breakage.
Please add a blank line above to separate the paragraphs.

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@18 
PS6, Line 18: reverted after a proper fix is implemented.
Don’t you have commits in Chromium OS’ SeaBIOS repository to work with this? Maybe mention that?

[Felix Held (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@18 
PS6, Line 18: reverted after a proper fix is implemented.
> Don’t you have commits in Chromium OS’ SeaBIOS repository to work with this? Maybe mention that?

there is a commit adding cbfs file names with revision number in cros downstream https://chromium-review.googlesource.com/c/chromiumos/third_party/seabios/+/... but even in combination with a links file it might not fully solve the issue for every possible case

[Felix Held (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Paul Menzel,

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

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

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

amd/mandolin: unbreak SeaBIOS VBIOS support

Commit 86ba0d73f34185533e5e2d4258aa3bf3dba40ed4 added VBIOS support for
Raven2 silicon and changed the VBIOS file names to the format including
the PCI device revision number. Upstream SeaBIOS expects the file to
have only the PCI vendor and device IDs in the CBFS file name, so it
doesn't find the VBIOS any more after that patch got applied. This patch
adds the path and CBFS file name to include the Picasso VBIOS a second
time under the CBFS file name SeaBIOS expects.

This is a workaround and not a clean solution, but avoids breakage.
It's separated from the rest of the Mandolin support, so it can just be
reverted after a proper fix is implemented.

https://chromium-review.googlesource.com/2015963/ in combination with a
links file in CBFS might solve the issue for most of the cases, but it's
not sure yet if for all, so a proper fix might require more than that.

BUG=b:153675508

Change-Id: I4d9042615965b6a2d9255c194cf23368264ffe54
Signed-off-by: Felix Held 
---
M src/mainboard/amd/mandolin/Kconfig
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42433/7

[Felix Held (Code Review)]

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

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

Patch Set 7:

(4 comments)

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@10 
PS6, Line 10: change
> changed

Done

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@11 
PS6, Line 11: SeaBIOS
> Upstream SeaBIOS, right?

Done

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@16 
PS6, Line 16: This is a workaround and not a clean solution, but avoids breakage.
> Please add a blank line above to separate the paragraphs.

Done

https://review.coreboot.org/c/coreboot/+/42433/6//COMMIT_MSG@18 
PS6, Line 18: reverted after a proper fix is implemented.
> there is a commit adding cbfs file names with revision number in cros downstream https://chromium-re […]

Done

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42433 )

Change subject: amd/mandolin: unbreak SeaBIOS VBIOS support
......................................................................

amd/mandolin: unbreak SeaBIOS VBIOS support

Commit 86ba0d73f34185533e5e2d4258aa3bf3dba40ed4 added VBIOS support for
Raven2 silicon and changed the VBIOS file names to the format including
the PCI device revision number. Upstream SeaBIOS expects the file to
have only the PCI vendor and device IDs in the CBFS file name, so it
doesn't find the VBIOS any more after that patch got applied. This patch
adds the path and CBFS file name to include the Picasso VBIOS a second
time under the CBFS file name SeaBIOS expects.

This is a workaround and not a clean solution, but avoids breakage.
It's separated from the rest of the Mandolin support, so it can just be
reverted after a proper fix is implemented.

https://chromium-review.googlesource.com/2015963/ in combination with a
links file in CBFS might solve the issue for most of the cases, but it's
not sure yet if for all, so a proper fix might require more than that.

BUG=b:153675508

Change-Id: I4d9042615965b6a2d9255c194cf23368264ffe54
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42433
Reviewed-by: Paul Menzel 
Reviewed-by: Raul Rangel 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/amd/mandolin/Kconfig
1 file changed, 12 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Raul Rangel: Looks good to me, approved

diff --git a/src/mainboard/amd/mandolin/Kconfig b/src/mainboard/amd/mandolin/Kconfig
index 9760e45..6ec77bf 100644
--- a/src/mainboard/amd/mandolin/Kconfig
+++ b/src/mainboard/amd/mandolin/Kconfig
@@ -91,4 +91,16 @@
      Picasso's LPC bus signals are MUXed with some of the EMMC signals.
      Select this option if LPC signals are required.

+#TODO: remove this hack to not break graphics in combination with SeaBIOS
+config VGA_BIOS_DGPU_ID
+	string
+	default "1002,15d8"
+	help
+	  The default VGA BIOS PCI vendor/device ID should be set to the
+	  result of the map_oprom_vendev() function in northbridge.c.
+
+config VGA_BIOS_DGPU_FILE
+	string
+	default "3rdparty/amd_blobs/picasso/PicassoGenericVbios.bin"
+
 endif # BOARD_AMD_MANDOLIN