Hello Justin Frodsham, Raul Rangel, Matt Papageorge,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39793
to review the following change.
Change subject: Rework map_oprom_vendev to add revision check and mapping ......................................................................
Rework map_oprom_vendev to add revision check and mapping
AMD's Family 17h SoCs share the same video device ID, but may need different video BIOSes. This adds the common code changes to check the vendor & device IDs along with the revision and select the correct video BIOS to use.
Change-Id: I2978a5693c904ddb09d23715cb309c4a356e0370 Signed-off-by: Martin Roth martinroth@chromium.org Reviewed-on: https://chromium-review.googlesource.com/2040455 Reviewed-by: Raul E Rangel rrangel@chromium.org Reviewed-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-by: Justin Frodsham justin.frodsham@amd.corp-partner.google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/device/pci_rom.c M src/include/device/pci_rom.h 2 files changed, 17 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39793/1
diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 6af20e8..7d48961 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -23,29 +23,34 @@ #include <arch/acpigen.h>
/* Rmodules don't like weak symbols. */ +void __weak map_oprom_vendev_rev(u32 *vendev, u8 *rev) { return; } u32 __weak map_oprom_vendev(u32 vendev) { return vendev; }
struct rom_header *pci_rom_probe(struct device *dev) { struct rom_header *rom_header = NULL; struct pci_data *rom_data; + u8 rev = pci_read_config8(dev, PCI_REVISION_ID); + u8 mapped_rev = rev; + u32 vendev = (dev->vendor << 16) | dev->device; + u32 mapped_vendev = vendev;
- /* If it's in FLASH, then don't check device for ROM. */ + /* If the ROM is in flash, then don't check the PCI device for it. */ 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); + map_oprom_vendev_rev(&mapped_vendev, &mapped_rev); + } else { + rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device); + mapped_vendev = map_oprom_vendev(vendev); }
- if (!rom_header) - rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device); - - u32 vendev = (dev->vendor << 16) | dev->device; - u32 mapped_vendev; - - mapped_vendev = map_oprom_vendev(vendev); - if (!rom_header) { - if (vendev != mapped_vendev) { + if (CONFIG(CHECK_REV_IN_OPROM_NAME) && + (vendev != mapped_vendev || rev != mapped_rev)) { + rom_header = cbfs_boot_map_optionrom_revision( + mapped_vendev >> 16, + mapped_vendev & 0xffff, mapped_rev); + } else if (vendev != mapped_vendev) { rom_header = cbfs_boot_map_optionrom( mapped_vendev >> 16, mapped_vendev & 0xffff); diff --git a/src/include/device/pci_rom.h b/src/include/device/pci_rom.h index 82f3c40..47db52c 100644 --- a/src/include/device/pci_rom.h +++ b/src/include/device/pci_rom.h @@ -45,6 +45,7 @@
void pci_rom_ssdt(struct device *device);
+void map_oprom_vendev_rev(u32 *vendev, u8 *rev); u32 map_oprom_vendev(u32 vendev);
int verified_boot_should_run_oprom(struct rom_header *rom_header);
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39793 )
Change subject: Rework map_oprom_vendev to add revision check and mapping ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39793 )
Change subject: Rework map_oprom_vendev to add revision check and mapping ......................................................................
Rework map_oprom_vendev to add revision check and mapping
AMD's Family 17h SoCs share the same video device ID, but may need different video BIOSes. This adds the common code changes to check the vendor & device IDs along with the revision and select the correct video BIOS to use.
Change-Id: I2978a5693c904ddb09d23715cb309c4a356e0370 Signed-off-by: Martin Roth martinroth@chromium.org Reviewed-on: https://chromium-review.googlesource.com/2040455 Reviewed-by: Raul E Rangel rrangel@chromium.org Reviewed-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-by: Justin Frodsham justin.frodsham@amd.corp-partner.google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/39793 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/device/pci_rom.c M src/include/device/pci_rom.h 2 files changed, 17 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 6af20e8..7d48961 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -23,29 +23,34 @@ #include <arch/acpigen.h>
/* Rmodules don't like weak symbols. */ +void __weak map_oprom_vendev_rev(u32 *vendev, u8 *rev) { return; } u32 __weak map_oprom_vendev(u32 vendev) { return vendev; }
struct rom_header *pci_rom_probe(struct device *dev) { struct rom_header *rom_header = NULL; struct pci_data *rom_data; + u8 rev = pci_read_config8(dev, PCI_REVISION_ID); + u8 mapped_rev = rev; + u32 vendev = (dev->vendor << 16) | dev->device; + u32 mapped_vendev = vendev;
- /* If it's in FLASH, then don't check device for ROM. */ + /* If the ROM is in flash, then don't check the PCI device for it. */ 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); + map_oprom_vendev_rev(&mapped_vendev, &mapped_rev); + } else { + rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device); + mapped_vendev = map_oprom_vendev(vendev); }
- if (!rom_header) - rom_header = cbfs_boot_map_optionrom(dev->vendor, dev->device); - - u32 vendev = (dev->vendor << 16) | dev->device; - u32 mapped_vendev; - - mapped_vendev = map_oprom_vendev(vendev); - if (!rom_header) { - if (vendev != mapped_vendev) { + if (CONFIG(CHECK_REV_IN_OPROM_NAME) && + (vendev != mapped_vendev || rev != mapped_rev)) { + rom_header = cbfs_boot_map_optionrom_revision( + mapped_vendev >> 16, + mapped_vendev & 0xffff, mapped_rev); + } else if (vendev != mapped_vendev) { rom_header = cbfs_boot_map_optionrom( mapped_vendev >> 16, mapped_vendev & 0xffff); diff --git a/src/include/device/pci_rom.h b/src/include/device/pci_rom.h index 82f3c40..47db52c 100644 --- a/src/include/device/pci_rom.h +++ b/src/include/device/pci_rom.h @@ -45,6 +45,7 @@
void pci_rom_ssdt(struct device *device);
+void map_oprom_vendev_rev(u32 *vendev, u8 *rev); u32 map_oprom_vendev(u32 vendev);
int verified_boot_should_run_oprom(struct rom_header *rom_header);
-
Felix Held (Code Review) -
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)