Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
sb/amd/sb800: Remove bit shift that does nothing
This bit shift attempts to set bits 8 and 9 of the byte variable (counting from 0). However, as the name suggests, this variable is only 8 bits wide, so the shift does nothing. Reading section 7.5 of the AMD SB800-Series Southbridges Register Programming Requirements manual, bits 8 and 9 are already set by default, so we can remove the bit shift.
Change-Id: I645236441e02925ee01339378d213cb343027363 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229582 --- M src/southbridge/amd/sb800/usb.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34395/1
diff --git a/src/southbridge/amd/sb800/usb.c b/src/southbridge/amd/sb800/usb.c index bc8c1c6..063750d 100644 --- a/src/southbridge/amd/sb800/usb.c +++ b/src/southbridge/amd/sb800/usb.c @@ -42,7 +42,6 @@ /* RPR 7.4 Enable the USB controller to get reset by any software that generate a PCIRst# condition */ byte = pm_ioread(0xF0); byte |= (1 << 2); - byte |= 3 << 8; /* rpr 7.5 */ pm_iowrite(0xF0, byte);
/* RPR 7.9 Disable OHCI MSI Capability. */
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
Patch Set 1:
Of course, this depends on how much we trust the hardware to do what the manual says
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG@13 PS1, Line 13: bits 8 and 9 are already set by default, so we can remove the bit shift. Since pm_iowrite() is eventually outb(), those bits 8 or 9 were neither read or written here, they would be register 0xF1?
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG@13 PS1, Line 13: bits 8 and 9 are already set by default, so we can remove the bit shift.
Since pm_iowrite() is eventually outb(), those bits 8 or 9 were neither read or written here, they […]
That would make sense. Do you think it's safe to try setting them there instead?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG@13 PS1, Line 13: bits 8 and 9 are already set by default, so we can remove the bit shift.
That would make sense. […]
Nah.. just improve the commit message if you like.
Hello Kyösti Mälkki, Timothy Pearson, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth, Damien Zammit,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34395
to look at the new patch set (#2).
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
sb/amd/sb800: Remove bit shift that does nothing
This bit shift attempts to set bits 8 and 9 of the byte variable (counting from 0). However, as the name suggests, this variable is only 8 bits wide, so the shift does nothing. Reading section 7.5 of the AMD SB800-Series Southbridges Register Programming Requirements manual, bits 8 and 9 are already set by default, so we can remove the bit shift. (Alternatively, we could try setting the corresponding bits one byte higher in 0xF1 if needed.)
Change-Id: I645236441e02925ee01339378d213cb343027363 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229582 --- M src/southbridge/amd/sb800/usb.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34395/2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34395/1//COMMIT_MSG@13 PS1, Line 13: bits 8 and 9 are already set by default, so we can remove the bit shift.
Nah.. just improve the commit message if you like.
Done
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34395 )
Change subject: sb/amd/sb800: Remove bit shift that does nothing ......................................................................
sb/amd/sb800: Remove bit shift that does nothing
This bit shift attempts to set bits 8 and 9 of the byte variable (counting from 0). However, as the name suggests, this variable is only 8 bits wide, so the shift does nothing. Reading section 7.5 of the AMD SB800-Series Southbridges Register Programming Requirements manual, bits 8 and 9 are already set by default, so we can remove the bit shift. (Alternatively, we could try setting the corresponding bits one byte higher in 0xF1 if needed.)
Change-Id: I645236441e02925ee01339378d213cb343027363 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229582 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34395 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/amd/sb800/usb.c 1 file changed, 0 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/southbridge/amd/sb800/usb.c b/src/southbridge/amd/sb800/usb.c index bc8c1c6..063750d 100644 --- a/src/southbridge/amd/sb800/usb.c +++ b/src/southbridge/amd/sb800/usb.c @@ -42,7 +42,6 @@ /* RPR 7.4 Enable the USB controller to get reset by any software that generate a PCIRst# condition */ byte = pm_ioread(0xF0); byte |= (1 << 2); - byte |= 3 << 8; /* rpr 7.5 */ pm_iowrite(0xF0, byte);
/* RPR 7.9 Disable OHCI MSI Capability. */
-
Jacob Garber (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)