Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
soc/intel/broadwell: add ACPI backlight support
Add framework to generate ACPI methods in SSDT for screen backlight control. Adjust params for gtt_ methods to match prototypes in i915.h and avoid conflicts.
To make use of this, individual boards will need to include default_brightness_levels.asl in their dsdt, as well as add 'register "gfx" = "GMA_STATIC_DISPLAYS(0)"' to their devicetree.
Change-Id: If93b7690ef36b5d19ca43957e8a1bef91ec5821d Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/soc/intel/broadwell/chip.h M src/soc/intel/broadwell/igd.c 2 files changed, 29 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/39941/1
diff --git a/src/soc/intel/broadwell/chip.h b/src/soc/intel/broadwell/chip.h index fabb95a..5ad7086 100644 --- a/src/soc/intel/broadwell/chip.h +++ b/src/soc/intel/broadwell/chip.h @@ -12,6 +12,8 @@ * GNU General Public License for more details. */
+#include <drivers/intel/gma/i915.h> + #ifndef _SOC_INTEL_BROADWELL_CHIP_H_ #define _SOC_INTEL_BROADWELL_CHIP_H_
@@ -130,6 +132,8 @@ */ int cdclk;
+ struct i915_gpu_controller_info gfx; + /* Enable S0iX support */ int s0ix_enable;
diff --git a/src/soc/intel/broadwell/igd.c b/src/soc/intel/broadwell/igd.c index ecb5417..7769410 100644 --- a/src/soc/intel/broadwell/igd.c +++ b/src/soc/intel/broadwell/igd.c @@ -24,11 +24,13 @@ #include <string.h> #include <reg_script.h> #include <cbmem.h> +#include <drivers/intel/gma/i915.h> #include <drivers/intel/gma/i915_reg.h> #include <drivers/intel/gma/libgfxinit.h> #include <drivers/intel/gma/opregion.h> #include <soc/cpu.h> #include <soc/nvs.h> +#include <soc/pci_devs.h> #include <soc/pm.h> #include <soc/ramstage.h> #include <soc/systemagent.h> @@ -258,7 +260,7 @@
static struct resource *gtt_res = NULL;
-static unsigned long gtt_read(unsigned long reg) +u32 gtt_read(u32 reg) { u32 val; val = read32(res2mmio(gtt_res, reg, 0)); @@ -266,7 +268,7 @@
}
-static void gtt_write(unsigned long reg, unsigned long data) +void gtt_write(u32 reg, u32 data) { write32(res2mmio(gtt_res, reg, 0), data); } @@ -279,9 +281,8 @@ gtt_write(reg, val); }
-static int gtt_poll(u32 reg, u32 mask, u32 value) -{ - unsigned int try = GT_RETRY; +int gtt_poll(u32 reg, u32 mask, u32 value) +{ unsigned int try = GT_RETRY; u32 data;
while (try--) { @@ -627,11 +628,30 @@ return current; }
+const struct i915_gpu_controller_info * +intel_gma_get_controller_info(void) +{ + struct device *dev = pcidev_on_root(SA_DEV_SLOT_IGD, 0); + if (!dev || !dev->enabled) + return NULL; + + struct soc_intel_broadwell_config *chip = dev->chip_info; + return &chip->gfx; +} + +static void gma_ssdt(struct device *device) +{ + const struct i915_gpu_controller_info *gfx = intel_gma_get_controller_info(); + if (gfx) + drivers_intel_gma_displays_ssdt_generate(gfx); +} + static struct device_operations igd_ops = { .read_resources = &pci_dev_read_resources, .set_resources = &pci_dev_set_resources, .enable_resources = &pci_dev_enable_resources, .init = &igd_init, + .acpi_fill_ssdt_generator = gma_ssdt, .ops_pci = &broadwell_pci_ops, .write_acpi_tables = gma_write_acpi_tables, };
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG@11 PS1, Line 11: to match prototypes in i915.h and avoid conflicts. `drivers/intel/gma/intel_ddi.c` seems to be the only reason to export these functions... maybe we should just merge it into haswell/, its only user.
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... PS1, Line 640: } Not sure if we need a separate function?
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... PS1, Line 642: device Pass `device` on?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG@11 PS1, Line 11: to match prototypes in i915.h and avoid conflicts.
`drivers/intel/gma/intel_ddi.c` seems to be the only reason […]
will look into that and rebase if appropriate
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... PS1, Line 640: }
Not sure if we need a separate function?
had modeled after Haswell implementation, but you're right, we don't. will merge
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... PS1, Line 642: device
Pass `device` on?
had modeled after Haswell implementation vs Skylake. will fix here and BYT/BSW as well
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39941
to look at the new patch set (#2).
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
soc/intel/broadwell: add ACPI backlight support
Add framework to generate ACPI methods in SSDT for screen backlight control. Adjust params for gtt_ methods to match prototypes in i915.h and avoid conflicts.
To make use of this, individual boards will need to include default_brightness_levels.asl in their dsdt, as well as add 'register "gfx" = "GMA_STATIC_DISPLAYS(0)"' to their devicetree.
Change-Id: If93b7690ef36b5d19ca43957e8a1bef91ec5821d Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/soc/intel/broadwell/chip.h M src/soc/intel/broadwell/igd.c 2 files changed, 18 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/39941/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG@11 PS1, Line 11: to match prototypes in i915.h and avoid conflicts.
will look into that and rebase if appropriate
looks like ironlake, sandybridge, and haswell all implement gtt_{read,write,poll} in their gma.c, so adjusting Broadwell's signatures seems like the cleanest solution ATM
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... PS1, Line 640: }
had modeled after Haswell implementation, but you're right, we don't. […]
Done
https://review.coreboot.org/c/coreboot/+/39941/1/src/soc/intel/broadwell/igd... PS1, Line 642: device
had modeled after Haswell implementation vs Skylake. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG@11 PS1, Line 11: to match prototypes in i915.h and avoid conflicts.
looks like ironlake, sandybridge, and haswell all implement gtt_{read,write,poll} in their gma. […]
Well, if it's a clean solution to add to a pile depends on its looks. To me it looks like a pile of technical debt. And I fear if we export functions as an API, people might think that was done on purpose and use it (chances are low in this case, though).
It's not your fault, and I know you just wanted one prototype and ran into the `i915.h` mess. So I'll leave it be.
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 635: gfx This can't be NULL (unless `chip` is negative). You can read the
&chip->gfx
above as
(const i915_gpu_controller_info *)((const uint8_t *) chip + offsetof(struct soc_intel_broadwell_config, gfx))
So it's at least the offset of `gfx` inside the config struct.
But we should check `chip` for NULL, e.g.
const struct soc_intel_broadwell_config *chip = device->chip_info;
if (chip) drivers_intel_gma_display_ssdt_generate(&chip->gfx);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 644: .acpi_fill_ssdt_generator = gma_generate_ssdt, ouch
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 630: device Everything else uses "dev"
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39941
to look at the new patch set (#3).
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
soc/intel/broadwell: add ACPI backlight support
Add framework to generate ACPI methods in SSDT for screen backlight control. Adjust params for gtt_ methods to match prototypes in i915.h and avoid conflicts.
To make use of this, individual boards will need to include default_brightness_levels.asl in their dsdt, as well as add 'register "gfx" = "GMA_STATIC_DISPLAYS(0)"' to their devicetree.
Change-Id: If93b7690ef36b5d19ca43957e8a1bef91ec5821d Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/soc/intel/broadwell/chip.h M src/soc/intel/broadwell/igd.c 2 files changed, 23 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/39941/3
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39941/1//COMMIT_MSG@11 PS1, Line 11: to match prototypes in i915.h and avoid conflicts.
Well, if it's a clean solution to add to a pile depends on its looks. […]
Ack
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 635: gfx
This can't be NULL (unless `chip` is negative). You can read the […]
Done
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 644: .acpi_fill_ssdt_generator = gma_generate_ssdt,
ouch
Done
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39941
to look at the new patch set (#4).
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
soc/intel/broadwell: add ACPI backlight support
Add framework to generate ACPI methods in SSDT for screen backlight control. Adjust params for gtt_ methods to match prototypes in i915.h and avoid conflicts.
To make use of this, individual boards will need to include default_brightness_levels.asl in their dsdt, as well as add 'register "gfx" = "GMA_STATIC_DISPLAYS(0)"' to their devicetree.
Change-Id: If93b7690ef36b5d19ca43957e8a1bef91ec5821d Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/soc/intel/broadwell/chip.h M src/soc/intel/broadwell/igd.c 2 files changed, 16 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/39941/4
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... File src/soc/intel/broadwell/igd.c:
https://review.coreboot.org/c/coreboot/+/39941/2/src/soc/intel/broadwell/igd... PS2, Line 630: device
Everything else uses "dev"
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
Patch Set 4: Code-Review+2
Matt DeVillier has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39941 )
Change subject: soc/intel/broadwell: add ACPI backlight support ......................................................................
soc/intel/broadwell: add ACPI backlight support
Add framework to generate ACPI methods in SSDT for screen backlight control. Adjust params for gtt_ methods to match prototypes in i915.h and avoid conflicts.
To make use of this, individual boards will need to include default_brightness_levels.asl in their dsdt, as well as add 'register "gfx" = "GMA_STATIC_DISPLAYS(0)"' to their devicetree.
Change-Id: If93b7690ef36b5d19ca43957e8a1bef91ec5821d Signed-off-by: Matt DeVillier matt.devillier@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39941 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/broadwell/chip.h M src/soc/intel/broadwell/igd.c 2 files changed, 16 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/broadwell/chip.h b/src/soc/intel/broadwell/chip.h index fabb95a..b68da91 100644 --- a/src/soc/intel/broadwell/chip.h +++ b/src/soc/intel/broadwell/chip.h @@ -15,6 +15,7 @@ #ifndef _SOC_INTEL_BROADWELL_CHIP_H_ #define _SOC_INTEL_BROADWELL_CHIP_H_
+#include <drivers/intel/gma/i915.h> #include <stdint.h>
struct soc_intel_broadwell_config { @@ -130,6 +131,8 @@ */ int cdclk;
+ struct i915_gpu_controller_info gfx; + /* Enable S0iX support */ int s0ix_enable;
diff --git a/src/soc/intel/broadwell/igd.c b/src/soc/intel/broadwell/igd.c index ecb5417..77375e4 100644 --- a/src/soc/intel/broadwell/igd.c +++ b/src/soc/intel/broadwell/igd.c @@ -24,6 +24,7 @@ #include <string.h> #include <reg_script.h> #include <cbmem.h> +#include <drivers/intel/gma/i915.h> #include <drivers/intel/gma/i915_reg.h> #include <drivers/intel/gma/libgfxinit.h> #include <drivers/intel/gma/opregion.h> @@ -258,7 +259,7 @@
static struct resource *gtt_res = NULL;
-static unsigned long gtt_read(unsigned long reg) +u32 gtt_read(u32 reg) { u32 val; val = read32(res2mmio(gtt_res, reg, 0)); @@ -266,7 +267,7 @@
}
-static void gtt_write(unsigned long reg, unsigned long data) +void gtt_write(u32 reg, u32 data) { write32(res2mmio(gtt_res, reg, 0), data); } @@ -279,9 +280,8 @@ gtt_write(reg, val); }
-static int gtt_poll(u32 reg, u32 mask, u32 value) -{ - unsigned int try = GT_RETRY; +int gtt_poll(u32 reg, u32 mask, u32 value) +{ unsigned int try = GT_RETRY; u32 data;
while (try--) { @@ -627,6 +627,13 @@ return current; }
+static void gma_generate_ssdt(struct device *dev) +{ + const struct soc_intel_broadwell_config *chip = dev->chip_info; + + drivers_intel_gma_displays_ssdt_generate(&chip->gfx); +} + static struct device_operations igd_ops = { .read_resources = &pci_dev_read_resources, .set_resources = &pci_dev_set_resources, @@ -634,6 +641,7 @@ .init = &igd_init, .ops_pci = &broadwell_pci_ops, .write_acpi_tables = gma_write_acpi_tables, + .acpi_fill_ssdt = gma_generate_ssdt, };
static const unsigned short pci_device_ids[] = {
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)