Change in coreboot[master]: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB
Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB
Change-Id: I31f8cddb9408d0c23c558b51f5d1f8787f0b700f Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/gma.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/42455/1
diff --git a/src/northbridge/intel/sandybridge/gma.c b/src/northbridge/intel/sandybridge/gma.c index f9f318c..3e8d0de 100644 --- a/src/northbridge/intel/sandybridge/gma.c +++ b/src/northbridge/intel/sandybridge/gma.c @@ -510,7 +510,8 @@ gtt_write(0xa18c, gtt_read(0xa18c) & ~1); gtt_poll(0x130090, (1 << 0), (0 << 0)); } else { - gtt_write(0xa188, 0x1fffe); + gtt_write(0xa188, gtt_read(0xa188) & 0x1ffff); + gtt_write(0xa188, gtt_read(0xa188) & ~1); if (gtt_poll(0x130040, (1 << 0), (0 << 0))) gtt_write(0xa188, gtt_read(0xa188) | 1); }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42455/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42455/1//COMMIT_MSG@8 PS1, Line 8: Maybe add the reason I commented on CB:42410 here?
Reference code does two separate writes. We shall follow suit.
https://review.coreboot.org/c/coreboot/+/42455/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42455/1/src/northbridge/intel/sandy... PS1, Line 513: 0x1ffff For consistency, use: ~0xfffe0000
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42455
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB
Reference code does two separate writes. We shall follow suit.
Change-Id: I31f8cddb9408d0c23c558b51f5d1f8787f0b700f Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/northbridge/intel/sandybridge/gma.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/42455/2
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42455/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42455/1//COMMIT_MSG@8 PS1, Line 8:
Maybe add the reason I commented on CB:42410 here? […]
Done
https://review.coreboot.org/c/coreboot/+/42455/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/gma.c:
https://review.coreboot.org/c/coreboot/+/42455/1/src/northbridge/intel/sandy... PS1, Line 513: 0x1ffff
For consistency, use: ~0xfffe0000
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
(2 comments)
I know it's a lot of work, but I think all the changes in this series (even individual bits) should be tested separately if they fix anything.
https://review.coreboot.org/c/coreboot/+/42455/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42455/2//COMMIT_MSG@7 PS2, Line 7: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB How can we tell if this is indeed correcting something?
https://review.coreboot.org/c/coreboot/+/42455/2//COMMIT_MSG@9 PS2, Line 9: Reference *Somebody else's*?
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
Patch Set 2:
(2 comments)
I know it's a lot of work, but I think all the changes in this series (even individual bits) should be tested separately if they fix anything.
It's quite difficult to do given that we have no idea what these bits mean. Or by testing you mean running it for a couple of days and making sure the system is still stable?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
I know it's a lot of work, but I think all the changes in this series (even individual bits) should be tested separately if they fix anything.
It's quite difficult to do given that we have no idea what these bits mean. Or by testing you mean running it for a couple of days and making sure the system is still stable?
Basically yes, do changes step by step until your problematic system is stable. If we merge this without detailed information which parts actually fix something, and then something goes wrong, it would be reverted as a whole.
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
Patch Set 2:
I know it's a lot of work, but I think all the changes in this series (even individual bits) should be tested separately if they fix anything.
It's quite difficult to do given that we have no idea what these bits mean. Or by testing you mean running it for a couple of days and making sure the system is still stable?
Basically yes, do changes step by step until your problematic system is stable. If we merge this without detailed information which parts actually fix something, and then something goes wrong, it would be reverted as a whole.
Okay. But if in the end I'll find out that my crashes are fixed by one or two or three lock bits (not all four) moved to the end, and the reference code sets them all in the end (which actually makes sense to me), I hope we agree that we should move all four locks to the end?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
I know it's a lot of work, but I think all the changes in this series (even individual bits) should be tested separately if they fix anything.
It's quite difficult to do given that we have no idea what these bits mean. Or by testing you mean running it for a couple of days and making sure the system is still stable?
Basically yes, do changes step by step until your problematic system is stable. If we merge this without detailed information which parts actually fix something, and then something goes wrong, it would be reverted as a whole.
Okay. But if in the end I'll find out that my crashes are fixed by one or two or three lock bits (not all four) moved to the end, and the reference code sets them all in the end (which actually makes sense to me), I hope we agree that we should move all four locks to the end?
I honestly don't care what we do as long as it is properly documented.
OTOH, if it turns out that all register writes are effective even if we don't move the locking, I see no reason to move it (assume the registers can be dumped).
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 2:
Patch Set 2:
I know it's a lot of work, but I think all the changes in this series (even individual bits) should be tested separately if they fix anything.
It's quite difficult to do given that we have no idea what these bits mean. Or by testing you mean running it for a couple of days and making sure the system is still stable?
Basically yes, do changes step by step until your problematic system is stable. If we merge this without detailed information which parts actually fix something, and then something goes wrong, it would be reverted as a whole.
Okay. But if in the end I'll find out that my crashes are fixed by one or two or three lock bits (not all four) moved to the end, and the reference code sets them all in the end (which actually makes sense to me), I hope we agree that we should move all four locks to the end?
I honestly don't care what we do as long as it is properly documented.
OTOH, if it turns out that all register writes are effective even if we don't move the locking, I see no reason to move it (assume the registers can be dumped).
Well, maybe there's a chance that somebody in possession of SA BIOS Spec will help us documenting this... :)
Attention is currently required from: Evgeny Zinoviev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42455 )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
Well, maybe there's a chance that somebody in possession of SA BIOS Spec will help us documenting this... :)
Sorry if I was too rude here. I didn't mean to bail out, just lost track of these patches at some point.
It's not that easy. In my copy of the BIOS Spec (maybe the latest maybe not, Intel doesn't provide access to these docs anymore) it says exactly what the code currently does. And that's why the code does it, it was written with the spec at hand.
Now we have documentation of which we don't know if it's the latest and we have refcode of which we don't know if it's the latest and we don't know if Intel updated the documentation at all after the last code changes. The only thing we know is that the code does exactly what was documented. So I say, if we want to change the code, we should document that better. If we don't, somebody might patch the code back to the old state, because that's what was documented.
Documentation starts with the commit message. Currently it says nothing about what was tested.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42455?usp=email )
Change subject: nb/intel/sandybridge/gma.c: Correct deasserting force wake on IVB ......................................................................
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)