Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
nb/intel/sandybridge/gma.c: Fix lock bits setting
PM lock bits are set too early at the moment. Set them after enabling PM interrupts.
Change-Id: I3a16e1ce1236597faad42f3812b2b8d2b5417efd Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/gma.c 1 file changed, 23 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/42447/1
diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index fee7513..8fe2de8 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -42,7 +42,7 @@ { 0xa23c, 0x0817fe19 }, { 0xa240, 0x00000000 }, { 0xa244, 0x00000000 }, - { 0xa248, 0x8000421e }, + { 0xa248, 0x0000421e }, { 0 }, };
@@ -65,7 +65,7 @@ { 0xa23c, 0x002a002b }, { 0xa240, 0x00000000 }, { 0xa244, 0x00000000 }, - { 0xa248, 0x8000421e }, + { 0xa248, 0x0000421e }, { 0 }, };
@@ -373,23 +373,20 @@ }
/* 3: Gear ratio map */ - gtt_write(0xa004, 0x00000010); + gtt_write(0xa004, 0x00000000);
/* 4: GFXPAUSE */ gtt_write(0xa000, 0x00070020);
- /* 5: Dynamic EU trip control */ - gtt_write(0xa080, 0x00000004); - - /* 6: ECO bits */ + /* 5: ECO bits */ reg32 = gtt_read(0xa180); - reg32 |= (1 << 26) | (1 << 31); + reg32 |= (1 << 26); /* (bit 20=1 for SNB step D1+ / IVB A0+) */ if (bridge_silicon_revision() >= SNB_STEP_D1) reg32 |= (1 << 20); gtt_write(0xa180, reg32);
- /* 6a: for SnB step D2+ only */ + /* 5a: for SnB step D2+ only */ if (((bridge_silicon_revision() & BASE_REV_MASK) == BASE_REV_SNB) && (bridge_silicon_revision() >= SNB_STEP_D2)) { reg32 = gtt_read(0x9400); @@ -408,14 +405,14 @@ reg32 |= (1 << 16); gtt_write(0x907c, reg32);
- /* 6b: Clocking reset controls */ + /* 5b: Clocking reset controls */ gtt_write(0x9424, 0x00000001); } else { - /* 6b: Clocking reset controls */ + /* 5b: Clocking reset controls */ gtt_write(0x9424, 0x00000000); }
- /* 7 */ + /* 6 */ if (gtt_poll(0x138124, (1 << 31), (0 << 31))) { gtt_write(0x138128, 0x00000029); /* Mailbox Data */ gtt_write(0x138124, 0x80000004); /* Mailbox Cmd for RC6 VID */ @@ -424,7 +421,7 @@ gtt_poll(0x138124, (1 << 31), (0 << 31)); }
- /* 8 */ + /* 7 */ gtt_write(0xa090, 0x00000000); /* RC Control */ gtt_write(0xa098, 0x03e80000); /* RC1e Wake Rate Limit */ gtt_write(0xa09c, 0x0028001e); /* RC6/6p Wake Rate Limit */ @@ -432,19 +429,19 @@ gtt_write(0xa0a8, 0x0001e848); /* RC Evaluation Interval */ gtt_write(0xa0ac, 0x00000019); /* RC Idle Hysteresis */
- /* 9 */ + /* 8 */ gtt_write(0x2054, 0x0000000a); /* Render Idle Max Count */ gtt_write(0x12054,0x0000000a); /* Video Idle Max Count */ gtt_write(0x22054,0x0000000a); /* Blitter Idle Max Count */
- /* 10 */ + /* 9 */ gtt_write(0xa0b0, 0x00000000); /* Unblock Ack to Busy */ gtt_write(0xa0b4, 0x000003e8); /* RC1e Threshold */ gtt_write(0xa0b8, 0x0000c350); /* RC6 Threshold */ gtt_write(0xa0bc, 0x000186a0); /* RC6p Threshold */ gtt_write(0xa0c0, 0x0000fa00); /* RC6pp Threshold */
- /* 11 */ + /* 10 */ gtt_write(0xa010, 0x000f4240); /* RP Down Timeout */ gtt_write(0xa014, 0x12060000); /* RP Interrupt Limits */ gtt_write(0xa02c, 0x00015f90); /* RP Up Threshold */ @@ -454,7 +451,7 @@ gtt_write(0xa070, 0x0000000a); /* RP Idle Hysteresis */
/* - * 11a: Enable Render Standby (RC6) + * 10a: Enable Render Standby (RC6) * * IvyBridge should also support DeepRenderStandby. * @@ -463,7 +460,7 @@ */ gtt_write(0xa090, 0x88040000); /* HW RC Control */
- /* 12: Normal Frequency Request */ + /* 11: Normal Frequency Request */ /* RPNFREQ_VAL comes from MCHBAR 0x5998 23:16 */ /* only the lower 7 bits are used and shifted left by 25 */ reg32 = MCHBAR32(0x5998); @@ -472,12 +469,18 @@ reg32 <<= 25; gtt_write(0xa008, reg32);
- /* 13: RP Control */ + /* 12: RP Control */ gtt_write(0xa024, 0x00000592);
- /* 14: Enable PM Interrupts */ + /* 13: Enable PM Interrupts */ gtt_write(0x4402c, 0x03000076);
+ /* 14: PM Lock Settings */ + 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)); + /* Clear 0x6c024 [8:6] */ reg32 = gtt_read(0x6c024); reg32 &= ~0x000001c0;
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42447/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42447/1//COMMIT_MSG@10 PS1, Line 10: PM interrupts. I think we should document what locks what first, at least in the commit message.
https://review.coreboot.org/c/coreboot/+/42447/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42447/1/src/northbridge/intel/sandy... PS1, Line 381: 5 The numbering refers to the sequence as it is documented in the SA BIOS Spec (which disagrees with the changes).
https://review.coreboot.org/c/coreboot/+/42447/1/src/northbridge/intel/sandy... PS1, Line 376: gtt_write(0xa004, 0x00000000); Why write 0 and not skip it like the old step 5? What do you know about these registers that you didn't write into the commit message?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42447/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42447/1/src/northbridge/intel/sandy... PS1, Line 381: 5
The numbering refers to the sequence as it is documented in the SA BIOS Spec […]
Oh, so the numbering comes from the SA BIOS Spec. Then, please let's keep the original numbering.
Does the SA BIOS Spec say this bit needs to be set here? Reference code doesn't write anything like that.
https://review.coreboot.org/c/coreboot/+/42447/1/src/northbridge/intel/sandy... PS1, Line 376: gtt_write(0xa004, 0x00000000);
Why write 0 and not skip it like the old step 5? What do you know about these registers that you did […]
Reference code writes zero at this point, and locks it later.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42447/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42447/1//COMMIT_MSG@10 PS1, Line 10: PM interrupts.
I think we should document what locks what first, at least in the […]
I don't know for sure.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42447/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42447/1//COMMIT_MSG@10 PS1, Line 10: PM interrupts.
I don't know for sure.
It can be tested...
Attention is currently required from: Angel Pons, Evgeny Zinoviev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42447 )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Patchset:
PS2: I guess we just need better commit messages here. Looking at the bug tracker and this series, it seems this the only patch that affects SNB, so should be what actually fixed your X220?
Are the steps numbered in the reference code? If not, I'd like to keep the numbering of the BIOS Spec, with comments what changed, e.g. /* Bit 4 (lock) must be set later. */
File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42447/comment/aa8e4f10_b4c08ffa PS1, Line 376: gtt_write(0xa004, 0x00000000);
Reference code writes zero at this point, and locks it later.
Please leave a comment or at least mention it in the commit message. The code currently does what Intel documented, if we deviate, we should mention why.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42447?usp=email )
Change subject: nb/intel/sandybridge/gma.c: Fix lock bits setting ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Evgeny Zinoviev (Code Review)
-
Martin L Roth (Code Review)
-
Nico Huber (Code Review)