Change in coreboot[master]: soc/mediatek/mt8192: Add SPI flash controller DMA read function

[CK HU (Code Review)]

CK HU has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44851 )

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

soc/mediatek/mt8192: Add SPI flash controller DMA read function

To speed up SPI flash read, enable DMA read function.

Signed-off-by: CK Hu 
Change-Id: Ic1679ef7940258350feeadac50ad8ea407fd7b90
---
M src/soc/mediatek/mt8192/flash_controller.c
1 file changed, 55 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44851/1

diff --git a/src/soc/mediatek/mt8192/flash_controller.c b/src/soc/mediatek/mt8192/flash_controller.c
index 304e4c3..0c37ae5 100644
--- a/src/soc/mediatek/mt8192/flash_controller.c
+++ b/src/soc/mediatek/mt8192/flash_controller.c
@@ -8,6 +8,7 @@
 #include <spi-generic.h>
 #include <stdint.h>
 #include <string.h>
+#include <symbols.h>
 #include <timer.h>
 #include <types.h>

@@ -91,6 +92,37 @@
    return 0;
 }

+static int dma_read(u32 addr, u8 *buf, u32 len, uintptr_t dma_buf,
+		    size_t dma_buf_len)
+{
+	struct stopwatch sw;
+
+	assert(IS_ALIGNED((uintptr_t)addr, SFLASH_DMA_ALIGN) &&
+	       IS_ALIGNED(len, SFLASH_DMA_ALIGN) &&
+	       len <= dma_buf_len);
+
+	/* do dma reset */
+	write32(&mt8192_nor->fdma_ctl, SFLASH_DMA_SW_RESET);
+	write32(&mt8192_nor->fdma_ctl, SFLASH_DMA_WDLE_EN);
+	/* flash source address and dram dest address */
+	write32(&mt8192_nor->fdma_fadr, addr);
+	write32(&mt8192_nor->fdma_dadr, dma_buf);
+	write32(&mt8192_nor->fdma_end_dadr, (dma_buf + len));
+	/* start dma */
+	write32(&mt8192_nor->fdma_ctl, SFLASH_DMA_TRIGGER | SFLASH_DMA_WDLE_EN);
+
+	stopwatch_init_usecs_expire(&sw, SFLASH_POLLINGREG_US);
+	while ((read32(&mt8192_nor->fdma_ctl) & SFLASH_DMA_TRIGGER) != 0) {
+		if (stopwatch_expired(&sw)) {
+			printk(BIOS_WARNING, "dma read timeout!\n");
+			return -1;
+		}
+	}
+
+	memcpy(buf, (const void *)dma_buf, len);
+	return 0;
+}
+
 static int pio_read(u32 addr, u8 *buf, u32 len)
 {
    set_sfpaddr(addr);
@@ -107,7 +139,29 @@
 static int nor_read(const struct spi_flash *flash, u32 addr, size_t len,
    	    void *buf)
 {
-	if (pio_read(addr, buf, len))
+	size_t done = 0;
+	uintptr_t dma_buf = (uintptr_t)_dma_coherent;
+	size_t dma_buf_len = REGION_SIZE(dma_coherent);
+	u32 next;
+
+	if (!IS_ALIGNED((uintptr_t)addr, SFLASH_DMA_ALIGN)) {
+		next = MIN(ALIGN_UP((uintptr_t)addr, SFLASH_DMA_ALIGN) -
+			   (uintptr_t)addr, len);
+		if (pio_read(addr, buf, next))
+			return -1;
+		done += next;
+	}
+
+	while (len - done >= SFLASH_DMA_ALIGN) {
+		next = MIN(dma_buf_len, ALIGN_DOWN(len - done,
+			   SFLASH_DMA_ALIGN));
+		if (dma_read(addr + done, buf + done, next, dma_buf,
+			     dma_buf_len))
+			return -1;
+		done += next;
+	}
+	next = len - done;
+	if (next > 0 && pio_read(addr + done, buf + done, next))
    	return -1;

return 0;

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44851/1/src/soc/mediatek/mt8192/fla... 
File src/soc/mediatek/mt8192/flash_controller.c:

https://review.coreboot.org/c/coreboot/+/44851/1/src/soc/mediatek/mt8192/fla... 
PS1, Line 142: 	size_t done = 0;
             : 	uintptr_t dma_buf = (uintptr_t)_dma_coherent;
             : 	size_t dma_buf_len = REGION_SIZE(dma_coherent);
             : 	u32 next;
             : 
             : 	if (!IS_ALIGNED((uintptr_t)addr, SFLASH_DMA_ALIGN)) {
             : 		next = MIN(ALIGN_UP((uintptr_t)addr, SFLASH_DMA_ALIGN) -
             : 			   (uintptr_t)addr, len);
             : 		if (pio_read(addr, buf, next))
             : 			return -1;
             : 		done += next;
             : 	}
             : 
             : 	while (len - done >= SFLASH_DMA_ALIGN) {
             : 		next = MIN(dma_buf_len, ALIGN_DOWN(len - done,
             : 			   SFLASH_DMA_ALIGN));
             : 		if (dma_read(addr + done, buf + done, next, dma_buf,
             : 			     dma_buf_len))
             : 			return -1;
             : 		done += next;
             : 	}
             : 	next = len - done;
             : 	if (next > 0 && pio_read(addr + done, buf + done, next))
             : 		return -1;
can we replace this by purely DMA calls, eg:

u32 start = addr - addr % SFLASH_DMA_ALIGN;
 u32 skip = addr - start;
 u32 total = ALIGN_UP(skip + len, SFLASH_DMA_ALIGN);
 u32 drop = total - skip - len;

size_t done = 0;
 u32 next = start;

/* DMA: [ skip | len | drop ] */

for (; done < total; ) {
    read_len = MIN(dma_buf_len, total - done);
    dma_read(start + done, read_len);
    done += read_len;

// decide the range to copy into buffer
    if (done == total)
       read_len -= drop;

memcpy(buf, dma_buf + skip, read_len - skip);
    skip = 0;
 }

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

Patch Set 1:

(1 comment)

Changing from PIO+DMA (boot time: 879,067) to full-DMA (871,016) saved 8051us ~ 8ms. Please replace the PIO+DMA with my version (full DMA).

https://review.coreboot.org/c/coreboot/+/44851/1/src/soc/mediatek/mt8192/fla... 
File src/soc/mediatek/mt8192/flash_controller.c:

https://review.coreboot.org/c/coreboot/+/44851/1/src/soc/mediatek/mt8192/fla... 
PS1, Line 142: 	size_t done = 0;
             : 	uintptr_t dma_buf = (uintptr_t)_dma_coherent;
             : 	size_t dma_buf_len = REGION_SIZE(dma_coherent);
             : 	u32 next;
             : 
             : 	if (!IS_ALIGNED((uintptr_t)addr, SFLASH_DMA_ALIGN)) {
             : 		next = MIN(ALIGN_UP((uintptr_t)addr, SFLASH_DMA_ALIGN) -
             : 			   (uintptr_t)addr, len);
             : 		if (pio_read(addr, buf, next))
             : 			return -1;
             : 		done += next;
             : 	}
             : 
             : 	while (len - done >= SFLASH_DMA_ALIGN) {
             : 		next = MIN(dma_buf_len, ALIGN_DOWN(len - done,
             : 			   SFLASH_DMA_ALIGN));
             : 		if (dma_read(addr + done, buf + done, next, dma_buf,
             : 			     dma_buf_len))
             : 			return -1;
             : 		done += next;
             : 	}
             : 	next = len - done;
             : 	if (next > 0 && pio_read(addr + done, buf + done, next))
             : 		return -1;
> can we replace this by purely DMA calls, eg: […]

I tested and this works well:

static int nor_read(const struct spi_flash *flash, u32 addr, size_t len,
                     void *buf)
 {
        uintptr_t dma_buf = (uintptr_t)_dma_coherent;
        size_t dma_buf_len = REGION_SIZE(dma_coherent);

setbits8(&mt8192_nor->read_dual, SFLASH_READ_DUAL_EN);
        write8(&mt8192_nor->prgdata[3], SFLASH_1_1_2_READ);

u32 start = ALIGN_DOWN(addr, SFLASH_DMA_ALIGN);
        u32 skip = addr - start;
        u32 total = ALIGN_UP(skip + len, SFLASH_DMA_ALIGN);
        u32 drop = total - skip - len;
        u32 done, read_len, copy_len;
        uint8_t *dest = (uint8_t *)buf;

/* DMA: start [ skip | len | drop ]=total end */
        for (done = 0; done < total; dest += copy_len) {
                read_len = MIN(dma_buf_len, total - done);
                if (dma_read(start + done, dma_buf, read_len, dma_buf_len))
                        return -1;

done += read_len;

// decide the range to copy into buffer
                if (done == total)
                        read_len -= drop;  /* Only drop in last iteration */

copy_len = read_len - skip;
                memcpy(dest, (uint8_t *)dma_buf + skip, copy_len);

if (skip)
                        skip = 0;  /* Only apply skip in first iteration. */
        }

return 0;
 }

[CK HU (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,

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

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

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

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

soc/mediatek/mt8192: Add SPI flash controller DMA read function

To speed up SPI flash read, enable DMA read function.

Signed-off-by: CK Hu 
Change-Id: Ic1679ef7940258350feeadac50ad8ea407fd7b90
---
M src/soc/mediatek/mt8192/flash_controller.c
1 file changed, 48 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44851/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44851 )

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/44851/2/src/soc/mediatek/mt8192/fla... 
File src/soc/mediatek/mt8192/flash_controller.c:

https://review.coreboot.org/c/coreboot/+/44851/2/src/soc/mediatek/mt8192/fla... 
PS2, Line 150:         }
code indent should use tabs where possible

https://review.coreboot.org/c/coreboot/+/44851/2/src/soc/mediatek/mt8192/fla... 
PS2, Line 150:         }
please, no spaces at the start of a line

https://review.coreboot.org/c/coreboot/+/44851/2/src/soc/mediatek/mt8192/fla... 
PS2, Line 151:         return 0;
code indent should use tabs where possible

https://review.coreboot.org/c/coreboot/+/44851/2/src/soc/mediatek/mt8192/fla... 
PS2, Line 151:         return 0;
please, no spaces at the start of a line

[CK HU (Code Review)]

CK HU has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44851 )

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

Patch Set 2:
> Patch Set 1:
>
> (1 comment)
>
> Changing from PIO+DMA (boot time: 879,067) to full-DMA (871,016) saved 8051us ~ 8ms. Please replace the PIO+DMA with my version (full DMA).

Done.

[CK HU (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,

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

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

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

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

soc/mediatek/mt8192: Add SPI flash controller DMA read function

To speed up SPI flash read, enable DMA read function.

Signed-off-by: CK Hu 
Change-Id: Ic1679ef7940258350feeadac50ad8ea407fd7b90
---
M src/soc/mediatek/mt8192/flash_controller.c
1 file changed, 47 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44851/3

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

Patch Set 3: Code-Review+2

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44851/1/src/soc/mediatek/mt8192/fla... 
File src/soc/mediatek/mt8192/flash_controller.c:

https://review.coreboot.org/c/coreboot/+/44851/1/src/soc/mediatek/mt8192/fla... 
PS1, Line 142: 	size_t done = 0;
             : 	uintptr_t dma_buf = (uintptr_t)_dma_coherent;
             : 	size_t dma_buf_len = REGION_SIZE(dma_coherent);
             : 	u32 next;
             : 
             : 	if (!IS_ALIGNED((uintptr_t)addr, SFLASH_DMA_ALIGN)) {
             : 		next = MIN(ALIGN_UP((uintptr_t)addr, SFLASH_DMA_ALIGN) -
             : 			   (uintptr_t)addr, len);
             : 		if (pio_read(addr, buf, next))
             : 			return -1;
             : 		done += next;
             : 	}
             : 
             : 	while (len - done >= SFLASH_DMA_ALIGN) {
             : 		next = MIN(dma_buf_len, ALIGN_DOWN(len - done,
             : 			   SFLASH_DMA_ALIGN));
             : 		if (dma_read(addr + done, buf + done, next, dma_buf,
             : 			     dma_buf_len))
             : 			return -1;
             : 		done += next;
             : 	}
             : 	next = len - done;
             : 	if (next > 0 && pio_read(addr + done, buf + done, next))
             : 		return -1;
> I tested and this works well: […]

Ack

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44851 )

Change subject: soc/mediatek/mt8192: Add SPI flash controller DMA read function
......................................................................

soc/mediatek/mt8192: Add SPI flash controller DMA read function

To speed up SPI flash read, enable DMA read function.

Signed-off-by: CK Hu 
Change-Id: Ic1679ef7940258350feeadac50ad8ea407fd7b90
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44851
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
---
M src/soc/mediatek/mt8192/flash_controller.c
1 file changed, 47 insertions(+), 9 deletions(-)

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

diff --git a/src/soc/mediatek/mt8192/flash_controller.c b/src/soc/mediatek/mt8192/flash_controller.c
index 304e4c3..b252880 100644
--- a/src/soc/mediatek/mt8192/flash_controller.c
+++ b/src/soc/mediatek/mt8192/flash_controller.c
@@ -8,6 +8,7 @@
 #include <spi-generic.h>
 #include <stdint.h>
 #include <string.h>
+#include <symbols.h>
 #include <timer.h>
 #include <types.h>

@@ -91,25 +92,62 @@
    return 0;
 }

-static int pio_read(u32 addr, u8 *buf, u32 len)
+static int dma_read(u32 addr, uintptr_t dma_buf, u32 len)
 {
-	set_sfpaddr(addr);
-	while (len) {
-		if (mt8192_nor_execute_cmd(SFLASH_RD_TRIGGER | SFLASH_AUTOINC))
-			return -1;
+	struct stopwatch sw;

-		*buf++ = read8(&mt8192_nor->rdata);
-		len--;
+	assert(IS_ALIGNED((uintptr_t)addr, SFLASH_DMA_ALIGN) &&
+	       IS_ALIGNED(len, SFLASH_DMA_ALIGN));
+
+	/* do dma reset */
+	write32(&mt8192_nor->fdma_ctl, SFLASH_DMA_SW_RESET);
+	write32(&mt8192_nor->fdma_ctl, SFLASH_DMA_WDLE_EN);
+	/* flash source address and dram dest address */
+	write32(&mt8192_nor->fdma_fadr, addr);
+	write32(&mt8192_nor->fdma_dadr, dma_buf);
+	write32(&mt8192_nor->fdma_end_dadr, (dma_buf + len));
+	/* start dma */
+	write32(&mt8192_nor->fdma_ctl, SFLASH_DMA_TRIGGER | SFLASH_DMA_WDLE_EN);
+
+	stopwatch_init_usecs_expire(&sw, SFLASH_POLLINGREG_US);
+	while ((read32(&mt8192_nor->fdma_ctl) & SFLASH_DMA_TRIGGER) != 0) {
+		if (stopwatch_expired(&sw)) {
+			printk(BIOS_WARNING, "dma read timeout!\n");
+			return -1;
+		}
    }
+
    return 0;
 }

static int nor_read(const struct spi_flash *flash, u32 addr, size_t len,
    	    void *buf)
 {
-	if (pio_read(addr, buf, len))
-		return -1;
+	uintptr_t dma_buf = (uintptr_t)_dma_coherent;
+	size_t dma_buf_len = REGION_SIZE(dma_coherent);
+	u32 start = ALIGN_DOWN(addr, SFLASH_DMA_ALIGN);
+	u32 skip = addr - start;
+	u32 total = ALIGN_UP(skip + len, SFLASH_DMA_ALIGN);
+	u32 drop = total - skip - len;
+	u32 done, read_len, copy_len;
+	uint8_t *dest = (uint8_t *)buf;

+	/* DMA: start [ skip | len | drop ] = total end */
+	for (done = 0; done < total; dest += copy_len) {
+		read_len = MIN(dma_buf_len, total - done);
+		if (dma_read(start + done, dma_buf, read_len))
+			return -1;
+
+		done += read_len;
+		/* decide the range to copy into buffer */
+		if (done == total)
+			read_len -= drop;  /* Only drop in last iteration */
+
+		copy_len = read_len - skip;
+		memcpy(dest, (uint8_t *)dma_buf + skip, copy_len);
+		if (skip)
+			skip = 0;  /* Only apply skip in first iteration. */
+	}
    return 0;
 }