drivers: flash: add Andes qspi-nor driver #58718

wtlee1995 · 2023-06-01T03:09:14Z

Added Andes qspi-nor driver which supports jedec sfdp parameters on adp_xc7k_ae350 board.

Signed-off-by: Wei-Tai Lee wtlee@andestech.com

edersondisouza · 2023-06-21T17:10:36Z

boards/riscv/adp_xc7k_ae350/adp_xc7k_ae350.dts

@@ -26,7 +26,8 @@
 		zephyr,console = &uart1;
 		zephyr,shell-uart = &uart1;
 		zephyr,sram = &dram;
-		zephyr,flash = &flash0;
+		zephyr,flash = &mx25u16;


Commit message is confusing - is flash or i2c driver being supported?

Thanks. I've edited the commit message.

edersondisouza · 2023-07-17T18:51:21Z

drivers/flash/flash_andes_qspi.c

+ * @param addr The address to send
+ * @param data The buffer to store or read the value
+ * @param length The size of the buffer
+ * @return 0 on success, negative errno code otherwise


This seems to actually always return 0 - maybe the goto exit below need to set a negative return value?

As I haven't identified the specific error case that this function should handle, I have temporarily set it to always return 0.
I've temporarily removed the comment ', negative errno code otherwise' until I identify the case that requires the function to return an error code.

edersondisouza · 2023-07-17T18:53:05Z

drivers/flash/flash_andes_qspi.c

+			(0x2 << TFMAT_ADDR_LEN_OFFSET)), QSPI_TFMAT(dev));
+		sys_write32(addr, QSPI_ADDR(dev));
+		/* Address phase enable */
+		tctrl |= TCTRL_ADDR_EN_MSK;


What if is_addressed is true and length is 0? It would exit early but it would had set the "address" access. Would something bad happen on next access?

No, it wouldn't. is_addressed is true and length is 0 is a common usage for andes flash driver.

Using Andes flash controller IP, I can split a spi transaction into three phases: command, address, and data. is_addressed means whether the address phase exists. length means the length of the data phase.
Some flash commands contain only the command and address phase, like erase commands, which don't contain data phase.

edersondisouza · 2023-07-17T18:56:26Z

drivers/flash/flash_andes_qspi.c

+
+			/* Don't write across a page boundary */
+			if (((addr + to_write - 1U) / page_size)
+			!= (addr / page_size)) {


That's a really peculiar indentation... maybe != should follow the second ( above?

edersondisouza · 2023-07-17T18:59:42Z

drivers/flash/flash_andes_qspi.c

+	release_device(dev);
+	return ret;
+}
+static int flash_andes_qspi_erase(const struct device *dev,


Nit: missing newline before this.

Thanks, I've edited it.

carlocaione · 2023-07-21T06:53:31Z

dts/bindings/mtd/andestech,qspi-nor.yaml

+description:
+  Properties supporting Zephyr qspi-nor flash driver control of
+  flash memories using the standard M25P80-based command set.
+
+compatible: "andestech,qspi-nor"


compatible before description

carlocaione · 2023-07-21T06:54:18Z

drivers/flash/Kconfig.andes

+	select FLASH_HAS_PAGE_LAYOUT
+	select FLASH_JESD216
+	select FLASH_HAS_DRIVER_ENABLED
+	depends on !(SPI_NOR)


useless ( )

carlocaione · 2023-07-21T06:54:50Z

drivers/flash/flash_andes_qspi.c

+	uint32_t tx_fifo_size;
+	uint32_t rx_fifo_size;
+	uint8_t	 *tx_buf;
+	uint8_t	 *rx_buf;


odd indentation

carlocaione · 2023-07-21T06:56:24Z

drivers/flash/flash_andes_qspi.c

+#define flash_andes_qspi_cmd_read(dev, opcode, dest, length)		\
+	flash_andes_qspi_access(dev, opcode, 0, 0, dest, length)
+#define flash_andes_qspi_cmd_addr_read(dev, opcode, addr, dest, length) \
+	flash_andes_qspi_access(dev, opcode,				\
+		ANDES_ACCESS_ADDRESSED, addr, dest, length)
+#define flash_andes_qspi_cmd_write(dev, opcode)				\
+	flash_andes_qspi_access(dev, opcode, ANDES_ACCESS_WRITE, 0, NULL, 0)
+#define flash_andes_qspi_cmd_addr_write(dev, opcode, addr, src, length)	\
+	flash_andes_qspi_access(dev, opcode,				\
+		ANDES_ACCESS_WRITE | ANDES_ACCESS_ADDRESSED,		\
+		addr, (void *)src, length)


move this on top, and make this looking better please.

carlocaione · 2023-07-21T06:57:39Z

drivers/flash/flash_andes_qspi.c

+#if defined(CONFIG_FLASH_ANDES_QSPI_SFDP_RUNTIME)	\
+	|| defined(CONFIG_FLASH_JESD216_API)


Please, make the code looks nicer:

#if defined(....) || \ defined(....)

carlocaione · 2023-07-21T06:58:31Z

drivers/flash/flash_andes_qspi.c

+	int ret;
+
+	ret = flash_andes_qspi_cmd_write(dev, (write_protect) ?
+		FLASH_ANDES_CMD_WRDI : FLASH_ANDES_CMD_WREN);
+
+	return ret;


ret not needed

carlocaione · 2023-07-21T06:59:44Z

drivers/flash/flash_andes_qspi.h

+#define FLASH_ANDES_CMD_WRDI	0x04	/* Write disable */
+#define FLASH_ANDES_CMD_PP	0x02	/* Page program */


Fix the indentation here and in this file

carlocaione · 2023-07-21T07:00:05Z

drivers/flash/flash_andes_qspi.h

+#define REG_TIMIN	0x40
+#define REG_CONFIG	0x7c
+
+#define QSPI_BASE		(((const struct flash_andes_qspi_config *) \


QSPI_BASE(dev)

Thanks for the review!
I've fixed the problems.

drivers/flash/flash_andes_qspi.c

Support flash driver on Andes adp_xc7k_ae350. Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

Add dts binding for Andes qspi-nor. Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

carlocaione

In general LGTM

de-nordic · 2023-08-02T09:06:42Z

drivers/flash/flash_andes_qspi.c

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


I am really sorry, this is my mistake: the last condition should be (flash_size - addr) > size and you can drop the size > flash_size..

(flash_size - addr) < size) is the case that if I write a chunk of data that exceed the flash size. And I should return error code in the case.
Following is the example: flash_size is 4KB, addr is 0, and size is 5KB

Updated. Drop the size > flash_size

You are right.

Add flash driver for Andes qspi. Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

Add adp_xc7k_ae350 support in spi_flash driver sample Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

de-nordic

One comment left. Otherwise OK.

de-nordic · 2023-08-03T08:12:13Z

drivers/flash/flash_andes_qspi.c

+	if (size == 0) {
+		return 0;
+	}


This should be before checking for error condition: if there is no work to do there is no reason to error out first. In read, write and erase.

de-nordic · 2023-08-03T08:24:00Z

drivers/flash/flash_andes_qspi.c

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


You are right.

kartben · 2023-08-11T14:28:18Z

@wtlee1995 can you update supported features for https://docs.zephyrproject.org/latest/boards/riscv/adp_xc7k_ae350/doc/index.html#supported-features?

wtlee1995 · 2023-08-14T01:31:29Z

Hi @kartben,
Two drivers for adp_xc7k_ae350 are currently under review. Once they are merged, we will proceed to update the document.

wtlee1995 requested review from nashif and de-nordic as code owners June 1, 2023 03:09

zephyrbot added area: RISCV RISCV Architecture (32-bit & 64-bit) area: Devicetree Binding PR modifies or adds a Device Tree binding area: Flash labels Jun 1, 2023

zephyrbot requested review from carlocaione, edersondisouza, fkokosinski, galak, katsuster, kgugala, mgielda and tgorochowik June 1, 2023 03:09

zephyrbot assigned de-nordic Jun 1, 2023

wtlee1995 force-pushed the andes_flash_upstream branch 2 times, most recently from 0fdad92 to 3ab2d14 Compare June 2, 2023 10:03

wtlee1995 force-pushed the andes_flash_upstream branch from 3ab2d14 to 359b497 Compare June 9, 2023 00:50

edersondisouza reviewed Jun 21, 2023

View reviewed changes

wtlee1995 force-pushed the andes_flash_upstream branch from 359b497 to 4813260 Compare June 26, 2023 00:30

wtlee1995 force-pushed the andes_flash_upstream branch from 0502f7f to 562b698 Compare July 12, 2023 02:27

wtlee1995 requested a review from edersondisouza July 17, 2023 06:16

edersondisouza reviewed Jul 17, 2023

View reviewed changes

wtlee1995 force-pushed the andes_flash_upstream branch 2 times, most recently from 81682d1 to 9d0898c Compare July 21, 2023 01:24

carlocaione requested changes Jul 21, 2023

View reviewed changes

de-nordic reviewed Jul 21, 2023

View reviewed changes

wtlee1995 added 2 commits August 2, 2023 09:47

boards: riscv: adp_xc7k_ae350: support flash driver

130d39c

Support flash driver on Andes adp_xc7k_ae350. Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

dts: bindings: mtd: add Andes qspi-nor

3e9b5d4

Add dts binding for Andes qspi-nor. Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

wtlee1995 force-pushed the andes_flash_upstream branch from 9d0898c to 0538eb3 Compare August 2, 2023 08:15

carlocaione previously approved these changes Aug 2, 2023

View reviewed changes

de-nordic reviewed Aug 2, 2023

View reviewed changes

wtlee1995 added 2 commits August 3, 2023 09:11

drivers: flash: add Andes qspi-nor driver

bc14893

Add flash driver for Andes qspi. Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

samples: drivers: spi_flash: add adp_xc7k_ae350 surport

d846c8b

Add adp_xc7k_ae350 support in spi_flash driver sample Signed-off-by: Wei-Tai Lee <wtlee@andestech.com>

wtlee1995 dismissed carlocaione’s stale review via d846c8b August 3, 2023 01:12

wtlee1995 force-pushed the andes_flash_upstream branch from 0538eb3 to d846c8b Compare August 3, 2023 01:12

carlocaione approved these changes Aug 3, 2023

View reviewed changes

de-nordic approved these changes Aug 3, 2023

View reviewed changes

carlescufi merged commit e6d6df5 into zephyrproject-rtos:main Aug 3, 2023
18 checks passed

wtlee1995 deleted the andes_flash_upstream branch August 9, 2023 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: flash: add Andes qspi-nor driver #58718

drivers: flash: add Andes qspi-nor driver #58718

wtlee1995 commented Jun 1, 2023

edersondisouza Jun 21, 2023

wtlee1995 Jun 26, 2023

edersondisouza Jul 17, 2023 •

edited

wtlee1995 Jul 21, 2023

edersondisouza Jul 17, 2023

wtlee1995 Jul 21, 2023

edersondisouza Jul 17, 2023

edersondisouza Jul 17, 2023

wtlee1995 Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

carlocaione Jul 21, 2023

wtlee1995 Aug 2, 2023

carlocaione left a comment

de-nordic Aug 2, 2023

wtlee1995 Aug 2, 2023

wtlee1995 Aug 3, 2023

de-nordic Aug 3, 2023

de-nordic left a comment

de-nordic Aug 3, 2023

de-nordic Aug 3, 2023

kartben commented Aug 11, 2023

wtlee1995 commented Aug 14, 2023

		#if defined(CONFIG_FLASH_ANDES_QSPI_SFDP_RUNTIME) \
		\|\| defined(CONFIG_FLASH_JESD216_API)

		#define FLASH_ANDES_CMD_WRDI 0x04 /* Write disable */
		#define FLASH_ANDES_CMD_PP 0x02 /* Page program */

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

drivers: flash: add Andes qspi-nor driver #58718

drivers: flash: add Andes qspi-nor driver #58718

Conversation

wtlee1995 commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edersondisouza Jul 17, 2023 • edited

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

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

carlocaione left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartben commented Aug 11, 2023

wtlee1995 commented Aug 14, 2023

edersondisouza Jul 17, 2023 •

edited