Change in ...coreboot[master]: drivers/apple: Add hybrid graphics driver
Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32719 Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 417 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/1 diff --git a/src/drivers/apple/hybrid_graphics/Kconfig b/src/drivers/apple/hybrid_graphics/Kconfig new file mode 100644 index 0000000..252373f --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/Kconfig @@ -0,0 +1,3 @@ +config DRIVERS_APPLE_HYBRID_GRAPHICS + bool + default n diff --git a/src/drivers/apple/hybrid_graphics/Makefile.inc b/src/drivers/apple/hybrid_graphics/Makefile.inc new file mode 100644 index 0000000..ea45b45 --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/Makefile.inc @@ -0,0 +1,15 @@ +# +# This file is part of the coreboot project. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; version 2 of the License. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# + +romstage-$(CONFIG_DRIVERS_APPLE_HYBRID_GRAPHICS) += gmux.c romstage.c +ramstage-$(CONFIG_DRIVERS_APPLE_HYBRID_GRAPHICS) += gmux.c hybrid_graphics.c diff --git a/src/drivers/apple/hybrid_graphics/chip.h b/src/drivers/apple/hybrid_graphics/chip.h new file mode 100644 index 0000000..39434f8 --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/chip.h @@ -0,0 +1,30 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Evgeny Zinoviev <me@ch1p.io> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _APPLE_HYBRID_GRAPHICS_CHIP_H_ +#define _APPLE_HYBRID_GRAPHICS_CHIP_H_ + +enum hybrid_graphics_req { + HYBRID_GRAPHICS_INTEGRATED = 0, + HYBRID_GRAPHICS_DISCRETE = 1 +}; + +#define HYBRID_GRAPHICS_DEFAULT_GPU HYBRID_GRAPHICS_INTEGRATED + +struct drivers_apple_hybrid_graphics_config { + unsigned int gmux_indexed; +}; + +#endif /* _APPLE_HYBRID_GRAPHICS_CHIP_H_ */ diff --git a/src/drivers/apple/hybrid_graphics/gmux.c b/src/drivers/apple/hybrid_graphics/gmux.c new file mode 100644 index 0000000..e1f763a --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/gmux.c @@ -0,0 +1,158 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) Canonical Ltd. <seth.forshee@canonical.com> + * Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de> + * Copyright (C) 2015 Lukas Wunner <lukas@wunner.de> + * Copyright (C) 2019 Evgeny Zinoviev <me@ch1p.io> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; version 2 of + * the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <delay.h> +#include <arch/io.h> +#include <device/device.h> +#include "gmux.h" +#include "chip.h" + +static int gmux_index_wait_ready(void) +{ + int i = 200; + u8 gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); + + while (i && (gwr & 0x01)) { + inb(GMUX_IOSTART + GMUX_PORT_READ); + gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); + udelay(100); + i--; + } + + return !!i; +} + +static int gmux_index_wait_complete(void) +{ + int i = 200; + u8 gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); + + while (i && !(gwr & 0x01)) { + gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); + udelay(100); + i--; + } + + if (gwr & 0x01) + inb(GMUX_IOSTART + GMUX_PORT_READ); + + return !!i; +} + +u8 gmux_pio_read8(int port) +{ + return inb(GMUX_IOSTART + port); +} + +u8 gmux_index_read8(int port) +{ + u8 val; + + gmux_index_wait_ready(); + outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ); + gmux_index_wait_complete(); + val = inb(GMUX_IOSTART + GMUX_PORT_VALUE); + + return val; +} + +void gmux_pio_write8(int port, u8 val) +{ + outb(val, GMUX_IOSTART + port); +} + + +void gmux_index_write8(int port, u8 val) +{ + outb(val, GMUX_IOSTART + GMUX_PORT_VALUE); + gmux_index_wait_ready(); + outb(port & 0xff, GMUX_IOSTART + GMUX_PORT_WRITE); + gmux_index_wait_complete(); +} + +u32 gmux_pio_read32(int port) +{ + return inl(GMUX_IOSTART + port); +} + +u32 gmux_index_read32(int port) +{ + u32 val; + + gmux_index_wait_ready(); + outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ); + gmux_index_wait_complete(); + val = inl(GMUX_IOSTART + GMUX_PORT_VALUE); + + return val; +} + +u8 gmux_read8(const struct device *dev, int port) +{ + const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; + if (config->gmux_indexed) { + return gmux_index_read8(port); + } else { + return gmux_pio_read8(port); + } +} + +void gmux_write8(const struct device *dev, int port, u8 val) +{ + const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; + if (config->gmux_indexed) { + gmux_index_write8(port, val); + } else { + gmux_pio_write8(port, val); + } +} + +u32 gmux_read32(const struct device *dev, int port) +{ + const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; + if (config->gmux_indexed) { + return gmux_index_read32(port); + } else { + return gmux_pio_read32(port); + } +} + +void gmux_dgpu_power_enable(const struct device *dev, bool enable) +{ + if (enable) { + gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 1); + gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 3); + } else { + gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 1); + gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 0); + } +} + +void gmux_switch(const struct device *dev, bool dgpu) +{ + if (dgpu) { + gmux_write8(dev, GMUX_PORT_SWITCH_DDC, 2); + gmux_write8(dev, GMUX_PORT_SWITCH_DISPLAY, 3); + } else { + gmux_write8(dev, GMUX_PORT_SWITCH_DDC, 1); + gmux_write8(dev, GMUX_PORT_SWITCH_DISPLAY, 2); + } +} + + diff --git a/src/drivers/apple/hybrid_graphics/gmux.h b/src/drivers/apple/hybrid_graphics/gmux.h new file mode 100644 index 0000000..18f6722 --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/gmux.h @@ -0,0 +1,60 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) Canonical Ltd. <seth.forshee@canonical.com> + * Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de> + * Copyright (C) 2015 Lukas Wunner <lukas@wunner.de> + * Copyright (C) 2019 Evgeny Zinoviev <me@ch1p.io> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; version 2 of + * the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef EC_APPLE_GMUX_H +#define EC_APPLE_GMUX_H + +#define GMUX_PORT_VERSION_MAJOR 0x04 +#define GMUX_PORT_VERSION_MINOR 0x05 +#define GMUX_PORT_VERSION_RELEASE 0x06 + +#define GMUX_PORT_SWITCH_DISPLAY 0x10 +#define GMUX_PORT_SWITCH_DDC 0x28 +#define GMUX_PORT_DISCRETE_POWER 0x50 +#define GMUX_PORT_MAX_BRIGHTNESS 0x70 +#define GMUX_PORT_BRIGHTNESS 0x74 +#define GMUX_PORT_VALUE 0xc2 +#define GMUX_PORT_READ 0xd0 +#define GMUX_PORT_WRITE 0xd4 + +#define GMUX_PORT_INTERRUPT_ENABLE 0x14 +#define GMUX_INTERRUPT_ENABLE 0xff +#define GMUX_INTERRUPT_DISABLE 0x00 + +#define GMUX_BRIGHTNESS_MASK 0x00ffffff +#define GMUX_MAX_BRIGHTNESS GMUX_BRIGHTNESS_MASK + +#define GMUX_IOSTART 0x700 + +u8 gmux_index_read8(int port); +u8 gmux_pio_read8(int port); +u8 gmux_read8(const struct device *dev, int port); + +void gmux_index_write8(int port, u8 val); +void gmux_pio_write8(int port, u8 val); +void gmux_write8(const struct device *dev, int port, u8 val); + +u32 gmux_index_read32(int port); +u32 gmux_pio_read32(int port); +u32 gmux_read32(const struct device *dev, int port); + +void gmux_switch(const struct device *dev, bool dgpu); +void gmux_dgpu_power_enable(const struct device *dev, bool enable); + +#endif /* EC_APPLE_GMUX_H */ diff --git a/src/drivers/apple/hybrid_graphics/hybrid_graphics.c b/src/drivers/apple/hybrid_graphics/hybrid_graphics.c new file mode 100644 index 0000000..804eb76 --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/hybrid_graphics.c @@ -0,0 +1,64 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Evgeny Zinoviev <me@ch1p.io> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <types.h> +#include <option.h> +#include <device/device.h> + +#include <southbridge/intel/common/gpio.h> +#include <console/console.h> +#include "chip.h" +#include "gmux.h" + +static void enable_dev(struct device *dev) +{ + printk(BIOS_INFO, "Hybrid graphics enable_dev\n"); + + const struct drivers_lenovo_hybrid_graphics_config *config; + enum hybrid_graphics_req mode; + u8 ver_major, ver_minor, ver_release; + u32 version, max_brightness, brightness; + + /* Don't confuse anyone else and disable the fake device */ + dev->enabled = 0; + + config = dev->chip_info; + if (!config) { + printk(BIOS_INFO, "Hybrid graphics: Not installed\n"); + return; + } + + version = gmux_index_read32(GMUX_PORT_VERSION_MAJOR); + ver_major = (version >> 24) & 0xff; + ver_minor = (version >> 16) & 0xff; + ver_release = (version >> 8) & 0xff; + max_brightness = gmux_index_read32(GMUX_PORT_MAX_BRIGHTNESS); + brightness = gmux_index_read32(GMUX_PORT_BRIGHTNESS) & GMUX_BRIGHTNESS_MASK; + + printk(BIOS_INFO, "gmux version: %d.%d.%d\n", + ver_major, ver_minor, ver_release); + printk(BIOS_INFO, "gmux max brightness: %d\n", max_brightness); + printk(BIOS_INFO, "gmux brightness: %d\n", brightness); + + mode = HYBRID_GRAPHICS_DEFAULT_GPU; + get_option(&mode, "hybrid_graphics_mode"); + + gmux_switch(dev, mode == HYBRID_GRAPHICS_DISCRETE); +} + +struct chip_operations drivers_apple_hybrid_graphics_ops = { + CHIP_NAME("Apple hybrid graphics driver") + .enable_dev = enable_dev +}; diff --git a/src/drivers/apple/hybrid_graphics/hybrid_graphics.h b/src/drivers/apple/hybrid_graphics/hybrid_graphics.h new file mode 100644 index 0000000..782be44 --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/hybrid_graphics.h @@ -0,0 +1,24 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Evgeny Zinoviev <me@ch1p.io> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _DRIVERS_APPLE_HYBRID_GRAPHICS_H_ +#define _DRIVERS_APPLE_HYBRID_GRAPHICS_H_ + +#define HYBRID_GRAPHICS_PORT 0xff +#define HYBRID_GRAPHICS_DEVICE 0xf + +void early_hybrid_graphics(bool *enable_igd, bool *enable_peg); + +#endif /* _DRIVERS_APPLE_HYBRID_GRAPHICS_CHIP_H_ */ diff --git a/src/drivers/apple/hybrid_graphics/romstage.c b/src/drivers/apple/hybrid_graphics/romstage.c new file mode 100644 index 0000000..9cd5098 --- /dev/null +++ b/src/drivers/apple/hybrid_graphics/romstage.c @@ -0,0 +1,63 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2017 Patrick Rudolph <siro@das-labor.org> + * Copyright (C) 2019 Evgeny Zinoviev <me@ch1p.io> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <types.h> +#include <option.h> +#include <device/device.h> +#include <console/console.h> +#include "hybrid_graphics.h" +#include "chip.h" +#include "gmux.h" + +void early_hybrid_graphics(bool *enable_igd, bool *enable_peg) +{ + const struct drivers_apple_hybrid_graphics_config *config; + const struct device *dev; + + enum hybrid_graphics_req mode = HYBRID_GRAPHICS_DEFAULT_GPU; + + printk(BIOS_INFO, "Hybrid graphics early_hybrid_graphics\n"); + + /* TODO: Use generic device instead of dummy PNP device */ + dev = dev_find_slot_pnp(HYBRID_GRAPHICS_PORT, HYBRID_GRAPHICS_DEVICE); + + if (!dev || !dev->chip_info) { + printk(BIOS_ERR, "Hybrid graphics: ERROR\n"); + *enable_igd = true; + *enable_peg = false; + return; + } + + config = dev->chip_info; + + get_option(&mode, "hybrid_graphics_mode"); + + if (mode == HYBRID_GRAPHICS_DISCRETE) { + printk(BIOS_DEBUG, "Hybrid graphics:" + " Disabling integrated GPU.\n"); + + *enable_igd = false; + *enable_peg = true; + } else if (mode == HYBRID_GRAPHICS_INTEGRATED) { + printk(BIOS_DEBUG, "Hybrid graphics:" + " Disabling discrete GPU.\n"); + + *enable_igd = true; + *enable_peg = false; + } + + gmux_dgpu_power_enable(dev, *enable_peg); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 1 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-MessageType: newchange
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 1: (99 comments) https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/ch... File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/ch... PS1, Line 27: unsigned int gmux_indexed; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 28: int i = 200; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 29: u8 gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 31: while (i && (gwr & 0x01)) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 32: inb(GMUX_IOSTART + GMUX_PORT_READ); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 32: inb(GMUX_IOSTART + GMUX_PORT_READ); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 33: gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 33: gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 34: udelay(100); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 34: udelay(100); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 35: i--; code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 35: i--; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 36: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 38: return !!i; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 43: int i = 200; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 44: u8 gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 46: while (i && !(gwr & 0x01)) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 47: gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 47: gwr = inb(GMUX_IOSTART + GMUX_PORT_WRITE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 48: udelay(100); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 48: udelay(100); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 49: i--; code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 49: i--; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 50: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 52: if (gwr & 0x01) please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 53: inb(GMUX_IOSTART + GMUX_PORT_READ); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 53: inb(GMUX_IOSTART + GMUX_PORT_READ); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 55: return !!i; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 60: return inb(GMUX_IOSTART + port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 65: u8 val; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 67: gmux_index_wait_ready(); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 68: outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 69: gmux_index_wait_complete(); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 70: val = inb(GMUX_IOSTART + GMUX_PORT_VALUE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 72: return val; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 77: outb(val, GMUX_IOSTART + port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 83: outb(val, GMUX_IOSTART + GMUX_PORT_VALUE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 84: gmux_index_wait_ready(); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 85: outb(port & 0xff, GMUX_IOSTART + GMUX_PORT_WRITE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 86: gmux_index_wait_complete(); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 91: return inl(GMUX_IOSTART + port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 96: u32 val; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 98: gmux_index_wait_ready(); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 99: outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 100: gmux_index_wait_complete(); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 101: val = inl(GMUX_IOSTART + GMUX_PORT_VALUE); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 103: return val; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 108: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 109: if (config->gmux_indexed) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 109: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 110: return gmux_index_read8(port); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 110: return gmux_index_read8(port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 111: } else { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 112: return gmux_pio_read8(port); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 112: return gmux_pio_read8(port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 113: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 118: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 119: if (config->gmux_indexed) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 119: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 120: gmux_index_write8(port, val); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 120: gmux_index_write8(port, val); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 121: } else { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 122: gmux_pio_write8(port, val); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 122: gmux_pio_write8(port, val); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 123: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 128: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 129: if (config->gmux_indexed) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 129: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 130: return gmux_index_read32(port); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 130: return gmux_index_read32(port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 131: } else { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 132: return gmux_pio_read32(port); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 132: return gmux_pio_read32(port); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 133: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 138: if (enable) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 139: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 1); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 139: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 1); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 140: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 3); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 140: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 3); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 141: } else { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 142: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 1); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 142: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 1); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 143: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 0); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 143: gmux_write8(dev, GMUX_PORT_DISCRETE_POWER, 0); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 144: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 149: if (dgpu) { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 150: gmux_write8(dev, GMUX_PORT_SWITCH_DDC, 2); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 150: gmux_write8(dev, GMUX_PORT_SWITCH_DDC, 2); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 151: gmux_write8(dev, GMUX_PORT_SWITCH_DISPLAY, 3); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 151: gmux_write8(dev, GMUX_PORT_SWITCH_DISPLAY, 3); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 152: } else { please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 153: gmux_write8(dev, GMUX_PORT_SWITCH_DDC, 1); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 153: gmux_write8(dev, GMUX_PORT_SWITCH_DDC, 1); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 154: gmux_write8(dev, GMUX_PORT_SWITCH_DISPLAY, 2); code indent should use tabs where possible https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 154: gmux_write8(dev, GMUX_PORT_SWITCH_DISPLAY, 2); please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/gm... PS1, Line 155: } please, no spaces at the start of a line https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/hy... PS1, Line 27: printk(BIOS_INFO, "Hybrid graphics enable_dev\n"); Prefer using '"%s...", __func__' to using 'enable_dev', this function's name, in a string https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/hy... PS1, Line 48: brightness = gmux_index_read32(GMUX_PORT_BRIGHTNESS) & GMUX_BRIGHTNESS_MASK; line over 80 characters https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/1/src/drivers/apple/hybrid_graphics/ro... PS1, Line 32: printk(BIOS_INFO, "Hybrid graphics early_hybrid_graphics\n"); Prefer using '"%s...", __func__' to using 'early_hybrid_graphics', this function's name, in a string -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 1 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-CC: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 09 May 2019 23:06:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#2). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 417 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 2 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 2: (9 comments) https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... PS2, Line 108: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; line over 80 characters https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... PS2, Line 109: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... PS2, Line 118: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; line over 80 characters https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... PS2, Line 119: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... PS2, Line 128: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; line over 80 characters https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/gm... PS2, Line 129: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/hy... PS2, Line 27: printk(BIOS_INFO, "Hybrid graphics enable_dev\n"); Prefer using '"%s...", __func__' to using 'enable_dev', this function's name, in a string https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/hy... PS2, Line 48: brightness = gmux_index_read32(GMUX_PORT_BRIGHTNESS) & GMUX_BRIGHTNESS_MASK; line over 80 characters https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/2/src/drivers/apple/hybrid_graphics/ro... PS2, Line 32: printk(BIOS_INFO, "Hybrid graphics early_hybrid_graphics\n"); Prefer using '"%s...", __func__' to using 'early_hybrid_graphics', this function's name, in a string -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 2 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 09 May 2019 23:11:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#3). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 413 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 3 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 3: (7 comments) https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... PS3, Line 108: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; line over 80 characters https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... PS3, Line 109: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... PS3, Line 118: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; line over 80 characters https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... PS3, Line 119: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... PS3, Line 128: const struct drivers_apple_hybrid_graphics_config *config = dev->chip_info; line over 80 characters https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/gm... PS3, Line 129: if (config->gmux_indexed) { braces {} are not necessary for any arm of this statement https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/3/src/drivers/apple/hybrid_graphics/hy... PS3, Line 46: brightness = gmux_index_read32(GMUX_PORT_BRIGHTNESS) & GMUX_BRIGHTNESS_MASK; line over 80 characters -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 3 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 09 May 2019 23:11:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#4). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 413 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 4 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/#/c/32719/4/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/4/src/drivers/apple/hybrid_graphics/hy... PS4, Line 46: brightness = gmux_index_read32(GMUX_PORT_BRIGHTNESS) & GMUX_BRIGHTNESS_MASK; line over 80 characters -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 4 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 09 May 2019 23:13:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#5). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 412 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 5 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#6). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 412 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 6 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#7). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 412 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 7 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 7: (2 comments) https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... PS7, Line 35: config = dev->chip_info; config isn't used https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/ro... PS7, Line 42: config = dev->chip_info; config isn't used -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 7 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 10 May 2019 07:30:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#8). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver Hybrid graphics driver for Apple MacBook Pro. Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 408 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 8: (2 comments) https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... PS7, Line 35: config = dev->chip_info;
config isn't used but it is used just the line below
https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/ro... PS7, Line 42: config = dev->chip_info;
config isn't used Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 10 May 2019 12:03:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/#/c/32719/8//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/32719/8//COMMIT_MSG@9 PS8, Line 9: Hybrid graphics driver for Apple MacBook Pro. Please extend the commit message a little bit. -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 10 May 2019 12:11:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 8: (2 comments) https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... PS7, Line 35: config = dev->chip_info;
but it is used just the line below just check for dev->chip_info directly
https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... PS7, Line 37: printk(BIOS_INFO, "Hybrid graphics: Not installed\n"); that should probably BIOS_ERR, as this is a miss configuration -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 10 May 2019 12:15:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evgeny Zinoviev <me@ch1p.com> Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Gerrit-MessageType: comment
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 8: (2 comments) https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... PS7, Line 35: config = dev->chip_info;
just check for dev->chip_info directly ok
https://review.coreboot.org/#/c/32719/7/src/drivers/apple/hybrid_graphics/hy... PS7, Line 37: printk(BIOS_INFO, "Hybrid graphics: Not installed\n");
that should probably BIOS_ERR, as this is a miss configuration agreed. but it is BIOS_DEBUG in lenovo/hybrid_graphics in the same place, I was a bit surprised and made it BIOS_INFO :)
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 10 May 2019 12:18:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evgeny Zinoviev <me@ch1p.com> Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 8: (14 comments) https://review.coreboot.org/#/c/32719/8//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/32719/8//COMMIT_MSG@9 PS8, Line 9: Hybrid graphics driver for Apple MacBook Pro. Would be a good place to give credit to the original author(s). https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... PS8, Line 24: #define HYBRID_GRAPHICS_DEFAULT_GPU HYBRID_GRAPHICS_INTEGRATED Make it a Kconfig? https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... PS8, Line 27: unsigned int `bool`? https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.h: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 43: #define GMUX_IOSTART 0x700 It would be wise to reserve this for the allocator. For this, you'd set up a `struct device_operations` in enable_dev(). The necessary operation would be `read_resources()` which can add a fixed resource. https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 45: int port Looking at the code it seems limited to 8 bits? So why isn't it a `u8`? https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 31: 0x01 It would be nice to give this a name. Looking at the code, it looks much like the `output buffer full` (OBF) of a classic keyboard controller. Whenever this bit is set in GMUX_PORT_WRITE, there is something to read in GMUX_PORT_READ. https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 32: inb(GMUX_IOSTART + GMUX_PORT_READ); Any chance you know what is read (and discarded) here? Some kind of status code, maybe? or is it just for synchronization? https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 34: udelay(100); I think this belongs before the line above it. Currently the sequence is <-- GMUX_PORT_WRITE <-- GMUX_PORT_READ <-- GMUX_PORT_WRITE delay <-- GMUX_PORT_READ <-- GMUX_PORT_WRITE delay ... <-- GMUX_PORT_READ <-- GMUX_PORT_WRITE delay Which seems odd at the start. https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 38: return !!i; Shouldn't this be `!(gwr & 0x01)`? i.e. what if the last read succeeded? https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 48: udelay(100); same as above https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 55: return !!i; same as above https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 68: outb((port & 0xff), GMUX_IOSTART + GMUX_PORT_READ); I am very curious if after this another bit in GMUX_PORT_WRITE is set; similar to IBF of a KBC. https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... PS8, Line 32: /* Don't confuse anyone else and disable the fake device */ Why? Who would get confused? https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... PS8, Line 30: /* TODO: Use generic device instead of dummy PNP device */ Why is a PNP dev used? -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sat, 11 May 2019 11:59:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... PS8, Line 30: /* TODO: Use generic device instead of dummy PNP device */
Why is a PNP dev used? The generic device couldn't be used at time of writing the lenovo hybrid graphics driver. I don't recall the actual problem. If you want to try it I can fix sconfig tool to support it.
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 8 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 12 May 2019 07:45:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#9). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver A hybrid graphics driver for Apple MacBook Pro. The driver logic is based on lenovo/hybrid_graphics. It is splitted into romstage and ramstage parts. The mainboard code calls the driver from romstage to get the GPU state. The driver reads the state from the `hybrid_grapihcs_mode` nvram option, switches dGPU power on or off according to the state and returns the state to the mainboard code. The mainboard code then has to hide the disabled PCI device. The ramstage part handles the graphics muxes. The muxes code is based on the apple-gmux linux driver, originally written by: * Canonical Ltd. <seth.forshee@canonical.com> * Andreas Heider, 2010-2012 <andreas@meetr.de> * Lukas Wunner, 2015 <lukas@wunner.de> Tested on MacBook Pro Retina 15 Mid 2012 (MacBook Pro 10,1). Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 8 files changed, 407 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 9 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 9: (5 comments) https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ch... PS8, Line 27: unsigned int
`bool`? Done
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.h: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 45: int port
Looking at the code it seems limited to 8 bits? So why isn't it a `u8`? you're right.
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/gm... PS8, Line 34: udelay(100);
I think this belongs before the line above it. Currently the […] Well, what you're saying seems logical, but this code is from apple-gmux linux driver. Maybe it's just a mistake, maybe not. I will eventually test all this more deeply. But for now I can't answer to this and some other your comments
https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/hy... PS8, Line 32: /* Don't confuse anyone else and disable the fake device */
Why? Who would get confused? This is a copy-pasta from lenovo/hybrid_graphics driver, siro should know :)
But my guess is that devices might be enumarated somewhere else in the code and this fake device should be marked as disabled because this is not a real device https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/#/c/32719/8/src/drivers/apple/hybrid_graphics/ro... PS8, Line 30: /* TODO: Use generic device instead of dummy PNP device */
The generic device couldn't be used at time of writing the lenovo hybrid graphics driver. […] It's based on lenovo/hybrid_graphics. I can rewrite it the right way, though I don't know it, tell me please where I can see an example or something
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 9 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 26 May 2019 22:15:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 9: (2 comments) https://review.coreboot.org/#/c/32719/8//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/32719/8//COMMIT_MSG@9 PS8, Line 9: Hybrid graphics driver for Apple MacBook Pro.
Please extend the commit message a little bit. Done
https://review.coreboot.org/#/c/32719/8//COMMIT_MSG@9 PS8, Line 9: Hybrid graphics driver for Apple MacBook Pro.
Would be a good place to give credit to the original author(s). Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 9 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 26 May 2019 22:22:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#13). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver A hybrid graphics driver for Apple MacBook Pro. The driver logic is based on lenovo/hybrid_graphics. It is splitted into romstage and ramstage parts. The mainboard code calls the driver from romstage to get the GPU state. The driver reads the state from the `hybrid_grapihcs_mode` nvram option, switches dGPU power on or off according to the state and returns the state to the mainboard code. The mainboard code then has to hide the disabled PCI device. The ramstage part handles the graphics muxes. The muxes code is based on the apple-gmux linux driver, originally written by: * Canonical Ltd. <seth.forshee@canonical.com> * Andreas Heider, 2010-2012 <andreas@meetr.de> * Lukas Wunner, 2015 <lukas@wunner.de> Tested on MacBook Pro Retina 15 Mid 2012 (MacBook Pro 10,1). Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/acpi/gmux.asl A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 9 files changed, 459 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/13 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 13 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 14: (9 comments) Sorry, I completely forgot about this review. If it ever happens again, feel free to ping me on irc. https://review.coreboot.org/c/coreboot/+/32719/14//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/32719/14//COMMIT_MSG@11 PS14, Line 11: splitted split https://review.coreboot.org/c/coreboot/+/32719/14/src/drivers/apple/hybrid_g... File src/drivers/apple/hybrid_graphics/acpi/gmux.asl: PS14: This file is very confusing. What does it do? https://review.coreboot.org/c/coreboot/+/32719/14/src/drivers/apple/hybrid_g... PS14, Line 30: Name (_PRW, Package (0x02) { 0x16, 0x03 }) Needs `gpi6_routing` in devicetree? NB. I don't quite understand why this would generate a wake event? https://review.coreboot.org/c/coreboot/+/32719/14/src/drivers/apple/hybrid_g... PS14, Line 33: Method (_L16, 0, NotSerialized) see above https://review.coreboot.org/c/coreboot/+/32719/14/src/drivers/apple/hybrid_g... PS14, Line 44: \GP06 |= Arg0 `|= 0` seems like a no-op??? Also, I'm confused. Is it an input or output? Should it be switched to input maybe during sleep? https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... File src/drivers/apple/hybrid_graphics/gmux.h: https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... PS8, Line 45: int port
you're right. Ack
https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... File src/drivers/apple/hybrid_graphics/gmux.c: https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... PS8, Line 34: udelay(100);
Well, what you're saying seems logical, but this code is from apple-gmux linux driver. […] Understood.
https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... PS8, Line 38: return !!i;
Shouldn't this be `!(gwr & 0x01)`? i.e. […] This however looks like a bug in the code flow and is not related to the hardware interface.
https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... File src/drivers/apple/hybrid_graphics/romstage.c: https://review.coreboot.org/c/coreboot/+/32719/8/src/drivers/apple/hybrid_gr... PS8, Line 30: /* TODO: Use generic device instead of dummy PNP device */
It's based on lenovo/hybrid_graphics. […] I would simply try `device generic` in the dt and find_dev_path() here. Maybe I think it's too easy, no idea what could go wrong.
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 14 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sat, 06 Jul 2019 22:09:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evgeny Zinoviev <me@ch1p.io> Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#18). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver A hybrid graphics driver for Apple MacBook Pro. The driver logic is based on lenovo/hybrid_graphics. It is splitted into romstage and ramstage parts. The mainboard code calls the driver from romstage to get the GPU state. The driver reads the state from the `hybrid_grapihcs_mode` nvram option, switches dGPU power on or off according to the state and returns the state to the mainboard code. The mainboard code then has to hide the disabled PCI device. The ramstage part handles the graphics muxes. The muxes code is based on the apple-gmux linux driver, originally written by: * Canonical Ltd. <seth.forshee@canonical.com> * Andreas Heider, 2010-2012 <andreas@meetr.de> * Lukas Wunner, 2015 <lukas@wunner.de> Tested on MacBook Pro Retina 15 Mid 2012 (MacBook Pro 10,1). Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/acpi/gmux.asl A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 9 files changed, 348 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/18 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 18 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Attention is currently required from: Evgeny Zinoviev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 20: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/32719/comment/f768c93f_4a6f2b03 PS20, Line 15: ih hi File src/drivers/apple/hybrid_graphics/acpi/gmux.asl: https://review.coreboot.org/c/coreboot/+/32719/comment/7e7d2e65_b882c41f PS20, Line 12: 0x07FF, // Range Maximum Is the allocator aware of this fixed I/O resource? File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/c/coreboot/+/32719/comment/20d31180_a5d9fbb6 PS20, Line 14: gmux_indexed I'd make this a Kconfig option. This way, you don't need to access the devicetree to read/write gmux registers. In fact, it makes the code much simpler, since you no longer need to pass around any `struct device`. -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 20 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Evgeny Zinoviev <me@ch1p.io> Gerrit-Comment-Date: Wed, 03 Feb 2021 19:48:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 23: (1 comment) File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/c/coreboot/+/32719/comment/bb8d5b38_3eb0f7de PS23, Line 42: get_option Needs update to use type safe option API -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 23 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Comment-Date: Mon, 18 Apr 2022 02:57:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 24: Verified+1 (1 comment) Commit Message: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-168281): https://review.coreboot.org/c/coreboot/+/32719/comment/c41c8d40_89b9a802 PS24, Line 10: 'splitted' may be misspelled - perhaps 'split'? -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 24 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Comment-Date: Sat, 14 Jan 2023 21:20:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Attention is currently required from: Evgeny Zinoviev. Hello build bot (Jenkins), Martin L Roth, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/32719 to look at the new patch set (#25). Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver A hybrid graphics driver for Apple MacBook Pro. The driver logic is based on lenovo/hybrid_graphics. It is splitted into romstage and ramstage parts. The mainboard code calls the driver from romstage to get the GPU state. The driver reads the state from the `hybrid_grapihcs_mode` nvram option, switches dGPU power on or off according to the state and returns the state to the mainboard code. The mainboard code then has to hide the disabled PCI device. The ramstage part handles the graphics muxes. The muxes code is based on the apple-gmux linux driver, originally written by: * Canonical Ltd. <seth.forshee@canonical.com> * Andreas Heider, 2010-2012 <andreas@meetr.de> * Lukas Wunner, 2015 <lukas@wunner.de> Tested on MacBook Pro Retina 15 Mid 2012 (MacBook Pro 10,1). Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.inc A src/drivers/apple/hybrid_graphics/acpi/gmux.asl A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 9 files changed, 376 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/25 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 25 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Evgeny Zinoviev <me@ch1p.io> Gerrit-MessageType: newpatchset
Attention is currently required from: Evgeny Zinoviev. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 25: (1 comment) Commit Message: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-168286): https://review.coreboot.org/c/coreboot/+/32719/comment/c0d8e194_44e325fc PS25, Line 10: 'splitted' may be misspelled - perhaps 'split'? -- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 25 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Evgeny Zinoviev <me@ch1p.io> Gerrit-Comment-Date: Sat, 14 Jan 2023 22:52:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Martin L Roth, Angel Pons, Nicholas Chin, Patrick Rudolph. Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32719 ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... Patch Set 28: (10 comments) This change is ready for review. Commit Message: https://review.coreboot.org/c/coreboot/+/32719/comment/c18e2e94_acd264e5 PS14, Line 11: splitted
split Done
Commit Message: https://review.coreboot.org/c/coreboot/+/32719/comment/b608e273_bcbdfad5 PS20, Line 15: ih
hi Done
File src/drivers/apple/hybrid_graphics/acpi/gmux.asl: https://review.coreboot.org/c/coreboot/+/32719/comment/4615384c_fd91fb63 PS20, Line 12: 0x07FF, // Range Maximum
Is the allocator aware of this fixed I/O resource? Please check out hybrid_graphics.c. I'm not sure I'm doing it right.
File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/c/coreboot/+/32719/comment/9a117fe6_6b56de5e PS20, Line 14: gmux_indexed
I'd make this a Kconfig option. […] Will do in next iterations.
File src/drivers/apple/hybrid_graphics/chip.h: https://review.coreboot.org/c/coreboot/+/32719/comment/d6cb4a8b_e0756e04 PS8, Line 24: #define HYBRID_GRAPHICS_DEFAULT_GPU HYBRID_GRAPHICS_INTEGRATED
Make it a Kconfig? Will do in next iterations.
File src/drivers/apple/hybrid_graphics/gmux.h: https://review.coreboot.org/c/coreboot/+/32719/comment/49f45f88_9d294f9a PS8, Line 43: #define GMUX_IOSTART 0x700
It would be wise to reserve this for the allocator. For this, […] Please check out hybrid_graphics.c. I'm not sure I'm doing it right.
File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/c/coreboot/+/32719/comment/52f0218b_4a4c8725 PS7, Line 35: config = dev->chip_info;
ok Done
https://review.coreboot.org/c/coreboot/+/32719/comment/d1be7987_3a3d55ca PS7, Line 37: printk(BIOS_INFO, "Hybrid graphics: Not installed\n");
agreed. […] Done
File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/c/coreboot/+/32719/comment/d24c23e6_17b9c87b PS8, Line 32: /* Don't confuse anyone else and disable the fake device */
This is a copy-pasta from lenovo/hybrid_graphics driver, siro should know :) […] Done
File src/drivers/apple/hybrid_graphics/hybrid_graphics.c: https://review.coreboot.org/c/coreboot/+/32719/comment/5cee364e_f37a7862 PS23, Line 42: get_option
Needs update to use type safe option API Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/32719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 28 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Nico Huber <nico.h@gmx.de> Gerrit-Attention: Martin L Roth <gaumless@gmail.com> Gerrit-Attention: Angel Pons <th3fanbus@gmail.com> Gerrit-Attention: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-Attention: Patrick Rudolph <siro@das-labor.org> Gerrit-Comment-Date: Sun, 15 Jan 2023 02:02:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Evgeny Zinoviev <me@ch1p.io> Comment-In-Reply-To: Nicholas Chin <nic.c3.14@gmail.com> Comment-In-Reply-To: Patrick Rudolph <siro@das-labor.org> Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Evgeny Zinoviev, Martin L Roth, Nicholas Chin, Nico Huber, Patrick Rudolph. Felix Singer has uploaded a new patch set (#30) to the change originally created by Evgeny Zinoviev. ( https://review.coreboot.org/c/coreboot/+/32719?usp=email ) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver A hybrid graphics driver for Apple MacBook Pro. The driver logic is based on lenovo/hybrid_graphics. It is split into romstage and ramstage parts. The mainboard code calls the driver from romstage to get the GPU state. The driver reads the state from the `hybrid_graphics_mode` nvram option, switches dGPU power on or off according to the state and returns the state to the mainboard code. The mainboard code then has to hide the disabled PCI device. The ramstage part handles the graphics muxes. The muxes code is based on the apple-gmux linux driver, originally written by: * Canonical Ltd. <seth.forshee@canonical.com> * Andreas Heider, 2010-2012 <andreas@meetr.de> * Lukas Wunner, 2015 <lukas@wunner.de> Tested on MacBook Pro Retina 15 Mid 2012 (MacBook Pro 10,1). Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.mk A src/drivers/apple/hybrid_graphics/acpi/gmux.asl A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 9 files changed, 356 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/30 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 30 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Patrick Rudolph <rudolphpatrick05@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Nico Huber <nico.h@gmx.de> Gerrit-Attention: Martin L Roth <gaumless@gmail.com> Gerrit-Attention: Angel Pons <th3fanbus@gmail.com> Gerrit-Attention: Evgeny Zinoviev <me@ch1p.io> Gerrit-Attention: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05@gmail.com>
Attention is currently required from: Angel Pons, Evgeny Zinoviev, Martin L Roth, Nicholas Chin, Nico Huber, Patrick Rudolph. Felix Singer has uploaded a new patch set (#31) to the change originally created by Evgeny Zinoviev. ( https://review.coreboot.org/c/coreboot/+/32719?usp=email ) The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: drivers/apple: Add hybrid graphics driver ...................................................................... drivers/apple: Add hybrid graphics driver A hybrid graphics driver for Apple MacBook Pro. The driver logic is based on lenovo/hybrid_graphics. It is split into romstage and ramstage parts. The mainboard code calls the driver from romstage to get the GPU state. The driver reads the state from the `hybrid_graphics_mode` nvram option, switches dGPU power on or off according to the state and returns the state to the mainboard code. The mainboard code then has to hide the disabled PCI device. The ramstage part handles the graphics muxes. The muxes code is based on the apple-gmux linux driver, originally written by: * Canonical Ltd. <seth.forshee@canonical.com> * Andreas Heider, 2010-2012 <andreas@meetr.de> * Lukas Wunner, 2015 <lukas@wunner.de> Tested on MacBook Pro Retina 15 Mid 2012 (MacBook Pro 10,1). Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Signed-off-by: Evgeny Zinoviev <me@ch1p.io> --- A src/drivers/apple/hybrid_graphics/Kconfig A src/drivers/apple/hybrid_graphics/Makefile.mk A src/drivers/apple/hybrid_graphics/acpi/gmux.asl A src/drivers/apple/hybrid_graphics/chip.h A src/drivers/apple/hybrid_graphics/gmux.c A src/drivers/apple/hybrid_graphics/gmux.h A src/drivers/apple/hybrid_graphics/hybrid_graphics.c A src/drivers/apple/hybrid_graphics/hybrid_graphics.h A src/drivers/apple/hybrid_graphics/romstage.c 9 files changed, 356 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/32719/31 -- To view, visit https://review.coreboot.org/c/coreboot/+/32719?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I22b66622cd2da0e9951ee726d650d204fbb8a5bc Gerrit-Change-Number: 32719 Gerrit-PatchSet: 31 Gerrit-Owner: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Evgeny Zinoviev <me@ch1p.io> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Patrick Rudolph <rudolphpatrick05@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Nico Huber <nico.h@gmx.de> Gerrit-Attention: Martin L Roth <gaumless@gmail.com> Gerrit-Attention: Angel Pons <th3fanbus@gmail.com> Gerrit-Attention: Evgeny Zinoviev <me@ch1p.io> Gerrit-Attention: Nicholas Chin <nic.c3.14@gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05@gmail.com>
participants (8)
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Evgeny Zinoviev (Code Review)
-
Felix Singer (Code Review)
-
Nicholas Chin (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)