drivers: flash: spi_nor: optimizations and better Micron flash support #88467

robhancocksed · 2025-04-10T22:27:08Z

Avoid providing redundant TX buffers to the SPI driver for the data portion of read commands where the TX data is not used. This is more efficient and also avoids passing uninitialized data to an external device.
Add support for the flag status register used on some Micron flash devices, which improves error detection and in some cases must be read for the flash device to function properly.
Add support for fast read commands using a dummy byte between the address and data phases, which often allows the flash device to operate at a higher SPI clock rate.

Note, there might be a compliance error raised, as I wasn't able to satisfy both clang-format and checkpatch with regard to how one line was formatted..

robhancocksed · 2025-04-10T23:12:27Z

Looks like the drivers.console.line_splitting.plus.some_option test is currently failing, for presumably unrelated reasons..

robhancocksed · 2025-04-24T17:09:15Z

Ping.. any feedback?

de-nordic · 2025-04-24T18:35:06Z

drivers/flash/spi_nor.c

-	struct spi_buf spi_buf[2] = {
-		{
-			.buf = buf,
-			.len = 1,
-		},
-		{
-			.buf = data,
-			.len = length
-		}
-	};
+	bool has_dummy = (access & NOR_ACCESS_DUMMY_BYTE) != 0U;
+	uint8_t cmd_buf[6] = {opcode};
+	struct spi_buf spi_buf_tx[2] = {{
+						.buf = cmd_buf,
+						.len = 1,
+					},
+					{.buf = is_write ? data : NULL, .len = length}};
+	struct spi_buf spi_buf_rx[2] = {{
+						.buf = NULL,
+						.len = 1,
+					},
+					{.buf = data, .len = length}};


Decide on one style of initialization, preferably where definition is split to several lines.

Agreed, though for some reason the clang-format formatting wanted it this way. Will see if the compliance checks will flag it if I use the multi-line style.

Ended up just using "clang-format off" to stop it from wanting to change this block of code.

de-nordic · 2025-04-24T18:38:47Z

drivers/flash/spi_nor.c

-#define ANY_INST_HAS_DPD DT_ANY_INST_HAS_BOOL_STATUS_OKAY(has_dpd)
-#define ANY_INST_HAS_T_EXIT_DPD DT_ANY_INST_HAS_PROP_STATUS_OKAY(t_exit_dpd)
-#define ANY_INST_HAS_DPD_WAKEUP_SEQUENCE DT_ANY_INST_HAS_PROP_STATUS_OKAY(dpd_wakeup_sequence)
-#define ANY_INST_HAS_RESET_GPIOS DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
-#define ANY_INST_HAS_WP_GPIOS DT_ANY_INST_HAS_PROP_STATUS_OKAY(wp_gpios)
-#define ANY_INST_HAS_HOLD_GPIOS DT_ANY_INST_HAS_PROP_STATUS_OKAY(hold_gpios)
-#define ANY_INST_USE_4B_ADDR_OPCODES DT_ANY_INST_HAS_BOOL_STATUS_OKAY(use_4b_addr_opcodes)
+#define ANY_INST_HAS_DPD                    DT_ANY_INST_HAS_BOOL_STATUS_OKAY(has_dpd)
+#define ANY_INST_HAS_T_EXIT_DPD             DT_ANY_INST_HAS_PROP_STATUS_OKAY(t_exit_dpd)
+#define ANY_INST_HAS_DPD_WAKEUP_SEQUENCE    DT_ANY_INST_HAS_PROP_STATUS_OKAY(dpd_wakeup_sequence)
+#define ANY_INST_HAS_RESET_GPIOS            DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
+#define ANY_INST_HAS_WP_GPIOS               DT_ANY_INST_HAS_PROP_STATUS_OKAY(wp_gpios)
+#define ANY_INST_HAS_HOLD_GPIOS             DT_ANY_INST_HAS_PROP_STATUS_OKAY(hold_gpios)
+#define ANY_INST_USE_4B_ADDR_OPCODES        DT_ANY_INST_HAS_BOOL_STATUS_OKAY(use_4b_addr_opcodes)


Change in formatting is not related to topic of the pr. Don't do unrelated changes like, even if it is style only - it makes it bloats pr. Style changes on lines with no logical changes should get own pr.

use-fast-read should have one of these too, and if there is not device in a system setup that has the property this should evaluate to false.

This is a case where clang-format wanted the new code formatted differently, and so I ended up updating the surrounding block of code to match. I can leave the other lines alone if preferred.

Don't run clang-format on pre-existing units. Overall style fix should be done in separate PR.

Ended up using "clang-format off" here as well.

de-nordic · 2025-04-25T14:25:48Z

drivers/flash/spi_nor.c

+	 * status register since it allows better error detection (and some devices
+	 * that have it require it to be read after a program operation).


Suggested change

* status register since it allows better error detection (and some devices

* that have it require it to be read after a program operation).

* status register since it allows better error detection; some devices,

* which have the register, require it to be read after a program operation.

What was the point of () ?

Reworded this.

de-nordic · 2025-04-25T14:27:48Z

drivers/flash/spi_nor.c

+		if (IS_ENABLED(CONFIG_SPI_NOR_SLEEP_WHILE_WAITING_UNTIL_READY)) {
+			/* Don't monopolise the CPU while waiting for ready */
+			k_sleep(poll_delay);
+		}


Unrelated change.

de-nordic · 2025-04-25T15:07:13Z

drivers/flash/spi_nor.c

-		}
-	};
+	bool has_dummy = (access & NOR_ACCESS_DUMMY_BYTE) != 0U;
+	uint8_t cmd_buf[6] = {opcode};


Needless variable name spawns unneeded changes in lines 419 and 423. Any style changes, renames got to other PR. Needless changes eclipse the subject of PR changes.

Removed variable rename.

de-nordic · 2025-04-25T15:08:49Z

drivers/flash/spi_nor.c

-			.len = length
-		}
-	};
+	uint8_t cmd_buf[5] = {opcode};


This line gets modified in two separate commits.

Not sure how you suggest handling this? Please have a look at the current change set.

de-nordic · 2025-04-25T15:19:29Z

drivers/flash/spi_nor.c

 		if (addr > SPI_NOR_3B_ADDR_MAX) {
-			ret = spi_nor_cmd_addr_read_4b(dev, SPI_NOR_CMD_READ_4B, addr, dest, size);
+			if (cfg->use_fast_read) {


This should be conditional on existence of device that have the fast read. I you have a system that does not have fast read or you do not want is (and do not enable it in dts), then this should not be compiled in.
Example here may be MCUboot, where users may prefer to choose smaller driver at a cost of slower boot.

This driver was providing SPI buffers for both TX and RX on the data payload portion of read transfers, even though the TX buffer is not meaningful in these cases. As well as being less efficient, this also caused likely uninitialized data to be transferred to the device, which is possibly problematic. Update to specify the TX buffer as NULL for the data payload SPI transfer, so that the SPI driver can generate dummy TX data internally. Signed-off-by: Robert Hancock <[email protected]>

Some Micron (and possibly other) SPI NOR devices implement a flag status register which provides more information on the success/failure of erase and program operations. In addition to better error checking, some of these devices actually don't function properly if the flag status register is not read after a program operation (subsequent reads will only return 0xFF bytes). Add a device tree parameter to indicate that the flag status register is supported. When specified, the flag status register will be used for ready/error checks rather than the standard status register. Signed-off-by: Robert Hancock <[email protected]>

Most SPI NOR flash devices support a "fast read" command which uses dummy bits between the address and the start of the data transfer. In many cases, the maximum SPI clock speed of the device is lower for the regular read command due to the limited time between the address and data phases, so using the fast read command will remove this restriction and allow for faster transfers. Add a device tree flag to indicate that fast reads should be used for the device. Signed-off-by: Robert Hancock <[email protected]>

github-actions bot added the area: Flash label Apr 10, 2025

github-actions bot requested review from de-nordic and rghaddab April 10, 2025 22:27

github-actions bot assigned de-nordic Apr 10, 2025

de-nordic requested changes Apr 25, 2025

View reviewed changes

robhancocksed added 3 commits April 25, 2025 11:15

robhancocksed force-pushed the spi-nor-enhancements branch from 1969727 to c3db128 Compare April 25, 2025 17:30

robhancocksed requested a review from de-nordic April 25, 2025 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: flash: spi_nor: optimizations and better Micron flash support #88467

drivers: flash: spi_nor: optimizations and better Micron flash support #88467

robhancocksed commented Apr 10, 2025

robhancocksed commented Apr 10, 2025

robhancocksed commented Apr 24, 2025

de-nordic Apr 24, 2025

robhancocksed Apr 25, 2025

robhancocksed Apr 25, 2025

de-nordic Apr 24, 2025

de-nordic Apr 25, 2025

robhancocksed Apr 25, 2025

de-nordic Apr 25, 2025 •

edited

Loading

robhancocksed Apr 25, 2025

de-nordic Apr 25, 2025

robhancocksed Apr 25, 2025

de-nordic Apr 25, 2025

robhancocksed Apr 25, 2025

de-nordic Apr 25, 2025

robhancocksed Apr 25, 2025

de-nordic Apr 25, 2025

robhancocksed Apr 25, 2025

de-nordic Apr 25, 2025

robhancocksed Apr 25, 2025

		* status register since it allows better error detection (and some devices
		* that have it require it to be read after a program operation).

drivers: flash: spi_nor: optimizations and better Micron flash support #88467

Are you sure you want to change the base?

drivers: flash: spi_nor: optimizations and better Micron flash support #88467

Conversation

robhancocksed commented Apr 10, 2025

robhancocksed commented Apr 10, 2025

robhancocksed commented Apr 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic Apr 25, 2025 •

edited

Loading