Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30506
Change subject: [TEST]sb/intel/common: Remove CAR_GLOBAL use ......................................................................
[TEST]sb/intel/common: Remove CAR_GLOBAL use
We have NO_CAR_GLOBAL_MIGRATION now. (yes?)
Change-Id: Ic2c90d264d851ab4abeca07f412d43d088ad96dc Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/common/gpio.c M src/southbridge/intel/common/pmbase.c M src/southbridge/intel/common/spi.c 3 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/30506/1
diff --git a/src/southbridge/intel/common/gpio.c b/src/southbridge/intel/common/gpio.c index 7c8cfe8..140839f 100644 --- a/src/southbridge/intel/common/gpio.c +++ b/src/southbridge/intel/common/gpio.c @@ -40,7 +40,7 @@ /* Don't assume GPIO_BASE is still the same */ return pci_read_config16(PCH_LPC_DEV, GPIO_BASE) & 0xfffe; #else - static u16 gpiobase CAR_GLOBAL; + static u16 gpiobase;
if (gpiobase) return gpiobase; diff --git a/src/southbridge/intel/common/pmbase.c b/src/southbridge/intel/common/pmbase.c index 2de57d6..b022629 100644 --- a/src/southbridge/intel/common/pmbase.c +++ b/src/southbridge/intel/common/pmbase.c @@ -42,7 +42,7 @@ /* Don't assume PMBASE is still the same */ return pci_read_config16(PCH_LPC_DEV, PMBASE) & 0xfffc; #else - static u16 pmbase CAR_GLOBAL; + static u16 pmbase;
if (pmbase) return pmbase; diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index 3ca0d6c..6ef6769 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -69,7 +69,7 @@
typedef struct spi_slave ich_spi_slave;
-static int g_ichspi_lock CAR_GLOBAL = 0; +static int g_ichspi_lock = 0;
typedef struct ich7_spi_regs { uint16_t spis; @@ -138,7 +138,7 @@ uint8_t fpr_max; } ich_spi_controller;
-static ich_spi_controller g_cntlr CAR_GLOBAL; +static ich_spi_controller g_cntlr;
enum { SPIS_SCIP = 0x0001,
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30506
to look at the new patch set (#5).
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
sb/intel/common: Remove CAR_GLOBAL use
We have NO_CAR_GLOBAL_MIGRATION now.
Change-Id: Ic2c90d264d851ab4abeca07f412d43d088ad96dc Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/common/gpio.c M src/southbridge/intel/common/pmbase.c M src/southbridge/intel/common/spi.c 3 files changed, 23 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/30506/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30506 )
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
Patch Set 6: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/30506/6/src/southbridge/intel/common/gpio.c File src/southbridge/intel/common/gpio.c:
https://review.coreboot.org/#/c/30506/6/src/southbridge/intel/common/gpio.c@... PS6, Line 44: if (gpiobase) At some point before POSTCAR_STAGE=y, this would have failed without car_get_var() stuff around it. But probably romstage after raminit did not play with GPIOs so we never caught the error.
https://review.coreboot.org/#/c/30506/6/src/southbridge/intel/common/pmbase.... File src/southbridge/intel/common/pmbase.c:
https://review.coreboot.org/#/c/30506/6/src/southbridge/intel/common/pmbase.... PS6, Line 44: static u16 pmbase; Like previous file.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30506 )
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
sb/intel/common: Remove CAR_GLOBAL use
We have NO_CAR_GLOBAL_MIGRATION now.
Change-Id: Ic2c90d264d851ab4abeca07f412d43d088ad96dc Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/30506 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/intel/common/gpio.c M src/southbridge/intel/common/pmbase.c M src/southbridge/intel/common/spi.c 3 files changed, 23 insertions(+), 26 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/southbridge/intel/common/gpio.c b/src/southbridge/intel/common/gpio.c index 30c5028..8a511c3 100644 --- a/src/southbridge/intel/common/gpio.c +++ b/src/southbridge/intel/common/gpio.c @@ -18,7 +18,6 @@ #include <arch/io.h> #include <device/device.h> #include <device/pci.h> -#include <arch/early_variables.h>
#include "gpio.h"
@@ -40,7 +39,7 @@ /* Don't assume GPIO_BASE is still the same */ return pci_read_config16(PCH_LPC_DEV, GPIO_BASE) & 0xfffe; #else - static u16 gpiobase CAR_GLOBAL; + static u16 gpiobase;
if (gpiobase) return gpiobase; diff --git a/src/southbridge/intel/common/pmbase.c b/src/southbridge/intel/common/pmbase.c index 8b3274f..563856c 100644 --- a/src/southbridge/intel/common/pmbase.c +++ b/src/southbridge/intel/common/pmbase.c @@ -18,7 +18,6 @@ #include <arch/io.h> #include <device/device.h> #include <device/pci.h> -#include <arch/early_variables.h> #include <assert.h> #include <security/vboot/vboot_common.h>
@@ -42,7 +41,7 @@ /* Don't assume PMBASE is still the same */ return pci_read_config16(PCH_LPC_DEV, PMBASE) & 0xfffc; #else - static u16 pmbase CAR_GLOBAL; + static u16 pmbase;
if (pmbase) return pmbase; diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index 4c59399..60c0b8d 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -16,7 +16,6 @@ */
/* This file is derived from the flashrom project. */ -#include <arch/early_variables.h> #include <stdint.h> #include <stdlib.h> #include <string.h> @@ -69,7 +68,7 @@
typedef struct spi_slave ich_spi_slave;
-static int g_ichspi_lock CAR_GLOBAL = 0; +static int g_ichspi_lock = 0;
typedef struct ich7_spi_regs { uint16_t spis; @@ -138,7 +137,7 @@ uint8_t fpr_max; } ich_spi_controller;
-static ich_spi_controller g_cntlr CAR_GLOBAL; +static ich_spi_controller g_cntlr;
enum { SPIS_SCIP = 0x0001, @@ -283,7 +282,7 @@
static void ich_set_bbar(uint32_t minaddr) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; const uint32_t bbar_mask = 0x00ffff00; uint32_t ichspi_bbar;
@@ -295,7 +294,7 @@
void spi_init(void) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint8_t *rcrb; /* Root Complex Register Block */ uint32_t rcba; /* Root Complex Base Address */ uint8_t bios_cntl; @@ -321,7 +320,7 @@ cntlr->data = (uint8_t *)ich7_spi->spid; cntlr->databytes = sizeof(ich7_spi->spid); cntlr->status = (uint8_t *)&ich7_spi->spis; - car_set_var(g_ichspi_lock, readw_(&ich7_spi->spis) & HSFS_FLOCKDN); + g_ichspi_lock = readw_(&ich7_spi->spis) & HSFS_FLOCKDN; cntlr->control = &ich7_spi->spic; cntlr->bbar = &ich7_spi->bbar; cntlr->preop = &ich7_spi->preop; @@ -331,7 +330,7 @@ ich9_spi = (ich9_spi_regs *)(rcrb + 0x3800); cntlr->ich9_spi = ich9_spi; hsfs = readw_(&ich9_spi->hsfs); - car_set_var(g_ichspi_lock, hsfs & HSFS_FLOCKDN); + g_ichspi_lock = hsfs & HSFS_FLOCKDN; cntlr->hsfs = hsfs; cntlr->opmenu = ich9_spi->opmenu; cntlr->menubytes = sizeof(ich9_spi->opmenu); @@ -428,13 +427,13 @@
static int spi_setup_opcode(spi_transaction *trans) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint16_t optypes; uint8_t opmenu[cntlr->menubytes];
trans->opcode = trans->out[0]; spi_use_out(trans, 1); - if (!car_get_var(g_ichspi_lock)) { + if (!g_ichspi_lock) { /* The lock is off, so just use index 0. */ writeb_(trans->opcode, cntlr->opmenu); optypes = readw_(cntlr->optype); @@ -509,7 +508,7 @@ */ static int ich_status_poll(u16 bitmask, int wait_til_set) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; int timeout = 600000; /* This will result in 6 seconds */ u16 status = 0;
@@ -530,7 +529,7 @@
static int spi_is_multichip(void) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; if (!(cntlr->hsfs & HSFS_FDV)) return 0; return !!((cntlr->flmap0 >> 8) & 3); @@ -539,7 +538,7 @@ static int spi_ctrlr_xfer(const struct spi_slave *slave, const void *dout, size_t bytesout, void *din, size_t bytesin) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint16_t control; int16_t opcode_index; int with_address; @@ -579,7 +578,7 @@ * in order to prevent the Management Engine from * issuing a transaction between WREN and DATA. */ - if (!car_get_var(g_ichspi_lock)) + if (!g_ichspi_lock) writew_(trans.opcode, cntlr->preop); return 0; } @@ -689,7 +688,7 @@ /* Sets FLA in FADDR to (addr & 0x01FFFFFF) without touching other bits. */ static void ich_hwseq_set_addr(uint32_t addr) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint32_t addr_old = readl_(&cntlr->ich9_spi->faddr) & ~0x01FFFFFF;
writel_((addr & 0x01FFFFFF) | addr_old, &cntlr->ich9_spi->faddr); @@ -702,7 +701,7 @@ static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout, unsigned int len) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint16_t hsfs; uint32_t addr;
@@ -742,7 +741,7 @@ static int ich_hwseq_erase(const struct spi_flash *flash, u32 offset, size_t len) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; u32 start, end, erase_size; int ret; uint16_t hsfc; @@ -792,7 +791,7 @@
static void ich_read_data(uint8_t *data, int len) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; int i; uint32_t temp32 = 0;
@@ -807,7 +806,7 @@ static int ich_hwseq_read(const struct spi_flash *flash, u32 addr, size_t len, void *buf) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint16_t hsfc; uint16_t timeout = 100 * 60; uint8_t block_len; @@ -853,7 +852,7 @@ */ static void ich_fill_data(const uint8_t *data, int len) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint32_t temp32 = 0; int i;
@@ -877,7 +876,7 @@ static int ich_hwseq_write(const struct spi_flash *flash, u32 addr, size_t len, const void *buf) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; uint16_t hsfc; uint16_t timeout = 100 * 60; uint8_t block_len; @@ -933,7 +932,7 @@ static int spi_flash_programmer_probe(const struct spi_slave *spi, struct spi_flash *flash) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr;
if (IS_ENABLED(CONFIG_SOUTHBRIDGE_INTEL_I82801GX)) return spi_flash_generic_probe(spi, flash); @@ -1013,7 +1012,7 @@ const struct region *region, const enum ctrlr_prot_type type) { - ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); + ich_spi_controller *cntlr = &g_cntlr; u32 start = region_offset(region); u32 end = start + region_sz(region) - 1; u32 reg;
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30506 )
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c@7... PS7, Line 71: static int g_ichspi_lock = 0 Well, that was a bad idea and I have overseen this patch. The issue now is, that this driver cannot be used in romsatge as in romstage simple global variables are not allowed!
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30506 )
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c@7... PS7, Line 71: static int g_ichspi_lock = 0
Well, that was a bad idea and I have overseen this patch. […]
Which board?
CAR_GLOBAL became no-op after boards moved to POSTCAR_STAGE; we are allowed to get rid of all car_get/set_var().
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30506 )
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c@7... PS7, Line 71: static int g_ichspi_lock = 0
Which board? […]
I was about enabling it on fsp_broadwell_de (mc_bdx1). This will be done as soon as the measured boot patches are ready. In this particular caseit was not a matter of CAR migration but about haveing global variables in romstage.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30506 )
Change subject: sb/intel/common: Remove CAR_GLOBAL use ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c File src/southbridge/intel/common/spi.c:
https://review.coreboot.org/#/c/30506/7/src/southbridge/intel/common/spi.c@7... PS7, Line 71: static int g_ichspi_lock = 0
I was about enabling it on fsp_broadwell_de (mc_bdx1). […]
Having CAR migration was _the_ reason for forbidden global variables in romstage. Do you user or need this driver in romstage before you have POSTCAR_STAGE=y?
https://review.coreboot.org/c/coreboot/+/29359
-
Arthur Heymans (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Werner Zeh (Code Review)