[coreboot-gerrit] Change in coreboot[master]: Add buffer_to/from_fifo32(_prefix) helpers

[Julius Werner (Code Review)]

Hello Hung-Te Lin,

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

https://review.coreboot.org/c/coreboot/+/34817

to review the following change.

Change subject: Add buffer_to/from_fifo32(_prefix) helpers
......................................................................

Add buffer_to/from_fifo32(_prefix) helpers

Many peripheral drivers across different SoCs regularly face the same
task of piping a transfer buffer into (or reading it out of) a 32-bit
FIFO register. Sometimes it's just one register, sometimes a whole array
of registers. Sometimes you actually transfer 4 bytes per register
read/write, sometimes only 2 (or even 1). Sometimes writes need to be
prefixed with one or two command bytes which makes the actual payload
buffer "misaligned" in relation to the FIFO and requires a bunch of
tricky bit packing logic to get right. Most of the times transfer
lengths are not guaranteed to be divisible by 4, which also requires a
bunch of logic to treat the potential unaligned end of the transfer
correctly.

We have a dozen different implementations of this same pattern across
coreboot. This patch introduces a new family of helper functions that
aims to solve all these use cases once and for all (*fingers crossed*),
and demonstrates their use in the Rockchip SPI and I2C drivers.

Change-Id: Ifcf37c6d56f949f620c347df05439b05c3b8d77d
Signed-off-by: Julius Werner 
---
M payloads/libpayload/include/libpayload.h
M payloads/libpayload/libc/lib.c
M src/device/Makefile.inc
A src/device/mmio.c
M src/include/device/mmio.h
M src/soc/rockchip/common/i2c.c
M src/soc/rockchip/common/spi.c
7 files changed, 183 insertions(+), 54 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/34817/1

diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h
index 57a3afc..a25cff3 100644
--- a/payloads/libpayload/include/libpayload.h
+++ b/payloads/libpayload/include/libpayload.h
@@ -446,6 +446,23 @@

/**
+ * @defgroup mmio MMIO helper functions
+ * @{
+ */
+void buffer_from_fifo32(void *buffer, size_t size, void *fifo,
+			int fifo_stride, int fifo_width);
+void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size,
+			     void *fifo, int fifo_stride, int fifo_width);
+static inline void buffer_to_fifo32(void *buffer, size_t size, void *fifo,
+				    int fifo_stride, int fifo_width)
+{
+	buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo,
+				fifo_stride, fifo_width);
+}
+/** @} */
+
+
+/**
  * @defgroup hash Hashing functions
  * @{
  */
diff --git a/payloads/libpayload/libc/lib.c b/payloads/libpayload/libc/lib.c
index bead1f8..d761aad 100644
--- a/payloads/libpayload/libc/lib.c
+++ b/payloads/libpayload/libc/lib.c
@@ -27,6 +27,7 @@
  * SUCH DAMAGE.
  */

+#include <assert.h>
 #include <libpayload.h>

/*
@@ -125,3 +126,52 @@
 {
    return NULL;
 }
+
+/*
+ * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the
+ * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit
+ * registers or 0 to read everything from the same register). fifo_width is
+ * the amount of bytes read per register (can be 1 through 4).
+ */
+void buffer_from_fifo32(void *buffer, size_t size, void *fifo,
+			int fifo_stride, int fifo_width)
+{
+	u8 *p = buffer;
+	int i, j;
+
+	assert(fifo_width > 0 && fifo_width <= sizeof(u32) &&
+	       fifo_stride % sizeof(u32) == 0);
+
+	for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) {
+		u32 val = read32(fifo);
+		for (j = 0; j < MIN(size - i, fifo_width); j++)
+			*p++ = (u8)(val >> (j * 8));
+	}
+}
+
+/*
+ * Version of buffer_to_fifo32() that can prepend a prefix of up to fifo_width
+ * size to the transfer. This is often useful for protocols where a command word
+ * precedes the actual payload data. The prefix must be packed in the low-order
+ * bytes of the 'prefix' u32 parameter and any high-order bytes exceeding prefsz
+ * must be 0. Note that 'size' counts total bytes written, including 'prefsz'.
+ */
+void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size,
+			     void *fifo, int fifo_stride, int fifo_width)
+{
+	u8 *p = buffer;
+	int i, j = prefsz;
+
+	assert(fifo_width > 0 && fifo_width <= sizeof(u32) &&
+	       fifo_stride % sizeof(u32) == 0 && prefsz <= fifo_width);
+
+	uint32_t val = prefix;
+	for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) {
+		for (; j < MIN(size - i, fifo_width); j++)
+			val |= *p++ << (j * 8);
+		write32(fifo, val);
+		val = 0;
+		j = 0;
+	}
+
+}
diff --git a/src/device/Makefile.inc b/src/device/Makefile.inc
index baa45be..966ca0d 100644
--- a/src/device/Makefile.inc
+++ b/src/device/Makefile.inc
@@ -53,3 +53,8 @@
 romstage-y += i2c.c
 ramstage-y += i2c.c
 ramstage-y += i2c_bus.c
+
+bootblock-y += mmio.c
+verstage-y += mmio.c
+romstage-y += mmio.c
+ramstage-y += mmio.c
diff --git a/src/device/mmio.c b/src/device/mmio.c
new file mode 100644
index 0000000..61a1130
--- /dev/null
+++ b/src/device/mmio.c
@@ -0,0 +1,55 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <assert.h>
+#include <device/mmio.h>
+
+/* Helper functions for various MMIO access patterns. */
+
+void buffer_from_fifo32(void *buffer, size_t size, void *fifo,
+			int fifo_stride, int fifo_width)
+{
+	u8 *p = buffer;
+	int i, j;
+
+	assert(fifo_width > 0 && fifo_width <= sizeof(u32) &&
+	       fifo_stride % sizeof(u32) == 0);
+
+	for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) {
+		u32 val = read32(fifo);
+		for (j = 0; j < MIN(size - i, fifo_width); j++)
+			*p++ = (u8)(val >> (j * 8));
+	}
+}
+
+void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size,
+			     void *fifo, int fifo_stride, int fifo_width)
+{
+	u8 *p = buffer;
+	int i, j = prefsz;
+
+	assert(fifo_width > 0 && fifo_width <= sizeof(u32) &&
+	       fifo_stride % sizeof(u32) == 0 && prefsz <= fifo_width);
+
+	uint32_t val = prefix;
+	for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) {
+		for (; j < MIN(size - i, fifo_width); j++)
+			val |= *p++ << (j * 8);
+		write32(fifo, val);
+		val = 0;
+		j = 0;
+	}
+
+}
diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h
index 34be1e8..4b6d1e2 100644
--- a/src/include/device/mmio.h
+++ b/src/include/device/mmio.h
@@ -17,5 +17,38 @@

#include <arch/mmio.h>
 #include <endian.h>
+#include <types.h>
+
+/*
+ * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the
+ * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit
+ * registers or 0 to read everything from the same register). fifo_width is
+ * the amount of bytes read per register (can be 1 through 4).
+ */
+void buffer_from_fifo32(void *buffer, size_t size, void *fifo,
+			int fifo_stride, int fifo_width);
+
+/*
+ * Version of buffer_to_fifo32() that can prepend a prefix of up to fifo_width
+ * size to the transfer. This is often useful for protocols where a command word
+ * precedes the actual payload data. The prefix must be packed in the low-order
+ * bytes of the 'prefix' u32 parameter and any high-order bytes exceeding prefsz
+ * must be 0. Note that 'size' counts total bytes written, including 'prefsz'.
+ */
+void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size,
+			     void *fifo, int fifo_stride, int fifo_width);
+
+/*
+ * Writes a transfer buffer into 32-bit FIFO registers. fifo_stride is the
+ * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit
+ * registers or 0 to write everything into the same register). fifo_width is
+ * the amount of bytes written per register (can be 1 through 4).
+ */
+static inline void buffer_to_fifo32(void *buffer, size_t size, void *fifo,
+				    int fifo_stride, int fifo_width)
+{
+	buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo,
+				fifo_stride, fifo_width);
+}

#endif
diff --git a/src/soc/rockchip/common/i2c.c b/src/soc/rockchip/common/i2c.c
index e5f5a9a..cd0ed9b 100644
--- a/src/soc/rockchip/common/i2c.c
+++ b/src/soc/rockchip/common/i2c.c
@@ -128,25 +128,21 @@
    uint8_t *data = segment.buf;
    int timeout = I2C_TIMEOUT_US;
    unsigned int bytes_remaining = segment.len;
-	unsigned int bytes_transferred = 0;
-	unsigned int words_transferred = 0;
-	unsigned int rxdata = 0;
    unsigned int con = 0;
-	unsigned int i, j;

write32(&reg_addr->i2c_mrxaddr, I2C_8BIT | segment.slave << 1 | 1);
    write32(&reg_addr->i2c_mrxraddr, 0);
    con = I2C_MODE_TRX | I2C_EN | I2C_ACT2NAK;
    while (bytes_remaining) {
-		bytes_transferred = MIN(bytes_remaining, 32);
-		bytes_remaining -= bytes_transferred;
+		size_t size = MIN(bytes_remaining, 32);
+		bytes_remaining -= size;
    	if (!bytes_remaining)
    		con |= I2C_EN | I2C_NAK;
-		words_transferred = ALIGN_UP(bytes_transferred, 4) / 4;

+		i2c_info("I2C Read::%zu bytes\n", size);
    	write32(&reg_addr->i2c_ipd, I2C_CLEANI);
    	write32(&reg_addr->i2c_con, con);
-		write32(&reg_addr->i2c_mrxcnt, bytes_transferred);
+		write32(&reg_addr->i2c_mrxcnt, size);

timeout = I2C_TIMEOUT_US;
    	while (timeout--) {
@@ -166,15 +162,8 @@
    		return I2C_TIMEOUT;
    	}

-		for (i = 0; i < words_transferred; i++) {
-			rxdata = read32(&reg_addr->rxdata[i]);
-			i2c_info("I2c Read::RXDATA[%d] = 0x%x\n", i, rxdata);
-			for (j = 0; j < 4; j++) {
-				if ((i * 4 + j) == bytes_transferred)
-					break;
-				*data++ = (rxdata >> (j * 8)) & 0xff;
-			}
-		}
+		buffer_from_fifo32(data, size, &reg_addr->rxdata, 4, 4);
+		data += size;
    	con = I2C_MODE_RX | I2C_EN | I2C_ACT2NAK;
    }
    return res;
@@ -186,32 +175,22 @@
    uint8_t *data = segment.buf;
    int timeout = I2C_TIMEOUT_US;
    int bytes_remaining = segment.len + 1;
-	int bytes_transferred = 0;
-	int words_transferred = 0;
-	unsigned int i;
-	unsigned int j = 1;
-	u32 txdata = 0;

-	txdata |= (segment.slave << 1);
+	/* Prepend one byte for the slave address to the transfer. */
+	u32 prefix = segment.slave << 1;
+	int prefsz = 1;
+
    while (bytes_remaining) {
-		bytes_transferred = MIN(bytes_remaining, 32);
-		words_transferred = ALIGN_UP(bytes_transferred, 4) / 4;
-		for (i = 0; i < words_transferred; i++) {
-			do {
-				if ((i * 4 + j) == bytes_transferred)
-					break;
-				txdata |= (*data++) << (j * 8);
-			} while (++j < 4);
-			write32(&reg_addr->txdata[i], txdata);
-			j = 0;
-			i2c_info("I2c Write::TXDATA[%d] = 0x%x\n", i, txdata);
-			txdata = 0;
-		}
+		size_t size = MIN(bytes_remaining, 32);
+		buffer_to_fifo32_prefix(data, prefix, prefsz, size,
+					&reg_addr->txdata, 4, 4);
+		data += size - prefsz;

+		i2c_info("I2C Write::%zu bytes\n", size);
    	write32(&reg_addr->i2c_ipd, I2C_CLEANI);
    	write32(&reg_addr->i2c_con,
    		I2C_EN | I2C_MODE_TX | I2C_ACT2NAK);
-		write32(&reg_addr->i2c_mtxcnt, bytes_transferred);
+		write32(&reg_addr->i2c_mtxcnt, size);

timeout = I2C_TIMEOUT_US;
    	while (timeout--) {
@@ -232,7 +211,9 @@
    		return I2C_TIMEOUT;
    	}

-		bytes_remaining -= bytes_transferred;
+		bytes_remaining -= size;
+		prefsz = 0;
+		prefix = 0;
    }
    return res;
 }
diff --git a/src/soc/rockchip/common/spi.c b/src/soc/rockchip/common/spi.c
index 7bde433..0307e24 100644
--- a/src/soc/rockchip/common/spi.c
+++ b/src/soc/rockchip/common/spi.c
@@ -221,23 +221,11 @@
    	 * sychronizing with the SPI clock which is pretty slow.
    	 */
    	if (*bytes_in && !(sr & SR_RF_EMPT)) {
-			int fifo = read32(&regs->rxflr) & RXFLR_LEVEL_MASK;
-			int val;
-
-			if (use_16bit)
-				xferred = fifo * 2;
-			else
-				xferred = fifo;
+			int w = use_16bit ? 2 : 1;
+			xferred = (read32(&regs->rxflr) & RXFLR_LEVEL_MASK) * w;
+			buffer_from_fifo32(in_buf, xferred, &regs->rxdr, 0, w);
    		*bytes_in -= xferred;
-			while (fifo-- > 0) {
-				val = read32(&regs->rxdr);
-				if (use_16bit) {
-					*in_buf++ = val & 0xff;
-					*in_buf++ = (val >> 8) & 0xff;
-				} else {
-					*in_buf++ = val & 0xff;
-				}
-			}
+			in_buf += xferred;
    	}

min_xfer -= xferred;