From 93c138aeac451e2fcdc4df183000f2cf6b4799b2 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:34:36 +0300 Subject: [PATCH 1/2] agent: scan NOR through flash_read, not the memory window that wraps at 1 MiB On hi3536dv100 (Xiongmai NBD8010S-KL-V2, W25Q128) `agent scan` reported 256/256 GOOD with per-sector CRCs repeating every 16 sectors: sector 16 (0x100000, kernel) carried sector 0's CRC (U-Boot). handle_scan() read NOR through the memory-mapped boot-mode window at FLASH_MEM. That window wraps at 1 MiB on some SoCs, which is why CMD_READ and CMD_CRC32 already go through the register path ("boot mode memory window wraps at 1MB on some SoCs", handle_crc32_cmd). So every sector past the first MiB was scanned as a copy of one inside it, and empty sectors were missed. Both passes now read the chip as CMD_READ does: - Pass 1 (CRC32 and the uniformity check) reads each sector with flash_read() into scan_buf. NOR sectors larger than scan_buf (128 KiB) are read in chunks, with the CRC chained across them. NAND keeps its one read per block. - Pass 2, NOR's stability check, is an independent flash_crc32() of the sector, a second read through the same path. Before, it was a second pass over the same wrapped window. On the board (agent built from this commit, flash = OpenIPC 16M image): before, 256 GOOD and 16 distinct CRCs; now 133 GOOD and 123 EMPTY. The 123 EMPTY are the all-0xFF sectors FLASH_STREAM skips for this image. There are 93 distinct CRCs, and every non-empty sector's scan CRC equals CMD_CRC32 for it. The scan takes 20 s (27 s before). make -C agent test passes. All ten SoC builds compile with the same warnings as master (compared line by line on hi3536dv100/FMC100 and hi3520dv200/HISFC350). --- agent/main.c | 61 +++++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/agent/main.c b/agent/main.c index a683c90..72ab6c2 100644 --- a/agent/main.c +++ b/agent/main.c @@ -1409,7 +1409,6 @@ static void handle_scan(const uint8_t *data __attribute__((unused)), for (uint32_t s = 0; s < num_sectors; s++) { uint8_t status = SCAN_GOOD; - const uint8_t *ptr; proto_drain_fifo(); @@ -1429,30 +1428,33 @@ static void handle_scan(const uint8_t *data __attribute__((unused)), } continue; } - /* Good block: read data area into RAM buf for the rest of the - * scan (memory-mapped reads don't work on NAND). */ - flash_read(s * sector_sz, scan_buf, sector_sz); - ptr = scan_buf; - } else { - /* NOR: direct mem-mapped read (FMC boot-mode window). */ - ptr = (const uint8_t *)(FLASH_MEM + s * sector_sz); + /* Good block: its data area is read below through + * flash_read(), as NOR's is (memory-mapped reads don't work on + * NAND). */ } - /* Pass 1: CRC32 */ - uint32_t crc1 = crc32(0, ptr, sector_sz); - - proto_drain_fifo(); - - /* Pattern check: sample first byte, scan for uniformity */ - uint8_t first = ptr[0]; - int all_ff = (first == 0xFF); - int all_same = 1; - - for (uint32_t i = 1; i < sector_sz; i++) { - uint8_t b = ptr[i]; - if (b != 0xFF) all_ff = 0; - if (b != first) all_same = 0; - if (!all_ff && !all_same) break; + /* Pass 1: CRC32 and the uniformity check, over bytes read through + * the register path. NOR used to read the memory-mapped boot-mode + * window, which wraps at 1 MiB on some SoCs (hi3536dv100): every + * sector past the first MiB was scanned as a copy of one in it, + * while CMD_READ and CMD_CRC32 (which use flash_read) read the + * chip correctly. NOR sectors larger than scan_buf go in chunks. */ + uint32_t crc1 = 0; + uint8_t first = 0; + int all_ff = 1, all_same = 1; + for (uint32_t off = 0; off < sector_sz; ) { + uint32_t n = sector_sz - off; + if (n > sizeof(scan_buf)) n = sizeof(scan_buf); + flash_read(s * sector_sz + off, scan_buf, n); + crc1 = crc32(crc1, scan_buf, n); + if (off == 0) first = scan_buf[0]; + for (uint32_t i = 0; i < n && (all_ff || all_same); i++) { + uint8_t b = scan_buf[i]; + if (b != 0xFF) all_ff = 0; + if (b != first) all_same = 0; + } + off += n; + proto_drain_fifo(); } if (all_ff) { @@ -1462,13 +1464,14 @@ static void handle_scan(const uint8_t *data __attribute__((unused)), } else if (all_same) { status = SCAN_STUCK_PAT; } else if (!is_nand) { - /* Pass 2: stability check — re-read CRC. NOR only because - * NAND re-reads always go through the same on-chip ECC and - * would never report differences (the chip auto-corrects - * single-bit flips). For NAND, ECC error would be a - * separate signal we don't yet expose. */ + /* Pass 2: stability check -- an independent second read of + * the sector, through the same register path. NOR only + * because NAND re-reads always go through the same on-chip + * ECC and would never report differences (the chip + * auto-corrects single-bit flips). For NAND, ECC error would + * be a separate signal we don't yet expose. */ proto_drain_fifo(); - uint32_t crc2 = crc32(0, ptr, sector_sz); + uint32_t crc2 = flash_crc32(s * sector_sz, sector_sz); if (crc2 != crc1) status = SCAN_UNSTABLE; } From 5c697504c17fa8e8cfbb93d2aeb7b75659826f16 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:45:25 +0300 Subject: [PATCH 2/2] agent: scan keeps the window above 16 MiB on FMC100, and marks stalled reads Qodo review on this branch. 1. Reads above 16 MiB. FMC100's register reads (flash_read, flash_crc32) send three address bytes, so on a 32/64 MiB FMC100 chip they cannot reach a sector at or above 16 MiB. CMD_READ and CMD_CRC32 share that limit today. The scan, though, read those sectors through the memory window before this branch, so it would have regressed there. scan_read() and scan_crc() now keep the window for FMC100 NOR offsets >= 16 MiB, exactly as before, and use the register path below that, where the 1 MiB wrap is. HISFC350's flash_read banks across the chip (hi3520dv200's 32 MiB MX25L25635E), so it is used everywhere there. SPI_DRIVER_FMC100 now tells main.c which driver it was built with. 2. Stalled reads. FMC100's NOR flash_read() ignored a timed-out fmc_wait_ready() and copied the I/O buffer anyway, and its NAND path ignored nand_read()'s 0xFF "never finished". flash_read() now returns -1 for either (HISFC350's memory-mapped reads return 0). The scan marks such a sector SCAN_READ_ERROR (0x05, already in the host's SectorStatus) with CRC 0, instead of judging stale bytes, which could pass as EMPTY. Other flash_read() callers keep ignoring the result. On the NBD8010S-KL-V2 (16 MiB W25Q128, so every sector takes the register path): 133 GOOD, 123 EMPTY, no READ_ERROR, and every GOOD sector's CRC equals CMD_CRC32's, in 20 s. The >= 16 MiB window branch is unchanged behaviour, not exercised here. make -C agent test passes. hi3536dv100 (FMC100) and hi3520dv200 (HISFC350) build with the same warnings as master, and four more SoCs build. --- agent/Makefile | 2 +- agent/main.c | 62 +++++++++++++++++++++++++++++++------- agent/spi_flash.c | 12 +++++--- agent/spi_flash.h | 4 ++- agent/spi_flash_hisfc350.c | 6 ++-- 5 files changed, 67 insertions(+), 19 deletions(-) diff --git a/agent/Makefile b/agent/Makefile index d19de55..2cec295 100644 --- a/agent/Makefile +++ b/agent/Makefile @@ -265,7 +265,7 @@ SPI_DRIVER ?= fmc100 ifeq ($(SPI_DRIVER),fmc100) SPI_FLASH_SRC = spi_flash.c # CMD_NAND (SPI NAND study ops) drives the FMC100 page engine. - CFLAGS += -DHAVE_NAND_STUDY + CFLAGS += -DHAVE_NAND_STUDY -DSPI_DRIVER_FMC100 else SPI_FLASH_SRC = spi_flash_$(SPI_DRIVER).c endif diff --git a/agent/main.c b/agent/main.c index 72ab6c2..3d95920 100644 --- a/agent/main.c +++ b/agent/main.c @@ -1378,6 +1378,38 @@ static void nand_study_op(uint8_t op, const uint8_t *arg, uint32_t alen, } #endif /* HAVE_NAND_STUDY */ +/* Where CMD_SCAN reads a sector from. The register path (flash_read, + * flash_crc32) is what CMD_READ and CMD_CRC32 use: the memory-mapped + * boot-mode window wraps at 1 MiB on some SoCs (hi3536dv100). FMC100's + * register reads send three address bytes, though, so on an FMC100 build + * a NOR offset at or above 16 MiB is read through the window, as the scan + * always did there. HISFC350's flash_read banks, and covers the chip. */ +#define SCAN_REG_READ_LIMIT 0x1000000u + +static int scan_via_window(uint32_t off) { +#ifdef SPI_DRIVER_FMC100 + return flash_info.flash_type != FLASH_TYPE_NAND && off >= SCAN_REG_READ_LIMIT; +#else + (void)off; + return 0; +#endif +} + +static int scan_read(uint32_t off, uint8_t *buf, uint32_t n) { + if (scan_via_window(off)) { + const volatile uint8_t *p = (const volatile uint8_t *)(FLASH_MEM + off); + for (uint32_t i = 0; i < n; i++) buf[i] = p[i]; + return 0; + } + return flash_read(off, buf, n); +} + +static uint32_t scan_crc(uint32_t off, uint32_t n) { + if (scan_via_window(off)) + return crc32(0, (const uint8_t *)(FLASH_MEM + off), n); + return flash_crc32(off, n); +} + static void handle_scan(const uint8_t *data __attribute__((unused)), uint32_t len __attribute__((unused))) { if (!flash_readable) { @@ -1433,19 +1465,24 @@ static void handle_scan(const uint8_t *data __attribute__((unused)), * NAND). */ } - /* Pass 1: CRC32 and the uniformity check, over bytes read through - * the register path. NOR used to read the memory-mapped boot-mode - * window, which wraps at 1 MiB on some SoCs (hi3536dv100): every - * sector past the first MiB was scanned as a copy of one in it, - * while CMD_READ and CMD_CRC32 (which use flash_read) read the - * chip correctly. NOR sectors larger than scan_buf go in chunks. */ + /* Pass 1: CRC32 and the uniformity check, over bytes read as + * scan_read() says (the register path where it reaches). NOR used + * to read only the memory-mapped window, which wraps at 1 MiB on + * some SoCs (hi3536dv100): every sector past the first MiB was + * scanned as a copy of one in it. NOR sectors larger than scan_buf + * go in chunks. */ uint32_t crc1 = 0; uint8_t first = 0; - int all_ff = 1, all_same = 1; + int all_ff = 1, all_same = 1, read_failed = 0; for (uint32_t off = 0; off < sector_sz; ) { uint32_t n = sector_sz - off; if (n > sizeof(scan_buf)) n = sizeof(scan_buf); - flash_read(s * sector_sz + off, scan_buf, n); + if (scan_read(s * sector_sz + off, scan_buf, n) != 0) { + /* A stalled register op left stale I/O-buffer bytes: + * nothing to judge the sector by. */ + read_failed = 1; + break; + } crc1 = crc32(crc1, scan_buf, n); if (off == 0) first = scan_buf[0]; for (uint32_t i = 0; i < n && (all_ff || all_same); i++) { @@ -1457,7 +1494,10 @@ static void handle_scan(const uint8_t *data __attribute__((unused)), proto_drain_fifo(); } - if (all_ff) { + if (read_failed) { + status = SCAN_READ_ERROR; + crc1 = 0; + } else if (all_ff) { status = SCAN_EMPTY; } else if (all_same && first == 0x00) { status = SCAN_STUCK_ZERO; @@ -1465,13 +1505,13 @@ static void handle_scan(const uint8_t *data __attribute__((unused)), status = SCAN_STUCK_PAT; } else if (!is_nand) { /* Pass 2: stability check -- an independent second read of - * the sector, through the same register path. NOR only + * the sector, from where pass 1 read it. NOR only * because NAND re-reads always go through the same on-chip * ECC and would never report differences (the chip * auto-corrects single-bit flips). For NAND, ECC error would * be a separate signal we don't yet expose. */ proto_drain_fifo(); - uint32_t crc2 = flash_crc32(s * sector_sz, sector_sz); + uint32_t crc2 = scan_crc(s * sector_sz, sector_sz); if (crc2 != crc1) status = SCAN_UNSTABLE; } diff --git a/agent/spi_flash.c b/agent/spi_flash.c index 7a5ad0d..1604e15 100644 --- a/agent/spi_flash.c +++ b/agent/spi_flash.c @@ -652,7 +652,8 @@ static uint8_t nand_read(uint32_t row, uint32_t column, return stalled ? 0xFF : status; } -void flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { +int flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { + int stalled = 0; if (current_flash_type == FLASH_TYPE_NAND) { /* NAND: convert byte offset to (page, column) and read page-by-page * via PAGE_READ + READ_FROM_CACHE. No mode-switching — NAND lives @@ -662,12 +663,13 @@ void flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { uint32_t col = addr % NAND_PAGE_SIZE; uint32_t in_page = NAND_PAGE_SIZE - col; uint32_t chunk = (len < in_page) ? len : in_page; - nand_read(row, col, buf, chunk); + if (nand_read(row, col, buf, chunk) == 0xFF) + stalled = 1; /* nand_read's "never finished" */ buf += chunk; addr += chunk; len -= chunk; } - return; + return stalled ? -1 : 0; } /* NOR: register-based reads (normal mode) instead of memory window. @@ -682,7 +684,8 @@ void flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { fmc_reg(FMC_DATA_NUM) = chunk; fmc_reg(FMC_OP_CFG) = OP_CFG_OEN_EN | OP_CFG_CS(0) | OP_CFG_ADDR_NUM(3); fmc_reg(FMC_OP) = FMC_OP_CMD1_EN | FMC_OP_ADDR_EN | FMC_OP_READ_DATA | FMC_OP_REG_OP_START; - fmc_wait_ready(); + if (fmc_wait_ready()) + stalled = 1; for (uint32_t i = 0; i < chunk; i++) buf[i] = iobuf[i]; @@ -693,6 +696,7 @@ void flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { } fmc_enter_boot(); + return stalled ? -1 : 0; } uint8_t flash_unlock_debug[3]; diff --git a/agent/spi_flash.h b/agent/spi_flash.h index 1c479be..4bd6e72 100644 --- a/agent/spi_flash.h +++ b/agent/spi_flash.h @@ -48,7 +48,9 @@ typedef struct { int flash_init(flash_info_t *info); /* Read flash via memory-mapped window (fastest) */ -void flash_read(uint32_t addr, uint8_t *buf, uint32_t len); +/* Returns 0, or -1 when a controller register op never finished: buf then + * holds stale I/O-buffer bytes, not flash contents. */ +int flash_read(uint32_t addr, uint8_t *buf, uint32_t len); /* Read flash JEDEC ID */ void flash_read_id(uint8_t id[3]); diff --git a/agent/spi_flash_hisfc350.c b/agent/spi_flash_hisfc350.c index 93d790f..d5d8cc9 100644 --- a/agent/spi_flash_hisfc350.c +++ b/agent/spi_flash_hisfc350.c @@ -305,14 +305,16 @@ static void read_step(uint32_t off, const volatile uint8_t *p, for (uint32_t i = 0; i < n; i++) dst[i] = p[i]; } -void flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { +int flash_read(uint32_t addr, uint8_t *buf, uint32_t len) { + /* Memory-mapped (banked) reads: nothing here can stall. */ if (!needs_bank_switch) { const volatile uint8_t *src = (const volatile uint8_t *)(FLASH_MEM + addr); for (uint32_t i = 0; i < len; i++) buf[i] = src[i]; - return; + return 0; } struct read_ctx c = { buf, addr }; flash_walk_banked(addr, len, read_step, &c); + return 0; } struct crc_ctx { uint32_t crc; };