Change in coreboot[master]: Add Option to force running the OptionROM

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Johanna Schander (Code Review)

25 Jul 2019 25 Jul '19

6:27 p.m.

Johanna Schander has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34571 )

Change subject: Add Option to force running the OptionROM ......................................................................

Add Option to force running the OptionROM

Some intel optionROMs that are shipped by vendors do not match the deviceIDs for their hardware. We can force them regardless and get working vga init for some of them.

Tested against the 8086,0406 image for Intel sklkbl 8086,5916 (HD620)

Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M src/device/Kconfig M src/device/pci_rom.c 2 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/34571/1

diff --git a/src/device/Kconfig b/src/device/Kconfig index e605bc2..bf793f6 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -95,6 +95,16 @@ more complete BIOS interrupt services available than coreboot, which some option ROMs require in order to function correctly.

+config IGNORE_VGA_ROM_ID_MISMATCH + bool "Ignore OptionROM ID matching" + depends on VGA_ROM_RUN + help + Execute VGA Option ROMs regardless of ID matching. + Some VGA OptionROMS that are shipped by vendors do not match + their devices ids. + Forcing them to run might result in success but should be done + quite carefully. + config RUN_FSP_GOP bool "Run a GOP driver" depends on HAVE_FSP_GOP diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 2b2d46d..8674b08 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -105,7 +105,10 @@ && (vendev == mapped_vendev)) { printk(BIOS_ERR, "ID mismatch: vendor ID %04x, " "device ID %04x\n", rom_data->vendor, rom_data->device); - return NULL; + if CONFIG(IGNORE_VGA_ROM_ID_MISMATCH) + printk(BIOS_INFO, "Ignoring"); + else + return NULL; }

printk(BIOS_SPEW, "PCI ROM image, Class Code %04x%02x, "

Attachments:

attachment.htm (text/html — 4.2 KB)

Show replies by date

Paul Menzel (Code Review)

25 Jul 25 Jul

7:34 p.m.

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

Change subject: Add Option to force running the OptionROM ......................................................................

Patch Set 1:

(7 comments)

Nice. But maybe this should be a run-time option.

https://review.coreboot.org/c/coreboot/+/34571/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/34571/1//COMMIT_MSG@7 PS1, Line 7: Add Option to force running the OptionROM device: Add option to force running Option ROMs

https://review.coreboot.org/c/coreboot/+/34571/1//COMMIT_MSG@9 PS1, Line 9: intel optionROMs Intel Option ROMs

https://review.coreboot.org/c/coreboot/+/34571/1//COMMIT_MSG@10 PS1, Line 10: deviceIDs device IDs

https://review.coreboot.org/c/coreboot/+/34571/1/src/device/Kconfig File src/device/Kconfig:

https://review.coreboot.org/c/coreboot/+/34571/1/src/device/Kconfig@99 PS1, Line 99: OptionROM Option ROM

https://review.coreboot.org/c/coreboot/+/34571/1/src/device/Kconfig@103 PS1, Line 103: OptionROMS Option ROMs

https://review.coreboot.org/c/coreboot/+/34571/1/src/device/Kconfig@103 PS1, Line 103: Some VGA OptionROMS that are shipped by vendors do not match Please add a blank line above.

https://review.coreboot.org/c/coreboot/+/34571/1/src/device/pci_rom.c File src/device/pci_rom.c:

https://review.coreboot.org/c/coreboot/+/34571/1/src/device/pci_rom.c@109 PS1, Line 109: printk(BIOS_INFO, "Ignoring"); It should be refactored, so that it prints on the same line as the first message (no line break), or be a longer message.

...

Ignoring mismatch, and executing anyway.

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 1 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 25 Jul 2019 17:34:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Johanna Schander (Code Review)

7:38 p.m.

Johanna Schander has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34571 )

Change subject: Add Option to force running the OptionROM ......................................................................

Patch Set 1:

...

Patch Set 1:

(7 comments)

Nice. But maybe this should be a run-time option.

Could you explain to me how?

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 1 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 25 Jul 2019 17:38:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Johanna Schander (Code Review)

8:33 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: Add Option to force running the OptionROM ......................................................................

Add Option to force running the OptionROM

Some intel optionROMs that are shipped by vendors do not match the deviceIDs for their hardware. We can force them regardless and get working vga init for some of them.

Tested against the 8086,0406 image for Intel sklkbl 8086,5916 (HD620)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/34571/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 2 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Johanna Schander (Code Review)

8:35 p.m.

New subject: Change in coreboot[master]: device/pci_rom: Add Option to force running the OptionROM

Hello build bot (Jenkins),

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

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

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

Change subject: device/pci_rom: Add Option to force running the OptionROM ......................................................................

device/pci_rom: Add Option to force running the OptionROM

Some intel optionROMs that are shipped by vendors do not match the deviceIDs for their hardware. We can force them regardless and get working vga init for some of them.

Tested against the 8086,0406 image for Intel sklkbl 8086,5916 (HD620)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/34571/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 3 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Nico Huber (Code Review)

10:41 p.m.

New subject: Change in coreboot[master]: device/pci_rom: Add Option to force running the OptionROM

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

Change subject: device/pci_rom: Add Option to force running the OptionROM ......................................................................

Patch Set 3: Code-Review-1

(1 comment)

I don't see why we would need yet another Kconfig option for this. Please make use of/enhance the existing mechanisms instead. Let me know if you need further pointers for map_oprom_vendev().

https://review.coreboot.org/c/coreboot/+/34571/3/src/device/pci_rom.c File src/device/pci_rom.c:

https://review.coreboot.org/c/coreboot/+/34571/3/src/device/pci_rom.c@105 PS3, Line 105: vendev == mapped_vendev coreboot already has a mechanism to handle an id mismatch: The driver for the integrated graphics device should map any compatible id to a default one (by implementing map_oprom_ vendev()). And the Kconfig default for VGA_BIOS_ID should reflect that id.

If such mapping happened `vendev == mapped_vendev` would be false and we'd already ignore the mismatch.

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 3 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 25 Jul 2019 20:41:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Johanna Schander (Code Review)

11:46 p.m.

New subject: Change in coreboot[master]: device/pci_rom: Add Option to force running the OptionROM

Johanna Schander has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34571 )

Change subject: device/pci_rom: Add Option to force running the OptionROM ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34571/3/src/device/pci_rom.c File src/device/pci_rom.c:

https://review.coreboot.org/c/coreboot/+/34571/3/src/device/pci_rom.c@105 PS3, Line 105: vendev == mapped_vendev

...

coreboot already has a mechanism to handle an id mismatch: […]

I was not able to get this to work, so i went with the stupid and simple approach.

therefore, i will abandon thins change. Thanks for the clarification <3

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 3 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 25 Jul 2019 21:46:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Johanna Schander (Code Review)

11:46 p.m.

New subject: Change in coreboot[master]: device/pci_rom: Add Option to force running the OptionROM

Johanna Schander has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34571 )

Change subject: device/pci_rom: Add Option to force running the OptionROM ......................................................................

Abandoned

Not needed

-- To view, visit https://review.coreboot.org/c/coreboot/+/34571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7a1e12d86b77b866077784d3837cf9359cc668 Gerrit-Change-Number: 34571 Gerrit-PatchSet: 3 Gerrit-Owner: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Johanna Schander coreboot@mimoja.de Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Patrick Rudolph siro@das-labor.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: abandon

1887

days inactive

1887

days old

coreboot-gerrit@coreboot.org

7 comments

3 participants

tags (0)

participants (3)

Johanna Schander (Code Review)
Nico Huber (Code Review)
Paul Menzel (Code Review)