Skip to content

Conversation

@MaureenHelm
Copy link
Member

Introduces a new flash driver for the FlexSPI peripheral on i.MX RT
SoCs. The hardware provides a flexible sequence engine (LUT) that
supports various types of external devices, including serial NOR flash,
serial NAND flash, HyperBus (HyperFlash/HyperRAM), and FPGAs. It
supports up to four connected devices in single/dual/quad/octal modes
and provides memory-mapped read/write access to these devices through
the AHB bus.

The driver implementation consists of a shared controller for each
FlexSPI peripheral instance, and protocol-specific device drivers for
each external device. The controller provides a private interface for
multiple devices to access the FlexSPI peripheral registers. FlexSPI
devices provide the public flash driver interface to applications or
subsystems like storage or flash file systems; they also provide
protocol-specific LUT sequences to the controller.

Currently the only device type supported is QSPI NOR flash, but other
types like HyperFlash will be added later.

XIP is not yet supported, as this requires additional work to relocate
code to RAM and managing interrupts.

Signed-off-by: Maureen Helm [email protected]

@github-actions
Copy link

github-actions bot commented Dec 28, 2020

The following projects have a revision update in this Pull Request:

Name Old Revision New Revision
hal_nxp zephyrproject-rtos/hal_nxp@74ec105 zephyrproject-rtos/hal_nxp@b916bca (master)

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@arvid-r
Copy link
Contributor

arvid-r commented Dec 29, 2020

This looks great. Thanks!

I'll try to look at the LUT part. We had lots of problems with that initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this attribute should be linked to CODE_LOCATION.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to get rid of CODE_LOCATION and start using a dts chosen node (zephyr,flash) instead. I made an attempt to do that in #30065 but it needs work.

@pabigot
Copy link
Contributor

pabigot commented Jan 7, 2021

I have the mimxrt1060_evk, and am unable to get any of the samples (littlefs, spi_flash, jesd216, latter two with code changes to make it compile) to work. littlefs builds and flashes, but dies in CLOCK_InitUsb1Pfd with no further output, not even the boot message.

Reflashing with hello-world works.

Is this board supposed to work, or does it use the flash in a way that's incompatible with this feature?

@MaureenHelm
Copy link
Member Author

MaureenHelm commented Jan 7, 2021

I have the mimxrt1060_evk, and am unable to get any of the samples (littlefs, spi_flash, jesd216, latter two with code changes to make it compile) to work. littlefs builds and flashes, but dies in CLOCK_InitUsb1Pfd with no further output, not even the boot message.

Reflashing with hello-world works.

Is this board supposed to work, or does it use the flash in a way that's incompatible with this feature?

No, this won't work on mimxrt1060_evk until XIP is supported in the flash driver. I specifically chose mimxrt1064_evk because it has two QSPI flashes - one internal to the SoC package that is used for XIP, and one external mounted on the board that I could use exclusively for storage. mimxrt1060_evk only has the latter QSPI flash, so it has to be used for both code and storage, and thus needs XIP support in the flash driver.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have tested the FlexSPI driver on board VisionSOM-RT with i.MX RT1052. When FlexSPI root clock is equal 120 MHz, amplitude of signal FlexSPI_CLK is too small. I attach two oscilloscope screenshot, with FlexSpiDiv==2 and FlexSpiDiv==3 (120 MHz and 90 MHz). It is depended on board capacity, so i suggest to use DTS for setup value of this divider.
Div2Div3

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is command D7h used here, not 20h (SPI_NOR_CMD_SE)? W25Q128JV as well as IS25WP064A support command 20h, but W25Q128JV not support D7h. I found command 20h in JESD251A (table 5), but i cannot understand is this command standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting point. The command might be defaultable, but for portability it should come from the JESD216 BFP DW8 and DW9. Is it possible to interact with the device using a default LUT first, to extract the SFDP data, then program a LUT that is correct for the discovered device?

Copy link
Member Author

Choose a reason for hiding this comment

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

The LUT is derived from the MCUXpresso SDK example evkmimxrt1064_flexspi_nor_polling_transfer. I don't know why 0xD7 was used there instead of 0x20, but experimentally it seems that the standard command also works with IS25WP064.

Copy link
Contributor

Choose a reason for hiding this comment

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

How can different flash chips be supported in the future? I guess the LUT might be subject to change if another chip is used, but now it is hardcoded in the driver.

Copy link
Contributor

@GrixaYrev GrixaYrev Jan 14, 2021

Choose a reason for hiding this comment

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

For example, board VisionSOM-RT have flash W25Q128JV, which do not support D7h. Also S25FL128L by CYPRESS and MT25QL128 by Micron support command 20h. So, it is universal to use command 20h.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the main purposes of adding SFDP support in Zephyr is to allow the driver to get the information (like erase command) from the flash chip directly, and configure itself. I do think doing that would handle the concerns in this thread, but other than changing the command to the standard 20h from the unusual D7h I don't think any of that has to be done now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the order of these commands important? Does it have to correspond to the look-up table index in Table 8-15 in the iMX RT1020 Reference Manual for example, for XIP to work in the future. Maybe I misunderstood how this works. It is a very flexible system, for good and bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on the footnote after the table, I think we can change them:

1. All the pre-defined LUT indexes are only applicable to
boot stage. User application can use the whole 16 LUT
entries freely based on their requirement.

I ordered them this way because I think we may eventually want to conditionalize quad and octal support. Something like this:

enum {
        /* SPI instructions */
        READ_ID,
        READ_STATUS_REG,
        WRITE_STATUS_REG,
        WRITE_ENABLE,
        PAGE_PROGRAM,
        ERASE_SECTOR,
        ERASE_CHIP,

#ifdef CONFIG_FLASH_FLEXSPI_NOR_QUAD
        /* Quad SPI instructions */
        READ_FAST_QUAD_OUTPUT,
        PAGE_PROGRAM_QUAD_INPUT,
        ENTER_QPI,
#endif

#ifdef CONFIG_FLASH_FLEXSPI_NOR_OCTAL
        /* Octal SPI instructions */
        READ_FAST_OCTAL_OUTPUT,
        PAGE_PROGRAM_OCTAL_INPUT,
        ENTER_OPI,
#endif
};

@MaureenHelm
Copy link
Member Author

I pushed a new version with the following changes:

  • Expanded dts property descriptions to include references to their associated hardware register fields
  • Reviewed and updated dts property defaults to reflect hardware register reset values. There are a few exceptions for boolean properties that are default enabled in hardware, but default disabled in dts. I believe it's not possible to unset a boolean dts property if it was already set by default.
  • Added an rx_clock_source property instead of hard-coding the value in the driver
  • Removed the ahb-base property and used the second MMIO reg property instead (DT_INST_REG_ADDR_BY_IDX(n, 1))
  • Replaced the chip-specific sector erase command with the standard SPI_NOR_CMD_SE command

I purposely did not rebase to make it easier to see what changed from the previous version.

@pabigot
Copy link
Contributor

pabigot commented Jan 14, 2021

I believe it's not possible to unset a boolean dts property if it was already set by default.

It is; in an overlay add:

&node {
    /delete-property/ bool-prop;
};

@MaureenHelm
Copy link
Member Author

Rebased to resolve conflicts after merging #30864 and to pick up #31042.

@pabigot please check that I've integrated the jesd216 binding correctly in dts/bindings/mtd/nxp,imx-flexspi-device.yaml

scripts/dts/gen_defines.py wouldn't let me set boolean properties as default true, so I set them in the SoC level dts/arm/nxp/nxp_rt.dtsi

@mbolivar-nordic
Copy link
Contributor

scripts/dts/gen_defines.py wouldn't let me set boolean properties as default true, so I set them in the SoC level dts/arm/nxp/nxp_rt.dtsi

I think that's an edtlib bug; I'm working on a patch.

@mbolivar-nordic
Copy link
Contributor

I think that's an edtlib bug; I'm working on a patch.

#31366

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we can't "fix" this sort of thing in edtlib, an alternative is to invert the devicetree property meaning. Something like no-ahb-bufferable in devicetree, and .ahb_bufferable = !DT_INST_PROP(..., no_ahb_bufferable) when initializing config.

Feel free to disregard if you're happier with setting this in .dtsi.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be confusing to tie a property to hardware register field but invert the logic, so keeping this in the dtsi. Thanks for your help

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be fine to have this be:

compatible = "adesto,at25sf128a", "nxp,imx-flexspi-nor";

Then if somebody has an Adesto-specific driver it'd be picked up. It also makes more clear exactly what the flash chip is. Not required, but suggested.

The same applies to all the other flash device nodes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like how this makes the flash chip more clear, but for it to work I think we'd need a compatible something like nxp,imx-flexspi-nor-at25sf128a. Because an Adesto-specific driver would be different for each of the various SoC QSPI peripherals.

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 not sure that's necessarily true. I'm thinking this would be useful in somebody's out-of-tree application where they didn't care that the underlying peripheral isn't represented in the compatible name, but their needs are such that their dedicated custom driver is preferred over the stock Zephyr one.

The primary purpose of the compatible is to accurately describe the device that's present in hardware, not the driver implementation to talk to that device. Kconfig figures out the best device. QSPI bus devices are perhaps an extreme case of this, but could be supported if we used jedec,qspi-nor and have the vendor-specific drivers all support that compatible, if it weren't for the fact the vendor drivers tend to need vendor-specific devicetree properties.

Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

There is a bug in mimxrt1064_evk in how zephyr,flash-controller is set. It should technically not be set, based on how we define zephyr,flash-controller as the controller node for where zephyr,flash is set. As we don't set zephyr,flash we shouldn't be setting this property.

This seems to be used by various tests & samples because of assumption that the flash controller we'd use for a test/sample would be the same as the one that Zephyr code is running out of.

@MaureenHelm
Copy link
Member Author

There is a bug in mimxrt1064_evk in how zephyr,flash-controller is set. It should technically not be set, based on how we define zephyr,flash-controller as the controller node for where zephyr,flash is set. As we don't set zephyr,flash we shouldn't be setting this property.

Ok, fixed. Please have another look. I also removed the xip property and set status = "disabled" instead.

Adds a hidden config symbol HAS_MCUX_FLEXSPI selected by NXP SoCs when
the FlexSPI peripheral is present. It will be used as a dependency for a
new FlexSPI flash driver to prevent users from accidentally enabling the
driver on platforms that don't have the necessary hardware.

Signed-off-by: Maureen Helm <[email protected]>
Copies the QSPI flash device tree node from the mimxrt1060_evk to the
mimxrt1064_evk board.

Signed-off-by: Maureen Helm <[email protected]>
Cleans up the HyperFlash device tree nodes on the mimxrt1050_evk and
mimxrt1060_evk_hyperflash boards to be more consistent with other
FlexSPI child nodes.

Signed-off-by: Maureen Helm <[email protected]>
Reworks the NXP FlexSPI device tree bindings to configure controller and
device properties needed for an upcoming FlexSPI flash driver.

Signed-off-by: Maureen Helm <[email protected]>
Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

Minor issue w/XIP prop in soc.c that can be removed.

Introduces a new flash driver for the FlexSPI peripheral on i.MX RT
SoCs. The hardware provides a flexible sequence engine (LUT) that
supports various types of external devices, including serial NOR flash,
serial NAND flash, HyperBus (HyperFlash/HyperRAM), and FPGAs. It
supports up to four connected devices in single/dual/quad/octal modes
and provides memory-mapped read/write access to these devices through
the AHB bus.

The driver implementation consists of a shared controller for each
FlexSPI peripheral instance, and protocol-specific device drivers for
each external device. The controller provides a private interface for
multiple devices to access the FlexSPI peripheral registers. FlexSPI
devices provide the public flash driver interface to applications or
subsystems like storage or flash file systems; they also provide
protocol-specific LUT sequences to the controller.

Currently the only device type supported is QSPI NOR flash, but other
types like HyperFlash will be added later.

XIP is not yet supported, as this requires additional work to relocate
code to RAM and managing interrupts.

Signed-off-by: Maureen Helm <[email protected]>
Enables the FlexSPI flash driver on the i.MX RT SoC family and
configures the peripheral clocks accordingly. We are careful to only
configure the peripheral clocks if we are not executing in place from
the FlexSPI flash.

Signed-off-by: Maureen Helm <[email protected]>
Enables the FlexSPI NOR flash driver, configures the FlexSPI pins, and
updates the board documentation accordingly on the mimxrt1064_evk.

Note that this SoC has two FlexSPI instances: one instance has an
in-package QSPI flash used for XIP; the other instance has a board-level
QSPI flash used for storage, not XIP. This patch enables the flash
driver on the non-XIP flash only.

Tested with:
  - samples/subsys/fs/littlefs
  - samples/drivers/flash_shell

Signed-off-by: Maureen Helm <[email protected]>

boards: arm: Rename flexspi_qspi to flexspi_nor for mimxrt1064_evk

Signed-off-by: Maureen Helm <[email protected]>
Adds the mimxrt1064_evk board to the platform_allow list for the
littlefs sample application.

Signed-off-by: Maureen Helm <[email protected]>
The flash shell can work with any flash driver instance, not just the
one chosen by zephyr,flash-controller. It's helpful for the flash shell
to use this instance by default, but not required. We can switch
instances at runtime with the "flash set_device" command.

Fix the flash shell so it can build when there isn't a chosen
zephyr,flash-controller available.

Signed-off-by: Maureen Helm <[email protected]>
The flash shell can now build when there isn't a chosen
zephyr,flash-controller available, so we can simplify the filter to
apply this sample to more boards.

Two STM32 H7 board configurations are excluded because the flash driver
isn't supported yet on the M4 core.

Signed-off-by: Maureen Helm <[email protected]>
@github-actions github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Jan 21, 2021
@nashif nashif merged commit a8d3c8e into zephyrproject-rtos:master Jan 22, 2021
@MaureenHelm MaureenHelm deleted the flexspi-flash branch January 22, 2021 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.