[flashrom-gerrit] Change in flashrom[master]: Add timeout check to SPI bitbang bus request

[Nico Huber (Code Review)]

Hello Carl-Daniel Hailfinger,

I'd like you to do a code review. Please visit

https://review.coreboot.org/25097

to review the following change.

Change subject: Add timeout check to SPI bitbang bus request
......................................................................

Add timeout check to SPI bitbang bus request

Am 05.10.2010 22:42 schrieb Iain Paton:
> > > root@p7fe-64:~/flashrom# ./flashrom -p nicintel_spi -V
> > > flashrom v0.9.2-r1186 on Linux 2.6.35-dt (x86_64), built with libpci
> > > 3.1.4, GCC 4.4.3, little endian
> > > Initializing nicintel_spi programmer
> > > Found "Intel 82540EM Gigabit Ethernet Controller" (8086:100e, BDF
> > > 06:02.0).
>
> I have a few of these cards, so maybe I'll try a different one just to
> be sure there's nothing odd going on with this one.

Here is an updated timeout patch (no functional changes for
nicintel_spi). A year has passed, and I'm still trying to find someone
who can test and ack it.

GatoLoko: This new patch also addresses the SB600 timeout issue you were
seeing.

Change-Id: I304546c98441fbd2e5daccb798557d016336a505
Signed-off-by: Carl-Daniel Hailfinger 
---
M bitbang_spi.c
M mcp6x_spi.c
M nicintel_spi.c
M ogp_spi.c
M programmer.h
M sb600spi.c
6 files changed, 61 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/25097/1

diff --git a/bitbang_spi.c b/bitbang_spi.c
index 391fd32..a306e5f 100644
--- a/bitbang_spi.c
+++ b/bitbang_spi.c
@@ -51,10 +51,11 @@
    return bitbang_spi_master->get_miso();
 }

-static void bitbang_spi_request_bus(void)
+static int bitbang_spi_request_bus(void)
 {
    if (bitbang_spi_master->request_bus)
-		bitbang_spi_master->request_bus();
+		return bitbang_spi_master->request_bus();
+	return 0;
 }

static void bitbang_spi_release_bus(void)
@@ -99,7 +100,7 @@

register_spi_programmer(&spi_programmer_bitbang);

-	/* FIXME: Run bitbang_spi_request_bus here or in programmer init? */
+	/* FIXME: Run bitbang_spi_request_bus here or per command? */
    bitbang_spi_set_cs(1);
    bitbang_spi_set_sck(0);
    bitbang_spi_set_mosi(0);
@@ -144,13 +145,15 @@
 static int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
    	const unsigned char *writearr, unsigned char *readarr)
 {
-	int i;
+	int i, ret;

/* FIXME: Run bitbang_spi_request_bus here or in programmer init?
     * Requesting and releasing the SPI bus is handled in here to allow the
     * programmer to use its own SPI engine for native accesses.
     */
-	bitbang_spi_request_bus();
+	ret = bitbang_spi_request_bus();
+	if (ret)
+		return ret;
    bitbang_spi_set_cs(0);
    for (i = 0; i < writecnt; i++)
    	bitbang_spi_readwrite_byte(writearr[i]);
diff --git a/mcp6x_spi.c b/mcp6x_spi.c
index d2c31be..c30e36a 100644
--- a/mcp6x_spi.c
+++ b/mcp6x_spi.c
@@ -39,22 +39,38 @@
 #define MCP6X_SPI_REQUEST	0
 #define MCP6X_SPI_GRANT		8

+/* 1000000 loops are roughly 1 s. */
+#define MAX_REQUEST_LOOPS      1000000
+
 void *mcp6x_spibar = NULL;

/* Cached value of last GPIO state. */
 static uint8_t mcp_gpiostate;

-static void mcp6x_request_spibus(void)
+static void mcp6x_release_spibus(void);
+
+static int mcp6x_request_spibus(void)
 {
+	int i = 0;
+
    mcp_gpiostate = mmio_readb(mcp6x_spibar + 0x530);
    mcp_gpiostate |= 1 << MCP6X_SPI_REQUEST;
    mmio_writeb(mcp_gpiostate, mcp6x_spibar + 0x530);

/* Wait until we are allowed to use the SPI bus. */
-	while (!(mmio_readw(mcp6x_spibar + 0x530) & (1 << MCP6X_SPI_GRANT))) ;
+	while (!(mmio_readw(mcp6x_spibar + 0x530) & (1 << MCP6X_SPI_GRANT))) {
+		if (i++ > MAX_REQUEST_LOOPS) {
+			/* Update the cache. */
+			mcp_gpiostate = mmio_readb(mcp6x_spibar + 0x530);
+			mcp6x_release_spibus();
+			msg_perr("%s: Timeout! Aborting.\n", __func__);
+			return TIMEOUT_ERROR;
+		}
+	}

/* Update the cache. */
    mcp_gpiostate = mmio_readb(mcp6x_spibar + 0x530);
+	return 0;
 }

static void mcp6x_release_spibus(void)
diff --git a/nicintel_spi.c b/nicintel_spi.c
index aacd68c..9f8631e 100644
--- a/nicintel_spi.c
+++ b/nicintel_spi.c
@@ -58,6 +58,9 @@
 // #define FL_BUSY	30
 // #define FL_ER	31

+/* 1000000 loops are roughly 1 s. */
+#define MAX_REQUEST_LOOPS      1000000
+
 uint8_t *nicintel_spibar;

const struct pcidev_status nics_intel_spi[] = {
@@ -69,16 +72,27 @@
    {},
 };

-static void nicintel_request_spibus(void)
+static void nicintel_release_spibus(void);
+
+static int nicintel_request_spibus(void)
 {
    uint32_t tmp;
+	int i = 0;

tmp = pci_mmio_readl(nicintel_spibar + FLA);
    tmp |= 1 << FL_REQ;
    pci_mmio_writel(tmp, nicintel_spibar + FLA);

/* Wait until we are allowed to use the SPI bus. */
-	while (!(pci_mmio_readl(nicintel_spibar + FLA) & (1 << FL_GNT))) ;
+	while (!(pci_mmio_readl(nicintel_spibar + FLA) & (1 << FL_GNT))) {
+		if (i++ > MAX_REQUEST_LOOPS) {
+			nicintel_release_spibus();
+			msg_perr("%s: Timeout! Aborting.\n", __func__);
+			return TIMEOUT_ERROR;
+		}
+	}
+
+	return 0;
 }

static void nicintel_release_spibus(void)
diff --git a/ogp_spi.c b/ogp_spi.c
index dbaa57a..aa9d737 100644
--- a/ogp_spi.c
+++ b/ogp_spi.c
@@ -50,9 +50,13 @@
    {},
 };

-static void ogp_request_spibus(void)
+static int ogp_request_spibus(void)
 {
+	/* FIXME: Do we have to check if any other SPI accesses are running
+	 * right now? Can the request fail?
+	 */
    pci_mmio_writel(1, ogp_spibar + ogp_reg_sel);
+	return 0;
 }

static void ogp_release_spibus(void)
diff --git a/programmer.h b/programmer.h
index b16e4f8..59c7d13 100644
--- a/programmer.h
+++ b/programmer.h
@@ -131,7 +131,7 @@
    void (*set_sck) (int val);
    void (*set_mosi) (int val);
    int (*get_miso) (void);
-	void (*request_bus) (void);
+	int (*request_bus) (void);
    void (*release_bus) (void);
 };

diff --git a/sb600spi.c b/sb600spi.c
index 9d82b47..7049924 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -80,12 +80,19 @@
    return ret;
 }

-static void execute_command(void)
+static int execute_command(void)
 {
+	int timeout = 10000000;
+
    mmio_writeb(mmio_readb(sb600_spibar + 2) | 1, sb600_spibar + 2);

-	while (mmio_readb(sb600_spibar + 2) & 1)
+	while ((mmio_readb(sb600_spibar + 2) & 1) && --timeout)
    	;
+	if (!timeout) {
+		msg_perr("SB600: execute_command timeout!\n");
+		return TIMEOUT_ERROR;
+	}
+	return 0;
 }

static int sb600_spi_send_command(unsigned int writecnt, unsigned int readcnt,
@@ -96,6 +103,7 @@
    unsigned char cmd = *writearr++;
    unsigned int readoffby1;
    unsigned char readwrite;
+	int ret;

writecnt--;

@@ -144,7 +152,9 @@
    	return SPI_PROGRAMMER_ERROR;

msg_pspew("Executing: \n");
-	execute_command();
+	ret = execute_command();
+	if (ret)
+		return ret;

/*
     * After the command executed, we should find out the index of the