Attention is currently required from: Tim Crawford, Jeremy Soller, Tim Wawrzynczak, Paul Menzel. Angel Pons 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: Code-Review+1
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57034/comment/fd379ae5_1675c5ac PS2, Line 10: GC6 What does GC6 mean?
Patchset:
PS2: I like the idea, thank you for making CB:43615 more generic
File src/drivers/gfx/nvidia/Kconfig:
PS2: Given that this driver is for NVIDIA Optimus, I'd place its code in a subfolder so that the path is `drivers/gfx/nvidia/optimus`. This would provide additional information about what the driver is meant to be used for.
File src/drivers/gfx/nvidia/acpi/gpu.asl:
https://review.coreboot.org/c/coreboot/+/57034/comment/76c791d8_0df5d3b5 PS2, Line 39: CONFIG_MMCONF_BASE_ADDRESS + 0x8000 CONFIG_MMCONF_BASE_ADDRESS + CONFIG_DRIVERS_GFX_NVIDIA_BRIDGE << 15 ?
Another option: use a SSDT. This would be more complicated, but would avoid having to hardcode ASL path names and offsets for PCI config space registers.
File src/drivers/gfx/nvidia/chip.h:
https://review.coreboot.org/c/coreboot/+/57034/comment/9c6b7cd7_4320d438 PS2, Line 7: /* TODO: Set GPIOs in devicetree? */ Yes
File src/drivers/gfx/nvidia/nvidia.c:
https://review.coreboot.org/c/coreboot/+/57034/comment/a2d911c1_32aa1398 PS2, Line 44: nit: just one blank line?
https://review.coreboot.org/c/coreboot/+/57034/comment/9c3c09d7_0eb70a32 PS2, Line 47: if (!dev->enabled || dev->path.type != DEVICE_PATH_PCI) Maybe sanity-check the PCI vendor ID?