add spi nand driver #50690

rogeryou · 2022-09-27T09:17:22Z

This PR is updated for #43915.

Add SPI NAND block device driver for using SPI NAND Flash like Macronix Flash MX31LF4GE4BC.
This driver is tested on STM32L562E-DK.

The license of BCH code is updated.

erwango

Thanks for this contribution.
Some clean up required.
My main request would be to remove STM32 mentions, since there is nothing STM32 specific here aside from the board used for test.

samples/drivers/spi_nand/README.rst

samples/drivers/spi_nand/boards/stm32l562e_dk.overlay

drivers/flash/CMakeLists.txt.orig

drivers/flash/Kconfig.orig

drivers/flash/bch.c

drivers/flash/spi_nand.c

carlescufi · 2022-09-27T10:36:30Z

@rogeryou can you please format your commits as described here:
https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-guidelines

rogeryou · 2022-09-28T03:20:00Z

@rogeryou can you please format your commits as described here: https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-guidelines

I have formated my commits.

Laczen · 2022-09-29T12:29:23Z

@rogeryou, thank you for this contribution. It would be very nice to have nand flash support in zephyr.

Regarding the PR, I have seen that the ecc is hidden for the user as to be able to use the standard flash interface. This is a good thing, but doesn't this also make any other part of the oob data unavailable ? IMHO it would be good to extend the flash driver for nand flashes with read/write possibilities of the oob data. This would allow adding bad block info to the oob area.

RyKolb · 2022-11-01T03:24:34Z

@rogeryou
It seems as though part of this driver is written for either a 2048 or 4096 page size NAND flash, and yet other parts use the defined page/block offset/mask. Is there reasoning behind this?

drivers/flash/spi_nand.h

samples/drivers/spi_nand/boards/stm32l562e_dk.conf

samples/drivers/spi_nand/prj.conf

samples/drivers/spi_nand/sample.yaml

samples/drivers/spi_nand/src/main.c

rogeryou · 2022-11-23T06:02:40Z

@rogeryou, thank you for this contribution. It would be very nice to have nand flash support in zephyr.

Regarding the PR, I have seen that the ecc is hidden for the user as to be able to use the standard flash interface. This is a good thing, but doesn't this also make any other part of the oob data unavailable ? IMHO it would be good to extend the flash driver for nand flashes with read/write possibilities of the oob data. This would allow adding bad block info to the oob area.

Thank you for your suggestion. We will add APIs that operate OOB later.

rogeryou · 2022-11-23T06:43:05Z

@rogeryou It seems as though part of this driver is written for either a 2048 or 4096 page size NAND flash, and yet other parts use the defined page/block offset/mask. Is there reasoning behind this?

Could you tell me which part of the program you mentioned？

sonarqubecloud · 2025-06-09T14:15:52Z

Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.8% Duplication on New Code

See analysis details on SonarQube Cloud

daniel-0723 · 2025-06-09T14:49:49Z

@de-nordic Please review again.

github-actions · 2025-09-13T00:33:53Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

maxmclau · 2025-09-17T17:17:36Z

would love to see this cleaned up and merged

arnout · 2025-09-17T18:31:45Z

+1 to getting this merged. In a couple of weeks time we'll have the hardware to test this if that is useful to make it progress.

Please remove the stale label.

pdgendt · 2025-09-22T07:02:41Z

There's a build issue for the documentation, but the action is no longer available. Try to rebase and fix the doc issues.

daniel-0723 · 2025-09-22T07:08:00Z

There's a build issue for the documentation, but the action is no longer available. Try to rebase and fix the doc issues.

OK, I will try to fix it.

de-nordic · 2025-09-22T11:10:23Z

drivers/flash/spi_nand.c

+struct spi_nand_config {
+	/* Devicetree SPI configuration */
+	struct spi_dt_spec spi;
+	uint8_t id[2];


Why not use SPI_NAND_ID_LEN here?

de-nordic · 2025-09-22T11:25:00Z

drivers/flash/spi_nand.c

+	if ((addr < 0) || ((addr + size) > data->flash_size)) {
+		return -EINVAL;
+	}


With size == 0 we are supposed to just return 0.
Additionally, to be safe against int overflows, the condition should be:

if ((addr < 0) || (addr >= data->flash_size) || (size > data->flash_size) || ((data->flash_size - addr) < size))

de-nordic · 2025-09-22T11:25:28Z

drivers/flash/spi_nand.c

+	uint32_t written_bytes = 0;
+
+	/* should be between 0 and flash size */
+	if ((addr < 0) || ((addr + size) > flash_size)) {


if ((addr < 0) || (addr >= data->flash_size) || (size > data->flash_size) || ((data->flash_size - addr) < size))

de-nordic · 2025-09-22T11:26:08Z

drivers/flash/spi_nand.c

+		} else {
+#if IS_ENABLED(CONFIG_SPI_NAND_SOFTWARE_ECC)


else if (IS_ENABLED(...)

de-nordic · 2025-09-22T11:26:24Z

drivers/flash/spi_nand.c

+	struct spi_nand_data *data = dev->data;
+	int ret = 0;
+	/* erase area must be subregion of device */
+	if ((addr < 0) || ((size + addr) > data->flash_size)) {


if ((addr < 0) || (addr >= data->flash_size) || (size > data->flash_size) || ((data->flash_size - addr) < size))

embed

Fix the errors and try compiling with CONFIG_SPI_NAND_SOFTWARE_ECC=y to test the bch code as well.

embed · 2025-09-22T15:41:03Z

dts/bindings/mtd/jedec,spi-nand.yaml

+      spi-max-frequency = <500000>;
+      id = [c2 1E];
+      cs-wait-delay = <0>;
+      page-size = <2048>;


Suggested change

page-size = <2048>;

page-size = <2048>;

flash-size = <536870912>;

Since flash-size is mandatory too :-)

embed · 2025-09-23T09:33:40Z

drivers/flash/bch.c

+		k_heap_free(&bch_heap, bch->buf2);
+		k_heap_free(&bch_heap, bch->buf3);
+		k_heap_free(&bch_heap, bch->g);
+		k_heap_free(bch);


Found by copilot, this is not how you call this function.

Suggested change

k_heap_free(bch);

k_heap_free(&bch_heap, bch);

embed · 2025-09-23T09:45:03Z

drivers/flash/spi_nand.c

+			return ret;
+		}
+
+		if (!(reg & SPI_NAND_WIP_BIT) == 0U) {


Why is there the inversion? WIP bit must be zero for us to return. (thank you copilot)

Suggested change

if (!(reg & SPI_NAND_WIP_BIT) == 0U) {

if ((reg & SPI_NAND_WIP_BIT) == 0U) {

embed · 2025-09-23T09:59:20Z

drivers/flash/bch.c

+		LOG_DBG("bch init failed, params should be m: 8 ~ 13, t: 1 ~ 12\r\n");
+		return -EINVAL;
+	}
+	bch = bch_alloc(sizeof(bch_t), &err);


The struct members bch->n and other fields are used before being initialized. The struct should be initialized with proper values (m, t, n) before using them for allocation sizes. (copilot)

Suggested change

bch = bch_alloc(sizeof(bch_t), &err);

bch = bch_alloc(sizeof(bch_t), &err);

if (bch == NULL) {

return -ENOMEM;

}

bch->m = m;

bch->t = t;

bch->n = (1 << m) - 1;

bch->ecc_words = (m * t + 31) / 32;

bch->len = (bch->n + 1) / 8;

also delete the duplicate bch->m = m; etc a few lines down below.

Does not seem to be tested...

de-nordic · 2025-09-23T13:49:44Z

drivers/flash/spi_nand.c

+		LOG_ERR("invalid eccbytes %d, should be %d\n", eccbytes, data->nbc.bch->ecc_words);
+		return -EINVAL;
+	}
+	data->page_buf = (uint8_t *)k_malloc(data->page_size + data->oob_size);


No. https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html, rule 14.

de-nordic · 2025-09-23T13:52:16Z

drivers/flash/bch.c

+	bch->a_pow = bch_alloc((bch->n + 1) * sizeof(*bch->a_pow), &err);
+	bch->a_log = bch_alloc((bch->n + 1) * sizeof(*bch->a_log), &err);
+	bch->mod_tab = bch_alloc(bch->ecc_words * 16 * 4 * sizeof(*bch->mod_tab), &err);
+	bch->ecc = bch_alloc(bch->ecc_words * sizeof(*bch->ecc), &err);
+	bch->ecc2 = bch_alloc(bch->ecc_words * sizeof(*bch->ecc2), &err);
+	bch->buf = bch_alloc((t + 1) * sizeof(*bch->buf), &err);
+	bch->buf2 = bch_alloc((t + 1) * sizeof(*bch->buf2), &err);
+	bch->buf3 = bch_alloc(bch->len, &err);
+	bch->syn = bch_alloc(t * 2 * sizeof(*bch->syn), &err);
+	bch->elp = bch_alloc((t + 1) * sizeof(*bch->elp), &err);
+	bch->g = bch_alloc((bch->ecc_words + 1) * sizeof(*bch->g), &err);


This breaks https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html, rule 14. Haven't this already been mentioned in the review for this part?

The heap is defined with K_HEAP_DEFINE, which is static allocation. Is this not allowed in Zephyr?

@de-nordic , Could you explain more about the dynamic memory allocation issue. I followed your suggestion to modify the memory allocation method to static allocation before. And I found this in drivers/flash/flash_cadence_nand_ll.c.

@de-nordic , Could you explain more about the dynamic memory allocation issue. I followed your suggestion to modify the memory allocation method to static allocation before. And I found this in drivers/flash/flash_cadence_nand_ll.c.

They should not be doing that either. But they potentially kick their users where it hurts, and here seems to be an universal NAND driver.

What you have here is not static. The buffer allocated for your local heap is static, but allocation here is dynamic, aside for literally calling alloc, in lines like

bch->buf3 = bch_alloc(bch->len, &err);

where size of allocated buffer is evaluated at run-time. This also means that overall size of bch object tree is only known at run-time, which makes it hard to evaluate CONFIG_SPI_NAND_BCH_HEAP_SIZE and probably nobody would be able to give you the right value to serve their system, as doing the math requires literally looking at the alloc sequence and calculating the worst case scenario for it.

This also means you are risking situations where order of bch object allocations, because they may be variable in size, can make a difference between success and a failure.
Failure, from lack of mem, is a problem in embedded system that is hard to address; yes, I know that there is nothing you can do when NAND fails, SPI fails, hardware starts to behave wobbly, but with RAM it is configuration or design issue, that will be artificial risk added on top. Both hardware and lack of RAM errors can lead to device replacement, the former just happens the later should be avoided.

It will be also hard to estimate correct size of HEAP here, so probably it will be too much, just in case - which is what?

I am not even sure I want to kick at this, because I want the driver. I just know that one of the first questions I will be asked is how to calculate the memory size for it properly, because people want to use just enough RAM for their case and stop getting at random read/write errors.

If we just take ecc_bits from the device tree instead of the ONFI table, then everything can be calculated statically (other parameters like page size already come from device tree).

@arnout , you are right. The ecc_bits is only runtime factor that affects the heap size. And it can be specified in devicetree. Then what's your suggestion? @de-nordic

Yes it would be perfect if we could calculate all the things at compile time, by looking at devices we want to serve. Generally I would prefer to get close, as much as possible, to just allowing user to select how many bch objects can be used in parallel and do the math for them, for the worst scenario in the system - biggest "ecc" requirements for a single object of the "worst" device.
note also that the bch support may end up being used not only by your code here. In the future I would preferably moved it out to ECC subsystem.

It's not actually possible to do parallel writes (or reads) anyway, so one ECC object per spinand device should be OK, right? You'd need to define a macro to instantiate that object based on DTS parameters, so it's a little bit finnicky.

note also that the bch support may end up being used not only by your code here.

Are you thinking of other (non-SPI) NAND, like parallel NAND? Or something else entirely? It is my impression that NAND is on its way out (replaced by eMMC used in SPI mode). And parallel NAND has been declining for some time already.

Use device tree to get as much compile time characteristics as possible and calculate the the heap size automatically, or at least calculate the biggest bch object and allow user to select number of such objects. You can also set minimal requirement for number of such objects in Kconfig specifically for your driver.

de-nordic · 2025-09-23T14:00:08Z

drivers/flash/bch.c

+	if (ptr == NULL) {
+		*err = 1;
+	}
+	return ptr;


I am quite sure there is memset missing here, for zeroing the reserved buffer. Otherwise sequence

void *ptr = bch_alloc(something, something); // success here bch_free(ptr);

may end up in attempt to free some bogus pointers.

de-nordic · 2025-10-09T09:00:11Z

drivers/flash/bch.c

+#include <zephyr/logging/log.h>
+
+#ifndef CONFIG_SPI_NAND_BCH_HEAP_SIZE
+#define CONFIG_SPI_NAND_BCH_HEAP_SIZE 51200


No non-Kconfig local CONFIG_ definitions, use some alias for that.

rogeryou requested review from erwango, nashif and nvlsianpu as code owners September 27, 2022 09:17

zephyrbot added area: Devicetree platform: STM32 ST Micro STM32 area: Flash labels Sep 27, 2022

zephyrbot requested review from ABOSTM, FRASTM, GeorgeCGV, galak, gmarull and mbolivar-nordic September 27, 2022 09:17

zephyrbot assigned nvlsianpu Sep 27, 2022

erwango requested changes Sep 27, 2022

View reviewed changes

erwango removed the platform: STM32 ST Micro STM32 label Sep 27, 2022

rogeryou force-pushed the macronix_spi_nand_v1 branch from 9755c14 to 901b863 Compare September 28, 2022 03:16

carlescufi requested review from Laczen and removed request for GeorgeCGV, gmarull and mbolivar-nordic November 2, 2022 14:12

erwango previously requested changes Nov 8, 2022

View reviewed changes

rogeryou closed this Nov 23, 2022

rogeryou reopened this Nov 23, 2022

zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Nov 23, 2022

daniel-0723 force-pushed the macronix_spi_nand_v1 branch from e352357 to 84f8da1 Compare June 9, 2025 13:51

danieldegrasse modified the milestones: v4.2.0, v4.3.0 Jul 14, 2025

github-actions bot added the Stale label Sep 13, 2025

nandojve removed the Stale label Sep 17, 2025

nandojve requested review from bbooher, coran21, daniel-0723, de-nordic, diego-v-daq, embed, jacoba-at, noctaia, pdgendt and sslupsky September 17, 2025 19:06

embed mentioned this pull request Sep 22, 2025

drivers: Flash: Add SPI NAND Flash driver support Byteflies/zephyr#3

Merged

de-nordic requested changes Sep 22, 2025

View reviewed changes

embed suggested changes Sep 23, 2025

View reviewed changes

de-nordic reviewed Sep 23, 2025

View reviewed changes

de-nordic reviewed Oct 9, 2025

View reviewed changes

	page-size = <2048>;
	page-size = <2048>;
	flash-size = <536870912>;

	if (!(reg & SPI_NAND_WIP_BIT) == 0U) {
	if ((reg & SPI_NAND_WIP_BIT) == 0U) {

-	bch = bch_alloc(sizeof(bch_t), &err);
+	bch = bch_alloc(sizeof(bch_t), &err);
+	if (bch == NULL) {
+		return -ENOMEM;
+	}
+	bch->m = m;
+	bch->t = t;
+	bch->n = (1 << m) - 1;
+	bch->ecc_words = (m * t + 31) / 32;
+	bch->len = (bch->n + 1) / 8;

add spi nand driver #50690

Are you sure you want to change the base?

add spi nand driver #50690

Conversation

rogeryou commented Sep 27, 2022

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlescufi commented Sep 27, 2022

Uh oh!

rogeryou commented Sep 28, 2022

Uh oh!

Laczen commented Sep 29, 2022

Uh oh!

RyKolb commented Nov 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rogeryou commented Nov 23, 2022

Uh oh!

rogeryou commented Nov 23, 2022

Uh oh!

sonarqubecloud bot commented Jun 9, 2025

Quality Gate passed

Uh oh!

daniel-0723 commented Jun 9, 2025

Uh oh!

github-actions bot commented Sep 13, 2025

Uh oh!

maxmclau commented Sep 17, 2025

Uh oh!

arnout commented Sep 17, 2025

Uh oh!

pdgendt commented Sep 22, 2025

Uh oh!

daniel-0723 commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

embed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment