Hello Richard Spiegel, Martin Roth, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32659
to review the following change.
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
soc/amd/stoneyridge: Remove sb_util.c
pm_acpi_pm_cnt_blk() is only used in the SMI handler when reissuing the SLP_TYP cycle to send the device into S3/S5. This must be an I/O cycle, not MMIO. Make this a static in smihandler.c.
Remove unused pm_acpi_pm_evt_blk().
Relocate the reamining functions to get/save UMA information to southbridge.c. Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com
Change-Id: I90c4394e3cf26f4ad60a078948a84303bda693d0 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/stoneyridge/Makefile.inc M src/soc/amd/stoneyridge/include/soc/southbridge.h D src/soc/amd/stoneyridge/sb_util.c M src/soc/amd/stoneyridge/smihandler.c M src/soc/amd/stoneyridge/southbridge.c 5 files changed, 29 insertions(+), 62 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/32659/1
diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index 57c886d..babd878 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -46,7 +46,6 @@ bootblock-y += monotonic_timer.c bootblock-y += pmutil.c bootblock-y += reset.c -bootblock-y += sb_util.c bootblock-y += tsc_freq.c bootblock-y += southbridge.c bootblock-y += nb_util.c @@ -61,7 +60,6 @@ romstage-y += monotonic_timer.c romstage-y += pmutil.c romstage-y += reset.c -romstage-y += sb_util.c romstage-y += smbus.c romstage-y += smbus_spd.c romstage-y += ramtop.c @@ -75,7 +73,6 @@ verstage-y += gpio.c verstage-y += i2c.c verstage-y += monotonic_timer.c -verstage-y += sb_util.c verstage-y += pmutil.c verstage-y += reset.c verstage-$(CONFIG_STONEYRIDGE_UART) += uart.c @@ -86,7 +83,6 @@ postcar-y += monotonic_timer.c postcar-$(CONFIG_STONEYRIDGE_UART) += uart.c postcar-y += ramtop.c -postcar-y += sb_util.c postcar-y += nb_util.c postcar-$(CONFIG_VBOOT_MEASURED_BOOT) += i2c.c postcar-y += tsc_freq.c @@ -101,7 +97,6 @@ ramstage-y += gpio.c ramstage-y += monotonic_timer.c ramstage-y += southbridge.c -ramstage-y += sb_util.c ramstage-y += northbridge.c ramstage-y += pmutil.c ramstage-y += reset.c @@ -121,7 +116,6 @@ smm-y += monotonic_timer.c smm-y += smihandler.c smm-y += smi_util.c -smm-y += sb_util.c smm-y += tsc_freq.c smm-$(CONFIG_DEBUG_SMI) += uart.c smm-$(CONFIG_SPI_FLASH) += spi.c diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index 40d6ec7..99e5eef 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -357,8 +357,6 @@ void southbridge_init(void *chip_info); void sb_read_mode(u32 mode); void sb_set_spi100(u16 norm, u16 fast, u16 alt, u16 tpm); -uint16_t pm_acpi_pm_cnt_blk(void); -uint16_t pm_acpi_pm_evt_blk(void); void bootblock_fch_early_init(void); void bootblock_fch_init(void); /** diff --git a/src/soc/amd/stoneyridge/sb_util.c b/src/soc/amd/stoneyridge/sb_util.c deleted file mode 100644 index 7869065..0000000 --- a/src/soc/amd/stoneyridge/sb_util.c +++ /dev/null @@ -1,54 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright 2017 Advanced Micro Devices, Inc. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include <arch/io.h> -#include <device/mmio.h> -#include <arch/acpi.h> -#include <amdblocks/acpimmio.h> -#include <soc/southbridge.h> - -uint16_t pm_acpi_pm_cnt_blk(void) -{ - return pm_read16(PM1_CNT_BLK); -} - -uint16_t pm_acpi_pm_evt_blk(void) -{ - return pm_read16(PM_EVT_BLK); -} - -void save_uma_size(uint32_t size) -{ - biosram_write32(BIOSRAM_UMA_SIZE, size); -} - -void save_uma_base(uint64_t base) -{ - biosram_write32(BIOSRAM_UMA_BASE, (uint32_t) base); - biosram_write32(BIOSRAM_UMA_BASE + 4, (uint32_t) (base >> 32)); -} - -uint32_t get_uma_size(void) -{ - return biosram_read32(BIOSRAM_UMA_SIZE); -} - -uint64_t get_uma_base(void) -{ - uint64_t base; - base = biosram_read32(BIOSRAM_UMA_BASE); - base |= ((uint64_t)(biosram_read32(BIOSRAM_UMA_BASE + 4)) << 32); - return base; -} diff --git a/src/soc/amd/stoneyridge/smihandler.c b/src/soc/amd/stoneyridge/smihandler.c index e08aee4..55367ea 100644 --- a/src/soc/amd/stoneyridge/smihandler.c +++ b/src/soc/amd/stoneyridge/smihandler.c @@ -118,6 +118,11 @@ smi_write32(SMI_REG_SMISTS4, smi_read32(SMI_REG_SMISTS4)); }
+static uint16_t pm_acpi_pm_cnt_blk(void) +{ + return pm_read16(PM1_CNT_BLK); +} + static void sb_slp_typ_handler(void) { uint32_t pci_ctrl, reg32; diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index a08d6b5..b3b9e36 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -649,3 +649,27 @@ * on entry into BS_DEV_ENABLE. */ BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_ENTRY, set_pci_irqs, NULL); + +void save_uma_size(uint32_t size) +{ + biosram_write32(BIOSRAM_UMA_SIZE, size); +} + +void save_uma_base(uint64_t base) +{ + biosram_write32(BIOSRAM_UMA_BASE, (uint32_t) base); + biosram_write32(BIOSRAM_UMA_BASE + 4, (uint32_t) (base >> 32)); +} + +uint32_t get_uma_size(void) +{ + return biosram_read32(BIOSRAM_UMA_SIZE); +} + +uint64_t get_uma_base(void) +{ + uint64_t base; + base = biosram_read32(BIOSRAM_UMA_BASE); + base |= ((uint64_t)(biosram_read32(BIOSRAM_UMA_BASE + 4)) << 32); + return base; +}
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32659/1/src/soc/amd/stoneyridge/sb_util.c File src/soc/amd/stoneyridge/sb_util.c:
https://review.coreboot.org/#/c/32659/1/src/soc/amd/stoneyridge/sb_util.c@a2... PS1, Line 27: : : : : Not needed?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32659/1/src/soc/amd/stoneyridge/sb_util.c File src/soc/amd/stoneyridge/sb_util.c:
https://review.coreboot.org/#/c/32659/1/src/soc/amd/stoneyridge/sb_util.c@a2... PS1, Line 27: : : : :
Not needed?
See the commit message re. its removal.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32659/1/src/soc/amd/stoneyridge/sb_util.c File src/soc/amd/stoneyridge/sb_util.c:
https://review.coreboot.org/#/c/32659/1/src/soc/amd/stoneyridge/sb_util.c@a2... PS1, Line 27: : : : :
See the commit message re. its removal.
What I'm saying is that pm_acpi_pm_cnt_blk was moved to SMI handler, but pm_acpi_pm_evt_blk is moved nowhere... Apparently it's not used, or compile would have complained... but why was it there to start with?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 2: Code-Review+2
Sorry, now I see what you mention. Yes, it's unused... Will it stay unused?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 2:
Will it stay unused?
I have no plans to bring it back.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32659/4/src/soc/amd/stoneyridge/smihandler.c File src/soc/amd/stoneyridge/smihandler.c:
https://review.coreboot.org/#/c/32659/4/src/soc/amd/stoneyridge/smihandler.c... PS4, Line 207: pm_acpi_pm_cnt_blk Why not just replace this with: pm_read16(PM1_CNT_BLK)
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32659/4/src/soc/amd/stoneyridge/smihandler.c File src/soc/amd/stoneyridge/smihandler.c:
https://review.coreboot.org/#/c/32659/4/src/soc/amd/stoneyridge/smihandler.c... PS4, Line 207: pm_acpi_pm_cnt_blk
Why not just replace this with: pm_read16(PM1_CNT_BLK)
Sounds good
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
Patch Set 6: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32659 )
Change subject: soc/amd/stoneyridge: Remove sb_util.c ......................................................................
soc/amd/stoneyridge: Remove sb_util.c
Obsolete pm_acpi_pm_cnt_blk(), and remove it and pm_acpi_pm_evt_blk().
Relocate the remaining functions to get/save UMA information to southbridge.c.
Change-Id: I90c4394e3cf26f4ad60a078948a84303bda693d0 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32659 Reviewed-by: Martin Roth martinroth@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/stoneyridge/Makefile.inc M src/soc/amd/stoneyridge/include/soc/southbridge.h D src/soc/amd/stoneyridge/sb_util.c M src/soc/amd/stoneyridge/smihandler.c M src/soc/amd/stoneyridge/southbridge.c 5 files changed, 25 insertions(+), 63 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index 57c886d..babd878 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -46,7 +46,6 @@ bootblock-y += monotonic_timer.c bootblock-y += pmutil.c bootblock-y += reset.c -bootblock-y += sb_util.c bootblock-y += tsc_freq.c bootblock-y += southbridge.c bootblock-y += nb_util.c @@ -61,7 +60,6 @@ romstage-y += monotonic_timer.c romstage-y += pmutil.c romstage-y += reset.c -romstage-y += sb_util.c romstage-y += smbus.c romstage-y += smbus_spd.c romstage-y += ramtop.c @@ -75,7 +73,6 @@ verstage-y += gpio.c verstage-y += i2c.c verstage-y += monotonic_timer.c -verstage-y += sb_util.c verstage-y += pmutil.c verstage-y += reset.c verstage-$(CONFIG_STONEYRIDGE_UART) += uart.c @@ -86,7 +83,6 @@ postcar-y += monotonic_timer.c postcar-$(CONFIG_STONEYRIDGE_UART) += uart.c postcar-y += ramtop.c -postcar-y += sb_util.c postcar-y += nb_util.c postcar-$(CONFIG_VBOOT_MEASURED_BOOT) += i2c.c postcar-y += tsc_freq.c @@ -101,7 +97,6 @@ ramstage-y += gpio.c ramstage-y += monotonic_timer.c ramstage-y += southbridge.c -ramstage-y += sb_util.c ramstage-y += northbridge.c ramstage-y += pmutil.c ramstage-y += reset.c @@ -121,7 +116,6 @@ smm-y += monotonic_timer.c smm-y += smihandler.c smm-y += smi_util.c -smm-y += sb_util.c smm-y += tsc_freq.c smm-$(CONFIG_DEBUG_SMI) += uart.c smm-$(CONFIG_SPI_FLASH) += spi.c diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index 1e86ba6..88e0225 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -356,8 +356,6 @@ void southbridge_init(void *chip_info); void sb_read_mode(u32 mode); void sb_set_spi100(u16 norm, u16 fast, u16 alt, u16 tpm); -uint16_t pm_acpi_pm_cnt_blk(void); -uint16_t pm_acpi_pm_evt_blk(void); void bootblock_fch_early_init(void); void bootblock_fch_init(void); /** diff --git a/src/soc/amd/stoneyridge/sb_util.c b/src/soc/amd/stoneyridge/sb_util.c deleted file mode 100644 index 7869065..0000000 --- a/src/soc/amd/stoneyridge/sb_util.c +++ /dev/null @@ -1,54 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright 2017 Advanced Micro Devices, Inc. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include <arch/io.h> -#include <device/mmio.h> -#include <arch/acpi.h> -#include <amdblocks/acpimmio.h> -#include <soc/southbridge.h> - -uint16_t pm_acpi_pm_cnt_blk(void) -{ - return pm_read16(PM1_CNT_BLK); -} - -uint16_t pm_acpi_pm_evt_blk(void) -{ - return pm_read16(PM_EVT_BLK); -} - -void save_uma_size(uint32_t size) -{ - biosram_write32(BIOSRAM_UMA_SIZE, size); -} - -void save_uma_base(uint64_t base) -{ - biosram_write32(BIOSRAM_UMA_BASE, (uint32_t) base); - biosram_write32(BIOSRAM_UMA_BASE + 4, (uint32_t) (base >> 32)); -} - -uint32_t get_uma_size(void) -{ - return biosram_read32(BIOSRAM_UMA_SIZE); -} - -uint64_t get_uma_base(void) -{ - uint64_t base; - base = biosram_read32(BIOSRAM_UMA_BASE); - base |= ((uint64_t)(biosram_read32(BIOSRAM_UMA_BASE + 4)) << 32); - return base; -} diff --git a/src/soc/amd/stoneyridge/smihandler.c b/src/soc/amd/stoneyridge/smihandler.c index e08aee4..d8438bb 100644 --- a/src/soc/amd/stoneyridge/smihandler.c +++ b/src/soc/amd/stoneyridge/smihandler.c @@ -199,7 +199,7 @@ * An IO cycle is required to trigger the STPCLK/STPGNT * handshake when the Pm1 write is reissued. */ - outw(pm1cnt | SLP_EN, pm_acpi_pm_cnt_blk()); + outw(pm1cnt | SLP_EN, pm_read16(PM1_CNT_BLK)); hlt(); } } diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index 10a23f2..45408ea 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -649,3 +649,27 @@ * on entry into BS_DEV_ENABLE. */ BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_ENTRY, set_pci_irqs, NULL); + +void save_uma_size(uint32_t size) +{ + biosram_write32(BIOSRAM_UMA_SIZE, size); +} + +void save_uma_base(uint64_t base) +{ + biosram_write32(BIOSRAM_UMA_BASE, (uint32_t) base); + biosram_write32(BIOSRAM_UMA_BASE + 4, (uint32_t) (base >> 32)); +} + +uint32_t get_uma_size(void) +{ + return biosram_read32(BIOSRAM_UMA_SIZE); +} + +uint64_t get_uma_base(void) +{ + uint64_t base; + base = biosram_read32(BIOSRAM_UMA_BASE); + base |= ((uint64_t)(biosram_read32(BIOSRAM_UMA_BASE + 4)) << 32); + return base; +}
-
Furquan Shaikh (Code Review) -
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Richard Spiegel (Code Review)