Change in coreboot[master]: drivers/system76/dgpu: Add driver for System76 switchable graphics
Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
drivers/system76/dgpu: Add driver for System76 switchable graphics
This adds a driver for System76 systems with switchable NVIDIA graphics. The driver provides ACPI support for dynamically powering on and off the GPU, a function for enabling the GPU power in the bootblock, and a hook in ramstage for moving GPU prefetch resources above 4 GiB.
Tested on system76/addw2, system76/gaze15, and system76/oryp6, which will be submitted in future patches.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I05833e88457bfc3612cc9a8cbf1eec68dcfc5566 --- A src/drivers/system76/dgpu/Kconfig A src/drivers/system76/dgpu/Makefile.inc A src/drivers/system76/dgpu/acpi/dgpu.asl A src/drivers/system76/dgpu/bootblock.c A src/drivers/system76/dgpu/ramstage.c 5 files changed, 313 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/43615/1
diff --git a/src/drivers/system76/dgpu/Kconfig b/src/drivers/system76/dgpu/Kconfig new file mode 100644 index 0000000..5096671 --- /dev/null +++ b/src/drivers/system76/dgpu/Kconfig @@ -0,0 +1,5 @@ +config DRIVERS_SYSTEM76_DGPU + bool + default n + help + System76 switchable graphics support diff --git a/src/drivers/system76/dgpu/Makefile.inc b/src/drivers/system76/dgpu/Makefile.inc new file mode 100644 index 0000000..a230f97 --- /dev/null +++ b/src/drivers/system76/dgpu/Makefile.inc @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only + +ramstage-$(CONFIG_DRIVERS_SYSTEM76_DGPU) += ramstage.c diff --git a/src/drivers/system76/dgpu/acpi/dgpu.asl b/src/drivers/system76/dgpu/acpi/dgpu.asl new file mode 100644 index 0000000..a721934 --- /dev/null +++ b/src/drivers/system76/dgpu/acpi/dgpu.asl @@ -0,0 +1,201 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +Device (_SB.PCI0.PEGP) { + Name (_ADR, 0x00010000) + + PowerResource (PWRR, 0, 0) { + Name (_STA, 1) + + Method (_ON) { + Debug = "PEGP.PWRR._ON" + If (_STA != 1) { + _SB.PCI0.PEGP.DEV0._ON () + _STA = 1 + } + } + + Method (_OFF) { + Debug = "PEGP.PWRR._OFF" + If (_STA != 0) { + _SB.PCI0.PEGP.DEV0._OFF () + _STA = 0 + } + } + } + + Name (_PR0, Package () { _SB.PCI0.PEGP.PWRR }) + Name (_PR2, Package () { _SB.PCI0.PEGP.PWRR }) + Name (_PR3, Package () { _SB.PCI0.PEGP.PWRR }) +} + +Device (_SB.PCI0.PEGP.DEV0) { + Name(_ADR, 0x00000000) + Name (_STA, 0xF) + Name (LTRE, 0) + + // Memory mapped PCI express registers + // Not sure what this stuff is, but it is used to get into GC6 + OperationRegion (RPCX, SystemMemory, 0xE0008000, 0x1000) + Field (RPCX, ByteAcc, NoLock, Preserve) { + PVID, 16, + PDID, 16, + CMDR, 8, + Offset (0x19), + PRBN, 8, + Offset (0x84), + D0ST, 2, + Offset (0xAA), + CEDR, 1, + Offset (0xAC), + , 4, + CMLW, 6, + Offset (0xB0), + ASPM, 2, + , 2, + P0LD, 1, + RTLK, 1, + Offset (0xC9), + , 2, + LREN, 1, + Offset (0x11A), + , 1, + VCNP, 1, + Offset (0x214), + Offset (0x216), + P0LS, 4, + Offset (0x248), + , 7, + Q0L2, 1, + Q0L0, 1, + Offset (0x504), + Offset (0x506), + PCFG, 2, + Offset (0x508), + TREN, 1, + Offset (0xC20), + , 4, + P0AP, 2, + Offset (0xC38), + , 3, + P0RM, 1, + Offset (0xC74), + P0LT, 4, + Offset (0xD0C), + , 20, + LREV, 1 + } + + Method (_ON) { + Debug = "PEGP.DEV0._ON" + + If (_STA != 0xF) { + Debug = " If DGPU_PWR_EN low" + If (! GTXS (DGPU_PWR_EN)) { + Debug = " DGPU_PWR_EN high" + STXS (DGPU_PWR_EN) + + Debug = " Sleep 16" + Sleep (16) + } + + Debug = " DGPU_RST_N high" + STXS(DGPU_RST_N) + + Debug = " Sleep 10" + Sleep (10) + + Debug = " Q0L0 = 1" + Q0L0 = 1 + + Debug = " Sleep 16" + Sleep (16) + + Debug = " While Q0L0" + Local0 = 0 + While (Q0L0) { + If ((Local0 > 4)) { + Debug = " While Q0L0 timeout" + Break + } + + Sleep (16) + Local0++ + } + + Debug = " P0RM = 0" + P0RM = 0 + + Debug = " P0AP = 0" + P0AP = 0 + + Debug = Concatenate(" LREN = ", ToHexString(LTRE)) + LREN = LTRE + + Debug = " CEDR = 1" + CEDR = 1 + + Debug = " CMDR |= 7" + CMDR |= 7 + + Debug = " _STA = 0xF" + _STA = 0xF + } + } + + Method (_OFF) { + Debug = "PEGP.DEV0._OFF" + + If (_STA != 0x5) { + Debug = Concatenate(" LTRE = ", ToHexString(LREN)) + LTRE = LREN + + Debug = " Q0L2 = 1" + Q0L2 = 1 + + Debug = " Sleep 16" + Sleep (16) + + Debug = " While Q0L2" + Local0 = Zero + While (Q0L2) { + If ((Local0 > 4)) { + Debug = " While Q0L2 timeout" + Break + } + + Sleep (16) + Local0++ + } + + Debug = " P0RM = 1" + P0RM = 1 + + Debug = " P0AP = 3" + P0AP = 3 + + Debug = " Sleep 10" + Sleep (10) + + Debug = " DGPU_RST_N low" + CTXS(DGPU_RST_N) + + Debug = " While DGPU_GC6 low" + Local0 = Zero + While (! GRXS(DGPU_GC6)) { + If ((Local0 > 4)) { + Debug = " While DGPU_GC6 low timeout" + + Debug = " DGPU_PWR_EN low" + CTXS (DGPU_PWR_EN) + Break + } + + Sleep (16) + Local0++ + } + + Debug = " _STA = 0x5" + _STA = 0x5 + } + } +} diff --git a/src/drivers/system76/dgpu/bootblock.c b/src/drivers/system76/dgpu/bootblock.c new file mode 100644 index 0000000..61f7fbf --- /dev/null +++ b/src/drivers/system76/dgpu/bootblock.c @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +//TODO: do not require this to be included in mainboard bootblock.c + +#include <console/console.h> +#include <delay.h> +#include <gpio.h> + +static void dgpu_power_enable(int onoff) { + printk(BIOS_DEBUG, "system76: DGPU power %d\n", onoff); + if (onoff) { + gpio_set(DGPU_RST_N, 0); + mdelay(4); + gpio_set(DGPU_PWR_EN, 1); + mdelay(4); + gpio_set(DGPU_RST_N, 1); + } else { + gpio_set(DGPU_RST_N, 0); + mdelay(4); + gpio_set(DGPU_PWR_EN, 0); + } + mdelay(50); +} diff --git a/src/drivers/system76/dgpu/ramstage.c b/src/drivers/system76/dgpu/ramstage.c new file mode 100644 index 0000000..eeb1607 --- /dev/null +++ b/src/drivers/system76/dgpu/ramstage.c @@ -0,0 +1,81 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <bootstate.h> +#include <console/console.h> +#include <device/pci.h> + +static void dgpu_read_resources(struct device *dev) { + printk(BIOS_INFO, "system76: dgpu_read_resources %s\n", dev_path(dev)); + + pci_dev_read_resources(dev); + + int bar; + // Find all BARs on DGPU, mark them above 4g if prefetchable + for (bar = PCI_BASE_ADDRESS_0; bar <= PCI_BASE_ADDRESS_5; bar += 4) { + printk(BIOS_INFO, " BAR at 0x%02x\n", bar); + + struct resource *res; + res = probe_resource(dev, bar); + if (res) { + if (res->flags & IORESOURCE_PREFETCH) { + printk(BIOS_INFO, " marked above 4g\n"); + res->flags |= IORESOURCE_ABOVE_4G; + } else { + printk(BIOS_INFO, " not prefetch\n"); + } + } else { + printk(BIOS_INFO, " not found\n"); + } + } +} + +static void dgpu_enable_resources(struct device *dev) { + printk(BIOS_INFO, "system76: dgpu_enable_resources %s\n", dev_path(dev)); + + dev->subsystem_vendor = CONFIG_SUBSYSTEM_VENDOR_ID; + dev->subsystem_device = CONFIG_SUBSYSTEM_DEVICE_ID; + printk(BIOS_INFO, " subsystem <- %04x/%04x\n", dev->subsystem_vendor, dev->subsystem_device); + pci_write_config32(dev, 0x40, ((dev->subsystem_device & 0xffff) << 16) | (dev->subsystem_vendor & 0xffff)); + + pci_dev_enable_resources(dev); +} + +static struct device_operations dgpu_pci_ops_dev = { + .read_resources = dgpu_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = dgpu_enable_resources, +#if CONFIG(HAVE_ACPI_TABLES) + .write_acpi_tables = pci_rom_write_acpi_tables, + .acpi_fill_ssdt = pci_rom_ssdt, +#endif + .init = pci_dev_init, + .ops_pci = &pci_dev_ops_pci, +}; + +static void dgpu_above_4g(void *unused) { + struct device *pdev; + + // Find PEG0 + pdev = pcidev_on_root(1, 0); + if (!pdev) { + printk(BIOS_ERR, "system76: failed to find PEG0\n"); + return; + } + printk(BIOS_INFO, "system76: PEG0 at %p, %04x:%04x\n", pdev, pdev->vendor, pdev->device); + + int fn; + for (fn = 0; fn < 8; fn++) { + struct device *dev; + + // Find DGPU functions + dev = pcidev_path_behind(pdev->link_list, PCI_DEVFN(0, fn)); + if (dev) { + printk(BIOS_INFO, "system76: DGPU fn %d at %p, %04x:%04x\n", fn, dev, dev->vendor, dev->device); + dev->ops = &dgpu_pci_ops_dev; + } else { + printk(BIOS_ERR, "system76: failed to find DGPU fn %d\n", fn); + } + } +} + +BOOT_STATE_INIT_ENTRY(BS_DEV_RESOURCES, BS_ON_ENTRY, dgpu_above_4g, NULL);
build bot (Jenkins) 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 1:
(34 comments)
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/b... File src/drivers/system76/dgpu/bootblock.c:
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/b... PS1, Line 9: static void dgpu_power_enable(int onoff) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... File src/drivers/system76/dgpu/ramstage.c:
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 7: static void dgpu_read_resources(struct device *dev) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 8: printk(BIOS_INFO, "system76: dgpu_read_resources %s\n", dev_path(dev)); Prefer using '"%s...", __func__' to using 'dgpu_read_resources', this function's name, in a string
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 10: pci_dev_read_resources(dev); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 17: struct resource *res; Statements should start on a tabstop
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 32: static void dgpu_enable_resources(struct device *dev) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 33: printk(BIOS_INFO, "system76: dgpu_enable_resources %s\n", dev_path(dev)); Prefer using '"%s...", __func__' to using 'dgpu_enable_resources', this function's name, in a string
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 37: printk(BIOS_INFO, " subsystem <- %04x/%04x\n", dev->subsystem_vendor, dev->subsystem_device); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 38: pci_write_config32(dev, 0x40, ((dev->subsystem_device & 0xffff) << 16) | (dev->subsystem_vendor & 0xffff)); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 55: static void dgpu_above_4g(void *unused) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 64: printk(BIOS_INFO, "system76: PEG0 at %p, %04x:%04x\n", pdev, pdev->vendor, pdev->device); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 66: int fn; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 67: for (fn = 0; fn < 8; fn++) { please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 67: for (fn = 0; fn < 8; fn++) { suspect code indent for conditional statements (4, 12)
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 68: struct device *dev; Statements should start on a tabstop
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 70: // Find DGPU functions code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 71: dev = pcidev_path_behind(pdev->link_list, PCI_DEVFN(0, fn)); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 71: dev = pcidev_path_behind(pdev->link_list, PCI_DEVFN(0, fn)); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 72: if (dev) { code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 72: if (dev) { please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 72: if (dev) { suspect code indent for conditional statements (8, 12)
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 73: printk(BIOS_INFO, "system76: DGPU fn %d at %p, %04x:%04x\n", fn, dev, dev->vendor, dev->device); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 73: printk(BIOS_INFO, "system76: DGPU fn %d at %p, %04x:%04x\n", fn, dev, dev->vendor, dev->device); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 73: printk(BIOS_INFO, "system76: DGPU fn %d at %p, %04x:%04x\n", fn, dev, dev->vendor, dev->device); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 74: dev->ops = &dgpu_pci_ops_dev; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 74: dev->ops = &dgpu_pci_ops_dev; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 75: } else { code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 75: } else { please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 75: } else { suspect code indent for conditional statements (8, 12)
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 76: printk(BIOS_ERR, "system76: failed to find DGPU fn %d\n", fn); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 76: printk(BIOS_ERR, "system76: failed to find DGPU fn %d\n", fn); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 77: } code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 77: } please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43615/1/src/drivers/system76/dgpu/r... PS1, Line 78: } please, no spaces at the start of a line
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43615
to look at the new patch set (#2).
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
drivers/system76/dgpu: Add driver for System76 switchable graphics
This adds a driver for System76 systems with switchable NVIDIA graphics. The driver provides ACPI support for dynamically powering on and off the GPU, a function for enabling the GPU power in the bootblock, and a hook in ramstage for moving GPU prefetch resources above 4 GiB.
Tested on system76/addw2, system76/gaze15, and system76/oryp6, which will be submitted in future patches.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I05833e88457bfc3612cc9a8cbf1eec68dcfc5566 --- A src/drivers/system76/dgpu/Kconfig A src/drivers/system76/dgpu/Makefile.inc A src/drivers/system76/dgpu/acpi/dgpu.asl A src/drivers/system76/dgpu/bootblock.c A src/drivers/system76/dgpu/ramstage.c 5 files changed, 323 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/43615/2
Michael Niewöhner 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 2:
(1 comment)
AFAIU this isn't s76 specific but very generic code which should work for other boards as well. Why not
https://review.coreboot.org/c/coreboot/+/43615/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43615/2//COMMIT_MSG@9 PS2, Line 9: This adds a driver for System76 systems with switchable NVIDIA : graphics. AFAIU this isn't s76 specific but very generic code which should work for other boards as well. Correct me, if I'm wrong, please
Jeremy Soller 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 2:
Patch Set 2:
(1 comment)
AFAIU this isn't s76 specific but very generic code which should work for other boards as well. Why not
I haven't seen any other vendors with NVIDIA GPUs supporting RTD3 using GC6, so I modeled this change after drivers/lenovo/hybrid_graphics.
I agree it probably is more generic, supporting any NVIDIA laptop with fairly recent GPUs, but I have no non-System76 hardware that is supported by coreboot to compare with.
Michael Niewöhner 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 2:
Patch Set 2:
Patch Set 2:
(1 comment)
AFAIU this isn't s76 specific but very generic code which should work for other boards as well. Why not
I haven't seen any other vendors with NVIDIA GPUs supporting RTD3 using GC6, so I modeled this change after drivers/lenovo/hybrid_graphics.
I agree it probably is more generic, supporting any NVIDIA laptop with fairly recent GPUs, but I have no non-System76 hardware that is supported by coreboot to compare with.
I see. Hmm, lenovo does some more bits but the gpio mechanism seems so be similiar. The lenovo driver is for older platforms, though (src/southbridge). IMHO that could have been done in ageneric way, too, while providing optional vendor additions.
Tim Crawford has uploaded a new patch set (#3) to the change originally created by Jeremy Soller. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
drivers/system76/dgpu: Add driver for System76 switchable graphics
This adds a driver for System76 systems with switchable NVIDIA graphics. The driver provides ACPI support for dynamically powering on and off the GPU, a function for enabling the GPU power in the bootblock, and a hook in ramstage for moving GPU prefetch resources above 4 GiB.
Tested on system76/addw2, system76/gaze15, and system76/oryp6, which will be submitted in future patches.
Change-Id: I05833e88457bfc3612cc9a8cbf1eec68dcfc5566 Signed-off-by: Jeremy Soller jeremy@system76.com Signed-off-by: Tim Crawford tcrawford@system76.com --- A src/drivers/system76/dgpu/Kconfig A src/drivers/system76/dgpu/Makefile.inc A src/drivers/system76/dgpu/acpi/dgpu.asl A src/drivers/system76/dgpu/bootblock.c A src/drivers/system76/dgpu/ramstage.c 5 files changed, 323 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/43615/3
Michael Niewöhner 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 3:
just realized that this could also be done like src/soc/intel/common/block/pcie/rtd3, so the gpios can be specified in the devicetree
Benjamin Doron 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 3:
Patch Set 3:
just realized that this could also be done like src/soc/intel/common/block/pcie/rtd3, so the gpios can be specified in the devicetree
Whether a GPIO is active high or active low can vary by board as well.
Attention is currently required from: Tim Crawford, Jeremy Soller. Benjamin Doron 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 3:
(1 comment)
Patchset:
PS3:
Patch Set 3:
Patch Set 3:
just realized that this could also be done like src/soc/intel/common/block/pcie/rtd3, so the gpios can be specified in the devicetree
Whether a GPIO is active high or active low can vary by board as well.
As can the delay sequence. Maybe let boards implement the bootblock code (achieved by the #include) and provide hooks in dgpu.asl if this is intended to support more boards?
Attention is currently required from: Tim Crawford, Benjamin Doron, Jeremy Soller. Michael Niewöhner 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 3:
(1 comment)
Patchset:
PS3:
Patch Set 3: […]
delay can be set via devicetree option. setting gpio in devicetree is already common practice (see touchpad acpi stuff for example)
Attention is currently required from: Tim Crawford, Benjamin Doron, Jeremy Soller. Tim Crawford has uploaded a new patch set (#4) to the change originally created by Jeremy Soller. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
drivers/system76/dgpu: Add driver for System76 switchable graphics
This adds a driver for System76 systems with switchable NVIDIA graphics. The driver provides ACPI support for dynamically powering on and off the GPU, a function for enabling the GPU power in the bootblock, and a hook in ramstage for moving GPU prefetch resources above 4 GiB.
Tested on system76/addw2, system76/gaze15, and system76/oryp6, which will be submitted in future patches.
Change-Id: I05833e88457bfc3612cc9a8cbf1eec68dcfc5566 Signed-off-by: Jeremy Soller jeremy@system76.com Signed-off-by: Tim Crawford tcrawford@system76.com --- A src/drivers/system76/dgpu/Kconfig A src/drivers/system76/dgpu/Makefile.inc A src/drivers/system76/dgpu/acpi/dgpu.asl A src/drivers/system76/dgpu/bootblock.c A src/drivers/system76/dgpu/ramstage.c 5 files changed, 331 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/43615/4
Attention is currently required from: Tim Crawford, Benjamin Doron. Tim Crawford has uploaded a new patch set (#6) to the change originally created by Jeremy Soller. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
drivers/system76/dgpu: Add driver for System76 switchable graphics
This adds a driver for System76 systems with switchable NVIDIA graphics. The driver provides ACPI support for dynamically powering on and off the GPU, a function for enabling the GPU power in the bootblock, and a hook in ramstage for moving GPU prefetch resources above 4 GiB.
Tested on system76/gaze15 and system76/oryp6.
Change-Id: I05833e88457bfc3612cc9a8cbf1eec68dcfc5566 Signed-off-by: Jeremy Soller jeremy@system76.com Signed-off-by: Tim Crawford tcrawford@system76.com --- A src/drivers/system76/dgpu/Kconfig A src/drivers/system76/dgpu/Makefile.inc A src/drivers/system76/dgpu/acpi/dgpu.asl A src/drivers/system76/dgpu/bootblock.c A src/drivers/system76/dgpu/ramstage.c 5 files changed, 330 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/43615/6
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.
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:
(1 comment)
File src/drivers/system76/dgpu/acpi/dgpu.asl:
https://review.coreboot.org/c/coreboot/+/43615/comment/318d605e_fbdf2d04 PS6, Line 139: CMDR |= 7 This enables Bus Master. I wonder if it's required
Attention is currently required from: Tim Crawford, Jeremy Soller, Benjamin Doron. Tim Crawford has uploaded a new patch set (#7) to the change originally created by Jeremy Soller. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
drivers/system76/dgpu: Add driver for System76 switchable graphics
This adds a driver for System76 systems with switchable NVIDIA graphics. The driver provides ACPI support for dynamically powering on and off the GPU, a function for enabling the GPU power in the bootblock, and a hook in ramstage for moving GPU prefetch resources above 4 GiB.
Tested on system76/gaze15 and system76/oryp6.
Change-Id: I05833e88457bfc3612cc9a8cbf1eec68dcfc5566 Signed-off-by: Jeremy Soller jeremy@system76.com Signed-off-by: Tim Crawford tcrawford@system76.com --- A src/drivers/system76/dgpu/Kconfig A src/drivers/system76/dgpu/Makefile.inc A src/drivers/system76/dgpu/acpi/dgpu.asl A src/drivers/system76/dgpu/bootblock.c A src/drivers/system76/dgpu/ramstage.c 5 files changed, 332 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/43615/7
Attention is currently required from: Jeremy Soller, Benjamin Doron, Angel Pons. Tim Crawford 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 7:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/43615/comment/9e0aba7f_1f4afcb3 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 i […]
I didn't notice any problem doing it in romstage (after cannonlake_memcfg_init).
File src/drivers/system76/dgpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43615/comment/6d6b2cce_c5cc7fa1 PS6, Line 5:
Kconfig help text is indented with 2 extra spaces: […]
Done
File src/drivers/system76/dgpu/bootblock.c:
https://review.coreboot.org/c/coreboot/+/43615/comment/b4e054e9_704b788e 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-s […]
I attempted to use a chip to set them in CB:57034, but can't get it to work.
https://review.coreboot.org/c/coreboot/+/43615/comment/c6c7c67a_0e5c0e50 PS6, Line 9: int
bool?
Done
https://review.coreboot.org/c/coreboot/+/43615/comment/a4d2b824_92292b45 PS6, Line 11: printk(BIOS_DEBUG, "system76: DGPU power %d\n", onoff);
nit: how about explicitly printing `on` and `off`? […]
Done
File src/drivers/system76/dgpu/ramstage.c:
https://review.coreboot.org/c/coreboot/+/43615/comment/6be6ac8a_5805f8bb PS6, Line 63: int
`unsigned int`?
Done
https://review.coreboot.org/c/coreboot/+/43615/comment/e9c54ac8_60bbceef PS6, Line 71: %p
Is there a need to print this pointer? This prints the address where the `struct device` resides, wh […]
Done
https://review.coreboot.org/c/coreboot/+/43615/comment/91f1ff6a_3fa5b592 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 a […]
NVIDIA GPUs have a max of 4 functions. fn 2 and 3 are for USB.
https://download.nvidia.com/XFree86/Linux-x86_64/470.63.01/README/dynamicpow...
Attention is currently required from: Jeremy Soller, Benjamin Doron, Angel Pons. Tim Crawford 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 7:
(1 comment)
File src/drivers/system76/dgpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43615/comment/14396b3f_1a06146f PS7, Line 8: hex "PCI bridge for the GPU devuce" Yeah, yeah. I misspelled it.
Attention is currently required from: Jeremy Soller, Benjamin Doron, Angel Pons. Tim Crawford 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 7:
(1 comment)
Patchset:
PS7: Rewrote as a more general NVIDIA Optimus driver in CB:57034.
Jeremy Soller has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43615 )
Change subject: drivers/system76/dgpu: Add driver for System76 switchable graphics ......................................................................
Abandoned
Rewritten in https://review.coreboot.org/c/coreboot/+/57034/
-
Angel Pons (Code Review) -
Benjamin Doron (Code Review)
-
build bot (Jenkins) (Code Review)
-
Jeremy Soller (Code Review)
-
Michael Niewöhner (Code Review)
-
Tim Crawford (Code Review)