Skip to content

Conversation

@pdgendt
Copy link
Contributor

@pdgendt pdgendt commented Nov 17, 2025

Add support for JEDEC octal mode.

Disclaimer: this code is AI-assisted

Fixes #99496

After applying changes from #99421 I get the following results

# before
uart:~$ flash read_test is25wx01g@0 0 0x1000 10
Loop #1 done in 200371 ns.
Loop #2 done in 26416 ns.
Loop #3 done in 29667 ns.
Loop #4 done in 44531 ns.
Loop #5 done in 14257 ns.
Loop #6 done in 13775 ns.
Loop #7 done in 8481 ns.
Loop #8 done in 24797 ns.
Loop #9 done in 8652 ns.
Loop #10 done in 13571 ns.
Total: 384523 ns, Per loop: 38452 ns, Speed: ~101.6MiBps

# after
uart:~$ flash read_test is25wx01g@0 0 0x1000 10
Loop #1 done in 13607 ns.
Loop #2 done in 4619 ns.
Loop #3 done in 4310 ns.
Loop #4 done in 6263 ns.
Loop #5 done in 8767 ns.
Loop #6 done in 4126 ns.
Loop #7 done in 4126 ns.
Loop #8 done in 4126 ns.
Loop #9 done in 4120 ns.
Loop #10 done in 4126 ns.
Total: 58196 ns, Per loop: 5819 ns, Speed: ~671.3MiBps

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for JEDEC octal mode (1-8-8) to the FlexSPI NOR flash driver, enabling significantly faster read speeds (~671 MiBps vs ~101 MiBps). The implementation reads SFDP DW19 to determine octal enable requirements and implements the S2B3 method (Status Register 2, Bit 3) for enabling octal mode.

  • Adds parsing of JESD216 DW19 to detect octal enable requirements
  • Implements octal enable sequence for S2B3 method (Status Register 2, Bit 3)
  • Configures 1-8-8 read mode when supported by the flash device

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


#define NOR_WRITE_SIZE 1
#define NOR_ERASE_VALUE 0xff

Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

These constants represent JESD216 DW19 Octal Enable Requirements but lack documentation explaining their purpose and origin. Consider adding a comment block explaining that these are bit field values from JESD216 SFDP DW19 bits [22:20] for Octal Enable Requirements, where 000b = no command needed, 001b = set bit 3 of Status Register 2, etc.

Suggested change
/*
* These constants represent the Octal Enable Requirement (OER) field values
* from the JESD216 Serial Flash Discoverable Parameters (SFDP) standard,
* specifically DW19 bits [22:20]. The OER field indicates what (if any)
* command or register modification is required to enable Octal mode.
* 000b = No command needed to enable Octal mode
* 001b = Set bit 3 of Status Register 2 to enable Octal mode
* (Other values may be defined in the standard for other requirements)
*/

Copilot uses AI. Check for mistakes.
Comment on lines +768 to +767
flexspi_lut[SCRATCH_CMD][0] =
FLEXSPI_LUT_SEQ(kFLEXSPI_Command_SDR, kFLEXSPI_1PAD, 0x65,
kFLEXSPI_Command_RADDR_SDR, kFLEXSPI_1PAD, 0x08);
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

The magic number 0x65 appears to be a Read Register command but is not defined as a named constant like other SPI NOR commands (e.g., SPI_NOR_CMD_WRSR2 on line 775). Consider defining this as a named constant such as SPI_NOR_CMD_RDREG or similar to improve code readability and maintainability.

Copilot uses AI. Check for mistakes.
int ret;
uint32_t buffer = 0;
flexspi_transfer_t transfer = {
.deviceAddress = 0x02,
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

The magic number 0x02 as deviceAddress appears to be a register address for reading Status Register 2, but lacks explanation. Consider adding a comment or defining this as a named constant like SR2_ADDRESS or STATUS_REG2_ADDR to clarify its purpose.

Copilot uses AI. Check for mistakes.
Comment on lines +772 to +770
FLEXSPI_LUT_SEQ(kFLEXSPI_Command_DUMMY_SDR, kFLEXSPI_1PAD, 0x08,
kFLEXSPI_Command_READ_SDR, kFLEXSPI_1PAD, 0x01);
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

The magic numbers 0x08 (dummy cycles/bits) appears twice in this function (lines 770, 772) without explanation. Consider defining these as named constants or adding comments to explain why 8 dummy cycles are required for this specific read register command.

Copilot uses AI. Check for mistakes.
if (ret < 0) {
return ret;
}

Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

This check verifies if bit 3 of Status Register 2 is already set (indicating octal mode is already enabled), but lacks a comment explaining this. Consider adding a comment like '/* Check if octal mode is already enabled (SR2 bit 3) */' to improve code clarity.

Suggested change
/* Check if octal mode is already enabled (SR2 bit 3) */

Copilot uses AI. Check for mistakes.
if (ret < 0) {
return ret;
}

Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

This line sets bit 3 to enable octal mode but lacks a comment explaining the purpose. Consider adding a comment like '/* Set bit 3 to enable octal mode */' to improve code clarity.

Suggested change
/* Set bit 3 to enable octal mode */

Copilot uses AI. Check for mistakes.
@ZhaoxiangJin
Copy link
Contributor

CC @Albort12138, @SuperHeroAbner


#define JESD216_DW19_OER_VAL_NONE 0U
#define JESD216_DW19_OER_VAL_S2B3 1U

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to jesd216.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some dw19 decode struct/function instead.

Add struct and decode function for dw19 with octal enable requirement.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
Add support for JEDEC octal mode.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
If supported, parse dw19 and print out the octal enable requirement.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
@sonarqubecloud
Copy link

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 18, 2025

Adding DNM, this appears to do "something", but not enable octal mode, investigating.

@pdgendt pdgendt added the DNM This PR should not be merged (Do Not Merge) label Nov 18, 2025
@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 18, 2025

It appears that 1-8-8 mode does get activated, haven't been able to verify HW that uses the octal enable requirement bit, as I don't have that.

It would be nice if vendors with access to JEDEC specs could review/test, thanks.

@pdgendt pdgendt removed the DNM This PR should not be merged (Do Not Merge) label Nov 18, 2025
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.

flash_mcux_flexspi_nor: Support Octal mode for IS25WX01G

7 participants