Attention is currently required from: Tim Crawford, Nico Huber, Jeremy Soller, Paul Menzel, Angel Pons, Michael Niewöhner. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57034 )
Change subject: drivers/gfx/nvidia: Add driver for NVIDIA Optimus ......................................................................
Patch Set 2:
(5 comments)
File src/drivers/gfx/nvidia/Kconfig:
https://review.coreboot.org/c/coreboot/+/57034/comment/c73c426d_a1372d36 PS2, Line 9: default 0x01 Can't this be determined at runtime from the parent device in the devicetree (should be the PCIe RP right?)
File src/drivers/gfx/nvidia/acpi/gpu.asl:
https://review.coreboot.org/c/coreboot/+/57034/comment/8c8abe94_1bb39d0f PS2, Line 39: CONFIG_MMCONF_BASE_ADDRESS + 0x8000
CONFIG_MMCONF_BASE_ADDRESS + CONFIG_DRIVERS_GFX_NVIDIA_BRIDGE << 15 ? […]
you mean an SSDT callback? `.acpi_fill_ssdt` ? +1 to this.
File src/drivers/gfx/nvidia/gpu.h:
https://review.coreboot.org/c/coreboot/+/57034/comment/9e90c8f9_2fbaf7f8 PS2, Line 9: /* GPIO for GPU_PWR_EN */ : unsigned int power_gpio; : /* GPIO for GPU_RST# */ : unsigned int reset_gpio; I have been meaning to break out a separate power resource driver.... reminds me maybe to get doing that 😊
File src/drivers/gfx/nvidia/nvidia.c:
https://review.coreboot.org/c/coreboot/+/57034/comment/02a9c9a8_6fddabf6 PS2, Line 13: : // Find all BARs on GPU, mark them above 4g if prefetchable : for (int bar = PCI_BASE_ADDRESS_0; bar <= PCI_BASE_ADDRESS_5; bar += 4) { : struct resource *res = probe_resource(dev, bar); : : if (res) { : if (res->flags & IORESOURCE_PREFETCH) { : printk(BIOS_INFO, " BAR at 0x%02x marked above 4g\n", bar); : res->flags |= IORESOURCE_ABOVE_4G; : } else { : printk(BIOS_DEBUG, " BAR at 0x%02x not prefetch\n", bar); : } : } else { : printk(BIOS_DEBUG, " BAR at 0x%02x not found\n", bar); : } : } : } Does it make sense to select `PCIEXP_HOTPLUG_PREFETCH_MEM_ABOVE_4G` instead when using the NVIDIA driver?
File src/drivers/gfx/nvidia/romstage.c:
https://review.coreboot.org/c/coreboot/+/57034/comment/b49ff7fe_e9ee8b23 PS2, Line 11: void nvidia_set_power(const struct nvidia_gpu_config *config) : { : if (!config->power_gpio || !config->reset_gpio) { : printk(BIOS_ERR, "%s: GPU_PWR_EN and GPU_RST# must be set\n", __func__); : return; : } : : printk(BIOS_DEBUG, "%s: GPU_PWR_EN = %d\n", : __func__, config->power_gpio); : printk(BIOS_DEBUG, "%s: GPU_RST# = %d\n", : __func__, config->reset_gpio); : : gpio_set(config->reset_gpio, 0); : mdelay(4); : : if (config->enable) { : gpio_set(config->power_gpio, 1); : mdelay(4); : gpio_set(config->reset_gpio, 1); : } else { : gpio_set(config->power_gpio, 0); : } : : mdelay(4); : } IMO, this is up to the mainboard code to perform correctly