Change in ...coreboot[master]: intel/quark: Fix COMMONLIB_STORAGE in CAR

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31191

Change subject: intel/quark: Fix COMMONLIB_STORAGE in CAR
......................................................................

intel/quark: Fix COMMONLIB_STORAGE in CAR

The allocation is not required before romstage,
so it can be just another CAR_GLOBAL instead of
polluting the linker script.

Change-Id: I0738a655f6cc924fbed92ea630f85406e3f58c0b
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/car.ld
M src/soc/intel/quark/storage_test.c
2 files changed, 7 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/31191/1

diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld
index 44b394e..58961ac 100644
--- a/src/arch/x86/car.ld
+++ b/src/arch/x86/car.ld
@@ -61,11 +61,6 @@
     * to reside in the migrated area (between _car_relocatable_data_start
     * and _car_relocatable_data_end). */
    TIMESTAMP(., 0x200)
-#if IS_ENABLED(CONFIG_COMMONLIB_STORAGE)
-	_car_drivers_storage_start = .;
-	. += 256;
-	_car_drivers_storage_end = .;
-#endif
    _car_ehci_dbg_info_start = .;
    /* Reserve sizeof(struct ehci_dbg_info). */
         . += 0x60;
diff --git a/src/soc/intel/quark/storage_test.c b/src/soc/intel/quark/storage_test.c
index 0a5e22d..d3cd1d3 100644
--- a/src/soc/intel/quark/storage_test.c
+++ b/src/soc/intel/quark/storage_test.c
@@ -33,8 +33,8 @@
 int log_full CAR_GLOBAL;
 long log_start_time CAR_GLOBAL;
 #endif
-extern uint8_t _car_drivers_storage_start;
-extern uint8_t _car_drivers_storage_end;
+
+static uint8_t drivers_storage[256] CAR_GLOBAL;

#define STORAGE_DEBUG  BIOS_DEBUG
 #define LOG_DEBUG  (IS_ENABLED(CONFIG_STORAGE_LOG) ? STORAGE_DEBUG : BIOS_NEVER)
@@ -169,16 +169,15 @@
    unsigned int previous_partition;
    struct sdhci_ctrlr *sdhci_ctrlr;

+	ASSERT(sizeof(struct sdhci_ctrlr) <= sizeof(drivers_storage));
+
    /* Get the structure addresses */
    media = NULL;
    if (ENV_ROMSTAGE)
-		media = car_get_var_ptr(&_car_drivers_storage_start);
+		media = car_get_var_ptr(drivers_storage);
    else
    	media = cbmem_find(CBMEM_ID_STORAGE_DATA);
    sdhci_ctrlr = (void *)(((uintptr_t)(media + 1) + 0x7) & ~7);
-	if (ENV_ROMSTAGE)
-		ASSERT((struct sdhci_ctrlr *)&_car_drivers_storage_end
-			>= (sdhci_ctrlr + 1));
    media->ctrlr = (struct sd_mmc_ctrlr *)sdhci_ctrlr;
    sdhci_ctrlr->ioaddr = (void *)bar;

@@ -247,19 +246,15 @@
 {
    struct storage_media *media;
    struct sdhci_ctrlr *sdhci_ctrlr;
-	size_t size;
+	size_t size = sizeof(drivers_storage);

/* Locate the data structures in CBMEM */
-	size = &_car_drivers_storage_end - &_car_drivers_storage_start;
-	ASSERT(size == 256);
    media = cbmem_add(CBMEM_ID_STORAGE_DATA, size);
    ASSERT(media != NULL);
    sdhci_ctrlr = (void *)(((uintptr_t)(media + 1) + 0x7) & ~7);
-	ASSERT((sdhci_ctrlr + 1)
-		<= (struct sdhci_ctrlr *)&_car_drivers_storage_end);

/* Migrate the data into CBMEM */
-	memcpy(media, &_car_drivers_storage_start, size);
+	memcpy(media, car_get_var_ptr(drivers_storage), size);
    media->ctrlr = &sdhci_ctrlr->sd_mmc_ctrlr;
 }

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31191 )

Change subject: intel/quark: Fix COMMONLIB_STORAGE in CAR
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31191 )

Change subject: intel/quark: Fix COMMONLIB_STORAGE in CAR
......................................................................

intel/quark: Fix COMMONLIB_STORAGE in CAR

The allocation is not required before romstage,
so it can be just another CAR_GLOBAL instead of
polluting the linker script.

Change-Id: I0738a655f6cc924fbed92ea630f85406e3f58c0b
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/31191
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/arch/x86/car.ld
M src/soc/intel/quark/storage_test.c
2 files changed, 7 insertions(+), 17 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld
index 0d0d585..6fe8f14 100644
--- a/src/arch/x86/car.ld
+++ b/src/arch/x86/car.ld
@@ -61,11 +61,6 @@
     * to reside in the migrated area (between _car_relocatable_data_start
     * and _car_relocatable_data_end). */
    TIMESTAMP(., 0x200)
-#if IS_ENABLED(CONFIG_COMMONLIB_STORAGE)
-	_car_drivers_storage_start = .;
-	. += 256;
-	_car_drivers_storage_end = .;
-#endif
    _car_ehci_dbg_info_start = .;
    /* Reserve sizeof(struct ehci_dbg_info). */
         . += 80;
diff --git a/src/soc/intel/quark/storage_test.c b/src/soc/intel/quark/storage_test.c
index 0a5e22d..d3cd1d3 100644
--- a/src/soc/intel/quark/storage_test.c
+++ b/src/soc/intel/quark/storage_test.c
@@ -33,8 +33,8 @@
 int log_full CAR_GLOBAL;
 long log_start_time CAR_GLOBAL;
 #endif
-extern uint8_t _car_drivers_storage_start;
-extern uint8_t _car_drivers_storage_end;
+
+static uint8_t drivers_storage[256] CAR_GLOBAL;

#define STORAGE_DEBUG  BIOS_DEBUG
 #define LOG_DEBUG  (IS_ENABLED(CONFIG_STORAGE_LOG) ? STORAGE_DEBUG : BIOS_NEVER)
@@ -169,16 +169,15 @@
    unsigned int previous_partition;
    struct sdhci_ctrlr *sdhci_ctrlr;

+	ASSERT(sizeof(struct sdhci_ctrlr) <= sizeof(drivers_storage));
+
    /* Get the structure addresses */
    media = NULL;
    if (ENV_ROMSTAGE)
-		media = car_get_var_ptr(&_car_drivers_storage_start);
+		media = car_get_var_ptr(drivers_storage);
    else
    	media = cbmem_find(CBMEM_ID_STORAGE_DATA);
    sdhci_ctrlr = (void *)(((uintptr_t)(media + 1) + 0x7) & ~7);
-	if (ENV_ROMSTAGE)
-		ASSERT((struct sdhci_ctrlr *)&_car_drivers_storage_end
-			>= (sdhci_ctrlr + 1));
    media->ctrlr = (struct sd_mmc_ctrlr *)sdhci_ctrlr;
    sdhci_ctrlr->ioaddr = (void *)bar;

@@ -247,19 +246,15 @@
 {
    struct storage_media *media;
    struct sdhci_ctrlr *sdhci_ctrlr;
-	size_t size;
+	size_t size = sizeof(drivers_storage);

/* Locate the data structures in CBMEM */
-	size = &_car_drivers_storage_end - &_car_drivers_storage_start;
-	ASSERT(size == 256);
    media = cbmem_add(CBMEM_ID_STORAGE_DATA, size);
    ASSERT(media != NULL);
    sdhci_ctrlr = (void *)(((uintptr_t)(media + 1) + 0x7) & ~7);
-	ASSERT((sdhci_ctrlr + 1)
-		<= (struct sdhci_ctrlr *)&_car_drivers_storage_end);

/* Migrate the data into CBMEM */
-	memcpy(media, &_car_drivers_storage_start, size);
+	memcpy(media, car_get_var_ptr(drivers_storage), size);
    media->ctrlr = &sdhci_ctrlr->sd_mmc_ctrlr;
 }