[coreboot-gerrit] Change in ...coreboot[master]: src/device/pci: Add support for discrete VGA initialization and OpROM...

[mikeb mikeb (Code Review)]

mikeb mikeb 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 1:
> Patch Set 1:
> > > The Kconfig option seems useless.
> >
> > Which option, and why useless?
>
> I assume CONFIG_MULTIPLE_VGA_ADAPTERS? and have to say, I never
> understood it either. It doesn't seem to do anything AMD specific
> so why do other platforms don't need it? IMHO, we should get rid
> of that option, first, or document what it does if it does some-
> thing useful.

There is only small code at pci_rom.c (line ~150) which is common for all the boards:

/*
     * We check to see if the device thinks it is a VGA device not
     * whether the ROM image is for a VGA device because some
     * devices have a mismatch between the hardware and the ROM.
     */
    if ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
#if !IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)
    	extern struct device *vga_pri; /* Primary VGA device (device.c). */
    	if (dev != vga_pri) return NULL; /* Only one VGA supported. */
#endif

All the other code related to this option - is AMD only:

cd ./coreboot/
find . -type f -print0 | xargs -0 grep -l -n "MULTIPLE_VGA_ADAPTERS"
./src/northbridge/amd/amdfam10/northbridge.c
./src/northbridge/amd/agesa/family15tn/northbridge.c
./src/northbridge/amd/agesa/family16kb/northbridge.c
./src/northbridge/amd/pi/00660F01/northbridge.c
./src/northbridge/amd/pi/00730F01/northbridge.c
./src/northbridge/amd/pi/00630F01/northbridge.c
./src/device/pci_rom.c
./src/device/Kconfig

This code inside northbridge/amd files seems to be important, e.g. ./src/northbridge/amd/agesa/family15tn/northbridge.c (line ~370)

#if IS_ENABLED(CONFIG_MULTIPLE_VGA_ADAPTERS)
    		extern struct device *vga_pri; // the primary vga device, defined in device.c
    		printk(BIOS_DEBUG, "VGA: vga_pri bus num = %d bus range [%d,%d]\n", vga_pri->bus->secondary,
    				link->secondary,link->subordinate);
    		/* We need to make sure the vga_pri is under the link */
    		if ((vga_pri->bus->secondary >= link->secondary) &&
    		    (vga_pri->bus->secondary <= link->subordinate))
#endif

But enabling such pieces of code for all the AMD boards (by removing CONFIG_MULTIPLE_VGA_ADAPTERS option) maybe could cause problems for some of them