Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 10:
(11 comments)
https://review.coreboot.org/#/c/31448/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31448/10//COMMIT_MSG@10 PS10, Line 10: CONFIG_MULTIPLE_VGA_ADAPTERS You want to handle non VGA compatible PCI devices in a similar way as VGA compatible devices. This does not have much to do with integrated or discrete.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@795 PS10, Line 795: MULTIPLE_VGA_ADAPTERS This name is confusing. All it does is allowing coreboot to initialize PCI_CLASS_DISPLAY_OTHER in a similar fashion to PCI_CLASS_DISLAY?
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@830 PS10, Line 830: Integrated use a ternary operator in the printk statement. It would also be better to split off adding these debug messages into a different patch. PCI_CLASS_DISPLAY_VGA means VGA compatible, not integrated or discrete.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@839 PS10, Line 839: Integrated use a ternary operator in the printk statement.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@190 PS1, Line 190: run_rom = (struct rom_header *)(uintptr_t)PCI_VGA_RAM_IMAGE_START; : } else if (IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : if ((dev->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { : run_rom = (struct rom_header *)(uintptr_t)PCI_RAM_IMAGE_START; : } : } use a switch statement for the PCI class.
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@196 PS1, Line 196: : if (run_rom == NULL) : return NULL; isn't this captured by the next statement?
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@231 PS1, Line 231: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_VGA_RAM_IMAGE_START ? : "initialized " : "", : rom); : } else { : /* Copy Discrete VGA VBIOS from CBFS to ACPI VFCT. */ : printk(BIOS_DEBUG, " Copying %sVBIOS image from %p\n", : rom == (struct rom_header *) : (uintptr_t)PCI_RAM_IMAGE_START ? : "initialized " : "", : rom); : /* TODO: do this also for Integrated VGA VBIOS. */ : } isn't this the same?
https://review.coreboot.org/#/c/31448/1/src/device/pci_rom.c@269 PS1, Line 269: if (!IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)) { : if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) { : return current; : } : } else { : /* Write ACPI VFCT only for Discrete VGA. */ : if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) { : ; : } else { : return current; : } : /* TODO: do this also for Integrated VGA. */ : } use a switch statement.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) { this ought to depend on the device itself, not on Kconfig...
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@231 PS10, Line 231: /* Copy Integrated VGA VBIOS from CBFS to ACPI VFCT. */ There is just a printk statement below, no copying is yet happening.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@272 PS10, Line 272: /* Write ACPI VFCT only for Discrete VGA. */ : if ((device->class >> 8) == PCI_CLASS_DISPLAY_OTHER) : ; : else : return current; invert the statement.