Change in flashrom[master]: sb600spi.c: Don't access spibar directly - flashrom-gerrit

29 Oct 2019

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/36433 )
Change subject: sb600spi.c: Don't access spibar directly
......................................................................
sb600spi.c: Don't access spibar directly
Constrain the manner in which the global state of spibar is
set and then subsequently accessed. This makes it easier to
write unit-tests and instrument the code.
Change-Id: Ic26372b9c1baebb20716eea1db1e942239ed3e48
Signed-off-by: Edward O'Callaghan quasisec@chromium.org
---
M sb600spi.c
1 file changed, 53 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/36433/1

diff --git a/sb600spi.c b/sb600spi.c
index 23b36ee..7bdd11f 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -40,7 +40,13 @@
  *};
  */
-static uint8_t *sb600_spibar = NULL;
+static uint8_t *g_sb600_spibar = NULL;
+
+static inline uint8_t * get_spibar(void)
+{
+	return g_sb600_spibar;
+}
+
 enum amd_chipset {
    CHIPSET_AMD_UNKNOWN,
    CHIPSET_SB6XX,
@@ -180,16 +186,18 @@
static void reset_internal_fifo_pointer(void)
 {
-	mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);
+	uint8_t *spibar = get_spibar();
+	mmio_writeb(mmio_readb(spibar + 2) | 0x10, spibar + 2);
/* FIXME: This loop needs a timeout and a clearer message. */
-	while (mmio_readb(sb600_spibar + 0xD) & 0x7)
+	while (mmio_readb(spibar + 0xD) & 0x7)
    	msg_pspew("reset\n");
 }
static int compare_internal_fifo_pointer(uint8_t want)
 {
-	uint8_t have = mmio_readb(sb600_spibar + 0xd) & 0x07;
+	uint8_t *spibar = get_spibar();
+	uint8_t have = mmio_readb(spibar + 0xd) & 0x07;
    want %= FIFO_SIZE_OLD;
    if (have != want) {
    	msg_perr("AMD SPI FIFO pointer corruption! Pointer is %d, wanted %d\n", have, want);
@@ -224,8 +232,9 @@
 static void execute_command(void)
 {
    msg_pspew("Executing... ");
-	mmio_writeb(mmio_readb(sb600_spibar + 2) | 1, sb600_spibar + 2);
-	while (mmio_readb(sb600_spibar + 2) & 1)
+	uint8_t *spibar = get_spibar();
+	mmio_writeb(mmio_readb(spibar + 2) | 1, spibar + 2);
+	while (mmio_readb(spibar + 2) & 1)
    	;
    msg_pspew("done\n");
 }
@@ -239,7 +248,8 @@
    unsigned char cmd = *writearr++;
    writecnt--;
    msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt);
-	mmio_writeb(cmd, sb600_spibar + 0);
+	uint8_t *spibar = get_spibar();
+	mmio_writeb(cmd, spibar + 0);
int ret = check_readwritecnt(flash, writecnt, readcnt);
    if (ret != 0)
@@ -253,14 +263,14 @@
     */
    unsigned int readoffby1 = (writecnt > 0) ? 0 : 1;
    uint8_t readwrite = (readcnt + readoffby1) << 4 | (writecnt);
-	mmio_writeb(readwrite, sb600_spibar + 1);
+	mmio_writeb(readwrite, spibar + 1);
reset_internal_fifo_pointer();
    msg_pspew("Filling FIFO: ");
    unsigned int count;
    for (count = 0; count < writecnt; count++) {
    	msg_pspew("[%02x]", writearr[count]);
-		mmio_writeb(writearr[count], sb600_spibar + 0xC);
+		mmio_writeb(writearr[count], spibar + 0xC);
    }
    msg_pspew("\n");
    if (compare_internal_fifo_pointer(writecnt))
@@ -291,7 +301,7 @@
    /* Skip the bytes we sent. */
    msg_pspew("Skipping: ");
    for (count = 0; count < writecnt; count++) {
-		msg_pspew("[%02x]", mmio_readb(sb600_spibar + 0xC));
+		msg_pspew("[%02x]", mmio_readb(spibar + 0xC));
    }
    msg_pspew("\n");
    if (compare_internal_fifo_pointer(writecnt))
@@ -299,14 +309,14 @@
msg_pspew("Reading FIFO: ");
    for (count = 0; count < readcnt; count++) {
-		readarr[count] = mmio_readb(sb600_spibar + 0xC);
+		readarr[count] = mmio_readb(spibar + 0xC);
    	msg_pspew("[%02x]", readarr[count]);
    }
    msg_pspew("\n");
    if (compare_internal_fifo_pointer(writecnt+readcnt))
    	return SPI_PROGRAMMER_ERROR;
-	if (mmio_readb(sb600_spibar + 1) != readwrite) {
+	if (mmio_readb(spibar + 1) != readwrite) {
    	msg_perr("Unexpected change in AMD SPI read/write count!\n");
    	msg_perr("Something else is accessing the flash chip and causes random corruption.\n"
    		 "Please stop all applications and drivers and IPMI which access the flash chip.\n");
@@ -325,21 +335,22 @@
    unsigned char cmd = *writearr++;
    writecnt--;
    msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt);
-	mmio_writeb(cmd, sb600_spibar + 0);
+	uint8_t *spibar = get_spibar();
+	mmio_writeb(cmd, spibar + 0);
int ret = check_readwritecnt(flash, writecnt, readcnt);
    if (ret != 0)
    	return ret;
/* Use the extended TxByteCount and RxByteCount registers. */
-	mmio_writeb(writecnt, sb600_spibar + 0x48);
-	mmio_writeb(readcnt, sb600_spibar + 0x4b);
+	mmio_writeb(writecnt, spibar + 0x48);
+	mmio_writeb(readcnt, spibar + 0x4b);
msg_pspew("Filling buffer: ");
    unsigned int count;
    for (count = 0; count < writecnt; count++) {
    	msg_pspew("[%02x]", writearr[count]);
-		mmio_writeb(writearr[count], sb600_spibar + 0x80 + count);
+		mmio_writeb(writearr[count], spibar + 0x80 + count);
    }
    msg_pspew("\n");
@@ -347,7 +358,7 @@
msg_pspew("Reading buffer: ");
    for (count = 0; count < readcnt; count++) {
-		readarr[count] = mmio_readb(sb600_spibar + 0x80 + (writecnt + count) % FIFO_SIZE_YANGTZE);
+		readarr[count] = mmio_readb(spibar + 0x80 + (writecnt + count) % FIFO_SIZE_YANGTZE);
    	msg_pspew("[%02x]", readarr[count]);
    }
    msg_pspew("\n");
@@ -375,16 +386,17 @@
 {
    bool success = false;
    uint8_t speed = spispeed->speed;
+	uint8_t *spibar = get_spibar();
msg_pdbg("Setting SPI clock to %s (0x%x).\n", spispeed->name, speed);
    if (amd_gen >= CHIPSET_YANGTZE) {
-		rmmio_writew((speed << 12) | (speed << 8) | (speed << 4) | speed, sb600_spibar + 0x22);
-		uint16_t tmp = mmio_readw(sb600_spibar + 0x22);
+		rmmio_writew((speed << 12) | (speed << 8) | (speed << 4) | speed, spibar + 0x22);
+		uint16_t tmp = mmio_readw(spibar + 0x22);
    	success = (((tmp >> 12) & 0xf) == speed && ((tmp >> 8) & 0xf) == speed &&
    		   ((tmp >> 4) & 0xf) == speed && ((tmp >> 0) & 0xf) == speed);
    } else {
-		rmmio_writeb((mmio_readb(sb600_spibar + 0xd) & ~(0x3 << 4)) | (speed << 4), sb600_spibar + 0xd);
-		success = (speed == ((mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3));
+		rmmio_writeb((mmio_readb(spibar + 0xd) & ~(0x3 << 4)) | (speed << 4), spibar + 0xd);
+		success = (speed == ((mmio_readb(spibar + 0xd) >> 4) & 0x3));
    }
if (!success) {
@@ -396,11 +408,12 @@
static int set_mode(struct pci_dev *dev, uint8_t read_mode)
 {
-	uint32_t tmp = mmio_readl(sb600_spibar + 0x00);
+	uint8_t *spibar = get_spibar();
+	uint32_t tmp = mmio_readl(spibar + 0x00);
    tmp &= ~(0x6 << 28 | 0x1 << 18); /* Clear mode bits */
    tmp |= ((read_mode & 0x6) << 28) | ((read_mode & 0x1) << 18);
-	rmmio_writel(tmp, sb600_spibar + 0x00);
-	if (tmp != mmio_readl(sb600_spibar + 0x00))
+	rmmio_writel(tmp, spibar + 0x00);
+	if (tmp != mmio_readl(spibar + 0x00))
    	return 1;
    return 0;
 }
@@ -409,6 +422,7 @@
 {
    uint32_t tmp;
    uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */
+	uint8_t *spibar = get_spibar();
char *spispeed = extract_programmer_param("spispeed");
    if (spispeed != NULL) {
@@ -450,7 +464,7 @@
    		"Normal (up to 66 MHz)", /* 6 */
    		"Fast Read",		 /* 7 (Not defined in the Bolton datasheet.) */
    	};
-		tmp = mmio_readl(sb600_spibar + 0x00);
+		tmp = mmio_readl(spibar + 0x00);
    	uint8_t read_mode = ((tmp >> 28) & 0x6) | ((tmp >> 18) & 0x1);
    	msg_pdbg("SpiReadMode=%s (%i)\n", spireadmodes[read_mode], read_mode);
    	if (read_mode != 6) {
@@ -463,11 +477,11 @@
    	}
if (amd_gen >= CHIPSET_YANGTZE) {
-			tmp = mmio_readb(sb600_spibar + 0x20);
+			tmp = mmio_readb(spibar + 0x20);
    		msg_pdbg("UseSpi100 is %sabled\n", (tmp & 0x1) ? "en" : "dis");
    		if ((tmp & 0x1) == 0) {
-				rmmio_writeb(tmp | 0x1, sb600_spibar + 0x20);
-				tmp = mmio_readb(sb600_spibar + 0x20) & 0x1;
+				rmmio_writeb(tmp | 0x1, spibar + 0x20);
+				tmp = mmio_readb(spibar + 0x20) & 0x1;
    			if (tmp == 0) {
    				msg_perr("Enabling Spi100 failed.\n");
    				return 1;
@@ -475,7 +489,7 @@
    			msg_pdbg("Enabling Spi100 succeeded.\n");
    		}
-			tmp = mmio_readw(sb600_spibar + 0x22); /* SPI 100 Speed Config */
+			tmp = mmio_readw(spibar + 0x22); /* SPI 100 Speed Config */
    		msg_pdbg("NormSpeedNew is %s\n", spispeeds[(tmp >> 12) & 0xf].name);
    		msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf].name);
    		msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf].name);
@@ -483,15 +497,15 @@
    	}
    } else {
    	if (amd_gen >= CHIPSET_SB89XX && amd_gen <= CHIPSET_HUDSON234) {
-			bool fast_read = (mmio_readl(sb600_spibar + 0x00) >> 18) & 0x1;
+			bool fast_read = (mmio_readl(spibar + 0x00) >> 18) & 0x1;
    		msg_pdbg("Fast Reads are %sabled\n", fast_read ? "en" : "dis");
    		if (fast_read) {
    			msg_pdbg("Disabling them temporarily.\n");
-				rmmio_writel(mmio_readl(sb600_spibar + 0x00) & ~(0x1 << 18),
-					     sb600_spibar + 0x00);
+				rmmio_writel(mmio_readl(spibar + 0x00) & ~(0x1 << 18),
+					     spibar + 0x00);
    		}
    	}
-		tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3;
+		tmp = (mmio_readb(spibar + 0xd) >> 4) & 0x3;
    	msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name);
    }
    return set_speed(dev, amd_gen, &spispeeds[spispeed_idx]);
@@ -581,14 +595,14 @@
    	return 0;
/* Physical memory has to be mapped at page (4k) boundaries. */
-	sb600_spibar = rphysmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000);
-	if (sb600_spibar == ERROR_PTR)
+	g_sb600_spibar = rphysmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000);
+	if (g_sb600_spibar == ERROR_PTR)
    	return ERROR_FATAL;
/* The low bits of the SPI base address are used as offset into
     * the mapped page.
     */
-	sb600_spibar += tmp & 0xfff;
+	g_sb600_spibar += tmp & 0xfff;
int amd_gen = determine_generation(dev);
    if (amd_gen < 0)
@@ -649,7 +663,8 @@
     *
     *  <1> see handle_speed
     */
-	tmp = mmio_readl(sb600_spibar + 0x00);
+	uint8_t *spibar = get_spibar();
+	tmp = mmio_readl(spibar + 0x00);
    msg_pdbg("(0x%08" PRIx32 ") SpiArbEnable=%i", tmp, (tmp >> 19) & 0x1);
    if (amd_gen >= CHIPSET_YANGTZE)
    	msg_pdbg(", IllegalAccess=%i", (tmp >> 21) & 0x1);
@@ -679,7 +694,7 @@
    }
if (amd_gen >= CHIPSET_SB89XX) {
-		tmp = mmio_readb(sb600_spibar + 0x1D);
+		tmp = mmio_readb(spibar + 0x1D);
    	msg_pdbg("Using SPI_CS%d\n", tmp & 0x3);
    	/* FIXME: Handle SpiProtect* configuration on Yangtze. */
    }
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36433
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic26372b9c1baebb20716eea1db1e942239ed3e48
Gerrit-Change-Number: 36433
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org
Gerrit-MessageType: newchange



    