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？

daniel-0723 · 2025-01-28T08:58:21Z

Hi @de-nordic , we have fixed the compliance and documentation, please review again.

de-nordic

There should be configuration for existing flash tests added, as the driver implements the Flash API and should work with these tests and samples we already have.

Some comments left, still reviewing.

de-nordic · 2025-02-05T16:22:17Z

drivers/flash/CMakeLists.txt

@@ -63,6 +63,8 @@ zephyr_library_sources_ifdef(CONFIG_SOC_FLASH_SMARTBOND flash_smartbond.c)
 zephyr_library_sources_ifdef(CONFIG_SOC_FLASH_TELINK_B91 soc_flash_b91.c)
 zephyr_library_sources_ifdef(CONFIG_SOC_FLASH_XMC4XXX soc_flash_xmc4xxx.c)
 zephyr_library_sources_ifdef(CONFIG_SPI_FLASH_AT45 spi_flash_at45.c)
+zephyr_library_sources_ifdef(CONFIG_SPI_NAND bch.c)


Not to be fixed with this PR, but just want to mention that I will propose enhancement where bch implementation gets moved to CRC subsystem.

de-nordic · 2025-02-05T16:22:54Z

drivers/flash/Kconfig.nand

@@ -0,0 +1,38 @@
+# Copyright (c) 2022 Macronix International Co., Ltd.


You probably want to update date in copyrights.

drivers/flash/Kconfig.nand

de-nordic · 2025-02-05T16:23:47Z

drivers/flash/Kconfig.nand

+	  Device is connected to SPI bus, it has to
+	  be initialized after SPI driver.
+
+config SPI_NAND_CS_WAIT_DELAY


Shouldn't this be defined in DTS for a device?

de-nordic · 2025-02-05T16:24:03Z

drivers/flash/Kconfig.nand

+	help
+	  This is the wait delay (in us) to allow for CS switching to take effect
+
+config SPI_NAND_IDLE_IN_DPD


Shouldn't this be defined in dts?

de-nordic · 2025-02-05T16:32:30Z

drivers/flash/spi_nand.c

+		LOG_ERR("Enable continuous read failed: %d\n", secur_reg);
+	}
+
+out: release_device(dev);


Labels alone in the line, drop the function call to the next line.

de-nordic · 2025-02-05T16:35:06Z

drivers/flash/spi_nand.c

+	return ret;
+}
+
+static int spi_nand_conti_read_enable(const struct device *dev)


Can we merge the spi_nand_conti_read_enable and spi_nand_conti_read_disable. They are looking very similar.

de-nordic · 2025-02-05T16:36:09Z

drivers/flash/spi_nand.c

+		size -= chunk;
+	}
+
+out: release_device(dev);


Labels get own lines.

de-nordic · 2025-02-05T16:37:53Z

drivers/flash/spi_nand.c

+		offset = addr % data->page_size;
+		chunk = (offset + size < data->page_size) ?
+			size : (data->page_size - offset);
+		read_bytes = chunk;


Do we need read_bytes; it seem that is equivalent of chunk, through the loop, and not used outside.

drivers/flash/spi_nand.c

embed · 2025-02-25T16:25:00Z

samples/drivers/spi_nand/README.rst

+   :name: JEDEC SPI-NAND flash
+   :relevant-api: flash_interface
+
+   Use the flash API to interact with an SPI NOR serial flash memory device.


Suggested change

Use the flash API to interact with an SPI NOR serial flash memory device.

Use the flash API to interact with an SPI NAND serial flash memory device.

daniel-0723 · 2025-03-19T10:17:08Z

Hi @de-nordic @embed , since Zephyr has supported Multi-bit SPI bus last year, do we need to rework this PR based on MSPI (drivers/mspi.h) instead of SPI (drivers/spi.h)? Many controller IP core such as FlexSPI, stm32-ospi, cadence-xSPI or Renesas-xSPI support QSPI NAND Flash.

embed · 2025-03-19T11:30:43Z

@daniel-0723 I follow the philosophy of first make it work. How much effort is it to finish this as a proper SPI driver and then have another PR for adding MSPI support? I would vote for meeting the original PR goal, that is a working generic SPI-NAND driver. CC: @de-nordic

On another note, I can't reply to your comment for some reason so I made a new comment.

embed · 2025-03-19T11:40:31Z

drivers/flash/spi_nand.h

+#define SPI_NAND_CMD_ECC_STAT_READ   0x7C
+
+/* Page, sector, and block size are standard, not configurable. */
+#define SPI_NAND_PAGE_SIZE    2048


Page and block sizes should either come from device tree, or better from ONFI table.

+1 for leveraging the ONFI table.

Hi @embed @bbooher , I have updated the PR, now page sizes and sub page size come from ONFI table.

de-nordic · 2025-05-18T20:28:18Z

drivers/flash/spi_nand.h

+
+/* Status register bits */
+#define SPI_NAND_WIP_BIT         BIT(0)  /* Write in progress */
+#define SPI_NAND_WEL_BIT         BIT(1)  /* Write enable latch */


Unused, remove.

de-nordic · 2025-05-18T20:34:50Z

drivers/flash/spi_nand.h

+/* Status Register Bits 0xC0 */
+#define SPINAND_STATUS_BIT_WIP             0x1  /* Write In Progress */
+#define SPINAND_STATUS_BIT_WEL             0x2  /* Write Enable Latch */
+#define SPINAND_STATUS_BIT_ERASE_FAIL      0x4  /* Erase failed */
+#define SPINAND_STATUS_BIT_PROGRAM_FAIL    0x8  /* Program failed */
+#define SPINAND_STATUS_BIT_ECC_STATUS_MASK 0x30 /* ECC status */
+#define SPINAND_STATUS_ECC_STATUS_NO_ERR     0x00
+#define SPINAND_STATUS_ECC_STATUS_ERR_COR    0x10
+#define SPINAND_STATUS_ECC_STATUS_ERR_NO_COR 0x20


Unused, remove.

de-nordic · 2025-05-18T20:35:14Z

drivers/flash/spi_nand.h

+#define SPINAND_SECURE_BIT_CONT        0x04  /* continuous read enable */
+#define SPINAND_SECURE_BIT_ECC_EN      0x10  /* On-die ECC enable */
+#define SPINAND_SECURE_BIT_OTP_EN      0x40
+#define SPINAND_SECURE_BIT_OTP_PROT    0x80


Unused, remove.

de-nordic · 2025-05-18T20:35:27Z

drivers/flash/spi_nand.h

+#define SPINAND_STATUS_ECC_STATUS_ERR_NO_COR 0x20
+
+/* Secure OTP Register Bits  0xB0 */
+#define SPINAND_SECURE_BIT_QE          0x01  /* Quad enable */


Unused, remove.

de-nordic · 2025-05-18T20:36:01Z

drivers/flash/spi_nand.h

+#define  SPINAND_BLOCK_PROT_BIT_SP      0x01
+#define  SPINAND_BLOCK_PROT_BIT_COMPLE  0x02
+#define  SPINAND_BLOCK_PROT_BIT_INVERT  0x04
+#define  SPINAND_BLOCK_PROT_BIT_BP0     0x08
+#define  SPINAND_BLOCK_PROT_BIT_BP1     0x10
+#define  SPINAND_BLOCK_PROT_BIT_BP2     0x20
+#define  SPINAND_BLOCK_PROT_BIT_BPRWD   0x80


Unused, remove.

de-nordic · 2025-05-18T21:17:50Z

drivers/flash/spi_nand.c

+	}
+
+	/* size must be a multiple of sub-page */
+	if ((size % driver_cfg->sub_page_size) != 0) {


size can still be bigger then the device.

de-nordic · 2025-05-18T21:22:05Z

drivers/flash/spi_nand.c

+	while (size > 0) {
+
+		/* Don't write more than a page. */
+		offset = addr % data->page_size;


I assume that page_size may be bigger than sub_page_size, in which case this alignment in this line may lead to offset pointing to whole page before offset of sub_page_size, that has been accepted by line 534.

de-nordic · 2025-05-18T21:23:55Z

drivers/flash/spi_nand.c

+	if ((size % data->block_size) != 0) {
+		return -EINVAL;
+	}
+


No check for addr being positive and size + addr to fit into real device size.

de-nordic · 2025-05-18T21:25:00Z

drivers/flash/spi_nand.c

+
+	/* verify that eccbytes has the expected value */
+	if (data->nbc.bch->ecc_words * 4 != eccbytes) {
+		LOG_ERR("invalid eccbytes %u, should be %u\n",


Shouldn't this print hex rather than decimal?

de-nordic · 2025-05-18T21:26:23Z

drivers/flash/spi_nand.c

+
+	data->page_buf = (uint8_t *)k_malloc(data->page_size + data->oob_size);
+	if (data->page_buf == NULL) {
+		LOG_ERR("Not enougn heap\n");


Why pass-through? Shouldn't that just fail the entire operation already?

Add SPI NAND Flash driver support Signed-off-by: Daniel Zhang <danielzhang@mxic.com.cn>

sonarqubecloud · 2025-05-28T02:31:49Z

Quality Gate passed

Issues
33 New issues
0 Accepted issues

Measures
2 Security Hotspots
0.0% Coverage on New Code
2.4% Duplication on New Code

See analysis details on SonarQube Cloud

pdgendt · 2025-05-28T06:45:36Z

drivers/flash/spi_nand.c

+	return &flash_nand_parameters;
+}
+
+static const struct flash_driver_api spi_nand_api = {


Driver API needs to be wrapped in macro.

Suggested change

static const struct flash_driver_api spi_nand_api = {

static DEVICE_API(flash, spi_nand_api) = {

pdgendt · 2025-05-28T06:46:48Z

drivers/flash/spi_nand.c

+DEVICE_DT_INST_DEFINE(0, &spi_nand_init, NULL,
+		 &spi_nand_data_0, &spi_nand_config_0,
+		 POST_KERNEL, CONFIG_SPI_NAND_INIT_PRIORITY,
+		 &spi_nand_api);


If it would be possible to have multiple instances use, DT_INST_FOREACH_STATUS_OKAY

pdgendt · 2025-05-28T06:47:17Z

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

+    required: true
+    type: int
+    description: |
+      Size (in bytes) of the Flash memory.


Add a trailing newline to the file.

pdgendt · 2025-05-28T06:48:01Z

samples/drivers/spi_nand/prj.conf

@@ -0,0 +1,3 @@
+CONFIG_STDOUT_CONSOLE=y
+CONFIG_FLASH=y
+CONFIG_HEAP_MEM_POOL_SIZE=100000


Missing trailing newline.

pdgendt · 2025-05-28T06:48:33Z

samples/drivers/spi_nand/CMakeLists.txt

Do we want driver specific samples? Can't you re-use the existing flash driver sample?

pdgendt · 2025-05-28T06:54:12Z

drivers/flash/spi_nand.c

+		LOG_ERR("get feature failed: %d", ret);
+		goto out;
+	}
+	if (!(secur_reg & SPINAND_SECURE_BIT_CONT)) {


Suggested change

if (!(secur_reg & SPINAND_SECURE_BIT_CONT)) {

if ((secur_reg & SPINAND_SECURE_BIT_CONT) == 0U) {

pdgendt · 2025-05-28T06:55:11Z

drivers/flash/spi_nand.c

+		LOG_ERR("get feature failed: %d", ret);
+		goto out0;
+	}
+	if (!(secur_reg & SPINAND_SECURE_BIT_OTP_EN)) {


Suggested change

if (!(secur_reg & SPINAND_SECURE_BIT_OTP_EN)) {

if ((secur_reg & SPINAND_SECURE_BIT_OTP_EN) == 0U) {

pdgendt · 2025-05-28T06:55:49Z

drivers/flash/spi_nand.c

+			release_device(dev);
+		}
+
+		if (onfi_table[ONFI_CONT_READ_168] & 0x02) {


Add a define for the 0x02 constant?

Suggested change

if (onfi_table[ONFI_CONT_READ_168] & 0x02) {

if ((onfi_table[ONFI_CONT_READ_168] & 0x02) != 0U) {

pdgendt · 2025-05-28T06:56:07Z

drivers/flash/spi_nand.c

+		goto out1;
+	}
+
+	if (secur_reg & SPINAND_SECURE_BIT_OTP_EN) {


Suggested change

if (secur_reg & SPINAND_SECURE_BIT_OTP_EN) {

if ((secur_reg & SPINAND_SECURE_BIT_OTP_EN) != 0U) {

pdgendt · 2025-05-28T06:56:25Z

drivers/flash/spi_nand.c

+		goto out;
+	}
+	/* Only clear if GET_FEATURE worked and something's set. */
+	if (reg & SPINAND_BLOCK_PROT_BIT_BP_MASK) {


Suggested change

if (reg & SPINAND_BLOCK_PROT_BIT_BP_MASK) {

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

rogeryou requested review from nashif, erwango 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, galak, GeorgeCGV, 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 6 times, most recently from 66a06e6 to 9d94e4c Compare January 28, 2025 01:25

de-nordic reviewed Feb 5, 2025

View reviewed changes

embed reviewed Feb 25, 2025

View reviewed changes

daniel-0723 force-pushed the macronix_spi_nand_v1 branch from 9d94e4c to e43cdde Compare February 27, 2025 08:33

de-nordic assigned de-nordic and unassigned nvlsianpu Feb 27, 2025

de-nordic added this to the v4.2.0 milestone Feb 27, 2025

daniel-0723 force-pushed the macronix_spi_nand_v1 branch from e43cdde to 04e8854 Compare February 27, 2025 10:57

embed reviewed Mar 19, 2025

View reviewed changes

daniel-0723 force-pushed the macronix_spi_nand_v1 branch from 04e8854 to 0c4ca12 Compare March 26, 2025 10:34

daniel-0723 force-pushed the macronix_spi_nand_v1 branch 4 times, most recently from bce4642 to 6077f25 Compare April 25, 2025 09:44

de-nordic requested changes May 18, 2025

View reviewed changes

daniel-0723 force-pushed the macronix_spi_nand_v1 branch 3 times, most recently from 1ef8ab7 to 7ae596a Compare May 27, 2025 10:21

drivers: Flash: Add SPI NAND Flash driver support

eaf79f9

Add SPI NAND Flash driver support Signed-off-by: Daniel Zhang <danielzhang@mxic.com.cn>

daniel-0723 force-pushed the macronix_spi_nand_v1 branch from 555fd5d to eaf79f9 Compare May 28, 2025 01:57

pdgendt requested changes May 28, 2025

View reviewed changes

		@@ -0,0 +1,38 @@
		# Copyright (c) 2022 Macronix International Co., Ltd.

	Use the flash API to interact with an SPI NOR serial flash memory device.
	Use the flash API to interact with an SPI NAND serial flash memory device.

	static const struct flash_driver_api spi_nand_api = {
	static DEVICE_API(flash, spi_nand_api) = {

	if (!(secur_reg & SPINAND_SECURE_BIT_CONT)) {
	if ((secur_reg & SPINAND_SECURE_BIT_CONT) == 0U) {

	if (!(secur_reg & SPINAND_SECURE_BIT_OTP_EN)) {
	if ((secur_reg & SPINAND_SECURE_BIT_OTP_EN) == 0U) {

	if (onfi_table[ONFI_CONT_READ_168] & 0x02) {
	if ((onfi_table[ONFI_CONT_READ_168] & 0x02) != 0U) {

	if (secur_reg & SPINAND_SECURE_BIT_OTP_EN) {
	if ((secur_reg & SPINAND_SECURE_BIT_OTP_EN) != 0U) {

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

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!

daniel-0723 commented Jan 28, 2025

Uh oh!

de-nordic 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-0723 commented Mar 19, 2025

Uh oh!

embed commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

embed commented Mar 19, 2025 •

edited

Loading