Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33774
Change subject: sb/amd/sb{700,800}: Cleanup index manipulations ......................................................................
sb/amd/sb{700,800}: Cleanup index manipulations
It looks like in days gone by that these switches were once parts of loops that incremented 'index' as they went along. However, we don't have any loops anymore, so remove the needless increments and streamline the rest of the assignments.
Change-Id: Iaabee984333c273af7810f9c11ed26bbb2a995d1 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: scan-build 8.0.0 --- M src/southbridge/amd/sb700/sb700.c M src/southbridge/amd/sb800/sb800.c 2 files changed, 6 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/33774/1
diff --git a/src/southbridge/amd/sb700/sb700.c b/src/southbridge/amd/sb700/sb700.c index e3594fd..6f59179 100644 --- a/src/southbridge/amd/sb700/sb700.c +++ b/src/southbridge/amd/sb700/sb700.c @@ -171,7 +171,6 @@ index = 8; set_sm_enable_bits(sm_dev, 0xac, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 3; break; case PCI_DEVFN(0x12, 0): case PCI_DEVFN(0x12, 1): @@ -179,7 +178,6 @@ index = dev->path.pci.devfn & 3; set_sm_enable_bits(sm_dev, 0x68, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x13, 0): case PCI_DEVFN(0x13, 1): @@ -187,34 +185,27 @@ index = (dev->path.pci.devfn & 3) + 4; set_sm_enable_bits(sm_dev, 0x68, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x14, 5): index = 7; set_sm_enable_bits(sm_dev, 0x68, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x14, 0): - index = 0; break; case PCI_DEVFN(0x14, 1): - index = 1; break; case PCI_DEVFN(0x14, 2): index = 3; set_pmio_enable_bits(sm_dev, 0x59, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 4; break; case PCI_DEVFN(0x14, 3): index = 20; set_sm_enable_bits(sm_dev, 0x64, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 1; break; case PCI_DEVFN(0x14, 4): - index = 4; break; default: printk(BIOS_DEBUG, "unknown dev: %s deviceid=%4x\n", dev_path(dev), diff --git a/src/southbridge/amd/sb800/sb800.c b/src/southbridge/amd/sb800/sb800.c index eda4d30..8935cff 100644 --- a/src/southbridge/amd/sb800/sb800.c +++ b/src/southbridge/amd/sb800/sb800.c @@ -295,9 +295,9 @@
switch (dev->path.pci.devfn - (devfn - (0x14 << 3))) { case PCI_DEVFN(0x11, 0): - index = 8; - set_pmio_enable_bits(0xDA, 1 << 0, - (dev->enabled ? 1 : 0) << 0); + index = 0; + set_pmio_enable_bits(0xDA, 1 << index, + (dev->enabled ? 1 : 0) << index); /* Set the device ID of SATA as 0x4390 to reduce the confusing. */ dword = pci_read_config32(dev, 0x40); dword |= 1 << 0; @@ -305,7 +305,6 @@ pci_write_config16(dev, 0x2, 0x4390); dword &= ~1; pci_write_config32(dev, 0x40, dword);//for (;;); - index += 32 * 3; break; case PCI_DEVFN(0x12, 0): case PCI_DEVFN(0x12, 2): @@ -318,15 +317,13 @@ index = (dev->path.pci.devfn & 0x3) / 2 + 2; set_pmio_enable_bits(0xEF, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x14, 0): - index = 0; break; case PCI_DEVFN(0x14, 1): - index = 1; - set_pmio_enable_bits(0xDA, 1 << 3, - (dev->enabled ? 0 : 1) << 3); + index = 3; + set_pmio_enable_bits(0xDA, 1 << index, + (dev->enabled ? 0 : 1) << index); break; case PCI_DEVFN(0x14, 2): index = 0; @@ -356,7 +353,6 @@ break; case PCI_DEVFN(0x15, 0): set_sb800_gpp(dev); - index = 4; break; case PCI_DEVFN(0x15, 1): case PCI_DEVFN(0x15, 2):
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33774 )
Change subject: sb/amd/sb{700,800}: Cleanup index manipulations ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
out of topic: sb800.c: we have "index = (dev->path.pci.devfn & 0x3) / 2" I don't know if (path.pci.devfn & 0x3) is an even number
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb700/sb700.c File src/southbridge/amd/sb700/sb700.c:
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb700/sb700.c@a1... PS1, Line 107: why it is set = -1 ?
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb800/sb800.c File src/southbridge/amd/sb800/sb800.c:
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb800/sb800.c@a2... PS1, Line 231: out of topic, but why "-1" ?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33774 )
Change subject: sb/amd/sb{700,800}: Cleanup index manipulations ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/33774/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33774/1//COMMIT_MSG@7 PS1, Line 7: Cleanup The verb is spelled with a space: Clean up.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33774 )
Change subject: sb/amd/sb{700,800}: Cleanup index manipulations ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb700/sb700.c File src/southbridge/amd/sb700/sb700.c:
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb700/sb700.c@a1... PS1, Line 107:
why it is set = -1 ?
No need to initialize it here.
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb700/sb700.c@15... PS1, Line 156: return; I wonder what this was supposed to do.
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb800/sb800.c File src/southbridge/amd/sb800/sb800.c:
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb800/sb800.c@a2... PS1, Line 231:
out of topic, but why "-1" ?
No need and it's not out of topic to remove it.
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb800/sb800.c@a3... PS1, Line 340: No need to keep this either.
https://review.coreboot.org/#/c/33774/1/src/southbridge/amd/sb800/sb800.c@31... PS1, Line 311: index = (dev->path.pci.devfn & 0x3) / 2; The case has even devfn of 12.0 or 12.2, it's fine.
Hello Kyösti Mälkki, HAOUAS Elyes, Paul Menzel, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33774
to look at the new patch set (#2).
Change subject: sb/amd/sb{700,800}: Clean up index manipulations ......................................................................
sb/amd/sb{700,800}: Clean up index manipulations
It looks like in days gone by that these switches were once parts of loops that incremented 'index' as they went along. However, we don't have any loops anymore, so remove the needless increments and streamline the rest of the assignments.
Change-Id: Iaabee984333c273af7810f9c11ed26bbb2a995d1 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: scan-build 8.0.0 --- M src/southbridge/amd/sb700/sb700.c M src/southbridge/amd/sb800/sb800.c 2 files changed, 8 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/33774/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33774 )
Change subject: sb/amd/sb{700,800}: Clean up index manipulations ......................................................................
Patch Set 2: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33774 )
Change subject: sb/amd/sb{700,800}: Clean up index manipulations ......................................................................
sb/amd/sb{700,800}: Clean up index manipulations
It looks like in days gone by that these switches were once parts of loops that incremented 'index' as they went along. However, we don't have any loops anymore, so remove the needless increments and streamline the rest of the assignments.
Change-Id: Iaabee984333c273af7810f9c11ed26bbb2a995d1 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: scan-build 8.0.0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33774 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/amd/sb700/sb700.c M src/southbridge/amd/sb800/sb800.c 2 files changed, 8 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/southbridge/amd/sb700/sb700.c b/src/southbridge/amd/sb700/sb700.c index e3594fd..1836f34 100644 --- a/src/southbridge/amd/sb700/sb700.c +++ b/src/southbridge/amd/sb700/sb700.c @@ -104,7 +104,7 @@ { struct device *sm_dev = NULL; struct device *bus_dev = NULL; - int index = -1; + int index; u32 deviceid; u32 vendorid;
@@ -171,7 +171,6 @@ index = 8; set_sm_enable_bits(sm_dev, 0xac, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 3; break; case PCI_DEVFN(0x12, 0): case PCI_DEVFN(0x12, 1): @@ -179,7 +178,6 @@ index = dev->path.pci.devfn & 3; set_sm_enable_bits(sm_dev, 0x68, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x13, 0): case PCI_DEVFN(0x13, 1): @@ -187,34 +185,27 @@ index = (dev->path.pci.devfn & 3) + 4; set_sm_enable_bits(sm_dev, 0x68, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x14, 5): index = 7; set_sm_enable_bits(sm_dev, 0x68, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x14, 0): - index = 0; break; case PCI_DEVFN(0x14, 1): - index = 1; break; case PCI_DEVFN(0x14, 2): index = 3; set_pmio_enable_bits(sm_dev, 0x59, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 4; break; case PCI_DEVFN(0x14, 3): index = 20; set_sm_enable_bits(sm_dev, 0x64, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 1; break; case PCI_DEVFN(0x14, 4): - index = 4; break; default: printk(BIOS_DEBUG, "unknown dev: %s deviceid=%4x\n", dev_path(dev), diff --git a/src/southbridge/amd/sb800/sb800.c b/src/southbridge/amd/sb800/sb800.c index eda4d30..801cc65 100644 --- a/src/southbridge/amd/sb800/sb800.c +++ b/src/southbridge/amd/sb800/sb800.c @@ -228,7 +228,7 @@ { struct device *sm_dev = NULL; struct device *bus_dev = NULL; - int index = -1; + int index; u32 deviceid; u32 vendorid;
@@ -295,9 +295,9 @@
switch (dev->path.pci.devfn - (devfn - (0x14 << 3))) { case PCI_DEVFN(0x11, 0): - index = 8; - set_pmio_enable_bits(0xDA, 1 << 0, - (dev->enabled ? 1 : 0) << 0); + index = 0; + set_pmio_enable_bits(0xDA, 1 << index, + (dev->enabled ? 1 : 0) << index); /* Set the device ID of SATA as 0x4390 to reduce the confusing. */ dword = pci_read_config32(dev, 0x40); dword |= 1 << 0; @@ -305,7 +305,6 @@ pci_write_config16(dev, 0x2, 0x4390); dword &= ~1; pci_write_config32(dev, 0x40, dword);//for (;;); - index += 32 * 3; break; case PCI_DEVFN(0x12, 0): case PCI_DEVFN(0x12, 2): @@ -318,15 +317,13 @@ index = (dev->path.pci.devfn & 0x3) / 2 + 2; set_pmio_enable_bits(0xEF, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 2; break; case PCI_DEVFN(0x14, 0): - index = 0; break; case PCI_DEVFN(0x14, 1): - index = 1; - set_pmio_enable_bits(0xDA, 1 << 3, - (dev->enabled ? 0 : 1) << 3); + index = 3; + set_pmio_enable_bits(0xDA, 1 << index, + (dev->enabled ? 0 : 1) << index); break; case PCI_DEVFN(0x14, 2): index = 0; @@ -337,7 +334,6 @@ index = 0; set_pmio_enable_bits(0xEC, 1 << index, (dev->enabled ? 1 : 0) << index); - index += 32 * 1; break; case PCI_DEVFN(0x14, 4): index = 0; @@ -356,7 +352,6 @@ break; case PCI_DEVFN(0x15, 0): set_sb800_gpp(dev); - index = 4; break; case PCI_DEVFN(0x15, 1): case PCI_DEVFN(0x15, 2):
-
HAOUAS Elyes (Code Review) -
Jacob Garber (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)