Attention is currently required from: Tim Crawford, Jeremy Soller, Benjamin Doron. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
Patch Set 6:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/43615/comment/fd64b74f_01d71822 PS6, Line 12: bootblock Out of curiosity, why does this need to be done so early? I would guess it's needed for early PCIe init (in FSP-M) to succeed?
File src/drivers/system76/dgpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43615/comment/9a5a7f00_b2ad7569 PS6, Line 5: Kconfig help text is indented with 2 extra spaces:
help System76 switchable graphics support
File src/drivers/system76/dgpu/bootblock.c:
https://review.coreboot.org/c/coreboot/+/43615/comment/46d9787e_122fc09b PS6, Line 3: //TODO: do not require this to be included in mainboard bootblock.c I see why you need to do it this way: the `DGPU_RST_N` and `DGPU_PWR_EN` definitions are mainboard-specific.
https://review.coreboot.org/c/coreboot/+/43615/comment/5f415c7b_98a108df PS6, Line 9: int bool?
https://review.coreboot.org/c/coreboot/+/43615/comment/a5a70b6a_58b472b1 PS6, Line 11: printk(BIOS_DEBUG, "system76: DGPU power %d\n", onoff); nit: how about explicitly printing `on` and `off`?
printk(BIOS_DEBUG, "system76: DGPU power %s\n", onoff ? "on" : "off");
https://review.coreboot.org/c/coreboot/+/43615/comment/4f7c38d4_475ce455 PS6, Line 24: mdelay(50); Hmmmm, is this delay always needed? It's quite big
File src/drivers/system76/dgpu/ramstage.c:
https://review.coreboot.org/c/coreboot/+/43615/comment/a4d6a339_6429bec9 PS6, Line 11: printk(BIOS_INFO, "system76: %s %s\n", __func__, dev_path(dev)); I'd use BIOS_DEBUG for these prints
https://review.coreboot.org/c/coreboot/+/43615/comment/01bcc35c_63fc1bf0 PS6, Line 36: dev->subsystem_vendor = CONFIG_SUBSYSTEM_VENDOR_ID; : dev->subsystem_device = CONFIG_SUBSYSTEM_DEVICE_ID; : : printk(BIOS_INFO, "%s subsystem <- %04x/%04x\n", : dev_path(dev), dev->subsystem_vendor, dev->subsystem_device); : pci_write_config32(dev, 0x40, : ((dev->subsystem_device & 0xffff) << 16) | : (dev->subsystem_vendor & 0xffff)); `struct pci_operations` has a `set_subsystem` function pointer, which should be used to do this. You can use `src/southbridge/intel/bd82x6x/usb_ehci.c` as an example.
https://review.coreboot.org/c/coreboot/+/43615/comment/12caab4e_789e9fc7 PS6, Line 60: static void dgpu_above_4g(void *unused) Looking at the whole function and the `DRIVERS_SYSTEM76_DGPU_BUS` Kconfig, looks like you may want to use a chip to bind the corresponding devices to this driver. The devicetree would look like this:
device pci 01.0 on chip drivers/system76/dgpu device pci 00.0 on end device pci 00.1 on end end end
You'd need to know in advance how many functions the dGPU has, though. If the dGPU is disabled, you can disable its PCI devices in the chip driver's `enable_dev` callback.
https://review.coreboot.org/c/coreboot/+/43615/comment/15e2495f_09b560be PS6, Line 63: int `unsigned int`?
https://review.coreboot.org/c/coreboot/+/43615/comment/4208d775_6e30d70b PS6, Line 71: %p Is there a need to print this pointer? This prints the address where the `struct device` resides, which doesn't seem useful.
https://review.coreboot.org/c/coreboot/+/43615/comment/e6d84995_fbba9c2d PS6, Line 85: fn); I haven't seen any dGPU with more than 2 functions (function 1 is usually audio), so I'd expect to always see errors here.