Enable quad and octal mode in spi_nor.c to support QPI Flash and Octal Flash #65659

daniel-0723 · 2023-11-23T08:53:20Z

This PR is to support SPI extended modes, like 1-1-2/1-2-2/1-1-4/1-4-4/1-4D-4D/4-4-4/8-8-8/8D-8D-8D.

1-4D-4D :
1 means command is sent on 1 line in STR mode；
the first 4D means address is sent on 4 lines in DTR mode；
the second 4D means data is sent on 4 lines in DTD mode；

8D-8D-8D :
the first 8D means command is sent on 8 lines in DTR mode；
the second 8D means address is sent on 8 lines in DTR mode；
the third 8D means data is sent on 8 lines in DTD mode；

github-actions · 2023-11-23T08:53:58Z

Hello @daniel-0723, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

erwango · 2023-11-23T10:25:54Z

Thanks for this contribution.

Please:

Check your commit message. It should conform to the following template:

<dir>: <dir>: <Title summarizing the change>

<A description useful to the reader>

Signed-off-by: foo <[email protected]>

Also, your PR introduces a lot of changes that would deserve an individual explanation. So please split your change into multiple commits with proper justification for each of them.

erwango · 2023-11-23T10:27:34Z

drivers/spi/ospi_ll_stm32.c

@@ -0,0 +1,388 @@
+/*


There is already a OSPI STM32 driver (see drivers/flash/flash_stm32_ospi.c), please explain the added value of this one and why the existing one can't be fixed.

You can refer to this link for more information.

There is already a OSPI STM32 driver (see drivers/flash/flash_stm32_ospi.c)

What we wanted to do before was to make spi_nor.c compatible with ospi controller, so we took STM32 ospi as an example. And implemented ospi_ll_stm32.c which is partially porting from (drivers/flash/flash_stm32_ospi.c), provided interface for spi_nor.c to call.

erwango · 2023-11-23T10:27:53Z

drivers/spi/ospi_ll_stm32.c

@@ -0,0 +1,388 @@
+/*
+ * Copyright (c) 2022 Macronix.


Please use you own copyright.

erwango · 2023-11-23T10:28:24Z

drivers/spi/ospi_ll_stm32.c

Why "ll", you're using HAL API

erwango · 2023-11-23T10:28:54Z

drivers/spi/Kconfig.stm32_ospi

+	bool "STM32 MCU OSPI controller driver"
+	default y
+	depends on SOC_FAMILY_STM32
+	select USE_STM32_LL_OSPI


You're not using ll API, not required

erwango · 2023-11-23T10:29:25Z

drivers/spi/Kconfig.stm32_ospi

+#if SPI_STM32
+
+#config SPI_STM32_INTERRUPT
+#	bool "STM32 MCU SPI Interrupt Support"
+#	help
+#	  Enable Interrupt support for the SPI Driver of STM32 family.
+
+#config SPI_STM32_DMA
+#	bool "STM32 MCU SPI DMA Support"
+#	select DMA
+#	help
+#	  Enable the SPI DMA mode for SPI instances
+#	  that enable dma channels in their device tree node.
+
+#config SPI_STM32_USE_HW_SS
+#	bool "STM32 Hardware Slave Select support"
+#	default y
+#	help
+#	  Use Slave Select pin instead of software Slave Select.
+
+
+#endif # SPI_STM32


If not needed, please clean it.

erwango · 2023-11-23T10:30:01Z

dts/bindings/spi/spi-device.yaml


 include: [base.yaml, power.yaml]

-on-bus: spi


This should be properly explained as it will have many impacts.

What we want to do is to make spi_nor.c compatible with ospi and qspi controllers ("bus: qspi" and "bus: ospi") such as STM32 qspi/ospi controller, not just limited to spi controller ("bus: spi"). But "on-bus: spi" property bings the compatible "jedec_spi_nor" with "bus: spi".

Also , I found dts/bindings/mtd/nxp,xspi-device.yaml includes spi-device.yaml. In other words, NXP xspi nodes are binding to spi controller ("bus: spi") because the "on-bus: spi" property. This can work now is because the NXP flexspi controller has no "bus: ospi" or "bus: qspi" property (dts/bindings/spi/nxp,imx-flexspi.yaml).

erwango · 2023-11-23T10:30:17Z

include/zephyr/drivers/spi.h

 struct spi_buf {
 	void *buf;
 	size_t len;
+#ifdef CONFIG_SPI_EXTENDED_MODES


This should be done in a dedicated commit.

This should be done in a dedicated commit.

Do we need to submit a new pull request for it? Or can there be multiple commits in a pull request?

erwango · 2023-11-23T10:30:53Z

boards/arm/stm32l562e_dk/stm32l562e_dk_common.dtsi

-		compatible = "st,stm32-ospi-nor";
+		compatible = "jedec,spi-nor";
+		label = "MX25LM51245";
 		reg = <0>;
-		ospi-max-frequency = <DT_FREQ_M(50)>;
-		size = <DT_SIZE_M(512)>; /* 64 MBytes */
-		spi-bus-width = <OSPI_OPI_MODE>;
-		data-rate = <OSPI_DTR_TRANSFER>;
-		four-byte-opcodes;
+		spi-max-frequency = <DT_FREQ_M(50)>;
+		size = <DT_SIZE_M(64)>;
+                jedec-id = [c2 85 3a];
+                duplex = <0>;
+                frame-format = <0>;


Please explain these changes

What we wanted to do before was to make spi_nor.c compatible with ospi controller and nodes. If this is possible in the future, these nodes such as mx25lm51245g will be compatible with "jedec,spi-nor".

de-nordic

IMHO there is more work needed to cut down what is compiled in and allow to select which modes are supported.
As far as I can tell the bits that state SPI line configuration are set at compile time, for example with SPI_CONFIG_DT_INST, which means that most of mode switching code will never be used as the design of hardware exactly specifies by how many lines the SPI device is connected.
A designer of hardware should be allowed to pre-select supported modes and cut support to only these, to cut on flash usage by the driver.

de-nordic · 2023-11-27T10:51:46Z

drivers/flash/spi_nor.c

+#ifdef CONFIG_SPI_EXTENDED_MODES
+
+	rc = spi_nor_change_protocol(dev, spi_config_get_lines(spi_cfg));
+	//rc = spi_nor_change_protocol(dev, PROTO_8_8_8);


tbursztyka

If you really need quad/octal modes, please refer to #39991

Which is a more generic SPI API update than yours. (ALL generic SPI specific configurations bits should land in spi.h, but it should stay generic and provide no specialization towards a use case such as nor flash for instance).

I would be interested to know if you could use that API update instead.

de-nordic · 2024-01-31T14:07:29Z

If you really need quad/octal modes, please refer to #39991

Which is a more generic SPI API update than yours. (ALL generic SPI specific configurations bits should land in spi.h, but it should stay generic and provide no specialization towards a use case such as nor flash for instance).

I would be interested to know if you could use that API update instead.

Planning to reopen the PR?

tbursztyka · 2024-02-01T11:21:03Z

Planning to reopen the PR?

Sure

github-actions · 2024-04-02T00:29:40Z

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.

rruuaanng · 2025-01-13T02:37:47Z

@daniel-0723 You can continue your work. Based on the discussion, there are still some unresolved issues.

github-actions · 2025-03-15T00:35:02Z

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.

daniel-0723 · 2025-03-16T15:07:22Z

Hi @de-nordic @rruuaanng , I have updated this PR's branch, please remove the "stale" label and review it.

rruuaanng · 2025-03-17T02:19:19Z

@daniel-0723 You may temporarily adopt the suggestions made by the other maintainers mentioned above, as they appear to be incomplete. Once they are completed, I will review them again.

daniel-0723 · 2025-03-17T03:46:22Z

which means that most of mode switching code will never be used as the design of hardware exactly specifies by how many lines the SPI device is connected.

Many Flash have low power and high performance modes. Also, Flash in 8D-8D-8D mode consumes more power than in 1S-1S-1S but of high performance, which requires the Flash mode can be switched in driver code to meet low power and high performance scenario.

Enable quad and octal mode in spi_nor.c Signed-off-by: Daniel Zhang <[email protected]>

tbursztyka · 2025-03-18T18:52:46Z

I already told you this needs to be redone almost completely.

It should be generic (dual/quad/octal modes are generic and not specifically meant for flash NOR only)
It should not bloat the thing up. (the amount of attributes you add in spi_buf is unbelievably too large)
Your changes on spi_buf is a direct no-go anyway: unaligned struct, bitfields on a public API, etc etc...

Again, I proposed years ago #39991 , nobody backed it up (sure it would have needed changes and refinements). In meetings it was decided that it was unnecessary, that qspi and all could get a direct flash driver, and no other device would ever need it. And then this happened: #71769

daniel-0723 · 2025-03-19T08:33:41Z

And then this happened: #71769

Thank you for your comment. I think maybe I should rework it and move these work to drivers/flash/flash_mspi_nor.c.

github-actions · 2025-05-19T00:41:10Z

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.

etienne-lms · 2025-03-19T11:06:24Z

drivers/flash/spi_nor.c

+		cr2 = 0x02;
+		ret = spi_nor_wrcr2(dev, cr2);
+		data->nor_protocol = PROTO_8D_8D_8D;
+		break;


default case missing

etienne-lms · 2025-05-19T17:24:58Z

drivers/flash/spi_nor.c

 static int spi_nor_read(const struct device *dev, off_t addr, void *dest,
 			size_t size)
 {
+	struct spi_nor_data *data = dev->data;


This will trigger a build warning on data being unused when CONFIG_SPI_EXTENDED_MODES is disabled.

Suggestion: at line 1052:

- struct spi_nor_data *data = dev->data; + uint8_t read_cmd = SPI_NOR_CMD_READ; const size_t flash_size = dev_flash_size(dev); int ret; + if (IS_ENABLED(CONFIG_SPI_EXTENDED_MODES)) + read_cmd = ((struct spi_nor_data *)(dev->data))->read_cmd;

Ditto in spi_nor_write().

etienne-lms · 2025-05-19T17:30:05Z

drivers/flash/spi_nor.c

+		nor_protocol = PROTO_1_1_1;
+	}
+
+	return nor_protocol;


Nitpicking suggestion:

static enum spi_nor_protocol spi_config_get_lines(const struct spi_config *config) { switch (config->operation & SPI_LINES_MASK) { case SPI_LINES_SINGLE: return PROTO_1_1_1; case SPI_LINES_DUAL: return PROTO_1_2_2; case SPI_LINES_QUAD: return PROTO_4_4_4; case SPI_LINES_OCTAL: return PROTO_8_8_8; default: return PROTO_1_1_1; } }

etienne-lms · 2025-05-19T17:30:38Z

drivers/flash/spi_nor.c

 #endif

+#ifdef CONFIG_SPI_EXTENDED_MODES
+


Could you remove this empty line?

etienne-lms · 2025-05-19T17:31:20Z

include/zephyr/drivers/spi.h

 	 */
 	size_t len;
+
+	#ifdef CONFIG_SPI_EXTENDED_MODES


Prefer without this tabulation indentation.

etienne-lms · 2025-05-19T17:34:31Z

drivers/flash/spi_nor.c

+		data->read_cmd = SPI_NOR_CMD_READ;
+		data->program_cmd = SPI_NOR_CMD_PP;
+		data->erase_cmd = SPI_NOR_CMD_SE;
+		break;


Could shrink the base cases:

case PROTO_1_1_1: case PROTO_1_1_2: case PROTO_1_2_2: case PROTO_1_1_4: case PROTO_1_4_4: case PROTO_1_4D_4D: data->nor_protocol = nor_protocol; data->flag_access_32bit = false; data->read_cmd = SPI_NOR_CMD_READ; data->program_cmd = SPI_NOR_CMD_PP; data->erase_cmd = SPI_NOR_CMD_SE; break;

etienne-lms · 2025-05-19T17:35:35Z

drivers/flash/spi_nor.c

+	int ret = spi_nor_cmd_write(dev, SPI_NOR_CMD_WREN);
+
+	if (ret == 0) {
+


Could you remove this empty line?

etienne-lms · 2025-05-19T17:36:12Z

drivers/flash/spi_nor.c

+ * function.
+ *
+ * @param dev Device struct
+ * @param sr The new value of the configuration register2


Replace sr with cr.

github-actions · 2025-07-19T00:40:58Z

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.

daniel-0723 requested review from ABOSTM, FRASTM and erwango as code owners November 23, 2023 08:53

zephyrbot added area: SPI SPI bus area: Devicetree Binding PR modifies or adds a Device Tree binding platform: STM32 ST Micro STM32 area: Flash labels Nov 23, 2023

zephyrbot requested review from Desvauxm-st, GeorgeCGV, de-nordic, galak, gautierg-st, tbursztyka and teburd November 23, 2023 08:54

zephyrbot assigned tbursztyka Nov 23, 2023

erwango requested changes Nov 23, 2023

View reviewed changes

de-nordic requested changes Nov 27, 2023

View reviewed changes

tbursztyka requested changes Jan 31, 2024

View reviewed changes

tbursztyka mentioned this pull request Feb 1, 2024

SPI: Add API support for DDR, Dual, Quad and Octal modes #39991

Closed

github-actions bot added the Stale label Apr 2, 2024

github-actions bot closed this Apr 16, 2024

daniel-0723 mentioned this pull request Jan 13, 2025

Suggestion on the framework of Flash driver #46399

Open

rruuaanng reopened this Jan 13, 2025

zephyrbot added the area: Devicetree label Jan 13, 2025

zephyrbot requested a review from djiatsaf-st January 13, 2025 02:35

zephyrbot requested review from marwaiehm-st, mathieuchopstm and rghaddab January 13, 2025 02:35

github-actions bot removed the Stale label Jan 14, 2025

github-actions bot added the Stale label Mar 15, 2025

github-actions bot removed the Stale label Mar 17, 2025

daniel-0723 force-pushed the macronix_octal_quad_mode branch from 1464b46 to eb75cb9 Compare March 17, 2025 02:21

rruuaanng requested review from de-nordic, erwango and tbursztyka March 17, 2025 02:25

daniel-0723 force-pushed the macronix_octal_quad_mode branch 2 times, most recently from 95284a5 to 9e9b4a8 Compare March 17, 2025 15:12

drivers: Flash: Enable quad and octal mode in spi_nor.c

9cc87b2

Enable quad and octal mode in spi_nor.c Signed-off-by: Daniel Zhang <[email protected]>

daniel-0723 force-pushed the macronix_octal_quad_mode branch from 9e9b4a8 to 9cc87b2 Compare March 18, 2025 07:11

github-actions bot added the Stale label May 19, 2025

etienne-lms reviewed May 19, 2025

View reviewed changes

github-actions bot removed the Stale label May 20, 2025

github-actions bot added the Stale label Jul 19, 2025

github-actions bot closed this Aug 2, 2025

		int ret = spi_nor_cmd_write(dev, SPI_NOR_CMD_WREN);

		if (ret == 0) {

Enable quad and octal mode in spi_nor.c to support QPI Flash and Octal Flash #65659

Enable quad and octal mode in spi_nor.c to support QPI Flash and Octal Flash #65659

Uh oh!

Conversation

daniel-0723 commented Nov 23, 2023

Uh oh!

github-actions bot commented Nov 23, 2023

Uh oh!

erwango commented Nov 23, 2023

Uh oh!

erwango Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-0723 Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

daniel-0723 Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-0723 Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

de-nordic commented Jan 31, 2024

Uh oh!

tbursztyka commented Feb 1, 2024

Uh oh!

github-actions bot commented Apr 2, 2024

Uh oh!

rruuaanng commented Jan 13, 2025

Uh oh!

github-actions bot commented Mar 15, 2025

Uh oh!

daniel-0723 commented Mar 16, 2025

Uh oh!

rruuaanng commented Mar 17, 2025

Uh oh!

daniel-0723 commented Mar 17, 2025

Uh oh!

tbursztyka commented Mar 18, 2025

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented May 19, 2025

Uh oh!

Choose a reason for hiding this comment

erwango Nov 23, 2023 •

edited

Loading

daniel-0723 Mar 17, 2025 •

edited

Loading

daniel-0723 Mar 17, 2025 •

edited

Loading

daniel-0723 Mar 17, 2025 •

edited

Loading

daniel-0723 commented Mar 19, 2025 •

edited

Loading