Change in coreboot[master]: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p
Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42450 )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p
Add Kconfig options to enable or disable RC6 and RC6p.
TODO
- Not sure if it's OK to allow to enable RC6p alone without RC6p. If yes then I'll update the patch.
Change-Id: I6166d04b3bcb7a55f1d03c397d87eaa62c64b48b Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/sandybridge/gma.c 2 files changed, 26 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/42450/1
diff --git a/src/northbridge/intel/sandybridge/Kconfig b/src/northbridge/intel/sandybridge/Kconfig index ef6dc3d..95f1deb 100644 --- a/src/northbridge/intel/sandybridge/Kconfig +++ b/src/northbridge/intel/sandybridge/Kconfig @@ -129,4 +129,18 @@ config INTEL_GMA_BCLV_OFFSET default 0x48254
+config ENABLE_RC6 + bool "Enable RC6 (Render Standby)" + default y + help + Select this if you want to enable RC6 (Render Standby). + +config ENABLE_RC6P + depends on ENABLE_RC6 + bool "Enable RC6p (Deep Render Standby)" + default n + help + Select this if you want to enable RC6p (Deep Render Standby). + This should only be selected on Ivy Bridge. + endif diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index 8fe2de8..d976530 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -450,15 +450,15 @@ gtt_write(0xa06c, 0x000493e0); /* RP Down EI */ gtt_write(0xa070, 0x0000000a); /* RP Idle Hysteresis */
- /* - * 10a: Enable Render Standby (RC6) - * - * IvyBridge should also support DeepRenderStandby. - * - * Unfortunately it does not work reliably on all SKUs so - * disable it here and it can be enabled by the kernel. - */ - gtt_write(0xa090, 0x88040000); /* HW RC Control */ + /* 10a: Enable Render Standby (RC6) and Deep Render Standby (RC6p) */ + reg32 = 0; +#if CONFIG(ENABLE_RC6) + reg32 |= 0x88040000; +#if CONFIG(ENABLE_RC6P) + reg32 |= 0x00020000; +#endif +#endif + gtt_write(0xa090, reg32);
/* 11: Normal Frequency Request */ /* RPNFREQ_VAL comes from MCHBAR 0x5998 23:16 */ @@ -516,8 +516,10 @@ gtt_write(0xa188, gtt_read(0xa188) | 1); }
- /* 16: SW RC Control */ + /* 16: SW RC state: RC6 deepest */ +#if CONFIG(ENABLE_RC6) || CONFIG(ENABLE_RC6P) gtt_write(0xa094, 0x00060000); +#endif
/* Setup Digital Port Hotplug */ reg32 = gtt_read(0xc4030);
Hello Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42450
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p
Add Kconfig options to enable or disable RC6 and RC6p.
TODO
- Not sure if it's OK to allow to enable RC6p alone without RC6. If yes then I'll update the patch.
Change-Id: I6166d04b3bcb7a55f1d03c397d87eaa62c64b48b Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/sandybridge/gma.c 2 files changed, 26 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/42450/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42450 )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
Patch Set 2: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 132: ENABLE_RC6 Make it clear that it's for the iGPU?
GMA_ENABLE_RC6
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 138: ENABLE_RC6P GMA_ENABLE_RC6P
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 144: This should only be selected on Ivy Bridge. We can check that it's Ivy Bridge at runtime, see comments on the code. Maybe say:
Select this if you want to enable RC6p (Deep Render Standby) for Ivy Bridge.
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 455: #if CONFIG(ENABLE_RC6) Why not make this code flexible enough so that RC6p can be enabled correctly without RC6? It's just separating the checks and changing a mask. We can control the dependency in Kconfig. Also, there's no need for preprocessor (compiler will optimize the checks):
reg32 = 0;
if (CONFIG(GMA_ENABLE_RC6)) reg32 |= 0x88040000;
if (CONFIG(GMA_ENABLE_RC6P) && (bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB) reg32 |= 0x88020000;
gtt_write(0xa090, 0x88040000); /* HW RC Control */
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 461: gtt_write(0xa090, reg32); Where did the comment go?
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 520: #if CONFIG(ENABLE_RC6) || CONFIG(ENABLE_RC6P) How about:
if (CONFIG(ENABLE_RC6) || (CONFIG(GMA_ENABLE_RC6P) && (bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB)) gtt_write(0xa094, 0x00060000);
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42450
to look at the new patch set (#3).
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p
Add Kconfig options to enable or disable RC6 and RC6p.
Change-Id: I6166d04b3bcb7a55f1d03c397d87eaa62c64b48b Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/sandybridge/gma.c 2 files changed, 26 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/42450/3
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42450 )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 132: ENABLE_RC6
Make it clear that it's for the iGPU? […]
Done
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 138: ENABLE_RC6P
GMA_ENABLE_RC6P
Done
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 144: This should only be selected on Ivy Bridge.
We can check that it's Ivy Bridge at runtime, see comments on the code. Maybe say: […]
Done
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 455: #if CONFIG(ENABLE_RC6)
Why not make this code flexible enough so that RC6p can be enabled correctly without RC6? It's just […]
Okay, done.
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 461: gtt_write(0xa090, reg32);
Where did the comment go?
It was lost :)
https://review.coreboot.org/c/coreboot/+/42450/2/src/northbridge/intel/sandy... PS2, Line 520: #if CONFIG(ENABLE_RC6) || CONFIG(ENABLE_RC6P)
How about: […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42450 )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42450/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42450/3/src/northbridge/intel/sandy... PS3, Line 144: Bridge. Could you please elaborate in the helpdesk, why it’s not enabled by default.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42450 )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42450/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42450/3/src/northbridge/intel/sandy... PS3, Line 144: Bridge.
Could you please elaborate in the helpdesk, why it’s not enabled by default.
What's a helpdesk? Do you mean expanding this message so that people know what to select here?
FWIW, the OS enables RC6p if it wants to anyway.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42450?usp=email )
Change subject: nb/intel/sandybridge/gma.c: Add Kconfig options for RC6 and RC6p ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Evgeny Zinoviev (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)