[riot-notifications] [RIOT-OS/RIOT] cpu/sam0_common: implement QSPI peripheral, add support to mtd_spi_nor (#15300)

iosabi notifications at github.com
Sat Jan 30 21:11:52 CET 2021


@iosabi commented on this pull request.



> +/**
+ * @brief   Read data from external memory.
+ *          Typically it is implemented by quad read command (`0x6B`).
+ *
+ * @pre     The @p bus has been acquired with @ref qspi_acquire
+ *
+ * @param[in]   bus      QSPI device
+ * @param[in]   address  Address to read from
+ * @param[out]  data     Buffer to store the data
+ * @param[in]   len      Number of byte to read
+ *
+ * @return      length of bytes read, may be smaller than @p len if the
+ *              driver has a maximal transfer size.
+ *              negative error
+ */
+ssize_t qspi_read(qspi_t bus, uint32_t addr, void *data, size_t len);

I'm a bit puzzled by this interface. I think it is mixing the flash concepts (board or driver defined) with the qspi concepts (cpu defined). I don't think the cpu's qspi module should have knowledge about 0x6B being a read command. This depends on the mode you have configured on the flash and varies between flash chips.

QSPI read operation means that all four IO0~3 are controller-driven for a certain number of clocks (opcode + 0 to 4 address bytes) and then they are peripheral-driven for the remaining number or clocks. This support is what I have seen in various cpus.

Depending on the flash chip, flash chip mode or the opcode the address would be also in quad mode, but I don't think that's something this interface should deal with. The `flags` in configure function would probably need to have the address-length, quad vs dual mode, and the number of dummy bytes, which also changes at runtime because of the different modes/commands you can send to a flash (not all commands are just fast_reads or fast_writes).

I'd propose for QSPI operation to have only these two instead of the 4 functions here:
```C
ssize_t qspi_read(qspi_t bus, uint8_t cmd, uint32_t addr, void *data, size_t len);
ssize_t qspi_write(qspi_t bus, uint8_t cmd, uint32_t addr, const void *data, size_t len);
```

These functions could also serve as the single-mode operation using the 1bit mode in the flags. I didn't see full-duplex like in regular SPI blocks (read from IO1 while write to IO0) supported in the QN9080 or nRF52840. Howver, at least a function that lets you do send N bytes in single mode and then receive M bytes would be very useful, again this could be just qspi_read with the proper flags. In QN9080 I think N<=5 and in NRF52840 N<=8 is possible, and it is needed to send other commands.

> +} qspi_mode_t;
+#endif
+
+/**
+ * @brief   Standard QSPI erase block sizes
+ */
+#ifndef HAVE_QSPI_ERASE_SIZE_T
+typedef enum {
+    QSPI_ERASE_4K   = SFLASH_CMD_ERASE_SECTOR,
+    QSPI_ERASE_64K  = SFLASH_CMD_ERASE_BLOCK,
+    QSPI_ERASE_CHIP = SFLASH_CMD_ERASE_CHIP,
+} qspi_erase_size_t;
+#endif
+
+/**
+ * @brief   Configuration Options for QSPI

I would add here number of dummy cycles after the address, more address length options (1-4 bytes) and whether the address is also transmitted in quad mode.

> +#ifndef HAVE_QSPI_ERASE_SIZE_T
+typedef enum {
+    QSPI_ERASE_4K   = SFLASH_CMD_ERASE_SECTOR,
+    QSPI_ERASE_64K  = SFLASH_CMD_ERASE_BLOCK,
+    QSPI_ERASE_CHIP = SFLASH_CMD_ERASE_CHIP,
+} qspi_erase_size_t;
+#endif
+
+/**
+ * @brief   Configuration Options for QSPI
+ * @{
+ */
+#define QSPI_FLAG_1BIT          (0x0)   /**< single data line           */
+#define QSPI_FLAG_2BIT          (0x1)   /**< two parallel data lines    */
+#define QSPI_FLAG_4BIT          (0x2)   /**< four parallel data lines   */
+#define QSPI_FLAG_8BIT          (0x3)   /**< eight parallel data lines  */

This means IO0 to IO7 pins? Do you have an example CPU that supports this?

> +
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <limits.h>
+
+#include "periph_cpu.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   Serial Flash JEDEC commands
+ */
+typedef enum {

I don't think this belongs here. In the NOR case this is not in the cpu's spi case, and we could use this module with chips that don't necessarily conform to these registers.

> + *
+ * @param[in] bus       QSPI device to initialize
+ */
+void qspi_init(qspi_t bus);
+
+/**
+ * @brief   Configure the QSPI peripheral settings
+ *
+ * @pre     The @p bus has been acquired with @ref qspi_acquire
+ *
+ * @param[in] bus       QSPI device to configure
+ * @param[in] mode      QSPI mode @see qspi_mode_t
+ * @param[in] flags     QSPI Configuration Options
+ * @param[in] clk_hz    QSPI frequency in Hz
+ */
+void qspi_configure(qspi_t bus, qspi_mode_t mode, uint32_t flags, uint32_t clk_hz);

qn9080 and nrf52840 both support memory mapping a flash to a region. To do that you need to program a command (like the fast_read command) and an address format, so then you can read from MCU memory directly which will generate a QSPI transaction automatically, even with some caching, allowing very fast reads without any call overhead. This is often incompatible with running a command at the same time, so something like qspi_configure but for configuring memory reads that returns the pointer/size would be good to add here. It is probably an addition to this module, meaning that you would need to have MODULE_QSPI_MAPPING defined, but it is worth considering the idea in this design so we don't have to change it later. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15300#pullrequestreview-579795233
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210130/c9c03744/attachment.htm>


More information about the notifications mailing list