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;

[Julius Werner (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

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, 186 insertions(+), 55 deletions(-)

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

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34817 )

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

Patch Set 2: Code-Review+2

sounds like a good idea, although I think you can move the rockchip changes to its own CL.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34817 )

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

Patch Set 2: Code-Review+1

seems like mips arch is still doing write32(v, a) instead of write32(a, v).... I thought we should have fixed that long ago?

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34817 )

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

Patch Set 2:

maybe we should replace write32 as setbits_le32?

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34817 )

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

Patch Set 2:

oh, seems like coreboot mips is doing (a, v) but libpayload mips/arch/io.h is doing (v, a). I think we should fix libpayload.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34817 )

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

Patch Set 2:
> oh, seems like coreboot mips is doing (a, v) but libpayload mips/arch/io.h is doing (v, a). I think we should fix libpayload.

Argh... bloody MIPS. I could change them all but there's still a bunch of depthcharge drivers using the current version, I don't really want to untangle them all right now. Might be best if we delete MIPS entirely instead, it's not been used by anyone and causing tons of problems for years now. I'll #ifdef it out for now and suggest that on the mailing list.

Change in coreboot[master]: rockchip: Use new buffer_to/from_fifo32(_prefix) helpers

[Julius Werner (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: rockchip: Use new buffer_to/from_fifo32(_prefix) helpers
......................................................................

rockchip: Use new buffer_to/from_fifo32(_prefix) helpers

This patch changes the Rockchip SPI and I2C drivers to use the new
buffer_from_fifo32()/buffer_to_fifo32_prefix() helpers when accessing
their FIFOs (mostly just to demonstrate that/how the helpers work).

Change-Id: Ifcf37c6d56f949f620c347df05439b05c3b8d77d
Signed-off-by: Julius Werner 
---
M src/soc/rockchip/common/i2c.c
M src/soc/rockchip/common/spi.c
2 files changed, 23 insertions(+), 54 deletions(-)

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

Change in coreboot[master]: rockchip: Use new buffer_to/from_fifo32(_prefix) helpers

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34817 )

Change subject: rockchip: Use new buffer_to/from_fifo32(_prefix) helpers
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: rockchip: Use new buffer_to/from_fifo32(_prefix) helpers

[Patrick Georgi (Code Review)]

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

Change subject: rockchip: Use new buffer_to/from_fifo32(_prefix) helpers
......................................................................

rockchip: Use new buffer_to/from_fifo32(_prefix) helpers

This patch changes the Rockchip SPI and I2C drivers to use the new
buffer_from_fifo32()/buffer_to_fifo32_prefix() helpers when accessing
their FIFOs (mostly just to demonstrate that/how the helpers work).

Change-Id: Ifcf37c6d56f949f620c347df05439b05c3b8d77d
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34817
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
---
M src/soc/rockchip/common/i2c.c
M src/soc/rockchip/common/spi.c
2 files changed, 23 insertions(+), 54 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Hung-Te Lin: Looks good to me, approved

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;