ACPI: Drop redundant CBMEM_ID_ACPI_GNVS allocations

Allocation now happens prior to device enumeration. The
step cbmem_add() is a no-op here, if reached for some
boards. The memset() here is also redundant and becomes
harmful with followup works, as it would wipe out the
CBMEM console and ChromeOS related fields without them
being set again.

Change-Id: I9b2625af15cae90b9c1eb601e606d0430336609f
Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48701
Reviewed-by: Lance Zhao
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Nico Huber <nico.h@gmx.de>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>

src/acpi/Kconfig

@@ -24,6 +24,12 @@ config ACPI_INTEL_HARDWARE_SLEEP_VALUES
 	  Provide common definitions for Intel hardware PM1_CNT register sleep
 	  values.
 
+config ACPI_SOC_NVS
+	bool
+	help
+	  Set to indicate <soc/nvs.h> exists for the platform with a definition
+	  for global_nvs.
+
 config ACPI_NO_PCAT_8259
 	bool
 	help

src/acpi/Makefile.inc

@@ -12,6 +12,7 @@ ramstage-y += acpigen_usb.c
 ramstage-y += device.c
 ramstage-$(CONFIG_CHROMEOS) += chromeos-gnvs.c
 ramstage-y += gnvs.c
+ramstage-$(CONFIG_ACPI_SOC_NVS) += nvs.c
 ramstage-y += pld.c
 ramstage-y += sata.c
 ramstage-y += soundwire.c

src/acpi/gnvs.c

@@ -29,12 +29,7 @@ static void gnvs_assign_cbmc(void)
 		*gnvs_cbmc = (uintptr_t)cbmem_find(CBMEM_ID_CONSOLE);
 }
 
-/* Platforms that implement GNVS will need to implement these. */
-__weak size_t gnvs_size_of_array(void)
-{
-	return 0;
-}
-
+/* Needs implementation in platform code. */
 __weak uint32_t *gnvs_cbmc_ptr(struct global_nvs *gnvs_)
 {
 	return NULL;

src/acpi/nvs.c

@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <acpi/acpi_gnvs.h>
+#include <soc/nvs.h>
+#include <stdint.h>
+
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
+uint32_t *gnvs_cbmc_ptr(struct global_nvs *gnvs)
+{
+	return &gnvs->cbmc;
+}
+
+/* Some <soc/nvs.h> have no chromeos entry. */
+#if CONFIG(MAINBOARD_HAS_CHROMEOS)
+void *gnvs_chromeos_ptr(struct global_nvs *gnvs)
+{
+	return &gnvs->chromeos;
+}
+#endif

src/drivers/amd/agesa/Makefile.inc

@@ -20,6 +20,7 @@ ramstage-y += def_callouts.c
 ramstage-y += eventlog.c
 ramstage-y += heapmanager.c
 ramstage-y += acpi_tables.c
+ramstage-y += nvs.c
 
 romstage-$(CONFIG_CPU_AMD_AGESA) += oem_s3.c
 ramstage-$(CONFIG_CPU_AMD_AGESA) += oem_s3.c s3_mtrr.c

src/drivers/amd/agesa/nvs.c

@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <acpi/acpi_gnvs.h>
+#include <stdint.h>
+
+size_t gnvs_size_of_array(void)
+{
+	return 0;
+}

src/mainboard/protectli/vault_bsw/acpi_tables.c

@@ -8,7 +8,6 @@
 
 void acpi_create_gnvs(struct global_nvs *gnvs)
 {
-	memset(gnvs, 0, sizeof(*gnvs));
 
 	acpi_init_gnvs(gnvs);
 

src/soc/amd/cezanne/Kconfig

@@ -9,6 +9,7 @@ if SOC_AMD_CEZANNE
 
 config SOC_SPECIFIC_OPTIONS
 	def_bool y
+	select ACPI_SOC_NVS
 	select ARCH_BOOTBLOCK_X86_32
 	select ARCH_VERSTAGE_X86_32
 	select ARCH_ROMSTAGE_X86_32

src/soc/amd/common/block/lpc/lpc.c

@@ -100,7 +100,6 @@ static void lpc_init(struct device *dev)
 static void lpc_read_resources(struct device *dev)
 {
 	struct resource *res;
-	struct global_nvs *gnvs;
 
 	/* Get the normal pci resources of this device */
 	pci_dev_read_resources(dev);
@@ -134,10 +133,6 @@ static void lpc_read_resources(struct device *dev)
 	res->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
 
 	compact_resources(dev);
-
-	/* Allocate ACPI NVS in CBMEM */
-	gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-	printk(BIOS_DEBUG, "ACPI GNVS at %p\n", gnvs);
 }
 
 static void lpc_set_resources(struct device *dev)

src/soc/amd/picasso/Kconfig

@@ -17,6 +17,7 @@ config CPU_SPECIFIC_OPTIONS
 	select X86_AMD_FIXED_MTRRS
 	select X86_AMD_INIT_SIPI
 	select ACPI_AMD_HARDWARE_SLEEP_VALUES
+	select ACPI_SOC_NVS
 	select DRIVERS_I2C_DESIGNWARE
 	select DRIVERS_USB_PCI_XHCI
 	select GENERIC_GPIO_LIB

src/soc/amd/picasso/acpi.c

@@ -396,8 +396,6 @@ unsigned long southbridge_write_acpi_tables(const struct device *device,
 
 void acpi_create_gnvs(struct global_nvs *gnvs)
 {
-	/* Clear out GNVS. */
-	memset(gnvs, 0, sizeof(*gnvs));
 
 	if (CONFIG(CONSOLE_CBMEM))
 		gnvs->cbmc = (uintptr_t)cbmem_find(CBMEM_ID_CONSOLE);

src/soc/amd/stoneyridge/Kconfig

@@ -10,6 +10,7 @@ if SOC_AMD_STONEYRIDGE
 config CPU_SPECIFIC_OPTIONS
 	def_bool y
 	select ACPI_AMD_HARDWARE_SLEEP_VALUES
+	select ACPI_SOC_NVS
 	select ARCH_ALL_STAGES_X86_32
 	select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
 	select COLLECT_TIMESTAMPS_NO_TSC

src/soc/amd/stoneyridge/acpi.c

@@ -164,8 +164,6 @@ unsigned long southbridge_write_acpi_tables(const struct device *device,
 
 void acpi_create_gnvs(struct global_nvs *gnvs)
 {
-	/* Clear out GNVS. */
-	memset(gnvs, 0, sizeof(*gnvs));
 
 	if (CONFIG(CONSOLE_CBMEM))
 		gnvs->cbmc = (uintptr_t)cbmem_find(CBMEM_ID_CONSOLE);

src/soc/intel/apollolake/acpi.c

@@ -77,8 +77,6 @@ void acpi_create_gnvs(struct global_nvs *gnvs)
 	struct soc_intel_apollolake_config *cfg;
 	cfg = config_of_soc();
 
-	/* Clear out GNVS. */
-	memset(gnvs, 0, sizeof(*gnvs));
 
 	if (CONFIG(CONSOLE_CBMEM))
 		gnvs->cbmc = (uintptr_t) cbmem_find(CBMEM_ID_CONSOLE);

src/soc/intel/apollolake/chip.c

@@ -28,7 +28,6 @@
 #include <soc/intel/common/vbt.h>
 #include <soc/iomap.h>
 #include <soc/itss.h>
-#include <soc/nvs.h>
 #include <soc/pci_devs.h>
 #include <soc/pm.h>
 #include <soc/systemagent.h>
@@ -318,9 +317,6 @@ static void soc_init(void *data)
 	 */
 	p2sb_unhide();
 
-	/* Allocate ACPI NVS in CBMEM */
-	cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-
 	if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) {
 		printk(BIOS_INFO, "Skip setting RAPL per configuration\n");
 	} else {

src/soc/intel/baytrail/ramstage.c

@@ -2,6 +2,7 @@
 
 #include <arch/cpu.h>
 #include <acpi/acpi.h>
+#include <acpi/acpi_gnvs.h>
 #include <cbmem.h>
 #include <console/console.h>
 #include <cpu/intel/microcode.h>
@@ -143,15 +144,9 @@ static void s3_save_acpi_wake_source(struct global_nvs *gnvs)
 
 static void s3_resume_prepare(void)
 {
-	struct global_nvs *gnvs;
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
-	gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-	if (gnvs == NULL)
-		return;
-
-	if (!acpi_is_wakeup_s3())
-		memset(gnvs, 0, sizeof(struct global_nvs));
-	else
+	if (gnvs && acpi_is_wakeup_s3())
 		s3_save_acpi_wake_source(gnvs);
 }
 

src/soc/intel/baytrail/southcluster.c

@@ -489,11 +489,6 @@ static void southcluster_inject_dsdt(const struct device *device)
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);

src/soc/intel/braswell/acpi.c

@@ -382,11 +382,6 @@ void southcluster_inject_dsdt(const struct device *device)
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);

src/soc/intel/braswell/ramstage.c

@@ -2,6 +2,7 @@
 
 #include <arch/cpu.h>
 #include <acpi/acpi.h>
+#include <acpi/acpi_gnvs.h>
 #include <cbmem.h>
 #include <console/console.h>
 #include <cpu/intel/microcode.h>
@@ -135,24 +136,11 @@ int soc_fill_acpi_wake(uint32_t *pm1, uint32_t **gpe0)
 	return 1;
 }
 
-static void s3_resume_prepare(void)
-{
-	struct global_nvs *gnvs;
-
-	gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-	if (!acpi_is_wakeup_s3() && gnvs)
-		memset(gnvs, 0, sizeof(struct global_nvs));
-}
-
 static void set_board_id(void)
 {
-	struct global_nvs *gnvs;
-
-	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		printk(BIOS_ERR, "Unable to locate Global NVS\n");
+	struct global_nvs *gnvs = acpi_get_gnvs();
+	if (!gnvs)
 		return;
-	}
 	gnvs->bdid = board_id();
 }
 
@@ -165,9 +153,6 @@ void soc_init_pre_device(struct soc_intel_braswell_config *config)
 	/* Allow for SSE instructions to be executed. */
 	write_cr4(read_cr4() | CR4_OSFXSR | CR4_OSXMMEXCPT);
 
-	/* Indicate S3 resume to rest of ramstage. */
-	s3_resume_prepare();
-
 	/* Perform silicon specific init. */
 	intel_silicon_init();
 	set_max_freq();

src/soc/intel/broadwell/pch/lpc.c

@@ -593,8 +593,6 @@ static void pch_lpc_add_io_resources(struct device *dev)
 
 static void pch_lpc_read_resources(struct device *dev)
 {
-	struct global_nvs *gnvs;
-
 	/* Get the normal PCI resources of this device. */
 	pci_dev_read_resources(dev);
 
@@ -603,11 +601,6 @@ static void pch_lpc_read_resources(struct device *dev)
 
 	/* Add IO resources. */
 	pch_lpc_add_io_resources(dev);
-
-	/* Allocate ACPI NVS in CBMEM */
-	gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-	if (!acpi_is_wakeup_s3() && gnvs)
-		memset(gnvs, 0, sizeof(struct global_nvs));
 }
 
 static void southcluster_inject_dsdt(const struct device *device)
@@ -615,11 +608,6 @@ static void southcluster_inject_dsdt(const struct device *device)
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);

src/soc/intel/broadwell/ramstage.c

@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
 #include <acpi/acpi.h>
+#include <acpi/acpi_gnvs.h>
 #include <cbmem.h>
 #include <console/console.h>
 #include <device/device.h>
@@ -63,15 +64,9 @@ static void save_acpi_wake_source(struct global_nvs *gnvs)
 
 static void s3_resume_prepare(void)
 {
-	struct global_nvs *gnvs;
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
-	gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-	if (gnvs == NULL)
-		return;
-
-	if (!acpi_is_wakeup_s3())
-		memset(gnvs, 0, sizeof(struct global_nvs));
-	else
+	if (gnvs && acpi_is_wakeup_s3())
 		save_acpi_wake_source(gnvs);
 }
 

src/soc/intel/common/Kconfig.common

@@ -2,6 +2,7 @@ config SOC_INTEL_COMMON
 	bool
 	select AZALIA_PLUGIN_SUPPORT
 	select HAVE_DISPLAY_MTRRS
+	select ACPI_SOC_NVS
 	help
 	  common code for Intel SOCs
 

src/soc/intel/common/block/acpi/acpi.c

@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
 
+#include <acpi/acpi_gnvs.h>
 #include <acpi/acpigen.h>
 #include <arch/cpu.h>
 #include <arch/ioapic.h>
@@ -7,7 +8,6 @@
 #include <bootstate.h>
 #include <cbmem.h>
 #include <cf9_reset.h>
-#include <acpi/acpi_gnvs.h>
 #include <console/console.h>
 #include <cpu/intel/turbo.h>
 #include <cpu/intel/common/common.h>
@@ -244,11 +244,6 @@ void southbridge_inject_dsdt(const struct device *device)
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);

src/soc/intel/denverton_ns/acpi.c

@@ -251,11 +251,6 @@ void southcluster_inject_dsdt(const struct device *device)
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);

src/soc/intel/quark/include/soc/nvs.h

@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef SOC_INTEL_QUARK_NVS_H
+#define SOC_INTEL_QUARK_NVS_H
+
+#include <stdint.h>
+
+struct __packed global_nvs {
+	uint32_t	cbmc; /* 0x00 - 0x03 - coreboot Memory Console */
+};
+
+#endif /* SOC_INTEL_QUARK_NVS_H */

src/soc/intel/skylake/acpi.c

@@ -545,11 +545,6 @@ void southbridge_inject_dsdt(const struct device *device)
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);

src/southbridge/intel/bd82x6x/lpc.c

@@ -642,12 +642,16 @@ static void pch_lpc_enable(struct device *dev)
 	pch_enable(dev);
 }
 
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
 void southbridge_inject_dsdt(const struct device *dev)
 {
-	struct global_nvs *gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
 	if (gnvs) {
-		memset(gnvs, 0, sizeof(*gnvs));
 
 		acpi_create_gnvs(gnvs);
 

src/southbridge/intel/i82371eb/acpi_tables.c

@@ -2,6 +2,7 @@
 
 #include <console/console.h>
 #include <acpi/acpi.h>
+#include <acpi/acpi_gnvs.h>
 #include <acpi/acpigen.h>
 #include <device/device.h>
 #include "i82371eb.h"
@@ -44,3 +45,8 @@ unsigned long acpi_fill_mcfg(unsigned long current)
 	/* chipset doesn't have mmconfig */
 	return current;
 }
+
+size_t gnvs_size_of_array(void)
+{
+	return 0;
+}

src/southbridge/intel/i82801dx/fadt.c

@@ -2,6 +2,7 @@
 
 #include <device/pci_ops.h>
 #include <acpi/acpi.h>
+#include <acpi/acpi_gnvs.h>
 #include <version.h>
 
 /* FIXME: This needs to go into a separate .h file
@@ -79,3 +80,8 @@ void acpi_fill_fadt(acpi_fadt_t *fadt)
 	fadt->x_gpe0_blk.addrl = pmbase + 0x28;
 	fadt->x_gpe0_blk.addrh = 0x0;
 }
+
+size_t gnvs_size_of_array(void)
+{
+	return 0;
+}

src/southbridge/intel/i82801gx/lpc.c

@@ -479,12 +479,16 @@ static void lpc_final(struct device *dev)
 	outb(POST_OS_BOOT, 0x80);
 }
 
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
 void southbridge_inject_dsdt(const struct device *dev)
 {
-	struct global_nvs *gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
 	if (gnvs) {
-		memset(gnvs, 0, sizeof(*gnvs));
 
 		gnvs->apic = 1;
 		gnvs->mpen = 1; /* Enable Multi Processing */

src/southbridge/intel/i82801ix/lpc.c

@@ -453,12 +453,16 @@ static void i82801ix_lpc_read_resources(struct device *dev)
 	res->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
 }
 
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
 void southbridge_inject_dsdt(const struct device *dev)
 {
-	struct global_nvs *gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
 	if (gnvs) {
-		memset(gnvs, 0, sizeof(*gnvs));
 		acpi_create_gnvs(gnvs);
 
 

src/southbridge/intel/i82801jx/lpc.c

@@ -477,12 +477,16 @@ static void i82801jx_lpc_read_resources(struct device *dev)
 	}
 }
 
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
 void southbridge_inject_dsdt(const struct device *dev)
 {
-	struct global_nvs *gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
 	if (gnvs) {
-		memset(gnvs, 0, sizeof(*gnvs));
 		acpi_create_gnvs(gnvs);
 
 

src/southbridge/intel/ibexpeak/lpc.c

@@ -542,12 +542,16 @@ static void pch_lpc_enable(struct device *dev)
 	pch_enable(dev);
 }
 
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
 void southbridge_inject_dsdt(const struct device *dev)
 {
-	struct global_nvs *gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
+	struct global_nvs *gnvs = acpi_get_gnvs();
 
 	if (gnvs) {
-		memset(gnvs, 0, sizeof(*gnvs));
 
 		acpi_create_gnvs(gnvs);
 

src/southbridge/intel/lynxpoint/lpc.c

@@ -661,8 +661,6 @@ static void pch_lpc_add_io_resources(struct device *dev)
 
 static void pch_lpc_read_resources(struct device *dev)
 {
-	struct global_nvs *gnvs;
-
 	/* Get the normal PCI resources of this device. */
 	pci_dev_read_resources(dev);
 
@@ -671,11 +669,6 @@ static void pch_lpc_read_resources(struct device *dev)
 
 	/* Add IO resources. */
 	pch_lpc_add_io_resources(dev);
-
-	/* Allocate ACPI NVS in CBMEM */
-	gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs));
-	if (!acpi_is_wakeup_s3() && gnvs)
-		memset(gnvs, 0, sizeof(struct global_nvs));
 }
 
 static void pch_lpc_enable(struct device *dev)
@@ -687,16 +680,16 @@ static void pch_lpc_enable(struct device *dev)
 	pch_enable(dev);
 }
 
+size_t gnvs_size_of_array(void)
+{
+	return sizeof(struct global_nvs);
+}
+
 void southbridge_inject_dsdt(const struct device *dev)
 {
 	struct global_nvs *gnvs;
 
 	gnvs = cbmem_find(CBMEM_ID_ACPI_GNVS);
-	if (!gnvs) {
-		gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs));
-		if (gnvs)
-			memset(gnvs, 0, sizeof(*gnvs));
-	}
 
 	if (gnvs) {
 		acpi_create_gnvs(gnvs);