Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
soc/intel/cannonlake: Add i915_gpu_controller_info
This adds the i915_gpu_controller_info struct to chip.h and implements intel_igd_get_controller_info. Due to conflicts with the GMA driver ACPI, gfx.asl had to be commented out. I believe gfx.asl should be removed, but would like to hear alternatives.
Tested on system76/lemp9 - ACPI backlight control was verified.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I027b5fc37527fbfcf985262c8a1a048e0363410e --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/acpi/southbridge.asl M src/soc/intel/cannonlake/chip.h A src/soc/intel/cannonlake/graphics.c 4 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/43616/1
diff --git a/src/soc/intel/cannonlake/Makefile.inc b/src/soc/intel/cannonlake/Makefile.inc index 96f1f97..e6d330a 100644 --- a/src/soc/intel/cannonlake/Makefile.inc +++ b/src/soc/intel/cannonlake/Makefile.inc @@ -38,6 +38,7 @@ ramstage-y += fsp_params.c ramstage-y += gspi.c ramstage-y += i2c.c +ramstage-y += graphics.c ramstage-y += lockdown.c ramstage-y += lpc.c ramstage-y += me.c diff --git a/src/soc/intel/cannonlake/acpi/southbridge.asl b/src/soc/intel/cannonlake/acpi/southbridge.asl index 12269d3..9cdd3b5 100644 --- a/src/soc/intel/cannonlake/acpi/southbridge.asl +++ b/src/soc/intel/cannonlake/acpi/southbridge.asl @@ -17,7 +17,7 @@ #endif
/* GFX 00:02.0 */ -#include "gfx.asl" +//TODO: fix inclusion when using gma ACPI #include "gfx.asl"
/* LPC 0:1f.0 */ #include <soc/intel/common/block/acpi/acpi/lpc.asl> diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 2923efc..3457f4d 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -5,6 +5,7 @@
#include <intelblocks/cfg.h> #include <drivers/i2c/designware/dw_i2c.h> +#include <drivers/intel/gma/gma.h> #include <intelblocks/gpio.h> #include <intelblocks/gspi.h> #include <intelblocks/lpc_lib.h> @@ -478,6 +479,9 @@ * Only override CPU flex ratio if don't want to boot with non-turbo max. */ uint8_t cpu_ratio_override; + + /* i915 struct for GMA backlight control */ + struct i915_gpu_controller_info gfx; };
typedef struct soc_intel_cannonlake_config config_t; diff --git a/src/soc/intel/cannonlake/graphics.c b/src/soc/intel/cannonlake/graphics.c new file mode 100644 index 0000000..fda4998 --- /dev/null +++ b/src/soc/intel/cannonlake/graphics.c @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <intelblocks/graphics.h> +#include <soc/ramstage.h> + +const struct i915_gpu_controller_info * +intel_igd_get_controller_info(const struct device *device) +{ + struct soc_intel_cannonlake_config *chip = device->chip_info; + return &chip->gfx; +}
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43616/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43616/1//COMMIT_MSG@11 PS1, Line 11: I believe : gfx.asl should be removed, but would like to hear alternatives. It seems empty (the _ADR only has meaning if there is something more associated with the device node). I would remove it.
https://review.coreboot.org/c/coreboot/+/43616/1//COMMIT_MSG@14 PS1, Line 14: Tested on system76/lemp9 - ACPI backlight control was verified. Alas, there are many paths to test. The most common one with modern Linux distributions is that ACPI only receives an event and then calls the i915 driver via IGD OpRegion to actually change the brightness. If there is no graphics driver loaded (yet), the ASL code writes registers directly and these registers have to be configured as they differ from chip to chip, see `INTEL_GMA_BCL*` in `src/drivers/intel/gma/Kconfig`.
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... PS1, Line 483: backlight control It's mostly about the available display ports and only indirectly related to backlight controls (i.e. when an internal panel is advertised to the OS).
Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... PS1, Line 483: backlight control
It's mostly about the available display ports and only indirectly related to […]
Should the comment just be dropped? Skylake is the only other chip that documents this field (with this comment).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... PS1, Line 483: backlight control
Should the comment just be dropped? Skylake is the only other chip that documents this field (with t […]
A misleading comment is worse than no comment at all.
Tim Crawford has uploaded a new patch set (#2) to the change originally created by Jeremy Soller. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
soc/intel/cannonlake: Add i915_gpu_controller_info
This adds the i915_gpu_controller_info struct to chip.h and implements intel_igd_get_controller_info. Due to conflicts with the GMA driver ACPI, gfx.asl had to be commented out. I believe gfx.asl should be removed, but would like to hear alternatives.
Tested on system76/lemp9 - ACPI backlight control was verified.
Change-Id: I027b5fc37527fbfcf985262c8a1a048e0363410e Signed-off-by: Jeremy Soller jeremy@system76.com Signed-off-by: Tim Crawford tcrawford@system76.com --- M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/acpi/southbridge.asl M src/soc/intel/cannonlake/chip.h A src/soc/intel/cannonlake/graphics.c 4 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/43616/2
Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... PS1, Line 483: backlight control
A misleading comment is worse than no comment at all.
Comment dropped.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 2:
(1 comment)
Please also check the *Resolved* box.
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/43616/1/src/soc/intel/cannonlake/ch... PS1, Line 483: backlight control
Comment dropped.
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43616/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43616/1//COMMIT_MSG@11 PS1, Line 11: I believe : gfx.asl should be removed, but would like to hear alternatives.
It seems empty (the _ADR only has meaning if there is something more associated with the device node […]
seems to be a stub device to make linux happy; see commit ad87b2a0391
Attention is currently required from: Jeremy Soller. Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2: Done in CB:48749 and CB:48862
Jeremy Soller has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43616 )
Change subject: soc/intel/cannonlake: Add i915_gpu_controller_info ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Jeremy Soller (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Tim Crawford (Code Review)