Hello Martin Roth,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39792
to review the following change.
Change subject: src/device: Add option to look at revision in option roms ......................................................................
src/device: Add option to look at revision in option roms
AMD's Family 17h SOCs have the same vendor and device IDs for their graphics blocks, but need different video BIOSes. The only difference is the revision number.
Add a Kconfig option that allows us to add the revision number of the graphics device to the PCI option rom saved in CBFS.
Because searching CBFS takes a non-trivial amount of time, only enable the option if it's needed. If it's not used, or if nothing matches, the check will fall through and search for an option rom with no version.
BUG=b:145817712 TEST=With surrounding patches, loads dali vbios
Change-Id: Icb610a2abe7fcd0f4dc3716382b9853551240a7a Signed-off-by: Martin Roth martinroth@chromium.org Reviewed-on: https://chromium-review.googlesource.com/2013181 Reviewed-by: Martin Roth martinroth@google.com Tested-by: Martin Roth martinroth@google.com --- M src/device/Kconfig M src/device/pci_rom.c M src/include/cbfs.h M src/lib/cbfs.c 4 files changed, 41 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/39792/1
diff --git a/src/device/Kconfig b/src/device/Kconfig index 66130cc..de33b04 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -669,14 +669,14 @@ depends on VGA_BIOS default "1106,3230" help - The comma-separated PCI vendor and device ID that would associate - your VGA BIOS to your video card. + The comma-separated PCI vendor and device ID with optional revision if that + feature is enabled that would associate your vBIOS to your video card.
- Example: 1106,3230 + Example: 1106,3230 or 1106,3230,a3
In the above example 1106 is the PCI vendor ID (in hex, but without the "0x" prefix) and 3230 specifies the PCI device ID of the - video card (also in hex, without "0x" prefix). + video card (also in hex, without "0x" prefix). a3 specifies the revision.
Under GNU/Linux you can run `lspci -nn` to list the IDs of your PCI devices.
@@ -698,17 +698,23 @@ string "Graphics device PCI IDs" depends on VGA_BIOS_SECOND help - The comma-separated PCI vendor and device ID that would associate - your vBIOS to your video card. + The comma-separated PCI vendor and device ID with optional revision if that + feature is enabled that would associate your vBIOS to your video card.
- Example: 1106,3230 + Example: 1106,3230 or 1106,3230,a3
In the above example 1106 is the PCI vendor ID (in hex, but without the "0x" prefix) and 3230 specifies the PCI device ID of the - video card (also in hex, without "0x" prefix). + video card (also in hex, without "0x" prefix). a3 specifies the revision.
Under GNU/Linux you can run `lspci -nn` to list the IDs of your PCI devices.
+config CHECK_REV_IN_OPROM_NAME + def_bool n + help + Select this in the platform BIOS or chipset if the option rom has a revision + that needs to be checked when searching CBFS. + config VGA_BIOS_DGPU bool "Add a discrete VGA BIOS image" depends on VGA_BIOS diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 27f2d8d..6af20e8 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -27,11 +27,17 @@
struct rom_header *pci_rom_probe(struct device *dev) { - struct rom_header *rom_header; + struct rom_header *rom_header = NULL; struct pci_data *rom_data;
/* If it's in FLASH, then don't check device for ROM. */ - rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device); + if (CONFIG(CHECK_REV_IN_OPROM_NAME)) { + uint8_t rev = pci_read_config8(dev, PCI_REVISION_ID); + rom_header = cbfs_boot_map_optionrom_revision(dev->vendor, dev->device, rev); + } + + if (!rom_header) + rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device);
u32 vendev = (dev->vendor << 16) | dev->device; u32 mapped_vendev; diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 8233686..feb73a1 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -24,6 +24,8 @@
/* Return mapping of option ROM found in boot device. NULL on error. */ void *cbfs_boot_map_optionrom(uint16_t vendor, uint16_t device); +/* Return mapping of option ROM with revision number. Returns NULL on error. */ +void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev); /* Locate file by name and optional type. Return 0 on success. < 0 on error. */ int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type); /* Map file into memory leaking the mapping. Only should be used when diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index b8f3d5c..4f0b443 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -159,6 +159,12 @@ return (c <= 9) ? (c + '0') : (c - 10 + 'a'); }
+static void tohex8(unsigned int val, char *dest) +{ + dest[0] = tohex4((val >> 4) & 0xf); + dest[1] = tohex4(val & 0xf); +} + static void tohex16(unsigned int val, char *dest) { dest[0] = tohex4(val >> 12); @@ -177,6 +183,17 @@ return cbfs_boot_map_with_leak(name, CBFS_TYPE_OPTIONROM, NULL); }
+void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev) +{ + char name[20] = "pciXXXX,XXXX,XX.rom"; + + tohex16(vendor, name + 3); + tohex16(device, name + 8); + tohex8(rev, name + 13); + + return cbfs_boot_map_with_leak(name, CBFS_TYPE_OPTIONROM, NULL); +} + size_t cbfs_boot_load_file(const char *name, void *buf, size_t buf_size, uint32_t type) {
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39792 )
Change subject: src/device: Add option to look at revision in option roms ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39792 )
Change subject: src/device: Add option to look at revision in option roms ......................................................................
src/device: Add option to look at revision in option roms
AMD's Family 17h SOCs have the same vendor and device IDs for their graphics blocks, but need different video BIOSes. The only difference is the revision number.
Add a Kconfig option that allows us to add the revision number of the graphics device to the PCI option rom saved in CBFS.
Because searching CBFS takes a non-trivial amount of time, only enable the option if it's needed. If it's not used, or if nothing matches, the check will fall through and search for an option rom with no version.
BUG=b:145817712 TEST=With surrounding patches, loads dali vbios
Change-Id: Icb610a2abe7fcd0f4dc3716382b9853551240a7a Signed-off-by: Martin Roth martinroth@chromium.org Reviewed-on: https://chromium-review.googlesource.com/2013181 Reviewed-by: Martin Roth martinroth@google.com Tested-by: Martin Roth martinroth@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39792 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/device/Kconfig M src/device/pci_rom.c M src/include/cbfs.h M src/lib/cbfs.c 4 files changed, 41 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/device/Kconfig b/src/device/Kconfig index 66130cc..de33b04 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -669,14 +669,14 @@ depends on VGA_BIOS default "1106,3230" help - The comma-separated PCI vendor and device ID that would associate - your VGA BIOS to your video card. + The comma-separated PCI vendor and device ID with optional revision if that + feature is enabled that would associate your vBIOS to your video card.
- Example: 1106,3230 + Example: 1106,3230 or 1106,3230,a3
In the above example 1106 is the PCI vendor ID (in hex, but without the "0x" prefix) and 3230 specifies the PCI device ID of the - video card (also in hex, without "0x" prefix). + video card (also in hex, without "0x" prefix). a3 specifies the revision.
Under GNU/Linux you can run `lspci -nn` to list the IDs of your PCI devices.
@@ -698,17 +698,23 @@ string "Graphics device PCI IDs" depends on VGA_BIOS_SECOND help - The comma-separated PCI vendor and device ID that would associate - your vBIOS to your video card. + The comma-separated PCI vendor and device ID with optional revision if that + feature is enabled that would associate your vBIOS to your video card.
- Example: 1106,3230 + Example: 1106,3230 or 1106,3230,a3
In the above example 1106 is the PCI vendor ID (in hex, but without the "0x" prefix) and 3230 specifies the PCI device ID of the - video card (also in hex, without "0x" prefix). + video card (also in hex, without "0x" prefix). a3 specifies the revision.
Under GNU/Linux you can run `lspci -nn` to list the IDs of your PCI devices.
+config CHECK_REV_IN_OPROM_NAME + def_bool n + help + Select this in the platform BIOS or chipset if the option rom has a revision + that needs to be checked when searching CBFS. + config VGA_BIOS_DGPU bool "Add a discrete VGA BIOS image" depends on VGA_BIOS diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 27f2d8d..6af20e8 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -27,11 +27,17 @@
struct rom_header *pci_rom_probe(struct device *dev) { - struct rom_header *rom_header; + struct rom_header *rom_header = NULL; struct pci_data *rom_data;
/* If it's in FLASH, then don't check device for ROM. */ - rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device); + if (CONFIG(CHECK_REV_IN_OPROM_NAME)) { + uint8_t rev = pci_read_config8(dev, PCI_REVISION_ID); + rom_header = cbfs_boot_map_optionrom_revision(dev->vendor, dev->device, rev); + } + + if (!rom_header) + rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device);
u32 vendev = (dev->vendor << 16) | dev->device; u32 mapped_vendev; diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 8233686..feb73a1 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -24,6 +24,8 @@
/* Return mapping of option ROM found in boot device. NULL on error. */ void *cbfs_boot_map_optionrom(uint16_t vendor, uint16_t device); +/* Return mapping of option ROM with revision number. Returns NULL on error. */ +void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev); /* Locate file by name and optional type. Return 0 on success. < 0 on error. */ int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type); /* Map file into memory leaking the mapping. Only should be used when diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index b8f3d5c..4f0b443 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -159,6 +159,12 @@ return (c <= 9) ? (c + '0') : (c - 10 + 'a'); }
+static void tohex8(unsigned int val, char *dest) +{ + dest[0] = tohex4((val >> 4) & 0xf); + dest[1] = tohex4(val & 0xf); +} + static void tohex16(unsigned int val, char *dest) { dest[0] = tohex4(val >> 12); @@ -177,6 +183,17 @@ return cbfs_boot_map_with_leak(name, CBFS_TYPE_OPTIONROM, NULL); }
+void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev) +{ + char name[20] = "pciXXXX,XXXX,XX.rom"; + + tohex16(vendor, name + 3); + tohex16(device, name + 8); + tohex8(rev, name + 13); + + return cbfs_boot_map_with_leak(name, CBFS_TYPE_OPTIONROM, NULL); +} + size_t cbfs_boot_load_file(const char *name, void *buf, size_t buf_size, uint32_t type) {
-
Felix Held (Code Review) -
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)