Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
nb/intel/sandybridge/gma.c: Remove useless if condition
There's a useless check with both branches doing the same: enabling RC6 and disabling RC6p. In past, this condition would enable RC6p in IVB but not on SNB. Then, at some point, RC6p was considered unstable and was disabled, but the condition remained.
It's not needed so let's remove it.
Change-Id: I926bb682d1b9d21185048224490b966c33204b6a Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/gma.c 1 file changed, 8 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/42410/1
diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index 83a0279..8117c62 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -453,18 +453,14 @@ gtt_write(0xa06c, 0x000493e0); /* RP Down EI */ gtt_write(0xa070, 0x0000000a); /* RP Idle Hysteresis */
- /* 11a: Enable Render Standby (RC6) */ - if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB) { - /* - * 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 */ - } else { - gtt_write(0xa090, 0x88040000); /* HW RC Control */ - } + /* 11a: 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 */
/* 12: Normal Frequency Request */ /* RPNFREQ_VAL comes from MCHBAR 0x5998 23:16 */
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 456: /* 11a: Enable Render Standby (RC6) Multi-line comments start like this:
/* * 11a: Enable ...
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42410
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
nb/intel/sandybridge/gma.c: Remove useless if condition
There's a useless check with both branches doing the same: enabling RC6 and disabling RC6p. In past, this condition would enable RC6p in IVB but not on SNB. Then, at some point, RC6p was considered unstable and was disabled, but the condition remained.
It's not needed so let's remove it.
Change-Id: I926bb682d1b9d21185048224490b966c33204b6a Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/gma.c 1 file changed, 9 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/42410/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 2:
(6 comments)
Some things that seem to be wrong
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 376: gtt_write(0xa004, 0x00000010); lockbit
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 382: gtt_write(0xa080, 0x00000004); lockbit
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 386: (1 << 31) lockbit
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 479: lockbits are set here:
gtt_write(0xa248, gtt_read(0xa248) | (1 << 31)); gtt_write(0xa004, gtt_read(0xa004) | (1 << 4)); gtt_write(0xa080, gtt_read(0xa080) | (1 << 2)); gtt_write(0xa180, gtt_read(0xa180) | (1 << 31));
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 510: gtt_write(0xa188, 0x1fffe); this should be ANDing the register with 0x0001ffff instead
bit0 is cleared in a separate write
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 515: /* 16: SW RC Control */ RC6 deepest
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 456: /* 11a: Enable Render Standby (RC6)
Multi-line comments start like this: […]
Done
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 510: gtt_write(0xa188, 0x1fffe);
this should be ANDing the register with 0x0001ffff instead […]
not sure why there should be a separate write? there are no polls between them. can't we just do something like this?
reg32 = gtt_read(0xa188); reg32 &= 0x1ffff; reg32 &= ~1; gtt_write(0xa188, reg32);
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42410/1/src/northbridge/intel/sandy... PS1, Line 510: gtt_write(0xa188, 0x1fffe);
not sure why there should be a separate write? there are no polls between them. […]
The intermediate value of a register may be required to properly transition from one state to another. I've seen that in raminit code when disabling special modes.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42410 )
Change subject: nb/intel/sandybridge/gma.c: Remove useless if condition ......................................................................
nb/intel/sandybridge/gma.c: Remove useless if condition
There's a useless check with both branches doing the same: enabling RC6 and disabling RC6p. In past, this condition would enable RC6p in IVB but not on SNB. Then, at some point, RC6p was considered unstable and was disabled, but the condition remained.
It's not needed so let's remove it.
Change-Id: I926bb682d1b9d21185048224490b966c33204b6a Signed-off-by: Evgeny Zinoviev me@ch1p.io Reviewed-on: https://review.coreboot.org/c/coreboot/+/42410 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/gma.c 1 file changed, 9 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index 83a0279..fee7513 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -453,18 +453,15 @@ gtt_write(0xa06c, 0x000493e0); /* RP Down EI */ gtt_write(0xa070, 0x0000000a); /* RP Idle Hysteresis */
- /* 11a: Enable Render Standby (RC6) */ - if ((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_IVB) { - /* - * 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 */ - } else { - gtt_write(0xa090, 0x88040000); /* HW RC Control */ - } + /* + * 11a: 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 */
/* 12: Normal Frequency Request */ /* RPNFREQ_VAL comes from MCHBAR 0x5998 23:16 */
-
Angel Pons (Code Review) -
Evgeny Zinoviev (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)