Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie.c: Fix subtle blunder ......................................................................
sb/intel/lynxpoint/pcie.c: Fix subtle blunder
If only C had a decent type system...
Change-Id: I85734a68a42ec65b124d68514039a1dda7946adc Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/pcie.c 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/45713/1
diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 5e69fca..3e1f63b 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -283,8 +283,7 @@ /* Update PECR1 register. */ pci_or_config8(dev, 0xe8, 1);
- /* FIXME: Are we supposed to update this register with a constant boolean? */ - pci_update_config8(dev, 0x324, ~(1 << 5), (1 < 5)); + pci_or_config8(dev, 0x324, 1 << 5);
/* Per-Port CLKREQ# handling. */ if (is_lp && gpio_is_native(18 + rp - 1))
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie.c: Fix subtle blunder ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG@7 PS1, Line 7: sb/intel/lynxpoint/pcie.c: Fix subtle blunder Please use a more descriptive commit message about the change.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie.c: Fix subtle blunder ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG@7 PS1, Line 7: sb/intel/lynxpoint/pcie.c: Fix subtle blunder
Please use a more descriptive commit message about the change.
Mind providing ideas, please? I think this would qualify as a "subtle blunder". Or is it about the commit message body?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie.c: Fix subtle blunder ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... PS2, Line 286: pci_or_config8(dev, 0x324, 1 << 5); is it intended that this is not doing the same as line 287? ((1 < 5) vs (1 << 5))
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie.c: Fix subtle blunder ......................................................................
Patch Set 2: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG@7 PS1, Line 7: sb/intel/lynxpoint/pcie.c: Fix subtle blunder
Mind providing ideas, please? I think this would qualify as a "subtle blunder". […]
Please just rewrite both, e.g.
sb/intel/lynxpoint/pcie: Fix clock gating routine
The use of `1 < 5` as a bit mask was obviously a typo. Fix it to `1 << 5`.
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... PS2, Line 286: pci_or_config8(dev, 0x324, 1 << 5);
is it intended that this is not doing the same as line 287? ((1 < 5) vs (1 << 5))
If it wasn't changing anything, the commit message would be a lie ;)
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45713
to look at the new patch set (#3).
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
sb/intel/lynxpoint/pcie: Fix clock gating routine
The use of `1 < 5` as a bit mask was obviously a typo. Correct it as `1 << 5` to match what Intel doc #493816 (Lynx Point PCH BWG) states.
Change-Id: I85734a68a42ec65b124d68514039a1dda7946adc Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/pcie.c 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/45713/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45713/1//COMMIT_MSG@7 PS1, Line 7: sb/intel/lynxpoint/pcie.c: Fix subtle blunder
Please just rewrite both, e.g. […]
Done, and confirmed that the bit indeed needs to be set.
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... PS2, Line 286: pci_or_config8(dev, 0x324, 1 << 5);
If it wasn't changing anything, the commit message would be a lie ;)
Yes, it is intentional. The original code was wrong, and the new code matches what the BWG states.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... PS2, Line 286: pci_or_config8(dev, 0x324, 1 << 5);
Yes, it is intentional. The original code was wrong, and the new code matches what the BWG states.
Ack. Maybe add comment what that bit does?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... PS2, Line 286: pci_or_config8(dev, 0x324, 1 << 5);
Ack. […]
It does magic! Honestly, I've no clue.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/45713/2/src/southbridge/intel/lynxp... PS2, Line 286: pci_or_config8(dev, 0x324, 1 << 5);
It does magic! Honestly, I've no clue.
(∩｀-´)⊃━☆ﾟ.*･｡ﾟ
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3: Code-Review+2
Looks reasonable. Were you able to test this?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
Patch Set 3:
Patch Set 3: Code-Review+2
Looks reasonable. Were you able to test this?
I tested on the Asrock B85M Pro4 and it still boots.
Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45713 )
Change subject: sb/intel/lynxpoint/pcie: Fix clock gating routine ......................................................................
sb/intel/lynxpoint/pcie: Fix clock gating routine
The use of `1 < 5` as a bit mask was obviously a typo. Correct it as `1 << 5` to match what Intel doc #493816 (Lynx Point PCH BWG) states.
Change-Id: I85734a68a42ec65b124d68514039a1dda7946adc Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45713 Reviewed-by: Michael Niewöhner foss@mniewoehner.de Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/intel/lynxpoint/pcie.c 1 file changed, 1 insertion(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Nico Huber: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 112c9d3..4a245b1 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -283,8 +283,7 @@ /* Update PECR1 register. */ pci_or_config8(dev, 0xe8, 1);
- /* FIXME: Are we supposed to update this register with a constant boolean? */ - pci_update_config8(dev, 0x324, ~(1 << 5), (1 < 5)); + pci_or_config8(dev, 0x324, 1 << 5);
/* Per-Port CLKREQ# handling. */ if (is_lp && gpio_is_native(18 + rp - 1))
-
Angel Pons (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)