Change in coreboot[master]: [WIP] soc/amd: factor out vbnv_cmos_failed() into common block
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: [WIP] soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
[WIP] soc/amd: factor out vbnv_cmos_failed() into common block
still untested.
Change-Id: I7f976c6c5a2a715e1a5372bb93fe657d0d86c848 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- A src/soc/amd/common/block/vbnv_cmos/Kconfig A src/soc/amd/common/block/vbnv_cmos/Makefile.inc R src/soc/amd/common/block/vbnv_cmos/vbnv_cmos.c M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc D src/soc/amd/stoneyridge/pmutil.c 8 files changed, 12 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/47584/1
diff --git a/src/soc/amd/common/block/vbnv_cmos/Kconfig b/src/soc/amd/common/block/vbnv_cmos/Kconfig new file mode 100644 index 0000000..9130167 --- /dev/null +++ b/src/soc/amd/common/block/vbnv_cmos/Kconfig @@ -0,0 +1,6 @@ +config SOC_AMD_COMMON_BLOCK_VBNV_CMOS + bool + default n + help + Select this option to add the AMD common VBOOT CMOS functions that + VBOOT_VBNV_CMOS needs to the build. diff --git a/src/soc/amd/common/block/vbnv_cmos/Makefile.inc b/src/soc/amd/common/block/vbnv_cmos/Makefile.inc new file mode 100644 index 0000000..79b65c9 --- /dev/null +++ b/src/soc/amd/common/block/vbnv_cmos/Makefile.inc @@ -0,0 +1,4 @@ +bootblock-$(CONFIG_SOC_AMD_COMMON_BLOCK_VBNV_CMOS) += vbnv_cmos.c +verstage-$(CONFIG_SOC_AMD_COMMON_BLOCK_VBNV_CMOS) += vbnv_cmos.c +romstage-$(CONFIG_SOC_AMD_COMMON_BLOCK_VBNV_CMOS) += vbnv_cmos.c +ramstage-$(CONFIG_SOC_AMD_COMMON_BLOCK_VBNV_CMOS) += vbnv_cmos.c diff --git a/src/soc/amd/picasso/pmutil.c b/src/soc/amd/common/block/vbnv_cmos/vbnv_cmos.c similarity index 100% rename from src/soc/amd/picasso/pmutil.c rename to src/soc/amd/common/block/vbnv_cmos/vbnv_cmos.c diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 9ebc651..8fab58c 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -43,6 +43,7 @@ select SOC_AMD_COMMON_BLOCK_SMBUS select SOC_AMD_COMMON_BLOCK_SMU select SOC_AMD_COMMON_BLOCK_PSP_GEN2 + select SOC_AMD_COMMON_BLOCK_VBNV_CMOS if VBOOT_VBNV_CMOS select PROVIDES_ROM_SHARING select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH select PARALLEL_MP diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index ffc1463..6ec4cd1 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -23,14 +23,12 @@ bootblock-y += gpio.c bootblock-y += smi_util.c bootblock-y += config.c -bootblock-y += pmutil.c bootblock-y += reset.c bootblock-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += bootblock/vboot_bootblock.c
romstage-y += i2c.c romstage-y += romstage.c romstage-y += gpio.c -romstage-y += pmutil.c romstage-y += reset.c romstage-y += memmap.c romstage-y += uart.c @@ -45,7 +43,6 @@ romstage-y += mrc_cache.c
verstage-y += i2c.c -verstage-y += pmutil.c verstage-y += config.c verstage-y += aoac.c verstage_x86-y += gpio.c @@ -66,7 +63,6 @@ ramstage-y += gpio.c ramstage-y += aoac.c ramstage-y += southbridge.c -ramstage-y += pmutil.c ramstage-y += reset.c ramstage-y += acp.c ramstage-y += sata.c diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index 0e32005..baba752 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -37,6 +37,7 @@ select SOC_AMD_COMMON_BLOCK_CAR select SOC_AMD_COMMON_BLOCK_S3 select SOC_AMD_COMMON_BLOCK_SMBUS + select SOC_AMD_COMMON_BLOCK_VBNV_CMOS if VBOOT_VBNV_CMOS select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH select PARALLEL_MP select PARALLEL_MP_AP_WORK diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index fb0a45b..311ea68 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -17,7 +17,6 @@ bootblock-y += i2c.c bootblock-y += enable_usbdebug.c bootblock-y += monotonic_timer.c -bootblock-y += pmutil.c bootblock-y += tsc_freq.c bootblock-y += southbridge.c bootblock-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c @@ -28,7 +27,6 @@ romstage-y += enable_usbdebug.c romstage-y += gpio.c romstage-y += monotonic_timer.c -romstage-y += pmutil.c romstage-y += smbus_spd.c romstage-y += memmap.c romstage-$(CONFIG_STONEYRIDGE_UART) += uart.c @@ -40,7 +38,6 @@ verstage-y += gpio.c verstage-y += i2c.c verstage-y += monotonic_timer.c -verstage-y += pmutil.c verstage-$(CONFIG_STONEYRIDGE_UART) += uart.c verstage-y += tsc_freq.c
@@ -61,7 +58,6 @@ ramstage-y += monotonic_timer.c ramstage-y += southbridge.c ramstage-y += northbridge.c -ramstage-y += pmutil.c ramstage-y += sata.c ramstage-y += memmap.c ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c diff --git a/src/soc/amd/stoneyridge/pmutil.c b/src/soc/amd/stoneyridge/pmutil.c deleted file mode 100644 index a2ad2db..0000000 --- a/src/soc/amd/stoneyridge/pmutil.c +++ /dev/null @@ -1,14 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <acpi/acpi.h> -#include <soc/southbridge.h> -#include <amdblocks/acpimmio.h> -#include <security/vboot/vboot_common.h> -#include <security/vboot/vbnv.h> -#include <pc80/mc146818rtc.h> - -int vbnv_cmos_failed(void) -{ - /* If CMOS power has failed, the century will be set to 0xff */ - return cmos_read(RTC_CLK_ALTCENTURY) == 0xff; -}
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47584
to look at the new patch set (#2).
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
soc/amd: factor out vbnv_cmos_failed() into common block
Change-Id: I7f976c6c5a2a715e1a5372bb93fe657d0d86c848 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- A src/soc/amd/common/block/vbnv_cmos/Kconfig A src/soc/amd/common/block/vbnv_cmos/Makefile.inc R src/soc/amd/common/block/vbnv_cmos/vbnv_cmos.c M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/Makefile.inc M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc D src/soc/amd/stoneyridge/pmutil.c 8 files changed, 12 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/47584/2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... File src/soc/amd/common/block/vbnv_cmos/Kconfig:
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... PS2, Line 1: SOC_AMD_COMMON_BLOCK_VBNV_CMOS I think of common "block" as representing an IP block in the platform. VBNV is not really an IP block.
What do you think about: 1. Create a directory vboot/ under soc/amd/common which is included if CONFIG_SOC_AMD_COMMON is selected 2. Create a file vbnv.c under vboot/ which is included if CONFIG_VBOOT_VBNV_CMOS is selected.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... File src/soc/amd/common/block/vbnv_cmos/Kconfig:
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... PS2, Line 1: SOC_AMD_COMMON_BLOCK_VBNV_CMOS
I think of common "block" as representing an IP block in the platform. […]
oh, I interpreted the block part as firmware building blocks which 90% of the time is more or less the same, but not in this case. putting the functionality under soc/amd/common/vboot and not having an extra Kconfig symbol for this sounds good to me; all AMD SoCs need that when selecting CONFIG_VBOOT_VBNV_CMOS anyway. will push an updated version later today
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
Patch Set 2: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... File src/soc/amd/common/block/vbnv_cmos/Kconfig:
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... PS2, Line 1: SOC_AMD_COMMON_BLOCK_VBNV_CMOS
oh, I interpreted the block part as firmware building blocks which 90% of the time is more or less t […]
Thanks Furquan, I'd wondered about the "Block" name. I guess I thought of it similarly to Felix.
Hello build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47584
to look at the new patch set (#3).
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
soc/amd: factor out vbnv_cmos_failed() into common block
Change-Id: I7f976c6c5a2a715e1a5372bb93fe657d0d86c848 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/Makefile.inc A src/soc/amd/common/vboot/Makefile.inc R src/soc/amd/common/vboot/vbnv_cmos.c M src/soc/amd/picasso/Makefile.inc M src/soc/amd/stoneyridge/Makefile.inc D src/soc/amd/stoneyridge/pmutil.c 6 files changed, 10 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/47584/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into common block ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47584/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47584/3//COMMIT_MSG@7 PS3, Line 7: block not block anymore.
I think it would also be helpful to have some text capturing the addition of vboot/ to amd/common in the commit message.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47584
to look at the new patch set (#4).
Change subject: soc/amd: factor out vbnv_cmos_failed() into soc/amd/common/vboot ......................................................................
soc/amd: factor out vbnv_cmos_failed() into soc/amd/common/vboot
Change-Id: I7f976c6c5a2a715e1a5372bb93fe657d0d86c848 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/Makefile.inc A src/soc/amd/common/vboot/Makefile.inc R src/soc/amd/common/vboot/vbnv_cmos.c M src/soc/amd/picasso/Makefile.inc M src/soc/amd/stoneyridge/Makefile.inc D src/soc/amd/stoneyridge/pmutil.c 6 files changed, 10 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/47584/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into soc/amd/common/vboot ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47584/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47584/3//COMMIT_MSG@7 PS3, Line 7: block
not block anymore. […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into soc/amd/common/vboot ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... File src/soc/amd/common/block/vbnv_cmos/Kconfig:
https://review.coreboot.org/c/coreboot/+/47584/2/src/soc/amd/common/block/vb... PS2, Line 1: SOC_AMD_COMMON_BLOCK_VBNV_CMOS
Thanks Furquan, I'd wondered about the "Block" name. I guess I thought of it similarly to Felix.
Done
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47584 )
Change subject: soc/amd: factor out vbnv_cmos_failed() into soc/amd/common/vboot ......................................................................
soc/amd: factor out vbnv_cmos_failed() into soc/amd/common/vboot
Change-Id: I7f976c6c5a2a715e1a5372bb93fe657d0d86c848 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/47584 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/Makefile.inc A src/soc/amd/common/vboot/Makefile.inc R src/soc/amd/common/vboot/vbnv_cmos.c M src/soc/amd/picasso/Makefile.inc M src/soc/amd/stoneyridge/Makefile.inc D src/soc/amd/stoneyridge/pmutil.c 6 files changed, 10 insertions(+), 23 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/common/Makefile.inc b/src/soc/amd/common/Makefile.inc index c0757c5..418539b 100644 --- a/src/soc/amd/common/Makefile.inc +++ b/src/soc/amd/common/Makefile.inc @@ -1 +1,4 @@ -subdirs-$(CONFIG_SOC_AMD_COMMON) += block +ifeq ($(CONFIG_SOC_AMD_COMMON),y) +subdirs-y += block +subdirs-y += vboot +endif diff --git a/src/soc/amd/common/vboot/Makefile.inc b/src/soc/amd/common/vboot/Makefile.inc new file mode 100644 index 0000000..aff927a --- /dev/null +++ b/src/soc/amd/common/vboot/Makefile.inc @@ -0,0 +1,6 @@ +ifeq ($(CONFIG_VBOOT_VBNV_CMOS),y) +bootblock-y += vbnv_cmos.c +verstage-y += vbnv_cmos.c +romstage-y += vbnv_cmos.c +ramstage-y += vbnv_cmos.c +endif diff --git a/src/soc/amd/picasso/pmutil.c b/src/soc/amd/common/vboot/vbnv_cmos.c similarity index 100% rename from src/soc/amd/picasso/pmutil.c rename to src/soc/amd/common/vboot/vbnv_cmos.c diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index ffc1463..6ec4cd1 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -23,14 +23,12 @@ bootblock-y += gpio.c bootblock-y += smi_util.c bootblock-y += config.c -bootblock-y += pmutil.c bootblock-y += reset.c bootblock-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += bootblock/vboot_bootblock.c
romstage-y += i2c.c romstage-y += romstage.c romstage-y += gpio.c -romstage-y += pmutil.c romstage-y += reset.c romstage-y += memmap.c romstage-y += uart.c @@ -45,7 +43,6 @@ romstage-y += mrc_cache.c
verstage-y += i2c.c -verstage-y += pmutil.c verstage-y += config.c verstage-y += aoac.c verstage_x86-y += gpio.c @@ -66,7 +63,6 @@ ramstage-y += gpio.c ramstage-y += aoac.c ramstage-y += southbridge.c -ramstage-y += pmutil.c ramstage-y += reset.c ramstage-y += acp.c ramstage-y += sata.c diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index fb0a45b..311ea68 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -17,7 +17,6 @@ bootblock-y += i2c.c bootblock-y += enable_usbdebug.c bootblock-y += monotonic_timer.c -bootblock-y += pmutil.c bootblock-y += tsc_freq.c bootblock-y += southbridge.c bootblock-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c @@ -28,7 +27,6 @@ romstage-y += enable_usbdebug.c romstage-y += gpio.c romstage-y += monotonic_timer.c -romstage-y += pmutil.c romstage-y += smbus_spd.c romstage-y += memmap.c romstage-$(CONFIG_STONEYRIDGE_UART) += uart.c @@ -40,7 +38,6 @@ verstage-y += gpio.c verstage-y += i2c.c verstage-y += monotonic_timer.c -verstage-y += pmutil.c verstage-$(CONFIG_STONEYRIDGE_UART) += uart.c verstage-y += tsc_freq.c
@@ -61,7 +58,6 @@ ramstage-y += monotonic_timer.c ramstage-y += southbridge.c ramstage-y += northbridge.c -ramstage-y += pmutil.c ramstage-y += sata.c ramstage-y += memmap.c ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c diff --git a/src/soc/amd/stoneyridge/pmutil.c b/src/soc/amd/stoneyridge/pmutil.c deleted file mode 100644 index a2ad2db..0000000 --- a/src/soc/amd/stoneyridge/pmutil.c +++ /dev/null @@ -1,14 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <acpi/acpi.h> -#include <soc/southbridge.h> -#include <amdblocks/acpimmio.h> -#include <security/vboot/vboot_common.h> -#include <security/vboot/vbnv.h> -#include <pc80/mc146818rtc.h> - -int vbnv_cmos_failed(void) -{ - /* If CMOS power has failed, the century will be set to 0xff */ - return cmos_read(RTC_CLK_ALTCENTURY) == 0xff; -}
-
Felix Held (Code Review) -
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)