Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/sam0_common: implement QSPI peripheral, add support to mtd_spi_nor #15300

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Oct 26, 2020

Contribution description

This implements the QSPI peripheral found on the samd5x family and extends the mtd_spi_nor driver to make use of it.

The QSPI peripheral can only be used on a fixed set of pins, but it doesn't use any SERCOM.

same54-xpro comes with a 256 MBit flash that is configured as a MTD device with this PR.

Right now this uses the QSPI in memory mapped mode. I just copied that part from the Adafruit_SPIFlash library.
The memory is mapped from 0x04000000 up to 0x05000000.

Unfortunately it seems like the flash start address of that mapping can not be changed, so we are limited to 16 MiB.
Alternatively we can operate the device in SPI mode and read / write data to the TXDATA/RXDATA registers using DMA.
That way we should be able to address the whole memory.

TOOD

  • only 16 MiB can be memory mapped. Trying to access memory beyond that will lead to a fault. That means only half of the memory on the same54-xpro can be used. It should be possible to use non-memory mapped mode and DMA to work around this.
  • Double Data Rate requires a separate clock with 2x the QSPI frequency. We can use a second DPLL for that, but this is out of scope for this PR
  • XIP - out of scope for this PR

Testing procedure

  • tests/periph_qspi provides a low-level test which can be used to read / write / erase the QSPI flash directly
  • examples/filesystem or any of the file system tests that use the MTD subsystem should work

Issues/PRs references

@benpicco benpicco requested review from bergzand and dylad October 26, 2020 19:20
@benpicco benpicco added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 26, 2020
@bergzand
Copy link
Member

Does it make sense to split 2a3a56d to a separate PR so that we have somewhere to discuss the QSPI api?

@benpicco
Copy link
Contributor Author

Just the API?
Sure, can do.

@bergzand
Copy link
Member

It might make sense to also split out the SPI NOR changes at some point so that we can write multiple implementations for QSPI in parallel.

@benpicco benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 25, 2020
* driver has a maximal transfer size.
* negative error
*/
ssize_t qspi_read(qspi_t bus, uint32_t addr, void *data, size_t len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

#endif

/**
* @brief Configuration Options for QSPI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STM32H7 supports Octo-SPI 🐙

/**
* @brief Serial Flash JEDEC commands
*/
typedef enum {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SAM D5x/E5x has that too, but the memory region where this is mapped to is fixed.
I have a branch where I implemented qspi_xip_mount() - haven't pushed that yet because I couldn't get the flash chip out of XIP mode without doing a power reset.

I assumed that in this mode you can only read, not write to the flash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the datasheet it seems that you can only read, but it is not very clear. The nRF52840 has registers for configuring the READ, WRITE and ERASE opcodes, but I can't quite tell if the chip handles writes or not. QN9080 says it only handles reads and actually XIP is not recommended because it is too slow.

I think this is another example of code that shouldn't go into the qspi cpu driver: sending commands to enabled/disable the xip mode in the flash chip (looking at your code I understand you mean no-opcode mode). Some chips use 0xa5 for entering this mode and 0xff to exit, but this depends on the flash chip, not the cpu, and it varies a bit across manufacturers. For this what's needed is a way to set the dummy bytes to send (probably in the flags, or similar).

The XIP mount memory region is normally a window of certain fixed size and address; but you could have a different base flash address if the flash is larger than the window. I don't see this feature in the samd5x, but nrf52840 has the XIPOFFSET register for this purpose. I think that when configuring the memory mapping (xip_mount in your branch) the caller should pass the configuration: command (if any), whether an command is needed, the flash base address; and the driver would return the memory address corresponding to the requested base address. If windowing is supported you would set that in the xipoffset register for example, but if there are alignment requirements for this offset or having an offset is not supported the driver could return an address into the fixed region (like AHB_XIP+base_address or AHB_XIP + base_address % min_alignment while setting the offset to another register). The caller in this example would be a qspi_nor driver, which is different from a qspi_flash driver for example but the same across CPUs. I'd imagine that the qspi_nor driver can send commands to enable the XIP mode in the flash (depending on the flash) and then tell the qspi.h interface to do xip_mount with no opcode; or don't set the XIP mode in the flash if not supported and then tell the qspi.h to do xip_mount using certain opcode for reads.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@stale stale bot closed this Apr 17, 2022
@aabadie
Copy link
Contributor

aabadie commented Apr 17, 2022

This still seems to be useful and would be a nice addition if merged.

@aabadie aabadie reopened this Apr 17, 2022
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Apr 17, 2022
@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Apr 17, 2022
@stale
Copy link

stale bot commented Nov 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms State: stale State: The issue / PR has no activity for >185 days State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants